Skip to content

Ensure block height manager is restarted when BFT coordinator is#8308

Merged
matthew1001 merged 13 commits intohyperledger:mainfrom
matthew1001:bft-round-timer
Mar 12, 2025
Merged

Ensure block height manager is restarted when BFT coordinator is#8308
matthew1001 merged 13 commits intohyperledger:mainfrom
matthew1001:bft-round-timer

Conversation

@matthew1001
Copy link
Contributor

@matthew1001 matthew1001 commented Feb 14, 2025

PR description

Some changes were made to the BFT mining coordinator to ensure it could be stopped and started (see #5861). A bug in those changes meant that when it was restarted, a new block height manager wasn't created.

This PR ensures that when BFT mining is restarted, so is the block height manager.

Fixed Issue(s)

Fixes #8307

Signed-off-by: Matthew Whitehead <matthew1001@gmail.com>
matthew1001 and others added 6 commits February 19, 2025 11:09
…ithout calling reset

Signed-off-by: Matthew Whitehead <matthew1001@gmail.com>
Signed-off-by: Matthew Whitehead <matthew1001@gmail.com>
Signed-off-by: Matthew Whitehead <matthew1001@gmail.com>
Signed-off-by: Matthew Whitehead <matthew1001@gmail.com>
Signed-off-by: Matthew Whitehead <matthew1001@gmail.com>
@matthew1001 matthew1001 marked this pull request as ready for review February 19, 2025 14:56
@jflo jflo requested a review from jframe February 20, 2025 21:32
LOG.debug("Interrupted while waiting for BftProcessor to stop.", e);
Thread.currentThread().interrupt();
}
eventHandler.reset();
Copy link
Contributor

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

Reset seems odd here in that we trying to stop all processing of BFT here. A stop method to prevent any more processing in the controller would make sense. With everything stopped, we won't be processing BFT events anyway so maybe it's not worth it?

And by a stop method on the controller I'm thinking it would change the started state to false as it does right now in the reset method and also replace the block height manager with the noop version so it wouldn't do any processing of events.

Copy link
Contributor Author

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

Yeah I originally called it stop(), but as it wasn't doing anything other than reset the state var I wasn't sure it was quite right. But I'm fine with using that term, and I'll add in replacing the block height manager at the same time as you've suggested.

matthew1001 and others added 4 commits February 26, 2025 11:53
Signed-off-by: Matthew Whitehead <matthew.whitehead@kaleido.io>
…pped

Signed-off-by: Matthew Whitehead <matthew1001@gmail.com>
Signed-off-by: Matt Whitehead <matthew.whitehead@kaleido.io>
Signed-off-by: Matthew Whitehead <matthew1001@gmail.com>
@matthew1001
Copy link
Contributor Author

@jframe I think it's ready for another look now, thanks

Copy link
Contributor

@jframe jframe left a comment

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

LGTM

Signed-off-by: Matt Whitehead <matthew.whitehead@kaleido.io>
@matthew1001 matthew1001 merged commit 083b1d3 into hyperledger:main Mar 12, 2025
43 checks passed
marcosio pushed a commit to IoBuilders/besu that referenced this pull request Mar 12, 2025
…erledger#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>
Signed-off-by: Marcos Serradilla Diez <marcos@io.builders>
matthew1001 added a commit to kaleido-io/besu that referenced this pull request Mar 25, 2025
…erledger#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>
Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment

Labels

None yet

Projects

None yet

Development

Successfully merging this pull request may close these issues.

Restarted BFT validators can fail to agree on new blocks under certain conditions

2 participants