Missing "rsa" feature should not silently fail kademlia handshakes

[MatthiasvB]

Description

Please excuse my use of inaccurate terms and lack of understanding. I'm just starting with libp2p.

I'm not sure if this perhaps better be a bug report.

Requests should not silently fail when the "rsa" feature isn't activated.

Motivation

So I've built a node to the point where I'm able to discover peers via kademlia. There was one harsh bump in the road to get there, which was that initially all outgoing requests to the bootstrap nodes almost immediately failed, no explanation given. It took me a few hours of meticulously comparing my implementation with the examples from the repo until I tried adding the "rsa" feature, which fixed it. I can only guess why that's needed, but I don't understand why missing this feature doesn't fail loudly. This is one of the points where I'd expect a panic!, no?

Current Implementation

Currently everything just runs, just not successfully at all

Are you planning to do it yourself in a pull request?

No, lack of experience

[elenaf9]

Could you provide some output logs? And did you have any other crypto feature (ed25519, secp256k1, ecdsa) enabled?

We should already output a DecodingError if a remote peer sends us during connection establishment a key for which we don't have the matching crypto feature enabled in libp2p-identity.

[dariusc93]

Are you using the default ipfs bootstraps or bootstrap with rsa public keys? If so you would need to enable the "rsa" feature. Same with the other keys such as ed25519, etc., if you are using them or connecting to peers with those keys. Without it, there should be an error when attempting to connect to the peer.

[MatthiasvB]

The error I encountered is exactly the same as when you run the ipfs-kad example from the repo, but remove the "rsa" feature there as well. I don't see any output hinting at the underlying problem.

I don't know if that is feasible, but ideally I'd get a speaking error about the missing feature either at build time or at least immediately on node startup, crashing the program.

     Running `/home/matthias/Documents/development/opensource/rust-libp2p/target/debug/ipfs-kad-example get-peers`
OutboundQueryProgressed { id: QueryId(0), result: Bootstrap(Ok(BootstrapOk { peer: PeerId("12D3KooWNX8dR6CbDFck5P7WXyESWPPZoghyQQvGaDBadfBBob4G"), num_remaining: 1 })), stats: QueryStats { requests: 4, success: 0, failure: 4, start: Some(Instant { tv_sec: 6852, tv_nsec: 532958504 }), end: Some(Instant { tv_sec: 6853, tv_nsec: 924692416 }) }, step: ProgressStep { count: 1, last: false } }
OutboundQueryProgressed { id: QueryId(0), result: Bootstrap(Ok(BootstrapOk { peer: PeerId("1AX5msr2xBzNmsE5CKEnMjJRFnJ44MCPAFZW53aSHgDnE6"), num_remaining: 0 })), stats: QueryStats { requests: 4, success: 0, failure: 4, start: Some(Instant { tv_sec: 6853, tv_nsec: 925321150 }), end: Some(Instant { tv_sec: 6854, tv_nsec: 289312039 }) }, step: ProgressStep { count: 2, last: true } }

Another hint: The README has a mistake, imho: The command to run it (if you do want to specify a peer id) is cargo run -- get-peers --peer-id [PEER_ID] not cargo run -- get-peers [PEER_ID]. But for the sake of this test, just cargo run -- get-peers is fine

[elenaf9]

When you log in the ipfs-kad example all swarm events you do get some OutgoingConnectionError events that are the reason why the kademlia discovery fails.

However, because of how our dns transport reports errors when resolving dns addresses, not all transport errors from inner transports are included in this event.
That's the reason why even thought we already have an error variant that informs you about the missing rsa feature that causes a handshake to fail, it never surfaced (but you can still see it when running the example with RUST_LOG=debug).
I opened #5878 for this. Would you be interested in working on that issue @MatthiasvB?

I don't know if that is feasible, but ideally I'd get a speaking error about the missing feature either at build time or at least immediately on node startup, crashing the program.

The rsa feature is optional on purpose. You need it if you want to support connection establishment with a remote peer that uses RSA keys, but users may want to disable rsa keys and only use one of the other crypto protocols.

[MatthiasvB]

Thank you very much for the analysis and explanation. I will keep RUST_LOG=debug in mind for the future.

Does this mean that when I only enable rsa and not any other crypto features, there will still be connections failing? So I should simply enable all crypto features if I want maximum compatibility?

With regards to whether I want to work on it: Want? Yeah sure. But like I said, I'm relatively new to Rust, unfamiliar with the rust-libp2p code base as well as the details of the network strategies employed therein. Given your explanation I may be able to muddle through, but I also don't see me finding the time in the near future, tbh. So I suppose I could check back here eventually and see if it's still open, but that may be a while

[elenaf9]

Does this mean that when I only enable rsa and not any other crypto features, there will still be connections failing? So I should simply enable all crypto features if I want maximum compatibility?

Depends on what crypto keys are generally used by the nodes in your network.
In the IPFS network ed25519 is the default, which is enabled anyway by libp2p-noise if you use is. The bootstrap nodes only use rsa due to legacy reasons I think.

But yes if you want to be sure you can just enable all of them.