feat(l1): switch EIP-8070 blob sampling on at Amsterdam

[MegaRedHand]

Motivation

eth/72 (EIP-8070) is gated behind --blob-sampling, which defaults to off, so a default node never advertises the capability.

The gate is load-bearing rather than cosmetic: eth/72 always elides blob payloads from PooledTransactions, so blobs reach the node only through GetCells, and only the sampler/provider state machine issues those. A node that advertised eth/72 without running the loop would accept blob transactions it could never reconstruct.

What the gate got wrong is what it keys on. A CLI flag has no relationship to when the network starts speaking eth/72; the fork does.

Changes

Blob sampling now switches itself on once the chain head reaches Amsterdam, the fork EIP-8070 rides on.

• Blockchain::blob_sampling_enabled() replaces the public Mempool::blob_sampling_enabled field as the single answer to "is the state machine active", and resolves the fork check.
• BlockchainOptions::blob_sampling_enabled becomes force_blob_sampling, reflecting its new job: bring activation forward, for devnets and for chains that schedule Amsterdam later than they want the state machine running. --blob-sampling and --blob-eager-provider are unchanged for operators.
• The four p2p read sites go through Blockchain instead of reaching into the mempool.
• Store::latest_block_timestamp() is added so the fork check places the head against the schedule without cloning a header.

Rollups are excluded explicitly. They reject blob transactions outright, so their fork schedule must not drag the blobpool state machine in. BlockchainType alone cannot carry this: native rollups run as BlockchainType::L1 on purpose, because their blocks must re-execute under an unmodified L1 environment, so the type check would have let a native rollup that schedules Amsterdam start advertising eth/72. An explicit blob_txs_supported flag, cleared by both rollup entry points, carries the intent; the type check stays as a backstop.

Latching

The flag is one-way, for the same reason the eager-provider latch is: a reorg back below the fork boundary would otherwise flap the advertised capability set and churn peer connections over a boundary the chain is about to cross again.

It resolves lazily on the p2p paths that ask the question (per connection, per announcement) rather than on block import, so the import path pays nothing and the steady state after the fork is one atomic load.

The Hive - Devp2p tests job

That job has failed on every main run since 2026-09-07. The cause is upstream, in geth's test helper:

// cmd/devp2p/internal/ethtest/conn.go, s.dialAs()
conn.caps = []p2p.Cap{
    {Name: "eth", Version: 72},
    {Name: "eth", Version: 70},
    {Name: "eth", Version: 69},
}
conn.ourHighestProtoVersion = 72

eth/71 is missing from the list, while negotiateEthProtocol picks the highest remote version <= ourHighestProtoVersion without consulting that list at all. Against a node topping out at eth/71 the simulator therefore speaks eth/71 while we negotiate eth/70, our validate_status rejects the version mismatch, and we disconnect. Status still passes because the simulator reads our Status before sending its own, so the whole eth and snap suites fail on the next message. GetBlockAccessLists is the one eth test that passes, because it uses dialEth71(), which overrides the caps list.

Advertising eth/72 puts both sides on the version the helper does advertise. This is a real fix for us rather than a workaround, but the upstream helper is still wrong for any client that tops out at eth/71, so it is worth reporting separately.

It does not go green, and it un-skips three tests that did not pass

Measured against main at c00743efc (23 failures: 18 eth, 5 snap):

	Count
Fixed	20	all 4 real snap failures, plus 16 eth
Pre-existing, still failing	1	BlobViolations
Newly exercised, were skipped	3	BlobTxAvailabilityFailure, GetCells, BlobTxWithInvalidCells

Counts here are real tests. Each suite also reports a client launch case that fails whenever anything under it does, so main's 23 is 21 real failures plus two wrappers.

Advertising eth/72 un-skips the three tests that previously reported skipping: node negotiated eth/71, eth/72 required, and they did not pass on first contact. Turning the capability on by default while they fail would ship a default-on wire protocol that fails conformance, so the causes are fixed here rather than deferred. Reproduced locally against hive 43ea47be with the simulator's own go-ethereum: 4 failures before, 2 after.

