test: add txpool config upstream regression tests - #266
Open
NikhilSharmaWe wants to merge 3 commits into
Open
Conversation
Signed-off-by: Nikhil Sharma <nikhilsharma230303@gmail.com>
|
No actionable comments were generated in the recent review. 🎉 ℹ️ Recent review info⚙️ Run configurationConfiguration used: defaults Review profile: CHILL Plan: Pro Run ID: 📒 Files selected for processing (2)
🚧 Files skipped from review as they are similar to previous changes (2)
📝 WalkthroughWalkthroughAdds Berachain txpool defaults, initializes them during startup, routes pool EIP-7594 handling through config, and adds regression tests for pinned and inherited txpool arguments. ChangesTxpool defaults and validation pinning
Estimated code review effort: 3 (Moderate) | ~25 minutes Sequence Diagram(s)sequenceDiagram
participant main_fn as "main()"
participant defaults_fn as "berachain_txpool_defaults()"
participant engine as "DefaultEngineValues"
participant build_fn as "build_pool()"
participant config as "pool::config"
main_fn->>defaults_fn: create txpool defaults
defaults_fn-->>main_fn: DefaultTxPoolValues
main_fn->>engine: try_init(DefaultTxPoolValues) before CLI parsing
build_fn->>config: set_eip7594(BERACHAIN_ACCEPTS_EIP7594)
Possibly related PRs
Suggested reviewers: 🚥 Pre-merge checks | ✅ 5✅ Passed checks (5 passed)
✨ Finishing Touches🧪 Generate unit tests (beta)
Thanks for using CodeRabbit! It's free for OSS, and your support helps us grow. If you like it, consider giving us a shout-out. Comment |
Contributor
There was a problem hiding this comment.
Pull request overview
Adds txpool configuration regression coverage and wires Berachain-specific txpool defaults into the node startup path, so upstream Reth default changes surface as test failures that require an explicit adopt/override decision.
Changes:
- Add integration tests that validate the
berachain_txpool_defaults().try_init()→TxPoolArgs::default()path and track upstream drift. - Introduce
src/pool/config.rswith pinned txpool limits plus assertions for inherited/unpinned defaults. - Replace hardcoded
.no_eip7594()with a policy constant (BERACHAIN_ACCEPTS_EIP7594) when building the txpool validator.
Reviewed changes
Copilot reviewed 5 out of 5 changed files in this pull request and generated 2 comments.
Show a summary per file
| File | Description |
|---|---|
| tests/txpool_defaults.rs | Integration test ensuring txpool defaults are initialized before TxPoolArgs::default() and match pinned + inherited expectations. |
| tests/reth_txpool_upstream.rs | Integration test validating an inherited default (max_batch_size) against live upstream TxPoolArgs::default(). |
| src/pool/mod.rs | Uses a single policy constant to control EIP-7594 acceptance when building the txpool validator. |
| src/pool/config.rs | Defines pinned Berachain txpool defaults and regression assertions vs upstream constants. |
| src/main.rs | Initializes Berachain txpool defaults via try_init() before CLI parsing. |
💡 Add Copilot custom instructions for smarter, more guided reviews. Learn how to get started.
Comment on lines
+23
to
+26
| /// Inherited from reth v1.11.4 (`DefaultTxPoolValues::default().max_batch_size`). | ||
| /// No upstream named constant; validated live in `tests/txpool_defaults.rs`. | ||
| #[doc(hidden)] | ||
| pub const INHERITED_TXPOOL_MAX_BATCH_SIZE: usize = 1; |
Comment on lines
+17
to
+18
| /// Inherited from reth v1.11.4 (`MIN_PROTOCOL_BASE_FEE`). Berachain chain min is 1 gwei. | ||
| pub const MINIMAL_PROTOCOL_BASEFEE: u64 = 7; |
![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
🧹 Nitpick comments (1)
tests/reth_txpool_upstream.rs (1)
1-12: 📐 Maintainability & Code Quality | 🔵 Trivial | 💤 Low valueTest correctness depends on this remaining the only test in the binary.
The module doc correctly notes that
TxPoolArgs::default()must observe upstream values before anytry_initruns. Since this isolation is enforced only by a comment, a future contributor adding a second test (or one that triggerstry_init) into this binary could silently invalidate the assertion. Consider asserting the invariant defensively or keeping a prominent marker so the constraint isn't lost.🤖 Prompt for AI Agents
Verify each finding against current code. Fix only still-valid issues, skip the rest with a brief reason, keep changes minimal, and validate. In `@tests/reth_txpool_upstream.rs` around lines 1 - 12, The test binary currently relies on a comment-only invariant that `inherited_max_batch_size_matches_reth_upstream` remains the only test so `TxPoolArgs::default()` is checked before any `try_init` runs. Add a defensive guard in this test module to enforce that isolation explicitly, or introduce a clear marker/structure around `inherited_max_batch_size_matches_reth_upstream` so future additions that could trigger `try_init` are prevented from silently breaking the assertion.
🤖 Prompt for all review comments with AI agents
Verify each finding against current code. Fix only still-valid issues, skip the
rest with a brief reason, keep changes minimal, and validate.
Inline comments:
In `@src/pool/config.rs`:
- Around line 17-18: Confirm whether MINIMAL_PROTOCOL_BASEFEE in
src/pool/config.rs is intentionally set to 7 wei despite the Berachain chain
minimum being 1 gwei, and clarify the surrounding doc comment accordingly. If
the 7-wei inherited value is deliberate, update the comment near
MINIMAL_PROTOCOL_BASEFEE to explain that it is the txpool’s protocol floor from
Reth and separate it from the chain/block minimum; if not, adjust the constant
to match the intended pool policy. Refer to MINIMAL_PROTOCOL_BASEFEE and the
comment text so the distinction is explicit.
---
Nitpick comments:
In `@tests/reth_txpool_upstream.rs`:
- Around line 1-12: The test binary currently relies on a comment-only invariant
that `inherited_max_batch_size_matches_reth_upstream` remains the only test so
`TxPoolArgs::default()` is checked before any `try_init` runs. Add a defensive
guard in this test module to enforce that isolation explicitly, or introduce a
clear marker/structure around `inherited_max_batch_size_matches_reth_upstream`
so future additions that could trigger `try_init` are prevented from silently
breaking the assertion.
🪄 Autofix (Beta)
Fix all unresolved CodeRabbit comments on this PR:
- Push a commit to this branch (recommended)
- Create a new PR with the fixes
ℹ️ Review info
⚙️ Run configuration
Configuration used: defaults
Review profile: CHILL
Plan: Pro
Run ID: 87f5c386-036b-4373-90ab-ba60ed88b8bf
📒 Files selected for processing (5)
src/main.rssrc/pool/config.rssrc/pool/mod.rstests/reth_txpool_upstream.rstests/txpool_defaults.rs
Signed-off-by: Nikhil Sharma <nikhilsharma230303@gmail.com>
Signed-off-by: Nikhil Sharma <nikhilsharma230303@gmail.com>
Author
|
@calbera PTAL, thanks |
This file contains hidden or bidirectional Unicode text that may be interpreted or compiled differently than what appears below. To review, open the file in an editor that reveals hidden Unicode characters.
Learn more about bidirectional Unicode characters
Sign up for free
to join this conversation on GitHub.
Already have an account?
Sign in to comment
Add this suggestion to a batch that can be applied as a single commit.This suggestion is invalid because no changes were made to the code.Suggestions cannot be applied while the pull request is closed.Suggestions cannot be applied while viewing a subset of changes.Only one suggestion per line can be applied in a batch.Add this suggestion to a batch that can be applied as a single commit.Applying suggestions on deleted lines is not supported.You must change the existing code in this line in order to create a valid suggestion.Outdated suggestions cannot be applied.This suggestion has been applied or marked resolved.Suggestions cannot be applied from pending reviews.Suggestions cannot be applied on multi-line comments.Suggestions cannot be applied while the pull request is queued to merge.Suggestion cannot be applied right now. Please check back later.
Summary
Adds upstream regression tests for txpool configuration (closes #88).
Pinned limits - subpool sizes, account slots, price bumps, gas limit, max tx input bytes, and queued lifetime are defined in
src/pool/config.rsand compared against Reth v1.11.4 exported constants. Tests fail on dependency bumps so we explicitly choose to adopt or override.Unpinned inherited fields - validation tasks, buffer sizes, local tx flags, and backup settings are still inherited from Reth; separate tests catch drift there too.
Runtime wiring -
main.rscallsberachain_txpool_defaults().try_init()before CLI parsing (same pattern as engine defaults). Integration tests cover thetry_init->TxPoolArgs::default()path and live upstreammax_batch_size.Pool builder -
BERACHAIN_ACCEPTS_EIP7594replaces hardcoded.no_eip7594()(BRIP-0010 policy).Test plan
cargo test --lib pool::configcargo test --test txpool_defaultscargo test --test reth_txpool_upstreamSummary by CodeRabbit
New Features
Bug Fixes
Tests