Skip to content

feat(l1): add --mempool.private flag for non-propagating local txs#6576

Open
ilitteri wants to merge 9 commits into
mainfrom
feat/mempool-private
Open

feat(l1): add --mempool.private flag for non-propagating local txs#6576
ilitteri wants to merge 9 commits into
mainfrom
feat/mempool-private

Conversation

@ilitteri
Copy link
Copy Markdown
Collaborator

@ilitteri ilitteri commented May 6, 2026

Motivation

Mirror reth's --txpool.no-local-transactions-propagation: a flag to keep RPC-submitted transactions private to this node so they're only included in blocks built locally — no peer gossip. Use cases:

  • Operators running a private validator wanting MEV-protected ordering
  • Devnet A/B tests where one node should never propagate
  • Searcher / sequencer setups separating local vs public mempools

Description

New flag --mempool.private (env ETHREX_MEMPOOL_PRIVATE, off by default), placed under Node options next to the existing --mempool.maxsize. When enabled, transactions submitted via this node's eth_sendRawTransaction enter the mempool and may be included in locally-built blocks but are NOT inserted into the broadcast pool. P2P-received transactions are unaffected.

Plumbing follows the no-feature-gated-args pattern (separate methods + shared private helper):

  • BlockchainOptions::private_mempool: bool (default false).
  • Mempool gets a sibling add_transaction_no_broadcast next to add_transaction; both share a private add_transaction_inner(broadcast: bool) so the broadcast-pool insertion is the only behavioral difference.
  • Blockchain::add_local_transaction_to_pool and add_local_blob_transaction_to_pool are the RPC entry points — they consult options.private_mempool and route to the no-broadcast variant when set. The existing add_transaction_to_pool (used by the P2P / sync paths) keeps current behavior and always broadcasts.
  • eth_sendRawTransaction handler now calls the local variants.

P2P-gossip race + perf hardening

The new-peer pooled-hash dump (send_all_pooled_tx_hashes) and the local-submission early-return both have to handle the case where a tx is already in the public pool via gossip:

  • Mempool::get_txs_for_new_peer_dump returns the broadcast-eligible snapshot under a single read lock — replaces the per-tx is_private lock the previous loop took, which under a hot pool would serialize peer handshakes against any mempool write.
  • add_transaction_to_pool_inner (+ EIP-4844 path) logs a warn! when contains_tx short-circuits with broadcast=false. If the tx has already been seen via gossip, the local --mempool.private request cannot retroactively un-broadcast it; the log surfaces this to the operator. State is intentionally unchanged — mutating the existing entry would itself violate the original submitter's intent.

Checklist

  • Updated STORE_SCHEMA_VERSION — N/A

Mirrors reth's --txpool.no-local-transactions-propagation. When set,
transactions submitted via this node's eth_sendRawTransaction enter
the mempool and may be included in blocks built locally, but their
hashes are NOT inserted into the broadcast pool — peers never see
them via Transactions / NewPooledTransactionHashes. Transactions
received from peers continue to propagate as before.

Plumbing
--------

- BlockchainOptions gains a `private_mempool: bool` (default false).
- Mempool gets a sibling `add_transaction_no_broadcast` that skips
  the broadcast_pool insertion. Both methods share an internal
  `add_transaction_inner(broadcast: bool)`.
- Blockchain exposes `add_local_transaction_to_pool` and
  `add_local_blob_transaction_to_pool` for the RPC entry point.
  These check `options.private_mempool` and route to the
  no-broadcast variant when set; the existing
  `add_transaction_to_pool` (used by P2P/sync paths) keeps current
  behavior and always broadcasts.
- The eth_sendRawTransaction handler now calls the local variants.
- CLI flag: `--mempool.private` (env: ETHREX_MEMPOOL_PRIVATE),
  matching the existing `--mempool.maxsize` prefix.

Test
----

