Subscribe to topic and decode SignedSSVMessage by diegomrsantos · Pull Request #80 · sigp/anchor

diegomrsantos · 2024-12-24T13:31:36Z

Issue Addressed

Initial implementation for #74. The aim of this PR is to able to subscribe to a topic, receive and decode SignedSSVMessage's.

Proposed Changes

Dependencies: Updated dependencies in Cargo.toml including ethereum_ssz, ethereum_ssz_derive, and hex.
Network Types: Added ssv_message module to src/types.rs.
Network Implementation:
- Enhanced network.rs to include ssv_message::SignedSSVMessage and ssz::Decode.
- Subscribed to ssv.v2.9 in the run method.
- Handled GossipSub events to process and log SignedSSVMessage.
Gossipsub Configuration: Changed validation_mode from Strict to Permissive. An error happened when running the system on Holesky using the previous mode.
New Files: Added new file ssv_message.rs in anchor/network/src/types/.

Additional Info

As in the previous subnet search PR, subnet 9 is an arbitrary choice for test purposes and has no specific significance.

dknopik

Left some remarks. Looking good!

anchor/network/src/network.rs

dknopik · 2025-01-16T12:20:19Z

anchor/network/src/types/ssv_message.rs

+impl Encode for MessageID {
+    fn is_ssz_fixed_len() -> bool {
+        true
+    }
+
+    fn ssz_append(&self, buf: &mut Vec<u8>) {
+        buf.extend_from_slice(&self.0);
+    }
+
+    fn ssz_fixed_len() -> usize {
+        MESSAGE_ID_LEN
+    }
+
+    fn ssz_bytes_len(&self) -> usize {
+        MESSAGE_ID_LEN
+    }
+}
+
+impl Decode for MessageID {
+    fn is_ssz_fixed_len() -> bool {
+        true
+    }
+
+    fn ssz_fixed_len() -> usize {
+        MESSAGE_ID_LEN
+    }
+
+    fn from_ssz_bytes(bytes: &[u8]) -> Result<Self, DecodeError> {
+        if bytes.len() != MESSAGE_ID_LEN {
+            return Err(DecodeError::InvalidByteLength {
+                len: bytes.len(),
+                expected: MESSAGE_ID_LEN,
+            });
+        }
+        let mut id = [0u8; MESSAGE_ID_LEN];
+        id.copy_from_slice(bytes);
+        Ok(MessageID(id))
+    }
+}


impls look good, but it's unfortunate that this is necessary, using the derive macro would be nicer. I'll see if we can improve the situation upstream 👍

Yeah, it fails with an error. Should we add a comment explaining why this is necessary?

anchor/network/src/types/ssv_message.rs

dknopik · 2025-01-16T12:25:45Z

anchor/network/src/types/ssv_message.rs

+impl Encode for MsgType {
+    fn is_ssz_fixed_len() -> bool {
+        true
+    }
+
+    fn ssz_append(&self, buf: &mut Vec<u8>) {
+        let value: u64 = match self {
+            MsgType::SSVConsensusMsgType => 0,
+            MsgType::SSVPartialSignatureMsgType => 1,
+        };
+        buf.extend_from_slice(&value.to_le_bytes());
+    }
+
+    fn ssz_fixed_len() -> usize {
+        U64_SIZE
+    }
+
+    fn ssz_bytes_len(&self) -> usize {
+        U64_SIZE
+    }
+}
+
+impl Decode for MsgType {
+    fn is_ssz_fixed_len() -> bool {
+        true
+    }
+
+    fn ssz_fixed_len() -> usize {
+        U64_SIZE
+    }
+
+    fn from_ssz_bytes(bytes: &[u8]) -> Result<Self, DecodeError> {
+        if bytes.len() != U64_SIZE {
+            return Err(DecodeError::InvalidByteLength {
+                len: bytes.len(),
+                expected: U64_SIZE,
+            });
+        }
+        let value = u64::from_le_bytes(bytes.try_into().unwrap());
+        MsgType::try_from_u64(value)
+    }
+}


Same here, would be nice for the macro to support generating this if there is #[repr(u*)] on an enum, will check upstream. Otherwise, lgtm.

dknopik · 2025-01-16T12:46:00Z

anchor/network/src/types/ssv_message.rs

