[PAN-2595] Consolidate local enode representation #1376

mbaxter · 2019-04-30T19:25:31Z

PR description

This PR consolidates local enode representations around a single canonical representation. Previously, there were a few places where we would construct EnodeURL instances to represent the local node. However, this representation is only truly accurate once the network starts up as ports can be chosen on startup.

Peers can be added to the maintained peer list before the network is fully started and permissions can be properly checked. So, for consistency, don't gate addition to the maintained peer list using permissions. Just don't connect to peers who are currently not allowed.

mbaxter · 2019-04-30T19:27:21Z

ethereum/eth/src/test/java/tech/pegasys/pantheon/ethereum/eth/transactions/TestNodeList.java

@@ -59,12 +59,6 @@ public TestNode create(
    return node;
  }

-  public void startNetworks() {


Each individual network is already started here

mbaxter · 2019-04-30T19:30:12Z

...src/main/java/tech/pegasys/pantheon/ethereum/p2p/InsufficientPeersPermissioningProvider.java


 /**
 * A permissioning provider that only provides an answer when we have no peers outside of our
 * bootnodes
 */
 public class InsufficientPeersPermissioningProvider implements ContextualNodePermissioningProvider {
-  private final EnodeURL selfEnode;
+  private final Supplier<Optional<EnodeURL>> selfEnode;


Previously, we passed in a static EnodeURL constructed in RunnerBuilder. This enode wasn't 100% accurate as the network can be configured to choose a port on startup, and the representation constructed in the builder would not accurately reflect the real ports being used. We're now passing a supplier that returns the canonical local enode representation when available.

mbaxter · 2019-04-30T19:30:57Z

...src/main/java/tech/pegasys/pantheon/ethereum/p2p/InsufficientPeersPermissioningProvider.java

-    } else if (checkEnode(sourceEnode) && checkEnode(destinationEnode)) {
+    } else if (!maybeSelfEnode.isPresent()) {
+      // The local node is not yet ready, so we can't validate enodes yet
+      return Optional.empty();


We'll now only return an answer when the network is ready.

mbaxter · 2019-04-30T19:34:51Z

ethereum/p2p/src/main/java/tech/pegasys/pantheon/ethereum/p2p/api/P2PNetwork.java

-   * @return boolean representing whether or not the peer has been added to the list or was already
-   *     on it
+   * @return boolean representing whether or not the peer has been added to the list, false is
+   *     returned if the peer was already on the list
   */
  boolean addMaintainConnectionPeer(final Peer peer);


Previously, peer permission would be checked as peers were added to this list. The changes in this PR delay the point at which we're able to check permission to the point where our local node is up and running. As a result, if we were to gate entrance into the list based on permissions, there would be a period where no nodes could be added to this list while the node is starting up. So, this API has been updated such that a peer can be added to the "maintain connection" list whether or not it is permitted on the network. However, no peers will actually be connected to without first checking permissions.

mbaxter · 2019-04-30T19:35:41Z

ethereum/p2p/src/main/java/tech/pegasys/pantheon/ethereum/p2p/network/DefaultP2PNetwork.java

@@ -346,7 +347,7 @@ protected void initChannel(final SocketChannel ch) {
                return;
              }

-              if (!isPeerConnectionAllowed(connection)) {
+              if (!isPeerAllowed(connection)) {


Permission checks have been centralized to one method.

mbaxter · 2019-04-30T19:36:27Z

ethereum/p2p/src/main/java/tech/pegasys/pantheon/ethereum/p2p/network/DefaultP2PNetwork.java

+  void checkMaintainedConnectionPeers() {
+    peerMaintainConnectionList.stream()
+        .filter(p -> !isConnectingOrConnected(p))
+        .filter(this::isPeerAllowed)


Check permissions before connecting :\

Shouldn't we drop them from the maintained list if they aren't permitted?

I don't think so. The peer might later be permitted.

mbaxter · 2019-04-30T19:37:04Z

ethereum/p2p/src/main/java/tech/pegasys/pantheon/ethereum/p2p/network/DefaultP2PNetwork.java

-          if (!blockAddedObserverId.isPresent()) {
-            blockAddedObserverId =
-                OptionalLong.of(blockchain.get().observeBlockAdded(this::handleBlockAddedEvent));
+    if (started.compareAndSet(false, true)) {


Start can be called at most once

nit: Should we switch this to use an early exit pattern rather than wrapping the whole method in an if?

mbaxter · 2019-04-30T19:38:02Z

ethereum/p2p/src/main/java/tech/pegasys/pantheon/ethereum/p2p/network/DefaultP2PNetwork.java

      return false;
    }

+    Optional<EnodeURL> maybeEnode = getLocalEnode();
+    if (!maybeEnode.isPresent()) {


Be restrictive around permissions until we have a valid representation of our local enode.

ajsutton

LGTM. Would just be good to confirm the permissioning related changes with someone from Bunyip.

ajsutton · 2019-04-30T20:26:12Z

ethereum/p2p/src/main/java/tech/pegasys/pantheon/ethereum/p2p/network/DefaultP2PNetwork.java

-          if (!blockAddedObserverId.isPresent()) {
-            blockAddedObserverId =
-                OptionalLong.of(blockchain.get().observeBlockAdded(this::handleBlockAddedEvent));
+    if (started.compareAndSet(false, true)) {


nit: Should we switch this to use an early exit pattern rather than wrapping the whole method in an if?

ajsutton · 2019-04-30T20:29:18Z

ethereum/p2p/src/main/java/tech/pegasys/pantheon/ethereum/p2p/network/DefaultP2PNetwork.java

  }

-  private EnodeURL buildSelfEnodeURL() {
+  private void setLocalEnode() {


nit: this probably should be createLocalEnode since it doesn't follow the typical setter pattern.

ajsutton · 2019-04-30T21:29:31Z

...reum/p2p/src/test/java/tech/pegasys/pantheon/ethereum/p2p/network/DefaultP2PNetworkTest.java

    final Peer peer = mockPeer();
    network.peerMaintainConnectionList.add(peer);

    network.checkMaintainedConnectionPeers();
    verify(network, times(1)).connect(peer);
  }

+  @Test
+  public void checkMaintainedConnectionPeersDoesNotConnectToDisallowedPeer() {


@lucassaldanha can you confirm that the permissioning controls should prevent peers added via staticnodes.json or admin_addPeer from connecting? It makes sense to me but I seem to recall some discussions and details...

For the staticnodes.json list we apply the same rules as for bootnodes: they must be permitted.

For admin_addPeer is the same logic as connecting to another peer. If the peer is not allowed it won't connect to the peer (and I believe the addPeer method will give you a nice error response saying why).

The "admin_addPeer" method will no longer return error messages if a non-permitted peer is added. Discussed this with @lucassaldanha offline and we determined that there is no need to keep these error messages.

lucassaldanha

LGTM. My only question is if we should remove peers not allowed from the maintained peer list at the moment we find out they aren't allowed.

lucassaldanha · 2019-04-30T22:16:07Z

ethereum/p2p/src/main/java/tech/pegasys/pantheon/ethereum/p2p/network/DefaultP2PNetwork.java

+  void checkMaintainedConnectionPeers() {
+    peerMaintainConnectionList.stream()
+        .filter(p -> !isConnectingOrConnected(p))
+        .filter(this::isPeerAllowed)


Shouldn't we drop them from the maintained list if they aren't permitted?

ajsutton · 2019-04-30T22:33:31Z

I'd be inclined to keep forbidden peers in the maintained list because they may become permitted later - especially with on-chain permissioning. You may have setup a new node, added it as a peer for your existing nodes and are now getting approvals from the consortium to allow it to actually connect.

mbaxter added 6 commits April 30, 2019 15:24

Create a LocalNode class for holding information about the local node

45fc6b7

Use LocalNode within DefaultP2PNetwork

4f86e9a

P2PNetwork should only be started once

c97239a

Don't construct local enode representation in RunnerBuilder

27e1d2d

Remove LocalNode in favor of a plain Optional

e95d068

mbaxter changed the title ~~Pan 2595/consolidate local enode rep~~ [PAN-2595] Consolidate local enode representation Apr 30, 2019

mbaxter commented Apr 30, 2019

View reviewed changes

mbaxter added 3 commits April 30, 2019 16:01

Test that we don't connect to disallowed maintained peers

eab4662

Use more precise times over atLeast

d4c8f2e

Fix method name

98e7be1

mbaxter marked this pull request as ready for review April 30, 2019 20:17

mbaxter added 2 commits April 30, 2019 16:17

Merge branch 'master' into PAN-2595/consolidate-local-enode-rep

128521b

Cut unused code

7335b49

ajsutton approved these changes Apr 30, 2019

View reviewed changes

mbaxter added 2 commits April 30, 2019 17:45

Code review cleanup

c3c25bd

Merge branch 'master' into PAN-2595/consolidate-local-enode-rep

ff867bb

lucassaldanha self-assigned this Apr 30, 2019

lucassaldanha reviewed Apr 30, 2019

View reviewed changes

Remove dead code

b286f7d

mbaxter merged commit 694a44a into PegaSysEng:master May 1, 2019

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

[PAN-2595] Consolidate local enode representation (PegaSysEng#1376)

0069f61

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

[PAN-2595] Consolidate local enode representation (PegaSysEng#1376)

dd1f236

Provide feedback

Saved searches

Use saved searches to filter your results more quickly

[PAN-2595] Consolidate local enode representation #1376

[PAN-2595] Consolidate local enode representation #1376

mbaxter commented Apr 30, 2019 •

edited

Loading

mbaxter Apr 30, 2019

mbaxter Apr 30, 2019

mbaxter Apr 30, 2019

mbaxter Apr 30, 2019

mbaxter Apr 30, 2019

mbaxter Apr 30, 2019

lucassaldanha Apr 30, 2019

Errorific May 1, 2019

mbaxter Apr 30, 2019

ajsutton Apr 30, 2019

mbaxter Apr 30, 2019

ajsutton left a comment

ajsutton Apr 30, 2019

ajsutton Apr 30, 2019

ajsutton Apr 30, 2019

lucassaldanha Apr 30, 2019

mbaxter May 1, 2019

lucassaldanha left a comment

lucassaldanha Apr 30, 2019

ajsutton commented Apr 30, 2019

[PAN-2595] Consolidate local enode representation #1376

[PAN-2595] Consolidate local enode representation #1376

Conversation

mbaxter commented Apr 30, 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

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

ajsutton 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

Choose a reason for hiding this comment

Choose a reason for hiding this comment

lucassaldanha left a comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

ajsutton commented Apr 30, 2019

mbaxter commented Apr 30, 2019 •

edited

Loading