Skip to content

feat(ssv_types): add PayloadAttestationVote with QbftData and value checker - #1047

Merged
mergify[bot] merged 2 commits into
sigp:epbsfrom
shane-moore:feat/payload-attestation-vote
May 22, 2026
Merged

mergify[bot] merged 2 commits into
sigp:epbsfrom
shane-moore:feat/payload-attestation-vote

Conversation

@shane-moore

@shane-moore shane-moore commented May 21, 2026 •

Copy link
Copy Markdown
Member

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 full PayloadAttestationData (slot reconstructed from the duty) under DOMAIN_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 to BeaconVote. QbftData impl with sha256(self.as_ssz_bytes()) hash for cross-operator determinism. Mirrors BeaconVote::hash shape.
  • PayloadAttestationVoteValidator + PayloadAttestationVoteValidationError — placed next to BeaconVoteValidationError. One rule: reject zero beacon_block_root. Uses the same do_validation -> Result -> match Ok/Err -> warn -> bool shape as BeaconVoteValidator and AggregatorCommitteeDataValidator for consistency.
  • 6 unit tests in the existing mod tests block under a new ═══ PayloadAttestationVote Tests ═══ section.

Reading order: start at the new PayloadAttestationVote struct (~L909); skim down to the validator (~L1205); the test block at end of mod tests mirrors the AssignedAggregator / AggregatorCommittee test patterns.

Intentionally unchanged:

Risks, Trade-offs, and Mitigations

Validator is structurally weaker than BeaconVoteValidator. No slashing-protection call, no comparison of payload_present / blob_data_available against 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 -> Result shape this PR uses is exactly the extension point).

Validator boilerplate. The single-rule check could be inlined in validate directly. The do_validation wrapper 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:

  • SSZ roundtrip across all 4 (payload_present, blob_data_available) combinations.
  • Wire-byte layout: fixed 34 bytes (32-byte root + 2 bools); position-by-position assertions.
  • Hash determinism: same inputs → same hash (two independently constructed values, mirroring SelectionProofBatchId precedent); different root or flipped boolean → different hash.
  • Validator zero-root rejection: do_validation returns Err(ZeroBeaconBlockRoot).
  • Validator non-zero acceptance: 4-combo loop documents that payload-status flags are not validated.
  • Validator ignores 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):

Additional Info / Next Steps

N/A

@shane-moore

Copy link
Copy Markdown
Member Author

@claude, please take a look

@claude-code-actions-sigp

claude-code-actions-sigp Bot commented May 21, 2026 •

Copy link
Copy Markdown

Claude finished @shane-moore's task in 1m 35s —— View job


Review

  • Read changed file anchor/common/ssv_types/src/consensus.rs
  • Compare against sibling validators (BeaconVoteValidator, AggregatorCommitteeDataValidator)
  • Verify SSZ layout & hash determinism claims
  • Check tests
  • Post review

Summary

The PR is small, additive (single file, +182/0), well-tested, and the validator/QbftData shapes match the sibling-validator precedent in the same file. No correctness, safety, or security issues spotted. A few low-priority observations below — none are blockers.

Confirmed

  • Pattern parity. PayloadAttestationVoteValidator::validate mirrors AggregatorCommitteeDataValidator::validate (anchor/common/ssv_types/src/consensus.rs:602-614) almost line-for-line: same do_validation(value) (no _our_value), same Ok/Err arm, same warn!(%err, ...) call. The choice to keep do_validation even for a single rule is justified by the sibling-validator precedent (AggregatorCommitteeDataValidator does the same) and the open SIP-94 thread that may add a second rule.
  • Hash impl parity. Identical body to BeaconVote::hash at consensus.rs:899-906. Deferring the sha256(ssz_bytes) dedup refactor is the right call — keeps this PR scope-clean.
  • Trait surface. QbftData requires Debug + Clone + Encode + Decode (consensus.rs:45-49); derives on the struct cover all four. QbftDecidable<E> (anchor/qbft_manager/src/lib.rs:418) is correctly deferred to feat(qbft_manager): wire PTC committee-scoped QBFT instances and fork gate #1035 since the slot/committee identity comes from the QBFT instance, not the value.
  • SC-2 contract. _our_value is correctly ignored in validate, and a dedicated test (test_payload_attestation_vote_validator_ignores_start_value) pins this at the trait surface — not just structurally.
  • Test coverage. SSZ roundtrip across all 4 boolean combos, fixed-length wire layout (34B with byte-position assertions), hash determinism (independently constructed equal values + 3 negative cases), validator zero-rejection via matches!, and 4-combo non-zero acceptance loop. Coverage is appropriate for the surface area.
  • Imports/derives. warn, Sha256, Encode/Decode, TreeHash, arbitrary-fuzz are all already in scope (consensus.rs:13,19,15,16,21); no new dependencies.