+pub struct SSVMessage {
+    msg_type: MsgType,
+    msg_id: MessageID, // Fixed-size [u8; 56]
+    data: Vec<u8>,     // Variable-length byte array


I do not like the Vec<u8> here - but unfortunately, it's probably the best option.

The other option would be an enum with ssz_derive's #[ssz(enum_behaviour = "transparent")], but this is inefficient (as it blindly tries to decode each variant until one variant does not fail to decode).

Longer term, IMO the SSV spec should switch to a SSZ union - or separate topics for separate message types.

I agree now. My argument of decoupling this type from the application doesn't make much as the msg_type: MsgType already has application knowledge, doesn't it? Also, not sure that decoding this at the application level is the right place for that. Would this inefficiency severely affect performance? No way around that? But another possible issue is the EthSpec everywhere in the code.

Not decoding it at the networking layer would make more sense if the msg_type was encoded inside the data. In this way, we would know nothing about the data and just deliver it to the application. If messages are created/deleted/modified nothing would change at this layer.

I think we are pretty flexible in being able to push msg_type down into data. As long as qbft can work with serialized data without having to introduce EthSpec I dont think we have a ton of hard constraints. Some internal abstraction layer will be needed regardless. Something like

Network -> decode -> Validator store -> Qbft manager/Signature Collector -> Validator Store -> encoder -> Network

It's even simpler. We would just not decode msg_type and pass it as a number. It'd make even more sense with the current code, I guess.

dknopik · 2025-01-16T12:48:50Z

anchor/network/src/types/ssv_message.rs

+    signatures: Vec<Vec<u8>>, // Vec of Vec<u8>, max 13 elements, each up to 256 bytes
+    operator_ids: Vec<OperatorID>, // Vec of OperatorID (u64), max 13 elements
+    ssv_message: SSVMessage,  // SSVMessage: Required field
+    full_data: Vec<u8>,       // Variable-length byte array, max 4,194,532 bytes


I think it's preferable to use ssz_types::VariableList (basically a Vec wrapper) here to enforce the maximum length.

The docs seem to say the list is truncated if it's bigger than the max.

There's a validation in the current code checking the length.

The docs seem to say the list is truncated if it's bigger than the max.

Yeah, the From implementation unfortunately truncates for some reason. However, the Decode impl does check: https://docs.rs/ssz_types/0.10.0/src/ssz_types/variable_list.rs.html#274-313

jking-aus

nice -- good discussion

diegomrsantos force-pushed the subscribe-to-topic branch from 4773cb0 to 2172c8d Compare December 24, 2024 13:45

diegomrsantos force-pushed the subscribe-to-topic branch 4 times, most recently from 9c09396 to cf4576e Compare January 8, 2025 14:54

diegomrsantos changed the title ~~Subscribe to topic~~ Subscribe to topic and decode SignedSSVMessage Jan 8, 2025

diegomrsantos added the network label Jan 13, 2025

diegomrsantos added 4 commits January 16, 2025 11:26

subscribe to topic

bdb53a6

make validation permissive

6ad4fa4

add SSVMessage

3fff562

decode SignedSSVMessage received from gossipsub

44d28bf

diegomrsantos force-pushed the subscribe-to-topic branch from 69c40c7 to 44d28bf Compare January 16, 2025 10:27

fix Cargo.lock

1113182

diegomrsantos marked this pull request as ready for review January 16, 2025 10:47

diegomrsantos requested review from AgeManning, dknopik, jking-aus and jxs January 16, 2025 10:48

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

diegomrsantos self-assigned this Jan 16, 2025

dknopik reviewed Jan 16, 2025

View reviewed changes

diegomrsantos added 2 commits January 16, 2025 14:41

fix logs

4ed0ccd

impl TryFrom<u64> for MsgType

bc48f41

jking-aus approved these changes Jan 20, 2025

View reviewed changes

jking-aus merged commit 3cd5894 into sigp:unstable Jan 20, 2025
9 checks passed

Comments

Conversation

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

Uh oh!

Issue Addressed

Proposed Changes

Additional Info

Uh oh!

dknopik 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!

Choose a reason for hiding this comment

Uh oh!

dknopik Jan 16, 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

Uh oh!

Choose a reason for hiding this comment

Uh oh!

Choose a reason for hiding this comment

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 Dec 24, 2024 •

edited

Loading

dknopik Jan 16, 2025 •

edited

Loading