Skip to content

feat(l1): enforce minimum priority-fee floor at mempool admission#6604

Open
ilitteri wants to merge 10 commits into
mainfrom
feat/mempool-min-tip-floor
Open

feat(l1): enforce minimum priority-fee floor at mempool admission#6604
ilitteri wants to merge 10 commits into
mainfrom
feat/mempool-min-tip-floor

Conversation

@ilitteri
Copy link
Copy Markdown
Collaborator

@ilitteri ilitteri commented May 11, 2026

Motivation

MIN_GAS_TIP = 1_000_000 (1 Mwei) has lived in crates/common/types/constants.rs since before PR #6509, where it was explicitly documented as not a mempool admission gate — only the RPC gas-price estimator consults it. Today, zero-tip transactions enter the mempool and only get filtered out at block-build time. This change promotes the concept into a configurable admission gate so under-floor transactions are rejected at ingress.

Description

  • BlockchainOptions gains min_tip_wei: u64 plus a new DEFAULT_MIN_TIP_WEI = 1 constant. The default matches geth's mempool PriceLimit = 1 wei — effectively just rejects zero-tip transactions while not interfering with any realistic real-world tip.
  • Blockchain::validate_transaction compares the raw gas tip cap (tx.gas_tip_cap() — returns max_priority_fee_per_gas for typed txs, gas_price for legacy) against min_tip_wei, matching geth's PriceLimit check on tx.GasTipCap(), reth's check on max_priority_fee_per_gas, and erigon's check on TxnSlot.GetTipCap(). Under-floor txs are rejected with MempoolError::TipBelowMinimum { actual, limit }. A floor of 0 disables the check entirely.
  • The decision uses the raw tip cap rather than the base-fee-aware effective tip on purpose: a tx that paid the floor at admission should not be reclassified as under-floor when base fee oscillates upward. (Nethermind's MinGasPriceTxFilter is the only peer that uses effective-tip semantics, and it lives in the block producer, not txpool admission.)
  • New CLI flag --mempool.min-tip (env ETHREX_MEMPOOL_MIN_TIP), plumbed through both L1 and L2 initializers.
  • MIN_GAS_TIP keeps its original scope as the RPC gas-price-estimator floor; the doc comment is updated to point at DEFAULT_MIN_TIP_WEI for the mempool admission concern.
  • Blockchain::default_with_store (the test helper used across the blockchain, rpc, and ethrex-test crates) explicitly sets min_tip_wei = 0 so unrelated tests stay decoupled from this rule.

Behavioral change

Admission becomes stricter: zero-tip transactions submitted via eth_sendRawTransaction or received over P2P are now rejected at ingress. Operators that want to admit zero-tip transactions (local devnets, internal sequencer flows) can pass --mempool.min-tip 0.

Tests

Integration tests in test/tests/blockchain/mempool_tests.rs: zero-tip 1559 rejected under a 1 Mwei floor, at-floor 1559 passes the tip check, floor of zero admits zero-tip, legacy gas_price below floor rejected, default floor admits 1-wei tip and rejects 0-wei, blob tx under floor rejected, options field consulted in validate_transaction.

ilitteri added 3 commits May 11, 2026 18:52
Promotes MIN_GAS_TIP (1 Mwei, defined in crates/common/types/constants.rs)
from an RPC-estimator-only constant into a configurable admission gate
on the mempool. Zero-tip transactions, which today silently sit in the
pool until block-build time, are now rejected at ingress.

- `BlockchainOptions` gains `min_tip_wei: u64` (default
  `MIN_GAS_TIP = 1_000_000`) plus a `DEFAULT_MIN_TIP_WEI` constant.
- `Blockchain::validate_transaction` computes the effective tip
  (`max_priority_fee_per_gas` for typed txs, `gas_price - base_fee`
  for legacy) and rejects with `MempoolError::TipBelowMinimum
  { actual, limit }` when below the configured floor. A floor of 0
  disables the check, matching the same off-switch path as devnet
  configurations and `eth_sendRawTransaction` smoke tests.
- New CLI flag `--mempool.min-tip` (env `ETHREX_MEMPOOL_MIN_TIP`),
  plumbed through both L1 and L2 initializers.
- Updates the `MIN_GAS_TIP` doc comment to reflect its dual role
  (admission floor + RPC estimator floor), replacing the prior
  "NOT a mempool admission gate" wording.
- `Blockchain::default_with_store` (the test helper used across the
  blockchain, rpc, and ethrex-test crates) now explicitly sets
  `min_tip_wei = 0` so unrelated tests stay decoupled from this rule.
- Five integration tests in `test/tests/blockchain/mempool_tests.rs`:
  zero-tip 1559 rejected at default, at-floor 1559 passes the tip
  check, floor of zero admits zero-tip, legacy effective tip below
  floor rejected, and `BlockchainOptions::min_tip_wei` is correctly
  consulted in `validate_transaction`.
Copilot AI review requested due to automatic review settings May 11, 2026 21:59
@ilitteri ilitteri requested review from a team, ManuelBilbao and avilagaston9 as code owners May 11, 2026 21:59
@github-actions github-actions Bot added the L1 Ethereum client label May 11, 2026
@github-actions
Copy link
Copy Markdown

🤖 Kimi Code Review

The PR introduces a configurable mempool minimum tip floor with correct implementation and comprehensive test coverage. No critical issues found.

Minor observations:

  1. Line 2490-2501 (crates/blockchain/blockchain.rs): The unwrap_or(u64::MAX) fallback for legacy transaction gas price conversion is conservative (allows tx when in doubt), which is acceptable given that u64::MAX wei exceeds any realistic tip floor.

  2. Line 468 (test/tests/blockchain/mempool_tests.rs): The helper blockchain_with_min_tip creates a blockchain with default options then immediately clones and mutates them. This is slightly inefficient but acceptable for test code.

  3. Documentation: The distinction between MIN_GAS_TIP (RPC estimation) and DEFAULT_MIN_TIP_WEI (mempool admission) is clearly documented in the comments, preventing confusion about the two different defaults.

Security check: The admission check correctly handles:

  • Legacy transactions: gas_price - base_fee (with saturating arithmetic)
  • Typed transactions: max_priority_fee_per_gas directly
  • Pre-London headers: defaults base_fee to 0

The validation ordering (after fee cap check, before intrinsic gas) is correct.


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

@github-actions
Copy link
Copy Markdown

🤖 Claude Code Review

Now I have enough context to write a thorough review.


PR #6604feat(l1): enforce minimum priority-fee floor at mempool admission

Overview

This PR adds a configurable min-tip admission gate (min_tip_wei) to BlockchainOptions, wires it through the CLI and both L1/L2 initializers, and enforces it inside Blockchain::validate_transaction. The default is 1 wei (matches geth's PriceLimit), effectively filtering zero-tip transactions while leaving all real-world traffic alone. Operators can pass --mempool.min-tip 0 to disable. Five integration tests cover the main cases.

The motivation is clear and the implementation is generally correct. Below are findings from most to least severe.


Correctness

EIP-1559 effective tip is not capped at max_fee_per_gas - base_fee at admission time

blockchain.rs:2496–2497

let effective_tip = match tx.max_priority_fee() {
    Some(tip) => tip,   // uses raw max_priority_fee_per_gas

For EIP-1559 transactions, the actual miner tip at inclusion is min(max_priority_fee_per_gas, max_fee_per_gas - base_fee). A transaction with max_priority_fee_per_gas = 1 wei, max_fee_per_gas = 1 wei passes the floor at admission, but if the block's base_fee = 1 wei at inclusion time, the effective miner tip is 0 — below the floor that was checked. This is an inherent limitation of any admission-time check for EIP-1559 transactions (the future base fee is unknown), and geth takes the same approach. Worth a brief comment making this design tradeoff explicit.

u64::try_from(gas_price).unwrap_or(u64::MAX) — overflow path

blockchain.rs:2500

let gas_price_u64 = u64::try_from(tx.gas_price()).unwrap_or(u64::MAX);
gas_price_u64.saturating_sub(base_fee)

If gas_price overflows u64 (implausible in practice, since u64::MAX ≈ 18.4 ETH as a gas price), the fallback is u64::MAX - base_fee, which is enormous and will always pass the floor. This is the correct behaviour — a gigantic gas price should never be blocked — but the silent unwrap_or(u64::MAX) feels fragile without a comment. Either a short inline note or a debug_assert!(tx.gas_price() <= U256::from(u64::MAX)) would help a future reader.


Minor Issues

Unnecessary clone in blockchain_with_min_tip test helper

mempool_tests.rs:478–480

let mut opts = bc.options.clone();
opts.min_tip_wei = min_tip_wei;
bc.options = opts;

Since options is pub and the field is directly assignable, this can be:

bc.options.min_tip_wei = min_tip_wei;

Missing test: EIP-2930 transactions fall into the legacy branch

max_priority_fee() returns None for EIP2930Transaction, so the code takes the gas_price.saturating_sub(base_fee) path. There is no test covering this. An EIP-2930 tx with gas_price just below the floor should be rejected.

Missing test: at-floor for legacy transactions

The suite has zero_tip_eip1559_rejected_under_default_floor (below floor) and at_floor_eip1559_passes_tip_check (at floor) for EIP-1559, but for legacy transactions only the below-floor case (legacy_effective_tip_below_floor_rejected) is tested. An at-floor legacy test would complete the symmetry.

DEFAULT_MIN_TIP_WEI constant declared after the impl Default block that references it

blockchain.rs:254

In Rust, constant ordering is fine at the module level, but having the constant appear 5 lines after the impl Default that uses it is an unusual reading order. Moving it immediately above impl Default for BlockchainOptions would improve readability.

Doc comment on default_with_store describes the current task context

blockchain.rs:353–359

/// Test-permissive `Blockchain` constructor. Mirrors `BlockchainOptions::default`
/// but disables admission-policy gates (e.g. the min-tip floor) so that
/// unrelated tests don't need to set every mempool option explicitly.

The phrase "unrelated tests don't need to set every mempool option explicitly" anchors the comment to this PR's narrative rather than explaining the lasting invariant. A tighter version: "Returns a Blockchain suitable for unit tests: identical to BlockchainOptions::default but with all admission-policy floors set to 0, so tests remain decoupled from policy defaults."


Things Done Well

  • The min_tip_wei = 0 escape hatch in default_with_store is the right call — existing tests shouldn't need to be touched just because a new gate was introduced.
  • Reusing MempoolError rather than a new error type keeps the error surface clean.
  • Structured error fields (actual, limit) in TipBelowMinimum are good for observability/logging.
  • The CLI and L2 initializer are both plumbed correctly.
  • The PR correctly separates MIN_GAS_TIP (RPC estimator) from DEFAULT_MIN_TIP_WEI (admission gate) and updates the doc comment to reflect that.

Summary

No correctness bugs. The EIP-1559 admission-time approximation is a known, acceptable design tradeoff (shared with geth) — just worth documenting. The unwrap_or(u64::MAX) fallback should have a comment. The test helper has an unnecessary clone and is missing coverage for EIP-2930 and the at-floor legacy case.


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

@github-actions
Copy link
Copy Markdown

github-actions Bot commented May 11, 2026

Lines of code report

Total lines added: 30
Total lines removed: 0
Total lines changed: 30

Detailed view
+----------------------------------------+-------+------+
| File                                   | Lines | Diff |
+----------------------------------------+-------+------+
| ethrex/cmd/ethrex/cli.rs               | 1118  | +10  |
+----------------------------------------+-------+------+
| ethrex/cmd/ethrex/initializers.rs      | 656   | +1   |
+----------------------------------------+-------+------+
| ethrex/cmd/ethrex/l2/initializers.rs   | 382   | +1   |
+----------------------------------------+-------+------+
| ethrex/crates/blockchain/blockchain.rs | 2524  | +16  |
+----------------------------------------+-------+------+
| ethrex/crates/blockchain/error.rs      | 151   | +2   |
+----------------------------------------+-------+------+

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 promotes the “minimum tip” concept into a configurable mempool admission rule by adding a min_tip_wei floor to BlockchainOptions and enforcing it in Blockchain::validate_transaction, with CLI plumbing and new integration tests to validate admission behavior.

Changes:

  • Add BlockchainOptions::min_tip_wei with a new default DEFAULT_MIN_TIP_WEI = 1 and wire it through L1/L2 initializers and CLI (--mempool.min-tip / ETHREX_MEMPOOL_MIN_TIP).
  • Enforce the configured minimum effective tip during Blockchain::validate_transaction, returning MempoolError::TipBelowMinimum { actual, limit } on rejection.
  • Add integration tests covering zero-tip and under-floor tx admission behavior, and update docs/comments to clarify MIN_GAS_TIP scope.

Reviewed changes

Copilot reviewed 8 out of 8 changed files in this pull request and generated 4 comments.

Show a summary per file
File Description
test/tests/blockchain/mempool_tests.rs Adds integration tests and helpers for min-tip admission floor behavior.
docs/CLI.md Documents the new --mempool.min-tip CLI option and default.
crates/common/types/constants.rs Updates MIN_GAS_TIP docs to clarify it’s for RPC estimators, not mempool admission.
crates/blockchain/error.rs Introduces MempoolError::TipBelowMinimum { actual, limit }.
crates/blockchain/blockchain.rs Adds min_tip_wei option, a default constant, and enforces the floor in validate_transaction; adjusts test helper constructor.
cmd/ethrex/l2/initializers.rs Plumbs CLI min-tip option into L2 BlockchainOptions.
cmd/ethrex/initializers.rs Plumbs CLI min-tip option into L1 BlockchainOptions.
cmd/ethrex/cli.rs Adds the new CLI flag/env var and defaults for mempool_min_tip.

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

Comment thread crates/blockchain/blockchain.rs Outdated
Comment on lines +2490 to +2503
// Admission-time minimum tip floor. For typed (1559-family) txs, the
// effective tip is `max_priority_fee_per_gas`. For legacy / EIP-2930
// it's `gas_price.saturating_sub(base_fee)`. A floor of 0 disables
// the check. Saturating arithmetic guards against pre-London headers
// where `base_fee_per_gas` is `None`.
if self.options.min_tip_wei > 0 {
let effective_tip = match tx.max_priority_fee() {
Some(tip) => tip,
None => {
let base_fee = header.base_fee_per_gas.unwrap_or(0);
let gas_price_u64 = u64::try_from(tx.gas_price()).unwrap_or(u64::MAX);
gas_price_u64.saturating_sub(base_fee)
}
};
Comment thread docs/CLI.md Outdated
[default: 10000]

--mempool.min-tip <MIN_TIP_WEI>
Minimum effective priority fee (in wei) required for a transaction to be admitted into the mempool. For typed transactions this is `max_priority_fee_per_gas`; for legacy transactions it is `gas_price - base_fee`. Set to 0 to disable the floor.
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.

Comment thread cmd/ethrex/cli.rs
Comment on lines +186 to +193
#[arg(
help = "Minimum effective priority fee (in wei) required for a transaction to be admitted into the mempool. For typed transactions this is `max_priority_fee_per_gas`; for legacy transactions it is `gas_price - base_fee`. Set to 0 to disable the floor.",
long = "mempool.min-tip",
default_value_t = DEFAULT_MIN_TIP_WEI,
value_name = "MIN_TIP_WEI",
help_heading = "Node options",
env = "ETHREX_MEMPOOL_MIN_TIP"
)]
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.

assert!(!matches!(res, Err(MempoolError::TipBelowMinimum { .. })));
}

#[tokio::test]
@greptile-apps
Copy link
Copy Markdown

greptile-apps Bot commented May 11, 2026

Greptile Summary

This PR adds a configurable minimum effective-tip floor to the mempool admission path, enforcing min_tip_wei (default 1 wei, matching geth's PriceLimit) before a transaction is accepted via RPC or P2P. Transactions submitted to default_with_store (the test-only constructor) get min_tip_wei = 0 to keep unrelated tests unaffected.

  • BlockchainOptions gains min_tip_wei: u64 with DEFAULT_MIN_TIP_WEI = 1; a floor of 0 disables the check entirely.
  • validate_transaction computes the effective tip per transaction type and rejects with MempoolError::TipBelowMinimum when below the floor; PrivilegedL2Transaction still bypasses the check via the existing early-return guard.
  • New --mempool.min-tip CLI flag / ETHREX_MEMPOOL_MIN_TIP env var plumbed through both L1 and L2 initializers, with five new integration tests covering the key admission cases.

Confidence Score: 4/5

Safe to merge; the admission gate is well-placed, the per-transaction-type tip computation is correct, and PrivilegedL2Transaction correctly bypasses the check via the existing early return.

The core logic is correct across all transaction types. Two minor points: the DEFAULT_MIN_TIP_WEI constant is placed after the impl Default block that references it (style), and the two negative-assertion tests pass even when an unrelated error fires, which could let a silent tip-check bypass go undetected.

The negative-assertion tests in test/tests/blockchain/mempool_tests.rs are worth a second look to ensure they actually catch a tip-check regression.

Important Files Changed

Filename Overview
crates/blockchain/blockchain.rs Adds min_tip_wei to BlockchainOptions, computes effective tip correctly per transaction type in validate_transaction, and sets min_tip_wei = 0 in default_with_store to decouple existing tests.
crates/blockchain/error.rs Adds TipBelowMinimum { actual: u64, limit: u64 } variant to MempoolError with a clear human-readable message.
test/tests/blockchain/mempool_tests.rs Five new tests cover zero-tip rejection, at-floor acceptance, floor=0 bypass, legacy tip calculation, and option-plumbing; negative-assertion tests pass even on unrelated errors, which is intentional but slightly masks regressions.
cmd/ethrex/cli.rs Adds mempool_min_tip: u64 CLI arg with --mempool.min-tip / ETHREX_MEMPOOL_MIN_TIP, defaulting to DEFAULT_MIN_TIP_WEI.
cmd/ethrex/initializers.rs Plumbs opts.mempool_min_tip into BlockchainOptions::min_tip_wei for the L1 path.
cmd/ethrex/l2/initializers.rs Plumbs opts.node_opts.mempool_min_tip into BlockchainOptions::min_tip_wei for the L2 path.
crates/common/types/constants.rs Updates doc comment on MIN_GAS_TIP to clarify its scope is only the RPC gas-price estimator, pointing to DEFAULT_MIN_TIP_WEI for the mempool concern.
docs/CLI.md Documents the new --mempool.min-tip option with its default value and env var.

Sequence Diagram

sequenceDiagram
    participant Caller as RPC / P2P
    participant BC as Blockchain::validate_transaction
    participant Opts as BlockchainOptions

    Caller->>BC: send raw transaction
    BC->>BC: early-return if PrivilegedL2Transaction
    BC->>BC: check init-code size, data size, gas limits
    BC->>BC: "check max_priority_fee <= max_fee_per_gas"
    BC->>Opts: read min_tip_wei
    alt "min_tip_wei > 0"
        BC->>BC: compute effective_tip
        alt "effective_tip < min_tip_wei"
            BC-->>Caller: Err(TipBelowMinimum)
        else "effective_tip >= min_tip_wei"
            BC->>BC: continue remaining checks
            BC-->>Caller: Ok(Option)
        end
    else "min_tip_wei == 0"
        BC->>BC: skip tip floor check
        BC-->>Caller: Ok(Option)
    end
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/blockchain.rs:240-256
The `DEFAULT_MIN_TIP_WEI` constant is declared after the `impl Default for BlockchainOptions` block that already references it. Rust resolves this at compile time without issue, but by convention constants should be defined before the types and impls that use them. Moving it above `BlockchainOptions` improves readability.

```suggestion
/// Default min-tip floor (wei). Matches geth's mempool `PriceLimit = 1 wei`.
/// Effectively just rejects zero-tip transactions at admission.
pub const DEFAULT_MIN_TIP_WEI: u64 = 1;

impl Default for BlockchainOptions {
    fn default() -> Self {
        Self {
            max_mempool_size: MAX_MEMPOOL_SIZE_DEFAULT,
            perf_logs_enabled: false,
            r#type: BlockchainType::default(),
            max_blobs_per_block: None,
            precompute_witnesses: false,
            precompile_cache_enabled: true,
            min_tip_wei: DEFAULT_MIN_TIP_WEI,
        }
    }
}
```

### Issue 2 of 2
test/tests/blockchain/mempool_tests.rs:504-511
**Negative assertions mask unrelated validation failures**

`at_floor_eip1559_passes_tip_check` and `floor_of_zero_admits_zero_tip` both use `assert!(!matches!(res, Err(MempoolError::TipBelowMinimum { .. })))`. This passes when `validate_transaction` returns `Ok(_)` OR when it returns any other `Err` variant. If a future refactor reorders checks and the tip guard is accidentally skipped while another error fires, these tests would still pass, providing false confidence. The real fix is to assert `Ok(_)` or to explicitly document that a different error is acceptable here.

Reviews (1): Last reviewed commit: "feat(l1): use DEFAULT_MIN_TIP_WEI consta..." | Re-trigger Greptile

Comment thread crates/blockchain/blockchain.rs Outdated
Comment on lines +240 to +256
@@ -242,10 +246,15 @@ impl Default for BlockchainOptions {
max_blobs_per_block: None,
precompute_witnesses: false,
precompile_cache_enabled: true,
min_tip_wei: DEFAULT_MIN_TIP_WEI,
}
}
}