Observations (non-blocking)

  1. do_validation visibility. pub fn do_validation is exposed (consensus.rs:1236), matching BeaconVoteValidator::do_validation and AggregatorCommitteeDataValidator::do_validation. Useful for tests calling it directly with matches!(..., Err(_)). Consistent — no change needed.

  2. #[derive(Default)] on a unit struct. PayloadAttestationVoteValidator is a unit struct, so Default is mechanically trivial but unused in this PR (no ::default() call site). Sibling AggregatorCommitteeDataValidator carries PhantomData and so needs an explicit impl Default. This is harmless and likely useful for feat(qbft_manager): wire PTC committee-scoped QBFT instances and fork gate #1035 wiring (Box::<PayloadAttestationVoteValidator>::default()), so leave as-is.

  3. type Hash = Hash256 consistency. Matches BeaconVote and is what QbftDecidable<E> (qbft_manager/src/lib.rs:418) requires (QbftData<Hash = Hash256>). feat(qbft_manager): wire PTC committee-scoped QBFT instances and fork gate #1035 will be able to add the impl without changes here. Good.

  4. Wire-byte test is doing real work. test_payload_attestation_vote_ssz_byte_layout asserts position-by-position rather than just round-tripping. Catches accidental field reordering — worth keeping as the canonical layout pin.

  5. Naming nit (very minor, take or leave). PayloadAttestationVoteValidator vs. PayloadAttestationVoteValidationError — both names are long but match the file convention (BeaconVoteValidator / BeaconVoteValidationError). Consistent.

Risk acknowledgment

The PR description's framing of the SC-2 trade-off (no slashing-protection, no local-view comparison on payload-status flags) is correct given:

  • PTC is non-slashable per SIP-94 §3,
  • Strict equality on payload_present/blob_data_available would amplify normal BN drift around the 75% slot deadline into cluster-level QBFT deadlocks,
  • The wasted-signature blast radius is bounded.

If the SIP discussion lands on strict equality, the do_validation -> Result shape this PR uses is the correct extension point — adding a second rule is a 5-line follow-up.

Verdict

LGTM as a self-contained additive change. Safe to merge ahead of the #1035 / #1037 consumers.
· branch: feat/payload-attestation-vote

@codecov-commenter

codecov-commenter commented May 21, 2026 •

Copy link
Copy Markdown

Codecov Report

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

Files with missing lines Patch % Lines
anchor/common/ssv_types/src/consensus.rs 95.83% 4 Missing ⚠️
Additional details and impacted files
@@           Coverage Diff           @@
##             epbs    #1047   +/-   ##
=======================================
  Coverage        ?   62.03%           
=======================================
  Files           ?      156           
  Lines           ?    26237           
  Branches        ?        0           
=======================================
  Hits            ?    16277           
  Misses          ?     9960           
  Partials        ?        0           
Flag Coverage Δ
rust 62.03% <95.83%> (?)

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

☔ View full report in Codecov by Sentry.
📢 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.

@diegomrsantos

Copy link
Copy Markdown
Contributor

Can you add /// docs for PayloadAttestationVote, its fields, and PayloadAttestationVoteValidationError? A short description of the vote shape and the zero-root rejection would make the contract clearer where it is defined.

fn validate(
&self,
value: &PayloadAttestationVote,
_our_value: &PayloadAttestationVote,

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.

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)

Copy link
Copy Markdown
Member Author

Choose a reason for hiding this comment

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

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));

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.

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.

Copy link
Copy Markdown
Member Author

Choose a reason for hiding this comment

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

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
@shane-moore

shane-moore commented May 21, 2026 •

Copy link
Copy Markdown
Member Author

Can you add /// docs for PayloadAttestationVote, its fields, and PayloadAttestationVoteValidationError? A short description of the vote shape and the zero-root rejection would make the contract clearer where it is defined.

done in 15769b1: struct-level /// on the stripped shape + post-consensus signing context, per-field docs on all three fields, summary /// on PayloadAttestationVoteValidationError, and a variant-level /// on ZeroBeaconBlockRoot explaining the zero-hash rejection.

@diegomrsantos diegomrsantos left a comment •

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.

Looks good now. The docs make the SIP-94 assumption explicit and pinned, and the status-flag disagreement test no longer pins root mismatch behavior.

@mergify

mergify Bot commented May 22, 2026 •

Copy link
Copy Markdown

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
  • check-success=test-suite-success

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

Projects

None yet

Development

Successfully merging this pull request may close these issues.

3 participants