fix yParity flakey test (hyperledger#6151)

Fix the flakeiness in EthGetTransactionByHashTest as well as some other sonar identified cleanup. Signed-off-by: Danno Ferrin <[email protected]>
jflo · Nov 9, 2023 · 4b58d07 · 4b58d07
1 parent 9710a9a
commit 4b58d07
Show file tree

Hide file tree

Showing 4 changed files with 73 additions and 30 deletions.
diff --git a/...g/hyperledger/besu/ethereum/api/jsonrpc/internal/methods/EthGetTransactionByHashTest.java b/...g/hyperledger/besu/ethereum/api/jsonrpc/internal/methods/EthGetTransactionByHashTest.java
@@ -15,6 +15,7 @@
 package org.hyperledger.besu.ethereum.api.jsonrpc.internal.methods;
 
 import static org.assertj.core.api.Assertions.assertThat;
+import static org.junit.jupiter.api.Assertions.fail;
 import static org.mockito.Mockito.verifyNoInteractions;
 import static org.mockito.Mockito.verifyNoMoreInteractions;
 import static org.mockito.Mockito.when;
@@ -71,7 +72,8 @@ void returnsCorrectMethodName() {
 
   @Test
   void shouldReturnErrorResponseIfMissingRequiredParameter() {
-    final JsonRpcRequest request = new JsonRpcRequest("2.0", method.getName(), new Object[] {});
+    final JsonRpcRequest request =
+        new JsonRpcRequest(JSON_RPC_VERSION, method.getName(), new Object[] {});
     final JsonRpcRequestContext context = new JsonRpcRequestContext(request);
 
     final JsonRpcErrorResponse expectedResponse =
@@ -92,7 +94,7 @@ void shouldReturnNullResultWhenTransactionDoesNotExist() {
         .thenReturn(Optional.empty());
 
     final JsonRpcRequest request =
-        new JsonRpcRequest("2.0", method.getName(), new Object[] {transactionHash});
+        new JsonRpcRequest(JSON_RPC_VERSION, method.getName(), new Object[] {transactionHash});
     final JsonRpcRequestContext context = new JsonRpcRequestContext(request);
 
     final JsonRpcSuccessResponse expectedResponse =
@@ -115,7 +117,7 @@ void shouldReturnPendingTransactionWhenTransactionExistsAndIsPending() {
 
     final JsonRpcRequest request =
         new JsonRpcRequest(
-            "2.0", method.getName(), new Object[] {transaction.getHash().toHexString()});
+            JSON_RPC_VERSION, method.getName(), new Object[] {transaction.getHash().toHexString()});
     final JsonRpcRequestContext context = new JsonRpcRequestContext(request);
 
     final JsonRpcSuccessResponse expectedResponse =
@@ -141,7 +143,7 @@ void shouldReturnCompleteTransactionWhenTransactionExistsInBlockchain() {
 
     final JsonRpcRequest request =
         new JsonRpcRequest(
-            "2.0", method.getName(), new Object[] {transaction.getHash().toHexString()});
+            JSON_RPC_VERSION, method.getName(), new Object[] {transaction.getHash().toHexString()});
     final JsonRpcRequestContext context = new JsonRpcRequestContext(request);
 
     final JsonRpcSuccessResponse expectedResponse =
@@ -181,8 +183,23 @@ void validateResultSpec() {
     assertThat(result.getRaw()).isNotNull();
     assertThat(result.getTo()).isNotNull();
     assertThat(result.getValue()).isNotNull();
-    assertThat(result.getYParity()).isNotNull();
-    assertThat(result.getV()).isNotNull();
+    switch (result.getType()) {
+      case "0x0":
+        assertThat(result.getYParity()).isNull();
+        assertThat(result.getV()).isNotNull();
+        break;
+      case "0x1":
+      case "0x2":
+        assertThat(result.getYParity()).isNotNull();
+        assertThat(result.getV()).isNotNull();
+        break;
+      case "0x3":
+        assertThat(result.getYParity()).isNotNull();
+        assertThat(result.getV()).isNull();
+        break;
+      default:
+        fail("unknownType " + result.getType());
+    }
     assertThat(result.getR()).isNotNull();
     assertThat(result.getS()).isNotNull();
   }

diff --git a/...c/main/java/org/hyperledger/besu/ethereum/core/encoding/BlobPooledTransactionDecoder.java b/...c/main/java/org/hyperledger/besu/ethereum/core/encoding/BlobPooledTransactionDecoder.java
@@ -30,6 +30,10 @@
  */
 public class BlobPooledTransactionDecoder {
 
+  private BlobPooledTransactionDecoder() {
+    // no instances
+  }
+
   /**
    * Decodes a blob transaction from the provided RLP input.
    *
@@ -44,7 +48,6 @@ public static Transaction decode(final RLPInput input) {
     List<KZGCommitment> commitments = input.readList(KZGCommitment::readFrom);
     List<KZGProof> proofs = input.readList(KZGProof::readFrom);
     input.leaveList();
-    builder.kzgBlobs(commitments, blobs, proofs);
-    return builder.build();
+    return builder.kzgBlobs(commitments, blobs, proofs).build();
   }
 }
diff --git a/.../src/test/java/org/hyperledger/besu/ethereum/core/encoding/TransactionRLPDecoderTest.java b/.../src/test/java/org/hyperledger/besu/ethereum/core/encoding/TransactionRLPDecoderTest.java
@@ -17,6 +17,7 @@
 import static org.assertj.core.api.Assertions.assertThat;
 import static org.assertj.core.api.Assertions.assertThatThrownBy;
 import static org.hyperledger.besu.evm.account.Account.MAX_NONCE;
+import static org.junit.jupiter.api.Assumptions.assumeTrue;
 
 import org.hyperledger.besu.datatypes.Wei;
 import org.hyperledger.besu.ethereum.core.Transaction;
@@ -32,7 +33,6 @@
 import org.junit.jupiter.api.Test;
 import org.junit.jupiter.params.ParameterizedTest;
 import org.junit.jupiter.params.provider.MethodSource;
-import org.junit.jupiter.params.provider.ValueSource;
 
 class TransactionRLPDecoderTest {
 
@@ -82,15 +82,22 @@ void shouldDecodeWithHighNonce() {
   private static Collection<Object[]> dataTransactionSize() {
     return Arrays.asList(
         new Object[][] {
-          {FRONTIER_TX_RLP, "FRONTIER_TX_RLP"},
-          {EIP1559_TX_RLP, "EIP1559_TX_RLP"},
-          {NONCE_64_BIT_MAX_MINUS_2_TX_RLP, "NONCE_64_BIT_MAX_MINUS_2_TX_RLP"}
+          {FRONTIER_TX_RLP, "FRONTIER_TX_RLP", true},
+          {EIP1559_TX_RLP, "EIP1559_TX_RLP", true},
+          {NONCE_64_BIT_MAX_MINUS_2_TX_RLP, "NONCE_64_BIT_MAX_MINUS_2_TX_RLP", true},
+          {
+            "01f89a0130308263309430303030303030303030303030303030303030303030f838f7943030303030303030303030303030303030303030e0a0303030303030303030303030303030303030303030303030303030303030303001a03130303130303031313031313031303130303030323030323030323030313030a03030303030303030303030303030303030303030303030303030303030303030",
+            "EIP1559 list too small",
+            false
+          }
         });
   }
 
   @ParameterizedTest(name = "[{index}] {1}")
   @MethodSource("dataTransactionSize")
-  void shouldCalculateCorrectTransactionSize(final String rlp_tx, final String ignoredName) {
+  void shouldCalculateCorrectTransactionSize(
+      final String rlp_tx, final String ignoredName, final boolean valid) {
+    assumeTrue(valid);
     // Create bytes from String
     final Bytes bytes = Bytes.fromHexString(rlp_tx);
     // Decode bytes into a transaction
@@ -101,13 +108,27 @@ void shouldCalculateCorrectTransactionSize(final String rlp_tx, final String ign
     assertThat(transaction.getSize()).isEqualTo(transactionBytes.size());
   }
 
-  @ParameterizedTest
-  @ValueSource(strings = {FRONTIER_TX_RLP, EIP1559_TX_RLP, NONCE_64_BIT_MAX_MINUS_2_TX_RLP})
-  void shouldReturnCorrectEncodedBytes(final String txRlp) {
+  @ParameterizedTest(name = "[{index}] {1}")
+  @MethodSource("dataTransactionSize")
+  void shouldReturnCorrectEncodedBytes(
+      final String txRlp, final String ignoredName, final boolean valid) {
+    assumeTrue(valid);
     final Transaction transaction = decodeRLP(RLP.input(Bytes.fromHexString(txRlp)));
     assertThat(transaction.encoded()).isEqualTo(Bytes.fromHexString(txRlp));
   }
 
+  @ParameterizedTest(name = "[{index}] {1}")
+  @MethodSource("dataTransactionSize")
+  void shouldDecodeRLP(final String txRlp, final String ignoredName, final boolean valid) {
+    if (valid) {
+      // thrown exceptions will break test
+      decodeRLP(RLP.input(Bytes.fromHexString(txRlp)));
+    } else {
+      assertThatThrownBy(() -> decodeRLP(RLP.input(Bytes.fromHexString(txRlp))))
+          .isInstanceOf(RLPException.class);
+    }
+  }
+
   private Transaction decodeRLP(final RLPInput input) {
     return TransactionDecoder.decodeRLP(input, EncodingContext.POOLED_TRANSACTION);
   }

diff --git a/ethereum/rlp/src/main/java/org/hyperledger/besu/ethereum/rlp/AbstractRLPInput.java b/ethereum/rlp/src/main/java/org/hyperledger/besu/ethereum/rlp/AbstractRLPInput.java
@@ -147,17 +147,16 @@ private void prepareCurrentItem() {
   }
 
   private void validateCurrentItem() {
-    if (currentKind == RLPDecodingHelpers.Kind.SHORT_ELEMENT) {
-      // Validate that a single byte SHORT_ELEMENT payload is not <= 0x7F. If it is, is should have
-      // been written as a BYTE_ELEMENT.
-      if (currentPayloadSize == 1
-          && currentPayloadOffset < size
-          && (payloadByte(0) & 0xFF) <= 0x7F) {
-        throwMalformed(
-            "Malformed RLP item: single byte value 0x%s should have been "
-                + "written without a prefix",
-            hex(currentPayloadOffset, currentPayloadOffset + 1));
-      }
+    // Validate that a single byte SHORT_ELEMENT payload is not <= 0x7F. If it is, is should have
+    // been written as a BYTE_ELEMENT.
+    if (currentKind == RLPDecodingHelpers.Kind.SHORT_ELEMENT
+        && currentPayloadSize == 1
+        && currentPayloadOffset < size
+        && (payloadByte(0) & 0xFF) <= 0x7F) {
+      throwMalformed(
+          "Malformed RLP item: single byte value 0x%s should have been "
+              + "written without a prefix",
+          hex(currentPayloadOffset, currentPayloadOffset + 1));
     }
 
     if (currentPayloadSize > 0 && currentPayloadOffset >= size) {
@@ -186,9 +185,9 @@ public boolean isDone() {
 
   private String hex(final long start, final long taintedEnd) {
     final long end = Math.min(taintedEnd, size);
-    final long size = end - start;
-    if (size < 10) {
-      return inputHex(start, Math.toIntExact(size));
+    final long length = end - start;
+    if (length < 10) {
+      return inputHex(start, Math.toIntExact(length));
     } else {
       return String.format("%s...%s", inputHex(start, 4), inputHex(end - 4, 4));
     }
@@ -245,6 +244,9 @@ private void checkElt(final String what) {
     if (currentItem >= size) {
       throw error("Cannot read a %s, input is fully consumed", what);
     }
+    if (depth > 0 && currentPayloadOffset + currentPayloadSize > endOfListOffset[depth - 1]) {
+      throw error("Cannot read a %s, too large for enclosing list", what);
+    }
     if (isEndOfCurrentList()) {
       throw error("Cannot read a %s, reached end of current list", what);
     }