Skip to content

Commit 083b1d3

Browse files
matthew1001matthew1001
authoredMar 12, 2025··
Ensure block height manager is restarted when BFT coordinator is (#8308)
* Ensure block height manager is restarted when BFT coordinator is Signed-off-by: Matthew Whitehead <matthew1001@gmail.com> * Refactor and add runtime exception if an attempt is made to restart without calling reset Signed-off-by: Matthew Whitehead <matthew1001@gmail.com> * Update BFT adaptor Signed-off-by: Matthew Whitehead <matthew1001@gmail.com> * Uupdate changelog Signed-off-by: Matthew Whitehead <matthew1001@gmail.com> * Add QBFT implementation and tests Signed-off-by: Matthew Whitehead <matthew1001@gmail.com> * Update IBFT tests Signed-off-by: Matthew Whitehead <matthew1001@gmail.com> * Rename reset() -> stop() Signed-off-by: Matthew Whitehead <matthew.whitehead@kaleido.io> * Replace the height manager with a no-op height manager while it's stopped Signed-off-by: Matthew Whitehead <matthew1001@gmail.com> * Javadoc Signed-off-by: Matthew Whitehead <matthew1001@gmail.com> --------- Signed-off-by: Matthew Whitehead <matthew1001@gmail.com> Signed-off-by: Matthew Whitehead <matthew.whitehead@kaleido.io> Signed-off-by: Matt Whitehead <matthew.whitehead@kaleido.io>
·
25.3.0
1 parent 72ac7bd commit 083b1d3

File tree

13 files changed

+117
-2
lines changed

13 files changed

+117
-2
lines changed
 

‎CHANGELOG.md

+1
Original file line numberDiff line numberDiff line change
@@ -48,6 +48,7 @@
4848
### Bug fixes
4949
- 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)
5050
- 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)
51+
- Fix issue with new QBFT/IBFT blocks being produced under certain circumstances. [#8308](https://github.com/hyperledger/besu/issues/8308)
5152

5253
## 25.2.2 hotfix
5354
- Pectra - Sepolia: Fix for deposit contract log decoding [#8383](https://github.com/hyperledger/besu/pull/8383)

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

+1
Original file line numberDiff line numberDiff line change
@@ -115,6 +115,7 @@ public void stop() {
115115
LOG.debug("Interrupted while waiting for BftProcessor to stop.", e);
116116
Thread.currentThread().interrupt();
117117
}
118+
eventHandler.stop();
118119
bftExecutors.stop();
119120
}
120121
}

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

