3943 stop blocks on finalized #4058
Conversation
besu/src/main/java/org/hyperledger/besu/controller/TransitionBesuControllerBuilder.java
Outdated
Show resolved
Hide resolved
consensus/merge/src/main/java/org/hyperledger/besu/consensus/merge/PostMergeContext.java
Outdated
Show resolved
Hide resolved
...eum/eth/src/main/java/org/hyperledger/besu/consensus/merge/NewForkchoiceMessageListener.java
Outdated
Show resolved
Hide resolved
jflo
force-pushed
the
3943-stop-blocks-on-finalized
branch
from
6604d0b
to
f653815
Compare
![github-advanced-security[bot]](https://avatars.githubusercontent.com/in/57789?s=60&v=4){kind=small}
| public interface ForkchoiceMessageListener { | ||
|
|
||
| void onNewForkchoiceMessage( | ||
| final Hash headBlockHash, |
Check notice
Code scanning / CodeQL
Useless parameter
| void onNewForkchoiceMessage( | ||
| final Hash headBlockHash, | ||
| final Optional<Hash> maybeFinalizedBlockHash, | ||
| final Hash safeBlockHash); |
Check notice
Code scanning / CodeQL
Useless parameter
| private Optional<Difficulty> powTerminalDifficulty; | ||
| private final StampedLock powTerminalDifficultyLock = new StampedLock(); | ||
| private Hash lastFinalized = Hash.ZERO; | ||
| private final AtomicLong numFinalizedSeen = new AtomicLong(0); |
There was a problem hiding this comment.
I think we are leaking too much of the merge implementation details into protocol manager.
How about either:
- extracting and encapsulate the merge specific behavior into an Optional "PeerFilterAction"
- or subclassing a merge-specific EthProtocolManager
There was a problem hiding this comment.
Went with option 1, added an Optional peer filter.
| ConsensusContext cc = protocolContext.getConsensusContext(ConsensusContext.class); | ||
| if (cc instanceof MergeContext) { | ||
| protocolContext | ||
| .getConsensusContext(MergeContext.class) | ||
| .observeNewIsPostMergeState(ethProtocolManager); | ||
| protocolContext | ||
| .getConsensusContext(MergeContext.class) | ||
| .addNewForkchoiceMessageListener(ethProtocolManager); | ||
| } | ||
|
|
||
| return ethProtocolManager; | ||
| } |
There was a problem hiding this comment.
if we passed in an Optional or however we encapsulate the merge-specific logic, we wouldn't need to do this kind of introspection on class type.
Signed-off-by: Justin Florentine <[email protected]>
…ks, or connecting with td > ttd Signed-off-by: Justin Florentine <[email protected]>
Signed-off-by: Justin Florentine <[email protected]>
Signed-off-by: Justin Florentine <[email protected]>
Signed-off-by: Justin Florentine <[email protected]>
Signed-off-by: Justin Florentine <[email protected]>
Signed-off-by: Justin Florentine <[email protected]>
jflo
force-pushed
the
3943-stop-blocks-on-finalized
branch
from
d5bcb32
to
caa7e6d
Compare
| @@ -409,4 +439,35 @@ public List<Bytes> getForkIdAsBytesList() { | |||
| ? Collections.emptyList() | |||
| : chainHeadForkId.getForkIdAsBytesList(); | |||
| } | |||
|
|
|||
| private boolean isFinalized() { | |||
| return this.numFinalizedSeen.get() > 1; | |||
There was a problem hiding this comment.
why does it have to be more than one?
There was a problem hiding this comment.
Specs say not to disconnect till the 2nd finalized epoch.
Signed-off-by: Justin Florentine <[email protected]>
| } | ||
|
|
||
| private boolean isFinalized() { | ||
| return this.numFinalizedSeen.get() > 1; |
There was a problem hiding this comment.
why > 1 rather than >= 1 ? just to allow a bit more time before disconnects?
There was a problem hiding this comment.
Spec describes it as waiting till the 2nd finalized block.
* adds interfaces for tracking merge state and forchoices * after 2 finalizations from fcu, disconnect any peers sending new blocks, or connecting with td > ttd * tests for preventing pow peers from joining * refactored to separate out merge logic Signed-off-by: Justin Florentine <[email protected]>
fixed #3943