feat(qbft_manager): wire PTC committee-scoped QBFT instances and fork gate

[shane-moore]

Problem, Evidence, and Context

Closes #1035. PR #1033 (issue #1032) added Role::PTCCommittee to qbft_manager::receive_data as a placeholder arm with a //todo(cstar) marker that rejected all PTC traffic. PR #1047 (issue #1034) added PayloadAttestationVote with QbftData + value checker in ssv_types. This PR completes the qbft_manager wire-up so the downstream issue #1037 (sign_payload_attestation) can compile against consensus.decide_instance::<PayloadAttestationVote>(...).

Per SIP-94 §3, PTC runs one QBFT instance per cluster per slot over PayloadAttestationVote. The manager's existing four-role pattern (Committee, AggregatorCommittee, Proposer, plus a placeholder for PTCCommittee) gets the fourth role wired in here.

Change Overview

Direct structural mirror of the Role::AggregatorCommittee precedent added in PR #1033, substituting Fork::CStar for Fork::Boole and PayloadAttestationVote for AggregatorCommitteeConsensusData<E>. Single-crate change (qbft_manager/), +215/-6 across 3 files.

Reading order: start at the replaced routing arm in lib.rs and compare against the AggregatorCommittee template it mirrors immediately above it. Then look at the four supporting edits (new PTCCommitteeInstanceId type, new map field on QbftManager, constructor init, cleaner .retain entry, QbftDecidable for PayloadAttestationVote impl alongside the other three impls). Finally tests/ptc_tests.rs for two fork-gating tests mirroring tests/aggregator_tests.rs::test_aggregator_committee_rejected_before_boole and tests.rs::manager_tests::test_aggregator_committee_accepted_after_boole.

Intentionally unchanged:

