-
Notifications
You must be signed in to change notification settings - Fork 5
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
Introduce identify
and rendezvous
network protocols
#304
Conversation
Codecov ReportPatch coverage:
Additional details and impacted files@@ Coverage Diff @@
## main #304 +/- ##
==========================================
- Coverage 92.42% 90.94% -1.48%
==========================================
Files 51 52 +1
Lines 3987 4065 +78
==========================================
+ Hits 3685 3697 +12
- Misses 302 368 +66
Help us with your feedback. Take ten seconds to tell us how you rate us. Have a feature suggestion? Share it here. ☔ View full report in Codecov by Sentry. |
…tion establishment
@@ -13,6 +14,9 @@ use crate::network::identity::Identity; | |||
/// Key pair file name. | |||
const KEY_PAIR_FILE_NAME: &str = "private-key"; | |||
|
|||
/// The namespace used by the `identify` network behaviour. | |||
pub const NODE_NAMESPACE: &str = "aquadoggo"; |
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
We might want to make this user-definable in the future. The namespace value is used when registering a rendezvous client with a rendezvous server and querying for other registered peers.
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
Yes, ideally that parameter would be reused by other discovery mechanisms, like mDNS (probably not a "namespace" there but something similar?)
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
Agreed, sharing a common identifier would be best. mDNS uses a 'service name' which is currently not user-configurable in the Rust implementation (see libp2p/rust-libp2p#2898 for more detail).
@@ -1,5 +1,6 @@ | |||
// SPDX-License-Identifier: AGPL-3.0-or-later | |||
|
|||
mod behaviour; |
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
Moving the network behaviour into a module makes the service less busy.
let remote: Multiaddr = addr.parse()?; | ||
swarm.dial(remote)?; | ||
// Dial each peer identified by the multi-address provided via `--remote-node-addresses` if given | ||
for addr in network_config.remote_peers.clone() { |
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
Ensure we dial all remote nodes and not just the first one in the list.
aquadoggo/src/network/service.rs
Outdated
swarm | ||
.behaviour_mut() | ||
.rendezvous_client | ||
.as_mut() |
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
This looks messy, unfortunately. We have to call .as_mut()
because the rendezvous_client
behaviour is behind a Toggle
type.
@@ -140,13 +110,34 @@ pub async fn network_service( | |||
} => { | |||
debug!("ConnectionClosed: {peer_id} {endpoint:?} {num_established} {cause:?}") | |||
} | |||
SwarmEvent::ConnectionEstablished { peer_id, .. } | |||
if network_config.rendezvous_client | |||
&& network_config.rendezvous_peer_id.unwrap() == peer_id => |
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
We should always be safe to unwrap the rendezvous_peer_id
here because the CLI will ensure it's provided if rendezvous_client
was set to true
.
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
Give it a comment in the code 👍🏻
address.clone() | ||
}; | ||
|
||
swarm.dial(address_with_p2p).unwrap(); |
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
Dial every peer we learn about from the rendezvous server.
@@ -25,7 +27,7 @@ struct Cli { | |||
|
|||
/// URLs of remote nodes to replicate with. | |||
#[arg(short, long)] | |||
remote_node_addresses: Vec<String>, | |||
remote_node_addresses: Vec<Multiaddr>, |
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
By using Multiaddr
and PeerId
(instead of String
) we get validation of the input automatically :) No need to try and .parse()
later on.
This is now ready for review. I've left some comments to explain bits and pieces. I'm interested to hear what you think of the In a future PR, I'm quite keen to separate the swarm event loop into handlers to make it a bit neater. So, for example, there would be a |
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
Looking all very good, mostly just small questions
@@ -13,6 +14,9 @@ use crate::network::identity::Identity; | |||
/// Key pair file name. | |||
const KEY_PAIR_FILE_NAME: &str = "private-key"; | |||
|
|||
/// The namespace used by the `identify` network behaviour. | |||
pub const NODE_NAMESPACE: &str = "aquadoggo"; |
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
Yes, ideally that parameter would be reused by other discovery mechanisms, like mDNS (probably not a "namespace" there but something similar?)
@@ -140,13 +110,34 @@ pub async fn network_service( | |||
} => { | |||
debug!("ConnectionClosed: {peer_id} {endpoint:?} {num_established} {cause:?}") | |||
} | |||
SwarmEvent::ConnectionEstablished { peer_id, .. } | |||
if network_config.rendezvous_client | |||
&& network_config.rendezvous_peer_id.unwrap() == peer_id => |
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
Give it a comment in the code 👍🏻
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
All looks really good to me, don't have any change requests!
👏 👏 👏
@@ -71,7 +75,23 @@ pub struct NetworkConfiguration { | |||
pub quic_port: u16, | |||
|
|||
/// The addresses of remote peers to replicate from. | |||
pub remote_peers: Vec<String>, | |||
pub remote_peers: Vec<Multiaddr>, |
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
Nice!
Addresses #303
NetworkBehaviour
into a separate module (behaviour.rs
)identify
network behaviour / protocolidentify::Event::Received
eventidentify::Event::Sent
eventidentify::Event::Error
eventrendezvous
network behaviour / protocolrendezvous::client::Event::Registered
eventrendezvous::client::Event::Discovered
eventrendezvous::client::Event::Failed
eventrendezvous::server::Event::PeerRegistered
eventrendezvous::server::Event::DiscoverServed
eventString
withMultiaddr
for--remote-node-addresses
and--rendezvous-address
(results in validation by parser)String
withPeerId
for--rendezvous-peer-id
(results in validation by parser)glyph
to list of authorsREADME
with latest usage instructions📋 Checklist
CHANGELOG.md