Skip to content

Conversation

@ilitteri
Copy link
Contributor

Motivation

ethrex-l2-common needs for the SP1 and RISC0 guests' ELF to exist to compile. guest_program generates said ELFs in its build.rs, but as ethrex-l2-common, it's compiled before the build.rs runs, causing a compilation error.

Description

  • Embed SP1 guest program in the guest_program crate.
  • Move logic that needs said ELFs to the sequencer (who isn't a dependency from the guest_program crate).
  • Remove sp1 and risc0 features from ethrex-l2-common as they are not needed anymore.

ethrex-l2-common doesn't need sp1 and risc0 features anymore
@ilitteri ilitteri self-assigned this Oct 29, 2025
@ilitteri ilitteri requested a review from a team as a code owner October 29, 2025 21:12
Copilot AI review requested due to automatic review settings October 29, 2025 21:12
@github-actions github-actions bot added the L2 Rollup client label Oct 29, 2025
Copy link
Contributor

Copilot AI left a comment

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

Pull Request Overview

This PR refactors the handling of ZKVM program ELF files by centralizing their definitions in the guest_program module and removing the redundant logic from the common crate. The main goal is to simplify the codebase by eliminating duplicate ELF loading code and streamlining feature flag dependencies.

Key changes:

  • Centralized ZKVM program ELF constants (ZKVM_SP1_PROGRAM_ELF, ZKVM_RISC0_PROGRAM_ELF) in the guest_program module
  • Removed the aligned_vm_program_code() method from ProverType in the common crate and replaced it with direct ELF access in the sequencer
  • Updated feature flag dependencies to propagate through the workspace more cleanly

Reviewed Changes

Copilot reviewed 7 out of 7 changed files in this pull request and generated 2 comments.

Show a summary per file
File Description
crates/l2/sequencer/l1_proof_sender.rs Updated to directly access ZKVM ELF constants and added feature flag validation when sending proofs to Aligned Layer
crates/l2/prover/src/guest_program/src/lib.rs Added ZKVM_SP1_PROGRAM_ELF constant definition with conditional compilation based on sp1 feature flag
crates/l2/prover/src/backend/sp1.rs Updated to use centralized ZKVM_SP1_PROGRAM_ELF constant instead of local definition
crates/l2/common/src/prover.rs Removed aligned_vm_program_code() method and associated constants, added documentation clarifying vk_path usage
crates/l2/common/Cargo.toml Removed sp1 and risc0 feature flags from common crate
crates/l2/Cargo.toml Added sp1 and risc0 feature flags that propagate to guest_program
cmd/ethrex/Cargo.toml Removed ethrex-l2-common feature dependencies for sp1 and risc0

💡 Add Copilot custom instructions for smarter, more guided reviews. Learn how to get started.

@github-actions
Copy link

github-actions bot commented Oct 29, 2025

Lines of code report

Total lines added: 47
Total lines removed: 55
Total lines changed: 102

Detailed view
+------------------------------------------------------+-------+------+
| File                                                 | Lines | Diff |
+------------------------------------------------------+-------+------+
| ethrex/cmd/ethrex/l2/deployer.rs                     | 1140  | +17  |
+------------------------------------------------------+-------+------+
| ethrex/crates/l2/common/src/prover.rs                | 128   | -50  |
+------------------------------------------------------+-------+------+
| ethrex/crates/l2/prover/src/backend/sp1.rs           | 126   | -5   |
+------------------------------------------------------+-------+------+
| ethrex/crates/l2/prover/src/guest_program/src/lib.rs | 12    | +8   |
+------------------------------------------------------+-------+------+
| ethrex/crates/l2/sequencer/l1_proof_sender.rs        | 434   | +22  |
+------------------------------------------------------+-------+------+

/// proof verification. Aligned Layer uses a different vk in SP1's case.
///
/// There's no need to have the following verifying keys here since
/// they are used by the deployer and they are not used by the sequencer.
Copy link
Contributor

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

Can't we delete this so?

Copy link
Contributor Author

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

I think the comment is misleading, that functionality is actually used by the deployer. Unless you meant to remove that from there and use it directly in the deployer bin.

Copy link
Contributor

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

Yes, I the comment is confusing too. Maybe something like ...?

This function can be moved since it's used by the deployer but not the sequencer

Copy link
Contributor Author

@ilitteri ilitteri Oct 30, 2025

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

I'll move the functionality there then.

@ManuelBilbao ManuelBilbao added this pull request to the merge queue Oct 30, 2025
Merged via the queue into main with commit fb15bd9 Oct 30, 2025
42 checks passed
@ManuelBilbao ManuelBilbao deleted the fix_l2_compilation branch October 30, 2025 20:39
Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment

Labels

L2 Rollup client

Projects

Status: Done

Development

Successfully merging this pull request may close these issues.

5 participants