-
-
Notifications
You must be signed in to change notification settings - Fork 3.1k
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
reduce dht bandwidth consumption #986
Conversation
Signed-off-by: Jeromy <[email protected]>
@@ -226,5 +226,5 @@ func (dht *IpfsDHT) handleAddProvider(ctx context.Context, p peer.ID, pmes *pb.M | |||
dht.providers.AddProvider(ctx, key, p) | |||
} | |||
|
|||
return pmes, nil // send back same msg as confirmation. |
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.
hmm so then we really have no way of confirming. could we maybe comment this out and make note of this? from a protocol standpoint it will be useful to confirm these things-- but perhaps the protocol can change.
By the way, muxing all dht traffic over one stream will really, really help.
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 change makes it more closely follow kademlia, and makes more sense with us wanting to change over to a UDP protocol for the DHT (not saying thats happening soon, just those concepts).
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.
agreed on that. but some dht protocols (particularly if we start doing cryptographic proofs on things) may call for other constructions too. in any case, not very important. either way
One comment, else LGTM |
reduce dht bandwidth consumption
reduce dht bandwidth consumption
A couple small changes to reduce overall dht bandwidth consumption