Skip to content

feat(l1): cap mempool pending transactions per sender (default 16)#6603

Open
ilitteri wants to merge 10 commits into
mainfrom
feat/mempool-account-slots-cap
Open

feat(l1): cap mempool pending transactions per sender (default 16)#6603
ilitteri wants to merge 10 commits into
mainfrom
feat/mempool-account-slots-cap

Conversation

@ilitteri
Copy link
Copy Markdown
Collaborator

Motivation

A single sender can currently occupy 100 % of the 10 000-tx pool. This trivially bypasses the size cap (#6599), EIP-3607 check (#6600) and percentage-RBF rule (#6601) by submitting many distinct nonces from one address. A per-sender pending-slot cap bounds that blast radius.

Description

  • BlockchainOptions gains max_pending_txs_per_account: usize (default 16) plus a DEFAULT_ACCOUNT_SLOTS constant.
  • 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::MaxPendingTxsPerAccountExceeded { count, limit }.
  • New CLI flag --mempool.max-pending-txs-per-account (env ETHREX_MEMPOOL_MAX_PENDING_TXS_PER_ACCOUNT), plumbed through both L1 and L2 initializers.

Behavioral change

Admission becomes stricter: senders already at 16 pending transactions submitting a 17th at a new nonce are rejected at ingress. Replacements remain allowed and route through the existing RBF logic.

Tests

Five unit tests in mempool.rs covering count_for_sender: empty pool, single tx, multiple nonces from same sender, isolation between senders, unknown sender returns zero. The six-line wire-up inside validate_transaction is 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.

ilitteri added 3 commits May 11, 2026 18:30
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`).
Copilot AI review requested due to automatic review settings May 11, 2026 21:46
@ilitteri ilitteri requested review from a team, ManuelBilbao and avilagaston9 as code owners May 11, 2026 21:46
@github-actions github-actions Bot added the L1 Ethereum client label May 11, 2026
@github-actions
Copy link
Copy Markdown

🤖 Kimi Code Review

The 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 Found

1. Race Condition (TOCTOU) in Account Limit Check

File: crates/blockchain/blockchain.rs (lines 2518-2525)

The check acquires a read lock, drops it, and later add_transaction acquires a write lock. Between these operations, another transaction could be added for the same sender, potentially exceeding the cap.

// 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 here

Recommendation: 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 Duplication

Files:

  • cmd/ethrex/cli.rs (line 189: default_value_t = 16)
  • cmd/ethrex/cli.rs (line 462: hardcoded 16)
  • crates/blockchain/blockchain.rs (line 256: DEFAULT_MAX_PENDING_TXS_PER_ACCOUNT)

The default value 16 is defined in three separate locations. If changed in one place, the others become inconsistent.

Recommendation: Import and use DEFAULT_MAX_PENDING_TXS_PER_ACCOUNT from blockchain crate in cli.rs since cmd/ethrex already depends on crates/blockchain.

3. Error Message Accuracy

File: crates/blockchain/error.rs (line 86)

#[error("Sender has {count} pending transactions, exceeds the per-account cap of {limit}")]

When count == limit, the message claims the count "exceeds" the limit, which is logically incorrect (16 does not exceed 16).

Recommendation: Change to: "Sender has {count} pending transactions, reaches/exceeds the per-account cap of {limit}" or "Per-account cap of {limit} exceeded (current: {count})".

4. Potential Performance Note

File: crates/blockchain/mempool.rs (lines 456-460)

The count_for_sender method iterates over all nonces for a sender. While bounded by the cap, it executes on every new transaction for non-replacement cases.

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

  • Correct bypass logic: Replacement transactions correctly bypass the cap check (line 2518: if tx_to_replace_hash.is_none())
  • Proper isolation: count_for_sender correctly uses range queries on the (Address, u64) composite key
  • Good test coverage: Tests cover empty pools, single/multiple nonces, and sender isolation
  • Consistent configuration: Properly threaded through L1 and L2 initializers

Summary

The 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

@github-actions
Copy link
Copy Markdown

github-actions Bot commented May 11, 2026

Lines of code report

Total lines added: 105
Total lines removed: 0
Total lines changed: 105

Detailed view
+----------------------------------------+-------+------+
| File                                   | Lines | Diff |
+----------------------------------------+-------+------+
| ethrex/cmd/ethrex/cli.rs               | 1118  | +10  |
+----------------------------------------+-------+------+
| ethrex/cmd/ethrex/initializers.rs      | 656   | +1   |
+----------------------------------------+-------+------+
| ethrex/cmd/ethrex/l2/initializers.rs   | 382   | +1   |
+----------------------------------------+-------+------+
| ethrex/crates/blockchain/blockchain.rs | 2519  | +11  |
+----------------------------------------+-------+------+
| ethrex/crates/blockchain/error.rs      | 153   | +4   |
+----------------------------------------+-------+------+
| ethrex/crates/blockchain/mempool.rs    | 542   | +78  |
+----------------------------------------+-------+------+

@github-actions
Copy link
Copy Markdown

🤖 Claude Code Review

Now 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

count_for_sender range logic (mempool.rs:456–460) — correct. The range (sender, 0)..=(sender, u64::MAX) on a BTreeMap<(H160, u64), H256> isolates exactly the sender's entries because BTreeMap uses lexicographic tuple ordering: any key whose first element is strictly less or strictly greater than sender falls outside the bounds.

Replacement bypass logic (blockchain.rs:2518) — correct. Calling find_tx_to_replace first and only checking the cap when tx_to_replace_hash.is_none() ensures replacements can always proceed regardless of the sender's slot count.


Bug: max_pending_txs_per_account = 0 silently blocks all new transactions

With limit 0, count_for_sender returns 0 for an empty slot, and 0 >= 0 causes every non-replacement ingress to be rejected. There is no input validation or minimum enforced anywhere in the CLI or BlockchainOptions. An operator who sets --mempool.max-pending-txs-per-account 0 (perhaps intending "unlimited") will silently brick ingress.

Suggested fix — add a clap value_parser that rejects 0:

value_parser = clap::value_parser!(usize).range(1..),

Or validate at startup and error out clearly.


Duplicated magic number 16

The value appears in three independent places:

  • cli.rs default_value_t = 16
  • cli.rs Default impl hardcoded 16
  • blockchain.rs DEFAULT_MAX_PENDING_TXS_PER_ACCOUNT

Since cmd/ethrex already depends on crates/blockchain, the CLI could import and use the constant directly:

use ethrex_blockchain::blockchain::DEFAULT_MAX_PENDING_TXS_PER_ACCOUNT;

#[arg(default_value_t = DEFAULT_MAX_PENDING_TXS_PER_ACCOUNT, ...)]
pub mempool_max_pending_txs_per_account: usize,

// and in Default impl:
mempool_max_pending_txs_per_account: DEFAULT_MAX_PENDING_TXS_PER_ACCOUNT,

Error message inaccuracy (error.rs:86)

#[error("Sender has {count} pending transactions, exceeds the per-account cap of {limit}")]

When count == limit (e.g., 16 == 16), the message claims the count "exceeds" the limit, which is false. Suggest:

"Sender has {count} pending transactions, the per-account cap is {limit}"

TOCTOU race between count and insert

Between count_for_sender releasing its read lock and add_transaction acquiring a write lock, a concurrent request for the same sender can slip through. The cap is a soft admission limit (not a consensus rule), so minor over-admission is acceptable — but it should be documented, since operators may be surprised if a node under concurrent load admits e.g. 18 instead of 16.


Missing test: cap enforcement at validate_transaction

All five new unit tests exercise only count_for_sender. There is no test verifying the end-to-end enforcement path:

  • A sender at the cap is rejected for a new nonce.
  • A sender at the cap is accepted for a replacement at an existing nonce.

The replacement bypass is a subtle invariant that's explicitly called out in the PR description and in comments — it deserves a regression test. Without one, a future refactor of validate_transaction could silently break it.


Minor nits

  • count_for_sender is documented as "Used by the per-sender slot cap at admission" in its doc comment. This is useful context but ties the method to a specific caller. Neutral on this.
  • DEFAULT_MAX_PENDING_TXS_PER_ACCOUNT is placed after the Default impl that references it. Rust allows forward references within a module, so this compiles, but putting the constant before the Default impl would be more readable.

Summary

The implementation is functionally correct for the common case. The max_pending_txs_per_account = 0 footgun is the most impactful item to address before merging. The duplicated magic number 16 is a maintenance risk. Tests for the actual enforcement path (particularly the replacement bypass) should be added. The error message inaccuracy is minor but easy to fix.


Automated review by Claude (Anthropic) · sonnet · custom prompt

Copy link
Copy Markdown
Contributor

Copilot AI left a comment

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

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_sender using 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.

Comment thread crates/blockchain/error.rs Outdated
TxMaxInitCodeSizeError,
#[error("Transaction max data size exceeded")]
TxMaxDataSizeError,
#[error("Sender has {count} pending transactions, exceeds the per-account cap of {limit}")]
Copy link
Copy Markdown
Collaborator Author

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

Comment thread crates/blockchain/blockchain.rs Outdated
Comment on lines +2518 to +2529
// 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,
});
}
}
Copy link
Copy Markdown
Collaborator Author

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

Comment thread cmd/ethrex/cli.rs
Comment on lines +186 to +194
#[arg(
help = "Maximum number of pending transactions a single sender may hold in the mempool. Replacements at an existing (sender, nonce) bypass this cap.",
long = "mempool.max-pending-txs-per-account",
default_value_t = 16,
value_name = "MAX_PENDING_TXS_PER_ACCOUNT",
help_heading = "Node options",
env = "ETHREX_MEMPOOL_MAX_PENDING_TXS_PER_ACCOUNT"
)]
pub mempool_max_pending_txs_per_account: usize,
Copy link
Copy Markdown
Collaborator Author

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

Comment thread cmd/ethrex/cli.rs
Comment on lines 459 to 463
dev: Default::default(),
force: false,
mempool_max_size: Default::default(),
mempool_max_pending_txs_per_account: 16,
tx_broadcasting_time_interval: Default::default(),
Copy link
Copy Markdown
Collaborator Author

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

@greptile-apps
Copy link
Copy Markdown

greptile-apps Bot commented May 11, 2026

Greptile Summary

This PR caps pending mempool transactions per sender at 16 (configurable) to prevent a single address from monopolizing the pool. The cap is enforced in validate_transaction by counting the sender's existing slots via a BTreeMap range scan, with replacements at the same (sender, nonce) correctly bypassing the check.

  • count_for_sender is added to Mempool, using txs_by_sender_nonce.range((sender, 0)..=(sender, u64::MAX)) — O(log n + k) — with five unit tests covering empty pool, single tx, multiple nonces, sender isolation, and unknown sender.
  • BlockchainOptions gains max_pending_txs_per_account (default 16) and the value is plumbed through both L1 and L2 CLI initializers via a new --mempool.max-pending-txs-per-account flag.
  • The count check and the subsequent add_transaction call are not held under a common lock, so the cap can be slightly exceeded when multiple transactions from the same sender are submitted concurrently.

Confidence Score: 3/5

The new per-sender cap is implemented correctly for the single-threaded case but can be exceeded under concurrent submissions from the same address because the count check and the insertion are not atomic.

The core validation path reads the sender count under a read lock, releases that lock, then re-acquires a write lock in add_transaction. Two simultaneous submissions from the same address can both see count=15 < 16, both pass validation, and both be inserted — ending with 17 pending slots. Given that the explicit goal of this PR is to bound per-sender pool footprint, allowing the bound to be silently overshot undermines the stated invariant. The rest of the change looks correct.

crates/blockchain/blockchain.rs — the validate_transaction function where the count check and add are split across two non-overlapping lock acquisitions.

Important Files Changed

Filename Overview
crates/blockchain/blockchain.rs Adds per-account cap check in validate_transaction; the count check and the subsequent add_transaction call are not atomic, creating a TOCTOU window where the cap can be exceeded under concurrent submissions.
crates/blockchain/mempool.rs Adds count_for_sender using a BTreeMap range over txs_by_sender_nonce; implementation and unit tests look correct.
crates/blockchain/error.rs New MaxPendingTxsPerAccountExceeded variant added; error message is slightly misleading when count equals the limit exactly.
cmd/ethrex/cli.rs New CLI flag --mempool.max-pending-txs-per-account with default 16 and env var wired correctly; default in Options::default() matches.
cmd/ethrex/initializers.rs Threads max_pending_txs_per_account from CLI opts into BlockchainOptions for L1 initializer.
cmd/ethrex/l2/initializers.rs Threads max_pending_txs_per_account from CLI opts into BlockchainOptions for L2 initializer.
docs/CLI.md CLI docs updated to document new flag and its default value.

Sequence Diagram

sequenceDiagram
    participant Caller
    participant Blockchain
    participant Mempool

    Caller->>Blockchain: add_transaction_to_pool(tx)
    Blockchain->>Mempool: find_tx_to_replace(sender, nonce, tx)
    Mempool-->>Blockchain: None (new nonce) or Some(hash) (replacement)

    alt New nonce (no replacement)
        Blockchain->>Mempool: count_for_sender(sender) [read lock → release]
        Mempool-->>Blockchain: count
        alt "count >= max_pending_txs_per_account"
            Blockchain-->>Caller: Err(MaxPendingTxsPerAccountExceeded)
        end
    end

    Note over Blockchain,Mempool: TOCTOU window — lock released

    Blockchain->>Mempool: add_transaction(hash, sender, mtx) [write lock]
    Mempool-->>Blockchain: Ok(())
    Blockchain-->>Caller: Ok(hash)
Loading
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:2521-2529
**TOCTOU race allows cap to be exceeded under concurrent load**

The count check in `validate_transaction` and the actual `add_transaction` call hold separate, non-overlapping locks. Between releasing the read lock after `count_for_sender` and acquiring the write lock inside `add_transaction`, another concurrent submission from the same sender can pass the same check. Both threads see `count = 15 < 16`, both pass, and both are inserted, leaving the sender with 17 pending slots instead of the intended 16. Wrapping the validate-and-add sequence in a single write-locked section — or moving the count check inside `add_transaction` where the write lock is already held — would close this window.

### Issue 2 of 2
crates/blockchain/error.rs:86-87
When `count == limit` the message reads "has 16 pending transactions, exceeds the per-account cap of 16", but 16 does not exceed 16. The phrasing should reflect that admitting a new transaction *would* exceed the cap.

```suggestion
    #[error("Sender has {count} pending transactions; adding a new one would exceed the per-account cap of {limit}")]
    MaxPendingTxsPerAccountExceeded { count: usize, limit: usize },
```

Reviews (1): Last reviewed commit: "rename(l1): mempool per-account cap from..." | Re-trigger Greptile

Comment thread crates/blockchain/blockchain.rs Outdated
Comment on lines +2521 to +2529
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,
});
}
}
Copy link
Copy Markdown

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

P1 TOCTOU race allows cap to be exceeded under concurrent load

The count check in validate_transaction and the actual add_transaction call hold separate, non-overlapping locks. Between releasing the read lock after count_for_sender and acquiring the write lock inside add_transaction, another concurrent submission from the same sender can pass the same check. Both threads see count = 15 < 16, both pass, and both are inserted, leaving the sender with 17 pending slots instead of the intended 16. Wrapping the validate-and-add sequence in a single write-locked section — or moving the count check inside add_transaction where the write lock is already held — would close this window.

Prompt To Fix With AI
This is a comment left during a code review.
Path: crates/blockchain/blockchain.rs
Line: 2521-2529

Comment:
**TOCTOU race allows cap to be exceeded under concurrent load**

The count check in `validate_transaction` and the actual `add_transaction` call hold separate, non-overlapping locks. Between releasing the read lock after `count_for_sender` and acquiring the write lock inside `add_transaction`, another concurrent submission from the same sender can pass the same check. Both threads see `count = 15 < 16`, both pass, and both are inserted, leaving the sender with 17 pending slots instead of the intended 16. Wrapping the validate-and-add sequence in a single write-locked section — or moving the count check inside `add_transaction` where the write lock is already held — would close this window.

How can I resolve this? If you propose a fix, please make it concise.

Copy link
Copy Markdown
Collaborator Author

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

Comment thread crates/blockchain/error.rs Outdated
Comment on lines +86 to +87
#[error("Sender has {count} pending transactions, exceeds the per-account cap of {limit}")]
MaxPendingTxsPerAccountExceeded { count: usize, limit: usize },
Copy link
Copy Markdown

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

P2 When count == limit the message reads "has 16 pending transactions, exceeds the per-account cap of 16", but 16 does not exceed 16. The phrasing should reflect that admitting a new transaction would exceed the cap.

Suggested change
#[error("Sender has {count} pending transactions, exceeds the per-account cap of {limit}")]
MaxPendingTxsPerAccountExceeded { count: usize, limit: usize },
#[error("Sender has {count} pending transactions; adding a new one would exceed the per-account cap of {limit}")]
MaxPendingTxsPerAccountExceeded { count: usize, limit: usize },
Prompt To Fix With AI
This is a comment left during a code review.
Path: crates/blockchain/error.rs
Line: 86-87

Comment:
When `count == limit` the message reads "has 16 pending transactions, exceeds the per-account cap of 16", but 16 does not exceed 16. The phrasing should reflect that admitting a new transaction *would* exceed the cap.

```suggestion
    #[error("Sender has {count} pending transactions; adding a new one would exceed the per-account cap of {limit}")]
    MaxPendingTxsPerAccountExceeded { count: usize, limit: usize },
```

How can I resolve this? If you propose a fix, please make it concise.

Copy link
Copy Markdown
Collaborator Author

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

@github-actions
Copy link
Copy Markdown

🤖 Codex Code Review

Findings

  1. High: The new sender-cap check is raceable. crates/blockchain/blockchain.rs:2516 reads replacement/count state during validate_transaction, but the tx is only inserted later in crates/blockchain/blockchain.rs:2369 and crates/blockchain/mempool.rs:151. Two concurrent submissions from the same sender can both observe count == limit - 1 and both be admitted, so the cap is bypassable. This needs to be enforced in the same write-side mempool operation that resolves replacement and inserts the tx.

  2. Medium: The cap counts stale txs that are already invalid at the current head. crates/blockchain/mempool.rs:456 counts every (sender, nonce) slot, but block import only removes exact included hashes in crates/blockchain/blockchain.rs:2386, and the L1 payload path keeps failed txs in the mempool in crates/blockchain/payload.rs:688. If a different same-nonce tx is mined elsewhere, the local stale tx still consumes one of the sender’s slots and can cause false MaxPendingTxsPerAccountExceeded rejections for later valid txs. I’d either prune nonce < sender_acc_info.nonce entries before enforcing the cap, or exclude them from the count.

Aside from those points, the wiring is straightforward.

I couldn’t run cargo check in this sandbox because cargo/git cache writes are blocked here, so this review is from diff inspection only.


Automated review by OpenAI Codex · gpt-5.4 · custom prompt

ilitteri added 2 commits May 11, 2026 18:57
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.
Copy link
Copy Markdown
Collaborator

@MegaRedHand MegaRedHand left a comment

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

LGTM

Comment on lines +459 to +461
.txs_by_sender_nonce
.range((sender, 0)..=(sender, u64::MAX))
.count();
Copy link
Copy Markdown
Collaborator

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

This is bounded, so I think it's OK for now.

ilitteri added a commit that referenced this pull request May 11, 2026
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`).
ilitteri added 2 commits May 11, 2026 21:38
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.
ilitteri added a commit to benbencik/ethrex that referenced this pull request May 13, 2026
…ambdaclass#6599)

