feat: limit the number of msg send and PFBs in prepare proposal #3942
Conversation
rach-id
requested review from
cmwaters and
ninabarbakadze
and removed request for
a team
📝 Walkthrough📝 WalkthroughWalkthroughThe changes in this pull request primarily enhance the testing framework and transaction filtering logic within the application. New test cases are added to validate transaction limits for different types, while modifications to existing functions improve transaction filtering by enforcing caps on specific transaction types. Additionally, new constants are introduced to define these limits, and the setup process for test applications is made more flexible through new initialization functions. Changes
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
|
| testApp := NewTestApp() | ||
| genesisState, valSet, kr := GenesisStateWithSingleValidator(testApp, genAccounts...) | ||
|
|
||
| // hacky way of changing the gov max square size without changing the consts |
There was a problem hiding this comment.
@cmwaters I changed the max square size in here in this hacky way. If you think it's fine, I can make this PR ready for review.
| @@ -6,4 +6,9 @@ const ( | |||
| SubtreeRootThreshold int = 64 | |||
| TxSizeCostPerByte uint64 = 10 | |||
| GasPerBlobByte uint32 = 8 | |||
| // MsgSendTransactionCap maximum number of msg send transactions that a block can contain | |||
| MsgSendTransactionCap = 3200 | |||
There was a problem hiding this comment.
will be adding couple comments referencing from where we get the numbers and the actual numbers once this PR is merged and the benchmarks one also is.
app/validate_txs.go
Outdated
| msgTypes := msgTypes(sdkTx) | ||
| if count := countOccurrence(msgTypes, sdk.MsgTypeURL(&types2.MsgPayForBlobs{})); count != 0 { |
There was a problem hiding this comment.
I'm not sure if relying on the strings in the msg alone is bullet proof 🤔. Do you know if a msg can be faked and cause the node to do undue execution in process proposal?
There was a problem hiding this comment.
if I understand correctly, the msg is taken after casting the sdk transaction so it should have the right value.
Do you mean we manually cast the transaction to a MsgSend and see? casts are often too expensive, that's why I just used the msg type URL
app/validate_txs.go
Outdated
| msgSendTransactionCount := 0 | ||
| for _, tx := range txs { | ||
| sdkTx, err := dec(tx) | ||
| if err != nil { | ||
| logger.Error("decoding already checked transaction", "tx", tmbytes.HexBytes(coretypes.Tx(tx).Hash()), "error", err) | ||
| continue | ||
| } | ||
| msgTypes := msgTypes(sdkTx) | ||
| if count := countOccurrence(msgTypes, sdk.MsgTypeURL(&types.MsgSend{})); count != 0 { | ||
| if msgSendTransactionCount+count > v3consts.MsgSendTransactionCap { | ||
| logger.Debug("skipping tx because the msg send transaction cap was reached", "tx", tmbytes.HexBytes(coretypes.Tx(tx).Hash())) | ||
| continue | ||
| } | ||
| msgSendTransactionCount += count | ||
| } |
There was a problem hiding this comment.
for posterity: iirc we're optimizing for simplicity here, and therefore simply counting the number of messages. in the future, we could use a multidimensional gas price model that would account for the savings of only verifying a single signature for multiple msgs
app/validate_txs.go
Outdated
| // countOccurrence takes a strings slice and counts the number | ||
| // of time the provided item exists in that slice. | ||
| func countOccurrence(slice []string, item string) int { | ||
| count := 0 | ||
| for _, v := range slice { | ||
| if v == item { | ||
| count++ | ||
| } | ||
| } | ||
| return count | ||
| } |
There was a problem hiding this comment.
very very optional
we might be able to refactor this and msgTypes into a single function that returns all of the different msg types in a map or smth
There was a problem hiding this comment.
we will need to sequentially go over the message types in all cases, same complexity. Or you mean we make them part of a single function?
There was a problem hiding this comment.
Or you mean we make them part of a single function?
yeah, just this. very optional
![coderabbitai[bot]](https://avatars.githubusercontent.com/in/347564?s=60&v=4){kind=small}
There was a problem hiding this comment.
Actionable comments posted: 5
🧹 Outside diff range and nitpick comments (1)
pkg/appconsts/v3/app_consts.go (1)
9-13: Consider documenting the overall impact of transaction caps and their future adjustments.The introduction of MsgSendTransactionCap and PFBTransactionCap is a significant change that could impact the system's behavior and performance. To ensure clarity and maintainability, please consider:
- Documenting the combined effect of these caps on the overall block size and transaction throughput.
- Explaining the rationale behind having separate caps for different transaction types.
- Outlining any plans for future adjustments to these caps based on network performance or other factors.
- Adding a comment referencing the benchmarks or analysis used to determine these values, as mentioned in the previous review.
This documentation will be crucial for understanding the system's constraints and for future optimizations.
📜 Review details
Configuration used: .coderabbit.yaml
Review profile: CHILL
📒 Files selected for processing (4)
- app/test/prepare_proposal_test.go (2 hunks)
- app/validate_txs.go (5 hunks)
- pkg/appconsts/v3/app_consts.go (1 hunks)
- test/util/test_app.go (4 hunks)
🧰 Additional context used
🔇 Additional comments (8)
pkg/appconsts/v3/app_consts.go (2)
12-13: Please provide more context for the PFBTransactionCap value and clarify the term "PFB".The introduction of this constant is beneficial for limiting PFB messages per block. However, could you please address the following points:
- What does "PFB" stand for in this context?
- How was the value of 2700 determined?
- What are the implications of this limit on the system's performance and behavior?
- Is this limit subject to change based on network conditions or future optimizations?
Adding comments in the code to explain these points and the meaning of "PFB" would greatly improve maintainability and understanding for future developers.
To ensure this constant is used correctly throughout the codebase, let's run a quick check:
#!/bin/bash # Search for usages of PFBTransactionCap rg --type go "PFBTransactionCap"
9-10: Please provide more context for the MsgSendTransactionCap value.The introduction of this constant is a good step towards limiting the number of transactions per block. However, could you please provide more information on:
- How was the value of 3200 determined?
- What are the implications of this limit on the system's performance and behavior?
- Is this limit subject to change based on network conditions or future optimizations?
Adding comments in the code to explain these points would greatly improve maintainability and understanding for future developers.
To ensure this constant is used correctly throughout the codebase, let's run a quick check:
app/test/prepare_proposal_test.go (1)
231-231:⚠️ Potential issueVerify the block's
MaxBytessettingLine 231 sets
consensusParams.Block.MaxBytes = 128, which seems unusually low given that the test involves a large number of transactions. This small block size may prevent any transactions from being included in the block during testing, potentially causing the test to not function as intended. Please verify if this value is intentional or if it should be increased to accommodate the test transactions.test/util/test_app.go (5)
67-69: Code refactored for better modularityThe
initialiseTestAppfunction is now used to initialize the test app, promoting code reuse and improving modularity.
71-75: Added function for setting up test app with custom max square sizeThe new
SetupTestAppWithGenesisValSetAndMaxSquareSizefunction allows specifying a custommaxSquareSize, enhancing the flexibility of the test setup.
Line range hint
77-85: NewinitialiseTestAppfunction improves code organizationThe
initialiseTestAppfunction encapsulates the app initialization logic, improving code readability and maintainability.
190-192: Updated function to useInitialiseTestAppWithGenesisThe
NewTestAppWithGenesisSetfunction now usesInitialiseTestAppWithGenesisto initialize the app with the genesis state, ensuring consistency across test setups.
Line range hint
209-240: NewInitialiseTestAppWithGenesisfunction centralizes initialization logicThe
InitialiseTestAppWithGenesisfunction centralizes the app initialization with a given genesis state, enhancing code maintainability and reducing duplication.
There was a problem hiding this comment.
Actionable comments posted: 1
🧹 Outside diff range and nitpick comments (2)
app/validate_txs.go (2)
4-5: Consider using a more descriptive alias instead oftypes2The alias
types2for the imported packagegithub.com/celestiaorg/celestia-app/v3/x/blob/typesis not very descriptive and could lead to confusion. Consider renaming it toblobtypesorblobTypesto enhance readability and clarity.
131-136: Rename variabletypesto avoid confusion with imported packagesUsing
typesas a variable name may cause confusion due to the imported packages namedtypes. Consider renaming the variable tomessageTypesormsgTypeListfor better clarity and to prevent potential conflicts.
This PR limits the number of transactions in prepare proposal.