feat(ssv_types): add GloasBeaconVote with QbftData for Gloas slots - #1059
Conversation
Closes sigp#1025. Adds the QBFT-decided value type for committee attestation duties under Gloas (Fork::CStar) and the fork-gated dispatch surface. Under Gloas, AttestationData.index is BN-supplied and part of the signed attestation root, so it must travel through QBFT rather than being reconstructed locally at signing time. - New `GloasBeaconVote { block_root, source, target, attestation_data_index }` alongside `BeaconVote`. Separate type so SSZ wire bytes mutually reject on length mismatch across the fork boundary (112 vs 120 fixed bytes). `QbftData` impl mirrors `BeaconVote::hash()` (SHA-256 over SSZ bytes). - `QbftManager`: new instance map, `QbftDecidable<E>` impl, cleaner retention, and fork-gated routing in the `Role::Committee` arm of `receive_data`. - `validator_store::sign_committee_attestations`: fork-branched `decide_instance` call; the Gloas branch references a `create_gloas_beacon_vote_validator()` stub whose body lands in sigp#1026. A `TODO(sigp#1027)` marker flags where the decided index is dropped today; sigp#1027 plumbs it into each validator's `attestation.data.index`. Pre-Gloas paths are unchanged.
Codecov Report❌ Patch coverage is Additional details and impacted files@@ Coverage Diff @@
## epbs #1059 +/- ##
=======================================
Coverage ? 62.11%
=======================================
Files ? 156
Lines ? 26380
Branches ? 0
=======================================
Hits ? 16387
Misses ? 9993
Partials ? 0
Flags with carried forward coverage won't be shown. Click here to find out more. ☔ View full report in Codecov by Sentry. 🚀 New features to boost your workflow:
|
|
The new dispatch routes all Filed as #1061 (depends on #1027, since sync needs to seed the same |
| pub block_root: Hash256, | ||
| pub source: Checkpoint, | ||
| pub target: Checkpoint, | ||
| pub attestation_data_index: u64, |
There was a problem hiding this comment.
nit: per-field /// docs would mirror Diego's request on #1047 (comment, satisfied in 15769b1). The struct-level doc here is solid; field-level docs mostly buy attestation_data_index showing up in LSP hover with its SIP-94 semantics, but matching the precedent across the sibling QBFT-value types in the same file keeps the contract visible where it's defined.
| pub block_root: Hash256, | |
| pub source: Checkpoint, | |
| pub target: Checkpoint, | |
| pub attestation_data_index: u64, | |
| /// LMD-GHOST vote: root of the beacon block being attested to. | |
| pub block_root: Hash256, | |
| /// FFG source checkpoint, copied from the BN-supplied `AttestationData`. | |
| pub source: Checkpoint, | |
| /// FFG target checkpoint, copied from the BN-supplied `AttestationData`. | |
| pub target: Checkpoint, | |
| /// BN-supplied `AttestationData.index`. Under Gloas this encodes the | |
| /// attester's fork-choice view of payload status (`0` = `EMPTY`, | |
| /// `1` = `FULL` for non-same-slot attestations) and participates in the | |
| /// signed attestation root, so it must travel through QBFT rather than | |
| /// being reconstructed locally. | |
| pub attestation_data_index: u64, |
BeaconVote itself is undocumented at the field level today; happy to file a one-line follow-up to backfill it for symmetry (or roll it into #1026). Not a merge blocker either way.
| 0, | ||
| "Committee message pre-CStar must NOT spawn a GloasBeaconVote instance" | ||
| ); | ||
| } |
There was a problem hiding this comment.
nit: like @jnhsigmap suggested on sibling PR (#1057 discussion), worth pinning the slot-to-epoch + active_fork(epoch) + >= Fork::CStar composition at the activation boundary with a non-zero CStar epoch. Today both tests use ForkSchedule::new(fork, ..., "test"), which puts the chosen fork at epoch 0, so slot.epoch(spe) and active_fork(epoch) aren't exercised on the boundary, and an off-by-one in any of the three would still pass.
| } | |
| } | |
| /// Pin the slot-to-epoch + `active_fork(epoch)` + `>= Fork::CStar` composition | |
| /// at the activation boundary. Off-by-one in any of the three would flip | |
| /// exactly one of the assertions below. | |
| #[tokio::test] | |
| async fn test_committee_message_routes_at_cstar_activation_boundary() { | |
| use std::collections::BTreeMap; | |
| use types::Epoch; | |
| const CSTAR_ACTIVATION_EPOCH: u64 = 5; | |
| const SLOTS_PER_EPOCH: u64 = 32; | |
| let setup = setup_test(1); | |
| let mut configs = BTreeMap::new(); | |
| configs.insert(Fork::Alan, (Epoch::new(0), DomainType::default())); | |
| configs.insert( | |
| Fork::CStar, | |
| (Epoch::new(CSTAR_ACTIVATION_EPOCH), DomainType::default()), | |
| ); | |
| let schedule = ForkSchedule::from_fork_configs(configs, "test") | |
| .expect("Alan@0 + CStar@5 is a valid schedule"); | |
| let manager = build_manager(&setup, schedule); | |
| let last_pre_cstar = CSTAR_ACTIVATION_EPOCH * SLOTS_PER_EPOCH - 1; | |
| let (signed, qbft) = build_committee_message(last_pre_cstar); | |
| manager.receive_data(signed, qbft).expect("pre-CStar dispatch"); | |
| assert_eq!(manager.beacon_vote_instances.len(), 1); | |
| assert_eq!(manager.gloas_beacon_vote_instances.len(), 0); | |
| let first_cstar = CSTAR_ACTIVATION_EPOCH * SLOTS_PER_EPOCH; | |
| let (signed, qbft) = build_committee_message(first_cstar); | |
| manager.receive_data(signed, qbft).expect("CStar dispatch"); | |
| assert_eq!(manager.gloas_beacon_vote_instances.len(), 1); | |
| // BeaconVote map still holds the pre-CStar instance. | |
| assert_eq!(manager.beacon_vote_instances.len(), 1); | |
| } |
Not a merge blocker. If you'd rather defer, happy to file one follow-up issue covering all three fork-boundary gaps (this CStar gate, the AggregatorCommittee Boole gate in aggregator_tests.rs, and the PTCCommittee CStar gate on #1057).
|
@claude, can you take a look? keep my comments on the pr in mind as well |
|
Claude finished @shane-moore's task in 3m 41s —— View job PR Review
Took a pass over the PR with Shane's comments in mind. Type/dispatch surface is well-scoped and the test set covers the stated acceptance criteria. Three things stand out, none of which I'd treat as a merge blocker (they line up with what Shane already flagged): 1.
|
…dary Addresses review feedback on sigp#1059. Existing tests put CStar at epoch 0, so the slot-to-epoch + `active_fork(epoch)` lookup wasn't exercised — an off-by-one in either would pass. New boundary test uses `from_fork_configs(Alan@0 + CStar@5)` and asserts dispatch at slots `5 * 32 - 1` and `5 * 32` lands in the correct map. Also adds per-field `///` docs on `GloasBeaconVote` for symmetry with `PayloadAttestationVote`.
shane-moore
left a comment
shane-moore
left a comment
There was a problem hiding this comment.
well done! great start getting into gloas work 🎉
3 participants
Problem, Evidence, and Context
Under Gloas (
Fork::CStar),AttestationData.indexis BN-supplied and part of the signed attestation root. SSV's currentBeaconVotedropsindexand lets each operator reconstruct it locally at signing time, which was safe pre-Gloas (deterministically0post-Electra, duty-derived earlier) but is unsafe post-Gloas: operators can agree onblock_root/source/targetand still produce different signing roots, and an operator can be led to sign bothindex=0andindex=1for the same(source, target, slot)without local slashing protection tripping.Resolves #1025. Spec source: SIP-94 §2.
Change Overview
This PR adds only the type and dispatch surface; value-checking (#1026) and end-to-end plumbing (#1027) ride on top.
GloasBeaconVote { block_root, source, target, attestation_data_index }next toBeaconVote. Kept as a separate type so the two SSZ wire formats mutually reject on length mismatch across the fork boundary (112 vs 120 fixed bytes).QbftDataimpl mirrorsBeaconVote::hash()(SHA-256 over SSZ bytes).QbftManager: newgloas_beacon_vote_instancesmap,QbftDecidable<E>impl, retention in the cleaner, and fork-gated routing in theRole::Committeearm ofreceive_data.validator_store::sign_committee_attestations: fork-brancheddecide_instancecall. The Gloas branch referencescreate_gloas_beacon_vote_validator(), a stub returningtodo!()whose body lands in feat(ssv_types): fork-aware GloasBeaconVoteValidator with slashability #1026 — wiring it now makes feat(ssv_types): fork-aware GloasBeaconVoteValidator with slashability #1026 a body-only change. ATODO(#1027)marker flags where the decidedattestation_data_indexis currently discarded; feat(validator_store): apply decided AttestationDataIndex in metadata-service and committee signing #1027 plumbs it into each validator'sattestation.data.indexbefore signing.Risks, Trade-offs, and Mitigations
create_gloas_beacon_vote_validator()istodo!(); any committee-attestation slot atactive_fork(epoch) >= Fork::CStarwill panic. The panic is intentional and obvious, and feat(ssv_types): fork-aware GloasBeaconVoteValidator with slashability #1026 is the immediate follow-up. Pre-Gloas operation is unaffected.FORK_PREPARATION_EPOCHSdual-subscription window.BeaconVoteand renames the Gloas type back once Gloas has activated on all networks.Validation
cargo test -p ssv_types --lib— 92 passed (3 new: SSZ round-trip, hash determinism with explicitSha256(SSZ-bytes)assertion plus cross-index guard, length-mismatch rejection).cargo test -p qbft_manager --lib— 18 passed (2 new: dispatch routes committee messages to the Gloas map atFork::CStar, to the legacy map pre-CStar).cargo test -p anchor_validator_store --lib— 33 passed, unchanged.make cargo-fmt-checkandmake lintclean.Rollback
Pure-additive at the type layer; the fork-gate in
receive_dataand the fork-branch insign_committee_attestationscollapse cleanly to the pre-PR shape if reverted. No config, data, or operational impact pre-Gloas.Blockers / Dependencies
GloasBeaconVoteValidator) is the immediate follow-up — required before Gloas activation to replace thetodo!()stub.attestation_data_indexthrough the signing path.