/// Default min-tip floor (wei). Matches geth's mempool `PriceLimit = 1 wei`.
/// Effectively just rejects zero-tip transactions at admission.
pub const DEFAULT_MIN_TIP_WEI: u64 = 1;
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 The DEFAULT_MIN_TIP_WEI constant is declared after the impl Default for BlockchainOptions block that already references it. Rust resolves this at compile time without issue, but by convention constants should be defined before the types and impls that use them. Moving it above BlockchainOptions improves readability.

Suggested change
/// Default min-tip floor (wei). Matches geth's mempool `PriceLimit = 1 wei`.
/// Effectively just rejects zero-tip transactions at admission.
pub const DEFAULT_MIN_TIP_WEI: u64 = 1;
impl Default for BlockchainOptions {
fn default() -> Self {
Self {
max_mempool_size: MAX_MEMPOOL_SIZE_DEFAULT,
perf_logs_enabled: false,
r#type: BlockchainType::default(),
max_blobs_per_block: None,
precompute_witnesses: false,
precompile_cache_enabled: true,
min_tip_wei: DEFAULT_MIN_TIP_WEI,
}
}
}
Prompt To Fix With AI
This is a comment left during a code review.
Path: crates/blockchain/blockchain.rs
Line: 240-256

Comment:
The `DEFAULT_MIN_TIP_WEI` constant is declared after the `impl Default for BlockchainOptions` block that already references it. Rust resolves this at compile time without issue, but by convention constants should be defined before the types and impls that use them. Moving it above `BlockchainOptions` improves readability.

