Upgrade DHT version #479
Merged
Upgrade DHT version #479
Conversation
This file contains bidirectional Unicode text that may be interpreted or compiled differently than what appears below. To review, open the file in an editor that reveals hidden Unicode characters.
Learn more about bidirectional Unicode characters
aschmahmann
changed the title
aschmahmann
force-pushed
the
feat/dht-upgrade
branch
from
d9307e1
to
4c17e75
Compare
aschmahmann
force-pushed
the
feat/mode-switching
branch
from
74ca902
to
d7523e1
Compare
aschmahmann
force-pushed
the
feat/dht-upgrade
branch
2 times, most recently
from
13bf3de
to
bf603e0
Compare
aschmahmann
force-pushed
the
feat/mode-switching
branch
from
5577ec7
to
a65ba4e
Compare
aschmahmann
force-pushed
the
feat/dht-upgrade
branch
from
bf603e0
to
1a4490b
Compare
|
Changed base to cypress. |
Stebalien
reviewed
Mar 6, 2020
aarshkshah1992
suggested changes
Mar 9, 2020
|
@aschmahmann Thanks for the brilliant offline explanation of why we need all this & how it all works. Just some comments requesting documentation to explain this to new code-readers who might not be well versed in the subtleties involved in protocol upgrades. |
aarshkshah1992
approved these changes
Mar 9, 2020
aschmahmann
force-pushed
the
feat/dht-upgrade
branch
from
1a4490b
to
0f3efa4
Compare
…e logic in a single location. add comments explaining that we only want to add peers speaking primary protocols in our routing tables.
…e primary protocols
…ons, left implemented and deprecated in options package
aschmahmann
commented
Mar 9, 2020
| if only { | ||
| return dht.Mode(dht.ModeClient) | ||
| } | ||
| return dht.Mode(dht.ModeAuto) |
There was a problem hiding this comment.
Is making the default here ModeAuto instead of doing nothing when only ==false ok?
Stebalien
approved these changes
Mar 10, 2020
| protocols []protocol.ID | ||
|
|
||
| // DHT protocols we can respond to (may contain protocols in addition to the primary protocols) | ||
| serverProtocols []protocol.ID |
There was a problem hiding this comment.
We probably also need kad1Protocol and kad2Protocol for figuring out how we should respond to peers.
There was a problem hiding this comment.
SGTM, we can tackle that once we actually have different protocol handlers to use (hopefully won't be too long 🙏)
aschmahmann
added a commit
that referenced
this pull request
Mar 10, 2020
* upgraded the protocol id to version 2 (i.e. /kad/2.0.0) and made it so v2 peers running in server mode respond to queries from v1 peers. Note: v2 peers will only send queries using the v2 protocol, will only add v2 peers to their routing tables, and will only tell v1 peers about v2 peers. * to run a forked network we now use network specific protocol prefixes instead of manually setting protocol IDs. Use the ProtocolPrefix option instead of the Protocols option. * emit errors during initialization if the user misuses the default protocol prefix by setting parameters inconsistent with the default protocol's network specification * since the Client option has been deprecated it's been removed from the dht's options. While deprecated it is still available in the dht options package. Setting `Client(false)` now puts the node into ModeAuto.
Stebalien
pushed a commit
that referenced
this pull request
Apr 3, 2020
* upgraded the protocol id to version 2 (i.e. /kad/2.0.0) and made it so v2 peers running in server mode respond to queries from v1 peers. Note: v2 peers will only send queries using the v2 protocol, will only add v2 peers to their routing tables, and will only tell v1 peers about v2 peers. * to run a forked network we now use network specific protocol prefixes instead of manually setting protocol IDs. Use the ProtocolPrefix option instead of the Protocols option. * emit errors during initialization if the user misuses the default protocol prefix by setting parameters inconsistent with the default protocol's network specification * since the Client option has been deprecated it's been removed from the dht's options. While deprecated it is still available in the dht options package. Setting `Client(false)` now puts the node into ModeAuto.
Sign up for free
to join this conversation on GitHub.
Already have an account?
Sign in to comment
Add this suggestion to a batch that can be applied as a single commit.
This suggestion is invalid because no changes were made to the code.
Suggestions cannot be applied while the pull request is closed.
Suggestions cannot be applied while viewing a subset of changes.
Only one suggestion per line can be applied in a batch.
Add this suggestion to a batch that can be applied as a single commit.
Applying suggestions on deleted lines is not supported.
You must change the existing code in this line in order to create a valid suggestion.
Outdated suggestions cannot be applied.
This suggestion has been applied or marked resolved.
Suggestions cannot be applied from pending reviews.
Suggestions cannot be applied on multi-line comments.
Suggestions cannot be applied while the pull request is queued to merge.
Suggestion cannot be applied right now. Please check back later.
Relates to libp2p/go-libp2p#780
Is currently targeted as merging into
feat/mode-switching, but after #469 is merged this PR will be redirected to merge ontocypress