feat(l1): add cumulative balance check across sender pending txs#6606
feat(l1): add cumulative balance check across sender pending txs#6606ilitteri wants to merge 2 commits into
Conversation
Reject mempool admission when a sender's total cost (gas + value + blob
gas) summed across their pending pool entries plus the new tx exceeds
the sender's on-chain balance. Without this gate, a sender at the
per-sender slot cap can have only one of N pending txs be fundable
while the other N-1 are guaranteed-fail at execution and just waste
pool slots until eviction. All major peer EL clients enforce this; this
brings ethrex to parity.
Adds `Mempool::sum_cost_for_sender`, which ranges the existing
`txs_by_sender_nonce` index for the sender and saturating-sums each
tx's `cost_without_base_fee()`. A malformed tx returning `None` is
counted as `U256::MAX` so the cumulative check fails closed.
In `Blockchain::validate_transaction`, the new check runs after
`find_tx_to_replace`. For a replacement at an existing (sender, nonce)
the old tx's cost is subtracted from the running total before adding
the new tx's cost, so replacements don't double-count.
Introduces `MempoolError::InsufficientCumulativeBalance { required,
available }` with a descriptive message.
Unit tests in `crates/blockchain/mempool.rs` cover the helper: empty
pool returns zero, sums across multiple nonces of the same sender,
ignores other senders, and reflects removals (the underlying mechanism
for replacement subtraction). Integration tests through
`validate_transaction` that exercise the new branch require seeding
sender account state, which the existing test pattern in
`test/tests/blockchain/mempool_tests.rs` does not support; the helper
unit tests plus the small, reviewable wiring in `validate_transaction`
cover the new logic.
🤖 Kimi Code ReviewThe PR implements a cumulative balance check for mempool transaction admission—a critical anti-DoS measure. The logic is sound and the implementation follows Rust best practices. Correctness & Security
Code Quality
Potential Issues
Testing
Nitpick
Verdict: LGTM. The implementation correctly prevents "guaranteed-fail" mempool spam while handling replacements atomically with respect to cost calculation. Automated review by Kimi (Moonshot AI) · kimi-k2.5 · custom prompt |
🤖 Claude Code ReviewNow I have all the context needed to write the review. PR #6606 —
|
Lines of code reportTotal lines added: Detailed view |
There was a problem hiding this comment.
Pull request overview
This PR adds a cumulative balance check during mempool admission to prevent a sender from filling their per-sender pending slots with transactions that are individually valid but collectively unfundable, aligning ethrex behavior with other major execution clients.
Changes:
- Added
Mempool::sum_cost_for_senderto compute a sender’s total pending transaction cost (saturating sum ofcost_without_base_fee()). - Extended
Blockchain::validate_transactionto reject admission when(existing_pending_cost + new_tx_cost) > sender_balance, with special handling for same-(sender, nonce) replacements. - Introduced
MempoolError::InsufficientCumulativeBalance { required, available }for a descriptive failure reason, plus unit tests for the helper.
Reviewed changes
Copilot reviewed 3 out of 3 changed files in this pull request and generated 3 comments.
| File | Description |
|---|---|
| crates/blockchain/mempool.rs | Adds a per-sender cumulative cost helper and unit tests. |
| crates/blockchain/error.rs | Adds a new mempool error variant for cumulative-balance rejection. |
| crates/blockchain/blockchain.rs | Enforces cumulative pending-cost vs. on-chain balance during tx validation (including replacement handling). |
💡 Add Copilot custom instructions for smarter, more guided reviews. Learn how to get started.
| let Some(tx) = inner.transaction_pool.get(hash) else { | ||
| continue; | ||
| }; |
| let mut existing_cost = self.mempool.sum_cost_for_sender(sender)?; | ||
| if let Some(replace_hash) = tx_to_replace_hash | ||
| && let Some(old_tx) = self.mempool.get_transaction_by_hash(replace_hash)? | ||
| { | ||
| let old_cost = old_tx.cost_without_base_fee().unwrap_or(U256::MAX); | ||
| existing_cost = existing_cost.saturating_sub(old_cost); | ||
| } |
| let new_cost = tx | ||
| .cost_without_base_fee() | ||
| .ok_or(MempoolError::InvalidTxGasvalues)?; | ||
| let total = existing_cost.saturating_add(new_cost); | ||
| if total > sender_balance { | ||
| return Err(MempoolError::InsufficientCumulativeBalance { | ||
| required: total, | ||
| available: sender_balance, | ||
| }); | ||
| } |
🤖 Codex Code ReviewFindings
I couldn’t run Automated review by OpenAI Codex · gpt-5.4 · custom prompt |
Greptile SummaryThis PR adds a cumulative balance check at mempool admission time, preventing a sender from filling their per-sender slot cap with transactions whose aggregate cost exceeds their on-chain balance — matching the behaviour of geth, reth, nethermind, and erigon.
Confidence Score: 4/5Safe to merge; the new check is additive and conservative, and the core replacement-subtraction arithmetic is correct for all normal inputs. The implementation is clean and the saturation arithmetic correctly handles the common cases. The one non-trivial edge — where a The replacement-subtraction block in
|
| Filename | Overview |
|---|---|
| crates/blockchain/blockchain.rs | Adds cumulative balance check in validate_transaction after find_tx_to_replace; uses correct saturation arithmetic and replacement deduction, but calls cost_without_base_fee() twice on the same immutable tx. |
| crates/blockchain/mempool.rs | Adds sum_cost_for_sender using BTreeMap range over txs_by_sender_nonce; correctly uses saturation arithmetic and treats None-cost txs as U256::MAX; unit tests cover the four key paths. |
| crates/blockchain/error.rs | Adds InsufficientCumulativeBalance { required, available } variant to MempoolError with a clear error message; adds the necessary U256 import. |
Flowchart
%%{init: {'theme': 'neutral'}}%%
flowchart TD
A[validate_transaction] --> B{Account exists?}
B -- No --> E1[Err: NotEnoughBalance]
B -- Yes --> C{single tx_cost > balance?}
C -- Yes --> E2[Err: NotEnoughBalance]
C -- No --> D[find_tx_to_replace]
D --> F[sum_cost_for_sender]
F --> G{tx_to_replace found in pool?}
G -- Yes --> H[existing_cost -= old_cost]
G -- No --> I[existing_cost unchanged]
H --> J[total = existing_cost + new_cost]
I --> J
J --> K{total > sender_balance?}
K -- Yes --> E3[Err: InsufficientCumulativeBalance]
K -- No --> L[Continue validation...]
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:2525-2527
`cost_without_base_fee()` is called twice on the same immutable `tx` — once inside the `if-let` block (stored as `tx_cost`) and again here as `new_cost`. Since `tx` is immutable and the function is deterministic, the two values are always equal. The computation is cheap, but the double call is confusing and invites divergence if the two sites are ever edited separately. `tx_cost` could be extracted from the `if-let` block's return value so it is available here (similar to how `sender_balance` is threaded out of the block).
```suggestion
// `tx_cost` was already validated above; reuse it here to avoid
// a redundant call to cost_without_base_fee().
let new_cost = tx_cost;
```
### Issue 2 of 2
crates/blockchain/blockchain.rs:2519-2523
**Saturation arithmetic loses sibling-tx costs when replacing a `None`-cost tx**
When a tx whose `cost_without_base_fee()` returns `None` is somehow in the pool (e.g. from a pool inconsistency), `sum_cost_for_sender` saturates to `U256::MAX`. If that very tx is the one being replaced, `old_cost` is also `U256::MAX`, and `U256::MAX.saturating_sub(U256::MAX) = 0` — silently erasing every other sibling tx's contribution. The cumulative check then compares only `new_cost` against `sender_balance`, potentially admitting a tx whose true total (sibling costs + new_cost) exceeds balance.
Per the PR's own invariant these txs shouldn't reach the pool, but if one does, the replacement path becomes the one code path where the fail-closed guarantee breaks. A targeted comment here, or an assertion that `old_cost != U256::MAX`, would make the assumption explicit.
Reviews (1): Last reviewed commit: "feat(l1): add cumulative balance check a..." | Re-trigger Greptile
![greptile-apps[bot]](https://avatars.githubusercontent.com/in/867647?s=60&v=4){kind=small}
| let new_cost = tx | ||
| .cost_without_base_fee() | ||
| .ok_or(MempoolError::InvalidTxGasvalues)?; |
There was a problem hiding this comment.
cost_without_base_fee() is called twice on the same immutable tx — once inside the if-let block (stored as tx_cost) and again here as new_cost. Since tx is immutable and the function is deterministic, the two values are always equal. The computation is cheap, but the double call is confusing and invites divergence if the two sites are ever edited separately. tx_cost could be extracted from the if-let block's return value so it is available here (similar to how sender_balance is threaded out of the block).
| let new_cost = tx | |
| .cost_without_base_fee() | |
| .ok_or(MempoolError::InvalidTxGasvalues)?; | |
| // `tx_cost` was already validated above; reuse it here to avoid | |
| // a redundant call to cost_without_base_fee(). | |
| let new_cost = tx_cost; |
Prompt To Fix With AI
This is a comment left during a code review.
Path: crates/blockchain/blockchain.rs
Line: 2525-2527
Comment:
`cost_without_base_fee()` is called twice on the same immutable `tx` — once inside the `if-let` block (stored as `tx_cost`) and again here as `new_cost`. Since `tx` is immutable and the function is deterministic, the two values are always equal. The computation is cheap, but the double call is confusing and invites divergence if the two sites are ever edited separately. `tx_cost` could be extracted from the `if-let` block's return value so it is available here (similar to how `sender_balance` is threaded out of the block).
```suggestion
// `tx_cost` was already validated above; reuse it here to avoid
// a redundant call to cost_without_base_fee().
let new_cost = tx_cost;
```
How can I resolve this? If you propose a fix, please make it concise.| if let Some(replace_hash) = tx_to_replace_hash | ||
| && let Some(old_tx) = self.mempool.get_transaction_by_hash(replace_hash)? | ||
| { | ||
| let old_cost = old_tx.cost_without_base_fee().unwrap_or(U256::MAX); | ||
| existing_cost = existing_cost.saturating_sub(old_cost); |
There was a problem hiding this comment.
None-cost tx
When a tx whose cost_without_base_fee() returns None is somehow in the pool (e.g. from a pool inconsistency), sum_cost_for_sender saturates to U256::MAX. If that very tx is the one being replaced, old_cost is also U256::MAX, and U256::MAX.saturating_sub(U256::MAX) = 0 — silently erasing every other sibling tx's contribution. The cumulative check then compares only new_cost against sender_balance, potentially admitting a tx whose true total (sibling costs + new_cost) exceeds balance.
Per the PR's own invariant these txs shouldn't reach the pool, but if one does, the replacement path becomes the one code path where the fail-closed guarantee breaks. A targeted comment here, or an assertion that old_cost != U256::MAX, would make the assumption explicit.
Prompt To Fix With AI
This is a comment left during a code review.
Path: crates/blockchain/blockchain.rs
Line: 2519-2523
Comment:
**Saturation arithmetic loses sibling-tx costs when replacing a `None`-cost tx**
When a tx whose `cost_without_base_fee()` returns `None` is somehow in the pool (e.g. from a pool inconsistency), `sum_cost_for_sender` saturates to `U256::MAX`. If that very tx is the one being replaced, `old_cost` is also `U256::MAX`, and `U256::MAX.saturating_sub(U256::MAX) = 0` — silently erasing every other sibling tx's contribution. The cumulative check then compares only `new_cost` against `sender_balance`, potentially admitting a tx whose true total (sibling costs + new_cost) exceeds balance.
Per the PR's own invariant these txs shouldn't reach the pool, but if one does, the replacement path becomes the one code path where the fail-closed guarantee breaks. A targeted comment here, or an assertion that `old_cost != U256::MAX`, would make the assumption explicit.
How can I resolve this? If you propose a fix, please make it concise.…ed tx cleanly Phase 2 review (Copilot + greptile) flagged four related issues with the saturating arithmetic in the cumulative-balance gate: 1. `sum_cost_for_sender` silently skipped entries when `txs_by_sender_nonce` pointed to a hash missing from `transaction_pool`, undercounting. 2. The replacement adjustment `saturating_sub(old_cost)` on a saturated sum could drop the total to 0 and admit a tx that should be rejected. 3. When a tx with `cost_without_base_fee() = None` was both summed (as MAX) AND replaced (`old_cost` also MAX), `MAX - MAX = 0` erased every sibling tx's contribution. 4. `cost_without_base_fee()` was called twice on the same `tx` (once for the single-tx balance check, once as `new_cost`), inviting drift. Single rewrite addresses all four: - `sum_cost_for_sender` now takes an `exclude: Option<H256>` so the caller can drop the replaced tx's contribution at scan time instead of via a non-invertible `saturating_sub` afterward. - Index/pool inconsistency and `None`-cost txs return `MempoolError` rather than silently saturating, so the gate can't be bypassed by an invariant violation. - `checked_add` instead of `saturating_add` for the running total. - `tx_cost` in `validate_transaction` is computed once and used for both the single-tx and cumulative checks. Existing unit tests updated to the new `(sender, None)` signature.
| // `None`-cost or missing tx can't silently zero the total via | ||
| // `MAX - MAX = 0`. It also fails closed on any inconsistency so the | ||
| // gate can't be bypassed by an invariant violation. | ||
| let existing_cost = self |
There was a problem hiding this comment.
TOCTOU between this cumulative check (mempool read lock) and the caller's later add_transaction/add_transaction_no_broadcast (mempool write lock). Two concurrent submissions from the same sender can both pass the check independently with stale existing_cost, then both insert, ending up above the cumulative budget.
Concrete scenario: sender balance = 10. Existing pending costs = 5 (one tx). Two parallel RPC submissions of cost 3 each arrive on different worker tasks.
- Task A:
sum_cost_for_sender→ 5,+3 = 8 ≤ 10→ pass → write-lock-insert. - Task B (concurrent):
sum_cost_for_sender→ 5 (still, A hasn't inserted yet),+3 = 8 ≤ 10→ pass → write-lock-insert. - Final state: 5 + 3 + 3 = 11 > 10. Cumulative budget exceeded.
The existing single-tx balance check at :2500 has the same shape against the on-chain balance, so this isn't a new class of bug — but the cumulative gate's whole purpose is rate-limiting one sender, which is exactly the threat model where parallel-from-same-sender matters. Two ways to close it:
- Per-sender admission mutex (cheap; only contended for adversarial senders).
- Move the cumulative check inside the write-lock-holding
add_transaction_inner— re-dosum_cost_for_senderafter acquiring the write lock. Slightly more lock time but no races.
Not blocking — the window is small and only adversarial parallel submission exploits it, but worth noting for follow-up.
Motivation
A sender at the per-sender slot cap (#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
Mempool::sum_cost_for_sender(sender, exclude: Option<H256>) -> Result<U256, MempoolError>rangestxs_by_sender_noncefor the sender and sumscost_without_base_fee()per pending tx. Theexcludeparameter 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_transactioncomputes the new tx's cost ONCE, performs the single-tx balance check, thenexisting_cost = sum_cost_for_sender(sender, tx_to_replace_hash)?and rejects withMempoolError::InsufficientCumulativeBalance { required, available }whenexisting_cost + tx_cost > sender_balance.txs_by_sender_noncereferences a hash missing fromtransaction_pool, or if any included tx's cost can't be computed,sum_cost_for_senderreturns an error rather than silently undercounting. The gate cannot be bypassed by an invariant violation.checked_addrather thansaturating_addfor the running total so overflow surfaces asInvalidTxGasvaluesrather 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
InsufficientCumulativeBalancerequire seeded sender-account fixtures not currently present intest/tests/blockchain/mempool_tests.rs. Same pattern as #6603 / #6576 / #6600 deferrals.