feat!: fork schedule primitives and genesis/STF params split - #187
feat!: fork schedule primitives and genesis/STF params split#187prajwolrg wants to merge 12 commits into
Conversation
|
Commit: 40533ba
|
06d3317 to
d2d1a3f
Compare
|
@codex review |
![chatgpt-codex-connector[bot]](https://avatars.githubusercontent.com/in/1144995?s=60&v=4){kind=small}
There was a problem hiding this comment.
💡 Codex Review
Here are some automated review suggestions for this pull request.
Reviewed commit: d2d1a3f466
ℹ️ About Codex in GitHub
Your team has set up Codex to review pull requests in this repo. Reviews are triggered when you
- Open a pull request for review
- Mark a draft as ready
- Comment "@codex review".
If Codex has suggestions, it will comment; otherwise it will react with 👍.
Codex can also answer questions or update the PR. Try commenting "@codex address that feedback".
d2d1a3f to
23a773a
Compare
f39db77 to
3b5a0f0
Compare
EVM-style named forks with L1 activation heights. The schedule is not committed state: proving artifacts bake 0/MAX extremes (each only ever executes one side of an upgrade boundary) while the worker tracks the real activation height, so every executor agrees on the gate outcome at every height. Upgrade actions will carry forks as raw u16 ids, because the artifact that enacts a fork's activation predates the fork and cannot know it; ForkId maps the ids a binary knows and leaves the rest to be skipped by the consumer.
AsmParams was genesis-only, leaving no home for configuration of the per-block state transition. Split it by consumer: GenesisParams is consumed once to build the genesis anchor state, StfConfig (the base fork schedule) configures the state transition function for every block. Both sections are serde-flattened, so the params file stays a single flat object: the split is a property of the Rust types, not something operators need to spell out.
The trait declared the pipeline as an instance method and built genesis
through &self, so every executor had to thread a spec value and the
invocation order was only a doc-comment invariant ("MUST NOT change
behavior per stage").
The pipeline is now a type-level subprotocol list, making invocation
order a compile-time constant of the spec type: it cannot vary per
stage, per execution, or with runtime configuration. The spec also owns
its Params type and derives everything configuration-dependent from it
(genesis state, base STF params) through pure functions, so the worker
takes the single params value at its boundary and constructs genesis
behind the spec — a genesis built by a different spec can no longer be
adopted silently.
The handle exposes the genesis block so downstream services (the Moho
worker, the prover input builder) read the chain's genesis point from
the worker rather than re-deriving it from params.
Genesis hand-rolled the same three subprotocols the spec already declares, calling their state constructors directly (leaving Subprotocol::init dead framework surface) and hand-ordering the sections to match the ascending-ID layout the STF's section export asserts — an invariant nothing checked at genesis. Drive it through the same Stage traversal as every execution stage, locating each config in the params list by its InitConfig type (hence the new Any bound), so the pipeline and the genesis layout cannot drift apart.
…structs process_txs and process_msgs took loose ambient args (header verification state, verified aux data, L1 block ref). Bundle the read-only inputs of each phase into a method-aligned context struct (ProcessTxsCtx / ProcessMsgsCtx) passed as the final parameter: each field's purpose gets a documented home, and future context can grow without breaking every implementor's signature again. Capabilities (collector, relayer) stay as plain args; only read-only inputs live in ctx.
The spec type is stateless, so params reach the STF entry points as explicit arguments; the StrataAsmSpec struct carries them only across interfaces that thread a single spec value (the Moho runtime). Guest programs hardcode their params, making the verifying key commit to them; the native prover host bakes the same schedule its guest counterpart would; the worker passes the base params the spec derives from its params file. Each hook ctx gains the params, and pre-processing gets its own ctx carrying the target block height, not otherwise derivable in that phase: aux-data requests must be gateable on exactly the fork conditions that gate the processing which consumes them, and message handling in lockstep with the tx processing that produced the messages. Nothing consumes the params yet; the first fork gate will.
3b5a0f0 to
32bd1d3
Compare
bewakes
left a comment
bewakes
left a comment
There was a problem hiding this comment.
Looks good. Some comments, nits and queries.
Just a thought, not for this PR: I wonder if we could also encode the stages sequence in type level.
delbonis
left a comment
delbonis
left a comment
There was a problem hiding this comment.
There's a lot of very good stuff here.
I have a few comments about stuff "visible on the edges". In this vein more generally, I am not sure if it makes sense for all of this new stuff to be included in the common crate, it might make sense to break some of this out some more.
| /// Placeholder for the first protocol upgrade; renamed once that upgrade | ||
| /// is defined. | ||
| Fork1 = 0, | ||
| } |
There was a problem hiding this comment.
I'm not sure how to think about the naming here. V1 reads like linear versioning to me — v3 implies v2 and v1 already happened and were superseded. Forks feel more like named gates we branch on ("if fork X is active, do this; else that"), which fits fork-style names better; Fork1 is just the placeholder until the first real upgrade gets a proper name. It also makes me wonder whether the id should be a short fixed-width string instead of a number — something like bytes6, similar to how we treat magic bytes — so forks are named rather than numbered. Curious what you think.
| #[derive(Debug, Clone, PartialEq, Eq, Serialize, Deserialize)] | ||
| pub struct ForkSchedule { | ||
| /// Activation height of [`ForkId::Fork1`]. | ||
| pub fork1: u64, |
There was a problem hiding this comment.
Maybe we could use enum_map? Not sure if it's worth adding another dep. We could also just make it be an array that we do as u8 as usize on the enum instance.
I'm just seeing a lot of match fork { ... } in the impl blocks here.
There was a problem hiding this comment.
This hangs on the naming question in the V1 thread: if ForkId stays a small dense u8 enum, an array indexed by discriminant (or enum_map) would clean up these matches nicely; if we instead go with named ids (bytes6-style strings), the shape changes anyway. Will pick this up once the id representation settles.
| /// [`Stage`] that builds each subprotocol's genesis section from its config. | ||
| /// | ||
| /// Configs are located in the params' heterogeneous list by their type: each | ||
| /// subprotocol's `InitConfig` type appears in exactly one | ||
| /// [`SubprotocolInstance`] variant. | ||
| struct GenesisSectionStage<'p> { | ||
| params: &'p GenesisParams, | ||
| sections: Vec<SectionState>, | ||
| } | ||
|
|
There was a problem hiding this comment.
Genesis state? The "section" is just a container for the state.
There was a problem hiding this comment.
Done in 5c4cffd — renamed to GenesisStateStage and reworded the doc.
| let config: &dyn Any = match instance { | ||
| SubprotocolInstance::Admin(config) => config, | ||
| SubprotocolInstance::Bridge(config) => config, | ||
| SubprotocolInstance::Checkpoint(config) => config, | ||
| }; | ||
| config.downcast_ref::<S::InitConfig>() |
There was a problem hiding this comment.
This is kinda wacky, you might be able to reorganize this to just be a match based on type ID to avoid the upcast-and-immediate-downcast hack.
I would also think these guts might make sense to be split out into somewhere more standalone.
There was a problem hiding this comment.
I don't think matching on type ID can remove the downcast: this runs inside invoke_subprotocol<S: Subprotocol>, so even after matching S::ID we'd be holding a concrete &AdministrationInitConfig and still need Any::downcast_ref to get to &S::InitConfig — generic code can't make that conversion otherwise. The type-keyed search is sound since each InitConfig type appears in exactly one SubprotocolInstance variant (and an id() on the enum would drag the txs-crate ID constants into params, or duplicate them).
On splitting the guts out: I prototyped moving the lookup into the params crate (AsmGenesisParams::init_config::<C>() with the &dyn Any match as a private detail), but it only relocates the erasure — the mechanism ends up hidden behind a generic pub API away from its single call site, which read worse than the ~10 self-contained lines here. For reference, the before this PR hand-rolled construction avoided Any entirely by naming concrete types, at the cost of duplicating the spec's subprotocol list and hand-maintaining the ascending-ID section order the STF asserts. See that here. I don't have a strong opinion on this.
There was a problem hiding this comment.
Hmm I could see that indirection being awkward to do something about.
If anything else, could move this match and downcast stuff into an accessor on SubprotocolInstance just to keep the areas of concern more self-contained.
Review follow-ups on #187: fork activation heights move from raw u64 to L1Height so gates share the height domain the rest of the code uses (L1Height::MAX takes over as the never-active sentinel), and several names are clarified — activation_height_of / set_fork_activation (it is a plain setter, not a scheduler), PreProcessTxsCtx::block_height (target_height read like a destination), and AsmGenesisParams / AsmRuntimeParams to match OL-side params naming.
Completes the Asm* naming alignment from review: the executor-facing params now match AsmParams/AsmGenesisParams/AsmRuntimeParams and the existing AsmStf* convention (AsmStfProgram, AsmStfUpdate).
Yes, this'll eventually be broken out. The |
The stage's product is each subprotocol's genesis state; the section is just the envelope it gets packed into.
There was a problem hiding this comment.
💡 Codex Review
Here are some automated review suggestions for this pull request.
Reviewed commit: 5c4cffd41e
ℹ️ About Codex in GitHub
Your team has set up Codex to review pull requests in this repo. Reviews are triggered when you
- Open a pull request for review
- Mark a draft as ready
- Comment "@codex review".
If Codex has suggestions, it will comment; otherwise it will react with 👍.
Codex can also answer questions or update the PR. Try commenting "@codex address that feedback".
| let spec = self.spec.ok_or(WorkerError::MissingDependency("spec"))?; | ||
|
|
||
| let genesis_state = S::construct_genesis_state(¶ms); | ||
| let stf_params = S::stf_params(¶ms); |
There was a problem hiding this comment.
The threading here is clean, but I think there's one gap worth closing before Fork1 gates anything real: nothing binds this S::stf_params(¶ms) to the schedule the proving artifacts actually baked.
The guest hardcodes AsmStfParams::default() in guest-builder/sp1/guest-asm/src/main.rs:9, and the shipped examples already disagree with that: the Python factory defaults fork1_height = 0 while the artifacts bake "never".
Today this is harmless since nothing gates on Fork1, but the failure mode once it does is nasty and silent: the worker collects fork-gated aux requests in pre_process under {fork1: 0}, the guest processes under {fork1: MAX}, aux request/consumption lockstep breaks across the worker-to-guest boundary, and the worker advances state the proof chain can't attest.
There was a problem hiding this comment.
Yes, this is known. This laid groundwork for #188.
| /// ids carried in ASM VK upgrade actions. Actions carry the raw id rather | ||
| /// than this enum so that artifacts predating a fork can still parse and | ||
| /// enact the upgrade that activates it; consumers that act on the id (the | ||
| /// worker) map the ones they know via [`TryFrom`] and skip the rest. |
There was a problem hiding this comment.
I think this wording will steer PR#188 the wrong way.
The upgradability doc is explicit that a worker seeing an activation for a fork it doesn't know MUST halt, since it's running old software past an upgrade it can't execute (the exact silent-limp failure the doc's section 1 is about).
The primitive itself is fine, TryFrom returning Err supports either policy. But could we reword this to separate the two situations?
Parse-time tolerance ("an old binary must still parse an upgrade action naming a future fork")
vs act-time policy ("the worker must refuse to proceed past an activation it can't apply").
There was a problem hiding this comment.
Reworded in 59d3408 to split the two boundaries you named:
- Parse-time tolerance lives on the action/wire layer:
AsmStfVkUpdatecarries the rawu16, never mapping throughForkId, so an artifact predating a fork still parses and enacts the upgrade that activates it. - Act-time policy is the worker's: it maps via
TryFrom, and an unknown id is not skipped — it means the binary is running old software past an upgrade it can't execute, so it halts (exactly the section-1 silent-limp failure).
And yes, #188 already implements the halt correctly — discover_fork_activations returns WorkerError::UnsupportedForkActivation on an unknown id and leaves the block uncommitted ("the worker cannot safely follow the chain until it is restarted with an image that supports that fork"). This was only ever a doc bug; the primitive and the consumer were already right.
| pub enum ForkId { | ||
| /// Placeholder for the first protocol upgrade; renamed once that upgrade | ||
| /// is defined. | ||
| Fork1 = 0, |
There was a problem hiding this comment.
the doc-comment says discriminants are stable and key persisted records, but the derived serde uses the name ("fork1"), and the params file field is literally forks.fork1. So when Fork1 gets "renamed once that upgrade is defined" (per its own doc), every params file and anything persisted by name breaks.
Whichever way the Fork1 vs V1 vs bytes-name question lands, I think the conclusion is the same: the wire/persisted representation should be the stable numeric id, and the human-facing name should either be pinned with #[serde(rename)] or documented as renameable-with-migration.
There was a problem hiding this comment.
Pinning serde to u8 isn't needed, because nothing that must stay stable across a rename goes through ForkId's serde in the first place:
- Persisted fork-activation records key on the raw discriminant byte (storage encodes it directly, then
ForkId::try_from(byte)on read). - VK actions carry the numeric id.
So a persisted record or an in-flight action is already immune to a rename — the byte and the id don't change. That's the "stable numeric id on wire/persisted" you asked for, and it's already true via those encodings, not via serde.
Given that, the human-readable string serde is the better form — and renaming is the expected path, not a hazard: Fork1 is a placeholder, and renaming it to the real fork name once defined is a routine config migration of the human-facing surfaces (the variant's serde form and the ForkSchedule params field), which is your "documented renameable-with-migration" option.
| #[derive(Debug, Clone, PartialEq, Eq, Serialize, Deserialize)] | ||
| pub struct AsmRuntimeParams { | ||
| /// Base fork activation schedule. | ||
| pub forks: ForkSchedule, |
There was a problem hiding this comment.
No #[serde(default)] on forks means an existing deployment upgrading to this binary fails to boot until the operator adds the section.
I actually think loud is the right direction here but it's a breaking migration for every running node.
If it's intentional, could we call it out in the PR description / release notes?
There was a problem hiding this comment.
Intentional, and yes — this is a full breaking change, not a backwards-compatible add. I don't expect existing workers to already carry the forks section, and I'd rather they fail loudly at boot than silently start with a defaulted (empty) fork schedule and diverge from proving artifacts that bake in a real one.
That's part of why the whole thing lives on asm-upgradability rather than main: it's not a drop-in upgrade of a running node. When this branch merges it'll come with the params migration called out in the release notes.
| } | ||
|
|
||
| /// Sets the activation height of `fork`. | ||
| pub fn set_fork_activation(&mut self, fork: ForkId, height: L1Height) { |
There was a problem hiding this comment.
the overlay should build a fresh effective schedule per height rather than calling this on a long-lived base. A base mutated in place survives a sync rebase, which is exactly how a reorged-out activation could leak into the new branch.
Nothing un-calls set_fork_activation
There was a problem hiding this comment.
Good instinct on the primitive — set_fork_activation having no inverse is a leak hazard if a single long-lived schedule is mutated forward and never rebuilt. That's exactly why #188 doesn't consume it that way. The worker keeps two schedules:
base_forks— the configured base, treated as immutable.set_fork_activationis never called on it.fork_schedule— the disposable effective overlay actually executed against.
The two exist precisely so the effective schedule is always derivable, not accumulated: the authoritative record of activations is the persisted ForkActivation set, and effective_schedule(&base_forks, &activations) rebuilds the overlay as a fresh clone of the base with only the surviving records re-applied. Keeping the base pristine is what makes that rebuild possible — collapsing to one schedule is what would force the "un-call" you're pointing at.
So the fresh-per-branch rebuild you're asking for is wired at the one place staleness can arise — the rebase, before any block is re-processed (service.rs):
state.rollback_fork_activations(base_block.height())?;// rollback_fork_activations
self.context.prune_fork_activations_after(base_height)?;
let activations = self.context.list_fork_activations()?;
self.fork_schedule = effective_schedule(&self.base_forks, &activations);Prune persistence above the fork point, then rebuild the overlay from the immutable base + survivors — a reorged-out activation is dropped, not leaked. We rebuild per rebase rather than per height: within one branch the overlay only ever gains activations moving forward, and any branch switch already forces the full rebuild, so per-height would be redundant work. Covered by the rollback_prunes_and_recomputes unit test plus the reorg-rollback leg of the regtest integration test.
TL;DR: the primitive alone can't prevent the leak, agreed — the consumer does, by never mutating the base and rebuilding the effective schedule from persisted records on every rebase.
Model fork activation as `Option<L1Height>` so a disabled fork is `None` rather than the `L1Height::MAX` sentinel. The sentinel forced a degenerate boundary (a "never" fork was still active at MAX under the plain `>=` comparison) and required test fixtures to carry a magic max-int that also had to be capped at i64::MAX to survive the prover's signed-64-bit TOML. `None` removes both hazards and reads as intent on the wire (`"fork1": null`).
The ForkId doc conflated parse-time tolerance with act-time policy, reading as if the worker skips activations for fork ids it doesn't know. It doesn't: an unknown id means the worker is running old software past an upgrade it cannot execute, and it halts rather than limp along on stale rules. Separate the two boundaries so the doc can't be read as endorsing the silent-limp failure.
e99dd21 to
0e5ad8a
Compare
The reviewer read the "stable discriminant" line as contradicting the name-based serde. It doesn't: persisted fork-activation records key on the raw discriminant byte and VK actions carry the numeric id — neither uses serde — so both survive a variant rename. The variant name is the human-readable form (serde plus the mirrored ForkSchedule params field) and is meant to change: Fork1 is a placeholder, and renaming it once the upgrade is defined is a routine config migration that leaves persisted and wire data untouched. Spell that split out rather than implying the name is either the stable id or an unused label.
0e5ad8a to
0683d28
Compare
delbonis
left a comment
delbonis
left a comment
There was a problem hiding this comment.
This is in a pretty good state now, it's unfortunate there's still these hacky dyn hacks.
| impl TryFrom<u8> for ForkId { | ||
| type Error = u8; | ||
|
|
||
| fn try_from(value: u8) -> Result<Self, Self::Error> { | ||
| match value { | ||
| 0 => Ok(ForkId::Fork1), | ||
| invalid => Err(invalid), | ||
| } | ||
| } | ||
| } | ||
|
|
||
| impl TryFrom<u16> for ForkId { | ||
| type Error = u16; | ||
|
|
||
| fn try_from(value: u16) -> Result<Self, Self::Error> { | ||
| u8::try_from(value) | ||
| .ok() | ||
| .and_then(|v| ForkId::try_from(v).ok()) | ||
| .ok_or(value) | ||
| } | ||
| } |
There was a problem hiding this comment.
There's an int-enum crate that can help with these.
There was a problem hiding this comment.
Yeah I'm going to raise again that I dislike centering the logic around the forks as opposed to versions, as centering the changes makes it so that the "version 0" condition before any fork is trigger has to be kinda a special default case.
| /// Invokes the stage with each subprotocol, in the declared order. | ||
| fn call_subprotocols(stage: &mut impl Stage) { | ||
| Self::Subprotocols::for_each(stage); | ||
| } |
There was a problem hiding this comment.
Does this make sense to be a trait fn at this time? What kind of scenario would we want to override this?
| fn pre_process_txs( | ||
| &mut self, | ||
| txs: &[TxInputRef<'_>], | ||
| collector: &mut AuxRequestCollector, | ||
| ctx: &PreProcessTxsCtx<'_>, | ||
| ); |
There was a problem hiding this comment.
I was thinking about this a while ago, is it possible to feature flag this so it doesn't get compiled in sp1 builds (where it would never be used)?
There was a problem hiding this comment.
yes, we can do that.
| let config: &dyn Any = match instance { | ||
| SubprotocolInstance::Admin(config) => config, | ||
| SubprotocolInstance::Bridge(config) => config, | ||
| SubprotocolInstance::Checkpoint(config) => config, | ||
| }; | ||
| config.downcast_ref::<S::InitConfig>() |
There was a problem hiding this comment.
Hmm I could see that indirection being awkward to do something about.
If anything else, could move this match and downcast stuff into an accessor on SubprotocolInstance just to keep the areas of concern more self-contained.
|
[codex assisted] Non-blocking thoughts after reading through this PR. The overall shape makes sense to me: split genesis/runtime params, thread STF params through the execution paths, and make subprotocol order harder to drift. A few things I’d keep an eye on before real fork-gated behavior lands:
|
|
Superseded by #202, which carries the same groundwork with reduced scope and clean history: the spec/params primitives (renamed per discussion here — ForkId → SpecId with V1, ForkSchedule → SpecActivation) and the genesis/STF params split, with all review follow-ups from this PR incorporated. The AsmSpec type-level pipeline, stage-driven genesis, hook ctx structs, and the params threading built on them are extracted to a follow-up PR (preserved on the upgradability-asmspec-pipeline branch). |
4 participants
Description
Groundwork for fork-based ASM upgradeability (#183), split out so the machinery PR is pure behavior. This PR defines the types and threads them everywhere, but nothing consumes them yet — no fork gates, no activation discovery, so behavior is unchanged end to end.
What it contains:
crates/common/src/fork.rs):ForkId,ForkSchedule,StfParams,ForkActivation. The schedule is not committed state — each proving artifact bakes its own copy (the VK commits to it) and the worker gets its own via params. Upgrade actions (in the stacked PR) carry forks as raw u16 ids, because the artifact that enacts a fork's activation predates the fork and cannot know it;ForkIdmaps the ids a binary knows and consumers skip the rest. The single variant is a placeholder (Fork1); the PR stacked on top renames it when the first real fork gates something.AsmParamsbecomesGenesisParams(consumed once, to build the genesis anchor state) +StfConfig(the base fork schedule), flattened in the serialized form so the params file stays a single flat object.PreProcessTxsCtxalso carries the target block height, which pre-processing previously could not see — a future gate must decide identically in both phases to keep aux request/consumption in lockstep).AsmSpecreduced to a type-level pipeline declaration: subprotocol invocation order becomes a compile-time constant, and the spec owns itsParamstype — the worker takes the single params value at its boundary and derives the genesis state and base STF params behind the spec, with genesis sections driven by the same subprotocol list every execution stage traverses.Type of Change
Notes to Reviewers
Suggested read order:
crates/common/src/fork.rs(types and the 0/MAX baking invariant),crates/params/src/{genesis,stf}.rs(the split), then commit-by-commit for the spec refactor and the threading — each commit compiles clippy-clean on its own.Breaking surfaces: the params JSON gains a required
forkssection, and theAsmSpec/Subprotocoltrait signatures change (process_txs/pre_process_txstake params/ctx). No behavioral change: every executor still runs identical logic, since nothing gates on the schedule yet.Checklist
Related Issues
Base of #183 (fork-based ASM upgradeability), which is stacked on this branch.