CipherLabs
/
besu
mirror of https://github.com/hyperledger/besu
2
0
Fork 0
Code Issues Packages Projects Releases Wiki Activity

Bugfix/revert nonce distance (#2708)

Browse Source 
Signed-off-by: garyschulte <garyschulte@gmail.com>
pull/2712/head
garyschulte 3 years ago committed by GitHub
parent f92cbdc389
commit 3a260b9009
No known key found for this signature in database
GPG Key ID: 4AEE18F83AFDEB23
7 changed files with 27 additions and 122 deletions
Show all changes Ignore whitespace when comparing lines Ignore changes in amount of whitespace Ignore changes in whitespace at EOL
Show Stats Download Patch File Download Diff File
  1. 2
      besu/src/test/java/org/hyperledger/besu/services/BesuEventsImplTest.java
  2. 2
      ethereum/eth/src/main/java/org/hyperledger/besu/ethereum/eth/transactions/TransactionPoolConfiguration.java
  3. 13
      ethereum/eth/src/main/java/org/hyperledger/besu/ethereum/eth/transactions/sorter/AbstractPendingTransactionsSorter.java
  4. 2
      ethereum/eth/src/main/java/org/hyperledger/besu/ethereum/eth/transactions/sorter/BaseFeePendingTransactionsSorter.java
  5. 1
      ethereum/eth/src/main/java/org/hyperledger/besu/ethereum/eth/transactions/sorter/GasPricePendingTransactionsSorter.java
  6. 117
      ethereum/eth/src/test/java/org/hyperledger/besu/ethereum/eth/transactions/BaseFeePendingTransactionsTest.java
  7. 12
      ethereum/eth/src/test/java/org/hyperledger/besu/ethereum/eth/transactions/GasPricePendingTransactionsTest.java

2
besu/src/test/java/org/hyperledger/besu/services/BesuEventsImplTest.java
Unescape View File

@ -120,7 +120,7 @@ public class BesuEventsImplTest {
when(mockEthContext.getEthMessages()).thenReturn(mockEthMessages);
when(mockEthContext.getEthPeers()).thenReturn(mockEthPeers);
when(mockEthContext.getScheduler()).thenReturn(mockEthScheduler);
when(mockEthPeers.streamAvailablePeers()).thenAnswer(__ -> Stream.empty());
when(mockEthPeers.streamAvailablePeers()).thenAnswer(z -> Stream.empty());
when(mockProtocolContext.getBlockchain()).thenReturn(blockchain);
when(mockProtocolContext.getWorldStateArchive()).thenReturn(mockWorldStateArchive);
when(mockProtocolSchedule.getByBlockNumber(anyLong())).thenReturn(mockProtocolSpec);

2
ethereum/eth/src/main/java/org/hyperledger/besu/ethereum/eth/transactions/TransactionPoolConfiguration.java
Unescape View File

@ -25,7 +25,7 @@ import org.immutables.value.Value;
@Value.Style(allParameters = true)
public interface TransactionPoolConfiguration {
int DEFAULT_TX_MSG_KEEP_ALIVE = 60;
int MAX_PENDING_TRANSACTIONS = 4096 * 8;
int MAX_PENDING_TRANSACTIONS = 4096;
int MAX_PENDING_TRANSACTIONS_HASHES = 4096;
int DEFAULT_TX_RETENTION_HOURS = 13;
Percentage DEFAULT_PRICE_BUMP = Percentage.fromInt(10);

13
ethereum/eth/src/main/java/org/hyperledger/besu/ethereum/eth/transactions/sorter/AbstractPendingTransactionsSorter.java
Unescape View File

@ -131,19 +131,6 @@ public abstract class AbstractPendingTransactionsSorter {
pendingTransactions::size);
}
protected Long distanceFromNextNonce(final TransactionInfo incomingTx) {
final TransactionsForSenderInfo inPool = transactionsBySender.get(incomingTx.getSender());
if ((inPool == null)
|| (inPool.streamTransactionInfos().count() < 1)) { // nothing in pool, you're next
return 0L;
}
long minNonceForAccount =
inPool.streamTransactionInfos().mapToLong(TransactionInfo::getNonce).min().getAsLong();
// despite this looking backwards, it produces the sort order we want.
// greater distances produce more negative results, which are then .reversed()
return minNonceForAccount - incomingTx.getNonce();
}
public void evictOldTransactions() {
final Instant removeTransactionsBefore =
clock.instant().minus(maxTransactionRetentionHours, ChronoUnit.HOURS);

2
ethereum/eth/src/main/java/org/hyperledger/besu/ethereum/eth/transactions/sorter/BaseFeePendingTransactionsSorter.java
Unescape View File

@ -85,7 +85,6 @@ public class BaseFeePendingTransactionsSorter extends AbstractPendingTransaction
.get()
.getValue()
.longValue())
.thenComparing(this::distanceFromNextNonce)
.thenComparing(TransactionInfo::getSequence)
.reversed());
@ -99,7 +98,6 @@ public class BaseFeePendingTransactionsSorter extends AbstractPendingTransaction
.getMaxFeePerGas()
.map(maxFeePerGas -> maxFeePerGas.getValue().longValue())
.orElse(transactionInfo.getGasPrice().toLong()))
.thenComparing(this::distanceFromNextNonce)
.thenComparing(TransactionInfo::getSequence)
.reversed());