`add_transaction_no_broadcast_keeps_tx_out_of_broadcast_pool` shows
that a tx routed via the no-broadcast path is queryable from the
mempool but absent from `get_txs_for_broadcast`.
Copilot AI review requested due to automatic review settings May 6, 2026 12:05
@ilitteri ilitteri requested review from a team, ManuelBilbao and avilagaston9 as code owners May 6, 2026 12:05
@github-actions github-actions Bot added the L1 Ethereum client label May 6, 2026
@github-actions
Copy link
Copy Markdown

github-actions Bot commented May 6, 2026

🤖 Kimi Code Review

This PR implements a private mempool feature (mirroring Reth's --txpool.no-local-transactions-propagation) that prevents RPC-submitted transactions from being gossiped to peers while keeping them available for local block building. The implementation is correct and well-structured.

Summary of Changes

The PR cleanly separates transaction ingestion paths:

  1. P2P path: Uses existing add_transaction_to_pool / add_blob_transaction_to_pool methods → always broadcasts
  2. RPC path: New add_local_transaction_to_pool / add_local_blob_transaction_to_pool methods → broadcasts only if private_mempool: false

Code Quality & Correctness

crates/blockchain/mempool.rs: The addition of add_transaction_no_broadcast and the internal add_transaction_inner helper is clean and maintains the existing locking discipline. The separation of broadcast_pool from the main transaction pool is correctly handled.

crates/blockchain/blockchain.rs: The doc comments clearly distinguish between P2P and RPC entry points. The logic correctly checks !self.options.private_mempool to determine broadcast behavior for local transactions.

crates/networking/rpc/eth/transaction.rs: Correctly routes all eth_sendRawTransaction calls through the new local methods.

cmd/ethrex/cli.rs: The CLI argument is well-documented and properly integrated into the Default implementation.

Security & Consensus

  • No consensus risks: This is a networking/mempool policy change, not a consensus rule change.
  • Privacy guarantee: The implementation correctly ensures that when private_mempool is enabled, transactions are inserted into the mempool but never added to broadcast_pool, preventing gossip to peers.
  • Validation preserved: Both private and public paths undergo identical transaction validation (signature recovery, balance checks, blob validation), ensuring invalid transactions cannot bypass checks via the private path.

Minor Suggestions

  1. Test coverage extension (test/tests/blockchain/mempool_tests.rs):
    The current test validates the mempool level. Consider adding an integration test in blockchain.rs tests to verify that add_local_transaction_to_pool respects the BlockchainOptions::private_mempool flag, though the current unit test provides good coverage for the core logic.

  2. Metrics/Tracing (optional):
    Consider adding a tracing::debug! log in add_transaction_to_pool_inner indicating whether a transaction is being broadcast or kept private. This aids debugging node operator issues:

    if broadcast {
        tracing::debug!(%hash, "Transaction queued for P2P broadcast");
    } else {
        tracing::debug!(%hash, "Transaction added to private mempool (no broadcast)");
    }
  3. Naming consistency:
    The CLI flag is mempool_private while the BlockchainOptions field is private_mempool. Consider aligning them (e.g., --mempool.privateprivate_mempool is fine as-is since CLI args often use dot notation for grouping).

Verdict

Approve. The implementation is correct, secure, and follows Rust best practices. The separation of concerns between P2P and RPC ingestion paths is architecturally sound and prevents accidental leakage of private transactions to the broadcast pool.


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

@github-actions
Copy link
Copy Markdown

github-actions Bot commented May 6, 2026

Lines of code report

Total lines added: 105
Total lines removed: 7
Total lines changed: 112

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                 | 2566  | +58  |
+--------------------------------------------------------+-------+------+
| ethrex/crates/blockchain/mempool.rs                    | 499   | +35  |
+--------------------------------------------------------+-------+------+
| ethrex/crates/networking/p2p/rlpx/connection/server.rs | 1357  | -7   |
+--------------------------------------------------------+-------+------+

@github-actions
Copy link
Copy Markdown

github-actions Bot commented May 6, 2026

🤖 Claude Code Review

PR Review: feat(l1): add --mempool.private flag for non-propagating local txs

Overall this is a clean, well-structured feature. The design (inner helper pattern, separate local/P2P entry points, BlockchainOptions field) is idiomatic and the routing logic is correct. Below are observations ranging from nitpicks to one moderate concern.


Correctness

add_blobs_bundle orphan on partial failure (pre-existing, not introduced here)
In add_blob_transaction_to_pool_inner (blockchain.rs):

self.mempool.add_blobs_bundle(hash, blobs_bundle)?;
// ...
self.mempool.add_transaction_no_broadcast(hash, sender, mempool_tx)?;

If the second call fails, the blob bundle remains in storage with no corresponding transaction. This existed before the PR, but the refactor is a good opportunity to note it.

notify_waiters() fires for private transactions — correct. The payload builder needs to know new txs arrived; good that it's not gated on broadcast.

P2P path always broadcasts — correct. add_transaction_to_pool is untouched and always calls the broadcast variant, matching the stated spec.


CLI / Config

Redundant default_value_t alongside SetTrue (cli.rs:180):

default_value_t = false,
action = ArgAction::SetTrue,

ArgAction::SetTrue already implies false as the absent-flag default. The explicit default_value_t = false is harmless but noisy; removing it is cleaner and matches how clap's own documentation shows boolean flags.


Documentation

Confusing "EIP-equivalent" phrasing (blockchain.rs, doc on add_local_blob_transaction_to_pool):

/// EIP-equivalent of `add_blob_transaction_to_pool` for transactions

"EIP-equivalent" doesn't mean anything here — there's no EIP being referenced. It should just read "Equivalent of add_blob_transaction_to_pool…"


Test Coverage

Only the Mempool layer is exercised; the Blockchain routing is not.
The new test (mempool_tests.rs) directly calls Mempool::add_transaction_no_broadcast, which verifies the low-level mechanism. However, the actual routing logic — Blockchain::add_local_transaction_to_pool reading self.options.private_mempool and choosing the right variant — has no test. A small integration test that constructs a Blockchain with private_mempool: true, submits a transaction, and asserts it's absent from the broadcast pool would close this gap. That's the code path users will hit in production.

No test for blob transactions with private_mempool: true.
add_local_blob_transaction_to_pool follows the same pattern but isn't exercised separately. Given the c-kzg feature gate, even a basic unit test behind #[cfg(feature = "c-kzg")] would add confidence.

Synthetic sender in the test is fine — signature verification is a blockchain-layer concern, not a mempool concern, so bypassing it here is appropriate.


Minor: L2 context

private_mempool is correctly wired through the L2 initializer. Since L2 doesn't support blob transactions (comment confirms max_blobs_per_block: None), the add_local_blob_transaction_to_pool path is unreachable on L2. No bug, just worth noting in case L2 blob support is ever added.


Summary

Area Verdict
Core correctness Correct
Security No issues
CLI flag Minor: redundant default_value_t
Docs Minor: "EIP-equivalent" phrasing
Test coverage Moderate gap: Blockchain-level routing and blob-tx private path untested

The implementation is solid. The main ask before merging is a test that exercises Blockchain::add_local_transaction_to_pool with private_mempool: true end-to-end — everything else is polish.


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

@greptile-apps
Copy link
Copy Markdown

greptile-apps Bot commented May 6, 2026

Greptile Summary

Adds a --mempool.private flag (env ETHREX_MEMPOOL_PRIVATE) that prevents RPC-submitted transactions from being gossiped to peers while still allowing them to be included in locally-built blocks. The plumbing follows an inner-helper pattern: Mempool::add_transaction_inner(broadcast: bool) is shared by add_transaction and the new add_transaction_no_broadcast, and Blockchain exposes add_local_*_to_pool entry points that consult BlockchainOptions::private_mempool before routing.

  • Mempool gains add_transaction_no_broadcast / add_transaction_inner; broadcast-pool insertion is correctly gated on the broadcast flag, and tx_added.notify_waiters() still fires for both paths so the payload builder wakes up normally.
  • eth_sendRawTransaction is rerouted to add_local_blob_transaction_to_pool / add_local_transaction_to_pool; P2P paths in rlpx/ and l1_watcher continue using the broadcast-always variants.
  • Both L1 and L2 initializers wire opts.mempool_privateBlockchainOptions::private_mempool; a unit test verifies that get_txs_for_broadcast never returns privately-added transactions.

Confidence Score: 3/5

Safe to merge for the happy path, but the interaction between a privately-submitted transaction and a subsequent P2P receipt of the same tx breaks the documented invariant and should be resolved first.

The core machinery is correct, but both inner helpers share a deduplication path that can permanently suppress a tx from broadcast_pool when the private RPC path races ahead of a P2P receipt of the same transaction.

crates/blockchain/blockchain.rs — both add_transaction_to_pool_inner (line 2421) and add_blob_transaction_to_pool_inner (line 2356) need the early-return path to promote the tx into broadcast_pool when broadcast = true and the tx is already present but absent from broadcast_pool.

Important Files Changed

Filename Overview
crates/blockchain/blockchain.rs Adds add_local_transaction_to_pool, add_local_blob_transaction_to_pool, and shared _inner helpers; the contains_tx early-return in both inner helpers prevents the P2P path from promoting a privately-submitted tx into broadcast_pool.
crates/blockchain/mempool.rs Adds add_transaction_no_broadcast and shared add_transaction_inner helper; correctly gates broadcast_pool insertion on the broadcast flag. Logic is clean.
crates/networking/rpc/eth/transaction.rs Routes eth_sendRawTransaction through add_local_blob_transaction_to_pool / add_local_transaction_to_pool; correct — no other RPC entry points call pool methods.
cmd/ethrex/cli.rs Adds --mempool.private / ETHREX_MEMPOOL_PRIVATE CLI flag with SetTrue action; default false, docs and help text are accurate.
cmd/ethrex/initializers.rs Correctly wires opts.mempool_privateBlockchainOptions::private_mempool in the L1 initializer.
cmd/ethrex/l2/initializers.rs Correctly wires opts.node_opts.mempool_privateBlockchainOptions::private_mempool for the L2 initializer.
test/tests/blockchain/mempool_tests.rs New test directly exercises add_transaction_no_broadcast and confirms the tx stays in transaction_pool but is absent from get_txs_for_broadcast; thorough and self-contained.

Sequence Diagram

sequenceDiagram
    participant RPC as eth_sendRawTransaction
    participant BC as Blockchain
    participant MP as Mempool
    participant P2P as P2P Network

    Note over RPC,P2P: private_mempool = true

    RPC->>BC: add_local_transaction_to_pool(tx)
    BC->>BC: add_transaction_to_pool_inner(broadcast=false)
    BC->>MP: add_transaction_no_broadcast(hash, sender, tx)
    MP->>MP: transaction_pool.insert(hash, tx)
    Note right of MP: broadcast_pool NOT updated
    MP-->>BC: Ok(())
    BC-->>RPC: Ok(hash)

    P2P->>BC: add_transaction_to_pool(tx)
    BC->>BC: add_transaction_to_pool_inner(broadcast=true)
    BC->>MP: contains_tx(hash)?
    alt tx already in pool (private submission race)
        MP-->>BC: true - early return Ok(hash)
        Note right of BC: broadcast_pool still missing hash
    else tx not yet seen
        BC->>MP: add_transaction(hash, sender, tx)
        MP->>MP: transaction_pool.insert + broadcast_pool.insert
    end
Loading

Comments Outside Diff (1)

  1. crates/blockchain/blockchain.rs, line 2421-2423 (link)

    P1 Silent de-duplication hides broadcast-pool omission for P2P path

    Both add_transaction_to_pool_inner and add_blob_transaction_to_pool_inner return early on contains_tx, which checks only transaction_pool — not broadcast_pool. If a transaction is first submitted via private RPC (broadcast = false), it lands in transaction_pool but never enters broadcast_pool. When the same tx later arrives through P2P (broadcast = true), contains_tx returns true and the function returns Ok(hash) without inserting the hash into broadcast_pool. The tx is silently never relayed to peers despite the documented guarantee that "P2P-received transactions are unaffected." The same window exists in add_blob_transaction_to_pool_inner at line 2356.

    Prompt To Fix With AI
    This is a comment left during a code review.
    Path: crates/blockchain/blockchain.rs
    Line: 2421-2423
    
    Comment:
    **Silent de-duplication hides broadcast-pool omission for P2P path**
    
    Both `add_transaction_to_pool_inner` and `add_blob_transaction_to_pool_inner` return early on `contains_tx`, which checks only `transaction_pool` — not `broadcast_pool`. If a transaction is first submitted via private RPC (`broadcast = false`), it lands in `transaction_pool` but never enters `broadcast_pool`. When the same tx later arrives through P2P (`broadcast = true`), `contains_tx` returns `true` and the function returns `Ok(hash)` without inserting the hash into `broadcast_pool`. The tx is silently never relayed to peers despite the documented guarantee that "P2P-received transactions are unaffected." The same window exists in `add_blob_transaction_to_pool_inner` at line 2356.
    
    How can I resolve this? If you propose a fix, please make it concise.
Prompt To Fix All With AI
Fix the following 1 code review issue. Work through them one at a time, proposing concise fixes.

---

### Issue 1 of 1
crates/blockchain/blockchain.rs:2421-2423
**Silent de-duplication hides broadcast-pool omission for P2P path**

Both `add_transaction_to_pool_inner` and `add_blob_transaction_to_pool_inner` return early on `contains_tx`, which checks only `transaction_pool` — not `broadcast_pool`. If a transaction is first submitted via private RPC (`broadcast = false`), it lands in `transaction_pool` but never enters `broadcast_pool`. When the same tx later arrives through P2P (`broadcast = true`), `contains_tx` returns `true` and the function returns `Ok(hash)` without inserting the hash into `broadcast_pool`. The tx is silently never relayed to peers despite the documented guarantee that "P2P-received transactions are unaffected." The same window exists in `add_blob_transaction_to_pool_inner` at line 2356.

Reviews (1): Last reviewed commit: "feat(l1): add --mempool.private flag for..." | Re-trigger Greptile

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 adds a node-level “private local mempool” mode so transactions submitted via RPC are kept local (eligible for locally-built blocks) but are not queued for P2P propagation, mirroring reth’s --txpool.no-local-transactions-propagation.

Changes:

  • Introduces --mempool.private / ETHREX_MEMPOOL_PRIVATE and plumbs it into BlockchainOptions::private_mempool.
  • Splits mempool insertion into broadcast vs. no-broadcast paths (add_transaction vs. add_transaction_no_broadcast) with shared internal logic.
  • Routes eth_sendRawTransaction through new “local” blockchain entry points that honor the flag.

Reviewed changes

Copilot reviewed 7 out of 7 changed files in this pull request and generated no comments.

Show a summary per file
File Description
cmd/ethrex/cli.rs Adds the --mempool.private CLI/env flag and defaults.
cmd/ethrex/initializers.rs Wires the CLI flag into L1 BlockchainOptions.
cmd/ethrex/l2/initializers.rs Wires the CLI flag into L2 BlockchainOptions.
crates/blockchain/blockchain.rs Adds private_mempool option and introduces local vs. P2P mempool insertion APIs (broadcast vs. no-broadcast).
crates/blockchain/mempool.rs Implements add_transaction_no_broadcast via a shared internal helper toggling broadcast-pool insertion.
crates/networking/rpc/eth/transaction.rs Switches eth_sendRawTransaction to use the local insertion APIs so the flag is enforced for RPC submissions.
test/tests/blockchain/mempool_tests.rs Adds a unit test asserting no-broadcast transactions never appear in get_txs_for_broadcast().

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

@github-actions
Copy link
Copy Markdown

github-actions Bot commented May 6, 2026

🤖 Codex Code Review

Findings

  1. crates/networking/p2p/rlpx/eth/transactions.rs:220, crates/blockchain/blockchain.rs:2609, crates/blockchain/mempool.rs:167
    The new “private mempool” mode only skips broadcast_pool insertion; it does not persist any “private” bit on the mempool entry itself. Because GetPooledTransactions serves any tx found in the mempool by hash, a peer that knows the hash can still fetch the full transaction, and for blob txs it can also fetch the full sidecar via get_p2p_transaction_by_hash. That means the change suppresses proactive gossip, but it does not fully enforce the CLI promise that these txs are “not propagated to peers.” This needs a persistent per-entry privacy marker, and the P2P serve path should reject those entries.

Open Question

Summary
Other than that privacy-state gap, the wiring from CLI -> BlockchainOptions -> RPC entrypoints looks consistent, and I did not see consensus, EVM, or gas-accounting risk in this diff.

I could not run tests in this environment: cargo test failed because rustup/cargo attempted writes under read-only /home/runner/.rustup and /home/runner/.cargo, and fetching libssz was also blocked.


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

Copy link
Copy Markdown
Contributor

@iovoid iovoid left a comment

Choose a reason for hiding this comment

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

We leak the private transactions through send_all_pooled_tx_hashes, which is used to send a list of pooled transactions to new peers. We also send them if asked by txhash.

Comment thread crates/blockchain/blockchain.rs Outdated
.await
}

