Skip to content

fix(l1): report a block whose receipts are absent instead of answering empty - #7208

Merged
ilitteri merged 1 commit into
mainfrom
fix/raw-receipts-completeness
Aug 24, 2026
Merged

fix(l1): report a block whose receipts are absent instead of answering empty#7208
ilitteri merged 1 commit into
mainfrom
fix/raw-receipts-completeness

Conversation

@ilitteri

Copy link
Copy Markdown
Collaborator

Motivation

get_receipts_for_block returns a bare Vec (crates/storage/store.rs:1491-1497), so "this block's receipts are not stored" and "this block had no transactions" are the same value. get_all_block_receipts passed that straight through with no length check, so debug_getRawReceipts answered an empty list for a block that has transactions — a wrong answer rather than a reported failure.

The sibling by-index path already guards against this (crates/networking/rpc/eth/block.rs:408-414, "Return 500 on receipt count mismatch"); the all-receipts path did not.

This matters ahead of history pruning (#6673). Today an absent receipt set means corruption, which is rare. Once pruning lands it becomes a normal steady-state outcome, so every path that conflates "absent" with "empty" starts returning wrong answers on every node rather than on a corrupted one. Making these paths honest is a prerequisite for pruning being safe to enable, and it is worth doing on its own merits regardless.

Description

get_all_block_receipts now checks the receipt count against the block's own transaction count, and reports a block whose body is absent rather than validating against nothing. Genesis keeps its existing short circuit, since it legitimately has no receipts.

The error is RpcErr::Internal to match the sibling path. Once #7069 lands its PrunedHistoryUnavailable variant (JSON-RPC code 4444), both sites should move to it together — an absent-because-pruned block deserves a distinct code from a corrupt one, and splitting that out keeps this change reviewable.

Tests

Two tests in test/tests/rpc/raw_receipts_completeness_tests.rs. The harness stores blocks and their transactions but no receipts, which is exactly the shape a pruned block presents — body present, receipts gone:

  • a block with one transaction and no stored receipts must report a failure rather than return an empty list. Verified to fail without the change (test result: FAILED) and pass with it
  • genesis must still answer with an empty list, so the completeness check does not break the one block that legitimately has none

get_receipts_for_block returns a bare Vec, so "this block's receipts are not
stored" and "this block had no transactions" are the same value. get_all_block_receipts
passed that straight through, so debug_getRawReceipts answered an empty list for a
block that has transactions — a wrong answer rather than a reported failure.

Check the receipt count against the block's own transaction count, and report a
block whose body is absent rather than validating against nothing. This mirrors the
mismatch check the by-index receipt path already performs. Genesis keeps its
existing short circuit, since it legitimately has none.

This matters ahead of history pruning (#6673): an absent receipt set stops being a
corruption signal and becomes a normal steady-state outcome, so every path that
conflates it with emptiness starts returning wrong answers on every node rather
than on a corrupted one.
@ilitteri
ilitteri requested a review from a team as a code owner August 24, 2026 19:57
@github-actions

Copy link
Copy Markdown

⚠️ 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.rsunsync_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 github-actions Bot added the L1 Ethereum client label Aug 24, 2026
@github-actions

Copy link
Copy Markdown

🤖 Kimi Code Review

The PR correctly addresses the ambiguity between "receipts not stored" and "block had no transactions" in the RPC layer. This is an important fix for future pruning support.

crates/networking/rpc/eth/block.rs

Lines 456-477: The validation logic is sound. However, consider using get("result") instead of indexing to avoid panics if the field is absent.

test/tests/rpc/raw_receipts_completeness_tests.rs

Lines 24-34: The assertion response["result"] assumes the field always exists. If the JSON-RPC response omits the result field when error is present (per JSON-RPC 2.0 spec), this will panic. Use response.get("result") for safety.

Lines 55-58: Same as above regarding response["result"] indexing.

General observations:

  1. Storage consistency: The two storage calls (get_receipts_for_block and get_block_body_by_hash) introduce a potential race condition if pruning occurs between them. Given the immutable nature of historical Ethereum data in this context, this is likely acceptable, but consider documenting this assumption.

  2. Performance: Two separate database lookups are now required for this RPC method. This is correct for data integrity, but consider adding a comment or TODO about potentially batching these reads if this becomes a hot path.

  3. Error handling: Returning RpcErr::Internal for data integrity violations is appropriate. The error messages include the block hash for debugging without leaking sensitive information.

The logic correctly preserves the early return for genesis (block 0) while adding necessary validation for all other blocks.


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

@github-actions

Copy link
Copy Markdown

🤖 Claude Code Review

Review: PR 7208 — Report a block whose receipts are absent instead of answering empty

Summary: Fixes get_all_block_receipts (backing debug_getRawReceipts) so that a block with a stored body but missing receipts reports a JSON-RPC error instead of silently returning an empty list — matching the existing safeguard on the sibling by-index path (get_all_block_rpc_receipts, block.rs:407-414).

Correctness

  • The fix is correct: it validates receipts.len() against body.transactions.len(), mirroring the established pattern at block.rs:409, and preserves the genesis short-circuit (block.rs:452-454) which legitimately has zero receipts.
  • Fetching the body via get_block_body_by_hash(block_hash) (block.rs:462), where block_hash is derived from the already-fetched header, is actually safer than the caller's original by-number header lookup (block.rs:310) — it pins the body to the exact block identified by that header, avoiding a reorg-window mismatch between a by-number header fetch and a by-hash body fetch.
  • Error variant (RpcErr::Internal) correctly maps to a JSON-RPC error response (utils.rs:121, rpc.rs:414), consistent with the sibling path.

Minor / non-blocking observations

  • Extra sequential I/O (block.rs:456,462): get_receipts_for_block and get_block_body_by_hash are independent lookups but run sequentially with two .awaits. Since debug_getRawReceipts isn't a hot path, this is likely fine, but tokio::join! would shave one round trip if this ever becomes latency-sensitive.
  • Duplicated mismatch-error construction: the "Expected N, got M" error string is now built in two places (block.rs:409-414 and 472-477) with slightly different wording. The PR description already flags this as deliberately deferred pending the PrunedHistoryUnavailable variant from feat(l1): add debug_getRawBlockAccessList and align BAL getter errors #7069 — reasonable to leave as-is for reviewability.
  • API asymmetry: get_all_block_rpc_receipts takes body: BlockBody as a parameter while get_all_block_receipts fetches it internally by hash. Not a bug (different call sites have different data available going in), just a slight inconsistency worth a one-line note if a future refactor unifies these two functions.

Tests

  • Both new tests (raw_receipts_completeness_tests.rs) correctly exercise the fix: one confirms a block with a transaction but no stored receipts now errors instead of returning [], the other confirms genesis is unaffected. Verified against test_utils.rs helpers (setup_store, add_legacy_tx_blocks, call_http) — all exist and behave as the test assumes.
  • Nit: the genesis test also calls add_legacy_tx_blocks(&store, 1, 1) before querying block 0x0, which is unused setup carried over from copy-pasting the other test — harmless but could be dropped for clarity.

Overall this is a small, well-scoped, low-risk correctness fix with good test coverage. No blocking issues found.


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

@github-actions

Copy link
Copy Markdown

Lines of code report

Total lines added: 15
Total lines removed: 0
Total lines changed: 15

Detailed view
+-------------------------------------------+-------+------+
| File                                      | Lines | Diff |
+-------------------------------------------+-------+------+
| ethrex/crates/networking/rpc/eth/block.rs | 412   | +15  |
+-------------------------------------------+-------+------+

@github-actions

Copy link
Copy Markdown

🤖 Codex Code Review

  1. crates/networking/rpc/eth/block.rs introduces a behavior regression for header-only historical blocks. debug_getRawReceipts now turns get_block_body_by_hash(...)=None into RpcErr::Internal, but snap-synced nodes intentionally have canonical headers without bodies below the pivot (crates/networking/p2p/sync/backfill.rs). Other RPCs already treat missing body/history as unavailable data and return null instead of a 500 (crates/networking/rpc/eth/block.rs). The new receipt-count validation is good when a body exists, but None here should stay on the “data unavailable” path rather than being classified as an internal error.

Aside from that, the receipt-count check itself is sound, and the new tests cover the main missing-receipts regression plus the genesis exception.

I could not run cargo test in this environment because rustup failed to create temporary files under a read-only /home/runner/.rustup.


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

@github-project-automation github-project-automation Bot moved this to In Review in ethrex_l1 Aug 24, 2026
@ilitteri
ilitteri added this pull request to the merge queue Aug 24, 2026
Merged via the queue into main with commit 3b6c82d Aug 24, 2026
61 checks passed
@ilitteri
ilitteri deleted the fix/raw-receipts-completeness branch August 24, 2026 21:33
@github-project-automation github-project-automation Bot moved this from In Review to Done in ethrex_l1 Aug 24, 2026
Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment

Labels

L1 Ethereum client

Projects

Status: Done

Development

Successfully merging this pull request may close these issues.

2 participants