Add QBFT manager by dknopik · Pull Request #77 · sigp/anchor

dknopik · 2024-12-20T17:04:48Z

Refactor the current qbft crate to be sync and general purpose and create qbft_manager which integrates into the client architecture such as the processor. This module is going to be used by the validator store (to trigger QBFT instances) and the network (to pass on network messages related to QBFT).

There is still quite a bit to do to clean this up, but this initial version should be able to get us going.

Depends on #76.

jking-aus

This is excellent work mate -- tonnes of great additions, in particular I like the idea of splitting out the instance manager from the consensus mechanism. Probably two or three PRs of work here tbh

anchor/common/qbft/src/types.rs

jking-aus · 2025-01-08T04:57:01Z

anchor/common/qbft/src/types.rs

+        *operator_id
+            == *committee
+                .get(((round.get() - Round::default().get()) + *instance_height) % committee.len())
+                .expect("slice bounds kept by modulo length")


could this panic if committee.len = 0? assume there is a sanity check somewhere to prevent this

Actually - yes, modulo/remainder panics if the len is 0. And no, I don't think this is currently checked 😅 - I will extend the validation done by the config builder.

anchor/common/qbft/src/config.rs

anchor/common/qbft/src/types.rs

diegomrsantos · 2025-01-08T17:54:59Z

anchor/common/ssv_types/src/message.rs

+// the fields better
+
+#[derive(Clone, Debug)]
+pub struct SignedSsvMessage<E: EthSpec> {


I see there's some overlapping with https://github.com/sigp/anchor/pull/80/files#diff-46abd9dbbf9cc4366e3258211d1bf4ff7fce2df918e1e870e607150429fc97a2

Yeah, this was just a rough draft for my understanding as the comment says - we should probably switch to yours :)

There are better things in your code, like using an Enum for the raw data. We could keep both for now, but think about a way of merging them and where it should be in the code. Should we use the same type for the data parsed from the network and the one used in application logic?

We should probably use them in application code as well. In some cases, we rely on them being the same as networking messages - for example, PREPARE and COMMIT messages refer to the full data by hash, so we need the messages to be identical to the format used by go-ssv so that the hashes agree.

I propose we both merge our versions and then align them in an own PR. Intuitively, I would put them in ssv_types, but we can figure that out then :)

What about the full_data is passed as raw bytes to the application and the decoding is done there?

In my opinion, the network stack should verify that full_data is actually a valid object, so that illegal messages with junk data are not further gossiped. But I am not sure if that is actually done by go-ssv...

But yeah, I think we will (unfortunately) need some kind of type separation - we can only decode as soon as we know the data version (i.e. fork) that is specified in the message itself

It's possible to create a valid object with junk data anyway. I believe the more comprehensive validation in Gossipsub is done by the application.

This is possibly related https://docs.rs/libp2p-gossipsub/latest/libp2p_gossipsub/struct.Behaviour.html#method.report_message_validation_result

anchor/common/ssv_types/src/message.rs

anchor/common/qbft/src/config.rs

diegomrsantos · 2025-01-10T13:40:06Z

anchor/common/qbft/src/config.rs

-    pub pr: usize,
-    pub committee_size: usize,
-    pub committee_members: HashSet<OperatorId>,
+    pub committee_members: IndexSet<OperatorId>,


Should those fields be private?

No, IMO users should be able to use the Config directly instead of using the builder

(but then we should probably move the validation)

No, IMO users should be able to use the Config directly instead of using the builder

In this way, I don't see the point of the builder.

The Builder is for ease of use when default values are desired. As the defaults are dependent on other values, a Default impl does not suffice.

The purpose of this builder pattern I mentioned is to facilitate the creation of an immutable config instance making it clearer what fields are required and optional, as well as centralizing the validation.

Okay, fair point 👍

diegomrsantos · 2025-01-10T14:11:57Z

anchor/common/qbft/src/config.rs

