all: new p2p node representation #17643

fjl · 2018-09-11T17:32:13Z

This PR is the long-announced change to clean up p2p node information
handling throughout the codebase.

The decision to submit all these changes in a single PR was not easy. An
alternative would have been to create a temporary backwards-compatibility
layer which would have been in place while all packages were adapted. I
decided to follow through with the big PR because API requirements were not
clear when I started working on this. The new p2p/enode package changed
many times during this process.

Porting to enode.Node brings support for ENR features to every place that
deals with node information, but more work will be needed to actually start
relaying node records across protocols. For now, most records handled in code
are unsigned records created through enode.NewV4. Changes in devp2p and
discovery will enable relaying signed records and updating them.

The new p2p/enode package

The new package defines a type Node, which wraps a node record and provides
access to common information such as IP address and ports as methods.
This package is the new home of the enode URL parser and the node database.

Changes to existing code

The core of this change are the following replacements across the codebase:

discover.Node -> enode.Node
discover.NodeID (pubkey) -> enode.ID (hash of pubkey)

There are other changes in certain packages to deal with the fact
that node IDs aren't public keys anymore. I've tried to document those changes
in the individual commits.

zelig

You better rebase when chunk refactor and kademlia refactor PRs are merged.
@janos will help you.
I think in a followup PR we should get rid of BzzAddr altogether.

zelig · 2018-09-11T21:17:49Z

swarm/network/protocol.go

-		UAddr: []byte(discover.NewNode(id, host, port, port).String()),
-	}
+	node := enode.NewV4(&key.PublicKey, net.IP{127, 0, 0, 1}, 30303, 30303)
+	return &BzzAddr{OAddr: node.ID().Bytes(), UAddr: []byte(node.String())}


use NewAddr method here for DRY

zelig · 2018-09-11T21:22:54Z

swarm/network/protocol.go

-// ID returns the nodeID from the underlay enode address
-func (a *BzzAddr) ID() discover.NodeID {
-	return discover.MustParseNode(string(a.UAddr)).ID
+// ID returns the node identifier in the underlay.


since this ID now is the same as OAddr, the code can simplify immensely.
We would have saved months if this had been done earlier :)

actually that doesn't appear to be true for bzz. aren't there cases where oaddr and uaddr are derived from different public keys?

holiman · 2018-09-14T06:53:21Z

@fjl, this is kind of difficult to audit with 👀 only.

What would be a good deployment scenario to test this? Could I e.g. deploy a couple of nodes built on this PR to the monitoring environment and check how it works? And if so, are there any

special config flags to use?
fast/full/light sync?
any particular metrics that we want to visualize on datadog?

fjl · 2018-09-14T07:11:35Z

The PR should behave same as any other node, no flags are needed. Would be nice to have it running to see if there are crashes. We can check CPU usage, that might be interesting.

janos · 2018-09-14T15:48:50Z

p2p/simulations/adapters/inproc.go

