Skip to content

feat(fork): reorder fork ordinality so CStar can activate without Boole - #1087

Closed
shane-moore wants to merge 2 commits into
sigp:epbsfrom
shane-moore:feat/fork-cstar-before-boole
Closed

shane-moore wants to merge 2 commits into
sigp:epbsfrom
shane-moore:feat/fork-cstar-before-boole

Conversation

@shane-moore

Copy link
Copy Markdown
Member

Problem, Evidence, and Context (Required)

SSV core has deprioritized the Boole fork indefinitely. CStar (the SSV-side Gloas rollout) now ships first, on a hard deadline tied to Ethereum's Gloas fork. Boole may still happen later, so its logic must be retained.

Anchor's fork machinery assumes the chronology Alan < Boole < CStar: Fork derives Ord from variant order, and every behavior gate is an ordinal comparison (active_fork(epoch) >= Fork::X). With that ordering, activating CStar makes every >= Fork::Boole gate evaluate true, silently switching on Boole behaviors (MinHash subnet topology, AggregatorCommittee wire roles, the QBFT epoch shift) that the network never agreed to. The scheduling layer already supports skipping Boole; the problem is purely the behavior layer.

Closes #1085.

Change Overview (Required)

Reorder the Fork enum to Alan, CStar, Boole so the derived Ord becomes Alan < CStar < Boole. Because every gate is an ordinal comparison, this single change flips them all at once: >= Fork::Boole behaviors stay dormant while Boole is unscheduled, and the new ordering forces any future Boole activation to land after CStar. CStar therefore lands directly on Alan-era behavior.

Only one site needed a behavior re-key: the two subnet-topology match arms that explicitly grouped Boole | CStar. They now read Fork::Alan | Fork::CStar => committee-based, Fork::Boole => MinHash. No other ordinal gate needed a code change.

Reading order:

  1. common/fork/src/fork.rs — the enum reorder (the conceptual core).
  2. subnet_service/src/{subnet,service}.rs — the one behavior re-key.
  3. common/fork/src/schedule.rs — schedule-validation tests (cstar-without-boole accepted; boole-before-cstar rejected). Note from_fork_configs needs no code change: it iterates a BTreeMap<Fork, _> in enum order, so the reorder makes the validation correct automatically.
  4. The test additions in message_validator and ssv_network_config, plus the built-in fork-schedule example comments.

What did not change: Boole logic is fully retained (explicitly not removed). Name-based parsing, Display, and gossip topic strings are order-free and unchanged. Pre-CStar (Alan-only) behavior is byte-for-byte identical. No wire/serialization format changes (Fork serde is name-based; it is never serialized by ordinal).

Risks, Trade-offs, and Mitigations (Required)

  • Blast radius: the reorder ripples to every crate that gates on Fork, which is why the diff spans 4 crates. It is one atomic change and cannot be split. Each affected gate was audited; only the subnet match arms required a code change.
  • ForkSchedule::new(fork) semantics shift: it seeds every fork <= fork, so new(Fork::Boole) now co-activates CStar. The validator_store committee-attestation harness uses a Boole schedule and consequently now exercises the real GloasBeaconVote path. This is the genuinely-correct behavior for a Boole network (which is necessarily post-CStar), so it is improved coverage, not a regression. Documented in the new doc comment.
  • Wire constants (provisional): CStar takes domain 0x00000002 (the increment after Alan), Boole takes 0x00000003 if later activated. Anchor is the first mover; go-ssv has not started Gloas-fork planning. These anchor-side choices should be memorialized for eventual go-ssv parity. No built-in network schedules either fork today (they carry both only as commented examples), so nothing live is affected.

Validation (Required)

  • make cargo-fmt-check and make lint (clippy -D warnings): clean.
  • Full cargo test on every affected crate passes: fork, subnet_service (55), ssv_network_config, message_validator (67), qbft_manager (31), anchor_validator_store (41), network (52).
  • New tests encode the acceptance criteria: alan@0 + cstar@N (no boole) validates and active_fork(N) == CStar; a boole-before-cstar schedule is rejected; CStar inherits committee-based subnet topology; legacy Aggregator/SyncCommittee wire roles are accepted and AggregatorCommittee is rejected under CStar; no epoch shift in round-robin proposer selection under CStar.
  • The rest of the workspace test run is green; the only failure is spec_tests, which requires Go-generated fixtures absent in a fresh checkout (environmental, unrelated to this diff).

Rollback (Required for behavior or runtime changes; optional otherwise)

