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

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.

added tests for addrfilter fix: don't add unresponsive DHT servers to the Routing Table (libp2p#820) * 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> fix: don't add unresponsive DHT servers to the Routing Table (libp2p#820) * 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) chore: release v0.24.0 chore: Update .github/workflows/stale.yml [skip ci] fix: leaking go routines fix: bump kbucket for abba bug refactor: remove goprocess fix: decrease tests noise, update kbucket and fix fixRTIUfNeeded chore: release v0.24.1 chore: update go-libp2p to a non deadlocky version chore: release v0.24.2 chore: Update .github/workflows/stale.yml [skip ci] tracing: fix DHT keys as string attribute not being valid utf-8 chore: release v0.24.3 fix the server mode log in handleNewMessage by changing it to debug chore: delete templates [skip ci] (libp2p#861) ci: uci/copy-templates (libp2p#862) * chore: add or force update .github/workflows/go-test.yml * chore: add or force update .github/workflows/go-check.yml * chore: add or force update .github/workflows/releaser.yml * chore: add or force update .github/workflows/release-check.yml * chore: add or force update .github/workflows/tagpush.yml chore: bump go.mod to Go 1.20 and run go fix chore: bump go-libp2p and/or quic-go to latest version tracing: add protocol messages client tracing chore: release v0.25.0 add provider record addresses to peerstore fixes issue libp2p#868 fixing base32 import fix: apply addrFilters in the dht (libp2p#872) * fix: correctly apply addrFilters in the dht This still does not do the fullrt client but it wasn't doing it before either. * Add address filter tests * use channel instead of waitgroup to catch timeouts * filter multiaddresses when serving provider records --------- Co-authored-by: Dennis Trautwein <git@dtrautwein.eu> fix: properly iterate in tracing for protocol messenger MB Copy Paste Typo, This caused panics while tracing. chore: use go-libp2p-routing-helpers for tracing needs go-libp2p-routing-helpers has an optimized implementation that does nothing if we are not tracing, it also properly log all IO of the request. perf: don't buffer the output of FindProvidersAsync We always select it against ctx and we don't rely on the fact it will always be non blocking since when count is zero we can't realistically preallocate. Buffering use more memory makes more garbage and is less efficient than direct copies. chore: release v0.25.1 add ctx canceled err check return canceled err not retry reset stream and return err when ctx canceled release v0.25.2 chore: Update .github/workflows/stale.yml [skip ci] chore: bump go.mod to Go 1.21 and run go fix chore: run go mod tidy chore: add or force update .github/workflows/go-test.yml chore: add or force update .github/workflows/go-check.yml chore: add or force update .github/workflows/releaser.yml chore: add or force update .github/workflows/release-check.yml chore: add or force update .github/workflows/tagpush.yml chore: add or force update .github/workflows/go-test.yml chore: add or force update .github/workflows/go-check.yml chore: add or force update .github/workflows/releaser.yml chore: add or force update .github/workflows/release-check.yml chore: add or force update .github/workflows/tagpush.yml chore: add or force update .github/workflows/go-test.yml chore: add or force update .github/workflows/go-check.yml chore: add or force update .github/workflows/releaser.yml chore: add or force update .github/workflows/release-check.yml chore: add or force update .github/workflows/tagpush.yml findnode(self) now returns multiple peers don't lookup check if not enough peers in table enhanced tests moved rt size check after request Upgrade to go-log v2.5.1

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

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

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

Loading

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 Loading

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

Loading