2496 ping pong encoding error (hyperledger#2575)

* change enr sequence field to be a long scalar instead of open ended byte array
* handles missing source addresses

Signed-off-by: Justin Florentine <justin.florentine@consensys.net>

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

@@ -14,7 +14,6 @@
  */
 package org.hyperledger.besu.ethereum.p2p.discovery;
 
-import static com.google.common.base.Preconditions.checkArgument;
 import static org.hyperledger.besu.util.NetworkUtility.checkPort;
 import static org.hyperledger.besu.util.Preconditions.checkGuard;
 
@@ -36,17 +35,15 @@
  * used in various Discovery messages.
  */
 public class Endpoint {
-  private final String host;
+  private final Optional<String> host;
   private final int udpPort;
   private final Optional<Integer> tcpPort;
 
   public Endpoint(final String host, final int udpPort, final Optional<Integer> tcpPort) {
-    checkArgument(
-        host != null && InetAddresses.isInetAddress(host), "host requires a valid IP address");
     checkPort(udpPort, "UDP");
     tcpPort.ifPresent(port -> checkPort(port, "TCP"));
 
-    this.host = host;
+    this.host = Optional.ofNullable(host);
     this.udpPort = udpPort;
     this.tcpPort = tcpPort;
   }
@@ -66,14 +63,14 @@ public static Endpoint fromEnode(final EnodeURL enode) {
   public EnodeURL toEnode(final Bytes nodeId) {
     return EnodeURLImpl.builder()
         .nodeId(nodeId)
-        .ipAddress(host)
+        .ipAddress(host.orElse(""))
         .listeningPort(tcpPort.orElse(udpPort))
         .discoveryPort(udpPort)
         .build();
   }
 
   public String getHost() {
-    return host;
+    return host.orElse("");
   }
 
   public int getUdpPort() {
@@ -144,7 +141,16 @@ public void encodeStandalone(final RLPOutput out) {
    * @param out The RLP output stream.
    */
   public void encodeInline(final RLPOutput out) {
-    out.writeInetAddress(InetAddresses.forString(host));
+    if (host.isPresent()) {
+      InetAddress hostAddress = InetAddresses.forString(host.get());
+      if (hostAddress != null) {
+        out.writeInetAddress(hostAddress);
+      } else { // present, but not a parseable IP address
+        out.writeNull();
+      }
+    } else {
+      out.writeNull();
+    }
     out.writeIntScalar(udpPort);
     tcpPort.ifPresentOrElse(out::writeIntScalar, out::writeNull);
   }
@@ -164,7 +170,13 @@ public static Endpoint decodeInline(final RLPInput in, final int fieldCount) {
         "Invalid number of components in RLP representation of an endpoint: expected 2 or 3 elements but got %s",
         fieldCount);
 
-    final InetAddress addr = in.readInetAddress();
+    String hostAddress = null;
+    if (!in.nextIsNull()) {
+      InetAddress addr = in.readInetAddress();
+      hostAddress = addr.getHostAddress();
+    } else {
+      in.skipNext();
+    }
     final int udpPort = in.readIntScalar();
 
     // Some mainnet packets have been shown to either not have the TCP port field at all,
@@ -177,7 +189,7 @@ public static Endpoint decodeInline(final RLPInput in, final int fieldCount) {
         tcpPort = Optional.of(in.readIntScalar());
       }
     }
-    return new Endpoint(addr.getHostAddress(), udpPort, tcpPort);
+    return new Endpoint(hostAddress, udpPort, tcpPort);
   }
 
   /**

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

@@ -232,7 +232,10 @@ private void handlePacket(final DatagramPacket datagram) {
             handleIncomingPacket(endpoint, event.result());
           } else {
             if (event.cause() instanceof PeerDiscoveryPacketDecodingException) {
-              LOG.debug("Discarding invalid peer discovery packet: {}", event.cause().getMessage());
+              LOG.debug(
+                  "Discarding invalid peer discovery packet: {}, {}",
+                  event.cause().getMessage(),
+                  event.cause());
             } else {
               LOG.error("Encountered error while handling packet", event.cause());
             }

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

@@ -0,0 +1,22 @@
+/*
+ * Copyright 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.
+ *
+ * SPDX-License-Identifier: Apache-2.0
+ */
+
+package org.hyperledger.besu.ethereum.p2p.discovery.internal;
+
+public class DevP2PException extends RuntimeException {
+  public DevP2PException(final String reason) {
+    super(reason);
+  }
+}

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

@@ -467,7 +467,7 @@ void bond(final DiscoveryPeer peer) {
         interaction -> {
           final PingPacketData data =
               PingPacketData.create(
-                  localPeer.getEndpoint(),
+                  Optional.of(localPeer.getEndpoint()),
                   peer.getEndpoint(),
                   localPeer.getNodeRecord().map(NodeRecord::getSeq).orElse(null));
           createPacket(
@@ -561,6 +561,7 @@ private void dispatchInteraction(final Peer peer, final PeerInteractionState sta
   private void respondToPing(
       final PingPacketData packetData, final Bytes pingHash, final DiscoveryPeer sender) {
     if (packetData.getExpiration() < Instant.now().getEpochSecond()) {
+      LOG.debug("ignoring expired PING");
       return;
     }
     // We don't care about the `from` field of the ping, we pong to the `sender`

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

@@ -24,6 +24,8 @@
 import java.util.Optional;
 
 import org.apache.tuweni.units.bigints.UInt64;
+import org.slf4j.Logger;
+import org.slf4j.LoggerFactory;
 
 public class PingPacketData implements PacketData {
 
@@ -41,6 +43,7 @@ public class PingPacketData implements PacketData {
 
   /* Current sequence number of the sending node’s record */
   private final UInt64 enrSeq;
+  private static final Logger LOG = LoggerFactory.getLogger(PingPacketData.class);
 
   private PingPacketData(
       final Optional<Endpoint> maybeFrom,
@@ -56,40 +59,97 @@ private PingPacketData(
     this.enrSeq = enrSeq;
   }
 
-  public static PingPacketData create(final Endpoint from, final Endpoint to, final UInt64 enrSeq) {
+  public static PingPacketData create(
+      final Optional<Endpoint> from, final Endpoint to, final UInt64 enrSeq) {
     checkArgument(
         enrSeq != null && UInt64.ZERO.compareTo(enrSeq) < 0, "enrSeq cannot be null or negative");
     return create(from, to, PacketData.defaultExpiration(), enrSeq);
   }
 
   static PingPacketData create(
-      final Endpoint from, final Endpoint to, final long expirationSec, final UInt64 enrSeq) {
+      final Optional<Endpoint> from,
+      final Endpoint to,
+      final long expirationSec,
+      final UInt64 enrSeq) {
     checkArgument(
         enrSeq != null && UInt64.ZERO.compareTo(enrSeq) < 0, "enrSeq cannot be null or negative");
-    return new PingPacketData(Optional.of(from), to, expirationSec, enrSeq);
+    return new PingPacketData(from, to, expirationSec, enrSeq);
   }
 
   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 Optional<Endpoint> from = Endpoint.maybeDecodeStandalone(in);
-    final Endpoint to = Endpoint.decodeStandalone(in);
+    Optional<Endpoint> from = Optional.empty();
+    Optional<Endpoint> to = Optional.empty();
+    if (in.nextIsList()) {
+      to = Endpoint.maybeDecodeStandalone(in);
+      // https://github.com/ethereum/devp2p/blob/master/discv4.md#ping-packet-0x01
+      if (in.nextIsList()) { // if there are two, the first is the from address, next is the to
+        // address
+        from = to;
+        to = Endpoint.maybeDecodeStandalone(in);
+      }
+    } else {
+      throw new DevP2PException("missing address in ping packet");
+    }
     final long expiration = in.readLongScalar();
     UInt64 enrSeq = null;
     if (!in.isEndOfCurrentList()) {
       try {
-        enrSeq = UInt64.valueOf(in.readLongScalar());
+        enrSeq = UInt64.valueOf(in.readBigIntegerScalar());
+        LOG.debug("read PING enr as long scalar");
       } catch (MalformedRLPInputException malformed) {
+        LOG.debug("failed to read PING enr as scalar, trying to read bytes instead");
         enrSeq = UInt64.fromBytes(in.readBytes());
       }
     }
     in.leaveListLenient();
+    return new PingPacketData(from, to.get(), expiration, enrSeq);
+  }
+
+  /**
+   * Used by test classes to read legacy encodes of Pings which used a byte array for the ENR field
+   *
+   * @deprecated Only to be used by internal tests to confirm backward compatibility.
+   * @param in input stream to read from
+   * @return PingPacketData parsed from input, using legacy encode.
+   */
+  @Deprecated
+  public static PingPacketData legacyReadFrom(final RLPInput in) { // only for testing, do not use
+    in.enterList();
+    // The first element signifies the "version", but this value is ignored as of EIP-8
+    in.readBigIntegerScalar();
+    final Optional<Endpoint> from = Endpoint.maybeDecodeStandalone(in);
+    final Endpoint to = Endpoint.decodeStandalone(in);
+    final long expiration = in.readLongScalar();
+    UInt64 enrSeq = null;
+    if (!in.isEndOfCurrentList()) {
+      enrSeq = UInt64.fromBytes(in.readBytes());
+    }
+    in.leaveListLenient();
     return new PingPacketData(from, to, expiration, enrSeq);
   }
 
   @Override
   public void writeTo(final RLPOutput out) {
+    out.startList();
+    out.writeIntScalar(VERSION);
+    if (maybeFrom.isPresent()) {
+      maybeFrom.get().encodeStandalone(out);
+    }
+    to.encodeStandalone(out);
+    out.writeLongScalar(expiration);
+    out.writeBigIntegerScalar(enrSeq.toBigInteger());
+    out.endList();
+  }
+
+  /**
+   * @deprecated Only to be used by internal tests to confirm backward compatibility.
+   * @param out stream to write to
+   */
+  @Deprecated
+  public void legacyWriteTo(final RLPOutput out) {
     out.startList();
     out.writeIntScalar(VERSION);
     maybeFrom
@@ -100,7 +160,13 @@ public void writeTo(final RLPOutput out) {
         .encodeStandalone(out);
     to.encodeStandalone(out);
     out.writeLongScalar(expiration);
-    out.writeLongScalar(enrSeq.toLong());
+    out.writeBytes(
+        getEnrSeq()
+            .orElseThrow(
+                () ->
+                    new IllegalStateException(
+                        "Attempting to serialize invalid PING packet. Missing 'enrSeq' field"))
+            .toBytes());
     out.endList();
   }
 

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

@@ -23,6 +23,8 @@
 
 import org.apache.tuweni.bytes.Bytes;
 import org.apache.tuweni.units.bigints.UInt64;
+import org.slf4j.Logger;
+import org.slf4j.LoggerFactory;
 
 public class PongPacketData implements PacketData {
 
@@ -37,6 +39,7 @@ public class PongPacketData implements PacketData {
 
   /* Current sequence number of the sending node’s record */
   private final UInt64 enrSeq;
+  private static final Logger LOG = LoggerFactory.getLogger(PongPacketData.class);
 
   private PongPacketData(
       final Endpoint to, final Bytes pingHash, final long expiration, final UInt64 enrSeq) {
@@ -59,8 +62,10 @@ public static PongPacketData readFrom(final RLPInput in) {
     UInt64 enrSeq = null;
     if (!in.isEndOfCurrentList()) {
       try {
-        enrSeq = UInt64.valueOf(in.readLongScalar());
+        enrSeq = UInt64.valueOf(in.readBigIntegerScalar());
+        LOG.debug("read PONG enr from scalar");
       } catch (MalformedRLPInputException malformed) {
+        LOG.debug("failed to read PONG enr from scalar, trying as byte array");
         enrSeq = UInt64.fromBytes(in.readBytes());
       }
     }
@@ -74,7 +79,48 @@ public void writeTo(final RLPOutput out) {
     to.encodeStandalone(out);
     out.writeBytes(pingHash);
     out.writeLongScalar(expiration);
-    out.writeLongScalar(enrSeq.toLong());
+    out.writeBigIntegerScalar(enrSeq.toBigInteger());
+    out.endList();
+  }
+
+  /**
+   * Used by test classes to read legacy encodes of Pongs which used a byte array for the ENR field
+   *
+   * @deprecated Only to be used by internal tests to confirm backward compatibility.
+   * @param in input stream being read from
+   * @return PongPacketData parsed from input, using legacy encode
+   */
+  @Deprecated
+  public static PongPacketData legacyReadFrom(final RLPInput in) {
+    in.enterList();
+    final Endpoint to = Endpoint.decodeStandalone(in);
+    final Bytes hash = in.readBytes();
+    final long expiration = in.readLongScalar();
+    UInt64 enrSeq = null;
+    if (!in.isEndOfCurrentList()) {
+      enrSeq = UInt64.fromBytes(in.readBytes());
+    }
+    in.leaveListLenient();
+    return new PongPacketData(to, hash, expiration, enrSeq);
+  }
+
+  /**
+   * @deprecated Only to be used by internal tests to confirm backward compatibility.
+   * @param out output stream being written to
+   */
+  @Deprecated
+  public void legacyWriteTo(final RLPOutput out) {
+    out.startList();
+    to.encodeStandalone(out);
+    out.writeBytes(pingHash);
+    out.writeLongScalar(expiration);
+    out.writeBytes(
+        getEnrSeq()
+            .orElseThrow(
+                () ->
+                    new IllegalStateException(
+                        "Attempting to serialize invalid PONG packet. Missing 'enrSeq' field"))
+            .toBytes());
     out.endList();
   }
 

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

@@ -97,7 +97,7 @@ public Packet createPingPacket(
     return Packet.create(
         PacketType.PING,
         PingPacketData.create(
-            fromAgent.getAdvertisedPeer().get().getEndpoint(),
+            Optional.of(fromAgent.getAdvertisedPeer().get().getEndpoint()),
             toAgent.getAdvertisedPeer().get().getEndpoint(),
             UInt64.ONE),
         fromAgent.getNodeKey());