From 9e7624e2af3bd13374944a3a09424024cec7c191 Mon Sep 17 00:00:00 2001 From: Matthew Whitehead Date: Fri, 20 Sep 2024 14:30:12 +0100 Subject: [PATCH 1/8] Support BFT mining coordinator being temporarily stopped while syncing happens Signed-off-by: Matthew Whitehead --- .../controller/QbftBesuControllerBuilder.java | 15 ++++++++++++--- .../besu/consensus/common/bft/BftEventQueue.java | 5 +++++ .../besu/consensus/common/bft/BftExecutors.java | 2 +- .../besu/consensus/common/bft/BftProcessor.java | 9 ++++++++- .../bft/blockcreation/BftMiningCoordinator.java | 11 +++++++++-- 5 files changed, 35 insertions(+), 7 deletions(-) diff --git a/besu/src/main/java/org/hyperledger/besu/controller/QbftBesuControllerBuilder.java b/besu/src/main/java/org/hyperledger/besu/controller/QbftBesuControllerBuilder.java index 3d3412d4860..aec03ec7ee5 100644 --- a/besu/src/main/java/org/hyperledger/besu/controller/QbftBesuControllerBuilder.java +++ b/besu/src/main/java/org/hyperledger/besu/controller/QbftBesuControllerBuilder.java @@ -295,9 +295,18 @@ protected MiningCoordinator createMiningCoordinator( .getValue() .getBlockPeriodSeconds())); - if (syncState.isInitialSyncPhaseDone()) { - miningCoordinator.enable(); - } + syncState.subscribeSyncStatus( + syncStatus -> { + if (syncState.syncTarget().isPresent()) { + // We're syncing so stop doing other stuff + LOG.info("Stopping QBFT mining coordinator while we are syncing"); + miningCoordinator.stop(); + } else { + LOG.info("Starting QBFT mining coordinator following sync"); + miningCoordinator.enable(); + miningCoordinator.start(); + } + }); syncState.subscribeCompletionReached( new BesuEvents.InitialSyncCompletionListener() { diff --git a/consensus/common/src/main/java/org/hyperledger/besu/consensus/common/bft/BftEventQueue.java b/consensus/common/src/main/java/org/hyperledger/besu/consensus/common/bft/BftEventQueue.java index 4221fd1fac7..eec41ed4e9c 100644 --- a/consensus/common/src/main/java/org/hyperledger/besu/consensus/common/bft/BftEventQueue.java +++ b/consensus/common/src/main/java/org/hyperledger/besu/consensus/common/bft/BftEventQueue.java @@ -48,6 +48,11 @@ public void start() { started.set(true); } + /** Stop the event queue. Until it has been started no events will be queued for processing. */ + public void stop() { + started.set(false); + } + private boolean isStarted() { return started.get(); } diff --git a/consensus/common/src/main/java/org/hyperledger/besu/consensus/common/bft/BftExecutors.java b/consensus/common/src/main/java/org/hyperledger/besu/consensus/common/bft/BftExecutors.java index 30038add3d4..709d65faaad 100644 --- a/consensus/common/src/main/java/org/hyperledger/besu/consensus/common/bft/BftExecutors.java +++ b/consensus/common/src/main/java/org/hyperledger/besu/consensus/common/bft/BftExecutors.java @@ -78,7 +78,7 @@ public static BftExecutors create( /** Start. */ public synchronized void start() { - if (state != State.IDLE) { + if (state != State.IDLE && state != State.STOPPED) { // Nothing to do return; } diff --git a/consensus/common/src/main/java/org/hyperledger/besu/consensus/common/bft/BftProcessor.java b/consensus/common/src/main/java/org/hyperledger/besu/consensus/common/bft/BftProcessor.java index 93df77ea778..8834a30b2e4 100644 --- a/consensus/common/src/main/java/org/hyperledger/besu/consensus/common/bft/BftProcessor.java +++ b/consensus/common/src/main/java/org/hyperledger/besu/consensus/common/bft/BftProcessor.java @@ -44,8 +44,13 @@ public BftProcessor(final BftEventQueue incomingQueue, final EventMultiplexer ev this.eventMultiplexer = eventMultiplexer; } + /** Indicate to the processor that it should can be started */ + public synchronized void start() { + shutdown = false; + } + /** Indicate to the processor that it should gracefully stop at its next opportunity */ - public void stop() { + public synchronized void stop() { shutdown = true; } @@ -67,6 +72,8 @@ public void run() { while (!shutdown) { nextEvent().ifPresent(eventMultiplexer::handleBftEvent); } + + incomingQueue.stop(); } catch (final Throwable t) { LOG.error("BFT Mining thread has suffered a fatal error, mining has been halted", t); } 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 83cd61beb31..f5f31a3326b 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 @@ -93,7 +93,9 @@ public BftMiningCoordinator( @Override public void start() { - if (state.compareAndSet(State.IDLE, State.RUNNING)) { + if (state.compareAndSet(State.IDLE, State.RUNNING) + || state.compareAndSet(State.STOPPED, State.RUNNING)) { + bftProcessor.start(); bftExecutors.start(); blockAddedObserverId = blockchain.observeBlockAdded(this); eventHandler.start(); @@ -110,7 +112,7 @@ public void stop() { try { bftProcessor.awaitStop(); } catch (final InterruptedException e) { - LOG.debug("Interrupted while waiting for IbftProcessor to stop.", e); + LOG.debug("Interrupted while waiting for BftProcessor to stop.", e); Thread.currentThread().interrupt(); } bftExecutors.stop(); @@ -135,6 +137,11 @@ public boolean enable() { @Override public boolean disable() { + if (state.get() == State.PAUSED + || state.compareAndSet(State.IDLE, State.PAUSED) + || state.compareAndSet(State.RUNNING, State.PAUSED)) { + return true; + } return false; } From addaa5f20b472fd229c36668bc64bab77a9ddde5 Mon Sep 17 00:00:00 2001 From: Matthew Whitehead Date: Fri, 20 Sep 2024 14:44:43 +0100 Subject: [PATCH 2/8] Apply same change to IbftBesuControllerBuilder Signed-off-by: Matthew Whitehead --- .../controller/IbftBesuControllerBuilder.java | 15 ++++++++++++--- 1 file changed, 12 insertions(+), 3 deletions(-) diff --git a/besu/src/main/java/org/hyperledger/besu/controller/IbftBesuControllerBuilder.java b/besu/src/main/java/org/hyperledger/besu/controller/IbftBesuControllerBuilder.java index 58412029fc3..738dcfc5963 100644 --- a/besu/src/main/java/org/hyperledger/besu/controller/IbftBesuControllerBuilder.java +++ b/besu/src/main/java/org/hyperledger/besu/controller/IbftBesuControllerBuilder.java @@ -253,9 +253,18 @@ protected MiningCoordinator createMiningCoordinator( .getValue() .getBlockPeriodSeconds())); - if (syncState.isInitialSyncPhaseDone()) { - ibftMiningCoordinator.enable(); - } + syncState.subscribeSyncStatus( + syncStatus -> { + if (syncState.syncTarget().isPresent()) { + // We're syncing so stop doing other stuff + LOG.info("Stopping IBFT mining coordinator while we are syncing"); + ibftMiningCoordinator.stop(); + } else { + LOG.info("Starting IBFT mining coordinator following sync"); + ibftMiningCoordinator.enable(); + ibftMiningCoordinator.start(); + } + }); syncState.subscribeCompletionReached( new BesuEvents.InitialSyncCompletionListener() { From d17087c7b05a8bab430dffe732851c4f0763dca8 Mon Sep 17 00:00:00 2001 From: Matthew Whitehead Date: Tue, 8 Oct 2024 11:17:27 +0100 Subject: [PATCH 3/8] Add changelog entry Signed-off-by: Matthew Whitehead --- CHANGELOG.md | 1 + 1 file changed, 1 insertion(+) diff --git a/CHANGELOG.md b/CHANGELOG.md index 5bcbfa51b7f..85dc72ac707 100644 --- a/CHANGELOG.md +++ b/CHANGELOG.md @@ -28,6 +28,7 @@ - Corrects a regression where custom plugin services are not initialized correctly. [#7625](https://github.com/hyperledger/besu/pull/7625) - Fix for IBFT2 chains using the BONSAI DB format [#7631](https://github.com/hyperledger/besu/pull/7631) - Fix reading `tx-pool-min-score` option from configuration file [#7623](https://github.com/hyperledger/besu/pull/7623) +- If a BFT validator node is syncing, pause block production until sync has completed [#7657](https://github.com/hyperledger/besu/pull/7657) ## 24.9.1 From 1c2a7d0e3249e147ffd2a7a2db65e30ed56f5659 Mon Sep 17 00:00:00 2001 From: Matthew Whitehead Date: Tue, 8 Oct 2024 11:53:11 +0100 Subject: [PATCH 4/8] Add event queue start/stop test Signed-off-by: Matthew Whitehead --- .../common/bft/BftEventQueueTest.java | 34 +++++++++++++++++++ 1 file changed, 34 insertions(+) diff --git a/consensus/common/src/test/java/org/hyperledger/besu/consensus/common/bft/BftEventQueueTest.java b/consensus/common/src/test/java/org/hyperledger/besu/consensus/common/bft/BftEventQueueTest.java index b39bef3a439..6fbf701bc17 100644 --- a/consensus/common/src/test/java/org/hyperledger/besu/consensus/common/bft/BftEventQueueTest.java +++ b/consensus/common/src/test/java/org/hyperledger/besu/consensus/common/bft/BftEventQueueTest.java @@ -134,6 +134,40 @@ public void doNotAddUntilStarted() throws InterruptedException { assertThat(queue.poll(0, TimeUnit.MICROSECONDS)).isNull(); } + @Test + public void supportsStopAndRestart() throws InterruptedException { + final BftEventQueue queue = new BftEventQueue(MAX_QUEUE_SIZE); + queue.start(); + + assertThat(queue.poll(0, TimeUnit.MICROSECONDS)).isNull(); + final DummyBftEvent dummyMessageEvent = new DummyBftEvent(); + final DummyRoundExpiryBftEvent dummyRoundTimerEvent = new DummyRoundExpiryBftEvent(); + final DummyNewChainHeadBftEvent dummyNewChainHeadEvent = new DummyNewChainHeadBftEvent(); + + queue.add(dummyMessageEvent); + queue.add(dummyRoundTimerEvent); + queue.add(dummyNewChainHeadEvent); + assertThat(queue.poll(0, TimeUnit.MICROSECONDS)).isNotNull(); + assertThat(queue.poll(0, TimeUnit.MICROSECONDS)).isNotNull(); + assertThat(queue.poll(0, TimeUnit.MICROSECONDS)).isNotNull(); + assertThat(queue.poll(0, TimeUnit.MICROSECONDS)).isNull(); + + queue.stop(); + queue.add(dummyMessageEvent); + queue.add(dummyRoundTimerEvent); + queue.add(dummyNewChainHeadEvent); + assertThat(queue.poll(0, TimeUnit.MICROSECONDS)).isNull(); + + queue.start(); + queue.add(dummyMessageEvent); + queue.add(dummyRoundTimerEvent); + queue.add(dummyNewChainHeadEvent); + assertThat(queue.poll(0, TimeUnit.MICROSECONDS)).isNotNull(); + assertThat(queue.poll(0, TimeUnit.MICROSECONDS)).isNotNull(); + assertThat(queue.poll(0, TimeUnit.MICROSECONDS)).isNotNull(); + assertThat(queue.poll(0, TimeUnit.MICROSECONDS)).isNull(); + } + @Test public void alwaysAddBlockTimerExpiryEvents() throws InterruptedException { final BftEventQueue queue = new BftEventQueue(MAX_QUEUE_SIZE); From 8510dafdfa0e977126be7fda6547fe8e028fbad0 Mon Sep 17 00:00:00 2001 From: Matthew Whitehead Date: Tue, 8 Oct 2024 12:01:56 +0100 Subject: [PATCH 5/8] Add BFT mining coordinator tests Signed-off-by: Matthew Whitehead --- .../blockcreation/BftMiningCoordinator.java | 2 +- .../BftMiningCoordinatorTest.java | 24 +++++++++++++++++++ 2 files changed, 25 insertions(+), 1 deletion(-) 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 f5f31a3326b..795a064b6bc 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 @@ -147,7 +147,7 @@ public boolean disable() { @Override public boolean isMining() { - return true; + return state.get() == State.RUNNING; } @Override diff --git a/consensus/common/src/test/java/org/hyperledger/besu/consensus/common/bft/blockcreation/BftMiningCoordinatorTest.java b/consensus/common/src/test/java/org/hyperledger/besu/consensus/common/bft/blockcreation/BftMiningCoordinatorTest.java index cb0dffba9b1..9b5ba7a727e 100644 --- a/consensus/common/src/test/java/org/hyperledger/besu/consensus/common/bft/blockcreation/BftMiningCoordinatorTest.java +++ b/consensus/common/src/test/java/org/hyperledger/besu/consensus/common/bft/blockcreation/BftMiningCoordinatorTest.java @@ -19,6 +19,7 @@ import static org.mockito.Mockito.never; import static org.mockito.Mockito.verify; import static org.mockito.Mockito.when; +import static org.mockito.internal.verification.VerificationModeFactory.times; import org.hyperledger.besu.consensus.common.bft.BftEventQueue; import org.hyperledger.besu.consensus.common.bft.BftExecutors; @@ -82,6 +83,29 @@ public void stopsMining() { verify(bftProcessor).stop(); } + @Test + public void restartsMiningAfterStop() { + // Shouldn't stop without first starting + assertThat(bftMiningCoordinator.isMining()).isFalse(); + bftMiningCoordinator.stop(); + verify(bftProcessor, never()).stop(); + + bftMiningCoordinator.enable(); + bftMiningCoordinator.start(); + assertThat(bftMiningCoordinator.isMining()).isTrue(); + + bftMiningCoordinator.stop(); + assertThat(bftMiningCoordinator.isMining()).isFalse(); + verify(bftProcessor).stop(); + + bftMiningCoordinator.start(); + assertThat(bftMiningCoordinator.isMining()).isTrue(); + + // BFT process should be started once for every time the mining + // coordinator is restarted + verify(bftProcessor, times(2)).start(); + } + @Test public void getsMinTransactionGasPrice() { final Wei minGasPrice = Wei.of(10); From 2c2b96ada614cb5576f2e97dc8e2faacdf09a0ae Mon Sep 17 00:00:00 2001 From: Matthew Whitehead Date: Tue, 8 Oct 2024 12:03:01 +0100 Subject: [PATCH 6/8] Typo Signed-off-by: Matthew Whitehead --- .../common/bft/blockcreation/BftMiningCoordinatorTest.java | 3 +-- 1 file changed, 1 insertion(+), 2 deletions(-) diff --git a/consensus/common/src/test/java/org/hyperledger/besu/consensus/common/bft/blockcreation/BftMiningCoordinatorTest.java b/consensus/common/src/test/java/org/hyperledger/besu/consensus/common/bft/blockcreation/BftMiningCoordinatorTest.java index 9b5ba7a727e..328f9fd7b75 100644 --- a/consensus/common/src/test/java/org/hyperledger/besu/consensus/common/bft/blockcreation/BftMiningCoordinatorTest.java +++ b/consensus/common/src/test/java/org/hyperledger/besu/consensus/common/bft/blockcreation/BftMiningCoordinatorTest.java @@ -85,7 +85,6 @@ public void stopsMining() { @Test public void restartsMiningAfterStop() { - // Shouldn't stop without first starting assertThat(bftMiningCoordinator.isMining()).isFalse(); bftMiningCoordinator.stop(); verify(bftProcessor, never()).stop(); @@ -101,7 +100,7 @@ public void restartsMiningAfterStop() { bftMiningCoordinator.start(); assertThat(bftMiningCoordinator.isMining()).isTrue(); - // BFT process should be started once for every time the mining + // BFT processor should be started once for every time the mining // coordinator is restarted verify(bftProcessor, times(2)).start(); } From eff00ca36f024f8e89d1b08867f3ab6a7e2d27e8 Mon Sep 17 00:00:00 2001 From: Matt Whitehead Date: Wed, 9 Oct 2024 11:36:17 +0100 Subject: [PATCH 7/8] Update consensus/common/src/main/java/org/hyperledger/besu/consensus/common/bft/BftEventQueue.java Co-authored-by: Sally MacFarlane Signed-off-by: Matt Whitehead --- .../hyperledger/besu/consensus/common/bft/BftEventQueue.java | 2 +- 1 file changed, 1 insertion(+), 1 deletion(-) diff --git a/consensus/common/src/main/java/org/hyperledger/besu/consensus/common/bft/BftEventQueue.java b/consensus/common/src/main/java/org/hyperledger/besu/consensus/common/bft/BftEventQueue.java index eec41ed4e9c..8f6190c4cfc 100644 --- a/consensus/common/src/main/java/org/hyperledger/besu/consensus/common/bft/BftEventQueue.java +++ b/consensus/common/src/main/java/org/hyperledger/besu/consensus/common/bft/BftEventQueue.java @@ -48,7 +48,7 @@ public void start() { started.set(true); } - /** Stop the event queue. Until it has been started no events will be queued for processing. */ + /** Stop the event queue. No events will be queued for processing until it is started. */ public void stop() { started.set(false); } From 05fb7c73b8b762f927556ad80643eecd14a5d63e Mon Sep 17 00:00:00 2001 From: Matt Whitehead Date: Wed, 9 Oct 2024 11:36:24 +0100 Subject: [PATCH 8/8] Update consensus/common/src/main/java/org/hyperledger/besu/consensus/common/bft/BftProcessor.java Co-authored-by: Sally MacFarlane Signed-off-by: Matt Whitehead --- .../org/hyperledger/besu/consensus/common/bft/BftProcessor.java | 2 +- 1 file changed, 1 insertion(+), 1 deletion(-) diff --git a/consensus/common/src/main/java/org/hyperledger/besu/consensus/common/bft/BftProcessor.java b/consensus/common/src/main/java/org/hyperledger/besu/consensus/common/bft/BftProcessor.java index 8834a30b2e4..81be83b469e 100644 --- a/consensus/common/src/main/java/org/hyperledger/besu/consensus/common/bft/BftProcessor.java +++ b/consensus/common/src/main/java/org/hyperledger/besu/consensus/common/bft/BftProcessor.java @@ -44,7 +44,7 @@ public BftProcessor(final BftEventQueue incomingQueue, final EventMultiplexer ev this.eventMultiplexer = eventMultiplexer; } - /** Indicate to the processor that it should can be started */ + /** Indicate to the processor that it can be started */ public synchronized void start() { shutdown = false; }