Disregard Malformed From Fields and Use Sender Endpoints for Pongs (#1236)

* add hive test

Signed-off-by: Ratan Rai Sur <ratan.r.sur@gmail.com>

* garbage endpoint in peerdisccontrollertest

Signed-off-by: Ratan Rai Sur <ratan.r.sur@gmail.com>

* rlp deserialization test

Signed-off-by: Ratan Rai Sur <ratan.r.sur@gmail.com>

* make things work with optional

Signed-off-by: Ratan Rai Sur <ratan.r.sur@gmail.com>

* PeerDiscoveryPacketPcapSedesTest changes from Optional getFrom

Signed-off-by: Ratan Rai Sur <ratan.r.sur@gmail.com>

* rlp deserialization test

Signed-off-by: Ratan Rai Sur <ratan.r.sur@gmail.com>

* allow empty from field in ping

Signed-off-by: Ratan Rai Sur <ratan.r.sur@gmail.com>

* comment

Signed-off-by: Ratan Rai Sur <ratan.r.sur@gmail.com>

* logging

Signed-off-by: Ratan Rai Sur <ratan.r.sur@gmail.com>

* cleanup rebase

Signed-off-by: Ratan Rai Sur <ratan.r.sur@gmail.com>

* scaffolding

Signed-off-by: Ratan Rai Sur <ratan.r.sur@gmail.com>

* pass hive

Signed-off-by: Ratan Rai Sur <ratan.r.sur@gmail.com>

* remove some stuff

Signed-off-by: Ratan Rai Sur <ratan.r.sur@gmail.com>

* comment

Signed-off-by: Ratan Rai Sur <ratan.r.sur@gmail.com>

* leave list properly

Signed-off-by: Ratan Rai Sur <ratan.r.sur@gmail.com>

* spotless

Signed-off-by: Ratan Rai Sur <ratan.r.sur@gmail.com>

* rename port -> udpPort

Signed-off-by: Ratan Rai Sur <ratan.r.sur@gmail.com>

* move comment

Signed-off-by: Ratan Rai Sur <ratan.r.sur@gmail.com>

* fix documentation bug

Signed-off-by: Ratan Rai Sur <ratan.r.sur@gmail.com>

* remove redundant supression

Signed-off-by: Ratan Rai Sur <ratan.r.sur@gmail.com>

* more accurate tests

Signed-off-by: Ratan Rai Sur <ratan.r.sur@gmail.com>

* empty commit to trigger build

Signed-off-by: Ratan Rai Sur <ratan.r.sur@gmail.com>

ethereum/p2p/src/main/java/org/hyperledger/besu/ethereum/p2p/discovery/Endpoint.java

@@ -24,6 +24,7 @@ import org.hyperledger.besu.util.NetworkUtility;
 
 import java.net.InetAddress;
 import java.util.Objects;
+import java.util.Optional;
 import java.util.OptionalInt;
 
 import com.google.common.net.InetAddresses;
@@ -38,15 +39,25 @@ public class Endpoint {
   private final int udpPort;
   private final OptionalInt tcpPort;
 
+  private static class IllegalPortException extends IllegalArgumentException {
+    IllegalPortException(final String message) {
+      super(message);
+    }
+  }
+
+  private static void checkPort(final int port, final String portTypeName) {
+    if (!NetworkUtility.isValidPort(port)) {
+      throw new IllegalPortException(
+          String.format(
+              "%s port requires a value between 1 and 65535. Got %d.", portTypeName, port));
+    }
+  }
+
   public Endpoint(final String host, final int udpPort, final OptionalInt tcpPort) {
     checkArgument(
         host != null && InetAddresses.isInetAddress(host), "host requires a valid IP address");
-    checkArgument(
-        NetworkUtility.isValidPort(udpPort), "UDP port requires a value between 1 and 65535");
-    tcpPort.ifPresent(
-        p ->
-            checkArgument(
-                NetworkUtility.isValidPort(p), "TCP port requires a value between 1 and 65535"));
+    checkPort(udpPort, "UDP");
+    tcpPort.ifPresent(port -> checkPort(port, "TCP"));
 
     this.host = host;
     this.udpPort = udpPort;
@@ -54,10 +65,13 @@ public class Endpoint {
   }
 
   public static Endpoint fromEnode(final EnodeURL enode) {
-    checkArgument(
-        enode.getDiscoveryPort().isPresent(),
-        "Attempt to create a discovery endpoint for an enode with discovery disabled.");
-    final int discoveryPort = enode.getDiscoveryPort().getAsInt();
+    final int discoveryPort =
+        enode
+            .getDiscoveryPort()
+            .orElseThrow(
+                () ->
+                    new IllegalArgumentException(
+                        "Attempt to create a discovery endpoint for an enode with discovery disabled."));
     final OptionalInt listeningPort = enode.getListeningPort();
     return new Endpoint(enode.getIp().getHostAddress(), discoveryPort, listeningPort);
   }
@@ -144,12 +158,8 @@ public class Endpoint {
    */
   public void encodeInline(final RLPOutput out) {
     out.writeInetAddress(InetAddresses.forString(host));
-    out.writeUnsignedShort(udpPort);
-    if (tcpPort.isPresent()) {
-      out.writeUnsignedShort(tcpPort.getAsInt());
-    } else {
-      out.writeNull();
-    }
+    out.writeIntScalar(udpPort);
+    tcpPort.ifPresentOrElse(out::writeIntScalar, out::writeNull);
   }
 
   /**
@@ -164,11 +174,11 @@ public class Endpoint {
     checkGuard(
         fieldCount == 2 || fieldCount == 3,
         PeerDiscoveryPacketDecodingException::new,
-        "Invalid number of components in RLP representation of an endpoint: expected 2 o 3 elements but got %s",
+        "Invalid number of components in RLP representation of an endpoint: expected 2 or 3 elements but got %s",
         fieldCount);
 
     final InetAddress addr = in.readInetAddress();
-    final int udpPort = in.readUnsignedShort();
+    final int udpPort = in.readIntScalar();
 
     // Some mainnet packets have been shown to either not have the TCP port field at all,
     // or to have an RLP NULL value for it.
@@ -177,7 +187,7 @@ public class Endpoint {
       if (in.nextIsNull()) {
         in.skipNext();
       } else {
-        tcpPort = OptionalInt.of(in.readUnsignedShort());
+        tcpPort = OptionalInt.of(in.readIntScalar());
       }
     }
     return new Endpoint(addr.getHostAddress(), udpPort, tcpPort);
@@ -195,4 +205,20 @@ public class Endpoint {
     in.leaveList();
     return endpoint;
   }
+
+  /**
+   * Attempts to decodes the RLP stream as a standalone Endpoint instance, which is not part of a
+   * Peer. If the from field is malformed, consumes the rest of the items in the list and returns
+   * Optional.empty().
+   *
+   * @param in The RLP input stream from which to read.
+   * @return Some decoded endpoint if it was possible to decode it, empty otherwise.
+   */
+  public static Optional<Endpoint> maybeDecodeStandalone(final RLPInput in) {
+    try {
+      return Optional.of(decodeStandalone(in));
+    } catch (IllegalPortException __) {
+      return Optional.empty();
+    }
+  }
 }

ethereum/p2p/src/main/java/org/hyperledger/besu/ethereum/p2p/discovery/PeerDiscoveryAgent.java

@@ -182,24 +182,29 @@ public abstract class PeerDiscoveryAgent {
   }
 
   protected void handleIncomingPacket(final Endpoint sourceEndpoint, final Packet packet) {
-    OptionalInt tcpPort = OptionalInt.empty();
-    if (packet.getPacketData(PingPacketData.class).isPresent()) {
-      final PingPacketData ping = packet.getPacketData(PingPacketData.class).orElseGet(null);
-      if (ping != null && ping.getFrom() != null && ping.getFrom().getTcpPort().isPresent()) {
-        tcpPort = ping.getFrom().getTcpPort();
-      }
-    }
+    final int udpPort = sourceEndpoint.getUdpPort();
+    final int tcpPort =
+        packet
+            .getPacketData(PingPacketData.class)
+            .flatMap(PingPacketData::getFrom)
+            .flatMap(
+                fromEndpoint -> {
+                  final OptionalInt maybePort = fromEndpoint.getTcpPort();
+                  return maybePort.isPresent()
+                      ? Optional.of(maybePort.getAsInt())
+                      : Optional.empty();
+                })
+            .orElse(udpPort);
 
     // Notify the peer controller.
-    String host = sourceEndpoint.getHost();
-    int port = sourceEndpoint.getUdpPort();
+    final String host = sourceEndpoint.getHost();
     final DiscoveryPeer peer =
         DiscoveryPeer.fromEnode(
             EnodeURL.builder()
                 .nodeId(packet.getNodeId())
                 .ipAddress(host)
-                .listeningPort(tcpPort.orElse(port))
-                .discoveryPort(port)
+                .listeningPort(tcpPort)
+                .discoveryPort(udpPort)
                 .build());
 
     controller.ifPresent(c -> c.onMessage(packet, peer));

ethereum/p2p/src/main/java/org/hyperledger/besu/ethereum/p2p/discovery/internal/FindNeighborsPacketData.java

@@ -27,7 +27,7 @@ public class FindNeighborsPacketData implements PacketData {
   /* Node ID. */
   private final Bytes target;
 
-  /* In millis after epoch. */
+  /* In seconds after epoch. */
   private final long expiration;
 
   private FindNeighborsPacketData(final Bytes target, final long expiration) {

ethereum/p2p/src/main/java/org/hyperledger/besu/ethereum/p2p/discovery/internal/NeighborsPacketData.java

@@ -26,7 +26,7 @@ public class NeighborsPacketData implements PacketData {
 
   private final List<DiscoveryPeer> peers;
 
-  /* In millis after epoch. */
+  /* In seconds after epoch. */
   private final long expiration;
 
   private NeighborsPacketData(final List<DiscoveryPeer> peers, final long expiration) {
@@ -37,7 +37,6 @@ public class NeighborsPacketData implements PacketData {
     this.expiration = expiration;
   }
 
-  @SuppressWarnings("unchecked")
   public static NeighborsPacketData create(final List<DiscoveryPeer> peers) {
     return new NeighborsPacketData(peers, PacketData.defaultExpiration());
   }

ethereum/p2p/src/main/java/org/hyperledger/besu/ethereum/p2p/discovery/internal/PeerDiscoveryController.java

@@ -541,7 +541,9 @@ public class PeerDiscoveryController {
     if (packetData.getExpiration() < Instant.now().getEpochSecond()) {
       return;
     }
-    final PongPacketData data = PongPacketData.create(packetData.getFrom(), pingHash);
+    // We don't care about the `from` field of the ping, we pong to the `sender`
+    final PongPacketData data = PongPacketData.create(sender.getEndpoint(), pingHash);
+
     sendPacket(sender, PacketType.PONG, data);
   }
 

ethereum/p2p/src/main/java/org/hyperledger/besu/ethereum/p2p/discovery/internal/PingPacketData.java

@@ -20,26 +20,28 @@ import org.hyperledger.besu.ethereum.p2p.discovery.Endpoint;
 import org.hyperledger.besu.ethereum.rlp.RLPInput;
 import org.hyperledger.besu.ethereum.rlp.RLPOutput;
 
+import java.util.Optional;
+
 public class PingPacketData implements PacketData {
 
   /* Fixed value that represents we're using v5 of the P2P discovery protocol. */
   private static final int VERSION = 5;
 
-  /* Source. */
-  private final Endpoint from;
+  /* Source. If the field is garbage this is empty and we might need to recover it another way. From our bonded peers, for example. */
+  private final Optional<Endpoint> maybeFrom;
 
   /* Destination. */
   private final Endpoint to;
 
-  /* In millis after epoch. */
+  /* In seconds after epoch. */
   private final long expiration;
 
-  private PingPacketData(final Endpoint from, final Endpoint to, final long expiration) {
-    checkArgument(from != null, "source endpoint cannot be null");
+  private PingPacketData(
+      final Optional<Endpoint> maybeFrom, final Endpoint to, final long expiration) {
     checkArgument(to != null, "destination endpoint cannot be null");
     checkArgument(expiration >= 0, "expiration cannot be negative");
 
-    this.from = from;
+    this.maybeFrom = maybeFrom;
     this.to = to;
     this.expiration = expiration;
   }
@@ -49,14 +51,14 @@ public class PingPacketData implements PacketData {
   }
 
   static PingPacketData create(final Endpoint from, final Endpoint to, final long expirationSec) {
-    return new PingPacketData(from, to, expirationSec);
+    return new PingPacketData(Optional.of(from), to, expirationSec);
   }
 
   public static PingPacketData readFrom(final RLPInput in) {
     in.enterList();
     // The first element signifies the "version", but this value is ignored as of EIP-8
     in.readBigIntegerScalar();
-    final Endpoint from = Endpoint.decodeStandalone(in);
+    final Optional<Endpoint> from = Endpoint.maybeDecodeStandalone(in);
     final Endpoint to = Endpoint.decodeStandalone(in);
     final long expiration = in.readLongScalar();
     in.leaveListLenient();
@@ -67,14 +69,19 @@ public class PingPacketData implements PacketData {
   public void writeTo(final RLPOutput out) {
     out.startList();
     out.writeIntScalar(VERSION);
-    from.encodeStandalone(out);
+    maybeFrom
+        .orElseThrow(
+            () ->
+                new IllegalStateException(
+                    "Attempting to serialize invalid PING packet. Missing 'from' field"))
+        .encodeStandalone(out);
     to.encodeStandalone(out);
     out.writeLongScalar(expiration);
     out.endList();
   }
 
-  public Endpoint getFrom() {
-    return from;
+  public Optional<Endpoint> getFrom() {
+    return maybeFrom;
   }
 
   public Endpoint getTo() {
@@ -87,6 +94,13 @@ public class PingPacketData implements PacketData {
 
   @Override
   public String toString() {
-    return "PingPacketData{" + "from=" + from + ", to=" + to + ", expiration=" + expiration + '}';
+    return "PingPacketData{"
+        + "from="
+        + maybeFrom.map(Object::toString).orElse("INVALID")
+        + ", to="
+        + to
+        + ", expiration="
+        + expiration
+        + '}';
   }
 }

ethereum/p2p/src/main/java/org/hyperledger/besu/ethereum/p2p/discovery/internal/PongPacketData.java

@@ -28,7 +28,7 @@ public class PongPacketData implements PacketData {
   /* Hash of the PING packet. */
   private final Bytes pingHash;
 
-  /* In millis after epoch. */
+  /* In seconds after epoch. */
   private final long expiration;
 
   private PongPacketData(final Endpoint to, final Bytes pingHash, final long expiration) {

ethereum/p2p/src/test/java/org/hyperledger/besu/ethereum/p2p/discovery/PeerDiscoveryBootstrappingTest.java

@@ -59,7 +59,7 @@ public class PeerDiscoveryBootstrappingTest {
     final PingPacketData pingData = pingPacket.getPacketData(PingPacketData.class).get();
     assertThat(pingData.getExpiration())
         .isGreaterThanOrEqualTo(System.currentTimeMillis() / 1000 - 10000);
-    assertThat(pingData.getFrom()).isEqualTo(agent.getAdvertisedPeer().get().getEndpoint());
+    assertThat(pingData.getFrom()).contains(agent.getAdvertisedPeer().get().getEndpoint());
     assertThat(pingData.getTo()).isEqualTo(testAgent.getAdvertisedPeer().get().getEndpoint());
   }
 

ethereum/p2p/src/test/java/org/hyperledger/besu/ethereum/p2p/discovery/PeerDiscoveryPacketPcapSedesTest.java

@@ -14,7 +14,6 @@
  */
 package org.hyperledger.besu.ethereum.p2p.discovery;
 
-import static com.google.common.net.InetAddresses.isInetAddress;
 import static org.assertj.core.api.Assertions.assertThat;
 
 import org.hyperledger.besu.ethereum.p2p.discovery.internal.FindNeighborsPacketData;
@@ -30,9 +29,11 @@ import java.util.ArrayList;
 import java.util.Collection;
 import java.util.List;
 
+import com.google.common.net.InetAddresses;
 import io.pkts.Pcap;
 import io.pkts.protocol.Protocol;
 import io.vertx.core.buffer.Buffer;
+import org.assertj.core.api.Condition;
 import org.junit.Test;
 import org.junit.runner.RunWith;
 import org.junit.runners.Parameterized;
@@ -83,12 +84,13 @@ public class PeerDiscoveryPacketPcapSedesTest {
 
         assertThat(ping.getTo()).isNotNull();
         assertThat(ping.getFrom()).isNotNull();
-        assertThat(isInetAddress(ping.getTo().getHost())).isTrue();
-        assertThat(isInetAddress(ping.getFrom().getHost())).isTrue();
+        assertThat(ping.getTo().getHost()).satisfies(validInetAddressCondition);
+        assertThat(ping.getFrom().map(Endpoint::getHost))
+            .hasValueSatisfying(validInetAddressCondition);
         assertThat(ping.getTo().getUdpPort()).isPositive();
-        assertThat(ping.getFrom().getUdpPort()).isPositive();
+        assertThat(ping.getFrom().get().getUdpPort()).isPositive();
         ping.getTo().getTcpPort().ifPresent(p -> assertThat(p).isPositive());
-        ping.getFrom().getTcpPort().ifPresent(p -> assertThat(p).isPositive());
+        ping.getFrom().get().getTcpPort().ifPresent(p -> assertThat(p).isPositive());
         assertThat(ping.getExpiration()).isPositive();
         // because of the version upgrade the ping packet won't re-serialize, so we're done
         return;
@@ -98,7 +100,7 @@ public class PeerDiscoveryPacketPcapSedesTest {
         final PongPacketData pong = packet.getPacketData(PongPacketData.class).get();
 
         assertThat(pong.getTo()).isNotNull();
-        assertThat(isInetAddress(pong.getTo().getHost())).isTrue();
+        assertThat(pong.getTo().getHost()).satisfies(validInetAddressCondition);
         assertThat(pong.getTo().getUdpPort()).isPositive();
         pong.getTo().getTcpPort().ifPresent(p -> assertThat(p).isPositive());
         assertThat(pong.getPingHash().toArray()).hasSize(32);
@@ -118,12 +120,12 @@ public class PeerDiscoveryPacketPcapSedesTest {
       case NEIGHBORS:
         assertThat(packet.getPacketData(NeighborsPacketData.class)).isPresent();
         final NeighborsPacketData neighbors = packet.getPacketData(NeighborsPacketData.class).get();
-        assertThat(neighbors.getExpiration()).isGreaterThan(0);
+        assertThat(neighbors.getExpiration()).isPositive();
         assertThat(neighbors.getNodes()).isNotEmpty();
 
         for (final DiscoveryPeer p : neighbors.getNodes()) {
           assertThat(NetworkUtility.isValidPort(p.getEndpoint().getUdpPort())).isTrue();
-          assertThat(isInetAddress(p.getEndpoint().getHost())).isTrue();
+          assertThat(p.getEndpoint().getHost()).satisfies(validInetAddressCondition);
           assertThat(p.getId().toArray()).hasSize(64);
         }
 
@@ -133,4 +135,7 @@ public class PeerDiscoveryPacketPcapSedesTest {
     final byte[] encoded = packet.encode().getBytes();
     assertThat(encoded).isEqualTo(data);
   }
+
+  private final Condition<String> validInetAddressCondition =
+      new Condition<>(InetAddresses::isInetAddress, "checks for valid InetAddresses");
 }

ethereum/p2p/src/test/java/org/hyperledger/besu/ethereum/p2p/discovery/internal/PingPacketDataTest.java

@@ -38,7 +38,7 @@ public class PingPacketDataTest {
     final Bytes serialized = RLP.encode(packet::writeTo);
     final PingPacketData deserialized = PingPacketData.readFrom(RLP.input(serialized));
 
-    assertThat(deserialized.getFrom()).isEqualTo(from);
+    assertThat(deserialized.getFrom()).contains(from);
     assertThat(deserialized.getTo()).isEqualTo(to);
     assertThat(deserialized.getExpiration()).isGreaterThan(currentTimeSec);
   }
@@ -61,7 +61,7 @@ public class PingPacketDataTest {
     final Bytes serialized = out.encoded();
     final PingPacketData deserialized = PingPacketData.readFrom(RLP.input(serialized));
 
-    assertThat(deserialized.getFrom()).isEqualTo(from);
+    assertThat(deserialized.getFrom()).contains(from);
     assertThat(deserialized.getTo()).isEqualTo(to);
     assertThat(deserialized.getExpiration()).isEqualTo(time);
   }
@@ -86,7 +86,7 @@ public class PingPacketDataTest {
     final Bytes serialized = out.encoded();
     final PingPacketData deserialized = PingPacketData.readFrom(RLP.input(serialized));
 
-    assertThat(deserialized.getFrom()).isEqualTo(from);
+    assertThat(deserialized.getFrom()).contains(from);
     assertThat(deserialized.getTo()).isEqualTo(to);
     assertThat(deserialized.getExpiration()).isEqualTo(time);
   }
@@ -109,7 +109,30 @@ public class PingPacketDataTest {
     final Bytes serialized = out.encoded();
     final PingPacketData deserialized = PingPacketData.readFrom(RLP.input(serialized));
 
-    assertThat(deserialized.getFrom()).isEqualTo(from);
+    assertThat(deserialized.getFrom()).contains(from);
+    assertThat(deserialized.getTo()).isEqualTo(to);
+    assertThat(deserialized.getExpiration()).isEqualTo(time);
+  }
+
+  @Test
+  public void readFrom_lowPortValues() {
+    final int version = 4;
+    final Endpoint from = new Endpoint("0.1.2.1", 1, OptionalInt.of(1));
+    final Endpoint to = new Endpoint("127.0.0.2", 30303, OptionalInt.empty());
+    final long time = System.currentTimeMillis();
+
+    final BytesValueRLPOutput out = new BytesValueRLPOutput();
+    out.startList();
+    out.writeIntScalar(version);
+    from.encodeStandalone(out);
+    to.encodeStandalone(out);
+    out.writeLongScalar(time);
+    out.endList();
+
+    final Bytes serialized = out.encoded();
+    final PingPacketData deserialized = PingPacketData.readFrom(RLP.input(serialized));
+
+    assertThat(deserialized.getFrom()).contains(from);
     assertThat(deserialized.getTo()).isEqualTo(to);
     assertThat(deserialized.getExpiration()).isEqualTo(time);
   }