[PAN-2614] Add simple PeerPermissions interface #1446

mbaxter · 2019-05-14T21:45:36Z

PR description

This PR adds a simple PeerPermissions interface as a first step in trying to define a common interface for permission-related logic within the p2p package.

Summary of changes:

Create a basic PeerPermissions interface
Break up PeerBlacklist into several smaller classes and put a subset of these classes behind the new interface
- Create a PeerPermissionsBlacklist class that just holds a set of peers that are not allowed. The list can be optionally capped with a max number of tracked peers.
- Create a LimitedSet class to handle creating a capped set
- Move logic for blacklisting peers based on disconnect into a PeerReputationManager class
- Separate out lists of nodes that are banned because of bad behavior vs banned via the CLI
- Delete a bunch of duplicate tests in the p2p test suite that were validating logic that now belongs in PeerReputationManager
Validate that arguments to --banned-node-ids are valid node ids
Don't indiscriminately drop peers from our peer table for any disconnect
- Instead, drop peers when they are not allowed
- Since we're no longer dropping peers based on any disconnect, when pulling discovery peers to connect to, sort them so that we start with the least recently contacted peers first
Delete dead code related to tracking dropped peer events
Add a PeerDiscoveryController Builder to make updates to the long list of constructor dependencies easier
Validate peers before initiating outbound connections

Rather than dropping peers from the peer table for any disconnect, drop peers from the table on permissions changes, when peers are requested to be disconnected, and when peers are disconnecting for failed permissions check.

mbaxter · 2019-05-14T22:15:01Z

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

