feat: reject deprecated roles after Boole fork

[shane-moore]

Issue Addressed

Feature parity with ssv-go, catching up with their recent commits. This PR supports #770.

Proposed Changes

Reject deprecated Role::Aggregator and Role::SyncCommittee messages after the Boole fork activates. SSV-Go's
validRoleAtSlot() rejects these roles post-Boole, so Anchor must do the same for P2P compatibility.

• Add RoleNotActiveAfterFork validation error
• Add fork-aware validation in consensus message and partial signature handlers

Additional Info

N/A

[shane-moore]

@claude, feel free to take a look

[dknopik]

LGTM!

[dknopik]

Actually - can we deduplicate this by handling it somewhere else?

changed my mind

[dknopik]

I am not sure, but maybe we should use the fork indicated by the domain type instead?

[shane-moore]

investigate a bit. seems we want to use the message's slot as the source of truth for determining which fork's validation rules apply

the DomainType we derive from a message id is actually something we validate against the expected fork derived from the slot in validate_topic_and_domain

anchor/anchor/message_validator/src/lib.rs

Lines 608 to 632 in c5201af

	// Validate domain against the fork active at the message's slot
	let expected_domain = self
	.subnet_service
	.router()
	.domain_type_for_epoch(message_epoch)
	.ok_or_else(|| {
	debug!(
	?expected_fork,
	?message_epoch,
	"Unknown fork, cannot validate domain"
	);
	ValidationFailure::WrongDomain
	})?;
	let msg_domain = msg_id.domain();
	if msg_domain != expected_domain {
	debug!(
	?msg_domain,
	?expected_domain,
	fork = ?expected_fork,
	?message_slot,
	"Message domain does not match expected domain for slot's fork"
	);
	return Err(ValidationFailure::WrongDomain);
	}

we basically want to validate that the DomainType from the message isn't from an incorrect topic. but use slot across the board to validate against

[claude-code-actions-sigp]

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

---

Code Review Complete ✅

I've reviewed the PR that adds fork-aware validation to reject deprecated Role::Aggregator and Role::SyncCommittee messages after the Boole fork. The implementation is functionally correct and follows Rust best practices, but there's an important refactoring opportunity to address.

---

Critical: Code Duplication

The fork validation logic is duplicated identically in two places:

• consensus_message.rs:49-62
• partial_signature.rs:47-60

This violates DRY principle and makes maintenance harder. I've added inline comments on both locations. The code also calls fork_schedule.active_fork(epoch) multiple times with the same arguments.

Recommendation: Extract into a helper function in lib.rs:

fn validate_role_active_after_fork(
    role: Role,
    slot: Slot,
    slots_per_epoch: u64,
    fork_schedule: &ForkSchedule,
) -> Result<(), ValidationFailure> {
    if matches!(role, Role::Aggregator | Role::SyncCommittee) {
        let epoch = slot.epoch(slots_per_epoch);
        let current_fork = fork_schedule.active_fork(epoch);
        if current_fork >= Fork::Boole {
            return Err(ValidationFailure::RoleNotActiveAfterFork {
                role,
                current_fork,
                deprecated_since_fork: Fork::Boole,
            });
        }
    }
    Ok(())
}

Then call it from both locations:

validate_role_active_after_fork(
    validation_context.role,
    slot,
    validation_context.slots_per_epoch,
    &validation_context.fork_schedule,
)?;

This addresses @dknopik's comment about deduplication.

---

Review Comment: Domain Type vs Slot-Based Fork

Regarding @dknopik's question about using the fork indicated by domain type:

The current slot-based approach (slot → epoch → fork lookup) is correct and consistent with SIP-43 validation patterns used elsewhere in the codebase. The domain type in the message should already match the slot's fork (validated by validate_topic_and_domain in lib.rs:608-634), so this is architecturally sound. Using slot-based lookup is the right choice here.

---

Other Findings

Positive:

• ✅ No .unwrap() or .expect() without justification
• ✅ Proper error handling with Result<T, ValidationFailure>
• ✅ Good test coverage with both message types tested
• ✅ Memory safe - no unsafe code, proper borrowing
• ✅ Clear error messages and type safety

Suggestions:

1. Missing boundary test: Add a test that validates the exact fork boundary (last slot before Boole vs first slot at Boole)
2. Asymmetry in AggregatorCommittee checks: The "safety net" check for AggregatorCommittee only validates before Boole (lines 36-47), but has no after-fork check. Consider adding a comment explaining this is intentional since AggregatorCommittee is the Boole replacement role.

---

Summary

The code is functionally correct and follows Rust best practices. The primary issue is code duplication which should be addressed before merging. Once refactored, this is production-ready.

Recommendation: Extract the duplicated validation logic into a helper function as shown above.

[claude-code-actions-sigp]

Code Duplication: This validation logic is duplicated in partial_signature.rs:47-60. Consider extracting to a helper function validate_role_active_after_fork() in lib.rs to improve maintainability.

Also, cache the active_fork(epoch) result - it's currently called 3 times (lines 40, 54, 58) with the same arguments:

