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

p2p, p2p/discover, p2p/discv5: implement UDP port sharing #15200

Merged
merged 4 commits into from
Jan 22, 2018

Conversation

zsfelfoldi
Copy link
Contributor

@zsfelfoldi zsfelfoldi commented Sep 25, 2017

This PR breaks v5 DHT connectivity. les clients running this PR won't find servers running an earlier version.

This PR affects p2p/discv5 "topic discovery" by running it on the same UDP port where the old discovery works. This is realized by giving an "unhandled" packet channel to the old v4 discovery packet handler where all invalid packets are sent. These packets are then processed by v5. v5 packets are always invalid when interpreted by v4 and vice versa. This is ensured by adding one to the first byte of the packet hash in v5 packets.

DiscoveryV5Bootnodes is also changed to point to new bootnodes that are implementing the changed packet format with modified hash. Existing and new v5 bootnodes are both running on different ports ATM.

@fjl fjl added this to the 1.8.0 milestone Nov 13, 2017
@@ -376,6 +368,7 @@ func encodePacket(priv *ecdsa.PrivateKey, ptype byte, req interface{}) (p, hash
// add the hash to the front. Note: this doesn't protect the
// packet in any way.
hash = crypto.Keccak256(packet[macSize:])
hash[0]++ // guarantee incompatibility between v4 and temporary v5 packet formats
Copy link
Contributor

Choose a reason for hiding this comment

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

This makes the format incompatible with v4, but also breaks compatibility with the previous v5 format. The DHT will be split. I'm not sure that's OK. We're about to break the format anyway though, so maybe it is OK after all. I held off merging this PR because of this issue.

Copy link
Contributor Author

Choose a reason for hiding this comment

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

Port sharing breaks compatibility anyway, not because of the format but because of the changed port address (the +1/-1 offset was hardcoded). I think if we do not break compatibility with existing v5 format that would cause hell. I want to get rid of the dual port chaos and have a fresh start. Right now the server advertising of the latest release is broken anyway (there was a bug that broke registration when registering multiple topics) so we have nothing to lose. I hope if we release this and people update then we will at least return to pre-LES/2 usability which will be fine until we can release the new and reworked discovery.

Copy link
Contributor

Choose a reason for hiding this comment

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

OK

@fjl fjl merged commit 92580d6 into ethereum:master Jan 22, 2018
prestonvanloon pushed a commit to prestonvanloon/go-ethereum that referenced this pull request Apr 2, 2018
…5200)

This commit affects p2p/discv5 "topic discovery" by running it on
the same UDP port where the old discovery works. This is realized
by giving an "unhandled" packet channel to the old v4 discovery
packet handler where all invalid packets are sent. These packets
are then processed by v5. v5 packets are always invalid when
interpreted by v4 and vice versa. This is ensured by adding one
to the first byte of the packet hash in v5 packets.

DiscoveryV5Bootnodes is also changed to point to new bootnodes
that are implementing the changed packet format with modified
hash. Existing and new v5 bootnodes are both running on different
ports ATM.
mariameda pushed a commit to NiluPlatform/go-nilu that referenced this pull request Aug 23, 2018
…5200)

This commit affects p2p/discv5 "topic discovery" by running it on
the same UDP port where the old discovery works. This is realized
by giving an "unhandled" packet channel to the old v4 discovery
packet handler where all invalid packets are sent. These packets
are then processed by v5. v5 packets are always invalid when
interpreted by v4 and vice versa. This is ensured by adding one
to the first byte of the packet hash in v5 packets.

DiscoveryV5Bootnodes is also changed to point to new bootnodes
that are implementing the changed packet format with modified
hash. Existing and new v5 bootnodes are both running on different
ports ATM.
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.

2 participants