libp2p / rust-libp2p

The Rust Implementation of the libp2p networking stack.
https://libp2p.io
MIT License
4.39k stars 912 forks source link

Kademlia inserts addresses even when requested not to #4882

Open nazar-pc opened 8 months ago

nazar-pc commented 8 months ago

There is BucketInserts enum in Kademlia that is supposed to control when to insert addresses of connected peers into k-buckets or not. If I'm reading it correctly, it only has effect if particular peer ID is not yet in k-buckets, however it if it already there, insertion configuration is ignored: https://github.com/libp2p/rust-libp2p/blob/e151ecbbf5d2d8a663f6db4d88230469a0eab58f/protocols/kad/src/behaviour.rs#L1223-L1253

This looks like an oversight that will often result in observed ephemeral non-dialable (in the long run anyway) addresses from incoming connections being added to k-buckets and shared with other peers.

The solution here is to not insert an address in k-bucket unless BucketInserts::OnConnected is used (status should still be updated though).

P.S. I'm having a hard time putting such reports into pre-defined bug report template, hence writing it free-form instead.

thomaseizinger commented 8 months ago

If I'm reading it correctly, it only has effect if particular peer ID is not yet in k-buckets, however it if it already there, insertion configuration is ignored:

@mxinden as the original author probably can answer this the best but from reading the docs on BucketInserts, I am taking away that this only controls the initial insert. Once a peer is in a bucket, the entry will get updated regardless:

https://github.com/libp2p/rust-libp2p/blob/5a4a462899ac937cb1a0dc84ef29333a7ad1b047/protocols/kad/src/behaviour.rs#L137-L141

This looks like an oversight that will often result in observed ephemeral non-dialable (in the long run anyway) addresses from incoming connections being added to k-buckets and shared with other peers.

This is definitely a bug somewhere but I am not sure #4884 is the solution to it. The current logic SHOULD be that we only insert addresses that we dialed and thus avoid things like ephemeral ports that are non-dialable but perhaps this check is missing in some code branch?

P.S. I'm having a hard time putting such reports into pre-defined bug report template, hence writing it free-form instead.

No worries at all. They are mostly a guideline. Your bug reports are also high quality without the template so ignoring it is fine in that case :)

nazar-pc commented 8 months ago

I am taking away that this only controls the initial insert

This might indeed be the intention when it was introduced, however the way I see it from a user point of view is to have control over what is inserted into Kademlia. So it kind of defeats the purpose of having an option if I can control first insertion, but not further updates.

At the same time looking at the code I do see the value of default behavior being to insert dialed address initially and adding extra addresses later too, it is just when user wants to control the process it ends up being neither here nor there.

thomaseizinger commented 8 months ago

I am taking away that this only controls the initial insert

This might indeed be the intention when it was introduced, however the way I see it from a user point of view is to have control over what is inserted into Kademlia. So it kind of defeats the purpose of having an option if I can control first insertion, but not further updates.

So what you'd want is an entirely "manual" DHT? To be honest, I don't even see the value of the current configuration option. Why wouldn't you want to add a peer to the routing table if it is confirmed that they speak kademlia and you managed to dial them?

cc @mxinden

nazar-pc commented 8 months ago

We want to be able to control whether non-global addresses end up in k-buckets or not for example. It is currently not possible to control fully because Kademlia will insert them later anyway, while https://github.com/libp2p/rust-libp2p/pull/4884 allows us to prevent it not only during first insertion, but during any subsequent updates as well.

thomaseizinger commented 8 months ago

Is that because you run nodes on your LAN but also in other networks and you don't want to add your LAN nodes to the table and have the nodes on other networks fail to reach them?

How often does this happen for it to be an actual problem?

I am asking because I'd much prefer adding features / fixes to our kademlia implementation rather than adding more configuration options. The former allows all users to benefit from it and it means we can write tests against the desired behaviour. Not to say that configuration options aren't useful but we should be considerate on whether they are actually needed.

nazar-pc commented 7 months ago

Is that because you run nodes on your LAN but also in other networks and you don't want to add your LAN nodes to the table and have the nodes on other networks fail to reach them?

Yes, there is also a privacy aspect because by default it exposes information about networking situation of the machine where node is running.

How often does this happen for it to be an actual problem?

It happens quite often and in case of some providers (Hetzner) may result in machine being banned because it tries to reach non-public addresses. We have hit this a few times in the past and had to introduce corresponding options that hide private addresses as useless in >99% of cases anyway.

thomaseizinger commented 6 months ago

How often does this happen for it to be an actual problem?

It happens quite often and in case of some providers (Hetzner) may result in machine being banned because it tries to reach non-public addresses.

For this, we have the global_only transport: https://docs.rs/libp2p/latest/libp2p/core/transport/global_only/index.html

Does that help?

nazar-pc commented 6 months ago

I think it does, I was not aware it existed, but we'd still store for the most part useless addresses in memory, the goal was to skip them completely everywhere we can.

thomaseizinger commented 6 months ago

I think it does, I was not aware it existed, but we'd still store for the most part useless addresses in memory, the goal was to skip them completely everywhere we can.

I am a bit reluctant to add / change configuration behaviour here without fully understanding why you have a significant number of garbage addresses in your DHT. If that is actually the case, then we should fix some bug somewhere such that other users don't also run into that. Disabling functionality and requiring users to filter addresses manually is not a useful fix IMO.