From 4b58d071f6c34ce4e79c0f2ce7ce8d43a520e1ee Mon Sep 17 00:00:00 2001 From: Danno Ferrin Date: Thu, 9 Nov 2023 16:31:53 -0700 Subject: [PATCH] fix yParity flakey test (#6151) Fix the flakeiness in EthGetTransactionByHashTest as well as some other sonar identified cleanup. Signed-off-by: Danno Ferrin --- .../methods/EthGetTransactionByHashTest.java | 29 ++++++++++++--- .../BlobPooledTransactionDecoder.java | 7 +++- .../encoding/TransactionRLPDecoderTest.java | 37 +++++++++++++++---- .../besu/ethereum/rlp/AbstractRLPInput.java | 30 ++++++++------- 4 files changed, 73 insertions(+), 30 deletions(-) diff --git a/ethereum/api/src/test/java/org/hyperledger/besu/ethereum/api/jsonrpc/internal/methods/EthGetTransactionByHashTest.java b/ethereum/api/src/test/java/org/hyperledger/besu/ethereum/api/jsonrpc/internal/methods/EthGetTransactionByHashTest.java index 34ab6ba93eb..42018f2aebc 100644 --- a/ethereum/api/src/test/java/org/hyperledger/besu/ethereum/api/jsonrpc/internal/methods/EthGetTransactionByHashTest.java +++ b/ethereum/api/src/test/java/org/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/ethereum/core/src/main/java/org/hyperledger/besu/ethereum/core/encoding/BlobPooledTransactionDecoder.java b/ethereum/core/src/main/java/org/hyperledger/besu/ethereum/core/encoding/BlobPooledTransactionDecoder.java index 1ccd5c4ad42..bac3d472aca 100644 --- a/ethereum/core/src/main/java/org/hyperledger/besu/ethereum/core/encoding/BlobPooledTransactionDecoder.java +++ b/ethereum/core/src/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 commitments = input.readList(KZGCommitment::readFrom); List 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/ethereum/core/src/test/java/org/hyperledger/besu/ethereum/core/encoding/TransactionRLPDecoderTest.java b/ethereum/core/src/test/java/org/hyperledger/besu/ethereum/core/encoding/TransactionRLPDecoderTest.java index 245561158b0..4a84a5d56f0 100644 --- a/ethereum/core/src/test/java/org/hyperledger/besu/ethereum/core/encoding/TransactionRLPDecoderTest.java +++ b/ethereum/core/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 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 index 93a0bcebdf4..b75ae3224aa 100644 --- 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); }