1
ethereum/eth/src/main/java/org/hyperledger/besu/ethereum/eth/transactions/sorter/GasPricePendingTransactionsSorter.java
Unescape View File

@ -41,7 +41,6 @@ public class GasPricePendingTransactionsSorter extends AbstractPendingTransactio
new TreeSet<>(
comparing(TransactionInfo::isReceivedFromLocalSource)
.thenComparing(TransactionInfo::getGasPrice)
.thenComparing(this::distanceFromNextNonce)
.thenComparing(TransactionInfo::getSequence)
.reversed());

117
ethereum/eth/src/test/java/org/hyperledger/besu/ethereum/eth/transactions/BaseFeePendingTransactionsTest.java
Unescape View File

@ -17,8 +17,8 @@ package org.hyperledger.besu.ethereum.eth.transactions;
import static org.assertj.core.api.Assertions.assertThat;
import static org.mockito.Mockito.mock;
import static org.mockito.Mockito.verify;
import static org.mockito.Mockito.verifyNoInteractions;
import static org.mockito.Mockito.verifyNoMoreInteractions;
import static org.mockito.Mockito.verifyZeroInteractions;
import static org.mockito.Mockito.when;
import org.hyperledger.besu.crypto.KeyPair;
@ -124,30 +124,15 @@ public class BaseFeePendingTransactionsTest {
@Test
public void shouldDropOldestTransactionWhenLimitExceeded() {
final Transaction oldestTransaction =
new TransactionTestFixture()
.value(Wei.of(1L))
.nonce(0L)
.createTransaction(SIGNATURE_ALGORITHM.get().generateKeyPair());
final Transaction oldestTransaction = createTransaction(0);
transactions.addRemoteTransaction(oldestTransaction);
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(createTransaction(i));
}
assertThat(transactions.size()).isEqualTo(MAX_TRANSACTIONS);
assertThat(metricsSystem.getCounterValue(REMOVED_COUNTER, REMOTE, DROPPED)).isZero();
final Transaction lastTransaction =
new TransactionTestFixture()
.value(Wei.of(1L))
.nonce(MAX_TRANSACTIONS + 1)
.createTransaction(SIGNATURE_ALGORITHM.get().generateKeyPair());
transactions.addRemoteTransaction(lastTransaction);
transactions.addRemoteTransaction(createTransaction(MAX_TRANSACTIONS + 1));
assertThat(transactions.size()).isEqualTo(MAX_TRANSACTIONS);
assertTransactionNotPending(oldestTransaction);
assertThat(metricsSystem.getCounterValue(REMOVED_COUNTER, REMOTE, DROPPED)).isEqualTo(1);
@ -170,11 +155,6 @@ public class BaseFeePendingTransactionsTest {
@Test
public void shouldPrioritizeLocalTransaction() {
transactions.subscribeDroppedTransactions(
transaction ->
assertThat(transactions.getLocalTransactions().contains(transaction)).isFalse());
final Transaction localTransaction = createTransaction(0);
transactions.addLocalTransaction(localTransaction);
@ -187,14 +167,9 @@ public class BaseFeePendingTransactionsTest {
@Test
public void shouldPrioritizeGasPriceThenTimeAddedToPool() {
transactions.subscribeDroppedTransactions(
transaction -> assertThat(transaction.getGasPrice().get().toLong()).isLessThan(100));
final List<Transaction> lowGasPriceTransactions =
IntStream.range(0, MAX_TRANSACTIONS)
.mapToObj(
i ->
transactionWithNonceSenderAndGasPrice(
i + 1, SIGNATURE_ALGORITHM.get().generateKeyPair(), 10 + i))
.mapToObj(i -> transactionWithNonceSenderAndGasPrice(i + 1, KEYS1, 10))
.collect(Collectors.toUnmodifiableList());
// Fill the pool
@ -202,8 +177,7 @@ public class BaseFeePendingTransactionsTest {
// 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);
transactionWithNonceSenderAndGasPrice(MAX_TRANSACTIONS + 1, KEYS1, 100);
transactions.addRemoteTransaction(highGasPriceTransaction);
assertThat(transactions.size()).isEqualTo(MAX_TRANSACTIONS);
@ -214,12 +188,14 @@ public class BaseFeePendingTransactionsTest {
@Test
public void shouldStartDroppingLocalTransactionsWhenPoolIsFullOfLocalTransactions() {
transactions.subscribeDroppedTransactions(this::assertTransactionNotPending);
final Transaction firstLocalTransaction = createTransaction(0);
transactions.addLocalTransaction(firstLocalTransaction);
for (int i = 0; i <= MAX_TRANSACTIONS; i++) {
for (int i = 1; i <= MAX_TRANSACTIONS; i++) {
transactions.addLocalTransaction(createTransaction(i));
}
assertThat(transactions.size()).isEqualTo(MAX_TRANSACTIONS);
assertTransactionNotPending(firstLocalTransaction);
}
@Test
@ -233,7 +209,6 @@ public class BaseFeePendingTransactionsTest {
@Test
public void shouldNotNotifyListenerAfterUnsubscribe() {
final long id = transactions.subscribePendingTransactions(listener);
transactions.addRemoteTransaction(transaction1);
@ -241,8 +216,10 @@ public class BaseFeePendingTransactionsTest {
verify(listener).onTransactionAdded(transaction1);
transactions.unsubscribePendingTransactions(id);
verifyNoMoreInteractions(listener);
transactions.addRemoteTransaction(transaction2);
verifyZeroInteractions(listener);
}
@Test
@ -302,7 +279,7 @@ public class BaseFeePendingTransactionsTest {
transactions.transactionAddedToBlock(transaction1);
verifyNoInteractions(droppedListener);
verifyZeroInteractions(droppedListener);
}
@Test
@ -498,7 +475,7 @@ public class BaseFeePendingTransactionsTest {
assertTransactionNotPending(transaction1b);
assertTransactionPending(transaction1);
assertThat(transactions.size()).isEqualTo(1);
verifyNoInteractions(listener);
verifyZeroInteractions(listener);
}
@Test
@ -706,18 +683,18 @@ public class BaseFeePendingTransactionsTest {
@Test
public void assertThatCorrectNonceIsReturned() {
assertThat(transactions.getNextNonceForSender(transaction1.getSender())).isEmpty();
addLocalTransactions(1, 2, 4);
addLocalTransactions(1, 2, 4, 5);
assertThat(transactions.getNextNonceForSender(transaction1.getSender()))
.isPresent()
.hasValue(3);
addLocalTransactions(3);
assertThat(transactions.getNextNonceForSender(transaction1.getSender()))
.isPresent()
.hasValue(5);
addLocalTransactions(5);
.hasValue(6);
addLocalTransactions(6, 10);
assertThat(transactions.getNextNonceForSender(transaction1.getSender()))
.isPresent()
.hasValue(6);
.hasValue(7);
}
@Test
@ -751,60 +728,4 @@ public class BaseFeePendingTransactionsTest {
when(blockHeader.getBaseFee()).thenReturn(Optional.empty());
return blockHeader;
}
@Test
public void shouldIgnoreFutureNoncedTxs() {
// create maxtx transactions with valid addresses/nonces
// all addresses should be unique, chained txs will be checked in another test.
// TODO: how do we test around reorgs? do we?
List<Transaction> toValidate = new ArrayList<>((int) transactions.maxSize());
for (int entries = 1; entries <= transactions.maxSize(); entries++) {
KeyPair kp = SIGNATURE_ALGORITHM.get().generateKeyPair();
Address a = Util.publicKeyToAddress(kp.getPublicKey());
Transaction t =
new TransactionTestFixture()
.sender(a)
.value(Wei.of(2))
.maxPriorityFeePerGas(Optional.of(Wei.of(2L)))
.nonce(entries)
.createTransaction(kp);
transactions.addRemoteTransaction(t);
toValidate.add(t);
}
// create maxtx transaction with nonces in the future, could be any volume though since pool
// already full
List<Transaction> attackTxs = new ArrayList<>();
KeyPair attackerKp = SIGNATURE_ALGORITHM.get().generateKeyPair();
Address attackerA = Util.publicKeyToAddress(attackerKp.getPublicKey());
for (int entries = 10;
entries < transactions.maxSize() + 10;
entries++) { // badguy nonces are 2 digits
Transaction t =
new TransactionTestFixture()
.sender(attackerA)
.value(Wei.of(2))
.nonce(entries)
.maxPriorityFeePerGas(Optional.of(Wei.of(2L)))
.createTransaction(attackerKp);
attackTxs.add(t);
transactions.addRemoteTransaction(t); // all but the last one of these should be dropped
}
// assert txpool contains 1st attack
assertThat(transactions.getTransactionByHash(attackTxs.get(0).getHash())).isNotEmpty();
// assert txpool does not contain rest of attack
attackTxs.stream()
.skip(1L)
.forEach(t -> assertThat(transactions.getTransactionByHash(t.getHash())).isEmpty());
// assert that only 1 of the valid batch was purged
long droppedValidCount =
toValidate.stream()
.filter(t -> transactions.getTransactionByHash(t.getHash()).isEmpty())
.count();
assertThat(droppedValidCount).isEqualTo(1L);
}
}

12
ethereum/eth/src/test/java/org/hyperledger/besu/ethereum/eth/transactions/GasPricePendingTransactionsTest.java
Unescape View File

@ -210,15 +210,15 @@ public class GasPricePendingTransactionsTest {
@Test
public void shouldStartDroppingLocalTransactionsWhenPoolIsFullOfLocalTransactions() {
for (int i = 0; i < MAX_TRANSACTIONS; i++) {
final Transaction firstLocalTransaction = createTransaction(0);
transactions.addLocalTransaction(firstLocalTransaction);
for (int i = 1; i <= MAX_TRANSACTIONS; i++) {
transactions.addLocalTransaction(createTransaction(i));
}
final Transaction lastLocalTransaction = createTransaction(5);
transactions.addLocalTransaction(lastLocalTransaction);
assertThat(transactions.size()).isEqualTo(MAX_TRANSACTIONS);
assertTransactionNotPending(lastLocalTransaction);
assertTransactionNotPending(firstLocalTransaction);
}
@Test
@ -717,7 +717,7 @@ public class GasPricePendingTransactionsTest {
addLocalTransactions(6, 10);
assertThat(transactions.getNextNonceForSender(transaction1.getSender()))
.isPresent()
.hasValue(6);
.hasValue(7);
}
@Test

Write Preview
Loading…
Cancel
Save