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.

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

refactor: version constants for msg limits #3982

Closed

Conversation

rootulp
Copy link
Collaborator

@rootulp rootulp commented Oct 16, 2024

Closes #3973

@rootulp rootulp self-assigned this Oct 16, 2024
@rootulp rootulp marked this pull request as ready for review October 16, 2024 14:37
@rootulp rootulp requested a review from a team as a code owner October 16, 2024 14:37
Copy link
Contributor

coderabbitai bot commented Oct 16, 2024

📝 Walkthrough

Walkthrough

The 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

File(s) Change Summary
app/test/prepare_proposal_test.go Updated transaction cap constants to be version-dependent; modified test cases accordingly.
app/validate_txs.go Updated filterStdTxs and filterBlobTxs functions to retrieve transaction capacity based on version.
pkg/appconsts/global_consts.go Removed static constants NonPFBTransactionCap and PFBTransactionCap.
pkg/appconsts/v2/app_consts.go, pkg/appconsts/v3/app_consts.go Added version-specific constants for transaction caps (200 for NonPFB, 600 for PFB).
pkg/appconsts/versioned_consts.go, pkg/appconsts/versioned_consts_test.go Introduced functions for retrieving version-specific constants and updated tests for validation.

Assessment against linked issues

Objective Addressed Explanation
Version the parameters used to limit tx types (#3973)

Possibly related PRs

Suggested labels

optimization, WS: Maintenance 🔧, external

Suggested reviewers

  • liamsi
  • evan-forbes
  • cmwaters
  • ninabarbakadze

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?

❤️ Share
🪧 Tips

Chat

There are 3 ways to chat with CodeRabbit:

‼️ IMPORTANT
Auto-reply has been disabled for this repository in the CodeRabbit settings. The CodeRabbit bot will not respond to your replies unless it is explicitly tagged.

  • Files and specific lines of code (under the "Files changed" tab): Tag @coderabbitai in a new review comment at the desired location with your query. Examples:
    • @coderabbitai generate unit testing code for this file.
    • @coderabbitai modularize this function.
  • PR comments: Tag @coderabbitai in a new PR comment to ask questions about the PR branch. For the best results, please provide a very specific query, as very limited context is provided in this mode. Examples:
    • @coderabbitai gather interesting stats about this repository and render them as a table. Additionally, render a pie chart showing the language distribution in the codebase.
    • @coderabbitai read src/utils.ts and generate unit testing code.
    • @coderabbitai read the files in the src/scheduler package and generate a class diagram using mermaid and a README in the markdown format.
    • @coderabbitai help me debug CodeRabbit configuration file.

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)

  • @coderabbitai pause to pause the reviews on a PR.
  • @coderabbitai resume to resume the paused reviews.
  • @coderabbitai review to trigger an incremental review. This is useful when automatic reviews are disabled for the repository.
  • @coderabbitai full review to do a full review from scratch and review all the files again.
  • @coderabbitai summary to regenerate the summary of the PR.
  • @coderabbitai resolve resolve all the CodeRabbit review comments.
  • @coderabbitai configuration to show the current CodeRabbit configuration for the repository.
  • @coderabbitai help to get help.

Other keywords and placeholders

  • Add @coderabbitai ignore anywhere in the PR description to prevent this PR from being reviewed.
  • Add @coderabbitai summary to generate the high-level summary at a specific location in the PR description.
  • Add @coderabbitai anywhere in the PR title to generate the title automatically.

Documentation and Community

  • Visit our Documentation for detailed information on how to use CodeRabbit.
  • Join our Discord Community to get help, request features, and share feedback.
  • Follow us on X/Twitter for updates and announcements.

Copy link
Contributor

@coderabbitai coderabbitai bot left a comment