let epoch = slot.epoch(validation_context.slots_per_epoch);
let current_fork = validation_context.fork_schedule.active_fork(epoch);
if current_fork >= Fork::Boole {
    return Err(ValidationFailure::RoleNotActiveAfterFork {
        role: validation_context.role,
        current_fork,
        deprecated_since_fork: Fork::Boole,
    });
}

[claude-code-actions-sigp]

Code Duplication: This validation logic is duplicated from consensus_message.rs:49-62. Extract to a shared helper function to avoid maintaining logic in two places.

Also cache active_fork(epoch) - it's called 3 times with same arguments (lines 38, 52, 56).

[shane-moore]

@claude, please take a look after 7823568

[claude-code-actions-sigp]

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

---

Code Review Complete ✅

I've reviewed the PR after commit 7823568 and the code duplication issue has been successfully addressed!

---

Excellent Refactoring

The duplicate fork validation logic has been extracted into a helper function validate_role_for_fork() in lib.rs:789-824. Both consensus messages and partial signatures now call this shared function:

• consensus_message.rs:38 calls validate_role_for_fork(slot, &validation_context)?
• partial_signature.rs:36 calls validate_role_for_fork(messages.slot, &validation_context)?

This is exactly what was recommended and eliminates the maintenance burden of duplicated logic.

---

Review Findings

Safety & Best Practices:

• ✅ No .unwrap() or .expect() without justification
• ✅ Proper error handling with Result<T, ValidationFailure>
• ✅ Memory safe - no unsafe code, proper borrowing
• ✅ Clear error messages with context (RoleNotActiveAfterFork includes role, current_fork, and deprecated_since_fork)
• ✅ Type safety - uses enum matching for role validation

Correctness:

• ✅ Slot-based fork validation is correct per SIP-43 (Shane's explanation at consensus_message.rs:? is accurate)
• ✅ Both deprecated roles (Aggregator, SyncCommittee) rejected after Boole
• ✅ AggregatorCommittee correctly rejected before Boole (safety net check)
• ✅ Fork lookups use cached active_fork(epoch) results efficiently

Test Coverage:

• ✅ Comprehensive test coverage for both message types:
  • test_aggregator_consensus_message_rejected_after_boole (consensus_message.rs:1721)
  • test_aggregator_partial_sig_rejected_after_boole (partial_signature.rs:1673)
• ⚠️ Missing boundary test: No test validates the exact fork boundary (last slot before Boole vs first slot at Boole)

---

Minor Suggestions

1. Boundary Test - Consider adding a test that validates messages at the exact fork transition:

   #[test]
   fn test_deprecated_roles_at_fork_boundary() {
       // Test last slot of Alan (should accept Aggregator)
       // Test first slot of Boole (should reject Aggregator)
   }

2. Documentation Clarity - The comment at lib.rs:801-809 explains the "safety net" for AggregatorCommittee before Boole. Consider adding a mirror comment at lib.rs:812-821 explaining why there's no corresponding check for AggregatorCommittee after Boole (it's the replacement role).

---

Summary

This implementation is production-ready. The refactoring successfully addressed the code duplication concern while maintaining correctness and adding comprehensive test coverage. The slot-based validation approach is architecturally sound and aligns with SIP-43 validation patterns.

Recommendation: ✅ Ready to merge (optional: add boundary test for extra confidence)

---

[shane-moore]

@claude, i think we can do without hte boundary tests. there's no complex boundary behavior happening, i..e no intermediate states that could fail at transitions. and the ForkSchedule already has its own unit tests for epoch to fork mapping logic

[claude-code-actions-sigp]

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

---

Agreed - Boundary Tests Not Required ✅

You're absolutely right, Shane. After reviewing the code:

1. The validation logic is simple and deterministic:

• lib.rs:812-821 uses a straightforward comparison: active_fork(epoch) >= Fork::Boole
• No intermediate states, no complex branching - it's a pure boolean check
• The fork boundary is handled atomically by ForkSchedule.active_fork()

2. ForkSchedule's unit tests already cover the boundary:

• schedule.rs:282-296 tests active_fork() at epochs 99 (Alan) and 100 (Boole)
• schedule.rs:319-333 tests the preparation window boundary behavior
• These tests prove the epoch→fork mapping works correctly at transitions

3. Your existing tests are sufficient:

• test_aggregator_consensus_message_rejected_after_boole (consensus_message.rs:1721)
• test_aggregator_partial_sig_rejected_after_boole (partial_signature.rs:1673)
• Both create schedules with Fork::Boole active and verify rejection
• The actual boundary behavior is delegated to ForkSchedule, which is already tested

Verdict: The separation of concerns is clean - ForkSchedule handles epoch-to-fork mapping (with boundary tests), and your validation logic simply uses that result. Adding redundant boundary tests here would just duplicate coverage that already exists in schedule.rs.

---

Final Review Summary

✅ Production Ready

The refactoring successfully eliminated code duplication, and the implementation is architecturally sound. The slot-based validation approach aligns with SIP-43 patterns, and test coverage is comprehensive without being redundant.

What was validated:

• ✅ No safety issues (proper error handling, no .unwrap())
• ✅ Correct fork validation logic with role deprecation
• ✅ Efficient implementation (caches active_fork() results)
• ✅ Clear error messages with context
• ✅ Appropriate test coverage (delegates boundary testing to ForkSchedule)

Recommendation: Ready to merge.

---