Implement SSV Handshake Protocol #125

diegomrsantos · 2025-02-04T20:17:16Z

Issue Addressed

Implements #116

Proposed Changes

Dependencies:
- Updated dependencies in Cargo.toml including async-trait, quick-protobuf, serde_json, and thiserror.
Handshake Protocol:
- Added a new module handshake with multiple new files including README.md, codec.rs, envelope.proto, envelope.rs, mod.rs, and node_info.rs.
Network Behaviour:
- Enhanced behaviour.rs to include handshake::Behaviour and added a new field handshake to AnchorBehaviour.
Network Implementation:
- Enhanced network.rs:
- Updated to use the Handshake Behaviour
- Created DefaultNodeInfoProvider to provide NodeInfo.

Additional Info

There's a spec in README.md file

diegomrsantos · 2025-02-04T22:21:06Z

anchor/network/src/network.rs

+        false,
+    );
+
+    let domain = format!("0x{}", hex::encode(vec![0x0, 0x0, 0x5, 0x2]));


This is based on https://github.com/ssvlabs/ssv/blob/main/network/p2p/p2p_setup.go#L181 and https://github.com/ssvlabs/ssv/blob/main/networkconfig/holesky.go#L14

This needs to be configurable per network - so it should be integrated into the ssv_network_config stuff

jxs

Hi Diego, thanks for starting this.
Left some comments

jxs · 2025-02-05T15:32:46Z

anchor/network/Cargo.toml

 task_executor = { workspace = true }
 tokio = { workspace = true }
 tracing = { workspace = true }
 types = { workspace = true }
 version = { workspace = true }
+serde_json = "1.0.137"
+thiserror = "1.0.69"
+prost = "0.13.4"


we use quick-protobuf in libp2p which is also available in the dependency chain, so you shouldn't need to depend on prost which is also helpful as you don't need to specifiy prost-build in the build dependencies

I changed to quick-protobuf. But I found prost much easier to use and straightforward. It also seems to be more actively maintained.

anchor/network/src/behaviour.rs

anchor/network/src/handshake/behaviour.rs

jxs · 2025-02-05T15:43:25Z

anchor/network/src/handshake/behaviour.rs

