Skip to content

ci(l1): run hive from a fork that pins the devp2p simulator's go-ethereum - #7265

Open
ilitteri wants to merge 1 commit into
mainfrom
ci/pin-hive-devp2p-geth
Open

ilitteri wants to merge 1 commit into
mainfrom
ci/pin-hive-devp2p-geth

Conversation

@ilitteri

@ilitteri ilitteri commented Sep 9, 2026

Copy link
Copy Markdown
Collaborator

Motivation

Every PR's Hive - Devp2p tests job has been red since 2026-09-03, which fails the required Integration Test check. Upstream hive's devp2p simulator clones go-ethereum master at image build time, so hive_version never pinned that suite: geth's Osaka testdata + eth/72 commit changed the testchain's fork id and every eth and snap test now fails at the status handshake with wrong fork ID, against a client that did not change. The daily hive report shows the same collapse (Eth 22/22 → 1/25 overnight on the same ethrex commit).

Description

Both hive workflows now use lambdaclass/hive-ci, a fork of ethereum/hive at the previously pinned commit plus one change to simulators/devp2p/Dockerfile: the go-ethereum checkout is fetched at a fixed commit (157c94647, the one before the breaking change) exposed as ARG GETH_REF. Moving the devp2p suite forward becomes a deliberate bump of that arg. Adopting eth/72 + Osaka in the devp2p suite is separate work; until then this restores the coverage we had.

Evidence

This PR also triggers the daily hive report workflow, so the report was generated from the fork on this branch: P2P is back to Eth 22/22, Snap 6/6, Discovery V4 16/16, total 124359/124370 (99.99%). The 2026-09-03 report on upstream hive, after geth moved, read Eth 1/25 and Snap 0/6. Every hive suite passes on the fork and the required Integration Test check is green.

Checklist

  • No Store changes.

…reum

Upstream hive's devp2p simulator clones go-ethereum master at image build time,
and both the test binary and the /testchain fixtures come from that checkout.
Pinning `hive_version` therefore never pinned the devp2p suite. On 2026-09-02
geth added Osaka testdata and eth/72 tests, and from the next run on every eth
and snap test failed at the status handshake with "wrong fork ID", in the daily
report and in every PR's Hive - Devp2p job, which the required Integration Test
check aggregates.

Point both workflows at lambdaclass/hive-ci, a fork identical to the pinned
upstream commit plus one change: the simulator fetches a fixed go-ethereum
commit exposed as the GETH_REF build arg.
@ilitteri
ilitteri requested a review from a team as a code owner September 9, 2026 14:23
@github-actions github-actions Bot added the L1 Ethereum client label Sep 9, 2026
@github-actions

github-actions Bot commented Sep 9, 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 commented Sep 9, 2026

Copy link
Copy Markdown

🤖 Kimi Code Review

I'll review this PR which changes the Hive testing infrastructure to use a LambdaClass fork instead of upstream ethereum/hive.

Review

Overall Assessment

This is a CI infrastructure change to address a breaking upstream dependency. The change is straightforward and well-documented, but there are several concerns to address.

Issues Found

1. Date in comment appears to be in the future (.github/workflows/daily_hive_report.yaml:196, .github/workflows/pr-main_l1.yaml:417)

# ...its eth/snap suite changed under us on 2026-09-02 (Osaka testdata...

The date 2026-09-02 is in the future. This appears to be a typo — likely should be 2025-09-02 or an earlier date. If this is meant to document when the breakage occurred, an incorrect future date reduces confidence in the change and could confuse future maintainers.

Suggestion: Verify and correct the date.

2. Fork maintenance burden and visibility

The PR introduces lambdaclass/hive-ci as a long-term dependency. The comments explain the rationale well, but there's no documented plan for:

  • When/how to return to upstream ethereum/hive
  • Who maintains the fork
  • How to update GETH_REF in the fork when needed

Suggestion: Consider adding a comment or tracking issue reference for returning to upstream once the upstream issue is resolved (e.g., if ethereum/hive pins geth themselves or makes the reference configurable).

3. Commit hash verification

The hive_version: bda630136e76df758b1ab3f6879af3ae0b106813 is referenced but there's no easy way for reviewers to verify what this commit contains without manually fetching the fork.

Suggestion: Consider adding a direct GitHub compare link in a PR description or comment, e.g.:

# Verify: https://github.com/lambdaclass/hive-ci/commit/bda630136e76df758b1ab3f6879af3ae0b106813

4. Inconsistent comment formatting between files

In daily_hive_report.yaml, the comment uses // for jq syntax reference:

# #1524 wrapped the `//` value in parens...

This old jq-specific context is removed, which is correct since the new fork makes this historical detail less relevant. However, the two files now have slightly different comment structures — daily_hive_report.yaml has a shorter comment without the GETH_REF mention, while pr-main_l1.yaml explicitly mentions GETH_REF.

