-
Notifications
You must be signed in to change notification settings - Fork 235
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
Peer Diversity in the Routing Table #658
Conversation
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
Left comments here and in the other PR. Most of them are code based and probably boil down to:
- Could we just have a generic stateful filter we add/remove from, and then define the one we're using as a DHT subpackage?
However I do have a concept based concern regarding how we handle changes to peer addresses and multiple peer addresses.
- I'm concerned that because we don't have any event listeners plugged in that can perform RT peer removal that we're opening ourselves up for attack.
More info on both of these in the comment threads.
dht.go
Outdated
df, err := peerdiversity.NewFilter(dht.rtPeerDiversityFilter, "RT", func(p peer.ID) int { | ||
return kb.CommonPrefixLen(dht.selfKey, kb.ConvertPeerID(p)) | ||
}) |
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
Using "RT" as the tag doesn't really give us much. What if there are multiple DHT instantiations (e.g. WAN/LAN)? Maybe, aside from this PR, the DHT needs something like a WithLogTag()
that adds a tag to the logs using logger.With
.
dht_test.go
Outdated
bhost.New(swarmt.GenSwarm(t, ctx, swarmt.OptDisableReuseport)), | ||
h, |
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
did you mean to move this, or was it just for debugging?
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
Just for debugging. Have removed this.
go.mod
Outdated
@@ -18,7 +18,7 @@ require ( | |||
github.com/libp2p/go-eventbus v0.1.0 | |||
github.com/libp2p/go-libp2p v0.8.2 | |||
github.com/libp2p/go-libp2p-core v0.5.4 | |||
github.com/libp2p/go-libp2p-kbucket v0.4.2 | |||
github.com/libp2p/go-libp2p-kbucket v0.4.3-0.20200601163705-6904e7977e6a |
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
TODO: after libp2p/go-libp2p-kbucket#88 lands use a release here instead of a commit
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
This was done.
@aschmahmann I think I've addressed most of your comments. Please take a look. |
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
LGTM
This reverts commit df4f28c. Ongoing work is being done in feat/dht-hardening-0.7 and this change isn't quite ready.
Revert "Peer Diversity in the Routing Table (#658)"
This reverts commit c985184.
Kbucket PR at libp2p/go-libp2p-kbucket#88.