fix: replace retired --enclave.* reth flags in local-network binaries - #454
Open
samlaf wants to merge 1 commit into
Open
fix: replace retired --enclave.* reth flags in local-network binaries#454samlaf wants to merge 1 commit into
samlaf wants to merge 1 commit into
Conversation
seismic-reth removed the --enclave.* CLI namespace in SeismicSystems/seismic-reth#444, and clap fails hard on unknown args — every binary that spawns reth (testnet and the e2e scenario bins) stopped being able to boot nodes against newer builds. Replace --enclave.mock-server with --seismic.purpose-keys-source built-in (local networks run no key custodian) and drop --enclave.endpoint-port, inert since SeismicSystems/seismic-reth#435. The flag block was copy-pasted across all nine binaries, which is why the drift went unnoticed: nothing in CI runs these binaries, so a retired flag only surfaces when someone runs one by hand. Centralize the spawn configuration in summit_types::reth::reth_spawner. The spawner also honors an SRETH_BIN env var (falling back to `reth` on PATH), so CI and local runs can pin a specific seismic-reth build, e.g. a prebuilt release. Move the testnet launcher to its own crate (testnet/bin, package summit-testnet). testnet builds with default features, but the reth spawn machinery is e2e-gated in summit-types; a separate crate can enable summit-types/e2e without that feature leaking into production builds of summit — the gate is what previously forced testnet onto a parallel alloy-node-bindings copy of the spawn flags. `cargo run --bin testnet` works unchanged, and the alloy-node-bindings dependency is gone. Replace the blanket testnet/ gitignore with a testnet/.gitignore covering the actual runtime state (node*/* except the checked-in *.pem keys, plus stores/): the blanket rule hid the directory from search tools and would have silently ignored the new crate. The spawner now creates the reth datadir with create_dir_all, so the .gitkeep placeholders are no longer needed. ## Suggestion These binaries should ideally run in CI against a pinned seismic-reth version, so flag drift like this surfaces when it lands instead of at the next manual run. The pieces exist: seismic-reth releases publish prebuilt binaries, and SRETH_BIN points the spawner at a fetched one. A scheduled job running `testnet --only-reth` (or a full e2e scenario) against that pin would close the gap.
This file contains hidden or bidirectional Unicode text that may be interpreted or compiled differently than what appears below. To review, open the file in an editor that reveals hidden Unicode characters.
Learn more about bidirectional Unicode characters
Sign up for free
to join this conversation on GitHub.
Already have an account?
Sign in to comment
Add this suggestion to a batch that can be applied as a single commit.This suggestion is invalid because no changes were made to the code.Suggestions cannot be applied while the pull request is closed.Suggestions cannot be applied while viewing a subset of changes.Only one suggestion per line can be applied in a batch.Add this suggestion to a batch that can be applied as a single commit.Applying suggestions on deleted lines is not supported.You must change the existing code in this line in order to create a valid suggestion.Outdated suggestions cannot be applied.This suggestion has been applied or marked resolved.Suggestions cannot be applied from pending reviews.Suggestions cannot be applied on multi-line comments.Suggestions cannot be applied while the pull request is queued to merge.Suggestion cannot be applied right now. Please check back later.
Human Summary
fix now broken reth enclave flags in all bins. Refactored so that their copy-pasted code uses a shared function launcher. Only issue is that that launcher lives under the e2e flag under summit/types (personally think they should live in a seperate test utils crate but havent thought through whether there are any issues with this and didnt want to create too much churn), so to use it the
testnetbin now had to be started with the e2e flag (which it previously didn't) like all other bins. So moved the testnet bin under testnet/bin.This is just my suggestion... but feel free to do the simple "fix all bins directly without the shared util". Think the util is nice though because I added the SRETH_BIN env var which allows running a bin pointing at an exact reth binary in case we want to test a new build without installing it for example.
Also haven't run the testnet binary myself.. think it might still need a fix to the
eth_genesis_hash(see this suggestion from asymmetric researcher). Could be done here or in a follow-up PR.LLM Summary
seismic-reth removed the --enclave.* CLI namespace in SeismicSystems/seismic-reth#444, and clap fails hard on unknown args — every binary that spawns reth (testnet and the e2e scenario bins) stopped being able to boot nodes against newer builds. Replace --enclave.mock-server with
--seismic.purpose-keys-source built-in (local networks run no key custodian) and drop --enclave.endpoint-port, inert since SeismicSystems/seismic-reth#435.
The flag block was copy-pasted across all nine binaries, which is why the drift went unnoticed: nothing in CI runs these binaries, so a retired flag only surfaces when someone runs one by hand. Centralize the spawn configuration in summit_types::reth::reth_spawner. The spawner also honors an SRETH_BIN env var (falling back to
rethon PATH), so CI and local runs can pin a specific seismic-reth build, e.g. a prebuilt release.Move the testnet launcher to its own crate (testnet/bin, package summit-testnet). testnet builds with default features, but the reth spawn machinery is e2e-gated in summit-types; a separate crate can enable summit-types/e2e without that feature leaking into production builds of summit — the gate is what previously forced testnet onto a parallel alloy-node-bindings copy of the spawn flags.
cargo run --bin testnetworks unchanged, and the alloy-node-bindings dependency is gone.Replace the blanket testnet/ gitignore with a testnet/.gitignore covering the actual runtime state (node*/* except the checked-in *.pem keys, plus stores/): the blanket rule hid the directory from search tools and would have silently ignored the new crate. The spawner now creates the reth datadir with create_dir_all, so the .gitkeep placeholders are no longer needed.
Suggestion
These binaries should ideally run in CI against a pinned seismic-reth version, so flag drift like this surfaces when it lands instead of at the next manual run. The pieces exist: seismic-reth releases publish prebuilt binaries, and SRETH_BIN points the spawner at a fetched one. A scheduled job running
testnet --only-reth(or a full e2e scenario) against that pin would close the gap.