+21
Original file line numberDiff line numberDiff line change
@@ -77,6 +77,20 @@ protected BaseBftController(
7777
public void start() {
7878
if (started.compareAndSet(false, true)) {
7979
startNewHeightManager(blockchain.getChainHeadHeader());
80+
} else {
81+
// In normal circumstances the height manager should only be started once. If the caller
82+
// has stopped the height manager (e.g. while sync completes) they must call stop() before
83+
// starting the height manager again.
84+
throw new IllegalStateException(
85+
"Attempt to start new height manager without stopping previous manager");
86+
}
87+
}
88+
89+
@Override
90+
public void stop() {
91+
if (started.compareAndSet(true, false)) {
92+
stopCurrentHeightManager(blockchain.getChainHeadHeader());
93+
LOG.debug("Height manager stopped");
8094
}
8195
}
8296

@@ -206,6 +220,13 @@ public void handleRoundExpiry(final RoundExpiry roundExpiry) {
206220
*/
207221
protected abstract BaseBlockHeightManager getCurrentHeightManager();
208222

223+
/**
224+
* Stop the current height manager by creating a no-op block height manager.
225+
*
226+
* @param parentHeader the parent header
227+
*/
228+
protected abstract void stopCurrentHeightManager(final BlockHeader parentHeader);
229+
209230
private void startNewHeightManager(final BlockHeader parentHeader) {
210231
createNewHeightManager(parentHeader);
211232
final long newChainHeight = getCurrentHeightManager().getChainHeight();

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

+3
Original file line numberDiff line numberDiff line change
@@ -25,6 +25,9 @@ public interface BftEventHandler {
2525
/** Start. */
2626
void start();
2727

28+
/** Stop. */
29+
void stop();
30+
2831
/**
2932
* Handle message event.
3033
*

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

+8-1
Original file line numberDiff line numberDiff line change
@@ -68,7 +68,14 @@ public BaseIbftBlockHeightManager create(final BlockHeader parentHeader) {
6868
}
6969
}
7070

71-
private BaseIbftBlockHeightManager createNoOpBlockHeightManager(final BlockHeader parentHeader) {
71+
/**
72+
* Create a no-op block height manager.
73+
*
74+
* @param parentHeader the parent header
75+
* @return the no-op height manager
76+
*/
77+
protected BaseIbftBlockHeightManager createNoOpBlockHeightManager(
78+
final BlockHeader parentHeader) {
7279
return new NoOpBlockHeightManager(parentHeader);
7380
}
7481

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

+5
Original file line numberDiff line numberDiff line change
@@ -117,4 +117,9 @@ protected void createNewHeightManager(final BlockHeader parentHeader) {
117117
protected BaseBlockHeightManager getCurrentHeightManager() {
118118
return currentHeightManager;
119119
}
120+
121+
@Override
122+
protected void stopCurrentHeightManager(final BlockHeader parentHeader) {
123+
currentHeightManager = ibftBlockHeightManagerFactory.createNoOpBlockHeightManager(parentHeader);
124+
}
120125
}

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

+19
Original file line numberDiff line numberDiff line change
@@ -14,6 +14,8 @@
1414
*/
1515
package org.hyperledger.besu.consensus.ibft.statemachine;
1616

17+
import static org.assertj.core.api.Assertions.assertThatNoException;
18+
import static org.assertj.core.api.Assertions.assertThatThrownBy;
1719
import static org.assertj.core.util.Lists.newArrayList;
1820
import static org.mockito.ArgumentMatchers.any;
1921
import static org.mockito.ArgumentMatchers.anyLong;
@@ -478,4 +480,21 @@ private void setupRoundChange(
478480
when(roundChangeMessageData.decode()).thenReturn(roundChange);
479481
roundChangeMessage = new DefaultMessage(null, roundChangeMessageData);
480482
}
483+
484+
@Test
485+
public void heightManagerCanOnlyBeStartedOnceIfNotStopped() {
486+
constructIbftController();
487+
ibftController.start();
488+
assertThatThrownBy(() -> ibftController.start())
489+
.isInstanceOf(IllegalStateException.class)
490+
.hasMessage("Attempt to start new height manager without stopping previous manager");
491+
}
492+
493+
@Test
494+
public void heightManagerCanBeRestartedIfStopped() {
495+
constructIbftController();
496+
ibftController.start();
497+
ibftController.stop();
498+
assertThatNoException().isThrownBy(() -> ibftController.start());
499+
}
481500
}

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

+7-1
Original file line numberDiff line numberDiff line change
@@ -85,7 +85,13 @@ public void isEarlyRoundChangeEnabled(final boolean isEarlyRoundChangeEnabled) {
8585
this.isEarlyRoundChangeEnabled = isEarlyRoundChangeEnabled;
8686
}
8787

88-
private BaseQbftBlockHeightManager createNoOpBlockHeightManager(
88+
/**
89+
* Creates a no-op height manager
90+
*
91+
* @param parentHeader the parent header
92+
* @return the no-op height manager
93+
*/
94+
protected BaseQbftBlockHeightManager createNoOpBlockHeightManager(
8995
final QbftBlockHeader parentHeader) {
9096
return new NoOpBlockHeightManager(parentHeader);
9197
}

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

+19
Original file line numberDiff line numberDiff line change
@@ -139,10 +139,29 @@ private BaseQbftBlockHeightManager getCurrentHeightManager() {
139139
return currentHeightManager;
140140
}
141141

142+
/* Replace the current height manager with a no-op height manager. */
143+
private void stopCurrentHeightManager(final QbftBlockHeader parentHeader) {
144+
currentHeightManager = qbftBlockHeightManagerFactory.createNoOpBlockHeightManager(parentHeader);
145+
}
146+
142147
@Override
143148
public void start() {
144149
if (started.compareAndSet(false, true)) {
145150
startNewHeightManager(blockchain.getChainHeadHeader());
151+
} else {
152+
// In normal circumstances the height manager should only be started once. If the caller
153+
// has stopped the height manager (e.g. while sync completes) they must call stop() before
154+
// starting the height manager again.
155+
throw new IllegalStateException(
156+
"Attempt to start new height manager without stopping previous manager");
157+
}
158+
}
159+
160+
@Override
161+
public void stop() {
162+
if (started.compareAndSet(true, false)) {
163+
stopCurrentHeightManager(blockchain.getChainHeadHeader());
164+
LOG.debug("QBFT height manager stop");
146165
}
147166
}
148167

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

+3
Original file line numberDiff line numberDiff line change
@@ -24,6 +24,9 @@ public interface QbftEventHandler {
2424
/** Start. */
2525
void start();
2626

27+
/** Stop. */
28+
void stop();
29+
2730
/**
2831
* Handle errorMessage event.
2932
*

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

+19
Original file line numberDiff line numberDiff line change
@@ -14,6 +14,8 @@
1414
*/
1515
package org.hyperledger.besu.consensus.qbft.core.statemachine;
1616

17+
import static org.assertj.core.api.Assertions.assertThatNoException;
18+
import static org.assertj.core.api.Assertions.assertThatThrownBy;
1719
import static org.assertj.core.util.Lists.newArrayList;
1820
import static org.mockito.ArgumentMatchers.any;
1921
import static org.mockito.ArgumentMatchers.anyLong;
@@ -484,4 +486,21 @@ private void setupRoundChange(
484486
when(roundChangeMessageData.decode(blockEncoder)).thenReturn(roundChange);
485487
roundChangeMessage = new DefaultMessage(null, roundChangeMessageData);
486488
}
489+
490+
@Test
491+
public void heightManagerCanOnlyBeStartedOnceIfNotStopped() {
492+
constructQbftController();
493+
qbftController.start();
494+
assertThatThrownBy(() -> qbftController.start())
495+
.isInstanceOf(IllegalStateException.class)
496+
.hasMessage("Attempt to start new height manager without stopping previous manager");
497+
}
498+
499+
@Test
500+
public void heightManagerCanBeRestartedIfStopped() {
501+
constructQbftController();
502+
qbftController.start();
503+
qbftController.stop();
504+
assertThatNoException().isThrownBy(() -> qbftController.start());
505+
}
487506
}

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

+5
Original file line numberDiff line numberDiff line change
@@ -41,6 +41,11 @@ public void start() {
4141
qbftEventHandler.start();
4242
}
4343

44+
@Override
45+
public void stop() {
46+
qbftEventHandler.stop();
47+
}
48+
4449
@Override
4550
public void handleMessageEvent(final BftReceivedMessageEvent msg) {
4651
qbftEventHandler.handleMessageEvent(msg);

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

+6
Original file line numberDiff line numberDiff line change
@@ -52,6 +52,12 @@ void startDelegatesToQbftEventHandler() {
5252
verify(qbftEventHandler).start();
5353
}
5454

55+
@Test
56+
void stopDelegatesToQbftEventHandler() {
57+
handler.stop();
58+
verify(qbftEventHandler).stop();
59+
}
60+
5561
@Test
5662
void handleMessageEventDelegatesToQbftEventHandler() {
5763
handler.handleMessageEvent(bftReceivedMessageEvent);

0 commit comments

Comments
 (0)
Please sign in to comment.