feat(l1): punishSpammer — drop highest-nonce half on per-sender cap breach#6625
feat(l1): punishSpammer — drop highest-nonce half on per-sender cap breach#6625ilitteri wants to merge 5 commits into
Conversation
…reach When a sender exceeds the per-account pending-tx cap, drop the highest-nonce half (rounded up) of their existing pool entries in addition to rejecting the new transaction. A sender hitting the cap is likely spamming with future nonces; dropping their upper-half entries reclaims pool budget for transactions more likely to execute next. The breaching tx is still rejected — punishment doesn't help the spammer, it only frees pool budget for legitimate senders. The prune happens inside Mempool::add_transaction under the same write lock that does the cap check, so the count, removal, and rejection are all atomic. Blob bundles are removed for dropped EIP-4844 entries via remove_transaction_with_lock. Adds an opt-out: --mempool.punish-spammer (env ETHREX_MEMPOOL_PUNISH_SPAMMER, default true). Plumbed through BlockchainOptions::punish_spammer for both L1 and L2 initializers.
ilitteri
requested review from
a team,
ManuelBilbao and
avilagaston9
as code owners
🤖 Kimi Code ReviewThe implementation is solid and follows Rust best practices. The spammer punishment logic is correctly implemented with proper locking, and the test coverage is comprehensive (including critical 4844 blob bundle cleanup). Minor suggestions:
for victim_hash in &victims {
inner.remove_transaction_with_lock(victim_hash)
.expect("mempool invariant violated: hash in txs_by_sender_nonce missing from txs_by_hash");
}However, the current error propagation is safe and acceptable.
Documentation: Security: Testing: Verdict: LGTM. The code is correct, well-tested, and handles the consensus-critical mempool resource limits appropriately. Automated review by Kimi (Moonshot AI) · kimi-k2.5 · custom prompt |
Lines of code reportTotal lines added: Detailed view |
Greptile SummaryThis PR implements an Erigon-style "punishSpammer" policy: when a sender's pending-tx count hits the per-account cap, the mempool now also evicts the highest-nonce
Confidence Score: 4/5The punishment eviction is correctly locked, nonce-ordered, and handles blob bundles. Two minor rough edges in the mempool bookkeeping are worth addressing before this pattern is used at higher spam volumes. The cap-breach detection, victim selection, and blob-bundle cleanup are all correct and well-tested. The two concerns are: crates/blockchain/mempool.rs — the new punishment block in
|
| Filename | Overview |
|---|---|
| crates/blockchain/mempool.rs | Core change: adds punish_spammer path in add_transaction that drops ceil(N/2) highest-nonce entries on cap breach. Logic is correct and lock-safe; two minor issues: txs_order not pruned for removed victims (amplifying pre-existing staleness), and the error's count field reflects pre-removal state. |
| crates/blockchain/blockchain.rs | Threads punish_spammer from BlockchainOptions through to Mempool::add_transaction at both call sites; adds field with correct default (true). Straightforward plumbing, no issues. |
| cmd/ethrex/cli.rs | Adds --mempool.punish-spammer CLI flag with ArgAction::Set (correct for bool args), env var, and default. Default impl also updated consistently. |
| cmd/ethrex/initializers.rs | Passes opts.mempool_punish_spammer into BlockchainOptions for the L1 initializer. Correct and minimal. |
| cmd/ethrex/l2/initializers.rs | Same plumbing for L2 initializer. Consistent with L1 path. |
| docs/CLI.md | Documents the new --mempool.punish-spammer flag with env var and default. Accurate and complete. |
Flowchart
%%{init: {'theme': 'neutral'}}%%
flowchart TD
A["add_transaction(hash, sender, tx, cap, punish_spammer)"] --> B["Acquire write lock"]
B --> C["count = txs_by_sender_nonce.range(sender).count()"]
C --> D{count >= cap?}
D -- No --> E["Normal insertion path\n(prune check → insert → notify)"]
E --> F["Ok(())"]
D -- Yes --> G{punish_spammer?}
G -- No --> H["return Err(MaxPendingTxsPerAccountExceeded)"]
G -- Yes --> I["dropped_count = ceil(count / 2)"]
I --> J["victims = top dropped_count hashes (rev order)"]
J --> K["new_top_nonce = nth(dropped_count) from rev iter"]
K --> L["for each victim:\nremove_transaction_with_lock(victim)"]
L --> M["removes from transaction_pool,\ntxs_by_sender_nonce,\nbroadcast_pool,\nblobs_bundle_pool"]
M --> N["warn! log (sender, dropped_count, new_top_nonce)"]
N --> H
Comments Outside Diff (1)
-
crates/blockchain/mempool.rs, line 206-209 (link)Error
countreflects pre-punishment sender countcountis captured before victims are removed, soMaxPendingTxsPerAccountExceeded { count, limit }always reports the breach-time size. After punishment fires, the sender's actual pool occupancy iscount - dropped_count(half the cap), but any caller that inspects the error'scountfield to understand current pool state will see an inflated value. At minimum the doc-comment on the struct field should note this; ideallycountis either not included in the punish path or updated to the post-removal value.Prompt To Fix With AI
This is a comment left during a code review. Path: crates/blockchain/mempool.rs Line: 206-209 Comment: Error `count` reflects pre-punishment sender count `count` is captured before victims are removed, so `MaxPendingTxsPerAccountExceeded { count, limit }` always reports the breach-time size. After punishment fires, the sender's actual pool occupancy is `count - dropped_count` (half the cap), but any caller that inspects the error's `count` field to understand current pool state will see an inflated value. At minimum the doc-comment on the struct field should note this; ideally `count` is either not included in the punish path or updated to the post-removal value. 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/mempool.rs:195-197
`txs_order` not pruned during punishment removals
`remove_transaction_with_lock` removes a hash from `transaction_pool`, `txs_by_sender_nonce`, `broadcast_pool`, and `blobs_bundle_pool`, but never from `txs_order`. When punishment drops `ceil(N/2)` entries, all those hashes stay in `txs_order` as stale tombstones. Under sustained spam, `txs_order.len()` can diverge significantly from `transaction_pool.len()`, making the prune-threshold check (`txs_order.len() > mempool_prune_threshold`) fire prematurely on every legitimate insertion and causing `remove_oldest_transaction` to iterate through many dead entries before finding a live one to evict. This is a pre-existing gap in `remove_transaction_with_lock`, but the new batch-removal path amplifies it — a single cap-breach event can add up to 8 stale entries at once instead of one.
### Issue 2 of 2
crates/blockchain/mempool.rs:206-209
Error `count` reflects pre-punishment sender count
`count` is captured before victims are removed, so `MaxPendingTxsPerAccountExceeded { count, limit }` always reports the breach-time size. After punishment fires, the sender's actual pool occupancy is `count - dropped_count` (half the cap), but any caller that inspects the error's `count` field to understand current pool state will see an inflated value. At minimum the doc-comment on the struct field should note this; ideally `count` is either not included in the punish path or updated to the post-removal value.
Reviews (1): Last reviewed commit: "feat(l1): punishSpammer — drop highest-n..." | Re-trigger Greptile
![greptile-apps[bot]](https://avatars.githubusercontent.com/in/867647?s=60&v=4){kind=small}
| for victim_hash in &victims { | ||
| inner.remove_transaction_with_lock(victim_hash)?; | ||
| } |
There was a problem hiding this comment.
txs_order not pruned during punishment removals
remove_transaction_with_lock removes a hash from transaction_pool, txs_by_sender_nonce, broadcast_pool, and blobs_bundle_pool, but never from txs_order. When punishment drops ceil(N/2) entries, all those hashes stay in txs_order as stale tombstones. Under sustained spam, txs_order.len() can diverge significantly from transaction_pool.len(), making the prune-threshold check (txs_order.len() > mempool_prune_threshold) fire prematurely on every legitimate insertion and causing remove_oldest_transaction to iterate through many dead entries before finding a live one to evict. This is a pre-existing gap in remove_transaction_with_lock, but the new batch-removal path amplifies it — a single cap-breach event can add up to 8 stale entries at once instead of one.
Prompt To Fix With AI
This is a comment left during a code review.
Path: crates/blockchain/mempool.rs
Line: 195-197
Comment:
`txs_order` not pruned during punishment removals
`remove_transaction_with_lock` removes a hash from `transaction_pool`, `txs_by_sender_nonce`, `broadcast_pool`, and `blobs_bundle_pool`, but never from `txs_order`. When punishment drops `ceil(N/2)` entries, all those hashes stay in `txs_order` as stale tombstones. Under sustained spam, `txs_order.len()` can diverge significantly from `transaction_pool.len()`, making the prune-threshold check (`txs_order.len() > mempool_prune_threshold`) fire prematurely on every legitimate insertion and causing `remove_oldest_transaction` to iterate through many dead entries before finding a live one to evict. This is a pre-existing gap in `remove_transaction_with_lock`, but the new batch-removal path amplifies it — a single cap-breach event can add up to 8 stale entries at once instead of one.
How can I resolve this? If you propose a fix, please make it concise.
🤖 Claude Code ReviewNow I have enough context to write the full review. Review:
|
| Finding | Severity |
|---|---|
? propagation in removal loop — latent wrong-error-type risk if remove_transaction_with_lock gains a failure path |
Low (not a current bug) |
count in error struct is pre-prune |
Nit |
new_top_nonce computed pre-removal needs clarifying comment |
Nit |
| Repeated-punishment behavior undocumented | Nit |
| Blob test doesn't verify versioned-hash secondary map | Nit |
The punish logic, locking model, and test coverage are all correct. The best-effort removal fix (swapping ? for if let Err(e)) is the one change worth making before merge.
Automated review by Claude (Anthropic) · sonnet · custom prompt
🤖 Codex Code ReviewFindings
Aside from those points, the highest-nonce eviction logic itself is straightforward and the new unit coverage for victim cleanup is useful. I couldn’t run Automated review by OpenAI Codex · gpt-5.4 · custom prompt |
…prune count) Phase 2 cross-check produced three confirmed refinements: 1. **Skip prune when count <= 1**. With cap=1 the previous code wiped the sender's only pending tx on every cap-breach attempt — especially harmful under PR #6630's delegated cap=1 where every collision would leave the sender with nothing. Now the prune is gated on `count > 1`. 2. **Single-pass range scan**. The three independent `txs_by_sender_nonce.range(...)` traversals (count + victims + new_top_nonce) collapse into one `.range().rev().collect()`. The victims are taken from the front of the reversed Vec, new_top_nonce from `entries.get(dropped_count)`. 3. **Post-prune count in error**. `MaxPendingTxsPerAccountExceeded.count` now reports the sender's count after the prune (was: pre-prune), matching the actual post-state visible to RPC clients. Two new tests: punish_spammer_skips_prune_when_count_is_one, punish_spammer_reports_post_prune_count.
Phase 2 review (greptile P2): `remove_transaction_with_lock` does not touch `txs_order`. When `punishSpammer` drops `ceil(N/2)` entries under the same write lock, all those hashes stay in `txs_order` as stale tombstones. Under sustained spam, `txs_order.len()` diverges significantly from `transaction_pool.len()`, and the `mempool_prune_threshold` check (which compares `txs_order.len()`) overcounts the pool's apparent fill — possibly triggering the costly lazy-rebuild path more often than needed. After the punishment removals, re-sweep `txs_order` against the live `transaction_pool` once. Scoped to the punishment-fire branch so the common single-add path stays untouched. Existing `punish_spammer_drops_highest_nonce_half_at_cap` test now asserts `txs_order.len() == transaction_pool.len()` after punishment.
…ool.punish-spammer CI's CLI help consistency check expected this line after the [default: true] entry. Regenerating with `cargo run --bin ethrex -- --help` now produces it because clap emits possible values for bool args by default.
When this branch added max_pending_txs_per_account and punish_spammer 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 spammer-cap behavior, so pass usize::MAX / false to opt out.
1 participant
Motivation
When a sender exceeds the per-sender pending-tx cap (#6603), the default behavior is to reject only the new transaction. A persistent spammer can keep pinning their existing 16 (or whatever the cap is) pool slots indefinitely, paying nothing while denying other senders the budget. The geth/erigon punishment pattern: on cap breach, ALSO drop the breaching sender's highest-nonce half of pool entries.
Description
Mempool::add_transactionaccepts apunish_spammer: boolflag. When set AND the sender's pending count is>= max_pending_txs_per_accountAND the count is> 1, the per-sender entries are sorted by nonce descending and the topceil(N/2)are removed viaremove_transaction_with_lock. The new incoming tx is still rejected withMaxPendingTxsPerAccountExceeded { count: post_prune_count, limit }.<= 1so a tighter cap (e.g. delegated_sender_cap = 1 from feat(l1): cap 7702-delegated EOAs at 1 in-flight mempool tx #6630) doesn't make every collision wipe the sender's lone tx.txs_orderagainst the livetransaction_poolso dropped hashes don't leave tombstones that skew the lazymempool_prune_thresholdaccounting.--mempool.punish-spammer(envETHREX_MEMPOOL_PUNISH_SPAMMER, defaulttrue). Operators who want pure-reject (no eviction) semantics can disable it.Behavioral change
Senders that hit the per-account cap are penalised: they lose their upper-half pool entries on every cap-breach attempt, in addition to having the new tx rejected. Default-on for all networks; operators can opt out via the CLI flag.
Notes
Stacked on top of
feat/mempool-account-slots-cap(PR #6603) because the atomic cap check it extends only exists on that branch. The PR diff shows only the punishment-specific delta.