Transaction pool price validation improvements and fixes (#4598)

Signed-off-by: Fabio Di Fabio <fabio.difabio@consensys.net>

CHANGELOG.md

@@ -22,6 +22,7 @@
 - Add field `type` to Transaction receipt object (eth_getTransactionReceipt) [#4505](https://github.com/hyperledger/besu/issues/4505)
 - Print an overview of configuration and system information at startup [#4451](https://github.com/hyperledger/besu/pull/4451)
 - Do not send new payloads to backward sync if initial sync is in progress [#4720](https://github.com/hyperledger/besu/issues/4720)
+- Improve the way transaction fee cap validation is done on London fee market to not depend on transient network conditions [#4598](https://github.com/hyperledger/besu/pull/4598) 
 
 ### Bug Fixes
 - Restore updating chain head and finalized block during backward sync [#4718](https://github.com/hyperledger/besu/pull/4718)

ethereum/core/src/main/java/org/hyperledger/besu/ethereum/mainnet/feemarket/FeeMarket.java

@@ -19,7 +19,6 @@ import org.hyperledger.besu.ethereum.core.Transaction;
 import org.hyperledger.besu.ethereum.core.feemarket.TransactionPriceCalculator;
 
 import java.util.Optional;
-import java.util.function.Supplier;
 
 public interface FeeMarket {
 
@@ -29,10 +28,7 @@ public interface FeeMarket {
 
   TransactionPriceCalculator getTransactionPriceCalculator();
 
-  Wei minTransactionPriceInNextBlock(
-      Transaction transaction, Supplier<Optional<Wei>> baseFeeSupplier);
-
-  boolean satisfiesFloorTxCost(Transaction txn);
+  boolean satisfiesFloorTxFee(Transaction txn);
 
   static BaseFeeMarket london(final long londonForkBlockNumber) {
     return london(londonForkBlockNumber, Optional.empty());

ethereum/core/src/main/java/org/hyperledger/besu/ethereum/mainnet/feemarket/LegacyFeeMarket.java

@@ -14,13 +14,9 @@
  */
 package org.hyperledger.besu.ethereum.mainnet.feemarket;
 
-import org.hyperledger.besu.datatypes.Wei;
 import org.hyperledger.besu.ethereum.core.Transaction;
 import org.hyperledger.besu.ethereum.core.feemarket.TransactionPriceCalculator;
 
-import java.util.Optional;
-import java.util.function.Supplier;
-
 public class LegacyFeeMarket implements FeeMarket {
 
   private final TransactionPriceCalculator txPriceCalculator;
@@ -35,13 +31,7 @@ public class LegacyFeeMarket implements FeeMarket {
   }
 
   @Override
-  public Wei minTransactionPriceInNextBlock(
-      final Transaction transaction, final Supplier<Optional<Wei>> baseFeeSupplier) {
-    return txPriceCalculator.price(transaction, Optional.empty());
-  }
-
-  @Override
-  public boolean satisfiesFloorTxCost(final Transaction txn) {
+  public boolean satisfiesFloorTxFee(final Transaction txn) {
     return true;
   }
 }

ethereum/core/src/main/java/org/hyperledger/besu/ethereum/mainnet/feemarket/LondonFeeMarket.java

@@ -17,11 +17,9 @@ package org.hyperledger.besu.ethereum.mainnet.feemarket;
 import org.hyperledger.besu.config.GenesisConfigFile;
 import org.hyperledger.besu.datatypes.Wei;
 import org.hyperledger.besu.ethereum.core.Transaction;
-import org.hyperledger.besu.ethereum.core.feemarket.BaseFee;
 import org.hyperledger.besu.ethereum.core.feemarket.TransactionPriceCalculator;
 
 import java.util.Optional;
-import java.util.function.Supplier;
 
 import org.apache.tuweni.units.bigints.UInt256s;
 import org.slf4j.Logger;
@@ -73,17 +71,7 @@ public class LondonFeeMarket implements BaseFeeMarket {
   }
 
   @Override
-  public Wei minTransactionPriceInNextBlock(
-      final Transaction transaction, final Supplier<Optional<Wei>> baseFeeSupplier) {
-    final Optional<Wei> baseFee = baseFeeSupplier.get();
-    Optional<Wei> minBaseFeeInNextBlock =
-        baseFee.map(bf -> new BaseFee(this, bf).getMinNextValue());
-
-    return this.getTransactionPriceCalculator().price(transaction, minBaseFeeInNextBlock);
-  }
-
-  @Override
-  public boolean satisfiesFloorTxCost(final Transaction txn) {
+  public boolean satisfiesFloorTxFee(final Transaction txn) {
     // ensure effective baseFee is at least above floor
     return txn.getGasPrice()
         .map(Optional::of)

ethereum/core/src/test/java/org/hyperledger/besu/ethereum/mainnet/feemarket/LondonFeeMarketTest.java

@@ -41,7 +41,7 @@ public class LondonFeeMarketTest {
             .createTransaction(KEY_PAIR1);
 
     final LondonFeeMarket londonFeeMarket = new LondonFeeMarket(0);
-    assertThat(londonFeeMarket.satisfiesFloorTxCost(transaction)).isTrue();
+    assertThat(londonFeeMarket.satisfiesFloorTxFee(transaction)).isTrue();
   }
 
   @Test
@@ -55,7 +55,7 @@ public class LondonFeeMarketTest {
             .createTransaction(KEY_PAIR1);
 
     final LondonFeeMarket londonFeeMarket = new LondonFeeMarket(0);
-    assertThat(londonFeeMarket.satisfiesFloorTxCost(transaction)).isFalse();
+    assertThat(londonFeeMarket.satisfiesFloorTxFee(transaction)).isFalse();
   }
 
   @Test
@@ -69,6 +69,6 @@ public class LondonFeeMarketTest {
             .createTransaction(KEY_PAIR1);
 
     final LondonFeeMarket londonFeeMarket = new LondonFeeMarket(0, Optional.of(Wei.ZERO));
-    assertThat(londonFeeMarket.satisfiesFloorTxCost(transaction)).isTrue();
+    assertThat(londonFeeMarket.satisfiesFloorTxFee(transaction)).isTrue();
   }
 }

ethereum/core/src/test/java/org/hyperledger/besu/ethereum/mainnet/feemarket/ZeroBaseFeeMarketTest.java

@@ -57,21 +57,6 @@ public class ZeroBaseFeeMarketTest {
         .isEqualTo(LondonFeeMarket.DEFAULT_SLACK_COEFFICIENT);
   }
 
-  @Test
-  public void minTransactionPriceInNextBlockShouldHandleZeroBaseFee() {
-    final Transaction transaction =
-        new TransactionTestFixture()
-            .type(TransactionType.EIP1559)
-            .maxFeePerGas(Optional.of(Wei.of(8)))
-            .maxPriorityFeePerGas(Optional.of(Wei.of(8)))
-            .gasPrice(null)
-            .createTransaction(KEY_PAIR1);
-    assertThat(
-            zeroBaseFeeMarket.minTransactionPriceInNextBlock(
-                transaction, () -> Optional.of(Wei.ZERO)))
-        .isEqualTo(Wei.of(8));
-  }
-
   @Test
   public void getTransactionPriceCalculatorShouldBeEIP1559() {
     // only eip1559 will read the fee per gas values
@@ -96,7 +81,7 @@ public class ZeroBaseFeeMarketTest {
             .type(TransactionType.FRONTIER)
             .gasPrice(Wei.of(7))
             .createTransaction(KEY_PAIR1);
-    assertThat(zeroBaseFeeMarket.satisfiesFloorTxCost(transaction)).isTrue();
+    assertThat(zeroBaseFeeMarket.satisfiesFloorTxFee(transaction)).isTrue();
   }
 
   @Test
@@ -108,7 +93,7 @@ public class ZeroBaseFeeMarketTest {
             .maxPriorityFeePerGas(Optional.of(Wei.ZERO))
             .gasPrice(null)
             .createTransaction(KEY_PAIR1);
-    assertThat(zeroBaseFeeMarket.satisfiesFloorTxCost(transaction)).isTrue();
+    assertThat(zeroBaseFeeMarket.satisfiesFloorTxFee(transaction)).isTrue();
   }
 
   @Test

ethereum/eth/src/main/java/org/hyperledger/besu/ethereum/eth/transactions/TransactionPool.java

@@ -113,13 +113,12 @@ public class TransactionPool implements BlockAddedObserver {
   public ValidationResult<TransactionInvalidReason> addLocalTransaction(
       final Transaction transaction) {
     final ValidationResultAndAccount validationResult = validateLocalTransaction(transaction);
+
     if (validationResult.result.isValid()) {
-      if (!configuration.getTxFeeCap().isZero()
-          && minTransactionGasPrice(transaction).compareTo(configuration.getTxFeeCap()) > 0) {
-        return ValidationResult.invalid(TransactionInvalidReason.TX_FEECAP_EXCEEDED);
-      }
+
       final TransactionAddedStatus transactionAddedStatus =
           pendingTransactions.addLocalTransaction(transaction, validationResult.maybeAccount);
+
       if (!transactionAddedStatus.equals(ADDED)) {
         if (transactionAddedStatus.equals(ALREADY_KNOWN)) {
           duplicateTransactionCounter.labels(LOCAL).inc();
@@ -133,6 +132,7 @@ public class TransactionPool implements BlockAddedObserver {
                       return INTERNAL_ERROR;
                     }));
       }
+
       final Collection<Transaction> txs = singletonList(transaction);
       transactionBroadcaster.onTransactionsAdded(txs);
     }
@@ -140,38 +140,31 @@ public class TransactionPool implements BlockAddedObserver {
     return validationResult.result;
   }
 
-  private boolean effectiveGasPriceIsAboveConfiguredMinGasPrice(final Transaction transaction) {
-    return transaction
-        .getGasPrice()
-        .map(Optional::of)
-        .orElse(transaction.getMaxFeePerGas())
-        .map(g -> g.greaterOrEqualThan(miningParameters.getMinTransactionGasPrice()))
-        .orElse(false);
+  private Optional<Wei> getMaxGasPrice(final Transaction transaction) {
+    return transaction.getGasPrice().map(Optional::of).orElse(transaction.getMaxFeePerGas());
+  }
+
+  private boolean isMaxGasPriceBelowConfiguredMinGasPrice(final Transaction transaction) {
+    return getMaxGasPrice(transaction)
+        .map(g -> g.lessThan(miningParameters.getMinTransactionGasPrice()))
+        .orElse(true);
   }
 
   public void addRemoteTransactions(final Collection<Transaction> transactions) {
     final List<Transaction> addedTransactions = new ArrayList<>(transactions.size());
     LOG.trace("Adding {} remote transactions", transactions.size());
+
     for (final Transaction transaction : transactions) {
+
       if (pendingTransactions.containsTransaction(transaction.getHash())) {
         traceLambda(LOG, "Discard already present transaction {}", transaction::toTraceLog);
         // We already have this transaction, don't even validate it.
         duplicateTransactionCounter.labels(REMOTE).inc();
         continue;
       }
-      final Wei transactionGasPrice = minTransactionGasPrice(transaction);
-      if (transactionGasPrice.compareTo(miningParameters.getMinTransactionGasPrice()) < 0) {
-        traceLambda(
-            LOG,
-            "Discard transaction {} below min gas price {}",
-            transaction::toTraceLog,
-            miningParameters::getMinTransactionGasPrice);
-        pendingTransactions
-            .signalInvalidAndGetDependentTransactions(transaction)
-            .forEach(pendingTransactions::removeTransaction);
-        continue;
-      }
+
       final ValidationResultAndAccount validationResult = validateRemoteTransaction(transaction);
+
       if (validationResult.result.isValid()) {
         final TransactionAddedStatus status =
             pendingTransactions.addRemoteTransaction(transaction, validationResult.maybeAccount);
@@ -198,6 +191,7 @@ public class TransactionPool implements BlockAddedObserver {
             .forEach(pendingTransactions::removeTransaction);
       }
     }
+
     if (!addedTransactions.isEmpty()) {
       transactionBroadcaster.onTransactionsAdded(addedTransactions);
       traceLambda(
@@ -286,22 +280,10 @@ public class TransactionPool implements BlockAddedObserver {
     final FeeMarket feeMarket =
         protocolSchedule.getByBlockNumber(chainHeadBlockHeader.getNumber()).getFeeMarket();
 
-    // Check whether it's a GoQuorum transaction
-    boolean goQuorumCompatibilityMode = getTransactionValidator().getGoQuorumCompatibilityMode();
-    if (transaction.isGoQuorumPrivateTransaction(goQuorumCompatibilityMode)) {
-      final Optional<Wei> weiValue = ofNullable(transaction.getValue());
-      if (weiValue.isPresent() && !weiValue.get().isZero()) {
-        return ValidationResultAndAccount.invalid(
-            TransactionInvalidReason.ETHER_VALUE_NOT_SUPPORTED);
-      }
-    }
-
-    // allow local transactions to be below minGas as long as we are mining and the transaction is
-    // executable:
-    if ((!effectiveGasPriceIsAboveConfiguredMinGasPrice(transaction)
-            && !miningParameters.isMiningEnabled())
-        || (!feeMarket.satisfiesFloorTxCost(transaction))) {
-      return ValidationResultAndAccount.invalid(TransactionInvalidReason.GAS_PRICE_TOO_LOW);
+    final TransactionInvalidReason priceInvalidReason =
+        validatePrice(transaction, isLocal, feeMarket);
+    if (priceInvalidReason != null) {
+      return ValidationResultAndAccount.invalid(priceInvalidReason);
     }
 
     final ValidationResult<TransactionInvalidReason> basicValidationResult =
@@ -355,6 +337,43 @@ public class TransactionPool implements BlockAddedObserver {
     }
   }
 
+  private TransactionInvalidReason validatePrice(
+      final Transaction transaction, final boolean isLocal, final FeeMarket feeMarket) {
+
+    // Check whether it's a GoQuorum transaction
+    boolean goQuorumCompatibilityMode = getTransactionValidator().getGoQuorumCompatibilityMode();
+    if (transaction.isGoQuorumPrivateTransaction(goQuorumCompatibilityMode)) {
+      final Optional<Wei> weiValue = ofNullable(transaction.getValue());
+      if (weiValue.isPresent() && !weiValue.get().isZero()) {
+        return TransactionInvalidReason.ETHER_VALUE_NOT_SUPPORTED;
+      }
+    }
+
+    if (isLocal) {
+      if (!configuration.getTxFeeCap().isZero()
+          && getMaxGasPrice(transaction).get().greaterThan(configuration.getTxFeeCap())) {
+        return TransactionInvalidReason.TX_FEECAP_EXCEEDED;
+      }
+      // allow local transactions to be below minGas as long as we are mining
+      // or at least gas price is above the configured floor
+      if ((!miningParameters.isMiningEnabled()
+              && isMaxGasPriceBelowConfiguredMinGasPrice(transaction))
+          || !feeMarket.satisfiesFloorTxFee(transaction)) {
+        return TransactionInvalidReason.GAS_PRICE_TOO_LOW;
+      }
+    } else {
+      if (isMaxGasPriceBelowConfiguredMinGasPrice(transaction)) {
+        traceLambda(
+            LOG,
+            "Discard transaction {} below min gas price {}",
+            transaction::toTraceLog,
+            miningParameters::getMinTransactionGasPrice);
+        return TransactionInvalidReason.GAS_PRICE_TOO_LOW;
+      }
+    }
+    return null;
+  }
+
   private boolean strictReplayProtectionShouldBeEnforceLocally(
       final BlockHeader chainHeadBlockHeader) {
     return configuration.getStrictTransactionReplayProtectionEnabled()
@@ -375,17 +394,6 @@ public class TransactionPool implements BlockAddedObserver {
     return blockchain.getBlockHeader(blockchain.getChainHeadHash());
   }
 
-  private Wei minTransactionGasPrice(final Transaction transaction) {
-    return getChainHeadBlockHeader()
-        .map(
-            chainHeadBlockHeader ->
-                protocolSchedule
-                    .getByBlockNumber(chainHeadBlockHeader.getNumber())
-                    .getFeeMarket()
-                    .minTransactionPriceInNextBlock(transaction, chainHeadBlockHeader::getBaseFee))
-        .orElse(Wei.ZERO);
-  }
-
   public interface TransactionBatchAddedListener {
 
     void onTransactionsAdded(Iterable<Transaction> transactions);

ethereum/eth/src/test/java/org/hyperledger/besu/ethereum/eth/transactions/AbstractTransactionPoolTest.java

@@ -108,7 +108,7 @@ public abstract class AbstractTransactionPoolTest {
   protected MutableBlockchain blockchain;
   private TransactionBroadcaster transactionBroadcaster;
 
-  private AbstractPendingTransactionsSorter transactions;
+  protected AbstractPendingTransactionsSorter transactions;
   private final Transaction transaction1 = createTransaction(1);
   private final Transaction transaction2 = createTransaction(2);
 
@@ -348,7 +348,8 @@ public abstract class AbstractTransactionPoolTest {
     transactionPool.addRemoteTransactions(singletonList(transaction));
 
     assertTransactionNotPending(transaction);
-    verifyNoInteractions(transactionValidator); // Reject before validation
+    verify(transactionValidator).getGoQuorumCompatibilityMode();
+    verifyNoMoreInteractions(transactionValidator);
   }
 
   @Test

ethereum/eth/src/test/java/org/hyperledger/besu/ethereum/eth/transactions/TransactionPoolLondonTest.java

@@ -17,6 +17,7 @@ package org.hyperledger.besu.ethereum.eth.transactions;
 import static java.util.Arrays.asList;
 import static java.util.Collections.emptyList;
 import static java.util.stream.Collectors.toList;
+import static org.assertj.core.api.Assertions.assertThat;
 import static org.mockito.Mockito.when;
 
 import org.hyperledger.besu.config.StubGenesisConfigOptions;
@@ -164,7 +165,7 @@ public class TransactionPoolLondonTest extends AbstractTransactionPoolTest {
   public void shouldAcceptZeroGasPriceFrontierTxsWhenMinGasPriceIsZeroAndLondonWithZeroBaseFee() {
     when(miningParameters.getMinTransactionGasPrice()).thenReturn(Wei.ZERO);
     when(protocolSpec.getFeeMarket()).thenReturn(FeeMarket.london(0, Optional.of(Wei.ZERO)));
-    whenBlockBaseFeeIsZero();
+    whenBlockBaseFeeIs(Wei.ZERO);
 
     final Transaction frontierTransaction = createFrontierTransaction(0, Wei.ZERO);
 
@@ -176,7 +177,7 @@ public class TransactionPoolLondonTest extends AbstractTransactionPoolTest {
   public void shouldAcceptZeroGasPrice1559TxsWhenMinGasPriceIsZeroAndLondonWithZeroBaseFee() {
     when(miningParameters.getMinTransactionGasPrice()).thenReturn(Wei.ZERO);
     when(protocolSpec.getFeeMarket()).thenReturn(FeeMarket.london(0, Optional.of(Wei.ZERO)));
-    whenBlockBaseFeeIsZero();
+    whenBlockBaseFeeIs(Wei.ZERO);
 
     final Transaction transaction = createTransaction(0, Wei.ZERO);
 
@@ -193,6 +194,81 @@ public class TransactionPoolLondonTest extends AbstractTransactionPoolTest {
     assertLocalTransactionValid(frontierTransaction);
   }
 
+  @Test
+  public void shouldRejectRemote1559TxsWhenMaxFeePerGasBelowMinGasPrice() {
+    final Wei genesisBaseFee = Wei.of(100L);
+    final Wei minGasPrice = Wei.of(200L);
+    final Wei lastBlockBaseFee = minGasPrice.add(50L);
+    final Wei txMaxFeePerGas = minGasPrice.subtract(1L);
+
+    assertThat(
+            add1559TxAndGetPendingTxsCount(
+                genesisBaseFee, minGasPrice, lastBlockBaseFee, txMaxFeePerGas, false))
+        .isEqualTo(0);
+  }
+
+  @Test
+  public void shouldAcceptRemote1559TxsWhenMaxFeePerGasIsAtLeastEqualToMinGasPrice() {
+    final Wei genesisBaseFee = Wei.of(100L);
+    final Wei minGasPrice = Wei.of(200L);
+    final Wei lastBlockBaseFee = minGasPrice.add(50L);
+    final Wei txMaxFeePerGas = minGasPrice;
+
+    assertThat(
+            add1559TxAndGetPendingTxsCount(
+                genesisBaseFee, minGasPrice, lastBlockBaseFee, txMaxFeePerGas, false))
+        .isEqualTo(1);
+  }
+
+  @Test
+  public void shouldRejectLocal1559TxsWhenMaxFeePerGasBelowMinGasPrice() {
+    final Wei genesisBaseFee = Wei.of(100L);
+    final Wei minGasPrice = Wei.of(200L);
+    final Wei lastBlockBaseFee = minGasPrice.add(50L);
+    final Wei txMaxFeePerGas = minGasPrice.subtract(1L);
+
+    assertThat(
+            add1559TxAndGetPendingTxsCount(
+                genesisBaseFee, minGasPrice, lastBlockBaseFee, txMaxFeePerGas, true))
+        .isEqualTo(0);
+  }
+
+  @Test
+  public void shouldAcceptLocal1559TxsWhenMaxFeePerGasIsAtLeastEqualToMinMinGasPrice() {
+    final Wei genesisBaseFee = Wei.of(100L);
+    final Wei minGasPrice = Wei.of(200L);
+    final Wei lastBlockBaseFee = minGasPrice.add(50L);
+    final Wei txMaxFeePerGas = minGasPrice;
+
+    assertThat(
+            add1559TxAndGetPendingTxsCount(
+                genesisBaseFee, minGasPrice, lastBlockBaseFee, txMaxFeePerGas, true))
+        .isEqualTo(1);
+  }
+
+  private int add1559TxAndGetPendingTxsCount(
+      final Wei genesisBaseFee,
+      final Wei minGasPrice,
+      final Wei lastBlockBaseFee,
+      final Wei txMaxFeePerGas,
+      final boolean isLocal) {
+    when(miningParameters.getMinTransactionGasPrice()).thenReturn(minGasPrice);
+    when(protocolSpec.getFeeMarket()).thenReturn(FeeMarket.london(0, Optional.of(genesisBaseFee)));
+    whenBlockBaseFeeIs(lastBlockBaseFee);
+
+    final Transaction transaction = createTransaction(0, txMaxFeePerGas);
+
+    givenTransactionIsValid(transaction);
+
+    if (isLocal) {
+      transactionPool.addLocalTransaction(transaction);
+    } else {
+      transactionPool.addRemoteTransactions(List.of(transaction));
+    }
+
+    return transactions.size();
+  }
+
   @Test
   @Override
   @Ignore
@@ -200,10 +276,10 @@ public class TransactionPoolLondonTest extends AbstractTransactionPoolTest {
     // ignore since this is going to fail until the branch with the fix is released
   }
 
-  private void whenBlockBaseFeeIsZero() {
+  private void whenBlockBaseFeeIs(final Wei baseFee) {
     final BlockHeader header =
         BlockHeaderBuilder.fromHeader(blockchain.getChainHeadHeader())
-            .baseFee(Wei.ZERO)
+            .baseFee(baseFee)
             .blockHeaderFunctions(new MainnetBlockHeaderFunctions())
             .parentHash(blockchain.getChainHeadHash())
             .buildBlockHeader();