Validate qbft logic by diegomrsantos · Pull Request #191 · sigp/anchor

diegomrsantos · 2025-03-14T17:49:46Z

Issue Addressed

Implements https://github.com/ssvlabs/ssv/blob/main/message/validation/consensus_validation.go#L172

Proposed Changes

Copilot

Pull Request Overview

This PR focuses on validating and enforcing QBFT consensus logic while integrating a new Round type and slot_clock dependency into various modules. Key changes include:

Introducing a new Round type in ssv_types and removing the embedded Round implementation from qbft_types.
Updating consensus validation logic to use slot_clock and improve state management via consensus_state.
Adding and updating tests and dependency configurations across several crates.

Reviewed Changes

Copilot reviewed 19 out of 19 changed files in this pull request and generated 1 comment.

Show a summary per file

File	Description
anchor/common/ssv_types/src/round.rs	Introduces a new Round type backed by NonZeroUsize
anchor/message_validator/src/consensus_state.rs	Implements consensus state management for operators
anchor/message_validator/src/consensus_message.rs	Updates consensus validation logic and round validation functions
anchor/message_validator/src/message_counts.rs	Adds message count tracking and limits validation
Others (Cargo.toml, network, qbft modules, etc.)	Update dependency and parameter passing for slot_clock integration

anchor/message_validator/src/message_counts.rs

Copilot

Pull Request Overview

This PR refactors and improves the QBFT logic by updating consensus message validation and round handling, and by integrating a generic slot clock for improved time‐based validations.

Introduces a new Round type in the ssv_types module and refactors related QBFT types accordingly.
Enhances consensus message validation with detailed round and leader checks using the slot_clock interface.
Updates multiple modules (validator, network, message_receiver) to use a generic SlotClock and improved consensus state management.

Reviewed Changes

Copilot reviewed 21 out of 21 changed files in this pull request and generated 1 comment.

Show a summary per file

File	Description
anchor/common/ssv_types/src/round.rs	Introduces a new Round type wrapping NonZeroUsize for round safety.
anchor/message_validator/src/message_counts.rs	Adjusts message count validations with updated error reporting.
anchor/message_validator/src/consensus_state.rs	Refactors operator state management for consensus message updates.
anchor/message_validator/src/consensus_message.rs	Implements new QBFT logic with round estimation, leader selection, and time‐based validations.
anchor/message_validator/src/lib.rs	Updates error variants and integrates consensus state handling.
anchor/message_receiver/src/lib.rs & manager.rs	Renames and refactors the MessageReceiver into a generic NetworkMessageReceiver.
anchor/message_sender/src/network.rs	Adapts NetworkMessageSender to use a generic SlotClock for time-based operations.
Various Cargo.toml updates	Adds dependencies (slot_clock, dashmap, parking_lot) and ensures consistent integration.
anchor/common/qbft/src/qbft_types.rs	Removes the duplicate Round implementation in favor of the new one.
anchor/network/src/discovery.rs	Minor adjustments to error mappings and ENR building logic.
anchor/common/qbft/src/config.rs	Adjusts usage of the Round type in QBFT configuration.

Comments suppressed due to low confidence (1)

anchor/message_sender/src/network.rs:22

[nitpick] The introduction of the generic parameter S: SlotClock improves flexibility, but consider adding documentation or more explicit bounds to clarify the expected behavior and constraints for implementers of SlotClock.