/// EIP-equivalent of `add_blob_transaction_to_pool` for transactions
Copy link
Copy Markdown
Contributor

Choose a reason for hiding this comment

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

Equivalent to which EIP?

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-project-automation github-project-automation Bot moved this to In Progress in ethrex_l1 May 6, 2026
…sactions

PR #6576 review (@iovoid): the original `--mempool.private` change only
filtered the *broadcast* pool, leaving two P2P paths that still leaked
the locally-submitted txs to peers:

1. `send_all_pooled_tx_hashes` (in p2p/rlpx/connection/server.rs) sends
   every mempool tx hash to a freshly-connected peer.
2. `GetPooledTransactions::handle` calls
   `Blockchain::get_p2p_transaction_by_hash`, which served any tx in the
   mempool to whoever asked by hash.

Fix: track admission as private explicitly.

- New `Mempool::private_pool` (`FxHashSet<H256>`) populated by
  `add_transaction_no_broadcast`, cleared on tx removal alongside
  `broadcast_pool`.
- New `Mempool::is_private(hash) -> Result<bool, StoreError>` for the
  P2P paths to consult.
- `send_all_pooled_tx_hashes` skips any tx whose hash is private before
  forwarding to the new peer.
- `Blockchain::get_p2p_transaction_by_hash` now returns the same
  `not found`-shaped error for private hashes (the eth/68 spec
  explicitly allows skipping unavailable txs, so peers handle this
  cleanly).