Revert the commit. No config, data, or operational impact: no production network schedules Boole or CStar today, and the change is a pure in-memory ordering with no wire-format effect.

Additional Info / Next Steps (Optional)

The issue also proposed centralizing the scattered ordinal gates into ForkSchedule capability methods (min_hash_subnets_active(epoch), etc.). That hardening is intentionally deferred to a follow-up PR to keep this one behavior-only and independently reviewable. The reorder is fully correct without it.

SSV core deprioritized Boole indefinitely; CStar (the SSV-side Gloas
rollout) now ships first. Reorder the Fork enum to Alan, CStar, Boole so
the derived Ord becomes Alan < CStar < Boole. This single change flips
every ordinal gate at once: `>= Fork::Boole` behaviors (MinHash subnets,
AggregatorCommittee wire roles, QBFT epoch shift) stay dormant when CStar
is active, and a future Boole is forced to schedule after CStar.

Re-key the two subnet topology match arms that grouped Boole and CStar so
CStar inherits Alan committee-based topology. Boole logic is retained for
possible later activation. Update built-in fork-schedule example comments
to the new chronology (cstar domain 0x00000002, boole 0x00000003).

Closes sigp#1085
@shane-moore

Copy link
Copy Markdown
Member Author

@claude, please take a look

@claude-code-actions-sigp

claude-code-actions-sigp Bot commented Jun 14, 2026 •

Copy link
Copy Markdown

Claude finished @shane-moore's task in 4m 58s —— View job


PR Review — feat(fork): reorder fork ordinality so CStar can activate without Boole

Verdict: LGTM with one observation worth a small follow-up. The reorder is a tight, well-scoped change that delivers exactly what the PR description claims.


What I checked

  • Core reorder (anchor/common/fork/src/fork.rs)
    • Enum order is now Alan, CStar, Boole; derived Ord is Alan < CStar < Boole ✓
    • Fork::all(), name(), and FromStr are realigned, all order-free APIs (Display, topic_prefix, gossip topic format ssv.v2.<subnet> / /ssv/<net>/<fork>/<subnet>) remain name-driven and unchanged on the wire ✓
    • Doc comments accurately describe the new chronology and the "CStar inherits Alan subnet topology" semantics
  • Schedule (anchor/common/fork/src/schedule.rs)
    • from_fork_configs validates monotonic epochs (epoch <= prev) by iterating the BTreeMap<Fork, _> in enum order, so the reorder makes "boole-before-cstar" naturally invalid — confirmed by the new test_from_fork_configs_boole_before_cstar_rejected and test_from_fork_configs_cstar_without_boole
    • ForkSchedule::new(fork) doc comment correctly notes that new(Fork::Boole) now seeds CStar too
  • Behavior gates — swept every Fork:: ordinal site outside tests:
    • subnet_service/src/{subnet,service}.rs — match arms re-keyed (Alan | CStar => committee-based, Boole => MinHash) ✓
    • message_validator/src/lib.rs:845/854/863 — < Fork::Boole, >= Fork::Boole, < Fork::CStar gates remain semantically correct under the new ordering ✓
    • message_validator/src/consensus_message.rs:338 — round-robin epoch shift >= Fork::Boole correctly stays off under CStar; the new test_round_robin_proposer_cstar_fork pins this
    • validator_store/src/lib.rs:1507, 2798, 2854, 2964, 3100 and metadata_service.rs:614 — all >= Fork::CStar/>= Fork::Boole gates evaluate exactly as intended under the new chronology ✓
  • Built-in fork schedules — comment-only example reorder; all three networks (mainnet/holesky/hoodi) carry no scheduled CStar/Boole epochs, so production behavior is unchanged ✓
  • Test additions — well-targeted: test_from_operators_for_fork_cstar_inherits_committee_topology (subnet topology under CStar with a discriminating operator set), test_aggregator_committee_consensus_message_rejected_at_cstar (Boole-only role rejected under CStar), test_round_robin_proposer_cstar_fork (no epoch shift under CStar), and the partial_signature test now correctly uses Fork::Alan since generate_fork_schedule(Fork::Boole) would have inadvertently exercised PTC code under the new semantics
  • Wire-domain swap — only impacts test constants (CSTAR_DOMAIN = 0x00000002, BOOLE_DOMAIN = 0x00000003); no runtime config consumes these

Findings

1. Validation logic in from_fork_configs is technically order-tolerant but the invariant rests on enum order alone

