diff --git a/CHANGELOG.md b/CHANGELOG.md index 448b7b12819..9e2e87de28d 100644 --- a/CHANGELOG.md +++ b/CHANGELOG.md @@ -8,6 +8,7 @@ - Resets engine QoS timer with every call to the engine API instead of only when ExchangeTransitionConfiguration is called [#4411](https://github.com/hyperledger/besu/issues/4411) - ExchangeTransitionConfiguration mismatch will only submit a debug log not a warning anymore [#4411](https://github.com/hyperledger/besu/issues/4411) - Upgrade besu-native to 0.6.1 and include linux arm64 build of bls12-381 [#4416](https://github.com/hyperledger/besu/pull/4416) +- Transaction pool improvements to avoid filling the pool with not executable transactions, that could result in empty or semi-empty block proposals [#4425](https://github.com/hyperledger/besu/pull/4425) ### Bug Fixes - Retry block creation if there is a transient error and we still have time, to mitigate empty block issue [#4407](https://github.com/hyperledger/besu/pull/4407) diff --git a/consensus/merge/src/main/java/org/hyperledger/besu/consensus/merge/blockcreation/MergeCoordinator.java b/consensus/merge/src/main/java/org/hyperledger/besu/consensus/merge/blockcreation/MergeCoordinator.java index 26cb558150b..3e75367cb87 100644 --- a/consensus/merge/src/main/java/org/hyperledger/besu/consensus/merge/blockcreation/MergeCoordinator.java +++ b/consensus/merge/src/main/java/org/hyperledger/besu/consensus/merge/blockcreation/MergeCoordinator.java @@ -190,6 +190,11 @@ public PayloadIdentifier preparePayload( Result result = validateBlock(emptyBlock); if (result.blockProcessingOutputs.isPresent()) { mergeContext.putPayloadById(payloadIdentifier, emptyBlock); + debugLambda( + LOG, + "Built empty block proposal {} for payload {}", + emptyBlock::toLogString, + payloadIdentifier::toShortHexString); } else { LOG.warn( "failed to execute empty block proposal {}, reason {}", diff --git a/consensus/merge/src/test/java/org/hyperledger/besu/consensus/merge/blockcreation/MergeCoordinatorTest.java b/consensus/merge/src/test/java/org/hyperledger/besu/consensus/merge/blockcreation/MergeCoordinatorTest.java index 7927c1233b6..211384c6688 100644 --- a/consensus/merge/src/test/java/org/hyperledger/besu/consensus/merge/blockcreation/MergeCoordinatorTest.java +++ b/consensus/merge/src/test/java/org/hyperledger/besu/consensus/merge/blockcreation/MergeCoordinatorTest.java @@ -186,7 +186,7 @@ public void shouldRetryBlockCreationIfStillHaveTime() { .thenReturn(CompletableFuture.failedFuture(new StorageException("lock"))) .thenAnswer(i -> CompletableFuture.completedFuture(i.getArgument(0, Supplier.class).get())); - transactions.addLocalTransaction(localTransaction0); + transactions.addLocalTransaction(localTransaction0, Optional.empty()); var payloadId = coordinator.preparePayload( diff --git a/ethereum/api/src/integration-test/java/org/hyperledger/besu/ethereum/api/jsonrpc/methods/fork/frontier/EthGetFilterChangesIntegrationTest.java b/ethereum/api/src/integration-test/java/org/hyperledger/besu/ethereum/api/jsonrpc/methods/fork/frontier/EthGetFilterChangesIntegrationTest.java index f6b1d21e157..333720ac4cc 100644 --- a/ethereum/api/src/integration-test/java/org/hyperledger/besu/ethereum/api/jsonrpc/methods/fork/frontier/EthGetFilterChangesIntegrationTest.java +++ b/ethereum/api/src/integration-test/java/org/hyperledger/besu/ethereum/api/jsonrpc/methods/fork/frontier/EthGetFilterChangesIntegrationTest.java @@ -219,7 +219,7 @@ public void shouldReturnHashesIfNewPendingTransactions() { JsonRpcResponse actual = method.response(request); assertThat(actual).usingRecursiveComparison().isEqualTo(expected); - transactions.addRemoteTransaction(transaction); + transactions.addRemoteTransaction(transaction, Optional.empty()); // We've added one transaction, so there should be one new hash. expected = diff --git a/ethereum/api/src/integration-test/java/org/hyperledger/besu/ethereum/api/jsonrpc/methods/fork/london/EthGetFilterChangesIntegrationTest.java b/ethereum/api/src/integration-test/java/org/hyperledger/besu/ethereum/api/jsonrpc/methods/fork/london/EthGetFilterChangesIntegrationTest.java index 3863e406070..703bd22ef0c 100644 --- a/ethereum/api/src/integration-test/java/org/hyperledger/besu/ethereum/api/jsonrpc/methods/fork/london/EthGetFilterChangesIntegrationTest.java +++ b/ethereum/api/src/integration-test/java/org/hyperledger/besu/ethereum/api/jsonrpc/methods/fork/london/EthGetFilterChangesIntegrationTest.java @@ -219,7 +219,7 @@ public void shouldReturnHashesIfNewPendingTransactions() { JsonRpcResponse actual = method.response(request); assertThat(actual).usingRecursiveComparison().isEqualTo(expected); - transactions.addRemoteTransaction(transaction); + transactions.addRemoteTransaction(transaction, Optional.empty()); // We've added one transaction, so there should be one new hash. expected = diff --git a/ethereum/blockcreation/src/main/java/org/hyperledger/besu/ethereum/blockcreation/BlockTransactionSelector.java b/ethereum/blockcreation/src/main/java/org/hyperledger/besu/ethereum/blockcreation/BlockTransactionSelector.java index 50031b00ada..e38cb75914e 100644 --- a/ethereum/blockcreation/src/main/java/org/hyperledger/besu/ethereum/blockcreation/BlockTransactionSelector.java +++ b/ethereum/blockcreation/src/main/java/org/hyperledger/besu/ethereum/blockcreation/BlockTransactionSelector.java @@ -14,6 +14,8 @@ */ package org.hyperledger.besu.ethereum.blockcreation; +import static org.hyperledger.besu.util.Slf4jLambdaHelper.traceLambda; + import org.hyperledger.besu.datatypes.Address; import org.hyperledger.besu.datatypes.Wei; import org.hyperledger.besu.ethereum.chain.Blockchain; @@ -39,6 +41,7 @@ import java.util.List; import java.util.concurrent.CancellationException; import java.util.function.Supplier; +import java.util.stream.Collectors; import com.google.common.collect.Lists; import org.apache.tuweni.bytes.Bytes; @@ -82,6 +85,12 @@ private void update( transactions.add(transaction); receipts.add(receipt); cumulativeGasUsed += gasUsed; + traceLambda( + LOG, + "New selected transaction {}, total transactions {}, cumulative gas used {}", + transaction::toTraceLog, + transactions::size, + () -> cumulativeGasUsed); } public List getTransactions() { @@ -95,6 +104,13 @@ public List getReceipts() { public long getCumulativeGasUsed() { return cumulativeGasUsed; } + + public String toTraceLog() { + return "cumulativeGasUsed=" + + cumulativeGasUsed + + ", transactions=" + + transactions.stream().map(Transaction::toTraceLog).collect(Collectors.joining("; ")); + } } private final Supplier isCancelled; @@ -142,8 +158,13 @@ If running in a thread, it can be cancelled via the isCancelled supplier (which in this throwing an CancellationException). */ public TransactionSelectionResults buildTransactionListForBlock() { + LOG.debug("Transaction pool size {}", pendingTransactions.size()); + traceLambda( + LOG, "Transaction pool content {}", () -> pendingTransactions.toTraceLog(false, false)); pendingTransactions.selectTransactions( pendingTransaction -> evaluateTransaction(pendingTransaction)); + traceLambda( + LOG, "Transaction selection result result {}", transactionSelectionResult::toTraceLog); return transactionSelectionResult; } @@ -173,8 +194,10 @@ private TransactionSelectionResult evaluateTransaction(final Transaction transac } if (transactionTooLargeForBlock(transaction)) { - LOG.trace("{} too large to select for block creation", transaction); + traceLambda( + LOG, "Transaction {} too large to select for block creation", transaction::toTraceLog); if (blockOccupancyAboveThreshold()) { + traceLambda(LOG, "Block occupancy above threshold, completing operation"); return TransactionSelectionResult.COMPLETE_OPERATION; } else { return TransactionSelectionResult.CONTINUE; @@ -183,6 +206,7 @@ private TransactionSelectionResult evaluateTransaction(final Transaction transac // If the gas price specified by the transaction is less than this node is willing to accept, // do not include it in the block. + // ToDo: why we accept this in the pool in the first place then? final Wei actualMinTransactionGasPriceInBlock = feeMarket .getTransactionPriceCalculator() @@ -212,7 +236,7 @@ private TransactionSelectionResult evaluateTransaction(final Transaction transac validationResult.getErrorMessage(), processableBlockHeader.getParentHash().toHexString(), transaction.getHash().toHexString()); - return transactionSelectionResultForInvalidResult(validationResult); + return transactionSelectionResultForInvalidResult(transaction, validationResult); } else { // valid GoQuorum private tx, we need to hand craft the receipt and increment the nonce effectiveResult = publicResultForWhenWeHaveAPrivateTransaction(transaction); @@ -233,23 +257,34 @@ private TransactionSelectionResult evaluateTransaction(final Transaction transac if (!effectiveResult.isInvalid()) { worldStateUpdater.commit(); - LOG.trace("Selected {} for block creation", transaction); + traceLambda(LOG, "Selected {} for block creation", transaction::toTraceLog); updateTransactionResultTracking(transaction, effectiveResult); } else { - return transactionSelectionResultForInvalidResult(effectiveResult.getValidationResult()); + return transactionSelectionResultForInvalidResult( + transaction, effectiveResult.getValidationResult()); } return TransactionSelectionResult.CONTINUE; } private TransactionSelectionResult transactionSelectionResultForInvalidResult( + final Transaction transaction, final ValidationResult invalidReasonValidationResult) { // If the transaction has an incorrect nonce, leave it in the pool and continue if (invalidReasonValidationResult .getInvalidReason() .equals(TransactionInvalidReason.INCORRECT_NONCE)) { + traceLambda( + LOG, + "Incorrect nonce for transaction {} keeping it in the pool", + transaction::toTraceLog); return TransactionSelectionResult.CONTINUE; } // If the transaction was invalid for any other reason, delete it, and continue. + traceLambda( + LOG, + "Delete invalid transaction {}, reason {}", + transaction::toTraceLog, + invalidReasonValidationResult::getInvalidReason); return TransactionSelectionResult.DELETE_TRANSACTION_AND_CONTINUE; } @@ -318,6 +353,13 @@ private boolean transactionTooLargeForBlock(final Transaction transaction) { private boolean blockOccupancyAboveThreshold() { final double gasAvailable = processableBlockHeader.getGasLimit(); final double gasUsed = transactionSelectionResult.getCumulativeGasUsed(); - return (gasUsed / gasAvailable) >= minBlockOccupancyRatio; + final double occupancyRatio = gasUsed / gasAvailable; + LOG.trace( + "Min block occupancy ratio {}, gas used {}, available {}, used/available {}", + minBlockOccupancyRatio, + gasUsed, + gasAvailable, + occupancyRatio); + return occupancyRatio >= minBlockOccupancyRatio; } } diff --git a/ethereum/blockcreation/src/test/java/org/hyperledger/besu/ethereum/blockcreation/BlockTransactionSelectorTest.java b/ethereum/blockcreation/src/test/java/org/hyperledger/besu/ethereum/blockcreation/BlockTransactionSelectorTest.java index e5f6cdb995c..7071e6b9422 100644 --- a/ethereum/blockcreation/src/test/java/org/hyperledger/besu/ethereum/blockcreation/BlockTransactionSelectorTest.java +++ b/ethereum/blockcreation/src/test/java/org/hyperledger/besu/ethereum/blockcreation/BlockTransactionSelectorTest.java @@ -81,19 +81,20 @@ public class BlockTransactionSelectorTest { private final MetricsSystem metricsSystem = new NoOpMetricsSystem(); private final Blockchain blockchain = new ReferenceTestBlockchain(); - private final GasPricePendingTransactionsSorter pendingTransactions = - new GasPricePendingTransactionsSorter( - ImmutableTransactionPoolConfiguration.builder().txPoolMaxSize(5).build(), - TestClock.system(ZoneId.systemDefault()), - metricsSystem, - BlockTransactionSelectorTest::mockBlockHeader); - private final MutableWorldState worldState = - InMemoryKeyValueStorageProvider.createInMemoryWorldState(); + private GasPricePendingTransactionsSorter pendingTransactions; + private MutableWorldState worldState; @Mock private MainnetTransactionProcessor transactionProcessor; @Mock private MainnetTransactionValidator transactionValidator; @Before public void setup() { + worldState = InMemoryKeyValueStorageProvider.createInMemoryWorldState(); + pendingTransactions = + new GasPricePendingTransactionsSorter( + ImmutableTransactionPoolConfiguration.builder().txPoolMaxSize(5).build(), + TestClock.system(ZoneId.systemDefault()), + metricsSystem, + BlockTransactionSelectorTest::mockBlockHeader); when(transactionProcessor.getTransactionValidator()).thenReturn(transactionValidator); when(transactionValidator.getGoQuorumCompatibilityMode()).thenReturn(true); } @@ -158,7 +159,7 @@ public void emptyPendingTransactionsResultsInEmptyVettingResult() { @Test public void failedTransactionsAreIncludedInTheBlock() { final Transaction transaction = createTransaction(1); - pendingTransactions.addRemoteTransaction(transaction); + pendingTransactions.addRemoteTransaction(transaction, Optional.empty()); when(transactionProcessor.processTransaction( any(), any(), any(), eq(transaction), any(), any(), anyBoolean(), any())) @@ -199,7 +200,7 @@ public void invalidTransactionsTransactionProcessingAreSkippedButBlockStillFills for (int i = 0; i < 5; i++) { final Transaction tx = createTransaction(i); transactionsToInject.add(tx); - pendingTransactions.addRemoteTransaction(tx); + pendingTransactions.addRemoteTransaction(tx, Optional.empty()); } when(transactionProcessor.processTransaction( @@ -255,7 +256,7 @@ public void subsetOfPendingTransactionsIncludedWhenBlockGasLimitHit() { for (int i = 0; i < 5; i++) { final Transaction tx = createTransaction(i); transactionsToInject.add(tx); - pendingTransactions.addRemoteTransaction(tx); + pendingTransactions.addRemoteTransaction(tx, Optional.empty()); } when(transactionProcessor.processTransaction( @@ -317,7 +318,7 @@ public void transactionOfferingGasPriceLessThanMinimumIsIdentifiedAndRemovedFrom FeeMarket.legacy()); final Transaction tx = createTransaction(1); - pendingTransactions.addRemoteTransaction(tx); + pendingTransactions.addRemoteTransaction(tx, Optional.empty()); final BlockTransactionSelector.TransactionSelectionResults results = selector.buildTransactionListForBlock(); @@ -389,8 +390,8 @@ public void useSingleGasSpaceForAllTransactions() { TransactionProcessingResult.successful( new ArrayList<>(), 0, 0, Bytes.EMPTY, ValidationResult.valid())); - pendingTransactions1559.addRemoteTransaction(fillingLegacyTx); - pendingTransactions1559.addRemoteTransaction(extraEIP1559Tx); + pendingTransactions1559.addRemoteTransaction(fillingLegacyTx, Optional.empty()); + pendingTransactions1559.addRemoteTransaction(extraEIP1559Tx, Optional.empty()); final BlockTransactionSelector.TransactionSelectionResults results = selector.buildTransactionListForBlock(); @@ -441,7 +442,7 @@ public void transactionTooLargeForBlockDoesNotPreventMoreBeingAddedIfBlockOccupa .createTransaction(keyPair)); for (final Transaction tx : transactionsToInject) { - pendingTransactions.addRemoteTransaction(tx); + pendingTransactions.addRemoteTransaction(tx, Optional.empty()); } final BlockTransactionSelector.TransactionSelectionResults results = @@ -504,10 +505,10 @@ public void transactionSelectionStopsWhenSufficientBlockOccupancyIsReached() { .nonce(4) .createTransaction(keyPair); - pendingTransactions.addRemoteTransaction(transaction1); - pendingTransactions.addRemoteTransaction(transaction2); - pendingTransactions.addRemoteTransaction(transaction3); - pendingTransactions.addRemoteTransaction(transaction4); + pendingTransactions.addRemoteTransaction(transaction1, Optional.empty()); + pendingTransactions.addRemoteTransaction(transaction2, Optional.empty()); + pendingTransactions.addRemoteTransaction(transaction3, Optional.empty()); + pendingTransactions.addRemoteTransaction(transaction4, Optional.empty()); final BlockTransactionSelector.TransactionSelectionResults results = selector.buildTransactionListForBlock(); @@ -544,8 +545,8 @@ public void shouldDiscardTransactionsThatFailValidation() { final Transaction invalidTransaction = txTestFixture.nonce(2).gasLimit(2).createTransaction(keyPair); - pendingTransactions.addRemoteTransaction(validTransaction); - pendingTransactions.addRemoteTransaction(invalidTransaction); + pendingTransactions.addRemoteTransaction(validTransaction, Optional.empty()); + pendingTransactions.addRemoteTransaction(invalidTransaction, Optional.empty()); when(transactionProcessor.processTransaction( eq(blockchain), @@ -588,7 +589,7 @@ public void transactionWithIncorrectNonceRemainsInPoolAndNotSelected() { final Transaction futureTransaction = txTestFixture.nonce(5).gasLimit(1).createTransaction(keyPair); - pendingTransactions.addRemoteTransaction(futureTransaction); + pendingTransactions.addRemoteTransaction(futureTransaction, Optional.empty()); when(transactionProcessor.processTransaction( eq(blockchain), 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 184456c93e6..fc2bd1857bf 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 @@ -902,6 +902,23 @@ public String toString() { return sb.append("}").toString(); } + public String toTraceLog() { + final StringBuilder sb = new StringBuilder(); + sb.append(isContractCreation() ? "ContractCreation" : "MessageCall").append(", "); + sb.append(getSender()).append(", "); + sb.append(getType()).append(", "); + sb.append(getNonce()).append(", "); + getGasPrice().ifPresent(gasPrice -> sb.append(gasPrice.toBigInteger()).append(", ")); + if (getMaxPriorityFeePerGas().isPresent() && getMaxFeePerGas().isPresent()) { + sb.append(getMaxPriorityFeePerGas().map(Wei::toBigInteger).get()).append(", "); + sb.append(getMaxFeePerGas().map(Wei::toBigInteger).get()).append(", "); + } + sb.append(getGasLimit()).append(", "); + sb.append(getValue().toBigInteger()).append(", "); + if (getTo().isPresent()) sb.append(getTo().get()).append(", "); + return sb.append("}").toString(); + } + public Optional
contractAddress() { if (isContractCreation()) { return Optional.of(Address.contractAddress(getSender(), getNonce())); 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 e650fdd208f..c40c868f906 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 @@ -48,10 +48,10 @@ import org.hyperledger.besu.plugin.services.metrics.Counter; import org.hyperledger.besu.plugin.services.metrics.LabelledMetric; +import java.util.ArrayList; import java.util.Collection; -import java.util.HashSet; +import java.util.List; import java.util.Optional; -import java.util.Set; import org.slf4j.Logger; import org.slf4j.LoggerFactory; @@ -109,15 +109,14 @@ void handleConnect(final EthPeer peer) { public ValidationResult addLocalTransaction( final Transaction transaction) { - final ValidationResult validationResult = - validateLocalTransaction(transaction); - if (validationResult.isValid()) { + final ValidationResultAndAccount validationResult = validateLocalTransaction(transaction); + if (validationResult.result.isValid()) { if (!configuration.getTxFeeCap().isZero() && minTransactionGasPrice(transaction).compareTo(configuration.getTxFeeCap()) > 0) { return ValidationResult.invalid(TransactionInvalidReason.TX_FEECAP_EXCEEDED); } final TransactionAddedStatus transactionAddedStatus = - pendingTransactions.addLocalTransaction(transaction); + pendingTransactions.addLocalTransaction(transaction, validationResult.maybeAccount); if (!transactionAddedStatus.equals(ADDED)) { duplicateTransactionCounter.labels(LOCAL).inc(); return ValidationResult.invalid(transactionAddedStatus.getInvalidReason().orElseThrow()); @@ -126,7 +125,7 @@ && minTransactionGasPrice(transaction).compareTo(configuration.getTxFeeCap()) > transactionBroadcaster.onTransactionsAdded(txs); } - return validationResult; + return validationResult.result; } private boolean effectiveGasPriceIsAboveConfiguredMinGasPrice(final Transaction transaction) { @@ -139,35 +138,62 @@ private boolean effectiveGasPriceIsAboveConfiguredMinGasPrice(final Transaction } public void addRemoteTransactions(final Collection transactions) { - final Set addedTransactions = new HashSet<>(transactions.size()); + final List addedTransactions = new ArrayList<>(transactions.size()); + LOG.trace("Adding {} remote transactions", transactions.size()); for (final Transaction transaction : transactions) { if (pendingTransactions.containsTransaction(transaction.getHash())) { + traceLambda(LOG, "Discard already present transaction {}", transaction::toTraceLog); // We already have this transaction, don't even validate it. duplicateTransactionCounter.labels(REMOTE).inc(); continue; } final Wei transactionGasPrice = minTransactionGasPrice(transaction); if (transactionGasPrice.compareTo(miningParameters.getMinTransactionGasPrice()) < 0) { + traceLambda( + LOG, + "Discard transaction {} below min gas price {}", + transaction::toTraceLog, + miningParameters::getMinTransactionGasPrice); + pendingTransactions + .signalInvalidTransaction(transaction) + .forEach(pendingTransactions::removeTransaction); continue; } - final ValidationResult validationResult = - validateRemoteTransaction(transaction); - if (validationResult.isValid()) { - final boolean added = pendingTransactions.addRemoteTransaction(transaction); - if (added) { - addedTransactions.add(transaction); - } else { - duplicateTransactionCounter.labels(REMOTE).inc(); + final ValidationResultAndAccount validationResult = validateRemoteTransaction(transaction); + if (validationResult.result.isValid()) { + final TransactionAddedStatus status = + pendingTransactions.addRemoteTransaction(transaction, validationResult.maybeAccount); + switch (status) { + case ADDED: + traceLambda(LOG, "Added remote transaction {}", transaction::toTraceLog); + addedTransactions.add(transaction); + break; + case ALREADY_KNOWN: + traceLambda(LOG, "Duplicate remote transaction {}", transaction::toTraceLog); + duplicateTransactionCounter.labels(REMOTE).inc(); + break; + default: + traceLambda(LOG, "Transaction added status {}", status::name); } } else { - LOG.trace( - "Validation failed ({}) for transaction {}. Discarding.", - validationResult.getInvalidReason(), - transaction); + traceLambda( + LOG, + "Discard invalid transaction {}, reason {}", + transaction::toTraceLog, + validationResult.result::getInvalidReason); + pendingTransactions + .signalInvalidTransaction(transaction) + .forEach(pendingTransactions::removeTransaction); } } if (!addedTransactions.isEmpty()) { transactionBroadcaster.onTransactionsAdded(addedTransactions); + traceLambda( + LOG, + "Added {} transactions to the pool, current pool size {}, content {}", + addedTransactions::size, + pendingTransactions::size, + () -> pendingTransactions.toTraceLog(true, true)); } } @@ -189,9 +215,14 @@ public void unsubscribeDroppedTransactions(final long id) { @Override public void onBlockAdded(final BlockAddedEvent event) { + LOG.trace("Block added event {}", event); event.getAddedTransactions().forEach(pendingTransactions::transactionAddedToBlock); pendingTransactions.manageBlockAdded(event.getBlock()); - addRemoteTransactions(event.getRemovedTransactions()); + var readdTransactions = event.getRemovedTransactions(); + if (!readdTransactions.isEmpty()) { + LOG.trace("Readding {} transactions from a block event", readdTransactions.size()); + addRemoteTransactions(readdTransactions); + } } private MainnetTransactionValidator getTransactionValidator() { @@ -204,17 +235,15 @@ public AbstractPendingTransactionsSorter getPendingTransactions() { return pendingTransactions; } - private ValidationResult validateLocalTransaction( - final Transaction transaction) { + private ValidationResultAndAccount validateLocalTransaction(final Transaction transaction) { return validateTransaction(transaction, true); } - private ValidationResult validateRemoteTransaction( - final Transaction transaction) { + private ValidationResultAndAccount validateRemoteTransaction(final Transaction transaction) { return validateTransaction(transaction, false); } - private ValidationResult validateTransaction( + private ValidationResultAndAccount validateTransaction( final Transaction transaction, final boolean isLocal) { final BlockHeader chainHeadBlockHeader = getChainHeadBlockHeader().orElse(null); @@ -222,8 +251,8 @@ private ValidationResult validateTransaction( traceLambda( LOG, "rejecting transaction {} due to chain head not available yet", - () -> transaction.getHash()); - return ValidationResult.invalid(CHAIN_HEAD_NOT_AVAILABLE); + transaction::getHash); + return ValidationResultAndAccount.invalid(CHAIN_HEAD_NOT_AVAILABLE); } final FeeMarket feeMarket = @@ -234,7 +263,8 @@ private ValidationResult validateTransaction( if (transaction.isGoQuorumPrivateTransaction(goQuorumCompatibilityMode)) { final Optional weiValue = ofNullable(transaction.getValue()); if (weiValue.isPresent() && !weiValue.get().isZero()) { - return ValidationResult.invalid(TransactionInvalidReason.ETHER_VALUE_NOT_SUPPORTED); + return ValidationResultAndAccount.invalid( + TransactionInvalidReason.ETHER_VALUE_NOT_SUPPORTED); } } @@ -243,7 +273,7 @@ private ValidationResult validateTransaction( if ((!effectiveGasPriceIsAboveConfiguredMinGasPrice(transaction) && !miningParameters.isMiningEnabled()) || (!feeMarket.satisfiesFloorTxCost(transaction))) { - return ValidationResult.invalid(TransactionInvalidReason.GAS_PRICE_TOO_LOW); + return ValidationResultAndAccount.invalid(TransactionInvalidReason.GAS_PRICE_TOO_LOW); } final ValidationResult basicValidationResult = @@ -253,24 +283,25 @@ private ValidationResult validateTransaction( chainHeadBlockHeader.getBaseFee(), TransactionValidationParams.transactionPool()); if (!basicValidationResult.isValid()) { - return basicValidationResult; + return new ValidationResultAndAccount(basicValidationResult); } if (isLocal && strictReplayProtectionShouldBeEnforceLocally(chainHeadBlockHeader) && transaction.getChainId().isEmpty()) { // Strict replay protection is enabled but the tx is not replay-protected - return ValidationResult.invalid(TransactionInvalidReason.REPLAY_PROTECTED_SIGNATURE_REQUIRED); + return ValidationResultAndAccount.invalid( + TransactionInvalidReason.REPLAY_PROTECTED_SIGNATURE_REQUIRED); } if (transaction.getGasLimit() > chainHeadBlockHeader.getGasLimit()) { - return ValidationResult.invalid( + return ValidationResultAndAccount.invalid( TransactionInvalidReason.EXCEEDS_BLOCK_GAS_LIMIT, String.format( "Transaction gas limit of %s exceeds block gas limit of %s", transaction.getGasLimit(), chainHeadBlockHeader.getGasLimit())); } if (transaction.getType().equals(TransactionType.EIP1559) && !feeMarket.implementsBaseFee()) { - return ValidationResult.invalid( + return ValidationResultAndAccount.invalid( TransactionInvalidReason.INVALID_TRANSACTION_FORMAT, "EIP-1559 transaction are not allowed yet"); } @@ -282,17 +313,21 @@ && strictReplayProtectionShouldBeEnforceLocally(chainHeadBlockHeader) worldState -> { try { final Account senderAccount = worldState.get(transaction.getSender()); - return getTransactionValidator() - .validateForSender( - transaction, senderAccount, TransactionValidationParams.transactionPool()); + return new ValidationResultAndAccount( + senderAccount, + getTransactionValidator() + .validateForSender( + transaction, + senderAccount, + TransactionValidationParams.transactionPool())); } catch (MerkleTrieException ex) { LOG.debug( "MerkleTrieException while validating transaction for sender {}", transaction.getSender()); - return ValidationResult.invalid(CHAIN_HEAD_WORLD_STATE_NOT_AVAILABLE); + return ValidationResultAndAccount.invalid(CHAIN_HEAD_WORLD_STATE_NOT_AVAILABLE); } }) - .orElseGet(() -> ValidationResult.invalid(CHAIN_HEAD_WORLD_STATE_NOT_AVAILABLE)); + .orElseGet(() -> ValidationResultAndAccount.invalid(CHAIN_HEAD_WORLD_STATE_NOT_AVAILABLE)); } private boolean strictReplayProtectionShouldBeEnforceLocally( @@ -330,4 +365,29 @@ public interface TransactionBatchAddedListener { void onTransactionsAdded(Iterable transactions); } + + private static class ValidationResultAndAccount { + final ValidationResult result; + final Optional maybeAccount; + + ValidationResultAndAccount( + final Account account, final ValidationResult result) { + this.result = result; + this.maybeAccount = Optional.ofNullable(account); + } + + ValidationResultAndAccount(final ValidationResult result) { + this.result = result; + this.maybeAccount = Optional.empty(); + } + + static ValidationResultAndAccount invalid( + final TransactionInvalidReason reason, final String message) { + return new ValidationResultAndAccount(ValidationResult.invalid(reason, message)); + } + + static ValidationResultAndAccount invalid(final TransactionInvalidReason reason) { + return new ValidationResultAndAccount(ValidationResult.invalid(reason)); + } + } } diff --git a/ethereum/eth/src/main/java/org/hyperledger/besu/ethereum/eth/transactions/TransactionsForSenderInfo.java b/ethereum/eth/src/main/java/org/hyperledger/besu/ethereum/eth/transactions/TransactionsForSenderInfo.java index cfca069d84b..18c1adfb67a 100644 --- a/ethereum/eth/src/main/java/org/hyperledger/besu/ethereum/eth/transactions/TransactionsForSenderInfo.java +++ b/ethereum/eth/src/main/java/org/hyperledger/besu/ethereum/eth/transactions/TransactionsForSenderInfo.java @@ -15,27 +15,30 @@ package org.hyperledger.besu.ethereum.eth.transactions; -import org.hyperledger.besu.ethereum.eth.transactions.sorter.AbstractPendingTransactionsSorter; import org.hyperledger.besu.ethereum.eth.transactions.sorter.AbstractPendingTransactionsSorter.TransactionInfo; +import org.hyperledger.besu.evm.account.Account; import java.util.Map; import java.util.NavigableMap; import java.util.Optional; import java.util.OptionalLong; import java.util.TreeMap; +import java.util.stream.Collectors; import java.util.stream.Stream; public class TransactionsForSenderInfo { - private final NavigableMap - transactionsInfos; + private final NavigableMap transactionsInfos; private OptionalLong nextGap = OptionalLong.empty(); - public TransactionsForSenderInfo() { - transactionsInfos = new TreeMap<>(); + private Optional maybeSenderAccount; + + public TransactionsForSenderInfo(final Optional maybeSenderAccount) { + this.transactionsInfos = new TreeMap<>(); + this.maybeSenderAccount = maybeSenderAccount; } - public void addTransactionToTrack( - final long nonce, final AbstractPendingTransactionsSorter.TransactionInfo transactionInfo) { + public void addTransactionToTrack(final TransactionInfo transactionInfo) { + final long nonce = transactionInfo.getNonce(); synchronized (transactionsInfos) { if (!transactionsInfos.isEmpty()) { final long expectedNext = transactionsInfos.lastKey() + 1; @@ -59,6 +62,18 @@ public void removeTrackedTransaction(final long nonce) { } } + public void updateSenderAccount(final Optional maybeSenderAccount) { + this.maybeSenderAccount = maybeSenderAccount; + } + + public long getSenderAccountNonce() { + return maybeSenderAccount.map(Account::getNonce).orElse(0L); + } + + public Optional getSenderAccount() { + return maybeSenderAccount; + } + private void findGap() { // find first gap long expectedValue = transactionsInfos.firstKey(); @@ -97,4 +112,17 @@ public Stream streamTransactionInfos() { public TransactionInfo getTransactionInfoForNonce(final long nonce) { return transactionsInfos.get(nonce); } + + public String toTraceLog() { + return "{" + + "senderAccount " + + maybeSenderAccount + + ", transactions " + + transactionsInfos.entrySet().stream() + .map(e -> "(" + e.getKey() + ")" + e.getValue().toTraceLog()) + .collect(Collectors.joining("; ")) + + ", nextGap " + + nextGap + + '}'; + } } 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 8a0aad6d5d9..374de16b69c 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 @@ -15,7 +15,10 @@ package org.hyperledger.besu.ethereum.eth.transactions.sorter; import static org.hyperledger.besu.ethereum.eth.transactions.sorter.AbstractPendingTransactionsSorter.TransactionAddedStatus.ADDED; +import static org.hyperledger.besu.ethereum.eth.transactions.sorter.AbstractPendingTransactionsSorter.TransactionAddedStatus.LOWER_NONCE_INVALID_TRANSACTION_KNOWN; import static org.hyperledger.besu.ethereum.eth.transactions.sorter.AbstractPendingTransactionsSorter.TransactionAddedStatus.REJECTED_UNDERPRICED_REPLACEMENT; +import static org.hyperledger.besu.util.Slf4jLambdaHelper.debugLambda; +import static org.hyperledger.besu.util.Slf4jLambdaHelper.traceLambda; import org.hyperledger.besu.datatypes.Address; import org.hyperledger.besu.datatypes.Hash; @@ -30,6 +33,7 @@ import org.hyperledger.besu.ethereum.eth.transactions.TransactionPoolReplacementHandler; import org.hyperledger.besu.ethereum.eth.transactions.TransactionsForSenderInfo; import org.hyperledger.besu.ethereum.transaction.TransactionInvalidReason; +import org.hyperledger.besu.evm.account.Account; import org.hyperledger.besu.metrics.BesuMetricCategory; import org.hyperledger.besu.plugin.services.MetricsSystem; import org.hyperledger.besu.plugin.services.metrics.Counter; @@ -39,7 +43,6 @@ import java.time.Clock; import java.time.Instant; import java.time.temporal.ChronoUnit; -import java.util.ArrayList; import java.util.Collection; import java.util.HashMap; import java.util.HashSet; @@ -50,10 +53,15 @@ import java.util.Optional; import java.util.OptionalLong; import java.util.Set; +import java.util.Spliterator; +import java.util.Spliterators; +import java.util.TreeSet; import java.util.concurrent.ConcurrentHashMap; import java.util.concurrent.atomic.AtomicLong; +import java.util.function.Consumer; import java.util.function.Supplier; import java.util.stream.Collectors; +import java.util.stream.StreamSupport; import com.google.common.annotations.VisibleForTesting; import org.slf4j.Logger; @@ -66,6 +74,7 @@ *

This class is safe for use across multiple threads. */ public abstract class AbstractPendingTransactionsSorter { + private static final int DEFAULT_LOWEST_INVALID_KNOWN_NONCE_CACHE = 10_000; private static final Logger LOG = LoggerFactory.getLogger(AbstractPendingTransactionsSorter.class); @@ -78,6 +87,8 @@ public abstract class AbstractPendingTransactionsSorter { protected final Map transactionsBySender = new ConcurrentHashMap<>(); + protected final LowestInvalidNonceCache lowestInvalidKnownNonceCache = + new LowestInvalidNonceCache(DEFAULT_LOWEST_INVALID_KNOWN_NONCE_CACHE); protected final Subscribers pendingTransactionSubscribers = Subscribers.create(); @@ -133,7 +144,7 @@ public void evictOldTransactions() { .filter(transaction -> transaction.getAddedToPoolAt().isBefore(removeTransactionsBefore)) .forEach( transactionInfo -> { - LOG.trace("Evicted {} due to age", transactionInfo); + traceLambda(LOG, "Evicted {} due to age", transactionInfo::toTraceLog); removeTransaction(transactionInfo.getTransaction()); }); } @@ -145,21 +156,33 @@ public List getLocalTransactions() { .collect(Collectors.toList()); } - public boolean addRemoteTransaction(final Transaction transaction) { + public TransactionAddedStatus addRemoteTransaction( + final Transaction transaction, final Optional maybeSenderAccount) { + + if (lowestInvalidKnownNonceCache.hasInvalidLowerNonce(transaction)) { + debugLambda( + LOG, + "Dropping transaction {} since the sender has an invalid transaction with lower nonce", + transaction::toTraceLog); + return LOWER_NONCE_INVALID_TRANSACTION_KNOWN; + } + final TransactionInfo transactionInfo = new TransactionInfo(transaction, false, clock.instant()); - final TransactionAddedStatus transactionAddedStatus = addTransaction(transactionInfo); - final boolean added = transactionAddedStatus.equals(ADDED); - if (added) { + final TransactionAddedStatus transactionAddedStatus = + addTransaction(transactionInfo, maybeSenderAccount); + if (transactionAddedStatus.equals(ADDED)) { + lowestInvalidKnownNonceCache.registerValidTransaction(transaction); remoteTransactionAddedCounter.inc(); } - return added; + return transactionAddedStatus; } @VisibleForTesting - public TransactionAddedStatus addLocalTransaction(final Transaction transaction) { + public TransactionAddedStatus addLocalTransaction( + final Transaction transaction, final Optional maybeSenderAccount) { final TransactionAddedStatus transactionAdded = - addTransaction(new TransactionInfo(transaction, true, clock.instant())); + addTransaction(new TransactionInfo(transaction, true, clock.instant()), maybeSenderAccount); if (transactionAdded.equals(ADDED)) { localTransactionAddedCounter.inc(); } @@ -173,6 +196,7 @@ public void removeTransaction(final Transaction transaction) { public void transactionAddedToBlock(final Transaction transaction) { doRemoveTransaction(transaction, true); + lowestInvalidKnownNonceCache.registerValidTransaction(transaction); } protected void incrementTransactionRemovedCounter( @@ -191,7 +215,7 @@ protected void incrementTransactionRemovedCounter( // right now. public void selectTransactions(final TransactionSelector selector) { synchronized (lock) { - final List transactionsToRemove = new ArrayList<>(); + final Set transactionsToRemove = new HashSet<>(); final Map accountTransactions = new HashMap<>(); final Iterator prioritizedTransactions = prioritizedTransactions(); while (prioritizedTransactions.hasNext()) { @@ -208,6 +232,7 @@ public void selectTransactions(final TransactionSelector selector) { switch (result) { case DELETE_TRANSACTION_AND_CONTINUE: transactionsToRemove.add(transactionToProcess); + signalInvalidTransaction(transactionToProcess).forEach(transactionsToRemove::add); break; case CONTINUE: break; @@ -232,40 +257,48 @@ protected AccountTransactionOrder createSenderTransactionOrder(final Address add } protected TransactionAddedStatus addTransactionForSenderAndNonce( - final TransactionInfo transactionInfo) { - final TransactionInfo existingTransaction = - getTrackedTransactionBySenderAndNonce(transactionInfo); - if (existingTransaction != null) { + final TransactionInfo transactionInfo, final Optional maybeSenderAccount) { + + TransactionsForSenderInfo txsSenderInfo = + transactionsBySender.computeIfAbsent( + transactionInfo.getSender(), + address -> new TransactionsForSenderInfo(maybeSenderAccount)); + + TransactionInfo existingTxInfo = + txsSenderInfo.getTransactionInfoForNonce(transactionInfo.getNonce()); + + if (existingTxInfo != null) { if (!transactionReplacementHandler.shouldReplace( - existingTransaction, transactionInfo, chainHeadHeaderSupplier.get())) { + existingTxInfo, transactionInfo, chainHeadHeaderSupplier.get())) { + traceLambda( + LOG, "Reject underpriced transaction replacement {}", transactionInfo::toTraceLog); return REJECTED_UNDERPRICED_REPLACEMENT; } - removeTransaction(existingTransaction.getTransaction()); + traceLambda( + LOG, + "Replace existing transaction {}, with new transaction {}", + existingTxInfo::toTraceLog, + transactionInfo::toTraceLog); + removeTransaction(existingTxInfo.getTransaction()); } - trackTransactionBySenderAndNonce(transactionInfo); - return ADDED; - } - protected void trackTransactionBySenderAndNonce(final TransactionInfo transactionInfo) { - final TransactionsForSenderInfo transactionsForSenderInfo = - transactionsBySender.computeIfAbsent( - transactionInfo.getSender(), key -> new TransactionsForSenderInfo()); - transactionsForSenderInfo.addTransactionToTrack(transactionInfo.getNonce(), transactionInfo); + txsSenderInfo.updateSenderAccount(maybeSenderAccount); + txsSenderInfo.addTransactionToTrack(transactionInfo); + traceLambda(LOG, "Tracked transaction by sender {}", txsSenderInfo::toTraceLog); + return ADDED; } protected void removeTransactionTrackedBySenderAndNonce(final Transaction transaction) { Optional.ofNullable(transactionsBySender.get(transaction.getSender())) .ifPresent( - transactionsForSender -> - transactionsForSender.removeTrackedTransaction(transaction.getNonce())); - } - - protected TransactionInfo getTrackedTransactionBySenderAndNonce( - final TransactionInfo transactionInfo) { - final TransactionsForSenderInfo transactionsForSenderInfo = - transactionsBySender.computeIfAbsent( - transactionInfo.getSender(), key -> new TransactionsForSenderInfo()); - return transactionsForSenderInfo.getTransactionInfoForNonce(transactionInfo.getNonce()); + transactionsForSender -> { + transactionsForSender.removeTrackedTransaction(transaction.getNonce()); + traceLambda( + LOG, + "Tracked transaction by sender {} after the removal of {}", + transactionsForSender::toTraceLog, + transaction::toTraceLog); + }); } protected void notifyTransactionAdded(final Transaction transaction) { @@ -327,7 +360,86 @@ protected abstract void doRemoveTransaction( protected abstract Iterator prioritizedTransactions(); - protected abstract TransactionAddedStatus addTransaction(final TransactionInfo transactionInfo); + protected abstract TransactionAddedStatus addTransaction( + final TransactionInfo transactionInfo, final Optional maybeSenderAccount); + + Optional lowestValueTxForRemovalBySender( + final NavigableSet txSet) { + return txSet.descendingSet().stream() + .filter( + tx -> + transactionsBySender + .get(tx.getSender()) + .maybeLastTx() + .filter(tx::equals) + .isPresent()) + .findFirst(); + } + + public String toTraceLog( + final boolean withTransactionsBySender, final boolean withLowestInvalidNonce) { + synchronized (lock) { + StringBuilder sb = + new StringBuilder( + "Transactions in order { " + + StreamSupport.stream( + Spliterators.spliteratorUnknownSize( + prioritizedTransactions(), Spliterator.ORDERED), + false) + .map( + txInfo -> { + TransactionsForSenderInfo txsSenderInfo = + transactionsBySender.get(txInfo.getSender()); + long nonceDistance = + txInfo.getNonce() - txsSenderInfo.getSenderAccountNonce(); + return "nonceDistance: " + + nonceDistance + + ", senderAccount: " + + txsSenderInfo.getSenderAccount() + + ", " + + txInfo.toTraceLog(); + }) + .collect(Collectors.joining("; ")) + + " }"); + + if (withTransactionsBySender) { + sb.append( + ", Transactions by sender { " + + transactionsBySender.entrySet().stream() + .map(e -> "(" + e.getKey() + ") " + e.getValue().toTraceLog()) + .collect(Collectors.joining("; ")) + + " }"); + } + if (withLowestInvalidNonce) { + sb.append( + ", Lowest invalid nonce by sender cache {" + + lowestInvalidKnownNonceCache.toTraceLog() + + "}"); + } + return sb.toString(); + } + } + + public List signalInvalidTransaction(final Transaction transaction) { + final long invalidNonce = lowestInvalidKnownNonceCache.registerInvalidTransaction(transaction); + + TransactionsForSenderInfo txsForSender = transactionsBySender.get(transaction.getSender()); + if (txsForSender != null) { + return txsForSender + .streamTransactionInfos() + .filter(txInfo -> txInfo.getTransaction().getNonce() > invalidNonce) + .peek( + txInfo -> + traceLambda( + LOG, + "Transaction {} piked for removal since there is a lowest invalid nonce {} for the sender", + txInfo::toTraceLog, + () -> invalidNonce)) + .map(TransactionInfo::getTransaction) + .collect(Collectors.toList()); + } + return List.of(); + } /** * Tracks the additional metadata associated with transactions to enable prioritization for mining @@ -389,6 +501,16 @@ public static List toTransactionList( .map(TransactionInfo::getTransaction) .collect(Collectors.toUnmodifiableList()); } + + public String toTraceLog() { + return "{sequence: " + + sequence + + ", addedAt: " + + addedToPoolAt + + ", " + + transaction.toTraceLog() + + "}"; + } } public enum TransactionSelectionResult { @@ -406,6 +528,8 @@ public interface TransactionSelector { public enum TransactionAddedStatus { ALREADY_KNOWN(TransactionInvalidReason.TRANSACTION_ALREADY_KNOWN), REJECTED_UNDERPRICED_REPLACEMENT(TransactionInvalidReason.TRANSACTION_REPLACEMENT_UNDERPRICED), + NONCE_TOO_FAR_IN_FUTURE_FOR_SENDER(), + LOWER_NONCE_INVALID_TRANSACTION_KNOWN(), ADDED(); private final Optional invalidReason; @@ -423,15 +547,196 @@ public Optional getInvalidReason() { } } - Optional lowestValueTxForRemovalBySender(NavigableSet txSet) { - return txSet.descendingSet().stream() - .filter( - tx -> - transactionsBySender - .get(tx.getSender()) - .maybeLastTx() - .filter(tx::equals) - .isPresent()) - .findFirst(); + private static class LowestInvalidNonceCache { + private final int maxSize; + private final Map lowestInvalidKnownNonceBySender; + private final NavigableSet evictionOrder = new TreeSet<>(); + + public LowestInvalidNonceCache(final int maxSize) { + this.maxSize = maxSize; + this.lowestInvalidKnownNonceBySender = new HashMap<>(maxSize); + } + + synchronized long registerInvalidTransaction(final Transaction transaction) { + final Address sender = transaction.getSender(); + final long invalidNonce = transaction.getNonce(); + final InvalidNonceStatus currStatus = lowestInvalidKnownNonceBySender.get(sender); + if (currStatus == null) { + final InvalidNonceStatus newStatus = new InvalidNonceStatus(sender, invalidNonce); + addInvalidNonceStatus(newStatus); + traceLambda( + LOG, + "Added invalid nonce status {}, cache status {}", + newStatus::toString, + this::toString); + return invalidNonce; + } + + updateInvalidNonceStatus( + currStatus, + status -> { + if (invalidNonce < currStatus.nonce) { + currStatus.updateNonce(invalidNonce); + } else { + currStatus.newHit(); + } + }); + traceLambda( + LOG, + "Updated invalid nonce status {}, cache status {}", + currStatus::toString, + this::toString); + + return currStatus.nonce; + } + + synchronized void registerValidTransaction(final Transaction transaction) { + final InvalidNonceStatus currStatus = + lowestInvalidKnownNonceBySender.get(transaction.getSender()); + if (currStatus != null) { + evictionOrder.remove(currStatus); + lowestInvalidKnownNonceBySender.remove(transaction.getSender()); + traceLambda( + LOG, + "Valid transaction, removed invalid nonce status {}, cache status {}", + currStatus::toString, + this::toString); + } + } + + synchronized boolean hasInvalidLowerNonce(final Transaction transaction) { + final InvalidNonceStatus currStatus = + lowestInvalidKnownNonceBySender.get(transaction.getSender()); + if (currStatus != null && transaction.getNonce() > currStatus.nonce) { + updateInvalidNonceStatus(currStatus, status -> status.newHit()); + traceLambda( + LOG, + "New hit for invalid nonce status {}, cache status {}", + currStatus::toString, + this::toString); + return true; + } + return false; + } + + private void updateInvalidNonceStatus( + final InvalidNonceStatus status, final Consumer updateAction) { + evictionOrder.remove(status); + updateAction.accept(status); + evictionOrder.add(status); + } + + private void addInvalidNonceStatus(final InvalidNonceStatus newStatus) { + if (lowestInvalidKnownNonceBySender.size() >= maxSize) { + final InvalidNonceStatus statusToEvict = evictionOrder.pollFirst(); + lowestInvalidKnownNonceBySender.remove(statusToEvict.address); + traceLambda( + LOG, + "Evicted invalid nonce status {}, cache status {}", + statusToEvict::toString, + this::toString); + } + lowestInvalidKnownNonceBySender.put(newStatus.address, newStatus); + evictionOrder.add(newStatus); + } + + synchronized String toTraceLog() { + return "by eviction order " + + StreamSupport.stream(evictionOrder.spliterator(), false) + .map(InvalidNonceStatus::toString) + .collect(Collectors.joining("; ")); + } + + @Override + public String toString() { + return "LowestInvalidNonceCache{" + + "maxSize: " + + maxSize + + ", currentSize: " + + lowestInvalidKnownNonceBySender.size() + + ", evictionOrder: [size: " + + evictionOrder.size() + + ", first evictable: " + + evictionOrder.first() + + "]" + + '}'; + } + + private static class InvalidNonceStatus implements Comparable { + final Address address; + long nonce; + long hits; + long lastUpdate; + + InvalidNonceStatus(final Address address, final long nonce) { + this.address = address; + this.nonce = nonce; + this.hits = 1L; + this.lastUpdate = System.currentTimeMillis(); + } + + void updateNonce(final long nonce) { + this.nonce = nonce; + newHit(); + } + + void newHit() { + this.hits++; + this.lastUpdate = System.currentTimeMillis(); + } + + @Override + public boolean equals(final Object o) { + if (this == o) { + return true; + } + if (o == null || getClass() != o.getClass()) { + return false; + } + + InvalidNonceStatus that = (InvalidNonceStatus) o; + + return address.equals(that.address); + } + + @Override + public int hashCode() { + return address.hashCode(); + } + + /** + * An InvalidNonceStatus is smaller than another when it has fewer hits and was last access + * earlier, the address is the last tiebreaker + * + * @param o the object to be compared. + * @return 0 if they are equal, negative if this is smaller, positive if this is greater + */ + @Override + public int compareTo(final InvalidNonceStatus o) { + final int cmpHits = Long.compare(this.hits, o.hits); + if (cmpHits != 0) { + return cmpHits; + } + final int cmpLastUpdate = Long.compare(this.lastUpdate, o.lastUpdate); + if (cmpLastUpdate != 0) { + return cmpLastUpdate; + } + return this.address.compareTo(o.address); + } + + @Override + public String toString() { + return "{" + + "address=" + + address + + ", nonce=" + + nonce + + ", hits=" + + hits + + ", lastUpdate=" + + lastUpdate + + '}'; + } + } } } diff --git a/ethereum/eth/src/main/java/org/hyperledger/besu/ethereum/eth/transactions/sorter/BaseFeePendingTransactionsSorter.java b/ethereum/eth/src/main/java/org/hyperledger/besu/ethereum/eth/transactions/sorter/BaseFeePendingTransactionsSorter.java index be5a064b3cb..4e3c9aaf99c 100644 --- a/ethereum/eth/src/main/java/org/hyperledger/besu/ethereum/eth/transactions/sorter/BaseFeePendingTransactionsSorter.java +++ b/ethereum/eth/src/main/java/org/hyperledger/besu/ethereum/eth/transactions/sorter/BaseFeePendingTransactionsSorter.java @@ -18,12 +18,17 @@ import static java.util.stream.Collectors.toUnmodifiableList; import static org.hyperledger.besu.ethereum.eth.transactions.sorter.AbstractPendingTransactionsSorter.TransactionAddedStatus.ADDED; import static org.hyperledger.besu.ethereum.eth.transactions.sorter.AbstractPendingTransactionsSorter.TransactionAddedStatus.ALREADY_KNOWN; +import static org.hyperledger.besu.ethereum.eth.transactions.sorter.AbstractPendingTransactionsSorter.TransactionAddedStatus.NONCE_TOO_FAR_IN_FUTURE_FOR_SENDER; +import static org.hyperledger.besu.util.Slf4jLambdaHelper.traceLambda; import org.hyperledger.besu.datatypes.Wei; import org.hyperledger.besu.ethereum.core.Block; import org.hyperledger.besu.ethereum.core.BlockHeader; import org.hyperledger.besu.ethereum.core.Transaction; import org.hyperledger.besu.ethereum.eth.transactions.TransactionPoolConfiguration; +import org.hyperledger.besu.ethereum.eth.transactions.TransactionsForSenderInfo; +import org.hyperledger.besu.evm.account.Account; +import org.hyperledger.besu.evm.account.AccountState; import org.hyperledger.besu.plugin.services.MetricsSystem; import java.time.Clock; @@ -36,6 +41,7 @@ import java.util.function.Supplier; import java.util.stream.Stream; +import com.google.errorprone.annotations.Keep; import org.slf4j.Logger; import org.slf4j.LoggerFactory; @@ -77,6 +83,7 @@ public BaseFeePendingTransactionsSorter( .getAsBigInteger() .longValue()) .thenComparing(TransactionInfo::getAddedToPoolAt) + .thenComparing(TransactionInfo::getSequence) .reversed()); private final NavigableSet prioritizedTransactionsDynamicRange = @@ -90,8 +97,15 @@ public BaseFeePendingTransactionsSorter( .map(maxFeePerGas -> maxFeePerGas.getAsBigInteger().longValue()) .orElse(transactionInfo.getGasPrice().toLong())) .thenComparing(TransactionInfo::getAddedToPoolAt) + .thenComparing(TransactionInfo::getSequence) .reversed()); + private final TreeSet transactionsByEvictionOrder = + new TreeSet<>( + comparing(TransactionInfo::isReceivedFromLocalSource) + .reversed() + .thenComparing(TransactionInfo::getSequence)); + @Override public void manageBlockAdded(final Block block) { block.getHeader().getBaseFee().ifPresent(this::updateBaseFee); @@ -103,11 +117,24 @@ protected void doRemoveTransaction(final Transaction transaction, final boolean final TransactionInfo removedTransactionInfo = pendingTransactions.remove(transaction.getHash()); if (removedTransactionInfo != null) { - if (!prioritizedTransactionsDynamicRange.remove(removedTransactionInfo)) + transactionsByEvictionOrder.remove(removedTransactionInfo); + if (prioritizedTransactionsDynamicRange.remove(removedTransactionInfo)) { + traceLambda( + LOG, "Removed dynamic range transaction {}", removedTransactionInfo::toTraceLog); + } else { removedTransactionInfo .getTransaction() .getMaxPriorityFeePerGas() - .ifPresent(__ -> prioritizedTransactionsStaticRange.remove(removedTransactionInfo)); + .ifPresent( + __ -> { + if (prioritizedTransactionsStaticRange.remove(removedTransactionInfo)) { + traceLambda( + LOG, + "Removed static range transaction {}", + removedTransactionInfo::toTraceLog); + } + }); + } removeTransactionTrackedBySenderAndNonce(transaction); incrementTransactionRemovedCounter( removedTransactionInfo.isReceivedFromLocalSource(), addedToBlock); @@ -149,7 +176,7 @@ public TransactionInfo next() { currentStaticRangeTransaction = getNextOptional(staticRangeIterable); return best; } else { - // there are both static and dynamic txs remaining so we need to compare them by their + // there are both static and dynamic txs remaining, so we need to compare them by their // effective priority fees final Wei dynamicRangeEffectivePriorityFee = currentDynamicRangeTransaction @@ -183,62 +210,72 @@ private Optional getNextOptional( } @Override - protected TransactionAddedStatus addTransaction(final TransactionInfo transactionInfo) { + protected TransactionAddedStatus addTransaction( + final TransactionInfo transactionInfo, final Optional maybeSenderAccount) { Optional droppedTransaction = Optional.empty(); final Transaction transaction = transactionInfo.getTransaction(); synchronized (lock) { if (pendingTransactions.containsKey(transactionInfo.getHash())) { + traceLambda(LOG, "Already known transaction {}", transactionInfo::toTraceLog); return ALREADY_KNOWN; } + if (transaction.getNonce() - maybeSenderAccount.map(AccountState::getNonce).orElse(0L) + >= poolConfig.getTxPoolMaxFutureTransactionByAccount()) { + traceLambda( + LOG, + "Transaction {} not added because nonce too far in the future for sender {}", + transaction::toTraceLog, + maybeSenderAccount::toString); + return NONCE_TOO_FAR_IN_FUTURE_FOR_SENDER; + } + final TransactionAddedStatus transactionAddedStatus = - addTransactionForSenderAndNonce(transactionInfo); + addTransactionForSenderAndNonce(transactionInfo, maybeSenderAccount); if (!transactionAddedStatus.equals(ADDED)) { + traceLambda( + LOG, + "Not added with status {}, transaction {}", + transactionAddedStatus::name, + transactionInfo::toTraceLog); return transactionAddedStatus; } // check if it's in static or dynamic range + final String kind; if (isInStaticRange(transaction, baseFee)) { + kind = "static"; prioritizedTransactionsStaticRange.add(transactionInfo); } else { + kind = "dynamic"; prioritizedTransactionsDynamicRange.add(transactionInfo); } - LOG.trace("Adding {} to pending transactions", transactionInfo); + traceLambda( + LOG, + "Adding {} to pending transactions, range type {}", + transactionInfo::toTraceLog, + kind::toString); pendingTransactions.put(transactionInfo.getHash(), transactionInfo); + transactionsByEvictionOrder.add(transactionInfo); - // check if this sender exceeds the transactions by sender limit: - var senderTxInfos = transactionsBySender.get(transactionInfo.getSender()); - if (senderTxInfos.transactionCount() > poolConfig.getTxPoolMaxFutureTransactionByAccount()) { - droppedTransaction = senderTxInfos.maybeLastTx().map(TransactionInfo::getTransaction); - droppedTransaction.ifPresent( - tx -> LOG.trace("Evicted {} due to too many transactions from sender", tx)); - - } else { - // else if we are over txpool limit, select the lowest value transaction to evict - if (pendingTransactions.size() > poolConfig.getTxPoolMaxSize()) { - final Stream.Builder removalCandidates = Stream.builder(); - if (!prioritizedTransactionsDynamicRange.isEmpty()) - lowestValueTxForRemovalBySender(prioritizedTransactionsDynamicRange) - .ifPresent(removalCandidates::add); - if (!prioritizedTransactionsStaticRange.isEmpty()) - lowestValueTxForRemovalBySender(prioritizedTransactionsStaticRange) - .ifPresent(removalCandidates::add); + // if we are over txpool limit, select a transaction to evict + if (pendingTransactions.size() > poolConfig.getTxPoolMaxSize()) { + LOG.trace( + "Tx pool size {} over limit {} selecting a transaction to evict", + pendingTransactions.size(), + poolConfig.getTxPoolMaxSize()); + droppedTransaction = getTransactionToEvict(); - droppedTransaction = - removalCandidates - .build() - .min( - Comparator.comparing( - txInfo -> txInfo.getTransaction().getEffectivePriorityFeePerGas(baseFee))) - .map(TransactionInfo::getTransaction); - } + droppedTransaction.ifPresent( + toRemove -> { + doRemoveTransaction(toRemove, false); + traceLambda( + LOG, + "Evicted transaction {} due to transaction pool size, effective price {}", + toRemove::toTraceLog, + () -> toRemove.getEffectivePriorityFeePerGas(baseFee)); + }); } - - droppedTransaction.ifPresent( - toRemove -> { - doRemoveTransaction(toRemove, false); - LOG.trace("Evicted {} due to transaction pool size", toRemove); - }); } notifyTransactionAdded(transaction); @@ -246,6 +283,66 @@ protected TransactionAddedStatus addTransaction(final TransactionInfo transactio return ADDED; } + private Optional getTransactionToEvict() { + // select transaction to drop by lowest sequence and then by max nonce for the sender + final TransactionInfo firstTransactionInfo = transactionsByEvictionOrder.first(); + final TransactionsForSenderInfo transactionsForSenderInfo = + transactionsBySender.get(firstTransactionInfo.getSender()); + traceLambda( + LOG, + "Oldest transaction info {} will pick transaction with highest nonce for that sender {}", + firstTransactionInfo::toTraceLog, + transactionsForSenderInfo::toTraceLog); + return transactionsForSenderInfo.maybeLastTx().map(TransactionInfo::getTransaction); + } + + @Keep + private Optional selectLowestValueTransaction() { + Optional droppedTransaction; + final Stream.Builder removalCandidates = Stream.builder(); + if (!prioritizedTransactionsDynamicRange.isEmpty()) + lowestValueTxForRemovalBySender(prioritizedTransactionsDynamicRange) + .ifPresent( + tx -> { + traceLambda( + LOG, + "Selected for removal dynamic range transaction {} effective price {}", + tx::toTraceLog, + () -> + tx.getTransaction() + .getEffectivePriorityFeePerGas(baseFee) + .getAsBigInteger()); + removalCandidates.add(tx); + }); + if (!prioritizedTransactionsStaticRange.isEmpty()) + lowestValueTxForRemovalBySender(prioritizedTransactionsStaticRange) + .ifPresent( + tx -> { + traceLambda( + LOG, + "Selected for removal static range transaction {} effective price {}", + tx::toTraceLog, + () -> + tx.getTransaction() + .getEffectivePriorityFeePerGas(baseFee) + .getAsBigInteger()); + removalCandidates.add(tx); + }); + + droppedTransaction = + removalCandidates + .build() + .min( + Comparator.comparing( + txInfo -> + txInfo + .getTransaction() + .getEffectivePriorityFeePerGas(baseFee) + .getAsBigInteger())) + .map(TransactionInfo::getTransaction); + return droppedTransaction; + } + private boolean isInStaticRange(final Transaction transaction, final Optional baseFee) { return transaction .getMaxPriorityFeePerGas() @@ -259,7 +356,11 @@ private boolean isInStaticRange(final Transaction transaction, final Optional { - LOG.trace("Moving {} from static to dynamic gas fee paradigm", transactionInfo); + traceLambda( + LOG, + "Moving {} from static to dynamic gas fee paradigm", + transactionInfo::toTraceLog); prioritizedTransactionsStaticRange.remove(transactionInfo); prioritizedTransactionsDynamicRange.add(transactionInfo); }); @@ -290,7 +394,10 @@ public void updateBaseFee(final Wei newBaseFee) { .collect(toUnmodifiableList()) .forEach( transactionInfo -> { - LOG.trace("Moving {} from dynamic to static gas fee paradigm", transactionInfo); + traceLambda( + LOG, + "Moving {} from dynamic to static gas fee paradigm", + transactionInfo::toTraceLog); prioritizedTransactionsDynamicRange.remove(transactionInfo); prioritizedTransactionsStaticRange.add(transactionInfo); }); 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 bc5d6cb2435..a0d31ecada5 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 @@ -20,6 +20,7 @@ import org.hyperledger.besu.ethereum.core.BlockHeader; import org.hyperledger.besu.ethereum.core.Transaction; import org.hyperledger.besu.ethereum.eth.transactions.TransactionPoolConfiguration; +import org.hyperledger.besu.evm.account.Account; import org.hyperledger.besu.plugin.services.MetricsSystem; import java.time.Clock; @@ -47,6 +48,7 @@ public class GasPricePendingTransactionsSorter extends AbstractPendingTransactio comparing(TransactionInfo::isReceivedFromLocalSource) .thenComparing(TransactionInfo::getGasPrice) .thenComparing(TransactionInfo::getAddedToPoolAt) + .thenComparing(TransactionInfo::getSequence) .reversed()); public GasPricePendingTransactionsSorter( @@ -82,7 +84,8 @@ protected Iterator prioritizedTransactions() { } @Override - protected TransactionAddedStatus addTransaction(final TransactionInfo transactionInfo) { + protected TransactionAddedStatus addTransaction( + final TransactionInfo transactionInfo, final Optional maybeSenderAccount) { Optional droppedTransaction = Optional.empty(); synchronized (lock) { if (pendingTransactions.containsKey(transactionInfo.getHash())) { @@ -90,7 +93,7 @@ protected TransactionAddedStatus addTransaction(final TransactionInfo transactio } final TransactionAddedStatus transactionAddedStatus = - addTransactionForSenderAndNonce(transactionInfo); + addTransactionForSenderAndNonce(transactionInfo, maybeSenderAccount); if (!transactionAddedStatus.equals(TransactionAddedStatus.ADDED)) { return transactionAddedStatus; } diff --git a/ethereum/eth/src/test/java/org/hyperledger/besu/ethereum/eth/manager/task/GetPooledTransactionsFromPeerTaskTest.java b/ethereum/eth/src/test/java/org/hyperledger/besu/ethereum/eth/manager/task/GetPooledTransactionsFromPeerTaskTest.java index 665f7d00ee3..890125f1762 100644 --- a/ethereum/eth/src/test/java/org/hyperledger/besu/ethereum/eth/manager/task/GetPooledTransactionsFromPeerTaskTest.java +++ b/ethereum/eth/src/test/java/org/hyperledger/besu/ethereum/eth/manager/task/GetPooledTransactionsFromPeerTaskTest.java @@ -50,7 +50,7 @@ protected List generateDataToBeRequested() { .gasLimit(100000) .chainId(Optional.empty()) .createTransaction(keyPair); - assertThat(transactionPool.getPendingTransactions().addLocalTransaction(tx)) + assertThat(transactionPool.getPendingTransactions().addLocalTransaction(tx, Optional.empty())) .isEqualTo(TransactionAddedStatus.ADDED); requestedData.add(tx); } diff --git a/ethereum/eth/src/test/java/org/hyperledger/besu/ethereum/eth/transactions/BaseFeePendingTransactionsTest.java b/ethereum/eth/src/test/java/org/hyperledger/besu/ethereum/eth/transactions/BaseFeePendingTransactionsTest.java index a14dab7d6d2..809001a130a 100644 --- a/ethereum/eth/src/test/java/org/hyperledger/besu/ethereum/eth/transactions/BaseFeePendingTransactionsTest.java +++ b/ethereum/eth/src/test/java/org/hyperledger/besu/ethereum/eth/transactions/BaseFeePendingTransactionsTest.java @@ -15,6 +15,9 @@ package org.hyperledger.besu.ethereum.eth.transactions; import static org.assertj.core.api.Assertions.assertThat; +import static org.hyperledger.besu.ethereum.eth.transactions.sorter.AbstractPendingTransactionsSorter.TransactionAddedStatus.ADDED; +import static org.hyperledger.besu.ethereum.eth.transactions.sorter.AbstractPendingTransactionsSorter.TransactionAddedStatus.ALREADY_KNOWN; +import static org.hyperledger.besu.ethereum.eth.transactions.sorter.AbstractPendingTransactionsSorter.TransactionAddedStatus.REJECTED_UNDERPRICED_REPLACEMENT; import static org.mockito.Mockito.mock; import static org.mockito.Mockito.verify; import static org.mockito.Mockito.verifyNoInteractions; @@ -35,6 +38,7 @@ import org.hyperledger.besu.ethereum.eth.transactions.sorter.AbstractPendingTransactionsSorter.TransactionAddedStatus; import org.hyperledger.besu.ethereum.eth.transactions.sorter.AbstractPendingTransactionsSorter.TransactionSelectionResult; import org.hyperledger.besu.ethereum.eth.transactions.sorter.BaseFeePendingTransactionsSorter; +import org.hyperledger.besu.evm.account.Account; import org.hyperledger.besu.metrics.StubMetricsSystem; import org.hyperledger.besu.testutil.TestClock; @@ -95,13 +99,13 @@ public class BaseFeePendingTransactionsTest { @Test public void shouldReturnExclusivelyLocalTransactionsWhenAppropriate() { final Transaction localTransaction0 = createTransaction(0); - transactions.addLocalTransaction(localTransaction0); + transactions.addLocalTransaction(localTransaction0, Optional.empty()); assertThat(transactions.size()).isEqualTo(1); - transactions.addRemoteTransaction(transaction1); + transactions.addRemoteTransaction(transaction1, Optional.empty()); assertThat(transactions.size()).isEqualTo(2); - transactions.addRemoteTransaction(transaction2); + transactions.addRemoteTransaction(transaction2, Optional.empty()); assertThat(transactions.size()).isEqualTo(3); final List localTransactions = transactions.getLocalTransactions(); @@ -110,11 +114,11 @@ public void shouldReturnExclusivelyLocalTransactionsWhenAppropriate() { @Test public void shouldAddATransaction() { - transactions.addRemoteTransaction(transaction1); + transactions.addRemoteTransaction(transaction1, Optional.empty()); assertThat(transactions.size()).isEqualTo(1); assertThat(metricsSystem.getCounterValue(ADDED_COUNTER, REMOTE)).isEqualTo(1); - transactions.addRemoteTransaction(transaction2); + transactions.addRemoteTransaction(transaction2, Optional.empty()); assertThat(transactions.size()).isEqualTo(2); assertThat(metricsSystem.getCounterValue(ADDED_COUNTER, REMOTE)).isEqualTo(2); } @@ -126,7 +130,7 @@ public void shouldReturnEmptyOptionalWhenNoTransactionWithGivenHashExists() { @Test public void shouldGetTransactionByHash() { - transactions.addRemoteTransaction(transaction1); + transactions.addRemoteTransaction(transaction1, Optional.empty()); assertTransactionPending(transaction1); } @@ -134,83 +138,101 @@ public void shouldGetTransactionByHash() { public void shouldDropOldestTransactionWhenLimitExceeded() { final Transaction oldestTransaction = transactionWithNonceSenderAndGasPrice(0, SIGNATURE_ALGORITHM.get().generateKeyPair(), 10L); - senderLimitedTransactions.addRemoteTransaction(oldestTransaction); + final Account oldestSender = mock(Account.class); + when(oldestSender.getNonce()).thenReturn(0L); + senderLimitedTransactions.addRemoteTransaction(oldestTransaction, Optional.of(oldestSender)); for (int i = 1; i < MAX_TRANSACTIONS; i++) { + final Account sender = mock(Account.class); + when(sender.getNonce()).thenReturn((long) i); senderLimitedTransactions.addRemoteTransaction( transactionWithNonceSenderAndGasPrice( - i, SIGNATURE_ALGORITHM.get().generateKeyPair(), 10L)); + i, SIGNATURE_ALGORITHM.get().generateKeyPair(), 10L), + Optional.of(sender)); } assertThat(senderLimitedTransactions.size()).isEqualTo(MAX_TRANSACTIONS); assertThat(metricsSystem.getCounterValue(REMOVED_COUNTER, REMOTE, DROPPED)).isZero(); - senderLimitedTransactions.addRemoteTransaction(createTransaction(MAX_TRANSACTIONS + 1)); + final Account lastSender = mock(Account.class); + when(lastSender.getNonce()).thenReturn(6L); + senderLimitedTransactions.addRemoteTransaction( + createTransaction(MAX_TRANSACTIONS + 1), Optional.of(lastSender)); assertThat(senderLimitedTransactions.size()).isEqualTo(MAX_TRANSACTIONS); assertTransactionNotPending(oldestTransaction); assertThat(metricsSystem.getCounterValue(REMOVED_COUNTER, REMOTE, DROPPED)).isEqualTo(1); } @Test - public void shouldDropFutureTransactionWhenSenderLimitExceeded() { + public void shouldDropTransactionWithATooFarNonce() { Transaction furthestFutureTransaction = null; for (int i = 0; i < MAX_TRANSACTIONS; i++) { furthestFutureTransaction = transactionWithNonceSenderAndGasPrice(i, KEYS1, 10L); - senderLimitedTransactions.addRemoteTransaction(furthestFutureTransaction); + senderLimitedTransactions.addRemoteTransaction(furthestFutureTransaction, Optional.empty()); } assertThat(senderLimitedTransactions.size()).isEqualTo(MAX_TRANSACTIONS_BY_SENDER); - assertThat(metricsSystem.getCounterValue(REMOVED_COUNTER, REMOTE, DROPPED)).isEqualTo(1L); assertThat(senderLimitedTransactions.getTransactionByHash(furthestFutureTransaction.getHash())) .isEmpty(); } @Test public void shouldHandleMaximumTransactionLimitCorrectlyWhenSameTransactionAddedMultipleTimes() { - transactions.addRemoteTransaction(createTransaction(0)); - transactions.addRemoteTransaction(createTransaction(0)); + transactions.addRemoteTransaction(createTransaction(0), Optional.empty()); + transactions.addRemoteTransaction(createTransaction(0), Optional.empty()); for (int i = 1; i < MAX_TRANSACTIONS; i++) { - transactions.addRemoteTransaction(createTransaction(i)); + transactions.addRemoteTransaction(createTransaction(i), Optional.empty()); } assertThat(transactions.size()).isEqualTo(MAX_TRANSACTIONS); - transactions.addRemoteTransaction(createTransaction(MAX_TRANSACTIONS + 1)); - transactions.addRemoteTransaction(createTransaction(MAX_TRANSACTIONS + 2)); + transactions.addRemoteTransaction(createTransaction(MAX_TRANSACTIONS + 1), Optional.empty()); + transactions.addRemoteTransaction(createTransaction(MAX_TRANSACTIONS + 2), Optional.empty()); assertThat(transactions.size()).isEqualTo(MAX_TRANSACTIONS); } @Test public void shouldPrioritizeLocalTransaction() { final Transaction localTransaction = createTransaction(0); - transactions.addLocalTransaction(localTransaction); + transactions.addLocalTransaction(localTransaction, Optional.empty()); for (int i = 1; i <= MAX_TRANSACTIONS; i++) { - transactions.addRemoteTransaction(createTransaction(i)); + transactions.addRemoteTransaction(createTransaction(i), Optional.empty()); } assertThat(transactions.size()).isEqualTo(MAX_TRANSACTIONS); assertTransactionPending(localTransaction); } @Test - public void shouldPrioritizeGasPriceThenTimeAddedToPool() { + public void shouldEvictHighestNonceForSenderOfTheOldestTransactionFirst() { + final Account firstSender = mock(Account.class); + when(firstSender.getNonce()).thenReturn(0L); + + final KeyPair firstSenderKeys = SIGNATURE_ALGORITHM.get().generateKeyPair(); + // first sender sends 2 txs + final Transaction oldestTx = transactionWithNonceSenderAndGasPrice(1, firstSenderKeys, 9); + final Transaction penultimateTx = transactionWithNonceSenderAndGasPrice(2, firstSenderKeys, 11); + transactions.addRemoteTransaction(oldestTx, Optional.of(firstSender)); + transactions.addRemoteTransaction(penultimateTx, Optional.of(firstSender)); + final List lowGasPriceTransactions = - IntStream.range(0, MAX_TRANSACTIONS) + IntStream.range(0, MAX_TRANSACTIONS - 2) .mapToObj( i -> transactionWithNonceSenderAndGasPrice( i + 1, SIGNATURE_ALGORITHM.get().generateKeyPair(), 10)) .collect(Collectors.toUnmodifiableList()); - // Fill the pool with transasctions from random senders - lowGasPriceTransactions.forEach(transactions::addRemoteTransaction); + // Fill the pool with transactions from random senders + lowGasPriceTransactions.forEach(tx -> transactions.addRemoteTransaction(tx, Optional.empty())); - // This should kick the oldest tx with the low gas price out, namely the first one we added + // This should kick the tx with the highest nonce for the sender of the oldest tx, that is + // the penultimate tx final Transaction highGasPriceTransaction = transactionWithNonceSenderAndGasPrice(MAX_TRANSACTIONS + 1, KEYS1, 100); - transactions.addRemoteTransaction(highGasPriceTransaction); + transactions.addRemoteTransaction(highGasPriceTransaction, Optional.empty()); assertThat(transactions.size()).isEqualTo(MAX_TRANSACTIONS); - - assertTransactionPending(highGasPriceTransaction); - assertTransactionNotPending(lowGasPriceTransactions.get(0)); - lowGasPriceTransactions.stream().skip(1).forEach(this::assertTransactionPending); + assertTransactionNotPending(penultimateTx); + assertTransactionPending(oldestTx); + IntStream.range(0, MAX_TRANSACTIONS - 2) + .forEach(i -> assertTransactionPending(lowGasPriceTransactions.get(i))); } @Test @@ -219,7 +241,7 @@ public void shouldStartDroppingLocalTransactionsWhenPoolIsFullOfLocalTransaction for (int i = 0; i <= MAX_TRANSACTIONS; i++) { lastLocalTransactionForSender = createTransaction(i); - transactions.addLocalTransaction(lastLocalTransactionForSender); + transactions.addLocalTransaction(lastLocalTransactionForSender, Optional.empty()); } assertThat(transactions.size()).isEqualTo(MAX_TRANSACTIONS); assertTransactionNotPending(lastLocalTransactionForSender); @@ -229,7 +251,7 @@ public void shouldStartDroppingLocalTransactionsWhenPoolIsFullOfLocalTransaction public void shouldNotifyListenerWhenRemoteTransactionAdded() { transactions.subscribePendingTransactions(listener); - transactions.addRemoteTransaction(transaction1); + transactions.addRemoteTransaction(transaction1, Optional.empty()); verify(listener).onTransactionAdded(transaction1); } @@ -238,13 +260,13 @@ public void shouldNotifyListenerWhenRemoteTransactionAdded() { public void shouldNotNotifyListenerAfterUnsubscribe() { final long id = transactions.subscribePendingTransactions(listener); - transactions.addRemoteTransaction(transaction1); + transactions.addRemoteTransaction(transaction1, Optional.empty()); verify(listener).onTransactionAdded(transaction1); transactions.unsubscribePendingTransactions(id); - transactions.addRemoteTransaction(transaction2); + transactions.addRemoteTransaction(transaction2, Optional.empty()); verifyNoMoreInteractions(listener); } @@ -253,14 +275,14 @@ public void shouldNotNotifyListenerAfterUnsubscribe() { public void shouldNotifyListenerWhenLocalTransactionAdded() { transactions.subscribePendingTransactions(listener); - transactions.addLocalTransaction(transaction1); + transactions.addLocalTransaction(transaction1, Optional.empty()); verify(listener).onTransactionAdded(transaction1); } @Test public void shouldNotifyDroppedListenerWhenRemoteTransactionDropped() { - transactions.addRemoteTransaction(transaction1); + transactions.addRemoteTransaction(transaction1, Optional.empty()); transactions.subscribeDroppedTransactions(droppedListener); @@ -271,8 +293,8 @@ public void shouldNotifyDroppedListenerWhenRemoteTransactionDropped() { @Test public void shouldNotNotifyDroppedListenerAfterUnsubscribe() { - transactions.addRemoteTransaction(transaction1); - transactions.addRemoteTransaction(transaction2); + transactions.addRemoteTransaction(transaction1, Optional.empty()); + transactions.addRemoteTransaction(transaction2, Optional.empty()); final long id = transactions.subscribeDroppedTransactions(droppedListener); @@ -289,7 +311,7 @@ public void shouldNotNotifyDroppedListenerAfterUnsubscribe() { @Test public void shouldNotifyDroppedListenerWhenLocalTransactionDropped() { - transactions.addLocalTransaction(transaction1); + transactions.addLocalTransaction(transaction1, Optional.empty()); transactions.subscribeDroppedTransactions(droppedListener); @@ -300,7 +322,7 @@ public void shouldNotifyDroppedListenerWhenLocalTransactionDropped() { @Test public void shouldNotNotifyDroppedListenerWhenTransactionAddedToBlock() { - transactions.addRemoteTransaction(transaction1); + transactions.addRemoteTransaction(transaction1, Optional.empty()); transactions.subscribeDroppedTransactions(droppedListener); @@ -311,8 +333,8 @@ public void shouldNotNotifyDroppedListenerWhenTransactionAddedToBlock() { @Test public void selectTransactionsUntilSelectorRequestsNoMore() { - transactions.addRemoteTransaction(transaction1); - transactions.addRemoteTransaction(transaction2); + transactions.addRemoteTransaction(transaction1, Optional.empty()); + transactions.addRemoteTransaction(transaction2, Optional.empty()); final List parsedTransactions = Lists.newArrayList(); transactions.selectTransactions( @@ -327,8 +349,8 @@ public void selectTransactionsUntilSelectorRequestsNoMore() { @Test public void selectTransactionsUntilPendingIsEmpty() { - transactions.addRemoteTransaction(transaction1); - transactions.addRemoteTransaction(transaction2); + transactions.addRemoteTransaction(transaction1, Optional.empty()); + transactions.addRemoteTransaction(transaction2, Optional.empty()); final List parsedTransactions = Lists.newArrayList(); transactions.selectTransactions( @@ -347,8 +369,8 @@ public void shouldNotSelectReplacedTransaction() { final Transaction transaction1 = transactionWithNonceSenderAndGasPrice(1, KEYS1, 1); final Transaction transaction2 = transactionWithNonceSenderAndGasPrice(1, KEYS1, 2); - transactions.addRemoteTransaction(transaction1); - transactions.addRemoteTransaction(transaction2); + transactions.addRemoteTransaction(transaction1, Optional.empty()); + transactions.addRemoteTransaction(transaction2, Optional.empty()); final List parsedTransactions = Lists.newArrayList(); transactions.selectTransactions( @@ -362,8 +384,8 @@ public void shouldNotSelectReplacedTransaction() { @Test public void invalidTransactionIsDeletedFromPendingTransactions() { - transactions.addRemoteTransaction(transaction1); - transactions.addRemoteTransaction(transaction2); + transactions.addRemoteTransaction(transaction1, Optional.empty()); + transactions.addRemoteTransaction(transaction2, Optional.empty()); final List parsedTransactions = Lists.newArrayList(); transactions.selectTransactions( @@ -387,7 +409,7 @@ public void shouldReturnEmptyOptionalAsMaximumNonceWhenNoTransactionsPresent() { @Test public void shouldReturnEmptyOptionalAsMaximumNonceWhenLastTransactionForSenderRemoved() { final Transaction transaction = transactionWithNonceAndSender(1, KEYS1); - transactions.addRemoteTransaction(transaction); + transactions.addRemoteTransaction(transaction, Optional.empty()); transactions.removeTransaction(transaction); assertThat(transactions.getNextNonceForSender(SENDER1)).isEmpty(); } @@ -397,9 +419,9 @@ public void shouldReplaceTransactionWithSameSenderAndNonce() { final Transaction transaction1 = transactionWithNonceSenderAndGasPrice(1, KEYS1, 1); final Transaction transaction1b = transactionWithNonceSenderAndGasPrice(1, KEYS1, 2); final Transaction transaction2 = transactionWithNonceSenderAndGasPrice(2, KEYS1, 1); - assertThat(transactions.addRemoteTransaction(transaction1)).isTrue(); - assertThat(transactions.addRemoteTransaction(transaction2)).isTrue(); - assertThat(transactions.addRemoteTransaction(transaction1b)).isTrue(); + assertThat(transactions.addRemoteTransaction(transaction1, Optional.empty())).isEqualTo(ADDED); + assertThat(transactions.addRemoteTransaction(transaction2, Optional.empty())).isEqualTo(ADDED); + assertThat(transactions.addRemoteTransaction(transaction1b, Optional.empty())).isEqualTo(ADDED); assertTransactionNotPending(transaction1); assertTransactionPending(transaction1b); @@ -416,12 +438,13 @@ public void shouldReplaceTransactionWithSameSenderAndNonce_multipleReplacements( for (int i = 0; i < replacedTxCount; i++) { final Transaction duplicateTx = transactionWithNonceSenderAndGasPrice(1, KEYS1, i + 1); replacedTransactions.add(duplicateTx); - transactions.addRemoteTransaction(duplicateTx); + transactions.addRemoteTransaction(duplicateTx, Optional.empty()); } final Transaction finalReplacingTx = transactionWithNonceSenderAndGasPrice(1, KEYS1, 100); final Transaction independentTx = transactionWithNonceSenderAndGasPrice(2, KEYS1, 1); - assertThat(transactions.addRemoteTransaction(independentTx)).isTrue(); - assertThat(transactions.addRemoteTransaction(finalReplacingTx)).isTrue(); + assertThat(transactions.addRemoteTransaction(independentTx, Optional.empty())).isEqualTo(ADDED); + assertThat(transactions.addRemoteTransaction(finalReplacingTx, Optional.empty())) + .isEqualTo(ADDED); // All tx's except the last duplicate should be removed replacedTransactions.forEach(this::assertTransactionNotPending); @@ -446,17 +469,17 @@ public void shouldReplaceTransactionWithSameSenderAndNonce_multipleReplacements( transactionWithNonceSenderAndGasPrice(1, KEYS1, (i * 110 / 100) + 1); replacedTransactions.add(duplicateTx); if (i % 2 == 0) { - transactions.addRemoteTransaction(duplicateTx); + transactions.addRemoteTransaction(duplicateTx, Optional.empty()); remoteDuplicateCount++; } else { - transactions.addLocalTransaction(duplicateTx); + transactions.addLocalTransaction(duplicateTx, Optional.empty()); } } final Transaction finalReplacingTx = transactionWithNonceSenderAndGasPrice(1, KEYS1, 100); final Transaction independentTx = transactionWithNonceSenderAndGasPrice(2, KEYS1, 1); - assertThat(transactions.addLocalTransaction(finalReplacingTx)) + assertThat(transactions.addLocalTransaction(finalReplacingTx, Optional.empty())) .isEqualTo(TransactionAddedStatus.ADDED); - assertThat(transactions.addRemoteTransaction(independentTx)).isTrue(); + assertThat(transactions.addRemoteTransaction(independentTx, Optional.empty())).isEqualTo(ADDED); // All tx's except the last duplicate should be removed replacedTransactions.forEach(this::assertTransactionNotPending); @@ -480,8 +503,8 @@ public void shouldReplaceTransactionWithSameSenderAndNonce_multipleReplacements( public void shouldReplaceOnlyTransactionFromSenderWhenItHasTheSameNonce() { final Transaction transaction1 = transactionWithNonceSenderAndGasPrice(1, KEYS1, 1); final Transaction transaction1b = transactionWithNonceSenderAndGasPrice(1, KEYS1, 2); - assertThat(transactions.addRemoteTransaction(transaction1)).isTrue(); - assertThat(transactions.addRemoteTransaction(transaction1b)).isTrue(); + assertThat(transactions.addRemoteTransaction(transaction1, Optional.empty())).isEqualTo(ADDED); + assertThat(transactions.addRemoteTransaction(transaction1b, Optional.empty())).isEqualTo(ADDED); assertTransactionNotPending(transaction1); assertTransactionPending(transaction1b); @@ -494,10 +517,11 @@ public void shouldReplaceOnlyTransactionFromSenderWhenItHasTheSameNonce() { public void shouldNotReplaceTransactionWithSameSenderAndNonceWhenGasPriceIsLower() { final Transaction transaction1 = transactionWithNonceSenderAndGasPrice(1, KEYS1, 2); final Transaction transaction1b = transactionWithNonceSenderAndGasPrice(1, KEYS1, 1); - assertThat(transactions.addRemoteTransaction(transaction1)).isTrue(); + assertThat(transactions.addRemoteTransaction(transaction1, Optional.empty())).isEqualTo(ADDED); transactions.subscribePendingTransactions(listener); - assertThat(transactions.addRemoteTransaction(transaction1b)).isFalse(); + assertThat(transactions.addRemoteTransaction(transaction1b, Optional.empty())) + .isEqualTo(REJECTED_UNDERPRICED_REPLACEMENT); assertTransactionNotPending(transaction1b); assertTransactionPending(transaction1); @@ -507,16 +531,16 @@ public void shouldNotReplaceTransactionWithSameSenderAndNonceWhenGasPriceIsLower @Test public void shouldTrackMaximumNonceForEachSender() { - transactions.addRemoteTransaction(transactionWithNonceAndSender(0, KEYS1)); + transactions.addRemoteTransaction(transactionWithNonceAndSender(0, KEYS1), Optional.empty()); assertMaximumNonceForSender(SENDER1, 1); - transactions.addRemoteTransaction(transactionWithNonceAndSender(1, KEYS1)); + transactions.addRemoteTransaction(transactionWithNonceAndSender(1, KEYS1), Optional.empty()); assertMaximumNonceForSender(SENDER1, 2); - transactions.addRemoteTransaction(transactionWithNonceAndSender(2, KEYS1)); + transactions.addRemoteTransaction(transactionWithNonceAndSender(2, KEYS1), Optional.empty()); assertMaximumNonceForSender(SENDER1, 3); - transactions.addRemoteTransaction(transactionWithNonceAndSender(20, KEYS2)); + transactions.addRemoteTransaction(transactionWithNonceAndSender(20, KEYS2), Optional.empty()); assertMaximumNonceForSender(SENDER2, 21); assertMaximumNonceForSender(SENDER1, 3); } @@ -527,9 +551,9 @@ public void shouldIterateTransactionsFromSameSenderInNonceOrder() { final Transaction transaction2 = transactionWithNonceAndSender(1, KEYS1); final Transaction transaction3 = transactionWithNonceAndSender(2, KEYS1); - transactions.addLocalTransaction(transaction1); - transactions.addLocalTransaction(transaction2); - transactions.addLocalTransaction(transaction3); + transactions.addLocalTransaction(transaction1, Optional.empty()); + transactions.addLocalTransaction(transaction2, Optional.empty()); + transactions.addLocalTransaction(transaction3, Optional.empty()); final List iterationOrder = new ArrayList<>(); transactions.selectTransactions( @@ -546,8 +570,8 @@ public void shouldNotForceNonceOrderWhenSendersDiffer() { final Transaction transaction1 = transactionWithNonceAndSender(0, KEYS1); final Transaction transaction2 = transactionWithNonceAndSender(1, KEYS2); - transactions.addLocalTransaction(transaction1); - transactions.addLocalTransaction(transaction2); + transactions.addLocalTransaction(transaction1, Optional.empty()); + transactions.addLocalTransaction(transaction2, Optional.empty()); final List iterationOrder = new ArrayList<>(); transactions.selectTransactions( @@ -566,10 +590,10 @@ public void shouldNotIncreasePriorityOfTransactionsBecauseOfNonceOrder() { final Transaction transaction3 = transactionWithNonceAndSender(2, KEYS1); final Transaction transaction4 = transactionWithNonceAndSender(5, KEYS2); - transactions.addLocalTransaction(transaction1); - transactions.addLocalTransaction(transaction4); - transactions.addLocalTransaction(transaction2); - transactions.addLocalTransaction(transaction3); + transactions.addLocalTransaction(transaction1, Optional.empty()); + transactions.addLocalTransaction(transaction4, Optional.empty()); + transactions.addLocalTransaction(transaction2, Optional.empty()); + transactions.addLocalTransaction(transaction3, Optional.empty()); final List iterationOrder = new ArrayList<>(); transactions.selectTransactions( @@ -627,9 +651,9 @@ public void shouldEvictMultipleOldTransactions() { metricsSystem, BaseFeePendingTransactionsTest::mockBlockHeader); - transactions.addRemoteTransaction(transaction1); + transactions.addRemoteTransaction(transaction1, Optional.empty()); assertThat(transactions.size()).isEqualTo(1); - transactions.addRemoteTransaction(transaction2); + transactions.addRemoteTransaction(transaction2, Optional.empty()); assertThat(transactions.size()).isEqualTo(2); clock.step(2L, ChronoUnit.HOURS); @@ -650,7 +674,7 @@ public void shouldEvictSingleOldTransaction() { clock, metricsSystem, BaseFeePendingTransactionsTest::mockBlockHeader); - transactions.addRemoteTransaction(transaction1); + transactions.addRemoteTransaction(transaction1, Optional.empty()); assertThat(transactions.size()).isEqualTo(1); clock.step(2L, ChronoUnit.HOURS); transactions.evictOldTransactions(); @@ -670,10 +694,10 @@ public void shouldEvictExclusivelyOldTransactions() { clock, metricsSystem, BaseFeePendingTransactionsTest::mockBlockHeader); - transactions.addRemoteTransaction(transaction1); + transactions.addRemoteTransaction(transaction1, Optional.empty()); assertThat(transactions.size()).isEqualTo(1); clock.step(3L, ChronoUnit.HOURS); - transactions.addRemoteTransaction(transaction2); + transactions.addRemoteTransaction(transaction2, Optional.empty()); assertThat(transactions.size()).isEqualTo(2); transactions.evictOldTransactions(); assertThat(transactions.size()).isEqualTo(1); @@ -682,12 +706,13 @@ public void shouldEvictExclusivelyOldTransactions() { @Test public void shouldNotIncrementAddedCounterWhenRemoteTransactionAlreadyPresent() { - transactions.addLocalTransaction(transaction1); + transactions.addLocalTransaction(transaction1, Optional.empty()); assertThat(transactions.size()).isEqualTo(1); assertThat(metricsSystem.getCounterValue(ADDED_COUNTER, LOCAL)).isEqualTo(1); assertThat(metricsSystem.getCounterValue(ADDED_COUNTER, REMOTE)).isEqualTo(0); - assertThat(transactions.addRemoteTransaction(transaction1)).isFalse(); + assertThat(transactions.addRemoteTransaction(transaction1, Optional.empty())) + .isEqualTo(ALREADY_KNOWN); assertThat(transactions.size()).isEqualTo(1); assertThat(metricsSystem.getCounterValue(ADDED_COUNTER, LOCAL)).isEqualTo(1); assertThat(metricsSystem.getCounterValue(ADDED_COUNTER, REMOTE)).isEqualTo(0); @@ -695,13 +720,13 @@ public void shouldNotIncrementAddedCounterWhenRemoteTransactionAlreadyPresent() @Test public void shouldNotIncrementAddedCounterWhenLocalTransactionAlreadyPresent() { - transactions.addRemoteTransaction(transaction1); + transactions.addRemoteTransaction(transaction1, Optional.empty()); assertThat(transactions.size()).isEqualTo(1); assertThat(metricsSystem.getCounterValue(ADDED_COUNTER, LOCAL)).isEqualTo(0); assertThat(metricsSystem.getCounterValue(ADDED_COUNTER, REMOTE)).isEqualTo(1); - assertThat(transactions.addLocalTransaction(transaction1)) - .isEqualTo(TransactionAddedStatus.ALREADY_KNOWN); + assertThat(transactions.addLocalTransaction(transaction1, Optional.empty())) + .isEqualTo(ALREADY_KNOWN); assertThat(transactions.size()).isEqualTo(1); assertThat(metricsSystem.getCounterValue(ADDED_COUNTER, LOCAL)).isEqualTo(0); assertThat(metricsSystem.getCounterValue(ADDED_COUNTER, REMOTE)).isEqualTo(1); @@ -774,7 +799,9 @@ private void addLocalTransactions(final long... nonces) { private void addLocalTransactions( final AbstractPendingTransactionsSorter sorter, final long... nonces) { for (final long nonce : nonces) { - sorter.addLocalTransaction(createTransaction(nonce)); + final Account sender = mock(Account.class); + when(sender.getNonce()).thenReturn(1L); + sorter.addLocalTransaction(createTransaction(nonce), Optional.of(sender)); } } diff --git a/ethereum/eth/src/test/java/org/hyperledger/besu/ethereum/eth/transactions/GasPricePendingTransactionsTest.java b/ethereum/eth/src/test/java/org/hyperledger/besu/ethereum/eth/transactions/GasPricePendingTransactionsTest.java index 5b47ad7aee1..164d9be2c18 100644 --- a/ethereum/eth/src/test/java/org/hyperledger/besu/ethereum/eth/transactions/GasPricePendingTransactionsTest.java +++ b/ethereum/eth/src/test/java/org/hyperledger/besu/ethereum/eth/transactions/GasPricePendingTransactionsTest.java @@ -15,6 +15,9 @@ package org.hyperledger.besu.ethereum.eth.transactions; import static org.assertj.core.api.Assertions.assertThat; +import static org.hyperledger.besu.ethereum.eth.transactions.sorter.AbstractPendingTransactionsSorter.TransactionAddedStatus.ADDED; +import static org.hyperledger.besu.ethereum.eth.transactions.sorter.AbstractPendingTransactionsSorter.TransactionAddedStatus.ALREADY_KNOWN; +import static org.hyperledger.besu.ethereum.eth.transactions.sorter.AbstractPendingTransactionsSorter.TransactionAddedStatus.REJECTED_UNDERPRICED_REPLACEMENT; import static org.mockito.Mockito.mock; import static org.mockito.Mockito.verify; import static org.mockito.Mockito.verifyNoInteractions; @@ -85,13 +88,13 @@ public class GasPricePendingTransactionsTest { @Test public void shouldReturnExclusivelyLocalTransactionsWhenAppropriate() { final Transaction localTransaction0 = createTransaction(0); - transactions.addLocalTransaction(localTransaction0); + transactions.addLocalTransaction(localTransaction0, Optional.empty()); assertThat(transactions.size()).isEqualTo(1); - transactions.addRemoteTransaction(transaction1); + transactions.addRemoteTransaction(transaction1, Optional.empty()); assertThat(transactions.size()).isEqualTo(2); - transactions.addRemoteTransaction(transaction2); + transactions.addRemoteTransaction(transaction2, Optional.empty()); assertThat(transactions.size()).isEqualTo(3); final List localTransactions = transactions.getLocalTransactions(); @@ -100,11 +103,11 @@ public void shouldReturnExclusivelyLocalTransactionsWhenAppropriate() { @Test public void shouldAddATransaction() { - transactions.addRemoteTransaction(transaction1); + transactions.addRemoteTransaction(transaction1, Optional.empty()); assertThat(transactions.size()).isEqualTo(1); assertThat(metricsSystem.getCounterValue(ADDED_COUNTER, REMOTE)).isEqualTo(1); - transactions.addRemoteTransaction(transaction2); + transactions.addRemoteTransaction(transaction2, Optional.empty()); assertThat(transactions.size()).isEqualTo(2); assertThat(metricsSystem.getCounterValue(ADDED_COUNTER, REMOTE)).isEqualTo(2); } @@ -116,7 +119,7 @@ public void shouldReturnEmptyOptionalWhenNoTransactionWithGivenHashExists() { @Test public void shouldGetTransactionByHash() { - transactions.addRemoteTransaction(transaction1); + transactions.addRemoteTransaction(transaction1, Optional.empty()); assertTransactionPending(transaction1); } @@ -127,14 +130,14 @@ public void shouldDropOldestTransactionWhenLimitExceeded() { .value(Wei.of(1L)) .nonce(0L) .createTransaction(SIGNATURE_ALGORITHM.get().generateKeyPair()); - transactions.addRemoteTransaction(oldestTransaction); + transactions.addRemoteTransaction(oldestTransaction, Optional.empty()); for (int i = 1; i < MAX_TRANSACTIONS; i++) { final Transaction newerTransaction = new TransactionTestFixture() .value(Wei.of(1L)) .nonce(0L) .createTransaction(SIGNATURE_ALGORITHM.get().generateKeyPair()); - transactions.addRemoteTransaction(newerTransaction); + transactions.addRemoteTransaction(newerTransaction, Optional.empty()); } assertThat(transactions.size()).isEqualTo(MAX_TRANSACTIONS); assertThat(metricsSystem.getCounterValue(REMOVED_COUNTER, REMOTE, DROPPED)).isZero(); @@ -145,7 +148,7 @@ public void shouldDropOldestTransactionWhenLimitExceeded() { .nonce(MAX_TRANSACTIONS + 1) .createTransaction(SIGNATURE_ALGORITHM.get().generateKeyPair()); - transactions.addRemoteTransaction(lastTransaction); + transactions.addRemoteTransaction(lastTransaction, Optional.empty()); assertThat(transactions.size()).isEqualTo(MAX_TRANSACTIONS); assertTransactionNotPending(oldestTransaction); assertThat(metricsSystem.getCounterValue(REMOVED_COUNTER, REMOTE, DROPPED)).isEqualTo(1); @@ -166,7 +169,7 @@ public void shouldDropFutureTransactionWhenSenderLimitExceeded() { Transaction furthestFutureTransaction = null; for (int i = 0; i < MAX_TRANSACTIONS; i++) { furthestFutureTransaction = transactionWithNonceSenderAndGasPrice(i, KEYS1, 10L); - senderLimitedtransactions.addRemoteTransaction(furthestFutureTransaction); + senderLimitedtransactions.addRemoteTransaction(furthestFutureTransaction, Optional.empty()); } assertThat(senderLimitedtransactions.size()).isEqualTo(MAX_TRANSACTIONS_BY_SENDER); assertThat(metricsSystem.getCounterValue(REMOVED_COUNTER, REMOTE, DROPPED)).isEqualTo(1L); @@ -177,26 +180,26 @@ public void shouldDropFutureTransactionWhenSenderLimitExceeded() { @Test public void shouldHandleMaximumTransactionLimitCorrectlyWhenSameTransactionAddedMultipleTimes() { - transactions.addRemoteTransaction(createTransaction(0)); - transactions.addRemoteTransaction(createTransaction(0)); + transactions.addRemoteTransaction(createTransaction(0), Optional.empty()); + transactions.addRemoteTransaction(createTransaction(0), Optional.empty()); for (int i = 1; i < MAX_TRANSACTIONS; i++) { - transactions.addRemoteTransaction(createTransaction(i)); + transactions.addRemoteTransaction(createTransaction(i), Optional.empty()); } assertThat(transactions.size()).isEqualTo(MAX_TRANSACTIONS); - transactions.addRemoteTransaction(createTransaction(MAX_TRANSACTIONS + 1)); - transactions.addRemoteTransaction(createTransaction(MAX_TRANSACTIONS + 2)); + transactions.addRemoteTransaction(createTransaction(MAX_TRANSACTIONS + 1), Optional.empty()); + transactions.addRemoteTransaction(createTransaction(MAX_TRANSACTIONS + 2), Optional.empty()); assertThat(transactions.size()).isEqualTo(MAX_TRANSACTIONS); } @Test public void shouldPrioritizeLocalTransaction() { final Transaction localTransaction = createTransaction(0); - transactions.addLocalTransaction(localTransaction); + transactions.addLocalTransaction(localTransaction, Optional.empty()); for (int i = 1; i <= MAX_TRANSACTIONS; i++) { - transactions.addRemoteTransaction(createTransaction(i)); + transactions.addRemoteTransaction(createTransaction(i), Optional.empty()); } assertThat(transactions.size()).isEqualTo(MAX_TRANSACTIONS); assertTransactionPending(localTransaction); @@ -215,13 +218,13 @@ public void shouldPrioritizeGasPriceThenTimeAddedToPool() { .collect(Collectors.toUnmodifiableList()); // Fill the pool - lowGasPriceTransactions.forEach(transactions::addRemoteTransaction); + lowGasPriceTransactions.forEach(tx -> transactions.addRemoteTransaction(tx, Optional.empty())); // This should kick the oldest tx with the low gas price out, namely the first one we added final Transaction highGasPriceTransaction = transactionWithNonceSenderAndGasPrice( MAX_TRANSACTIONS + 10, SIGNATURE_ALGORITHM.get().generateKeyPair(), 100); - transactions.addRemoteTransaction(highGasPriceTransaction); + transactions.addRemoteTransaction(highGasPriceTransaction, Optional.empty()); assertThat(transactions.size()).isEqualTo(MAX_TRANSACTIONS); assertTransactionPending(highGasPriceTransaction); @@ -235,7 +238,7 @@ public void shouldStartDroppingLocalTransactionsWhenPoolIsFullOfLocalTransaction for (int i = 0; i <= MAX_TRANSACTIONS; i++) { lastLocalTransactionForSender = createTransaction(i); - transactions.addLocalTransaction(lastLocalTransactionForSender); + transactions.addLocalTransaction(lastLocalTransactionForSender, Optional.empty()); } assertThat(transactions.size()).isEqualTo(MAX_TRANSACTIONS); assertTransactionNotPending(lastLocalTransactionForSender); @@ -245,7 +248,7 @@ public void shouldStartDroppingLocalTransactionsWhenPoolIsFullOfLocalTransaction public void shouldNotifyListenerWhenRemoteTransactionAdded() { transactions.subscribePendingTransactions(listener); - transactions.addRemoteTransaction(transaction1); + transactions.addRemoteTransaction(transaction1, Optional.empty()); verify(listener).onTransactionAdded(transaction1); } @@ -254,13 +257,13 @@ public void shouldNotifyListenerWhenRemoteTransactionAdded() { public void shouldNotNotifyListenerAfterUnsubscribe() { final long id = transactions.subscribePendingTransactions(listener); - transactions.addRemoteTransaction(transaction1); + transactions.addRemoteTransaction(transaction1, Optional.empty()); verify(listener).onTransactionAdded(transaction1); transactions.unsubscribePendingTransactions(id); - transactions.addRemoteTransaction(transaction2); + transactions.addRemoteTransaction(transaction2, Optional.empty()); verifyNoMoreInteractions(listener); } @@ -269,14 +272,14 @@ public void shouldNotNotifyListenerAfterUnsubscribe() { public void shouldNotifyListenerWhenLocalTransactionAdded() { transactions.subscribePendingTransactions(listener); - transactions.addLocalTransaction(transaction1); + transactions.addLocalTransaction(transaction1, Optional.empty()); verify(listener).onTransactionAdded(transaction1); } @Test public void shouldNotifyDroppedListenerWhenRemoteTransactionDropped() { - transactions.addRemoteTransaction(transaction1); + transactions.addRemoteTransaction(transaction1, Optional.empty()); transactions.subscribeDroppedTransactions(droppedListener); @@ -287,8 +290,8 @@ public void shouldNotifyDroppedListenerWhenRemoteTransactionDropped() { @Test public void shouldNotNotifyDroppedListenerAfterUnsubscribe() { - transactions.addRemoteTransaction(transaction1); - transactions.addRemoteTransaction(transaction2); + transactions.addRemoteTransaction(transaction1, Optional.empty()); + transactions.addRemoteTransaction(transaction2, Optional.empty()); final long id = transactions.subscribeDroppedTransactions(droppedListener); @@ -305,7 +308,7 @@ public void shouldNotNotifyDroppedListenerAfterUnsubscribe() { @Test public void shouldNotifyDroppedListenerWhenLocalTransactionDropped() { - transactions.addLocalTransaction(transaction1); + transactions.addLocalTransaction(transaction1, Optional.empty()); transactions.subscribeDroppedTransactions(droppedListener); @@ -316,7 +319,7 @@ public void shouldNotifyDroppedListenerWhenLocalTransactionDropped() { @Test public void shouldNotNotifyDroppedListenerWhenTransactionAddedToBlock() { - transactions.addRemoteTransaction(transaction1); + transactions.addRemoteTransaction(transaction1, Optional.empty()); transactions.subscribeDroppedTransactions(droppedListener); @@ -327,8 +330,8 @@ public void shouldNotNotifyDroppedListenerWhenTransactionAddedToBlock() { @Test public void selectTransactionsUntilSelectorRequestsNoMore() { - transactions.addRemoteTransaction(transaction1); - transactions.addRemoteTransaction(transaction2); + transactions.addRemoteTransaction(transaction1, Optional.empty()); + transactions.addRemoteTransaction(transaction2, Optional.empty()); final List parsedTransactions = Lists.newArrayList(); transactions.selectTransactions( @@ -343,8 +346,8 @@ public void selectTransactionsUntilSelectorRequestsNoMore() { @Test public void selectTransactionsUntilPendingIsEmpty() { - transactions.addRemoteTransaction(transaction1); - transactions.addRemoteTransaction(transaction2); + transactions.addRemoteTransaction(transaction1, Optional.empty()); + transactions.addRemoteTransaction(transaction2, Optional.empty()); final List parsedTransactions = Lists.newArrayList(); transactions.selectTransactions( @@ -363,8 +366,8 @@ public void shouldNotSelectReplacedTransaction() { final Transaction transaction1 = transactionWithNonceSenderAndGasPrice(1, KEYS1, 1); final Transaction transaction2 = transactionWithNonceSenderAndGasPrice(1, KEYS1, 2); - transactions.addRemoteTransaction(transaction1); - transactions.addRemoteTransaction(transaction2); + transactions.addRemoteTransaction(transaction1, Optional.empty()); + transactions.addRemoteTransaction(transaction2, Optional.empty()); final List parsedTransactions = Lists.newArrayList(); transactions.selectTransactions( @@ -378,8 +381,8 @@ public void shouldNotSelectReplacedTransaction() { @Test public void invalidTransactionIsDeletedFromPendingTransactions() { - transactions.addRemoteTransaction(transaction1); - transactions.addRemoteTransaction(transaction2); + transactions.addRemoteTransaction(transaction1, Optional.empty()); + transactions.addRemoteTransaction(transaction2, Optional.empty()); final List parsedTransactions = Lists.newArrayList(); transactions.selectTransactions( @@ -403,7 +406,7 @@ public void shouldReturnEmptyOptionalAsMaximumNonceWhenNoTransactionsPresent() { @Test public void shouldReturnEmptyOptionalAsMaximumNonceWhenLastTransactionForSenderRemoved() { final Transaction transaction = transactionWithNonceAndSender(1, KEYS1); - transactions.addRemoteTransaction(transaction); + transactions.addRemoteTransaction(transaction, Optional.empty()); transactions.removeTransaction(transaction); assertThat(transactions.getNextNonceForSender(SENDER1)).isEmpty(); } @@ -413,9 +416,9 @@ public void shouldReplaceTransactionWithSameSenderAndNonce() { final Transaction transaction1 = transactionWithNonceSenderAndGasPrice(1, KEYS1, 1); final Transaction transaction1b = transactionWithNonceSenderAndGasPrice(1, KEYS1, 2); final Transaction transaction2 = transactionWithNonceSenderAndGasPrice(2, KEYS1, 1); - assertThat(transactions.addRemoteTransaction(transaction1)).isTrue(); - assertThat(transactions.addRemoteTransaction(transaction2)).isTrue(); - assertThat(transactions.addRemoteTransaction(transaction1b)).isTrue(); + assertThat(transactions.addRemoteTransaction(transaction1, Optional.empty())).isEqualTo(ADDED); + assertThat(transactions.addRemoteTransaction(transaction2, Optional.empty())).isEqualTo(ADDED); + assertThat(transactions.addRemoteTransaction(transaction1b, Optional.empty())).isEqualTo(ADDED); assertTransactionNotPending(transaction1); assertTransactionPending(transaction1b); @@ -432,12 +435,13 @@ public void shouldReplaceTransactionWithSameSenderAndNonce_multipleReplacements( for (int i = 0; i < replacedTxCount; i++) { final Transaction duplicateTx = transactionWithNonceSenderAndGasPrice(1, KEYS1, i + 1); replacedTransactions.add(duplicateTx); - transactions.addRemoteTransaction(duplicateTx); + transactions.addRemoteTransaction(duplicateTx, Optional.empty()); } final Transaction finalReplacingTx = transactionWithNonceSenderAndGasPrice(1, KEYS1, 100); final Transaction independentTx = transactionWithNonceSenderAndGasPrice(2, KEYS1, 1); - assertThat(transactions.addRemoteTransaction(independentTx)).isTrue(); - assertThat(transactions.addRemoteTransaction(finalReplacingTx)).isTrue(); + assertThat(transactions.addRemoteTransaction(independentTx, Optional.empty())).isEqualTo(ADDED); + assertThat(transactions.addRemoteTransaction(finalReplacingTx, Optional.empty())) + .isEqualTo(ADDED); // All tx's except the last duplicate should be removed replacedTransactions.forEach(this::assertTransactionNotPending); @@ -462,17 +466,17 @@ public void shouldReplaceTransactionWithSameSenderAndNonce_multipleReplacements( transactionWithNonceSenderAndGasPrice(1, KEYS1, (i * 110 / 100) + 1); replacedTransactions.add(duplicateTx); if (i % 2 == 0) { - transactions.addRemoteTransaction(duplicateTx); + transactions.addRemoteTransaction(duplicateTx, Optional.empty()); remoteDuplicateCount++; } else { - transactions.addLocalTransaction(duplicateTx); + transactions.addLocalTransaction(duplicateTx, Optional.empty()); } } final Transaction finalReplacingTx = transactionWithNonceSenderAndGasPrice(1, KEYS1, 100); final Transaction independentTx = transactionWithNonceSenderAndGasPrice(2, KEYS1, 1); - assertThat(transactions.addLocalTransaction(finalReplacingTx)) + assertThat(transactions.addLocalTransaction(finalReplacingTx, Optional.empty())) .isEqualTo(TransactionAddedStatus.ADDED); - assertThat(transactions.addRemoteTransaction(independentTx)).isTrue(); + assertThat(transactions.addRemoteTransaction(independentTx, Optional.empty())).isEqualTo(ADDED); // All tx's except the last duplicate should be removed replacedTransactions.forEach(this::assertTransactionNotPending); @@ -496,8 +500,8 @@ public void shouldReplaceTransactionWithSameSenderAndNonce_multipleReplacements( public void shouldReplaceOnlyTransactionFromSenderWhenItHasTheSameNonce() { final Transaction transaction1 = transactionWithNonceSenderAndGasPrice(1, KEYS1, 1); final Transaction transaction1b = transactionWithNonceSenderAndGasPrice(1, KEYS1, 2); - assertThat(transactions.addRemoteTransaction(transaction1)).isTrue(); - assertThat(transactions.addRemoteTransaction(transaction1b)).isTrue(); + assertThat(transactions.addRemoteTransaction(transaction1, Optional.empty())).isEqualTo(ADDED); + assertThat(transactions.addRemoteTransaction(transaction1b, Optional.empty())).isEqualTo(ADDED); assertTransactionNotPending(transaction1); assertTransactionPending(transaction1b); @@ -510,10 +514,11 @@ public void shouldReplaceOnlyTransactionFromSenderWhenItHasTheSameNonce() { public void shouldNotReplaceTransactionWithSameSenderAndNonceWhenGasPriceIsLower() { final Transaction transaction1 = transactionWithNonceSenderAndGasPrice(1, KEYS1, 2); final Transaction transaction1b = transactionWithNonceSenderAndGasPrice(1, KEYS1, 1); - assertThat(transactions.addRemoteTransaction(transaction1)).isTrue(); + assertThat(transactions.addRemoteTransaction(transaction1, Optional.empty())).isEqualTo(ADDED); transactions.subscribePendingTransactions(listener); - assertThat(transactions.addRemoteTransaction(transaction1b)).isFalse(); + assertThat(transactions.addRemoteTransaction(transaction1b, Optional.empty())) + .isEqualTo(REJECTED_UNDERPRICED_REPLACEMENT); assertTransactionNotPending(transaction1b); assertTransactionPending(transaction1); @@ -523,16 +528,16 @@ public void shouldNotReplaceTransactionWithSameSenderAndNonceWhenGasPriceIsLower @Test public void shouldTrackMaximumNonceForEachSender() { - transactions.addRemoteTransaction(transactionWithNonceAndSender(0, KEYS1)); + transactions.addRemoteTransaction(transactionWithNonceAndSender(0, KEYS1), Optional.empty()); assertMaximumNonceForSender(SENDER1, 1); - transactions.addRemoteTransaction(transactionWithNonceAndSender(1, KEYS1)); + transactions.addRemoteTransaction(transactionWithNonceAndSender(1, KEYS1), Optional.empty()); assertMaximumNonceForSender(SENDER1, 2); - transactions.addRemoteTransaction(transactionWithNonceAndSender(2, KEYS1)); + transactions.addRemoteTransaction(transactionWithNonceAndSender(2, KEYS1), Optional.empty()); assertMaximumNonceForSender(SENDER1, 3); - transactions.addRemoteTransaction(transactionWithNonceAndSender(20, KEYS2)); + transactions.addRemoteTransaction(transactionWithNonceAndSender(20, KEYS2), Optional.empty()); assertMaximumNonceForSender(SENDER2, 21); assertMaximumNonceForSender(SENDER1, 3); } @@ -543,9 +548,9 @@ public void shouldIterateTransactionsFromSameSenderInNonceOrder() { final Transaction transaction2 = transactionWithNonceAndSender(1, KEYS1); final Transaction transaction3 = transactionWithNonceAndSender(2, KEYS1); - transactions.addLocalTransaction(transaction1); - transactions.addLocalTransaction(transaction2); - transactions.addLocalTransaction(transaction3); + transactions.addLocalTransaction(transaction1, Optional.empty()); + transactions.addLocalTransaction(transaction2, Optional.empty()); + transactions.addLocalTransaction(transaction3, Optional.empty()); final List iterationOrder = new ArrayList<>(); transactions.selectTransactions( @@ -562,8 +567,8 @@ public void shouldNotForceNonceOrderWhenSendersDiffer() { final Transaction transaction1 = transactionWithNonceAndSender(0, KEYS1); final Transaction transaction2 = transactionWithNonceAndSender(1, KEYS2); - transactions.addLocalTransaction(transaction1); - transactions.addLocalTransaction(transaction2); + transactions.addLocalTransaction(transaction1, Optional.empty()); + transactions.addLocalTransaction(transaction2, Optional.empty()); final List iterationOrder = new ArrayList<>(); transactions.selectTransactions( @@ -582,10 +587,10 @@ public void shouldNotIncreasePriorityOfTransactionsBecauseOfNonceOrder() { final Transaction transaction3 = transactionWithNonceAndSender(2, KEYS1); final Transaction transaction4 = transactionWithNonceAndSender(5, KEYS2); - transactions.addLocalTransaction(transaction1); - transactions.addLocalTransaction(transaction4); - transactions.addLocalTransaction(transaction2); - transactions.addLocalTransaction(transaction3); + transactions.addLocalTransaction(transaction1, Optional.empty()); + transactions.addLocalTransaction(transaction4, Optional.empty()); + transactions.addLocalTransaction(transaction2, Optional.empty()); + transactions.addLocalTransaction(transaction3, Optional.empty()); final List iterationOrder = new ArrayList<>(); transactions.selectTransactions( @@ -642,9 +647,9 @@ public void shouldEvictMultipleOldTransactions() { metricsSystem, () -> null); - transactions.addRemoteTransaction(transaction1); + transactions.addRemoteTransaction(transaction1, Optional.empty()); assertThat(transactions.size()).isEqualTo(1); - transactions.addRemoteTransaction(transaction2); + transactions.addRemoteTransaction(transaction2, Optional.empty()); assertThat(transactions.size()).isEqualTo(2); clock.step(2L, ChronoUnit.HOURS); @@ -664,7 +669,7 @@ public void shouldEvictSingleOldTransaction() { clock, metricsSystem, () -> null); - transactions.addRemoteTransaction(transaction1); + transactions.addRemoteTransaction(transaction1, Optional.empty()); assertThat(transactions.size()).isEqualTo(1); clock.step(2L, ChronoUnit.HOURS); transactions.evictOldTransactions(); @@ -683,10 +688,10 @@ public void shouldEvictExclusivelyOldTransactions() { clock, metricsSystem, () -> null); - transactions.addRemoteTransaction(transaction1); + transactions.addRemoteTransaction(transaction1, Optional.empty()); assertThat(transactions.size()).isEqualTo(1); clock.step(3L, ChronoUnit.HOURS); - transactions.addRemoteTransaction(transaction2); + transactions.addRemoteTransaction(transaction2, Optional.empty()); assertThat(transactions.size()).isEqualTo(2); transactions.evictOldTransactions(); assertThat(transactions.size()).isEqualTo(1); @@ -695,12 +700,13 @@ public void shouldEvictExclusivelyOldTransactions() { @Test public void shouldNotIncrementAddedCounterWhenRemoteTransactionAlreadyPresent() { - transactions.addLocalTransaction(transaction1); + transactions.addLocalTransaction(transaction1, Optional.empty()); assertThat(transactions.size()).isEqualTo(1); assertThat(metricsSystem.getCounterValue(ADDED_COUNTER, LOCAL)).isEqualTo(1); assertThat(metricsSystem.getCounterValue(ADDED_COUNTER, REMOTE)).isEqualTo(0); - assertThat(transactions.addRemoteTransaction(transaction1)).isFalse(); + assertThat(transactions.addRemoteTransaction(transaction1, Optional.empty())) + .isEqualTo(ALREADY_KNOWN); assertThat(transactions.size()).isEqualTo(1); assertThat(metricsSystem.getCounterValue(ADDED_COUNTER, LOCAL)).isEqualTo(1); assertThat(metricsSystem.getCounterValue(ADDED_COUNTER, REMOTE)).isEqualTo(0); @@ -708,13 +714,13 @@ public void shouldNotIncrementAddedCounterWhenRemoteTransactionAlreadyPresent() @Test public void shouldNotIncrementAddedCounterWhenLocalTransactionAlreadyPresent() { - transactions.addRemoteTransaction(transaction1); + transactions.addRemoteTransaction(transaction1, Optional.empty()); assertThat(transactions.size()).isEqualTo(1); assertThat(metricsSystem.getCounterValue(ADDED_COUNTER, LOCAL)).isEqualTo(0); assertThat(metricsSystem.getCounterValue(ADDED_COUNTER, REMOTE)).isEqualTo(1); - assertThat(transactions.addLocalTransaction(transaction1)) - .isEqualTo(TransactionAddedStatus.ALREADY_KNOWN); + assertThat(transactions.addLocalTransaction(transaction1, Optional.empty())) + .isEqualTo(ALREADY_KNOWN); assertThat(transactions.size()).isEqualTo(1); assertThat(metricsSystem.getCounterValue(ADDED_COUNTER, LOCAL)).isEqualTo(0); assertThat(metricsSystem.getCounterValue(ADDED_COUNTER, REMOTE)).isEqualTo(1); @@ -761,7 +767,7 @@ public void assertThatCorrectNonceIsReturnedWithRepeatedTXes() { private void addLocalTransactions(final long... nonces) { for (final long nonce : nonces) { - transactions.addLocalTransaction(createTransaction(nonce)); + transactions.addLocalTransaction(createTransaction(nonce), Optional.empty()); } } diff --git a/ethereum/eth/src/test/java/org/hyperledger/besu/ethereum/eth/transactions/PendingMultiTypesTransactionsTest.java b/ethereum/eth/src/test/java/org/hyperledger/besu/ethereum/eth/transactions/PendingMultiTypesTransactionsTest.java index a1aba4b07ed..6d879d02c24 100644 --- a/ethereum/eth/src/test/java/org/hyperledger/besu/ethereum/eth/transactions/PendingMultiTypesTransactionsTest.java +++ b/ethereum/eth/src/test/java/org/hyperledger/besu/ethereum/eth/transactions/PendingMultiTypesTransactionsTest.java @@ -15,6 +15,7 @@ package org.hyperledger.besu.ethereum.eth.transactions; import static org.assertj.core.api.Assertions.assertThat; +import static org.hyperledger.besu.ethereum.eth.transactions.sorter.AbstractPendingTransactionsSorter.TransactionAddedStatus.ALREADY_KNOWN; import static org.mockito.Mockito.mock; import static org.mockito.Mockito.when; @@ -72,14 +73,14 @@ public class PendingMultiTypesTransactionsTest { @Test public void shouldReturnExclusivelyLocal1559TransactionsWhenAppropriate() { final Transaction localTransaction0 = create1559Transaction(0, 19, 20, KEYS1); - transactions.addLocalTransaction(localTransaction0); + transactions.addLocalTransaction(localTransaction0, Optional.empty()); assertThat(transactions.size()).isEqualTo(1); List localTransactions = transactions.getLocalTransactions(); assertThat(localTransactions.size()).isEqualTo(1); final Transaction remoteTransaction1 = create1559Transaction(1, 19, 20, KEYS1); - transactions.addRemoteTransaction(remoteTransaction1); + transactions.addRemoteTransaction(remoteTransaction1, Optional.empty()); assertThat(transactions.size()).isEqualTo(2); localTransactions = transactions.getLocalTransactions(); @@ -94,15 +95,15 @@ public void shouldReplaceTransactionWithLowestMaxFeePerGas() { final Transaction localTransaction3 = create1559Transaction(0, 240, 20, KEYS4); final Transaction localTransaction4 = create1559Transaction(0, 260, 20, KEYS5); final Transaction localTransaction5 = create1559Transaction(0, 900, 20, KEYS6); - transactions.addLocalTransaction(localTransaction0); - transactions.addLocalTransaction(localTransaction1); - transactions.addLocalTransaction(localTransaction2); - transactions.addLocalTransaction(localTransaction3); - transactions.addLocalTransaction(localTransaction4); + transactions.addLocalTransaction(localTransaction0, Optional.empty()); + transactions.addLocalTransaction(localTransaction1, Optional.empty()); + transactions.addLocalTransaction(localTransaction2, Optional.empty()); + transactions.addLocalTransaction(localTransaction3, Optional.empty()); + transactions.addLocalTransaction(localTransaction4, Optional.empty()); transactions.updateBaseFee(Wei.of(300L)); - transactions.addLocalTransaction(localTransaction5); + transactions.addLocalTransaction(localTransaction5, Optional.empty()); assertThat(transactions.size()).isEqualTo(5); transactions.selectTransactions( @@ -120,12 +121,12 @@ public void shouldEvictTransactionWithLowestMaxFeePerGasAndLowestTip() { final Transaction localTransaction3 = create1559Transaction(0, 240, 20, KEYS4); final Transaction localTransaction4 = create1559Transaction(0, 260, 20, KEYS5); final Transaction localTransaction5 = create1559Transaction(0, 900, 20, KEYS6); - transactions.addLocalTransaction(localTransaction0); - transactions.addLocalTransaction(localTransaction1); - transactions.addLocalTransaction(localTransaction2); - transactions.addLocalTransaction(localTransaction3); - transactions.addLocalTransaction(localTransaction4); - transactions.addLocalTransaction(localTransaction5); // causes eviction + transactions.addLocalTransaction(localTransaction0, Optional.empty()); + transactions.addLocalTransaction(localTransaction1, Optional.empty()); + transactions.addLocalTransaction(localTransaction2, Optional.empty()); + transactions.addLocalTransaction(localTransaction3, Optional.empty()); + transactions.addLocalTransaction(localTransaction4, Optional.empty()); + transactions.addLocalTransaction(localTransaction5, Optional.empty()); // causes eviction assertThat(transactions.size()).isEqualTo(5); @@ -144,12 +145,12 @@ public void shouldEvictLegacyTransactionWithLowestEffectiveMaxPriorityFeePerGas( final Transaction localTransaction3 = create1559Transaction(0, 240, 20, KEYS4); final Transaction localTransaction4 = create1559Transaction(0, 260, 20, KEYS5); final Transaction localTransaction5 = create1559Transaction(0, 900, 20, KEYS6); - transactions.addLocalTransaction(localTransaction0); - transactions.addLocalTransaction(localTransaction1); - transactions.addLocalTransaction(localTransaction2); - transactions.addLocalTransaction(localTransaction3); - transactions.addLocalTransaction(localTransaction4); - transactions.addLocalTransaction(localTransaction5); // causes eviction + transactions.addLocalTransaction(localTransaction0, Optional.empty()); + transactions.addLocalTransaction(localTransaction1, Optional.empty()); + transactions.addLocalTransaction(localTransaction2, Optional.empty()); + transactions.addLocalTransaction(localTransaction3, Optional.empty()); + transactions.addLocalTransaction(localTransaction4, Optional.empty()); + transactions.addLocalTransaction(localTransaction5, Optional.empty()); // causes eviction assertThat(transactions.size()).isEqualTo(5); transactions.selectTransactions( @@ -167,12 +168,12 @@ public void shouldEvictEIP1559TransactionWithLowestEffectiveMaxPriorityFeePerGas final Transaction localTransaction3 = create1559Transaction(0, 240, 20, KEYS4); final Transaction localTransaction4 = create1559Transaction(0, 260, 20, KEYS5); final Transaction localTransaction5 = create1559Transaction(0, 900, 20, KEYS6); - transactions.addLocalTransaction(localTransaction0); - transactions.addLocalTransaction(localTransaction1); - transactions.addLocalTransaction(localTransaction2); - transactions.addLocalTransaction(localTransaction3); - transactions.addLocalTransaction(localTransaction4); - transactions.addLocalTransaction(localTransaction5); // causes eviction + transactions.addLocalTransaction(localTransaction0, Optional.empty()); + transactions.addLocalTransaction(localTransaction1, Optional.empty()); + transactions.addLocalTransaction(localTransaction2, Optional.empty()); + transactions.addLocalTransaction(localTransaction3, Optional.empty()); + transactions.addLocalTransaction(localTransaction4, Optional.empty()); + transactions.addLocalTransaction(localTransaction5, Optional.empty()); // causes eviction assertThat(transactions.size()).isEqualTo(5); transactions.selectTransactions( @@ -188,9 +189,9 @@ public void shouldChangePriorityWhenBaseFeeIncrease() { final Transaction localTransaction1 = create1559Transaction(1, 100, 20, KEYS2); final Transaction localTransaction2 = create1559Transaction(2, 100, 19, KEYS2); - transactions.addLocalTransaction(localTransaction0); - transactions.addLocalTransaction(localTransaction1); - transactions.addLocalTransaction(localTransaction2); + transactions.addLocalTransaction(localTransaction0, Optional.empty()); + transactions.addLocalTransaction(localTransaction1, Optional.empty()); + transactions.addLocalTransaction(localTransaction2, Optional.empty()); final List iterationOrder = new ArrayList<>(); transactions.selectTransactions( @@ -223,9 +224,9 @@ public void shouldChangePriorityWhenBaseFeeDecrease() { transactions.updateBaseFee(Wei.of(110L)); - transactions.addLocalTransaction(localTransaction0); - transactions.addLocalTransaction(localTransaction1); - transactions.addLocalTransaction(localTransaction2); + transactions.addLocalTransaction(localTransaction0, Optional.empty()); + transactions.addLocalTransaction(localTransaction1, Optional.empty()); + transactions.addLocalTransaction(localTransaction2, Optional.empty()); final List iterationOrder = new ArrayList<>(); transactions.selectTransactions( @@ -257,10 +258,10 @@ public void shouldCorrectlyPrioritizeMultipleTransactionTypesBasedOnNonce() { final Transaction localTransaction2 = create1559Transaction(2, 100, 19, KEYS2); final Transaction localTransaction3 = createLegacyTransaction(0, 20, KEYS1); - transactions.addLocalTransaction(localTransaction0); - transactions.addLocalTransaction(localTransaction1); - transactions.addLocalTransaction(localTransaction2); - transactions.addLocalTransaction(localTransaction3); + transactions.addLocalTransaction(localTransaction0, Optional.empty()); + transactions.addLocalTransaction(localTransaction1, Optional.empty()); + transactions.addLocalTransaction(localTransaction2, Optional.empty()); + transactions.addLocalTransaction(localTransaction3, Optional.empty()); final List iterationOrder = new ArrayList<>(); transactions.selectTransactions( @@ -281,10 +282,10 @@ public void shouldCorrectlyPrioritizeMultipleTransactionTypesBasedOnGasPayed() { final Transaction localTransaction2 = createLegacyTransaction(0, 20, KEYS3); final Transaction localTransaction3 = createLegacyTransaction(1, 2000, KEYS3); - transactions.addLocalTransaction(localTransaction0); - transactions.addLocalTransaction(localTransaction1); - transactions.addLocalTransaction(localTransaction2); - transactions.addLocalTransaction(localTransaction3); + transactions.addLocalTransaction(localTransaction0, Optional.empty()); + transactions.addLocalTransaction(localTransaction1, Optional.empty()); + transactions.addLocalTransaction(localTransaction2, Optional.empty()); + transactions.addLocalTransaction(localTransaction3, Optional.empty()); final List iterationOrder = new ArrayList<>(); transactions.selectTransactions( @@ -313,12 +314,12 @@ public void shouldSelectNoTransactionsIfPoolEmpty() { @Test public void shouldAdd1559Transaction() { final Transaction remoteTransaction0 = create1559Transaction(0, 19, 20, KEYS1); - transactions.addRemoteTransaction(remoteTransaction0); + transactions.addRemoteTransaction(remoteTransaction0, Optional.empty()); assertThat(transactions.size()).isEqualTo(1); assertThat(metricsSystem.getCounterValue(ADDED_COUNTER, REMOTE)).isEqualTo(1); final Transaction remoteTransaction1 = create1559Transaction(1, 19, 20, KEYS1); - transactions.addRemoteTransaction(remoteTransaction1); + transactions.addRemoteTransaction(remoteTransaction1, Optional.empty()); assertThat(transactions.size()).isEqualTo(2); assertThat(metricsSystem.getCounterValue(ADDED_COUNTER, REMOTE)).isEqualTo(2); } @@ -326,12 +327,13 @@ public void shouldAdd1559Transaction() { @Test public void shouldNotIncrementAddedCounterWhenRemote1559TransactionAlreadyPresent() { final Transaction localTransaction0 = create1559Transaction(0, 19, 20, KEYS1); - transactions.addLocalTransaction(localTransaction0); + transactions.addLocalTransaction(localTransaction0, Optional.empty()); assertThat(transactions.size()).isEqualTo(1); assertThat(metricsSystem.getCounterValue(ADDED_COUNTER, LOCAL)).isEqualTo(1); assertThat(metricsSystem.getCounterValue(ADDED_COUNTER, REMOTE)).isEqualTo(0); - assertThat(transactions.addRemoteTransaction(localTransaction0)).isFalse(); + assertThat(transactions.addRemoteTransaction(localTransaction0, Optional.empty())) + .isEqualTo(ALREADY_KNOWN); assertThat(transactions.size()).isEqualTo(1); assertThat(metricsSystem.getCounterValue(ADDED_COUNTER, LOCAL)).isEqualTo(1); assertThat(metricsSystem.getCounterValue(ADDED_COUNTER, REMOTE)).isEqualTo(0); @@ -340,12 +342,12 @@ public void shouldNotIncrementAddedCounterWhenRemote1559TransactionAlreadyPresen @Test public void shouldAddMixedTransactions() { final Transaction remoteTransaction0 = create1559Transaction(0, 19, 20, KEYS1); - transactions.addRemoteTransaction(remoteTransaction0); + transactions.addRemoteTransaction(remoteTransaction0, Optional.empty()); assertThat(transactions.size()).isEqualTo(1); assertThat(metricsSystem.getCounterValue(ADDED_COUNTER, REMOTE)).isEqualTo(1); final Transaction remoteTransaction1 = createLegacyTransaction(1, 5000, KEYS1); - transactions.addRemoteTransaction(remoteTransaction1); + transactions.addRemoteTransaction(remoteTransaction1, Optional.empty()); assertThat(transactions.size()).isEqualTo(2); assertThat(metricsSystem.getCounterValue(ADDED_COUNTER, REMOTE)).isEqualTo(2); } diff --git a/ethereum/eth/src/test/java/org/hyperledger/besu/ethereum/eth/transactions/TransactionPoolTest.java b/ethereum/eth/src/test/java/org/hyperledger/besu/ethereum/eth/transactions/TransactionPoolTest.java index a847aaa9d41..060c54cdeeb 100644 --- a/ethereum/eth/src/test/java/org/hyperledger/besu/ethereum/eth/transactions/TransactionPoolTest.java +++ b/ethereum/eth/src/test/java/org/hyperledger/besu/ethereum/eth/transactions/TransactionPoolTest.java @@ -208,13 +208,13 @@ public void mainNetValueTransferSucceeds() { @Test public void shouldReturnExclusivelyLocalTransactionsWhenAppropriate() { final Transaction localTransaction0 = createTransaction(0); - transactions.addLocalTransaction(localTransaction0); + transactions.addLocalTransaction(localTransaction0, Optional.empty()); assertThat(transactions.size()).isEqualTo(1); - transactions.addRemoteTransaction(transaction1); + transactions.addRemoteTransaction(transaction1, Optional.empty()); assertThat(transactions.size()).isEqualTo(2); - transactions.addRemoteTransaction(transaction2); + transactions.addRemoteTransaction(transaction2, Optional.empty()); assertThat(transactions.size()).isEqualTo(3); List localTransactions = transactions.getLocalTransactions(); @@ -223,7 +223,7 @@ public void shouldReturnExclusivelyLocalTransactionsWhenAppropriate() { @Test public void shouldRemoveTransactionsFromPendingListWhenIncludedInBlockOnchain() { - transactions.addRemoteTransaction(transaction1); + transactions.addRemoteTransaction(transaction1, Optional.empty()); assertTransactionPending(transaction1); appendBlock(transaction1); @@ -232,8 +232,8 @@ public void shouldRemoveTransactionsFromPendingListWhenIncludedInBlockOnchain() @Test public void shouldRemoveMultipleTransactionsAddedInOneBlock() { - transactions.addRemoteTransaction(transaction1); - transactions.addRemoteTransaction(transaction2); + transactions.addRemoteTransaction(transaction1, Optional.empty()); + transactions.addRemoteTransaction(transaction2, Optional.empty()); appendBlock(transaction1, transaction2); assertTransactionNotPending(transaction1); @@ -243,7 +243,7 @@ public void shouldRemoveMultipleTransactionsAddedInOneBlock() { @Test public void shouldIgnoreUnknownTransactionsThatAreAddedInABlock() { - transactions.addRemoteTransaction(transaction1); + transactions.addRemoteTransaction(transaction1, Optional.empty()); appendBlock(transaction1, transaction2); assertTransactionNotPending(transaction1); @@ -253,7 +253,7 @@ public void shouldIgnoreUnknownTransactionsThatAreAddedInABlock() { @Test public void shouldNotRemovePendingTransactionsWhenABlockAddedToAFork() { - transactions.addRemoteTransaction(transaction1); + transactions.addRemoteTransaction(transaction1, Optional.empty()); final BlockHeader commonParent = getHeaderForCurrentChainHead(); final Block canonicalHead = appendBlock(Difficulty.of(1000), commonParent); appendBlock(Difficulty.ONE, commonParent, transaction1); @@ -265,8 +265,8 @@ public void shouldNotRemovePendingTransactionsWhenABlockAddedToAFork() { @Test public void shouldRemovePendingTransactionsFromAllBlocksOnAForkWhenItBecomesTheCanonicalChain() { - transactions.addRemoteTransaction(transaction1); - transactions.addRemoteTransaction(transaction2); + transactions.addRemoteTransaction(transaction1, Optional.empty()); + transactions.addRemoteTransaction(transaction2, Optional.empty()); final BlockHeader commonParent = getHeaderForCurrentChainHead(); final Block originalChainHead = appendBlock(Difficulty.of(1000), commonParent); @@ -284,8 +284,8 @@ public void shouldRemovePendingTransactionsFromAllBlocksOnAForkWhenItBecomesTheC public void shouldReadTransactionsFromThePreviousCanonicalHeadWhenAReorgOccurs() { givenTransactionIsValid(transaction1); givenTransactionIsValid(transaction2); - transactions.addRemoteTransaction(transaction1); - transactions.addRemoteTransaction(transaction2); + transactions.addRemoteTransaction(transaction1, Optional.empty()); + transactions.addRemoteTransaction(transaction2, Optional.empty()); final BlockHeader commonParent = getHeaderForCurrentChainHead(); final Block originalFork1 = appendBlock(Difficulty.of(1000), commonParent, transaction1); final Block originalFork2 = @@ -311,8 +311,8 @@ public void shouldReadTransactionsFromThePreviousCanonicalHeadWhenAReorgOccurs() public void shouldNotReadTransactionsThatAreInBothForksWhenReorgHappens() { givenTransactionIsValid(transaction1); givenTransactionIsValid(transaction2); - transactions.addRemoteTransaction(transaction1); - transactions.addRemoteTransaction(transaction2); + transactions.addRemoteTransaction(transaction1, Optional.empty()); + transactions.addRemoteTransaction(transaction2, Optional.empty()); final BlockHeader commonParent = getHeaderForCurrentChainHead(); final Block originalFork1 = appendBlock(Difficulty.of(1000), commonParent, transaction1); final Block originalFork2 = @@ -440,7 +440,7 @@ public void shouldNotAddRemoteTransactionsWhenGasPriceBelowMinimum() { } @Test - public void shouldNotAddRemoteTransactionsThatAreInvalidAccordingToInvariantChecks() { + public void shouldNotAddRemoteTransactionsWhenThereIsAnLowestInvalidNonceForTheSender() { givenTransactionIsValid(transaction2); when(transactionValidator.validate(eq(transaction1), any(Optional.class), any())) .thenReturn(ValidationResult.invalid(NONCE_TOO_LOW)); @@ -448,23 +448,22 @@ public void shouldNotAddRemoteTransactionsThatAreInvalidAccordingToInvariantChec transactionPool.addRemoteTransactions(asList(transaction1, transaction2)); assertTransactionNotPending(transaction1); - assertTransactionPending(transaction2); - verify(transactionBroadcaster).onTransactionsAdded(singleton(transaction2)); + assertTransactionNotPending(transaction2); + verify(transactionBroadcaster, never()).onTransactionsAdded(singletonList(transaction2)); } @Test public void shouldNotAddRemoteTransactionsThatAreInvalidAccordingToStateDependentChecks() { + givenTransactionIsValid(transaction1); givenTransactionIsValid(transaction2); - when(transactionValidator.validate(eq(transaction1), any(Optional.class), any())) - .thenReturn(valid()); when(transactionValidator.validateForSender( - eq(transaction1), eq(null), any(TransactionValidationParams.class))) + eq(transaction2), eq(null), any(TransactionValidationParams.class))) .thenReturn(ValidationResult.invalid(NONCE_TOO_LOW)); transactionPool.addRemoteTransactions(asList(transaction1, transaction2)); - assertTransactionNotPending(transaction1); - assertTransactionPending(transaction2); - verify(transactionBroadcaster).onTransactionsAdded(singleton(transaction2)); + assertTransactionPending(transaction1); + assertTransactionNotPending(transaction2); + verify(transactionBroadcaster).onTransactionsAdded(singletonList(transaction1)); verify(transactionValidator).validate(eq(transaction1), any(Optional.class), any()); verify(transactionValidator) .validateForSender(eq(transaction1), eq(null), any(TransactionValidationParams.class)); @@ -545,7 +544,6 @@ public void shouldDiscardRemoteTransactionThatAlreadyExistsBeforeValidation() { TransactionPoolConfiguration.DEFAULT); when(pendingTransactions.containsTransaction(transaction1.getHash())).thenReturn(true); - transactionPool.addRemoteTransactions(singletonList(transaction1)); verify(pendingTransactions).containsTransaction(transaction1.getHash()); @@ -574,8 +572,8 @@ public void shouldNotNotifyBatchListenerWhenRemoteTransactionDoesNotReplaceExist transactionPool.addRemoteTransactions(singletonList(transaction2)); assertTransactionPending(transaction1); - verify(transactionBroadcaster).onTransactionsAdded(singleton(transaction1)); - verify(transactionBroadcaster, never()).onTransactionsAdded(singleton(transaction2)); + verify(transactionBroadcaster).onTransactionsAdded(singletonList(transaction1)); + verify(transactionBroadcaster, never()).onTransactionsAdded(singletonList(transaction2)); } @Test @@ -1167,7 +1165,7 @@ private void assertLocalTransactionValid(final Transaction tx) { private void assertRemoteTransactionValid(final Transaction tx) { transactionPool.addRemoteTransactions(List.of(tx)); - verify(transactionBroadcaster).onTransactionsAdded(singleton(tx)); + verify(transactionBroadcaster).onTransactionsAdded(singletonList(tx)); assertTransactionPending(tx); } }