Skip to content

Commit

Permalink
Implement one approach to eliminating sender TX grouping. Note - this…
Browse files Browse the repository at this point in the history 
… is WIP for discussion with others. Not sure this is the implementation we want.

Signed-off-by: Matthew Whitehead <matthew1001@gmail.com>
  • Loading branch information
@matthew1001
matthew1001 committed Oct 18, 2023
1 parent c474dea commit a8c727a
Show file tree
Hide file tree
Showing 5 changed files with 170 additions and 19 deletions.
15 changes: 10 additions & 5 deletions besu/src/test/java/org/hyperledger/besu/cli/options/stable/TransactionPoolOptionsTest.java
View file
Original file line number Diff line number Diff line change
Expand Up @@ -147,12 +147,17 @@ public void priceBump() {
}

@Test
public void disableSenderTXGrouping() {
final Boolean senderTXGrouping = Boolean.FALSE;
public void disableSenderTXGroupingOn() {
internalTestSuccess(
config -> assertThat(config.getNoSenderTXGrouping()).isEqualTo(senderTXGrouping),
"--tx-pool-disable-sender-grouping",
senderTXGrouping.toString());
config -> assertThat(config.getDisableSenderTXGrouping()).isTrue(),
"--tx-pool-disable-sender-grouping=true");
}

@Test
public void disableSenderTXGroupingOff() {
internalTestSuccess(
config -> assertThat(config.getDisableSenderTXGrouping()).isFalse(),
"--tx-pool-disable-sender-grouping=false");
}

@Test
Expand Down
5 changes: 0 additions & 5 deletions ...ain/java/org/hyperledger/besu/ethereum/eth/transactions/TransactionPoolConfiguration.java
View file
Original file line number Diff line number Diff line change
Expand Up @@ -97,11 +97,6 @@ default Percentage getPriceBump() {
return DEFAULT_PRICE_BUMP;
}

@Value.Default
default Boolean getNoSenderTXGrouping() {
return DEFAULT_DISABLE_SENDER_TX_GROUPING;
}

@Value.Default
default Wei getTxFeeCap() {
return DEFAULT_RPC_TX_FEE_CAP;
Expand Down
37 changes: 36 additions & 1 deletion .../hyperledger/besu/ethereum/eth/transactions/sorter/AbstractPendingTransactionsSorter.java
View file
Original file line number Diff line number Diff line change
Expand Up @@ -252,14 +252,17 @@ public void selectTransactions(final TransactionSelector selector) {
final Set<Transaction> transactionsToRemove = new HashSet<>();
final Map<Address, AccountTransactionOrder> accountTransactions = new HashMap<>();
final Iterator<PendingTransaction> prioritizedTransactions = prioritizedTransactions();

while (prioritizedTransactions.hasNext()) {
final PendingTransaction highestPriorityPendingTransaction = prioritizedTransactions.next();
final AccountTransactionOrder accountTransactionOrder =
accountTransactions.computeIfAbsent(
highestPriorityPendingTransaction.getSender(), this::createSenderTransactionOrder);

for (final PendingTransaction transactionToProcess :
accountTransactionOrder.transactionsToProcess(highestPriorityPendingTransaction)) {
accountTransactionOrder.transactionsToProcess(
highestPriorityPendingTransaction,
poolConfig.getDisableSenderTXGrouping() ? 1 : Integer.MAX_VALUE)) {
final TransactionSelectionResult result =
selector.evaluateTransaction(transactionToProcess);

Expand All @@ -275,6 +278,38 @@ public void selectTransactions(final TransactionSelector selector) {
}
}
}

if (poolConfig.getDisableSenderTXGrouping()) {
// But then do a cheeky extra bit of TX processing...
while (accountTransactions.size() > 0) {
// Get each value, and see if it has a deferred TX...
List<Map.Entry<Address, AccountTransactionOrder>> accounts =
accountTransactions.entrySet().stream().toList();
Address addr = accounts.get(0).getKey();
AccountTransactionOrder nextAccount = accounts.get(0).getValue();
Optional<PendingTransaction> nextTransactionForSender =
nextAccount.getHighestPriorityDeferredTransaction();
if (nextTransactionForSender.isPresent()) {
PendingTransaction transactionToEvaluate = nextTransactionForSender.get();
final TransactionSelectionResult result =
selector.evaluateTransaction(transactionToEvaluate);

if (result.discard()) {
transactionsToRemove.add(transactionToEvaluate.getTransaction());
transactionsToRemove.addAll(
signalInvalidAndGetDependentTransactions(transactionToEvaluate.getTransaction()));
}

if (result.stop()) {
transactionsToRemove.forEach(this::removeTransaction);
return;
}
} else {
// This account is empty - remove from the list
accountTransactions.remove(addr);
}
}
}
transactionsToRemove.forEach(this::removeTransaction);
}
}
Expand Down
21 changes: 20 additions & 1 deletion ...n/java/org/hyperledger/besu/ethereum/eth/transactions/sorter/AccountTransactionOrder.java
View file
Original file line number Diff line number Diff line change
Expand Up @@ -20,6 +20,7 @@
import java.util.Comparator;
import java.util.List;
import java.util.NavigableSet;
import java.util.Optional;
import java.util.TreeSet;
import java.util.stream.Stream;

