Skip to content

ci(l1): pin Amsterdam EELS branch and make CI job required#6495

Merged
ilitteri merged 4 commits intomainfrom
ci/pin-amsterdam-hive
Apr 21, 2026
Merged

ci(l1): pin Amsterdam EELS branch and make CI job required#6495
ilitteri merged 4 commits intomainfrom
ci/pin-amsterdam-hive

Conversation

@edg-l
Copy link
Copy Markdown
Contributor

@edg-l edg-l commented Apr 16, 2026

Summary

  • Pin the EELS reference branch to commit 5c6e20ab (devnets/bal/3 @ 2026-04-14) instead of tracking the branch directly, which is a moving target that can break CI unexpectedly.
  • Remove optional: true from the Amsterdam Consume Engine job so it becomes a required check in PR CI.
  • Apply the same pin to the daily hive report workflow.

Test plan

  • Amsterdam Consume Engine hive job passes in CI with the pinned commit

@edg-l edg-l requested a review from a team as a code owner April 16, 2026 14:06
@github-actions github-actions Bot added the L1 Ethereum client label Apr 16, 2026
@github-actions
Copy link
Copy Markdown

🤖 Kimi Code Review

Review Summary
Good practice change to pin floating dependencies for CI stability. One minor documentation error to fix.

Issues Found

1. Incorrect date in comment (.github/workflows/pr-main_l1.yaml, line 238)

# Source: ethereum/execution-specs devnets/bal/3 @ 2026-04-14

The date 2026-04-14 appears to be in the future. If this is a typo, correct it to the actual date (likely 2025-04-14 or similar).

2. Significant CI policy change
Removing optional: true (line 241) makes Amsterdam fork tests mandatory for PR merges. While pinning the EELS commit mitigates upstream breakage, ensure the team is ready to block merges on these tests if the Amsterdam spec is still evolving. The logic is sound (pinning enables requiring), but verify this aligns with current development priorities.

Positive Aspects

  • Security/Reproducibility: Pinning to commit hash 5c6e20abf3586f52d9e58393203ca07f2d0151fe instead of branch devnets/bal/3 prevents supply chain attacks and ensures deterministic CI runs.
  • Consistency: Both workflow files updated to use the same pinned commit.
  • Documentation: Updated comments explain why the pin exists and when to bump it.

Recommendation: Fix the date typo, then merge.


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

@greptile-apps
Copy link
Copy Markdown

greptile-apps Bot commented Apr 16, 2026

Greptile Summary

This PR pins the EELS dependency in both the PR and daily Hive CI workflows to a specific commit hash (5c6e20abf3586f52d9e58393203ca07f2d0151fe from devnets/bal/3 @ 2026-04-14) instead of the mutable branch name, and promotes the Amsterdam Consume Engine job from optional to required.

The commit hash is consistent across both workflow files, and the buildarg splitting mechanism in pr-main_l1.yaml (line 285: for arg in $SIM_BUILDARG) correctly handles the space-separated fixtures=... branch=... format unchanged from before.

Confidence Score: 5/5

Safe to merge — straightforward CI hardening with no logic changes.

Both changes are mechanical: swapping a mutable branch name for a pinned commit hash (consistently across both workflow files) and removing optional: true. No application logic is touched, the buildarg parsing mechanism is unchanged, and the commit hash is identical in both files.

No files require special attention.

Important Files Changed

Filename Overview
.github/workflows/pr-main_l1.yaml Pins EELS branch to commit hash 5c6e20abf3586f52d9e58393203ca07f2d0151fe and removes optional: true to make Amsterdam Consume Engine a required CI check.
.github/workflows/daily_hive_report.yaml Applies the same EELS commit pin (5c6e20abf3586f52d9e58393203ca07f2d0151fe) to the daily Hive report workflow for consistency.

Flowchart

%%{init: {'theme': 'neutral'}}%%
flowchart TD
    A[PR / Daily Schedule Triggered] --> B{Workflow}
    B --> C[pr-main_l1.yaml]
    B --> D[daily_hive_report.yaml]

    C --> E[Matrix: Consume Engine Amsterdam]
    E --> F["buildarg: fixtures=bal@v5.6.1\nbranch=5c6e20abf..."]
    F --> G[Hive splits on whitespace\nfor arg in SIM_BUILDARG]
    G --> H["--sim.buildarg fixtures=...\n--sim.buildarg branch=5c6e20abf..."]
    H --> I[Run Hive Simulation]
    I --> J[Required check ✅\noptional: true REMOVED]

    D --> K{SIM_LIMIT contains fork_Amsterdam?}
    K -- Yes --> L["branch=5c6e20abf... pinned commit"]
    K -- No --> M["branch=forks/osaka"]
    L --> N[Run daily Hive report]
    M --> N
Loading

Reviews (1): Last reviewed commit: "ci(l1): add original branch comment to d..." | Re-trigger Greptile