```suggestion
/// Default min-tip floor (wei). Matches geth's mempool `PriceLimit = 1 wei`.
/// Effectively just rejects zero-tip transactions at admission.
pub const DEFAULT_MIN_TIP_WEI: u64 = 1;

impl Default for BlockchainOptions {
    fn default() -> Self {
        Self {
            max_mempool_size: MAX_MEMPOOL_SIZE_DEFAULT,
            perf_logs_enabled: false,
            r#type: BlockchainType::default(),
            max_blobs_per_block: None,
            precompute_witnesses: false,
            precompile_cache_enabled: true,
            min_tip_wei: DEFAULT_MIN_TIP_WEI,
        }
    }
}
```

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

Note: If this suggestion doesn't match your team's coding style, reply to this and let me know. I'll remember it for next time!

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.

Comment on lines +504 to +511
Err(MempoolError::TipBelowMinimum {
actual: 0,
limit: 1_000_000,
}),
));
}

#[tokio::test]
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 Negative assertions mask unrelated validation failures

at_floor_eip1559_passes_tip_check and floor_of_zero_admits_zero_tip both use assert!(!matches!(res, Err(MempoolError::TipBelowMinimum { .. }))). This passes when validate_transaction returns Ok(_) OR when it returns any other Err variant. If a future refactor reorders checks and the tip guard is accidentally skipped while another error fires, these tests would still pass, providing false confidence. The real fix is to assert Ok(_) or to explicitly document that a different error is acceptable here.