Expand Down Expand Up @@ -50,16 +51,34 @@ public AccountTransactionOrder(final Stream<PendingTransaction> senderTransactio
*/
public Iterable<PendingTransaction> transactionsToProcess(
final PendingTransaction nextTransactionInPriorityOrder) {
return transactionsToProcess(nextTransactionInPriorityOrder, Integer.MAX_VALUE);
}

public Iterable<PendingTransaction> transactionsToProcess(
final PendingTransaction nextTransactionInPriorityOrder, final int maxTransactionsToReturn) {
deferredTransactions.add(nextTransactionInPriorityOrder);
final List<PendingTransaction> transactionsToApply = new ArrayList<>();
while (!deferredTransactions.isEmpty()
&& !transactionsForSender.isEmpty()
&& deferredTransactions.first().equals(transactionsForSender.first())) {
&& deferredTransactions.first().equals(transactionsForSender.first())
&& transactionsToApply.size() < maxTransactionsToReturn) {
final PendingTransaction transaction = deferredTransactions.first();
transactionsToApply.add(transaction);
deferredTransactions.remove(transaction);
transactionsForSender.remove(transaction);
}
return transactionsToApply;
}

public Optional<PendingTransaction> getHighestPriorityDeferredTransaction() {
Optional<PendingTransaction> transactionToApply = Optional.empty();
if (!deferredTransactions.isEmpty()
&& !transactionsForSender.isEmpty()
&& deferredTransactions.first().equals(transactionsForSender.first())) {
transactionToApply = Optional.of(deferredTransactions.first());
deferredTransactions.remove(transactionToApply.get());
transactionsForSender.remove(transactionToApply.get());
}
return transactionToApply;
}
}
111 changes: 104 additions & 7 deletions ...yperledger/besu/ethereum/eth/transactions/sorter/AbstractPendingTransactionsTestBase.java
View file
Original file line number Diff line number Diff line change
Expand Up @@ -64,6 +64,7 @@
public abstract class AbstractPendingTransactionsTestBase {

protected static final int MAX_TRANSACTIONS = 5;
protected static final int MAX_TRANSACTIONS_LARGE_POOL = 15;
private static final float LIMITED_TRANSACTIONS_BY_SENDER_PERCENTAGE = 0.8f;
protected static final Supplier<SignatureAlgorithm> SIGNATURE_ALGORITHM =
Suppliers.memoize(SignatureAlgorithmFactory::getInstance);
Expand Down Expand Up @@ -92,9 +93,29 @@ public abstract class AbstractPendingTransactionsTestBase {
.build();
protected PendingTransactions senderLimitedTransactions =
getPendingTransactions(senderLimitedConfig, Optional.empty());
protected AbstractPendingTransactionsSorter transactionsLarge =
getPendingTransactions(
ImmutableTransactionPoolConfiguration.builder()
.txPoolMaxSize(MAX_TRANSACTIONS_LARGE_POOL)
.txPoolLimitByAccountPercentage(Fraction.fromFloat(1.0f))
.build(),
Optional.empty());
protected AbstractPendingTransactionsSorter transactionsLargeNoSenderGrouping =
getPendingTransactions(
ImmutableTransactionPoolConfiguration.builder()
.txPoolMaxSize(MAX_TRANSACTIONS_LARGE_POOL)
.disableSenderTXGrouping(true)
.txPoolLimitByAccountPercentage(Fraction.fromFloat(1.0f))
.build(),
Optional.empty());

protected final Transaction transaction1 = createTransaction(2);
protected final Transaction transaction2 = createTransaction(1);
protected final Transaction transaction3 = createTransaction(3);

protected final Transaction transaction1Sdr2 = createTransactionSender2(1);
protected final Transaction transaction2Sdr2 = createTransactionSender2(2);
protected final Transaction transaction3Sdr2 = createTransactionSender2(3);

protected final PendingTransactionAddedListener listener =
mock(PendingTransactionAddedListener.class);
Expand Down Expand Up @@ -309,19 +330,86 @@ public void shouldNotNotifyDroppedListenerWhenTransactionAddedToBlock() {
}

@Test
public void selectTransactionsUntilSelectorRequestsNoMore() {
transactions.addRemoteTransaction(transaction1, Optional.empty());
transactions.addRemoteTransaction(transaction2, Optional.empty());
public void selectTransactionsInDefaultOrder() {
assertThat(transactionsLarge.addRemoteTransaction(transaction1, Optional.empty()))
.isEqualTo(ADDED);
assertThat(transactionsLarge.addRemoteTransaction(transaction2, Optional.empty()))
.isEqualTo(ADDED);
assertThat(transactionsLarge.addRemoteTransaction(transaction1Sdr2, Optional.empty()))
.isEqualTo(ADDED);
assertThat(transactionsLarge.addRemoteTransaction(transaction2Sdr2, Optional.empty()))
.isEqualTo(ADDED);
assertThat(transactionsLarge.addRemoteTransaction(transaction3Sdr2, Optional.empty()))
.isEqualTo(ADDED);
assertThat(transactionsLarge.addRemoteTransaction(transaction3, Optional.empty()))
.isEqualTo(ADDED);

final List<Transaction> parsedTransactions = Lists.newArrayList();
transactions.selectTransactions(
transactionsLarge.selectTransactions(
pendingTx -> {
parsedTransactions.add(pendingTx.getTransaction());
return TransactionSelectionResult.BLOCK_OCCUPANCY_ABOVE_THRESHOLD;

if (parsedTransactions.size() == 6) {
return TransactionSelectionResult.BLOCK_OCCUPANCY_ABOVE_THRESHOLD;
}
return SELECTED;
});

assertThat(parsedTransactions.size()).isEqualTo(1);
assertThat(parsedTransactions.get(0)).isEqualTo(transaction2);
assertThat(parsedTransactions.size()).isEqualTo(6);

assertThat(parsedTransactions.get(0)).isEqualTo(transaction1Sdr2);
assertThat(parsedTransactions.get(1)).isEqualTo(transaction2Sdr2);
assertThat(parsedTransactions.get(2)).isEqualTo(transaction3Sdr2);
assertThat(parsedTransactions.get(3))
.isEqualTo(transaction2); // Transaction 2 is actually the lowest nonce for this sender
assertThat(parsedTransactions.get(4))
.isEqualTo(transaction1); // Transaction 1 is the next nonce for the sender
assertThat(parsedTransactions.get(5))
.isEqualTo(transaction3); // Transaction 3 is the next nonce for the sender
}

@Test
public void selectTransactionsInOrderNoGroupBySender() {
assertThat(
transactionsLargeNoSenderGrouping.addRemoteTransaction(transaction2, Optional.empty()))
.isEqualTo(ADDED);
assertThat(
transactionsLargeNoSenderGrouping.addRemoteTransaction(
transaction1Sdr2, Optional.empty()))
.isEqualTo(ADDED);
assertThat(
transactionsLargeNoSenderGrouping.addRemoteTransaction(
transaction2Sdr2, Optional.empty()))
.isEqualTo(ADDED);
assertThat(
transactionsLargeNoSenderGrouping.addRemoteTransaction(transaction1, Optional.empty()))
.isEqualTo(ADDED);
assertThat(
transactionsLargeNoSenderGrouping.addRemoteTransaction(
transaction3Sdr2, Optional.empty()))
.isEqualTo(ADDED);
assertThat(
transactionsLargeNoSenderGrouping.addRemoteTransaction(transaction3, Optional.empty()))
.isEqualTo(ADDED);

final List<Transaction> parsedTransactions = Lists.newArrayList();
transactionsLargeNoSenderGrouping.selectTransactions(
pendingTx -> {
parsedTransactions.add(pendingTx.getTransaction());

if (parsedTransactions.size() == 6) {
return TransactionSelectionResult.BLOCK_OCCUPANCY_ABOVE_THRESHOLD;
}
return SELECTED;
});

assertThat(parsedTransactions.size()).isEqualTo(6);

// Check that by setting --tx-pool-disable-sender-grouping=true then sdr 1 hasn't monopolized
// the pool selection just because its transaction (transaction 3) is the first one to be parsed
// by the selector
assertThat(parsedTransactions.get(0)).isEqualTo(transaction1Sdr2);
assertThat(parsedTransactions.get(1)).isEqualTo(transaction2);
}

@Test
Expand Down Expand Up @@ -613,9 +701,18 @@ protected Transaction createTransaction(final long transactionNumber) {
return new TransactionTestFixture()
.value(Wei.of(transactionNumber))
.nonce(transactionNumber)
.gasPrice(Wei.of(0))
.createTransaction(KEYS1);
}

protected Transaction createTransactionSender2(final long transactionNumber) {
return new TransactionTestFixture()
.value(Wei.of(transactionNumber))
.nonce(transactionNumber)
.gasPrice(Wei.of(0))
.createTransaction(KEYS2);
}

@Test
public void shouldEvictMultipleOldTransactions() {
final int maxTransactionRetentionHours = 1;
Expand Down

0 comments on commit a8c727a

Please sign in to comment.