@@ -108,7 +107,6 @@ public void startNode(final PantheonNode node) {
            .jsonRpcConfiguration(node.jsonRpcConfiguration())
            .webSocketConfiguration(node.webSocketConfiguration())
            .dataDir(node.homeDirectory())
-            .bannedNodeIds(Collections.emptySet())


We no longer need to configure an empty set of banned nodes

mbaxter · 2019-05-14T22:16:35Z

ethereum/p2p/src/main/java/tech/pegasys/pantheon/ethereum/p2p/discovery/PeerDiscoveryAgent.java

-   */
-  public long observePeerDroppedEvents(final Consumer<PeerDroppedEvent> observer) {
-    checkNotNull(observer);
-    return peerDroppedObservers.subscribe(observer);


This was dead code - listeners were never actually notified of dropped peers

mbaxter · 2019-05-14T22:17:26Z

ethereum/p2p/src/main/java/tech/pegasys/pantheon/ethereum/p2p/discovery/PeerDiscoveryAgent.java

@@ -80,7 +72,6 @@
  /* The keypair used to sign messages. */
  protected final SECP256K1.KeyPair keyPair;
  private final BytesValue id;
-  private final PeerTable peerTable;


PeerTable isn't used in PeerDiscoveryAgent - moved it into the PeerDiscoveryController where it is used.

mbaxter · 2019-05-14T22:18:51Z

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

@@ -367,27 +382,11 @@ private boolean addToPeerTable(final DiscoveryPeer peer) {
    return true;
  }

-  @VisibleForTesting
-  boolean dropFromPeerTable(final DiscoveryPeer peer) {


mbaxter · 2019-05-14T22:20:34Z

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

@@ -348,6 +349,7 @@ protected void initChannel(final SocketChannel ch) {

              if (!isPeerAllowed(connection)) {
                connection.disconnect(DisconnectReason.UNKNOWN);
+                peerDiscoveryAgent.dropPeer(connection.getPeer());


Previously we were dropping all peers who disconnected. Now, we're explicitly dropping a subset of disconnected peers.

mbaxter · 2019-05-14T22:21:06Z

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

@@ -407,8 +411,9 @@ void attemptPeerConnections() {
        streamDiscoveredPeers()
            .filter(peer -> peer.getStatus() == PeerDiscoveryStatus.BONDED)
            .filter(peer -> !isConnected(peer) && !isConnecting(peer))
+            .sorted(Comparator.comparing(DiscoveryPeer::getLastAttemptedConnection))


Try connecting to discovery peers who haven't recently been contacted.

mbaxter · 2019-05-14T22:25:31Z

.../java/tech/pegasys/pantheon/ethereum/p2p/discovery/internal/PeerDiscoveryControllerTest.java

-
-    doReturn(EvictResult.absent()).when(peerTableSpy).tryEvict(any());
-
-    controller = getControllerBuilder().peerDroppedObservers(peerDroppedSubscribers).build();


This test hit the dead code that was removed

mbaxter · 2019-05-15T15:38:42Z

ethereum/p2p/src/main/java/tech/pegasys/pantheon/ethereum/p2p/discovery/PeerDiscoveryAgent.java

  protected static final Logger LOG = LogManager.getLogger();

  // The devp2p specification says only accept packets up to 1280, but some
  // clients ignore that, so we add in a little extra padding.
  private static final int MAX_PACKET_SIZE_BYTES = 1600;
-  private static final long PEER_REFRESH_INTERVAL_MS = MILLISECONDS.convert(30, TimeUnit.MINUTES);


Moved this constant into PeerDiscoveryController where it is used

shemnon

Would some of the pre-review comments be better served as code comments?
all "consider" comments are truly optional.

shemnon · 2019-05-16T15:44:54Z

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

+    }
+
+    // If we have an explicit list of peers, drop each peer from our discovery table
+    affectedPeers.ifPresent(peers -> peers.forEach(this::dropPeer));


If they are affected why are we summarily dropping them instead of re-evaluating? I'm confused.

The current interface just has a blanket rule where peers are fully allowed or fully disallowed. So if the permissions were updated to add restrictions and a list of peers were provided, it means those peers are no longer allowed. The follow-up PR will make these checks more granular such that we'll re-evaluate at this point.

shemnon · 2019-05-16T16:15:23Z

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

      return;
    }

    // Load the peer from the table, or use the instance that comes in.
    final Optional<DiscoveryPeer> maybeKnownPeer = peerTable.get(sender);
    final DiscoveryPeer peer = maybeKnownPeer.orElse(sender);
    final boolean peerKnown = maybeKnownPeer.isPresent();
-    final boolean peerBlacklisted = peerBlacklist.contains(peer);


Without the blacklist peers that are "bad" will still hit our peer table. Is this acceptable per what we need to do? Will we advertise that we've seen "blackbeard" even if we refuse to talk to him? Should we be tracking peer-no-grata? Or are we checking permissions at every time we would pull said peer out of the peer table?

The blacklist basically transformed into peerPermissions which gets checked in the method isPeerPermittedToSendMessages above on line 307. So we're still filtering out the bad peers here.

shemnon · 2019-05-16T16:18:53Z

...in/java/tech/pegasys/pantheon/ethereum/p2p/discovery/internal/RecursivePeerRefreshState.java

+
+  @FunctionalInterface
+  public interface OutboundDiscoveryMessagingPermissions {
+    boolean isPermitted(Peer remotePeer);


Maybe isRemotePermitted? In case we add this interface to an uber-permissions object with inbound permissions implicit on the local peer, so the names won't collide?

I've got a follow-up PR that will rework these interfaces. Right now I'm moving towards an API like: isPermitted(localNode, remotePeer, action).

shemnon · 2019-05-16T16:23:24Z

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

@@ -778,7 +761,6 @@ private void validate() {
      checkState(
          supportedCapabilities != null && supportedCapabilities.size() > 0,
          "Supported capabilities must be set and non-empty.");
-      checkState(peerBlacklist != null, "PeerBlacklist must be set.");


do we handle null peerPermissions correctly?

I think so - peerPermissions gets initialized with a default value, and the setter does a null check so that it can't get overwritten with a null value.

shemnon · 2019-05-16T16:26:50Z

ethereum/p2p/src/main/java/tech/pegasys/pantheon/ethereum/p2p/permissions/PeerPermissions.java

+  }
+
+  private static class CombinedPeerPermissions extends PeerPermissions {
+    private final List<PeerPermissions> permissions;


Consider ImmutableList. Since we are allowing a list to shortcut other CombinedPeerPermissions by collapsing it into this list we should make it explicit that the object we are shortcutting won't mutate. There is an ImmutableList collector as part of Guava.

shemnon · 2019-05-16T16:32:37Z

...p/src/main/java/tech/pegasys/pantheon/ethereum/p2p/permissions/PeerPermissionsBlacklist.java

+  private PeerPermissionsBlacklist(final int initialCapacity, final OptionalInt maxSize) {
+    if (maxSize.isPresent()) {
+      blacklist =
+          LimitedSet.create(initialCapacity, maxSize.getAsInt(), Mode.DROP_LEAST_RECENTLY_ACCESSED);


Consider Guava's Cache instead: https://github.com/google/guava/wiki/CachesExplained - it doesn't do Least Recently Accessed 100% but it does have better concurrency then being wrapped in a synchronized set.

Under the covers Java's sets are Maps with the key being the entry and I can't remember what the value is.

I may follow-up on this suggestion in another PR. But skipping for now.

shemnon · 2019-05-16T16:47:47Z

pantheon/src/main/java/tech/pegasys/pantheon/cli/PantheonCommand.java

+    }
+  }
+
+  private static void validateNodeIdString(final String nodeId) {


could this be part of ENodeURL?

shemnon · 2019-05-16T16:50:11Z

pantheon/src/test/java/tech/pegasys/pantheon/cli/PantheonCommandTest.java


    assertThat(commandOutput.toString()).isEmpty();
    assertThat(commandErrorOutput.toString()).isEmpty();
  }

+  @Test
+  public void callingWithBannedNodeidsOptionButNoValueMustDisplayErrorAndUsage() {


Can these be moved to 986? It makes the diff more comprehensible.

The bannedNodeIds test was sandwiched in between some bootnode tests, so moved things around a bit for a more coherent test layout 🤷‍♀

mbaxter added 14 commits May 14, 2019 15:19

Add LimitedSet utility

d818464

Add a minimal PeerPermissions interface

885389d

Break up PeerBlacklist into smaller components using new interface

5467e30

Dedupe identical functions

69c244d

Modify PeerPermissions callback to consume info on type of update

7dc7111

PeerPermissions update supports an optional list of affected peers

c252f00

Simplify permissions checks in discovery controller

3d8926d

Remove peer dropped event handling (dead code)

83c93e9

Add PeerDiscoveryController Builder

0b3b7f4

Be more selective when dropping peers from table

b95a78e

Rather than dropping peers from the peer table for any disconnect, drop peers from the table on permissions changes, when peers are requested to be disconnected, and when peers are disconnecting for failed permissions check.

Validate banned-node-ids as valid node ids

08ca7b3

Connect to peers that have not been recently contacted

6d63cd3

Fix PantheonCommandTest by using valid node id

650c05b

Merge branch 'master' into PAN-2614/add-peer-permissions-interface

0d6d45e

mbaxter commented May 14, 2019

View reviewed changes

mbaxter marked this pull request as ready for review May 14, 2019 22:28

mbaxter added 4 commits May 14, 2019 19:09

Check permissions at discovery layer considering directionality

ab94628

Rename interface for clarity

13033b2

Validate peers before initiating connection

0bfb168

Merge branch 'master' into PAN-2614/add-peer-permissions-interface

e1b0045

mbaxter commented May 15, 2019

View reviewed changes

mbaxter added 2 commits May 15, 2019 12:04

Clean up PeerPermissions utils

8740f74

Merge branch 'master' into PAN-2614/add-peer-permissions-interface

2f5daf7

shemnon approved these changes May 16, 2019

View reviewed changes

mbaxter added 4 commits May 17, 2019 10:48

Merge branch 'master' into PAN-2614/add-peer-permissions-interface

ff332f4

Use ImmutableList

69f8d0a

Move utility method to EnodeURL

e05836f

Merge branch 'master' into PAN-2614/add-peer-permissions-interface

01a3a1c

mbaxter merged commit c227d1b into PegaSysEng:master May 17, 2019

Provide feedback

Saved searches

Use saved searches to filter your results more quickly

[PAN-2614] Add simple PeerPermissions interface #1446

[PAN-2614] Add simple PeerPermissions interface #1446

mbaxter commented May 14, 2019 •

edited

Loading

mbaxter May 14, 2019

mbaxter May 14, 2019

mbaxter May 14, 2019

mbaxter May 14, 2019

mbaxter May 14, 2019

mbaxter May 14, 2019

mbaxter May 14, 2019

mbaxter May 15, 2019

shemnon left a comment

shemnon May 16, 2019

mbaxter May 17, 2019

shemnon May 16, 2019

mbaxter May 17, 2019

shemnon May 16, 2019

mbaxter May 17, 2019

shemnon May 16, 2019

mbaxter May 17, 2019

shemnon May 16, 2019

shemnon May 16, 2019

mbaxter May 17, 2019

shemnon May 16, 2019

shemnon May 16, 2019

mbaxter May 17, 2019


		doReturn(EvictResult.absent()).when(peerTableSpy).tryEvict(any());

		controller = getControllerBuilder().peerDroppedObservers(peerDroppedSubscribers).build();

[PAN-2614] Add simple PeerPermissions interface #1446

[PAN-2614] Add simple PeerPermissions interface #1446

Conversation

mbaxter commented May 14, 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

shemnon 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

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 14, 2019 •

edited

Loading