anchor/common/fork/src/schedule.rs:158-169 walks a BTreeMap<Fork, _> and rejects when epoch <= prev. That works because BTreeMap<Fork, _> iterates in enum order, and you reordered the enum. The invariant the validation enforces ("forks ordered by activation epoch must match forks ordered by enum") is correct, but it is now silently load-bearing on the enum's derive(Ord) ordering. If someone reorders variants in the future (e.g., alphabetical sort, accidental rebase), from_fork_configs could silently accept a misordered schedule.

A cheap belt-and-suspenders: add an assertion that Fork::all() is strictly increasing, or change from_fork_configs to iterate Fork::all() directly instead of the BTreeMap order. Not a blocker — current behavior is correct — but worth flagging since you call this dependency out yourself in the from_fork_configs reading-order note.

2. ForkSchedule::new(Fork::Boole) now seeds CStar (intentional, but semantically broader than before)

anchor/validator_store/src/testing/common.rs:195-199 constructs the harness's schedule with Fork::Boole, which previously seeded only Alan+Boole and now seeds Alan+CStar+Boole. The PR description correctly identifies this and treats it as improved coverage (a Boole network is necessarily post-CStar). I agree, and the doc comment on new flags it. No action needed — just confirming the call sites I checked behave correctly under the change.

3. Wire-domain reassignment is anchor-side only

CSTAR_DOMAIN = 0x00000002 and BOOLE_DOMAIN = 0x00000003 are currently only used as test constants and example YAML comments. No live network has these scheduled. The PR description's note about memorializing this for go-ssv parity is correct; this should land in an issue/spec note before any network actually schedules CStar. Not a blocker for this PR.

4. Minor: variant doc comment for Fork::Boole could be sharper

The Boole variant's doc says "If it ever activates, it must do so after CStar; this variant ordering makes schedule validation reject a boole epoch at or before cstar's." Strictly the validation rejects epoch <= prev_fork.epoch with the special-case allowance for epoch == 0, so a boole epoch equal to cstar's is rejected only when it's > 0. Reads fine as-is.


Things that are NOT issues (sanity-checked)

  • Topic parsing (subnet_service/src/topic.rs) is name-based, untouched ✓
  • Network handshake / connection tests (network/src/handshake/mod.rs, peer_manager/connection.rs) use Fork::Boole as a "second fork" sentinel; semantics survive because the test schedules don't include CStar ✓
  • QBFT manager tests (qbft_manager/src/tests/gloas_dispatch_tests.rs) construct schedules with explicit Alan@0 + CStar@N configs that exercise the CStar boundary directly ✓
  • serde for Fork is name-based — no risk of ordinal-encoded persisted state changing meaning ✓

Validation I could not run

I'm in a sandboxed environment and was unable to run cargo check/make test/make lint directly. The PR description reports clean cargo-fmt-check, clippy -D warnings, and full passes on the affected crates (fork, subnet_service, ssv_network_config, message_validator, qbft_manager, anchor_validator_store, network); the code I read is consistent with that.


Summary

This is a small, surgical reorder with clear acceptance tests. The single-site behavior re-key (subnet topology) is correctly identified and tested, and every other ordinal gate I audited still expresses the right intent under the new chronology. The only real maintenance concern is that the schedule validation's correctness now silently depends on the enum's variant order matching fork chronology — worth a small hardening assertion in a follow-up but not a blocker for this PR.
| Branch: feat/fork-cstar-before-boole

@codecov-commenter

codecov-commenter commented Jun 14, 2026 •

Copy link
Copy Markdown

Codecov Report

❌ Patch coverage is 96.99248% with 4 lines in your changes missing coverage. Please review.
⚠️ Please upload report for BASE (epbs@a7eff24). Learn more about missing BASE report.

Files with missing lines Patch % Lines
anchor/subnet_service/src/service.rs 0.00% 3 Missing ⚠️
anchor/message_validator/src/consensus_message.rs 98.36% 1 Missing ⚠️
Additional details and impacted files
@@           Coverage Diff           @@
##             epbs    #1087   +/-   ##
=======================================
  Coverage        ?   63.80%           
=======================================
  Files           ?      162           
  Lines           ?    28113           
  Branches        ?        0           
=======================================
  Hits            ?    17938           
  Misses          ?    10175           
  Partials        ?        0           
Flag Coverage Δ
rust 63.80% <96.99%> (?)

Flags with carried forward coverage won't be shown. Click here to find out more.

☔ View full report in Codecov by Harness.
📢 Have feedback on the report? Share it here.

🚀 New features to boost your workflow:
  • ❄️ Test Analytics: Detect flaky tests, report on failures, and find test suite problems.

