-
Notifications
You must be signed in to change notification settings - Fork 201
feat(l1): require percentage fee bump for mempool RBF (10%/100%) #6601
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
59a8725
02668cb
f903683
7283c27
7c50aac
3bee5a5
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 |
|---|---|---|
|
|
@@ -456,38 +456,65 @@ impl Mempool { | |
| sender: Address, | ||
| nonce: u64, | ||
| tx: &Transaction, | ||
| price_bump_percent: u64, | ||
| blob_price_bump_percent: u64, | ||
| ) -> Result<Option<H256>, MempoolError> { | ||
| let Some(tx_in_pool) = self.contains_sender_nonce(sender, nonce, tx.hash())? else { | ||
| return Ok(None); | ||
| }; | ||
| let is_a_replacement_tx = { | ||
| // EIP-1559 values | ||
| let old_tx_max_fee_per_gas = tx_in_pool.max_fee_per_gas().unwrap_or_default(); | ||
| let old_tx_max_priority_fee_per_gas = tx_in_pool.max_priority_fee().unwrap_or_default(); | ||
| let new_tx_max_fee_per_gas = tx.max_fee_per_gas().unwrap_or_default(); | ||
| let new_tx_max_priority_fee_per_gas = tx.max_priority_fee().unwrap_or_default(); | ||
|
|
||
| // Legacy tx values | ||
| let old_tx_gas_price = tx_in_pool.gas_price(); | ||
| let new_tx_gas_price = tx.gas_price(); | ||
|
|
||
| // EIP-4844 values | ||
| let old_tx_max_fee_per_blob = tx_in_pool.max_fee_per_blob_gas(); | ||
| let new_tx_max_fee_per_blob = tx.max_fee_per_blob_gas(); | ||
|
|
||
| let eip4844_higher_fees = if let (Some(old_blob_fee), Some(new_blob_fee)) = | ||
| (old_tx_max_fee_per_blob, new_tx_max_fee_per_blob) | ||
| { | ||
| new_blob_fee > old_blob_fee | ||
| } else { | ||
| true // We are marking it as always true if the tx is not eip-4844 | ||
| }; | ||
|
|
||
| let eip1559_higher_fees = new_tx_max_fee_per_gas > old_tx_max_fee_per_gas | ||
| && new_tx_max_priority_fee_per_gas > old_tx_max_priority_fee_per_gas; | ||
| let legacy_higher_fees = new_tx_gas_price > old_tx_gas_price; | ||
| // Blob replacements use a stricter bump (default 100%) because blob | ||
| // sidecars are expensive to re-propagate; all other tx types use the | ||
| // base bump (default 10%). | ||
| let bump = if matches!(tx, Transaction::EIP4844Transaction(_)) { | ||
| blob_price_bump_percent | ||
| } else { | ||
| price_bump_percent | ||
| }; | ||
|
|
||
| eip4844_higher_fees && (eip1559_higher_fees || legacy_higher_fees) | ||
| // The new tx must bump every applicable fee field on its own tx type | ||
| // by at least `bump` percent compared to the in-pool tx at the same | ||
| // (sender, nonce). Each peer EL client enforces this independently per | ||
| // field. | ||
| let is_a_replacement_tx = match tx { | ||
| Transaction::LegacyTransaction(_) => { | ||
| is_bumped_u256(tx_in_pool.gas_price(), tx.gas_price(), bump) | ||
| } | ||
| Transaction::EIP4844Transaction(_) => { | ||
| let bumped_fee = is_bumped_u64( | ||
| tx_in_pool.max_fee_per_gas().unwrap_or_default(), | ||
| tx.max_fee_per_gas().unwrap_or_default(), | ||
| bump, | ||
| ); | ||
|
Comment on lines
+494
to
+498
|
||
| let bumped_tip = is_bumped_u64( | ||
| tx_in_pool.max_priority_fee().unwrap_or_default(), | ||
| tx.max_priority_fee().unwrap_or_default(), | ||
| bump, | ||
| ); | ||
| let bumped_blob = is_bumped_u256( | ||
| tx_in_pool.max_fee_per_blob_gas().unwrap_or_default(), | ||
| tx.max_fee_per_blob_gas().unwrap_or_default(), | ||
| bump, | ||
| ); | ||
| bumped_fee && bumped_tip && bumped_blob | ||
| } | ||
| // EIP-2930 / EIP-1559 / EIP-7702 / FeeToken / Privileged: 1559-style | ||
| // pair of fee fields. (Privileged L2 txs bypass admission entirely, | ||
| // so the branch is unreachable in practice; we keep it for | ||
| // exhaustiveness.) | ||
| _ => { | ||
| let bumped_fee = is_bumped_u64( | ||
| tx_in_pool.max_fee_per_gas().unwrap_or_default(), | ||
| tx.max_fee_per_gas().unwrap_or_default(), | ||
| bump, | ||
| ); | ||
| let bumped_tip = is_bumped_u64( | ||
| tx_in_pool.max_priority_fee().unwrap_or_default(), | ||
| tx.max_priority_fee().unwrap_or_default(), | ||
| bump, | ||
| ); | ||
| bumped_fee && bumped_tip | ||
| } | ||
| }; | ||
|
|
||
| if !is_a_replacement_tx { | ||
|
|
@@ -498,6 +525,23 @@ impl Mempool { | |
| } | ||
| } | ||
|
|
||
| /// Returns true iff `new >= floor(existing * (100 + bump_percent) / 100)`, | ||
| /// using saturating arithmetic. A `bump_percent` of 0 collapses to | ||
| /// `new >= existing`. | ||
| fn is_bumped_u64(existing: u64, new: u64, bump_percent: u64) -> bool { | ||
| let multiplier = 100u64.saturating_add(bump_percent); | ||
| let threshold = existing.saturating_mul(multiplier) / 100; | ||
| new >= threshold | ||
| } | ||
|
Comment on lines
+544
to
+550
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.
When Prompt To Fix With AIThis is a comment left during a code review.
Path: crates/blockchain/mempool.rs
Line: 531-535
Comment:
**Saturating multiply silently lowers the required threshold**
When `existing` is large (close to `u64::MAX`) and `bump_percent` is 100, `existing.saturating_mul(200)` saturates to `u64::MAX`, and dividing by 100 yields `≈ u64::MAX / 100`, which is *far below* the mathematically correct threshold of `2 × existing`. The test `is_bumped_u64_saturating_does_not_panic` locks in this incorrect behavior as expected. Fee values this large are impossible on mainnet, but using `u128` intermediate arithmetic would be more correct and clearer.
How can I resolve this? If you propose a fix, please make it concise. |
||
|
|
||
|
|
||
| /// U256 variant of [`is_bumped_u64`]. Used for `gas_price` (legacy) and | ||
| /// `max_fee_per_blob_gas` (EIP-4844). | ||
| fn is_bumped_u256(existing: U256, new: U256, bump_percent: u64) -> bool { | ||
| let multiplier = U256::from(100u64.saturating_add(bump_percent)); | ||
| let threshold = existing.saturating_mul(multiplier) / U256::from(100u64); | ||
| new >= threshold | ||
|
Comment on lines
+555
to
+563
|
||
| } | ||
|
|
||
| /// Filter applied by the payload builder when querying pending transactions | ||
| /// from the pool. NOT a mempool admission gate — all fields here are | ||
| /// query-time filters used to pick block-includable transactions. Admission | ||
|
|
@@ -574,3 +618,167 @@ pub fn transaction_intrinsic_gas( | |
|
|
||
| Ok(gas) | ||
| } | ||
|
|
||
| #[cfg(test)] | ||
| mod tests { | ||
| use super::*; | ||
| use ethrex_common::types::{EIP1559Transaction, EIP4844Transaction}; | ||
|
|
||
| // --- helpers -------------------------------------------------------- | ||
|
|
||
| fn add_to_pool(pool: &Mempool, sender: Address, tx: Transaction) -> H256 { | ||
| let mtx = MempoolTransaction::new(tx, sender); | ||
| let hash = mtx.hash(); | ||
| pool.add_transaction(hash, sender, mtx).unwrap(); | ||
| hash | ||
| } | ||
|
|
||
| fn eip1559(nonce: u64, max_fee: u64, max_priority: u64) -> Transaction { | ||
| Transaction::EIP1559Transaction(EIP1559Transaction { | ||
| nonce, | ||
| max_fee_per_gas: max_fee, | ||
| max_priority_fee_per_gas: max_priority, | ||
| ..Default::default() | ||
| }) | ||
| } | ||
|
|
||
| fn eip4844(nonce: u64, max_fee: u64, max_priority: u64, blob_fee: u64) -> Transaction { | ||
| Transaction::EIP4844Transaction(EIP4844Transaction { | ||
| nonce, | ||
| max_fee_per_gas: max_fee, | ||
| max_priority_fee_per_gas: max_priority, | ||
| max_fee_per_blob_gas: U256::from(blob_fee), | ||
| ..Default::default() | ||
| }) | ||
| } | ||
|
|
||
| // --- is_bumped_u64 ------------------------------------------------- | ||
|
|
||
| #[test] | ||
| fn is_bumped_u64_exact_10_percent_accepted() { | ||
| assert!(is_bumped_u64(100, 110, 10)); | ||
| } | ||
|
|
||
| #[test] | ||
| fn is_bumped_u64_just_below_10_percent_rejected() { | ||
| assert!(!is_bumped_u64(100, 109, 10)); | ||
| } | ||
|
|
||
| #[test] | ||
| fn is_bumped_u64_zero_bump_allows_equal() { | ||
| assert!(is_bumped_u64(100, 100, 0)); | ||
| } | ||
|
|
||
| #[test] | ||
| fn is_bumped_u64_zero_existing_always_accepted() { | ||
| assert!(is_bumped_u64(0, 0, 100)); | ||
| assert!(is_bumped_u64(0, 1, 100)); | ||
| } | ||
|
|
||
| #[test] | ||
| fn is_bumped_u64_saturating_does_not_panic() { | ||
| // Multiplier saturates at u64::MAX; threshold stays bounded. | ||
| assert!(is_bumped_u64(u64::MAX, u64::MAX, 100)); | ||
| } | ||
|
|
||
| // --- is_bumped_u256 ------------------------------------------------ | ||
|
|
||
| #[test] | ||
| fn is_bumped_u256_blob_100_percent_accepted() { | ||
| assert!(is_bumped_u256(U256::from(50u64), U256::from(100u64), 100)); | ||
| } | ||
|
|
||
| #[test] | ||
| fn is_bumped_u256_blob_99_percent_rejected() { | ||
| assert!(!is_bumped_u256(U256::from(100u64), U256::from(199u64), 100)); | ||
| } | ||
|
|
||
| // --- find_tx_to_replace --------------------------------------------- | ||
|
|
||
| #[test] | ||
| fn replacement_1_wei_bump_rejected_at_10_percent() { | ||
| // Spec scenario: "Strict-greater-than but below 10% bump rejected" | ||
| let pool = Mempool::new(64); | ||
| let sender = Address::from_low_u64_be(1); | ||
| let old = eip1559(0, 1_000, 100); | ||
| add_to_pool(&pool, sender, old); | ||
|
|
||
| let new = eip1559(0, 1_001, 100); // +1 wei on max_fee, no bump on tip | ||
| let err = pool | ||
| .find_tx_to_replace(sender, 0, &new, 10, 100) | ||
| .unwrap_err(); | ||
| assert!(matches!(err, MempoolError::UnderpricedReplacement)); | ||
| } | ||
|
|
||
| #[test] | ||
| fn replacement_full_10_percent_bump_on_both_axes_accepted() { | ||
| // Spec scenario: "10% bump on both axes accepted" | ||
| let pool = Mempool::new(64); | ||
| let sender = Address::from_low_u64_be(1); | ||
| let old = eip1559(0, 1_000, 100); | ||
| let old_hash = add_to_pool(&pool, sender, old); | ||
|
|
||
| let new = eip1559(0, 1_100, 110); | ||
| let found = pool | ||
| .find_tx_to_replace(sender, 0, &new, 10, 100) | ||
| .unwrap() | ||
| .expect("replacement should be admitted"); | ||
| assert_eq!(found, old_hash); | ||
| } | ||
|
|
||
| #[test] | ||
| fn replacement_asymmetric_bump_rejected() { | ||
| // Spec scenario: "10% bump on only one axis rejected" | ||
| let pool = Mempool::new(64); | ||
| let sender = Address::from_low_u64_be(1); | ||
| let old = eip1559(0, 1_000, 100); | ||
| add_to_pool(&pool, sender, old); | ||
|
|
||
| let new = eip1559(0, 1_100, 105); // 10% on fee cap, 5% on tip | ||
| let err = pool | ||
| .find_tx_to_replace(sender, 0, &new, 10, 100) | ||
| .unwrap_err(); | ||
| assert!(matches!(err, MempoolError::UnderpricedReplacement)); | ||
| } | ||
|
|
||
| #[test] | ||
| fn blob_replacement_50_percent_bump_rejected() { | ||
| // Spec scenario: "Blob 50% bump rejected" | ||
| let pool = Mempool::new(64); | ||
| let sender = Address::from_low_u64_be(2); | ||
| let old = eip4844(0, 1_000, 100, 50); | ||
| add_to_pool(&pool, sender, old); | ||
|
|
||
| // 50% bump on all three fields, but blob threshold is 100%. | ||
| let new = eip4844(0, 1_500, 150, 75); | ||
| let err = pool | ||
| .find_tx_to_replace(sender, 0, &new, 10, 100) | ||
| .unwrap_err(); | ||
| assert!(matches!(err, MempoolError::UnderpricedReplacement)); | ||
| } | ||
|
|
||
| #[test] | ||
| fn blob_replacement_100_percent_bump_on_three_axes_accepted() { | ||
| // Spec scenario: "Blob 100% bump on three axes accepted" | ||
| let pool = Mempool::new(64); | ||
| let sender = Address::from_low_u64_be(2); | ||
| let old = eip4844(0, 1_000, 100, 50); | ||
| let old_hash = add_to_pool(&pool, sender, old); | ||
|
|
||
| let new = eip4844(0, 2_000, 200, 100); | ||
| let found = pool | ||
| .find_tx_to_replace(sender, 0, &new, 10, 100) | ||
| .unwrap() | ||
| .expect("blob replacement should be admitted"); | ||
| assert_eq!(found, old_hash); | ||
| } | ||
|
|
||
| #[test] | ||
| fn no_existing_tx_returns_none() { | ||
| let pool = Mempool::new(64); | ||
| let sender = Address::from_low_u64_be(1); | ||
| let new = eip1559(0, 1_000, 100); | ||
| let res = pool.find_tx_to_replace(sender, 0, &new, 10, 100).unwrap(); | ||
| assert!(res.is_none()); | ||
| } | ||
| } | ||
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.
The
bumpselector checks only the new transaction's type. If a blob (EIP-4844) tx is already in the pool and a non-blob (EIP-1559) tx is submitted as a replacement at the same(sender, nonce), the code picksprice_bump_percent(10%) instead ofblob_price_bump_percent(100%). That is the exact spam vector the 100% threshold was added to block: an attacker can keep cycling a blob tx out of the pool with cheap 10%-bumped non-blob replacements, forcing repeated re-validation and re-gossip at near-zero cost. The selection should use the blob bump if either the in-pool tx or the incoming tx is EIP-4844.Prompt To Fix With AI