+}
+
+/// Network behaviour handling the handshake protocol.
+pub struct HandshakeBehaviour {


skimming through the NetworkBehaviour implementation, and without knowing much about the requirements I wonder if we can suffice our needs by just implementing the Codec trait and not needing to have request-response as a sub behaviour

What would this approach look like?

using the request-response Behaviour and then doing the validation envelope signing validation logic in the Codec implementation.
Does that make sense Diego?

Yes, having the validation in the codec is the way to go. About only one behavior, I'll need to think more about it.

I think it's nice to keep the behavior and centralize everything related to the handshake there, exposing only the events to the main behavior.

anchor/network/src/handshake/error.rs

jxs · 2025-02-05T15:46:32Z

anchor/network/src/handshake/error.rs

+use thiserror::Error;
+
+#[derive(Error, Debug)]
+pub enum HandshakeError {


do we need thiserror::Error for our Errors?
We are probably fine having an Enum and impl'ing Display and Error for it like request_response::OutboundFailure does it for example.
Nonetheless since it's probably already in the dependency chain feel free to ignore this

This needs to be revisited, but It'll probably be removed.

Another advantage is the #[from] see dc79ba7

anchor/network/src/handshake/codec.rs

anchor/network/src/handshake/record/record.rs

# Conflicts: # Cargo.lock # anchor/network/Cargo.toml

dknopik · 2025-02-10T13:14:47Z

anchor/network/Cargo.toml

 ssz_types = "0.10"
 subnet_tracker = { workspace = true }
 task_executor = { workspace = true }
+thiserror = "1.0.69"


Should use workspace dependencies

It's not defined in the root. Should it be defined there and inherited even if it's used only here for now?

hmm, good question. We should align with the others on that

rule of thumb I use: if it's only used by a subcrate import it there, when it becomes used by two or more, move it to workspace

I've been taking the approach to keep it local if its unlikely to be used in other crates.

If its a general crate, its generally best to put in the workspace dependencies, in case for some reason some other crate decides to use it.

For this specific one, it seems fairly general and conceivable that another crate MIGHT use it in the future, so might be worth chucking in workspace deps.

anchor/network/src/behaviour.rs

anchor/network/src/handshake/node_info.rs

dknopik · 2025-02-10T13:41:43Z

anchor/network/src/network.rs

+        false,
+    );
+
+    let domain = format!("0x{}", hex::encode(vec![0x0, 0x0, 0x5, 0x2]));


This needs to be configurable per network - so it should be integrated into the ssv_network_config stuff

dknopik · 2025-02-10T13:43:08Z

anchor/network/src/network.rs

+pub struct DefaultNodeInfoProvider {
+    node_info: Arc<Mutex<NodeInfo>>,
+}
+
+impl DefaultNodeInfoProvider {
+    pub fn new(node_info: NodeInfo) -> Self {
+        Self {
+            node_info: Arc::new(Mutex::new(node_info)),
+        }
+    }
+}
+
+impl NodeInfoProvider for DefaultNodeInfoProvider {
+    fn get_node_info(&self) -> NodeInfo {
+        // In a real implementation, consider handling lock poisoning.
+        self.node_info.lock().unwrap().clone()
+    }
+}
+


I guess this is a todo for another PR?

was going to ask similar, do we need the NodeInfoProvider trait for this? can't we just use NodeInfo?

Might be better to do a read/write mutex if we are doing lots of reads. From this snippet of code, it looks like we can't deadlock.

This reverts commit c75a225.

jxs

Hi Diego, thanks for driving this! Left some comments

jxs · 2025-02-10T15:43:00Z

anchor/network/Cargo.toml

 ssz_types = "0.10"
 subnet_tracker = { workspace = true }
 task_executor = { workspace = true }
+thiserror = "1.0.69"


rule of thumb I use: if it's only used by a subcrate import it there, when it becomes used by two or more, move it to workspace

jxs · 2025-02-10T15:45:51Z

anchor/network/src/handshake/README.md

@@ -0,0 +1,272 @@
+# SSV NodeInfo Handshake Protocol Specification


this is awesome work Diego, thanks for the time putting in elaborating this document ❤️
This feels a spec, what do you think about upstreaming it to the spec repo so that it lives there?
So that we proper PR this document and it becomes canon

jxs · 2025-02-10T15:47:15Z

anchor/network/src/handshake/envelope/codec.rs

+    }
+}
+
+/// A `Codec` that reads/writes an **`Envelope`**


Suggested change

/// A `Codec` that reads/writes an **`Envelope`**

/// A `Codec` that reads/writes an **`Envelope`**.

jxs · 2025-02-10T15:47:55Z

anchor/network/src/handshake/envelope/codec.rs

