Stateless QBFT consensus message semantic validation by diegomrsantos · Pull Request #171 · sigp/anchor

diegomrsantos · 2025-03-04T16:46:35Z

Issue Addressed

Proposed Changes

Add an initial validation for QBFT consensus messages that doesn't depend on the consensus state.

Additional Info

Validation using consensus state is coming in the next PR.

diegomrsantos · 2025-03-05T14:39:17Z

This is based on #160, as soon as it gets merged, I'll remove the common code and make it ready for review. But feel free to take a look in the mean time.

anchor/common/qbft/src/lib.rs

diegomrsantos · 2025-03-10T15:56:01Z

@jking-aus @dknopik @Zacholme7 Sorry for the big PR. I can move the tests to a new one if necessary.

diegomrsantos · 2025-03-11T11:33:36Z

anchor/message_validator/src/lib.rs

+        let committee_id = match signed_ssv_message.ssv_message().msg_id().duty_executor() {
+            Some(DutyExecutor::Committee(id)) => id,
+            _ => return Err(ValidationFailure::NonExistentCommitteeID),
+        };


This is incomplete, I'm not handling the validator case. But this PR is already quite big, I will handle it in my next one which is coming soon.

…ion and VoluntaryExit) fix max_round

diegomrsantos · 2025-03-11T12:20:36Z

anchor/message_validator/src/lib.rs

+    hash
+}
+
+#[cfg(test)]


The tests could be greatly simplified if it's possible to call the validation functions without creating a validator. It's an interesting idea that I'll explore in a new PR.

Zacholme7

This is awesome, thanks for doing this. Have a few comments.

anchor/common/qbft/src/lib.rs

Zacholme7 · 2025-03-11T13:16:15Z

anchor/database/src/state.rs

+
+pub struct WatchableNetworkState {
+    state_rx: watch::Receiver<NetworkState>,
+}
+
+impl WatchableNetworkState {
+    pub fn new(state_rx: watch::Receiver<NetworkState>) -> Self {
+        Self { state_rx }
+    }
+}
+
+impl NetworkStateService for WatchableNetworkState {
+    fn get_cluster_members(&self, committee_id: &CommitteeId) -> Option<IndexSet<OperatorId>> {
+        let db_state = self.state_rx.borrow();
+        db_state
+            .multi_state
+            .clusters
+            .get_all_by(committee_id)
+            .and_then(|clusters| clusters.first().cloned())
+            .map(|cluster| cluster.cluster_members)
+    }
+}


Why is this needed?

Can get_cluster_members just be defined on the network state and then use state() and watch() on the database to get access to NetworkState?

You mean why the trait is needed? If so, it's much easier to use it in the tests then recreating the whole NetworkState. But after talking to @dknopik, there might be an easier solution if we extract the validation to functions that can be called without a validator instance. I'll explore this design in a new PR.

anchor/database/src/state.rs

anchor/message_validator/src/lib.rs

anchor/database/src/state.rs

…idation merge unstable

jking-aus

great work as usual diego -- just a couple of comments and resolve the conflicts please

jking-aus · 2025-03-12T05:42:58Z

anchor/common/ssv_types/src/consensus.rs

