Skip to content

feat(l1): require percentage fee bump for mempool RBF (10%/100%)#6601

Open
ilitteri wants to merge 6 commits into
mainfrom
feat/mempool-rbf-price-bump
Open

feat(l1): require percentage fee bump for mempool RBF (10%/100%)#6601
ilitteri wants to merge 6 commits into
mainfrom
feat/mempool-rbf-price-bump

Conversation

@ilitteri
Copy link
Copy Markdown
Collaborator

@ilitteri ilitteri commented May 11, 2026

Motivation

The previous strict-> replacement rule lets a replacement bump fees by a single wei, enabling near-zero-cost replacement-spam that forces nodes to re-validate and re-gossip on every iteration (the Nethermind-#3094 attack class). Every major Ethereum execution client requires a percentage bump on each applicable fee field: 10% for non-blob, 100% for blob.

Description

  • BlockchainOptions gains price_bump_percent (default 10) and blob_price_bump_percent (default 100), exposed through new constants DEFAULT_PRICE_BUMP_PERCENT and DEFAULT_BLOB_PRICE_BUMP_PERCENT.
  • Mempool::find_tx_to_replace now takes the bump percents and applies the appropriate one per tx type:
    • blob bump for EIP-4844 (covers all three fee fields including max_fee_per_blob_gas)
    • standard bump for the 1559 fee pair on non-blob typed txs
    • standard bump for gas_price on legacy
    • replaces the prior OR-of-everything logic with a per-type match
  • Type-change rejection: replacing a tx with a different Transaction variant (e.g. legacy → 1559, or blob → non-blob) is rejected via std::mem::discriminant. Cross-type replacement skews accounting (blob vs non-blob slot reservation) and matches geth's tx.Type() == old.Type() guard.
  • New is_bumped_u64 / is_bumped_u256 helpers using checked_mul u128 arithmetic so a huge existing value rejects under the floor instead of silently saturating to u64::MAX / U256::MAX.
  • New CLI flags --mempool.price-bump (env ETHREX_MEMPOOL_PRICE_BUMP) and --mempool.blob-price-bump (env ETHREX_MEMPOOL_BLOB_PRICE_BUMP), plumbed through both L1 and L2 initializers.
  • Unit tests in mempool.rs covering helper edge cases plus end-to-end scenarios: 1-wei bump rejected, full 10% bump accepted, asymmetric bump rejected, blob 50% rejected, blob 100% on three axes accepted, zero-fee case, overflow rejected, blob↔non-blob and legacy-bump cross-type rejection.

Replaces the strict-`>` replacement check in `find_tx_to_replace` with a
configurable per-fee-field percentage threshold, matching the universal
defaults across geth (`PriceBump=10`, `blobpool.PriceBump=100`), reth
(`default_price_bump=10`, `replace_blob_tx_price_bump=100`), nethermind
(10% / 2× blob), erigon (`PriceBump=10`, `BlobPriceBump=100`) and besu
(`DEFAULT_PRICE_BUMP=10`, `DEFAULT_BLOB_PRICE_BUMP=100`).

The previous strict-`>` rule let a replacement bump fees by a single
wei, enabling near-zero-cost replacement-spam that forced nodes to
re-validate and re-gossip on every iteration. This is the
Nethermind-#3094 attack class.

Changes:

- `BlockchainOptions` gains `price_bump_percent` (default 10) and
  `blob_price_bump_percent` (default 100), exposed through two new
  consts `DEFAULT_PRICE_BUMP_PERCENT` / `DEFAULT_BLOB_PRICE_BUMP_PERCENT`.
- `Mempool::find_tx_to_replace` now takes the bump percents and applies
  the appropriate one per tx type: blob bump for EIP-4844 (covers all
  three fee fields including `max_fee_per_blob_gas`), standard bump for
  the 1559 fee pair, or for `gas_price` on legacy. Replaces the prior
  OR-of-everything logic with a per-type match. Saturating arithmetic,
  matching the existing style.
- New `is_bumped_u64` / `is_bumped_u256` helpers backing the check.
- New CLI flags `--mempool.price-bump` (env `ETHREX_MEMPOOL_PRICE_BUMP`)
  and `--mempool.blob-price-bump` (env
  `ETHREX_MEMPOOL_BLOB_PRICE_BUMP`), plumbed through both L1 and L2
  initializers.
- 13 unit tests in `mempool.rs` covering helper edge cases plus
  end-to-end scenarios: 1-wei bump rejected, full 10% bump accepted,
  asymmetric bump rejected, blob 50% rejected, blob 100% on three axes
  accepted, zero-fee case, saturating arithmetic.
Copilot AI review requested due to automatic review settings May 11, 2026 19:24
@ilitteri ilitteri requested review from a team, ManuelBilbao and avilagaston9 as code owners May 11, 2026 19:24
@github-actions github-actions Bot added the L1 Ethereum client label May 11, 2026
@github-actions
Copy link
Copy Markdown

🤖 Kimi Code Review

This PR correctly implements configurable RBF (Replace-By-Fee) price bump requirements for the mempool, matching the behavior of other EL clients (geth, reth, etc.). The implementation is solid with comprehensive test coverage.

Minor suggestions:

cmd/ethrex/cli.rs (lines 471-472)
Use the constants defined in blockchain.rs instead of hardcoded literals to prevent drift:

mempool_price_bump: DEFAULT_PRICE_BUMP_PERCENT,
mempool_blob_price_bump: DEFAULT_BLOB_PRICE_BUMP_PERCENT,

You'll need to import DEFAULT_PRICE_BUMP_PERCENT and DEFAULT_BLOB_PRICE_BUMP_PERCENT from crates/blockchain.

crates/blockchain/mempool.rs (lines 462-468)
Consider whether the bump percentage should be determined by the existing pool transaction type rather than the new transaction. Currently, replacing a blob tx with a non-blob tx uses the lower 10% bump (since the new tx is EIP-1559), while replacing a non-blob with a blob uses 100%.

This is likely intentional (removing blobs reduces network load), but verify this matches your L2 requirements. If you want to enforce the stricter bump whenever a blob is being replaced (regardless of replacement type), check tx_in_pool instead of tx.


Automated review by Kimi (Moonshot AI) · kimi-k2.5 · custom prompt

@github-actions
Copy link
Copy Markdown

github-actions Bot commented May 11, 2026

Lines of code report

Total lines added: 264
Total lines removed: 0
Total lines changed: 264

Detailed view
+----------------------------------------+-------+------+
| File                                   | Lines | Diff |
+----------------------------------------+-------+------+
| ethrex/cmd/ethrex/cli.rs               | 1129  | +21  |
+----------------------------------------+-------+------+
| ethrex/cmd/ethrex/initializers.rs      | 657   | +2   |
+----------------------------------------+-------+------+
| ethrex/cmd/ethrex/l2/initializers.rs   | 383   | +2   |
+----------------------------------------+-------+------+
| ethrex/crates/blockchain/blockchain.rs | 2520  | +12  |
+----------------------------------------+-------+------+
| ethrex/crates/blockchain/error.rs      | 153   | +4   |
+----------------------------------------+-------+------+
| ethrex/crates/blockchain/mempool.rs    | 687   | +223 |
+----------------------------------------+-------+------+

@github-actions
Copy link
Copy Markdown

🤖 Claude Code Review

Now I have everything I need. Here is my review:


Review: feat(l1): require percentage fee bump for mempool RBF (10%/100%)

Summary

This PR correctly addresses the RBF spam attack (Nethermind-#3094 class) by replacing the strict > replacement gate with a percentage-based bump. The overall approach, constants (10%/100%), and structure match peer client behavior. The tests are well-chosen and cover the important end-to-end paths.

There is one concrete security regression introduced by the refactor, plus a few smaller issues.


Critical: EIP-2930 transactions bypass the fee bump check

File: crates/blockchain/mempool.rs, catch-all _ arm (~line 510)

Transaction::EIP2930Transaction.max_fee_per_gas() returns None and max_priority_fee() returns None (confirmed in transaction.rs lines 1459 and 1347). Both unwrap_or_default() calls therefore yield 0.

When an EIP-2930 replacement falls into the _ arm, the check becomes:

is_bumped_u64(0, 0, 10)  // threshold = 0 * 110/100 = 0; 0 >= 0 → true
is_bumped_u64(0, 0, 10)  // same

Result: any EIP-2930 transaction unconditionally passes the replacement check, regardless of how much (or how little) the gas_price actually changed. This is a regression — the old code compared gas_price directly.

The fix is to treat EIP-2930 like legacy, since both use a single gas_price field:

Transaction::LegacyTransaction(_) | Transaction::EIP2930Transaction(_) => {
    is_bumped_u256(tx_in_pool.gas_price(), tx.gas_price(), bump)
}

EIP-2930 is rarely used in practice, but the bypass is complete and deterministic.


Medium: Incorrect test comment for saturation case

File: crates/blockchain/mempool.rs, test is_bumped_u64_saturating_does_not_panic

// Multiplier saturates at u64::MAX; threshold stays bounded.
assert!(is_bumped_u64(u64::MAX, u64::MAX, 100));

The comment is wrong. The multiplier is 100 + 100 = 200 — it does not saturate. What saturates is u64::MAX.saturating_mul(200), which clamps to u64::MAX, making the threshold u64::MAX / 100 ≈ 1.84 × 10¹⁷.

The assertion also deserves scrutiny: with a 100% bump requirement and existing = u64::MAX, the mathematical threshold is 2 × u64::MAX, which no u64 value can satisfy, so the replacement should be rejected. Due to saturation, it returns true instead. This is practically irrelevant (fees at u64::MAX are 18 ETH/gas, astronomical), but the assertion documents incorrect behavior as expected behavior, which will mislead future readers.

A better test name and comment would be: is_bumped_u64_near_max_does_not_panic, and the comment should acknowledge the saturation trade-off rather than misidentifying where saturation occurs.


Low: Bump rate determined by new tx type, not pool tx type

File: crates/blockchain/mempool.rs, ~line 480

let bump = if matches!(tx, Transaction::EIP4844Transaction(_)) {
    blob_price_bump_percent
} else {
    price_bump_percent
};

If there is an EIP-4844 tx in the pool and the replacement is a non-blob tx (EIP-1559), only the 10% bump applies, and max_fee_per_blob_gas is not checked. This is arguably correct (the replacement isn't a blob tx, so blob sidecar re-propagation is not a concern), but it means a sender can effectively evict a blob tx with a cheap 10% bump by switching to a non-blob type. Whether that is the intended policy is worth documenting explicitly.


Positive notes

  • The per-type match is far cleaner than the old OR-of-everything logic and correctly enforces a bump on every applicable field simultaneously.
  • is_bumped_u64 / is_bumped_u256 use floor division that matches what geth, reth, and others implement — no accidental rounding up.
  • The blob fee check now covers all three axes (max_fee_per_gas, max_priority_fee_per_gas, max_fee_per_blob_gas) instead of only the blob fee field.
  • CLI flags, defaults, and constant names follow the pattern of peer clients.
  • Tests for helper functions and end-to-end scenarios are thorough; the zero-fee and asymmetric-bump cases are particularly useful.

Required change before merge

Add an explicit Transaction::EIP2930Transaction arm (or merge it with LegacyTransaction) that checks gas_price via is_bumped_u256, and add a test that confirms a 1-wei EIP-2930 bump is rejected.


Automated review by Claude (Anthropic) · sonnet · custom prompt

@github-actions
Copy link
Copy Markdown

🤖 Codex Code Review

Findings

  1. crates/blockchain/mempool.rs:501-516 regresses EIP-2930 replacement checks. EIP2930Transaction is sent through the 1559-style branch, but max_fee_per_gas() and max_priority_fee() return None for 2930 (crates/common/types/transaction.rs:1344-1352, 1456-1464). That makes both thresholds 0, so any same-nonce 2930 tx can replace an existing 2930 tx regardless of gas_price. This weakens mempool anti-spam policy; 2930 should be compared on gas_price like legacy, or via normalized fee-cap/tip-cap helpers. A 2930 replacement test is missing.

  2. crates/blockchain/mempool.rs:469-498 applies the blob bump based on the incoming tx type, not the tx already in the pool. That disagrees with the exposed config/docs in cmd/ethrex/cli.rs:187-197 (“required to replace a … pooled transaction”). Today, replacing a blob tx with a plain tx only needs the plain bump, while replacing a plain tx with a blob tx incorrectly needs the blob bump. If cross-type replacements are supported, the bump needs to key off the evicted tx; otherwise reject type switches explicitly.

  3. crates/blockchain/mempool.rs:528-542 under-enforces the configured bump. floor(existing * (100 + bump) / 100) lets existing=1, new=1, bump=10 pass even though there is no 10% increase, and the saturating multiply lowers the threshold near u64::MAX/U256::MAX. This should be implemented with ceil/cross-multiplication instead of flooring a saturated product.

The CLI/initializer plumbing into BlockchainOptions looks consistent.

I couldn’t run the Rust tests here because toolchain/dependency resolution is blocked in this sandbox.


Automated review by OpenAI Codex · gpt-5.4 · custom prompt

Copy link
Copy Markdown
Contributor

Copilot AI left a comment

Choose a reason for hiding this comment

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

Pull request overview

This PR hardens mempool Replace-By-Fee (RBF) admission rules by requiring percentage-based fee bumps (default 10% for non-blob transactions and 100% for EIP-4844 blob transactions), and exposes these thresholds via new CLI options and BlockchainOptions.

Changes:

  • Add configurable price_bump_percent and blob_price_bump_percent to BlockchainOptions, with defaults and CLI plumbing for both L1 and L2.
  • Update Mempool::find_tx_to_replace to enforce per-tx-type fee-field bump rules and introduce is_bumped_u64 / is_bumped_u256 helpers.
  • Add unit tests for bump helpers and basic replacement scenarios.

Reviewed changes

Copilot reviewed 5 out of 5 changed files in this pull request and generated 3 comments.

Show a summary per file
File Description
crates/blockchain/mempool.rs Implements percentage-based RBF checks, adds bump helpers, and adds unit tests.
crates/blockchain/blockchain.rs Adds new mempool bump configuration to BlockchainOptions and wires it into tx validation.
cmd/ethrex/l2/initializers.rs Plumbs CLI bump options into L2 blockchain initialization.
cmd/ethrex/initializers.rs Plumbs CLI bump options into L1 blockchain initialization.
cmd/ethrex/cli.rs Adds CLI flags/env vars for configuring mempool RBF bump thresholds.

💡 Add Copilot custom instructions for smarter, more guided reviews. Learn how to get started.

Comment on lines +484 to +488
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 thread crates/blockchain/mempool.rs Outdated
Comment on lines +529 to +536
/// 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 +539 to +542
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
@greptile-apps
Copy link
Copy Markdown

greptile-apps Bot commented May 11, 2026

Greptile Summary

This PR replaces the simple strict-greater-than fee comparison for mempool RBF with a percentage-bump check: 10% for non-blob transactions and 100% for EIP-4844 blob transactions, matching the defaults used by geth, reth, nethermind, erigon, and besu. The change adds price_bump_percent / blob_price_bump_percent fields to BlockchainOptions, two new CLI flags, and 13 unit tests.

  • The bump-rate selector in find_tx_to_replace only inspects the new transaction's type — if the existing pool tx is EIP-4844 blob and the replacement is non-blob, the 100% blob bump is not enforced (only 10% is required), allowing cheap cycling of blob transactions out of the pool.
  • CLI flags and struct wiring are clean and correctly plumbed through both the L1 and L2 initializers.

Confidence Score: 3/5

The blob bump protection can be bypassed by replacing a blob tx with a non-blob tx at 10%; should be fixed before merging.

The blob-to-non-blob cross-type replacement path uses the wrong bump rate, meaning the 100% blob protection added in this PR can be circumvented with a well-chosen replacement sequence. The rest of the change — CLI wiring, struct fields, helper functions, and non-blob RBF logic — is correct and well-tested.

crates/blockchain/mempool.rs — specifically the bump selector in find_tx_to_replace and the is_bumped_u64 saturation behavior

Important Files Changed

Filename Overview
crates/blockchain/mempool.rs Core RBF logic rewritten with percentage bump helpers; bump-type selector only checks the new tx's type, allowing blob txs to be replaced cheaply via a cross-type non-blob replacement.
crates/blockchain/blockchain.rs Adds price_bump_percent and blob_price_bump_percent fields to BlockchainOptions and plumbs them into find_tx_to_replace; straightforward and correct.
cmd/ethrex/cli.rs Adds two new CLI flags with correct defaults and env-var bindings.
cmd/ethrex/initializers.rs Plumbs new CLI options into BlockchainOptions for L1; straightforward.
cmd/ethrex/l2/initializers.rs Plumbs new CLI options into BlockchainOptions for L2; straightforward.

Flowchart

%%{init: {'theme': 'neutral'}}%%
flowchart TD
    A[New TX submitted] --> B{contains_sender_nonce?}
    B -- No --> C[Return None]
    B -- Yes --> D{New TX type?}
    D -- EIP-4844 --> E[bump = 100%]
    D -- Other --> F[bump = 10%]
    E --> G[Check max_fee_per_gas, max_priority_fee_per_gas, max_fee_per_blob_gas]
    G --> J{All bumped?}
    J -- Yes --> K[Replacement admitted]
    J -- No --> L[UnderpricedReplacement]
    F -- Legacy --> M[Check gas_price]
    F -- 1559/Other --> N[Check max_fee_per_gas and max_priority_fee_per_gas]
    M --> Q{Passes?}
    N --> P{Both pass?}
    Q -- Yes --> K
    Q -- No --> L
    P -- Yes --> K
    P -- No --> L
Loading
Prompt To Fix All With AI
Fix the following 2 code review issues. Work through them one at a time, proposing concise fixes.

---

### Issue 1 of 2
crates/blockchain/mempool.rs:463-470
**Blob bump rate bypassed by cross-type replacement**

The `bump` selector 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 picks `price_bump_percent` (10%) instead of `blob_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.

### Issue 2 of 2
crates/blockchain/mempool.rs:531-535
**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.

Reviews (1): Last reviewed commit: "feat(l1): require percentage fee bump fo..." | Re-trigger Greptile

Comment on lines 463 to +470
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
Copy link
Copy Markdown

Choose a reason for hiding this comment

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

P1 Blob bump rate bypassed by cross-type replacement

The bump selector 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 picks price_bump_percent (10%) instead of blob_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
This is a comment left during a code review.
Path: crates/blockchain/mempool.rs
Line: 463-470

Comment:
**Blob bump rate bypassed by cross-type replacement**

The `bump` selector 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 picks `price_bump_percent` (10%) instead of `blob_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.

How can I resolve this? If you propose a fix, please make it concise.

Comment on lines +531 to +535
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
}
Copy link
Copy Markdown

Choose a reason for hiding this comment

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

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

Prompt To Fix With AI
This 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.

…rithmetic, named constants

Review-driven changes for the RBF replacement path:

1. Reject type-change replacements
   A blob transaction in the pool must not be displaced by a non-blob
   transaction at the same (sender, nonce) — the blob's sidecar is
   expensive to re-propagate. The inverse is also rejected for
   symmetry. Peer clients keep blob/non-blob in separate sub-pools and
   get this rejection for free; ethrex's single pool needs an explicit
   `std::mem::discriminant` check. Returns the existing
   `MempoolError::UnderpricedReplacement`.

2. Checked-mul arithmetic in `is_bumped_u64` / `is_bumped_u256`
   The previous saturating-mul implementation silently *admitted* an
   under-priced replacement when `existing * (100 + bump)` exceeded the
   integer width. Switched to u128 / U256::checked_mul with overflow →
   reject. The test that documented the wrong semantic
   (`is_bumped_u64_saturating_does_not_panic`) is renamed and re-asserts
   the correct rejection at huge `existing` values.

3. Reference named constants in cli.rs
   `default_value_t` and the `Options::default` impl for
   `mempool_price_bump` / `mempool_blob_price_bump` now reference
   `DEFAULT_PRICE_BUMP_PERCENT` / `DEFAULT_BLOB_PRICE_BUMP_PERCENT`
   instead of magic literals 10 / 100. Help text and docs/CLI.md drop
   peer-client name-drops.

4. Tests
   Added three end-to-end tests:
   - `blob_cannot_be_replaced_by_non_blob`
   - `non_blob_cannot_be_replaced_by_blob`
   - `legacy_replacement_requires_10_percent_bump` (closes the
     previously-untested legacy path)
   Plus the renamed overflow test.

5. Comment fix
   Clarified the `_ =>` arm comment: the PrivilegedL2 variant inside
   this arm is unreachable in practice (early-returned in
   validate_transaction), but the other variants (2930, 1559, 7702,
   FeeToken) do hit it. Previous comment said "this branch" was
   unreachable, which was wrong.
Comment thread crates/blockchain/mempool.rs Outdated
// its expensive sidecar) or vice versa. ethrex has a single pool,
// so the same guarantee has to be enforced here.
if std::mem::discriminant(tx) != std::mem::discriminant(tx_in_pool.transaction()) {
return Err(MempoolError::UnderpricedReplacement);
Copy link
Copy Markdown
Collaborator

Choose a reason for hiding this comment

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

We should return a new error variant here

Copy link
Copy Markdown
Collaborator Author

Choose a reason for hiding this comment

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

Copy link
Copy Markdown
Collaborator

@MegaRedHand MegaRedHand left a comment

Choose a reason for hiding this comment

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

LGTM. Left a comment

…F rejection

@MegaRedHand: "We should return a new error variant here" — the
type-change rejection in `find_tx_to_replace` was reusing
`UnderpricedReplacement`, which conflates two distinct failure modes:
"new tx is the same type but didn't bump enough" vs. "new tx is a
different type than the in-pool tx (e.g. non-blob trying to replace
blob)". Splitting them lets RPC clients distinguish "retry with higher
fees" from "retry with the same type" without re-parsing error text.

Cross-client audit also flagged this overloading: peer clients separate
"underpriced", "replacement-underpriced", "type-mismatch" / "blob-
replace-tx" diagnostics. ethrex now matches with two variants
(`UnderpricedReplacement` for the fee-bump case, `ReplacementTypeMismatch`
for the discriminant-mismatch case).

The three discriminant-mismatch tests in `mempool.rs` are updated to
expect the new variant.
ilitteri added a commit that referenced this pull request May 12, 2026
Phase 2 cross-check found that the original PR guard only checks the
blob-count when BOTH old and new are EIP-4844. A non-blob tx replacing
an in-pool blob tx is treated as a degenerate shrink to 0 blobs and
rejected with the same error.

Removes this PR's dependency on PR #6601's type-discriminant check
landing first to close the cross-type spam vector. The TODO is updated
to note that the cross-type arm becomes unreachable once #6601 merges.

New test: rejects_non_blob_replacement_of_blob_tx.
@github-actions
Copy link
Copy Markdown

github-actions Bot commented May 13, 2026

⚠️ Known Issues — intentionally skipped tests

Source: docs/known_issues.md

Known Issues

Tests intentionally excluded from CI. Source of truth for the Known
Issues
section the L1 workflow appends to each ef-tests job summary
and posts as a sticky PR comment.

Hive — bal-devnet-6 Amsterdam consume-engine tests — 32 functions / 54 cases

Same root cause as the blockchain-runner skip list (see EF Tests —
Blockchain
below): snobal-devnet-6 fixtures expect bal-devnet-6 spec
semantics, but our impl runs ahead due to the bal-devnet-7-prep
set_delegation SELFDESTRUCT-style refund subtraction. These fixtures
are routed through hive's eels/consume-engine simulator and produce
the same failures. Excluded via KNOWN_EXCLUDED_TESTS (substring
match on test_<name>[fork_Amsterdam, anchoring to the Amsterdam fork
so legacy Prague/Osaka variants still run).

Affected EELS test functions (32)
  • test_auth_refund_block_gas_accounting
  • test_auth_refund_bypasses_one_fifth_cap
  • test_auth_state_gas_scales_with_cpsb
  • test_auth_with_calldata_and_access_list
  • test_auth_with_multiple_sstores
  • test_authorization_exact_state_gas_boundary
  • test_authorization_to_precompile_address
  • test_authorization_with_sstore
  • test_bal_7702_delegation_clear
  • test_bal_7702_delegation_create
  • test_bal_7702_delegation_update
  • test_bal_7702_double_auth_reset
  • test_bal_7702_double_auth_swap
  • test_bal_7702_null_address_delegation_no_code_change
  • test_bal_all_transaction_types
  • test_bal_selfdestruct_to_7702_delegation
  • test_bal_withdrawal_to_7702_delegation
  • test_duplicate_signer_authorizations
  • test_existing_account_auth_header_gas_used_uses_worst_case
  • test_existing_account_refund
  • test_existing_account_refund_enables_sstore
  • test_existing_auth_with_reverted_execution_preserves_intrinsic
  • test_many_authorizations_state_gas
  • test_mixed_auths_header_gas_used_uses_worst_case
  • test_mixed_new_and_existing_auths
  • test_mixed_valid_and_invalid_auths
  • test_multi_tx_block_auth_refund_and_sstore
  • test_multiple_refund_types_in_one_tx
  • test_simple_gas_accounting
  • test_sstore_state_gas_all_tx_types
  • test_transfer_with_all_tx_types
  • test_varying_calldata_costs

EF Tests — Stateless coverage narrowed to EIP-8025 optional-proofs

make -C tooling/ef_tests/blockchain test calls test-stateless-zkevm
instead of test-stateless. The zkevm@v0.3.3 fixtures are filled against
bal@v5.6.1, out of sync with current bal spec; the broad target trips ~549
fixtures. Re-broaden once the zkevm bundle is regenerated.

Why and resolution path

PR #6527 broadened
test-stateless to extract the entire for_amsterdam/ tree from the
zkevm bundle and run all of it under --features stateless; combined with
this branch's bal-devnet-6+ semantics (and bal-devnet-7-prep
set_delegation re-application) that scope produces ~549
GasUsedMismatch / ReceiptsRootMismatch /
BlockAccessListHashMismatch failures.

test-stateless-zkevm filters cargo to the eip8025_optional_proofs
suite, which still validates the stateless harness without the bal-version
mismatch.

Re-broaden by switching test: back to test-stateless in
tooling/ef_tests/blockchain/Makefile once the zkevm bundle is regenerated
against the current bal spec.

EF Tests — Blockchain bal-devnet-6 (Amsterdam fork) — 74 tests

snobal-devnet-6 fixtures expect bal-devnet-6 spec semantics, but our impl
runs ahead due to the bal-devnet-7-prep set_delegation SELFDESTRUCT-style
refund subtraction. Skipped in
tooling/ef_tests/blockchain/tests/all.rs::SKIPPED_BASE, anchored on
[fork_Amsterdam so legacy Prague / Osaka variants still run.

Bucket breakdown (74 total) and resolution path
EIP Bucket Count
EIP-7702 set_code_txs 24
EIP-7702 set_code_txs_2 15
EIP-7702 gas 1
EIP-8037 state_gas_set_code 17
EIP-8037 state_gas_pricing 1
EIP-8037 state_gas_sstore 1
EIP-7928 block_access_lists_eip7702 8
EIP-7928 block_access_lists 1
EIP-7778 gas_accounting 3
EIP-7708 transfer_logs 1
EIP-7976 refunds 1
EIP-1344 chainid (Amsterdam fork-transition fixture) 1
Total 74

Re-enable once we either:

  • (a) bump fixtures to a snobal-devnet-7 release that locks in the new
    accounting; or
  • (b) revert the bal-devnet-7-prep subtraction for bal-devnet-6
    compatibility.
Full test list (74)

EIP-7702 — for_amsterdam/prague/eip7702_set_code_tx/set_code_txs/

  • delegation_clearing
  • delegation_clearing_and_set
  • delegation_clearing_failing_tx
  • delegation_clearing_tx_to
  • eoa_tx_after_set_code
  • ext_code_on_chain_delegating_set_code
  • ext_code_on_self_delegating_set_code
  • ext_code_on_self_set_code
  • ext_code_on_set_code
  • many_delegations
  • nonce_overflow_after_first_authorization
  • nonce_validity
  • reset_code
  • self_code_on_set_code
  • self_sponsored_set_code
  • set_code_multiple_valid_authorization_tuples_same_signer_increasing_nonce
  • set_code_multiple_valid_authorization_tuples_same_signer_increasing_nonce_self_sponsored
  • set_code_to_log
  • set_code_to_non_empty_storage_non_zero_nonce
  • set_code_to_self_destruct
  • set_code_to_self_destructing_account_deployed_in_same_tx
  • set_code_to_sstore
  • set_code_to_sstore_then_sload
  • set_code_to_system_contract

EIP-7702 — for_amsterdam/prague/eip7702_set_code_tx/set_code_txs_2/

  • call_pointer_to_created_from_create_after_oog_call_again
  • call_to_precompile_in_pointer_context
  • contract_storage_to_pointer_with_storage
  • delegation_replacement_call_previous_contract
  • double_auth
  • pointer_measurements
  • pointer_normal
  • pointer_reentry
  • pointer_resets_an_empty_code_account_with_storage
  • pointer_reverts
  • pointer_to_pointer
  • pointer_to_precompile
  • pointer_to_static
  • pointer_to_static_reentry
  • static_to_pointer

EIP-7702 — for_amsterdam/prague/eip7702_set_code_tx/gas/

  • account_warming

EIP-8037 — for_amsterdam/amsterdam/eip8037_state_creation_gas_cost_increase/state_gas_set_code/

  • auth_refund_block_gas_accounting
  • auth_refund_bypasses_one_fifth_cap
  • auth_with_calldata_and_access_list
  • auth_with_multiple_sstores
  • authorization_exact_state_gas_boundary
  • authorization_to_precompile_address
  • authorization_with_sstore
  • duplicate_signer_authorizations
  • existing_account_auth_header_gas_used_uses_worst_case
  • existing_account_refund
  • existing_account_refund_enables_sstore
  • existing_auth_with_reverted_execution_preserves_intrinsic
  • many_authorizations_state_gas
  • mixed_auths_header_gas_used_uses_worst_case
  • mixed_new_and_existing_auths
  • mixed_valid_and_invalid_auths
  • multi_tx_block_auth_refund_and_sstore

EIP-8037 — state_gas_pricing/

  • auth_state_gas_scales_with_cpsb

EIP-8037 — state_gas_sstore/

  • sstore_state_gas_all_tx_types

EIP-7928 — for_amsterdam/amsterdam/eip7928_block_level_access_lists/block_access_lists_eip7702/

  • bal_7702_delegation_clear
  • bal_7702_delegation_create
  • bal_7702_delegation_update
  • bal_7702_double_auth_reset
  • bal_7702_double_auth_swap
  • bal_7702_null_address_delegation_no_code_change
  • bal_selfdestruct_to_7702_delegation
  • bal_withdrawal_to_7702_delegation

EIP-7928 — block_access_lists/

  • bal_all_transaction_types

EIP-7778 — for_amsterdam/amsterdam/eip7778_block_gas_accounting_without_refunds/gas_accounting/

  • multiple_refund_types_in_one_tx
  • simple_gas_accounting
  • varying_calldata_costs

EIP-7708 — for_amsterdam/amsterdam/eip7708_eth_transfer_logs/transfer_logs/

  • transfer_with_all_tx_types

EIP-7976 — for_amsterdam/amsterdam/eip7976_increase_calldata_floor_cost/refunds/

  • gas_refunds_from_data_floor

EIP-1344 — for_amsterdam/istanbul/eip1344_chainid/chainid/

  • chainid (Amsterdam fork-transition fixture)

Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment

Labels

L1 Ethereum client

Projects

Status: No status

Development

Successfully merging this pull request may close these issues.

3 participants