pub struct NetworkMessageSender<S: SlotClock> {

anchor/message_validator/src/consensus_message.rs

Co-authored-by: Copilot <175728472+Copilot@users.noreply.github.com>

Copilot

Copilot encountered an error and was unable to review this pull request. You can try again by re-requesting a review.

Copilot

Pull Request Overview

This PR refines the QBFT logic validation and consensus state management while integrating a dedicated Round type and the slot_clock dependency. Key changes include:

Added a new Round type in anchor/common/ssv_types/src/round.rs to encapsulate round logic.
Extended and integrated consensus state and QBFT validation in message validator modules with enhanced error details.
Updated network, message receiver, and message sender modules to propagate the slot_clock and updated validator interfaces.

Reviewed Changes

Copilot reviewed 21 out of 21 changed files in this pull request and generated 2 comments.

Show a summary per file

File	Description
anchor/common/ssv_types/src/round.rs	Introduces the Round type with conversion, defaulting, and arithmetic operators.
anchor/message_validator/src/consensus_state.rs	Adds consensus state management and updates operator/signer state handling.
anchor/message_validator/src/consensus_message.rs	Integrates new QBFT logic, including round validation based on slot timing.
anchor/message_validator/src/lib.rs	Updates the ValidationFailure enum and validator integration to support new consensus logic.
anchor/message_receiver/src/lib.rs	Refactors and renames the message receiver to NetworkMessageReceiver with a trait implementation.
anchor/client/src/lib.rs, anchor/message_sender/src/network.rs, anchor/network/*	Updates integration points to pass slot_clock and updated validator instances.

anchor/common/ssv_types/src/round.rs

anchor/message_validator/src/consensus_state.rs

# Conflicts: # anchor/client/src/lib.rs

Copilot

Pull Request Overview

This PR refactors and extends the consensus validation and message handling logic, incorporating enhanced QBFT logic, a generic SlotClock, and improved error contexts. Key changes include new modules for Round handling and ConsensusState, modifications to Validator and MessageReceiver to support a generic SlotClock, and updates to error variants and testing for consensus message validation.

Reviewed Changes

Copilot reviewed 21 out of 21 changed files in this pull request and generated 2 comments.

Show a summary per file

File	Description
anchor/common/ssv_types/src/round.rs	Adds a dedicated Round type with conversions and arithmetic.
anchor/message_validator/src/{message_counts,consensus_state,consensus_message,lib.rs}	Implements refined consensus state tracking, QBFT logic validation, and enhanced error variants.
anchor/message_receiver/src/{lib.rs,manager.rs}	Adapts the message receiver to use a trait and generic SlotClock.
anchor/message_sender/src/network.rs	Updates NetworkMessageSender to be generic over SlotClock.
Various Cargo.toml files and qbft modules	Introduces SlotClock dependency and removes redundant inlined Round type.

Comments suppressed due to low confidence (1)

anchor/message_validator/src/lib.rs:37

[nitpick] With the updated error variants (e.g. RoundAlreadyAdvanced now including fields), consider adding a brief code comment to clarify the use of wildcard matches for these variants to aid future developers’ readability.

| ValidationFailure::RoundAlreadyAdvanced { got: _, want: _ } => MessageAcceptance::Ignore,

anchor/common/ssv_types/src/round.rs

anchor/message_sender/src/network.rs

Zacholme7

Phew this was dense! Awesome job though this is great and super useful. Definitely should get @dknopik and @jking-aus eyes on this too.

Zacholme7 · 2025-03-26T20:21:24Z

anchor/message_validator/src/consensus_state.rs

+}
+
+/// Hash a list of operator IDs to create a unique identifier, using SHA-256
+fn hash_operators(operators: &Vec<OperatorId>) -> [u8; 32] {


nit: &Vec<OperatorId> to &[OperatorId]

Zacholme7 · 2025-03-26T20:32:48Z

anchor/message_validator/src/consensus_state.rs

+
+/// ConsensusState manages the state for consensus validation across operators and slots
+pub(crate) struct ConsensusState {
+    operators: HashMap<OperatorId, RefCell<OperatorState>>,


It looks like it all works good here, but this RefCell is very hard to reason about with all the clones and could cause some unexpected runtime panics.

I recommend we either really reason about this, also use a DashMap here, or use the HashMap Entry API more directly

Good catch, thanks. I removed the RefCell.

Sorry, unresolving: Now that we have the DashMap, is an Arc<Mutex<_>> really necessary?

This is a different map and there I need the Mutex cause I can't change the consensus state concurrently

anchor/message_validator/src/lib.rs

Zacholme7 · 2025-03-26T21:00:05Z

anchor/message_validator/src/lib.rs

+        self.consensus_state_map
+            .entry(message_id.clone())
+            .or_insert_with(|| {
+                // Create a new consensus state with storage for two epochs worth of slots


Is there anything that is cleaning up ConsensusState after two epochs have passed?

I'm not sure if this comment is correct. When does the consensus instance associated with the message IDs stop being active?

Im probably just confused. It doesn't stop being active, but my understanding from the comment was that a new fresh instance would be created every two epochs. What happens to the consensus state after two epochs pass and the storage is taken up?

The message id does not change between instance heights. The only case where we do not clean up is when we leave a committee or a validator is removed

This kind of inherits the confusing "message id" from go-ssv, which I hope to get rid of in a future SIP ;)

The comment is incorrect, I removed it. It's one ConsensusState per message id

Zacholme7 · 2025-03-26T21:01:46Z

anchor/message_validator/src/consensus_message.rs

+    if signers.len() == 1 {
+        validate_round_in_allowed_spread(consensus_message, received_at, slot_clock)?;
+    }
+


I think we can now remove some of these checks from qbft. separate issue

Do you remember where it is done?

I will have to take a look, would be good to group this into issue #182. Just a passover the whole pipeline to make sure we are not performing redundant work.

Zacholme7 · 2025-03-26T21:05:33Z

anchor/message_validator/src/lib.rs

-pub struct Validator {
+pub struct Validator<S: SlotClock> {
    network_state_rx: Receiver<NetworkState>,
+    consensus_state_map: DashMap<MessageId, Arc<Mutex<ConsensusState>>>,


nit: In terms of readability it might help to document the structure of the map. It goes very deep and has a lot of moving parts

Here is my understanding. Top level we have MessageID -> ConsensusState where ConsensusState holds all the state for Operators pertaining to MessageID.

OperatorState contains specific operator data for each slot via a circular buffer. For each slot, there is SignerState which represents signer info if this operator was a signer.

That's right. I'll add more docs in the next PR.

Zacholme7 · 2025-03-26T21:16:30Z

anchor/message_validator/src/consensus_state.rs

+    // Create a buffer (no sorting, to match Go implementation)
+    let mut buffer = Vec::new();
+
+    // Write each operator ID to the buffer in little endian order
+    for &operator in operators {
+        buffer.extend_from_slice(&operator.0.to_le_bytes());
+    }
+
+    // Hash the buffer with SHA-256
+    let mut hasher = Sha256::new();
+    hasher.update(&buffer);
+    hasher.finalize().into()


Is there any reason behind not sorting? Otherwise this is the same computation for CommitteeId

There's a validation checking this list is sorted, so I'm not sure sorting is necessary.

If the operators are sorted, this is a duplicate of computing CommitteeId. Can that just be used here?

anchor/anchor/common/ssv_types/src/committee.rs

Lines 26 to 40 in 045cb2c

impl From<Vec<OperatorId>> for CommitteeId {

fn from(mut operator_ids: Vec<OperatorId>) -> Self {

// Sort the operator IDs

operator_ids.sort();

let mut hasher = Sha256::new();

// Add the operator IDs as 32 byte values

for id in operator_ids {

hasher.update((*id as u32).to_le_bytes());

}

// Hash it all

<[u8; COMMITTEE_ID_LEN]>::from(hasher.finalize()).into()

}

}

Pushed a new version, let me know what you think

anchor/message_validator/src/message_counts.rs

dknopik

Nice! Some remarks, but mostly nitpicks.

Anyway, before we merge this I'd like to test this on interop when convenient.

dknopik · 2025-03-27T14:48:17Z

anchor/message_validator/src/lib.rs

            | ValidationFailure::LateSlotMessage
            | ValidationFailure::SlotAlreadyAdvanced
-            | ValidationFailure::RoundAlreadyAdvanced
+            | ValidationFailure::RoundAlreadyAdvanced { got: _, want: _ }


Nitpick: use .. instead of listing the fields, is a bit cleaner. Same below.

dknopik · 2025-03-27T14:53:20Z

anchor/message_receiver/src/manager.rs

    network_state_rx: watch::Receiver<NetworkState>,
    outcome_tx: mpsc::Sender<Outcome>,
-    validator: Validator,
+    validator: Validator<S>,


Now that the Validator actually has state, this needs to be an Arc, right? Or else the state of the receiver validator and sender validator diverges.

Good question. Do we want to share the consensus state? I think so.

dknopik · 2025-03-27T15:05:20Z

anchor/message_validator/src/consensus_message.rs

+        if signers.is_empty() {
+            return Err(ValidationFailure::NoSigners);
+        }
+
+        let signer = signers[0];


very very nitpick, but a pattern I like in these cases is

let Some(signer) = signers.first() else { return Err(ValidationFailure::NoSigners); }

to combine the check and retrieval.

Nice suggestion, thanks. Had to use:

let Some(&signer) = signers.first() else { return Err(ValidationFailure::NoSigners); };

dknopik · 2025-03-27T15:14:51Z

anchor/message_validator/src/consensus_message.rs

+    // Calculate the first round index
+    let first_round_index = if height != FIRST_HEIGHT {
+        // If not the first height, add height % len(committee) to the index
+        height % committee.len() as u64
+    } else {
+        0
+    };


I don't get this. 0 % anything is 0, so why the differentiation?

You're right, copied their approach and didn't catch this.

dknopik · 2025-03-27T15:27:27Z

anchor/message_validator/src/consensus_message.rs

+    let slot = Slot::new(consensus_message.height);
+    let slot_start_time = match slot_clock.start_of(slot) {
+        Some(time) => UNIX_EPOCH + time,
+        None => return Err(ValidationFailure::SlotStartTimeNotFound),
+    };


The attestation voting only starts one third into the round - so always starting to measure at the start of the slot seems incorrect

diegomrsantos

Thanks a lot, replied to most of the comments. I'll finish tomorrow

diegomrsantos · 2025-03-27T19:44:54Z

anchor/message_receiver/src/manager.rs

    network_state_rx: watch::Receiver<NetworkState>,
    outcome_tx: mpsc::Sender<Outcome>,
-    validator: Validator,
+    validator: Validator<S>,


Good question. Do we want to share the consensus state? I think so.

diegomrsantos · 2025-03-27T19:52:34Z

anchor/message_validator/src/consensus_message.rs

+        if signers.is_empty() {
+            return Err(ValidationFailure::NoSigners);
+        }
+
+        let signer = signers[0];


Nice suggestion, thanks. Had to use:

let Some(&signer) = signers.first() else { return Err(ValidationFailure::NoSigners); };

diegomrsantos · 2025-03-27T19:54:30Z

anchor/message_validator/src/consensus_message.rs

+    if signers.len() == 1 {
+        validate_round_in_allowed_spread(consensus_message, received_at, slot_clock)?;
+    }
+


diegomrsantos · 2025-03-27T20:03:08Z

anchor/message_validator/src/consensus_message.rs

+    // Calculate the first round index
+    let first_round_index = if height != FIRST_HEIGHT {
+        // If not the first height, add height % len(committee) to the index
+        height % committee.len() as u64
+    } else {
+        0
+    };


You're right, copied their approach and didn't catch this.

diegomrsantos · 2025-03-27T20:05:20Z

anchor/message_validator/src/consensus_state.rs

+}
+
+/// Hash a list of operator IDs to create a unique identifier, using SHA-256
+fn hash_operators(operators: &Vec<OperatorId>) -> [u8; 32] {


diegomrsantos · 2025-03-27T20:08:49Z

anchor/message_validator/src/lib.rs

+        self.consensus_state_map
+            .entry(message_id.clone())
+            .or_insert_with(|| {
+                // Create a new consensus state with storage for two epochs worth of slots


The comment is incorrect, I removed it. It's one ConsensusState per message id

diegomrsantos · 2025-03-27T20:10:06Z

anchor/message_validator/src/lib.rs

            | ValidationFailure::LateSlotMessage
            | ValidationFailure::SlotAlreadyAdvanced
-            | ValidationFailure::RoundAlreadyAdvanced
+            | ValidationFailure::RoundAlreadyAdvanced { got: _, want: _ }


diegomrsantos · 2025-03-27T20:47:46Z

anchor/message_validator/src/consensus_state.rs

+    // Create a buffer (no sorting, to match Go implementation)
+    let mut buffer = Vec::new();
+
+    // Write each operator ID to the buffer in little endian order
+    for &operator in operators {
+        buffer.extend_from_slice(&operator.0.to_le_bytes());
+    }
+
+    // Hash the buffer with SHA-256
+    let mut hasher = Sha256::new();
+    hasher.update(&buffer);
+    hasher.finalize().into()


Pushed a new version, let me know what you think

dknopik

LGTM, thanks!

diegomrsantos self-assigned this Mar 14, 2025

diegomrsantos added the validation label Mar 14, 2025

diegomrsantos force-pushed the validate-qbft-logic branch 8 times, most recently from 91b4120 to 381199f Compare March 21, 2025 13:49

diegomrsantos requested a review from Copilot March 21, 2025 14:03

Copilot AI reviewed Mar 21, 2025

View reviewed changes

anchor/message_validator/src/message_counts.rs Outdated Show resolved Hide resolved

diegomrsantos added 3 commits March 25, 2025 14:44

add validate qbft logic

c171db7

remove Arc from SlotClock param

3c319a6

fix conflicts

78c7ed0

diegomrsantos force-pushed the validate-qbft-logic branch from 42f138b to 78c7ed0 Compare March 25, 2025 17:44

Merge branch 'unstable' into validate-qbft-logic

72b401e

diegomrsantos marked this pull request as ready for review March 25, 2025 19:21

diegomrsantos requested review from Zacholme7, Copilot, dknopik and jking-aus March 25, 2025 19:22

Copilot AI reviewed Mar 25, 2025

View reviewed changes

anchor/message_validator/src/consensus_message.rs Outdated Show resolved Hide resolved

diegomrsantos and others added 2 commits March 25, 2025 20:38

Update anchor/message_validator/src/message_counts.rs

78dfa65

Co-authored-by: Copilot <175728472+Copilot@users.noreply.github.com>

remove unwrap from validate_round_in_allowed_spread

cc6e8b3

diegomrsantos requested a review from Copilot March 25, 2025 19:43

Copilot AI reviewed Mar 25, 2025

View reviewed changes

diegomrsantos requested a review from Copilot March 25, 2025 19:46

Copilot AI reviewed Mar 25, 2025

View reviewed changes

anchor/common/ssv_types/src/round.rs Show resolved Hide resolved

anchor/message_validator/src/consensus_state.rs Outdated Show resolved Hide resolved

remove println

6c4bd50

Merge branch 'unstable' into validate-qbft-logic

a49b0a9

# Conflicts: # anchor/client/src/lib.rs

diegomrsantos requested a review from Copilot March 26, 2025 10:44

Copilot AI reviewed Mar 26, 2025

View reviewed changes

anchor/common/ssv_types/src/round.rs Show resolved Hide resolved

anchor/message_sender/src/network.rs Show resolved Hide resolved

diegomrsantos added 4 commits March 26, 2025 19:55

small improvement

079ff28

small improvements

8fced11

pass role to validate_round_in_allowed_spread

e608736

more improvements

6a1a4e2

diegomrsantos force-pushed the validate-qbft-logic branch from 859f519 to 6a1a4e2 Compare March 26, 2025 21:07

Zacholme7 reviewed Mar 26, 2025

View reviewed changes

diegomrsantos and others added 3 commits March 26, 2025 22:26

add slots_per_epoch param

444d776

remove RefCell

1802f49

Merge branch 'unstable' into validate-qbft-logic

14fc0de

dknopik reviewed Mar 27, 2025

View reviewed changes

diegomrsantos added 7 commits March 27, 2025 20:48

Make Validator an Arc

c1dd2d7

improve pattern

9c2beab

simplify round_robin_proposer

2868c95

use &[OperatorId] instead of Vec

6344362

remove incorrect comment

f8bef0e

use .. instead of listing the fields

b0c804e

use CommitteeId

7046e93

diegomrsantos commented Mar 27, 2025

View reviewed changes

diegomrsantos requested a review from Copilot March 28, 2025 09:29

This comment was marked as resolved.

Sign in to view

diegomrsantos requested a review from dknopik March 31, 2025 11:44

dknopik approved these changes Mar 31, 2025

View reviewed changes

dknopik merged commit d2eb521 into sigp:unstable Mar 31, 2025
10 checks passed

dknopik mentioned this pull request Apr 3, 2025

Remove unnecessary mutex in validation #225

Merged

	impl From<Vec<OperatorId>> for CommitteeId {
	fn from(mut operator_ids: Vec<OperatorId>) -> Self {
	// Sort the operator IDs
	operator_ids.sort();
	let mut hasher = Sha256::new();

	// Add the operator IDs as 32 byte values
	for id in operator_ids {
	hasher.update((*id as u32).to_le_bytes());
	}

	// Hash it all
	<[u8; COMMITTEE_ID_LEN]>::from(hasher.finalize()).into()
	}
	}

Comments

Conversation

diegomrsantos commented Mar 14, 2025 • edited Loading Uh oh! There was an error while loading. Please reload this page.

Uh oh!

Issue Addressed

Proposed Changes

Uh oh!

Copilot AI left a comment

Choose a reason for hiding this comment

Pull Request Overview

Reviewed Changes

Uh oh!

Uh oh!

Copilot AI left a comment

Choose a reason for hiding this comment

Pull Request Overview

Reviewed Changes

Uh oh!

Uh oh!

Copilot AI left a comment

Choose a reason for hiding this comment

Uh oh!

Copilot AI left a comment

Choose a reason for hiding this comment

Pull Request Overview

Reviewed Changes

Uh oh!

Uh oh!

Uh oh!

Copilot AI left a comment

Choose a reason for hiding this comment

Pull Request Overview

Reviewed Changes

Uh oh!

Uh oh!

Uh oh!

Zacholme7 left a comment

Choose a reason for hiding this comment

Uh oh!

Choose a reason for hiding this comment

Uh oh!

Choose a reason for hiding this comment

Uh oh!

Choose a reason for hiding this comment

Uh oh!

Choose a reason for hiding this comment

Uh oh!

Choose a reason for hiding this comment

Uh oh!

Choose a reason for hiding this comment

Uh oh!

Uh oh!

Uh oh!

Choose a reason for hiding this comment

Uh oh!

Choose a reason for hiding this comment

Uh oh!

Choose a reason for hiding this comment

Uh oh!

Choose a reason for hiding this comment

Uh oh!

Choose a reason for hiding this comment

Uh oh!

Choose a reason for hiding this comment

Uh oh!

Zacholme7 Mar 26, 2025 • edited Loading Uh oh! There was an error while loading. Please reload this page.

Uh oh!

Choose a reason for hiding this comment

Uh oh!

Choose a reason for hiding this comment

Uh oh!

Choose a reason for hiding this comment

Uh oh!

Choose a reason for hiding this comment

Uh oh!

Choose a reason for hiding this comment

Uh oh!

Choose a reason for hiding this comment

Uh oh!

Choose a reason for hiding this comment

diegomrsantos commented Mar 14, 2025 •

edited

Loading

Zacholme7 Mar 26, 2025 •

edited

Loading