From ab5d38f426fcd1407612d713b609cdcad3d197cf Mon Sep 17 00:00:00 2001 From: Matthew Whitehead Date: Fri, 14 Feb 2025 12:32:00 +0000 Subject: [PATCH 1/9] Ensure block height manager is restarted when BFT coordinator is Signed-off-by: Matthew Whitehead --- .../common/bft/blockcreation/BftMiningCoordinator.java | 1 + .../common/bft/statemachine/BaseBftController.java | 7 +++++++ .../consensus/common/bft/statemachine/BftEventHandler.java | 3 +++ 3 files changed, 11 insertions(+) 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 795a064b6bc..79ee9af1a89 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 void stop() { 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 58d579f28e1..bb0e1619ba4 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 @@ -80,6 +80,13 @@ public void start() { } } + @Override + public void stop() { + if (started.compareAndSet(true, false)) { + LOG.debug("Stopped current height manager"); + } + } + @Override public void handleMessageEvent(final BftReceivedMessageEvent msg) { final MessageData data = msg.getMessage().getData(); 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 223a17ed060..c307ab57111 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. * From 6bf8c2c2fa045f3db2cf0c8cf39aceaa2b177d9f Mon Sep 17 00:00:00 2001 From: Matthew Whitehead Date: Wed, 19 Feb 2025 11:09:25 +0000 Subject: [PATCH 2/9] Refactor and add runtime exception if an attempt is made to restart without calling reset Signed-off-by: Matthew Whitehead --- .../common/bft/blockcreation/BftMiningCoordinator.java | 2 +- .../common/bft/statemachine/BaseBftController.java | 9 +++++++-- .../common/bft/statemachine/BftEventHandler.java | 4 ++-- 3 files changed, 10 insertions(+), 5 deletions(-) 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 79ee9af1a89..45b3ec402e7 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,7 +115,7 @@ public void stop() { LOG.debug("Interrupted while waiting for BftProcessor to stop.", e); Thread.currentThread().interrupt(); } - eventHandler.stop(); + eventHandler.reset(); 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 bb0e1619ba4..79b729318c5 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,13 +77,18 @@ 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 reset() before + // starting the height manager again. + throw new IllegalStateException("Attempt to start new height manager without resetting"); } } @Override - public void stop() { + public void reset() { if (started.compareAndSet(true, false)) { - LOG.debug("Stopped current height manager"); + LOG.debug("Height manager reset"); } } 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 c307ab57111..4bc4d1c783c 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,8 +25,8 @@ public interface BftEventHandler { /** Start. */ void start(); - /** Stop. */ - void stop(); + /** Reset. */ + void reset(); /** * Handle message event. From c1d29c245a0b4b5560cc9adc125452635b2f3064 Mon Sep 17 00:00:00 2001 From: Matthew Whitehead Date: Wed, 19 Feb 2025 11:21:40 +0000 Subject: [PATCH 3/9] Update BFT adaptor Signed-off-by: Matthew Whitehead --- .../besu/consensus/qbft/adaptor/BftEventHandlerAdaptor.java | 5 +++++ 1 file changed, 5 insertions(+) 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 955fe45ce24..9894ba01926 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 void start() { qbftEventHandler.start(); } + @Override + public void reset() { + // Not implemented + } + @Override public void handleMessageEvent(final BftReceivedMessageEvent msg) { qbftEventHandler.handleMessageEvent(msg); From 07a4f4b4e20041bf5ecc017c579b23a3f89de6e7 Mon Sep 17 00:00:00 2001 From: Matthew Whitehead Date: Wed, 19 Feb 2025 13:08:53 +0000 Subject: [PATCH 4/9] Uupdate changelog Signed-off-by: Matthew Whitehead --- CHANGELOG.md | 1 + 1 file changed, 1 insertion(+) diff --git a/CHANGELOG.md b/CHANGELOG.md index fa4331f1374..023c0a3442f 100644 --- a/CHANGELOG.md +++ b/CHANGELOG.md @@ -25,6 +25,7 @@ ### Bug fixes - Upgrade Netty to version 4.1.118 to fix CVE-2025-24970 [#8275](https://github.com/hyperledger/besu/pull/8275) - Add missing RPC method `debug_accountRange` to `RpcMethod.java` and implemented its handler. [#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.0 From 921ca0686ae5a193346ab361fe75309208274eeb Mon Sep 17 00:00:00 2001 From: Matthew Whitehead Date: Wed, 19 Feb 2025 14:49:52 +0000 Subject: [PATCH 5/9] Add QBFT implementation and tests Signed-off-by: Matthew Whitehead --- .../core/statemachine/QbftController.java | 12 ++++++++++++ .../qbft/core/types/QbftEventHandler.java | 3 +++ .../core/statemachine/QbftControllerTest.java | 19 +++++++++++++++++++ .../qbft/adaptor/BftEventHandlerAdaptor.java | 2 +- .../adaptor/BftEventHandlerAdaptorTest.java | 6 ++++++ 5 files changed, 41 insertions(+), 1 deletion(-) 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 c2fdaaf70af..c5349a74ba0 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 @@ -143,6 +143,18 @@ private BaseQbftBlockHeightManager getCurrentHeightManager() { 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 reset() before + // starting the height manager again. + throw new IllegalStateException("Attempt to start new height manager without resetting"); + } + } + + @Override + public void reset() { + if (started.compareAndSet(true, false)) { + LOG.debug("QBFT height manager reset"); } } 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 c90a484d878..f1d7f5209b6 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(); + /** Reset. */ + void reset(); + /** * 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 75cd3d024ee..ea0c2b60980 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 @@ private void setupRoundChange( when(roundChangeMessageData.decode(blockEncoder)).thenReturn(roundChange); roundChangeMessage = new DefaultMessage(null, roundChangeMessageData); } + + @Test + public void heightManagerCanOnlyBeStartedOnceWithoutReset() { + constructQbftController(); + qbftController.start(); + assertThatThrownBy(() -> qbftController.start()) + .isInstanceOf(IllegalStateException.class) + .hasMessage("Attempt to start new height manager without resetting"); + } + + @Test + public void heightManagerCanBeRestartedIfReset() { + constructQbftController(); + qbftController.start(); + qbftController.reset(); + 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 9894ba01926..ecbcb15fb23 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 @@ -43,7 +43,7 @@ public void start() { @Override public void reset() { - // Not implemented + qbftEventHandler.reset(); } @Override 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 9ba5e484ed4..3386babeeae 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 @@ void startDelegatesToQbftEventHandler() { verify(qbftEventHandler).start(); } + @Test + void resetDelegatesToQbftEventHandler() { + handler.reset(); + verify(qbftEventHandler).reset(); + } + @Test void handleMessageEventDelegatesToQbftEventHandler() { handler.handleMessageEvent(bftReceivedMessageEvent); From 9f27d28f2d71882f3064c7dbad781fb56a906a8c Mon Sep 17 00:00:00 2001 From: Matthew Whitehead Date: Wed, 19 Feb 2025 14:56:00 +0000 Subject: [PATCH 6/9] Update IBFT tests Signed-off-by: Matthew Whitehead --- .../ibft/statemachine/IbftControllerTest.java | 19 +++++++++++++++++++ 1 file changed, 19 insertions(+) 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 6999ae398a8..9ffd96da37a 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 @@ private void setupRoundChange( when(roundChangeMessageData.decode()).thenReturn(roundChange); roundChangeMessage = new DefaultMessage(null, roundChangeMessageData); } + + @Test + public void heightManagerCanOnlyBeStartedOnceWithoutReset() { + constructIbftController(); + ibftController.start(); + assertThatThrownBy(() -> ibftController.start()) + .isInstanceOf(IllegalStateException.class) + .hasMessage("Attempt to start new height manager without resetting"); + } + + @Test + public void heightManagerCanBeRestartedIfReset() { + constructIbftController(); + ibftController.start(); + ibftController.reset(); + assertThatNoException().isThrownBy(() -> ibftController.start()); + } } From 9a0087b42faaaa1aa45a6532682edde1f76e5196 Mon Sep 17 00:00:00 2001 From: Matthew Whitehead Date: Wed, 26 Feb 2025 11:53:01 +0000 Subject: [PATCH 7/9] Rename reset() -> stop() Signed-off-by: Matthew Whitehead --- .../common/bft/blockcreation/BftMiningCoordinator.java | 2 +- .../common/bft/statemachine/BaseBftController.java | 9 +++++---- .../common/bft/statemachine/BftEventHandler.java | 4 ++-- .../consensus/ibft/statemachine/IbftControllerTest.java | 8 ++++---- .../consensus/qbft/core/statemachine/QbftController.java | 9 +++++---- .../besu/consensus/qbft/core/types/QbftEventHandler.java | 4 ++-- .../qbft/core/statemachine/QbftControllerTest.java | 8 ++++---- .../consensus/qbft/adaptor/BftEventHandlerAdaptor.java | 4 ++-- .../qbft/adaptor/BftEventHandlerAdaptorTest.java | 6 +++--- 9 files changed, 28 insertions(+), 26 deletions(-) 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 45b3ec402e7..79ee9af1a89 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,7 +115,7 @@ public void stop() { LOG.debug("Interrupted while waiting for BftProcessor to stop.", e); Thread.currentThread().interrupt(); } - eventHandler.reset(); + 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 79b729318c5..003ebbc939f 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 @@ -79,16 +79,17 @@ public void start() { 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 reset() before + // 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 resetting"); + throw new IllegalStateException( + "Attempt to start new height manager without stopping previous manager"); } } @Override - public void reset() { + public void stop() { if (started.compareAndSet(true, false)) { - LOG.debug("Height manager reset"); + LOG.debug("Height manager stopped"); } } 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 4bc4d1c783c..c307ab57111 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,8 +25,8 @@ public interface BftEventHandler { /** Start. */ void start(); - /** Reset. */ - void reset(); + /** Stop. */ + void stop(); /** * Handle message event. 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 9ffd96da37a..3a2de65c58e 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 @@ -482,19 +482,19 @@ private void setupRoundChange( } @Test - public void heightManagerCanOnlyBeStartedOnceWithoutReset() { + public void heightManagerCanOnlyBeStartedOnceIfNotStopped() { constructIbftController(); ibftController.start(); assertThatThrownBy(() -> ibftController.start()) .isInstanceOf(IllegalStateException.class) - .hasMessage("Attempt to start new height manager without resetting"); + .hasMessage("Attempt to start new height manager without stopping previous manager"); } @Test - public void heightManagerCanBeRestartedIfReset() { + public void heightManagerCanBeRestartedIfStopped() { constructIbftController(); ibftController.start(); - ibftController.reset(); + ibftController.stop(); assertThatNoException().isThrownBy(() -> ibftController.start()); } } 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 c5349a74ba0..094427dc018 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 @@ -145,16 +145,17 @@ public void start() { 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 reset() before + // 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 resetting"); + throw new IllegalStateException( + "Attempt to start new height manager without stopping previous manager"); } } @Override - public void reset() { + public void stop() { if (started.compareAndSet(true, false)) { - LOG.debug("QBFT height manager reset"); + 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 f1d7f5209b6..1a827359cf5 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,8 +24,8 @@ public interface QbftEventHandler { /** Start. */ void start(); - /** Reset. */ - void reset(); + /** 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 ea0c2b60980..6c322b36e47 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 @@ -488,19 +488,19 @@ private void setupRoundChange( } @Test - public void heightManagerCanOnlyBeStartedOnceWithoutReset() { + public void heightManagerCanOnlyBeStartedOnceIfNotStopped() { constructQbftController(); qbftController.start(); assertThatThrownBy(() -> qbftController.start()) .isInstanceOf(IllegalStateException.class) - .hasMessage("Attempt to start new height manager without resetting"); + .hasMessage("Attempt to start new height manager without stopping previous manager"); } @Test - public void heightManagerCanBeRestartedIfReset() { + public void heightManagerCanBeRestartedIfStopped() { constructQbftController(); qbftController.start(); - qbftController.reset(); + 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 ecbcb15fb23..58c570f3fda 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 @@ -42,8 +42,8 @@ public void start() { } @Override - public void reset() { - qbftEventHandler.reset(); + public void stop() { + qbftEventHandler.stop(); } @Override 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 3386babeeae..a15b7d21cc6 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 @@ -53,9 +53,9 @@ void startDelegatesToQbftEventHandler() { } @Test - void resetDelegatesToQbftEventHandler() { - handler.reset(); - verify(qbftEventHandler).reset(); + void stopDelegatesToQbftEventHandler() { + handler.stop(); + verify(qbftEventHandler).stop(); } @Test From df2332fbe8d2df5898cd6495b4d1c5bf05463979 Mon Sep 17 00:00:00 2001 From: Matthew Whitehead Date: Thu, 27 Feb 2025 09:47:54 +0000 Subject: [PATCH 8/9] Replace the height manager with a no-op height manager while it's stopped Signed-off-by: Matthew Whitehead --- .../common/bft/statemachine/BaseBftController.java | 4 ++++ .../ibft/statemachine/IbftBlockHeightManagerFactory.java | 3 ++- .../besu/consensus/ibft/statemachine/IbftController.java | 5 +++++ .../core/statemachine/QbftBlockHeightManagerFactory.java | 2 +- .../consensus/qbft/core/statemachine/QbftController.java | 6 ++++++ 5 files changed, 18 insertions(+), 2 deletions(-) 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 003ebbc939f..29ab137f5bc 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 @@ -89,6 +89,7 @@ public void start() { @Override public void stop() { if (started.compareAndSet(true, false)) { + stopCurrentHeightManager(blockchain.getChainHeadHeader()); LOG.debug("Height manager stopped"); } } @@ -219,6 +220,9 @@ public void handleRoundExpiry(final RoundExpiry roundExpiry) { */ protected abstract BaseBlockHeightManager getCurrentHeightManager(); + /** Replaces the current height manager with a no-op height manager. */ + protected abstract void stopCurrentHeightManager(final BlockHeader parentHeader); + private void startNewHeightManager(final BlockHeader parentHeader) { createNewHeightManager(parentHeader); final long newChainHeight = getCurrentHeightManager().getChainHeight(); 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 50a518b4e19..352789a47ab 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,8 @@ public BaseIbftBlockHeightManager create(final BlockHeader parentHeader) { } } - private BaseIbftBlockHeightManager createNoOpBlockHeightManager(final BlockHeader parentHeader) { + 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 e2156bd7d6c..dc92aa53c31 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 @@ protected void createNewHeightManager(final BlockHeader parentHeader) { protected BaseBlockHeightManager getCurrentHeightManager() { return currentHeightManager; } + + @Override + protected void stopCurrentHeightManager(final BlockHeader parentHeader) { + currentHeightManager = ibftBlockHeightManagerFactory.createNoOpBlockHeightManager(parentHeader); + } } 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 cc957513d91..19a4d29c4af 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,7 @@ public void isEarlyRoundChangeEnabled(final boolean isEarlyRoundChangeEnabled) { this.isEarlyRoundChangeEnabled = isEarlyRoundChangeEnabled; } - private BaseQbftBlockHeightManager createNoOpBlockHeightManager( + 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 094427dc018..2471f416415 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,6 +139,11 @@ 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)) { @@ -155,6 +160,7 @@ public void start() { @Override public void stop() { if (started.compareAndSet(true, false)) { + stopCurrentHeightManager(blockchain.getChainHeadHeader()); LOG.debug("QBFT height manager stop"); } } From 03b17a689108ba452b792dfce8268e3f5e770a9f Mon Sep 17 00:00:00 2001 From: Matthew Whitehead Date: Thu, 27 Feb 2025 10:16:07 +0000 Subject: [PATCH 9/9] Javadoc Signed-off-by: Matthew Whitehead --- .../common/bft/statemachine/BaseBftController.java | 6 +++++- .../ibft/statemachine/IbftBlockHeightManagerFactory.java | 6 ++++++ .../core/statemachine/QbftBlockHeightManagerFactory.java | 6 ++++++ 3 files changed, 17 insertions(+), 1 deletion(-) 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 29ab137f5bc..812094a404f 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 @@ -220,7 +220,11 @@ public void handleRoundExpiry(final RoundExpiry roundExpiry) { */ protected abstract BaseBlockHeightManager getCurrentHeightManager(); - /** Replaces the current height manager with a no-op height manager. */ + /** + * 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) { 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 352789a47ab..8218ea17383 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,6 +68,12 @@ public BaseIbftBlockHeightManager create(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/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 19a4d29c4af..b2bb40335bc 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,6 +85,12 @@ public void isEarlyRoundChangeEnabled(final boolean isEarlyRoundChangeEnabled) { this.isEarlyRoundChangeEnabled = isEarlyRoundChangeEnabled; } + /** + * 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);