refactor: version constants for msg limits #3982
Conversation
📝 WalkthroughWalkthroughThe changes in this pull request focus on implementing version-aware transaction limits for both PFB (Pay For Blob) and non-PFB transactions. This involves removing static constants and replacing them with dynamic retrieval methods based on the application version. The updates affect various files, including test cases that validate the new version-dependent transaction limits, ensuring that the application can adapt to future changes in transaction constraints. Changes
Assessment against linked issues
Possibly related PRs
Suggested labels
Suggested reviewers
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: 2
🧹 Outside diff range and nitpick comments (8)
pkg/appconsts/v2/app_consts.go (1)
Line range hint
1-12: Consider adding more comprehensive documentation.The addition of these constants is a good step towards implementing versioned parameters for transaction limits. To further improve maintainability and understanding:
- Consider adding a file-level comment explaining the purpose of this versioned constants file and how it relates to other versions.
- It might be helpful to include a brief explanation of how these constants are used in the broader context of the application, particularly in relation to block composition and transaction processing.
pkg/appconsts/versioned_consts.go (3)
49-51: LGTM! Consider adding a comment for future maintainers.The implementation of
NonPFBTransactionCapaligns well with the PR objectives and follows the pattern of existing functions in this file. It prepares for future versioning by including an unused version parameter.Consider adding a brief comment explaining the purpose of the unused parameter for future maintainers:
+// NonPFBTransactionCap returns the maximum number of non-PFB transactions allowed. +// The version parameter is currently unused but allows for future version-specific implementations. func NonPFBTransactionCap(_ uint64) int { return v3.NonPFBTransactionCap }
53-55: LGTM! Consider adding a comment for clarity.The
PFBTransactionCapfunction is well-implemented and consistent with theNonPFBTransactionCapfunction. It prepares for future versioning and aligns with the PR objectives.For consistency and clarity, consider adding a comment similar to the one suggested for
NonPFBTransactionCap:+// PFBTransactionCap returns the maximum number of PFB transactions allowed. +// The version parameter is currently unused but allows for future version-specific implementations. func PFBTransactionCap(_ uint64) int { return v3.PFBTransactionCap }
49-55: Overall assessment: Well-implemented and aligned with PR objectives.The addition of
NonPFBTransactionCapandPFBTransactionCapfunctions effectively implements versioning for transaction limit parameters. These changes align perfectly with the PR objectives and prepare the codebase for potential future modifications to transaction constraints.Key points:
- The new functions follow the existing pattern in the file, maintaining consistency.
- The unused version parameters allow for easy implementation of version-specific behavior in the future.
- The changes are minimal and non-disruptive to the existing codebase.
These modifications enhance the flexibility and maintainability of the code, addressing the concerns raised in issue #3973.
To further improve maintainability, consider creating a separate file for version-specific constants (e.g.,
v3/constants.go) if it doesn't already exist. This would allow for easier management of version-specific values in the future.pkg/appconsts/versioned_consts_test.go (1)
75-98: LGTM! Consider adding a comment explaining the purpose of these new constants.The new test cases for
NonPFBTransactionCapandPFBTransactionCapare well-structured and consistent with the existing test cases. They appropriately cover both v2 and v3 versions, which is excellent for ensuring backwards compatibility.Consider adding a brief comment above these new test cases to explain the purpose of
NonPFBTransactionCapandPFBTransactionCap. This would enhance the readability and maintainability of the test suite. For example:// Test cases for transaction cap constants // NonPFBTransactionCap: Maximum number of non-PFB transactions allowed // PFBTransactionCap: Maximum number of PFB transactions allowedapp/validate_txs.go (3)
60-60: Approve change with minor suggestion for clarityThe modification to use
appconsts.NonPFBTransactionCap(ctx.ConsensusParams().Version.AppVersion)is a good implementation of version-aware transaction limits. This change aligns well with the PR objectives and allows for future adjustments to the transaction cap without code modifications.To improve clarity, consider extracting the app version into a variable:
+ appVersion := ctx.ConsensusParams().Version.AppVersion - if nonPFBTransactionsCount+len(sdkTx.GetMsgs()) > appconsts.NonPFBTransactionCap(ctx.ConsensusParams().Version.AppVersion) { + if nonPFBTransactionsCount+len(sdkTx.GetMsgs()) > appconsts.NonPFBTransactionCap(appVersion) {This change would make the code more readable and easier to maintain.
104-104: Approve change with minor suggestion for consistencyThe modification to use
appconsts.PFBTransactionCap(ctx.ConsensusParams().Version.AppVersion)is a good implementation of version-aware transaction limits for PFB transactions. This change is consistent with the previous modification and aligns well with the PR objectives.For consistency with the previous suggestion and to improve clarity, consider extracting the app version into a variable:
+ appVersion := ctx.ConsensusParams().Version.AppVersion - if pfbTransactionCount+len(sdkTx.GetMsgs()) > appconsts.PFBTransactionCap(ctx.ConsensusParams().Version.AppVersion) { + if pfbTransactionCount+len(sdkTx.GetMsgs()) > appconsts.PFBTransactionCap(appVersion) {This change would make the code more readable and maintain consistency with the suggested change in
filterStdTxs.
Line range hint
1-150: Summary: Successfully implemented version-aware transaction limitsThe changes in this file successfully implement version-aware transaction limits for both non-PFB and PFB transactions. This implementation aligns well with the PR objectives of versioning the parameters used to limit transaction types.
Key points:
- Both
filterStdTxsandfilterBlobTxsfunctions now use dynamic, version-dependent transaction caps.- The implementation allows for future modifications to transaction limits without significant code changes.
- The changes are consistent and maintain the overall structure of the existing code.
These modifications effectively address the concerns raised in issue #3973 and provide the desired flexibility for future updates to transaction type limits.
Suggestion for further improvement:
Consider extracting theappVersioninto a variable in both functions for improved readability and maintainability, as mentioned in the previous comments.Overall, this implementation is a solid step towards enhancing the adaptability of the Celestia application with regards to transaction limit management.
📜 Review details
Configuration used: .coderabbit.yaml
Review profile: CHILL
📒 Files selected for processing (7)
- app/test/prepare_proposal_test.go (5 hunks)
- app/validate_txs.go (2 hunks)
- pkg/appconsts/global_consts.go (0 hunks)
- pkg/appconsts/v2/app_consts.go (1 hunks)
- pkg/appconsts/v3/app_consts.go (1 hunks)
- pkg/appconsts/versioned_consts.go (1 hunks)
- pkg/appconsts/versioned_consts_test.go (1 hunks)
💤 Files with no reviewable changes (1)
- pkg/appconsts/global_consts.go
🧰 Additional context used
🔇 Additional comments (7)
pkg/appconsts/v2/app_consts.go (2)
10-12: Approved addition of PFBTransactionCap constant with suggestions.The addition of this constant is in line with the PR objectives. The comment clearly explains its purpose.
- Could you provide context on why the value 600 was chosen for PFBTransactionCap?
- Consider adding a comment explaining the relationship between PFBTransactionCap and NonPFBTransactionCap, as the PFB cap is significantly higher.
#!/bin/bash # Search for any existing usage or references to the value 600 in the context of transaction limits rg --type go "600.*transaction" .
7-9: Approved addition of NonPFBTransactionCap constant.The addition of this constant aligns well with the PR objectives of implementing versioning for transaction limit parameters. The comment clearly explains its purpose.
Could you provide some context on why the value 200 was chosen for NonPFBTransactionCap? This information would be valuable for future reference and maintenance.
pkg/appconsts/v3/app_consts.go (3)
10-12: LGTM: NonPFBTransactionCap constant added successfullyThe addition of the
NonPFBTransactionCapconstant with a value of 200 is well-implemented. The accompanying comment clearly explains its purpose as the maximum number of non-PFB SDK messages allowed in a block. This change aligns well with the PR objectives of versioning parameters for transaction limits.
13-15: LGTM: PFBTransactionCap constant added successfullyThe addition of the
PFBTransactionCapconstant with a value of 600 is well-implemented. The accompanying comment clearly explains its purpose as the maximum number of PFB messages allowed in a block. This change aligns well with the PR objectives of versioning parameters for transaction limits.Could you please confirm if the higher cap for PFB transactions (600) compared to non-PFB transactions (200) is intentional? This difference might have implications for block composition and transaction processing.
10-15: Summary: Successful implementation of versioned transaction limitsThe addition of
NonPFBTransactionCapandPFBTransactionCapconstants successfully implements versioned parameters for transaction limits. This change aligns perfectly with the PR objectives and will facilitate easier modifications in future releases.To ensure complete implementation, please verify that these new constants are being used appropriately throughout the codebase. Run the following script to check for their usage:
If the constants are not yet used, consider opening a follow-up issue to update the relevant parts of the codebase.
✅ Verification successful
Verification Completed: Constants are Properly Utilized
Confirmed that
NonPFBTransactionCapandPFBTransactionCapare actively used across the codebase, including in versioned constants, application logic, and test suites. No issues found regarding their implementation or usage.🏁 Scripts executed
The following scripts were executed for the analysis:
Script:
#!/bin/bash # Description: Check usage of new transaction cap constants # Search for usage of NonPFBTransactionCap echo "Checking usage of NonPFBTransactionCap:" rg --type go "NonPFBTransactionCap" echo "\nChecking usage of PFBTransactionCap:" rg --type go "PFBTransactionCap"Length of output: 5850
app/test/prepare_proposal_test.go (2)
363-363: Verify the calculation for expected transactions with integer divisionIn line 363, the expected number of transactions is calculated using integer division:
expectedTransactions := multiPFBsPerTxs[:appconsts.PFBTransactionCap(testApp.AppVersion()) / numberOfMsgsPerTx]Since integer division truncates fractional parts, this might result in an off-by-one error if
appconsts.PFBTransactionCap(testApp.AppVersion())is not perfectly divisible bynumberOfMsgsPerTx. Please verify that this calculation accurately reflects the intended number of transactions.
357-358: Ensure consistency in test inputs and expected outputsIn the test cases starting at lines 357 and 367, the
inputTransactionsslices include an additional+50transactions beyond the cap:inputTransactions: pfbTxs[:appconsts.PFBTransactionCap(testApp.AppVersion()) + 50] inputTransactions: msgSendTxs[:appconsts.NonPFBTransactionCap(testApp.AppVersion()) + 50]However, the
expectedTransactionsslices only include transactions up to the cap:expectedTransactions: pfbTxs[:appconsts.PFBTransactionCap(testApp.AppVersion())] expectedTransactions: msgSendTxs[:appconsts.NonPFBTransactionCap(testApp.AppVersion())]Please ensure that the test cases are designed to accurately assess the capping logic. Confirm that including the extra transactions in
inputTransactionsis intentional and thatexpectedTransactionscorrectly reflects the transactions that should be processed after capping.Also applies to: 367-368
| @@ -297,7 +297,7 @@ func TestPrepareProposalCappingNumberOfMessages(t *testing.T) { | |||
| signers = append(signers, signer) | |||
| } | |||
|
|
|||
| numberOfPFBs := appconsts.PFBTransactionCap + 500 | |||
| numberOfPFBs := appconsts.PFBTransactionCap(testApp.AppVersion()) + 500 | |||
There was a problem hiding this comment.
🛠️ Refactor suggestion
Consider caching testApp.AppVersion() for improved readability
The function testApp.AppVersion() is called multiple times throughout the test. To enhance readability and avoid redundant function calls, consider storing the application version in a local variable:
appVersion := testApp.AppVersion()Then, replace subsequent calls with appVersion.
Also applies to: 336-336, 357-358, 362-363, 367-368, 379-380, 394-396
| expected := make([][]byte, 0, appconsts.NonPFBTransactionCap(testApp.AppVersion())+100) | ||
| expected = append(expected, msgSendTxs[:appconsts.NonPFBTransactionCap(testApp.AppVersion())]...) |
There was a problem hiding this comment.
🛠️ Refactor suggestion
Adjust the capacity of the expected slice for accuracy
In lines 379 and 394, the capacity of the expected slice includes an extra +100:
expected := make([][]byte, 0, appconsts.NonPFBTransactionCap(testApp.AppVersion()) + 100)
expected := make([][]byte, 0, appconsts.PFBTransactionCap(testApp.AppVersion()) + 100)Since you're appending only up to the transaction cap, the additional capacity may be unnecessary. Consider adjusting the capacity to match the exact number of expected transactions for clarity and to avoid potential misunderstandings.
Also applies to: 394-396
| @@ -72,6 +72,30 @@ func TestVersionedConsts(t *testing.T) { | |||
| expectedConstant: v3.MaxTxSize, | |||
| got: appconsts.MaxTxSize(v3.Version), | |||
| }, | |||
| { | |||
There was a problem hiding this comment.
doesn't have to be addressed in this pr but we probably need to come up with a more efficient way of testing this before it gets out of hand with all the versions
|
These values are just in prepare proposal. They are not part of consensus and thus don't need to be versioned. We can change them at will from minor release to minor release |
is a good point. I think we should close this PR and #3973 as won't do. |
this is what I thought as well, but I think someone pointed out that this would just save us the scenario where we can't increase them until some consensus breaking change. In that case, we don't have to have the consensus breaking change first, then cut a release that has the increased values. therefore I'm good with this now |
|
we can also version them later if that is the case, so I'm also good with keeping this closed until then |
Why can't we change them until some consensus breaking change? It's confusing because the constants are located in a file named They can change whenever. In retrospect, it might have been clearer to define these constants where they're used in |
|
We could argue that versioning can be beneficial in scenarios like one version is running 2mb blocks, with these limitations softly enforced by validators not changing the code, and the next version has some outstanding optimisations that won't be activated until a breaking upgrade is done that allows for higher limitations and 8mb blocks for example. In this case we would want to enforce a soft control on the blocks being produced until the upgrade height, and versioning would be beneficial. But I guess that it's fine to leave it for future us to worry about it and implement it if needed. We can always version anything in the code 😄
I think the confusion arises from these constants being recommended but not mandatory, being only softly enforced. The reason why I think they belong in app consts is to avoid having limitations scattered around the repo that devs will need to source dive to find them (similar to what sometimes happens in LibP2P where some consts are set in weird places, and you have no idea why the code is behaving in a certain way until you deep dive). Having the consts in the same place helps with this. We might think of adding a new file that contains non consensus breaking consts and put them all in there, to make the distinction between consensus breaking vs non-breaking. But I guess we don't need to worry much about this as this code will change in the near future with blocks increasing in size, and we might not even need to enforce such limitations. TL;DR: I see no harm in merging this PR or leaving it for our future selves to worry about 😄 |
I agree, this could help |
Closes #3973