From f062e08d0e46d3929c5a1e9694ac082be59b49a9 Mon Sep 17 00:00:00 2001 From: ilitteri Date: Mon, 11 May 2026 23:19:03 -0300 Subject: [PATCH 1/2] feat(l1): add cumulative balance check across sender pending txs 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. --- crates/blockchain/blockchain.rs | 30 +++++++- crates/blockchain/error.rs | 4 +- crates/blockchain/mempool.rs | 123 ++++++++++++++++++++++++++++++++ 3 files changed, 154 insertions(+), 3 deletions(-) diff --git a/crates/blockchain/blockchain.rs b/crates/blockchain/blockchain.rs index 98ebe601fd1..d8873ddbbf9 100644 --- a/crates/blockchain/blockchain.rs +++ b/crates/blockchain/blockchain.rs @@ -2486,7 +2486,7 @@ impl Blockchain { let maybe_sender_acc_info = self.storage.get_account_info(header_no, sender).await?; - if let Some(sender_acc_info) = maybe_sender_acc_info { + let sender_balance = if let Some(sender_acc_info) = maybe_sender_acc_info { if nonce < sender_acc_info.nonce || nonce == u64::MAX { return Err(MempoolError::NonceTooLow); } @@ -2498,15 +2498,41 @@ impl Blockchain { if tx_cost > sender_acc_info.balance { return Err(MempoolError::NotEnoughBalance); } + + sender_acc_info.balance } else { // An account that is not in the database cannot possibly have enough balance to cover the transaction cost return Err(MempoolError::NotEnoughBalance); - } + }; // Check the nonce of pendings TXs in the mempool from the same sender // If it exists check if the new tx has higher fees let tx_to_replace_hash = self.mempool.find_tx_to_replace(sender, nonce, tx)?; + // Cumulative balance check across this sender's pending transactions. + // Without this, a sender at the per-sender slot cap can have only one + // of their N pending txs be fundable, with the other N-1 being + // guaranteed-fail spam wasting pool space. For replacements at the same + // (sender, nonce), the old tx's cost is subtracted from the running + // total so we don't double-count it. + 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, + }); + } + if tx .chain_id() .is_some_and(|chain_id| chain_id != config.chain_id) diff --git a/crates/blockchain/error.rs b/crates/blockchain/error.rs index 39436472dd4..00c9aef8fd0 100644 --- a/crates/blockchain/error.rs +++ b/crates/blockchain/error.rs @@ -1,5 +1,5 @@ use ethrex_common::{ - H256, + H256, U256, types::{BlobsBundleError, BlockHash}, }; use ethrex_rlp::error::RLPDecodeError; @@ -107,6 +107,8 @@ pub enum MempoolError { InvalidChainId(u64), #[error("Account does not have enough balance to cover the tx cost")] NotEnoughBalance, + #[error("Sender's cumulative pending-tx cost ({required}) exceeds balance ({available})")] + InsufficientCumulativeBalance { required: U256, available: U256 }, #[error("Transaction gas fields are invalid")] InvalidTxGasvalues, #[error("Invalid pooled TxType, expected: {0}")] diff --git a/crates/blockchain/mempool.rs b/crates/blockchain/mempool.rs index bdc31f8aaac..67f395dcce8 100644 --- a/crates/blockchain/mempool.rs +++ b/crates/blockchain/mempool.rs @@ -364,6 +364,35 @@ impl Mempool { .map(|((_address, nonce), _hash)| nonce + 1)) } + /// Returns the saturating sum of `cost_without_base_fee()` for every + /// pending transaction from `sender` currently in the pool. + /// + /// Used at mempool admission to gate a sender's cumulative pending cost + /// against their on-chain balance: without this check, a sender at the + /// per-sender slot cap can have most of their pending txs be + /// guaranteed-fail at execution time and waste pool space. + /// + /// Any tx whose `cost_without_base_fee()` returns `None` (malformed gas + /// fields) is treated as `U256::MAX`, which forces the caller's cumulative + /// check to fail closed. This is conservative and intentional: such a tx + /// should never have been admitted, and biasing toward rejection here is + /// safer than silently undercounting. + pub fn sum_cost_for_sender(&self, sender: Address) -> Result { + let inner = self.read()?; + let mut total = U256::zero(); + for (_key, hash) in inner + .txs_by_sender_nonce + .range((sender, 0)..=(sender, u64::MAX)) + { + let Some(tx) = inner.transaction_pool.get(hash) else { + continue; + }; + let cost = tx.cost_without_base_fee().unwrap_or(U256::MAX); + total = total.saturating_add(cost); + } + Ok(total) + } + pub fn get_mempool_size(&self) -> Result<(u64, u64), MempoolError> { let txs_size = { let pool_lock = &self.read()?.transaction_pool; @@ -574,3 +603,97 @@ pub fn transaction_intrinsic_gas( Ok(gas) } + +#[cfg(test)] +mod tests { + use super::*; + use ethrex_common::types::{EIP1559Transaction, TxKind}; + + const MEMPOOL_MAX_SIZE_TEST: usize = 10_000; + + fn build_tx(nonce: u64, max_fee_per_gas: u64, gas_limit: u64, value: U256) -> Transaction { + Transaction::EIP1559Transaction(EIP1559Transaction { + nonce, + max_priority_fee_per_gas: 0, + max_fee_per_gas, + gas_limit, + to: TxKind::Call(Address::from_low_u64_be(1)), + value, + ..Default::default() + }) + } + + fn insert_tx(mempool: &Mempool, sender: Address, tx: Transaction) -> H256 { + let hash = H256::random(); + mempool + .add_transaction(hash, sender, MempoolTransaction::new(tx, sender)) + .expect("add_transaction"); + hash + } + + #[test] + fn sum_cost_for_sender_empty_pool_is_zero() { + let mempool = Mempool::new(MEMPOOL_MAX_SIZE_TEST); + let sender = Address::random(); + + let total = mempool.sum_cost_for_sender(sender).expect("sum"); + assert_eq!(total, U256::zero()); + } + + #[test] + fn sum_cost_for_sender_sums_multiple_nonces() { + let mempool = Mempool::new(MEMPOOL_MAX_SIZE_TEST); + let sender = Address::random(); + + let tx0 = build_tx(0, 10, 21_000, U256::from(100u64)); + let tx1 = build_tx(1, 20, 21_000, U256::from(200u64)); + let tx2 = build_tx(2, 30, 21_000, U256::from(300u64)); + + let expected = tx0.cost_without_base_fee().unwrap() + + tx1.cost_without_base_fee().unwrap() + + tx2.cost_without_base_fee().unwrap(); + + insert_tx(&mempool, sender, tx0); + insert_tx(&mempool, sender, tx1); + insert_tx(&mempool, sender, tx2); + + let total = mempool.sum_cost_for_sender(sender).expect("sum"); + assert_eq!(total, expected); + } + + #[test] + fn sum_cost_for_sender_ignores_other_senders() { + let mempool = Mempool::new(MEMPOOL_MAX_SIZE_TEST); + let sender_a = Address::random(); + let sender_b = Address::random(); + + let tx_a = build_tx(0, 10, 21_000, U256::from(100u64)); + let tx_b = build_tx(0, 50, 21_000, U256::from(999u64)); + + let expected_a = tx_a.cost_without_base_fee().unwrap(); + + insert_tx(&mempool, sender_a, tx_a); + insert_tx(&mempool, sender_b, tx_b); + + let total_a = mempool.sum_cost_for_sender(sender_a).expect("sum a"); + assert_eq!(total_a, expected_a); + } + + #[test] + fn sum_cost_for_sender_after_remove_drops_that_tx() { + let mempool = Mempool::new(MEMPOOL_MAX_SIZE_TEST); + let sender = Address::random(); + + let tx0 = build_tx(0, 10, 21_000, U256::from(100u64)); + let tx1 = build_tx(1, 10, 21_000, U256::from(200u64)); + let expected_after = tx1.cost_without_base_fee().unwrap(); + + let hash0 = insert_tx(&mempool, sender, tx0); + insert_tx(&mempool, sender, tx1); + + mempool.remove_transaction(&hash0).expect("remove"); + + let total = mempool.sum_cost_for_sender(sender).expect("sum"); + assert_eq!(total, expected_after); + } +} From e999ff92be2387074929a412f14ef256771b750f Mon Sep 17 00:00:00 2001 From: ilitteri Date: Tue, 12 May 2026 15:09:34 -0300 Subject: [PATCH 2/2] fix(l1): make cumulative balance check fail closed and exclude replaced 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` 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. --- crates/blockchain/blockchain.rs | 35 ++++++++++++------------ crates/blockchain/mempool.rs | 47 ++++++++++++++++++++++----------- 2 files changed, 49 insertions(+), 33 deletions(-) diff --git a/crates/blockchain/blockchain.rs b/crates/blockchain/blockchain.rs index d8873ddbbf9..78f6b4c0800 100644 --- a/crates/blockchain/blockchain.rs +++ b/crates/blockchain/blockchain.rs @@ -2484,6 +2484,12 @@ impl Blockchain { } }; + // Compute the new tx's cost once and reuse for both the single-tx + // balance check and the cumulative-balance check below. + let tx_cost = tx + .cost_without_base_fee() + .ok_or(MempoolError::InvalidTxGasvalues)?; + let maybe_sender_acc_info = self.storage.get_account_info(header_no, sender).await?; let sender_balance = if let Some(sender_acc_info) = maybe_sender_acc_info { @@ -2491,10 +2497,6 @@ impl Blockchain { return Err(MempoolError::NonceTooLow); } - let tx_cost = tx - .cost_without_base_fee() - .ok_or(MempoolError::InvalidTxGasvalues)?; - if tx_cost > sender_acc_info.balance { return Err(MempoolError::NotEnoughBalance); } @@ -2512,20 +2514,19 @@ impl Blockchain { // Cumulative balance check across this sender's pending transactions. // Without this, a sender at the per-sender slot cap can have only one // of their N pending txs be fundable, with the other N-1 being - // guaranteed-fail spam wasting pool space. For replacements at the same - // (sender, nonce), the old tx's cost is subtracted from the running - // total so we don't double-count it. - 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() + // guaranteed-fail spam wasting pool space. + // + // `sum_cost_for_sender` recomputes the sender total excluding the + // tx being replaced (instead of subtracting after the fact) so a + // `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 + .mempool + .sum_cost_for_sender(sender, tx_to_replace_hash)?; + let total = existing_cost + .checked_add(tx_cost) .ok_or(MempoolError::InvalidTxGasvalues)?; - let total = existing_cost.saturating_add(new_cost); if total > sender_balance { return Err(MempoolError::InsufficientCumulativeBalance { required: total, diff --git a/crates/blockchain/mempool.rs b/crates/blockchain/mempool.rs index 67f395dcce8..7847a7fb499 100644 --- a/crates/blockchain/mempool.rs +++ b/crates/blockchain/mempool.rs @@ -364,31 +364,46 @@ impl Mempool { .map(|((_address, nonce), _hash)| nonce + 1)) } - /// Returns the saturating sum of `cost_without_base_fee()` for every - /// pending transaction from `sender` currently in the pool. + /// Returns the sum of `cost_without_base_fee()` for every pending + /// transaction from `sender` currently in the pool, optionally excluding + /// `exclude` (used by the cumulative-balance admission gate to drop the + /// cost of a tx that's about to be replaced at the same nonce). /// /// Used at mempool admission to gate a sender's cumulative pending cost /// against their on-chain balance: without this check, a sender at the /// per-sender slot cap can have most of their pending txs be /// guaranteed-fail at execution time and waste pool space. /// - /// Any tx whose `cost_without_base_fee()` returns `None` (malformed gas - /// fields) is treated as `U256::MAX`, which forces the caller's cumulative - /// check to fail closed. This is conservative and intentional: such a tx - /// should never have been admitted, and biasing toward rejection here is - /// safer than silently undercounting. - pub fn sum_cost_for_sender(&self, sender: Address) -> Result { + /// Fails closed on any inconsistency: if `txs_by_sender_nonce` references + /// a hash missing from `transaction_pool`, or if any included tx's cost + /// can't be computed, the function returns an error rather than silently + /// undercounting (which would let a malformed or invariant-violating tx + /// bypass the cumulative check). + pub fn sum_cost_for_sender( + &self, + sender: Address, + exclude: Option, + ) -> Result { let inner = self.read()?; let mut total = U256::zero(); for (_key, hash) in inner .txs_by_sender_nonce .range((sender, 0)..=(sender, u64::MAX)) { - let Some(tx) = inner.transaction_pool.get(hash) else { + if Some(*hash) == exclude { continue; - }; - let cost = tx.cost_without_base_fee().unwrap_or(U256::MAX); - total = total.saturating_add(cost); + } + let tx = inner.transaction_pool.get(hash).ok_or_else(|| { + MempoolError::StoreError(StoreError::Custom(format!( + "mempool index/pool inconsistency: hash {hash:?} in sender-nonce index but missing from transaction_pool", + ))) + })?; + let cost = tx + .cost_without_base_fee() + .ok_or(MempoolError::InvalidTxGasvalues)?; + total = total + .checked_add(cost) + .ok_or(MempoolError::InvalidTxGasvalues)?; } Ok(total) } @@ -636,7 +651,7 @@ mod tests { let mempool = Mempool::new(MEMPOOL_MAX_SIZE_TEST); let sender = Address::random(); - let total = mempool.sum_cost_for_sender(sender).expect("sum"); + let total = mempool.sum_cost_for_sender(sender, None).expect("sum"); assert_eq!(total, U256::zero()); } @@ -657,7 +672,7 @@ mod tests { insert_tx(&mempool, sender, tx1); insert_tx(&mempool, sender, tx2); - let total = mempool.sum_cost_for_sender(sender).expect("sum"); + let total = mempool.sum_cost_for_sender(sender, None).expect("sum"); assert_eq!(total, expected); } @@ -675,7 +690,7 @@ mod tests { insert_tx(&mempool, sender_a, tx_a); insert_tx(&mempool, sender_b, tx_b); - let total_a = mempool.sum_cost_for_sender(sender_a).expect("sum a"); + let total_a = mempool.sum_cost_for_sender(sender_a, None).expect("sum a"); assert_eq!(total_a, expected_a); } @@ -693,7 +708,7 @@ mod tests { mempool.remove_transaction(&hash0).expect("remove"); - let total = mempool.sum_cost_for_sender(sender).expect("sum"); + let total = mempool.sum_cost_for_sender(sender, None).expect("sum"); assert_eq!(total, expected_after); } }