Skip to content
New issue

Have a question about this project? Sign up for a free GitHub account to open an issue and contact its maintainers and the community.

Sign up for GitHub

By clicking “Sign up for GitHub”, you agree to our terms of service and privacy statement. We’ll occasionally send you account related emails.

Already on GitHub? Sign in to your account

Jump to bottom

docs(specs): add missing versioned consts #4054

Open
wants to merge 10 commits into
base: main
Choose a base branch
from
+37 −24
Open

docs(specs): add missing versioned consts #4054

Show file tree
Hide file tree
Changes from 9 commits
Commits
Show all changes
10 commits
Select commit Hold shift + click to select a range
4f26e6a
docs(specs): add missing versioned consts
rootulp Nov 25, 2024
be6a63a
Update specs/src/parameters_v1.md
rootulp Nov 26, 2024
e97e996
Update specs/src/parameters_v2.md
rootulp Nov 26, 2024
9d87c1e
Update specs/src/parameters_v3.md
rootulp Nov 26, 2024
bace653
Update specs/src/parameters_v3.md
rootulp Nov 26, 2024
77053b7
Update specs/src/parameters_v3.md
rootulp Nov 26, 2024
39f2644
improve comments
rootulp Nov 26, 2024
879a427
prettify tables
rootulp Nov 26, 2024
08cb556
Merge branch 'main' into rp/versioned-const-specs
rootulp Nov 26, 2024
c5a916d
deprecate consts
rootulp Nov 27, 2024
File filter

Filter by extension

