Ensure block height manager is restarted when BFT coordinator is

CHANGELOG.md

@@ -40,6 +40,7 @@
 
 ### Bug fixes
 - Add missing RPC method `debug_accountRange` to `RpcMethod.java` so this method can be used with `--rpc-http-api-method-no-auth` [#8153](https://github.com/hyperledger/besu/issues/8153)
+- Fix issue with new QBFT/IBFT blocks being produced under certain circumstances. [#8308](https://github.com/hyperledger/besu/issues/8308)
 
 ## 25.2.1 hotfix
 - Pectra - update Holesky and Sepolia deposit contract addresses [#8346](https://github.com/hyperledger/besu/pull/8346)

consensus/common/src/main/java/org/hyperledger/besu/consensus/common/bft/blockcreation/BftMiningCoordinator.java

@@ -115,6 +115,7 @@ public void stop() {
         LOG.debug("Interrupted while waiting for BftProcessor to stop.", e);
         Thread.currentThread().interrupt();
       }
+      eventHandler.stop();
       bftExecutors.stop();
     }
   }

consensus/common/src/main/java/org/hyperledger/besu/consensus/common/bft/statemachine/BaseBftController.java

@@ -77,6 +77,20 @@ protected BaseBftController(
   public void start() {
     if (started.compareAndSet(false, true)) {
       startNewHeightManager(blockchain.getChainHeadHeader());
+    } else {
+      // In normal circumstances the height manager should only be started once. If the caller
+      // has stopped the height manager (e.g. while sync completes) they must call stop() before
+      // starting the height manager again.
+      throw new IllegalStateException(
+          "Attempt to start new height manager without stopping previous manager");
+    }
+  }
+
+  @Override
+  public void stop() {
+    if (started.compareAndSet(true, false)) {
+      stopCurrentHeightManager(blockchain.getChainHeadHeader());
+      LOG.debug("Height manager stopped");
     }
   }
 
@@ -206,6 +220,13 @@ public void handleRoundExpiry(final RoundExpiry roundExpiry) {
    */
   protected abstract BaseBlockHeightManager getCurrentHeightManager();
 
+  /**
+   * Stop the current height manager by creating a no-op block height manager.
+   *
+   * @param parentHeader the parent header
+   */
+  protected abstract void stopCurrentHeightManager(final BlockHeader parentHeader);
+
   private void startNewHeightManager(final BlockHeader parentHeader) {
     createNewHeightManager(parentHeader);
     final long newChainHeight = getCurrentHeightManager().getChainHeight();

consensus/common/src/main/java/org/hyperledger/besu/consensus/common/bft/statemachine/BftEventHandler.java

@@ -25,6 +25,9 @@ public interface BftEventHandler {
   /** Start. */
   void start();
 
+  /** Stop. */
+  void stop();
+
   /**
    * Handle message event.
    *

consensus/ibft/src/main/java/org/hyperledger/besu/consensus/ibft/statemachine/IbftBlockHeightManagerFactory.java

@@ -68,7 +68,14 @@ public BaseIbftBlockHeightManager create(final BlockHeader parentHeader) {
     }
   }
 
-  private BaseIbftBlockHeightManager createNoOpBlockHeightManager(final BlockHeader parentHeader) {
+  /**
+   * Create a no-op block height manager.
+   *
+   * @param parentHeader the parent header
+   * @return the no-op height manager
+   */
+  protected BaseIbftBlockHeightManager createNoOpBlockHeightManager(
+      final BlockHeader parentHeader) {
     return new NoOpBlockHeightManager(parentHeader);
   }
 

consensus/ibft/src/main/java/org/hyperledger/besu/consensus/ibft/statemachine/IbftController.java

@@ -117,4 +117,9 @@ protected void createNewHeightManager(final BlockHeader parentHeader) {
   protected BaseBlockHeightManager getCurrentHeightManager() {
     return currentHeightManager;
   }
+
+  @Override
+  protected void stopCurrentHeightManager(final BlockHeader parentHeader) {
+    currentHeightManager = ibftBlockHeightManagerFactory.createNoOpBlockHeightManager(parentHeader);
+  }
 }

consensus/ibft/src/test/java/org/hyperledger/besu/consensus/ibft/statemachine/IbftControllerTest.java

@@ -14,6 +14,8 @@
  */
 package org.hyperledger.besu.consensus.ibft.statemachine;
 
+import static org.assertj.core.api.Assertions.assertThatNoException;
+import static org.assertj.core.api.Assertions.assertThatThrownBy;
 import static org.assertj.core.util.Lists.newArrayList;
 import static org.mockito.ArgumentMatchers.any;
 import static org.mockito.ArgumentMatchers.anyLong;
@@ -478,4 +480,21 @@ private void setupRoundChange(
     when(roundChangeMessageData.decode()).thenReturn(roundChange);
     roundChangeMessage = new DefaultMessage(null, roundChangeMessageData);
   }
+
+  @Test
+  public void heightManagerCanOnlyBeStartedOnceIfNotStopped() {
+    constructIbftController();
+    ibftController.start();
+    assertThatThrownBy(() -> ibftController.start())
+        .isInstanceOf(IllegalStateException.class)
+        .hasMessage("Attempt to start new height manager without stopping previous manager");
+  }
+
+  @Test
+  public void heightManagerCanBeRestartedIfStopped() {
+    constructIbftController();
+    ibftController.start();
+    ibftController.stop();
+    assertThatNoException().isThrownBy(() -> ibftController.start());
+  }
 }

consensus/qbft-core/src/main/java/org/hyperledger/besu/consensus/qbft/core/statemachine/QbftBlockHeightManagerFactory.java

@@ -85,7 +85,13 @@ public void isEarlyRoundChangeEnabled(final boolean isEarlyRoundChangeEnabled) {
     this.isEarlyRoundChangeEnabled = isEarlyRoundChangeEnabled;
   }
 
-  private BaseQbftBlockHeightManager createNoOpBlockHeightManager(
+  /**
+   * Creates a no-op height manager
+   *
+   * @param parentHeader the parent header
+   * @return the no-op height manager
+   */
+  protected BaseQbftBlockHeightManager createNoOpBlockHeightManager(
       final QbftBlockHeader parentHeader) {
     return new NoOpBlockHeightManager(parentHeader);
   }

consensus/qbft-core/src/main/java/org/hyperledger/besu/consensus/qbft/core/statemachine/QbftController.java

@@ -139,10 +139,29 @@ private BaseQbftBlockHeightManager getCurrentHeightManager() {
     return currentHeightManager;
   }
 
+  /* Replace the current height manager with a no-op height manager. */
+  private void stopCurrentHeightManager(final QbftBlockHeader parentHeader) {
+    currentHeightManager = qbftBlockHeightManagerFactory.createNoOpBlockHeightManager(parentHeader);
+  }
+
   @Override
   public void start() {
     if (started.compareAndSet(false, true)) {
       startNewHeightManager(blockchain.getChainHeadHeader());
+    } else {
+      // In normal circumstances the height manager should only be started once. If the caller
+      // has stopped the height manager (e.g. while sync completes) they must call stop() before
+      // starting the height manager again.
+      throw new IllegalStateException(
+          "Attempt to start new height manager without stopping previous manager");
+    }
+  }
+
+  @Override
+  public void stop() {
+    if (started.compareAndSet(true, false)) {
+      stopCurrentHeightManager(blockchain.getChainHeadHeader());
+      LOG.debug("QBFT height manager stop");
     }
   }
 

consensus/qbft-core/src/main/java/org/hyperledger/besu/consensus/qbft/core/types/QbftEventHandler.java

@@ -24,6 +24,9 @@ public interface QbftEventHandler {
   /** Start. */
   void start();
 
+  /** Stop. */
+  void stop();
+
   /**
    * Handle errorMessage event.
    *

consensus/qbft-core/src/test/java/org/hyperledger/besu/consensus/qbft/core/statemachine/QbftControllerTest.java

@@ -14,6 +14,8 @@
  */
 package org.hyperledger.besu.consensus.qbft.core.statemachine;
 
+import static org.assertj.core.api.Assertions.assertThatNoException;
+import static org.assertj.core.api.Assertions.assertThatThrownBy;
 import static org.assertj.core.util.Lists.newArrayList;
 import static org.mockito.ArgumentMatchers.any;
 import static org.mockito.ArgumentMatchers.anyLong;
@@ -484,4 +486,21 @@ private void setupRoundChange(
     when(roundChangeMessageData.decode(blockEncoder)).thenReturn(roundChange);
     roundChangeMessage = new DefaultMessage(null, roundChangeMessageData);
   }
+
+  @Test
+  public void heightManagerCanOnlyBeStartedOnceIfNotStopped() {
+    constructQbftController();
+    qbftController.start();
+    assertThatThrownBy(() -> qbftController.start())
+        .isInstanceOf(IllegalStateException.class)
+        .hasMessage("Attempt to start new height manager without stopping previous manager");
+  }
+
+  @Test
+  public void heightManagerCanBeRestartedIfStopped() {
+    constructQbftController();
+    qbftController.start();
+    qbftController.stop();
+    assertThatNoException().isThrownBy(() -> qbftController.start());
+  }
 }

consensus/qbft/src/main/java/org/hyperledger/besu/consensus/qbft/adaptor/BftEventHandlerAdaptor.java

@@ -41,6 +41,11 @@ public void start() {
     qbftEventHandler.start();
   }
 
+  @Override
+  public void stop() {
+    qbftEventHandler.stop();
+  }
+
   @Override
   public void handleMessageEvent(final BftReceivedMessageEvent msg) {
     qbftEventHandler.handleMessageEvent(msg);

consensus/qbft/src/test/java/org/hyperledger/besu/consensus/qbft/adaptor/BftEventHandlerAdaptorTest.java

@@ -52,6 +52,12 @@ void startDelegatesToQbftEventHandler() {
     verify(qbftEventHandler).start();
   }
 
+  @Test
+  void stopDelegatesToQbftEventHandler() {
+    handler.stop();
+    verify(qbftEventHandler).stop();
+  }
+
   @Test
   void handleMessageEventDelegatesToQbftEventHandler() {
     handler.handleMessageEvent(bftReceivedMessageEvent);