fix(txpool): enforce EIP-3860 max initcode size in mempool#120
Open
panos-xyz wants to merge 2 commits into
Open
fix(txpool): enforce EIP-3860 max initcode size in mempool#120panos-xyz wants to merge 2 commits into
panos-xyz wants to merge 2 commits into
Conversation
morph-mainnet/hoodi genesis use morph-geth's non-standard `shanghaiBlock` field (inherited from scroll-tech's go-ethereum), which alloy's `Genesis` parser ignores in favour of the upstream `shanghaiTime` field. As a result, `is_shanghai_active_at_timestamp` returns false on `MorphChainSpec`, and reth's `EthTransactionValidator` silently skips the EIP-3860 max initcode size check (`fork_tracker.is_shanghai_activated()` gate at `reth_transaction_pool::validate::eth.rs:468`). This let oversized contract-creation transactions (>49 152 B initcode) into the mempool. They never landed on chain — morph-geth's tx pool rejects them at the same gate (`core/tx_pool.go:695`) — but they sat forever in pending, polluting the pool and misleading clients which treated the returned hash as success. Reproduced as item X1 in QA's boundary-fuzz pass: geth: -32000 "max initcode size exceeded: code size 50000 limit 49152" reth: 0x... (accepted, never on-chain) Force-activating Shanghai in the chainspec would flip `is_shanghai_active_at_timestamp` everywhere, in particular causing `EthStorage::read_block_bodies` to start filling `body.withdrawals` with `Some(vec![])` after Shanghai activation, leaking a `"withdrawals": []` field into RPC responses (morph-geth has no withdrawals slot on its `Body`, so this would diverge from morph-geth byte-for-byte). Instead, enforce the bound directly in `MorphTransactionValidator`, mirroring how morph already manages `MorphHardfork -> SpecId` outside reth's standard fork path. This keeps the chainspec untouched, the RPC output bit-identical to morph-geth, and the EIP-3860 size cap correctly enforced. The intrinsic-gas surcharge for initcode (also EIP-3860) is already covered: morph registers `Prague` at Viridian time, so reth's `ensure_intrinsic_gas` falls into the Prague branch (PRAGUE > SHANGHAI), which includes `initcode_cost`. Tests: - `validate_rejects_oversized_initcode`: 49 153 B initcode is rejected - `validate_accepts_initcode_at_limit`: 49 152 B initcode still admitted
![claude[bot]](https://avatars.githubusercontent.com/in/1236702?s=60&v=4){kind=small}
There was a problem hiding this comment.
Code review is billed via overage credits. To resume reviews, an organization admin can raise the monthly limit at claude.ai/admin-settings/claude-code.
Once credits are available, reopen this pull request to trigger a review.
📝 WalkthroughWalkthroughThis PR adds unconditional EIP-3860 initcode-size enforcement to the Morph txpool transaction validator. It introduces a 49,152-byte limit constant, enforces the limit as an early-return check during transaction admission, and includes unit tests verifying rejection above the limit and non-rejection at exactly the limit. ChangesEIP-3860 initcode-size enforcement at txpool layer
Estimated code review effort🎯 3 (Moderate) | ⏱️ ~20 minutes Possibly related PRs
Suggested reviewers
Poem
🚥 Pre-merge checks | ✅ 5✅ Passed checks (5 passed)
✏️ Tip: You can configure your own custom pre-merge checks in the settings. ✨ Finishing Touches📝 Generate docstrings
🧪 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 |
![coderabbitai[bot]](https://avatars.githubusercontent.com/in/347564?s=60&v=4){kind=small}
There was a problem hiding this comment.
Actionable comments posted: 2
🤖 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 `@crates/txpool/src/validator.rs`:
- Around line 869-875: The test currently only asserts on
TransactionValidationOutcome::Invalid but ignores
TransactionValidationOutcome::Error, so update the validation to handle both
variants: when outcome matches TransactionValidationOutcome::Invalid(_, err) or
TransactionValidationOutcome::Error(err) extract err.to_string() and assert that
it does not contain "max_init_code_size" or "init code size" (failing the
boundary test on Error outcomes as well). Locate the match/if-let that
references outcome and TransactionValidationOutcome::Invalid and extend it to
also check the TransactionValidationOutcome::Error variant, producing the same
failure message including the err string.
- Around line 775-783: Tests rely on inner validator fork defaults so they can
pass due to Shanghai/Cancun being active; update the tests that assert initcode
size behavior to explicitly disable Shanghai/Cancun in the inner validator
configuration (so MorphTransactionValidator’s own initcode check is the sole
enforcer). Locate the test functions around the #[test] blocks tied to
MorphTransactionValidator and create or inject an inner EthTransactionValidator
or fork config with Shanghai/Cancun deactivated (e.g., set activation timestamps
to None or pre-Shanghai), ensuring the assertions no longer depend on
is_shanghai_active_at_timestamp or chainspec defaults.
🪄 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: f677df3b-ff5a-4f92-bccc-8e0322f10dcb
📒 Files selected for processing (1)
crates/txpool/src/validator.rs
Comment on lines
+775
to
+783
| /// EIP-3860 mempool admission: contract creation transactions whose initcode | ||
| /// exceeds 49 152 bytes must be rejected. This used to slip through because | ||
| /// morph-mainnet/hoodi genesis use the non-standard `shanghaiBlock` field | ||
| /// (inherited from scroll-tech go-ethereum) that alloy's `Genesis` parser | ||
| /// ignores, leaving Shanghai un-registered in the hardforks table and | ||
| /// `is_shanghai_active_at_timestamp` permanently `false`. The chainspec | ||
| /// now force-activates Shanghai/Cancun at `Timestamp(0)` so reth's | ||
| /// `EthTransactionValidator` triggers the EIP-3860 size check. | ||
| #[test] |
There was a problem hiding this comment.
Isolate the regression tests from inner Shanghai-gated checks.
These tests currently rely on default inner-validator fork behavior (and comments suggest chainspec-based enforcement). That can let tests pass for the wrong reason if inner fork config changes. Explicitly disable Shanghai/Cancun here so the assertions stay pinned to MorphTransactionValidator’s explicit initcode check.
Suggested patch
- /// `is_shanghai_active_at_timestamp` permanently `false`. The chainspec
- /// now force-activates Shanghai/Cancun at `Timestamp(0)` so reth's
- /// `EthTransactionValidator` triggers the EIP-3860 size check.
+ /// `is_shanghai_active_at_timestamp` permanently `false`.
+ /// Keep inner Shanghai/Cancun checks disabled in this test so rejection
+ /// is proven to come from MorphTransactionValidator's explicit guard.
@@
- // Note: do NOT call `.no_shanghai()` here. The whole point is to verify
- // that the chainspec correctly activates Shanghai so the inner
- // `EthTransactionValidator` enforces EIP-3860.
+ // Disable Shanghai/Cancun on the inner validator so this test validates
+ // MorphTransactionValidator's explicit EIP-3860 guard path.
let eth_validator: EthTransactionValidator<
_,
crate::MorphPooledTransaction,
MorphEvmConfig,
> = EthTransactionValidatorBuilder::new(client, morph_evm_config)
+ .no_shanghai()
+ .no_cancun()
.disable_balance_check()
.build::<crate::MorphPooledTransaction, _>(InMemoryBlobStore::default());
@@
let eth_validator: EthTransactionValidator<
_,
crate::MorphPooledTransaction,
MorphEvmConfig,
> = EthTransactionValidatorBuilder::new(client, morph_evm_config)
+ .no_shanghai()
+ .no_cancun()
.disable_balance_check()
.build::<crate::MorphPooledTransaction, _>(InMemoryBlobStore::default());Also applies to: 790-799, 840-846
🤖 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 `@crates/txpool/src/validator.rs` around lines 775 - 783, Tests rely on inner
validator fork defaults so they can pass due to Shanghai/Cancun being active;
update the tests that assert initcode size behavior to explicitly disable
Shanghai/Cancun in the inner validator configuration (so
MorphTransactionValidator’s own initcode check is the sole enforcer). Locate the
test functions around the #[test] blocks tied to MorphTransactionValidator and
create or inject an inner EthTransactionValidator or fork config with
Shanghai/Cancun deactivated (e.g., set activation timestamps to None or
pre-Shanghai), ensuring the assertions no longer depend on
is_shanghai_active_at_timestamp or chainspec defaults.
Comment on lines
+869
to
+875
| if let TransactionValidationOutcome::Invalid(_, err) = &outcome { | ||
| let msg = err.to_string(); | ||
| assert!( | ||
| !msg.contains("max_init_code_size") && !msg.contains("init code size"), | ||
| "EIP-3860 must not reject initcode of exactly the size limit; got: {msg}" | ||
| ); | ||
| } |
There was a problem hiding this comment.
Fail the boundary test on Error outcomes.
Line 869 only checks the Invalid branch; an Error outcome currently passes silently. This can hide regressions in validation flow.
Suggested patch
- if let TransactionValidationOutcome::Invalid(_, err) = &outcome {
- let msg = err.to_string();
- assert!(
- !msg.contains("max_init_code_size") && !msg.contains("init code size"),
- "EIP-3860 must not reject initcode of exactly the size limit; got: {msg}"
- );
- }
+ match &outcome {
+ TransactionValidationOutcome::Invalid(_, err) => {
+ let msg = err.to_string();
+ assert!(
+ !msg.contains("max_init_code_size") && !msg.contains("init code size"),
+ "EIP-3860 must not reject initcode of exactly the size limit; got: {msg}"
+ );
+ }
+ TransactionValidationOutcome::Error(_, err) => {
+ panic!("unexpected validation error at initcode limit: {err:?}");
+ }
+ TransactionValidationOutcome::Valid { .. } => {}
+ }🤖 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 `@crates/txpool/src/validator.rs` around lines 869 - 875, The test currently
only asserts on TransactionValidationOutcome::Invalid but ignores
TransactionValidationOutcome::Error, so update the validation to handle both
variants: when outcome matches TransactionValidationOutcome::Invalid(_, err) or
TransactionValidationOutcome::Error(err) extract err.to_string() and assert that
it does not contain "max_init_code_size" or "init code size" (failing the
boundary test on Error outcomes as well). Locate the match/if-let that
references outcome and TransactionValidationOutcome::Invalid and extend it to
also check the TransactionValidationOutcome::Error variant, producing the same
failure message including the err string.
- Reorder the EIP-3860 size check to run after the L1-message / EIP-7702 / MorphTx type gates and before the inner validator, so a tx rejected purely on its type still surfaces its type error like morph-geth, while state lookups stay skipped for oversized payloads. - Reuse revm's canonical eip3860::MAX_INITCODE_SIZE constant (the same one reth's EthTransactionValidator uses) instead of a hand-copied 49_152 literal, and move it out of the use block. - Rewrite the test comments that wrongly claimed the fix force-activates Shanghai/Cancun in the chainspec; the chainspec is untouched and the rejection is enforced unconditionally by MorphTransactionValidator.
7 tasks
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
1 participant
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.
Closes #121
Summary
Reject contract-creation transactions whose initcode exceeds the EIP-3860 limit (49 152 B) at the txpool layer, fixing QA finding X1:
-32000 max initcode size exceeded: code size 50000 limit 49152Root cause
morph-mainnet / hoodi genesis use morph-geth's non-standard
shanghaiBlockfield (inherited from scroll-tech's go-ethereum). alloy'sGenesisparser only recognises the upstream timestamp-basedshanghaiTime, soMorphChainSpec.is_shanghai_active_at_timestampreturns false everywhere, and reth'sEthTransactionValidatorsilently skips the EIP-3860 size guard atreth_transaction_pool::validate::eth.rs:468.Why not just activate Shanghai in the chainspec?
Force-activating Shanghai/Cancun would flip
is_shanghai_active_at_timestampfor every consumer in reth, most notablyEthStorage::read_block_bodies, which would start fillingbody.withdrawalswithSome(vec![])after activation. morph-geth'sBodyhas no withdrawals slot, so RPC responses would diverge byte-for-byte. We would also need to overrideMorphPayloadAttributesBuilderto keep withdrawals/parent_beacon_block_root pinned toNone.Enforcing the bound directly in
MorphTransactionValidatorkeeps:This mirrors morph's existing pattern of managing fork semantics outside reth's standard fork path (e.g.
MorphHardfork -> SpecIdmapping incrates/chainspec/src/hardfork.rs).What about the EIP-3860 intrinsic-gas surcharge?
Also covered. morph registers
Pragueat Viridian time, so reth'sensure_intrinsic_gaslands onSpecId::PRAGUE(PRAGUE ⊃ SHANGHAI), which includesinitcode_cost. The only window where this would not hold is during cold-sync of pre-Viridian blocks on a fresh node — but mempool admission is not active during sync.Test plan
validate_rejects_oversized_initcode: 49 153 B initcode →ExceedsMaxInitCodeSizevalidate_accepts_initcode_at_limit: 49 152 B initcode is not blocked by EIP-3860got: Valid {...}(matching X1's reproducer)cargo nextest run --workspace— 656 passedcargo nextest run -p morph-node --features test-utils -E 'binary(it)'— 86 passedcargo clippy --all --all-targets -- -D warningscleancargo fmt --all -- --checkcleanSummary by CodeRabbit
Bug Fixes
Tests