Prompt To Fix With AI
This is a comment left during a code review.
Path: test/tests/blockchain/mempool_tests.rs
Line: 504-511

Comment:
**Negative assertions mask unrelated validation failures**

`at_floor_eip1559_passes_tip_check` and `floor_of_zero_admits_zero_tip` both use `assert!(!matches!(res, Err(MempoolError::TipBelowMinimum { .. })))`. This passes when `validate_transaction` returns `Ok(_)` OR when it returns any other `Err` variant. If a future refactor reorders checks and the tip guard is accidentally skipped while another error fires, these tests would still pass, providing false confidence. The real fix is to assert `Ok(_)` or to explicitly document that a different error is acceptable here.

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

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.

@github-actions
Copy link
Copy Markdown

🤖 Codex Code Review

Findings

  1. blockchain.rs: the new floor miscomputes the tip for typed transactions. For 1559/4844/7702 txs, the current effective tip is min(max_priority_fee_per_gas, max_fee_per_gas - base_fee_per_gas) and can be non-includable when max_fee_per_gas < base_fee_per_gas; the codebase already models that in transaction.rs. This patch uses max_priority_fee_per_gas directly, so a tx with tip_cap >= min_tip_wei but fee_cap < base_fee + min_tip_wei still passes admission. If fee_cap < base_fee, it then sits in the mempool because builder filtering rejects effective_gas_tip == None in mempool.rs. If base_fee <= fee_cap < base_fee + floor, it can still be locally built below the configured floor because payload building currently has no separate tip filter in payload.rs. The new tests only cover headers with base_fee_per_gas = None in mempool_tests.rs, mempool_tests.rs, so they miss this London-era case. This should use tx.effective_gas_tip(header.base_fee_per_gas) and the CLI/docs wording in cli.rs and CLI.md should be updated to the same formula.

