Skip to content

Commit

Permalink
Remove unsigned version of Discovery protocol (#19936)
Browse files Browse the repository at this point in the history 
---

## Release notes

Check each box that your changes affect. If none of the boxes relate to
your changes, release notes aren't required.

For each box you select, include information after the relevant heading
that describes the impact of your changes that a user might notice and
any actions they must take to implement updates.

- [ ] Protocol: 
- [x] Nodes (Validators and Full nodes): removes unsigned version of
Discovery protocol
- [ ] Indexer: 
- [ ] JSON-RPC: 
- [ ] GraphQL: 
- [ ] CLI: 
- [ ] Rust SDK:
- [ ] REST API:
  • Loading branch information
@aschran
aschran authored Oct 21, 2024
1 parent ee5a129 commit 314960e
Show file tree
Hide file tree
Showing 7 changed files with 83 additions and 186 deletions.
10 changes: 0 additions & 10 deletions crates/sui-config/src/p2p.rs
View file
Original file line number Diff line number Diff line change
Expand Up @@ -319,12 +319,6 @@ pub struct DiscoveryConfig {
/// to this peer, nor advertise this peer's info to other peers in the network.
#[serde(skip_serializing_if = "Vec::is_empty", default)]
pub allowlisted_peers: Vec<AllowlistedPeer>,

/// If true, Discovery will require all provided peer information to be signed
/// by the originating peer.
///
/// If unspecified, this will default to true.
pub enable_node_info_signatures: Option<bool>,
}

impl DiscoveryConfig {
Expand All @@ -351,10 +345,6 @@ impl DiscoveryConfig {
// defaults None to Public
self.access_type.unwrap_or(AccessType::Public)
}

pub fn enable_node_info_signatures(&self) -> bool {
self.enable_node_info_signatures.unwrap_or(true)
}
}

#[derive(Clone, Debug, Default, Deserialize, Serialize)]
Expand Down
9 changes: 0 additions & 9 deletions crates/sui-network/build.rs
View file
Original file line number Diff line number Diff line change
Expand Up @@ -141,15 +141,6 @@ fn build_anemo_services(out_dir: &Path) {
let discovery = anemo_build::manual::Service::builder()
.name("Discovery")
.package("sui")
.method(
anemo_build::manual::Method::builder()
.name("get_known_peers")
.route_name("GetKnownPeers")
.request_type("()")
.response_type("crate::discovery::GetKnownPeersResponse")
.codec_path(codec_path)
.build(),
)
.method(
anemo_build::manual::Method::builder()
.name("get_known_peers_v2")
Expand Down
2 changes: 1 addition & 1 deletion crates/sui-network/src/discovery/builder.rs
View file
Original file line number Diff line number Diff line change
Expand Up @@ -58,7 +58,7 @@ impl Builder {

// Apply rate limits from configuration as needed.
if let Some(limit) = discovery_config.get_known_peers_rate_limit {
discovery_server = discovery_server.add_layer_for_get_known_peers(
discovery_server = discovery_server.add_layer_for_get_known_peers_v2(
InboundRequestLayer::new(rate_limit::RateLimitLayer::new(
governor::Quota::per_second(limit),
rate_limit::WaitMode::Block,
Expand Down
162 changes: 50 additions & 112 deletions crates/sui-network/src/discovery/mod.rs
View file
Original file line number Diff line number Diff line change
Expand Up @@ -47,7 +47,6 @@ pub use generated::{
discovery_client::DiscoveryClient,
discovery_server::{Discovery, DiscoveryServer},
};
pub use server::GetKnownPeersResponse;
pub use server::GetKnownPeersResponseV2;

use self::metrics::Metrics;
Expand Down Expand Up @@ -268,7 +267,6 @@ impl DiscoveryEventLoop {
// Query the new node for any peers
self.tasks.spawn(query_peer_for_their_known_peers(
peer,
self.discovery_config.clone(),
self.state.clone(),
self.metrics.clone(),
self.allowlisted_peers.clone(),
Expand Down Expand Up @@ -424,58 +422,31 @@ async fn try_to_connect_to_seed_peers(

async fn query_peer_for_their_known_peers(
peer: Peer,
config: Arc<DiscoveryConfig>,
state: Arc<RwLock<State>>,
metrics: Metrics,
allowlisted_peers: Arc<HashMap<PeerId, Option<Multiaddr>>>,
) {
let mut client = DiscoveryClient::new(peer);

let request = Request::new(()).with_timeout(TIMEOUT);
let found_peers = if config.enable_node_info_signatures() {
client
.get_known_peers_v2(request)
.await
.ok()
.map(Response::into_inner)
.map(
|GetKnownPeersResponseV2 {
own_info,
mut known_peers,
}| {
if !own_info.addresses.is_empty() {
known_peers.push(own_info)
}
known_peers
},
)
} else {
client
.get_known_peers(request)
.await
.ok()
.map(Response::into_inner)
.map(
|GetKnownPeersResponse {
own_info,
mut known_peers,
}| {
if !own_info.addresses.is_empty() {
known_peers.push(own_info)
}
known_peers
.into_iter()
.map(|info| {
// SignedNodeInfo with fake default signatures will only work if
// signature verification is disabled.
SignedNodeInfo::new_from_data_and_sig(info, Ed25519Signature::default())
})
.collect()
},
)
};
let found_peers = client
.get_known_peers_v2(request)
.await
.ok()
.map(Response::into_inner)
.map(
|GetKnownPeersResponseV2 {
own_info,
mut known_peers,
}| {
if !own_info.addresses.is_empty() {
known_peers.push(own_info)
}
known_peers
},
);
if let Some(found_peers) = found_peers {
update_known_peers(config, state, metrics, found_peers, allowlisted_peers);
update_known_peers(state, metrics, found_peers, allowlisted_peers);
}
}

Expand All @@ -494,57 +465,27 @@ async fn query_connected_peers_for_their_known_peers(
.flat_map(|id| network.peer(id))
.choose_multiple(&mut rand::thread_rng(), config.peers_to_query());

let enable_node_info_signatures = config.enable_node_info_signatures();
let found_peers = peers_to_query
.into_iter()
.map(DiscoveryClient::new)
.map(|mut client| async move {
let request = Request::new(()).with_timeout(TIMEOUT);
if enable_node_info_signatures {
client
.get_known_peers_v2(request)
.await
.ok()
.map(Response::into_inner)
.map(
|GetKnownPeersResponseV2 {
own_info,
mut known_peers,
}| {
if !own_info.addresses.is_empty() {
known_peers.push(own_info)
}
known_peers
},
)
} else {
client
.get_known_peers(request)
.await
.ok()
.map(Response::into_inner)
.map(
|GetKnownPeersResponse {
own_info,
mut known_peers,
}| {
if !own_info.addresses.is_empty() {
known_peers.push(own_info)
}
known_peers
.into_iter()
.map(|info| {
// SignedNodeInfo with fake default signatures will only work if
// signature verification is disabled.
SignedNodeInfo::new_from_data_and_sig(
info,
Ed25519Signature::default(),
)
})
.collect()
},
)
}
client
.get_known_peers_v2(request)
.await
.ok()
.map(Response::into_inner)
.map(
|GetKnownPeersResponseV2 {
own_info,
mut known_peers,
}| {
if !own_info.addresses.is_empty() {
known_peers.push(own_info)
}
known_peers
},
)
})
.pipe(futures::stream::iter)
.buffer_unordered(config.peers_to_query())
Expand All @@ -553,11 +494,10 @@ async fn query_connected_peers_for_their_known_peers(
.collect::<Vec<_>>()
.await;

update_known_peers(config, state, metrics, found_peers, allowlisted_peers);
update_known_peers(state, metrics, found_peers, allowlisted_peers);
}

fn update_known_peers(
config: Arc<DiscoveryConfig>,
state: Arc<RwLock<State>>,
metrics: Metrics,
found_peers: Vec<SignedNodeInfo>,
Expand Down Expand Up @@ -602,24 +542,22 @@ fn update_known_peers(
{
continue;
}
if config.enable_node_info_signatures() {
let Ok(public_key) = Ed25519PublicKey::from_bytes(&peer_info.peer_id.0) else {
debug_fatal!(
// This should never happen.
"Failed to convert anemo PeerId {:?} to Ed25519PublicKey",
peer_info.peer_id
);
continue;
};
let msg = bcs::to_bytes(peer_info.data()).expect("BCS serialization should not fail");
if let Err(e) = public_key.verify(&msg, peer_info.auth_sig()) {
info!(
"Discovery failed to verify signature for NodeInfo for peer {:?}: {e:?}",
peer_info.peer_id
);
// TODO: consider denylisting the source of bad NodeInfo from future requests.
continue;
}
let Ok(public_key) = Ed25519PublicKey::from_bytes(&peer_info.peer_id.0) else {
debug_fatal!(
// This should never happen.
"Failed to convert anemo PeerId {:?} to Ed25519PublicKey",
peer_info.peer_id
);
continue;
};
let msg = bcs::to_bytes(peer_info.data()).expect("BCS serialization should not fail");
if let Err(e) = public_key.verify(&msg, peer_info.auth_sig()) {
info!(
"Discovery failed to verify signature for NodeInfo for peer {:?}: {e:?}",
peer_info.peer_id
);
// TODO: consider denylisting the source of bad NodeInfo from future requests.
continue;
}
let peer = VerifiedSignedNodeInfo::new_from_verified(peer_info);

Expand Down
23 changes: 1 addition & 22 deletions crates/sui-network/src/discovery/server.rs
View file
Original file line number Diff line number Diff line change
@@ -1,7 +1,7 @@
// Copyright (c) Mysten Labs, Inc.
// SPDX-License-Identifier: Apache-2.0

use super::{Discovery, NodeInfo, SignedNodeInfo, State, MAX_PEERS_TO_SEND};
use super::{Discovery, SignedNodeInfo, State, MAX_PEERS_TO_SEND};
use anemo::{Request, Response};
use rand::seq::IteratorRandom;
use serde::{Deserialize, Serialize};
Expand All @@ -10,12 +10,6 @@ use std::{
sync::{Arc, RwLock},
};

#[derive(Clone, Debug, Serialize, Deserialize)]
pub struct GetKnownPeersResponse {
pub own_info: NodeInfo,
pub known_peers: Vec<NodeInfo>,
}

#[derive(Clone, Debug, Serialize, Deserialize)]
pub struct GetKnownPeersResponseV2 {
pub own_info: SignedNodeInfo,
Expand All @@ -28,21 +22,6 @@ pub(super) struct Server {

#[anemo::async_trait]
impl Discovery for Server {
async fn get_known_peers(
&self,
request: Request<()>,
) -> Result<Response<GetKnownPeersResponse>, anemo::rpc::Status> {
let resp = self.get_known_peers_v2(request).await?;
Ok(resp.map(|body| GetKnownPeersResponse {
own_info: body.own_info.into_data(),
known_peers: body
.known_peers
.into_iter()
.map(|e| e.into_data())
.collect(),
}))
}

async fn get_known_peers_v2(
&self,
_request: Request<()>,
Expand Down
Loading

0 comments on commit 314960e

Please sign in to comment.