Skip to content

fix(l1): stop eth_feeHistory aborting when the window is below retained history - #7207

Open
ilitteri wants to merge 1 commit into
mainfrom
fix/fee-history-range-underflow
Open

fix(l1): stop eth_feeHistory aborting when the window is below retained history#7207
ilitteri wants to merge 1 commit into
mainfrom
fix/fee-history-range-underflow

Conversation

@ilitteri

Copy link
Copy Markdown
Collaborator

Motivation

get_range clamps its two bounds independently — the finish is capped by the latest block, the start is raised to the earliest block we still hold (crates/networking/rpc/eth/fee_market.rs:231-234):

let finish_block_num = expected_finish_block_num.min(latest_block_num);
let expected_start_block_num = (finish_block_num + 1).saturating_sub(block_count);
let start_block_num = earliest_block_num.max(expected_start_block_num);

On a snap-synced node the earliest block is the pivot, so a request whose window lies entirely below the pivot yields start > end. The caller then computes (:104-105):

let block_count = (end_block - start_block + 1) as usize;
let mut base_fee_per_gas = vec![0_u64; block_count + 1];

With overflow checks that panics. In release it wraps, and the allocation asks for roughly 1.8e19 elements, which aborts the process. A single unauthenticated eth_feeHistory call is therefore a remote kill on any snap-synced node — that is, on the standard mainnet deployment.

Reproduced against a store whose earliest block is 5:

eth_feeHistory("0x1", "0x1", [])
  → panicked at crates/networking/rpc/eth/fee_market.rs:110:28:
    attempt to subtract with overflow

This will get easier to hit, not harder: history pruning (#6673) makes a non-zero earliest block the normal steady state rather than a snap-sync artifact.

Description

Return the empty fee history the caller is entitled to when the requested window lies entirely below retained history, matching the existing block_count == 0 path immediately above. Partially-available windows are unaffected and still serve the retained part with oldestBlock clamped up.

Tests

Two tests in test/tests/rpc/fee_history_range_tests.rs, against a store with earliest = 5 and earliest = 4 respectively:

  • a window entirely below the earliest retained block returns an empty history rather than crashing — this test panics with attempt to subtract with overflow without the fix
  • a window partially below it still serves the retained part, with oldestBlock clamped to the earliest retained block, so the fix does not turn a servable request into an empty one

get_range clamps its two bounds independently: the finish is capped by the latest
block, the start is raised to the earliest block still held. On a snap-synced node
the earliest block is the pivot, so a request whose whole window lies below the
pivot yields start > end.

The caller then computes `(end_block - start_block + 1) as usize`. With overflow
checks that panics; in release it wraps, and the following `vec![0_u64; count + 1]`
asks the allocator for ~1.8e19 elements, which aborts the process. That makes a
single unauthenticated eth_feeHistory call a remote kill on any snap-synced node.

Answer the empty history the caller is entitled to instead, matching the existing
zero-block-count path.
@ilitteri
ilitteri requested a review from a team as a code owner August 24, 2026 19:52
@github-actions

github-actions Bot commented Aug 24, 2026

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

This PR fixes a critical DoS vulnerability where an unauthenticated eth_feeHistory RPC call could crash the node. The fix is correct and well-tested.

Security Assessment

  • Critical vulnerability fixed: The subtraction end_block - start_block + 1 on line 113 (original) would wrap on u64 when start > end, causing vec![0; huge] to abort the process. This is a remote crash exploit.
  • Correct mitigation: Returning an empty FeeHistoryResponse when the window is entirely below the earliest retained block is the appropriate behavior per JSON-RPC semantics.

Code Quality

  • crates/networking/rpc/eth/fee_market.rs lines 103-110: The defensive check is correctly placed immediately after get_range and before the subtraction. The explanatory comment is excellent—clearly documents the snap-sync edge case.
  • Line 109: Using FeeHistoryResponse::default() for the empty case is idiomatic, assuming it produces empty arrays (which the tests confirm).

Test Coverage

  • test/tests/rpc/fee_history_range_tests.rs: Good coverage of both the crash scenario (window entirely below pivot) and the clamping behavior (partial overlap).
  • Line 28: Consider also asserting that oldestBlock is "0x0" or the earliest available in the empty response case to ensure consistent API behavior.

Minor Suggestions

  1. Test completeness: In fee_history_partially_below_earliest_still_serves_the_retained_part, consider verifying the array lengths match the expected 3 retained blocks (plus 1 for baseFeePerGas tail):

    assert_eq!(result["baseFeePerGas"].as_array().unwrap().len(), 4); // 3 blocks + 1
    assert_eq!(result["gasUsedRatio"].as_array().unwrap().len(), 3);
  2. Documentation: The comment in fee_market.rs mentions "wraps in release"—technically it's defined behavior for u64 (wrapping), but the panic only occurs on the allocation. Consider clarifying: "which wraps to a very large u64, causing the subsequent vec! allocation to abort."

Verdict: LGTM. The fix correctly prevents the arithmetic underflow/overflow and subsequent memory allocation failure. The tests validate both the security fix and correct functional behavior.


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

@github-actions

Copy link
Copy Markdown

🤖 Claude Code Review

This is a small, well-targeted fix. Here's the review:

Review: Stop eth_feeHistory aborting when the window is below retained history

Correctness

The fix is correct and minimal. get_range (fee_market.rs:222-246) clamps start and end independently, so a request window entirely below the retained history (earliest_block_num) produces start_block > end_block. The added guard at fee_market.rs:109-112 catches exactly this case before the unchecked end_block - start_block + 1 subtraction at line 114, which is the actual overflow site. This mirrors the existing block_count == 0 early-return pattern (lines 96-99), so the codebase now handles both "nothing to return" cases the same way — good consistency.

I traced through get_range's arithmetic for both new tests and confirmed the numbers line up:

  • Test 1 (earliest=5, request newestBlock=1, count=1): finish=1, start=max(5, 1)=5start > end → triggers the new guard, matching the described crash-without-fix scenario.
  • Test 2 (earliest=4, request newestBlock=6, count=10): finish=6, start=max(4, 0)=4 → normal path, serving blocks 4..=6 with oldestBlock clamped to 4 — confirms the partial-overlap case is untouched.

Minor observations (non-blocking)

  • FeeHistoryResponse::default() returns oldest_block: String::default() (i.e., ""), not a hex-encoded block number like "0x0". This is pre-existing behavior shared with the block_count == 0 path (line 96-99), so this PR isn't introducing a new inconsistency, but it's worth flagging that neither path matches go-ethereum's typical convention of still returning a valid oldestBlock (usually the request's would-be start, clamped) even for an empty result. Not something this PR needs to fix, since it strictly preserves existing empty-response semantics.
  • The inline comment at lines 103-108 duplicates a lot of the PR/commit description. It's accurate and helpful for a subtle bug, so it's a reasonable place to leave it, but slightly verbose for the one branch it explains.

