Skip to content

feat(ssv_types): retarget PTC role to validator-scoped PTCAttester - #1080

Merged
mergify[bot] merged 4 commits into
sigp:epbsfrom
shane-moore:feat/ptc-attester-role
Jun 10, 2026
Merged

feat(ssv_types): retarget PTC role to validator-scoped PTCAttester#1080
mergify[bot] merged 4 commits into
sigp:epbsfrom
shane-moore:feat/ptc-attester-role

Conversation

@shane-moore

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

Copy link
Copy Markdown
Member

Problem, Evidence, and Context

SIP-94 §3 was rewritten to a validator-scoped, leaderless PTC design (modeled on
the ProposerPreferences flow in §5): a payload-timeliness attestation is a
standalone single-validator signature with no QBFT consensus round. The role
that merged earlier (#1033) modeled PTC as a committee-scoped, QBFT-bearing role,
which no longer matches the spec.

This PR retargets that role to a validator-scoped PTCAttester and gives PTC its
own partial-signature kind instead of overloading PostConsensus.

Change Overview

PTC moves out of the committee role-class into the validator-scoped, non-QBFT
class at every dispatch site: validator-scoped duty executor (not a committee
id), no max QBFT round, the pre-consensus message bucket, and a per-validator
one-signature-per-packet bound. It also gets a dedicated PartialSignatureKind,
so a PTC attestation is no longer indistinguishable on the wire from a
post-consensus signature.

Suggested reading order:

  1. ssv_types (msgid, partial_sig): the role rename + reclassification and
    the new partial-signature kind. A small is_qbft_role predicate is
    introduced (positively named, mirroring is_committee_role) and negated
    at the consensus-message guard.
  2. message_validator: the validation-path consequences (kind/role binding,
    packet bound, duty limit, and rejection of consensus messages for non-QBFT
    roles). Two committee-only helpers are deleted, subsumed by the shared
    per-validator bound.
  3. qbft_manager: the PTC routing stub is removed; PTC now rejects QBFT
    messages as an inconsistent message id in both executor branches.

What intentionally did not change:

  • Wire identifiers: the role byte stays 7; the dedicated kind takes value 7.
  • The Fork::CStar activation gate.

Risks, Trade-offs, and Mitigations

  • Contained blast radius: every Role / PartialSignatureKind match in the
    touched crates is exhaustive (no _ wildcard), so the compiler flagged each
    site that needed updating. cargo check --workspace is the gate.
  • Behavior change: a PTC consensus (QBFT) message is now rejected rather than
    routed (matching the leaderless design), and PTC partial-sig packets are
    capped at one signature per validator per packet (the old committee occurrence
    cap is subsumed by this bound). The rejection is a Reject gossip verdict
    (peer penalty on synced nodes), which is intentional: post-CStar a PTC
    consensus message is a protocol violation under the leaderless design.
    Previously such a message was accepted and propagated, then dropped
    internally by the qbft_manager stub.
  • Behavior change (bucketing flip): because PTCAttester leaves the committee
    role-class, two checks previously skipped for PTCCommittee now apply to PTC
    packets: the validator-index-in-committee check (ValidatorIndexMismatch) and
    the per-signer slot-advancement check (SlotAlreadyAdvanced). Both follow from
    validator-scoping (one validator per packet, monotonic per-validator slot).
  • Behavior change (TTL tightened): PTCAttester moves from the epoch-long
    lateness/TTL bucket (slots_per_epoch + 2 = 34 slots, inherited from the
    committee-scoped design) to the slot-bound Proposer/SyncCommittee bucket
    (3 slots). Per the Gloas spec, a payload attestation is gossip-valid only
    for its own slot and block-includable only at slot + 1, so older partials
    are useless and accepting them only widens the replay/relay surface.
  • Wire compatibility: the role byte is unchanged, but a PTC packet's
    partial-signature kind flips from PostConsensus (0) to PTCAttester
    (7), and the byte-7 MessageId layout changes from CommitteeId
    ([24..56]) to validator pubkey ([8..]). No deployed impact: no producer
    emits PTC partial signatures yet and CStar has never been fork-active, so
    only an epbs devnet mixing pre- and post-retarget nodes would need a
    lockstep upgrade.

Validation

  • cargo test -p ssv_types -p message_validator -p qbft_manager: 98 / 63 / 19
    pass, 0 failures (plus doc-tests). Existing PTC tests were retargeted to the
    validator-scoped role; coverage asserts the validator duty executor, the
    non-QBFT classification (not committee-scoped, no max round), the dedicated
    kind's SSZ round-trip, the kind-to-role binding, the per-validator packet
    bound, the consensus-message rejection for PTCAttester, the pre-CStar
    fork reject, the flat duty limit, and the slot-bound TTL (accepted 2 slots
    late, rejected 20 slots late).
  • cargo +nightly fmt --all -- --check: clean.
  • git grep PTCCommittee anchor/: empty.

Rollback

Revert the branch's four commits. No config, data, or migration impact; wire encodings
are unchanged, so there is no cross-version compatibility concern.

Blockers / Dependencies

None for merge. Follow-ups that build on this PR's new symbols: the PTC sign path
and the client spawn wiring.

Retargets the merged committee-scoped Role::PTCCommittee to a
validator-scoped, leaderless Role::PTCAttester per SIP-94 §3, and gives
PTC its own PartialSignatureKind::PTCAttester = 7 instead of reusing
PostConsensus. Wire bytes are unchanged (Role byte 7; kind value 7).

- msgid: rename the role, drop it from is_committee_role, max_round is
  None (non-QBFT), validator-scoped duty executor; add is_non_qbft_role.
- partial_sig: add PartialSignatureKind::PTCAttester = 7.
- message_validator: bucket PTC as pre-consensus, bind the PTCAttester
  role to the PTCAttester kind, enforce the per-validator
  one-message-per-packet bound (removing the dead committee occurrence
  helpers), flat duty_limit of Some(2), and reject consensus messages
  for non-QBFT roles via is_non_qbft_role.
- qbft_manager: drop the PTC routing stub; PTCAttester rejects QBFT
  messages as an inconsistent message id in both executor branches.
@shane-moore

Copy link
Copy Markdown
Member Author

@claude, please take a look

@claude-code-actions-sigp

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

Copy link
Copy Markdown

Claude encountered an error after 12m 9s —— View job


Review in progress

  • Read PR diff against origin/epbs
  • Review ssv_types changes (msgid, partial_sig)
  • Review message_validator changes
  • Review qbft_manager changes
  • Cross-check tests cover the asserted behaviors
  • Post review summary
    · branch feat/ptc-attester-role

@codecov-commenter

codecov-commenter commented Jun 6, 2026

Copy link
Copy Markdown

Codecov Report

❌ Patch coverage is 98.94737% with 2 lines in your changes missing coverage. Please review.
⚠️ Please upload report for BASE (epbs@213bdca). Learn more about missing BASE report.

Files with missing lines Patch % Lines
anchor/common/ssv_types/src/msgid.rs 97.29% 1 Missing ⚠️
anchor/message_validator/src/consensus_message.rs 97.43% 1 Missing ⚠️
Additional details and impacted files
@@           Coverage Diff           @@
##             epbs    #1080   +/-   ##
=======================================
  Coverage        ?   62.52%           
=======================================
  Files           ?      156           
  Lines           ?    26547           
  Branches        ?        0           
=======================================
  Hits            ?    16598           
  Misses          ?     9949           
  Partials        ?        0           
Flag Coverage Δ
rust 62.52% <98.94%> (?)

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.

@claude-code-actions-sigp

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

Copy link
Copy Markdown

Claude finished @shane-moore's task in 11m 45s —— View job


Claude Code Review

  • Read PR diff against origin/epbs
  • Review ssv_types changes (msgid, partial_sig)
  • Review message_validator changes
  • Review qbft_manager changes
  • Cross-check with SIP-94 §3 design intent
  • Post review

Summary

The retargeting from PTCCommitteePTCAttester is coherent and well‑scoped. Wire encodings (role byte 7, partial‑sig kind 7) are preserved, the new validator‑scoped semantics are applied consistently across msgid / message_validator / qbft_manager, and the new is_non_qbft_role() predicate is a clean way to express the consensus‑message guard. Test coverage retargets the four invariants the PR description claims (validator duty executor, no max round, kind/role binding, per‑packet > 1 rejection, pre‑CStar reject, long TTL bucket, flat duty limit).

I have a few questions / observations below — none are merge blockers, but two are worth either confirming intent in the PR description or following up on.


Observations

1. Two behavior changes that are correct, but maybe worth surfacing.

Because Role::PTCAttester no longer satisfies is_committee_role(), two checks that were skipped for the old PTCCommittee are now enforced for PTCAttester packets:

  • partial_signature.rs:128 — validator‑index‑in‑committee check now runs for PTC packets.
  • partial_signature.rs:192SlotAlreadyAdvanced rejection now runs for PTC packets.

Both are aligned with the validator‑scoped redesign (one validator per packet, monotonic per‑validator slot progression). I would just call this out explicitly in the PR description's "Behavior change" bullet, so reviewers understand the bucketing flip is two observable rule changes, not just the duty‑limit/packet‑bound change already mentioned.

2. Possible orphaned QBFT‑PTC types in ssv_types::consensus.

PayloadAttestationVote, PayloadAttestationVoteValidator, and PayloadAttestationVoteValidationError (anchor/common/ssv_types/src/consensus.rs:979, :1300, :1279) are explicitly QBFT consensus‑value types for PTC. With this PR they have no remaining caller (grep -rn "PayloadAttestationVote\b" anchor/ --include="*.rs" only matches the definitions and their tests). Two reasonable readings:

  • Intentional: kept for the follow‑up sign‑path / client‑spawn PRs (which the description mentions).
  • Dead code: should be deleted in this PR or in a near follow‑up.

If it's the former, a one‑line // Retained for follow-up … note next to PayloadAttestationVote would prevent the next reader from wondering. If it's the latter, it could be a separate cleanup.

3. is_non_qbft_role() is defined as max_round().is_none().

pub fn is_non_qbft_role(self) -> bool {
    self.max_round().is_none()
}

This is concise and currently correct, but it implicitly couples "non‑QBFT" to "has no max round". If a future role legitimately has max_round = None for an unrelated reason (e.g. an unbounded‑round consensus role), is_non_qbft_role() silently misclassifies it. Given the rule's only consumer (consensus_message.rs:124) is the consensus‑message admission gate, the safer and more obviously‑exhaustive form is to match on the variant set directly:

pub fn is_non_qbft_role(self) -> bool {
    matches!(
        self,
        Role::ValidatorRegistration | Role::VoluntaryExit | Role::PTCAttester,
    )
}

The compiler then forces a deliberate decision when a new Role variant is added, instead of inheriting whatever max_round() happens to return. Nit-level — happy either way, but worth a moment of thought.

4. qbft_manager Validator‑branch error log wording. anchor/qbft_manager/src/lib.rs:283

error!(?msg_id, "Unexpected role/executor combination in msg id");

For PTCAttester with a DutyExecutor::Validator, the role/executor combination is correct; what's unexpected is that a QBFT message arrived for a non‑QBFT role at all. Splitting that arm into its own match (or refining the message to "QBFT message for non-QBFT role" when the role is PTCAttester | ValidatorRegistration | VoluntaryExit) would make the log line actionable when this triggers in production. Small thing.

5. Test for test_ptc_attester_rejects_multiple_messages_per_packet is asserting the right thing.

I traced the path: validator_index: ValidatorIndex(123) is in the test committee's validator_indices, so the validator‑index check passes; message_count = 2 then trips the > 1 arm in validate_partial_sig_messages_by_duty_logic. The comment claiming the per‑validator bound "subsumes" the validator‑index occurrence cap is accurate for PTC's messages.len() ≤ 1 semantics.


Things I checked and are fine

  • Wire compatibility: role byte 7 and partial‑sig kind value 7 preserved; SSZ round‑trip and "invalid variant" boundary updated to 8u64.
  • is_committee_role() doc comment updated to drop PTC.
  • Role::PTCAttester reaches the Validator(pk) arm of Validator::validate_message's committee‑info dispatch — pre‑condition for the per‑validator path.
  • qbft_manager rejects PTCAttester from both DutyExecutor::Validator and DutyExecutor::Committee branches, preventing accidental routing if a malformed packet inverts the executor.
  • MessageCounts::validate_partial_signature_message and MessageCounts::add correctly bucket PTCAttester as pre‑consensus, matching the spec's leaderless design.
  • Role::Aggregator | Role::ValidatorRegistration | Role::PTCAttester => Ok(Some(2)) — the comment at lib.rs:1017 accurately describes why 2 is the limit (per‑validator counter + epoch/reorg margin).

Fix is_non_qbft_role to match variants explicitly →
• Branch: feat/ptc-attester-role

@shane-moore

shane-moore commented Jun 6, 2026

Copy link
Copy Markdown
Member Author

@claude, thanks for the careful pass. Dispositions:

1. Two newly-enforced checks. Agreed, surfaced in the description: added a "bucketing flip" bullet noting that PTCAttester leaving the committee class now applies the validator-index-in-committee check (ValidatorIndexMismatch) and the SlotAlreadyAdvanced check to PTC packets.

2. Orphaned QBFT-PTC types. Already handled by the open #1076 (issue #1074), which removes exactly these and touches only consensus.rs. They were orphaned before this PR too (no callers on epbs); #1080 deliberately leaves consensus.rs untouched (disjoint symbols), so no change here.

3. is_non_qbft_role via explicit matches!. Keeping max_round().is_none(). matches!(self, A | B | C) expands with an implicit _ => false, so it would not force a compile-time decision when a new Role is added (the variant silently classifies as QBFT). max_round() is the one genuinely exhaustive, no-wildcard match that does force that decision, so deriving the predicate from it keeps a single source of truth that can't drift from it. If a role ever needs max_round = None while still running consensus, we'd split the two concepts then.

4. Log wording at qbft_manager. That error!(...) is an unchanged context line: it already covered ValidatorRegistration | VoluntaryExit before this PR, which only added PTCAttester to the same arm. Leaving the wording as-is to stay within this PR's diff; it can be refined as a separate cleanup.

5. Confirmed, thanks for tracing it.

@claude-code-actions-sigp

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

Copy link
Copy Markdown

Claude finished @shane-moore's task in 25s —— View job


Dispositions acknowledged

Thanks for the thorough responses — all five reasoned, no further pushback from me.

1. Bucketing flip surfaced in description.

2. Orphaned types handled by #1076. Makes sense — disjoint-symbols rule keeps #1080 reviewable independently. No change here.

3. is_non_qbft_role deriving from max_round().is_none(). You're right and I want to flag that I was wrong on this one. I checked: max_round() matches every Role variant explicitly with no _ arm, so adding a variant is a compile error there; matches!(self, A | B | C) would expand to an implicit _ => false and silently classify a new variant as QBFT. So max_round().is_none() actually is the safer "single source of truth, can't drift" form here, not the looser one I suggested. Withdrawn.

4. Log wording. Fair — pre-existing line, keeping diff scope tight is the right call. Worth a separate small cleanup if anyone hits it in production.

5. 👍

No remaining concerns from my side. LGTM modulo your in-flight description tweak.

--- · Branch: feat/ptc-attester-role

Add classification asserts for PTCAttester (not committee-scoped, no
max round, non-QBFT) so a silent revert of the retarget fails a test,
and extend the non-consensus-role rejection test to cover PTCAttester.
Also regroup PTCAttester into the non-QBFT arm of qbft_manager's
committee-executor match; same InconsistentMessageId outcome, clearer
intent.
Payload attestations are gossip-valid only for their own slot and
block-includable only at slot + 1, so PTC partials older than that are
useless. Move PTCAttester from the epoch-long TTL bucket (34 slots,
inherited from the committee-scoped design) to the Proposer/SyncCommittee
slot-bound bucket (3 slots).
@jnhsigmap

Copy link
Copy Markdown
Contributor

@claude, thanks for the careful pass. Dispositions:

1. Two newly-enforced checks. Agreed, surfaced in the description: added a "bucketing flip" bullet noting that PTCAttester leaving the committee class now applies the validator-index-in-committee check (ValidatorIndexMismatch) and the SlotAlreadyAdvanced check to PTC packets.

2. Orphaned QBFT-PTC types. Already handled by the open #1076 (issue #1074), which removes exactly these and touches only consensus.rs. They were orphaned before this PR too (no callers on epbs); #1080 deliberately leaves consensus.rs untouched (disjoint symbols), so no change here.

3. is_non_qbft_role via explicit matches!. Keeping max_round().is_none(). matches!(self, A | B | C) expands with an implicit _ => false, so it would not force a compile-time decision when a new Role is added (the variant silently classifies as QBFT). max_round() is the one genuinely exhaustive, no-wildcard match that does force that decision, so deriving the predicate from it keeps a single source of truth that can't drift from it. If a role ever needs max_round = None while still running consensus, we'd split the two concepts then.

4. Log wording at qbft_manager. That error!(...) is an unchanged context line: it already covered ValidatorRegistration | VoluntaryExit before this PR, which only added PTCAttester to the same arm. Leaving the wording as-is to stay within this PR's diff; it can be refined as a separate cleanup.

5. Confirmed, thanks for tracing it.

For # 3: This would trip me up without the help of a coding agent in the distant future if making mods to this domain and still learning my way around here. Valid to use the max round if its tied semantically, but would suggest a test to ensure all roles follow the rules. Something like

  for role in [Role::Committee, Role::Aggregator, Role::AggregatorCommittee,
               Role::Proposer, Role::SyncCommittee] {
      assert!(!role.is_non_qbft_role(), "{role:?} runs QBFT");
      assert!(role.max_round().is_some(), "{role:?} must have a max round");
  }

technically non-blocking but would probably help maintainers later on. I see ptc_attester_is_validator_scoped_non_qbft checks PTCAttester adheres, but no test coverage for the others ^.

Less important - could be followed up later or ignored but I see Some(role) if role.is_non_qbft_role() seems to invalidate FailedToGetMaxRound if it's now tied to max_round. That branch won't be reached if is_non_qbft_role() returns None right?

otherwise lgtm 💯

@claude-code-actions-sigp

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

Copy link
Copy Markdown

Tasks

  • Read trigger comment and confirm context
  • Read consensus_message.rs to verify FailedToGetMaxRound reachability claim
  • Read msgid.rs to confirm role coverage and existing tests
  • Decide on test addition + reachability disposition
  • Implement (if applicable) + push
  • Post final response

View job run · Branch: feat/ptc-attester-role

@jnhsigmap

jnhsigmap commented Jun 10, 2026

Copy link
Copy Markdown
Contributor

ha ha, cancelled that run ^.
don't need claude to answer that one 😆

@petarjuki7

Copy link
Copy Markdown
Member

Great, looks good!
I have one idea, not a blocker.
Could we maybe change is_non_qbft_role() to is_qbft_role() and then use !is_qbft_role() where needed?
I think it reads a bit easier, so if sometime later in the code we have to use something like !is_non_qbft_role() it's easier to reason about.
It also mirrors how we use is_committee_role; it's named positively and used negated at every call site: !role.is_committee_role() appears multiple times.

Review feedback on sigp#1080:

- Rename is_non_qbft_role to is_qbft_role and negate at the
  consensus-message guard, mirroring how is_committee_role is named
  positively and negated at call sites (petarjuki7).
- Pin every role's QBFT classification in a test so a new role landing
  in the wrong max_round arm fails loudly instead of silently flipping
  validation behavior (jnhsigmap).
- Drop the unreachable FailedToGetMaxRound variant: the guard already
  rejects every role without a max round, so the fallback now returns
  UnexpectedConsensusMessage, which is also what the branch would mean
  if it ever fired (jnhsigmap).
@shane-moore

Copy link
Copy Markdown
Member Author

@jnhsigmap Both addressed in b50aaf63.

Classification test: added role_qbft_classification_is_pinned, extended from your sketch to pin both sides of the partition (the five QBFT roles plus ValidatorRegistration / VoluntaryExit / PTCAttester as non-QBFT), so a new role landing in the wrong max_round arm fails loudly either way.

FailedToGetMaxRound: your read is right, the branch is unreachable. One nuance: it was already dead before this PR, since the old ValidatorRegistration | VoluntaryExit guard exactly covered the None-returning roles (PTCCommittee had Some(4)). This PR just made the deadness definitional, so I cleaned it up here: the fetch is now a let-else whose fallback returns UnexpectedConsensusMessage (semantically what that branch would mean if it ever fired, and no panic on network input), and the variant is deleted. Nothing else referenced it.

@shane-moore

Copy link
Copy Markdown
Member Author

@petarjuki7 Good call, done in b50aaf63. is_qbft_role() returning max_round().is_some(), with the guard now Some(role) if !role.is_qbft_role(). Agreed on the is_committee_role symmetry, and renaming now is cheap since the follow-up PRs haven't started consuming the symbol yet.

@mergify

mergify Bot commented Jun 10, 2026

Copy link
Copy Markdown

Merge Queue Status

This pull request spent 10 minutes 16 seconds in the queue, including 7 minutes 43 seconds running CI.

Required conditions to merge
  • check-success=test-suite-success

mergify Bot added a commit that referenced this pull request Jun 10, 2026
@mergify
mergify Bot merged commit 5eab764 into sigp:epbs Jun 10, 2026
24 checks passed
@mergify mergify Bot removed the queued label Jun 10, 2026
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.

4 participants