Create Qbft Statemachine #1761
Merged
Create Qbft Statemachine #1761
Conversation
This file contains bidirectional Unicode text that may be interpreted or compiled differently than what appears below. To review, open the file in an editor that reveals hidden Unicode characters.
Learn more about bidirectional Unicode characters
rain-on
force-pushed
the
bft_controller
branch
2 times, most recently
from
863e456 to
8f8fbdb
Compare
added 2 commits
January 8, 2021 11:18
Signed-off-by: Trent Mohay <trent.mohay@consensys.net>
Signed-off-by: Trent Mohay <trent.mohay@consensys.net>
rain-on
force-pushed
the
bft_controller
branch
from
8f8fbdb to
de36de8
Compare
Signed-off-by: Trent Mohay <trent.mohay@consensys.net>
usmansaleem
reviewed
Jan 8, 2021
|
|
||
| protected <P extends BftMessage<?>> void consumeMessage( | ||
| final Message message, final P ibftMessage, final Consumer<P> handleMessage) { | ||
| LOG.trace("Received IBFT {} message", ibftMessage.getClass().getSimpleName()); |
There was a problem hiding this comment.
nit: trace log refers to IBFT. As well as method arg name is ibftMessage.
Signed-off-by: Trent Mohay <trent.mohay@consensys.net>
usmansaleem
reviewed
Jan 8, 2021
| import org.hyperledger.besu.ethereum.mainnet.ProtocolSchedule; | ||
| import org.hyperledger.besu.util.Subscribers; | ||
|
|
||
| public class IbftRoundFactory { |
There was a problem hiding this comment.
Shouldn't this class rename to QbftRoundFactory as it is in qbft package?
usmansaleem
reviewed
Jan 8, 2021
| @Override | ||
| public void handleBlockTimerExpiry(final ConsensusRoundIdentifier roundIdentifier) { | ||
| if (roundIdentifier.equals(currentRound.getRoundIdentifier())) { | ||
| currentRound.createAndSendProposalMessage(clock.millis() / 1000); |
There was a problem hiding this comment.
nit: divide by 1000L as millis returns a long value.
There was a problem hiding this comment.
line 236 is more readable way to convert to seconds I think.
usmansaleem
reviewed
Jan 8, 2021
| } | ||
|
|
||
| private <P extends Payload, M extends BftMessage<P>> void actionOrBufferMessage( | ||
| final M ibftMessage, |
jframe
reviewed
Jan 8, 2021
|
|
||
| protected <P extends BftMessage<?>> void consumeMessage( | ||
| final Message message, final P ibftMessage, final Consumer<P> handleMessage) { | ||
| LOG.trace("Received IBFT {} message", ibftMessage.getClass().getSimpleName()); |
| protected abstract void handleMessage(final Message message); | ||
|
|
||
| protected <P extends BftMessage<?>> void consumeMessage( | ||
| final Message message, final P ibftMessage, final Consumer<P> handleMessage) { |
| msgRoundIdentifier.getSequenceNumber() - 1L, rawMsg.getConnection()); | ||
| } else { | ||
| LOG.trace( | ||
| "IBFT message discarded as it is from a previous block height messageType={} chainHeight={} eventHeight={}", |
| import org.apache.logging.log4j.LogManager; | ||
| import org.apache.logging.log4j.Logger; | ||
|
|
||
| public class QbftMessageTransmitter { |
There was a problem hiding this comment.
Can you add a test for this. There is one for IbftMessageTransmitter that can be copied and tweaked.
...t/src/main/java/org/hyperledger/besu/consensus/qbft/statemachine/QbftBlockHeightManager.java
Show resolved
Hide resolved
| final BlockHeader parentHeader, | ||
| final BftFinalState finalState, | ||
| final RoundChangeManager roundChangeManager, | ||
| final IbftRoundFactory ibftRoundFactory, |
There was a problem hiding this comment.
Can the IbftRoundFactory to be moved to common and become a BftRoundFactory?
There was a problem hiding this comment.
no, because it needs the messagefactory and messagevalidators which are both specific to the implementation :(
| } | ||
|
|
||
| private <P extends Payload, M extends BftMessage<P>> void actionOrBufferMessage( | ||
| final M ibftMessage, |
| import org.hyperledger.besu.ethereum.mainnet.ProtocolSchedule; | ||
| import org.hyperledger.besu.util.Subscribers; | ||
|
|
||
| public class IbftRoundFactory { |
| default: | ||
| throw new IllegalArgumentException( | ||
| String.format( | ||
| "Received message with messageCode=%d does not conform to any recognised IBFT message structure", |
There was a problem hiding this comment.
change pare of log message from "IBFT message" to "QBFT message"
| final RoundChangeMetadata roundChangeMetadata, final long headerTimestamp) { | ||
| final Optional<Block> bestBlockFromRoundChange = roundChangeMetadata.getBlock(); | ||
| Block blockToPublish; | ||
| if (!bestBlockFromRoundChange.isPresent()) { |
There was a problem hiding this comment.
can simplify to bestBlockFromRoundChange.isEmpty()
Signed-off-by: Trent Mohay <trent.mohay@consensys.net>
rain-on
force-pushed
the
bft_controller
branch
from
0396ffc to
140348d
Compare
added 2 commits
January 8, 2021 14:48
Signed-off-by: Trent Mohay <trent.mohay@consensys.net>
Signed-off-by: Trent Mohay <trent.mohay@consensys.net>
jframe
approved these changes
Jan 8, 2021
| private final MessageFactory messageFactory = new MessageFactory(nodeKey); | ||
|
|
||
| private Block proposedBlock; | ||
| private BftExtraData proposedExtraData; |
There was a problem hiding this comment.
nit: this could be a local variable
added 2 commits
January 8, 2021 16:33
Signed-off-by: Trent Mohay <trent.mohay@consensys.net>
eum602
pushed a commit
to lacchain/besu
that referenced
this pull request
Nov 3, 2023
Qbft Statemachine classes copied over from ibft, and specialised. Signed-off-by: Trent Mohay <trent.mohay@consensys.net>
Sign up for free
to join this conversation on GitHub.
Already have an account?
Sign in to comment
Add this suggestion to a batch that can be applied as a single commit.
This suggestion is invalid because no changes were made to the code.
Suggestions cannot be applied while the pull request is closed.
Suggestions cannot be applied while viewing a subset of changes.
Only one suggestion per line can be applied in a batch.
Add this suggestion to a batch that can be applied as a single commit.
Applying suggestions on deleted lines is not supported.
You must change the existing code in this line in order to create a valid suggestion.
Outdated suggestions cannot be applied.
This suggestion has been applied or marked resolved.
Suggestions cannot be applied from pending reviews.
Suggestions cannot be applied on multi-line comments.
Suggestions cannot be applied while the pull request is queued to merge.
Suggestion cannot be applied right now. Please check back later.
Extracts commonality from IBFT classes into consensus/common, then copies IBFT statemachine classes into the Qbft package, an realigns messages to use qbft (rather than ibft) messages.