Tests

Both new tests are well-constructed, use realistic helper setup (setup_store, add_legacy_tx_blocks, update_earliest_block_number), assert on the actual JSON-RPC response shape, and — per the PR description — the first test reproduces the panic on main and passes only with the fix. The second test guards against a regression where the fix over-corrects and empties out a partially-servable window. Good coverage for the bug's boundary condition.

Security

This directly closes a remote, unauthenticated DoS (panic/abort via a crafted eth_feeHistory call) that becomes increasingly likely as snap-sync and history-pruning make non-zero earliest_block the norm rather than the exception. The fix is narrowly scoped and doesn't change behavior for any previously-working request shape.

Overall: solid, well-tested fix for a real remote-DoS bug. No blocking issues.


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

@github-actions

Copy link
Copy Markdown

🤖 Codex Code Review

Findings

  1. crates/networking/rpc/eth/fee_market.rs still returns FeeHistoryResponse::default() for the new empty-history case, and that derived default makes oldest_block an empty string via the struct definition. This fixes the underflow/OOM DoS, but it makes a malformed eth_feeHistory payload newly reachable for a normal non-zero request below the retained pivot: oldestBlock is supposed to be a quantity, not "". I’d switch this branch to an explicit empty response with a valid oldestBlock value, and extend the new test to assert that field instead of only checking array lengths.

The guard itself looks correct and important from a security perspective: it closes a remote allocation-abort path in an unauthenticated RPC method. I did not find other correctness or EVM/consensus issues in the diff.

I couldn’t run the Rust tests in this environment because cargo/rustup attempted to write under read-only home directories.


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

@github-actions

github-actions Bot commented Aug 24, 2026

Copy link
Copy Markdown

Lines of code report

Total lines added: 4
Total lines removed: 0
Total lines changed: 4

Detailed view
+------------------------------------------------+-------+------+
| File                                           | Lines | Diff |
+------------------------------------------------+-------+------+
| ethrex/crates/networking/rpc/eth/fee_market.rs | 239   | +4   |
+------------------------------------------------+-------+------+

@klaus993

Copy link
Copy Markdown
Contributor

/kimi

@klaus993

Copy link
Copy Markdown
Contributor

/kimi

@github-actions

Copy link
Copy Markdown

🤖 Kimi Code Review

⚠️ Review failed: Kimi API request failed with status 400: invalid temperature: only 0.6 is allowed for this model


Automated review by Kimi (Moonshot AI)

Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment

Labels

L1 Ethereum client

Projects

Status: No status

Development

Successfully merging this pull request may close these issues.

2 participants