From 02754e8136e85023474bd5c0b8e8c75c37bdcefd Mon Sep 17 00:00:00 2001 From: Fabio Di Fabio Date: Wed, 17 Apr 2024 01:31:19 +0200 Subject: [PATCH] Use tx effective gas price when comparing with min mineable gas price configuration (#6958) Signed-off-by: Fabio Di Fabio Co-authored-by: Sally MacFarlane --- .../BaseFeePrioritizedTransactions.java | 4 +- ...ingTransactionEstimatedMemorySizeTest.java | 12 ++--- .../BaseFeePrioritizedTransactionsTest.java | 48 ++++++++++--------- .../layered/BaseTransactionPoolTest.java | 18 +++++-- .../eth/transactions/layered/LayersTest.java | 6 ++- 5 files changed, 52 insertions(+), 36 deletions(-) diff --git a/ethereum/eth/src/main/java/org/hyperledger/besu/ethereum/eth/transactions/layered/BaseFeePrioritizedTransactions.java b/ethereum/eth/src/main/java/org/hyperledger/besu/ethereum/eth/transactions/layered/BaseFeePrioritizedTransactions.java index e1d10e9ee2..8c0cd37f0c 100644 --- a/ethereum/eth/src/main/java/org/hyperledger/besu/ethereum/eth/transactions/layered/BaseFeePrioritizedTransactions.java +++ b/ethereum/eth/src/main/java/org/hyperledger/besu/ethereum/eth/transactions/layered/BaseFeePrioritizedTransactions.java @@ -158,10 +158,10 @@ public class BaseFeePrioritizedTransactions extends AbstractPrioritizedTransacti // priority txs are promoted even if they pay less if (!pendingTransaction.hasPriority()) { - // check if max fee per gas is higher than the min gas price + // check if effective gas price is higher than the min gas price if (pendingTransaction .getTransaction() - .getMaxGasPrice() + .getEffectiveGasPrice(nextBlockBaseFee) .lessThan(miningParameters.getMinTransactionGasPrice())) { return false; } diff --git a/ethereum/eth/src/test/java/org/hyperledger/besu/ethereum/eth/transactions/PendingTransactionEstimatedMemorySizeTest.java b/ethereum/eth/src/test/java/org/hyperledger/besu/ethereum/eth/transactions/PendingTransactionEstimatedMemorySizeTest.java index 1710a6cb23..23d2c6f7c3 100644 --- a/ethereum/eth/src/test/java/org/hyperledger/besu/ethereum/eth/transactions/PendingTransactionEstimatedMemorySizeTest.java +++ b/ethereum/eth/src/test/java/org/hyperledger/besu/ethereum/eth/transactions/PendingTransactionEstimatedMemorySizeTest.java @@ -69,7 +69,7 @@ public class PendingTransactionEstimatedMemorySizeTest extends BaseTransactionPo @Test public void toSize() { TransactionTestFixture preparedTx = - prepareTransaction(TransactionType.ACCESS_LIST, 10, Wei.of(500), 10, 0); + prepareTransaction(TransactionType.ACCESS_LIST, 10, Wei.of(500), Wei.ZERO, 10, 0); Transaction txTo = preparedTx.to(Optional.of(Address.extract(Bytes32.random()))).createTransaction(KEYS1); BytesValueRLPOutput rlpOut = new BytesValueRLPOutput(); @@ -115,7 +115,7 @@ public class PendingTransactionEstimatedMemorySizeTest extends BaseTransactionPo public void payloadSize() { TransactionTestFixture preparedTx = - prepareTransaction(TransactionType.ACCESS_LIST, 10, Wei.of(500), 10, 0); + prepareTransaction(TransactionType.ACCESS_LIST, 10, Wei.of(500), Wei.ZERO, 10, 0); Transaction txPayload = preparedTx.createTransaction(KEYS1); BytesValueRLPOutput rlpOut = new BytesValueRLPOutput(); txPayload.writeTo(rlpOut); @@ -207,7 +207,7 @@ public class PendingTransactionEstimatedMemorySizeTest extends BaseTransactionPo final long containerSize, final long itemSize) { TransactionTestFixture preparedTx = - prepareTransaction(TransactionType.BLOB, 10, Wei.of(500), 10, 1); + prepareTransaction(TransactionType.BLOB, 10, Wei.of(500), Wei.of(50), 10, 1); Transaction txBlob = preparedTx.createTransaction(KEYS1); BytesValueRLPOutput rlpOut = new BytesValueRLPOutput(); TransactionEncoder.encodeRLP(txBlob, rlpOut, EncodingContext.POOLED_TRANSACTION); @@ -239,7 +239,7 @@ public class PendingTransactionEstimatedMemorySizeTest extends BaseTransactionPo @Test public void blobsWithCommitmentsSize() { TransactionTestFixture preparedTx = - prepareTransaction(TransactionType.BLOB, 10, Wei.of(500), 10, 1); + prepareTransaction(TransactionType.BLOB, 10, Wei.of(500), Wei.of(50), 10, 1); Transaction txBlob = preparedTx.createTransaction(KEYS1); BytesValueRLPOutput rlpOut = new BytesValueRLPOutput(); TransactionEncoder.encodeRLP(txBlob, rlpOut, EncodingContext.POOLED_TRANSACTION); @@ -268,7 +268,7 @@ public class PendingTransactionEstimatedMemorySizeTest extends BaseTransactionPo public void pendingTransactionSize() { TransactionTestFixture preparedTx = - prepareTransaction(TransactionType.ACCESS_LIST, 10, Wei.of(500), 10, 0); + prepareTransaction(TransactionType.ACCESS_LIST, 10, Wei.of(500), Wei.ZERO, 10, 0); Transaction txPayload = preparedTx.createTransaction(KEYS1); BytesValueRLPOutput rlpOut = new BytesValueRLPOutput(); txPayload.writeTo(rlpOut); @@ -300,7 +300,7 @@ public class PendingTransactionEstimatedMemorySizeTest extends BaseTransactionPo final List ales = List.of(ale1); TransactionTestFixture preparedTx = - prepareTransaction(TransactionType.ACCESS_LIST, 0, Wei.of(500), 0, 0); + prepareTransaction(TransactionType.ACCESS_LIST, 0, Wei.of(500), Wei.ZERO, 0, 0); Transaction txAccessList = preparedTx.accessList(ales).createTransaction(KEYS1); BytesValueRLPOutput rlpOut = new BytesValueRLPOutput(); txAccessList.writeTo(rlpOut); diff --git a/ethereum/eth/src/test/java/org/hyperledger/besu/ethereum/eth/transactions/layered/BaseFeePrioritizedTransactionsTest.java b/ethereum/eth/src/test/java/org/hyperledger/besu/ethereum/eth/transactions/layered/BaseFeePrioritizedTransactionsTest.java index 31785fa189..67c066de9c 100644 --- a/ethereum/eth/src/test/java/org/hyperledger/besu/ethereum/eth/transactions/layered/BaseFeePrioritizedTransactionsTest.java +++ b/ethereum/eth/src/test/java/org/hyperledger/besu/ethereum/eth/transactions/layered/BaseFeePrioritizedTransactionsTest.java @@ -37,7 +37,6 @@ import org.hyperledger.besu.ethereum.mainnet.feemarket.FeeMarket; import java.util.Comparator; import java.util.List; -import java.util.Map; import java.util.Optional; import java.util.Random; import java.util.function.BiFunction; @@ -45,9 +44,12 @@ import java.util.stream.Collectors; import java.util.stream.IntStream; import org.junit.jupiter.api.Test; +import org.junit.jupiter.params.ParameterizedTest; +import org.junit.jupiter.params.provider.EnumSource; public class BaseFeePrioritizedTransactionsTest extends AbstractPrioritizedTransactionsTestBase { private static final FeeMarket EIP1559_FEE_MARKET = FeeMarket.london(0L); + private static final Wei DEFAULT_BASE_FEE = DEFAULT_MIN_GAS_PRICE.subtract(2); private static final Random randomizeTxType = new Random(); @Override @@ -72,7 +74,7 @@ public class BaseFeePrioritizedTransactionsTest extends AbstractPrioritizedTrans @Override protected BlockHeader mockBlockHeader() { - return mockBlockHeader(Wei.ONE); + return mockBlockHeader(DEFAULT_BASE_FEE); } private BlockHeader mockBlockHeader(final Wei baseFee) { @@ -112,19 +114,9 @@ public class BaseFeePrioritizedTransactionsTest extends AbstractPrioritizedTrans keys); } - @Test - public void shouldPrioritizePriorityFeeThenTimeAddedToPoolOnlyEIP1559Txs() { - shouldPrioritizePriorityFeeThenTimeAddedToPoolSameTypeTxs(EIP1559); - } - - @Test - public void shouldPrioritizeGasPriceThenTimeAddedToPoolOnlyFrontierTxs() { - shouldPrioritizePriorityFeeThenTimeAddedToPoolSameTypeTxs(FRONTIER); - } - @Test public void shouldPrioritizeEffectivePriorityFeeThenTimeAddedToPoolOnMixedTypes() { - final var nextBlockBaseFee = Optional.of(Wei.ONE); + final var nextBlockBaseFee = Optional.of(DEFAULT_MIN_GAS_PRICE.subtract(1)); final PendingTransaction highGasPriceTransaction = createRemotePendingTransaction( @@ -167,7 +159,6 @@ public class BaseFeePrioritizedTransactionsTest extends AbstractPrioritizedTrans @Test public void txBelowCurrentMineableMinPriorityFeeIsNotPrioritized() { - setBaseFee(DEFAULT_MIN_GAS_PRICE.subtract(2)); miningParameters.setMinPriorityFeePerGas(Wei.of(5)); final PendingTransaction lowPriorityFeeTx = createRemotePendingTransaction( @@ -179,7 +170,6 @@ public class BaseFeePrioritizedTransactionsTest extends AbstractPrioritizedTrans @Test public void txWithPriorityBelowCurrentMineableMinPriorityFeeIsPrioritized() { - setBaseFee(DEFAULT_MIN_GAS_PRICE.subtract(2)); miningParameters.setMinPriorityFeePerGas(Wei.of(5)); final PendingTransaction lowGasPriceTx = createRemotePendingTransaction( @@ -188,11 +178,29 @@ public class BaseFeePrioritizedTransactionsTest extends AbstractPrioritizedTrans assertTransactionPrioritized(lowGasPriceTx); } - private void shouldPrioritizePriorityFeeThenTimeAddedToPoolSameTypeTxs( + @ParameterizedTest + @EnumSource( + value = TransactionType.class, + names = {"EIP1559", "BLOB"}) + public void txWithEffectiveGasPriceBelowCurrentMineableMinGasPriceIsNotPrioritized( + final TransactionType type) { + final PendingTransaction lowGasPriceTx = + createRemotePendingTransaction( + createTransaction(type, 0, DEFAULT_MIN_GAS_PRICE, Wei.ONE, 0, 1, KEYS1)); + assertThat(prioritizeTransaction(lowGasPriceTx)).isEqualTo(DROPPED); + assertEvicted(lowGasPriceTx); + assertTransactionNotPrioritized(lowGasPriceTx); + } + + @ParameterizedTest + @EnumSource( + value = TransactionType.class, + names = {"EIP1559", "FRONTIER"}) + public void shouldPrioritizePriorityFeeThenTimeAddedToPoolSameTypeTxs( final TransactionType transactionType) { final PendingTransaction highGasPriceTransaction = createRemotePendingTransaction( - createTransaction(0, DEFAULT_MIN_GAS_PRICE.multiply(20), KEYS1)); + createTransaction(0, DEFAULT_MIN_GAS_PRICE.multiply(200), KEYS1)); final var lowValueTxs = IntStream.range(0, MAX_TRANSACTIONS) @@ -202,7 +210,7 @@ public class BaseFeePrioritizedTransactionsTest extends AbstractPrioritizedTrans createTransaction( transactionType, 0, - DEFAULT_MIN_GAS_PRICE.add(1), + DEFAULT_MIN_GAS_PRICE.add(1).multiply(20), 0, SIGNATURE_ALGORITHM.get().generateKeyPair()))) .collect(Collectors.toUnmodifiableList()); @@ -210,8 +218,4 @@ public class BaseFeePrioritizedTransactionsTest extends AbstractPrioritizedTrans shouldPrioritizeValueThenTimeAddedToPool( lowValueTxs.iterator(), highGasPriceTransaction, lowValueTxs.get(0)); } - - private void setBaseFee(final Wei baseFee) { - transactions.blockAdded(EIP1559_FEE_MARKET, mockBlockHeader(baseFee), Map.of()); - } } diff --git a/ethereum/eth/src/test/java/org/hyperledger/besu/ethereum/eth/transactions/layered/BaseTransactionPoolTest.java b/ethereum/eth/src/test/java/org/hyperledger/besu/ethereum/eth/transactions/layered/BaseTransactionPoolTest.java index 160c60c66d..7d6e44059d 100644 --- a/ethereum/eth/src/test/java/org/hyperledger/besu/ethereum/eth/transactions/layered/BaseTransactionPoolTest.java +++ b/ethereum/eth/src/test/java/org/hyperledger/besu/ethereum/eth/transactions/layered/BaseTransactionPoolTest.java @@ -94,7 +94,13 @@ public class BaseTransactionPoolTest { protected Transaction createEIP4844Transaction( final long nonce, final KeyPair keys, final int gasFeeMultiplier, final int blobCount) { return createTransaction( - TransactionType.BLOB, nonce, Wei.of(5000L).multiply(gasFeeMultiplier), 0, blobCount, keys); + TransactionType.BLOB, + nonce, + Wei.of(5000L).multiply(gasFeeMultiplier), + Wei.of(5000L).multiply(gasFeeMultiplier).divide(10), + 0, + blobCount, + keys); } protected Transaction createTransaction( @@ -112,17 +118,20 @@ public class BaseTransactionPoolTest { final Wei maxGasPrice, final int payloadSize, final KeyPair keys) { - return createTransaction(type, nonce, maxGasPrice, payloadSize, 0, keys); + return createTransaction( + type, nonce, maxGasPrice, maxGasPrice.divide(10), payloadSize, 0, keys); } protected Transaction createTransaction( final TransactionType type, final long nonce, final Wei maxGasPrice, + final Wei maxPriorityFeePerGas, final int payloadSize, final int blobCount, final KeyPair keys) { - return prepareTransaction(type, nonce, maxGasPrice, payloadSize, blobCount) + return prepareTransaction( + type, nonce, maxGasPrice, maxPriorityFeePerGas, payloadSize, blobCount) .createTransaction(keys); } @@ -130,6 +139,7 @@ public class BaseTransactionPoolTest { final TransactionType type, final long nonce, final Wei maxGasPrice, + final Wei maxPriorityFeePerGas, final int payloadSize, final int blobCount) { @@ -145,7 +155,7 @@ public class BaseTransactionPoolTest { } if (type.supports1559FeeMarket()) { tx.maxFeePerGas(Optional.of(maxGasPrice)) - .maxPriorityFeePerGas(Optional.of(maxGasPrice.divide(10))); + .maxPriorityFeePerGas(Optional.of(maxPriorityFeePerGas)); if (type.supportsBlob() && blobCount > 0) { final var versionHashes = IntStream.range(0, blobCount) diff --git a/ethereum/eth/src/test/java/org/hyperledger/besu/ethereum/eth/transactions/layered/LayersTest.java b/ethereum/eth/src/test/java/org/hyperledger/besu/ethereum/eth/transactions/layered/LayersTest.java index c89540fcf7..5ba6bde8dc 100644 --- a/ethereum/eth/src/test/java/org/hyperledger/besu/ethereum/eth/transactions/layered/LayersTest.java +++ b/ethereum/eth/src/test/java/org/hyperledger/besu/ethereum/eth/transactions/layered/LayersTest.java @@ -59,6 +59,8 @@ import org.junit.jupiter.params.provider.MethodSource; public class LayersTest extends BaseTransactionPoolTest { private static final int MAX_PRIO_TRANSACTIONS = 3; private static final int MAX_FUTURE_FOR_SENDER = 10; + private static final Wei BASE_FEE = Wei.ONE; + private static final Wei MIN_GAS_PRICE = BASE_FEE; private final TransactionPoolConfiguration poolConfig = ImmutableTransactionPoolConfiguration.builder() @@ -96,7 +98,7 @@ public class LayersTest extends BaseTransactionPoolTest { this::transactionReplacementTester, FeeMarket.london(0L), new BlobCache(), - MiningParameters.newDefault()); + MiningParameters.newDefault().setMinTransactionGasPrice(MIN_GAS_PRICE)); private final LayeredPendingTransactions pendingTransactions = new LayeredPendingTransactions(poolConfig, prioritizedTransactions); @@ -1180,7 +1182,7 @@ public class LayersTest extends BaseTransactionPoolTest { private static BlockHeader mockBlockHeader() { final BlockHeader blockHeader = mock(BlockHeader.class); - when(blockHeader.getBaseFee()).thenReturn(Optional.of(Wei.ONE)); + when(blockHeader.getBaseFee()).thenReturn(Optional.of(BASE_FEE)); return blockHeader; }