-	return discover.NewNode(sn.ID, net.IP{127, 0, 0, 1}, 30303, 30303)
+// Node returns a node descriptor representing the SimNode
+func (sn *SimNode) Node() *enode.Node {
+	return enode.NewV4(&sn.config.PrivateKey.PublicKey, net.IP{127, 0, 0, 1}, 30303, 30303)


Hi @fjl, in order for network.TestHiveStatePersistance to pass constantly, the Node here has to have the same parameters for both Hive Register and On calls. Node() method should contain only this:

return sn.config.Node()

This will preserve the port numbers as well.

This looks like an old bug that is now more frequent, for some reason.

fjl · 2018-09-14T19:58:09Z

@janos Thank you! The test is fixed now.

holiman · 2018-09-17T10:43:41Z

p2p/discover/table.go

 	// Start the background expiration goroutine after loading seeds so that the search for
 	// seed nodes also considers older nodes that would otherwise be removed by the
 	// expiration.
-	tab.db.ensureExpirer()
+	// tab.db.ensureExpirer()


Why is this removed? And shouldn't the comment above also be removed?

holiman · 2018-09-17T10:45:16Z

p2p/discover/table.go

@@ -405,7 +396,7 @@ loop:
 	for _, ch := range waiting {
 		close(ch)
 	}
-	tab.db.close()
+	// tab.db.close()


zsfelfoldi · 2018-09-17T13:47:11Z

les/serverpool.go

+func (pool *serverPool) discoverNodes() {
+	ch := make(chan *discv5.Node)
+	go pool.server.DiscV5.SearchTopic(pool.topic, pool.discSetPeriod, ch, pool.discLookups)
+	for n := range ch {


SearchTopic returns when shutting down but does not close ch when exiting so this loop won't quit. You should close it in the goroutine after SearchTopic has returned.

zsfelfoldi

LES part looks fine except for the discoverNodes shutdown issue.

fjl · 2018-09-19T09:49:32Z

@holiman @zsfelfoldi PTAL

gballet

Just a minor nitpick; otherwise it looks good to me as far as Whisper is concerned.

gballet · 2018-09-19T15:01:31Z

whisper/whisperv5/api.go

 	if err != nil {
 		return false, err
 	}
-	return true, api.w.AllowP2PMessagesFromPeer(n.ID[:])
+	return true, api.w.AllowP2PMessagesFromPeer(n.ID().Bytes())


In v6, you write the same code as:

id := n.ID() err = api.w.SendP2PMessage(id[:], env)

I would appreciate if you used the same thing in both versions. This is so that if people (say, security auditors) want to compare v5 and v6, the diffs will show something that is as close as possible to only changes in the protocol. For that same reason, I would prefer if you picked n.ID().Bytes() over what is currently found in v6.

fjl · 2018-09-20T12:20:15Z

@gballet PTAL

gballet

LGTM on the whisper side

Package p2p/enode provides a generalized representation of p2p nodes which can contain arbitrary information in key/value pairs. It is also the new home for the node database. The "v4" identity scheme is also moved here from p2p/enr to remove the dependency on Ethereum crypto from that package. Record signature handling is changed significantly. The identity scheme registry is removed and acceptable schemes must be passed to any method that needs identity. This means records must now be validated explicitly after decoding. The enode API is designed to make signature handling easy and safe: most APIs around the codebase work with enode.Node, which is a wrapper around a valid record. Going from enr.Record to enode.Node requires a valid signature.

This ports the discovery code to the new node representation in p2p/enode. The wire protocol is unchanged, this can be considered a refactoring change. The Kademlia table can now deal with nodes using an arbitrary identity scheme. This requires a few incompatible API changes: - Table.Lookup is not available anymore. It used to take a public key as argument because v4 protocol requires one. Its replacement is LookupRandom. - Table.Resolve takes *enode.Node instead of NodeID. This is also for v4 protocol compatibility because nodes cannot be looked up by ID alone. - Types Node and NodeID are gone. Further commits in the series will be fixes all over the the codebase to deal with those removals.

zeerax25 · 2018-10-17T07:25:37Z

So the length of pubkey is not 128 bytes for URL: enode://:<ip_addr>: any more?

shazow · 2018-10-28T14:09:58Z

Is this change documented anywhere outside of this code/commit? Are other client implementations also expected to use this format, or is this going to be geth-only?

Package p2p/enode provides a generalized representation of p2p nodes which can contain arbitrary information in key/value pairs. It is also the new home for the node database. The "v4" identity scheme is also moved here from p2p/enr to remove the dependency on Ethereum crypto from that package. Record signature handling is changed significantly. The identity scheme registry is removed and acceptable schemes must be passed to any method that needs identity. This means records must now be validated explicitly after decoding. The enode API is designed to make signature handling easy and safe: most APIs around the codebase work with enode.Node, which is a wrapper around a valid record. Going from enr.Record to enode.Node requires a valid signature. * p2p/discover: port to p2p/enode This ports the discovery code to the new node representation in p2p/enode. The wire protocol is unchanged, this can be considered a refactoring change. The Kademlia table can now deal with nodes using an arbitrary identity scheme. This requires a few incompatible API changes: - Table.Lookup is not available anymore. It used to take a public key as argument because v4 protocol requires one. Its replacement is LookupRandom. - Table.Resolve takes *enode.Node instead of NodeID. This is also for v4 protocol compatibility because nodes cannot be looked up by ID alone. - Types Node and NodeID are gone. Further commits in the series will be fixes all over the the codebase to deal with those removals. * p2p: port to p2p/enode and discovery changes This adapts package p2p to the changes in p2p/discover. All uses of discover.Node and discover.NodeID are replaced by their equivalents from p2p/enode. New API is added to retrieve the enode.Node instance of a peer. The behavior of Server.Self with discovery disabled is improved. It now tries much harder to report a working IP address, falling back to 127.0.0.1 if no suitable address can be determined through other means. These changes were needed for tests of other packages later in the series. * p2p/simulations, p2p/testing: port to p2p/enode No surprises here, mostly replacements of discover.Node, discover.NodeID with their new equivalents. The 'interesting' API changes are: - testing.ProtocolSession tracks complete nodes, not just their IDs. - adapters.NodeConfig has a new method to create a complete node. These changes were needed to make swarm tests work. Note that the NodeID change makes the code incompatible with old simulation snapshots. * whisper/whisperv5, whisper/whisperv6: port to p2p/enode This port was easy because whisper uses []byte for node IDs and URL strings in the API. * eth: port to p2p/enode Again, easy to port because eth uses strings for node IDs and doesn't care about node information in any way. * les: port to p2p/enode Apart from replacing discover.NodeID with enode.ID, most changes are in the server pool code. It now deals with complete nodes instead of (Pubkey, IP, Port) triples. The database format is unchanged for now, but we should probably change it to use the node database later. * node: port to p2p/enode This change simply replaces discover.Node and discover.NodeID with their new equivalents. * swarm/network: port to p2p/enode Swarm has its own node address representation, BzzAddr, containing both an overlay address (the hash of a secp256k1 public key) and an underlay address (enode:// URL). There are no changes to the BzzAddr format in this commit, but certain operations such as creating a BzzAddr from a node ID are now impossible because node IDs aren't public keys anymore. Most swarm-related changes in the series remove uses of NewAddrFromNodeID, replacing it with NewAddr which takes a complete node as argument. ToOverlayAddr is removed because we can just use the node ID directly.

This reverts commit 1bdf536.

fjl requested review from gbalint, gballet, gluk256, holisticode, janos, acud, karalabe, nolash, nonsense, zelig and zsfelfoldi as code owners September 11, 2018 17:32

zelig reviewed Sep 12, 2018

View reviewed changes

fjl force-pushed the p2p-enode-final branch 2 times, most recently from 83eab98 to 1e01741 Compare September 13, 2018 13:46

janos reviewed Sep 14, 2018

View reviewed changes

fjl force-pushed the p2p-enode-final branch from 1e01741 to 89f31f4 Compare September 14, 2018 19:57

fjl force-pushed the p2p-enode-final branch from 89f31f4 to 6c69e4c Compare September 14, 2018 20:03

holiman reviewed Sep 17, 2018

View reviewed changes

zsfelfoldi reviewed Sep 17, 2018

View reviewed changes

gballet requested changes Sep 19, 2018

View reviewed changes

gballet approved these changes Sep 21, 2018

View reviewed changes

fjl force-pushed the p2p-enode-final branch from 93395bf to 24ec118 Compare September 24, 2018 21:56

fjl added 5 commits September 24, 2018 23:56

swarm/network/simulations: port to p2p/enode

46e3790

swarm/network/stream: port to p2p/enode

421a83d

swarm/pss: port to p2p/enode

8b3a50a

swarm: port to p2p/enode

92edb4d

cmd/...: port to p2p/enode

45eb95f

fjl force-pushed the p2p-enode-final branch from 24ec118 to 45eb95f Compare September 24, 2018 21:57

fjl merged commit 30cd5c1 into ethereum:master Sep 24, 2018

nambrot mentioned this pull request Oct 26, 2018

Developers SBAT to use the same geth version for the bootnode and normal nodes celo-org/celo-blockchain#78

Closed

DefiCake mentioned this pull request Jun 1, 2020

Cannot parse 32 bytes enodes in TOML configuration file #21156

Closed

benbaley mentioned this pull request Jan 14, 2021

Feature/merge ethereum PlatONnetwork/PlatON-Go#1675

Closed

trinhdn2 mentioned this pull request Oct 27, 2023

New P2P node implementation BuildOnViction/victionchain#395

Closed

wanwiset25 mentioned this pull request Jun 3, 2024

Increase txsize XinFinOrg/XDPoSChain#550

Merged

19 tasks

wanwiset25 added a commit to XinFinOrg/XDPoSChain that referenced this pull request Aug 23, 2024

Revert "all: new p2p node representation (ethereum#17643)"

aa17504

This reverts commit 1bdf536.

Provide feedback

Saved searches

Use saved searches to filter your results more quickly

all: new p2p node representation #17643

all: new p2p node representation #17643

fjl commented Sep 11, 2018 •

edited

Loading

zelig left a comment

zelig Sep 11, 2018

zelig Sep 11, 2018

fjl Sep 12, 2018

holiman commented Sep 14, 2018

fjl commented Sep 14, 2018 •

edited

Loading

janos Sep 14, 2018

fjl commented Sep 14, 2018

holiman Sep 17, 2018

holiman Sep 17, 2018

zsfelfoldi Sep 17, 2018

zsfelfoldi left a comment

fjl commented Sep 19, 2018

gballet left a comment

gballet Sep 19, 2018

fjl commented Sep 20, 2018

gballet left a comment

zeerax25 commented Oct 17, 2018

shazow commented Oct 28, 2018

all: new p2p node representation #17643

all: new p2p node representation #17643

Conversation

fjl commented Sep 11, 2018 • edited Loading

The new p2p/enode package

Changes to existing code

zelig left a comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

holiman commented Sep 14, 2018

fjl commented Sep 14, 2018 • edited Loading

Choose a reason for hiding this comment

fjl commented Sep 14, 2018

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

zsfelfoldi left a comment

Choose a reason for hiding this comment

fjl commented Sep 19, 2018

gballet left a comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

fjl commented Sep 20, 2018

gballet left a comment

Choose a reason for hiding this comment

zeerax25 commented Oct 17, 2018

shazow commented Oct 28, 2018

fjl commented Sep 11, 2018 •

edited

Loading

fjl commented Sep 14, 2018 •

edited

Loading