fix: don't add unresponsive DHT servers to the Routing Table #820

guillaumemichel · 2023-02-21T10:21:42Z

PR addressing #811

Change summary:

added a lookupCheck function to the IpfsDHT. This function consists in making a DHT FindPeer request to a peer, asking it for its own peer.ID https://github.com/guillaumemichel/go-libp2p-kad-dht/blob/cdd6898b8a9d41a1abce803325c98a14083013d6/dht.go#L327-L336
added a validPeerFound function, acting like the previous peerFound function, but assuming the given peer was just queried https://github.com/guillaumemichel/go-libp2p-kad-dht/blob/cdd6898b8a9d41a1abce803325c98a14083013d6/dht.go#L664-L676
peerFound performs the validRTPeer check and runs protocolCheck. Upon success it calls validPeerFound https://github.com/guillaumemichel/go-libp2p-kad-dht/blob/cdd6898b8a9d41a1abce803325c98a14083013d6/dht.go#L639-L662
peers that were just successfully queried with a DHT lookup are directly sent to validPeerFound https://github.com/guillaumemichel/go-libp2p-kad-dht/blob/cdd6898b8a9d41a1abce803325c98a14083013d6/query.go#L419-L420
IpfsDHT has a new recentlyCheckedPeers attribute, a map of the peerids contacted in the last lookupCheckInterval (5 seconds by default). It prevents network loops, e.g A sends a request to B, B wants to add A in its RT, B sends a request to A before adding it to its RT, A gets the requests and want to add B in its RT so it sends a new request to B, etc. If we tried to add a peer to the RT less than 5 seconds ago, we won't try again.
Before A sends a lookup request to check whether B behaves as expected, check if there is available space in A's RT in the bucket where B belongs. If this bucket is full, abort adding B to the RT and sending any request.

Todo

Implementation
Reviews and approval
Merge UsefulPeer function go-libp2p-kbucket#113
Update dependencies to https://github.com/libp2p/go-libp2p-kbucket
Update relevant documentation
Merge

… table

guillaumemichel · 2023-02-22T15:00:49Z

@Jorropo I fixed a flaw but some tests are still failing:

TestBootstrappersReplacable doesn't make sense to me. The concept of replaceable peers seems wrongly implemented in go-libp2p-kbucket. I suggest that we get rid of this test.
All tests from ext_test.go are failing. They are testing whether DHT requests fail as expected in scenarios the DHT peer is connected to unresponsive peers. However, with this new feature, invalid nodes aren't added to the routing table at all, hence the tests are failing.
All other tests are passing as expected.

For ext_test.go, I suggest to get rid of all tests, and add a new test where we try to connect to unresponsive peers and verify that they don't get added to the routing table. WDYT?

guillaumemichel · 2023-02-23T11:25:16Z

Just opened a PR in go-libp2p-kbucket. We need a function to test whether a peer.ID would be useful to the Routing Table, so that we don't bother sending requests to useless peers.

libp2p/go-libp2p-kbucket#113

Jorropo

I did a high level surface review, I'll rereview in depth once high level code quality is fixed.

dht.go

dht_test.go

yiannisbot · 2023-03-15T09:06:43Z

@guillaumemichel is this ready for a second round of reviews from @Jorropo so that we schedule it for one of the upcoming releases?
cc: @BigLep to be in the loop regarding scheduling this, or deferring for later depending on team's priorities.

BigLep · 2023-04-06T06:36:28Z

@guillaumemichel : what are the next steps here?

guillaumemichel · 2023-04-06T06:41:56Z

I am still waiting on a review from @Jorropo for this PR and for libp2p/go-libp2p-kbucket#113 on which it depends.

dht.go

dht_bootstrap_test.go

dht_net.go

internal/config/config.go

Jorropo · 2023-06-08T09:42:16Z

Also please resolve your conflicts.

BigLep · 2023-06-08T22:40:15Z

Per 2023-06-08 maintainer conversation: @Jorropo is going to take having this code all ready to go so that @guillaumemichel can review it first-thing 2023-06-12 when he returns so that the Kubo 0.21 release can happen afterwards.

optimizations.md

dht.go

Jorropo

LGTM Ty <3

* added check to avoid adding unresponsive dht peers to the dht routing table * removed lock in adding peers to the rt * made variable names more meaningful * fixed network loop and corrected tests * added UsefulPeer() references from current PR * go mod tidy * added delay in TestRefreshBelowMinRTThreshold * addressed review * go mod tidy * addressed Jorropo review * added comments * removed state of peers probed recently * fix conflicts merge * updated deps * added optimizations documentation * Update dht.go * updated md files --------- Co-authored-by: Jorropo <jorropo.pgm@gmail.com>

* added check to avoid adding unresponsive dht peers to the dht routing table * removed lock in adding peers to the rt * made variable names more meaningful * fixed network loop and corrected tests * added UsefulPeer() references from current PR * go mod tidy * added delay in TestRefreshBelowMinRTThreshold * addressed review * go mod tidy * addressed Jorropo review * added comments * removed state of peers probed recently * fix conflicts merge * updated deps * added optimizations documentation * Update dht.go * updated md files --------- Co-authored-by: Jorropo <jorropo.pgm@gmail.com> (cherry picked from commit 8c9fdff)

Partial revert of #2499 If a node is queried for it's own peer id, return it's own peer info. This is necessary because since libp2p/go-libp2p-kad-dht#820 go-libp2p-kad-dht won't add a peer to it's routing tables that doesn't have any DHT peers that are KAD-futher from it's own ID already.

Jorropo self-requested a review February 21, 2023 16:17

guillaumemichel marked this pull request as draft February 21, 2023 17:08

guillaumemichel self-assigned this Feb 21, 2023