• Existing three routing arms and their cleaner entries.
• message_validator/src/lib.rs already rejects PTC before CStar; this PR's gate is defense-in-depth.
• No changes to PayloadAttestationVote or its validator (both shipped in feat(ssv_types): add PayloadAttestationVote with QbftData and value checker #1047).

Risks, Trade-offs, and Mitigations

Defense-in-depth fork gate duplication. The < Fork::CStar check duplicates the message_validator gate. Same intentional duplication exists for < Fork::Boole on AggregatorCommittee; not a new pattern.

Test scope is tight. Two new fork-gating tests cover the security boundary. No end-to-end consensus test for PTC, no cleaner eviction test. These are symmetric gaps across all four routing arms; scope-bounded here to avoid cross-cutting test refactoring. Test boilerplate is duplicated across the AggregatorCommittee precedent and the new PTC tests but is small and not separately tracked.

Stub-removal completeness. Two markers from #1033 are gone after this PR: the //todo(cstar) line comment and the placeholder warn!(%slot, "Ignoring PTCCommittee message; routing not wired"). Verified by grep.

Validation

• cargo build -p qbft_manager --tests — clean.
• make cargo-fmt && make cargo-fmt-check — clean.
• make lint (clippy) — clean.
• cargo test -p qbft_manager — 18/18 pass (16 prior + 2 new PTC tests: test_ptc_committee_rejected_before_cstar and test_ptc_committee_accepted_after_cstar).
• Stub-removal greps return zero hits in anchor/qbft_manager/.

Rollback

Pure additive change behind Fork::CStar. Pre-CStar behavior is bit-for-bit unchanged (the new arm returns Err(QbftError::RoleNotActive) before any state mutation; the new map stays empty until the fork activates). Downstream consumer #1037 has not landed, so revert pre-CStar is a no-op.

Blockers / Dependencies

None for merge. Downstream:

• feat(validator_store): implement sign_payload_attestation #1037 feat(validator_store): implement sign_payload_attestation — first consumer of PTCCommitteeInstanceId + the QbftDecidable for PayloadAttestationVote impl shipped here.
• feat(client): spawn LH PayloadAttestationService with AnchorValidatorStore backend #1038 feat(client): spawn LH PayloadAttestationService — downstream of feat(validator_store): implement sign_payload_attestation #1037, not blocked by this PR.

Additional Info / Next Steps

N/A

[shane-moore]

@claude, please take a look

[claude-code-actions-sigp]

Claude finished @shane-moore's task in 2m 48s —— View job

---

Review: PR #1057 — feat(qbft_manager): wire PTC committee-scoped QBFT instances and fork gate

• Read PR diff against origin/epbs
• Inspect qbft_manager/src/lib.rs changes (routing arm, new map, QbftDecidable impl)
• Inspect new tests/ptc_tests.rs and tests.rs changes
• Verify symmetry with AggregatorCommittee precedent
• Check for stub/marker removal completeness

---

Summary

This is a clean, low-risk structural mirror of the Role::AggregatorCommittee precedent (PR #1033) with Fork::CStar substituted for Fork::Boole and PayloadAttestationVote substituted for AggregatorCommitteeConsensusData<E>. The four-role routing pattern is now symmetric across Committee / AggregatorCommittee / PTCCommittee, plus the Validator-scoped Proposer arm. No blockers.

Verified

• Symmetry with AggregatorCommittee precedent. Routing arm at anchor/qbft_manager/src/lib.rs:343-365 is a faithful mirror of the AggregatorCommittee arm at lib.rs:320-342 — same slot→epoch derivation, same < Fork::X comparator, same RoleNotActive short-circuit before any state mutation, same pass_to_instance::<D>(id, ...) shape.
• Cleaner entry added. payload_attestation_vote_instances.retain(...) at lib.rs:417-418 is parallel with the other three maps and uses the same cutoff derivation.
• QbftDecidable<E> for PayloadAttestationVote at lib.rs:543-560 follows the same shape as the BeaconVote and AggregatorCommitteeConsensusData<E> impls — MessageId::new(domain, Role::PTCCommittee, &DutyExecutor::Committee(id.committee)) is the correct triple per msgid::MessageId::new semantics.
• PTCCommittee is correctly excluded from the Validator arm. lib.rs:283 already lists Role::PTCCommittee alongside Role::Committee | Role::AggregatorCommittee as committee-scoped roles that must use DutyExecutor::Committee. No change needed.
• Stub markers fully removed. Greps for todo(cstar), TODO(cstar), and the placeholder warn! string return zero hits in anchor/qbft_manager/.
• Fork gate matches the message_validator gate. The check active_fork(epoch) < Fork::CStar mirrors message_validator/src/lib.rs:863 (role == Role::PTCCommittee && active_fork < Fork::CStar), so this is genuine defense-in-depth at the qbft_manager boundary, not divergent policy.
• Fork::CStar exists and orders correctly. common/fork/src/fork.rs:42 defines CStar, and the test at fork.rs:98 already asserts Fork::Boole < Fork::CStar, so the < comparison in the new gate is well-defined.
• Test parity. tests/ptc_tests.rs::test_ptc_committee_rejected_before_cstar mirrors aggregator_tests::test_aggregator_committee_rejected_before_boole (uses Fork::Boole as the "before" fork — correct since Boole < CStar). test_ptc_committee_accepted_after_cstar mirrors tests::manager_tests::test_aggregator_committee_accepted_after_boole.

Observations (non-blocking)

1. Test-boilerplate duplication is acknowledged. ptc_tests.rs is nearly verbatim against aggregator_tests.rs and the inline tests.rs accept-test. The PR explicitly defers extraction to issue refactor: clean up partial_signature.rs validation helpers and tests #1039; that's a reasonable scope boundary, especially since the duplication is symmetric across all four routing arms.

2. The "accepted" test's negative-assertion pattern is intentionally weak. assert!(!matches!(result, Err(QbftError::RoleNotActive))) will pass on any non-RoleNotActive outcome, including an Ok(()) that does nothing useful. This is the same shape as the aggregator precedent, so changing it here would create test asymmetry — better tracked alongside refactor: clean up partial_signature.rs validation helpers and tests #1039 if tightened.

3. E::slots_per_epoch() vs the manager's slots_per_epoch field. The new arm calls slot.epoch(E::slots_per_epoch()), identical to the aggregator arm at lib.rs:323. The manager carries a separate slots_per_epoch: NonZeroU64 field set in the constructor; nothing reads it on this path. Pre-existing pattern, not introduced here, but worth a follow-up: either route this through self.slots_per_epoch or remove the field if it's truly unused on the hot path. (Out of scope for this PR.)

4. Comment casing nit. // Unique Identifier for a PTC committee QBFT instance (lib.rs:75) capitalizes "Identifier" mid-sentence, matching the existing AggregatorCommitteeInstanceId and ProposerInstanceId comments — so it's consistent, just inherited oddness.

Verdict

Wire-up is correct, scope is tight, and the security boundary (pre-CStar rejection) has direct test coverage with the same shape as the AggregatorCommittee gate. No changes requested.
· feat/qbft-manager-ptc-wireup

[codecov-commenter]

Codecov Report

❌ Patch coverage is 63.33333% with 11 lines in your changes missing coverage. Please review.
⚠️ Please upload report for BASE (epbs@b9aa7ea). Learn more about missing BASE report.

Files with missing lines	Patch %	Lines
anchor/qbft_manager/src/lib.rs	63.33%	11 Missing ⚠️

Additional details and impacted files

@@           Coverage Diff           @@
##             epbs    #1057   +/-   ##
=======================================
  Coverage        ?   62.04%           
=======================================
  Files           ?      156           
  Lines           ?    26265           
  Branches        ?        0           
=======================================
  Hits            ?    16297           
  Misses          ?     9968           
  Partials        ?        0           

Flag	Coverage Δ
rust	62.04% <63.33%> (?)

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.

[shane-moore]

@claude, pushed c6dfd2c8 for obs 2: stronger accept-test assertion with positive is_ok() plus a payload_attestation_vote_instances.contains_key(...) + len() == 1 check to prove the message reached the PTC arm specifically. Filed #1058 to apply the same shape to the AggregatorCommittee precedent so the symmetry stays consistent.

On obs 3 (E::slots_per_epoch() vs self.slots_per_epoch): the field is actually used, just on a different path (decide_instance):

• lib.rs:222: Epoch::new(*instance_height as u64 / self.slots_per_epoch)
• lib.rs:224: DefaultLeaderFunction::new(self.slots_per_epoch, include_epoch_shift)

The field is typed NonZeroU64 because DefaultLeaderFunction::new requires it; removing it would break leader-function construction. The stylistic mix (u64 for slot.epoch(...), NonZeroU64 for the leader fn) is load-bearing, not vestigial, so no follow-up needed.

Obs 1 (test boilerplate) and obs 4 (comment casing) acknowledged as inherited from the AggregatorCommittee precedent; not introduced by this PR.

[claude-code-actions-sigp]

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

---

Re-review of c6dfd2c8 and follow-ups

• Read trigger comment
• Verify obs 2 strengthening in c6dfd2c8
• Verify obs 3 claims about self.slots_per_epoch usage at lib.rs:222 and lib.rs:224
• Confirm acknowledgments of obs 1 and obs 4
• Post final response

---

Verified

Obs 2 — accept-test strengthening (c6dfd2c8). Confirmed at tests/ptc_tests.rs:155-173:

• Positive assert!(result.is_ok(), ...) replaces the prior !matches!(Err(RoleNotActive)) negative form.
• payload_attestation_vote_instances.contains_key(&expected_id) proves the PTC routing arm specifically populated its map at (CommitteeId([0; 32]), InstanceHeight::from(100usize)), not just "some non-RoleNotActive outcome."
• len() == 1 rules out cross-map leakage (e.g., a regression that double-routed into both aggregator_committee_instances and payload_attestation_vote_instances).

This is a meaningful tightening: the prior assertion would have passed on a silent fall-through Ok(()), an unrelated Err, or even a wrong-map insertion. The new shape can only pass if the message reached the PTC arm and spawned the correct instance.

Obs 3 — self.slots_per_epoch is load-bearing. Confirmed by grep at lib.rs:155, 169, 180, 222, 224:

155:    slots_per_epoch: NonZeroU64,
222:        let epoch = Epoch::new(*instance_height as u64 / self.slots_per_epoch);
224:        let leader_fn = DefaultLeaderFunction::new(self.slots_per_epoch, include_epoch_shift);

The field is genuinely used on the decide_instance path (lines 222, 224). The NonZeroU64 typing is required because DefaultLeaderFunction::new takes NonZeroU64. Meanwhile slot.epoch() on the receive_data path takes a plain u64, which is what E::slots_per_epoch() (the EthSpec const) returns. So the apparent stylistic mix is two different API requirements, not vestigial code. Withdrawn — my prior "out of scope, possible follow-up" framing was wrong; there's nothing to follow up on.

Obs 1 and obs 4. Acknowledged as pre-existing patterns inherited from the AggregatorCommittee precedent, not introduced by this PR. #1058 is the right place to track the symmetric tightening of the aggregator accept test.

Verdict

c6dfd2c8 materially improves the security-boundary test. Obs 3 was an incorrect read on my part — the field is correctly used and correctly typed. No further changes requested. PR remains ready to merge.
· feat/qbft-manager-ptc-wireup

[jnhsigmap]

Looks good after a bit of reading 🙂, just one point that came up - we could test the fork-boundary to ensure that QbftManager rejects PTC traffic where CStar is activated at a non-zero epoch? We cover accepting at CStar but not rejecting prior.

Suggested change

	}
	}
	/// Test that at a non-zero CStar activation epoch the qbft_manager
	/// routing arm rejects PTC traffic at the last pre-fork slot (i.e. to catch off-by-one bugs).
	#[tokio::test]
	async fn test_ptc_committee_rejected_at_last_pre_cstar_slot() {
	use std::collections::BTreeMap;
	use ssv_types::{
	RSA_SIGNATURE_SIZE,
	message::{MsgType, SSVMessage, SignedSSVMessage},
	};
	use ssz::Encode;
	use types::{Epoch, MainnetEthSpec};
	const CSTAR_ACTIVATION_EPOCH: u64 = 5;
	let setup = setup_test(1);
	// CStar pinned to non-zero epoch.
	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 valid");
	let senders = processor::spawn(
	processor::Config {
	max_workers: 4,
	queue_size: Default::default(),
	},
	setup.executor,
	);
	let (network_tx, _network_rx) = mpsc::unbounded_channel();
	let spe = MainnetEthSpec::slots_per_epoch();
	let manager = QbftManager::<MainnetEthSpec, _>::new(
	senders,
	OperatorId(1).into(),
	setup.clock,
	Arc::new(MockMessageSender::new(network_tx, OperatorId(1))),
	NonZeroU64::new(spe).expect("slots_per_epoch is non-zero"),
	Arc::new(schedule),
	)
	.expect("manager creation");
	let last_pre_cstar_slot = CSTAR_ACTIVATION_EPOCH * spe - 1;
	let msg_id = MessageId::new(
	&DomainType([0; 4]),
	Role::PTCCommittee,
	&DutyExecutor::Committee(CommitteeId([0; 32])),
	);
	let qbft_message = QbftMessage {
	qbft_message_type: QbftMessageType::Proposal,
	height: last_pre_cstar_slot,
	round: 1,
	identifier: (&msg_id).into(),
	root: Hash256::from([0u8; 32]),
	data_round: 1,
	round_change_justification: ssv_types::VariableList::empty(),
	prepare_justification: ssv_types::VariableList::empty(),
	};
	let signed = SignedSSVMessage::new(
	vec![[0xAA; RSA_SIGNATURE_SIZE]],
	vec![OperatorId(1)],
	SSVMessage::new(
	MsgType::SSVConsensusMsgType,
	msg_id,
	qbft_message.as_ssz_bytes(),
	)
	.expect("SSVMessage creation"),
	vec![],
	)
	.expect("SignedSSVMessage creation");
	let result = manager.receive_data(signed, qbft_message);
	assert!(
	matches!(result, Err(QbftError::RoleNotActive)),
	"last pre-CStar slot {last_pre_cstar_slot} (epoch {}) must reject PTC message.",
	last_pre_cstar_slot / spe,
	);
	}

[petarjuki7]

Lgtm!

[shane-moore]

closing for now since we're going to move to a non-qbft ptc approach