Handle case where peers advertise a listening port of 0 (#1391)

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

ethereum/p2p/src/main/java/tech/pegasys/pantheon/ethereum/p2p/network/netty/DeFramer.java

@@ -168,12 +168,17 @@ final class DeFramer extends ByteToMessageDecoder {
   }
 
   private Peer createPeer(final PeerInfo peerInfo, final ChannelHandlerContext ctx) {
-    InetSocketAddress remoteAddress = ((InetSocketAddress) ctx.channel().remoteAddress());
+    final InetSocketAddress remoteAddress = ((InetSocketAddress) ctx.channel().remoteAddress());
+    int port = peerInfo.getPort();
+    if (port == 0) {
+      // Most peers advertise a port of "0", just set a default best guess in this case
+      port = EnodeURL.DEFAULT_LISTENING_PORT;
+    }
     return DefaultPeer.fromEnodeURL(
         EnodeURL.builder()
             .nodeId(peerInfo.getNodeId())
             .ipAddress(remoteAddress.getAddress())
-            .listeningPort(peerInfo.getPort())
+            .listeningPort(port)
             .build());
   }
 

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

@@ -27,7 +27,7 @@ public interface Peer extends PeerId {
    * @return The generated peer ID.
    */
   static BytesValue randomId() {
-    final byte[] id = new byte[64];
+    final byte[] id = new byte[EnodeURL.NODE_ID_SIZE];
     SecureRandomProvider.publicSecureRandom().nextBytes(id);
     return BytesValue.wrap(id);
   }

ethereum/p2p/src/main/java/tech/pegasys/pantheon/ethereum/p2p/wire/PeerInfo.java

@@ -77,6 +77,12 @@ public class PeerInfo {
     return capabilities;
   }
 
+  /**
+   * This value is meant to represent the port at which a peer is listening for connections.
+   * However, most peers actually advertise a port of "0" so this value is not reliable.
+   *
+   * @return (Unreliable) The tcp port on which the peer is listening for connections
+   */
   public int getPort() {
     return port;
   }

ethereum/p2p/src/test/java/tech/pegasys/pantheon/ethereum/p2p/network/netty/DeFramerTest.java

@@ -172,6 +172,59 @@ public class DeFramerTest {
     assertThat(out.size()).isEqualTo(1);
   }
 
+  @Test
+  public void decode_handlesHelloFromPeerWithAdvertisedPortOf0()
+      throws ExecutionException, InterruptedException {
+    ChannelFuture future = NettyMocks.channelFuture(false);
+    when(channel.closeFuture()).thenReturn(future);
+
+    final Peer peer = createRemotePeer();
+    final PeerInfo remotePeerInfo =
+        new PeerInfo(
+            peerInfo.getVersion(),
+            peerInfo.getClientId(),
+            peerInfo.getCapabilities(),
+            0,
+            peer.getId());
+    final DeFramer deFramer = createDeFramer(null);
+
+    HelloMessage helloMessage = HelloMessage.create(remotePeerInfo);
+    ByteBuf data = Unpooled.wrappedBuffer(helloMessage.getData().extractArray());
+    when(framer.deframe(eq(data)))
+        .thenReturn(new RawMessage(helloMessage.getCode(), helloMessage.getData()))
+        .thenReturn(null);
+    List<Object> out = new ArrayList<>();
+    deFramer.decode(ctx, data, out);
+
+    assertThat(connectFuture).isDone();
+    assertThat(connectFuture).isNotCompletedExceptionally();
+    PeerConnection peerConnection = connectFuture.get();
+    assertThat(peerConnection.getPeerInfo()).isEqualTo(remotePeerInfo);
+    assertThat(out).isEmpty();
+
+    final EnodeURL expectedEnode =
+        EnodeURL.builder()
+            .ipAddress(remoteAddress.getAddress())
+            .nodeId(peer.getId())
+            // Listening port should be replaced with default port
+            .listeningPort(EnodeURL.DEFAULT_LISTENING_PORT)
+            .build();
+    assertThat(peerConnection.getPeer().getEnodeURL()).isEqualTo(expectedEnode);
+
+    // Next phase of pipeline should be setup
+    verify(pipeline, times(1)).addLast(any());
+
+    // Next message should be pushed out
+    PingMessage nextMessage = PingMessage.get();
+    ByteBuf nextData = Unpooled.wrappedBuffer(nextMessage.getData().extractArray());
+    when(framer.deframe(eq(nextData)))
+        .thenReturn(new RawMessage(nextMessage.getCode(), nextMessage.getData()))
+        .thenReturn(null);
+    verify(pipeline, times(1)).addLast(any());
+    deFramer.decode(ctx, nextData, out);
+    assertThat(out.size()).isEqualTo(1);
+  }
+
   @Test
   public void decode_handlesUnexpectedPeerId() {
     ChannelFuture future = NettyMocks.channelFuture(false);

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

@@ -57,7 +57,6 @@ public interface DefaultCommandValues {
   SyncMode DEFAULT_SYNC_MODE = SyncMode.FULL;
   int FAST_SYNC_MAX_WAIT_TIME = 0;
   int FAST_SYNC_MIN_PEER_COUNT = 5;
-  int P2P_PORT = 30303;
   int DEFAULT_MAX_PEERS = 25;
 
   static Path getDefaultPantheonDataPath(final Object command) {

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

@@ -240,7 +240,7 @@ public class PantheonCommand implements DefaultCommandValues, Runnable {
       paramLabel = MANDATORY_PORT_FORMAT_HELP,
       description = "Port on which to listen for p2p communication (default: ${DEFAULT-VALUE})",
       arity = "1")
-  private final Integer p2pPort = P2P_PORT;
+  private final Integer p2pPort = EnodeURL.DEFAULT_LISTENING_PORT;
 
   @Option(
       names = {"--network-id"},

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

@@ -29,7 +29,8 @@ import com.google.common.primitives.Ints;
 
 public class EnodeURL {
 
-  private static final int NODE_ID_SIZE = 64;
+  public static final int DEFAULT_LISTENING_PORT = 30303;
+  public static final int NODE_ID_SIZE = 64;
   private static final Pattern DISCPORT_QUERY_STRING_REGEX =
       Pattern.compile("^discport=([0-9]{1,5})$");
   private static final Pattern NODE_ID_PATTERN = Pattern.compile("^[0-9a-fA-F]{128}$");