From 5a6af5065abb137e79b5784b3e41b50e797b4f0b Mon Sep 17 00:00:00 2001 From: Justin Florentine Date: Wed, 4 Oct 2023 17:05:40 -0400 Subject: [PATCH] test coverage and fix for subtle re-org bug prior to proposals Signed-off-by: Justin Florentine --- .../jsonrpc/AbstractJsonRpcTest.java | 13 ++++- ...gineCancunBlockBulidingAcceptanceTest.java | 5 +- .../internal/results/BlobsBundleV1.java | 11 +++++ .../engine/EngineNewPayloadEIP6110Test.java | 4 +- .../encoding/BlobTransactionEncodingTest.java | 4 +- .../eth/transactions/TransactionPool.java | 4 ++ .../AbstractTransactionPoolTest.java | 48 +++++++++++++++++++ 7 files changed, 83 insertions(+), 6 deletions(-) diff --git a/acceptance-tests/tests/src/test/java/org/hyperledger/besu/tests/acceptance/jsonrpc/AbstractJsonRpcTest.java b/acceptance-tests/tests/src/test/java/org/hyperledger/besu/tests/acceptance/jsonrpc/AbstractJsonRpcTest.java index 551f661f971..a4e90659680 100644 --- a/acceptance-tests/tests/src/test/java/org/hyperledger/besu/tests/acceptance/jsonrpc/AbstractJsonRpcTest.java +++ b/acceptance-tests/tests/src/test/java/org/hyperledger/besu/tests/acceptance/jsonrpc/AbstractJsonRpcTest.java @@ -83,9 +83,18 @@ public void test() throws IOException { testsContext.mapper.readValue(testCaseFileURI.toURL(), JsonRpcTestCase.class); final String rpcMethod = String.valueOf(testCase.getRequest().get("method")); - + OkHttpClient client = testsContext.httpClient; + if (System.getenv("BESU_DEBUG_CHILD_PROCESS_PORT") != null) { + // if running in debug mode, set a longer timeout + client = + testsContext + .httpClient + .newBuilder() + .readTimeout(900, java.util.concurrent.TimeUnit.SECONDS) + .build(); + } final Call testRequest = - testsContext.httpClient.newCall( + client.newCall( new Request.Builder() .url(getRpcUrl(rpcMethod)) .post(RequestBody.create(testCase.getRequest().toString(), MEDIA_TYPE_JSON)) diff --git a/acceptance-tests/tests/src/test/java/org/hyperledger/besu/tests/acceptance/jsonrpc/ExecutionEngineCancunBlockBulidingAcceptanceTest.java b/acceptance-tests/tests/src/test/java/org/hyperledger/besu/tests/acceptance/jsonrpc/ExecutionEngineCancunBlockBulidingAcceptanceTest.java index c5c694a21dd..a2ce2ad3091 100644 --- a/acceptance-tests/tests/src/test/java/org/hyperledger/besu/tests/acceptance/jsonrpc/ExecutionEngineCancunBlockBulidingAcceptanceTest.java +++ b/acceptance-tests/tests/src/test/java/org/hyperledger/besu/tests/acceptance/jsonrpc/ExecutionEngineCancunBlockBulidingAcceptanceTest.java @@ -41,7 +41,8 @@ public class ExecutionEngineCancunBlockBulidingAcceptanceTest extends AbstractJs private static JsonRpcTestsContext testsContext; - public ExecutionEngineCancunBlockBulidingAcceptanceTest(final String ignored, final URI testCaseFileURI) { + public ExecutionEngineCancunBlockBulidingAcceptanceTest( + final String ignored, final URI testCaseFileURI) { super(ignored, testsContext, testCaseFileURI); } @@ -77,6 +78,8 @@ protected void evaluateResponse( final ObjectNode blobsBundle = (ObjectNode) result.get("blobsBundle"); assertThat(execPayload.get("transactions").getNodeType()).isEqualTo(JsonNodeType.ARRAY); final ArrayNode transactions = (ArrayNode) execPayload.get("transactions"); + // actually, you need to decode the transactions and count how many unique + // versioned hashes are referenced amongst them. assertThat(blobsBundle.get("commitments").getNodeType()).isEqualTo(JsonNodeType.ARRAY); final ArrayNode commitments = (ArrayNode) blobsBundle.get("commitments"); assertThat(blobsBundle.get("blobs").getNodeType()).isEqualTo(JsonNodeType.ARRAY); diff --git a/ethereum/api/src/main/java/org/hyperledger/besu/ethereum/api/jsonrpc/internal/results/BlobsBundleV1.java b/ethereum/api/src/main/java/org/hyperledger/besu/ethereum/api/jsonrpc/internal/results/BlobsBundleV1.java index 4350c1d20cd..3377aa63cce 100644 --- a/ethereum/api/src/main/java/org/hyperledger/besu/ethereum/api/jsonrpc/internal/results/BlobsBundleV1.java +++ b/ethereum/api/src/main/java/org/hyperledger/besu/ethereum/api/jsonrpc/internal/results/BlobsBundleV1.java @@ -29,10 +29,13 @@ import com.fasterxml.jackson.annotation.JsonGetter; import com.fasterxml.jackson.annotation.JsonPropertyOrder; import org.apache.tuweni.bytes.Bytes; +import org.slf4j.Logger; +import org.slf4j.LoggerFactory; @JsonPropertyOrder({"commitments", "proofs", "blobs"}) public class BlobsBundleV1 { + private static final Logger LOG = LoggerFactory.getLogger(BlobsBundleV1.class); private final List commitments; private final List proofs; @@ -67,6 +70,14 @@ public BlobsBundleV1(final List transactions) { .map(Blob::getData) .map(Bytes::toString) .collect(Collectors.toList()); + + LOG.debug( + "BlobsBundleV1: totalTxs: {}, blobTxs: {}, commitments: {}, proofs: {}, blobs: {}", + transactions.size(), + blobsWithCommitments.size(), + commitments.size(), + proofs.size(), + blobs.size()); } public BlobsBundleV1( diff --git a/ethereum/api/src/test/java/org/hyperledger/besu/ethereum/api/jsonrpc/internal/methods/engine/EngineNewPayloadEIP6110Test.java b/ethereum/api/src/test/java/org/hyperledger/besu/ethereum/api/jsonrpc/internal/methods/engine/EngineNewPayloadEIP6110Test.java index 04eade1abaa..0e89e66e192 100644 --- a/ethereum/api/src/test/java/org/hyperledger/besu/ethereum/api/jsonrpc/internal/methods/engine/EngineNewPayloadEIP6110Test.java +++ b/ethereum/api/src/test/java/org/hyperledger/besu/ethereum/api/jsonrpc/internal/methods/engine/EngineNewPayloadEIP6110Test.java @@ -174,7 +174,7 @@ protected BlockHeader createBlockHeader( .baseFeePerGas(Wei.ONE) .timestamp(super.experimentalHardfork.milestone()) .excessBlobGas(BlobGas.ZERO) - .blobGasUsed(100L) + .blobGasUsed(0L) .buildHeader(); BlockHeader mockHeader = @@ -185,7 +185,7 @@ protected BlockHeader createBlockHeader( .timestamp(parentBlockHeader.getTimestamp() + 1) .withdrawalsRoot(maybeWithdrawals.map(BodyValidation::withdrawalsRoot).orElse(null)) .excessBlobGas(BlobGas.ZERO) - .blobGasUsed(100L) + .blobGasUsed(0L) .depositsRoot(maybeDeposits.map(BodyValidation::depositsRoot).orElse(null)) .parentBeaconBlockRoot( maybeParentBeaconBlockRoot.isPresent() ? maybeParentBeaconBlockRoot : null) diff --git a/ethereum/core/src/test/java/org/hyperledger/besu/ethereum/core/encoding/BlobTransactionEncodingTest.java b/ethereum/core/src/test/java/org/hyperledger/besu/ethereum/core/encoding/BlobTransactionEncodingTest.java index 9487e476685..6b33f69dd5a 100644 --- a/ethereum/core/src/test/java/org/hyperledger/besu/ethereum/core/encoding/BlobTransactionEncodingTest.java +++ b/ethereum/core/src/test/java/org/hyperledger/besu/ethereum/core/encoding/BlobTransactionEncodingTest.java @@ -41,7 +41,9 @@ private static Stream provideOpaqueBytesNoBlobsWithCommitments() { createArgument( "0x03f89d850120b996ed81f1843b9aca00847735940e8307a12094000000000000000000000000000000000010101001855f495f4955c0847735940ee1a001d552e24560ec2f168be1d4a6385df61c70afe4288f00a3ad172da1a6f2b4f280a0b6690786e5fe79df67dcb60e8a9e8555142c3c96ffd5097c838717f0a7f64129a0112f01ed0cd3b86495f01736fbbc1b793f71565223aa26f093471a4d8605d198"), createArgument( - "0x03f897850120b996ed80840bebc200843b9aca078303345094c8d369b164361a8961286cfbab3bc10f962185a88080c08411e1a300e1a0011df88a2971c8a7ac494a7ba37ec1acaa1fc1edeeb38c839b5d1693d47b69b080a032f122f06e5802224db4c8a58fd22c75173a713f63f89936f811c144b9e40129a043a2a872cbfa5727007adf6a48febe5f190d2e4cd5ed6122823fb6ff47ecda32")); + "0x03f897850120b996ed80840bebc200843b9aca078303345094c8d369b164361a8961286cfbab3bc10f962185a88080c08411e1a300e1a0011df88a2971c8a7ac494a7ba37ec1acaa1fc1edeeb38c839b5d1693d47b69b080a032f122f06e5802224db4c8a58fd22c75173a713f63f89936f811c144b9e40129a043a2a872cbfa5727007adf6a48febe5f190d2e4cd5ed6122823fb6ff47ecda32"), + createArgument( + "0x03f8928501a1f0ff4313843b9aca00843b9aca0082520894e7249813d8ccf6fa95a2203f46a64166073d58878080c001e1a00134a7258134a61a4f36f876480b75a12ec5c9fd5bcf8a27c42f78ffd6149eec01a0da6b8722b5df41d2458fc4486c85e1ac936e8437f2c4001bcde73b7352b4c830a017412017e67474a9d75edf392d7ced91a2bf11358215150b69b62cb8e0d01871")); } private static Stream provideOpaqueBytesForNetwork() throws IOException { diff --git a/ethereum/eth/src/main/java/org/hyperledger/besu/ethereum/eth/transactions/TransactionPool.java b/ethereum/eth/src/main/java/org/hyperledger/besu/ethereum/eth/transactions/TransactionPool.java index f6c8161bf0c..9b24f6cb539 100644 --- a/ethereum/eth/src/main/java/org/hyperledger/besu/ethereum/eth/transactions/TransactionPool.java +++ b/ethereum/eth/src/main/java/org/hyperledger/besu/ethereum/eth/transactions/TransactionPool.java @@ -469,6 +469,10 @@ && strictReplayProtectionShouldBeEnforcedLocally(chainHeadBlockHeader) return ValidationResultAndAccount.invalid( TransactionInvalidReason.INVALID_TRANSACTION_FORMAT, "EIP-1559 transaction are not allowed yet"); + } else if (transaction.getType().equals(TransactionType.BLOB) + && transaction.getBlobsWithCommitments().isEmpty()) { + return ValidationResultAndAccount.invalid( + TransactionInvalidReason.INVALID_BLOBS, "Blob transaction must have at least one blob"); } // Call the transaction validator plugin if one is available diff --git a/ethereum/eth/src/test/java/org/hyperledger/besu/ethereum/eth/transactions/AbstractTransactionPoolTest.java b/ethereum/eth/src/test/java/org/hyperledger/besu/ethereum/eth/transactions/AbstractTransactionPoolTest.java index 7fad9f1a057..beeb2b1e43b 100644 --- a/ethereum/eth/src/test/java/org/hyperledger/besu/ethereum/eth/transactions/AbstractTransactionPoolTest.java +++ b/ethereum/eth/src/test/java/org/hyperledger/besu/ethereum/eth/transactions/AbstractTransactionPoolTest.java @@ -52,6 +52,7 @@ import org.hyperledger.besu.datatypes.Wei; import org.hyperledger.besu.ethereum.ProtocolContext; import org.hyperledger.besu.ethereum.chain.MutableBlockchain; +import org.hyperledger.besu.ethereum.core.BlobTestFixture; import org.hyperledger.besu.ethereum.core.Block; import org.hyperledger.besu.ethereum.core.BlockBody; import org.hyperledger.besu.ethereum.core.BlockHeader; @@ -144,6 +145,7 @@ public abstract class AbstractTransactionPoolTest { protected PendingTransactions transactions; protected final Transaction transaction0 = createTransaction(0); protected final Transaction transaction1 = createTransaction(1); + protected final Transaction transactionBlob = createBlobTransaction(0); protected final Transaction transactionOtherSender = createTransaction(1, KEY_PAIR2); private ExecutionContextTestFixture executionContext; @@ -454,6 +456,40 @@ public void shouldNotReAddTransactionsThatAreInBothForksWhenReorgHappens() { assertTransactionPending(transaction1); } + @Test + public void shouldNotReAddBlobTxsWhenReorgHappens() { + givenTransactionIsValid(transaction0); + givenTransactionIsValid(transaction1); + givenTransactionIsValid(transactionBlob); + + addAndAssertRemoteTransactionValid(transaction0); + addAndAssertRemoteTransactionValid(transaction1); + addAndAssertRemoteTransactionInvalid(transactionBlob); + + final BlockHeader commonParent = getHeaderForCurrentChainHead(); + final Block originalFork1 = appendBlock(Difficulty.of(1000), commonParent, transaction0); + final Block originalFork2 = + appendBlock(Difficulty.of(10), originalFork1.getHeader(), transaction1); + final Block originalFork3 = + appendBlock(Difficulty.of(1), originalFork2.getHeader(), transactionBlob); + assertTransactionNotPending(transaction0); + assertTransactionNotPending(transaction1); + assertTransactionNotPending(transactionBlob); + + final Block reorgFork1 = appendBlock(Difficulty.ONE, commonParent); + verifyChainHeadIs(originalFork3); + + final Block reorgFork2 = appendBlock(Difficulty.of(2000), reorgFork1.getHeader()); + verifyChainHeadIs(reorgFork2); + + final Block reorgFork3 = appendBlock(Difficulty.of(3000), reorgFork2.getHeader()); + verifyChainHeadIs(reorgFork3); + + assertTransactionNotPending(transactionBlob); + assertTransactionPending(transaction0); + assertTransactionPending(transaction1); + } + @ParameterizedTest @ValueSource(booleans = {true, false}) public void addLocalTransaction_strictReplayProtectionOn_txWithChainId_chainIdIsConfigured( @@ -1189,6 +1225,18 @@ protected Transaction createFrontierTransaction(final int transactionNumber, fin .createTransaction(KEY_PAIR1); } + protected Transaction createBlobTransaction(final int nonce) { + return new TransactionTestFixture() + .nonce(nonce) + .gasLimit(blockGasLimit) + .gasPrice(null) + .maxFeePerGas(Optional.of(Wei.of(5000L))) + .maxPriorityFeePerGas(Optional.of(Wei.of(1000L))) + .type(TransactionType.BLOB) + .blobsWithCommitments(Optional.of(new BlobTestFixture().createBlobsWithCommitments(1))) + .createTransaction(KEY_PAIR1); + } + protected int add1559TxAndGetPendingTxsCount( final Wei genesisBaseFee, final Wei minGasPrice,