Skip to content

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

Merged
fjl merged 4 commits into
ethereum:masterfrom
zsfelfoldi:portshare
Jan 22, 2018
Merged

p2p, p2p/discover, p2p/discv5: implement UDP port sharing#15200
fjl merged 4 commits into
ethereum:masterfrom
zsfelfoldi:portshare

Conversation

@zsfelfoldi
Copy link
Copy Markdown
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
Comment thread p2p/discv5/udp.go Outdated
// 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
Copy Markdown
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
Copy Markdown
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
Copy Markdown
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