GetCells — cells requested from a peer that never announced the transaction. The custody-generation sweep walked blob_txs_missing_cells, which spans the whole pool, and queued a GetCells for every entry at whichever peer happened to trigger the sweep. devp2p caps/eth.md fetches cells "from peers that announced overlapping availability". The test caught it as a request carrying two hashes left over from an earlier test instead of the one just announced — the same two hashes, deterministically, in CI and locally. Now filtered by the announcement record the mempool already keeps.

BlobTxAvailabilityFailure — two unnecessary disconnects. The handler dropped any peer whose response carried a transaction outside the request; the stored announcement is trimmed to the hashes a given request asked for, so a peer answering the whole announcement it originally sent — which devp2p permits, and which geth does — was disconnected for it. Those entries are now tolerated and then dropped before they reach the pool, so nothing unasked-for is admitted. Separately, a type-3 transaction declaring no blob versioned hashes is invalid on its own terms and its empty sidecar agrees with it; that is a bad transaction, not a lying peer, and no longer costs the connection. makeBlobTxs(10, 4, ...) has a blob budget covering only its first two transactions, so eight of the ten it announces are exactly that shape. A sidecar that disagrees with its transaction still disconnects, which is what BlobTxWithoutSidecar and BlobTxWithMismatchedSidecar check, and both still pass.

BlobTxWithInvalidCells passes; it failed in one CI run and in neither local run, so it looks order- or timing-sensitive rather than broken.

What still fails

• BlobViolations announces a blob tx with a deliberately wrong size (Size() + 10) and expects a disconnect. validate_requested skips the size check for blob txs on purpose: the announced size covers the full wrapper while eth/72 delivers the elided form, so the received encoding cannot be compared against it directly. Catching this needs the full size reconstructed from the blob count, which is exact but fiddly RLP framing arithmetic, and getting it wrong disconnects honest peers. Left as is deliberately, and better done as its own change. Its second case, the wrong announced tx type, is caught today.
• client launch is not a separate failure. It is the hivesim.ClientTestSpec wrapper in simulators/devp2p/main.go that starts the client and runs geth's devp2p rlpx eth-test --tap binary, reporting each TAP line as a sub-test; runTAP ends in return cmd.Wait() and runEthTest calls t.Fatal on a non-zero exit, and the tool exits non-zero whenever any sub-test failed. So it double-counts, and it clears itself once BlobViolations does. The snap suite's own client launch already passes here, because nothing under it fails.

Also here: the announcement cell bitmap

cell_mask: None used to go on the wire as RLP nil. devp2p caps/eth.md types the element cells: B_16 — always 16 bytes, whose content "can be ignored when no blob transactions are announced" — while EIP-8070 says the field "MUST be set to nil" in that case. The two specs contradict each other. geth decodes the element into types.CustodyBitmap [16]byte and rejects anything shorter, so a nil mask makes every blob-free announcement undecodable to the rest of the network; this follows devp2p. Worth raising against EIP-8070 separately.

The round trip is no longer the identity: None encodes to an all-zero B_16 and decodes back as Some(0). That is harmless because receivers only consult the mask when the announcement actually carries a type-3 transaction, but it is a behaviour change on the wire and not only in memory.

Caveat

EIP-8070 is Draft and a Glamsterdam candidate, not scheduled for inclusion. Tying activation to Amsterdam presumes it lands there. If it slips to a later fork, the constant in Blockchain::blob_sampling_enabled moves with it.

Testing

cargo clippy --lib --bins -F debug,sync-test -- -D warnings clean. Full ethrex_tests binary: 1232 passed, 0 failed, including sampling_tests, mempool_cells_tests and eth72_engine_tests.

devp2p was run end to end rather than reasoned about, on hive 43ea47be with the simulator building its own go-ethereum, against images built from this branch:

