Ban Peers via CLI (#254)

* Ban Peers via CLI

As part of working on #251 I needed to be able to ban certain nodes from my
connection pool and let others connect.  This is a general solution to add
a --banned-nodeids CLI flag where the nodeIds of banned nodes are listed.


Signed-off-by: Adrian Sutton <adrian.sutton@consensys.net>

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

@@ -22,6 +22,7 @@ import tech.pegasys.pantheon.controller.PantheonController;
 import tech.pegasys.pantheon.ethereum.eth.sync.SynchronizerConfiguration.Builder;
 
 import java.io.IOException;
+import java.util.Collections;
 import java.util.HashMap;
 import java.util.Map;
 import java.util.concurrent.ExecutorService;
@@ -76,7 +77,8 @@ public class ThreadPantheonNodeRunner implements PantheonNodeRunner {
                 25,
                 node.jsonRpcConfiguration(),
                 node.webSocketConfiguration(),
-                node.homeDirectory());
+                node.homeDirectory(),
+                Collections.emptySet());
 
     nodeExecutor.submit(runner::execute);
 

ethereum/p2p/src/main/java/tech/pegasys/pantheon/ethereum/p2p/peers/PeerBlacklist.java

@@ -25,6 +25,19 @@ import java.util.Set;
 
 import com.google.common.collect.ImmutableSet;
 
+/**
+ * A list of nodes that the running client will not communicate with. This can be because of network
+ * issues, protocol issues, or by being explicitly set on the command line.
+ *
+ * <p>Peers are stored and identified strictly by their nodeId, the convenience methods taking
+ * {@link Peer}s and {@link PeerConnection}s redirect to the methods that take {@link BytesValue}
+ * object that represent the node ID of the banned nodes.
+ *
+ * <p>The storage list is not infinite. A default cap of 500 is applied and nodes are removed on a
+ * first added first removed basis. Adding a new copy of the same node will not affect the priority
+ * for removal. The exception to this is a list of banned nodes passed in by reference to the
+ * constructor. This list neither adds nor removes from that list passed in.
+ */
 public class PeerBlacklist implements DisconnectCallback {
   private static final int DEFAULT_BLACKLIST_CAP = 500;
 
@@ -46,16 +59,28 @@ public class PeerBlacklist implements DisconnectCallback {
                 }
               }));
 
