feat(l1): cap mempool queued (future-nonce) txs per sender (geth AccountQueue-style)#6603
Conversation
Adds the peer-policy per-sender pending-slot cap that every major Ethereum execution client enforces by default — geth `AccountSlots`, reth `TXPOOL_MAX_ACCOUNT_SLOTS_PER_SENDER`, erigon `AccountSlots`, nethermind `MaxPendingBlobTxsPerSender`, all defaulting to 16. Without this cap a single sender can occupy 100% of the 10,000-tx pool, which trivially bypasses the size cap (#6599), EIP-3607 check (#6600) and percentage-RBF rule (#6601) by submitting many nonces from one address. - `BlockchainOptions` gains `account_slots: usize` (default 16) plus a new `DEFAULT_ACCOUNT_SLOTS` constant referencing the peer-client norms inline. - `Mempool::count_for_sender(sender)` counts a sender's pending txs by ranging the existing `txs_by_sender_nonce: BTreeMap<(H160, u64), H256>` index — O(log n + k) where k is the per-sender count (≤ 16). No new state, no new invariant. - `Blockchain::validate_transaction` consults the count AFTER `find_tx_to_replace`, so replacement candidates at an existing `(sender, nonce)` bypass the cap — they don't grow the pool footprint. Senders at the cap submitting a new nonce are rejected with `MempoolError::SenderSlotsExceeded { count, limit }`. - New CLI flag `--mempool.account-slots` (env `ETHREX_MEMPOOL_ACCOUNT_SLOTS`), plumbed through L1 and L2 initializers. - Five unit tests in `mempool.rs` covering: empty pool, single tx, multiple nonces from same sender, isolation between senders, unknown sender returns zero. The wire-up inside `validate_transaction` is a six-line conditional that's visually verifiable; a full end-to-end integration test would require seeding sender-account storage state, which isn't currently done in `test/tests/blockchain/mempool_tests.rs`. All cargo fmt / clippy / unit + integration test runs clean (441 tests in `ethrex-test`).
🤖 Kimi Code ReviewThe PR implements a per-account pending transaction limit in the mempool (default: 16) to prevent DoS spam. Overall, the implementation is correct and follows Rust best practices. Issues Found1. Race Condition (TOCTOU) in Account Limit CheckFile: The check acquires a read lock, drops it, and later // Current flow:
let count = self.mempool.count_for_sender(sender)?; // read lock dropped here
if count >= self.options.max_pending_txs_per_account {
return Err(...);
}
// ... gap where another tx could be added ...
self.mempool.add_transaction(...); // write lock acquired hereRecommendation: While acceptable for mempool best-effort limits (not consensus-critical), document this behavior or consider acquiring the write lock earlier if strict enforcement is required. 2. Magic Number DuplicationFiles:
The default value Recommendation: Import and use 3. Error Message AccuracyFile: #[error("Sender has {count} pending transactions, exceeds the per-account cap of {limit}")]When Recommendation: Change to: 4. Potential Performance NoteFile: The let count = inner
.txs_by_sender_nonce
.range((sender, 0)..=(sender, u64::MAX))
.count();Note: This is O(k) where k is the sender's current tx count. With default cap of 16, this is negligible, but monitor if operators increase the limit significantly. Positive Aspects
SummaryThe implementation is sound and secure for its intended purpose. Address the magic number duplication (Item 2) before merging if possible; the race condition (Item 1) is acceptable for mempool admission control but should be documented. Automated review by Kimi (Moonshot AI) · kimi-k2.5 · custom prompt |
Lines of code reportTotal lines added: Detailed view |
🤖 Claude Code ReviewNow I have everything I need to write a thorough review. Review: feat(l1): cap mempool pending transactions per sender (default 16)The design is sound and the implementation is clean. The per-sender slot cap correctly addresses a real DoS vector. Most of the core issues are already flagged by the automated review; I'll expand on the significant ones and add a couple new ones. Correctness
Replacement bypass logic ( Bug:
|
There was a problem hiding this comment.
Pull request overview
This PR introduces a per-sender cap on the number of pending transactions allowed in the mempool (default 16) to reduce single-sender pool domination and related spam vectors, while allowing same-nonce replacements to bypass the cap.
Changes:
- Add
BlockchainOptions::max_pending_txs_per_account(default 16) and enforce it during transaction admission (with replacement bypass). - Implement
Mempool::count_for_senderusing the existing(sender, nonce)index, plus unit tests for the counting behavior. - Expose configuration via CLI flag/env var and document it; plumb the option through both L1 and L2 initializers.
Reviewed changes
Copilot reviewed 7 out of 7 changed files in this pull request and generated 4 comments.
Show a summary per file
| File | Description |
|---|---|
| docs/CLI.md | Documents the new --mempool.max-pending-txs-per-account flag (default 16). |
| crates/blockchain/mempool.rs | Adds count_for_sender and unit tests for per-sender counting. |
| crates/blockchain/error.rs | Adds MaxPendingTxsPerAccountExceeded { count, limit } error variant/message. |
| crates/blockchain/blockchain.rs | Adds option/default constant and enforces per-account pending-tx cap in validate_transaction. |
| cmd/ethrex/l2/initializers.rs | Plumbs the new mempool cap option into L2 blockchain initialization. |
| cmd/ethrex/initializers.rs | Plumbs the new mempool cap option into L1 blockchain initialization. |
| cmd/ethrex/cli.rs | Adds CLI flag/env var for the new cap and wires it into Options. |
💡 Add Copilot custom instructions for smarter, more guided reviews. Learn how to get started.
| // Per-account pending-tx cap. Replacement candidates (same | ||
| // `(sender, nonce)`) bypass the cap — they don't grow the | ||
| // sender's pool footprint. | ||
| if tx_to_replace_hash.is_none() { | ||
| let count = self.mempool.count_for_sender(sender)?; | ||
| if count >= self.options.max_pending_txs_per_account { | ||
| return Err(MempoolError::MaxPendingTxsPerAccountExceeded { | ||
| count, | ||
| limit: self.options.max_pending_txs_per_account, | ||
| }); | ||
| } | ||
| } |
![greptile-apps[bot]](https://avatars.githubusercontent.com/in/867647?s=60&v=4){kind=small}
🤖 Codex Code ReviewFindings
Aside from those points, the wiring is straightforward. I couldn’t run Automated review by OpenAI Codex · gpt-5.4 · custom prompt |
…instead of magic number
A wrong-chain submission from a sender already at the per-account cap was previously rejected as MaxPendingTxsPerAccountExceeded — the more specific InvalidChainId error was never reached. Reordering surfaces the correct error. The cap check has no side effects (read-only Mempool query) so the reorder is safe. Follow-up not in this PR: integration tests against validate_transaction exercising the cap rejection and replacement-at-cap bypass paths. Currently blocked on test-fixture infra that doesn't seed sender accounts in test/tests/blockchain/mempool_tests.rs.
| .txs_by_sender_nonce | ||
| .range((sender, 0)..=(sender, u64::MAX)) | ||
| .count(); |
There was a problem hiding this comment.
This is bounded, so I think it's OK for now.
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`).
Copilot + greptile flagged that the message "Sender has 16 pending
transactions, exceeds the per-account cap of 16" is wrong at the
boundary (16 doesn't exceed 16; the check fires at `count >= limit`).
Rephrase so the wording stays accurate at every value:
"Sender has {count} pending transactions; adding a new one would
exceed the per-account cap of {limit}"
Copilot + greptile (P1) flagged a TOCTOU race: the count check in `validate_transaction` (read lock) and `Mempool::add_transaction` (write lock) are not held jointly. Two concurrent submissions from the same sender at `count = 15` both pass the cap check and then both insert, leaving the sender with 17 pending txs against a 16 limit. `Mempool::add_transaction` now takes `max_pending_txs_per_account` and re-counts under the same write lock that performs the insertion, returning `MaxPendingTxsPerAccountExceeded` directly. The redundant check in `validate_transaction` is dropped — replacements implicitly bypass the cap because the old tx is removed first, so the post-removal count is at most cap-1.
🤖 Codex Code Review
I couldn’t run Automated review by OpenAI Codex · gpt-5.4 · custom prompt |
🤖 Claude Code ReviewBased on the diff and source review, here is my assessment. Review: PR 6603 — cap mempool queued (future-nonce) txs per senderCritical: TOCTOU race reintroduces the exact bug this PR previously fixed
Two concurrent This is not a new/subtle issue — commit Worse, the comment at // The per-account pending-tx cap is enforced atomically inside
// `Mempool::add_transaction` (under the same write lock as the
// insertion) so concurrent submissions can't both pass a stale
// count check and race past the limit.This is now false and actively misleading to future readers/reviewers. Fix: re-check the queued count inside Bug: cap check doesn't exclude the tx being replaced, causing spurious rejection of legitimate RBF at the boundaryThe cap check ( Concretely: if a sender already has exactly This contradicts the PR's own stated intent ("Replacements... bypass it implicitly") and isn't exercised by the added unit tests (all of which test fresh insertions, not replace-at-cap). Consider excluding the tx's own existing slot from the count when a same-nonce entry already exists, or moving the cap check after Design gap: the PR's stated threat (single sender occupying the whole pool) is only partially mitigatedThe motivation says "a single sender can currently occupy 100% of the 10,000-tx pool... by submitting many distinct nonces." The final implementation caps only the queued/future (nonce-gapped) subpool; the executable/pending (contiguous-nonce) subpool remains completely uncapped per sender (there is no analogue of geth's Stale documentation
Performance note
Minor
What's good
Automated review by Claude (Anthropic) · sonnet · custom prompt |
|
@iovoid thanks for the review — both points are addressed on the current head:
Ready for another look whenever you have a moment. |
iovoid
left a comment
iovoid
left a comment
There was a problem hiding this comment.
The PR description mentions the old design.
| return Err(MempoolError::InvalidChainId(config.chain_id)); | ||
| } | ||
|
|
||
| // The per-account pending-tx cap is enforced atomically inside |
There was a problem hiding this comment.
We release the lock after checking, so it's not truly atomic.
There was a problem hiding this comment.
Fixed in 9d936b5. The queued-cap check moved out of validate_transaction (where it read under a released read lock) and into Mempool::add_transaction, under the same write lock as the insert, so the check and insert are atomic — two concurrent future-nonce txs from one sender can no longer both pass a stale count. validate_transaction now just threads the sender's on-chain nonce out via a QueuedCap so the locked check can tell future from executable without an async storage read under the lock.
| if let Some(queued) = | ||
| self.mempool | ||
| .queued_count_if_future(sender, sender_acc_info.nonce, nonce)? | ||
| && queued >= self.options.max_queued_txs_per_account | ||
| { |
There was a problem hiding this comment.
This prevents RBF of queued transactions, we should probably check find_tx_to_replace first.
There was a problem hiding this comment.
Fixed in 9d936b5. The admission callers now check find_tx_to_replace first and pass queued_cap = None when it matched, so a replacement — which reuses the same (sender, nonce) slot and never grows the queue — is exempt from the cap on all paths (including frame txs, whose predecessor is removed inside add_transaction after the check).
…slots-cap # Conflicts: # crates/blockchain/blockchain.rs # crates/blockchain/error.rs # crates/blockchain/mempool.rs
|
@iovoid thanks — all three addressed:
Also merged latest |
| /// avoid a TOCTOU race (EIP-8141, review fix 1.6). | ||
| /// | ||
| /// The per-account **queued** (future/nonce-gapped) cap is enforced upstream | ||
| /// in `Blockchain::validate_transaction`, which has the sender's on-chain |
There was a problem hiding this comment.
You're right — that doc was stale from before the cap moved into add_transaction. Fixed in fbc476f: the doc now says the queued cap is enforced here (atomically under the write lock, via the passed QueuedCap), not upstream in validate_transaction.
| } | ||
|
|
||
| #[cfg(test)] | ||
| mod tests { |
There was a problem hiding this comment.
There are no tests for the rejection case.
There was a problem hiding this comment.
Added in fbc476f: add_transaction_rejects_future_tx_over_queued_cap (sender at cap → MaxQueuedTxsPerAccountExceeded with count/limit asserted), plus add_transaction_accepts_future_tx_below_queued_cap and add_transaction_never_caps_executable_txs to bracket the boundary and confirm executable txs are never capped.
…bdaclass#6606) ## Motivation A sender at the per-sender slot cap (lambdaclass#6603) can still spam if only a few of their N pending txs are fundable from balance — the other N-1 are guaranteed-fail and waste pool budget at execution time. All four peer EL clients (geth, reth, nethermind, erigon) gate this with a cumulative-balance check at admission. ## Description - New `Mempool::sum_cost_for_sender(sender, exclude: Option<H256>) -> Result<U256, MempoolError>` ranges `txs_by_sender_nonce` for the sender and sums `cost_without_base_fee()` per pending tx. The `exclude` parameter is for the replacement case: the caller passes the about-to-be-replaced tx's hash so its cost is dropped from the running total at scan time. - `Blockchain::validate_transaction` computes the new tx's cost ONCE, performs the single-tx balance check, then `existing_cost = sum_cost_for_sender(sender, tx_to_replace_hash)?` and rejects with `MempoolError::InsufficientCumulativeBalance { required, available }` when `existing_cost + tx_cost > sender_balance`. - **Fails closed on inconsistency**: if `txs_by_sender_nonce` references a hash missing from `transaction_pool`, or if any included tx's cost can't be computed, `sum_cost_for_sender` returns an error rather than silently undercounting. The gate cannot be bypassed by an invariant violation. - Uses `checked_add` rather than `saturating_add` for the running total so overflow surfaces as `InvalidTxGasvalues` rather than silently saturating. ## Behavioral change Admission becomes stricter: a sender's total pending tx cost must fit within their on-chain balance. Existing single-tx balance checks are unchanged; the new gate fires for the (sender at cap, multiple expensive txs queued) case that previously passed individual checks but failed at execution. ## Deferred Integration tests for `InsufficientCumulativeBalance` require seeded sender-account fixtures not currently present in `test/tests/blockchain/mempool_tests.rs`. Same pattern as lambdaclass#6603 / lambdaclass#6576 / lambdaclass#6600 deferrals. --------- Co-authored-by: ElFantasma <estebandh@gmail.com> Co-authored-by: Esteban Dimitroff Hodi <esteban.dimitroff@lambdaclass.com>
…aclass#6609) ## Motivation Even after the per-sender slot cap (lambdaclass#6603) lands, a flood of small senders submitting gapped-nonce transactions can pin pool budget — those nonces are unreachable until intermediate nonces land, so they sit forever while productive contiguous-nonce txs from other senders are evicted. Reject gapped-nonce admissions when the mempool is heavily occupied; in steady-state (low occupancy) the pool tolerates gapped nonces, but under pressure it prefers contiguous, executable nonces. ## Description - New `Mempool::occupancy_pct(&self) -> u8` returns the pool occupancy as an integer percentage in `[0, 100]`. Returns 0 for unlimited pools (`max_mempool_size == 0`) so pressure-gated rules treat unbounded capacity as "never under pressure". - `Blockchain::validate_transaction` rejects incoming txs whose nonce isn't the on-chain next (i.e. `nonce != sender_acc_nonce`) once `occupancy_pct()` is at or above the configured threshold. The comparison is integer-only, so the boundary is deterministic; a threshold of `100` disables the gate, and an unlimited pool (occupancy `0`) is therefore never gated. Replacement txs (same `(sender, nonce)` as a pool entry) bypass the gate since they're not gapped. - Reads occupancy ONCE per admission and reuses the value for both the gate decision and the `MempoolError::GapAdmissionDeniedUnderPressure { occupancy_pct, nonce_gap }` error message — no TOCTOU between the decision and the reported value. - New CLI flag `--mempool.gap-admit-occupancy-threshold` (env `ETHREX_MEMPOOL_GAP_ADMIT_OCCUPANCY_THRESHOLD`, default `DEFAULT_GAP_ADMIT_OCCUPANCY_THRESHOLD = 90`). Setting it to 100 disables the gate. > Note on terminology: the gate keys on "nonce not equal to the sender's on-chain nonce," so under pressure it also rejects a nonce that is contiguous with an *already-pending* tx (e.g. on-chain nonce 10 with a pending nonce 10 → a nonce 11 is rejected), not only true nonce gaps. This is intentional — it stops a sender from parking a chain of pending txs under pressure — but the "gapped" wording is looser than the exact rule (per @iovoid's review). ## Behavioral change When the pool is at or above the configured occupancy threshold, non-contiguous-with-on-chain-nonce admissions return `GapAdmissionDeniedUnderPressure`. Operators can disable the gate by setting the threshold to 100. ## Deferred Origin-aware exemption (skipping the gate for `TxOrigin::Local`) waits for lambdaclass#6608's TxOrigin enum to merge. --------- Co-authored-by: Esteban Dimitroff Hodi <esteban.dimitroff@lambdaclass.com> Co-authored-by: ElFantasma <estebandh@gmail.com>
Motivation
A single sender can currently occupy 100% of the 10,000-tx pool by parking many future (nonce-gapped) transactions from one address, trivially bypassing the size cap (#6599), EIP-3607 check (#6600) and percentage-RBF rule (#6601). This adds a per-account cap on the queued (future-nonce) subpool — geth's
AccountQueuemodel — to bound that blast radius without penalizing legitimate high-throughput senders.Description
BlockchainOptionsgainsmax_queued_txs_per_account: usize(defaultDEFAULT_MAX_QUEUED_TXS_PER_ACCOUNT = 64, matching geth'sAccountQueue).is_future/queued_count_for_sender, ranging the existingtxs_by_sender_nonceindex). Executable (contiguous-nonce) txs are never capped, so a legitimate high-throughput single sender is unaffected — only nonce-gap parking spam is bounded.Mempool::add_transaction, under the same write lock as the insertion, so two concurrent future-nonce submissions from one sender can't both pass a stale count and race past the limit.Blockchain::validate_transactionthreads the sender's on-chain nonce out (viaQueuedCap) so the locked check can distinguish future from executable without a storage read under the lock.find_tx_to_replacefirst and passesqueued_cap = Nonewhen it matches — a same-(sender, nonce)replace reuses the slot and never grows the queue, so it routes through the normal RBF logic even when the sender is at the cap.MempoolError::MaxQueuedTxsPerAccountExceeded { sender, count, limit }.--mempool.max-queued-txs-per-account(envETHREX_MEMPOOL_MAX_QUEUED_TXS_PER_ACCOUNT), plumbed through both L1 and L2 initializers.Behavioral change
Admission becomes stricter only for future-nonce txs: a sender with 64 queued (nonce-gapped) txs submitting a 65th future tx is rejected at ingress. Executable txs and replacements remain unaffected.
Tests
Unit tests in
mempool.rscover the future/queued classification and per-sender isolation (gapped_tx_is_future_and_counts_only_queued,executable_flood_is_never_capped,queued_cap_isolates_senders, and thequeued_count_if_futurecases). The atomic enforcement + replacement-exemption wiring in the admission path is exercised by the existing mempool integration tests.Checklist
add_transaction; replacements exempt.