before the eth/72 fixes:  suites=5 tests=62 failed=4
after:                    suites=5 tests=60 failed=2

discv4, discv5, snap and snap2 pass in full. Of the two remaining eth failures one is the client launch wrapper, so BlobViolations is the only real test still failing.

[github-actions]

⚠️ Known Issues — intentionally skipped tests

Source: docs/known_issues.md

rpc-compat log-bearing cases excluded

Where: KNOWN_EXCLUDED_TESTS in .github/scripts/check-hive-results.sh counts out
eight hive rpc-compat cases — the four eth_getLogs cases, eth_getBlockReceipts/get-block-receipts-latest,
and three eth_getTransactionReceipt cases. They are exactly the cases whose recorded
response contains at least one log object; every case with an empty log array still runs.
Note this leaves eth_getLogs with no rpc-compat coverage at all, since all four of its
cases are in the set.

Why: ethrex populates blockTimestamp on log objects, as geth, besu, nethermind, reth
and erigon all do. hive's rpc-compat compares responses byte-exactly (jsondiff.FullMatch;
the lenient checkJSONStructure path applies only to cases upstream marks speconly), and
the corpus is pinned to execution-apis d08382ae (2025-02-10), whose recordings predate the
field — it entered the schema in execution-apis#639 and the fixtures in #846 (2026-07-22).
So the extra key cannot match, and this is a property of the pin rather than of the response.

The pin cannot move, and this is not temporary. The pin sits one commit before
execution-apis#627, which moved the test chain to a pre-merge genesis: the current corpus has
~36 proof-of-work blocks before its terminal total difficulty. ethrex does not support
pre-merge chains and will not, so importing that chain.rlp fails at block 1 —
validate_block_header has no pre-London base-fee path. Every revision carrying
blockTimestamp in its fixtures also carries that chain, so there is no revision that
satisfies both. Nor can the corpus be patched locally: rpc-compat's Dockerfile clones
ethereum/execution-apis by hard-coded URL, so the branch buildarg cannot point at a fork.

Coverage: the field itself is pinned by
block_timestamp_is_on_the_log_and_not_on_the_receipt in
crates/networking/rpc/types/receipt.rs, which asserts it is present on each log and absent
from the receipt level.

Removal: delete the entries if ethrex ever gains pre-merge chain import, or if upstream
marks these cases speconly so they are type-checked instead of compared byte-for-byte.

---

The stateless schema id does not identify the encoding

Where: STATELESS_INPUT_SCHEMA_ID in crates/common/types/stateless_ssz.rs.

Upstream keeps the stateless input schema id at 0x1501
(fork_index 0x15 << 8 | revision 0x01) across incompatible body changes. Three
encodings have now shipped under it: tests-zkevm@v0.6.2, then #3248 + #3278,
then #3356, which moved state, codes and public_keys from SszList to
ProgressiveList. ethrex speaks the last one.

The consequence is that the 2-byte prefix cannot be used to detect a stale or
mismatched bundle. A wrong-dialect input is accepted by the id check and then
fails later — in SSZ decode, or on a root that does not match — rather than being
rejected up front for what it is. only_amsterdam_schema_id_decodes therefore
proves less than its name suggests.

Worth raising upstream: a revision field that does not move across a body change
provides no version negotiation at all.

---

ZisK guest program hash changes with the unsync_cell gate

Where: crates/common/types/block.rs, transaction.rs.

The gate on the single-threaded unsync_cell::OnceCell moved from
all(feature = "eip-8025", target_arch = "riscv64") to
all(feature = "zisk", target_arch = "riscv64") when the eip-8025 feature was removed.

The guest ELFs were previously built --features "<zkvm>-build-elf,ci", which never enabled
eip-8025, so they compiled the atomic once_cell variant. bin/zisk/Cargo.toml does enable
ethrex-common/zisk, so the ZisK guest now compiles the unsafe impl Sync cell instead.
That changes the ELF bytes and therefore the program hash and verification key.

