Skip to content
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.

Sign up for GitHub

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

Jump to bottom

feat(kona-exex): wait for l2 genesis l1 block #14

Merged
merged 10 commits into from
Aug 24, 2024
Merged

feat(kona-exex): wait for l2 genesis l1 block #14

merged 10 commits into from
Aug 24, 2024

Conversation

merklefruit
Copy link
Collaborator

closes #4

refcell
refcell approved these changes Aug 23, 2024
View reviewed changes
bin/kona-exex/src/main.rs Outdated Show resolved Hide resolved
@merklefruit merklefruit force-pushed the nico/feat/wait-for-l2-genesis branch from 016ef35 to d83ef08 Compare August 23, 2024 17:27
@merklefruit merklefruit requested a review from refcell August 23, 2024 17:30
@merklefruit merklefruit requested a review from shekhirin August 23, 2024 20:53
shekhirin
shekhirin approved these changes Aug 23, 2024
View reviewed changes
Copy link

@shekhirin shekhirin left a comment •
edited
Loading

Choose a reason for hiding this comment

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

LGTM, nit + direction suggestion

bin/hera/src/hera.rs Outdated Show resolved Hide resolved
bin/hera/src/hera.rs
@@ -24,13 +25,13 @@ impl HeraCli {
pub fn run(self) -> Result<()> {
match self.subcmd {
HeraSubCmd::ExEx(cli) => cli.run(|builder, args| async move {
Copy link

Choose a reason for hiding this comment

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

it works for now, but generally it means that we will not be able to have more exexes (if there's any more planned for L1) in the same binary, because this binary has a name hera.

I think a better approach would be to have bin/reth and bin/hera binaries, and in bin/reth have a CLI argument --hera that will enable the ExEx. In bin/hera, it's just a standalone Hera binary. Both ExEx and standalone binary should re-use same code.

Copy link
Collaborator Author

Choose a reason for hiding this comment

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

got it, this was rebased on #17 by @refcell so tagging him here.
If separating the 2 binaries is more extensible long-term, then I also agree to split them

Copy link

Choose a reason for hiding this comment

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

just thinking that if we're going towards the future with L1 node, derivation pipeline, L2 client, proposer, and challenger in one binary, then it would be better to split the components from the beginning

Copy link
Member

Choose a reason for hiding this comment

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

let's make an issue for it

Copy link
Collaborator

Choose a reason for hiding this comment

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

I see what you're saying here, but couldn't bin/reth just import the standalone binary runner defined in bin/hera and run it in a separate thread if the --hera flag is supplied?

Then we could have the best of both worlds where the hera binary allows you to run as standalone or as an exex, and a separate reth binary allows you to pass in --hera?

Copy link
Collaborator Author

Choose a reason for hiding this comment

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

If in the future there will be more exexes, it would make more sense to spawn them from reth binary imo.
For the hera binary I don't have a strong opinion on if it should also be able to start as an exex or not.

Copy link
Collaborator

Choose a reason for hiding this comment

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

Happy to go with splitting them - for the sake of unblocking, let's do it :)

@Rjected Rjected changed the title feat(kona-exex): wait for l2 genesis l1 blcok feat(kona-exex): wait for l2 genesis l1 block Aug 24, 2024
@Rjected Rjected merged commit 1f1bba1 into ithacaxyz:main Aug 24, 2024
11 checks passed
@merklefruit merklefruit mentioned this pull request Aug 24, 2024
Merged
merklefruit added a commit that referenced this pull request Aug 25, 2024
@merklefruit
feat: split reth and hera binaries (#23)
7f271af 
Followup from #14 

Many ways to do this, but for now here's what we have:

- the `reth` binary runs the default reth CLI commands with an optional
`--hera` flag. If set, we also try to parse all `HeraArgsExt` flags and
install the ExEx on top of it. The idea is that in the future we may
have more ExExes that can be added in this way.
- `hera` binary is currently empty, but will contain the standalone
rollup node entrypoint.
- refactored `hera-exex` to its own crate, this will make it possible to
also run hera as exex from the `hera` binary as well.

Note: there is a compilation error due to
op-rs/kona#454, unrelated to this
pr.

Open to feedback
merklefruit added a commit to merklefruit/op-rs that referenced this pull request Aug 25, 2024
@merklefruit
feat: split reth and hera binaries (ithacaxyz#23)
a8ad017 
Followup from ithacaxyz#14

Many ways to do this, but for now here's what we have:

- the `reth` binary runs the default reth CLI commands with an optional
`--hera` flag. If set, we also try to parse all `HeraArgsExt` flags and
install the ExEx on top of it. The idea is that in the future we may
have more ExExes that can be added in this way.
- `hera` binary is currently empty, but will contain the standalone
rollup node entrypoint.
- refactored `hera-exex` to its own crate, this will make it possible to
also run hera as exex from the `hera` binary as well.

Note: there is a compilation error due to
op-rs/kona#454, unrelated to this
pr.

Open to feedback

feat(kona-exex): step 1: wait for l2 genesis block

chore: typo

chore: rm useless clippy rules

chore: small type coercion

fix: commit tip before breaking loop

feat(hera): rebased on bin/ location change

chore: rename kona -> hera in cli

feat: split reth and hera binaries

chore: cargo lock

fix: last compile err

chore: add crate lints

feat: renamed bin, renamed driver, addressed pr review

chore: fmt

feat: optionally load rollup config from file
merklefruit added a commit to merklefruit/op-rs that referenced this pull request Aug 25, 2024
@merklefruit
feat: split reth and hera binaries (ithacaxyz#23)
31a4c11 
Followup from ithacaxyz#14

Many ways to do this, but for now here's what we have:

- the `reth` binary runs the default reth CLI commands with an optional
`--hera` flag. If set, we also try to parse all `HeraArgsExt` flags and
install the ExEx on top of it. The idea is that in the future we may
have more ExExes that can be added in this way.
- `hera` binary is currently empty, but will contain the standalone
rollup node entrypoint.
- refactored `hera-exex` to its own crate, this will make it possible to
also run hera as exex from the `hera` binary as well.

Note: there is a compilation error due to
op-rs/kona#454, unrelated to this
pr.

Open to feedback

feat(kona-exex): step 1: wait for l2 genesis block

chore: typo

chore: rm useless clippy rules

chore: small type coercion

fix: commit tip before breaking loop

feat(hera): rebased on bin/ location change

chore: rename kona -> hera in cli

feat: split reth and hera binaries

chore: cargo lock

fix: last compile err

chore: add crate lints

feat: renamed bin, renamed driver, addressed pr review

chore: fmt

feat: optionally load rollup config from file
merklefruit added a commit to merklefruit/op-rs that referenced this pull request Aug 25, 2024
@merklefruit
feat: split reth and hera binaries (ithacaxyz#23)
1ad9268 
Followup from ithacaxyz#14

Many ways to do this, but for now here's what we have:

- the `reth` binary runs the default reth CLI commands with an optional
`--hera` flag. If set, we also try to parse all `HeraArgsExt` flags and
install the ExEx on top of it. The idea is that in the future we may
have more ExExes that can be added in this way.
- `hera` binary is currently empty, but will contain the standalone
rollup node entrypoint.
- refactored `hera-exex` to its own crate, this will make it possible to
also run hera as exex from the `hera` binary as well.

Note: there is a compilation error due to
op-rs/kona#454, unrelated to this
pr.

Open to feedback

feat(kona-exex): step 1: wait for l2 genesis block

chore: typo

chore: rm useless clippy rules

chore: small type coercion

fix: commit tip before breaking loop

feat(hera): rebased on bin/ location change

chore: rename kona -> hera in cli

feat: split reth and hera binaries

chore: cargo lock

fix: last compile err

chore: add crate lints

feat: renamed bin, renamed driver, addressed pr review

chore: fmt

feat: optionally load rollup config from file

chore: rebase

chore: rebase

chore: rebase
@shekhirin shekhirin added C-enhancement New feature or request A-hera Area: OP Stack Rollup node binary labels Aug 25, 2024
Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment
Reviewers

@Rjected Rjected Rjected left review comments

@shekhirin shekhirin shekhirin approved these changes

@refcell refcell Awaiting requested review from refcell

Assignees
No one assigned
Labels
A-hera Area: OP Stack Rollup node binary C-enhancement New feature or request
Projects
None yet
Milestone
No milestone
Development

Successfully merging this pull request may close these issues.

feat(kona-exex): Wait for L2 Genesis L1 Block
4 participants
@merklefruit @shekhirin @Rjected @refcell