The fork machinery relies on Fork::all() being ordered identically to the
derived Ord: ForkSchedule::new's take_while and from_fork_configs's
BTreeMap iteration both assume it. The existing order tests pin only the
current three forks, so a future fork added in the wrong enum position
relative to all() could slip through. Assert the invariant directly.

Addresses review feedback on sigp#1085.
@shane-moore

shane-moore commented Jun 14, 2026 •

Copy link
Copy Markdown
Member Author

@claude, thanks for the review. Addressed Finding 1 in 49d7718.

Finding 1 (validation rests on enum order): Added test_all_forks_sorted_by_ord, which asserts Fork::all() is strictly increasing by Ord. One clarification: the current behavior is not actually silent today, test_fork_ordering (pins Alan < CStar < Boole) and test_all_forks (pins the all() contents) would both fail in CI on a reorder of the present three forks. The real residual gap is a future fork added in the wrong enum slot relative to all(), since those tests hardcode only today's forks. The new test closes that by asserting the general invariant that both ForkSchedule::new's take_while and from_fork_configs's BTreeMap iteration depend on. I left from_fork_configs iterating the BTreeMap rather than Fork::all(), since BTreeMap iteration order is Ord order by contract, so switching adds lookup complexity without removing the dependency.

Findings 2, 3, 4: no code change, agreed.

  • (2) new(Fork::Boole) seeding CStar is intended; the validator_store harness now exercises the real Gloas vote path, which is correct for a (necessarily post-CStar) Boole network.
  • (3) the wire-domain assignment (cstar 0x00000002, boole 0x00000003) is already memorialized in the issue's "Wire constants" section for go-ssv parity; nothing live schedules either fork.
  • (4) the Boole doc reads correctly for the normal case; the epoch == 0 corner is a pre-existing property of the genesis exception, not introduced here.

@claude-code-actions-sigp

claude-code-actions-sigp Bot commented Jun 14, 2026 •

Copy link
Copy Markdown

Claude finished @shane-moore's task in 59s —— View job


Follow-up review — 49d7718

Thanks for the quick turnaround. Read the new test against fork.rs and schedule.rs; all three points hold up.

  • Verify test_all_forks_sorted_by_ord pins the invariant
  • Confirm clarification re: existing test_fork_ordering / test_all_forks coverage
  • Confirm the BTreeMap iteration argument
  • Post final verdict

On the new test

anchor/common/fork/src/fork.rs:142-155 does what's claimed: Fork::all().windows(2).all(|pair| pair[0] < pair[1]) is the precise general invariant — strict monotonic increase under the derived Ord — that both ForkSchedule::new's take_while(|f| f <= &fork) and from_fork_configs's BTreeMap<Fork, _> iteration depend on. Any future fork inserted at the wrong enum slot relative to its position in all() will fail this test loudly rather than silently mis-seeding schedules. Good belt-and-suspenders.

On the "not actually silent today" clarification

You're right, I was imprecise. test_fork_ordering (fork.rs:104-107) pins Alan < CStar < Boole and test_all_forks (fork.rs:134-140) pins all() element-by-element, so reordering any of today's three variants would already fail CI in two places. The residual gap is purely the future-fork case where someone adds, say, a fourth variant between two existing ones in the enum but appends it to the end of all() — neither existing test catches that, and the new one does. Accurate framing.

On keeping from_fork_configs iterating the BTreeMap

Agreed. The function takes BTreeMap<Fork, (Epoch, DomainType)> as input (schedule.rs:142), and BTreeMap's iteration order is its key's Ord by contract — switching to iterate Fork::all() and lookup-by-key would add complexity without breaking the dependency on Ord, since the dependency is intrinsic to the map type. The new test catches the actual failure mode either way.

On findings 2 / 3 / 4

All three responses match what I'd expect — the new(Boole) semantics shift is genuinely improved coverage, the wire-domain assignment is already documented in the issue body, and the Boole variant doc is fine for the normal post-genesis case (the epoch == 0 exception is a pre-existing genesis property, untouched here).


Verdict

LGTM. The follow-up is a small, well-scoped hardening that makes the implicit Fork::all() ↔ derived-Ord invariant explicit, which was the only real residual concern from the first pass. Ready to merge from my side.
| Branch: feat/fork-cstar-before-boole

@shane-moore
shane-moore marked this pull request as draft June 15, 2026 15:50
@shane-moore

Copy link
Copy Markdown
Member Author

closing in favor of #1090

Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment

Labels

None yet

Projects

None yet

Development

Successfully merging this pull request may close these issues.

2 participants