This is intended (the guest is single-threaded, so the unsync cell is sound and cheaper), but it
is a VK change rather than a no-op refactor, and the diffstat presents it as a file rename
(eip8025_cell.rs → unsync_cell.rs). Anyone pinning a ZisK VK across this change must
re-register it. The stateless-validator crate now forwards ethrex-common/zisk from its own
zisk feature so the two ZisK guests do not disagree on the cell type.

---

Release signing key is an unprotected repository secret

Where: .github/workflows/tag_release.yaml.

MINISIGN_SECRET_KEY is a plain repository secret. There is no environment: on
finalize-release or dry-run-release-assets, and gh api repos/lambdaclass/ethrex/rulesets
shows only branch-targeted rulesets, so the github.ref_type == 'tag' condition is a workflow
check rather than an enforced boundary: anyone who can push a tag can reach the signing key.

This is a repository-settings change, not a code change, so it is recorded here rather than
fixed in the tree. Recommended:

1. Move MINISIGN_SECRET_KEY / MINISIGN_PASSWORD into a GitHub Environment with required
   reviewers, and add environment: to the two jobs that sign.
2. Add a ruleset targeting refs/tags/v* restricting who may create release tags.

Until then, the compromise of that key is silent and durable: signatures would still verify
against the committed .github/minisign.pub.

[github-actions]

🤖 Kimi Code Review

I'll review this PR which changes EIP-8070 blob sampling from a static flag to a fork-gated mechanism that activates at the Amsterdam hard fork. Let me analyze each area.

Overall Assessment

This is a well-structured PR that correctly transitions blob sampling from a manual operator flag to an automatic fork-gated mechanism. The core logic is sound, but I have several concerns around edge cases, atomic ordering, and potential race conditions.

---

Detailed Findings

1. Race Condition in blob_sampling_enabled() — crates/blockchain/blockchain.rs:4203-4215

pub fn blob_sampling_enabled(&self) -> bool {
    if self.mempool.blob_sampling_enabled() {
        return true;
    }
    // ... L2 check ...
    let head = self.storage.latest_block_timestamp();
    if !self.storage.get_chain_config().is_amsterdam_activated(head) {
        return false;
    }
    self.mempool.enable_blob_sampling();
    true
}

Problem: Multiple threads can race through the false path simultaneously between the load and the store. While swap in enable_blob_sampling is atomic, the log message can fire multiple times, and more critically, this creates a TOCTOU (time-of-check-time-of-use) window.

Specific issue: Between line 4212 (is_amsterdam_activated returning true) and line 4214 (enable_blob_sampling), another thread could have already called enable_blob_sampling. The swap handles this correctly (returns old value), but the window is unnecessary.

Suggestion: Use a compare-exchange loop or restructure to minimize the race window. The current code works due to swap's semantics, but the log duplication is a real issue:

// After fix - only log on actual transition
pub fn blob_sampling_enabled(&self) -> bool {
    if self.mempool.blob_sampling_enabled() {
        return true;
    }
    if !matches!(self.options.r#type, BlockchainType::L1) {
        return false;
    }
    let head = self.storage.latest_block_timestamp();
    if !self.storage.get_chain_config().is_amsterdam_activated(head) {
        return false;
    }
    self.mempool.enable_blob_sampling() // returns bool indicating if it was newly enabled
}

Then change enable_blob_sampling to return bool:

pub(crate) fn enable_blob_sampling(&self) -> bool {
    let was_enabled = self.blob_sampling.swap(true, Ordering::AcqRel);
    if !was_enabled {
        info!(...);
    }
    !was_enabled // true if we were the first to enable
}

---

2. Inconsistent Atomic Ordering — crates/blockchain/mempool.rs:731, 741, 766

pub fn blob_sampling_enabled(&self) -> bool {
    self.blob_sampling.load(Ordering::Acquire)  // line 731
}

pub(crate) fn enable_blob_sampling(&self) {
    if !self.blob_sampling.swap(true, Ordering::AcqRel) {  // line 741
        // ...
    }
}