**Motivation**

Every major Ethereum execution client (geth `txMaxSize`, reth
`DEFAULT_MAX_TX_INPUT_BYTES`, nethermind `MaxTxSize` / `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**

- New constants `MAX_TX_SIZE = 128 KiB` and `MAX_BLOB_TX_SIZE = 1 MiB`
in `crates/common/types/constants.rs`.
- `Blockchain::validate_transaction` enforces `MAX_TX_SIZE` on the
canonical RLP encoding of non-blob transactions.
- `Blockchain::add_blob_transaction_to_pool` enforces `MAX_BLOB_TX_SIZE`
on the **wire wrapper** — `Transaction::encode_canonical_to_vec().len()
+ BlobsBundle::encode_to_vec().len()` — to match geth (`tx.Size()`
includes the attached sidecar), nethermind (`MaxBlobTxSize` is checked
on the wrapper form), and erigon (`ValidateSerializedTxn` works 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.
- New error `MempoolError::TxSizeExceeded { actual, limit }`.
- Drops the redundant `MAX_TRANSACTION_DATA_SIZE` calldata-only check
from `validate_transaction` — the new encoded-size cap is strictly
tighter (a 128 KiB calldata payload always encodes to > 128 KiB).
- Unit test in `mempool_tests.rs` covering the non-blob path.
Integration test for the wrapper check deferred (the `c-kzg`-gated
`add_blob_transaction_to_pool` isn't currently exercised by
`mempool_tests`, same pattern as PRs lambdaclass#6603/lambdaclass#6576).
…slots-cap

# Conflicts:
#	crates/blockchain/error.rs
@github-actions
Copy link
Copy Markdown

github-actions Bot commented May 13, 2026

⚠️ Known Issues — intentionally skipped tests

Source: docs/known_issues.md

Known Issues

Tests intentionally excluded from CI. Source of truth for the Known
Issues
section the L1 workflow appends to each ef-tests job summary
and posts as a sticky PR comment.

Hive — bal-devnet-6 Amsterdam consume-engine tests — 32 functions / 54 cases

Same root cause as the blockchain-runner skip list (see EF Tests —
Blockchain
below): snobal-devnet-6 fixtures expect bal-devnet-6 spec
semantics, but our impl runs ahead due to the bal-devnet-7-prep
set_delegation SELFDESTRUCT-style refund subtraction. These fixtures
are routed through hive's eels/consume-engine simulator and produce
the same failures. Excluded via KNOWN_EXCLUDED_TESTS (substring
match on test_<name>[fork_Amsterdam, anchoring to the Amsterdam fork
so legacy Prague/Osaka variants still run).

Affected EELS test functions (32)
  • test_auth_refund_block_gas_accounting
  • test_auth_refund_bypasses_one_fifth_cap
  • test_auth_state_gas_scales_with_cpsb
  • test_auth_with_calldata_and_access_list
  • test_auth_with_multiple_sstores
  • test_authorization_exact_state_gas_boundary
  • test_authorization_to_precompile_address
  • test_authorization_with_sstore
  • test_bal_7702_delegation_clear
  • test_bal_7702_delegation_create
  • test_bal_7702_delegation_update
  • test_bal_7702_double_auth_reset
  • test_bal_7702_double_auth_swap
  • test_bal_7702_null_address_delegation_no_code_change
  • test_bal_all_transaction_types
  • test_bal_selfdestruct_to_7702_delegation
  • test_bal_withdrawal_to_7702_delegation
  • test_duplicate_signer_authorizations
  • test_existing_account_auth_header_gas_used_uses_worst_case
  • test_existing_account_refund
  • test_existing_account_refund_enables_sstore
  • test_existing_auth_with_reverted_execution_preserves_intrinsic
  • test_many_authorizations_state_gas
  • test_mixed_auths_header_gas_used_uses_worst_case
  • test_mixed_new_and_existing_auths
  • test_mixed_valid_and_invalid_auths
  • test_multi_tx_block_auth_refund_and_sstore
  • test_multiple_refund_types_in_one_tx
  • test_simple_gas_accounting
  • test_sstore_state_gas_all_tx_types
  • test_transfer_with_all_tx_types
  • test_varying_calldata_costs

EF Tests — Stateless coverage narrowed to EIP-8025 optional-proofs

make -C tooling/ef_tests/blockchain test calls test-stateless-zkevm
instead of test-stateless. The zkevm@v0.3.3 fixtures are filled against
bal@v5.6.1, out of sync with current bal spec; the broad target trips ~549
fixtures. Re-broaden once the zkevm bundle is regenerated.

Why and resolution path

PR #6527 broadened
test-stateless to extract the entire for_amsterdam/ tree from the
zkevm bundle and run all of it under --features stateless; combined with
this branch's bal-devnet-6+ semantics (and bal-devnet-7-prep
set_delegation re-application) that scope produces ~549
GasUsedMismatch / ReceiptsRootMismatch /
BlockAccessListHashMismatch failures.

test-stateless-zkevm filters cargo to the eip8025_optional_proofs
suite, which still validates the stateless harness without the bal-version
mismatch.

Re-broaden by switching test: back to test-stateless in
tooling/ef_tests/blockchain/Makefile once the zkevm bundle is regenerated
against the current bal spec.

EF Tests — Blockchain bal-devnet-6 (Amsterdam fork) — 74 tests

snobal-devnet-6 fixtures expect bal-devnet-6 spec semantics, but our impl
runs ahead due to the bal-devnet-7-prep set_delegation SELFDESTRUCT-style
refund subtraction. Skipped in
tooling/ef_tests/blockchain/tests/all.rs::SKIPPED_BASE, anchored on
[fork_Amsterdam so legacy Prague / Osaka variants still run.

Bucket breakdown (74 total) and resolution path
EIP Bucket Count
EIP-7702 set_code_txs 24
EIP-7702 set_code_txs_2 15
EIP-7702 gas 1
EIP-8037 state_gas_set_code 17
EIP-8037 state_gas_pricing 1
EIP-8037 state_gas_sstore 1
EIP-7928 block_access_lists_eip7702 8
EIP-7928 block_access_lists 1
EIP-7778 gas_accounting 3
EIP-7708 transfer_logs 1
EIP-7976 refunds 1
EIP-1344 chainid (Amsterdam fork-transition fixture) 1
Total 74

Re-enable once we either:

  • (a) bump fixtures to a snobal-devnet-7 release that locks in the new
    accounting; or
  • (b) revert the bal-devnet-7-prep subtraction for bal-devnet-6
    compatibility.
Full test list (74)

EIP-7702 — for_amsterdam/prague/eip7702_set_code_tx/set_code_txs/

  • delegation_clearing
  • delegation_clearing_and_set
  • delegation_clearing_failing_tx
  • delegation_clearing_tx_to
  • eoa_tx_after_set_code
  • ext_code_on_chain_delegating_set_code
  • ext_code_on_self_delegating_set_code
  • ext_code_on_self_set_code
  • ext_code_on_set_code
  • many_delegations
  • nonce_overflow_after_first_authorization
  • nonce_validity
  • reset_code
  • self_code_on_set_code
  • self_sponsored_set_code
  • set_code_multiple_valid_authorization_tuples_same_signer_increasing_nonce
  • set_code_multiple_valid_authorization_tuples_same_signer_increasing_nonce_self_sponsored
  • set_code_to_log
  • set_code_to_non_empty_storage_non_zero_nonce
  • set_code_to_self_destruct
  • set_code_to_self_destructing_account_deployed_in_same_tx
  • set_code_to_sstore
  • set_code_to_sstore_then_sload
  • set_code_to_system_contract

EIP-7702 — for_amsterdam/prague/eip7702_set_code_tx/set_code_txs_2/

  • call_pointer_to_created_from_create_after_oog_call_again
  • call_to_precompile_in_pointer_context
  • contract_storage_to_pointer_with_storage
  • delegation_replacement_call_previous_contract
  • double_auth
  • pointer_measurements
  • pointer_normal
  • pointer_reentry
  • pointer_resets_an_empty_code_account_with_storage
  • pointer_reverts
  • pointer_to_pointer
  • pointer_to_precompile
  • pointer_to_static
  • pointer_to_static_reentry
  • static_to_pointer

EIP-7702 — for_amsterdam/prague/eip7702_set_code_tx/gas/

  • account_warming

EIP-8037 — for_amsterdam/amsterdam/eip8037_state_creation_gas_cost_increase/state_gas_set_code/

  • auth_refund_block_gas_accounting
  • auth_refund_bypasses_one_fifth_cap
  • auth_with_calldata_and_access_list
  • auth_with_multiple_sstores
  • authorization_exact_state_gas_boundary
  • authorization_to_precompile_address
  • authorization_with_sstore
  • duplicate_signer_authorizations
  • existing_account_auth_header_gas_used_uses_worst_case
  • existing_account_refund
  • existing_account_refund_enables_sstore
  • existing_auth_with_reverted_execution_preserves_intrinsic
  • many_authorizations_state_gas
  • mixed_auths_header_gas_used_uses_worst_case
  • mixed_new_and_existing_auths
  • mixed_valid_and_invalid_auths
  • multi_tx_block_auth_refund_and_sstore

EIP-8037 — state_gas_pricing/

  • auth_state_gas_scales_with_cpsb

EIP-8037 — state_gas_sstore/

  • sstore_state_gas_all_tx_types

EIP-7928 — for_amsterdam/amsterdam/eip7928_block_level_access_lists/block_access_lists_eip7702/

  • bal_7702_delegation_clear
  • bal_7702_delegation_create
  • bal_7702_delegation_update
  • bal_7702_double_auth_reset
  • bal_7702_double_auth_swap
  • bal_7702_null_address_delegation_no_code_change
  • bal_selfdestruct_to_7702_delegation
  • bal_withdrawal_to_7702_delegation

EIP-7928 — block_access_lists/

  • bal_all_transaction_types

EIP-7778 — for_amsterdam/amsterdam/eip7778_block_gas_accounting_without_refunds/gas_accounting/

  • multiple_refund_types_in_one_tx
  • simple_gas_accounting
  • varying_calldata_costs

EIP-7708 — for_amsterdam/amsterdam/eip7708_eth_transfer_logs/transfer_logs/

  • transfer_with_all_tx_types

EIP-7976 — for_amsterdam/amsterdam/eip7976_increase_calldata_floor_cost/refunds/

  • gas_refunds_from_data_floor

EIP-1344 — for_amsterdam/istanbul/eip1344_chainid/chainid/

  • chainid (Amsterdam fork-transition fixture)

ilitteri added 2 commits May 13, 2026 18:31
When this branch added max_pending_txs_per_account to Mempool::add_transaction,
three test sites (one filter test, two blob-bundle tests) carried over from
main were left on the old 3-arg form. They are unrelated to the per-sender
cap, so pass usize::MAX to opt out.
@ilitteri ilitteri requested a review from a team as a code owner May 14, 2026 14:07
Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment

Labels

L1 Ethereum client

Projects

Status: No status

Development

Successfully merging this pull request may close these issues.

3 participants