Add an UNKNOWN DisconnectReason representing disconnects with no reas…

…on (#359)

ethereum/p2p/src/main/java/tech/pegasys/pantheon/ethereum/p2p/netty/ApiHandler.java

@@ -74,7 +74,7 @@ protected void channelRead0(final ChannelHandlerContext ctx, final MessageData o
           break;
         case WireMessageCodes.DISCONNECT:
           final DisconnectMessage disconnect = DisconnectMessage.readFrom(message);
-          DisconnectReason reason = null;
+          DisconnectReason reason = DisconnectReason.UNKNOWN;
           try {
             reason = disconnect.getReason();
             LOG.debug(

ethereum/p2p/src/main/java/tech/pegasys/pantheon/ethereum/p2p/netty/NettyP2PNetwork.java

@@ -241,7 +241,7 @@ protected void initChannel(final SocketChannel ch) {
               }
               // Reject incoming connections that are blacklisted
               if (peerBlacklist.contains(connection)) {
-                connection.disconnect(DisconnectReason.USELESS_PEER);
+                connection.disconnect(DisconnectReason.UNKNOWN);
                 return;
               }
 

ethereum/p2p/src/main/java/tech/pegasys/pantheon/ethereum/p2p/wire/messages/DisconnectMessage.java

@@ -12,18 +12,15 @@
  */
 package tech.pegasys.pantheon.ethereum.p2p.wire.messages;
 
-import static com.google.common.base.Preconditions.checkArgument;
-import static tech.pegasys.pantheon.util.Preconditions.checkGuard;
-
 import tech.pegasys.pantheon.ethereum.p2p.api.MessageData;
 import tech.pegasys.pantheon.ethereum.p2p.wire.AbstractMessageData;
-import tech.pegasys.pantheon.ethereum.p2p.wire.WireProtocolException;
 import tech.pegasys.pantheon.ethereum.rlp.BytesValueRLPOutput;
 import tech.pegasys.pantheon.ethereum.rlp.RLP;
 import tech.pegasys.pantheon.ethereum.rlp.RLPInput;
 import tech.pegasys.pantheon.ethereum.rlp.RLPOutput;
 import tech.pegasys.pantheon.util.bytes.BytesValue;
 
+import java.util.Optional;
 import java.util.stream.Stream;
 
 public final class DisconnectMessage extends AbstractMessageData {
@@ -75,16 +72,21 @@ public Data(final DisconnectReason reason) {
 
     public void writeTo(final RLPOutput out) {
       out.startList();
-      out.writeByte(reason.getValue());
+      out.writeBytesValue(reason.getValue());
       out.endList();
     }
 
     public static Data readFrom(final RLPInput in) {
-      final int size = in.enterList();
-      checkGuard(size == 1, WireProtocolException::new, "Expected list size 1, got: %s", size);
-      final DisconnectReason reason = DisconnectReason.forCode(in.readByte());
+      in.enterList();
+      BytesValue reasonData = in.readBytesValue();
       in.leaveList();
 
+      // Disconnect reason should be at most 1 byte, otherwise, just return UNKNOWN
+      final DisconnectReason reason =
+          reasonData.size() == 1
+              ? DisconnectReason.forCode(reasonData.get(0))
+              : DisconnectReason.UNKNOWN;
+
       return new Data(reason);
     }
 
@@ -100,6 +102,7 @@ public DisconnectReason getReason() {
    *     Protocol</a>
    */
   public enum DisconnectReason {
+    UNKNOWN(null),
     REQUESTED((byte) 0x00),
     TCP_SUBSYSTEM_ERROR((byte) 0x01),
     BREACH_OF_PROTOCOL((byte) 0x02),
@@ -115,29 +118,40 @@ public enum DisconnectReason {
     SUBPROTOCOL_TRIGGERED((byte) 0x10);
 
     private static final DisconnectReason[] BY_ID;
-    private final byte code;
+    private final Optional<Byte> code;
 
     static {
       final int maxValue =
-          Stream.of(DisconnectReason.values()).mapToInt(dr -> (int) dr.getValue()).max().getAsInt();
+          Stream.of(DisconnectReason.values())
+              .filter(r -> r.code.isPresent())
+              .mapToInt(r -> (int) r.code.get())
+              .max()
+              .getAsInt();
       BY_ID = new DisconnectReason[maxValue + 1];
-      Stream.of(DisconnectReason.values()).forEach(dr -> BY_ID[dr.getValue()] = dr);
+      Stream.of(DisconnectReason.values())
+          .filter(r -> r.code.isPresent())
+          .forEach(r -> BY_ID[r.code.get()] = r);
     }
 
-    public static DisconnectReason forCode(final byte taintedCode) {
-      final byte code = (byte) (taintedCode & 0xff);
-      checkArgument(code < BY_ID.length, "unrecognized disconnect reason");
-      final DisconnectReason reason = BY_ID[code];
-      checkArgument(reason != null, "unrecognized disconnect reason");
-      return reason;
+    public static DisconnectReason forCode(final Byte code) {
+      if (code == null || code >= BY_ID.length || code < 0 || BY_ID[code] == null) {
+        // Be permissive and just return unknown if the disconnect reason is bad
+        return UNKNOWN;
+      }
+      return BY_ID[code];
+    }
+
+    DisconnectReason(final Byte code) {
+      this.code = Optional.ofNullable(code);
     }
 
-    DisconnectReason(final byte code) {
-      this.code = code;
+    public BytesValue getValue() {
+      return code.map(BytesValue::of).orElse(BytesValue.EMPTY);
     }
 
-    public byte getValue() {
-      return code;
+    @Override
+    public String toString() {
+      return getValue().toString() + " " + name();
     }
   }
 }

ethereum/p2p/src/test/java/tech/pegasys/pantheon/ethereum/p2p/NettyP2PNetworkTest.java

@@ -378,7 +378,7 @@ public void rejectIncomingConnectionFromBlacklistedPeer() throws Exception {
       assertThat(connectFuture.get(5L, TimeUnit.SECONDS).getPeer().getNodeId()).isEqualTo(localId);
       assertThat(peerFuture.get(5L, TimeUnit.SECONDS).getPeer().getNodeId()).isEqualTo(localId);
       assertThat(reasonFuture.get(5L, TimeUnit.SECONDS))
-          .isEqualByComparingTo(DisconnectReason.USELESS_PEER);
+          .isEqualByComparingTo(DisconnectReason.UNKNOWN);
     }
   }
 

ethereum/p2p/src/test/java/tech/pegasys/pantheon/ethereum/p2p/wire/messages/DisconnectMessageTest.java

@@ -0,0 +1,91 @@
+/*
+ * Copyright 2018 ConsenSys AG.
+ *
+ * Licensed under the Apache License, Version 2.0 (the "License"); you may not use this file except in compliance with
+ * the License. You may obtain a copy of the License at
+ *
+ * http://www.apache.org/licenses/LICENSE-2.0
+ *
+ * Unless required by applicable law or agreed to in writing, software distributed under the License is distributed on
+ * an "AS IS" BASIS, WITHOUT WARRANTIES OR CONDITIONS OF ANY KIND, either express or implied. See the License for the
+ * specific language governing permissions and limitations under the License.
+ */
+package tech.pegasys.pantheon.ethereum.p2p.wire.messages;
+
+import static org.assertj.core.api.Assertions.assertThat;
+import static org.assertj.core.api.AssertionsForClassTypes.assertThatThrownBy;
+
+import tech.pegasys.pantheon.ethereum.p2p.api.MessageData;
+import tech.pegasys.pantheon.ethereum.p2p.wire.RawMessage;
+import tech.pegasys.pantheon.ethereum.p2p.wire.messages.DisconnectMessage.DisconnectReason;
+import tech.pegasys.pantheon.util.bytes.BytesValue;
+
+import org.junit.Test;
+
+public class DisconnectMessageTest {
+
+  @Test
+  public void readFromWithReason() {
+    MessageData messageData =
+        new RawMessage(WireMessageCodes.DISCONNECT, BytesValue.fromHexString("0xC103"));
+    DisconnectMessage disconnectMessage = DisconnectMessage.readFrom(messageData);
+
+    DisconnectReason reason = disconnectMessage.getReason();
+    assertThat(reason).isEqualTo(DisconnectReason.USELESS_PEER);
+  }
+
+  @Test
+  public void readFromWithNoReason() {
+    MessageData messageData =
+        new RawMessage(WireMessageCodes.DISCONNECT, BytesValue.fromHexString("0xC180"));
+    DisconnectMessage disconnectMessage = DisconnectMessage.readFrom(messageData);
+
+    DisconnectReason reason = disconnectMessage.getReason();
+    assertThat(reason).isEqualTo(DisconnectReason.UNKNOWN);
+  }
+
+  @Test
+  public void readFromWithInvalidReason() {
+    String[] invalidReasons = {
+      "0xC10C",
+      "0xC155",
+      // List containing a byte > 128 (negative valued)
+      "0xC281FF",
+      // List containing a multi-byte reason
+      "0xC3820101"
+    };
+
+    for (String invalidReason : invalidReasons) {
+      MessageData messageData =
+          new RawMessage(WireMessageCodes.DISCONNECT, BytesValue.fromHexString(invalidReason));
+      DisconnectMessage disconnectMessage = DisconnectMessage.readFrom(messageData);
+      DisconnectReason reason = disconnectMessage.getReason();
+      assertThat(reason).isEqualTo(DisconnectReason.UNKNOWN);
+    }
+  }
+
+  @Test
+  public void readFromWithWrongMessageType() {
+    MessageData messageData =
+        new RawMessage(WireMessageCodes.PING, BytesValue.fromHexString("0xC103"));
+    assertThatThrownBy(() -> DisconnectMessage.readFrom(messageData))
+        .isInstanceOf(IllegalArgumentException.class)
+        .hasMessageContaining("Message has code 2 and thus is not a DisconnectMessage");
+  }
+
+  @Test
+  public void createWithReason() {
+    DisconnectMessage disconnectMessage = DisconnectMessage.create(DisconnectReason.USELESS_PEER);
+
+    assertThat(disconnectMessage.getReason()).isEqualTo(DisconnectReason.USELESS_PEER);
+    assertThat(disconnectMessage.getData().toString()).isEqualToIgnoringCase("0xC103");
+  }
+
+  @Test
+  public void createWithUnknownReason() {
+    DisconnectMessage disconnectMessage = DisconnectMessage.create(DisconnectReason.UNKNOWN);
+
+    assertThat(disconnectMessage.getReason()).isEqualTo(DisconnectReason.UNKNOWN);
+    assertThat(disconnectMessage.getData().toString()).isEqualToIgnoringCase("0xC180");
+  }
+}