feat: increase default mempool TTLNumBlocks to 12
#3961
Conversation
rootulp
requested review from
cmwaters and
evan-forbes
and removed request for
a team
📝 Walkthrough📝 WalkthroughWalkthroughThe changes involve modifications to the Changes
Possibly related PRs
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: 1
📜 Review details
Configuration used: .coderabbit.yaml
Review profile: CHILL
📒 Files selected for processing (2)
- app/default_overrides.go (2 hunks)
- app/default_overrides_test.go (1 hunks)
🧰 Additional context used
🪛 GitHub Check: lint / golangci-lint
app/default_overrides.go
[failure] 267-267:
unnecessary conversion (unconvert)
🔇 Additional comments (4)
app/default_overrides_test.go (1)
83-87: LGTM! Changes align with PR objectives.The modifications to the
MempoolConfigstructure in theTestDefaultConsensusConfigfunction are consistent with the PR objectives:
TTLNumBlocksis updated to 12, aligning with the 75-secondTTLDuration.MaxTxBytesis set to 8 MiB (8 * mebibyte).MaxTxsBytesis set to 40 MiB (40 * mebibyte).The use of the
mebibyteconstant improves code readability and maintainability.To ensure consistency across the codebase, let's verify these changes:
✅ Verification successful
Verification Successful! All mempool configuration changes are consistent across the codebase.
🏁 Scripts executed
The following scripts were executed for the analysis:
Script:
#!/bin/bash # Description: Verify the mempool configuration changes in other files # Test 1: Check for TTLNumBlocks value echo "Checking for TTLNumBlocks value:" rg --type go "TTLNumBlocks.*12" --glob '!app/default_overrides_test.go' # Test 2: Check for MaxTxBytes value echo "Checking for MaxTxBytes value:" rg --type go "MaxTxBytes.*8.*mebibyte" --glob '!app/default_overrides_test.go' # Test 3: Check for MaxTxsBytes value echo "Checking for MaxTxsBytes value:" rg --type go "MaxTxsBytes.*40.*mebibyte" --glob '!app/default_overrides_test.go' # Test 4: Check for TTLDuration value echo "Checking for TTLDuration value:" rg --type go "TTLDuration.*75.*time\.Second" --glob '!app/default_overrides_test.go'Length of output: 907
app/default_overrides.go (3)
39-39: Definition ofmebibyteconstant improves readabilityDefining
mebibyteas1024 * 1024enhances code clarity by eliminating magic numbers when specifying sizes in mebibytes.
266-266: Increase ofTTLNumBlocksaligns withTTLDurationUpdating
cfg.Mempool.TTLNumBlocksto 12 ensures consistency withTTLDurationof 75 seconds, as 12 blocks × 6 seconds per block equals 72 seconds, closely matching the duration.
268-269: Usage ofmebibyteconstant enhances clarity in size configurationsSetting
MaxTxBytesandMaxTxsBytesusing themebibyteconstant improves readability and maintains consistency in how memory sizes are defined throughout the code.
app/default_overrides.go
Outdated
| cfg.Mempool.MaxTxsBytes = int64(upperBoundBytes) * cfg.Mempool.TTLNumBlocks | ||
| cfg.Mempool.Version = "v1" // prioritized mempool | ||
| cfg.Mempool.TTLNumBlocks = 12 | ||
| cfg.Mempool.TTLDuration = time.Duration(75 * time.Second) |
There was a problem hiding this comment.
Unnecessary conversion in TTLDuration assignment
The assignment to cfg.Mempool.TTLDuration unnecessarily wraps the multiplication in time.Duration(). Since time.Second is already a time.Duration, multiplying it by 75 yields a time.Duration, making the explicit conversion redundant.
Apply this diff to remove the unnecessary conversion:
- cfg.Mempool.TTLDuration = time.Duration(75 * time.Second)
+ cfg.Mempool.TTLDuration = 75 * time.Second📝 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.
| cfg.Mempool.TTLDuration = time.Duration(75 * time.Second) | |
| cfg.Mempool.TTLDuration = 75 * time.Second |
🧰 Tools
🪛 GitHub Check: lint / golangci-lint
[failure] 267-267:
unnecessary conversion (unconvert)
| @@ -259,19 +260,13 @@ func DefaultEvidenceParams() tmproto.EvidenceParams { | |||
| func DefaultConsensusConfig() *tmcfg.Config { | |||
| cfg := tmcfg.DefaultConfig() | |||
| // Set broadcast timeout to be 50 seconds in order to avoid timeouts for long block times | |||
| // TODO: make TimeoutBroadcastTx configurable per https://github.com/celestiaorg/celestia-app/issues/1034 | |||
There was a problem hiding this comment.
The linked GH issue is closed so this comment is stale
| upperBoundBytes := appconsts.DefaultSquareSizeUpperBound * appconsts.DefaultSquareSizeUpperBound * share.ContinuationSparseShareContentSize | ||
| cfg.Mempool.MaxTxBytes = upperBoundBytes | ||
| cfg.Mempool.MaxTxsBytes = int64(upperBoundBytes) * cfg.Mempool.TTLNumBlocks | ||
| cfg.Mempool.TTLNumBlocks = 12 |
There was a problem hiding this comment.
This is the only implementation change
| cfg.Mempool.TTLDuration = 75 * time.Second | ||
| cfg.Mempool.MaxTxBytes = 7_897_088 | ||
| cfg.Mempool.MaxTxsBytes = 39_485_440 |
There was a problem hiding this comment.
These 3 lines just set the values in a more clear way. The actual values are identical before + after this PR.
There was a problem hiding this comment.
Can we have an issue to bump this value past 8MB
There was a problem hiding this comment.
isn't this limited to 2MB anyway?
rootulp
changed the title
TTLNumBlocks to 12
There was a problem hiding this comment.
Actionable comments posted: 0
🧹 Outside diff range and nitpick comments (1)
app/default_overrides.go (1)
266-269: Approve mempool configuration changes with minor suggestionsThe changes to the mempool configuration align well with the PR objectives. The TTLNumBlocks and TTLDuration are now consistent, which is a good improvement.
However, there are slight discrepancies between the implemented values and the stated objectives:
- MaxTxBytes is set to 7,897,088 bytes, which is approximately 7.53 MiB, not exactly 8 MiB.
- MaxTxsBytes is set to 39,485,440 bytes, which is approximately 37.66 MiB, not exactly 40 MiB.
Consider adjusting these values to exactly match the PR objectives for clarity:
- cfg.Mempool.MaxTxBytes = 7_897_088 - cfg.Mempool.MaxTxsBytes = 39_485_440 + cfg.Mempool.MaxTxBytes = 8 * mebibyte + cfg.Mempool.MaxTxsBytes = 40 * mebibyteThis change would make use of the newly defined
mebibyteconstant and exactly match the stated objectives.
📜 Review details
Configuration used: .coderabbit.yaml
Review profile: CHILL
📒 Files selected for processing (2)
- app/default_overrides.go (2 hunks)
- app/default_overrides_test.go (1 hunks)
🚧 Files skipped from review as they are similar to previous changes (1)
- app/default_overrides_test.go
🧰 Additional context used
🪛 GitHub Check: lint / golangci-lint
app/default_overrides.go
[failure] 39-39:
constmebibyteis unused (unused)
🔇 Additional comments (1)
app/default_overrides.go (1)
39-40: Consider the usage of themebibyteconstantThe
mebibyteconstant is correctly defined, but it appears to be unused in the current file. Consider one of the following actions:
- If it's intended for future use, add a comment explaining its purpose.
- If it's used in other files, consider moving it to a more appropriate location or adding a comment explaining its usage.
- If it's not needed, remove the constant to keep the codebase clean.
To verify the usage of this constant across the codebase, you can run the following command:
✅ Verification successful
Verified usage of the
mebibyteconstant across the codebaseThe
mebibyteconstant is utilized in multiple test files, indicating its usage beyond theapp/default_overrides.gofile. If this constant is intended for broader application, consider relocating it to a shared test utilities package to improve maintainability.🏁 Scripts executed
The following scripts were executed for the analysis:
Script:
#!/bin/bash # Search for usage of the mebibyte constant rg --type go "mebibyte"Length of output: 1260
🧰 Tools
🪛 GitHub Check: lint / golangci-lint
[failure] 39-39:
constmebibyteis unused (unused)
There was a problem hiding this comment.
Actionable comments posted: 0
🧹 Outside diff range and nitpick comments (2)
app/default_overrides.go (2)
266-266: LGTM: MaxTxBytes updated, but consider precise 8 MiBThe
MaxTxByteshas been updated to 7,897,088 bytes (≈7.53 MiB), which aligns with the PR objective of updating the maximum transaction size to approximately 8 MiB for simplicity.However, if the goal is to set it to exactly 8 MiB, consider using 8,388,608 bytes instead.
If exact 8 MiB is desired, you can update the value as follows:
- cfg.Mempool.MaxTxBytes = 7_897_088 + cfg.Mempool.MaxTxBytes = 8 * 1024 * 1024 // Exactly 8 MiB
267-267: LGTM: MaxTxsBytes updated, but consider precise 40 MiBThe
MaxTxsByteshas been updated to 39,485,440 bytes (≈37.66 MiB), which aligns with the PR objective of raising the maximum transaction bytes limit to approximately 40 MiB for clarity.However, if the goal is to set it to exactly 40 MiB, consider using 41,943,040 bytes instead.
If exact 40 MiB is desired, you can update the value as follows:
- cfg.Mempool.MaxTxsBytes = 39_485_440 + cfg.Mempool.MaxTxsBytes = 40 * 1024 * 1024 // Exactly 40 MiB
📜 Review details
Configuration used: .coderabbit.yaml
Review profile: CHILL
📒 Files selected for processing (1)
- app/default_overrides.go (1 hunks)
🧰 Additional context used
🔇 Additional comments (2)
app/default_overrides.go (2)
264-264: LGTM: TTLNumBlocks adjustmentThe increase of
TTLNumBlocksfrom 5 to 12 aligns with the PR objectives. This change ensures consistency with the TTLDuration of 75 seconds, as 12 six-second blocks total 72 seconds.
265-265: LGTM: TTLDuration set and unnecessary conversion removedThe
TTLDurationis correctly set to 75 seconds, which is consistent with the PR objectives. Additionally, the unnecessary conversion totime.Durationhas been removed, addressing a previous review comment. This change improves code clarity and efficiency.
There was a problem hiding this comment.
I'm pretty indifferent about this. Keep in mind this PR is defaults so it won't change any existing nodes. Just new nodes.
I would slightly prefer to increase to 8MB which can path the way for increases in the cap when they come (which we will want them to).
I think it would be more useful to also update the descriptions in the TOML file since for example, MaxTxBytes isn't the only length cap, in fact the main one is in the antehandler (so we might even consider replacing this with a hard coded number).
Something I would advocate for is that the created toml file actually comments out all config values to start with (like .zshrc files do) and the user uncomments them if they want to change them, that way we can actually update the defaults and it will apply to all users.
This PR doesn't modify The motivation for this PR is to update constants based on the decrease of block time from 12 to 6 seconds (#3959 (comment)). Specifically, prior to this change, the mempool would evict transactions that are either 1m 15 seconds old or 6 blocks (i.e. 6 * 11.77 seconds = 70.62 seconds) old. After the block time reduction, transactions will be evicted from the mempool after 6 * 6 seconds = 36 seconds. If we want to retain existing mempool behavior, we should bump the |
| cfg.Mempool.TTLDuration = 75 * time.Second | ||
| cfg.Mempool.MaxTxBytes = 7_897_088 | ||
| cfg.Mempool.MaxTxsBytes = 39_485_440 |
There was a problem hiding this comment.
Can we have an issue to bump this value past 8MB
We could. I supposed that's a breaking change to end-users but we can consider removing the TTL num blocks feature. |
Motivation in the Mempool TTL duration section of #3959 (comment)
Description
Modify the default consensus node config for mempools so that:
MaxTxBytes from approx 7.5 MiB to 8 MiB because it's easier to understand and communicateMaxTxsBytes from approx 37.6 MiB to 40 MiB because it's easier to understand and communicateTesting
make install && ./scripts/single-node.shthen cat the config