No other independent correctness/security issues stood out in the option plumbing. I couldn’t run cargo test here because this sandbox blocks the needed network and writes under ~/.cargo/~/.rustup.


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

ilitteri added 5 commits May 11, 2026 19:19
Replaces the inline match-with-fallback that computed the effective tip
differently for typed (raw max_priority_fee_per_gas) vs legacy
(gas_price - base_fee) transactions. The previous code was asymmetric:
legacy subtracted base_fee while typed did not, so a typed tx with
max_priority_fee = 5 gwei but max_fee = base_fee + 0 (i.e. effective
inclusion tip of 0) would pass the floor.

Defers to `Transaction::effective_gas_tip(base_fee)` (already in the
codebase at `crates/common/types/transaction.rs:1504`), which implements
the EIP-1559 definition `min(max_priority_fee_per_gas, max_fee_per_gas
- base_fee)` for every transaction type. `None` from the helper means
the tx cannot pay for inclusion at the current base fee; treat that as
effective tip = 0 for the floor comparison.

Tests:
- `shipped_default_floor_rejects_zero_tip_admits_one` pins the actual
  shipped default (`DEFAULT_MIN_TIP_WEI = 1`, matching geth's
  `PriceLimit`). Previous suite only exercised manually-set floors
  (1 Mwei / 0 / 5 gwei).
