feat: implement deferred state updates for message validation

[diegomrsantos]

Issue Addressed

Improves message validation architecture by separating validation logic from state mutation.

Proposed Changes

1. Add StateUpdate types - New types to capture deferred state changes:

   • StateUpdate enum with None, Consensus, and PartialSignature variants
   • ConsensusStateUpdate and PartialSignatureStateUpdate structs

2. Add apply methods - New methods to apply deferred state updates:

   • DutyState::apply_consensus_update()
   • DutyState::apply_partial_signature_update()

3. Refactor validation - Validation functions now return (ValidatedSSVMessage, StateUpdate) instead of mutating state directly

4. Explicit commit - Validator::commit() method to apply state updates when processing

Motivation

Validation should be a pure operation that doesn't produce side effects. By returning a StateUpdate instead of mutating state during validation, callers have explicit control over when state changes are applied. This makes the code easier to reason about and test.

Additional Info

• All existing tests pass
• The old update_* methods are kept for test compatibility
• Added #[must_use] to StateUpdate to prevent accidental omission of commit

[claude-code-actions-sigp]

Claude finished @diegomrsantos's task in 1m 41s —— View job

---

Code Review Complete

I've completed a comprehensive review of PR #777. This PR implements deferred state updates for message validation and includes fork infrastructure (which appears to be a prerequisite for the validation changes).

---

Architecture & Design

Strengths:

1. Clean Separation of Concerns: The separation of validation from state mutation is well-executed. The validation functions now return (ValidatedSSVMessage, StateUpdate) instead of mutating state directly, making the code more testable and easier to reason about.

2. Fork Transition Support: The PR description mentions fork transitions as motivation, and the implementation correctly allows messages to be validated multiple times (on different topics) but only have state committed once. This is demonstrated in message_receiver/src/manager.rs:178-181.

3. Type Safety: The #[must_use] attribute on StateUpdate (line 24 of state_update.rs) is excellent - it prevents accidental omission of the commit call at compile time.

4. Explicit Commit Point: The Validator::commit() method (line 398 in lib.rs) provides a clear, explicit point where state changes are applied, improving control flow visibility.

---

Code Quality Issues

1. Missing Error Handling in apply_partial_signature_update

Location: duty_state.rs:128-151

The update_for_partial_signature method returns Result<(), ValidationFailure> (line 80), but the new apply_partial_signature_update method doesn't return a Result. This is inconsistent and could hide errors.

Issue: If state creation fails in the apply method, the error is silently ignored, whereas the old method would have returned an error.

// Current code (line 128-151)
pub(crate) fn apply_partial_signature_update(&mut self, update: &PartialSignatureStateUpdate) {
    let operator_state = self.get_or_create_operator(&update.signer);
    // ... state creation that could conceptually fail
}

Recommendation: Either:

• Make apply_* methods infallible by design (document why), or
• Return Result to match the pattern of update_* methods

2. Code Duplication Between update and apply_* Methods

Locations:

• duty_state.rs:58-76 vs duty_state.rs:117-122
• duty_state.rs:80-110 vs duty_state.rs:128-151
• duty_state.rs:245-267 vs duty_state.rs:272-291

The logic is nearly identical between the old update_* methods and new apply_* methods. This creates maintenance burden.

Example:

// update (lines 58-76) and apply_consensus_update (lines 117-122) do the same thing
// except one takes SignedSSVMessage/QbftMessage and the other takes ConsensusStateUpdate

Recommendation: The PR description states "The old update_* methods are kept for test compatibility" but this creates technical debt. Consider:

1. Updating all tests to use the new pattern
2. Removing the old methods
3. Or clearly document that old methods are deprecated with #[deprecated] attributes

3. Fork Infrastructure Scope Concern

Location: Multiple files in anchor/common/fork/

The PR adds significant fork infrastructure (~1000 lines):

• anchor/common/fork/ - entire new crate
• YAML fork schedule configuration files
• Fork monitoring service

Issue: The PR description focuses solely on "deferred state updates for message validation" with no mention of fork infrastructure. This makes the PR harder to review and understand.

Recommendation:

• Either split fork infrastructure into a separate PR, or
• Update the PR description to explicitly document the fork changes and explain why they're included together

---

Testing

Strengths:

1. Existing tests pass according to PR description
2. Tests cover key validation scenarios (duty_state.rs:415-518, consensus_message.rs:479-1434, partial_signature.rs:278-1159)

Gaps:

1. No tests for the new commit() method (lib.rs:398-410)
2. No tests demonstrating the fork transition use case mentioned in the PR description
3. No tests verifying #[must_use] works as intended (though this is enforced at compile time)
4. No tests for apply_* methods directly - they're only exercised indirectly

Recommendation: Add tests demonstrating:

• Calling validate() multiple times without commit() doesn't duplicate state
• Calling commit() correctly applies state updates
• Fork transition scenario (validate on multiple topics, commit once)

---

Security Considerations

No Security Issues Found

The changes maintain existing security properties:

• Signature verification still occurs before state updates
• Validation logic unchanged
• State mutation is deferred but not bypassed

---

Performance

Potential Concern: State updates are now cloned when creating ValidatedMessage (lib.rs:389):

ValidatedMessage::new(signed_ssv_message.clone(), validated, state_update)

Impact: Minimal - StateUpdate variants are lightweight (mostly primitive types and small vectors), and this only happens for valid messages.

---

Documentation

Strengths:

1. Excellent module-level documentation in state_update.rs:1-12 explaining the why
2. Clear comments at the commit point (manager.rs:178-180)
3. Good inline comments explaining fork transition reasoning

Gaps:

1. No documentation update for the changed validation architecture
2. Validator::commit() method lacks doc comments explaining when/why to call it
3. Missing examples of the new validation + commit pattern

Recommendation: Add rustdoc examples showing:

/// # Example
/// ```
/// let result = validator.validate(&message_data);
/// if let ValidationResult::Success(validated) = result {
///     // Only commit if we're processing the message
///     validator.commit(&msg_id, &validated.state_update);
/// }
/// ```

---

Specific Recommendations

1. HIGH PRIORITY: Clarify PR scope - either split fork infrastructure to separate PR or update description to explain it

2. MEDIUM PRIORITY: Add tests for Validator::commit() method and fork transition scenarios

3. MEDIUM PRIORITY: Remove deprecated update_* methods or add #[deprecated] attributes

4. LOW PRIORITY: Make error handling consistent between update_* and apply_* methods

5. LOW PRIORITY: Add rustdoc examples demonstrating the new validation + commit pattern

---

Summary

This is a well-designed refactoring that successfully separates validation from state mutation. The core implementation is solid with good type safety and clear intent. The main concerns are:

• Scope clarity: Fork infrastructure isn't mentioned in PR description
• Test coverage: Missing tests for the new commit() method
• Code duplication: Old and new update methods coexist

The changes are safe to merge once the scope question is addressed and test coverage is improved.

Verdict: Approve with recommendations for follow-up improvements.

---