diff --git a/CHANGELOG.md b/CHANGELOG.md index 28862a6de..9be575c81 100644 --- a/CHANGELOG.md +++ b/CHANGELOG.md @@ -48,6 +48,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) - Add a fallback pivot strategy when the safe block does not change for a long time, to make possible to complete the initial sync in case the chain is not finalizing [#8395](https://github.com/hyperledger/besu/pull/8395) +- Fix issue with new QBFT/IBFT blocks being produced under certain circumstances. [#8308](https://github.com/hyperledger/besu/issues/8308) ## 25.2.2 hotfix - Pectra - Sepolia: Fix for deposit contract log decoding [#8383](https://github.com/hyperledger/besu/pull/8383) diff --git a/consensus/common/src/main/java/org/hyperledger/besu/consensus/common/bft/blockcreation/BftMiningCoordinator.java b/consensus/common/src/main/java/org/hyperledger/besu/consensus/common/bft/blockcreation/BftMiningCoordinator.java index 795a064b6..79ee9af1a 100644 --- a/consensus/common/src/main/java/org/hyperledger/besu/consensus/common/bft/blockcreation/BftMiningCoordinator.java +++ b/consensus/common/src/main/java/org/hyperledger/besu/consensus/common/bft/blockcreation/BftMiningCoordinator.java @@ -115,6 +115,7 @@ public class BftMiningCoordinator implements MiningCoordinator, BlockAddedObserv LOG.debug("Interrupted while waiting for BftProcessor to stop.", e); Thread.currentThread().interrupt(); } + eventHandler.stop(); bftExecutors.stop(); } } diff --git a/consensus/common/src/main/java/org/hyperledger/besu/consensus/common/bft/statemachine/BaseBftController.java b/consensus/common/src/main/java/org/hyperledger/besu/consensus/common/bft/statemachine/BaseBftController.java index 58d579f28..812094a40 100644 --- a/consensus/common/src/main/java/org/hyperledger/besu/consensus/common/bft/statemachine/BaseBftController.java +++ b/consensus/common/src/main/java/org/hyperledger/besu/consensus/common/bft/statemachine/BaseBftController.java @@ -77,6 +77,20 @@ public abstract class BaseBftController implements BftEventHandler { 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 abstract class BaseBftController implements BftEventHandler { */ 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(); diff --git a/consensus/common/src/main/java/org/hyperledger/besu/consensus/common/bft/statemachine/BftEventHandler.java b/consensus/common/src/main/java/org/hyperledger/besu/consensus/common/bft/statemachine/BftEventHandler.java index 223a17ed0..c307ab571 100644 --- a/consensus/common/src/main/java/org/hyperledger/besu/consensus/common/bft/statemachine/BftEventHandler.java +++ b/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. * diff --git a/consensus/ibft/src/main/java/org/hyperledger/besu/consensus/ibft/statemachine/IbftBlockHeightManagerFactory.java b/consensus/ibft/src/main/java/org/hyperledger/besu/consensus/ibft/statemachine/IbftBlockHeightManagerFactory.java index 50a518b4e..8218ea173 100644 --- a/consensus/ibft/src/main/java/org/hyperledger/besu/consensus/ibft/statemachine/IbftBlockHeightManagerFactory.java +++ b/consensus/ibft/src/main/java/org/hyperledger/besu/consensus/ibft/statemachine/IbftBlockHeightManagerFactory.java @@ -68,7 +68,14 @@ public class IbftBlockHeightManagerFactory { } } - 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); } diff --git a/consensus/ibft/src/main/java/org/hyperledger/besu/consensus/ibft/statemachine/IbftController.java b/consensus/ibft/src/main/java/org/hyperledger/besu/consensus/ibft/statemachine/IbftController.java index e2156bd7d..dc92aa53c 100644 --- a/consensus/ibft/src/main/java/org/hyperledger/besu/consensus/ibft/statemachine/IbftController.java +++ b/consensus/ibft/src/main/java/org/hyperledger/besu/consensus/ibft/statemachine/IbftController.java @@ -117,4 +117,9 @@ public class IbftController extends BaseBftController { protected BaseBlockHeightManager getCurrentHeightManager() { return currentHeightManager; } + + @Override + protected void stopCurrentHeightManager(final BlockHeader parentHeader) { + currentHeightManager = ibftBlockHeightManagerFactory.createNoOpBlockHeightManager(parentHeader); + } } diff --git a/consensus/ibft/src/test/java/org/hyperledger/besu/consensus/ibft/statemachine/IbftControllerTest.java b/consensus/ibft/src/test/java/org/hyperledger/besu/consensus/ibft/statemachine/IbftControllerTest.java index 6999ae398..3a2de65c5 100644 --- a/consensus/ibft/src/test/java/org/hyperledger/besu/consensus/ibft/statemachine/IbftControllerTest.java +++ b/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 @@ public class IbftControllerTest { 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()); + } } diff --git a/consensus/qbft-core/src/main/java/org/hyperledger/besu/consensus/qbft/core/statemachine/QbftBlockHeightManagerFactory.java b/consensus/qbft-core/src/main/java/org/hyperledger/besu/consensus/qbft/core/statemachine/QbftBlockHeightManagerFactory.java index cc957513d..b2bb40335 100644 --- a/consensus/qbft-core/src/main/java/org/hyperledger/besu/consensus/qbft/core/statemachine/QbftBlockHeightManagerFactory.java +++ b/consensus/qbft-core/src/main/java/org/hyperledger/besu/consensus/qbft/core/statemachine/QbftBlockHeightManagerFactory.java @@ -85,7 +85,13 @@ public class QbftBlockHeightManagerFactory { 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); } diff --git a/consensus/qbft-core/src/main/java/org/hyperledger/besu/consensus/qbft/core/statemachine/QbftController.java b/consensus/qbft-core/src/main/java/org/hyperledger/besu/consensus/qbft/core/statemachine/QbftController.java index c2fdaaf70..2471f4164 100644 --- a/consensus/qbft-core/src/main/java/org/hyperledger/besu/consensus/qbft/core/statemachine/QbftController.java +++ b/consensus/qbft-core/src/main/java/org/hyperledger/besu/consensus/qbft/core/statemachine/QbftController.java @@ -139,10 +139,29 @@ public class QbftController implements QbftEventHandler { 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"); } } diff --git a/consensus/qbft-core/src/main/java/org/hyperledger/besu/consensus/qbft/core/types/QbftEventHandler.java b/consensus/qbft-core/src/main/java/org/hyperledger/besu/consensus/qbft/core/types/QbftEventHandler.java index c90a484d8..1a827359c 100644 --- a/consensus/qbft-core/src/main/java/org/hyperledger/besu/consensus/qbft/core/types/QbftEventHandler.java +++ b/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. * diff --git a/consensus/qbft-core/src/test/java/org/hyperledger/besu/consensus/qbft/core/statemachine/QbftControllerTest.java b/consensus/qbft-core/src/test/java/org/hyperledger/besu/consensus/qbft/core/statemachine/QbftControllerTest.java index 75cd3d024..6c322b36e 100644 --- a/consensus/qbft-core/src/test/java/org/hyperledger/besu/consensus/qbft/core/statemachine/QbftControllerTest.java +++ b/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 @@ public class QbftControllerTest { 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()); + } } diff --git a/consensus/qbft/src/main/java/org/hyperledger/besu/consensus/qbft/adaptor/BftEventHandlerAdaptor.java b/consensus/qbft/src/main/java/org/hyperledger/besu/consensus/qbft/adaptor/BftEventHandlerAdaptor.java index 955fe45ce..58c570f3f 100644 --- a/consensus/qbft/src/main/java/org/hyperledger/besu/consensus/qbft/adaptor/BftEventHandlerAdaptor.java +++ b/consensus/qbft/src/main/java/org/hyperledger/besu/consensus/qbft/adaptor/BftEventHandlerAdaptor.java @@ -41,6 +41,11 @@ public class BftEventHandlerAdaptor implements BftEventHandler { qbftEventHandler.start(); } + @Override + public void stop() { + qbftEventHandler.stop(); + } + @Override public void handleMessageEvent(final BftReceivedMessageEvent msg) { qbftEventHandler.handleMessageEvent(msg); diff --git a/consensus/qbft/src/test/java/org/hyperledger/besu/consensus/qbft/adaptor/BftEventHandlerAdaptorTest.java b/consensus/qbft/src/test/java/org/hyperledger/besu/consensus/qbft/adaptor/BftEventHandlerAdaptorTest.java index 9ba5e484e..a15b7d21c 100644 --- a/consensus/qbft/src/test/java/org/hyperledger/besu/consensus/qbft/adaptor/BftEventHandlerAdaptorTest.java +++ b/consensus/qbft/src/test/java/org/hyperledger/besu/consensus/qbft/adaptor/BftEventHandlerAdaptorTest.java @@ -52,6 +52,12 @@ class BftEventHandlerAdaptorTest { verify(qbftEventHandler).start(); } + @Test + void stopDelegatesToQbftEventHandler() { + handler.stop(); + verify(qbftEventHandler).stop(); + } + @Test void handleMessageEventDelegatesToQbftEventHandler() { handler.handleMessageEvent(bftReceivedMessageEvent);