Eth/68: treat transaction size as scalar unsigned int (#5640)

Signed-off-by: Fabio Di Fabio <fabio.difabio@consensys.net>

CHANGELOG.md

@@ -9,6 +9,7 @@
 
 ### Bug Fixes
 - Use the node's configuration to determine if DNS enode URLs are allowed in calls to `admin_addPeer` and `admin_removePeer` [#5584](https://github.com/hyperledger/besu/pull/5584)
+- Align the implementation of Eth/68 `NewPooledTransactionHashes` to other clients, using unsigned int for encoding size. [#5640](https://github.com/hyperledger/besu/pull/5640)
 
 ### Download Links
 

ethereum/eth/src/main/java/org/hyperledger/besu/ethereum/eth/encoding/TransactionAnnouncementDecoder.java

@@ -26,6 +26,8 @@ import java.util.ArrayList;
 import java.util.List;
 import java.util.stream.Collectors;
 
+import org.apache.tuweni.bytes.Bytes;
+
 public class TransactionAnnouncementDecoder {
 
   @FunctionalInterface
@@ -65,7 +67,7 @@ public class TransactionAnnouncementDecoder {
    * Decode the list of transactions in the NewPooledTransactionHashesMessage
    *
    * @param input input used to decode the NewPooledTransactionHashesMessage after Eth/68
-   *     <p>format: [[type_0: B_1, type_1: B_1, ...], [size_0: B_4, size_1: B_4, ...], ...]
+   *     <p>format: [[type_0: B_1, type_1: B_1, ...], [size_0: P, size_1: P, ...], ...]
    * @return the list of TransactionAnnouncement decoded from the message with size, type and hash
    */
   private static List<TransactionAnnouncement> decodeForEth68(final RLPInput input) {
@@ -77,7 +79,22 @@ public class TransactionAnnouncementDecoder {
       types.add(b == 0 ? TransactionType.FRONTIER : TransactionType.of(b));
     }
 
-    final List<Long> sizes = input.readList(in -> (long) in.readBytes().toInt());
+    List<Long> sizes =
+        input.readList(
+            in -> {
+              // for backward compatibility with previous Besu implementation be lenient and support
+              // also unsigned int with leading zeros.
+              // ToDo: this could be replaced with the simpler `RLPInput::readUnsignedIntScalar`
+              // after some months it has been released, since most of the Besus
+              // will be using the new implementation.
+              final Bytes intBytes = in.readBytes();
+              if (intBytes.size() > 4) {
+                throw new RLPException(
+                    "Expected max 4 bytes for unsigned int, but got " + intBytes.size() + " bytes");
+              }
+              return intBytes.toLong();
+            });
+
     final List<Hash> hashes = input.readList(rlp -> Hash.wrap(rlp.readBytes32()));
     input.leaveList();
     if (!(types.size() == hashes.size() && hashes.size() == sizes.size())) {

ethereum/eth/src/main/java/org/hyperledger/besu/ethereum/eth/encoding/TransactionAnnouncementEncoder.java

@@ -69,7 +69,7 @@ public class TransactionAnnouncementEncoder {
   /**
    * Encode a list of transactions for the NewPooledTransactionHashesMessage using the Eth/68
    *
-   * <p>format: [[type_0: B_1, type_1: B_1, ...], [size_0: B_4, size_1: B_4, ...], ...]
+   * <p>format: [[type_0: B_1, type_1: B_1, ...], [size_0: P, size_1: P, ...], ...]
    *
    * @param transactions the list to encode
    * @return the encoded value. The message data will contain hashes, types and sizes.
@@ -112,7 +112,7 @@ public class TransactionAnnouncementEncoder {
     }
     out.startList();
     out.writeBytes(Bytes.wrap((types)));
-    out.writeList(sizes, (h, w) -> w.writeInt(h));
+    out.writeList(sizes, (h, w) -> w.writeUnsignedInt(h));
     out.writeList(hashes, (h, w) -> w.writeBytes(h));
     out.endList();
     return out.encoded();

ethereum/eth/src/test/java/org/hyperledger/besu/ethereum/eth/transactions/NewPooledTransactionHashesMessageProcessorTest.java

@@ -272,7 +272,7 @@ public class NewPooledTransactionHashesMessageProcessorTest {
   public void shouldEncodeTransactionsCorrectly_Eth68() {
 
     final String expected =
-        "0xf87983000102cf840000000184000000028400000003f863a00000000000000000000000000000000000000000000000000000000000000001a00000000000000000000000000000000000000000000000000000000000000002a00000000000000000000000000000000000000000000000000000000000000003";
+        "0xf86d83000102c3010203f863a00000000000000000000000000000000000000000000000000000000000000001a00000000000000000000000000000000000000000000000000000000000000002a00000000000000000000000000000000000000000000000000000000000000003";
     final List<Hash> hashes =
         List.of(
             Hash.fromHexString(
@@ -291,6 +291,50 @@ public class NewPooledTransactionHashesMessageProcessorTest {
 
   @Test
   public void shouldDecodeBytesCorrectly_Eth68() {
+    /*
+     * [
+     * "0x0000102"]
+     * ["0x01","0x02","0x03"],
+     * ["0x0000000000000000000000000000000000000000000000000000000000000001",
+     *  "0x0000000000000000000000000000000000000000000000000000000000000002",
+     *  "0x0000000000000000000000000000000000000000000000000000000000000003"]
+     * ]
+     */
+
+    final Bytes bytes =
+        Bytes.fromHexString(
+            "0xf86d83000102c3010203f863a00000000000000000000000000000000000000000000000000000000000000001a00000000000000000000000000000000000000000000000000000000000000002a00000000000000000000000000000000000000000000000000000000000000003");
+
+    final List<TransactionAnnouncement> announcementList =
+        getDecoder(EthProtocol.ETH68).decode(RLP.input(bytes));
+
+    final TransactionAnnouncement frontier = announcementList.get(0);
+    assertThat(frontier.getHash())
+        .isEqualTo(
+            Hash.fromHexString(
+                "0x0000000000000000000000000000000000000000000000000000000000000001"));
+    assertThat(frontier.getType()).hasValue(TransactionType.FRONTIER);
+    assertThat(frontier.getSize()).hasValue(1L);
+
+    final TransactionAnnouncement accessList = announcementList.get(1);
+    assertThat(accessList.getHash())
+        .isEqualTo(
+            Hash.fromHexString(
+                "0x0000000000000000000000000000000000000000000000000000000000000002"));
+    assertThat(accessList.getType()).hasValue(TransactionType.ACCESS_LIST);
+    assertThat(accessList.getSize()).hasValue(2L);
+
+    final TransactionAnnouncement eip1559 = announcementList.get(2);
+    assertThat(eip1559.getHash())
+        .isEqualTo(
+            Hash.fromHexString(
+                "0x0000000000000000000000000000000000000000000000000000000000000003"));
+    assertThat(eip1559.getType()).hasValue(TransactionType.EIP1559);
+    assertThat(eip1559.getSize()).hasValue(3L);
+  }
+
+  @Test
+  public void shouldDecodeBytesCorrectly_PreviousImplementations_Eth68() {
     /*
      * [
      * "0x0000102"]
@@ -453,9 +497,11 @@ public class NewPooledTransactionHashesMessageProcessorTest {
                 TransactionAnnouncementDecoder.getDecoder(EthProtocol.ETH68)
                     .decode(RLP.input(invalidMessageBytes)));
 
-    final String expectedMessage = "Value of size 5 has more than 4 bytes";
-    final String actualMessage = exception.getCause().getMessage();
-    assertThat(actualMessage).contains(expectedMessage);
+    final String expectedMessage = "Expected max 4 bytes for unsigned int, but got 5 bytes";
+    assertThat(exception)
+        .hasCauseInstanceOf(RLPException.class)
+        .cause()
+        .hasMessage(expectedMessage);
   }
 
   @Test

ethereum/rlp/src/main/java/org/hyperledger/besu/ethereum/rlp/AbstractRLPInput.java

@@ -317,6 +317,12 @@ abstract class AbstractRLPInput implements RLPInput {
     return res;
   }
 
+  @Override
+  public long readUnsignedIntScalar() {
+    checkScalar("unsigned int scalar", 4);
+    return readLongScalar();
+  }
+
   @Override
   public BigInteger readBigIntegerScalar() {
     checkScalar("arbitrary precision scalar");

ethereum/rlp/src/main/java/org/hyperledger/besu/ethereum/rlp/RLPInput.java

@@ -262,6 +262,15 @@ public interface RLPInput {
     return (readInt()) & 0xFFFFFFFFL;
   }
 
+  /**
+   * Reads a scalar from the input and return is as an unsigned int contained in a long
+   *
+   * @return The next scalar item of this input as an unsigned int value as long
+   * @throws RLPException if the next item to read is a list, the input is at the end of its current
+   *     list (and {@link #leaveList()} hasn't been called) or if the next item is either too big to
+   *     fit an unsigned int or has leading zeros.
+   */
+  long readUnsignedIntScalar();
   /**
    * Reads an inet address from this input.
    *

ethereum/rlp/src/test/java/org/hyperledger/besu/ethereum/rlp/BytesValueRLPInputTest.java

@@ -150,6 +150,25 @@ public class BytesValueRLPInputTest {
     assertThat(in.isDone()).isTrue();
   }
 
+  @Test
+  public void assertUnsignedIntScalar() {
+    // Scalar should be encoded as the minimal byte array representing the number. For 0, that means
+    // the empty byte array, which is a short element of zero-length, so 0x80.
+    assertUnsignedIntScalar(0L, h("0x80"));
+
+    assertUnsignedIntScalar(1L, h("0x01"));
+    assertUnsignedIntScalar(15L, h("0x0F"));
+    assertUnsignedIntScalar(1024L, h("0x820400"));
+    assertUnsignedIntScalar((1L << 32) - 1, h("0x84ffffffff"));
+  }
+
+  private void assertUnsignedIntScalar(final long expected, final Bytes toTest) {
+    final RLPInput in = RLP.input(toTest);
+    assertThat(in.isDone()).isFalse();
+    assertThat(in.readUnsignedIntScalar()).isEqualTo(expected);
+    assertThat(in.isDone()).isTrue();
+  }
+
   @Test
   public void assertLongScalar() {
     // Scalar should be encoded as the minimal byte array representing the number. For 0, that means

ethereum/rlp/src/test/java/org/hyperledger/besu/ethereum/rlp/BytesValueRLPOutputTest.java

@@ -161,6 +161,24 @@ public class BytesValueRLPOutputTest {
     assertThatThrownBy(() -> out.writeByte((byte) 1)).isInstanceOf(IllegalStateException.class);
   }
 
+  @Test
+  public void unsignedIntScalar() {
+    // Scalar should be encoded as the minimal byte array representing the number. For 0, that means
+    // the empty byte array, which is a short element of zero-length, so 0x80.
+    assertUnsignedIntScalar(h("0x80"), 0);
+
+    assertUnsignedIntScalar(h("0x01"), 1);
+    assertUnsignedIntScalar(h("0x0F"), 15);
+    assertUnsignedIntScalar(h("0x820400"), 1024);
+    assertUnsignedIntScalar(h("0x84ffffffff"), (1L << 32) - 1);
+  }
+
+  private void assertUnsignedIntScalar(final Bytes expected, final long toTest) {
+    final BytesValueRLPOutput out = new BytesValueRLPOutput();
+    out.writeUnsignedInt(toTest);
+    assertThat(out.encoded()).isEqualTo(expected);
+  }
+
   @Test
   public void longScalar() {
     // Scalar should be encoded as the minimal byte array representing the number. For 0, that means