pub fn latch_eager_provider(&self) {
    if !self.blob_sampling_enabled() {  // line 766 - calls with Acquire
        return;
    }
    if !self.eager_provider.swap(true, Ordering::AcqRel) {  // line 769
        // ...
    }
}

Problem: blob_sampling uses Acquire load but AcqRel swap. This is technically fine (AcqRel is stronger), but latch_eager_provider at line 766 does an Acquire load of blob_sampling, then if true, does an AcqRel swap of eager_provider. There's no happens-before guarantee that the eager_provider store is visible to another thread that did enable_blob_sampling unless there's additional synchronization.

More critically: blob_sampling_enabled() at line 731 uses Acquire, but there's no corresponding Release store path for the initial false value. The swap(true, AcqRel) provides release semantics, but on the first check when still false, there's no synchronization established.

This is actually fine for this specific case because:

• The AtomicBool::new(false) initialization happens-before any thread access (program order)
• The AcqRel swap on enable provides both acquire and release semantics

However, for consistency and clarity, consider using Relaxed for the initial check if you don't need synchronization (it's just a flag), or document why Acquire is needed. Actually, Acquire is correct here because you want to ensure that if sampling is enabled, you see all prior writes by the thread that enabled it.

Minor suggestion: Consider SeqCst for enable_blob_sampling if you need global ordering visibility, but AcqRel should suffice with the Acquire load.

---

3. L2 Check in blob_sampling_enabled() — crates/blockchain/blockchain.rs:4206-4208

if !matches!(self.options.r#type, BlockchainType::L1) {
    return false;
}

Problem: This check is performed on every call after the fast-path check fails. However, L2 nodes are initialized with force_blob_sampling: false and the mempool's blob_sampling starts false. The only way this check matters is if someone manually calls enable_blob_sampling() on an L2, or if there's a code path that bypasses force_blob_sampling.

Question: Is this defensive against future code changes, or is there an actual code path? The comment says "L2 rejects blob txs outright, so its fork schedule must not drag the blobpool state machine in" — but L2 chains might not even have Amsterdam in their fork schedule, or if they do, is_amsterdam_activated would be the natural gate.

Suggestion: This is probably correct defensively, but consider whether checking the chain config directly would be more robust than the node type. What if an L1 chain is configured without Amsterdam? The current code would still call is_amsterdam_activated, which presumably returns false — correct behavior. The L2 check is an optimization and safety guard. Keep it, but consider adding a debug assertion or comment about L2 fork schedules.

---

4. latest_block_timestamp() Returns Stale Data — crates/storage/store.rs:1646-1649

pub fn latest_block_timestamp(&self) -> u64 {
    self.latest_block_header.get().timestamp
}

Problem: This returns a u64 by value from what appears to be a cached header. Need to verify thread-safety of latest_block_header.get().

Looking at the existing get_latest_block_number():

pub fn get_latest_block_number(&self) -> Result<u64, StoreError> {
    Ok(self.latest_block_header.get().number)
}

The new method bypasses the Result wrapper. Is latest_block_header thread-safe? Presumably it's an Arc<RwLock<...>> or similar, or an atomic cell. The .get() suggests it might be a once_cell or parking_lot::RwLockReadGuard.

Critical concern: If latest_block_header is behind a lock, this method acquires it. If it's an atomic pointer swap, it's lock-free. The PR doesn't show the type, but given get_latest_block_number returns Result, there might have been a lock that could fail (poisoned).

Missing context: I can't see the type of latest_block_header. If get() can panic or if there's synchronization cost, this needs documentation. The comment says "for callers that only need to place the head against a fork schedule and would otherwise clone a whole header" — this suggests it's optimizing away a clone.

Verify: That get() is actually cheaper than cloning a full header. If it's just reading from an Arc<Header> or similar, this is fine.

---

5. Fork Check Called Repeatedly — Performance Concern