+        self.identifier.role().and_then(|role| match role {
+            Role::Committee | Role::Aggregator => Some(12),
+            Role::Proposer | Role::SyncCommittee => Some(6),
+            _ => None,


this may be obvious and I'm not getting it but why are we hardcoding 12 and 6 as max rounds? is that ssv spec? 2secs per round and 2 slot window?

I copied it from here https://github.com/ssvlabs/ssv/blob/main/message/validation/consensus_validation.go#L370

all good we need to match them on this or we won't timeout at the same time. I would like to ask them where they're pulling this number from because I would assume it's just a guess like follow distance. I will just add an inline comment saying where it's from.

This is used only for validation, maybe I shouldn't have added it here.

I can move it to the validator if you think it's better.

anchor/database/src/state.rs

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

…antos/anchor into qbft-message-validation

jking-aus

lgtm

diegomrsantos force-pushed the qbft-message-validation branch 2 times, most recently from 91d58e5 to 2b07331 Compare March 5, 2025 11:31

diegomrsantos self-assigned this Mar 5, 2025

diegomrsantos added the validation label Mar 5, 2025

diegomrsantos force-pushed the qbft-message-validation branch from f1038ee to 4b08b92 Compare March 5, 2025 13:52

diegomrsantos changed the title ~~Qbft message validation~~ Stateless Qbft consensus message validation Mar 5, 2025

diegomrsantos changed the title ~~Stateless Qbft consensus message validation~~ Stateless QBFT consensus message validation Mar 5, 2025

diegomrsantos added 3 commits March 6, 2025 09:40

validation when signers > 1

8484ab2

add validate_consensus_message_semantics

7467879

add validate_justifications

a41c6cb

diegomrsantos force-pushed the qbft-message-validation branch from 4b08b92 to a41c6cb Compare March 6, 2025 08:40

diegomrsantos added 2 commits March 6, 2025 19:42

get committee members from the NetworkState

6109eb7

multi index with 4 indices

1b40fa0

diegomrsantos force-pushed the qbft-message-validation branch from c34cd52 to 1b40fa0 Compare March 7, 2025 13:33

diegomrsantos added 3 commits March 7, 2025 18:27

fix naming

e039197

add tests

dc3bd78

improve tests

878c0de

diegomrsantos commented Mar 10, 2025

View reviewed changes

anchor/common/qbft/src/lib.rs Show resolved Hide resolved

Merge branch 'unstable' into qbft-message-validation

e88d3cf

diegomrsantos requested review from Zacholme7, dknopik and jking-aus and removed request for Zacholme7 March 10, 2025 12:51

diegomrsantos marked this pull request as ready for review March 10, 2025 12:52

fix comments

e5158db

diegomrsantos force-pushed the qbft-message-validation branch from 0e902a6 to e5158db Compare March 10, 2025 15:25

add new test

87da566

diegomrsantos and others added 4 commits March 10, 2025 19:09

remove do_validate

5a966e5

decode msgs to PartialSignatureMessages

a4a58be

Merge branch 'unstable' into qbft-message-validation

72ae550

Merge branch 'unstable' into qbft-message-validation

fd3b7c9

diegomrsantos commented Mar 11, 2025

View reviewed changes

diegomrsantos and others added 2 commits March 11, 2025 12:34

Merge branch 'unstable' into qbft-message-validation

c933390

add Rule: Duty role has consensus (true except for ValidatorRegistrat…

ec791c7

…ion and VoluntaryExit) fix max_round

diegomrsantos commented Mar 11, 2025

View reviewed changes

diegomrsantos added the ready-for-review This PR is ready to be reviewed label Mar 11, 2025

Zacholme7 reviewed Mar 11, 2025

View reviewed changes

diegomrsantos and others added 5 commits March 11, 2025 15:47

remove unused code

66b4e3f

add comment

0e73245

improve comment for decided msg

a52f6f2

remove redundant root hash check

fab2f25

Merge remote-tracking branch 'network/unstable' into qbft-message-val…

51c9718

…idation merge unstable

jking-aus requested changes Mar 12, 2025

View reviewed changes

jking-aus and others added 3 commits March 12, 2025 21:13

small typos

75fe744

Merge branch 'unstable' into qbft-message-validation

74d07a8

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

Merge branch 'qbft-message-validation' of https://github.com/diegomrs…

a7c537d

…antos/anchor into qbft-message-validation

diegomrsantos changed the title ~~Stateless QBFT consensus message validation~~ Stateless QBFT consensus message semantic validation Mar 12, 2025

diegomrsantos requested a review from jking-aus March 12, 2025 20:54

Update consensus.rs

f4806ef

jking-aus approved these changes Mar 12, 2025

View reviewed changes

diegomrsantos merged commit 90335d8 into sigp:unstable Mar 12, 2025
10 checks passed

dknopik removed the ready-for-review This PR is ready to be reviewed label Nov 7, 2025

Comments

Conversation

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

Uh oh!

Issue Addressed

Proposed Changes

Additional Info

Uh oh!

diegomrsantos commented Mar 5, 2025

Uh oh!

Uh oh!

diegomrsantos commented Mar 10, 2025

Uh oh!

Choose a reason for hiding this comment

Uh oh!

Choose a reason for hiding this comment

Uh oh!

Zacholme7 left a comment

Choose a reason for hiding this comment

Uh oh!

Uh oh!

Choose a reason for hiding this comment

Uh oh!

Choose a reason for hiding this comment

Uh oh!

Uh oh!

Uh oh!

Uh oh!

jking-aus left a comment • 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!

diegomrsantos Mar 12, 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!

Uh oh!

jking-aus left a comment

Choose a reason for hiding this comment

Uh oh!

Uh oh!

Reviewers

Assignees

Labels

Projects

Milestone

Development

Uh oh!

4 participants

diegomrsantos commented Mar 4, 2025 •

edited

Loading

jking-aus left a comment •

edited

Loading

diegomrsantos Mar 12, 2025 •

edited

Loading