-
Notifications
You must be signed in to change notification settings - Fork 201
feat(l1): add cumulative balance check across sender pending txs #6606
New issue
Have a question about this project? Sign up for a free GitHub account to open an issue and contact its maintainers and the community.
By clicking “Sign up for GitHub”, you agree to our terms of service and privacy statement. We’ll occasionally send you account related emails.
Already on GitHub? Sign in to your account
base: main
Are you sure you want to change the base?
Changes from 1 commit
File filter
Filter by extension
Conversations
Jump to
Diff view
Diff view
There are no files selected for viewing
| Original file line number | Diff line number | Diff line change | ||||||||||||
|---|---|---|---|---|---|---|---|---|---|---|---|---|---|---|
|
|
@@ -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); | ||||||||||||||
| } | ||||||||||||||
|
Collaborator
Author
There was a problem hiding this comment. Choose a reason for hiding this commentThe reason will be displayed to describe this comment to others. Learn more. |
||||||||||||||
| let new_cost = tx | ||||||||||||||
| .cost_without_base_fee() | ||||||||||||||
| .ok_or(MempoolError::InvalidTxGasvalues)?; | ||||||||||||||
|
There was a problem hiding this comment. Choose a reason for hiding this commentThe reason will be displayed to describe this comment to others. Learn more.
Suggested change
Prompt To Fix With AIThis 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.
Collaborator
Author
There was a problem hiding this comment. Choose a reason for hiding this commentThe reason will be displayed to describe this comment to others. Learn more. |
||||||||||||||
| 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) | ||||||||||||||
|
|
||||||||||||||
| Original file line number | Diff line number | Diff line change |
|---|---|---|
|
|
@@ -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<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 { | ||
| continue; | ||
| }; | ||
|
Collaborator
Author
There was a problem hiding this comment. Choose a reason for hiding this commentThe reason will be displayed to describe this comment to others. Learn more. |
||
| 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); | ||
| } | ||
| } | ||
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
None-cost txWhen a tx whose
cost_without_base_fee()returnsNoneis somehow in the pool (e.g. from a pool inconsistency),sum_cost_for_sendersaturates toU256::MAX. If that very tx is the one being replaced,old_costis alsoU256::MAX, andU256::MAX.saturating_sub(U256::MAX) = 0— silently erasing every other sibling tx's contribution. The cumulative check then compares onlynew_costagainstsender_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
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
e999ff9