-
Couldn't load subscription status.
- Fork 113
refactor(l1,l2): separate binaries #4829
New issue
Have a question about this project? Sign up for a free GitHub account to open an issue and contact its maintainers and the community.
By clicking “Sign up for GitHub”, you agree to our terms of service and privacy statement. We’ll occasionally send you account related emails.
Already on GitHub? Sign in to your account
Conversation
- Separated make lint into lint-l1 and lint-l2 for it to cover both scenarios. - Made it so that make tests runs with l2 (not necesary for the test itself, but the benches are a dev dependency and break without that feature flag)
Lines of code reportTotal lines added: Detailed view |
b2e5f9f to
f2ebcf4
Compare
There was a problem hiding this 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 ethrex to separate L1 and L2 functionality into distinct binaries through feature flags. The main goal is to create optimized builds where the L1 client excludes unnecessary L2 dependencies, while L2 builds include additional database options.
Key changes include:
- Introduction of
l2andl2-sqlfeature flags to conditionally compile L2 functionality - Separation of build processes to create ethrex:main (L1) and ethrex:main-l2 (L2) Docker images
- Updated CI/CD workflows to handle both L1 and L2 builds independently
Reviewed Changes
Copilot reviewed 33 out of 33 changed files in this pull request and generated 4 comments.
Show a summary per file
| File | Description |
|---|---|
| cmd/ethrex/Cargo.toml | Restructured dependencies to be conditional on l2 feature flags |
| crates/networking/p2p/rlpx/message.rs | Added feature guards around L2 message handling |
| crates/networking/p2p/rlpx/connection/server.rs | Moved imports and added conditional compilation for L2 features |
| cmd/ethrex/build.rs | Extracted L2-specific build logic into separate module |
| .github/workflows/ | Updated CI workflows to build separate L1 and L2 artifacts |
| Makefile | Split linting commands to handle L1 and L2 separately |
Comments suppressed due to low confidence (1)
crates/l2/prover/src/backend/sp1.rs:1
- Missing space in error message. Should be 'Failed to read bytecode file'.
use std::{fmt::Debug, sync::OnceLock};
Tip: Customize your code reviews with copilot-instructions.md. Create the file or learn how to get started.
| datatest_stable::harness!(blockchain_runner, TEST_FOLDER, r".*"); | ||
|
|
||
| #[cfg(any(all(feature = "sp1", feature = "stateless"),))] | ||
| #[cfg(all(feature = "sp1", feature = "stateless"))] |
Copilot
AI
Oct 14, 2025
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
The cfg attribute syntax is incorrect. The trailing comma inside the any() macro was removed but the outer structure should use any() for mutually exclusive features, not all().
| endpoint | ||
| .join("/twirp/") | ||
| .expect("Failed to parse moongate server url") | ||
| .to_string(), | ||
| .as_ref(), |
Copilot
AI
Oct 14, 2025
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
The method call chain is incorrect. join() returns a Result<Url, url::ParseError>, but as_ref() is being called on the Url result. The original code passed &endpoint to preserve the reference, but now endpoint is moved and as_ref() is called on the wrong type.
| let mut batch_gas_used = 0_u64; | ||
|
|
||
| info!("Preparing state diff from block {first_block_of_batch}"); | ||
| info!("Preparing state diff from block {first_block_of_batch}, {batch_number}"); |
Copilot
AI
Oct 14, 2025
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
[nitpick] The log message format is unclear. Consider using named parameters or a clearer format like 'Preparing state diff from block {} for batch {}'.
| info!("Preparing state diff from block {first_block_of_batch}, {batch_number}"); | |
| info!("Preparing state diff from block {} for batch {}", first_block_of_batch, batch_number); |
| where | ||
| S: Unpin + Stream<Item = Result<Message, PeerConnectionError>>, | ||
| { | ||
| // This allow is because in l2 we mut the capabilities |
Copilot
AI
Oct 14, 2025
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
Grammar error in comment. Should be 'we mutate the capabilities' instead of 'we mut the capabilities'.
| // This allow is because in l2 we mut the capabilities | |
| // This allow is because in l2 we mutate the capabilities |
| tracker: TaskTracker, | ||
| blockchain: Arc<Blockchain>, | ||
| based_context: Option<P2PBasedContext>, | ||
| #[cfg(feature = "l2")] based_context: Option<P2PBasedContext>, |
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
This is not optimal and we should fix this in another PR.
| { | ||
| // This allow is because in l2 we mut the capabilities | ||
| // to include the l2 cap | ||
| #[allow(unused_mut)] |
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
| #[allow(unused_mut)] | |
| #[expect(unused_mut)] |
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
Block it until next version release
Motivation
We aim to reintroduce the L2 feature flag making so that ethrex has 3 available binaries, the L1 execution client, the L2 client and the L2+GPU client. This is done for the following advantages:
Description