-  public PeerBlacklist(final int blacklistCap) {
+  /** These nodes are always banned for the life of this list. They are not subject to rollover. */
+  private final Set<BytesValue> bannedNodeIds;
+
+  public PeerBlacklist(final int blacklistCap, final Set<BytesValue> bannedNodeIds) {
     this.blacklistCap = blacklistCap;
+    this.bannedNodeIds = bannedNodeIds;
+  }
+
+  public PeerBlacklist(final int blacklistCap) {
+    this(blacklistCap, Collections.emptySet());
+  }
+
+  public PeerBlacklist(final Set<BytesValue> bannedNodeIds) {
+    this(DEFAULT_BLACKLIST_CAP, bannedNodeIds);
   }
 
   public PeerBlacklist() {
-    this(DEFAULT_BLACKLIST_CAP);
+    this(DEFAULT_BLACKLIST_CAP, Collections.emptySet());
   }
 
   private boolean contains(final BytesValue nodeId) {
-    return blacklistedNodeIds.contains(nodeId);
+    return blacklistedNodeIds.contains(nodeId) || bannedNodeIds.contains(nodeId);
   }
 
   public boolean contains(final PeerConnection peer) {
@@ -70,7 +95,7 @@ public class PeerBlacklist implements DisconnectCallback {
     add(peer.getId());
   }
 
-  private void add(final BytesValue peerId) {
+  public void add(final BytesValue peerId) {
     blacklistedNodeIds.add(peerId);
   }
 

ethereum/p2p/src/test/java/tech/pegasys/pantheon/ethereum/p2p/peers/PeerBlacklistTest.java

@@ -21,13 +21,80 @@ import tech.pegasys.pantheon.ethereum.p2p.wire.PeerInfo;
 import tech.pegasys.pantheon.ethereum.p2p.wire.messages.DisconnectMessage.DisconnectReason;
 import tech.pegasys.pantheon.util.bytes.BytesValue;
 
+import java.util.Collections;
+
 import org.junit.Test;
 
 public class PeerBlacklistTest {
   private int nodeIdValue = 1;
 
   @Test
-  public void doesNotBlacklistPeerForNormalDisconnect() throws Exception {
+  public void directlyAddingPeerWorks() {
+    final PeerBlacklist blacklist = new PeerBlacklist();
+    final Peer peer = generatePeer();
+
+    assertThat(blacklist.contains(peer)).isFalse();
+
+    blacklist.add(peer);
+
+    assertThat(blacklist.contains(peer)).isTrue();
+  }
+
+  @Test
+  public void directlyAddingPeerByPeerIdWorks() {
+    final PeerBlacklist blacklist = new PeerBlacklist();
+    final Peer peer = generatePeer();
+
+    assertThat(blacklist.contains(peer)).isFalse();
+
+    blacklist.add(peer.getId());
+
+    assertThat(blacklist.contains(peer)).isTrue();
+  }
+
+  @Test
+  public void banningPeerByPeerIdWorks() {
+    final Peer peer = generatePeer();
+    final PeerBlacklist blacklist = new PeerBlacklist(Collections.singleton(peer.getId()));
+
+    assertThat(blacklist.contains(peer)).isTrue();
+
+    blacklist.add(peer.getId());
+
+    assertThat(blacklist.contains(peer)).isTrue();
+  }
+
+  @Test
+  public void bannedNodesDoNotRollover() {
+    final Peer bannedPeer = generatePeer();
+    final Peer peer1 = generatePeer();
+    final Peer peer2 = generatePeer();
+    final Peer peer3 = generatePeer();
+    final PeerBlacklist blacklist = new PeerBlacklist(2, Collections.singleton(bannedPeer.getId()));
+
+    assertThat(blacklist.contains(bannedPeer)).isTrue();
+    assertThat(blacklist.contains(peer1)).isFalse();
+    assertThat(blacklist.contains(peer2)).isFalse();
+    assertThat(blacklist.contains(peer3)).isFalse();
+
+    // fill to the limit
+    blacklist.add(peer1.getId());
+    blacklist.add(peer2.getId());
+    assertThat(blacklist.contains(bannedPeer)).isTrue();
+    assertThat(blacklist.contains(peer1)).isTrue();
+    assertThat(blacklist.contains(peer2)).isTrue();
+    assertThat(blacklist.contains(peer3)).isFalse();
+
+    // trigger rollover
+    blacklist.add(peer3.getId());
+    assertThat(blacklist.contains(bannedPeer)).isTrue();
+    assertThat(blacklist.contains(peer1)).isFalse();
+    assertThat(blacklist.contains(peer2)).isTrue();
+    assertThat(blacklist.contains(peer3)).isTrue();
+  }
+
+  @Test
+  public void doesNotBlacklistPeerForNormalDisconnect() {
     final PeerBlacklist blacklist = new PeerBlacklist();
     final PeerConnection peer = generatePeerConnection();
 
@@ -39,7 +106,7 @@ public class PeerBlacklistTest {
   }
 
   @Test
-  public void blacklistPeerForBadBehavior() throws Exception {
+  public void blacklistPeerForBadBehavior() {
 
     final PeerBlacklist blacklist = new PeerBlacklist();
     final PeerConnection peer = generatePeerConnection();
@@ -52,7 +119,7 @@ public class PeerBlacklistTest {
   }
 
   @Test
-  public void doesNotBlacklistPeerForOurBadBehavior() throws Exception {
+  public void doesNotBlacklistPeerForOurBadBehavior() {
     final PeerBlacklist blacklist = new PeerBlacklist();
     final PeerConnection peer = generatePeerConnection();
 
@@ -64,7 +131,7 @@ public class PeerBlacklistTest {
   }
 
   @Test
-  public void blacklistIncompatiblePeer() throws Exception {
+  public void blacklistIncompatiblePeer() {
     final PeerBlacklist blacklist = new PeerBlacklist();
     final PeerConnection peer = generatePeerConnection();
 
@@ -76,7 +143,7 @@ public class PeerBlacklistTest {
   }
 
   @Test
-  public void blacklistIncompatiblePeerWhoIssuesDisconnect() throws Exception {
+  public void blacklistIncompatiblePeerWhoIssuesDisconnect() {
     final PeerBlacklist blacklist = new PeerBlacklist();
     final PeerConnection peer = generatePeerConnection();
 
@@ -88,7 +155,7 @@ public class PeerBlacklistTest {
   }
 
   @Test
-  public void capsSizeOfList() throws Exception {
+  public void capsSizeOfList() {
 
     final PeerBlacklist blacklist = new PeerBlacklist(2);
     final PeerConnection peer1 = generatePeerConnection();
@@ -136,4 +203,10 @@ public class PeerBlacklistTest {
 
     return peer;
   }
+
+  private Peer generatePeer() {
+    final byte[] id = new byte[64];
+    id[0] = (byte) nodeIdValue++;
+    return new DefaultPeer(BytesValue.wrap(id), "10.9.8.7", 65535, 65534);
+  }
 }

pantheon/src/main/java/tech/pegasys/pantheon/RunnerBuilder.java

@@ -54,6 +54,7 @@ import tech.pegasys.pantheon.ethereum.p2p.netty.NettyP2PNetwork;
 import tech.pegasys.pantheon.ethereum.p2p.peers.PeerBlacklist;
 import tech.pegasys.pantheon.ethereum.p2p.wire.Capability;
 import tech.pegasys.pantheon.ethereum.p2p.wire.SubProtocol;
+import tech.pegasys.pantheon.util.bytes.BytesValue;
 
 import java.nio.file.Path;
 import java.util.Collection;
@@ -78,7 +79,8 @@ public class RunnerBuilder {
       final int maxPeers,
       final JsonRpcConfiguration jsonRpcConfiguration,
       final WebSocketConfiguration webSocketConfiguration,
-      final Path dataDir) {
+      final Path dataDir,
+      final Collection<String> bannedNodeIds) {
 
     Preconditions.checkNotNull(pantheonController);
 
@@ -122,6 +124,10 @@ public class RunnerBuilder {
             .setClientId(PantheonInfo.version())
             .setSupportedProtocols(subProtocols);
 
+    final PeerBlacklist peerBlacklist =
+        new PeerBlacklist(
+            bannedNodeIds.stream().map(BytesValue::fromHexString).collect(Collectors.toSet()));
+
     final NetworkRunner networkRunner =
         NetworkRunner.builder()
             .protocolManagers(protocolManagers)
@@ -134,7 +140,7 @@ public class RunnerBuilder {
                         networkConfig,
                         caps,
                         PeerRequirement.aggregateOf(protocolManagers),
-                        new PeerBlacklist()))
+                        peerBlacklist))
             .build();
 
     final Synchronizer synchronizer = pantheonController.getSynchronizer();

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

@@ -44,6 +44,7 @@ import java.nio.file.InvalidPathException;
 import java.nio.file.Path;
 import java.nio.file.Paths;
 import java.util.Collection;
+import java.util.Collections;
 import java.util.List;
 import java.util.Optional;
 import java.util.function.Function;
@@ -199,6 +200,14 @@ public class PantheonCommand implements Runnable {
   )
   private final Integer maxTrailingPeers = Integer.MAX_VALUE;
 
+  @Option(
+    names = {"--banned-nodeids"},
+    description = "A list of node IDs to ban from the p2p network.",
+    split = ",",
+    arity = "1..*"
+  )
+  private final Collection<String> bannedNodeIds = null;
+
   // TODO: Re-enable as per NC-1057/NC-1681
   //  @Option(
   //    names = {"--sync-mode"},
@@ -491,7 +500,8 @@ public class PantheonCommand implements Runnable {
             maxPeers,
             jsonRpcConfiguration,
             webSocketConfiguration,
-            dataDir);
+            dataDir,
+            bannedNodeIds == null ? Collections.emptySet() : bannedNodeIds);
 
     addShutdownHook(runner);
     runner.execute();

pantheon/src/test/java/tech/pegasys/pantheon/RunnerTest.java

@@ -127,7 +127,8 @@ public final class RunnerTest {
             3,
             aheadJsonRpcConfiguration,
             aheadWebSocketConfiguration,
-            dbAhead);
+            dbAhead,
+            Collections.emptySet());
     try {
 
       executorService.submit(runnerAhead::execute);
@@ -161,7 +162,8 @@ public final class RunnerTest {
               3,
               behindJsonRpcConfiguration,
               behindWebSocketConfiguration,
-              dbBehind);
+              dbBehind,
+              Collections.emptySet());
 
       executorService.submit(runnerBehind::execute);
       final Call.Factory client = new OkHttpClient();

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

@@ -93,6 +93,7 @@ public class PantheonCommandTest extends CommandTestAbstract {
             anyInt(),
             any(),
             any(),
+            any(),
             any()))
         .thenReturn(mockRunner);
   }
@@ -135,6 +136,7 @@ public class PantheonCommandTest extends CommandTestAbstract {
             eq(25),
             eq(defaultJsonRpcConfiguration),
             eq(defaultWebSocketConfiguration),
+            any(),
             any());
 
     final ArgumentCaptor<MiningParameters> miningArg =
@@ -262,6 +264,7 @@ public class PantheonCommandTest extends CommandTestAbstract {
             eq(42),
             eq(jsonRpcConfiguration),
             eq(webSocketConfiguration),
+            any(),
             any());
 
     final Collection<String> nodes =
@@ -315,6 +318,7 @@ public class PantheonCommandTest extends CommandTestAbstract {
             eq(25),
             eq(jsonRpcConfiguration),
             eq(webSocketConfiguration),
+            any(),
             any());
 
     verify(mockControllerBuilder).build(any(), any(), any(), eq(false), any(), eq(false));
@@ -368,7 +372,17 @@ public class PantheonCommandTest extends CommandTestAbstract {
 
     verify(mockRunnerBuilder)
         .build(
-            any(), any(), eq(false), any(), anyString(), anyInt(), anyInt(), any(), any(), any());
+            any(),
+            any(),
+            eq(false),
+            any(),
+            anyString(),
+            anyInt(),
+            anyInt(),
+            any(),
+            any(),
+            any(),
+            any());
 
     assertThat(commandOutput.toString()).isEmpty();
     assertThat(commandErrorOutput.toString()).isEmpty();
@@ -383,6 +397,15 @@ public class PantheonCommandTest extends CommandTestAbstract {
     assertThat(commandErrorOutput.toString()).startsWith(expectedErrorOutputStart);
   }
 
+  @Test
+  public void callingWithBannedNodeidsOptionButNoValueMustDisplayErrorAndUsage() {
+    parseCommand("--banned-nodeids");
+    assertThat(commandOutput.toString()).isEmpty();
+    final String expectedErrorOutputStart =
+        "Missing required parameter for option '--banned-nodeids' at index 0 (<bannedNodeIds>)";
+    assertThat(commandErrorOutput.toString()).startsWith(expectedErrorOutputStart);
+  }
+
   @Test
   public void bootnodesOptionMustBeUsed() {
     final String[] nodes = {"enode://001@123:4567", "enode://002@123:4567", "enode://003@123:4567"};
@@ -399,6 +422,7 @@ public class PantheonCommandTest extends CommandTestAbstract {
             anyInt(),
             any(),
             any(),
+            any(),
             any());
 
     assertThat(stringListArgumentCaptor.getValue().toArray()).isEqualTo(nodes);
@@ -407,6 +431,31 @@ public class PantheonCommandTest extends CommandTestAbstract {
     assertThat(commandErrorOutput.toString()).isEmpty();
   }
 
+  @Test
+  public void banNodeIdsOptionMustBeUsed() {
+    final String[] nodes = {"0001", "0002", "0003"};
+    parseCommand("--banned-nodeids", String.join(",", nodes));
+
+    verify(mockRunnerBuilder)
+        .build(
+            any(),
+            any(),
+            anyBoolean(),
+            any(),
+            anyString(),
+            anyInt(),
+            anyInt(),
+            any(),
+            any(),
+            any(),
+            stringListArgumentCaptor.capture());
+
+    assertThat(stringListArgumentCaptor.getValue().toArray()).isEqualTo(nodes);
+
+    assertThat(commandOutput.toString()).isEmpty();
+    assertThat(commandErrorOutput.toString()).isEmpty();
+  }
+
   @Test
   public void p2pHostAndPortOptionMustBeUsed() {
 
@@ -425,6 +474,7 @@ public class PantheonCommandTest extends CommandTestAbstract {
             anyInt(),
             any(),
             any(),
+            any(),
             any());
 
     assertThat(stringArgumentCaptor.getValue()).isEqualTo(host);
@@ -451,6 +501,7 @@ public class PantheonCommandTest extends CommandTestAbstract {
             intArgumentCaptor.capture(),
             any(),
             any(),
+            any(),
             any());
 
     assertThat(intArgumentCaptor.getValue()).isEqualTo(maxPeers);
@@ -497,6 +548,7 @@ public class PantheonCommandTest extends CommandTestAbstract {
             anyInt(),
             jsonRpcConfigArgumentCaptor.capture(),
             any(),
+            any(),
             any());
 
     assertThat(jsonRpcConfigArgumentCaptor.getValue().isEnabled()).isFalse();
@@ -520,6 +572,7 @@ public class PantheonCommandTest extends CommandTestAbstract {
             anyInt(),
             jsonRpcConfigArgumentCaptor.capture(),
             any(),
+            any(),
             any());
 
     assertThat(jsonRpcConfigArgumentCaptor.getValue().isEnabled()).isTrue();
@@ -543,6 +596,7 @@ public class PantheonCommandTest extends CommandTestAbstract {
             anyInt(),
             jsonRpcConfigArgumentCaptor.capture(),
             any(),
+            any(),
             any());
 
     assertThat(jsonRpcConfigArgumentCaptor.getValue().getRpcApis())
@@ -570,6 +624,7 @@ public class PantheonCommandTest extends CommandTestAbstract {
             anyInt(),
             jsonRpcConfigArgumentCaptor.capture(),
             any(),
+            any(),
             any());
 
     assertThat(jsonRpcConfigArgumentCaptor.getValue().getHost()).isEqualTo(host);
@@ -595,6 +650,7 @@ public class PantheonCommandTest extends CommandTestAbstract {
             anyInt(),
             jsonRpcConfigArgumentCaptor.capture(),
             any(),
+            any(),
             any());
 
     assertThat(jsonRpcConfigArgumentCaptor.getValue().getCorsAllowedDomains().toArray())
@@ -620,6 +676,7 @@ public class PantheonCommandTest extends CommandTestAbstract {
             anyInt(),
             jsonRpcConfigArgumentCaptor.capture(),
             any(),
+            any(),
             any());
 
     assertThat(jsonRpcConfigArgumentCaptor.getValue().getCorsAllowedDomains().toArray())
@@ -645,6 +702,7 @@ public class PantheonCommandTest extends CommandTestAbstract {
             anyInt(),
             jsonRpcConfigArgumentCaptor.capture(),
             any(),
+            any(),
             any());
 
     assertThat(jsonRpcConfigArgumentCaptor.getValue().getCorsAllowedDomains())
@@ -670,6 +728,7 @@ public class PantheonCommandTest extends CommandTestAbstract {
             anyInt(),
             jsonRpcConfigArgumentCaptor.capture(),
             any(),
+            any(),
             any());
 
     assertThat(jsonRpcConfigArgumentCaptor.getValue().getCorsAllowedDomains()).isEmpty();
@@ -741,6 +800,7 @@ public class PantheonCommandTest extends CommandTestAbstract {
             anyInt(),
             any(),
             wsRpcConfigArgumentCaptor.capture(),
+            any(),
             any());
 
     assertThat(wsRpcConfigArgumentCaptor.getValue().isEnabled()).isFalse();
@@ -764,6 +824,7 @@ public class PantheonCommandTest extends CommandTestAbstract {
             anyInt(),
             any(),
             wsRpcConfigArgumentCaptor.capture(),
+            any(),
             any());
 
     assertThat(wsRpcConfigArgumentCaptor.getValue().isEnabled()).isTrue();
@@ -787,6 +848,7 @@ public class PantheonCommandTest extends CommandTestAbstract {
             anyInt(),
             any(),
             wsRpcConfigArgumentCaptor.capture(),
+            any(),
             any());
 
     assertThat(wsRpcConfigArgumentCaptor.getValue().getRpcApis())
@@ -813,6 +875,7 @@ public class PantheonCommandTest extends CommandTestAbstract {
             anyInt(),
             any(),
             wsRpcConfigArgumentCaptor.capture(),
+            any(),
             any());
 
     assertThat(wsRpcConfigArgumentCaptor.getValue().getHost()).isEqualTo(host);