Skip to content
New issue

Have a question about this project? Sign up for a free GitHub account to open an issue and contact its maintainers and the community.

By clicking “Sign up for GitHub”, you agree to our terms of service and privacy statement. We’ll occasionally send you account related emails.

Already on GitHub? Sign in to your account

Implement SSV Handshake Protocol #125

Merged
merged 65 commits into from
Feb 14, 2025
Merged

Conversation

diegomrsantos
Copy link

@diegomrsantos diegomrsantos commented Feb 4, 2025

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

Copy link
Member

@jxs jxs left a comment

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

Hi Diego, thanks for starting this.
Left some comments

anchor/network/Cargo.toml Outdated Show resolved Hide resolved
anchor/network/src/behaviour.rs Outdated Show resolved Hide resolved
anchor/network/src/handshake/behaviour.rs Outdated Show resolved Hide resolved
anchor/network/src/handshake/behaviour.rs Outdated Show resolved Hide resolved
anchor/network/src/handshake/behaviour.rs Outdated Show resolved Hide resolved
}

/// Network behaviour handling the handshake protocol.
pub struct HandshakeBehaviour {
Copy link
Member

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

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

Copy link
Author

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

What would this approach look like?

Copy link
Member

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

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

Copy link
Author

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

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

Copy link
Author

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

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 Outdated Show resolved Hide resolved
anchor/network/src/handshake/error.rs Outdated Show resolved Hide resolved
anchor/network/src/handshake/codec.rs Outdated Show resolved Hide resolved
anchor/network/src/handshake/record/record.rs Outdated Show resolved Hide resolved
@diegomrsantos diegomrsantos force-pushed the handshake branch 5 times, most recently from fdd1b40 to 949101a Compare February 7, 2025 11:35
@diegomrsantos diegomrsantos force-pushed the handshake branch 2 times, most recently from 67c7cab to 7f74282 Compare February 13, 2025 14:27
@dknopik
Copy link
Member

dknopik commented Feb 14, 2025

Hey @diegomrsantos, hey @jxs,

thank you for your discussion in this PR.

You are both making a good case for your proposed variants. To better visualise the changes proposed by João (as I understand them), I implemented them here based on this PR.

To summarize the changes:

  • Move decoding of NodeInfo to Codec. This, in my opinion, is a good idea. As you said, Diego, it is good to separate the coding and decoding logic from the rest of the logic. With this change, the conversion of NodeInfo to the "Entries"-based JSON format is moved to that separated codec logic. This is desirable if I understood your argument correctly.
  • Do not use request_response::Behaviour as a sub behaviour, but as a top-level behaviour. This has, IMO, advantages and disadvantages. Handling the event on swarm level loosens encapsulation as said by Diego. I think this is a bit mitigated by moving the actual logic into the handshake module as I did. The advantage is reduced complexity by not having to impl NetworkBehaviour, and no risk of ignored Wakers or omitted events when poll is called.

Overall, I think I personally prefer this approach. Please take a look at my changes. Of course, I am fine with iterating more on the design. Looking forward to your thoughts.

Copy link
Member

@dknopik dknopik left a comment

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

Thanks for implementing this, especially with the lack of a pre-existing spec! It's also great that we now have a spec.

We'll handle potential improvements separately.

@dknopik dknopik merged commit afa6c0d into sigp:unstable Feb 14, 2025
11 checks passed
@dknopik dknopik mentioned this pull request Feb 14, 2025
Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment
Labels
Projects
None yet
Development

Successfully merging this pull request may close these issues.

4 participants