kad: Update routing table on kademlia established connections

[lexnv]

In this PR:

• Established connections from the kademlia component are reported immediately to the routing table iff the mode is in Automatic.
• The kademlia on connection established address is reported to the routing table for healthier tracking of addresses. This improves a bit the discoverability of peers, especially in cases where the RoutingTableUpdate::Manual option is set (ie Substrate)
• Refactor the Kbucket::entry function to iterate only once through the nodes, instead of twice
• Remove unneeded routing table ConnectionType's CannotConnect and CanConnect. The terminology comes from the kademlia peer status in low-level commands. However, the routing table only needs to know if the peer is connected or disconnected (similar to libp2p).

cc @paritytech/networking

[dmitry-markin]

• The kademlia on connection established address is reported to the routing table for healthier tracking of addresses. This improves a bit the discoverability of peers, especially in cases where the RoutingTableUpdate::Manual option is set (ie Substrate)

I was thinking about this at some point, but ended up deciding that it can make things worse for kademlia, as the endpoint addresses are not necessarily the reachable ones. I think the current implementation of reporting back the observed addresses by Identify protocol and adding to the routing table only the ones that survive the "externality" check using add_self_reported_address is fine.

[dmitry-markin]

The entry lookup optimization in bucket.rs looks good, but I don't think we should add endpoint addresses as is to the routing table. At least not in the Manual update mode.

As for Automatic routing table update mode, I actually wonder how a new node joining DHT should end up in the other nodes' routing tables. Current kademlia implementation spreads routing table entries to other peers via FIND_NODE and GET_VALUE responses, but we have a chicken and egg problem with new nodes joining the network. Correct me if I'm wrong, but it looks like the current Automatic implementation is going to have issues with connectivity (the network will never learn the address of a new-joiner). And it looks like this PR fixes it (even though it might not work good enough, as the endpoint address can have an ephemeral port, or can suffer NAT translation not suitable for incoming TCP connections).

[lexnv]

I see what you mean now, yep that makes sense! Thanks for the info 🙏

Thinking out loud, we can probably close the gap and make things a bit more resilient with:

• transport: Change AddressStore to keep track of more dial addresses #180
• identify: Update local node listen addresses #191

The first issue tracks addresses a bit more robustly from the transport manager perspective, without necessarily loosing track of dialed addresses, or potential addresses to dial in the future.

After reading your message and looking back over the issue of authority-discovery not finding external addresses, I realized that we keep a static list of "listen addrs" in the Idenitfy protocol. The second issue should close the gap and provide proper information back to the remote peer.

All this relies on the fact that we report peer addresses via add_self_reported_address that is called on the Discovery::Identified event. If we provide outdated, or very few addresses in the Identify response, then the other peer won't have good information to dial us in the future. And instead, the routing table of the remote peer will contain outdated (possibly) expired information about our addresses.

We are also populating the routing table on automatic below, and reporting the address to the transport manager:

litep2p/src/protocol/libp2p/kademlia/mod.rs

Lines 367 to 377 in 2d1a4b4

	self.service.add_known_address(&info.peer, info.addresses.iter().cloned());
	if std::matches!(self.update_mode, RoutingTableUpdateMode::Automatic) {
	self.routing_table.add_known_peer(
	info.peer,
	info.addresses.clone(),
	self.peers
	.get(&info.peer)
	.map_or(ConnectionType::NotConnected, |_| ConnectionType::Connected),
	);
	}

Making the transport manager a bit more robust in tracking addresses should help out.

Let me know if this sounds like a plan 🙏

[dmitry-markin]

Very good findings. Reporting discovered external addresses in Identify messages should improve things significantly. I would start from implementing it.