Skip to content

Commit 603263a

Browse files
fab-10suraneti
fab-10
authored and
suraneti
committed
Fix for tx incorrectly discarded when there is a timeout during block creation (besu-eth#6563)
 Signed-off-by: Fabio Di Fabio <fabio.difabio@consensys.net>
1 parent 2cf95c6 commit 603263a

File tree

3 files changed

+199
-29
lines changed

3 files changed

+199
-29
lines changed

‎CHANGELOG.md‎

Lines changed: 1 addition & 0 deletions
Original file line numberDiff line numberDiff line change
@@ -38,6 +38,7 @@
3838
### Bug fixes
3939
- Fix the way an advertised host configured with `--p2p-host` is treated when communicating with the originator of a PING packet [#6225](https://github.com/hyperledger/besu/pull/6225)
4040
- Fix `poa-block-txs-selection-max-time` option that was inadvertently reset to its default after being configured [#6444](https://github.com/hyperledger/besu/pull/6444)
41+
- Fix for tx incorrectly discarded when there is a timeout during block creation [#6563](https://github.com/hyperledger/besu/pull/6563)
4142

4243
### Download Links
4344

‎ethereum/blockcreation/src/main/java/org/hyperledger/besu/ethereum/blockcreation/txselection/BlockTransactionSelector.java‎

Lines changed: 36 additions & 28 deletions
Original file line numberDiff line numberDiff line change
@@ -383,33 +383,11 @@ private TransactionSelectionResult handleTransactionSelected(
383383
if (tooLate) {
384384
// even if this tx passed all the checks, it is too late to include it in this block,
385385
// so we need to treat it as not selected
386-
final var evaluationTimer = evaluationContext.getEvaluationTimer();
387-
388-
// check if this tx took too much to evaluate, and in case remove it from the pool
389-
final TransactionSelectionResult timeoutSelectionResult;
390-
if (evaluationTimer.elapsed(TimeUnit.MILLISECONDS) > blockTxsSelectionMaxTime) {
391-
LOG.atWarn()
392-
.setMessage(
393-
"Transaction {} is too late for inclusion, evaluated in {} that is over the max limit of {}ms"
394-
+ ", removing it from the pool")
395-
.addArgument(transaction::toTraceLog)
396-
.addArgument(evaluationTimer)
397-
.addArgument(blockTxsSelectionMaxTime)
398-
.log();
399-
timeoutSelectionResult = TX_EVALUATION_TOO_LONG;
400-
} else {
401-
LOG.atTrace()
402-
.setMessage("Transaction {} is too late for inclusion")
403-
.addArgument(transaction::toTraceLog)
404-
.addArgument(evaluationTimer)
405-
.log();
406-
timeoutSelectionResult = BLOCK_SELECTION_TIMEOUT;
407-
}
408386

409387
// do not rely on the presence of this result, since by the time it is added, the code
410388
// reading it could have been already executed by another thread
411389
return handleTransactionNotSelected(
412-
evaluationContext, timeoutSelectionResult, txWorldStateUpdater);
390+
evaluationContext, BLOCK_SELECTION_TIMEOUT, txWorldStateUpdater);
413391
}
414392

415393
pluginTransactionSelector.onTransactionSelected(evaluationContext, processingResult);
@@ -437,17 +415,47 @@ private TransactionSelectionResult handleTransactionNotSelected(
437415

438416
final var pendingTransaction = evaluationContext.getPendingTransaction();
439417

440-
transactionSelectionResults.updateNotSelected(
441-
evaluationContext.getTransaction(), selectionResult);
442-
pluginTransactionSelector.onTransactionNotSelected(evaluationContext, selectionResult);
418+
// check if this tx took too much to evaluate, and in case remove it from the pool
419+
final TransactionSelectionResult actualResult =
420+
isTimeout.get()
421+
? transactionTookTooLong(evaluationContext)
422+
? TX_EVALUATION_TOO_LONG
423+
: BLOCK_SELECTION_TIMEOUT
424+
: selectionResult;
425+
426+
transactionSelectionResults.updateNotSelected(evaluationContext.getTransaction(), actualResult);
427+
pluginTransactionSelector.onTransactionNotSelected(evaluationContext, actualResult);
443428
LOG.atTrace()
444-
.setMessage("Not selected {} for block creation with result {}, evaluated in {}")
429+
.setMessage(
430+
"Not selected {} for block creation with result {} (original result {}), evaluated in {}")
445431
.addArgument(pendingTransaction::toTraceLog)
432+
.addArgument(actualResult)
446433
.addArgument(selectionResult)
447434
.addArgument(evaluationContext.getEvaluationTimer())
448435
.log();
449436

450-
return selectionResult;
437+
return actualResult;
438+
}
439+
440+
private boolean transactionTookTooLong(final TransactionEvaluationContext evaluationContext) {
441+
final var evaluationTimer = evaluationContext.getEvaluationTimer();
442+
if (evaluationTimer.elapsed(TimeUnit.MILLISECONDS) > blockTxsSelectionMaxTime) {
443+
LOG.atWarn()
444+
.setMessage(
445+
"Transaction {} is too late for inclusion, evaluated in {} that is over the max limit of {}ms"
446+
+ ", removing it from the pool")
447+
.addArgument(evaluationContext.getPendingTransaction()::getHash)
448+
.addArgument(evaluationTimer)
449+
.addArgument(blockTxsSelectionMaxTime)
450+
.log();
451+
return true;
452+
}
453+
LOG.atTrace()
454+
.setMessage("Transaction {} is too late for inclusion")
455+
.addArgument(evaluationContext.getPendingTransaction()::toTraceLog)
456+
.log();
457+
458+
return false;
451459
}
452460

453461
private TransactionSelectionResult handleTransactionNotSelected(

‎ethereum/blockcreation/src/test/java/org/hyperledger/besu/ethereum/blockcreation/AbstractBlockTransactionSelectorTest.java‎

Lines changed: 162 additions & 1 deletion
Original file line numberDiff line numberDiff line change
@@ -18,6 +18,7 @@
1818
import static org.assertj.core.api.Assertions.entry;
1919
import static org.awaitility.Awaitility.await;
2020
import static org.hyperledger.besu.ethereum.core.MiningParameters.DEFAULT_NON_POA_BLOCK_TXS_SELECTION_MAX_TIME;
21+
import static org.hyperledger.besu.ethereum.transaction.TransactionInvalidReason.UPFRONT_COST_EXCEEDS_BALANCE;
2122
import static org.hyperledger.besu.plugin.data.TransactionSelectionResult.BLOCK_SELECTION_TIMEOUT;
2223
import static org.hyperledger.besu.plugin.data.TransactionSelectionResult.PRIORITY_FEE_PER_GAS_BELOW_CURRENT_MIN;
2324
import static org.hyperledger.besu.plugin.data.TransactionSelectionResult.SELECTED;
@@ -90,6 +91,7 @@
9091
import java.time.Instant;
9192
import java.util.ArrayList;
9293
import java.util.Arrays;
94+
import java.util.HashMap;
9395
import java.util.List;
9496
import java.util.Optional;
9597
import java.util.concurrent.CompletableFuture;
@@ -1026,6 +1028,152 @@ private void internalBlockSelectionTimeoutSimulation(
10261028
.isEqualTo(isLongProcessingTxDropped ? true : false);
10271029
}
10281030

1031+
@ParameterizedTest
1032+
@MethodSource("subsetOfPendingTransactionsIncludedWhenTxSelectionMaxTimeIsOver")
1033+
public void subsetOfInvalidPendingTransactionsIncludedWhenTxSelectionMaxTimeIsOver(
1034+
final boolean isPoa,
1035+
final boolean preProcessingTooLate,
1036+
final boolean processingTooLate,
1037+
final boolean postProcessingTooLate) {
1038+
1039+
internalBlockSelectionTimeoutSimulationInvalidTxs(
1040+
isPoa,
1041+
preProcessingTooLate,
1042+
processingTooLate,
1043+
postProcessingTooLate,
1044+
500,
1045+
BLOCK_SELECTION_TIMEOUT,
1046+
false,
1047+
UPFRONT_COST_EXCEEDS_BALANCE);
1048+
}
1049+
1050+
@ParameterizedTest
1051+
@MethodSource("subsetOfPendingTransactionsIncludedWhenTxSelectionMaxTimeIsOver")
1052+
public void invalidPendingTransactionsThatTakesTooLongToEvaluateIsDroppedFromThePool(
1053+
final boolean isPoa,
1054+
final boolean preProcessingTooLate,
1055+
final boolean processingTooLate,
1056+
final boolean postProcessingTooLate) {
1057+
1058+
internalBlockSelectionTimeoutSimulationInvalidTxs(
1059+
isPoa,
1060+
preProcessingTooLate,
1061+
processingTooLate,
1062+
postProcessingTooLate,
1063+
900,
1064+
TX_EVALUATION_TOO_LONG,
1065+
true,
1066+
UPFRONT_COST_EXCEEDS_BALANCE);
1067+
}
1068+
1069+
private void internalBlockSelectionTimeoutSimulationInvalidTxs(
1070+
final boolean isPoa,
1071+
final boolean preProcessingTooLate,
1072+
final boolean processingTooLate,
1073+
final boolean postProcessingTooLate,
1074+
final long longProcessingTxTime,
1075+
final TransactionSelectionResult longProcessingTxResult,
1076+
final boolean isLongProcessingTxDropped,
1077+
final TransactionInvalidReason txInvalidReason) {
1078+
1079+
final int txCount = 3;
1080+
final long fastProcessingTxTime = 200;
1081+
final var invalidSelectionResult = TransactionSelectionResult.invalid(txInvalidReason.name());
1082+
1083+
final Supplier<Answer<TransactionSelectionResult>> inTime = () -> invocation -> SELECTED;
1084+
1085+
final BiFunction<Transaction, Long, Answer<TransactionSelectionResult>> tooLate =
1086+
(p, t) ->
1087+
invocation -> {
1088+
final org.hyperledger.besu.ethereum.blockcreation.txselection
1089+
.TransactionEvaluationContext
1090+
ctx = invocation.getArgument(0);
1091+
if (ctx.getTransaction().equals(p)) {
1092+
Thread.sleep(t);
1093+
} else {
1094+
Thread.sleep(fastProcessingTxTime);
1095+
}
1096+
return invalidSelectionResult;
1097+
};
1098+
1099+
final ProcessableBlockHeader blockHeader = createBlock(301_000);
1100+
final Address miningBeneficiary = AddressHelpers.ofValue(1);
1101+
final int poaGenesisBlockPeriod = 1;
1102+
final int blockTxsSelectionMaxTime = 750;
1103+
1104+
final List<Transaction> transactionsToInject = new ArrayList<>(txCount);
1105+
for (int i = 0; i < txCount - 1; i++) {
1106+
final Transaction tx = createTransaction(i, Wei.of(7), 100_000);
1107+
transactionsToInject.add(tx);
1108+
if (processingTooLate) {
1109+
ensureTransactionIsInvalid(tx, txInvalidReason, fastProcessingTxTime);
1110+
} else {
1111+
ensureTransactionIsValid(tx);
1112+
}
1113+
}
1114+
1115+
final Transaction lateTx = createTransaction(2, Wei.of(7), 100_000);
1116+
transactionsToInject.add(lateTx);
1117+
if (processingTooLate) {
1118+
ensureTransactionIsInvalid(lateTx, txInvalidReason, longProcessingTxTime);
1119+
} else {
1120+
ensureTransactionIsValid(lateTx);
1121+
}
1122+
1123+
PluginTransactionSelector transactionSelector = mock(PluginTransactionSelector.class);
1124+
when(transactionSelector.evaluateTransactionPreProcessing(any()))
1125+
.thenAnswer(
1126+
preProcessingTooLate ? tooLate.apply(lateTx, longProcessingTxTime) : inTime.get());
1127+
1128+
when(transactionSelector.evaluateTransactionPostProcessing(any(), any()))
1129+
.thenAnswer(
1130+
postProcessingTooLate ? tooLate.apply(lateTx, longProcessingTxTime) : inTime.get());
1131+
1132+
final PluginTransactionSelectorFactory transactionSelectorFactory =
1133+
mock(PluginTransactionSelectorFactory.class);
1134+
when(transactionSelectorFactory.create()).thenReturn(transactionSelector);
1135+
1136+
final BlockTransactionSelector selector =
1137+
createBlockSelectorAndSetupTxPool(
1138+
isPoa
1139+
? createMiningParameters(
1140+
Wei.ZERO,
1141+
MIN_OCCUPANCY_100_PERCENT,
1142+
poaGenesisBlockPeriod,
1143+
PositiveNumber.fromInt(75))
1144+
: createMiningParameters(
1145+
Wei.ZERO,
1146+
MIN_OCCUPANCY_100_PERCENT,
1147+
PositiveNumber.fromInt(blockTxsSelectionMaxTime)),
1148+
transactionProcessor,
1149+
blockHeader,
1150+
miningBeneficiary,
1151+
Wei.ZERO,
1152+
transactionSelectorFactory);
1153+
1154+
transactionPool.addRemoteTransactions(transactionsToInject);
1155+
1156+
final TransactionSelectionResults results = selector.buildTransactionListForBlock();
1157+
1158+
// no tx is selected since all are invalid
1159+
assertThat(results.getSelectedTransactions()).isEmpty();
1160+
1161+
// all txs are not selected so wait until all are evaluated
1162+
// before checking the results
1163+
await().until(() -> results.getNotSelectedTransactions().size() == transactionsToInject.size());
1164+
final var expectedEntries = new HashMap<Transaction, TransactionSelectionResult>();
1165+
for (int i = 0; i < txCount - 1; i++) {
1166+
expectedEntries.put(
1167+
transactionsToInject.get(i), TransactionSelectionResult.invalid(txInvalidReason.name()));
1168+
}
1169+
expectedEntries.put(lateTx, longProcessingTxResult);
1170+
assertThat(results.getNotSelectedTransactions())
1171+
.containsExactlyInAnyOrderEntriesOf(expectedEntries);
1172+
1173+
assertThat(transactionPool.getTransactionByHash(lateTx.getHash()).isEmpty())
1174+
.isEqualTo(isLongProcessingTxDropped ? true : false);
1175+
}
1176+
10291177
private static Stream<Arguments>
10301178
subsetOfPendingTransactionsIncludedWhenTxSelectionMaxTimeIsOver() {
10311179

@@ -1170,9 +1318,22 @@ protected void ensureTransactionIsValid(
11701318

11711319
protected void ensureTransactionIsInvalid(
11721320
final Transaction tx, final TransactionInvalidReason invalidReason) {
1321+
ensureTransactionIsInvalid(tx, invalidReason, 0);
1322+
}
1323+
1324+
protected void ensureTransactionIsInvalid(
1325+
final Transaction tx,
1326+
final TransactionInvalidReason invalidReason,
1327+
final long processingTime) {
11731328
when(transactionProcessor.processTransaction(
11741329
any(), any(), any(), eq(tx), any(), any(), any(), anyBoolean(), any(), any()))
1175-
.thenReturn(TransactionProcessingResult.invalid(ValidationResult.invalid(invalidReason)));
1330+
.thenAnswer(
1331+
invocation -> {
1332+
if (processingTime > 0) {
1333+
Thread.sleep(processingTime);
1334+
}
1335+
return TransactionProcessingResult.invalid(ValidationResult.invalid(invalidReason));
1336+
});
11761337
}
11771338

11781339
private BlockHeader blockHeader(final long number) {

0 commit comments

Comments
 (0)