Correctly initialize the txpool as disabled on creation (hyperledger#…

…6890) Signed-off-by: Fabio Di Fabio <[email protected]> Co-authored-by: Sally MacFarlane <[email protected]>
kaleido-io · Jun 7, 2024 · 3885d39 · 3885d39
1 parent ba03499
commit 3885d39
Show file tree

Hide file tree

Showing 3 changed files with 60 additions and 48 deletions.
diff --git a/CHANGELOG.md b/CHANGELOG.md
@@ -45,6 +45,7 @@
 - Fix Shanghai/QBFT block import bug when syncing new nodes [#6765](https://github.com/hyperledger/besu/pull/6765)
 - Fix to avoid broadcasting full blob txs, instead of only the tx announcement, to a subset of nodes [#6835](https://github.com/hyperledger/besu/pull/6835)
 - Snap client fixes discovered during snap server testing [#6847](https://github.com/hyperledger/besu/pull/6847)
+- Correctly initialize the txpool as disabled on creation [#6890](https://github.com/hyperledger/besu/pull/6890)
 
 ### Download Links
 

diff --git a/...eum/eth/src/main/java/org/hyperledger/besu/ethereum/eth/transactions/TransactionPool.java b/...eum/eth/src/main/java/org/hyperledger/besu/ethereum/eth/transactions/TransactionPool.java
@@ -89,7 +89,7 @@ public class TransactionPool implements BlockAddedObserver {
   private static final Logger LOG = LoggerFactory.getLogger(TransactionPool.class);
   private static final Logger LOG_FOR_REPLAY = LoggerFactory.getLogger("LOG_FOR_REPLAY");
   private final Supplier<PendingTransactions> pendingTransactionsSupplier;
-  private volatile PendingTransactions pendingTransactions;
+  private volatile PendingTransactions pendingTransactions = new DisabledPendingTransactions();
   private final ProtocolSchedule protocolSchedule;
   private final ProtocolContext protocolContext;
   private final EthContext ethContext;

diff --git a/.../test/java/org/hyperledger/besu/ethereum/eth/transactions/TransactionPoolFactoryTest.java b/.../test/java/org/hyperledger/besu/ethereum/eth/transactions/TransactionPoolFactoryTest.java
@@ -16,6 +16,7 @@
 
 import static org.assertj.core.api.Assertions.assertThat;
 import static org.hyperledger.besu.ethereum.eth.transactions.TransactionPoolConfiguration.Implementation.LAYERED;
+import static org.hyperledger.besu.ethereum.eth.transactions.TransactionPoolConfiguration.Implementation.LEGACY;
 import static org.hyperledger.besu.ethereum.mainnet.MainnetProtocolSchedule.DEFAULT_CHAIN_ID;
 import static org.mockito.ArgumentMatchers.anyInt;
 import static org.mockito.ArgumentMatchers.anyLong;
@@ -27,6 +28,7 @@
 import static org.mockito.Mockito.when;
 
 import org.hyperledger.besu.config.StubGenesisConfigOptions;
+import org.hyperledger.besu.datatypes.Address;
 import org.hyperledger.besu.datatypes.Hash;
 import org.hyperledger.besu.ethereum.ProtocolContext;
 import org.hyperledger.besu.ethereum.chain.BadBlockManager;
@@ -262,33 +264,38 @@ public void incomingTransactionMessageHandlersRegisteredIfNoInitialSync() {
                 "transaction messages handler should be enabled"));
   }
 
+  @Test
+  public void txPoolStartsDisabledWhenInitialSyncPhaseIsRequired() {
+    // when using any of the initial syncs (SNAP, FAST, ...), txpool starts disabled
+    // and is enabled only after it is in sync
+    setupInitialSyncPhase(true);
+    assertThat(pool.isEnabled()).isFalse();
+  }
+
+  @Test
+  public void callingGetNextNonceForSenderOnDisabledTxPoolWorks() {
+    setupInitialSyncPhase(true);
+
+    assertThat(pool.getNextNonceForSender(Address.fromHexString("0x123abc"))).isEmpty();
+  }
+
+  @Test
+  public void txPoolStartsEnabledWhenFullSyncConfigured() {
+    // when using FULL sync, txpool starts enabled, because it could be
+    // that it is the first node on a new network and so, it is in sync with genesis
+    // and must accept txs. Otherwise, asap it find a sync target that is ahead, the txpool
+    // will be disabled, until in sync.
+    setupInitialSyncPhase(false);
+    assertThat(pool.isEnabled()).isTrue();
+  }
+
   private void setupInitialSyncPhase(final boolean hasInitialSyncPhase) {
     syncState = new SyncState(blockchain, ethPeers, hasInitialSyncPhase, Optional.empty());
     setupInitialSyncPhase(syncState);
   }
 
   private void setupInitialSyncPhase(final SyncState syncState) {
-    pool =
-        TransactionPoolFactory.createTransactionPool(
-            schedule,
-            context,
-            ethContext,
-            TestClock.fixed(),
-            new TransactionPoolMetrics(new NoOpMetricsSystem()),
-            syncState,
-            ImmutableTransactionPoolConfiguration.builder()
-                .txPoolMaxSize(1)
-                .pendingTxRetentionPeriod(1)
-                .unstable(
-                    ImmutableTransactionPoolConfiguration.Unstable.builder()
-                        .txMessageKeepAliveSeconds(1)
-                        .build())
-                .build(),
-            peerTransactionTracker,
-            transactionsMessageSender,
-            newPooledTransactionHashesMessageSender,
-            new BlobCache(),
-            MiningParameters.newDefault());
+    pool = createTransactionPool(LAYERED, syncState);
 
     ethProtocolManager =
         new EthProtocolManager(
@@ -312,8 +319,7 @@ private void setupInitialSyncPhase(final SyncState syncState) {
       createLegacyTransactionPool_shouldUseBaseFeePendingTransactionsSorter_whenLondonEnabled() {
     setupScheduleWith(new StubGenesisConfigOptions().londonBlock(0));
 
-    final TransactionPool pool =
-        createTransactionPool(TransactionPoolConfiguration.Implementation.LEGACY);
+    final TransactionPool pool = createAndEnableTransactionPool(LEGACY, syncState);
 
     assertThat(pool.pendingTransactionsImplementation())
         .isEqualTo(BaseFeePendingTransactionsSorter.class);
@@ -324,8 +330,7 @@ private void setupInitialSyncPhase(final SyncState syncState) {
       createLegacyTransactionPool_shouldUseGasPricePendingTransactionsSorter_whenLondonNotEnabled() {
     setupScheduleWith(new StubGenesisConfigOptions().berlinBlock(0));
 
-    final TransactionPool pool =
-        createTransactionPool(TransactionPoolConfiguration.Implementation.LEGACY);
+    final TransactionPool pool = createAndEnableTransactionPool(LEGACY, syncState);
 
     assertThat(pool.pendingTransactionsImplementation())
         .isEqualTo(GasPricePendingTransactionsSorter.class);
@@ -336,7 +341,7 @@ private void setupInitialSyncPhase(final SyncState syncState) {
       createLayeredTransactionPool_shouldUseBaseFeePendingTransactionsSorter_whenLondonEnabled() {
     setupScheduleWith(new StubGenesisConfigOptions().londonBlock(0));
 
-    final TransactionPool pool = createTransactionPool(LAYERED);
+    final TransactionPool pool = createAndEnableTransactionPool(LAYERED, syncState);
 
     assertThat(pool.pendingTransactionsImplementation())
         .isEqualTo(LayeredPendingTransactions.class);
@@ -349,7 +354,7 @@ private void setupInitialSyncPhase(final SyncState syncState) {
       createLayeredTransactionPool_shouldUseGasPricePendingTransactionsSorter_whenLondonNotEnabled() {
     setupScheduleWith(new StubGenesisConfigOptions().berlinBlock(0));
 
-    final TransactionPool pool = createTransactionPool(LAYERED);
+    final TransactionPool pool = createAndEnableTransactionPool(LAYERED, syncState);
 
     assertThat(pool.pendingTransactionsImplementation())
         .isEqualTo(LayeredPendingTransactions.class);
@@ -378,27 +383,33 @@ private void setupScheduleWith(final StubGenesisConfigOptions config) {
   }
 
   private TransactionPool createTransactionPool(
-      final TransactionPoolConfiguration.Implementation implementation) {
-    final TransactionPool txPool =
-        TransactionPoolFactory.createTransactionPool(
-            schedule,
-            protocolContext,
-            ethContext,
-            TestClock.fixed(),
-            new NoOpMetricsSystem(),
-            syncState,
-            ImmutableTransactionPoolConfiguration.builder()
-                .txPoolImplementation(implementation)
-                .txPoolMaxSize(1)
-                .pendingTxRetentionPeriod(1)
-                .unstable(
-                    ImmutableTransactionPoolConfiguration.Unstable.builder()
-                        .txMessageKeepAliveSeconds(1)
-                        .build())
-                .build(),
-            new BlobCache(),
-            MiningParameters.newDefault());
+      final TransactionPoolConfiguration.Implementation implementation, final SyncState syncState) {
+    return TransactionPoolFactory.createTransactionPool(
+        schedule,
+        context,
+        ethContext,
+        TestClock.fixed(),
+        new TransactionPoolMetrics(new NoOpMetricsSystem()),
+        syncState,
+        ImmutableTransactionPoolConfiguration.builder()
+            .txPoolImplementation(implementation)
+            .txPoolMaxSize(1)
+            .pendingTxRetentionPeriod(1)
+            .unstable(
+                ImmutableTransactionPoolConfiguration.Unstable.builder()
+                    .txMessageKeepAliveSeconds(1)
+                    .build())
+            .build(),
+        peerTransactionTracker,
+        transactionsMessageSender,
+        newPooledTransactionHashesMessageSender,
+        new BlobCache(),
+        MiningParameters.newDefault());
+  }
 
+  private TransactionPool createAndEnableTransactionPool(
+      final TransactionPoolConfiguration.Implementation implementation, final SyncState syncState) {
+    final TransactionPool txPool = createTransactionPool(implementation, syncState);
     txPool.setEnabled();
     return txPool;
   }