From 3399bc09c60ea7de8766fef760a04c5a6b9ada15 Mon Sep 17 00:00:00 2001
From: Alexandru Vasile <60601340+lexnv@users.noreply.github.com>
Date: Thu, 16 May 2024 15:01:29 +0300
Subject: [PATCH] network/discovery: Add to DHT only peers that support
genesis-based protocol (#3833)
MIME-Version: 1.0
Content-Type: text/plain; charset=UTF-8
Content-Transfer-Encoding: 8bit
This PR adds to the DHT only the peers that support the genesis/fork/kad
protocol.
Before this PR, any peer that supported the legacy `/kad/[id]` protocol
was added to the DHT.
This is the first step in removing the support for the legacy kad
protocols.
While I have adjusted unit tests to validate the appropriate behavior,
this still needs proper testing in our stack.
Part of https://github.com/paritytech/polkadot-sdk/issues/504.
cc @paritytech/networking
---------
Signed-off-by: Alexandru Vasile <alexandru.vasile@parity.io>
Co-authored-by: Bastian Köcher <git@kchr.de>
---
substrate/client/network/src/discovery.rs | 117 ++++++++++++++----
.../client/network/src/litep2p/discovery.rs | 13 +-
2 files changed, 105 insertions(+), 25 deletions(-)
diff --git a/substrate/client/network/src/discovery.rs b/substrate/client/network/src/discovery.rs
index 4e2121c5540..7d4481b0d06 100644
--- a/substrate/client/network/src/discovery.rs
+++ b/substrate/client/network/src/discovery.rs
@@ -105,7 +105,8 @@ pub struct DiscoveryConfig {
discovery_only_if_under_num: u64,
enable_mdns: bool,
kademlia_disjoint_query_paths: bool,
- kademlia_protocols: Vec<Vec<u8>>,
+ kademlia_protocol: Vec<u8>,
+ kademlia_legacy_protocol: Vec<u8>,
kademlia_replication_factor: NonZeroUsize,
}
@@ -121,7 +122,8 @@ impl DiscoveryConfig {
discovery_only_if_under_num: std::u64::MAX,
enable_mdns: false,
kademlia_disjoint_query_paths: false,
- kademlia_protocols: Vec::new(),
+ kademlia_protocol: Vec::new(),
+ kademlia_legacy_protocol: Vec::new(),
kademlia_replication_factor: NonZeroUsize::new(DEFAULT_KADEMLIA_REPLICATION_FACTOR)
.expect("value is a constant; constant is non-zero; qed."),
}
@@ -177,9 +179,8 @@ impl DiscoveryConfig {
fork_id: Option<&str>,
protocol_id: &ProtocolId,
) -> &mut Self {
- self.kademlia_protocols = Vec::new();
- self.kademlia_protocols.push(kademlia_protocol_name(genesis_hash, fork_id));
- self.kademlia_protocols.push(legacy_kademlia_protocol_name(protocol_id));
+ self.kademlia_protocol = kademlia_protocol_name(genesis_hash, fork_id);
+ self.kademlia_legacy_protocol = legacy_kademlia_protocol_name(protocol_id);
self
}
@@ -207,14 +208,19 @@ impl DiscoveryConfig {
discovery_only_if_under_num,
enable_mdns,
kademlia_disjoint_query_paths,
- kademlia_protocols,
+ kademlia_protocol,
+ kademlia_legacy_protocol,
kademlia_replication_factor,
} = self;
- let kademlia = if !kademlia_protocols.is_empty() {
+ let kademlia = if !kademlia_protocol.is_empty() {
let mut config = KademliaConfig::default();
config.set_replication_factor(kademlia_replication_factor);
+ // Populate kad with both the legacy and the new protocol names.
+ // Remove the legacy protocol:
+ // https://github.com/paritytech/polkadot-sdk/issues/504
+ let kademlia_protocols = [kademlia_protocol.clone(), kademlia_legacy_protocol];
config.set_protocol_names(kademlia_protocols.into_iter().map(Into::into).collect());
// By default Kademlia attempts to insert all peers into its routing table once a
// dialing attempt succeeds. In order to control which peer is added, disable the
@@ -266,6 +272,7 @@ impl DiscoveryConfig {
.expect("value is a constant; constant is non-zero; qed."),
),
records_to_publish: Default::default(),
+ kademlia_protocol,
}
}
}
@@ -309,6 +316,11 @@ pub struct DiscoveryBehaviour {
/// did not return the record(in `FinishedWithNoAdditionalRecord`). We will then put the record
/// to these peers.
records_to_publish: HashMap<QueryId, Record>,
+ /// The chain based kademlia protocol name (including genesis hash and fork id).
+ ///
+ /// Remove when all nodes are upgraded to genesis hash and fork ID-based Kademlia:
+ /// <https://github.com/paritytech/polkadot-sdk/issues/504>.
+ kademlia_protocol: Vec<u8>,
}
impl DiscoveryBehaviour {
@@ -366,23 +378,29 @@ impl DiscoveryBehaviour {
return
}
- if let Some(matching_protocol) = supported_protocols
+ // The supported protocols must include the chain-based Kademlia protocol.
+ //
+ // Extract the chain-based Kademlia protocol from `kademlia.protocol_name()`
+ // when all nodes are upgraded to genesis hash and fork ID-based Kademlia:
+ // https://github.com/paritytech/polkadot-sdk/issues/504.
+ if !supported_protocols
.iter()
- .find(|p| kademlia.protocol_names().iter().any(|k| k.as_ref() == p.as_ref()))
+ .any(|p| p.as_ref() == self.kademlia_protocol.as_slice())
{
- trace!(
- target: "sub-libp2p",
- "Adding self-reported address {} from {} to Kademlia DHT {}.",
- addr, peer_id, String::from_utf8_lossy(matching_protocol.as_ref()),
- );
- kademlia.add_address(peer_id, addr.clone());
- } else {
trace!(
target: "sub-libp2p",
"Ignoring self-reported address {} from {} as remote node is not part of the \
Kademlia DHT supported by the local node.", addr, peer_id,
);
+ return
}
+
+ trace!(
+ target: "sub-libp2p",
+ "Adding self-reported address {} from {} to Kademlia DHT.",
+ addr, peer_id
+ );
+ kademlia.add_address(peer_id, addr.clone());
}
}
@@ -1075,17 +1093,20 @@ mod tests {
.unwrap();
// Test both genesis hash-based and legacy
// protocol names.
- let protocol_name = if swarm_n % 2 == 0 {
- kademlia_protocol_name(genesis_hash, fork_id)
+ let protocol_names = if swarm_n % 2 == 0 {
+ vec![kademlia_protocol_name(genesis_hash, fork_id)]
} else {
- legacy_kademlia_protocol_name(&protocol_id)
+ vec![
+ legacy_kademlia_protocol_name(&protocol_id),
+ kademlia_protocol_name(genesis_hash, fork_id),
+ ]
};
swarms[swarm_n]
.0
.behaviour_mut()
.add_self_reported_address(
&other,
- &[protocol_name],
+ protocol_names.as_slice(),
addr,
);
@@ -1181,9 +1202,56 @@ mod tests {
&[kademlia_protocol_name(supported_genesis_hash, None)],
remote_addr.clone(),
);
+ {
+ let kademlia = discovery.kademlia.as_mut().unwrap();
+ assert!(
+ !kademlia
+ .kbucket(remote_peer_id)
+ .expect("Remote peer id not to be equal to local peer id.")
+ .is_empty(),
+ "Expect peer with supported protocol to be added."
+ );
+ }
+
+ let unsupported_peer_id = predictable_peer_id(b"00000000000000000000000000000002");
+ let unsupported_peer_addr: Multiaddr = "/memory/2".parse().unwrap();
+
+ // Check the unsupported peer is not present before and after the call.
+ {
+ let kademlia = discovery.kademlia.as_mut().unwrap();
+ assert!(
+ kademlia
+ .kbucket(unsupported_peer_id)
+ .expect("Remote peer id not to be equal to local peer id.")
+ .is_empty(),
+ "Expect unsupported peer not to be added."
+ );
+ }
+ // Note: legacy protocol is not supported without genesis hash and fork ID,
+ // if the legacy is the only protocol supported, then the peer will not be added.
+ discovery.add_self_reported_address(
+ &unsupported_peer_id,
+ &[legacy_kademlia_protocol_name(&supported_protocol_id)],
+ unsupported_peer_addr.clone(),
+ );
+ {
+ let kademlia = discovery.kademlia.as_mut().unwrap();
+ assert!(
+ kademlia
+ .kbucket(unsupported_peer_id)
+ .expect("Remote peer id not to be equal to local peer id.")
+ .is_empty(),
+ "Expect unsupported peer not to be added."
+ );
+ }
+
+ // Supported legacy and genesis based protocols are allowed to be added.
discovery.add_self_reported_address(
&another_peer_id,
- &[legacy_kademlia_protocol_name(&supported_protocol_id)],
+ &[
+ legacy_kademlia_protocol_name(&supported_protocol_id),
+ kademlia_protocol_name(supported_genesis_hash, None),
+ ],
another_addr.clone(),
);
@@ -1194,6 +1262,13 @@ mod tests {
kademlia.kbuckets().fold(0, |acc, bucket| acc + bucket.num_entries()),
"Expect peers with supported protocol to be added."
);
+ assert!(
+ !kademlia
+ .kbucket(another_peer_id)
+ .expect("Remote peer id not to be equal to local peer id.")
+ .is_empty(),
+ "Expect peer with supported protocol to be added."
+ );
}
}
}
diff --git a/substrate/client/network/src/litep2p/discovery.rs b/substrate/client/network/src/litep2p/discovery.rs
index 47a620db132..2120ea7c615 100644
--- a/substrate/client/network/src/litep2p/discovery.rs
+++ b/substrate/client/network/src/litep2p/discovery.rs
@@ -268,10 +268,10 @@ impl Discovery {
allow_non_global_addresses: config.allow_non_globals_in_dht,
public_addresses: config.public_addresses.iter().cloned().collect(),
next_kad_query: Some(Delay::new(KADEMLIA_QUERY_INTERVAL)),
- local_protocols: HashSet::from_iter([
- kademlia_protocol_name(genesis_hash, fork_id),
- legacy_kademlia_protocol_name(protocol_id),
- ]),
+ local_protocols: HashSet::from_iter([kademlia_protocol_name(
+ genesis_hash,
+ fork_id,
+ )]),
},
ping_config,
identify_config,
@@ -295,6 +295,11 @@ impl Discovery {
addresses: Vec<Multiaddr>,
) {
if self.local_protocols.is_disjoint(&supported_protocols) {
+ log::trace!(
+ target: "sub-libp2p",
+ "Ignoring self-reported address of peer {peer} as remote node is not part of the \
+ Kademlia DHT supported by the local node.",
+ );
return
}
--
GitLab