Test `add_transaction_no_broadcast_marks_tx_as_private_for_p2p_filters`
exercises the flag round-trip (add → is_private → remove → !is_private)
and covers the unknown-hash path. Existing
`add_transaction_no_broadcast_keeps_tx_out_of_broadcast_pool` is
unchanged.

Drive-by: rename "EIP-equivalent of …" doc on
`add_local_blob_transaction_to_pool` to "Local-RPC counterpart of …"
per @iovoid's inline comment.
@ilitteri
Copy link
Copy Markdown
Collaborator Author

ilitteri commented May 6, 2026

Pushed 2ca83911a addressing both review comments:

1. P2P leak paths (@iovoid CHANGES_REQUESTED) — fixed both leaks you identified:

  • New Mempool::private_pool (FxHashSet<H256>) populated by add_transaction_no_broadcast, cleared on tx removal alongside broadcast_pool.
  • New Mempool::is_private(hash) for P2P call sites to consult.
  • send_all_pooled_tx_hashes (in p2p/rlpx/connection/server.rs) now filters out private hashes before sending pooled-hash dumps to a newly-connected peer.
  • Blockchain::get_p2p_transaction_by_hash (called from GetPooledTransactions::handle) returns the same not found-shaped StoreError for private hashes — the eth/68 spec explicitly allows skipping unavailable txs, so peers handle this cleanly.

