Skip to content

feat(l1): add TxOrigin enum to thread RPC vs P2P submission through admission#6608

Open
ilitteri wants to merge 7 commits into
mainfrom
feat/mempool-tx-origin
Open

feat(l1): add TxOrigin enum to thread RPC vs P2P submission through admission#6608
ilitteri wants to merge 7 commits into
mainfrom
feat/mempool-tx-origin

Conversation

@ilitteri
Copy link
Copy Markdown
Collaborator

@ilitteri ilitteri commented May 12, 2026

Motivation

PR #6604 introduces an admission-time minimum tip floor that rejects P2P-received transactions whose effective gas tip is below an operator-configured threshold. The floor is the correct behavior for spam-sensitive ingress but is too restrictive for operator-controlled workflows (sequencers, internal services, key-rotation tooling) that submit transactions directly via RPC.

This PR adds the language and plumbing to distinguish those origins so the floor (and other future operator-friendly exemptions) can opt locally-submitted transactions out by default.

Description

  • New pub enum TxOrigin { Local, External } in crates/blockchain/mempool.rs. Orthogonal to PR feat(l1): add --mempool.private flag for non-propagating local txs #6576's is_private propagation flag.
  • Blockchain::validate_transaction gains an origin: TxOrigin parameter (marked _origin until feat(l1): enforce minimum priority-fee floor at mempool admission #6604 wires the exemption). Existing P2P entry points (add_transaction_to_pool, add_blob_transaction_to_pool) pass External; new add_local_transaction_to_pool and add_local_blob_transaction_to_pool helpers pass Local. Both are thin wrappers around shared private routes.
  • eth_sendRawTransaction now routes through the local variants.
  • validate_transaction carries a TODO(#6604) block marking exactly where the min-tip floor check should be gated once PR feat(l1): enforce minimum priority-fee floor at mempool admission #6604 merges. The opt-out operator flag (e.g. --mempool.nolocals) is intentionally NOT added here — exposing a no-op operator knob would be confusing. The flag should land in the same PR that wires the exemption.

Behavioral change

This PR is behavior-neutral on main by itself — it only adds the type and threading. The min-tip floor exemption activates when PR #6604 merges and the TODO(#6604) is wired (a one-line if wrap), at which point the companion operator flag also lands.

Deferred

Introduce a `TxOrigin::{Local, External}` enum that tags transactions
entering the mempool by their provenance (RPC vs P2P). Thread the
origin through `Blockchain::validate_transaction` and the four pool
entry points: `add_transaction_to_pool` and
`add_blob_transaction_to_pool` (P2P, `External`) plus new
`add_local_transaction_to_pool` and `add_local_blob_transaction_to_pool`
helpers (RPC, `Local`). `eth_sendRawTransaction` now routes through
the local variants.

Add `BlockchainOptions::nolocals` (default `false`) and a
`--mempool.nolocals` CLI flag (env `ETHREX_MEMPOOL_NOLOCALS`,
documented in `docs/CLI.md`) plumbed through both the L1 and L2
initializers. When set, locally-submitted txs are treated as
external for admission purposes.

`validate_transaction` computes `locals_exempt = !nolocals && origin
== Local` and carries a `TODO(#6604)` noting where the min-tip floor
exemption is wired once PR #6604 lands. Until then, the structure is
in place and behavior is unchanged for any check present on `main`.

Tests in `test/tests/blockchain/mempool_tests.rs` exercise the
threading: both origins reach the same downstream errors today, and
`add_local_transaction_to_pool` is shown to route through validation.
Copilot AI review requested due to automatic review settings May 12, 2026 02:37
@ilitteri ilitteri requested review from a team, ManuelBilbao and avilagaston9 as code owners May 12, 2026 02:37
@github-actions github-actions Bot added the L1 Ethereum client label May 12, 2026
@github-actions
Copy link
Copy Markdown

🤖 Kimi Code Review

This PR introduces infrastructure for origin-aware mempool admission policies (distinguishing RPC-submitted vs P2P-received transactions). The implementation is clean and well-documented.

Minor observations:

  1. Unused variable intentional but noted (crates/blockchain/blockchain.rs:2490)
    The _locals_exempt variable is correctly prefixed with underscore since it's currently unused (pending PR feat(l1): enforce minimum priority-fee floor at mempool admission #6604). Ensure the underscore is removed when the TODO is resolved.

  2. Test specificity (test/tests/blockchain/mempool_tests.rs:507-509)
    The test add_local_transaction_to_pool_routes_through_validation asserts is_err() but the comment mentions expecting NotEnoughBalance. Consider matching the specific error type to prevent regression if validation ordering changes:

    assert!(matches!(result, Err(MempoolError::NotEnoughBalance { .. })));

Security/Logic verification:

  • The exemption logic at line 2490 is correct: !self.options.nolocals && origin == TxOrigin::Local properly implements "locals are exempt by default, unless --mempool.nolocals is set".
  • The RPC handler correctly routes sendRawTransaction through add_local_* methods, ensuring local transactions are tagged appropriately.
  • No consensus-critical changes; this affects only mempool admission policy.

Code quality:

  • Good documentation on the TxOrigin enum explaining the distinction from propagation policy.
  • Clean API design with add_transaction_to_pool (external) and add_local_transaction_to_pool (local) providing clear entry points while sharing implementation via the _with_origin helpers.
  • CLI help text clearly explains the security/operational trade-off of the flag.

Approval: LGTM. The preparatory structure is sound and ready for the min-tip floor logic in the follow-up PR.


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

@github-actions
Copy link
Copy Markdown

🤖 Claude Code Review

Now I have everything I need to write a thorough review.


PR Review: feat(l1): TxOrigin enum and local-tx exemption from min-tip floor

Summary

This is a clean, well-scoped infrastructure PR. The design is sound — orthogonal enum, thin public wrappers, private _with_origin helper, and a clearly-marked TODO for the follow-on gating. No correctness bugs found. A few items worth addressing:


Items

1. TxOrigin is missing Hash and #[non_exhaustive]

crates/blockchain/mempool.rs — the new enum:

#[derive(Debug, Clone, Copy, PartialEq, Eq)]
pub enum TxOrigin { Local, External }
  • Missing Hash: since the type derives Eq + Copy, adding Hash is essentially free and lets it be used as a HashMap/HashSet key without future friction.
  • TxOrigin::Private is explicitly planned for T3.4. Marking the enum #[non_exhaustive] now avoids a future breaking change in downstream match arms.

Suggested:

#[derive(Debug, Clone, Copy, PartialEq, Eq, Hash)]
#[non_exhaustive]
pub enum TxOrigin { ... }

2. validate_transaction is a public, breaking API change

crates/blockchain/blockchain.rs:2475 — adding origin: TxOrigin as a required parameter to a pub async fn is a semver-breaking change. The PR correctly updates all in-tree call sites (tests pass), but any downstream crate depending on ethrex_blockchain will need a migration. If this library is published to crates.io or consumed externally, a semver major bump or a deprecation bridge (origin: Option<TxOrigin>) would be appropriate. At minimum, the changelog/PR description should note this explicitly.


3. Dead variable _locals_exempt is intentional but carries semantic risk

blockchain.rs:2489:

let _locals_exempt = !self.options.nolocals && origin == TxOrigin::Local;

The _ prefix suppresses the warning and the TODO comment is clear. The concern is that if PR #6604 wires the floor check and forgets to rename _locals_exemptlocals_exempt (dropping the suppression prefix), the exemption silently vanishes and the variable compiles cleanly. Consider adding an #[allow(unused_variables)]-style safety net in the TODO comment, or making the _locals_exempt binding a named helper (compute_locals_exempt()) so the grep surface is clearer for the follow-on PR author.


4. Weak assertion in add_local_transaction_to_pool_routes_through_validation

test/tests/blockchain/mempool_tests.rs:517:

assert!(result.is_err(), "local tx must be rejected by validation");

The other new test (validate_transaction_accepts_both_origins) uses matches! to assert the specific error variant. This test only asserts is_err(), which would pass even if the transaction were rejected for a completely unrelated reason (e.g., a serialisation issue in decode_canonical). The comment even notes the expected error: "The fixture tx has chain_id None, so it should hit NotEnoughBalance". Pinning to the expected variant makes the test self-documenting and prevents false passes:

assert!(matches!(result, Err(MempoolError::NotEnoughBalance { .. })));

5. --mempool.nolocals default not shown in CLI.md

docs/CLI.md — the two new entries for --mempool.nolocals don't include a [default: false] line, while other flags (e.g., --mempool.max-size) show [default: 10000]. If CLI.md is generated automatically this is fine, but if it is maintained manually it should be updated for consistency.


Minor / Nit

  • The RPC comment at transaction.rs:609 says the floor is "intended to protect against P2P spam". The min-tip floor equally guards block-builder economics (no incentive to build near-zero-fee blocks). The framing is not wrong but is slightly narrow.
  • add_transaction_to_pool_with_origin and add_blob_transaction_to_pool_with_origin are correctly async fn (non-pub). No concerns there.
  • nolocals field naming aligns with geth's --txpool.nolocals. Good consistency choice.

Overall

The implementation is correct and the design is solid. Point 1 (#[non_exhaustive] + Hash) and Point 4 (stronger assertion) are the most actionable changes; the rest are low-severity. Happy to approve once those are addressed.


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

@github-actions
Copy link
Copy Markdown

github-actions Bot commented May 12, 2026

Lines of code report

Total lines added: 57
Total lines removed: 0
Total lines changed: 57

Detailed view
+-------------------------------------------------+-------+------+
| File                                            | Lines | Diff |
+-------------------------------------------------+-------+------+
| ethrex/crates/blockchain/blockchain.rs          | 2549  | +41  |
+-------------------------------------------------+-------+------+
| ethrex/crates/blockchain/mempool.rs             | 469   | +5   |
+-------------------------------------------------+-------+------+
| ethrex/crates/networking/rpc/eth/transaction.rs | 588   | +11  |
+-------------------------------------------------+-------+------+

@greptile-apps
Copy link
Copy Markdown

greptile-apps Bot commented May 12, 2026

Greptile Summary

This PR introduces TxOrigin { Local, External } to distinguish RPC-submitted from P2P-received transactions at the mempool admission boundary, and wires --mempool.nolocals to opt out of the exemption. The change is behavior-neutral on main; the actual min-tip floor bypass activates when PR #6604 lands and the TODO(#6604) guard is unwrapped.

  • TxOrigin enum added to mempool.rs; validate_transaction gains an origin parameter and computes _locals_exempt as placeholder scaffolding for the floor check deferred to feat(l1): enforce minimum priority-fee floor at mempool admission #6604.
  • add_local_transaction_to_pool / add_local_blob_transaction_to_pool wrappers introduced; eth_sendRawTransaction now routes through the local variants while P2P paths retain TxOrigin::External.
  • BlockchainOptions::nolocals (default false) surfaced as --mempool.nolocals / ETHREX_MEMPOOL_NOLOCALS and threaded through both L1 and L2 initializers.

Confidence Score: 4/5

Safe to merge; the change is behavior-neutral on main and the exemption only activates after PR #6604 lands.

The plumbing is clean, all P2P and RPC call sites are correctly updated, and both L1/L2 initializers thread the new flag. The only open items are that _locals_exempt is computed but never read (intentional scaffolding), and the architecture-doc snippet for validate_transaction is now further out of date than before the PR.

docs/l1/architecture/crate_map.md — the validate_transaction public API snippet is stale and now diverges more from reality.

Important Files Changed

Filename Overview
crates/blockchain/mempool.rs Adds the TxOrigin enum with Local / External variants; clean, well-documented addition with no logic changes.
crates/blockchain/blockchain.rs Adds BlockchainOptions::nolocals, add_local_transaction_to_pool, add_local_blob_transaction_to_pool, and the origin: TxOrigin parameter to validate_transaction; _locals_exempt is computed but intentionally unused (scaffolding for #6604).
crates/networking/rpc/eth/transaction.rs Routes eth_sendRawTransaction through add_local_*_to_pool helpers, correctly tagging RPC-submitted transactions as TxOrigin::Local.
cmd/ethrex/cli.rs Adds --mempool.nolocals / ETHREX_MEMPOOL_NOLOCALS CLI flag with correct default (false) and proper help_heading.
cmd/ethrex/initializers.rs Threads nolocals: opts.mempool_nolocals into BlockchainOptions during L1 init; one-line plumbing change.
cmd/ethrex/l2/initializers.rs Same nolocals plumbing for L2 init path.
test/tests/blockchain/mempool_tests.rs Updates existing test calls to pass TxOrigin::External; adds three new scaffolding tests. The add_local_transaction_to_pool_routes_through_validation assertion is result.is_err() only (no specific error variant check), which is intentionally loose.
docs/CLI.md Adds --mempool.nolocals entry in both the L1 and L2 sections of the CLI reference; consistent with the arg definition.

Flowchart

%%{init: {'theme': 'neutral'}}%%
flowchart TD
    A[eth_sendRawTransaction] -->|EIP-4844| B[add_local_blob_transaction_to_pool]
    A -->|other tx types| C[add_local_transaction_to_pool]
    B --> D[add_blob_transaction_to_pool_with_origin\norigin = Local]
    C --> E[add_transaction_to_pool_with_origin\norigin = Local]
    F[P2P peer] -->|rlpx/transactions| G[add_transaction_to_pool\norigin = External]
    F -->|rlpx/server| G
    G --> E
    D --> H[validate_transaction\ntx, sender, origin]
    E --> H
    H --> I{locals_exempt?\n!nolocals and origin==Local}
    I -->|true - future: skip min-tip floor| J[TODO #6604: if !locals_exempt\ncheck min-tip floor]
    I -->|false - always enforce| J
    J --> K[remaining admission checks\nnonce, gas, balance, ...]
Loading

Comments Outside Diff (1)

  1. docs/l1/architecture/crate_map.md, line 108 (link)

    P2 Stale public-API snippet in architecture docs

    The snippet still shows the old two-argument, synchronous validate_transaction signature (tx only, returning Result<(), MempoolError>). After this PR the real signature is pub async fn validate_transaction(&self, tx: &Transaction, sender: Address, origin: TxOrigin) -> Result<Option<H256>, MempoolError>. The signature had already drifted (missing sender) before this PR landed, but the new origin parameter widens the gap further. Any reader relying on this for integration or review will be misled.

    Prompt To Fix With AI
    This is a comment left during a code review.
    Path: docs/l1/architecture/crate_map.md
    Line: 108
    
    Comment:
    **Stale public-API snippet in architecture docs**
    
    The snippet still shows the old two-argument, synchronous `validate_transaction` signature (`tx` only, returning `Result<(), MempoolError>`). After this PR the real signature is `pub async fn validate_transaction(&self, tx: &Transaction, sender: Address, origin: TxOrigin) -> Result<Option<H256>, MempoolError>`. The signature had already drifted (missing `sender`) before this PR landed, but the new `origin` parameter widens the gap further. Any reader relying on this for integration or review will be misled.
    
    How can I resolve this? If you propose a fix, please make it concise.
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
docs/l1/architecture/crate_map.md:108
**Stale public-API snippet in architecture docs**

The snippet still shows the old two-argument, synchronous `validate_transaction` signature (`tx` only, returning `Result<(), MempoolError>`). After this PR the real signature is `pub async fn validate_transaction(&self, tx: &Transaction, sender: Address, origin: TxOrigin) -> Result<Option<H256>, MempoolError>`. The signature had already drifted (missing `sender`) before this PR landed, but the new `origin` parameter widens the gap further. Any reader relying on this for integration or review will be misled.

### Issue 2 of 2
crates/blockchain/blockchain.rs:2493
**Unused variable `_locals_exempt` silences dead-code lint**

`_locals_exempt` is computed but never read; the leading underscore is the only thing preventing a compiler warning. While the PR description explains this is intentional scaffolding for PR #6604, the dead binding means the expression `!self.options.nolocals && origin == TxOrigin::Local` is never exercised in any code path today. If the follow-up PR forgets to remove the underscore when wiring the `if !_locals_exempt` guard, the exemption will silently remain dead. Prefer an explicit `#[allow(unused_variables)]` with an `// until #6604` annotation, or a `let _ = ...;` assignment, to make the intentional deferral even more visible to reviewers.

Reviews (1): Last reviewed commit: "feat(l1): add TxOrigin enum and local-tx..." | Re-trigger Greptile

Comment thread crates/blockchain/blockchain.rs Outdated
// `locals_exempt` gates origin-aware exemptions. When the operator sets
// `--mempool.nolocals`, locally-submitted txs are treated like external
// ones for admission purposes.
let _locals_exempt = !self.options.nolocals && origin == TxOrigin::Local;
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 Unused variable _locals_exempt silences dead-code lint

_locals_exempt is computed but never read; the leading underscore is the only thing preventing a compiler warning. While the PR description explains this is intentional scaffolding for PR #6604, the dead binding means the expression !self.options.nolocals && origin == TxOrigin::Local is never exercised in any code path today. If the follow-up PR forgets to remove the underscore when wiring the if !_locals_exempt guard, the exemption will silently remain dead. Prefer an explicit #[allow(unused_variables)] with an // until #6604 annotation, or a let _ = ...; assignment, to make the intentional deferral even more visible to reviewers.

Prompt To Fix With AI
This is a comment left during a code review.
Path: crates/blockchain/blockchain.rs
Line: 2493

Comment:
**Unused variable `_locals_exempt` silences dead-code lint**

`_locals_exempt` is computed but never read; the leading underscore is the only thing preventing a compiler warning. While the PR description explains this is intentional scaffolding for PR #6604, the dead binding means the expression `!self.options.nolocals && origin == TxOrigin::Local` is never exercised in any code path today. If the follow-up PR forgets to remove the underscore when wiring the `if !_locals_exempt` guard, the exemption will silently remain dead. Prefer an explicit `#[allow(unused_variables)]` with an `// until #6604` annotation, or a `let _ = ...;` assignment, to make the intentional deferral even more visible to reviewers.

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.

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 introduces transaction-origin plumbing (Local vs External) to support origin-aware mempool admission policies (notably a future “local tx” exemption from the min-tip floor), plus an operator flag (--mempool.nolocals) to disable those exemptions.

Changes:

  • Add TxOrigin { Local, External } and thread it through Blockchain::validate_transaction and mempool admission entry points.
  • Route eth_sendRawTransaction through new “local” add-to-pool APIs so RPC submissions are tagged Local.
  • Add BlockchainOptions::nolocals, CLI plumbing, docs updates, and a few tests covering the new origin parameter and local entry point.

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 Updates validation calls to pass TxOrigin and adds new tests for origin plumbing/local entry point.
docs/CLI.md Documents new --mempool.nolocals flag (L1 and L2 sections).
crates/networking/rpc/eth/transaction.rs Routes eth_sendRawTransaction through local mempool entry points (including blob tx path).
crates/blockchain/mempool.rs Introduces the TxOrigin enum.
crates/blockchain/blockchain.rs Adds nolocals option, introduces local/external add-to-pool wrappers, and extends validate_transaction with origin.
cmd/ethrex/l2/initializers.rs Plumbs CLI mempool_nolocals into L2 blockchain options.
cmd/ethrex/initializers.rs Plumbs CLI mempool_nolocals into L1 blockchain options.
cmd/ethrex/cli.rs Adds --mempool.nolocals / ETHREX_MEMPOOL_NOLOCALS option.

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

Comment on lines 609 to 618
// RPC-submitted transactions are tagged as `TxOrigin::Local` so they may
// bypass admission gates (such as the min-tip floor) intended to protect
// against P2P spam. See `Blockchain::add_local_transaction_to_pool`.
let hash = if let SendRawTransactionRequest::EIP4844(wrapped_blob_tx) = self {
context
.blockchain
.add_blob_transaction_to_pool(
.add_local_blob_transaction_to_pool(
wrapped_blob_tx.tx.clone(),
wrapped_blob_tx.blobs_bundle.clone(),
)
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 test/tests/blockchain/mempool_tests.rs Outdated
Comment on lines +511 to +529
// Threading check: the RPC entry point must route through validation. A
// transaction whose gas limit exceeds the block's must be rejected with
// `TxGasLimitExceededError`, proving that we did not accidentally bypass
// `validate_transaction` in `add_local_transaction_to_pool`.
let (config, header) = build_basic_config_and_header(false, false);
let store = setup_storage(config, header).await.expect("Storage setup");
let blockchain = Blockchain::default_with_store(store);

// A canonical legacy tx (sender derivable from signature) with `gas_limit`
// beyond the test block's limit (100_000_000). Reused from the existing
// mempool fixtures (`test_filter_mempool_transactions`).
let tx = Transaction::decode_canonical(&hex::decode("f86d80843baa0c4082f618946177843db3138ae69679a54b95cf345ed759450d870aa87bee538000808360306ba0151ccc02146b9b11adf516e6787b59acae3e76544fdcd75e77e67c6b598ce65da064c5dd5aae2fbb535830ebbdad0234975cd7ece3562013b63ea18cc0df6c97d4").unwrap()).unwrap();

let result = blockchain.add_local_transaction_to_pool(tx).await;
// The fixture tx has chain_id None, so it should hit NotEnoughBalance
// (sender not in storage) — same outcome as `add_transaction_to_pool`.
// The point of the assertion is that the call returns an error from
// `validate_transaction` rather than silently inserting.
assert!(result.is_err(), "local tx must be rejected by validation");
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 docs/CLI.md Outdated
[default: 10000]

--mempool.nolocals
Disable admission exemptions for locally-submitted transactions. When set, RPC-submitted txs are subject to the same admission gates (e.g. min-tip floor) as P2P-received ones.
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 Outdated
Comment on lines +186 to +195
#[arg(
help = "Disable admission exemptions for locally-submitted transactions. \
When set, RPC-submitted txs are subject to the same admission gates \
(e.g. min-tip floor) as P2P-received ones.",
long = "mempool.nolocals",
default_value_t = false,
help_heading = "Node options",
env = "ETHREX_MEMPOOL_NOLOCALS"
)]
pub mempool_nolocals: bool,
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

  1. Treating every eth_sendRawTransaction as TxOrigin::Local in crates/networking/rpc/eth/transaction.rs creates the wrong trust boundary. Local is documented as an operator-controlled RPC client in crates/blockchain/mempool.rs, but the normal HTTP RPC is unauthenticated, bound to 0.0.0.0 by default in cmd/ethrex/cli.rs, and served with permissive CORS in crates/networking/rpc/rpc.rs. The bypass is latent today because no origin-gated rule is wired yet, but once that lands, any remote client that can reach JSON-RPC will qualify for the “local” exemption. Local should be tied to an authenticated or loopback/private transport, not just the RPC entrypoint.

  2. --mempool.nolocals is currently advertised as functional, but the implementation is still a no-op. The flag is exposed in cmd/ethrex/cli.rs and documented in docs/CLI.md, while validate_transaction only computes _locals_exempt and leaves a TODO without using it in any admission branch in crates/blockchain/blockchain.rs. The new test even codifies that Local and External behave identically today in test/tests/blockchain/mempool_tests.rs. Either land the first origin-gated rule in the same PR or make the help/docs explicit that this is preparatory plumbing.

  3. The new routing test does not actually validate what its comments claim. In test/tests/blockchain/mempool_tests.rs, the comments talk about TxGasLimitExceededError, but the fixture decoded at test/tests/blockchain/mempool_tests.rs does not have a gas limit above the block limit, and the assertion at test/tests/blockchain/mempool_tests.rs is only result.is_err(). That will pass on unrelated failures and would not catch the local/external routing breaking. Use a deterministic fixture and assert the exact MempoolError.

No EVM opcode, gas-accounting, consensus, trie, or RLP issues stood out in this diff beyond the mempool admission boundary above.

I couldn’t run the Rust tests here because cargo test failed when rustup tried to create temp files under /home/runner/.rustup, which is read-only in this environment.


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

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).
ilitteri added 2 commits May 12, 2026 15:21
Phase 2 review (Copilot): `eth_sendRawTransaction` called
`Blockchain::add_local_blob_transaction_to_pool` unconditionally for
EIP-4844 requests, but that method is `#[cfg(feature = "c-kzg")]` in
`ethrex-blockchain`. Building `ethrex-rpc` without enabling the c-kzg
feature failed to compile because the called method didn't exist.

Add a forwarding `c-kzg` feature on `ethrex-rpc` and `#[cfg]`-gate the
EIP-4844 arm. When the feature is disabled, the handler returns a
clear `RpcErr::Internal` instead of failing to build.

Verified both `cargo check -p ethrex-rpc --no-default-features` and
`cargo check -p ethrex-rpc --features c-kzg` succeed.
Phase 2 review (Copilot):
- The previous test comment claimed the legacy fixture's `gas_limit`
  exceeded the block limit, but the RLP carries `0xF618` (63K gas),
  well below the 100M block cap. The fixture never exercised
  `TxGasLimitExceededError`.
- `assert!(result.is_err())` was too loose — it would pass on any
  error, including ones unrelated to `validate_transaction`.

Tighten the assertion to `NotEnoughBalance` OR `StoreError(_)` so the
test specifically proves admission reached the validation path. The
minimal test store doesn't seed account state, so the lookup may
surface as either variant; what we need is for the call to NOT return
`Ok(_)`. Updated the comment to match the actual fixture.

Also drop the companion `blockchain_options_default_keeps_local_exemption`
test that referenced the removed `nolocals` field.
@ilitteri ilitteri changed the title feat(l1): TxOrigin enum and local-tx exemption from min-tip floor feat(l1): add TxOrigin enum to thread RPC vs P2P submission through admission May 13, 2026
@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)

ilitteri added 2 commits May 13, 2026 18:31
When this branch added TxOrigin to validate_transaction, a tx-size test
carried over from main was left on the old 2-arg form. The test is
unrelated to origin-specific behavior, so use TxOrigin::External to match
the P2P-default path other tests in this file use.
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