From 25bb9fcd00b2d681a3729b8cdc4db51bae3b6b54 Mon Sep 17 00:00:00 2001 From: Justin Florentine Date: Tue, 7 Nov 2023 09:49:48 -0500 Subject: [PATCH] blob restoral in both legacy and layered implementations and test coverage of tx copy in builder Signed-off-by: Justin Florentine --- .../besu/ethereum/core/Transaction.java | 25 +++++++++++++++++++ .../ethereum/core/TransactionBuilderTest.java | 22 ++++++++++++++++ .../DisabledPendingTransactions.java | 5 ++++ .../eth/transactions/PendingTransactions.java | 2 +- .../eth/transactions/TransactionPool.java | 22 +++++++++------- .../layered/AbstractTransactionsLayer.java | 8 +++++- .../layered/LayeredPendingTransactions.java | 14 +++++++++++ .../AbstractPendingTransactionsSorter.java | 13 ++++++++++ .../GasPricePendingTransactionsSorter.java | 4 +++ .../AbstractTransactionPoolTest.java | 3 ++- 10 files changed, 106 insertions(+), 12 deletions(-) diff --git a/ethereum/core/src/main/java/org/hyperledger/besu/ethereum/core/Transaction.java b/ethereum/core/src/main/java/org/hyperledger/besu/ethereum/core/Transaction.java index 9d51e1a3c1f..8cd7689272d 100644 --- a/ethereum/core/src/main/java/org/hyperledger/besu/ethereum/core/Transaction.java +++ b/ethereum/core/src/main/java/org/hyperledger/besu/ethereum/core/Transaction.java @@ -1096,6 +1096,26 @@ public static class Builder { protected List versionedHashes = null; private BlobsWithCommitments blobsWithCommitments; + public Builder copiedFrom(final Transaction toCopy) { + this.transactionType = toCopy.transactionType; + this.nonce = toCopy.nonce; + this.gasPrice = toCopy.gasPrice.orElse(null); + this.maxPriorityFeePerGas = toCopy.maxPriorityFeePerGas.orElse(null); + this.maxFeePerGas = toCopy.maxFeePerGas.orElse(null); + this.maxFeePerBlobGas = toCopy.maxFeePerBlobGas.orElse(null); + this.gasLimit = toCopy.gasLimit; + this.to = toCopy.to; + this.value = toCopy.value; + this.signature = toCopy.signature; + this.payload = toCopy.payload; + this.accessList = toCopy.maybeAccessList; + this.sender = toCopy.sender; + this.chainId = toCopy.chainId; + this.versionedHashes = toCopy.versionedHashes.orElse(null); + this.blobsWithCommitments = toCopy.blobsWithCommitments.orElse(null); + return this; + } + public Builder type(final TransactionType transactionType) { this.transactionType = transactionType; return this; @@ -1260,5 +1280,10 @@ public Builder kzgBlobs( new BlobsWithCommitments(kzgCommitments, blobs, kzgProofs, versionedHashes); return this; } + + public Builder blobsWithCommitments(final BlobsWithCommitments blobsWithCommitments) { + this.blobsWithCommitments = blobsWithCommitments; + return this; + } } } diff --git a/ethereum/core/src/test/java/org/hyperledger/besu/ethereum/core/TransactionBuilderTest.java b/ethereum/core/src/test/java/org/hyperledger/besu/ethereum/core/TransactionBuilderTest.java index b06a63cb95b..090925b6ef7 100644 --- a/ethereum/core/src/test/java/org/hyperledger/besu/ethereum/core/TransactionBuilderTest.java +++ b/ethereum/core/src/test/java/org/hyperledger/besu/ethereum/core/TransactionBuilderTest.java @@ -17,8 +17,12 @@ import static java.util.stream.Collectors.toUnmodifiableSet; import static org.assertj.core.api.Assertions.assertThat; +import static org.junit.jupiter.api.Assertions.assertEquals; import static org.junit.jupiter.api.Assertions.fail; +import org.apache.tuweni.bytes.Bytes; +import org.apache.tuweni.bytes.DelegatingBytes; +import org.apache.tuweni.bytes.MutableBytes; import org.hyperledger.besu.crypto.KeyPair; import org.hyperledger.besu.crypto.SignatureAlgorithm; import org.hyperledger.besu.crypto.SignatureAlgorithmFactory; @@ -34,6 +38,8 @@ import java.util.stream.Stream; import com.google.common.base.Suppliers; +import org.hyperledger.besu.ethereum.rlp.BytesValueRLPOutput; +import org.hyperledger.besu.ethereum.rlp.RLPOutput; import org.junit.jupiter.api.Test; public class TransactionBuilderTest { @@ -76,4 +82,20 @@ public void zeroBlobTransactionIsInvalid() { assertThat(iea).hasMessage("Blob transaction must have at least one versioned hash"); } } + + @Test + @SuppressWarnings("ReferenceEquality") + public void copyFromIsIdentical() { + final TransactionTestFixture fixture = new TransactionTestFixture(); + final Transaction transaction = fixture.createTransaction(senderKeys); + final Transaction.Builder builder = Transaction.builder(); + final Transaction copy = builder.copiedFrom(transaction).build(); + assertThat(copy).isEqualTo(transaction); + assertThat(copy == transaction).isFalse(); + BytesValueRLPOutput sourceRLP = new BytesValueRLPOutput(); + transaction.writeTo(sourceRLP); + BytesValueRLPOutput copyRLP = new BytesValueRLPOutput(); + copy.writeTo(copyRLP); + assertEquals(sourceRLP.encoded(), copyRLP.encoded()); + } } diff --git a/ethereum/eth/src/main/java/org/hyperledger/besu/ethereum/eth/transactions/DisabledPendingTransactions.java b/ethereum/eth/src/main/java/org/hyperledger/besu/ethereum/eth/transactions/DisabledPendingTransactions.java index de9d6a07e8e..c52876ced75 100644 --- a/ethereum/eth/src/main/java/org/hyperledger/besu/ethereum/eth/transactions/DisabledPendingTransactions.java +++ b/ethereum/eth/src/main/java/org/hyperledger/besu/ethereum/eth/transactions/DisabledPendingTransactions.java @@ -116,4 +116,9 @@ public String toTraceLog() { public String logStats() { return "Disabled"; } + + @Override + public Optional restoreBlob(final Transaction transaction) { + return Optional.empty(); + } } diff --git a/ethereum/eth/src/main/java/org/hyperledger/besu/ethereum/eth/transactions/PendingTransactions.java b/ethereum/eth/src/main/java/org/hyperledger/besu/ethereum/eth/transactions/PendingTransactions.java index bedc0efff57..8fbbe403405 100644 --- a/ethereum/eth/src/main/java/org/hyperledger/besu/ethereum/eth/transactions/PendingTransactions.java +++ b/ethereum/eth/src/main/java/org/hyperledger/besu/ethereum/eth/transactions/PendingTransactions.java @@ -83,7 +83,7 @@ default void signalInvalidAndRemoveDependentTransactions(final Transaction trans // no-op } - void restoreBlob(Transaction transaction); + Optional restoreBlob(Transaction transaction); @FunctionalInterface interface TransactionSelector { 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 fb161af2209..5bb32f5e883 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 @@ -232,9 +232,6 @@ private ValidationResult addTransaction( final boolean hasPriority = isPriorityTransaction(transaction, isLocal); if (pendingTransactions.containsTransaction(transaction)) { - if (transaction.getType().supportsBlob()) { - pendingTransactions.restoreBlob(transaction); - } LOG.atTrace() .setMessage("Discard already present transaction {}") .addArgument(transaction::toTraceLog) @@ -243,22 +240,29 @@ private ValidationResult addTransaction( metrics.incrementRejected(isLocal, hasPriority, TRANSACTION_ALREADY_KNOWN, "txpool"); return ValidationResult.invalid(TRANSACTION_ALREADY_KNOWN); } + Transaction toAdd = transaction; // if adding a blob tx, and it is missing its blob, is a re-org and we should restore the blob // from cache. + if (transaction.getType().supportsBlob() && transaction.getBlobsWithCommitments().isEmpty()) { + final Optional maybeCachedBlob = pendingTransactions.restoreBlob(transaction); + if(maybeCachedBlob.isPresent()) { + toAdd = maybeCachedBlob.get(); + } + } final ValidationResultAndAccount validationResult = - validateTransaction(transaction, isLocal, hasPriority); + validateTransaction(toAdd, isLocal, hasPriority); if (validationResult.result.isValid()) { final TransactionAddedResult status = pendingTransactions.addTransaction( - PendingTransaction.newPendingTransaction(transaction, isLocal, hasPriority), + PendingTransaction.newPendingTransaction(toAdd, isLocal, hasPriority), validationResult.maybeAccount); if (status.isSuccess()) { LOG.atTrace() .setMessage("Added {} transaction {}") .addArgument(() -> isLocal ? "local" : "remote") - .addArgument(transaction::toTraceLog) + .addArgument(toAdd::toTraceLog) .log(); } else { final var rejectReason = @@ -271,7 +275,7 @@ private ValidationResult addTransaction( }); LOG.atTrace() .setMessage("Transaction {} rejected reason {}") - .addArgument(transaction::toTraceLog) + .addArgument(toAdd::toTraceLog) .addArgument(rejectReason) .log(); metrics.incrementRejected(isLocal, hasPriority, rejectReason, "txpool"); @@ -280,7 +284,7 @@ private ValidationResult addTransaction( } else { LOG.atTrace() .setMessage("Discard invalid transaction {}, reason {}") - .addArgument(transaction::toTraceLog) + .addArgument(toAdd::toTraceLog) .addArgument(validationResult.result::getInvalidReason) .log(); metrics.incrementRejected( @@ -288,7 +292,7 @@ private ValidationResult addTransaction( if (!isLocal && !INVALID_TX_CACHE_IGNORED_ERRORS.contains( validationResult.result.getInvalidReason())) { - pendingTransactions.signalInvalidAndRemoveDependentTransactions(transaction); + pendingTransactions.signalInvalidAndRemoveDependentTransactions(toAdd); } } diff --git a/ethereum/eth/src/main/java/org/hyperledger/besu/ethereum/eth/transactions/layered/AbstractTransactionsLayer.java b/ethereum/eth/src/main/java/org/hyperledger/besu/ethereum/eth/transactions/layered/AbstractTransactionsLayer.java index 474af3cdb43..de8b3ace422 100644 --- a/ethereum/eth/src/main/java/org/hyperledger/besu/ethereum/eth/transactions/layered/AbstractTransactionsLayer.java +++ b/ethereum/eth/src/main/java/org/hyperledger/besu/ethereum/eth/transactions/layered/AbstractTransactionsLayer.java @@ -25,6 +25,8 @@ import static org.hyperledger.besu.ethereum.eth.transactions.layered.TransactionsLayer.RemovalReason.REPLACED; import org.hyperledger.besu.datatypes.Address; +import org.hyperledger.besu.datatypes.Blob; +import org.hyperledger.besu.datatypes.BlobsWithCommitments; import org.hyperledger.besu.datatypes.Hash; import org.hyperledger.besu.ethereum.core.BlockHeader; import org.hyperledger.besu.ethereum.core.Transaction; @@ -76,7 +78,7 @@ public abstract class AbstractTransactionsLayer implements TransactionsLayer { private OptionalLong nextLayerOnDroppedListenerId = OptionalLong.empty(); protected long spaceUsed = 0; - private final Cache blobCache; + private final Cache blobCache; public AbstractTransactionsLayer( final TransactionPoolConfiguration poolConfig, @@ -610,4 +612,8 @@ boolean consistencyCheck( protected abstract void internalConsistencyCheck( final Map> prevLayerTxsBySender); + + public Cache getBlobCache() { + return blobCache; + } } diff --git a/ethereum/eth/src/main/java/org/hyperledger/besu/ethereum/eth/transactions/layered/LayeredPendingTransactions.java b/ethereum/eth/src/main/java/org/hyperledger/besu/ethereum/eth/transactions/layered/LayeredPendingTransactions.java index bd93f0d9fa0..fd94adbe80f 100644 --- a/ethereum/eth/src/main/java/org/hyperledger/besu/ethereum/eth/transactions/layered/LayeredPendingTransactions.java +++ b/ethereum/eth/src/main/java/org/hyperledger/besu/ethereum/eth/transactions/layered/LayeredPendingTransactions.java @@ -24,6 +24,7 @@ import static org.hyperledger.besu.ethereum.eth.transactions.layered.TransactionsLayer.RemovalReason.RECONCILED; import org.hyperledger.besu.datatypes.Address; +import org.hyperledger.besu.datatypes.BlobsWithCommitments; import org.hyperledger.besu.datatypes.Hash; import org.hyperledger.besu.ethereum.core.BlockHeader; import org.hyperledger.besu.ethereum.core.Transaction; @@ -535,4 +536,17 @@ public synchronized String toTraceLog() { public synchronized String logStats() { return prioritizedTransactions.logStats(); } + + @Override + public Optional restoreBlob(final Transaction transaction) { + Transaction.Builder txBuilder = Transaction.builder(); + txBuilder.copiedFrom(transaction); + final BlobsWithCommitments bwc = prioritizedTransactions.getBlobCache().getIfPresent(transaction.getHash()); + if(bwc != null) { + txBuilder.blobsWithCommitments(bwc); + return Optional.of(txBuilder.build()); + } else { + return Optional.empty(); + } + } } diff --git a/ethereum/eth/src/main/java/org/hyperledger/besu/ethereum/eth/transactions/sorter/AbstractPendingTransactionsSorter.java b/ethereum/eth/src/main/java/org/hyperledger/besu/ethereum/eth/transactions/sorter/AbstractPendingTransactionsSorter.java index 7c954c30af2..f368b891b38 100644 --- a/ethereum/eth/src/main/java/org/hyperledger/besu/ethereum/eth/transactions/sorter/AbstractPendingTransactionsSorter.java +++ b/ethereum/eth/src/main/java/org/hyperledger/besu/ethereum/eth/transactions/sorter/AbstractPendingTransactionsSorter.java @@ -547,4 +547,17 @@ public void signalInvalidAndRemoveDependentTransactions(final Transaction transa signalInvalidAndGetDependentTransactions(transaction).forEach(this::removeTransaction); } } + + @Override + public Optional restoreBlob(final Transaction transaction) { + Transaction.Builder txBuilder = Transaction.builder(); + txBuilder.copiedFrom(transaction); + final BlobsWithCommitments bwc = blobCache.getIfPresent(transaction.getHash()); + if(bwc != null) { + txBuilder.blobsWithCommitments(bwc); + return Optional.of(txBuilder.build()); + } else { + return Optional.empty(); + } + } } diff --git a/ethereum/eth/src/main/java/org/hyperledger/besu/ethereum/eth/transactions/sorter/GasPricePendingTransactionsSorter.java b/ethereum/eth/src/main/java/org/hyperledger/besu/ethereum/eth/transactions/sorter/GasPricePendingTransactionsSorter.java index 4726574a406..e3883bdc4f2 100644 --- a/ethereum/eth/src/main/java/org/hyperledger/besu/ethereum/eth/transactions/sorter/GasPricePendingTransactionsSorter.java +++ b/ethereum/eth/src/main/java/org/hyperledger/besu/ethereum/eth/transactions/sorter/GasPricePendingTransactionsSorter.java @@ -17,6 +17,7 @@ import static java.util.Comparator.comparing; import org.hyperledger.besu.ethereum.core.BlockHeader; +import org.hyperledger.besu.ethereum.core.Transaction; import org.hyperledger.besu.ethereum.eth.transactions.PendingTransaction; import org.hyperledger.besu.ethereum.eth.transactions.TransactionPoolConfiguration; import org.hyperledger.besu.plugin.services.MetricsSystem; @@ -25,6 +26,7 @@ import java.util.Comparator; import java.util.Iterator; import java.util.NavigableSet; +import java.util.Optional; import java.util.TreeSet; import java.util.function.Supplier; @@ -57,6 +59,7 @@ public void reset() { prioritizedTransactions.clear(); } + @Override public void manageBlockAdded(final BlockHeader blockHeader) { // nothing to do @@ -81,4 +84,5 @@ protected void removePrioritizedTransaction(final PendingTransaction removedPend protected PendingTransaction getLeastPriorityTransaction() { return prioritizedTransactions.last(); } + } 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 d437606a17a..593e3659094 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 @@ -490,9 +490,10 @@ public void shouldReAddBlobTxsWhenReorgHappens() { final Block reorgFork3 = appendBlock(Difficulty.of(3000), reorgFork2.getHeader()); verifyChainHeadIs(reorgFork3); - assertTransactionPending(transactionBlob); assertTransactionPending(transaction0); assertTransactionPending(transaction1); + assertTransactionPending(transactionBlob); + Optional maybeBlob = transactions.getTransactionByHash(transactionBlob.getHash()); assertThat(maybeBlob).isPresent(); Transaction restoredBlob = maybeBlob.get();