Update EnodeURL to support enodes with listening disabled #1403

mbaxter · 2019-05-06T18:20:14Z

PR description

Update EnodeURL to handle representations that indicate a node is not listening for incoming connections, is not running discovery, or both. When parsing string enode url's, a port of "0" is interpreted as "not listening".

Both getListeningPort() and getDiscoveryPort() now returnOptional values indicating whether listening / discovery are enabled. Previously, getDiscoveryPort() would return an empty Optional if the discovery port was not distinct from the listening port. Since an empty value now indicates "not listening", getDiscoveryPort() now returns a non-empty value if discovery is enabled implicitly or explicitly.

Bootnode validation has been added to ensure that bootnodes have discovery enabled. If bootnodes with discovery disabled are supplied via the CLI, an error is returned. In order to facilitate this change, DiscoveryConfiguration bootnodes are now set as EnodeURL's rather than URI objects.

Add bootnode validation to CLI command.

mbaxter · 2019-05-10T17:35:13Z

.../src/test/java/tech/pegasys/pantheon/tests/acceptance/dsl/node/ThreadPantheonNodeRunner.java

@@ -104,6 +112,7 @@ public void startNode(final PantheonNode node) {
            .metricsSystem(noOpMetricsSystem)
            .metricsConfiguration(node.metricsConfiguration())
            .p2pEnabled(node.isP2pEnabled())
+            .graphQLRpcConfiguration(GraphQLRpcConfiguration.createDefault())


Not sure why this was failing on my branch and not master - was getting a null pointer exception because this dependency wasn't set ...

Any ideas @shemnon ??

Looks like this was missed.

The big issue I see is that the threaded acceptance runner is only ever run by developers, the process runner is run on the jenkins boxes instead and would have broke there.

Ah, so maybe it was just a flaky test and when I ran it locally I got the null pointer?

mbaxter · 2019-05-10T17:45:29Z

...tech/pegasys/pantheon/ethereum/permissioning/NodeLocalConfigPermissioningControllerTest.java

@@ -194,19 +194,21 @@ public void whenNodesListeningPortsAreDifferentItShouldNotBePermitted() {
  }

  @Test
-  public void whenCheckingIfNodeIsPermittedDiscoveryPortShouldNotBeConsideredIfAbsent() {
+  public void
+      whenCheckingIfNodeIsPermittedDiscoveryPortShouldBeConsidered_implicitDiscPortMismatch() {


@lucassaldanha - How do you think these permissions should work wrt discovery? I've changed EnodeURL so that it can represent the case where discovery is actually disabled. So now the discoveryPort is only empty if discovery is disabled which meant I had to rework these checks a bit.

I think the code would be simpler if we just ignore the discovery port and use EnodeURL.sameListeningEndpoint to check enodes against the whitelist. Alternatively, we could check that the discovery port values are the same (so if one peer has discovery enabled and the other doesn't, they don't match).

What do you think?

I actually just went ahead and updated NodeLocalConfigPermissioningController to ignore the discovery port. Let me know if you disagree.

shemnon · 2019-05-10T19:50:07Z

...main/java/tech/pegasys/pantheon/ethereum/p2p/discovery/internal/PeerDiscoveryController.java

@@ -401,10 +401,10 @@ private void notifyPeerDropped(final DiscoveryPeer peer, final long now) {
  private void refreshTableIfRequired() {
    final long now = System.currentTimeMillis();
    if (lastRefreshTime + tableRefreshIntervalMs <= now) {
-      LOG.info("Peer table refresh triggered by timer expiry");
+      LOG.trace("Peer table refresh triggered by timer expiry");


would debug be more appropriate? Either way.

+1 for debug. This seems to be quite an atomic log message and not something that we are interested in different steps for tracing. But I'm ok with either.

shemnon · 2019-05-10T19:50:32Z

...main/java/tech/pegasys/pantheon/ethereum/p2p/discovery/internal/PeerDiscoveryController.java

      refreshTable();
    } else if (!peerRequirement.hasSufficientPeers()) {
-      LOG.info("Peer table refresh triggered by insufficient peers");
+      LOG.trace("Peer table refresh triggered by insufficient peers");


Would debug be more appropriate? Either way.

Same as above. +1 for Debug.

shemnon · 2019-05-10T20:55:48Z

util/src/main/java/tech/pegasys/pantheon/util/enode/EnodeURL.java

+      return false;
+    }
+
+    return Objects.equal(enodeA.nodeId, enodeB.nodeId)


Google's Objects.equal says not to use it after Java 7. Can these change to java.util.Object.equals? (the replacement Guava recommends)

yikes - we have a lot of these (Objects.hashCode too), wonder if we can we add an error-prone check for this. I'll open a ticket to clean this up globally.

I've got a PR ready to roll that conflicts with some of this code. If you want to submit it as is that's fine, I can get the PR out today.

https://pegasys1.atlassian.net/browse/PAN-2629

lucassaldanha · 2019-05-13T00:40:34Z

util/src/main/java/tech/pegasys/pantheon/util/enode/EnodeURL.java

      final OptionalInt discoveryPort) {
    checkArgument(
        nodeId.size() == NODE_ID_SIZE, "Invalid node id.  Expected id of length: 64 bytes.");
    checkArgument(
-        NetworkUtility.isValidPort(listeningPort),
+        !listeningPort.isPresent() || NetworkUtility.isValidPort(listeningPort.getAsInt()),


Question: Why do we need to check if the port is available for this representation of an EnodeURL? For example, what will happen if port 1234 is not available but we are only trying to instantiate a representation of another node's EnodeURL that is listening on 1234?

This just checks that the port is a valid port number, not that the port is available.

lucassaldanha · 2019-05-13T00:44:48Z

util/src/main/java/tech/pegasys/pantheon/util/enode/EnodeURL.java

@@ -246,18 +286,92 @@ public Builder ipAddress(final String ip) {
      return this;
    }

-    public Builder listeningPort(final Integer listeningPort) {
-      this.listeningPort = listeningPort;
+    public Builder ports(final int listeningAndDiscoveryPorts) {


I think this method name doesn't clearly communicate the outcome of using it. Either we remove it (and force the user to use listeningPort() and discoveryPort() or we rename it to something a bit clearer.
e.g. discoveryAndListeningPorts()

👍 updated

…#1403)

mbaxter force-pushed the PAN-2616/make-enode-ports-optional branch from 93d6313 to 8501ccb Compare May 8, 2019 17:59

mbaxter added 10 commits May 10, 2019 11:58

Update EnodeURL to support enodes with listening disabled

5e11699

Simplify method

3c47032

Return early if P2PNetwork is asked to connect to an invalid enode

7ccc0eb

Add tests for parsing 0-valued ports

513ab3c

Add tests for EnodeURL.Builder

977f55e

Run code formatter

042ef7b

Add tests for EnodeURL.sameListeningEndpoint()

5ebf7a3

Throw exception if PeerDiscoveryAgent receives invalid bootnodes

ca31fbf

Keep bootnodes as EnodeURLs within DiscoveryConfiguration

3b41c03

Move bootnode validation to DiscoveryConfiguration

bfa7c3f

Add bootnode validation to CLI command.

mbaxter force-pushed the PAN-2616/make-enode-ports-optional branch from 7ae7dac to bfa7c3f Compare May 10, 2019 16:03

mbaxter added 4 commits May 10, 2019 12:48

Add some more tests, run code formatter

2ef811b

Add missing graphql dependency to RunnerBuilder

dfd3ea0

Fix CLI tests

46af8ad

Quieter logs

72ff9fa

mbaxter commented May 10, 2019

View reviewed changes

mbaxter marked this pull request as ready for review May 10, 2019 17:50

mbaxter added 3 commits May 10, 2019 14:31

Require static-nodes to have a non-zero listening port

b40ab18

Require maintained peers to have a non-zero listening port

762aa06

Update localConfig node permissioning ignore discovery port

224e64a

shemnon approved these changes May 10, 2019

View reviewed changes

lucassaldanha reviewed May 13, 2019

View reviewed changes

mbaxter added 2 commits May 13, 2019 10:25

Address review comments

069777c

Merge branch 'master' into PAN-2616/make-enode-ports-optional

51d46ba

mbaxter merged commit d0279cb into PegaSysEng:master May 13, 2019

notlesh pushed a commit to notlesh/pantheon that referenced this pull request May 14, 2019

Update EnodeURL to support enodes with listening disabled (PegaSysEng…

32b587e

…#1403)

Provide feedback

Saved searches

Use saved searches to filter your results more quickly

Update EnodeURL to support enodes with listening disabled #1403

Update EnodeURL to support enodes with listening disabled #1403

mbaxter commented May 6, 2019 •

edited

Loading

mbaxter May 10, 2019

mbaxter May 10, 2019

shemnon May 10, 2019

mbaxter May 10, 2019

mbaxter May 10, 2019 •

edited

Loading

mbaxter May 10, 2019

shemnon May 10, 2019

lucassaldanha May 13, 2019

shemnon May 10, 2019

lucassaldanha May 13, 2019

shemnon May 10, 2019

mbaxter May 10, 2019

shemnon May 10, 2019

mbaxter May 10, 2019

lucassaldanha May 13, 2019

mbaxter May 13, 2019

lucassaldanha May 13, 2019

mbaxter May 13, 2019

Update EnodeURL to support enodes with listening disabled #1403

Update EnodeURL to support enodes with listening disabled #1403

Conversation

mbaxter commented May 6, 2019 • edited Loading

PR description

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

mbaxter May 10, 2019 • edited Loading

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

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

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

Choose a reason for hiding this comment

Choose a reason for hiding this comment

mbaxter commented May 6, 2019 •

edited

Loading

mbaxter May 10, 2019 •

edited

Loading