- `blob_tx_under_floor_rejected` confirms the EIP-4844 path also goes
  through the helper.
Cross-client audit found that geth, reth, and erigon all apply their
min-tip floor (PriceLimit / minimum_priority_fee) to the **raw**
priority fee cap — `tx.GasTipCap()` in geth, which returns
`max_priority_fee_per_gas` for typed txs and `gas_price` for legacy.
Only nethermind's `MinGasPriceTxFilter` (which lives in the block
producer, not txpool admission) uses an effective-tip computation
against the current base fee.

The previous implementation used `Transaction::effective_gas_tip(base_fee)`
= `min(max_priority_fee_per_gas, max_fee_per_gas - base_fee)`. With a
non-trivial floor (e.g. 1 gwei), this would reject otherwise-valid txs
at ingress as base fee oscillates upward and `fee_cap - base_fee` falls
below the floor. With the 1-wei shipped default this difference is
academic, but the semantics should match peers so operators tuning the
flag don't see surprising base-fee-dependent rejections.

Switch to `tx.gas_tip_cap()` — already U256, already returns gas_price
for legacy via Transaction's existing accessor. The check is now
independent of the current base fee.

Tests:
- All existing assertions still hold (every test built the tx with
  base_fee = None on the header, so effective_gas_tip already
  returned gas_tip_cap unchanged).