Every call to blob_sampling_enabled() that returns true from the mempool fast-path is cheap (one atomic load). But if sampling isn't enabled yet and we're pre-Amsterdam, every call does:

1. Atomic load (miss)
2. L2 type check
3. latest_block_timestamp() — potential lock acquire
4. get_chain_config() — potential clone or lock
5. is_amsterdam_activated() — computation
6. Then enable_blob_sampling() — atomic swap

This is called from:

• handle_incoming_message (per P2P message)
• Capability negotiation (per peer connection)
• latch_eager_provider (per payload build)

Question: How often is this called pre-Amsterdam? If it's on every P2P transaction announcement, this could be hot.

Suggestion: The comment at line 4198-4200 says "Latching happens here rather than on block import so that the fork check costs nothing on the import path" — but it shifts cost to P2P paths. Consider whether a background task or block import hook could do this more efficiently. However, given the fork transition is one-time and P2P traffic is bursty, this is likely acceptable. No change required, but document the trade-off.

---

6. Comment/Documentation Inconsistency — cmd/ethrex/cli.rs:527

help = "Enable EIP-8070 PeerDAS blob sampling (sampler/provider state machine) before Amsterdam. From Amsterdam onwards it is on regardless; until then the node always acts as provider (p=1.0).",

Problem: The help text says "before Amsterdam" but the flag is now force_blob_sampling. The help says "Enable... before Amsterdam" which implies it's a pre-Amsterdam opt-in, but the actual behavior is "force on early, regardless of fork schedule."

Suggested rewording:

help = "Force EIP-8070 PeerDAS blob sampling on before the Amsterdam fork activates. By default sampling auto-enables at Amsterdam; this flag enables it earlier for devnets or custom fork schedules. Until sampling is active, the node always acts as provider (p=1.0).",

---

7. blob_eager_provider Implies force_blob_sampling — Logic Verified

In initializers.rs:925:

force_blob_sampling: opts.blob_sampling || opts.blob_eager_provider,

And in BlockchainOptions docs:

Implies force_blob_sampling, since eager provider is a role within the sampling state machine.

This is correct. The new_with_eager_provider constructor sets both flags:

blob_sampling: AtomicBool::new(true),
eager_provider: AtomicBool::new(true),

---

8. Payload Building Calls latch_eager_provider — crates/blockchain/payload.rs:444

self.latch_eager_provider();

Previously:

self.mempool.latch_eager_provider();

This now goes through self.blob_sampling_enabled() first. Correct change — ensures eager provider only latches if sampling is actually enabled (or fork has activated).

---

9. Missing Test for Fork Transition

No tests are visible in this diff. The fork-gated behavior is critical for network compatibility. Consider adding:

1. Pre-Amsterdam: blob_sampling_enabled() returns false, then true after timestamp passes fork
2. L2 chain: never enables regardless of timestamp
3. force_blob_sampling: enables immediately regardless of timestamp
4. Reorg below fork: remains enabled (one-way latch behavior)

---

Summary

Item	Severity	File	Line	Description
1	Medium	blockchain.rs	4203-4215	Race condition allows duplicate log; return value from enable_blob_sampling ignored
2	Low	mempool.rs	731, 741, 766	Atomic ordering mix is correct but could be cleaner; verify SeqCst not needed
3	Low	blockchain.rs	4206-4208	L2 check is defensive; verify no L2 has Amsterdam in config
4	Medium	store.rs	1646-1649	Verify latest_block_header.get() thread-safety
5	Low	blockchain.rs	4183-4215	Fork check on hot path; acceptable but document trade-off
6	Low	cli.rs	527	Help text slightly misleading about "before Amsterdam" semantics
7	—	—	—	Correct: blob_eager_provider implies force_blob_sampling
8	—	payload.rs	444	Correct change to use latch_eager_provider wrapper
9	Medium	—	—	Missing tests for fork transition behavior

Recommended Changes

Priority 1 (must fix):

