feat(l1): enforce per-transaction wire-size cap in mempool admission#6599
Conversation
Adds the peer-policy size cap that every major Ethereum execution client
(geth `txMaxSize`, reth `DEFAULT_MAX_TX_INPUT_BYTES`, nethermind
`MaxTxSize` / `MaxBlobTxSize`, erigon size enforcement) ships by
default. Without it a single oversized transaction can chew bandwidth
and pool capacity at near-zero attacker cost.
- New constants `MAX_TX_SIZE = 128 KiB` and `MAX_BLOB_TX_SIZE = 1 MiB`
in `crates/common/types/constants.rs`.
- `Blockchain::validate_transaction` now rejects transactions whose
`Transaction`-canonical RLP encoding exceeds the cap. For EIP-4844 the
cap applies to the core tx only — the blob sidecar lives separately
in `BlobsBundle` and is already bounded by per-blob byte count plus
per-block blob count.
- New error `MempoolError::TxSizeExceeded { actual, limit }`.
- Two unit tests in `transaction.rs` confirming the wire-size signal
matches the cap.
ilitteri
requested review from
a team,
ManuelBilbao and
avilagaston9
as code owners
🤖 Kimi Code ReviewThe PR correctly implements mempool transaction size limits matching Geth/Reth/Nethermind defaults (128 KiB for standard txs, 1 MiB for EIP-4844). This is a peer policy limit, not consensus, and the implementation correctly excludes the blob sidecar for 4844 transactions. Issues and Suggestions: 1. Performance: Unnecessary allocation in hot path let encoded_len = tx.encode_canonical_to_vec().len();Allocating a 2. Missing test coverage for EIP-4844 limit 3. Minor: Test boundary condition Security/Consensus Notes:
Nit: Overall: Correct and safe, but optimize the allocation before merging if this is a hot path. Automated review by Kimi (Moonshot AI) · kimi-k2.5 · custom prompt |
🤖 Claude Code ReviewNow I have all the context needed. Here is the complete review: Code Review: feat(l1): enforce per-transaction wire-size cap in mempool admissionOverviewAdds peer-policy admission guards for oversized transactions: 128 KiB for standard transactions and 1 MiB for EIP-4844 blob transactions. These match the limits in geth, reth, and nethermind. The implementation is small, well-motivated, and the PR description is excellent. CorrectnessSize check placed after async I/O — minor ordering issue In if matches!(tx, &Transaction::PrivilegedL2Transaction(_)) {
return Ok(None);
}
// Size cap — fail fast before any async I/O
let encoded_len = tx.encode_canonical_to_vec().len();
// ...Overlapping check with
Boundary semantics are correct
Test CoverageThe two unit tests in
Design / StyleConstant placement inconsistency
Inline comment length The 4-line comment block at // Peer-policy wire cap (see MAX_TX_SIZE / MAX_BLOB_TX_SIZE in common/types/constants.rs)SummaryThe core logic is correct and the motivation is well-founded. The two actionable points worth addressing before merge:
Everything else is minor style feedback. Automated review by Claude (Anthropic) · sonnet · custom prompt |
Lines of code reportTotal lines added: Detailed view |
Greptile SummaryThis PR enforces a per-transaction wire-size cap at mempool admission — 128 KiB for standard transactions and 1 MiB for EIP-4844 blob transactions — matching the peer-policy defaults shipped by geth, reth, and nethermind. The check uses the canonical RLP encoding length and runs before the existing init-code and data-size guards.
Confidence Score: 4/5Safe to merge — no transaction is incorrectly admitted or rejected; the only behavioral change is that TxMaxDataSizeError is silently superseded by TxSizeExceeded for large-calldata non-creation txs The core admission logic is correct and well-commented. The TxMaxDataSizeError arm in blockchain.rs becomes dead code because the new wire-size guard uses the same threshold and runs first with encoding overhead always pushing the total above the limit. Test coverage covers only the EIP-1559 path; the blob-tx branch with MAX_BLOB_TX_SIZE has no corresponding test, leaving the larger 1 MiB cap unverified by any unit test. The ordering of guards in crates/blockchain/blockchain.rs around lines 2447–2470 deserves a second look for the interaction between TxSizeExceeded and the now-shadowed TxMaxDataSizeError
|
| Filename | Overview |
|---|---|
| crates/blockchain/blockchain.rs | Adds wire-size cap check before existing init-code/data-size checks; TxMaxDataSizeError is now unreachable for non-contract-creation txs since the new guard fires first at the same threshold |
| crates/blockchain/error.rs | Adds TxSizeExceeded { actual, limit } variant with a human-readable error message; integrates cleanly with the existing From for RpcErr catch-all |
| crates/common/types/constants.rs | Adds MAX_TX_SIZE (128 KiB) and MAX_BLOB_TX_SIZE (1 MiB) constants with accurate doc comments matching geth/reth/nethermind defaults |
| crates/common/types/transaction.rs | Adds two unit tests verifying the wire-size signal for EIP-1559 txs; blob tx (EIP-4844) coverage for MAX_BLOB_TX_SIZE is absent |
Flowchart
%%{init: {'theme': 'neutral'}}%%
flowchart TD
A[validate_transaction called] --> B{PrivilegedL2Transaction?}
B -- yes --> C[return Ok - bypass all checks]
B -- no --> D[encode_canonical_to_vec]
D --> E{EIP-4844 tx?}
E -- yes --> F[limit = MAX_BLOB_TX_SIZE 1 MiB]
E -- no --> G[limit = MAX_TX_SIZE 128 KiB]
F --> H{encoded_len > limit?}
G --> H
H -- yes --> I[Err TxSizeExceeded]
H -- no --> J{Contract creation AND Shanghai?}
J -- yes --> K{data > max_initcode_size?}
K -- yes --> L[Err TxMaxInitCodeSizeError]
K -- no --> M{Non-creation AND data >= 128 KiB?}
J -- no --> M
M -- yes --> N[Err TxMaxDataSizeError - now unreachable for non-creation txs]
M -- no --> O[Continue remaining checks...]
Comments Outside Diff (1)
-
crates/blockchain/blockchain.rs, line 2468-2470 (link)TxMaxDataSizeErrorbecomes unreachable for non-contract-creation txsThe new wire-size check (line 2447) runs before this data-length check and uses the same 128 KiB threshold. Because
encode_canonical_to_vec()always emits at least the type byte plus RLP framing in addition to the rawdatabytes, any tx whosedata.len() >= 131_072will already have an encoded size> MAX_TX_SIZE(131_072), so the earlier guard fires first. TheTxMaxDataSizeErrorarm is now dead code for regular call transactions; callers that pattern-match on that variant for non-creation txs will instead seeTxSizeExceeded.Prompt To Fix With AI
This is a comment left during a code review. Path: crates/blockchain/blockchain.rs Line: 2468-2470 Comment: **`TxMaxDataSizeError` becomes unreachable for non-contract-creation txs** The new wire-size check (line 2447) runs before this data-length check and uses the same 128 KiB threshold. Because `encode_canonical_to_vec()` always emits at least the type byte plus RLP framing in addition to the raw `data` bytes, any tx whose `data.len() >= 131_072` will already have an encoded size `> MAX_TX_SIZE` (131_072), so the earlier guard fires first. The `TxMaxDataSizeError` arm is now dead code for regular call transactions; callers that pattern-match on that variant for non-creation txs will instead see `TxSizeExceeded`. How can I resolve this? If you propose a fix, please make it concise.
Prompt To Fix All With AI
Fix the following 2 code review issues. Work through them one at a time, proposing concise fixes.
---
### Issue 1 of 2
crates/blockchain/blockchain.rs:2468-2470
**`TxMaxDataSizeError` becomes unreachable for non-contract-creation txs**
The new wire-size check (line 2447) runs before this data-length check and uses the same 128 KiB threshold. Because `encode_canonical_to_vec()` always emits at least the type byte plus RLP framing in addition to the raw `data` bytes, any tx whose `data.len() >= 131_072` will already have an encoded size `> MAX_TX_SIZE` (131_072), so the earlier guard fires first. The `TxMaxDataSizeError` arm is now dead code for regular call transactions; callers that pattern-match on that variant for non-creation txs will instead see `TxSizeExceeded`.
### Issue 2 of 2
crates/common/types/transaction.rs:3786-3793
**No test for the `MAX_BLOB_TX_SIZE` cap**
Both new tests exercise `MAX_TX_SIZE` via EIP-1559 transactions, but there is no corresponding test that constructs an `EIP4844Transaction` with an encoded size just over `MAX_BLOB_TX_SIZE` (1 MiB) and verifies the signal fires. The blob tx branch in `validate_transaction` is the riskier path (1 MiB limit vs 128 KiB), so a parallel "exceeds / below" pair for blob txs would meaningfully close the coverage gap.
Reviews (1): Last reviewed commit: "feat(l1): enforce per-transaction wire-s..." | Re-trigger Greptile
![greptile-apps[bot]](https://avatars.githubusercontent.com/in/867647?s=60&v=4){kind=small}
| #[test] | ||
| fn test_encoded_size_below_max_tx_size() { | ||
| // A small tx encodes well below the cap. | ||
| use crate::types::MAX_TX_SIZE; | ||
|
|
||
| let tx = Transaction::EIP1559Transaction(EIP1559Transaction::default()); | ||
| assert!(tx.encode_canonical_to_vec().len() <= MAX_TX_SIZE); | ||
| } |
There was a problem hiding this comment.
MAX_BLOB_TX_SIZE cap
Both new tests exercise MAX_TX_SIZE via EIP-1559 transactions, but there is no corresponding test that constructs an EIP4844Transaction with an encoded size just over MAX_BLOB_TX_SIZE (1 MiB) and verifies the signal fires. The blob tx branch in validate_transaction is the riskier path (1 MiB limit vs 128 KiB), so a parallel "exceeds / below" pair for blob txs would meaningfully close the coverage gap.
Prompt To Fix With AI
This is a comment left during a code review.
Path: crates/common/types/transaction.rs
Line: 3786-3793
Comment:
**No test for the `MAX_BLOB_TX_SIZE` cap**
Both new tests exercise `MAX_TX_SIZE` via EIP-1559 transactions, but there is no corresponding test that constructs an `EIP4844Transaction` with an encoded size just over `MAX_BLOB_TX_SIZE` (1 MiB) and verifies the signal fires. The blob tx branch in `validate_transaction` is the riskier path (1 MiB limit vs 128 KiB), so a parallel "exceeds / below" pair for blob txs would meaningfully close the coverage gap.
How can I resolve this? If you propose a fix, please make it concise.There was a problem hiding this comment.
Pull request overview
This PR adds a per-transaction wire-size admission cap to the L1 mempool, aligning ethrex’s peer-policy behavior with other execution clients to reduce bandwidth/mempool DoS risk from oversized transactions.
Changes:
- Introduces
MAX_TX_SIZE(128 KiB) andMAX_BLOB_TX_SIZE(1 MiB) as mempool admission size caps. - Adds a canonical-encoding size check in
Blockchain::validate_transaction, returning a newMempoolError::TxSizeExceeded { actual, limit }when exceeded. - Adds unit tests ensuring
Transaction::encode_canonical_to_vec()size behavior matches expectations around the cap.
Reviewed changes
Copilot reviewed 4 out of 4 changed files in this pull request and generated 1 comment.
| File | Description |
|---|---|
| crates/common/types/transaction.rs | Adds unit tests validating canonical encoding size vs MAX_TX_SIZE. |
| crates/common/types/constants.rs | Defines mempool admission size cap constants for non-blob and blob core transactions. |
| crates/blockchain/error.rs | Adds MempoolError::TxSizeExceeded { actual, limit } for size-cap rejections. |
| crates/blockchain/blockchain.rs | Enforces the per-transaction canonical wire-size cap during mempool validation. |
💡 Add Copilot custom instructions for smarter, more guided reviews. Learn how to get started.
| // nethermind `MaxTxSize` / `MaxBlobTxSize`. For EIP-4844 the canonical | ||
| // encoding of `Transaction` excludes the sidecar (which lives in the | ||
| // adjacent `BlobsBundle`), so this caps the core tx only. | ||
| let encoded_len = tx.encode_canonical_to_vec().len(); |
🤖 Codex Code Review
I did not see consensus, EVM opcode, or gas-accounting regressions in the diff beyond those mempool-admission concerns. I could not run targeted Automated review by OpenAI Codex · gpt-5.4 · custom prompt |
MegaRedHand
left a comment
MegaRedHand
left a comment
There was a problem hiding this comment.
LGTM. I think we can remove the unit tests
| // nethermind `MaxTxSize` / `MaxBlobTxSize`. For EIP-4844 the canonical | ||
| // encoding of `Transaction` excludes the sidecar (which lives in the | ||
| // adjacent `BlobsBundle`), so this caps the core tx only. | ||
| let encoded_len = tx.encode_canonical_to_vec().len(); |
There was a problem hiding this comment.
We might want to find another way to compute this.
…egration tests
Two changes from review:
1. Drop the now-redundant calldata-only size check
`MAX_TRANSACTION_DATA_SIZE = 128 KiB` (in `crates/blockchain/constants.rs`)
was byte-identical to the new `MAX_TX_SIZE`, and the new wire-size
check at the top of `validate_transaction` runs first and rejects
for all the same inputs (the encoded envelope is strictly larger
than `data().len()`). Delete the constant, the
`MempoolError::TxMaxDataSizeError` variant, and the dead check
block. No behavior change.
2. Replace the two tautological encoding tests in `transaction.rs`
(still landing — they only asserted that RLP doesn't compress) with
real integration tests in `test/tests/blockchain/mempool_tests.rs`
that exercise `validate_transaction` directly:
- `validate_transaction_rejects_oversize_non_blob` against
`MAX_TX_SIZE`
- `validate_transaction_rejects_oversize_blob_core` against
`MAX_BLOB_TX_SIZE` for an EIP-4844 tx
Both assert the `TxSizeExceeded { actual, limit }` shape so a
regression in the limit dispatch (non-blob vs blob) is also caught.
Cross-client audit flagged that geth (`core/types/transaction.go::Size`), nethermind (`MaxBlobTxSize`), and erigon (`ValidateSerializedTxn`) all compare their 1 MiB blob-tx cap against the wire form that **includes the sidecar** (blobs + commitments + proofs). The previous ethrex check in `validate_transaction` compared `Transaction::encode_canonical_to_vec` which only covers the core tx — the sidecar lives in the adjacent `BlobsBundle`. With 6 blobs (~786 KB blob data + ~100 KB commitments/proofs ≈ 900 KB) the worst-case wire wrapper can reach ~1.9 MiB while still passing the 1 MiB core-only check. Peers reject that on the wire so ethrex would be admitting txs nobody else will relay. Changes: - `validate_transaction` now only enforces `MAX_TX_SIZE` for non-blob txs. - `add_blob_transaction_to_pool_inner` runs a new wire-wrapper check before bundle validation: `core_tx_encoded + bundle_encoded <= MAX_BLOB_TX_SIZE`. Summing the two encoded sizes matches geth's `tx.Size()` semantic to within the ±few bytes of outer list framing, which is rounding error at this scale. - Drop `validate_transaction_rejects_oversize_blob_core` — the function it covered no longer applies to blob txs. Integration test for the new wrapper check deferred (same pattern as PRs #6603/#6576: the `c-kzg`-gated `add_blob_transaction_to_pool` isn't currently exercised by `mempool_tests`).
…ust to measure Both reviewers (Copilot + @MegaRedHand) flagged that `tx.encode_canonical_to_vec().len()` allocates a `Vec` purely to read its length on the mempool-admission hot path, and for typed txs also clones the cached canonical bytes. `encode_canonical_len()` counts the 1-byte EIP-2718 type prefix plus the inner tx's RLP `length()` (which `RLPEncode::length` provides allocation-free via the existing `ByteCounter`). The two admission size-check call sites (non-blob in `validate_transaction`, blob wire-wrapper in `add_blob_transaction_to_pool_inner`) now use this helper; the blob path also switches `BlobsBundle::encode_to_vec().len()` to `BlobsBundle::length()` for the same reason.
Per @MegaRedHand's review (approved with note: "I think we can remove the unit tests"). The two tests in `transaction.rs` (`test_encoded_size_ exceeds_max_tx_size` and `test_encoded_size_below_max_tx_size`) only re-derived the wire-size signal; the admission gate itself is covered by `validate_transaction_rejects_oversize_non_blob` in the integration test suite.
Codex + Claude review both flagged that the wire-size gate runs after secp256k1 sender recovery in `add_transaction_to_pool`. For a tx that fails the size check, the work was wasted; for a tx that passes, this is just ordering. Matches geth's order: `ValidateTransaction` runs the size check before any crypto. `add_transaction_to_pool` now applies the cap right after the `BlobTxNoBlobsBundle` rejection, before `contains_tx` and sender recovery. The same check remains inside `validate_transaction` so direct callers (integration tests, L2 paths) keep the guarantee unchanged. The blob wrapper-size check is already first in `add_blob_transaction_to_pool_inner` from a prior commit.
|
@MegaRedHand 3ee3f72 — removed the two encode-size sanity tests per your review. |
|
To use Codex here, create an environment for this repo. |
Motivation
Every major Ethereum execution client (geth
txMaxSize, rethDEFAULT_MAX_TX_INPUT_BYTES, nethermindMaxTxSize/MaxBlobTxSize, erigon size enforcement) ships a per-transaction wire-size cap at admission. Without one a single oversized transaction can chew bandwidth and pool capacity at near-zero attacker cost.Description
MAX_TX_SIZE = 128 KiBandMAX_BLOB_TX_SIZE = 1 MiBincrates/common/types/constants.rs.Blockchain::validate_transactionenforcesMAX_TX_SIZEon the canonical RLP encoding of non-blob transactions.Blockchain::add_blob_transaction_to_poolenforcesMAX_BLOB_TX_SIZEon the wire wrapper —Transaction::encode_canonical_to_vec().len() + BlobsBundle::encode_to_vec().len()— to match geth (tx.Size()includes the attached sidecar), nethermind (MaxBlobTxSizeis checked on the wrapper form), and erigon (ValidateSerializedTxnworks on the raw wire bytes which for blob txs is the wrapper-with-sidecar). The two encoded sizes are summed because ethrex stores the core tx and the bundle in separate structs; the ±few bytes of outer list framing are rounding error at the 1 MiB scale.MempoolError::TxSizeExceeded { actual, limit }.MAX_TRANSACTION_DATA_SIZEcalldata-only check fromvalidate_transaction— the new encoded-size cap is strictly tighter (a 128 KiB calldata payload always encodes to > 128 KiB).mempool_tests.rscovering the non-blob path. Integration test for the wrapper check deferred (thec-kzg-gatedadd_blob_transaction_to_poolisn't currently exercised bymempool_tests, same pattern as PRs feat(l1): cap mempool pending transactions per sender (default 16) #6603/feat(l1): add --mempool.private flag for non-propagating local txs #6576).