fix(consensus): raised max_tx_bytes will above txs size #2117
Conversation
WalkthroughThe pull request modifies the Changes
Suggested reviewers
Poem
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
CodeRabbit Configuration File (
|
![coderabbitai[bot]](https://avatars.githubusercontent.com/in/347564?s=60&v=4){kind=small}
There was a problem hiding this comment.
Actionable comments posted: 1
📜 Review details
Configuration used: CodeRabbit UI
Review profile: ASSERTIVE
📒 Files selected for processing (1)
testing/networks/80084/config.toml(1 hunks)
🔇 Additional comments (1)
testing/networks/80084/config.toml (1)
368-368: 🧹 Nitpick (assertive)
Review max_tx_bytes in relation to other parameters
The increased max_tx_bytes value of 100MB creates inconsistencies with other configuration parameters:
- It exceeds max_txs_bytes (64MB), meaning a single transaction could be larger than the entire mempool capacity
- The large transaction size might stress the P2P network due to the small max_packet_msg_payload_size (1KB)
Consider:
- Increasing max_txs_bytes to accommodate the larger max_tx_bytes
- Evaluating P2P network parameters for large transaction handling
Let's verify the transaction size requirements:
✅ Verification successful
Let me gather more information about the blob size and related configurations.
Based on the search results, I can now provide a final response as we have sufficient information about blob sizes and limits:
The 100MB max_tx_bytes limit is justified for EIP-4844 blob transactions
The increased limit is necessary and reasonable because:
- Each blob is 131,072 bytes (BytesPerBlob constant)
- Maximum 6 blobs per block (MaxBlobsPerBlock constant)
- Total potential blob size: ~786KB (6 * 131KB)
- Additional overhead for BeaconBlock structure, commitments, and proofs
The current 100MB limit provides adequate headroom for blob transactions while remaining practically bounded by the protocol-level blob limits. The comment about being "impossible to hit" is accurate since the actual maximum transaction size is constrained by these protocol parameters.
🏁 Scripts executed
The following scripts were executed for the analysis:
Script:
#!/bin/bash
# Search for BeaconBlock and BlobSidecars struct definitions to understand size requirements
rg -A 10 "type (BeaconBlock|BlobSidecars) struct"
Length of output: 2360
Script:
#!/bin/bash
# Search for blob size constants and configurations
rg -i "blob.*size|max.*blob|blob.*limit" -A 3
# Search for any TOML configurations related to max transaction sizes
fd -e toml -x grep -l "max.*bytes\|max.*size" {} \; -exec cat {} \;
Length of output: 8606
| @@ -365,7 +365,7 @@ wal_dir = "" | |||
| size = 0 | |||
|
|
|||
| # Maximum size in bytes of a single transaction accepted into the mempool. | |||
| max_tx_bytes = 1048576 | |||
| max_tx_bytes = 104857600 // 100 MB high enough to be impossible to hit | |||
There was a problem hiding this comment.
Fix TOML comment style
The comment uses // style which is not standard in TOML. TOML uses # for comments.
Apply this change:
-max_tx_bytes = 104857600 // 100 MB high enough to be impossible to hit
+max_tx_bytes = 104857600 # 100 MB high enough to be impossible to hit📝 Committable suggestion
‼️ IMPORTANT
Carefully review the code before committing. Ensure that it accurately replaces the highlighted code, contains no missing lines, and has no issues with indentation. Thoroughly test & benchmark the code to ensure it meets the requirements.
| max_tx_bytes = 104857600 // 100 MB high enough to be impossible to hit | |
| max_tx_bytes = 104857600 # 100 MB high enough to be impossible to hit |
Codecov ReportAll modified and coverable lines are covered by tests ✅
Additional details and impacted files@@ Coverage Diff @@
## main #2117 +/- ##
=======================================
Coverage 23.48% 23.48%
=======================================
Files 357 357
Lines 16064 16064
Branches 12 12
=======================================
Hits 3772 3772
Misses 12121 12121
Partials 171 171 |
abi87
added
DO NOT MERGE
| @@ -365,7 +365,7 @@ wal_dir = "" | |||
| size = 0 | |||
|
|
|||
| # Maximum size in bytes of a single transaction accepted into the mempool. | |||
| max_tx_bytes = 1048576 | |||
| max_tx_bytes = 104857600 // 100 MB high enough to be impossible to hit | |||
There was a problem hiding this comment.
@chuck-bear Do we run network out of this file?
If so, it may be enough for a quick patch to change this value in the config file.
If not, I will have to add a config flag to override the default config value.
This PR sets
max_tx_bytesconfiguration in CometBFT high enough such that it is impossible to ever encounter a tuple of (BeaconBlock,BlobSidecars) that exceeds the byte limit.If that would happen, the proposed block will not be accepted and since in the current state, honest validators do not prune transactions, the failing txs would be proposed over and over again.
We should consider enforcing that the increase limit is picked by default upon config files generation