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

Author: ilitteri

crates/blockchain/blockchain.rs

@@ -2975,6 +2975,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 {
@@ -3021,10 +3027,6 @@ impl Blockchain {
                 }
             }
 
-            let tx_cost = tx
-                .cost_without_base_fee()
-                .ok_or(MempoolError::InvalidTxGasvalues)?;
-
             if tx_cost > sender_acc_info.balance {
                 return Err(MempoolError::NotEnoughBalance);
             }
@@ -3042,20 +3044,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,

crates/blockchain/mempool.rs

@@ -625,31 +625,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<U256, MempoolError> {
+    /// 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<H256>,
+    ) -> Result<U256, MempoolError> {
         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)
     }
@@ -871,7 +886,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());
     }
 
@@ -892,7 +907,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);
     }
 
@@ -910,7 +925,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);
     }
 
@@ -928,7 +943,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);
     }
 }