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

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

Merged
merged 18 commits into from
Jun 12, 2023

Conversation

guillaumemichel
Copy link
Contributor

@guillaumemichel guillaumemichel commented Feb 21, 2023

PR addressing #811

Change summary:

Todo

@Jorropo Jorropo self-requested a review February 21, 2023 16:17
@guillaumemichel guillaumemichel marked this pull request as draft February 21, 2023 17:08
@guillaumemichel guillaumemichel self-assigned this Feb 21, 2023
@guillaumemichel guillaumemichel marked this pull request as ready for review February 22, 2023 14:41
@guillaumemichel
Copy link
Contributor Author

@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
Copy link
Contributor Author

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

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

@Jorropo Jorropo left a comment

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

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

dht.go Outdated Show resolved Hide resolved
dht.go Outdated Show resolved Hide resolved
dht.go Outdated Show resolved Hide resolved
dht.go Outdated Show resolved Hide resolved
dht.go Outdated Show resolved Hide resolved
dht_test.go Show resolved Hide resolved
dht_test.go Outdated Show resolved Hide resolved
dht_test.go Outdated Show resolved Hide resolved
dht_test.go Outdated Show resolved Hide resolved
dht_test.go Outdated Show resolved Hide resolved
@yiannisbot
Copy link

@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
Copy link

BigLep commented Apr 6, 2023

@guillaumemichel : what are the next steps here?

@guillaumemichel
Copy link
Contributor Author

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

@guillaumemichel guillaumemichel force-pushed the fix/unresponsive-peers branch 2 times, most recently from 10fbf09 to f083cb3 Compare April 6, 2023 08:27
@BigLep BigLep mentioned this pull request Apr 19, 2023
dht.go Outdated Show resolved Hide resolved
@Jorropo
Copy link
Contributor

Jorropo commented Jun 8, 2023

Also please resolve your conflicts.

@BigLep
Copy link

BigLep commented Jun 8, 2023

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 Outdated Show resolved Hide resolved
dht.go Outdated Show resolved Hide resolved
Copy link
Contributor

@Jorropo Jorropo left a comment

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

LGTM Ty <3

@Jorropo 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 Jorropo mentioned this pull request Jun 12, 2023
@Jorropo Jorropo merged commit 8c9fdff into libp2p:master Jun 12, 2023
Jorropo added a commit that referenced this pull request Jun 12, 2023
* 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>
Jorropo pushed a commit that referenced this pull request Jun 12, 2023
* 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)
achingbrain added a commit to libp2p/js-libp2p that referenced this pull request Apr 29, 2024
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.
achingbrain added a commit to libp2p/js-libp2p that referenced this pull request Apr 29, 2024
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.
cortze pushed a commit to cortze/go-libp2p-kad-dht that referenced this pull request Jul 30, 2024
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
Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment
Labels
None yet
Projects
None yet
Development

Successfully merging this pull request may close these issues.

4 participants