-    /// The quorum size required for the committee to reach consensus
-    pub fn quorum_size(&self) -> usize {
-        self.quorum_size
+    pub fn commmittee_members(&self) -> &IndexSet<OperatorId> {


We could add this to the beginning of this impl

/// Private constructor so it can only be built by our `ConfigBuilder`. fn from_builder(builder: &ConfigBuilder<F>) -> Self { Self { operator_id: builder.operator_id, instance_height: builder.instance_height, committee_members: builder.committee_members.clone(), round: builder.round, round_time: builder.round_time, max_rounds: builder.max_rounds, quorum_size: builder.quorum_size, leader_fn: builder.leader_fn.clone(), } }

diegomrsantos · 2025-01-10T19:08:37Z

anchor/common/qbft/src/lib.rs

        };
-
-        (in_sender, out_receiver, instance)
+        qbft.start_round();


I noticed that the constructor is doing more than just initializing data structures - start_round() is changing state and sending messages. It might be cleaner and easier to work with if construction and execution were kept separate. This way, the caller can control when the QBFT process starts, and it could also make testing simpler. What do you think?

Not calling start_round leaves the instance in an invalid state, inviting misuse. I'd prefer to leave it here.

Why does the constructor create an instance in an invalid state?

ah, sorry, got confused, you are right! should be fine without, even if messages come in without calling it. so yeah, we can change it.

valid comment but let's merge this in first

Is it worth creating an issue to address that?

diegomrsantos · 2025-01-10T19:23:29Z

anchor/common/qbft/src/lib.rs

-type RoundChangeMap<D> = HashMap<OperatorId, Option<ConsensusData<ValidatedData<D>>>>;
+type RoundChangeMap<D> = HashMap<OperatorId, Option<ConsensusData<D>>>;
+
+pub trait Data: Debug + Clone {


pub trait Hashable: Debug + Clone { type Digest: Debug + Clone + Eq + Hash; fn digest(&self) -> Self::Digest; }

What do you think about it? The motivation is to make the trait name more descriptive and avoid using Hash twice - it seems like a recursive definition.

I named it Data intentionally, as it does more than hashing through the bounds on Debug and Clone.

I don't have a strong opinion on Digest vs Hash, so feel free to change it.

anchor/common/qbft/src/lib.rs

Co-authored-by: diegomrsantos <diegomrsantos@gmail.com>

diegomrsantos · 2025-01-13T15:05:14Z

anchor/common/qbft/src/config.rs

+    pub fn operator_id(&self) -> OperatorId {
+        self.operator_id
+    }
+
+    pub fn committee_members(&self) -> &IndexSet<OperatorId> {
+        &self.committee_members
+    }
+
+    pub fn instance_height(&self) -> &InstanceHeight {
+        &self.instance_height
    }
-}

-impl<F: LeaderFunction + Clone> ConfigBuilder<F> {
-    pub fn operator_id(&mut self, operator_id: OperatorId) -> &mut Self {
-        self.config.operator_id = operator_id;
+    pub fn round(&self) -> Round {
+        self.round
+    }
+
+    pub fn round_time(&self) -> Duration {
+        self.round_time
+    }
+
+    pub fn quorum_size(&self) -> usize {
+        self.quorum_size
+    }
+
+    pub fn max_rounds(&self) -> usize {
+        self.max_rounds
+    }
+
+    pub fn leader_fn(&self) -> &F {
+        &self.leader_fn
+    }
+


do we need those?

yeah, at least the test need to access the builder data (now that it is impossible to update the Config, see c742d1d)

I am unsure if we now ended up with the most ergonomic/clean setup, but I think it's fine for now. We can revisit it later.

jking-aus

great work daniel - lots of good discussion as well

Zacholme7 · 2025-01-14T14:32:37Z

anchor/common/qbft/src/lib.rs

+        };
+        let operator_id = self.config.operator_id();
+        (self.send_message)(Message::Propose(operator_id, consensus_data.clone()));
+        self.received_propose(operator_id, consensus_data);


why are we calling the corresponding receive after each send?

dknopik force-pushed the split-qbft branch from 0a236d1 to 8e76342 Compare January 2, 2025 15:04

dknopik changed the title ~~[WIP] Split qbft~~ Add QBFT manager Jan 3, 2025

dknopik marked this pull request as ready for review January 3, 2025 09:16

dknopik requested a review from jking-aus January 3, 2025 09:16

dknopik mentioned this pull request Jan 3, 2025

Duty services #73

Merged

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

jking-aus reviewed Jan 8, 2025

View reviewed changes

dknopik added ready-for-review This PR is ready to be reviewed and removed ready-for-review This PR is ready to be reviewed labels Jan 8, 2025

diegomrsantos reviewed Jan 8, 2025

View reviewed changes

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

dknopik mentioned this pull request Jan 8, 2025

Move QBFT crate #85

Merged

Zacholme7 reviewed Jan 8, 2025

View reviewed changes

anchor/common/qbft/src/types.rs Outdated Show resolved Hide resolved

diegomrsantos reviewed Jan 8, 2025

View reviewed changes

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

diegomrsantos reviewed Jan 8, 2025

View reviewed changes

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

dknopik mentioned this pull request Jan 10, 2025

create signature collector and make processor stateless #76

Merged

dknopik added 8 commits January 10, 2025 10:22

implement initial qbft manager

e637ea1

clean up

e8b2f66

reimplement test and fix issues

e36655b

remove decided qbft instances

0bb5477

clean up qbft and fix leader selection

57ad5ae

improve qbft config validation

db7e476

avoid duplicate message

20e3240

avoid needless pub

8312017

dknopik force-pushed the split-qbft branch from 29698a7 to 507a9f7 Compare January 10, 2025 09:31

switch to IndexSet for committee

76e6280

diegomrsantos reviewed Jan 10, 2025

View reviewed changes

anchor/common/qbft/src/config.rs Outdated Show resolved Hide resolved

diegomrsantos reviewed Jan 10, 2025

View reviewed changes

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

diegomrsantos reviewed Jan 10, 2025

View reviewed changes

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

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

diegomrsantos reviewed Jan 10, 2025

View reviewed changes

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

dknopik force-pushed the split-qbft branch from 507a9f7 to 76e6280 Compare January 13, 2025 10:06

dknopik and others added 3 commits January 13, 2025 12:18

Apply some changes from review.

acb91a5

Co-authored-by: diegomrsantos <diegomrsantos@gmail.com>

Merge branch 'unstable' into split-qbft

780b31e

Refactor builder

c742d1d

Co-authored-by: diegomrsantos <diegomrsantos@gmail.com>

diegomrsantos reviewed Jan 13, 2025

View reviewed changes

Merge branch 'unstable' into split-qbft

6be39ac

jking-aus approved these changes Jan 14, 2025

View reviewed changes

jking-aus merged commit 9389ccd into sigp:unstable Jan 14, 2025
9 checks passed

Zacholme7 reviewed Jan 14, 2025

View reviewed changes

Zacholme7 mentioned this pull request Jan 14, 2025

Qbft tests #94

Merged

dknopik deleted the split-qbft branch February 5, 2025 14:14

Comments

Conversation

dknopik commented Dec 20, 2024 • edited Loading Uh oh! There was an error while loading. Please reload this page.

Uh oh!

Uh oh!

jking-aus 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!

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!

Choose a reason for hiding this comment

Uh oh!

diegomrsantos Jan 9, 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!

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!

Choose a reason for hiding this comment

Uh oh!

diegomrsantos Jan 10, 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!

diegomrsantos Jan 14, 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!

Uh oh!

Uh oh!

Uh oh!

Uh oh!

Choose a reason for hiding this comment

dknopik commented Dec 20, 2024 •

edited

Loading

diegomrsantos Jan 9, 2025 •

edited

Loading

diegomrsantos Jan 10, 2025 •

edited

Loading

diegomrsantos Jan 14, 2025 •

edited

Loading