Skip to content
Open
Show file tree
Hide file tree
Changes from all commits
Commits
File filter

Filter by extension

Filter by extension

Conversations
Failed to load comments.
Loading
Jump to
Jump to file
Failed to load files.
Loading
Diff view
Diff view
39 changes: 33 additions & 6 deletions crates/blockchain/blockchain.rs
Original file line number Diff line number Diff line change
Expand Up @@ -2484,29 +2484,56 @@ 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?;

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);
}

let tx_cost = tx
.cost_without_base_fee()
.ok_or(MempoolError::InvalidTxGasvalues)?;

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.
//
// `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
Copy link
Copy Markdown
Contributor

Choose a reason for hiding this comment

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

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:

  1. Per-sender admission mutex (cheap; only contended for adversarial senders).
  2. Move the cumulative check inside the write-lock-holding add_transaction_inner — re-do sum_cost_for_sender after 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.

.mempool
.sum_cost_for_sender(sender, tx_to_replace_hash)?;
let total = existing_cost
.checked_add(tx_cost)
.ok_or(MempoolError::InvalidTxGasvalues)?;
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)
Expand Down
4 changes: 3 additions & 1 deletion crates/blockchain/error.rs
Original file line number Diff line number Diff line change
@@ -1,5 +1,5 @@
use ethrex_common::{
H256,
H256, U256,
types::{BlobsBundleError, BlockHash},
};
use ethrex_rlp::error::RLPDecodeError;
Expand Down Expand Up @@ -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}")]
Expand Down
138 changes: 138 additions & 0 deletions crates/blockchain/mempool.rs
Original file line number Diff line number Diff line change
Expand Up @@ -364,6 +364,50 @@ impl Mempool {
.map(|((_address, nonce), _hash)| nonce + 1))
}

/// 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.
///
/// 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))
{
if Some(*hash) == exclude {
continue;
}
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)
}

pub fn get_mempool_size(&self) -> Result<(u64, u64), MempoolError> {
let txs_size = {
let pool_lock = &self.read()?.transaction_pool;
Expand Down Expand Up @@ -574,3 +618,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, None).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, None).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, None).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, None).expect("sum");
assert_eq!(total, expected_after);
}
}
Loading