- Renamed `legacy_effective_tip_below_floor_rejected` →
  `legacy_gas_price_below_floor_rejected` to reflect the actual
  measured quantity.
- Updated comments on the blob and legacy test cases.
After the cross-client fix switched the floor from effective tip
(`min(tip_cap, fee_cap - base_fee)`) to raw `gas_tip_cap()`, the help
text in cli.rs and docs/CLI.md still described the old semantics, and
the error message still said "Effective tip". Updated all three to
match: the floor is now compared against `max_priority_fee_per_gas`
for typed txs and `gas_price` for legacy, independent of current base
fee. Also reworded the error variant to "Tip cap … below the
configured minimum".

Addresses @copilot's review comments on cli.rs:193 and docs/CLI.md:95.
…ses it

@greptile: by convention constants should be defined before the types
and impls that reference them. Moved `DEFAULT_MIN_TIP_WEI` above
`BlockchainOptions` so the reading order matches the dependency order.
No behavior change.
@greptile (P2): the three min-tip tests using `assert!(!matches!(res,
Err(TipBelowMinimum {..})))` accidentally pass on ANY non-tip error
(NotEnoughBalance, StoreError, etc.), so a future refactor that
skips the tip check entirely would not be caught.

Replaced with positive assertions: at the floor / floor-of-zero / 1-wei
default, the tip check must pass and the next-stage account-lookup
error (`NotEnoughBalance` or `StoreError` when state isn't seeded)
must fire. If the tip check is accidentally skipped, the assertion
still requires a specific downstream error — that's the regression
guard greptile asked for.
ilitteri added a commit that referenced this pull request May 12, 2026
Phase 2 review (greptile + Copilot) flagged three related issues:
- `_locals_exempt` was a dead binding silenced by an underscore prefix;
  if PR #6604 forgot to remove the underscore when wiring the gate, the
  exemption would silently never fire.
- `--mempool.nolocals` CLI flag and `docs/CLI.md` entry implied the
  flag changes behavior, but with no origin-gated rule on main it's a
  no-op — an operator-visible knob that does nothing.
- `BlockchainOptions::nolocals` field had no consumers.

Strip the scaffold: keep the `TxOrigin` enum and the `origin` parameter
threading through `validate_transaction` (those are real plumbing, not
dead code), but drop the dead variable, the unused option, the no-op
CLI flag, and the two docs entries. The `TODO(#6604)` comment in
`validate_transaction` records exactly how the exemption should be
wired (and that the flag should land in the same PR).
…floor

# Conflicts:
#	crates/blockchain/error.rs
@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)

The reorgs simulator generates EIP-1559 transactions with
max_priority_fee_per_gas = 0, which would be rejected by the new mempool
min-tip floor (default 1 wei). Pass --mempool.min-tip=0 to the spawned ethrex
so reorg tests continue to exercise low-fee tx flows independent of the
admission floor.
@ilitteri ilitteri requested a review from a team as a code owner May 14, 2026 14:14
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.

2 participants