Necessary to add on identify event?

[dariusc93]

Been reviewing and going over libp2p for a bit and something ive done for awhile was adding a peer to kad upon receiving the identify event and checking to see if they support kademlia protocol, however after testing and reviewing the protocol itself, I think this segment of code is not needed since it looks like the protocol will add the peer to to the routing table upon connection, assuming the configuration has KademliaBucketInserts::OnConnected set. If KademliaBucketInserts::Manual is set, I would have to manually add the peer if they are routable.

So my question is, when is it ever appropriate or necessary to add the peer upon receiving the identify event and checking for the protocol? From my understanding, I could simply do this on receiving KademliaEvent::RoutablePeer and KademliaEvent::PendingRoutablePeer event if im manually adding them or if im doing it on connection, let the protocol handle it and handle adding them manually if the bucket is full.

[mxinden]

the protocol will add the peer to to the routing table upon connection, assuming the configuration has KademliaBucketInserts::OnConnected set

Not quite. Only once it established an out- or inbound stream, in other words only after it learned that the remote speaks the Kademlia protocol. See code paths around ProtocolConfirmed:

rust-libp2p/protocols/kad/src/handler.rs

Lines 215 to 222 in 9e62588

	/// The configured protocol name has been confirmed by the peer through
	/// a successfully negotiated substream.
	///
	/// This event is only emitted once by a handler upon the first
	/// successfully negotiated inbound or outbound substream and
	/// indicates that the connected peer participates in the Kademlia
	/// overlay network identified by the configured protocol name.
	ProtocolConfirmed { endpoint: ConnectedPoint },

I suggest keeping OnConnected and in addition call Kademlia::add_address on each identify event iff the remote peer speaks the Kademlia protocol. See https://github.com/mxinden/rust-libp2p-server/blob/4cc18a97d4852f7504c2dc18c0c44ffcdf718ab6/src/main.rs#L146-L173. Note that the manual call to Kademlia::add_address is going away with #3877.