Skip to content
Closed
Show file tree
Hide file tree
Changes from all commits
Commits
File filter

Filter by extension

Filter by extension


Conversations
Failed to load comments.
Loading
Jump to
Jump to file
Failed to load files.
Loading
Diff view
Diff view
2 changes: 2 additions & 0 deletions Cargo.lock

Some generated files are not rendered by default. Learn more about how customized files appear on GitHub.

10 changes: 5 additions & 5 deletions bin/asm-runner/src/bootstrap.rs
Original file line number Diff line number Diff line change
Expand Up @@ -74,16 +74,16 @@ pub(crate) async fn bootstrap(
// on a runtime worker thread here, so wrap the build in `block_in_place` to allow blocking;
// the worker's own loop runs on a dedicated sync thread where blocking is already fine.
let asm_worker = task::block_in_place(|| {
AsmWorkerBuilder::new()
AsmWorkerBuilder::<_, StrataAsmSpec>::new()
.with_context(worker_context)
.with_asm_spec(StrataAsmSpec)
.with_params(params.clone())
.with_params(params)
.launch(&executor)
})?;

let asm_worker = Arc::new(asm_worker);

// 6. Finish orchestrator wiring if it was configured.
let genesis_block = asm_worker.genesis_block();
let proof_rpc_deps = if let Some((orch_config, proof_db, moho_state_db, backend)) = orch_prep {
let rpc_deps = AsmProofRpcDeps {
proof_db: proof_db.clone(),
Expand Down Expand Up @@ -118,7 +118,7 @@ pub(crate) async fn bootstrap(
let moho_worker = MohoWorkerBuilder::new()
.with_context(moho_context)
.with_subscription(asm_worker.subscribe_blocks())
.with_genesis_block(params.anchor.block)
.with_genesis_block(genesis_block)
.with_asm_predicate(asm_predicate.clone())
.launch(&executor)
.await?;
Expand All @@ -133,7 +133,7 @@ pub(crate) async fn bootstrap(
aux_db.clone(),
bitcoin_client.clone(),
);
let input_builder = InputBuilder::new(params.anchor.block, asm_predicate, moho_predicate);
let input_builder = InputBuilder::new(genesis_block, asm_predicate, moho_predicate);

// Drive the prover from the *Moho* worker's commit stream, not the ASM
// worker's: the Moho worker emits a block only after it has persisted
Expand Down
1 change: 1 addition & 0 deletions crates/common/Cargo.toml
Original file line number Diff line number Diff line change
Expand Up @@ -32,5 +32,6 @@ zkaleido-logging.workspace = true
ssz_codegen.workspace = true

[dev-dependencies]
serde_json.workspace = true
strata-identifiers.workspace = true
strata-test-utils-arb.workspace = true
221 changes: 221 additions & 0 deletions crates/common/src/fork.rs

Copy link
Copy Markdown
Contributor

Choose a reason for hiding this comment

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

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.

Original file line number Diff line number Diff line change
@@ -0,0 +1,221 @@
//! Fork-based upgradeability primitives.
//!
//! The ASM upgrades EVM-style: STF logic is gated on named forks with L1
//! activation heights, so a single binary can execute both sides of an upgrade
//! boundary. The schedule is *not* part of committed state — it is baked into
//! each proving artifact (guest ELF / native host) and supplied to the worker
//! via params, with the invariant that every artifact agrees on the gate's
//! outcome at every height it executes (see `AsmStfParams`).

use serde::{Deserialize, Serialize};
use strata_identifiers::L1Height;

/// Identifies a named fork.
///
/// One variant per protocol upgrade, in activation order. The numeric
/// discriminant is the stable identity: it keys persisted fork-activation
/// records (stored as the raw discriminant byte) and is the raw id carried in
/// ASM VK upgrade actions. Neither path goes through this type's serde, so a
/// persisted record or an in-flight action is unaffected by a variant being
/// renamed — the byte and the id stay the same.
///
/// The variant name is the human-readable form: it is this type's serde
/// representation (snake_case) and is mirrored by the [`ForkSchedule`] params
/// field. Names are meant to change — `Fork1` is a placeholder for an upgrade
/// not yet defined — so renaming one once defined is a routine migration of the
/// human-facing config, leaving persisted records and wire actions untouched.
///
/// The id crosses two boundaries with opposite tolerances:
///
/// - Parse-time: ASM VK upgrade actions carry the raw id, not this enum, so an artifact predating a
/// fork can still parse and enact the upgrade that activates it — the wire format never requires
/// knowing the fork.
/// - Act-time: a consumer that must *apply* the fork's rules (the worker) maps the id via
/// [`TryFrom`]. An id it does not know is not skipped: it means the worker is running old
/// software past an upgrade it cannot execute, so it MUST halt rather than silently limp along on
/// stale rules.
#[derive(Debug, Clone, Copy, PartialEq, Eq, Hash, Serialize, Deserialize)]
#[serde(rename_all = "snake_case")]
#[repr(u8)]
Comment thread
prajwolrg marked this conversation as resolved.
pub enum ForkId {
/// Placeholder for the first protocol upgrade; renamed once that upgrade is
/// defined. The rename is a migration of the human-readable name (this
/// variant's serde form and the [`ForkSchedule`] params field); persisted
/// records and wire actions key on the numeric discriminant and are
/// unaffected.
Fork1 = 0,

Copy link
Copy Markdown
Collaborator

Choose a reason for hiding this comment

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

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.

@prajwolrg prajwolrg Jul 17, 2026

Copy link
Copy Markdown
Collaborator Author

Choose a reason for hiding this comment

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

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.

}

impl From<ForkId> for u8 {
fn from(fork: ForkId) -> Self {
fork as u8
}
}

impl From<ForkId> for u16 {
fn from(fork: ForkId) -> Self {
fork as u16
}
}

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)
}
}
Comment on lines +61 to +81

Copy link
Copy Markdown
Contributor

Choose a reason for hiding this comment

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

There's an int-enum crate that can help with these.


/// Activation heights for every named fork.
///
/// A fork with activation height `Some(n)` is active at L1 height `h` iff
/// `h >= n` — so `Some(0)` means active since genesis. `None` means disabled.
/// Proving artifacts bake one of the two extremes (`Some(0)` or `None` — an
/// artifact only ever executes one side of an upgrade boundary), while the
/// worker tracks the real activation height discovered from the ASM VK
/// upgrade log.
#[derive(Debug, Clone, PartialEq, Eq, Serialize, Deserialize)]
pub struct ForkSchedule {
/// Activation height of [`ForkId::Fork1`], or `None` if disabled.
pub fork1: Option<L1Height>,
}

impl ForkSchedule {
/// Schedule with every fork disabled (no activation height).
pub const fn all_disabled() -> Self {
Self { fork1: None }
}

/// Returns the activation height of `fork`, or `None` if disabled.
pub fn activation_height_of(&self, fork: ForkId) -> Option<L1Height> {
match fork {
ForkId::Fork1 => self.fork1,
}
}

/// Returns whether `fork` is active at L1 `height`.
pub fn is_active(&self, fork: ForkId, height: L1Height) -> bool {
self.activation_height_of(fork)
.is_some_and(|activation| height >= activation)
}

/// Sets the activation height of `fork`.
pub fn set_fork_activation(&mut self, fork: ForkId, height: L1Height) {

Copy link
Copy Markdown
Collaborator

Choose a reason for hiding this comment

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

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

Copy link
Copy Markdown
Collaborator Author

Choose a reason for hiding this comment

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

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_activation is 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.

match fork {
ForkId::Fork1 => self.fork1 = Some(height),
}
}
Comment thread
prajwolrg marked this conversation as resolved.
}

impl Default for ForkSchedule {
fn default() -> Self {
Self::all_disabled()
}
}

/// Protocol-rule parameters consumed by the STF, as opposed to the genesis
/// params that only seed the initial state.
///
/// Every executor of the STF carries its own copy: guest programs hardcode it
/// (so the proof's verifying key commits to it), native proving hosts bake it
/// into their closure, and the worker derives an effective copy from params
/// plus discovered fork activations.
///
/// `Default` inherits [`ForkSchedule`]'s default: everything disabled.
#[derive(Debug, Clone, Default, PartialEq, Eq, Serialize, Deserialize)]
pub struct AsmStfParams {
/// Fork activation schedule.
pub forks: ForkSchedule,
}

#[cfg(test)]
mod tests {
use super::*;

#[test]
fn is_active_boundaries() {
let sched = ForkSchedule { fork1: Some(100) };
assert!(!sched.is_active(ForkId::Fork1, 99));
assert!(sched.is_active(ForkId::Fork1, 100));
assert!(sched.is_active(ForkId::Fork1, 101));
}

#[test]
fn zero_means_always_active() {
let sched = ForkSchedule { fork1: Some(0) };
assert!(sched.is_active(ForkId::Fork1, 0));
assert!(sched.is_active(ForkId::Fork1, L1Height::MAX));
}

#[test]
fn none_means_never_active() {
let sched = ForkSchedule::all_disabled();
assert_eq!(sched.activation_height_of(ForkId::Fork1), None);
assert!(!sched.is_active(ForkId::Fork1, 0));
assert!(!sched.is_active(ForkId::Fork1, L1Height::MAX));
}

#[test]
fn set_fork_activation_overrides() {
let mut sched = ForkSchedule::all_disabled();
sched.set_fork_activation(ForkId::Fork1, 42);
assert_eq!(sched.activation_height_of(ForkId::Fork1), Some(42));
assert!(sched.is_active(ForkId::Fork1, 42));
assert!(!sched.is_active(ForkId::Fork1, 41));
}

#[test]
fn serde_roundtrip() {
let params = AsmStfParams {
forks: ForkSchedule { fork1: Some(7) },
};
let json = serde_json::to_string(&params).unwrap();
assert_eq!(json, r#"{"forks":{"fork1":7}}"#);
let back: AsmStfParams = serde_json::from_str(&json).unwrap();
assert_eq!(back, params);

let disabled = AsmStfParams {
forks: ForkSchedule::all_disabled(),
};
let json = serde_json::to_string(&disabled).unwrap();
assert_eq!(json, r#"{"forks":{"fork1":null}}"#);
let back: AsmStfParams = serde_json::from_str(&json).unwrap();
assert_eq!(back, disabled);
}

/// Serde is the human-readable form (the variant name). The stable numeric
/// identity used for persistence and the wire is exercised by
/// [`fork_id_u16_roundtrip`] instead.
#[test]
fn fork_id_serde_is_the_variant_name() {
assert_eq!(serde_json::to_string(&ForkId::Fork1).unwrap(), r#""fork1""#);
assert_eq!(
serde_json::from_str::<ForkId>(r#""fork1""#).unwrap(),
ForkId::Fork1
);
assert!(serde_json::from_str::<ForkId>(r#""nope""#).is_err());
}

/// Raw fork ids on the wire round-trip through the enum; unknown ids
/// surface as errors instead of misparsing.
#[test]
fn fork_id_u16_roundtrip() {
assert_eq!(u16::from(ForkId::Fork1), 0);
assert_eq!(ForkId::try_from(0u16).unwrap(), ForkId::Fork1);
assert_eq!(ForkId::try_from(0xFFFFu16), Err(0xFFFF));
}
}
2 changes: 2 additions & 0 deletions crates/common/src/lib.rs
Original file line number Diff line number Diff line change
Expand Up @@ -4,6 +4,7 @@
mod aux;
mod constants;
mod errors;
mod fork;
mod log;
mod manifest;
mod mmr;
Expand All @@ -28,6 +29,7 @@ mod ssz_generated {
pub use aux::*;
pub use constants::*;
pub use errors::*;
pub use fork::*;
pub use log::*;
pub use manifest::*;
pub use mmr::*;
Expand Down
Loading
Loading