refactor!: version gas scheduler variables #3735
Conversation
|
ninabarbakadze
force-pushed
the
nina/version-gas-scheduler-variables
branch
from
83ebe01 to
612c3de
Compare
ninabarbakadze
changed the title
ninabarbakadze
requested review from
cmwaters and
rach-id
and removed request for
a team
WalkthroughWalkthroughThe updates introduce a versioned gas scheduling system for the Celestia application, enabling gas costs associated with transactions to remain constant across different application versions. This includes restructuring gas consumption calculations, creating new constants for transaction size costs, and enhancing testing frameworks. Overall, these modifications aim to improve transaction processing efficiency and predictability for clients submitting transactions. Changes
Assessment against linked issues
Possibly related PRs
Suggested labels
Suggested reviewers
Recent review detailsConfiguration used: .coderabbit.yaml Files selected for processing (3)
Files skipped from review as they are similar to previous changes (2)
Additional context usedGitleaks
Additional comments not posted (6)
Thank you for using CodeRabbit. We offer it for free to the OSS community and would appreciate your support in helping us grow. If you find it useful, would you consider giving us a shout-out on your favorite social media? TipsChatThere are 3 ways to chat with CodeRabbit:
Note: Be mindful of the bot's finite context window. It's strongly recommended to break down tasks such as reading entire modules into smaller chunks. For a focused discussion, use review comments to chat about specific files and their changes, instead of using the PR comments. CodeRabbit Commands (Invoked using PR comments)
Other keywords and placeholders
Documentation and Community
|
![coderabbitai[bot]](https://avatars.githubusercontent.com/in/347564?s=60&v=4){kind=small}
There was a problem hiding this comment.
Actionable comments posted: 0
Outside diff range, codebase verification and nitpick comments (2)
x/blob/keeper/params_test.go (1)
12-12: Inconsistency Found:The
CreateKeeperfunction inkeeper_test.gois called with different version values (v2.Versionandv3.Version) instead ofappconsts.LatestVersion. To ensure consistency across the codebase, consider updating these calls to useappconsts.LatestVersion.
- File:
x/blob/keeper/keeper_test.go- Lines:
versions := []uint64{v2.Version, v3.Version} for _, version := range versions { t.Run("AppVersion_"+fmt.Sprint(version), func(t *testing.T) { k, _, ctx := CreateKeeper(t, version)Analysis chain
LGTM! Ensure the function usage is consistent.
The test changes align with the PR objectives and ensure the keeper is initialized with the latest version. Verify that all calls to
CreateKeeperinclude the new argument.
Let's verify the definition and usage of the
versionvariable inkeeper_test.goto ensure it aligns withappconsts.LatestVersion.Scripts executed
The following scripts were executed for the analysis:
Script:
#!/bin/bash # Description: Verify all function calls to `CreateKeeper` include the new argument `appconsts.LatestVersion`. # Test: Search for the function usage. Expect: Only occurrences of the new signature. rg --type go -A 5 $'CreateKeeper'Length of output: 3183
Script:
#!/bin/bash # Search for the definition and usage of the `version` variable in `keeper_test.go` rg --type go -A 5 -B 5 'version' x/blob/keeper/keeper_test.goLength of output: 1913
pkg/appconsts/initial_consts.go (1)
Update references to
DefaultGasPerBlobByteto use versioned parameters.The references to
DefaultGasPerBlobBytein the following files still use the old constant fromappconsts. Please update them to use the versioned constant frompkg/appconsts/versioned_consts.go:
x/blob/types/payforblob.gox/blob/types/params.goThis will ensure consistency with the move to versioned parameters.
Analysis chain
Line range hint
6-6:
Removal ofDefaultGasPerBlobByteis consistent with versioning changes.The removal of
DefaultGasPerBlobBytealigns with the move to versioned parameters. Ensure that all references to this constant are updated accordingly.Scripts executed
The following scripts were executed for the analysis:
Script:
#!/bin/bash # Description: Verify that all references to `DefaultGasPerBlobByte` are updated. # Test: Search for occurrences of `DefaultGasPerBlobByte`. Expect: No occurrences found. rg --type go 'DefaultGasPerBlobByte'Length of output: 446
Script:
#!/bin/bash # Extract relevant lines from the files where `DefaultGasPerBlobByte` is found. # Extract lines from x/blob/types/payforblob.go echo "File: x/blob/types/payforblob.go" rg --type go --context 5 'DefaultGasPerBlobByte' x/blob/types/payforblob.go # Extract lines from x/blob/types/params.go echo "File: x/blob/types/params.go" rg --type go --context 5 'DefaultGasPerBlobByte' x/blob/types/params.go # Extract lines from pkg/appconsts/versioned_consts.go echo "File: pkg/appconsts/versioned_consts.go" rg --type go --context 5 'DefaultGasPerBlobByte' pkg/appconsts/versioned_consts.goLength of output: 1935
| ) | ||
|
|
||
| func setup() (*app.App, sdk.Context, client.Context, sdk.AnteHandler, error) { | ||
| app, _, _ := testutil.NewTestAppWithGenesisSet(app.DefaultConsensusParams()) |
There was a problem hiding this comment.
Q: If we're using DefaultConsensusParams that will mean we're using app version 3 right? Has this been updated yet?
There was a problem hiding this comment.
No, it hasn't been updated yet but I left it as DefaultConsensusParams because we're planning to do so.
| // DefaultGasPerBlobByte is the default gas cost deducted per byte of blob | ||
| // included in a PayForBlobs txn | ||
| DefaultGasPerBlobByte = 8 | ||
|
|
There was a problem hiding this comment.
nit: this is breaking so we should prob add ! to the PR title.
[no change needed] If we want to prep for the v3 release, we could also write a changelog entry on how consumers of this package need to modify their code if they used DefaultGasPerBlobByte. We could write these entires in a new markdown file in a new directory like changelog/v3.md
There was a problem hiding this comment.
https://github.com/celestiaorg/celestia-app/blob/main/docs/release-notes/pending-release.md now exists so we should add a line that states this constant was removed and what library consumers should use instead.
ninabarbakadze
changed the title
celestia-bot
requested review from
a team and
staheri14
and removed request for
a team
There was a problem hiding this comment.
Actionable comments posted: 0
Outside diff range, codebase verification and nitpick comments (1)
pkg/appconsts/v2/app_consts.go (1)
Extensive usage of
NetworkMinGasPricefound in multiple files.The
NetworkMinGasPriceconstant is used extensively across the codebase, including in the following files:
x/paramfilter/test/gov_params_test.gox/minfee/params.gox/minfee/grpc_query_test.gox/minfee/query.pb.gw.gox/minfee/query.pb.gox/minfee/module.gox/minfee/genesis.pb.gox/minfee/grpc_query.gox/minfee/genesis.gopkg/user/tx_client.gotest/tokenfilter/setup.gopkg/appconsts/initial_consts.goapp/test/upgrade_test.goapp/test/testnode_test.goapp/ante/min_fee_test.goapp/ante/fee_checker.goPlease review and refactor the dependent code to ensure that removing
NetworkMinGasPricedoes not break any functionality related to transaction fee validation and querying the network minimum gas price.Analysis chain
Line range hint
1-7:
Verify the impact of removingNetworkMinGasPrice.The constant
NetworkMinGasPricehas been removed. Ensure that this removal does not break any functionality related to transaction fee validation.Scripts executed
The following scripts were executed for the analysis:
Script:
#!/bin/bash # Description: Verify the impact of removing `NetworkMinGasPrice`. # Test: Search for the usage of `NetworkMinGasPrice`. Expect: No occurrences of the removed constant. rg --type go 'NetworkMinGasPrice'Length of output: 13925
There was a problem hiding this comment.
Overall LGTM. We'll also need to add a params v3 page to the specs and mark the params: blob.GasPerBlobByte and auth.TxSizeCostPerByte as non governance modifiable on that page.
Ref: https://github.com/celestiaorg/celestia-app/blob/main/specs/src/specs/parameters_v2.md
There was a problem hiding this comment.
Actionable comments posted: 0
Outside diff range and nitpick comments (1)
specs/src/parameters_v3.md (1)
1-8: Excellent introduction! Minor language improvement suggested.The introduction provides a clear and concise overview of the document's purpose and scope. It effectively sets the right expectations by mentioning the governance changeability and hard fork requirements for certain parameters upfront.
To improve readability, consider adding a comma before 'or' in line 7:
-hardcoded in the application or they are blocked by the `x/paramfilter` module. +hardcoded in the application, or they are blocked by the `x/paramfilter` module.Tools
LanguageTool
[uncategorized] ~7-~7: Use a comma before ‘or’ if it connects two independent clauses (unless they are closely connected and short).
Context: ...ng manually hardcoded in the application or they are blocked by thex/paramfilter...(COMMA_COMPOUND_SENTENCE)
Review details
Configuration used: .coderabbit.yaml
Review profile: CHILL
Files selected for processing (11)
- app/ante/tx_size_test.go (1 hunks)
- app/test/std_sdk_test.go (1 hunks)
- app/test/upgrade_test.go (1 hunks)
- pkg/appconsts/initial_consts.go (1 hunks)
- pkg/appconsts/versioned_consts.go (1 hunks)
- specs/src/parameters_v3.md (1 hunks)
- x/blob/README.md (1 hunks)
- x/blob/ante/ante_test.go (2 hunks)
- x/blob/keeper/gas_test.go (3 hunks)
- x/blob/keeper/keeper_test.go (4 hunks)
- x/blob/types/payforblob.go (1 hunks)
Additional context used
LanguageTool
specs/src/parameters_v3.md
[uncategorized] ~7-~7: Use a comma before ‘or’ if it connects two independent clauses (unless they are closely connected and short).
Context: ...ng manually hardcoded in the application or they are blocked by thex/paramfilter...(COMMA_COMPOUND_SENTENCE)
Additional comments not posted (31)
pkg/appconsts/versioned_consts.go (6)
19-21: LGTM!The function has been updated to return the constant value from the
v3package, which aligns with the PR objective of transitioning to versioned parameters.
24-26: LGTM!The function has been updated to return the constant value from the
v3package, which aligns with the PR objective of transitioning to versioned parameters.
28-30: LGTM!The new function
TxSizeCostPerBytehas been added to return the constant value from thev3package, which aligns with the PR objective of transitioning to versioned parameters.
32-34: LGTM!The new function
GasPerBlobBytehas been added to return the constant value from thev3package, which aligns with the PR objective of transitioning to versioned parameters.
39-39: LGTM!The new variable
DefaultTxSizeCostPerBytehas been added and is assigned the value returned byTxSizeCostPerByte(LatestVersion), which aligns with the PR objective of transitioning to versioned parameters.The past review comment is no longer applicable as the
LatestVersionis now set tov3.Versionand the constants are defined for v3.
40-40: LGTM!The new variable
DefaultGasPerBlobBytehas been added and is assigned the value returned byGasPerBlobByte(LatestVersion), which aligns with the PR objective of transitioning to versioned parameters.The past review comment is no longer applicable as the
LatestVersionis now set tov3.Versionand the constants are defined for v3.pkg/appconsts/initial_consts.go (2)
29-29: Breaking change: Removal ofDefaultGasPerBlobByte.The removal of
DefaultGasPerBlobByteis a breaking change as it affects the public API of the package. Please consider the following suggestions:
- Add a
!to the PR title to indicate that this is a breaking change.- Write a changelog entry in a new file
changelog/v3.mdon how consumers of this package need to modify their code if they usedDefaultGasPerBlobByte.
30-33: LGTM!The introduction of
DefaultNetworkMinGasPriceis a useful addition. The constant is well-documented with comments explaining its purpose and version applicability.x/blob/keeper/gas_test.go (8)
6-6: LGTM!The import statement is necessary and correctly added.
27-27: LGTM!The gas consumption calculation is correctly updated to use the versioned
appconsts.GasPerBlobByteconstant.
32-32: LGTM!The gas consumption calculation is correctly updated to use the versioned
appconsts.GasPerBlobByteconstant.
37-37: LGTM!The gas consumption calculation is correctly updated to use the versioned
appconsts.GasPerBlobByteconstant.
42-42: LGTM!The gas consumption calculation is correctly updated to use the versioned
appconsts.GasPerBlobByteconstant.
47-47: LGTM!The gas consumption calculation is correctly updated to use the versioned
appconsts.GasPerBlobByteconstant.
53-53: LGTM!The
CreateKeeperfunction call is correctly updated to include theappconsts.LatestVersionconstant.
66-66: LGTM!The
CreateKeeperfunction call is correctly updated to include theappconsts.LatestVersionconstant.x/blob/ante/ante_test.go (3)
8-8: LGTM!The import statement for
appconstsis necessary for the updated test setup.
14-15: LGTM!The import statements for Tendermint protocol types are necessary for the updated test setup.
94-99: Great improvement to the test setup!The updated test context, which includes the latest application version in the header, ensures that the test environment more closely resembles the actual application environment. This change improves the fidelity of the test and may help catch issues related to versioning that might have been missed with the previous setup.
x/blob/keeper/keeper_test.go (2)
30-30: LGTM!The
CreateKeeperfunction is now called withappconsts.LatestVersion, ensuring that the tests run with the latest application version set in the context. This change aligns with the updated function signature.
Line range hint
75-90: Excellent work on improving the test setup!The updated
CreateKeeperfunction signature, which now accepts aversionparameter, allows for dynamically setting the application version in the context header. This change enhances the flexibility of the test setup, enabling the keeper to operate with different application versions as needed.Additionally, setting the keeper's parameters to a known, non-default value using
SetParams(ctx, types.DefaultParams())ensures a consistent starting point for the tests, improving their robustness.These modifications demonstrate a strong commitment to writing maintainable and reliable test code.
Also applies to: 104-104
app/ante/tx_size_test.go (3)
28-44: LGTM!The
setupfunction correctly initializes a test environment, including a test app, context, and client context with the necessary configurations. The code is well-structured and follows best practices.
46-137: Excellent test coverage!The
TestConsumeGasForTxSizefunction provides comprehensive test coverage for theConsumeGasForTxSizeDecorator. It covers different transaction versions and signature types, correctly calculates the expected gas consumption, and verifies the actual gas consumed matches the expected value. The test also validates that the simulation mode does not underestimate the gas consumption.The test cases are well-structured, and the assertions are thorough, ensuring the decorator behaves as expected in various scenarios.
139-184: Well-implemented helper function!The
createTestTxfunction is a well-implemented helper function that creates a test transaction given the necessary inputs. It follows a two-round process to gather signer information and sign the transaction correctly.The function properly sets the signatures on the transaction builder and returns the final transaction. The code is clean, readable, and follows best practices.
x/blob/types/payforblob.go (1)
167-167: Constant change aligns with versioned gas parameters objective.The modification to source the default transaction size cost per byte from
appconsts.DefaultTxSizeCostPerByteinstead ofauth.DefaultTxSizeCostPerBytealigns with the pull request's objective of transitioning gas scheduler variables to versioned parameters.Verify that
appconsts.DefaultTxSizeCostPerByteis defined with the intended value, as this constant will now be used for default gas estimation of pay-for-blob transactions. Changing this value would impact the gas costs.Verification successful
Verification of DefaultTxSizeCostPerByte implementation and usage
The constant
DefaultTxSizeCostPerByteis correctly implemented and used as intended:
- It's defined in the
appconstspackage, aligning with the PR objective.- The value is set to 10 in
v3/app_consts.go.- It's used consistently across the codebase, including in the ante handler for v3 and above.
While the
TxSizeCostPerBytefunction allows for version-dependent values, it currently always returns the v3 value. This design allows for future versioning if needed.Scripts executed
The following scripts were executed for the analysis:
Script:
#!/bin/bash # Description: Verify appconsts.DefaultTxSizeCostPerByte has the intended value. # Test: Search for the constant definition. Expect: Defined with the correct value. rg --type go $'DefaultTxSizeCostPerByte'Length of output: 264
Script:
#!/bin/bash # Description: Examine TxSizeCostPerByte function, LatestVersion, and usage of TxSizeCostPerByte # Check TxSizeCostPerByte function implementation echo "TxSizeCostPerByte function implementation:" rg --type go -A 10 "func TxSizeCostPerByte" # Check LatestVersion definition echo -e "\nLatestVersion definition:" rg --type go "LatestVersion.*=" # Check for other usage of TxSizeCostPerByte echo -e "\nOther usage of TxSizeCostPerByte:" rg --type go "TxSizeCostPerByte" | grep -v "func TxSizeCostPerByte" | grep -v "DefaultTxSizeCostPerByte"Length of output: 3551
x/blob/README.md (1)
52-52: Looks good! The existing comment has been addressed.The added clarification provides useful context about the
GasPerBlobByteparameter, distinguishing its behavior in different versions. This information will help developers understand how the parameter can be modified based on the application version they are working with.app/test/upgrade_test.go (1)
148-149: LGTM! The test coverage for the upgrade process is comprehensive.The changes in this test function ensure that the upgrade process from v1 to v2 is thoroughly tested for multiple modules. Querying module params before and after the upgrade helps verify the expected changes.
Note: The initialization of
NetworkMinGasPriceDecnow usesappconsts.DefaultNetworkMinGasPriceinstead ofv2.NetworkMinGasPrice. This change suggests a modification in the source of the gas price configuration, which may impact how the application handles network gas pricing during upgrades.app/test/std_sdk_test.go (1)
352-352: LGTM!The update to the assertion aligns with the removal of the versioned package import. Using the default value from
appconstssimplifies the test case and reduces the dependency on versioned constants.specs/src/parameters_v3.md (3)
9-15: Great job with the global parameters table!The table provides a clear and structured format for presenting the global parameters. The "Changeable via Governance" column is particularly helpful for users to quickly identify which parameters are immutable. The concise explanations in the "Summary" column effectively convey the purpose of each parameter.
16-69: Comprehensive and well-structured module parameters table!The module parameters table is extensive and covers a wide range of functionalities. The consistent structure, including the "Changeable via Governance" column, makes it easy to navigate and understand each parameter. The default values and summaries provide valuable context for users and developers alike.
70-72: Informative note about the mint module parameters!The note provides important information about the mint module parameters, clearly stating that they are not governance modifiable due to being hardcoded constants. Referencing the x/mint README.md file is a helpful addition for users who want to learn more about the reasoning behind this decision.
| {v2.Version, "SingleSignatureData v2", signing.SignatureV2{PubKey: priv1.PubKey()}}, | ||
| {v2.Version, "MultiSignatureData v2", signing.SignatureV2{PubKey: priv1.PubKey(), Data: multisig.NewMultisig(2)}}, | ||
| {appconsts.LatestVersion, fmt.Sprintf("SingleSignatureData v%d", appconsts.LatestVersion), signing.SignatureV2{PubKey: priv1.PubKey()}}, | ||
| {appconsts.LatestVersion, fmt.Sprintf("MultiSignatureData v%d", appconsts.LatestVersion), signing.SignatureV2{PubKey: priv1.PubKey(), Data: multisig.NewMultisig(2)}}, |
There was a problem hiding this comment.
These test cases don't contain an expected amount of gas to be consumed. IMO this test would be more likely to catch bugs if it verified that the amount of gas consumed by v2 differed from v3. Since the params for v2 and v3 are the same, one way to do that would be to change the value of the param for v2 and/or v3 and verify that the gas consumed was based on that.
| // DefaultGasPerBlobByte is the default gas cost deducted per byte of blob | ||
| // included in a PayForBlobs txn | ||
| DefaultGasPerBlobByte = 8 | ||
|
|
There was a problem hiding this comment.
https://github.com/celestiaorg/celestia-app/blob/main/docs/release-notes/pending-release.md now exists so we should add a line that states this constant was removed and what library consumers should use instead.
| // NetworkMinGasPrice is used by x/minfee to prevent transactions from being | ||
| // included in a block if they specify a gas price lower than this. | ||
| NetworkMinGasPrice float64 = 0.000001 // utia |
There was a problem hiding this comment.
We now have a place for this: https://github.com/celestiaorg/celestia-app/blob/main/docs/release-notes/pending-release.md
Co-authored-by: Rootul P <[email protected]>
Co-authored-by: Rootul P <[email protected]>
Co-authored-by: Rootul P <[email protected]>
|
DefaultGasPerBlobByte is still accessible through |
Oh good point, nvm disregard my comment about adding it to release notes. Does that mean the only breaking change in this PR is the removal of |
There was a problem hiding this comment.
Since this PR is marked breaking, I think we should at the very least add notes to pending-release.md about what was API breaking.
If we wanted to go above and beyond, we could add a line describing what this refactor does to pending-release.md so that we can use it in the release notes.
I want to merge this but will do in a follow up |
| // GasPerBlobByte is a versioned param from version 3 onwards. | ||
| var gasToConsume uint64 | ||
| if ctx.BlockHeader().Version.App <= v2.Version { | ||
| gasToConsume = types.GasToConsume(msg.BlobSizes, k.GasPerBlobByte(ctx)) | ||
| } else { | ||
| gasToConsume = types.GasToConsume(msg.BlobSizes, appconsts.GasPerBlobByte(ctx.BlockHeader().Version.App)) | ||
| } |
I missed this when reviewing #3735
Overview