Choose a reason for hiding this comment

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

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:

  1. Consider adding a file-level comment explaining the purpose of this versioned constants file and how it relates to other versions.
  2. 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 NonPFBTransactionCap aligns 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 PFBTransactionCap function is well-implemented and consistent with the NonPFBTransactionCap function. 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 NonPFBTransactionCap and PFBTransactionCap functions 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:

  1. The new functions follow the existing pattern in the file, maintaining consistency.
  2. The unused version parameters allow for easy implementation of version-specific behavior in the future.
  3. 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 NonPFBTransactionCap and PFBTransactionCap are 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 NonPFBTransactionCap and PFBTransactionCap. 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 allowed
app/validate_txs.go (3)

60-60: Approve change with minor suggestion for clarity

The 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 consistency

The 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 limits

The 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:

  1. Both filterStdTxs and filterBlobTxs functions now use dynamic, version-dependent transaction caps.
  2. The implementation allows for future modifications to transaction limits without significant code changes.
  3. 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 the appVersion into 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

📥 Commits

Files that changed from the base of the PR and between 3d49b60 and 00bef1e.

📒 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.

  1. Could you provide context on why the value 600 was chosen for PFBTransactionCap?
  2. 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 successfully

The addition of the NonPFBTransactionCap constant 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 successfully

The addition of the PFBTransactionCap constant 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 limits

The addition of NonPFBTransactionCap and PFBTransactionCap constants 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 NonPFBTransactionCap and PFBTransactionCap are 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 division

In 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 by numberOfMsgsPerTx. Please verify that this calculation accurately reflects the intended number of transactions.


357-358: Ensure consistency in test inputs and expected outputs

In the test cases starting at lines 357 and 367, the inputTransactions slices include an additional +50 transactions beyond the cap:

inputTransactions: pfbTxs[:appconsts.PFBTransactionCap(testApp.AppVersion()) + 50]
inputTransactions: msgSendTxs[:appconsts.NonPFBTransactionCap(testApp.AppVersion()) + 50]

However, the expectedTransactions slices 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 inputTransactions is intentional and that expectedTransactions correctly 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
Copy link
Contributor

Choose a reason for hiding this comment

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

🛠️ 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

Comment on lines +379 to +380
expected := make([][]byte, 0, appconsts.NonPFBTransactionCap(testApp.AppVersion())+100)
expected = append(expected, msgSendTxs[:appconsts.NonPFBTransactionCap(testApp.AppVersion())]...)
Copy link
Contributor

Choose a reason for hiding this comment

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

🛠️ 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

Copy link
Member

@rach-id rach-id left a comment

Choose a reason for hiding this comment

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

Thanks for handling this 🙏

@@ -72,6 +72,30 @@ func TestVersionedConsts(t *testing.T) {
expectedConstant: v3.MaxTxSize,
got: appconsts.MaxTxSize(v3.Version),
},
{
Copy link
Member

Choose a reason for hiding this comment

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

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

@cmwaters
Copy link
Contributor

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

@rootulp
Copy link
Collaborator Author

rootulp commented Oct 16, 2024

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.

@rootulp rootulp closed this Oct 16, 2024
@rootulp rootulp deleted the rp/version-constants-limit-tx branch October 16, 2024 18:41
@evan-forbes
Copy link
Member

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

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

@evan-forbes
Copy link
Member

we can also version them later if that is the case, so I'm also good with keeping this closed until then

@rootulp
Copy link
Collaborator Author

rootulp commented Oct 16, 2024

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.

Why can't we change them until some consensus breaking change? It's confusing because the constants are located in a file named global_consts.go but they don't actually need to remain constant for the lifetime of the network like the other constants in that file:

https://github.com/rootulp/celestia-app/blob/17b3a727e698c298658f329362ab77d5fb045684/pkg/appconsts/global_consts.go#L14

They can change whenever. In retrospect, it might have been clearer to define these constants where they're used in validate_txs.go.

@rach-id
Copy link
Member

rach-id commented Oct 17, 2024

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 😄

In retrospect, it might have been clearer to define these constants where they're used in validate_txs.go

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 😄

@rootulp
Copy link
Collaborator Author

rootulp commented Oct 17, 2024

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.

I agree, this could help

Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment
Labels
None yet
Projects
None yet
Development

Successfully merging this pull request may close these issues.

Version the parameters used to limit tx types
5 participants