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.

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

Make it possible to disable automatically inserting nodes we connect to to the DHT #1560

Closed
tomaka opened this issue Apr 29, 2020 · 4 comments · Fixed by #1628
Closed

Make it possible to disable automatically inserting nodes we connect to to the DHT #1560

tomaka opened this issue Apr 29, 2020 · 4 comments · Fixed by #1628
Assignees
Labels
priority:important The changes needed are critical for libp2p, or are blocking another project

Comments

@tomaka
Copy link
Member

tomaka commented Apr 29, 2020

In libp2p-kad, whenever we successfully connect to a node, we immediately insert its address in the DHT.

Unfortunately, in Substrate (and probably for other users for libp2p) we don't necessarily want that to happen.
In particular, light clients shouldn't be part of the DHT (paritytech/substrate#3303) and, now that we have multiple DHTs, we should only insert nodes in the DHT after having verified that they have the correct chain root (paritytech/substrate#5825).

There should be an option that disables the auto-insertion.

@mxinden
Copy link
Member

mxinden commented May 14, 2020

Summarizing a discussion in Riot there are two simple and one complex paths forward:

1. Disable automatic routing table updates

Offering an option in libp2p Kademlia to disable routing table updates when a new connection is established i.e. a simple condition in inject_connection_established.

fn inject_connection_established(&mut self, peer: &PeerId, _: &ConnectionId, endpoint: &ConnectedPoint) {
// The remote's address can only be put into the routing table,
// and thus shared with other nodes, if the local node is the dialer,
// since the remote address on an inbound connection is specific to
// that connection (e.g. typically the TCP port numbers).
let address = match endpoint {
ConnectedPoint::Dialer { address } => Some(address.clone()),
ConnectedPoint::Listener { .. } => None,
};
self.connection_updated(peer.clone(), address, NodeStatus::Connected);
}

It would remain enabled by default. When disabled, no entry will get into the routing table without an explicit call to Kademlia::add_address, which would be called by Substrate after identifying a peer as (a) part of the right protocol and (b) not a light-client.

https://github.com/paritytech/substrate/blob/c90dcc8d8b3c3661ce4b4e0fb6521fb7c4f9281c/client/network/src/behaviour.rs#L183-L199

2. Provide a method to remove nodes after-the-fact

Offering remove_address function, thus being able to remove a peer from the routing table after-the-fact.

Drawbacks of this approach are related to keeping the Kademlia internal peer blacklist in-sync. E.g. we connect to a node, thus it is added to the Kademlia routing table, but before identifying the node we disconnect. In this scenario the peer would stay in the Kademlia routing table forever.

3. Introduce structured prioritization to Kademlias routing table

See details in #1352. This would require us to have an authenticated address book, which we currently only have in an eventually consistent form with the authority discovery module.

@mxinden mxinden added priority:important The changes needed are critical for libp2p, or are blocking another project and removed priority:nicetohave labels May 14, 2020
@mxinden mxinden self-assigned this May 14, 2020
@romanb
Copy link
Contributor

romanb commented May 14, 2020

Note that 1. and 2. are not intended to be alternatives to choose from but rather both should be offered. Even when automatic insertions into the routing table are disabled and you use add_address, there can be legitimate reasons that you want to remove an address (or entire peer) again later (e.g. it is considered misbehaved, changed its role, whatever).

@romanb
Copy link
Contributor

romanb commented Jun 22, 2020

@mxinden Are you already working on it? If not, I can look into implementing options (1) and (2).

@mxinden
Copy link
Member

mxinden commented Jun 22, 2020

@romanb in regards to option (1) I created mxinden@ad3d5db which should pave the way for initial testing within Substrate. It is currently blocked due to paritytech/substrate#5938. Once that merged we can properly add peers to the DHT based on the result of the status message.

I am not aware of any efforts for (2).

Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment
Labels
priority:important The changes needed are critical for libp2p, or are blocking another project
Projects
None yet
Development

Successfully merging a pull request may close this issue.

3 participants