Suggestion: Align the comments for consistency, or make the shorter one in daily_hive_report.yaml also mention GETH_REF for clarity.

5. No fallback or rollback mechanism

If lambdaclass/hive-ci becomes unavailable or the specific commit is force-pushed away, CI will break with no automatic recovery.

Suggestion: This is acceptable for a temporary measure, but document the risk. Consider whether the fork should have branch protection rules preventing force-push to preserve the commit.

Positive Aspects

  1. Well-explained rationale: Comments clearly explain why the fork is needed (upstream geth master drift causing "wrong fork ID" failures)
  2. Pinned commit: Using a specific commit hash rather than a floating branch tag provides reproducibility
  3. Scoped change: Only affects the devp2p simulator's geth dependency; other hive functionality unchanged
  4. Both workflows updated consistently: Daily report and PR CI both moved to the fork

Minor Nit

In pr-main_l1.yaml:417, "Bump GETH_REF in the fork to move deliberately" is slightly awkward phrasing. "move deliberately" → "move deliberately when needed" or "control when we update" would be clearer.

Summary

Severity Issue Location
Medium Future date in comment (likely typo) Both workflow files, line ~196/417
Low Comment inconsistency between files Both files
Low Missing fork maintenance documentation PR generally
Informational No commit verification link PR generally

The change is correct and safe to merge after fixing the date typo. The fork-based approach is a reasonable pragmatic solution to upstream instability.


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

@github-actions

github-actions Bot commented Sep 9, 2026

Copy link
Copy Markdown

🤖 Codex Code Review

No findings.

Open question / residual risk:

  • .github/workflows/pr-main_l1.yaml:420 and .github/workflows/daily_hive_report.yaml:199: switching from ethereum/hive to lambdaclass/hive-ci is reasonable for reproducibility, and pinning hive_version to a commit SHA keeps the runs deterministic. The tradeoff is CI now trusts a fork rather than upstream. If that is intentional, the only thing I’d want is a lightweight guardrail or documentation showing the fork is kept equivalent to upstream 7c4c99eb except for the GETH_REF pin.

This PR is workflow-only, so I don’t see any direct EVM, consensus, gas-accounting, trie, or RLP correctness risk in the client itself.


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

@github-actions

github-actions Bot commented Sep 9, 2026

Copy link
Copy Markdown

🤖 Claude Code Review

YAML is syntactically fine and both usages are consistent. This is a CI-only change (no Rust/consensus code touched), so most of the usual ethrex review lenses (EVM/gas/trie/RLP) don't apply. Findings below are scoped to the actual diff.

Review

Overall: Small, well-scoped CI fix. Root cause (upstream hive's devp2p simulator building go-ethereum from master rather than a pin) is correctly diagnosed, and the fix — pin GETH_REF in a fork's Dockerfile rather than working around symptoms — is the right level of abstraction. The commit message/comments clearly explain why, which is good practice for CI configs that otherwise look like arbitrary hash bumps months later.

Points worth checking before merge:

  1. Trust boundary shift: this moves both hive-consuming workflows from ethereum/hive (upstream) to lambdaclass/hive-ci (org-owned fork). Since it's the same org that owns ethrex, this is a reasonable trust boundary, but it's worth confirming who has push access to lambdaclass/hive-ci and that it's not more permissive than necessary — a compromised fork here runs arbitrary Docker builds in CI with DOCKERHUB_TOKEN/secrets access (.github/workflows/daily_hive_report.yaml:180-182, pr-main_l1.yaml).

  2. Commit pin correctness: both files pin hive_version: bda630136e76df758b1ab3f6879af3ae0b106813 (daily_hive_report.yaml:196, pr-main_l1.yaml:419). I wasn't able to verify against the GitHub API in this sandbox that this SHA actually exists in lambdaclass/hive-ci (tool call was blocked pending approval) — worth a manual double-check that the hash isn't a typo of the intended commit, since a bad SHA would just fail the workflow rather than silently do the wrong thing, but still worth 10 seconds to confirm.

  3. Using a commit SHA (not a branch/tag) for hive_version is the right call here — it's immutable and avoids the exact "floating ref changed under us" problem this PR is fixing. Good.

  4. Comment duplication: the same 6-line explanatory comment is duplicated verbatim in both workflow files. Unavoidable given GitHub Actions has no shared-config mechanism for this, so not a real issue — just flagging it's something that will need to be kept in sync manually if the pin is bumped again later (the PR description already anticipates this: "moving the devp2p suite forward becomes a deliberate bump of that arg").

No correctness, memory-safety, or consensus-logic concerns — this PR touches only workflow YAML.


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

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.

1 participant