Skip to content

refactor(ssv_types): remove PayloadAttestationVote QBFT value object - #1076

Merged
mergify[bot] merged 1 commit into
sigp:epbsfrom
shane-moore:refactor/remove-payload-attestation-vote
Jun 10, 2026
Merged

refactor(ssv_types): remove PayloadAttestationVote QBFT value object#1076
mergify[bot] merged 1 commit into
sigp:epbsfrom
shane-moore:refactor/remove-payload-attestation-vote

Conversation

@shane-moore

@shane-moore shane-moore commented Jun 5, 2026

Copy link
Copy Markdown
Member

Problem, Evidence, and Context

Change Overview

  • Pure deletion confined to ssv_types (-203 lines, no other crates): the PayloadAttestationVote value object, the PayloadAttestationVoteValidator and its error enum, and the corresponding unit tests.
  • For the reviewer: this is a straight revert of feat(ssv_types): add PayloadAttestationVote with QbftData and value checker #1047 with no replacement code. Confirm the removed symbols are the QBFT value object and its validator, and that nothing outside this crate referenced them.
  • Intentionally unchanged: BeaconVote and GloasBeaconVote, their validators, and their tests. No new types, no behavior change.

Risks, Trade-offs, and Mitigations

Validation

  • cargo check -p ssv_types and cargo clippy -p ssv_types --all-targets: clean, no unused import / dead_code warnings.
  • cargo test -p ssv_types: 92 passed, 0 failed (+ 2 doc-tests); the BeaconVote / GloasBeaconVote suites are untouched and green.
  • cargo fmt -p ssv_types -- --check: clean.
  • git grep -n PayloadAttestationVote anchor/: no matches.

Rollback

Blockers / Dependencies

Additional Info / Next Steps

  • Remaining milestone Restructure codebase for future binaries #6 work (separate PRs): retarget the PTC role to a validator-scoped runner role with its own partial-signature kind, the PTC sign path, and the payload-attestation service wiring.

Reverts sigp#1047. Under the no-QBFT PTC redesign (SIP-94 §3, validator-scoped and leaderless), there is no per-slot QBFT instance over PTC data, so the PayloadAttestationVote consensus value, its QbftData impl, the PayloadAttestationVoteValidator, and their tests are dead code. The intended consumer (qbft_manager PTC routing, sigp#1057) never merged, so deletion is local to ssv_types.

Closes sigp#1074
@shane-moore

Copy link
Copy Markdown
Member Author

@claude, please take a look

@claude-code-actions-sigp

claude-code-actions-sigp Bot commented Jun 5, 2026

Copy link
Copy Markdown

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


Review

  • Read the changed file (anchor/common/ssv_types/src/consensus.rs)
  • Verify the deletion has no references elsewhere in the workspace
  • Confirm BeaconVote / GloasBeaconVote paths are intact
  • Post review feedback

LGTM. This is exactly what the description claims: a clean, surgical revert of #1047 with no behavior change and no collateral edits.

What I verified

  • Scope is correctly limited. git diff origin/epbs...HEAD --stat shows a single file changed — anchor/common/ssv_types/src/consensus.rs, +0/-203. No Cargo.toml, no exports, no other crates touched.
  • No dangling references. git grep -n "PayloadAttestationVote" -- anchor/ and a broader grep -rn "PayloadAttestationVote" over the whole tree both return zero matches. The only PayloadAttestation* symbols left are PayloadAttestationData / PayloadAttestationMessage in anchor/validator_store/src/lib.rs:68-69, 3105-3106, which are unrelated (types:: re-exports from the consensus types crate, not the deleted QBFT value object).
  • Imports remain live. The block deletion did not orphan any uses in consensus.rs:
    • tracing::warn — still used at lines 300, 632, 1020 (proposer / aggregator-committee / beacon-vote validators).
    • sha2::{Digest, Sha256} — still used at six call sites (lines 279, 584, 924, 964, 994, plus a test at 2187).
    • thiserror::Error — still used by remaining error enums.
    • ssz/ssz_derive/tree_hash*/typenum — all still used by the remaining value objects and validators.
  • Sibling types are untouched. BeaconVote (line 912), BeaconVoteValidator (line 1004), and GloasBeaconVote (line 943) are all present with their QbftData / QbftDataValidator impls, matching the description's "intentionally unchanged" claim.
  • No revival path is left dangling. The QBFT-design path is fully recoverable from feat(ssv_types): add PayloadAttestationVote with QbftData and value checker #1047 / git history; no half-deleted comments, dead pub use re-exports, or stale doc references remain in the file.

Notes

  • The deletion is ideologically consistent with SIP-94 §3's leaderless PTC redesign — there's no consensus value to negotiate, so there shouldn't be a QbftData impl for it. Keeping the type around "just in case" would be exactly the kind of speculative abstraction the project rules call out against.
  • I did not run make test / make lint / make cargo-fmt-check in this environment (sandboxed); relying on the author's reported cargo check/clippy/test/fmt runs and on CI to confirm. Given the change is pure deletion of self-contained items with no external callers, the residual risk from not running them locally is very low.

Findings

None. No correctness, security, performance, error-handling, idiom, design, or test-coverage issues introduced — there is no new code, only a removal of self-contained dead code. Approve from a review-feedback standpoint (formal approval is out of scope for me).
• Branch: refactor/remove-payload-attestation-vote

@codecov-commenter

Copy link
Copy Markdown

Codecov Report

✅ All modified and coverable lines are covered by tests.
⚠️ Please upload report for BASE (epbs@213bdca). Learn more about missing BASE report.

Additional details and impacted files
@@           Coverage Diff           @@
##             epbs    #1076   +/-   ##
=======================================
  Coverage        ?   62.28%           
=======================================
  Files           ?      156           
  Lines           ?    26404           
  Branches        ?        0           
=======================================
  Hits            ?    16445           
  Misses          ?     9959           
  Partials        ?        0           
Flag Coverage Δ
rust 62.28% <ø> (?)

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.

@jnhsigmap jnhsigmap 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.

cool 👍

@jnhsigmap

Copy link
Copy Markdown
Contributor

I think your refs in Links: is referencing incorrect PRs on a couple -> # 6 and # 4

@shane-moore

Copy link
Copy Markdown
Member Author

good catch, fixed, those were meant to be milestone links

@mergify
mergify Bot merged commit d8be7f1 into sigp:epbs Jun 10, 2026
23 checks passed
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