@github-actions
Copy link
Copy Markdown

🤖 Codex Code Review

  1. Medium: Removing optional: true from Consume Engine Amsterdam changes it from informational to merge-blocking. run-hive is configured with continue-on-error: ${{ matrix.optional || false }} at pr-main_l1.yaml, so the change at pr-main_l1.yaml through pr-main_l1.yaml is a behavioral gate change, not just a pin. The new comment only explains pinning to avoid upstream CI breakage; it does not justify promoting an Amsterdam test suite that was previously marked unstable into a required check. If the intent is only to freeze upstream inputs, optional: true should stay until the suite is explicitly ready to gate PRs.

No other correctness, security, or performance concerns stood out in this workflow-only diff.


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 Apr 16, 2026
Base automatically changed from fix/eip7778-block-gas-overflow to main April 16, 2026 19:29
edg-l added 4 commits April 17, 2026 17:08
Pin the EELS reference branch to a known-good commit instead of
tracking devnets/bal/3 which is a moving target. Remove optional: true
so Amsterdam tests are required in PR CI.
Move fixtures URL and EELS commit pin to
.github/config/hive/amsterdam.yaml so both pr-main_l1 and
daily_hive_report read from the same source of truth.
@iovoid iovoid force-pushed the ci/pin-amsterdam-hive branch from 222d098 to 0165cb5 Compare April 17, 2026 20:08
@ilitteri ilitteri added this pull request to the merge queue Apr 21, 2026
Merged via the queue into main with commit c405213 Apr 21, 2026
67 checks passed
@ilitteri ilitteri deleted the ci/pin-amsterdam-hive branch April 21, 2026 15:09
@github-project-automation github-project-automation Bot moved this from In Review to Done in ethrex_l1 Apr 21, 2026
avilagaston9 added a commit that referenced this pull request Apr 27, 2026
Brings in main commits since the prior merge: #6516 EIP-8025 compliance
(Electra-aligned ExecutionRequests typed container in NewPayloadRequest,
MAX_CONSOLIDATION_REQUESTS_PER_PAYLOAD corrected from 1 to 2,
to_encoded_requests() helper for EIP-7685 bytes, removal of
ExecutionPayloadHeader/NewPayloadRequestHeader, new byte-oriented
execution_program entrypoint that decodes the wire format internally and
returns valid: false instead of erroring on post-decode failures), #6463
BAL withdrawal reverse check (DB->BAL direction so a malicious builder
can't omit a withdrawal recipient from the BAL), #6505 Kademlia k-bucket
revert (PeerTableServer::spawn no longer takes a node_id), plus snap-sync
observability + dashboards (#6470), pivot-update crash fix (#6475),
weighted peer selection (#6428), txpool_contentFrom/txpool_inspect RPC
(#6446), block-by-block exec fallback (#6464), Amsterdam EELS branch pin
(#6495), and rollup store SQLite v9->v10 migration (#6514).

Conflict resolutions:
- crates/common/types/stateless_ssz.rs: this branch had already moved
  the EIP-8025 SSZ types out of crates/common/types/eip8025_ssz.rs into
  stateless_ssz.rs and tucked the native-rollup containers below them.
  Kept that layout, applied #6516's content updates to the EIP-8025
  section (renamed spec-limit constants, ExecutionRequests typed
  container with to_encoded_requests, dropped header types and their
  tests), pulled in the EncodedRequests import, and kept both the new
  test_execution_requests_to_encoded_bytes and the branch's stateless
  round-trip tests.
- crates/guest-program/src/l1/program.rs: adopted #6516's new
  execution_program(bytes: &[u8], crypto) API with the internal
  decode_eip8025 call, the validate_eip8025_execution helper, and the
  decode-failure test. Rewrote all `eip-8025` feature gates as
  `experimental-devnet` and all `eip8025_ssz::` paths as
  `stateless_ssz::` to match this branch's renames.
- crates/guest-program/bin/{sp1,risc0,zisk,openvm}/src/main.rs: applied
  #6516's simplification (drop decode_eip8025 import, pass &input
  straight to execution_program) under the experimental-devnet feature
  gate. Also flipped the rkyv::rancor::Error import gate from the old
  `eip-8025` name to `experimental-devnet` so the non-devnet build still
  has the import it needs.
- crates/prover/src/backend/exec.rs: kept #6516's updated comment ("raw
  input bytes" instead of "(NewPayloadRequest, ExecutionWitness)") under
  the experimental-devnet feature gate.

Auto-merged regions checked: crates/vm/backends/levm/mod.rs picked up
all of #6463's Part B (DB->BAL) reverse check intact, and
cmd/ethrex/l2/initializers.rs picked up #6505's PeerTableServer::spawn
signature change. Verified cargo fmt --all clean, cargo check --workspace
clean, cargo check --workspace --tests clean, and cargo check -p
ethrex-guest-program --features experimental-devnet --tests clean.
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.

4 participants