Filter by extension
Conversations
Failed to load comments.
Loading
Jump to
Jump to file
Failed to load files.
Loading
Diff view
Diff view
15 changes: 11 additions & 4 deletions pkg/appconsts/v1/app_consts.go
View file
Open in desktop
Original file line number Diff line number Diff line change
Expand Up @@ -6,10 +6,17 @@ const (
Version uint64 = 1
SquareSizeUpperBound int = 128
SubtreeRootThreshold int = 64
TimeoutPropose = time.Second * 10
TimeoutCommit = time.Second * 11
// TimeoutPropose wasn't a constant in v1, it was the default for a
// user-configurable timeout.
TimeoutPropose = time.Second * 10
// TimeoutCommit wasn't a constant in v1, it was the default for a
// user-configurable timeout.
TimeoutCommit = time.Second * 11
// UpgradeHeightDelay is the number of blocks after a quorum has been
// reached that the chain should upgrade to the new version. Assuming a block
// interval of 12 seconds, this is 7 days.
// reached that the chain should upgrade to the new version. Assuming a
// block interval of 12 seconds, this is 7 days.
//
// TODO: why does this constant exist in v1? v1 does not contain the signal
// module.
Copy link
Contributor

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

Yeah I think it should be removed

Copy link
Collaborator Author

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

I just marked them deprecated in the most recent commit on this PR. I don't really want to remove them b/c it's breaking.

If you want them to be removed, I can do it in a follow-up PR.

UpgradeHeightDelay = int64(7 * 24 * 60 * 60 / 12) // 7 days * 24 hours * 60 minutes * 60 seconds / 12 seconds per block = 50,400 blocks.
)
8 changes: 6 additions & 2 deletions pkg/appconsts/v2/app_consts.go
View file
Open in desktop
Original file line number Diff line number Diff line change
Expand Up @@ -6,8 +6,12 @@ const (
Version uint64 = 2
SquareSizeUpperBound int = 128
SubtreeRootThreshold int = 64
TimeoutPropose = time.Second * 10
TimeoutCommit = time.Second * 11
// TimeoutPropose wasn't a constant in v2, it was the default for a
// user-configurable timeout.
TimeoutPropose = time.Second * 10
// TimeoutCommit wasn't a constant in v2, it was the default for a
// user-configurable timeout.
TimeoutCommit = time.Second * 11
// UpgradeHeightDelay is the number of blocks after a quorum has been
// reached that the chain should upgrade to the new version. Assuming a block
// interval of 12 seconds, this is 7 days.
Expand Down
2 changes: 1 addition & 1 deletion pkg/appconsts/v3/app_consts.go
View file
Open in desktop
Original file line number Diff line number Diff line change
Expand Up @@ -13,6 +13,6 @@ const (
TimeoutCommit = time.Millisecond * 4200
// UpgradeHeightDelay is the number of blocks after a quorum has been
// reached that the chain should upgrade to the new version. Assuming a block
// interval of 12 seconds, this is 7 days.
// interval of 6 seconds, this is 7 days.
UpgradeHeightDelay = int64(7 * 24 * 60 * 60 / 6) // 7 days * 24 hours * 60 minutes * 60 seconds / 6 seconds per block = 100,800 blocks.
)
9 changes: 5 additions & 4 deletions specs/src/parameters_v1.md
View file
Open in desktop
Original file line number Diff line number Diff line change
Expand Up @@ -8,10 +8,11 @@ hardcoded in the application or they are blocked by the `x/paramfilter` module.

## Global parameters

| Parameter | Default | Summary | Changeable via Governance |
|-------------------|---------|------------------------------------------------------------------------------------------------------------------------|---------------------------|
| MaxBlockSizeBytes | 100MiB | Hardcoded value in CometBFT for the protobuf encoded block. | False |
| MaxSquareSize | 128 | Hardcoded maximum square size determined per shares per row or column for the original data square (not yet extended). | False |
| Parameter | Value | Summary | Changeable via Governance |
|----------------------|---------|--------------------------------------------------------------------------------------------------------------------------------------------------------------------|---------------------------|
| SquareSizeUpperBound | 128 | Hardcoded maximum square size which limits the number of shares per row or column for the original data square (not yet extended). | False |
| SubtreeRootThreshold | 64 | See [ADR-013](https://github.com/celestiaorg/celestia-app/blob/main/docs/architecture/adr-013-non-interactive-default-rules-for-zero-padding.md) for more details. | False |
| MaxBlockSizeBytes | 100 MiB | Hardcoded value in CometBFT for the protobuf encoded block. | False |
Comment on lines +13 to +15
Copy link
Contributor

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

💡 Codebase verification

Parameter naming inconsistency detected - MaxSquareSize vs SquareSizeUpperBound

The codebase shows inconsistency between the documented parameter name and its implementation:

  • The parameter is documented as SquareSizeUpperBound in the specs
  • However, the codebase extensively uses MaxSquareSize and GovMaxSquareSize in the implementation
  • The SubtreeRootThreshold parameter is consistently used and matches the documentation
  • The MiB unit formatting is consistently used across the spec files
🔗 Analysis chain

LGTM: Clear and well-documented parameter definitions

The changes improve clarity and completeness:

  1. SquareSizeUpperBound is a more descriptive name than the previous MaxSquareSize
  2. Added the missing SubtreeRootThreshold parameter with proper ADR reference
  3. Improved formatting consistency for binary units in MaxBlockSizeBytes

Let's verify the consistency of these parameter names across the codebase:

🏁 Scripts executed

The following scripts were executed for the analysis:

Script:

#!/bin/bash
# Description: Verify parameter naming consistency across the codebase

# Check for any remaining references to the old MaxSquareSize name
rg "MaxSquareSize" --type go

# Verify SubtreeRootThreshold usage
rg "SubtreeRootThreshold" --type go

# Check for consistent binary unit formatting
rg "MiB" --type md specs/

Length of output: 16613


## Module parameters

Expand Down
11 changes: 6 additions & 5 deletions specs/src/parameters_v2.md
View file
Open in desktop
Original file line number Diff line number Diff line change
Expand Up @@ -8,11 +8,12 @@ hardcoded in the application or they are blocked by the `x/paramfilter` module.

## Global parameters

| Parameter | Default | Summary | Changeable via Governance |
|--------------------|---------|------------------------------------------------------------------------------------------------------------------------|---------------------------|
| MaxBlockSizeBytes | 100MiB | Hardcoded value in CometBFT for the protobuf encoded block. | False |
| MaxSquareSize | 128 | Hardcoded maximum square size determined per shares per row or column for the original data square (not yet extended). | False |
| UpgradeHeightDelay | 50400 | Height based delay after a successful `MsgTryUpgrade` has been submitted. | False |
| Parameter | Value | Summary | Changeable via Governance |
|----------------------|--------------|--------------------------------------------------------------------------------------------------------------------------------------------------------------------|---------------------------|
| SquareSizeUpperBound | 128 | Hardcoded maximum square size which limits the number of shares per row or column for the original data square (not yet extended). | False |
| SubtreeRootThreshold | 64 | See [ADR-013](https://github.com/celestiaorg/celestia-app/blob/main/docs/architecture/adr-013-non-interactive-default-rules-for-zero-padding.md) for more details. | False |
| UpgradeHeightDelay | 50400 blocks | Height based delay after a successful `MsgTryUpgrade` has been submitted. | False |
| MaxBlockSizeBytes | 100 MiB | Hardcoded value in CometBFT for the protobuf encoded block. | False |

## Module parameters

Expand Down
18 changes: 11 additions & 7 deletions specs/src/parameters_v3.md
View file
Open in desktop
Original file line number Diff line number Diff line change
Expand Up @@ -8,11 +8,15 @@ hardcoded in the application or they are blocked by the `x/paramfilter` module.

## Global parameters

| Parameter | Default | Summary | Changeable via Governance |
|--------------------|---------|------------------------------------------------------------------------------------------------------------------------|---------------------------|
| MaxBlockSizeBytes | 100MiB | Hardcoded value in CometBFT for the protobuf encoded block. | False |
| MaxSquareSize | 128 | Hardcoded maximum square size determined per shares per row or column for the original data square (not yet extended). | False |
| UpgradeHeightDelay | 100800 | Height based delay after a successful `MsgTryUpgrade` has been submitted. | False |
| Parameter | Value | Summary | Changeable via Governance |
|----------------------|---------------|-------------------------------------------------------------------------------------------------------------------------------------------------------------------------------------------------------------------------------------------------------------------|---------------------------|
| SquareSizeUpperBound | 128 | Hardcoded maximum square size which limits the number of shares per row or column for the original data square (not yet extended). | False |
| SubtreeRootThreshold | 64 | See [ADR-013](https://github.com/celestiaorg/celestia-app/blob/main/docs/architecture/adr-013-non-interactive-default-rules-for-zero-padding.md) for more details. | False |
| MaxTxSize | 2 MiB | Maximum size of a transaction in bytes. | False |
| TimeoutPropose | 3500 ms | Specifies the time that validators wait during the proposal phase of the consensus process. See CometBFT [specs](https://github.com/celestiaorg/celestia-core/blob/v0.34.x-celestia/spec/consensus/consensus.md#propose-step-heighthroundr) for more details. | False |
| TimeoutCommit | 4200 ms | Specifies the duration that validators wait during the Commit phase of the consensus process. See CometBFT [specs](https://github.com/celestiaorg/celestia-core/blob/v0.34.x-celestia/spec/consensus/consensus.md#precommit-step-heighthroundr) for more details. | False |
| UpgradeHeightDelay | 100800 blocks | Height based delay after a successful `MsgTryUpgrade` has been submitted. | False |
| MaxBlockSizeBytes | 100 MiB | Hardcoded value in CometBFT for the protobuf encoded block. | False |

## Module parameters

Expand All @@ -22,9 +26,9 @@ hardcoded in the application or they are blocked by the `x/paramfilter` module.
| auth.SigVerifyCostED25519 | 590 | Gas used to verify Ed25519 signature. | True |
| auth.SigVerifyCostSecp256k1 | 1000 | Gas used to verify secp256k1 signature. | True |
| auth.TxSigLimit | 7 | Max number of signatures allowed in a multisig transaction. | True |
| auth.TxSizeCostPerByte | 10 | Gas used per transaction byte. | False |
| auth.TxSizeCostPerByte | 10 | Gas used per transaction byte. | False |
| bank.SendEnabled | true | Allow transfers. | False |
| blob.GasPerBlobByte | 8 | Gas used per blob byte. | False |
| blob.GasPerBlobByte | 8 | Gas used per blob byte. | False |
| blob.GovMaxSquareSize | 64 | Governance parameter for the maximum square size of the original data square. | True |
| consensus.block.MaxBytes | 1974272 bytes (~1.88 MiB) | Governance parameter for the maximum size of the protobuf encoded block. | True |
| consensus.block.MaxGas | -1 | Maximum gas allowed per block (-1 is infinite). | True |
Expand Down
Loading