• Item 1: Change enable_blob_sampling to return bool, use it to avoid duplicate logging

Priority 2 (should fix):

• Item 4: Verify and document latest_block_header.get() thread-safety
• Item 9: Add tests for fork transition behavior

Priority 3 (nice to have):

• Item 6: Clarify CLI help text

---

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

[github-actions]

🤖 Codex Code Review

1. crates/blockchain/blockchain.rs gates PeerDAS activation on is_amsterdam_activated(head) instead of the fork ordinal. That is inconsistent with the repo’s own fork-gating rule in crates/blockchain/blockchain.rs, which explicitly says field checks like is_amsterdam_activated are wrong when a chain schedules a later fork without an explicit amsterdamTime. The existing fixtures in test/tests/blockchain/mempool_tests.rs model exactly that case (hegota_time: Some(0), amsterdam_time: None). On those chains, blob_sampling_enabled() will stay false forever, so crates/networking/p2p/rlpx/connection/server.rs never advertises eth/72 and the payload-builder path never latches eager-provider mode either. This effectively disables EIP-8070 on post-Amsterdam-by-ordinal networks. The fix should be to gate on chain_config.fork(head_ts) >= Fork::Amsterdam (or a dedicated helper with that semantics), then add a regression test for the no-amsterdamTime Hegota setup.

Other than that, the refactor looks mechanically sound. I did not run tests; this is a static review.

---

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

[github-actions]

Lines of code report

Total lines added: 59
Total lines removed: 0
Total lines changed: 59

Detailed view

+-------------------------------------------------------------+-------+------+
| File                                                        | Lines | Diff |
+-------------------------------------------------------------+-------+------+
| ethrex/cmd/ethrex/initializers.rs                           | 1289  | +1   |
+-------------------------------------------------------------+-------+------+
| ethrex/cmd/ethrex/l2/initializers.rs                        | 546   | +2   |
+-------------------------------------------------------------+-------+------+
| ethrex/crates/blockchain/blockchain.rs                      | 3642  | +21  |
+-------------------------------------------------------------+-------+------+
| ethrex/crates/blockchain/mempool.rs                         | 1969  | +17  |
+-------------------------------------------------------------+-------+------+
| ethrex/crates/networking/p2p/rlpx/connection/server.rs      | 2308  | +12  |
+-------------------------------------------------------------+-------+------+
| ethrex/crates/networking/p2p/rlpx/eth/eth72/transactions.rs | 360   | +3   |
+-------------------------------------------------------------+-------+------+
| ethrex/crates/storage/store.rs                              | 5631  | +3   |
+-------------------------------------------------------------+-------+------+

[github-actions]

🤖 Claude Code Review

Review: PR 7299 — switch EIP-8070 blob sampling on at Amsterdam

Overall design is sound. Gating eth/72 on the fork rather than solely on an operator flag closes a real gap (a default node could otherwise never speak eth/72). The one-way latch to avoid capability flapping on reorg, the lazy resolution on p2p paths so block import pays nothing, and routing all four p2p call sites through Blockchain instead of reaching into Mempool directly are all good decisions, cleanly executed.

High-severity bug

Blockchain::blob_sampling_enabled() (crates/blockchain/blockchain.rs:4202-4216) misidentifies the native-rollup L2 as L1, silently enabling blob sampling on a chain that should never run it.

The L2 exclusion is:

if !matches!(self.options.r#type, BlockchainType::L1) {
    return false;
}

But cmd/ethrex/l2/initializers.rs::init_native_rollup_l2 deliberately sets r#type: BlockchainType::L1 (for EVM/precompile parity with L1) while also setting force_blob_sampling: false with the comment "L2 rejects blob txs; no eth/72 sampling." Because this node's type is L1, the exclusion never triggers. Its genesis fixture (fixtures/genesis/native_l2.json:87) sets "amsterdamTime": 0, so is_amsterdam_activated(head) is true from genesis. The very first call to blob_sampling_enabled() — e.g. the eth/72 offer check at rlpx/connection/server.rs:1279, or latch_eager_provider() invoked from payload.rs:441 on the first payload build — permanently latches sampling on via mempool.enable_blob_sampling(), overriding the explicit force_blob_sampling: false.

