From 762f824cb3adf23c72a6645f575057bfb543850b Mon Sep 17 00:00:00 2001
From: Alexandru Vasile <60601340+lexnv@users.noreply.github.com>
Date: Tue, 5 Nov 2024 11:22:28 +0200
Subject: [PATCH] authority-discovery: Populate DHT records with public listen
addresses (#6298)
MIME-Version: 1.0
Content-Type: text/plain; charset=UTF-8
Content-Transfer-Encoding: 8bit
This PR's main goal is to add public listen addresses to the DHT
authorities records. This change improves the discoverability of
validators that did not provide the `--public-addresses` flag.
This PR populates the authority DHT records with public listen addresses
if any.
The change effectively ensures that addresses are added to the DHT
record in following order:
1. Public addresses provided by CLI `--public-addresses`
2. Maximum of 4 public (global) listen addresses (if any)
3. Any external addresses discovered from the network (ie from
`/identify` protocol)
While at it, this PR adds the following constraints on the number of
addresses:
- Total number of addresses cached is bounded at 16 (increased from 10).
- A maximum number of 32 addresses are published to DHT records
(previously unbounded).
- A maximum of 4 global listen addresses are utilized.
This PR also removes the following warning:
`WARNING: No public address specified, validator node may not be
reachable.`
### Next Steps
- [ ] deploy and monitor in versi network
Closes: https://github.com/paritytech/polkadot-sdk/issues/6280
Part of: https://github.com/paritytech/polkadot-sdk/issues/5266
cc @paritytech/networking
---------
Signed-off-by: Alexandru Vasile <alexandru.vasile@parity.io>
Co-authored-by: Dmitry Markin <dmitry@markin.tech>
Co-authored-by: Bastian Köcher <git@kchr.de>
---
prdoc/pr_6298.prdoc | 25 +++
.../client/authority-discovery/src/worker.rs | 176 +++++++++++++-----
.../authority-discovery/src/worker/tests.rs | 4 +-
substrate/client/cli/src/commands/run_cmd.rs | 12 +-
4 files changed, 153 insertions(+), 64 deletions(-)
create mode 100644 prdoc/pr_6298.prdoc
diff --git a/prdoc/pr_6298.prdoc b/prdoc/pr_6298.prdoc
new file mode 100644
index 00000000000..fa8d73b1194
--- /dev/null
+++ b/prdoc/pr_6298.prdoc
@@ -0,0 +1,25 @@
+title: Populate authority DHT records with public listen addresses
+
+doc:
+ - audience: [ Node Dev, Node Operator ]
+ description: |
+ This PR populates the authority DHT records with public listen addresses if any.
+ The change effectively ensures that addresses are added to the DHT record in the
+ following order:
+ 1. Public addresses provided by CLI `--public-addresses`
+ 2. Maximum of 4 public (global) listen addresses (if any)
+ 3. Any external addresses discovered from the network (ie from `/identify` protocol)
+
+ While at it, this PR adds the following constraints on the number of addresses:
+ - Total number of addresses cached is bounded at 16 (increased from 10).
+ - A maximum number of 32 addresses are published to DHT records (previously unbounded).
+ - A maximum of 4 global listen addresses are utilized.
+
+ This PR replaces the following warning:
+ `WARNING: No public address specified, validator node may not be reachable.`
+ with a more descriptive one originated from the authority-discovery
+ mechanism itself: `No public addresses configured and no global listen addresses found`.
+
+crates:
+ - name: sc-authority-discovery
+ bump: patch
diff --git a/substrate/client/authority-discovery/src/worker.rs b/substrate/client/authority-discovery/src/worker.rs
index 6f4fbac77e0..9319fbe6321 100644
--- a/substrate/client/authority-discovery/src/worker.rs
+++ b/substrate/client/authority-discovery/src/worker.rs
@@ -71,7 +71,13 @@ pub mod tests;
const LOG_TARGET: &str = "sub-authority-discovery";
/// Maximum number of addresses cached per authority. Additional addresses are discarded.
-const MAX_ADDRESSES_PER_AUTHORITY: usize = 10;
+const MAX_ADDRESSES_PER_AUTHORITY: usize = 16;
+
+/// Maximum number of global listen addresses published by the node.
+const MAX_GLOBAL_LISTEN_ADDRESSES: usize = 4;
+
+/// Maximum number of addresses to publish in a single record.
+const MAX_ADDRESSES_TO_PUBLISH: usize = 32;
/// Maximum number of in-flight DHT lookups at any given point in time.
const MAX_IN_FLIGHT_LOOKUPS: usize = 8;
@@ -174,6 +180,9 @@ pub struct Worker<Client, Block: BlockT, DhtEventStream> {
metrics: Option<Metrics>,
+ /// Flag to ensure the warning about missing public addresses is only printed once.
+ warn_public_addresses: bool,
+
role: Role,
phantom: PhantomData<Block>,
@@ -271,20 +280,7 @@ where
config
.public_addresses
.into_iter()
- .map(|mut address| {
- if let Some(multiaddr::Protocol::P2p(peer_id)) = address.iter().last() {
- if peer_id != *local_peer_id.as_ref() {
- error!(
- target: LOG_TARGET,
- "Discarding invalid local peer ID in public address {address}.",
- );
- }
- // Always discard `/p2p/...` protocol for proper address comparison (local
- // peer id will be added before publishing).
- address.pop();
- }
- address
- })
+ .map(|address| AddressType::PublicAddress(address).without_p2p(local_peer_id))
.collect()
};
@@ -309,6 +305,7 @@ where
addr_cache,
role,
metrics,
+ warn_public_addresses: false,
phantom: PhantomData,
last_known_records: HashMap::new(),
}
@@ -373,47 +370,70 @@ where
}
}
- fn addresses_to_publish(&self) -> impl Iterator<Item = Multiaddr> {
+ fn addresses_to_publish(&mut self) -> impl Iterator<Item = Multiaddr> {
let local_peer_id = self.network.local_peer_id();
let publish_non_global_ips = self.publish_non_global_ips;
+
+ // Checks that the address is global.
+ let address_is_global = |address: &Multiaddr| {
+ address.iter().all(|protocol| match protocol {
+ // The `ip_network` library is used because its `is_global()` method is stable,
+ // while `is_global()` in the standard library currently isn't.
+ multiaddr::Protocol::Ip4(ip) => IpNetwork::from(ip).is_global(),
+ multiaddr::Protocol::Ip6(ip) => IpNetwork::from(ip).is_global(),
+ _ => true,
+ })
+ };
+
+ // These are the addresses the node is listening for incoming connections,
+ // as reported by installed protocols (tcp / websocket etc).
+ //
+ // We double check the address is global. In other words, we double check the node
+ // is not running behind a NAT.
+ // Note: we do this regardless of the `publish_non_global_ips` setting, since the
+ // node discovers many external addresses via the identify protocol.
+ let mut global_listen_addresses = self
+ .network
+ .listen_addresses()
+ .into_iter()
+ .filter_map(|address| {
+ address_is_global(&address)
+ .then(|| AddressType::GlobalListenAddress(address).without_p2p(local_peer_id))
+ })
+ .take(MAX_GLOBAL_LISTEN_ADDRESSES)
+ .peekable();
+
+ // Similar to listen addresses that takes into consideration `publish_non_global_ips`.
+ let mut external_addresses = self
+ .network
+ .external_addresses()
+ .into_iter()
+ .filter_map(|address| {
+ (publish_non_global_ips || address_is_global(&address))
+ .then(|| AddressType::ExternalAddress(address).without_p2p(local_peer_id))
+ })
+ .peekable();
+
+ let has_global_listen_addresses = global_listen_addresses.peek().is_some();
+ trace!(
+ target: LOG_TARGET,
+ "Node has public addresses: {}, global listen addresses: {}, external addresses: {}",
+ !self.public_addresses.is_empty(),
+ has_global_listen_addresses,
+ external_addresses.peek().is_some(),
+ );
+
+ let mut seen_addresses = HashSet::new();
+
let addresses = self
.public_addresses
.clone()
.into_iter()
- .chain(self.network.external_addresses().into_iter().filter_map(|mut address| {
- // Make sure the reported external address does not contain `/p2p/...` protocol.
- if let Some(multiaddr::Protocol::P2p(peer_id)) = address.iter().last() {
- if peer_id != *local_peer_id.as_ref() {
- error!(
- target: LOG_TARGET,
- "Network returned external address '{address}' with peer id \
- not matching the local peer id '{local_peer_id}'.",
- );
- debug_assert!(false);
- }
- address.pop();
- }
-
- if self.public_addresses.contains(&address) {
- // Already added above.
- None
- } else {
- Some(address)
- }
- }))
- .filter(move |address| {
- if publish_non_global_ips {
- return true
- }
-
- address.iter().all(|protocol| match protocol {
- // The `ip_network` library is used because its `is_global()` method is stable,
- // while `is_global()` in the standard library currently isn't.
- multiaddr::Protocol::Ip4(ip) if !IpNetwork::from(ip).is_global() => false,
- multiaddr::Protocol::Ip6(ip) if !IpNetwork::from(ip).is_global() => false,
- _ => true,
- })
- })
+ .chain(global_listen_addresses)
+ .chain(external_addresses)
+ // Deduplicate addresses.
+ .filter(|address| seen_addresses.insert(address.clone()))
+ .take(MAX_ADDRESSES_TO_PUBLISH)
.collect::<Vec<_>>();
if !addresses.is_empty() {
@@ -421,6 +441,21 @@ where
target: LOG_TARGET,
"Publishing authority DHT record peer_id='{local_peer_id}' with addresses='{addresses:?}'",
);
+
+ if !self.warn_public_addresses &&
+ self.public_addresses.is_empty() &&
+ !has_global_listen_addresses
+ {
+ self.warn_public_addresses = true;
+
+ error!(
+ target: LOG_TARGET,
+ "No public addresses configured and no global listen addresses found. \
+ Authority DHT record may contain unreachable addresses. \
+ Consider setting `--public-addr` to the public IP address of this node. \
+ This will become a hard requirement in future versions for authorities."
+ );
+ }
}
// The address must include the local peer id.
@@ -437,7 +472,8 @@ where
let key_store = match &self.role {
Role::PublishAndDiscover(key_store) => key_store,
Role::Discover => return Ok(()),
- };
+ }
+ .clone();
let addresses = serialize_addresses(self.addresses_to_publish());
if addresses.is_empty() {
@@ -946,6 +982,44 @@ where
}
}
+/// Removes the `/p2p/..` from the address if it is present.
+#[derive(Debug, Clone, PartialEq, Eq)]
+enum AddressType {
+ /// The address is specified as a public address via the CLI.
+ PublicAddress(Multiaddr),
+ /// The address is a global listen address.
+ GlobalListenAddress(Multiaddr),
+ /// The address is discovered via the network (ie /identify protocol).
+ ExternalAddress(Multiaddr),
+}
+
+impl AddressType {
+ /// Removes the `/p2p/..` from the address if it is present.
+ ///
+ /// In case the peer id in the address does not match the local peer id, an error is logged for
+ /// `ExternalAddress` and `GlobalListenAddress`.
+ fn without_p2p(self, local_peer_id: PeerId) -> Multiaddr {
+ // Get the address and the source str for logging.
+ let (mut address, source) = match self {
+ AddressType::PublicAddress(address) => (address, "public address"),
+ AddressType::GlobalListenAddress(address) => (address, "global listen address"),
+ AddressType::ExternalAddress(address) => (address, "external address"),
+ };
+
+ if let Some(multiaddr::Protocol::P2p(peer_id)) = address.iter().last() {
+ if peer_id != *local_peer_id.as_ref() {
+ error!(
+ target: LOG_TARGET,
+ "Network returned '{source}' '{address}' with peer id \
+ not matching the local peer id '{local_peer_id}'.",
+ );
+ }
+ address.pop();
+ }
+ address
+ }
+}
+
/// NetworkProvider provides [`Worker`] with all necessary hooks into the
/// underlying Substrate networking. Using this trait abstraction instead of
/// `sc_network::NetworkService` directly is necessary to unit test [`Worker`].
diff --git a/substrate/client/authority-discovery/src/worker/tests.rs b/substrate/client/authority-discovery/src/worker/tests.rs
index d71a85db8b8..8018b5ea492 100644
--- a/substrate/client/authority-discovery/src/worker/tests.rs
+++ b/substrate/client/authority-discovery/src/worker/tests.rs
@@ -1018,7 +1018,7 @@ fn addresses_to_publish_adds_p2p() {
));
let (_to_worker, from_service) = mpsc::channel(0);
- let worker = Worker::new(
+ let mut worker = Worker::new(
from_service,
Arc::new(TestApi { authorities: vec![] }),
network.clone(),
@@ -1056,7 +1056,7 @@ fn addresses_to_publish_respects_existing_p2p_protocol() {
});
let (_to_worker, from_service) = mpsc::channel(0);
- let worker = Worker::new(
+ let mut worker = Worker::new(
from_service,
Arc::new(TestApi { authorities: vec![] }),
network.clone(),
diff --git a/substrate/client/cli/src/commands/run_cmd.rs b/substrate/client/cli/src/commands/run_cmd.rs
index f91d18aca74..f79e5b558e3 100644
--- a/substrate/client/cli/src/commands/run_cmd.rs
+++ b/substrate/client/cli/src/commands/run_cmd.rs
@@ -201,17 +201,7 @@ impl CliConfiguration for RunCmd {
}
fn network_params(&self) -> Option<&NetworkParams> {
- let network_params = &self.network_params;
- let is_authority = self.role(self.is_dev().ok()?).ok()?.is_authority();
- if is_authority && network_params.public_addr.is_empty() {
- eprintln!(
- "WARNING: No public address specified, validator node may not be reachable.
- Consider setting `--public-addr` to the public IP address of this node.
- This will become a hard requirement in future versions."
- );
- }
-
- Some(network_params)
+ Some(&self.network_params)
}
fn keystore_params(&self) -> Option<&KeystoreParams> {
--
GitLab