From 90df5e5564176015c14f4b9d85a28eadd2036cc5 Mon Sep 17 00:00:00 2001 From: Fabio Di Fabio Date: Tue, 4 Feb 2025 10:52:34 +0100 Subject: [PATCH] Not all invalid transient not selected txs should be penalized (#8231) Signed-off-by: Fabio Di Fabio --- .../ProcessingResultTransactionSelector.java | 7 ++++--- .../AbstractBlockTransactionSelectorTest.java | 10 ++++----- .../LayeredPendingTransactionsTest.java | 2 +- plugin-api/build.gradle | 2 +- .../data/TransactionSelectionResult.java | 21 +++++++++++++++---- 5 files changed, 28 insertions(+), 14 deletions(-) diff --git a/ethereum/blockcreation/src/main/java/org/hyperledger/besu/ethereum/blockcreation/txselection/selectors/ProcessingResultTransactionSelector.java b/ethereum/blockcreation/src/main/java/org/hyperledger/besu/ethereum/blockcreation/txselection/selectors/ProcessingResultTransactionSelector.java index 5a9fd6030cd..120ad372a36 100644 --- a/ethereum/blockcreation/src/main/java/org/hyperledger/besu/ethereum/blockcreation/txselection/selectors/ProcessingResultTransactionSelector.java +++ b/ethereum/blockcreation/src/main/java/org/hyperledger/besu/ethereum/blockcreation/txselection/selectors/ProcessingResultTransactionSelector.java @@ -85,14 +85,15 @@ private TransactionSelectionResult transactionSelectionResultForInvalidResult( final ValidationResult invalidReasonValidationResult) { final TransactionInvalidReason invalidReason = invalidReasonValidationResult.getInvalidReason(); - // If the invalid reason is transient, then leave the transaction in the pool and continue + // If the invalid reason is transient, then penalize but leave the transaction in the pool and + // continue if (isTransientValidationError(invalidReason)) { LOG.atTrace() - .setMessage("Transient validation error {} for transaction {} keeping it in the pool") + .setMessage("Transient validation error {} for transaction {}, penalize it in the pool") .addArgument(invalidReason) .addArgument(transaction::toTraceLog) .log(); - return TransactionSelectionResult.invalidTransient(invalidReason.name()); + return TransactionSelectionResult.invalidPenalized(invalidReason.name()); } // If the transaction was invalid for any other reason, delete it, and continue. LOG.atTrace() diff --git a/ethereum/blockcreation/src/test/java/org/hyperledger/besu/ethereum/blockcreation/AbstractBlockTransactionSelectorTest.java b/ethereum/blockcreation/src/test/java/org/hyperledger/besu/ethereum/blockcreation/AbstractBlockTransactionSelectorTest.java index 8d9b51b97ff..9875a0149c0 100644 --- a/ethereum/blockcreation/src/test/java/org/hyperledger/besu/ethereum/blockcreation/AbstractBlockTransactionSelectorTest.java +++ b/ethereum/blockcreation/src/test/java/org/hyperledger/besu/ethereum/blockcreation/AbstractBlockTransactionSelectorTest.java @@ -852,7 +852,7 @@ public void transactionWithIncorrectNonceRemainsInPoolAndNotSelected() { .containsOnly( entry( futureTransaction, - TransactionSelectionResult.invalidTransient( + TransactionSelectionResult.invalidPenalized( TransactionInvalidReason.NONCE_TOO_HIGH.name()))); } @@ -1035,13 +1035,13 @@ private void internalBlockSelectionTimeoutSimulation( try { Thread.sleep(t); } catch (final InterruptedException e) { - return TransactionSelectionResult.invalidTransient(EXECUTION_INTERRUPTED.name()); + return TransactionSelectionResult.invalidPenalized(EXECUTION_INTERRUPTED.name()); } } else { try { Thread.sleep(fastProcessingTxTime); } catch (final InterruptedException e) { - return TransactionSelectionResult.invalidTransient(EXECUTION_INTERRUPTED.name()); + return TransactionSelectionResult.invalidPenalized(EXECUTION_INTERRUPTED.name()); } } return SELECTED; @@ -1190,13 +1190,13 @@ private void internalBlockSelectionTimeoutSimulationInvalidTxs( try { Thread.sleep(t); } catch (final InterruptedException e) { - return TransactionSelectionResult.invalidTransient(EXECUTION_INTERRUPTED.name()); + return TransactionSelectionResult.invalidPenalized(EXECUTION_INTERRUPTED.name()); } } else { try { Thread.sleep(fastProcessingTxTime); } catch (final InterruptedException e) { - return TransactionSelectionResult.invalidTransient(EXECUTION_INTERRUPTED.name()); + return TransactionSelectionResult.invalidPenalized(EXECUTION_INTERRUPTED.name()); } } return invalidSelectionResult; diff --git a/ethereum/eth/src/test/java/org/hyperledger/besu/ethereum/eth/transactions/layered/LayeredPendingTransactionsTest.java b/ethereum/eth/src/test/java/org/hyperledger/besu/ethereum/eth/transactions/layered/LayeredPendingTransactionsTest.java index 4c87b374a84..7f41a206732 100644 --- a/ethereum/eth/src/test/java/org/hyperledger/besu/ethereum/eth/transactions/layered/LayeredPendingTransactionsTest.java +++ b/ethereum/eth/src/test/java/org/hyperledger/besu/ethereum/eth/transactions/layered/LayeredPendingTransactionsTest.java @@ -537,7 +537,7 @@ public void temporarilyInvalidTransactionIsKeptInPendingTransactions() { pendingTransactions.selectTransactions( pendingTx -> { parsedTransactions.add(pendingTx); - return TransactionSelectionResult.invalidTransient( + return TransactionSelectionResult.invalidPenalized( GAS_PRICE_BELOW_CURRENT_BASE_FEE.name()); }); diff --git a/plugin-api/build.gradle b/plugin-api/build.gradle index 7d141da905f..097e0c2da42 100644 --- a/plugin-api/build.gradle +++ b/plugin-api/build.gradle @@ -71,7 +71,7 @@ Calculated : ${currentHash} tasks.register('checkAPIChanges', FileStateChecker) { description = "Checks that the API for the Plugin-API project does not change without deliberate thought" files = sourceSets.main.allJava.files - knownHash = 'CrhgK6qq7Ym5AARLCQ0lQvZQpsejUgoFcnL1rU+ZGBs=' + knownHash = 'FEieWer0x6AdCmvf02G7yGQxS5JRxsIYrRDpqsNgQ+0=' } check.dependsOn('checkAPIChanges') diff --git a/plugin-api/src/main/java/org/hyperledger/besu/plugin/data/TransactionSelectionResult.java b/plugin-api/src/main/java/org/hyperledger/besu/plugin/data/TransactionSelectionResult.java index b2fb5ab0c77..3bf162c9981 100644 --- a/plugin-api/src/main/java/org/hyperledger/besu/plugin/data/TransactionSelectionResult.java +++ b/plugin-api/src/main/java/org/hyperledger/besu/plugin/data/TransactionSelectionResult.java @@ -66,7 +66,8 @@ private enum BaseStatus implements Status { BLOCK_SELECTION_TIMEOUT_INVALID_TX(true, true, true), TX_EVALUATION_TOO_LONG(true, false, true), INVALID_TX_EVALUATION_TOO_LONG(true, true, true), - INVALID_TRANSIENT(false, false, true), + INVALID_TRANSIENT(false, false, false), + INVALID_PENALIZED(false, false, true), INVALID(false, true, false); private final boolean stop; @@ -160,21 +161,21 @@ public boolean penalize() { * price, but the selection should continue. */ public static final TransactionSelectionResult CURRENT_TX_PRICE_BELOW_MIN = - TransactionSelectionResult.invalidTransient("CURRENT_TX_PRICE_BELOW_MIN"); + TransactionSelectionResult.invalidPenalized("CURRENT_TX_PRICE_BELOW_MIN"); /** * The transaction has not been selected since its blob price is below the current network blob * price, but the selection should continue. */ public static final TransactionSelectionResult BLOB_PRICE_BELOW_CURRENT_MIN = - TransactionSelectionResult.invalidTransient("BLOB_PRICE_BELOW_CURRENT_MIN"); + TransactionSelectionResult.invalidPenalized("BLOB_PRICE_BELOW_CURRENT_MIN"); /** * The transaction has not been selected since its priority fee is below the configured min * priority fee per gas, but the selection should continue. */ public static final TransactionSelectionResult PRIORITY_FEE_PER_GAS_BELOW_CURRENT_MIN = - TransactionSelectionResult.invalidTransient("PRIORITY_FEE_PER_GAS_BELOW_CURRENT_MIN"); + TransactionSelectionResult.invalidPenalized("PRIORITY_FEE_PER_GAS_BELOW_CURRENT_MIN"); /** * The transaction has not been selected since its sender already had a previous transaction not @@ -217,6 +218,18 @@ public static TransactionSelectionResult invalidTransient(final String invalidRe return new TransactionSelectionResult(BaseStatus.INVALID_TRANSIENT, invalidReason); } + /** + * Return a selection result that identify the candidate transaction as temporarily invalid and + * that it should be penalized, this means that the transaction could become valid at a later + * time. + * + * @param invalidReason the reason why transaction is invalid + * @return the selection result + */ + public static TransactionSelectionResult invalidPenalized(final String invalidReason) { + return new TransactionSelectionResult(BaseStatus.INVALID_PENALIZED, invalidReason); + } + /** * Return a selection result that identify the candidate transaction as permanently invalid, this * means that it could be removed safely from the transaction pool.