+pub struct Codec;
+
+#[async_trait]
+impl RequestResponseCodec for Codec {


nit:

Suggested change

impl RequestResponseCodec for Codec {

impl request_response::Codec for Codec {

jxs · 2025-02-10T15:48:12Z

anchor/network/src/handshake/envelope/codec.rs

+
+const MAXIMUM_SIZE: u64 = 1024;
+
+impl From<envelope::Error> for io::Error {


do we need this conversion?

jxs · 2025-02-10T15:49:29Z

anchor/network/src/handshake/envelope/codec.rs

+    where
+        T: AsyncRead + Unpin + Send,
+    {
+        debug!("reading handsake request");


nit, we probably can lower these ones to trace! and leave a single debug! logging the request received

I agree. My idea for now is to leave everything as debug as it makes debugging while developing easier and when the system is more stable revisit the logs. What do you think?

jxs · 2025-02-10T15:52:25Z

anchor/network/src/handshake/envelope/envelope.proto

@@ -0,0 +1,8 @@
+syntax = "proto3";


can we create a dir generated that contains the protobuf file and the generated rust code?
So that it follows the upstream protocol conventions?

jxs · 2025-02-10T16:01:00Z

anchor/network/src/network.rs

+pub struct DefaultNodeInfoProvider {
+    node_info: Arc<Mutex<NodeInfo>>,
+}
+
+impl DefaultNodeInfoProvider {
+    pub fn new(node_info: NodeInfo) -> Self {
+        Self {
+            node_info: Arc::new(Mutex::new(node_info)),
+        }
+    }
+}
+
+impl NodeInfoProvider for DefaultNodeInfoProvider {
+    fn get_node_info(&self) -> NodeInfo {
+        // In a real implementation, consider handling lock poisoning.
+        self.node_info.lock().unwrap().clone()
+    }
+}
+


was going to ask similar, do we need the NodeInfoProvider trait for this? can't we just use NodeInfo?

jxs · 2025-02-10T16:16:43Z

anchor/network/src/handshake/mod.rs

+            .on_connection_handler_event(peer_id, connection_id, event);
+    }
+
+    fn poll(


implementing poll complicates things where I feel it's not needed.
following what I wrote on to what I wrote in #125 (comment), for example, by directly calling request_response::poll we need to bubble up all it's ToSwarm events, therefore we cannot have the match wildcard arm with empty curlies as it's being done.

Here we are only listening on request_response::Message::Request and request_response::Message::Response, then we bubble up RequestResponseEvent::OutboundFailure and RequestResponseEvent::InboundFailure re written as Error.

We can still isolate the validation and Codec logic here both if we:

bubble up request-response::Message and call handshake::handle_handshake_request and handshake::handle_handshake_response in the network::handle_handshake_event that you already wrote

or

add the Keypair and NodeInfo to the Codec and have the validation logic there

jxs · 2025-02-11T00:05:49Z

anchor/network/src/handshake/mod.rs

+    ) {
+        // Handle incoming request: send response then verify
+        let response = self.sealed_node_record();
+        let _ = self.behaviour.send_response(channel, response.clone()); // Any error here is handled by the InboundFailure handler


why are we responding here instead of on Codec::write_response?

Codec::write_response is called right after we receive a request, see here.

write_response accepts the response that you are sending here via ResponseChannel piggybacking on the suggestion above wrt to poll we can move the sealed_node_record function to the Codec wdyt?

dknopik · 2025-02-11T11:07:00Z

anchor/client/src/config.rs

@@ -16,7 +16,7 @@ pub const DEFAULT_EXECUTION_NODE_WS: &str = "ws://localhost:8545/";
 /// The default Data directory, relative to the users home directory
 pub const DEFAULT_ROOT_DIR: &str = ".anchor";
 /// Default network, used to partition the data storage
-pub const DEFAULT_HARDCODED_NETWORK: &str = "mainnet";
+pub const DEFAULT_HARDCODED_NETWORK: &str = "holesky";


We should not push this to the main branch

Should the default network be mainnet while in development?

dknopik · 2025-02-11T11:07:58Z

anchor/client/src/config.rs

        SsvNetworkConfig::load(testnet_dir.clone())
    } else {
+        println!("Loading default network {:?}", cli_args.network);


Use tracing.

Also, the network is not necessarily the default network, but can also be the network passed via --network.

I didn't mean to commit it, it was a mistake. I just wanted to see what the default network was. Do we need a log here?

dknopik · 2025-02-11T11:16:00Z

anchor/common/ssv_network_config/src/lib.rs

@@ -32,11 +33,12 @@ pub struct SsvNetworkConfig {
    pub ssv_boot_nodes: Option<Vec<Enr<CombinedKey>>>,
    pub ssv_contract: Address,
    pub ssv_contract_block: u64,
+    pub ssv_domain_type: String


I feel like it would be nicer to convert to our target type here already: a [u8; 4]. Then we catch config mistakes early. :)

Could you elaborate more on the target type?

dknopik · 2025-02-11T11:16:36Z

anchor/common/ssv_network_config/src/lib.rs

@@ -76,6 +81,7 @@ impl SsvNetworkConfig {
            ssv_boot_nodes,
            ssv_contract: read(&base_dir.join("ssv_contract_address.txt"))?,
            ssv_contract_block: read(&base_dir.join("ssv_contract_block.txt"))?,
+            ssv_domain_type: read(&base_dir.join("domain_type.txt"))?,


ssv_domain_type.txt

dknopik · 2025-02-11T11:17:31Z

anchor/network/src/config.rs

@@ -66,6 +66,8 @@ pub struct Config {

    /// Target number of connected peers.
    pub target_peers: usize,
+
+    pub domain_type: String,


I think it would be better to pass the SsvNetworkConfig into the network crate instead of duplicating the field here.

diegomrsantos commented Feb 4, 2025

View reviewed changes

jxs reviewed Feb 5, 2025

View reviewed changes

diegomrsantos force-pushed the handshake branch 5 times, most recently from fdd1b40 to 949101a Compare February 7, 2025 11:35

diegomrsantos added 23 commits February 7, 2025 16:30

handshake

e5a10ad

remove length prefixing

1b36113

fixing serialization

8dfba38

fix tag

fec6579

delete node_info from network types

2f898bc

remove unnecessary deps

335de8c

# Conflicts: # Cargo.lock # anchor/network/Cargo.toml

handle handshake event

537a988

many fixes

5162ae6

remove record

60c1c13

delete record mod and move things around

98b97bc

improve error handling

1d41a3e

remove pending_handshakes

b2bd4bf

fix wrong network id

573dd54

add NotifyHandler again

0114cdc

small change

e65e5b4

use NodeInfoProvider trait

5d2f6df

rename HandshakeEvent

97f6534

rename HandshakeBehaviour

f755ea5

move behaviour to mod.rs

8f2e545

create envelope mod

d0ab738

improve error handling

88e8a92

simplify error with thiserror

0c42c26

improve error handling

c096635

diegomrsantos added 5 commits February 7, 2025 16:35

rename HandshakeError

71f0de5

cargo clippy

7f47b7d

fix request handling

6411d4c

fix problems after rebase

288e0a2

use quick-protobuf

4be67aa

diegomrsantos force-pushed the handshake branch from 7cde7b3 to 4be67aa Compare February 10, 2025 10:50

diegomrsantos added 3 commits February 10, 2025 11:52

cargo fmt

cb9363c

cargo sort

399499c

cargo clippy

3ac485b

diegomrsantos self-assigned this Feb 10, 2025

diegomrsantos added the network label Feb 10, 2025

diegomrsantos added 3 commits February 10, 2025 13:56

limit the msg payload while reading

2a47eb0

protocol spec

a2611e1

remove deps

b53d49e

diegomrsantos marked this pull request as ready for review February 10, 2025 13:12

diegomrsantos requested a review from jxs February 10, 2025 13:12

diegomrsantos changed the title ~~Handshake~~ Implement SSV Handshake Protocol Feb 10, 2025

diegomrsantos added 2 commits February 10, 2025 14:16

remove println!

aab349e

update comments

cc5d0e3

dknopik reviewed Feb 10, 2025

View reviewed changes

update wordlist.txt

c26750e

diegomrsantos force-pushed the handshake branch from f269ae4 to c26750e Compare February 10, 2025 13:57

diegomrsantos added 3 commits February 10, 2025 15:01

ignore case in spellcheck

c75a225

Revert "ignore case in spellcheck"

d9791ce

This reverts commit c75a225.

changes after review

40dc739

diegomrsantos force-pushed the handshake branch from 27cbb40 to 40dc739 Compare February 10, 2025 15:14

use ssv_network_config

ca67c05

jxs reviewed Feb 11, 2025

View reviewed changes

dknopik reviewed Feb 11, 2025

View reviewed changes

		@@ -0,0 +1,272 @@
		# SSV NodeInfo Handshake Protocol Specification

	/// A `Codec` that reads/writes an `Envelope`
	/// A `Codec` that reads/writes an `Envelope`.

	impl RequestResponseCodec for Codec {
	impl request_response::Codec for Codec {


		const MAXIMUM_SIZE: u64 = 1024;

		impl From<envelope::Error> for io::Error {

Implement SSV Handshake Protocol #125

Are you sure you want to change the base?

Implement SSV Handshake Protocol #125

Conversation

diegomrsantos commented Feb 4, 2025 • edited Loading

Issue Addressed

Proposed Changes

Additional Info

diegomrsantos Feb 4, 2025 • edited Loading

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

jxs left a 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

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 7, 2025 • edited Loading

Choose a reason for hiding this comment

Choose a reason for hiding this comment

diegomrsantos Feb 10, 2025 • edited Loading

Choose a reason for hiding this comment

dknopik Feb 10, 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

Choose a reason for hiding this comment

jxs left a 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

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

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 11, 2025 • edited Loading

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

diegomrsantos commented Feb 4, 2025 •

edited

Loading

diegomrsantos Feb 4, 2025 •

edited

Loading

diegomrsantos Feb 7, 2025 •

edited

Loading

diegomrsantos Feb 10, 2025 •

edited

Loading

dknopik Feb 10, 2025 •

edited

Loading

diegomrsantos Feb 11, 2025 •

edited

Loading