added check to avoid adding unresponsive dht peers to the dht routing…

5362e55

… table

guillaumemichel force-pushed the fix/unresponsive-peers branch from 8549334 to 5362e55 Compare February 21, 2023 19:20

yiannisbot mentioned this pull request Feb 21, 2023

Milestone: DHT Optimisations probe-lab/roadmap#14

Closed

removed lock in adding peers to the rt

9357ac9

guillaumemichel marked this pull request as ready for review February 22, 2023 14:41

made variable names more meaningful

d0d6d73

guillaumemichel marked this pull request as draft February 23, 2023 09:56

guillaumemichel mentioned this pull request Feb 23, 2023

UsefulPeer function libp2p/go-libp2p-kbucket#113

Merged

guillaumemichel added 2 commits February 23, 2023 17:20

fixed network loop and corrected tests

8be8d1e

added UsefulPeer() references from current PR

7d046a2

guillaumemichel marked this pull request as ready for review February 23, 2023 16:33

guillaumemichel added 2 commits February 24, 2023 09:07

go mod tidy

cbbba91

added delay in TestRefreshBelowMinRTThreshold

be25008

Jorropo requested changes Mar 7, 2023

View reviewed changes

addressed review

5e2ef88

guillaumemichel force-pushed the fix/unresponsive-peers branch from f67dfa9 to 5e2ef88 Compare March 7, 2023 10:52

guillaumemichel requested a review from Jorropo March 7, 2023 10:53

go mod tidy

f083cb3

guillaumemichel force-pushed the fix/unresponsive-peers branch 2 times, most recently from 10fbf09 to f083cb3 Compare April 6, 2023 08:27

BigLep mentioned this pull request Apr 19, 2023

Release 0.21 ipfs/kubo#9814

Closed

Jorropo reviewed Jun 6, 2023

View reviewed changes

dht.go Outdated Show resolved Hide resolved

Jorropo reviewed Jun 8, 2023

View reviewed changes

dht.go Outdated Show resolved Hide resolved

dht.go Outdated Show resolved Hide resolved

dht_bootstrap_test.go Outdated Show resolved Hide resolved

dht_net.go Outdated Show resolved Hide resolved

internal/config/config.go Outdated Show resolved Hide resolved

guillaumemichel and others added 6 commits June 12, 2023 09:42

added comments

5cf8680

removed state of peers probed recently

7bdc622

Merge branch 'master' into fix/unresponsive-peers

a8a4d82

fix conflicts merge

e85f001

updated deps

89474e5

added optimizations documentation

810d7dd

Jorropo reviewed Jun 12, 2023

View reviewed changes

optimizations.md Outdated Show resolved Hide resolved

Jorropo requested changes Jun 12, 2023

View reviewed changes

dht.go Outdated Show resolved Hide resolved

Update dht.go

2450e45

Jorropo approved these changes Jun 12, 2023

View reviewed changes

Jorropo changed the title ~~Fix: don't add unresponsive DHT servers to the Routing Table~~ fix: don't add unresponsive DHT servers to the Routing Table Jun 12, 2023

Jorropo mentioned this pull request Jun 12, 2023

chore: release v0.21.1 #843

Closed

updated md files

38793b9

Jorropo merged commit 8c9fdff into libp2p:master Jun 12, 2023
11 checks passed

guillaumemichel mentioned this pull request Jun 12, 2023

Don't add unresponsive DHT servers to the RT #811

Closed

4 tasks

This was referenced Jun 27, 2023

Optimize Routing Table Module probe-lab/go-kademlia#2

Closed

[DHT] Don't add peers to the routing table who falsely advertise the DHT protocol #808

Closed

guillaumemichel mentioned this pull request Jul 6, 2023

Only add reachable peers to the routing table probe-lab/go-kademlia#53

Closed

Jorropo mentioned this pull request Jul 28, 2023

Large Number of Unavailable Peers probe-lab/network-measurements#57

Open

iand mentioned this pull request Sep 13, 2023

Test query interactions with routing table #887

Merged

xinaxu mentioned this pull request Mar 22, 2024

Bootstrap process not compatible with rust libp2p-kad #966

Closed

achingbrain mentioned this pull request Apr 29, 2024

kad: improve FIND_NODE response definition libp2p/specs#609

Open

achingbrain mentioned this pull request Apr 29, 2024

fix: return self in FIND_NODE for self libp2p/js-libp2p#2512

Merged

3 tasks

Provide feedback

Saved searches

Use saved searches to filter your results more quickly

fix: don't add unresponsive DHT servers to the Routing Table #820

fix: don't add unresponsive DHT servers to the Routing Table #820

guillaumemichel commented Feb 21, 2023 •

edited

guillaumemichel commented Feb 22, 2023

guillaumemichel commented Feb 23, 2023

Jorropo left a comment

yiannisbot commented Mar 15, 2023

BigLep commented Apr 6, 2023

guillaumemichel commented Apr 6, 2023

Jorropo commented Jun 8, 2023

BigLep commented Jun 8, 2023

Jorropo left a comment

fix: don't add unresponsive DHT servers to the Routing Table #820

fix: don't add unresponsive DHT servers to the Routing Table #820

Conversation

guillaumemichel commented Feb 21, 2023 • edited

Change summary:

Todo

guillaumemichel commented Feb 22, 2023

guillaumemichel commented Feb 23, 2023

Jorropo left a comment

Choose a reason for hiding this comment

yiannisbot commented Mar 15, 2023

BigLep commented Apr 6, 2023

guillaumemichel commented Apr 6, 2023

Jorropo commented Jun 8, 2023

BigLep commented Jun 8, 2023

Jorropo left a comment

Choose a reason for hiding this comment

guillaumemichel commented Feb 21, 2023 •

edited