test(l1): cover the peer-response classification the backfill fetch path relies on

[ilitteri]

Motivation

Historical chain backfill (#7024) shipped with its peer fetch path untested. The
rules that decide whether a short or misaligned batch response comes from an honest
partial-history peer or a dishonest one lived inline in request_block_bodies and
request_receipts, tangled with peer-table side effects, so nothing could exercise
them. That is the code where a scoring bug already slipped through once: success was
credited before validation, so a peer answering with one valid body and sixty-three
invented ones gained score instead of losing it.

Description

Extracts the decision into two pure functions, classify_body_response and
classify_receipt_response, returning a ResponseVerdict:

• Complete — every returned entry validated against its header, in order.
• Compacted — an entry matches a later requested header, so the peer omitted
  blocks it does not have. Expected after history expiry; costs the peer nothing.
• Fabricated — an entry matches no requested header at all.

The callers map the verdict onto the peer table as before. Adds 18 tests: aligned
responses, truncated prefixes, compacted responses, invented entries,
one-valid-then-junk, empty responses, and the version negotiation that selects the
receipt wire format.

Two notes beyond a pure refactor:

• Fabricated receipts now earn a critical failure, matching bodies where they were
  previously only soft-penalized. A response can no longer be charged both a critical
  and a soft failure.
• negotiated_eth_version reimplements the handshake's highest-mutual rule by hand,
  and had no guard against drifting from it. Picking wrongly there sends a peer a
  request it cannot decode — invisible locally, failing only on their side. That is
  what broke receipt fetching against most of mainnet before feat(l1): optional historical chain backfill + DB observability #7024 merged, so it is
  now pinned, including the case of an eth/68 peer that also speaks eth/71.

One test documents a real limit rather than asserting a behaviour: blocks with no
transactions share the same transactions, withdrawals and receipts roots, so an empty
body is genuinely interchangeable between them and the classification cannot tell
compacted from fabricated. Harmless, since identical bytes get stored either way, but
better pinned than rediscovered.

Also extracts next_batch_range, the arithmetic choosing which blocks the next
batch covers. It carries a non-obvious rule with no test behind it: reaching
frontier == floor is not finished on the first batch of a run, because that
block is the snap pivot, which snap stored a body for but no receipts. A node whose
pivot landed exactly on the floor would otherwise keep that block without receipts
forever while reporting itself complete.

How to Test

cargo test -p ethrex-p2p --lib response_classification
cargo test -p ethrex-p2p --lib negotiated_version
cargo test -p ethrex-p2p --lib backfill

Related Issues

Follow-up to #7024. Does not close #5979 (snap-sync record/replay): backfill_step
end to end still needs a peer harness, and these classifiers are the piece that
harness would drive with recorded mainnet responses.

[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 refactors response classification logic into testable functions and fixes a backfill batch range bug. Let me analyze each part carefully.

crates/networking/p2p/peer_handler.rs

Response Classification Logic

Line 133-156 (classify_body_response):

The Compacted detection has a subtle bug. Consider this scenario:

• Requested headers: [H0, H1, H2, H3]
• Response bodies: [B0, B2, B2] (where B2 matches H2)

At index 1, B2 doesn't match H1. The code checks block_headers[2..] and finds B2 matches H2, so it returns Compacted { keep: 1 }. But the response has two B2s — the second B2 at index 2 would need to match H2 or H3. The function exits early without validating that subsequent entries form a valid compacted sequence.

This is actually worse: a malicious peer could send [B0, B2, B2, B2, ...] and get classified as Compacted when it's actually fabricated garbage after the first valid compacted entry.

Line 143-155 (classify_receipt_response): Same issue.

Line 144: block_headers[idx + 1..] — potential panic if idx == block_headers.len() - 1 and we access block_headers[idx]? No, the slice idx+1.. would be empty, any() returns false, so it becomes Fabricated. But wait: if idx == block_headers.len() - 1, then block_headers[idx + 1..] is empty, so compacted is false, returning Fabricated. But the body at the last position could validly not match if the peer is honest and we're checking beyond what they have... actually no, if they sent more bodies than headers, that's already invalid elsewhere. But if block_bodies.len() > block_headers.len(), we could index out of bounds at block_headers[idx] in the loop condition itself.

Actually, looking more carefully: the loop is for (idx, body) in block_bodies.iter().enumerate(), and we access block_headers[idx]. If block_bodies.len() > block_headers.len(), this panics. The callers appear to request with matching lengths, but this function should be defensive.

Suggested fix for empty blocks test (line 1318-1339): The test empty_blocks_validate_despite_identical_roots is good, but doesn't cover the case where identical roots across different positions could mask compaction detection.

Caller Logic

Line 720-760 (body fetching):

The refactor correctly preserves behavior: Fabricated triggers record_critical_failure, and the soft penalty on empty/unusable responses is preserved.

Line 870-895 (receipt fetching):

Same structure, good. But note: the old code had a bug where receipt root mismatch on any block would just break and keep prior verified receipts. The new code correctly distinguishes Compacted (no penalty) from Fabricated (critical failure). However, Compacted case for receipts is silently accepted without debug logging — intentional or oversight?

Looking at old code: it had no compaction detection for receipts at all! It would break on first mismatch and treat all mismatches as truncations. The new code adds compaction detection, which is a behavior change. If a peer sends compacted receipts [R0, R2] for headers [H0, H1, H2], old code kept just R0, new code keeps R0 and doesn't penalize. This is more correct, but the PR description should highlight this.

Tests

Line 1247 (one_valid_entry_then_junk_is_still_fabricated):

response.extend(std::iter::repeat_n(junk, 7));

This creates [B0, J, J, J, J, J, J, J]. At index 1, J doesn't match H1. Checks H2..H7, none match. Returns Fabricated { keep: 1, at: 1 }. Good.

But what about [B0, B2, J, J, J, J, J, J]? This would be Compacted { keep: 1 } because B2 matches H2. The J entries are never validated. This is the bug I identified above.

Line 1280-1301 (empty_blocks_validate_despite_identical_roots):

Good test, but doesn't test the compaction case with empty blocks: [empty_body, empty_body] for headers [H0, H1, H2] where both empty bodies have identical roots. This would be misclassified as Complete { keep: 2 } when it's actually compacted (missing H1's body). Though with empty bodies, validate_block_body would pass for any position since all empty bodies are identical... actually no, validate_block_body checks against the specific header, and if the transactions root matches, it's valid. Two empty blocks have the same empty transactions root, so validate_block_body(H1, empty_body) would pass if H1 has empty transactions root.

Wait, that's actually correct behavior — if the body validates against the header, it's valid. The issue is distinguishing honest truncation from dishonest fabrication when bodies are identical.

crates/networking/p2p/sync/backfill.rs

next_batch_range function

Line 307-325:

fn next_batch_range(
    frontier: BlockNumber,
    floor: BlockNumber,
    first_batch: bool,
    batch_size: u64,
) -> Option<(BlockNumber, BlockNumber)> {
    if frontier < floor || (frontier == floor && !first_batch) {
        return None;
    }
    let hi = if first_batch { frontier } else { frontier - 1 };
    let lo = hi.saturating_sub(batch_size - 1).max(floor);
    Some((lo, hi))
}

Line 317: frontier - 1 when frontier == 0 and !first_batch. This would underflow! But frontier < floor check at line 314 handles frontier == 0, floor == 0 with !first_batch → returns None. What about frontier == 0, floor == 0, first_batch == true? Then hi = 0, lo = 0.max(0) = 0. OK.

What about frontier == 0, floor == 0, first_batch == false? Returns None. OK.

What about frontier == 1, floor == 0, first_batch == false? hi = 0, lo = 0.max(0) = 0. Returns Some((0, 0)). But wait, frontier < floor is false, frontier == floor is false. So we compute hi = frontier - 1 = 0. That's correct.

But frontier == 0, floor == 0 with first_batch == false: already handled.

Is there any case where frontier == 0 and !first_batch where frontier >= floor and frontier != floor? That requires floor < 0, but BlockNumber is u64, so no.

Actually wait — frontier == 0, floor == 0, first_batch == true is fine. But the old code had:

if frontier <= floor && !plan.first_batch { return Complete }
if frontier < floor { return Complete }

The new code: if frontier < floor || (frontier == floor && !first_batch). This combines both old checks correctly.

Line 319: hi.saturating_sub(batch_size - 1) — if batch_size is 0, this is saturating_sub(u64::MAX) which gives 0. But batch_size is a constant 64, so fine. If someone passes batch_size == 0, it's a bug but not exploitable.

Tests

Line 757-808: Good coverage of edge cases.

Line 785: next_batch_range(15_537_394, 15_537_394, true, 64) — this is the regression test for the actual bug. Good.

Line 808: next_batch_range(120, 100, false, 64) — tests clamping. Good.

Summary of Issues

Critical: Response classification doesn't validate compacted sequences

File: crates/networking/p2p/peer_handler.rs, Lines: 133-156, 166-189

The Compacted verdict only checks if a mismatched entry matches some later header, but doesn't verify that subsequent entries continue to match validly. A peer could send [valid_B0, valid_B2, junk, junk, ...] and get Compacted { keep: 1 } instead of Fabricated.

Fix: After detecting compaction at position idx, continue validating that remaining entries match subsequent headers in order:

// In classify_body_response, when compacted is true:
// Verify the rest of the response forms a valid compacted sequence
let mut header_idx = idx + 1;
for body in block_bodies[idx..].iter() {
    while header_idx < block_headers.len() {
        if validate_block_body(&block_headers[header_idx], body, &NativeCrypto).is_ok() {
            break;
        }
        header_idx += 1;
    }
    if header_idx >= block_headers.len() {
        // This body matches no remaining header
        return ResponseVerdict::Fabricated { keep, at: idx };
    }
    header_idx += 1;
}
return ResponseVerdict::Compacted { keep };

Actually, this is more nuanced. The "compacted" protocol: peer omits blocks they don't have. So [B0, B2, B3] for [H0, H1, H2, H3] is valid. But [B0, B2, B2] is not — the second B2 doesn't match H3.

The current code returns Compacted { keep: 1 } for [B0, B2, B2] which is wrong — it should be Fabricated { keep: 1, at: 2 } or similar.

Medium: Potential panic on mismatched lengths

File: crates/networking/p2p/peer_handler.rs, Lines: 136, 147

If block_bodies.len() > block_headers.len(), block_headers[idx] panics. Add a guard:

if block_bodies.len() > block_headers.len() {
    // Peer sent more bodies than requested headers — invalid
    return ResponseVerdict::Fabricated { keep: 0, at: block_headers.len() };
}

Or use get and handle None.

Minor: Missing debug log for compacted receipts

File: crates/networking/p2p/peer_handler.rs, Lines: 870-895

The receipt handling doesn't log Compacted cases unlike body handling. Add debug logging for consistency.

Minor: Test doesn't cover malicious compacted sequences

File: crates/networking/p2p/peer_handler.rs, Lines: 1247-1258

Add test:

#[test]
fn compacted_response_must_still_be_valid_sequence() {
    let (headers, bodies, _) = chain(4);
    // B0 is valid, B2 matches H2, but then B2 again doesn't match H3
    let bad = vec![bodies[0].clone(), bodies[2].clone(), bodies[2].clone()];
    assert_eq!(
        classify_body_response(&headers, &bad),
        ResponseVerdict::Fabricated { keep: 1, at: 2 }
    );
}

Style: NativeCrypto passed by reference repeatedly

File: crates/networking/p2p/peer_handler.rs, Lines: 140, 142, 177

&NativeCrypto is a ZST (zero-sized type) used as a trait parameter. Passing it repeatedly in loops is fine (optimized away), but slightly noisy. Not a real issue.

Positive Acknowledgments

1. The ResponseVerdict enum cleanly separates concerns and enables unit testing — excellent refactor.
2. next_batch_range extraction with comprehensive tests fixes a real consensus-adjacent bug (missing receipts on pivot block).
3. The negotiated version tests document an important mainnet interoperability issue (EIP-7975 / eth/71).
4. Comments are thorough and explain why rules exist, not just what they do.

---

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

[github-actions]

🤖 Codex Code Review

1. crates/networking/p2p/peer_handler.rs to peer_handler.rs: a fabricated receipts response can still earn the peer a success if it has a valid prefix. record_critical_failure(peer_id) is followed by record_success(peer_id) when verified > 0, so a peer that appended invented receipts is still treated as having succeeded for scoring purposes. That matters because peer selection does not exclude critically failed peers; it only gives them a low but non-zero weight (peer_table.rs to peer_table.rs). I’d avoid record_success for ResponseVerdict::Fabricated, or mark such peers disposable if “discarding peer” is the intended policy.

2. crates/networking/p2p/sync/backfill.rs to backfill.rs: diag.backfill_complete = frontier <= floor still reports completion too early on the exact case this patch is fixing, frontier == floor && first_batch == true. next_batch_range() correctly schedules one more batch for that case, but diagnostics can report backfill_complete = true during the whole network fetch/write window before the batch finishes. If operators or tests rely on SyncDiagnostics, this remains observably wrong. I’d derive backfill_complete from next_batch_range(...).is_none() or include !plan.first_batch in the condition.

I didn’t find other obvious correctness or EVM/consensus issues in the diff itself. I couldn’t run the targeted Rust tests here because cargo/rustup tried to write under /home/runner/.rustup, which is read-only in this sandbox.

---

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

[github-actions]

Lines of code report

Total lines added: 284
Total lines removed: 0
Total lines changed: 284

Detailed view

+-----------------------------------------------+-------+------+
| File                                          | Lines | Diff |
+-----------------------------------------------+-------+------+
| ethrex/crates/networking/p2p/peer_handler.rs  | 1046  | +238 |
+-----------------------------------------------+-------+------+
| ethrex/crates/networking/p2p/sync/backfill.rs | 540   | +46  |
+-----------------------------------------------+-------+------+

[github-actions]

🤖 Claude Code Review

Review Summary

Overall this is a well-executed, test-only-motivated refactor. Extracting classify_body_response, classify_receipt_response, and next_batch_range into pure functions is the right move — they're now directly testable, and I verified the extracted logic is behaviorally equivalent to the inline code it replaces (index-safety on block_headers[idx] is guaranteed by the pre-existing oversized-response guards in request_block_bodies_inner/request_receipts_inner, and the next_batch_range arithmetic can't underflow given its guard ordering). The 18 new tests are well-targeted, including the pivot/floor edge case and the honest "compacted" vs. dishonest "fabricated" distinction.

Finding 1 (real bug, pre-existing for bodies, newly extended to receipts): In both request_block_bodies (peer_handler.rs:743) and request_receipts (~line 888), when a response is classified Fabricated but has a non-empty valid prefix (keep > 0) — exactly the "one valid entry then junk" scenario this PR's own regression test is named for — the code calls record_critical_failure in the match arm, then falls through to if valid_upto > 0 { ...; record_success(peer_id)?; }, crediting the peer a success right after penalizing it critically. Since record_critical_failure does an absolute set to MIN_SCORE_CRITICAL (-150) rather than a decrement, the subsequent record_success bumps it back to -149, which doubles the peer's selection weight in do_get_random_peer (1 → 2 per peer_table.rs:1631). The dollar effect is small given the weighting clamp, but it's a logical contradiction that directly reintroduces (in weakened form) the exact bug this PR's motivation describes. This is pre-existing for bodies, but this PR is what adds the same record_critical_failure call to the receipts path, so it's a new instance there. Worth fixing by skipping record_success whenever verdict is Fabricated, even with keep > 0 (the valid prefix can still be returned/used without crediting score).

Finding 2 (minor/nit): ResponseVerdict::Fabricated { keep, at } always has keep == at by construction (both are the break index) — the two fields never diverge, so carrying both is redundant and could mislead a future editor into thinking they're independent.

Finding 3 (minor/observability): The Compacted debug log dropped the block_number field the original inline code logged, slightly reducing diagnostic detail — not a functional issue.

No consensus-correctness, RLP, or gas-accounting concerns here — this is peer-scoring/networking code, and the validation logic (validate_block_body, compute_receipts_root against receipts_root) is unchanged, just relocated.

---

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