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.

Sign up for GitHub

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

Jump to bottom

litep2p/discovery: Publish authority records with external addresses only #5176

Merged
merged 12 commits into from
Jul 31, 2024
Merged

litep2p/discovery: Publish authority records with external addresses only #5176

Show file tree
Hide file tree
Changes from 7 commits
Commits
Show all changes
12 commits
Select commit Hold shift + click to select a range
2d0893b
litep2p/discovery: Increase maximum external addresses in the LRU map
lexnv Jul 26, 2024
b24c9d3
litep2p/discovery: Confirm addresses on second reporting
lexnv Jul 26, 2024
251c2c8
authority-discovery: Do not publish empty addresses yet
lexnv Jul 29, 2024
7b92b57
authority-discovery: Change log to suggest we are not always publishing
lexnv Jul 29, 2024
fd5d697
net/litep2p: Align log with libp2p
lexnv Jul 29, 2024
5efd51b
litep2p/discovery: Only add global addresses to the extenral address
lexnv Jul 29, 2024
15c056b
litep2p/discovery: Keep old maybe nonglobal ip
lexnv Jul 29, 2024
0630bd0
Update substrate/client/network/src/litep2p/mod.rs
lexnv Jul 30, 2024
e339df3
discovery: Report addresses on different peers
lexnv Jul 30, 2024
f17a08c
authority: Refactor publish logs
lexnv Jul 30, 2024
8367766
Merge remote-tracking branch 'origin/lexnv/unknown-authority-records'…
lexnv Jul 30, 2024
331b006
discovery: Fix clippy
lexnv Jul 30, 2024
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
15 changes: 12 additions & 3 deletions substrate/client/authority-discovery/src/worker.rs
View file
Open in desktop
Original file line number Diff line number Diff line change
Expand Up @@ -418,7 +418,7 @@ where

debug!(
target: LOG_TARGET,
"Publishing authority DHT record peer_id='{local_peer_id}' addresses='{addresses:?}'",
"Authority DHT record peer_id='{local_peer_id}' with addresses='{addresses:?}'",
lexnv marked this conversation as resolved.
Show resolved Hide resolved
);

// The address must include the local peer id.
Expand All @@ -437,6 +437,17 @@ where
Role::Discover => return Ok(()),
};

let addresses = serialize_addresses(self.addresses_to_publish());
if addresses.is_empty() {
debug!(
target: LOG_TARGET,
"No addresses to publish. Skipping publication."
);

self.publish_interval.set_to_start();
return Ok(())
}

let keys =
Worker::<Client, Block, DhtEventStream>::get_own_public_keys_within_authority_set(
key_store.clone(),
Expand All @@ -459,8 +470,6 @@ where
self.query_interval.set_to_start();
}

let addresses = serialize_addresses(self.addresses_to_publish());

if let Some(metrics) = &self.metrics {
metrics.publish.inc();
metrics
Expand Down
16 changes: 11 additions & 5 deletions substrate/client/network/src/litep2p/discovery.rs
View file
Open in desktop
Original file line number Diff line number Diff line change
Expand Up @@ -66,12 +66,18 @@ const KADEMLIA_QUERY_INTERVAL: Duration = Duration::from_secs(5);
/// mDNS query interval.
const MDNS_QUERY_INTERVAL: Duration = Duration::from_secs(30);

/// Minimum number of confirmations received before an address is verified.
const MIN_ADDRESS_CONFIRMATIONS: usize = 5;

// The minimum number of peers we expect an answer before we terminate the request.
/// The minimum number of peers we expect an answer before we terminate the request.
const GET_RECORD_REDUNDANCY_FACTOR: usize = 4;

/// The maximum number of tracked external addresses we allow.
const MAX_EXTERNAL_ADDRESSES: u32 = 32;

/// Minimum number of confirmations received before an address is verified.
///
/// Note: all addresses are confirmed by libp2p on the first encounter. This aims to make
/// addresses a bit more robust.
const MIN_ADDRESS_CONFIRMATIONS: usize = 2;
lexnv marked this conversation as resolved.
Show resolved Hide resolved

/// Discovery events.
#[derive(Debug)]
pub enum DiscoveryEvent {
Expand Down Expand Up @@ -278,7 +284,7 @@ impl Discovery {
find_node_query_id: None,
pending_events: VecDeque::new(),
duration_to_next_find_query: Duration::from_secs(1),
address_confirmations: LruMap::new(ByLength::new(8)),
address_confirmations: LruMap::new(ByLength::new(MAX_EXTERNAL_ADDRESSES)),
Copy link
Member

Choose a reason for hiding this comment

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

How do we ensure that nodes do not report junk addresses to us?

Copy link
Member

Choose a reason for hiding this comment

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

Generally I don't get why we not bootstrap it with the addresses of our interfaces? If these addresses are global, the likelihood that the node is reachable via them should be quite big?

Copy link
Contributor Author

Choose a reason for hiding this comment

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

Yep that could also work, we can check our interfaces and iff we find a global, we'll propagate that. I think this wasn't considered before because it was assumed that most nodes are under nat / firewall.

Another approach for this would be to use a distributed address lookup service (instead of curl centralized.service.ip).

How do we ensure that nodes do not report junk addresses to us?

I don't think anything is protecting either libp2p or litep2p here. Litep2p ran into this error because it was waiting for 5 confirmations, instead of eagerly accepting all reported addresses 🤔

allow_non_global_addresses: config.allow_non_globals_in_dht,
public_addresses: config.public_addresses.iter().cloned().map(Into::into).collect(),
next_kad_query: Some(Delay::new(KADEMLIA_QUERY_INTERVAL)),
Expand Down
3 changes: 2 additions & 1 deletion substrate/client/network/src/litep2p/mod.rs
View file
Open in desktop
Original file line number Diff line number Diff line change
Expand Up @@ -920,7 +920,8 @@ impl<B: BlockT + 'static, H: ExHashT> NetworkBackend<B, H> for Litep2pNetworkBac
let mut addresses = self.external_addresses.write();

if addresses.insert(address.clone()) {
log::info!(target: LOG_TARGET, "discovered new external address for our node: {address}");
log::info!(target: LOG_TARGET, "🔍 Discovered new external address for our node: {address}");

lexnv marked this conversation as resolved.
Show resolved Hide resolved
}
}
Some(DiscoveryEvent::Ping { peer, rtt }) => {
Expand Down
Loading