[ilitteri]

Motivation

Following #7287, clients have been comparing their Sepolia Amsterdam fork IDs by hand ahead of the fork. geth, reth and nethermind all advertise 0x6c1d9423.

ethrex agrees. Printing what ForkId::new returns for the Sepolia preset:

BPO2 (1761607008)                      -> fork_hash=0x268956b6  fork_next=1791294816
just before Amsterdam (1791294815)     -> fork_hash=0x268956b6  fork_next=1791294816
Amsterdam activation (1791294816)      -> fork_hash=0x6c1d9423  fork_next=0
well after (2741159776)                -> fork_hash=0x6c1d9423  fork_next=0

So we match on both sides of the boundary: BPO2 stays the head fork until activation while announcing Amsterdam as fork_next, and from activation on Amsterdam is the head fork with nothing following.

Description

sepolia_test_cases already covers this boundary, but it drives ForkId through is_valid, which can also accept a remote hash by the EIP-2124 subset and superset rules (#2/#3) rather than by the direct hash comparison in rule #1. In the Amsterdam rows it does take rule #1, so the coverage is real — but the test does not pin the value, and a future change could keep it passing through a different rule while altering what we actually put on the wire.

This adds a test that asserts the advertised fork_hash and fork_next directly. No production change; the schedule itself landed in #7287.

Getting this wrong does not produce a wrong block, it partitions us: peers reject the handshake on fork-ID mismatch, so the failure looks like "no peers" rather than a test failure. That is worth a direct assertion.

How to Test

cargo test -p ethrex-common --lib fork_id

9 passed, including the new sepolia_advertises_amsterdam_fork_id and the unchanged sepolia_test_cases and hoodi_test_cases.

Notes for reviewers

• Unrelated, noticed while running lint: cargo clippy -p ethrex-common --all-targets -- -D warnings already fails on main with needless_return at crates/common/crypto/kzg.rs:289, in the non-c-kzg body of verify_cell_kzg_proof_batch_partial. It is another branch that nothing builds, so nothing lints it either. Not touched here.

Checklist

• Updated STORE_SCHEMA_VERSION (crates/storage/lib.rs) if the PR includes breaking changes to the Store requiring a re-sync. (Not needed: test only.)

[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. Four
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, then #3652 in tests-zkevm@v21.0.1, which removed
public_keys outright without bumping the revision. 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 diff focusing on the aspects you mentioned.

Review: Fork ID Test for Sepolia Amsterdam Boundary

Overall Assessment

This is a well-crafted regression test that pins critical network behavior. The code is largely correct and follows good practices, but I have several observations.

---

Specific Findings

1. Potential Test Fragility: Hardcoded Block Number head

File: crates/common/types/fork_id.rs, Lines 618-619

let head = 1_735_372;

The head value is used but never validated against the time parameter in the test assertions. The comment says "Any post-Merge head works"—if this assumption ever becomes false (e.g., future fork changes behavior), the test could pass with a misleading head value. Consider adding a brief assertion that head is indeed post-Merge, or document why this specific number was chosen.

2. Timestamp 1_735_372 Appears to Be a Block Number, Not a Timestamp

File: crates/common/types/fork_id.rs, Lines 618-619

The variable is named head and the comment says "Any post-Merge head works: Amsterdam is timestamp-based." However, 1_735_372 looks like a block number (Sepolia's Merge block was ~1,735,000). The ForkId::new signature takes time as its third parameter based on usage at lines 623 and 628.

Wait—looking more carefully at the call sites:

• Line 623: ForkId::new(genesis.config, genesis_header.clone(), 1_791_294_815, head)
• Line 628: ForkId::new(genesis.config, genesis_header.clone(), time, head)

The fourth parameter is head (block number), third is time (timestamp). This appears correct based on the function signature, but the naming is slightly confusing since head is typically used to mean "current head block hash/number." The comment could clarify that head here means block number for pre-Merge forks, ignored post-Merge.

3. Missing Edge Case: Exactly at Activation Boundary

File: crates/common/types/fork_id.rs, Lines 627-634

The test checks 1_791_294_816 (activation time) and 2_741_159_776 (far future), but doesn't explicitly test the transition at time = 1_791_294_815 with head at a boundary case. The "before" test uses timestamp 1_791_294_815 with head = 1_735_372—this is correct, but consider whether head being pre-Merge vs post-Merge affects the time interpretation.

Actually, re-reading: the test structure is correct. The "before" case at line 623 uses time = 1_791_294_815 (one second before activation), and the "at" cases start at 1_791_294_816. This is proper boundary testing.

4. Hex String Parsing: Minor Style Note

File: crates/common/types/fork_id.rs, Lines 624, 629-630

H32::from_str("0x268956b6").unwrap()

This is consistent with existing code in the file. However, unwrap() in tests is acceptable. No issue here.

5. Test Documentation Could Be Clearer on fork_next Semantics

File: crates/common/types/fork_id.rs, Lines 626, 632

assert_eq!(before.fork_next, 1_791_294_816);
// ...
assert_eq!(at.fork_next, 0, "fork next at {time}");

The fork_next value of 0 when no fork follows is EIP-2124 convention, but this isn't explicitly documented in the test. Consider adding a comment that 0 means "no subsequent fork scheduled."

6. No Verification That This Is Actually Amsterdam Fork

File: crates/common/types/fork_id.rs, Lines 617-634

The test verifies the hash values but doesn't programmatically verify that 1_791_294_816 corresponds to the Amsterdam fork in the genesis config. If the genesis config changes accidentally, the test failure would show wrong hash values rather than clearly indicating "Amsterdam timestamp changed." Consider:

// Verify Amsterdam activation time from config
let amsterdam_time = genesis.config.shanghai_time.expect("Sepolia has Shanghai/Amsterdam");
assert_eq!(amsterdam_time, 1_791_294_816, "Amsterdam timestamp changed - update test");

Wait—checking: Amsterdam is the same as Shanghai on Sepolia? Or is this a different fork? The comment says "Amsterdam" but the config field might be shanghai_time. If there's a specific amsterdam_time or this is an alias, the test should verify the mapping. This is important for maintainability—future developers may not know that "Amsterdam" = some specific config field.

---

Questions for PR Author

1. Is 1_791_294_816 available in genesis.config? If so, the test should derive it rather than hardcode, making the test self-documenting and resilient to config updates.

2. Why head = 1_735_372 specifically? If this is the Merge block number, consider naming it post_merge_block_number or adding a comment.

---

Positive Aspects

• Critical network compatibility test: Prevents accidental network partitioning—excellent justification in the doc comment (lines 607-612).
• Clear boundary testing: Before, at, and after activation.
• Descriptive assertion messages: The assert_eq! with , "fork hash at {time}" format strings helps diagnose failures.
• Follows existing patterns: Consistent with other tests in the module.

---

Suggested Improvements (Minor)

// Around line 618, consider:
let post_merge_head = 1_735_372; // Sepolia Merge block; post-Merge so timestamp-based forks apply

// Around line 623, optionally verify config consistency:
let amsterdam_activation = genesis.config.shanghai_time.expect("Sepolia Amsterdam timestamp");
assert_eq!(amsterdam_activation, 1_791_294_816);

If the config field is differently named or structured, adjust accordingly. The key is linking the hardcoded test value to the authoritative config source.

---

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

[github-actions]

🤖 Claude Code Review

This is a small, well-scoped test-only PR. Overall it's clean and does what it says. A few minor notes:

Findings

1. Minor style: missing blank line before doc comment (crates/common/types/fork_id.rs:608-609). Every other test in the file is separated by a blank line; the new /// doc comment butts directly against the closing } of remote_needs_software_update. Purely cosmetic, but easy to fix and rustfmt/cargo fmt won't insert it for you.

2. head value is a bit of a magic number (fork_id.rs:620). 1_735_372 is reused from sepolia_test_cases (the last block-number-gated fork boundary) without a named constant or comment explaining where it comes from beyond "any post-Merge head works." That's true and the comment says as much, so this is a nit, not a defect — but a short pointer like "same head used in sepolia_test_cases" would save a reader from having to verify it independently.

3. 2_741_159_776 "well after" timestamp isn't tied to anything meaningful. It's just an arbitrary future timestamp (matches the PR description). Since Amsterdam has no successor fork configured yet, this is fine as a sanity check that the head fork stays stable arbitrarily far in the future — no real bug, just worth knowing this isn't tied to any EIP/spec value.

4. Good test design overall: pinning the literal fork_hash/fork_next bytes via direct assert_eq! (rather than only exercising is_valid, which per EIP-2124 rules 2/3 could pass even if the advertised hash drifted) is exactly the right way to catch a silent fork-ID regression that would otherwise only manifest as network partitioning rather than a test failure. The doc comment clearly explains why this test exists alongside sepolia_test_cases, which is good given the subtlety.

5. No production code touched — confirmed via the diff; this only adds a test in crates/common/types/fork_id.rs. No consensus, gas, RLP, or state-trie logic is affected, so the broader Ethereum-specific review categories don't apply here.

6. Values used (0x268956b6/1791294816, 0x6c1d9423/0) match the existing Amsterdam boundary rows already present in sepolia_test_cases (fork_id.rs:505-523), so the pinned values are internally consistent with the schedule already in the codebase, not just asserted in isolation.

No correctness, security, or performance concerns — this is a test-only addition that improves regression coverage for a real (and previously noted, non-obvious) gap.

---

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

[github-actions]

🤖 Codex Code Review

No findings.

The PR is test-only in crates/common/types/fork_id.rs, and the added sepolia_advertises_amsterdam_fork_id case usefully closes a real gap: sepolia_test_cases validates compatibility through is_valid, but it does not pin the exact fork ID that Ethrex advertises on the wire. The new assertions at lines 624 through 636 correctly check both sides of the Amsterdam timestamp boundary and guard against a network-partitioning regression.

No security, consensus, gas-accounting, or performance concerns from this diff. I could not execute the test in this environment because cargo/rustup need write access under /home/runner/.rustup and /home/runner/.cargo, which are read-only here.

---

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

[github-actions]

Lines of code report

Total lines added: 17
Total lines removed: 0
Total lines changed: 17

Detailed view

+---------------------------------------+-------+------+
| File                                  | Lines | Diff |
+---------------------------------------+-------+------+
| ethrex/crates/common/types/fork_id.rs | 572   | +17  |
+---------------------------------------+-------+------+