boole-convergence polish: gating docs, startup fork validation, spectest bound (#2968 items 4, 6, 7) - #2973
boole-convergence polish: gating docs, startup fork validation, spectest bound (#2968 items 4, 6, 7)#2973momosh-ssv wants to merge 12 commits into
Conversation
The BooleFork() gate on the RoleAggregator/RoleSyncCommitteeContribution runners is a deliberate share-add-time snapshot: the resulting cross-population asymmetry is bounded to the Boole subsequent window (SlotsPerEpoch + booleSubsequentWindowLateSlots; 34 slots on mainnet) and affects only already-decided pre-fork aggregator stragglers. These runners retire wholesale with the Alan topics. Item 4 of #2968 (decision: document, don't slot-gate).
A garbage forks.boole epoch silently overflowed FirstSlotAtEpoch, and a custom network scheduling Boole without setting next_domain_type "activates" the fork with zero observable domain change (unmarshal defaults the next domain to the current one). Add Network.Validate() - hard error on the overflow and zero-slots-per-epoch cases, operator-actionable warning on the same-domain case - called right after the Network is assembled. Item 6 of #2968.
The remap's magic upper bound (79) was disconnected from the real v1.2.2 maximum (72). Derive it from TimeoutInstanceErrorCode - the last pre-Boole-era spec error code - plus the documented +2 shift, so a spec bump that adds codes can't silently widen the remap. Item 7 of #2968.
Greptile SummaryThis PR adds Boole fork configuration validation during node startup and tightens Alan spectest compatibility behavior.
Confidence Score: 5/5The PR appears safe to merge, with no concrete blocking or independently actionable non-blocking issues identified. The startup validation runs after the consensus client has initialized its beacon configuration, correctly handles the unscheduled-fork sentinel, and the remaining changes are scoped documentation and spectest compatibility refinements.
|
| Filename | Overview |
|---|---|
| cli/operator/node.go | Validates the fully assembled network configuration before continuing node startup and logs operator-actionable warnings. |
| networkconfig/network.go | Adds scheduled-Boole validation for zero slots per epoch, slot-conversion overflow, and unchanged domain types. |
| networkconfig/network_test.go | Covers validation errors, warnings, valid scheduled configurations, and the unscheduled sentinel. |
| operator/validator/controller.go | Documents why legacy runner selection is intentionally snapshotted when a share is added. |
| protocol/v2/qbft/spectest/error_code_map_alan.go | Replaces the Alan compatibility remapping magic bound with one derived from the final legacy error-code enum. |
Reviews (1): Last reviewed commit: "protocol/v2/qbft/spectest: derive the al..." | Re-trigger Greptile
Codecov Report✅ All modified and coverable lines are covered by tests. ☔ View full report in Codecov by Harness. 🚀 New features to boost your workflow:
|
iurii-ssv
left a comment
iurii-ssv
left a comment
There was a problem hiding this comment.
A few minor, non-blocking nits.
…l ahead A scheduled Boole fork whose NextDomainType equals DomainType is the signature of a config that forgot to set NextDomainType (the YAML unmarshal defaults it), so fail startup instead of warning while the fork is still ahead. Once the fork is active, equality is legitimate steady state - a post-fork config may collapse both fields to the post-fork domain - so it passes validation.
iurii-ssv
left a comment
iurii-ssv
left a comment
There was a problem hiding this comment.
A bunch of small suggestions.
It's a flake ❄ will fix in followups. |
EstimatedCurrentSlot panics while the wall clock is still before GenesisTime, so the unchanged-domain check would panic instead of returning the intended error on a not-yet-genesis network. Before genesis a scheduled fork is by definition not yet active, so guard the wall-clock read and keep reporting the misconfiguration as an error.
Once the unchanged-domain case became a hard error, no code path ever populated the warnings return: the node.go warn loop never ran and the test's expectedWarnings field only ever asserted emptiness. Collapse the signature to a plain error so it no longer advertises a code path that can't happen.
Validate and inBooleSubsequentWindowWithSlots duplicated the MaxUint64/SlotsPerEpoch bound verbatim; a cross-reference comment kept them associated but nothing kept them in lockstep. Extract maxEpochConvertibleToSlot so both sites derive the bound from one place.
iurii-ssv
left a comment
iurii-ssv
left a comment
There was a problem hiding this comment.
LGTM (couple nits below)
| beacon := *TestNetwork.Beacon | ||
| beacon.SlotsPerEpoch = test.slotsPerEpoch | ||
| slotsSinceGenesis := uint64(test.currentEpoch) * test.slotsPerEpoch | ||
| beacon.GenesisTime = time.Now().Add(-time.Duration(slotsSinceGenesis) * beacon.SlotDuration) |
There was a problem hiding this comment.
Nit (optional, maintainability) — non-blocking: this per-case beacon setup duplicates beaconAtEpoch just below (same *TestNetwork.Beacon copy + GenesisTime = now - slotsSinceGenesis*SlotDuration). It can't reuse beaconAtEpoch as-is because this test varies SlotsPerEpoch (including 0) and needs the preGenesis override — but a small beaconAt(epoch, slotsPerEpoch) helper (keeping the pre-genesis branch here) would fold the genesis math back into one place. Fine to leave if you'd rather not churn the helper.
| { | ||
| name: "clean_next_domain_equals_domain_after_fork", | ||
| boole: 10, | ||
| currentEpoch: 20, |
There was a problem hiding this comment.
Nit (optional, coverage) — non-blocking: the domain-equality cases cover currentEpoch before the fork (before_fork = 5) and well after (after_fork = 20), but not the exact activation epoch currentEpoch == boole (= 10), where BooleFork() flips on >=. This is a different axis from the fork-value boundaries already added (clean_genesis_fork, clean_at_max_epoch_boundary) — it's the wall-clock activation edge. Low value, since TestBooleForkAtSlot's at_fork case already pins the >= semantics, so genuinely fine to skip; flagging only to fully close the transition edge.
5 participants
Items 4, 6 and 7 of #2968, one commit each:
BooleFork()gate on the legacy AlanRoleAggregator/RoleSyncCommitteeContributionrunners is now documented as a deliberate share-add-time snapshot: the cross-population asymmetry is bounded to the Boole subsequent window (SlotsPerEpoch + booleSubsequentWindowLateSlots; 34 slots on mainnet), affects only already-decided pre-fork stragglers, and the runners retire with the Alan topics. Decision recorded on the issue: not worth restructuring the one-shot runner-map construction to slot-gate a dormant path.Network.Validate(), called right after theNetworkis assembled at startup: hard error when a scheduledforks.booleepoch would overflowFirstSlotAtEpoch(orSlotsPerEpochis zero — the review caught that the overflow guard alone would panic on that same malformed-config class); operator-actionable warning when Boole is scheduled butNextDomainType == DomainType(the exact signature of a forgottennext_domain_type, which would "activate" the fork with zero observable domain change).TimeoutInstanceErrorCode + 2(= 72, the real v1.2.2 maximum) instead of the disconnected magic 79, so a spec bump adding codes can't silently widen the remap. Builds under-tags alan_spec(that leg doesn't run in normal CI).