This directly contradicts the PR's own stated invariant ("L2 is excluded explicitly... its fork schedule must not drag the blobpool state machine in") for exactly the L2 variant that shares BlockchainType::L1. The node would advertise eth/72, elide blob payloads from PooledTransactions, and expect to serve GetCells — on a chain that rejects blob transactions outright. None of sampling_tests.rs, eth72_engine_tests.rs, or mempool_cells_tests.rs cover this configuration, so it isn't caught by the test suite mentioned in the PR description.

Suggested fix: the exclusion needs a signal independent of BlockchainType, e.g. checking max_blobs_per_block.is_none() (already None for both L2 variants) or adding an explicit "supports blobs" flag to BlockchainOptions rather than overloading r#type.

Minor / style

Command-query separation: Blockchain::blob_sampling_enabled() looks like a pure getter (&self -> bool) but has the side effect of permanently latching the mempool flag on its first true evaluation, including the info-log line. It's documented, but the name/signature invite an innocuous future caller (metrics, a status RPC) to accidentally trigger early activation. Worth a more explicit name or splitting the pure check from the latching action.

Things that look correct

• Store::latest_block_timestamp() clones a cheap Arc<BlockHeader> behind a mutex (not the header itself), consistent with the "no cloning" goal.
• The mempool's blob_sampling: AtomicBool swap/load ordering (AcqRel/Acquire) is consistent with the pre-existing eager_provider pattern; the one-way latch logic is correct (logs only on the false -> true transition).
• All previously-direct mempool.blob_sampling_enabled field reads in rlpx/connection/server.rs were correctly migrated to blockchain.blob_sampling_enabled(); grepped the repo and found no leftover call sites bypassing the new fork-aware path.
• BlockchainOptions renames (force_blob_sampling) are complete and consistent — no stale struct-literal usages of the old field name remain.

---

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

[Copilot]

Copilot review overview

🟡 Changes recommended

Critical activation and native-rollup L2 exclusion issues remain unresolved.

Get a fresh assessment by requesting another Copilot review.

Review effort: Lite
Findings: 4

Open (4)

• Exclude native-rollup L2 from Amsterdam blob sampling · New
• Check L2 exclusion before honoring mempool sampling latch · New
• Distinguish native-rollup L2 from L1 for capability gating · New
• Use resolved fork ordinal for Amsterdam activation · New

What changed in this PR

This PR changes EIP-8070 blob sampling from CLI-only activation to lazy, latched Amsterdam-fork activation, while centralizing the decision in Blockchain.

Changes:

• Adds fork-aware activation and latest-head timestamp access.
• Routes P2P and payload checks through blockchain-level state.
• Renames the startup override to force_blob_sampling and updates wiring.
• Preserves explicit L2 exclusion.

Final findings include four critical comments covering native-rollup L2 handling, later-fork schedules, and forced sampling bypassing L2 exclusion, plus one nit requesting focused tests.

File	Summary
crates/​storage/​store.rs	Adds latest-head timestamp access.
crates/​networking/​p2p/​rlpx/​connection/​server.rs	Uses centralized blockchain sampling state.
crates/​blockchain/​payload.rs	Routes eager-provider behavior through Blockchain.
crates/​blockchain/​mempool.rs	Adds atomic sampling state and latching.
crates/​blockchain/​blockchain.rs	Implements fork-based activation and centralized gating.
cmd/​ethrex/​l2/​initializers.rs	Updates L2 option wiring.
cmd/​ethrex/​initializers.rs	Wires the renamed sampling option.
cmd/​ethrex/​cli.rs	Clarifies blob-sampling help text.

---

💡 Add a code-review agent skill or configure MCP servers for context-aware, tailored reviews. Learn more in the docs.