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

Upgrade DHT version #479

Merged
merged 9 commits into from
Mar 10, 2020
Merged

Upgrade DHT version #479

merged 9 commits into from
Mar 10, 2020

Conversation

aschmahmann
Copy link
Contributor

@aschmahmann aschmahmann commented Mar 5, 2020

Relates to libp2p/go-libp2p#780

  • Allows for DHT upgrades by having different client and server protocols
  • Updates the default protocol IDs.
  • Includes a defensive check against people messing with DHT options but still utilizing the default protocol ID

Is currently targeted as merging into feat/mode-switching, but after #469 is merged this PR will be redirected to merge onto cypress

@aschmahmann aschmahmann changed the base branch from master to cypress March 5, 2020 19:09
@aschmahmann aschmahmann changed the base branch from cypress to feat/mode-switching March 5, 2020 19:10
@aschmahmann aschmahmann self-assigned this Mar 5, 2020
@aschmahmann aschmahmann changed the title Allow for DHT upgrades by having different client and server protocols Upgrade DHT version Mar 5, 2020
@aschmahmann aschmahmann force-pushed the feat/dht-upgrade branch 2 times, most recently from 13bf3de to bf603e0 Compare March 6, 2020 08:05
@Stebalien Stebalien changed the base branch from feat/mode-switching to cypress March 6, 2020 23:21
@Stebalien
Copy link
Member

Changed base to cypress.

opts/options.go Outdated Show resolved Hide resolved
opts/options.go Outdated Show resolved Hide resolved
opts/options.go Outdated Show resolved Hide resolved
subscriber_notifee.go Outdated Show resolved Hide resolved
dht_net.go Outdated Show resolved Hide resolved
dht.go Outdated Show resolved Hide resolved
@aarshkshah1992
Copy link
Contributor

aarshkshah1992 commented 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.

if only {
return dht.Mode(dht.ModeClient)
}
return dht.Mode(dht.ModeAuto)
Copy link
Contributor Author

Choose a reason for hiding this comment

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

Is making the default here ModeAuto instead of doing nothing when only ==false ok?

@Stebalien Stebalien added this to the Working Kademlia milestone Mar 10, 2020
Copy link
Member

@Stebalien Stebalien left a comment

Choose a reason for hiding this comment

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

Feel free to ignore my only comment, we can handle that in a future patch.

protocols []protocol.ID

// DHT protocols we can respond to (may contain protocols in addition to the primary protocols)
serverProtocols []protocol.ID
Copy link
Member

Choose a reason for hiding this comment

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

We probably also need kad1Protocol and kad2Protocol for figuring out how we should respond to peers.

Copy link
Contributor Author

Choose a reason for hiding this comment

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

SGTM, we can tackle that once we actually have different protocol handlers to use (hopefully won't be too long 🙏)

@aschmahmann aschmahmann merged commit 20355c5 into cypress Mar 10, 2020
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
Labels
None yet
Projects
None yet
Development

Successfully merging this pull request may close these issues.

4 participants