create common component for message serialization and signing #153

dknopik · 2025-02-20T16:54:12Z

Both the partial signature manager and the QBFT manager need to send messages. This PR introduces a component that schedules such tasks via the processor.

It also provides a mock message sender for use in tests.

diegomrsantos · 2025-02-21T09:59:32Z

anchor/message_sender/src/network.rs

+                move || {
+                    let signature = match sender.sign(&message) {
+                        Ok(signature) => signature,
+                        Err(err) => {


Do you know if we need to propagate this error?

This is one of those errors that really really should never happen, and where there is not really a way to fall back. So there is no reason to propagate it somewhere.

According to Murphy's law: Anything that can go wrong will go wrong :) It might be useful to test what happens to the rest of the system when this happens. I'll keep this in mind in my tasks.

diegomrsantos · 2025-02-21T10:06:19Z

anchor/message_sender/src/network.rs

+                        message.ssv_message,
+                        message.full_data,
+                    ) {
+                        Ok(signature) => signature,


very nit: signed_message

Good catch, thanks!

diegomrsantos · 2025-02-21T10:12:07Z

anchor/message_sender/src/network.rs

+                },
+                SIGNER_NAME,
+            )
+            .unwrap_or_else(|e| warn!("Failed to send to processor: {}", e));


maybe it's not necessary as I see that we already log in the processor

True, will change it

diegomrsantos · 2025-02-21T10:16:42Z

anchor/message_sender/src/lib.rs

+use ssv_types::consensus::UnsignedSSVMessage;
+use ssv_types::message::SignedSSVMessage;
+
+pub trait MessageSender: Send + Sync {


Very nice. In my PR, I was thinking about a way to split the "interface" and the implementation. First I created two crates, one with the trait and types and another with the implementation. The component C, which needs the functionality, imports only the interface crate, and who create C imports the implementation. But maybe two crates is too much overhead. I think we can achieve the same using one crate with different modules and features. Wdyt?

Yeah, I was also tempted to do that. This is for example what we do for the validator_store: This only contains the trait and types used in the interface, and there are creates lighthouse_validator_store in LH repo and anchor_validator_store in Anchor repo.

However, as you said, for small stuff like this, it is likely too much noise and too little benefit to split it up.

One benefit is enforcing modularity/encapsulation and to make sure that clients of the crate can only use what they are supposed to and nothing else. As a crate that provides a functionality grows, it's tempting for people to just use implementation details directly, bypassing the API when it's more convenient.

That's the whole point of the module system, right? Consider the message validator. It's harder to do that with only one crate as the network needs access to the interface and the client module needs access to the implementation, so both need to be public. That's why I mentioned features, I guess that's the only option if there's only one crate.

diegomrsantos · 2025-02-21T11:04:59Z

anchor/message_sender/src/network.rs

+        signer.sign_to_vec()
+    }
+
+    fn determine_subnet(&self, message: &SignedSSVMessage) -> Result<SubnetId, String> {


How about a specific error instead of a String?

diegomrsantos · 2025-02-21T11:09:21Z

anchor/message_sender/src/network.rs

+        database: watch::Receiver<NetworkState>,
+        operator_id: OperatorId,
+        subnet_count: usize,
+    ) -> Result<Arc<Self>, String> {


Would it be better to return only Self and let the caller decide if and when to wrap it in an Arc?

diegomrsantos · 2025-02-21T11:12:43Z

anchor/message_sender/src/network.rs

+        processor: processor::Senders,
+        network_tx: mpsc::Sender<(SubnetId, Vec<u8>)>,
+        private_key: Rsa<Private>,
+        database: watch::Receiver<NetworkState>,


a bit nit: how about network_state_rx?

diegomrsantos · 2025-02-21T11:23:05Z

anchor/message_sender/src/network.rs

+        };
+        match self.network_tx.try_send((subnet, message.as_ssz_bytes())) {
+            Ok(_) => debug!(?subnet, "Successfully sent message to network"),
+            Err(TrySendError::Closed(_)) => warn!("Network queue closed (shutting down?)"),


This error seems more critical. Should we propagate it and shut down the system?

To propagate it, we would need a return queue to the sender, and the sender would need to wait for it, defeating the purpose of putting this on another thread in the first place.

I believe this is fine If we can guarantee that this error happens if and only if the system is already shutting down. Otherwise, the client can keep calling sign_and_send and the network never receives a msg as it fails silently, right?

We could also add this at the beginning of sign_and_send

if self.network_tx.is_closed() { // return error }

diegomrsantos · 2025-02-21T12:02:16Z

anchor/message_sender/src/network.rs

+        let committee_id = match msg_id.duty_executor() {
+            Some(DutyExecutor::Committee(committee_id)) => committee_id,
+            Some(DutyExecutor::Validator(pubkey)) => {
+                let database = self.database.borrow();
+                let Some(metadata) = database.metadata().get_by(&pubkey) else {
+                    return Err(format!("Unknown validator: {pubkey}"));
+                };
+                let Some(cluster) = database.clusters().get_by(&metadata.cluster_id) else {
+                    return Err(format!(
+                        "Inconsistent database, no cluster for validator: {pubkey}"
+                    ));
+                };
+                cluster.committee_id()
+            }


Would it make sense to move this to the qbft manager and send the committee_id as another parameter in sign_and_send?

This might make sense. I am currently implementing the messaging infrastructure for the signature_collector, and I need to see if this would work there as well.

diegomrsantos · 2025-02-21T12:27:55Z

anchor/message_sender/src/testing.rs

+use ssv_types::OperatorId;
+use tokio::sync::mpsc;
+
+pub struct TestingMessageSender {


how about MockMessageSender or NoopSignerMessageSender?

diegomrsantos · 2025-02-21T12:31:23Z

anchor/message_sender/src/testing.rs

+
+impl TestingMessageSender {
+    pub fn new(
+        message_tx: mpsc::UnboundedSender<SignedSSVMessage>,


nit: this could be created inside the constructor if no assertion is needed in the tests

Zacholme7 · 2025-02-21T13:31:12Z

anchor/message_sender/src/network.rs

+                let Some(metadata) = database.metadata().get_by(&pubkey) else {
+                    return Err(format!("Unknown validator: {pubkey}"));
+                };
+                let Some(cluster) = database.clusters().get_by(&metadata.cluster_id) else {
+                    return Err(format!(
+                        "Inconsistent database, no cluster for validator: {pubkey}"
+                    ));
+                };


Im a bit rusty on what I wrote for the db, but cant this just be database.cluster().get_by(&pubkey)?

/// All of the clusters in the network /// Primary: cluster id. uniquely identifies a cluster /// Secondary: public key of the validator. uniquely identifies a cluster /// Tertiary: owner of the cluster. uniquely identifies a cluster pub(crate) type ClusterMultiIndexMap = MultiIndexMap<ClusterId, PublicKeyBytes, Address, Cluster, UniqueTag, UniqueTag>;

Ah yes, I think so, thanks!

dknopik · 2025-02-21T14:10:36Z

Thanks for all your feedback <3

I am currently integrating this into the signature collector, and will likely gather some ways to also improve this in the process of doing so. I will address your feedback as soon as I'm fully confident with the design.

create common component for message serialization and signing

82411fb

dknopik added the network label Feb 20, 2025

dknopik requested review from diegomrsantos and Zacholme7 February 20, 2025 16:54

diegomrsantos reviewed Feb 21, 2025

View reviewed changes

give queue to network and publish

f076a7a

diegomrsantos reviewed Feb 21, 2025

View reviewed changes

Zacholme7 reviewed Feb 21, 2025

View reviewed changes

dknopik marked this pull request as draft February 21, 2025 14:10

Provide feedback

Saved searches

Use saved searches to filter your results more quickly

create common component for message serialization and signing #153

create common component for message serialization and signing #153

dknopik commented Feb 20, 2025

diegomrsantos Feb 21, 2025

dknopik Feb 21, 2025

diegomrsantos Feb 21, 2025

diegomrsantos Feb 21, 2025

dknopik Feb 21, 2025

diegomrsantos Feb 21, 2025

dknopik Feb 21, 2025

diegomrsantos Feb 21, 2025 •

edited

Loading

dknopik Feb 21, 2025

diegomrsantos Feb 21, 2025 •

edited

Loading

diegomrsantos Feb 21, 2025 •

edited

Loading

diegomrsantos Feb 21, 2025

diegomrsantos Feb 21, 2025

diegomrsantos Feb 21, 2025

diegomrsantos Feb 21, 2025

dknopik Feb 21, 2025

diegomrsantos Feb 21, 2025 •

edited

Loading

diegomrsantos Feb 21, 2025

diegomrsantos Feb 21, 2025 •

edited

Loading

dknopik Feb 21, 2025

diegomrsantos Feb 21, 2025

diegomrsantos Feb 21, 2025

Zacholme7 Feb 21, 2025

dknopik Feb 21, 2025

dknopik commented Feb 21, 2025

create common component for message serialization and signing #153

Are you sure you want to change the base?

create common component for message serialization and signing #153

Conversation

dknopik commented Feb 20, 2025

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

diegomrsantos Feb 21, 2025 • edited Loading

Choose a reason for hiding this comment

Choose a reason for hiding this comment

diegomrsantos Feb 21, 2025 • edited Loading

Choose a reason for hiding this comment

diegomrsantos Feb 21, 2025 • edited Loading

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

diegomrsantos Feb 21, 2025 • edited Loading

Choose a reason for hiding this comment

Choose a reason for hiding this comment

diegomrsantos Feb 21, 2025 • edited Loading

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

dknopik commented Feb 21, 2025

diegomrsantos Feb 21, 2025 •

edited

Loading

diegomrsantos Feb 21, 2025 •

edited

Loading

diegomrsantos Feb 21, 2025 •

edited

Loading

diegomrsantos Feb 21, 2025 •

edited

Loading

diegomrsantos Feb 21, 2025 •

edited

Loading