New test add_transaction_no_broadcast_marks_tx_as_private_for_p2p_filters exercises the flag round-trip (add → is_private → remove → !is_private) and the unknown-hash path.

2. "Equivalent to which EIP?" inline (crates/blockchain/blockchain.rs:2329) — fixed: renamed "EIP-equivalent of …" → "Local-RPC counterpart of …". My bad on the wording.

Ready for re-review.

ilitteri added 2 commits May 11, 2026 17:55
Address review findings from the cross-check pass:

1) Mempool::get_txs_for_new_peer_dump: build the broadcast-eligible tx
   list under a single read lock. The previous send_all_pooled_tx_hashes
   loop took a separate is_private read lock per tx, which under a hot
   pool can serialize peer-handshake throughput against any mempool
   write. The new helper filters out privileged + private txs in one
   pass and returns the snapshot the P2P dump needs.

2) add_transaction_to_pool_inner (and the EIP-4844 counterpart) now log
   a warning when contains_tx short-circuits with broadcast=false. If a
   tx has already been seen via gossip, the local --mempool.private
   request cannot retroactively un-broadcast it; surfacing this to the
   operator avoids silent confusion. State is intentionally unchanged
   (mutating the existing entry would itself violate the original
   submitter's intent).

Integration tests for the P2P/RPC race ordering are not added here:
exercising both paths through Blockchain::add_*_transaction_to_pool
requires sender-account fixtures we don't have in mempool_tests yet.
ilitteri added a commit that referenced this pull request May 11, 2026
Cross-client audit flagged that geth (`core/types/transaction.go::Size`),
nethermind (`MaxBlobTxSize`), and erigon (`ValidateSerializedTxn`) all
compare their 1 MiB blob-tx cap against the wire form that **includes
the sidecar** (blobs + commitments + proofs). The previous ethrex check
in `validate_transaction` compared `Transaction::encode_canonical_to_vec`
which only covers the core tx — the sidecar lives in the adjacent
`BlobsBundle`. With 6 blobs (~786 KB blob data + ~100 KB
commitments/proofs ≈ 900 KB) the worst-case wire wrapper can reach
~1.9 MiB while still passing the 1 MiB core-only check. Peers reject
that on the wire so ethrex would be admitting txs nobody else will relay.

Changes:
- `validate_transaction` now only enforces `MAX_TX_SIZE` for non-blob txs.
- `add_blob_transaction_to_pool_inner` runs a new wire-wrapper check
  before bundle validation: `core_tx_encoded + bundle_encoded <=
  MAX_BLOB_TX_SIZE`. Summing the two encoded sizes matches geth's
  `tx.Size()` semantic to within the ±few bytes of outer list framing,
  which is rounding error at this scale.
- Drop `validate_transaction_rejects_oversize_blob_core` — the
  function it covered no longer applies to blob txs. Integration test
  for the new wrapper check deferred (same pattern as PRs #6603/#6576:
  the `c-kzg`-gated `add_blob_transaction_to_pool` isn't currently
  exercised by `mempool_tests`).
…vel config

Addresses @iovoid's "Equivalent to which EIP?" question — the flag is
not based on an EIP, it's a node-level config. Help text and docs/CLI.md
reworded to make that explicit and drop the reference to reth's flag
name (consistent with the no-peer-client-name-drops style established
elsewhere in this repo's CLI help text).
Code comments should describe the behavior on its own terms, not who
raised the concern. The "(per @iovoid PR #6576 review)" reference
becomes stale archaeology once the PR merges.
Copy link
Copy Markdown
Contributor

@ElFantasma ElFantasma left a comment

Choose a reason for hiding this comment

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

Two small observability concerns. Both follow-ups, not blocking.

let hash = transaction.hash();
if self.mempool.contains_tx(hash)? {
if !broadcast {
warn!(%hash, "tx already public; --mempool.private cannot retroactively un-broadcast");
Copy link
Copy Markdown
Contributor

Choose a reason for hiding this comment

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

The warn! correctly surfaces "this private submission lost the race to a public gossip" — but a log line is easy to miss for operators running with INFO+ and impossible to alert on. Consider adding a counter (METRICS_MEMPOOL.inc_private_collisions() or similar) so the rate of these races is visible in Grafana. Two reasons it matters:

  • An operator using --mempool.private for MEV-protected ordering needs to know how often their privacy was already breached by peer gossip before their RPC submission arrived. The warn gives one-time anecdotal data, a metric gives the rate.
  • The same warn-path exists in the blob path at :2357, so two log lines for the same condition — a single counter would collapse them.

Not blocking — happy to defer to a follow-up if you'd rather keep the PR scoped to the flag itself.

// somehow learned the hash. The spec for `GetPooledTransactions`
// explicitly allows skipping unavailable transactions, so we mirror
// the "not found" path the caller already handles.
if self.mempool.is_private(*hash)? {
Copy link
Copy Markdown
Contributor

Choose a reason for hiding this comment

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

Consistency observation with the get_txs_for_new_peer_dump optimization called out in the PR description ("replaces the per-tx is_private lock the previous loop took"): the same concern still applies to GetPooledTransactions::handle (networking/p2p/rlpx/eth/transactions.rs:228-237), which calls get_p2p_transaction_by_hash in a loop. Each call takes two read locks here (one for is_private, one for get_transaction_by_hash), and a GetPooledTransactions request can carry up to 256 hashes per the eth spec.

A batched get_p2p_transactions_by_hashes(&[H256]) -> Vec<P2PTransaction> that takes a single read lock and applies the is_private filter inline would mirror the dump-path fix and remove a similar peer-handshake-vs-mempool-write contention. Same direction as the existing TODO at transactions.rs:222 ("TODO(#1615): get transactions in batch instead of iterating over them") — this would dovetail with that.

Not blocking for this PR — the flag itself is correctly implemented and tested. Flagging because the description's lock-contention point applies broader than the one path.

ilitteri added a commit to benbencik/ethrex that referenced this pull request May 13, 2026
…ambdaclass#6599)

**Motivation**

Every major Ethereum execution client (geth `txMaxSize`, reth
`DEFAULT_MAX_TX_INPUT_BYTES`, nethermind `MaxTxSize` / `MaxBlobTxSize`,
erigon size enforcement) ships a per-transaction wire-size cap at
admission. Without one a single oversized transaction can chew bandwidth
and pool capacity at near-zero attacker cost.

**Description**

- New constants `MAX_TX_SIZE = 128 KiB` and `MAX_BLOB_TX_SIZE = 1 MiB`
in `crates/common/types/constants.rs`.
- `Blockchain::validate_transaction` enforces `MAX_TX_SIZE` on the
canonical RLP encoding of non-blob transactions.
- `Blockchain::add_blob_transaction_to_pool` enforces `MAX_BLOB_TX_SIZE`
on the **wire wrapper** — `Transaction::encode_canonical_to_vec().len()
+ BlobsBundle::encode_to_vec().len()` — to match geth (`tx.Size()`
includes the attached sidecar), nethermind (`MaxBlobTxSize` is checked
on the wrapper form), and erigon (`ValidateSerializedTxn` works on the
raw wire bytes which for blob txs is the wrapper-with-sidecar). The two
encoded sizes are summed because ethrex stores the core tx and the
bundle in separate structs; the ±few bytes of outer list framing are
rounding error at the 1 MiB scale.
- New error `MempoolError::TxSizeExceeded { actual, limit }`.
- Drops the redundant `MAX_TRANSACTION_DATA_SIZE` calldata-only check
from `validate_transaction` — the new encoded-size cap is strictly
tighter (a 128 KiB calldata payload always encodes to > 128 KiB).
- Unit test in `mempool_tests.rs` covering the non-blob path.
Integration test for the wrapper check deferred (the `c-kzg`-gated
`add_blob_transaction_to_pool` isn't currently exercised by
`mempool_tests`, same pattern as PRs lambdaclass#6603/lambdaclass#6576).
# Conflicts:
#	cmd/ethrex/initializers.rs
#	cmd/ethrex/l2/initializers.rs
#	crates/blockchain/blockchain.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)

ilitteri added 2 commits May 13, 2026 18:28
Rust 1.91 clippy::redundant_clone (now -D warnings in CI) flags the clone
because public_tx is not used after the call. Drop the clone.
@ilitteri ilitteri requested a review from a team as a code owner May 14, 2026 14:10
Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment

Labels

L1 Ethereum client

Projects

Status: In Progress

Development

Successfully merging this pull request may close these issues.

4 participants