feat(ssv_types): add PayloadAttestationVote with QbftData and value checker - #1047
Conversation
|
@claude, please take a look |
|
Claude finished @shane-moore's task in 1m 35s —— View job Review
SummaryThe PR is small, additive (single file, +182/0), well-tested, and the validator/ Confirmed
Observations (non-blocking)
Risk acknowledgmentThe PR description's framing of the SC-2 trade-off (no slashing-protection, no local-view comparison on payload-status flags) is correct given:
If the SIP discussion lands on strict equality, the VerdictLGTM as a self-contained additive change. Safe to merge ahead of the #1035 / #1037 consumers. |
Codecov Report❌ Patch coverage is
Additional details and impacted files@@ Coverage Diff @@
## epbs #1047 +/- ##
=======================================
Coverage ? 62.03%
=======================================
Files ? 156
Lines ? 26237
Branches ? 0
=======================================
Hits ? 16277
Misses ? 9960
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:
|
|
Can you add |
| fn validate( | ||
| &self, | ||
| value: &PayloadAttestationVote, | ||
| _our_value: &PayloadAttestationVote, |
There was a problem hiding this comment.
Can we treat this value-check behavior as still unsettled until the SIP-94 PTC discussion resolves? This PR bakes in the current model by ignoring _our_value and testing that divergent local values are accepted. That may be the right design, but the SIP discussion is still weighing leader observation vs local observation, especially because one PayloadAttestationVote can drive all local PTC validators for the slot.
I think we should either wait for that decision or make the PR text clear that this is provisional and may change with the SIP.
Relevant thread: ssvlabs/SIPs#94 (comment)
There was a problem hiding this comment.
went with the "mark provisional" path in 15769b1. validator doc-comment now opens with Reflects [SIP-94 §3] pinned to commit 7e8b5bd, with a closing paragraph naming the do_validation -> Result extension point if the SIP later requires local-view equality on the payload-status booleans. paired with the test-side narrowing on the other thread, the code now only pins what §3 currently says.
intentionally stronger than a generic "may change" disclaimer: future readers can diff the live SIP against 7e8b5bd to see exactly what's changed since this code was written. the §3 sentence i added in 0feaf57 on the correlated-observations-within-a-cluster point is part of that pinned snapshot.
the type shape is independent of the rule and adding the second do_validation clause is a few-line follow-up if §3 flips.
| let local_view = | ||
| create_payload_attestation_vote(Hash256::from_low_u64_be(0x2222), false, false); | ||
|
|
||
| assert!(validator.validate(&proposed, &local_view)); |
There was a problem hiding this comment.
I think this test is a bit too strong while the SIP-94 PTC value-check behavior is still being discussed.
test_payload_attestation_vote_validator_ignores_start_value does not only test the current implementation. It pins the rule that the validator must accept a proposed value even when the local value has different payload_present, blob_data_available, and beacon_block_root fields. That may be where the SIP lands, but it is the same leader-observation vs local-observation choice still under discussion.
Can we either remove this test for now, or reword it so it clearly documents current draft behavior rather than a final contract? If the intended rule is only about not comparing the payload-status booleans, I would also keep beacon_block_root the same in local_view; otherwise the test is pinning root mismatch as a separate accepted behavior too.
There was a problem hiding this comment.
took the narrowing path in 15769b1. renamed to test_payload_attestation_vote_validator_accepts_status_flag_disagreement, held beacon_block_root constant between proposed and local_view, and only the payload-status booleans differ. test comment is explicit: "Root-mismatch behavior is intentionally not pinned here; the trait surface (do_validation taking only value) and the zero-root rejection test cover the root contract."
so the test only pins what SC-2 of the pinned SIP-94 snapshot actually says (trust-leader on the payload-status booleans), not the broader "ignores all of start_value" framing the old test name implied.
- Add /// docs to struct, all three fields, and the validation error variant - Pin validator doc-comment to ssvlabs/SIPs §3 at commit 7e8b5bd; name the do_validation -> Result extension point if the SIP flips on local-view equality for payload-status flags - Narrow status-flag disagreement test: hold beacon_block_root constant, flip only payload-status booleans; root-mismatch behavior intentionally not pinned
done in 15769b1: struct-level |
Merge Queue Status
This pull request spent 11 minutes 12 seconds in the queue, including 9 minutes 53 seconds running CI. Required conditions to merge
|
Problem, Evidence, and Context
Closes #1034. Next ePBS milestone item after PR #1033 (
Role::PTCCommittee+ message-validator handling).Per SIP-94 §3 and the Gloas validator spec, PTC committees run QBFT each slot over a stripped
PayloadAttestationVote { beacon_block_root, payload_present, blob_data_available }(slot omitted, pinned by the QBFT instance). After consensus, each PTC-assigned validator signs the fullPayloadAttestationData(slot reconstructed from the duty) underDOMAIN_PTC_ATTESTER. This PR adds the SSZ container and value checker that the QBFT instance will use; downstream issues #1035 and #1037 wire the consumers.Change Overview
anchor/common/ssv_types/src/consensus.rs(single file, additive):PayloadAttestationVote— SSZ struct placed next toBeaconVote.QbftDataimpl withsha256(self.as_ssz_bytes())hash for cross-operator determinism. MirrorsBeaconVote::hashshape.PayloadAttestationVoteValidator+PayloadAttestationVoteValidationError— placed next toBeaconVoteValidationError. One rule: reject zerobeacon_block_root. Uses the samedo_validation -> Result -> match Ok/Err -> warn -> boolshape asBeaconVoteValidatorandAggregatorCommitteeDataValidatorfor consistency.mod testsblock under a new═══ PayloadAttestationVote Tests ═══section.Reading order: start at the new
PayloadAttestationVotestruct (~L909); skim down to the validator (~L1205); the test block at end ofmod testsmirrors theAssignedAggregator/AggregatorCommitteetest patterns.Intentionally unchanged:
qbft_managerrouting lands in feat(qbft_manager): wire PTC committee-scoped QBFT instances and fork gate #1035;sign_payload_attestationlands in feat(validator_store): implement sign_payload_attestation #1037).QbftDecidableimpl — that lives inqbft_manager(feat(qbft_manager): wire PTC committee-scoped QBFT instances and fork gate #1035).sha256(ssz_bytes)hash impls (ProposerConsensusData,AggregatorCommitteeConsensusData,BeaconVote) — deferred standalone refactor; not mixed into this feature PR.Risks, Trade-offs, and Mitigations
Validator is structurally weaker than
BeaconVoteValidator. No slashing-protection call, no comparison ofpayload_present/blob_data_availableagainst local BN view. This is intentional per SIP-94 SC-2: at the 75% slot deadline, operator BNs may legitimately disagree on envelope arrival, and a "match local view" gate would amplify normal BN drift into cluster-level QBFT deadlocks even with an honest leader. PTC is non-slashable, so the worst case from leader malice is a wasted cluster signature, bounded.Mitigation: the value check matches the SIP as-written. An open thread on SIP-94 line 138 between Diego and Shane discusses whether to strengthen this; if the SIP eventually demands strict equality, a follow-up PR adds the second rule (the
do_validation -> Resultshape this PR uses is exactly the extension point).Validator boilerplate. The single-rule check could be inlined in
validatedirectly. Thedo_validationwrapper is preserved deliberately to match the sibling-validator precedent and keep tests calling the same shape (do_validation+matches!) used elsewhere in the file.Validation
cargo test -p ssv_types— 86 unit + 2 doctests pass (6 new + 80 existing); no regressions.make cargo-fmt && make cargo-fmt-check— clean.make lint— clean (workspace-wide clippy).cargo check --workspace— clean.Test coverage for the new code:
(payload_present, blob_data_available)combinations.SelectionProofBatchIdprecedent); different root or flipped boolean → different hash.do_validationreturnsErr(ZeroBeaconBlockRoot).start_value: trait-surface guard for the SC-2 trust-leader contract.Rollback
Single-file additive change. Revert removes the type, validator, error enum, and tests. No production consumers yet, so revert has zero behavior impact.
Blockers / Dependencies
None for merge. Downstream consumers (under milestone #4):
feat(qbft_manager): wire PTC committee-scoped QBFT instances and fork gate— consumesPayloadAttestationVote+PayloadAttestationVoteValidator.feat(validator_store): implement sign_payload_attestation— signs the decided value.Additional Info / Next Steps
N/A