feat!: spec-versioned ASM VK upgrades with worker-side activation discovery - #204
feat!: spec-versioned ASM VK upgrades with worker-side activation discovery#204prajwolrg wants to merge 8 commits into
Conversation
|
Commit: 7ecb9f6
|
f775990 to
aed3dc0
Compare
![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: aed3dc0ff3
ℹ️ 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".
| /// enacted on the abandoned branch must not leak into re-processing; | ||
| /// any still on the new branch are re-discovered as its blocks re-apply. | ||
| pub(crate) fn rollback_spec_activations(&mut self, base_height: L1Height) -> WorkerResult<()> { | ||
| self.context.prune_spec_activations_after(base_height)?; |
There was a problem hiding this comment.
When a reorg returns to a previously processed block at the same height, sync_to_block initially ignores that stored target and later rebases from it when a child arrives, but this height-only prune keeps activations enacted by the abandoned block at that height. For example, after processing block B at height 150, switching to A at height 150 with an upgrade, and then switching back to B, rebasing from B retains A's activation and predicate, so block 151 is assigned the wrong effective schedule. Store the enacting block commitment with each activation and discard records that do not belong to the selected base's ancestry rather than deciding solely by height.
Useful? React with 👍 / 👎.
There was a problem hiding this comment.
💡 Codex Review
Here are some automated review suggestions for this pull request.
Reviewed commit: 4a645f9788
ℹ️ 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".
| // drop them before re-processing so the effective schedule cannot leak | ||
| // a rolled-back activation into the first re-processed block. A linear | ||
| // extension has nothing above the base, making this a no-op. | ||
| state.rollback_spec_activations(base_block.height())?; |
There was a problem hiding this comment.
When a reorg is attempted after an upgrade, this permanently deletes the old branch's activation records before any replacement block is committed. If fetching, transitioning, or storing a pending block then fails, process_input shuts the worker down while the old anchor states remain durable; on restart, AsmWorkerServiceState::new adopts the highest stored old anchor but reconstructs its schedule without the deleted activation. This leaves a committed anchor paired with the wrong effective schedule and can cause the next upgrade to be assigned the wrong successor, so pruning must be committed atomically with the replacement branch or deferred until that branch is durable.
Useful? React with 👍 / 👎.
| "anchor": asdict(self.anchor), | ||
| "subprotocols": self.subprotocols, | ||
| "spec_activation": {"v1": self.v1_height}, | ||
| "spec_activation": {"v0": self.v0_height, "v1": self.v1_height}, |
There was a problem hiding this comment.
When v1_height is left as the documented None, this emits "v1": null, but SpecScheduleRepr now maps values directly to L1Height and its tests explicitly reject that representation; an unscheduled version must be absent. Consequently callers selecting the advertised unscheduled configuration—and existing disabled-version parameter files produced in the former format—fail during ASM parameter deserialization instead of starting the runner.
Useful? React with 👍 / 👎.
| #[derive(Debug, Clone, PartialEq, Eq, Serialize, Deserialize)] | ||
| #[serde(try_from = "SpecScheduleRepr", into = "SpecScheduleRepr")] | ||
| pub struct SpecSchedule { | ||
| /// Activation height of each scheduled post-genesis version, indexed by | ||
| /// predecessor count: `upgrades[i]` activates discriminant `i + 1`. | ||
| upgrades: Vec<L1Height>, | ||
| } | ||
|
|
||
| /// A schedule update that would violate [`SpecSchedule`]'s invariants. | ||
| #[derive(Debug, Clone, Copy, PartialEq, Eq, Error)] |
There was a problem hiding this comment.
Derive order, and in other places.
There was a problem hiding this comment.
The codebase is split between the order of Debug, Clone vs Clone, Debug. I think this is also true if we put PartialEq before Eq.
Are you suggesting to put all the derive in the alphabetical order except for the non-standard derive?
There was a problem hiding this comment.
The order that I've always used is:
- std derives: in decreasing order of how "essential" the trait is, so
Copy, Clone, Debug, Hash, and then the rest - non-std derives: in alphabetical order, so
Deserialize, Serializealthough this can get confusing when different crates end up getting interleaved and I don't have a good solution
but uhh I guess it doesn't really matter
delbonis
left a comment
delbonis
left a comment
There was a problem hiding this comment.
I ran into the GitHub bug where it submits your PR reviews without you asking, so this comment also applies to the review that GitHub submitted for me a couple mins ago:
There's a couple of design questions I raised here, overall it's not bad.
There was a problem hiding this comment.
💡 Codex Review
Here are some automated review suggestions for this pull request.
Reviewed commit: 92b108866e
ℹ️ 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".
| # gapped schedule is rejected). Dynamic activations of later versions | ||
| # come from enacted ASM VK upgrades. | ||
| v0_height: int = 0 | ||
| v1_height: int | None = None |
There was a problem hiding this comment.
Although this field now defaults to None, build_asm_params still defaults its v1_height argument to 0 and passes it explicitly, while basic_env.py calls that helper without an override. Those generated environments therefore start with V1 already scheduled at height 0; because activation discovery selects the successor of the newest scheduled version, the first AsmStfUpdate is treated as unsupported V2 and shuts down the worker instead of activating V1. Align the helper's default with this new genesis schedule.
Useful? React with 👍 / 👎.
Fork awareness is meant to live at the spec level: the version table in strata-asm-spec and the schedule the worker and guest feed it. Subprotocols stay fork-unaware — upgrades ship as whole versioned implementations — so nothing below params ever needs SpecId, and keeping it in common wrongly advertised it as subprotocol vocabulary. Inside params, identity and schedule split: spec_id.rs keeps the SpecId enum, runtime.rs owns SpecActivation and the STF/runtime params. SpecActivation was common's last serde user and params' only common import, so common drops serde and params drops strata-asm-common.
SpecId previously started at V1 with genesis rules modeled as "no spec
version active". Give the genesis rules their own version instead: V0
is always active since genesis and V1 becomes the first on-chain
upgrade. This lets every ASM VK update name the version its artifact
implements — including updates that will ship while still on the
genesis spec — and gives gates a uniform is_active check.
The schedule (SpecSchedule) encodes those rules structurally instead of
one Option field per version: V0 is implicit and later versions only
ever activate in succession, so a gapped or v0-disabled schedule is
unrepresentable and a new SpecId variant is a one-line change — there
is no per-version code to keep in sync. The serialized form keeps the
per-version map ({"v0": 0, "v1": null}) and re-validates the
invariants on load, so a hand-edited schedule fails fast.
The raw identity narrows to u16 only: the discriminant is repr(u16),
the u8 conversions are gone, and the primitive conversions are derived
(num_enum, erroring with the raw id) so they cannot go stale when a
variant is added.
Discovered spec activations must survive worker restarts: the enacted update is dropped from the admin queue at enactment, so without a durable record a restarted worker could not reconstruct which versions the chain has activated (or the VK each boundary switched to). Keyed by (enacting_height, version) with a big-endian height prefix so put is an idempotent overwrite under crash-replay and prune_after (the reorg path) is a range scan. The predicate is the value, borsh-encoded. The version is stored and returned as its raw u16 id: mapping it to a known spec version is act-time worker logic, so the store stays opaque to it (the worker owns the typed record).
New SpecActivationStore concern on the worker context, wired to the sled store in the runner and the test context. The SpecActivationRecord it persists lives here rather than in strata-asm-common: a discovered activation is worker bookkeeping, not protocol surface (keeping it out of common also keeps strata-predicate out of the guest's common deps). The store speaks raw parts — the context impls map the raw version id back through SpecId on load, which is where act-time mapping belongs. Record-before-anchor-commit and prune-above-base are contracts the discovery logic (next commit) relies on for crash and reorg safety; they are documented on the trait so every backend upholds them.
The worker maintains an effective spec schedule: the base from runtime params (a new required builder input) overlaid with every activation discovered from enacted AsmStfUpdate logs. The log carries no version — future versions must be enactable by artifacts that predate them, so the wire cannot require knowing them. Instead each enacted upgrade activates the successor of the newest scheduled version (SpecSchedule::schedule_successor). A successor this binary has no variant for halts the worker without committing the block: it is running old software past an upgrade it cannot execute, and limping along on stale rules would silently diverge from the chain. Nothing is persisted for the rejected block, so a restart with a newer image simply retries it. Replaying persisted activations onto the base schedule is validated the same way: a record the schedule cannot fit (e.g. params downgraded below activations already committed) fails startup instead of silently producing a gapped schedule. Crash/reorg safety is carried entirely by ordering: the activation record is written before the enacting block's anchor commit (a committed anchor can never lack its activation), and every sync rebase prunes activations above the base before re-processing, so a reorged-out enactment cannot leak into the new branch — its blocks re-discover whatever survives. The activation height is derived, not stored: always the block after the enacting one. Nothing consumes the schedule yet — threading it into the STF hooks lands with the AsmSpec pipeline follow-up.
Worker-level integration coverage for the full on-chain choreography: the admin enacts an ASM VK upgrade, the worker derives V1 as the successor of genesis-active V0 and records the activation at H+1 carrying the enacted VK, and the enacting block's manifest holds the log it was discovered from. The reorg path abandons the submission block and asserts the activation rolls back, then re-enacts one height later once the resurrected admin txs re-mine. The unstake-style feature gating that motivated the old fork-based variant of this test is deliberately absent: nothing consumes the schedule yet.
…chedule entries
Both mutation paths now reject an activation height that would order a
version against its neighbor the wrong way, so an inverted schedule ("v2
active while v1 is not") is unrepresentable; equal heights stay legal
because several upgrades enacted in one block activate at the same height.
The worker maps an unknown successor to UnsupportedSpecActivation as
before and an out-of-order activation to InconsistentSpecSchedule.
The serde form now lists exactly the scheduled versions: an explicit
"v1": null was indistinguishable from omitting the key, so absence becomes
the only spelling of "unscheduled" and null is rejected at decode.
Public fields let callers construct or mutate a record without going through from_raw, bypassing the SpecId validation it does. Make the fields private and add a new() constructor plus accessors, so construction always goes through one of the two typed entry points.
92b1088 to
ce8eb2f
Compare
2 participants
Description
Reworks #188 on top of the spec-versioning model that #202 merged into
asm-upgradability, superseding the fork-based design with a clean history. The old PR's unstake gate is dropped entirely — nothing consumes the activation schedule yet (threading it into the STF hooks belongs to theAsmSpecpipeline follow-up) — so this PR carries only the upgrade machinery itself: how a spec activation gets on the chain, and how the worker learns about it.SpecIdis reworked so the genesis rules are a version of their own:V0is always active since genesis andV1is the first on-chain upgrade. The schedule (SpecSchedule) encodes those rules structurally rather than as oneOptionfield per version: it stores the activation heights of a contiguous run of successors withV0implicit, so a gapped or v0-disabled schedule is unrepresentable and adding aSpecIdvariant is a one-line change — the primitive conversions are derived (num_enum) and every schedule operation works off the discriminant, so there is no per-version code to forget. The raw identity narrows to u16 only, and the serialized form keeps the per-version map ({"v0": 0, "v1": null}) while re-validating the invariants on load, so a hand-edited params file fails fast.Nothing on the wire names the version: the ASM VK upgrade action (and the
AsmStfUpdatelog it emits on enactment) carries only the new predicate, because the upgrade that activates a version may be enacted by an artifact that predates it. The chain is still the single authority on which version an upgrade activates — the activating version is defined as the successor of the newest scheduled version (SpecSchedule::schedule_successor), chaining through multiple upgrades in one block. Consequence worth flagging: every enactment activates the next version, so a pure VK rotation on the current spec is deliberately not expressible for now.The worker discovers activations from those logs against its effective schedule (base from runtime params, a new required builder input, overlaid with prior discoveries). The discovered-activation record type (
SpecActivationRecord) lives in the worker crate, notstrata-asm-common: it is worker bookkeeping rather than protocol surface (this also keepsstrata-predicateout of common, and thus out of the guest's common deps), and the sled store persists raw parts with the raw version id mapped back throughSpecIdonly at the worker boundary. A successor this binary has no variant for halts the worker without committing the block — it is running old software past an upgrade it cannot execute. Replaying persisted activations onto the base schedule is validated the same way: a record the schedule cannot fit (e.g. params downgraded below activations already committed) fails startup instead of silently running on a gapped schedule.The subtle part is crash/reorg safety, carried entirely by ordering: the activation record is persisted (sled, keyed by enacting height and version) before the enacting block's anchor commit, so a committed anchor can never lack its activation; every sync rebase prunes activations above the base before re-processing, so a reorged-out enactment cannot leak into the new branch — its blocks re-discover whatever survives. The activation height is derived, not stored: always the block after the enacting one.
Type of Change
Notes to Reviewers
Review entry point:
SpecScheduleand its invariants incrates/params/src/runtime.rs, thendiscover_spec_activations/apply_spec_activationsincrates/worker/src/state.rs.Checklist
Related Issues
Part of fork-based ASM upgradeability (#183). Supersedes #188.
🤖 Generated with Claude Code