-
Notifications
You must be signed in to change notification settings - Fork 425
New issue
Have a question about this project? Sign up for a free GitHub account to open an issue and contact its maintainers and the community.
By clicking “Sign up for GitHub”, you agree to our terms of service and privacy statement. We’ll occasionally send you account related emails.
Already on GitHub? Sign in to your account
Gsf5 fixes #4312
Gsf5 fixes #4312
Conversation
|
||
// edge-case detected on GSF5 - during the transition we want to try process all transition blocks from CL client | ||
// The last condition: !parentBlockInfo.IsBeaconBody will be true for terminal blocks. Checking _posSwitcher.IsTerminal might not be the best, because we're loading parentHeader with DoNotCalculateTotalDifficulty option | ||
bool forceProcessing = !_poSSwitcher.TransitionFinished && (_blockTree.Head?.Number ?? 0) + 8 >= block.Number && !parentBlockInfo.IsBeaconBody; |
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
8?
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
We're using 8 here too: https://github.com/NethermindEth/nethermind/blob/master/src/Nethermind/Nethermind.Facade/Eth/EthSyncingInfo.cs#L37
I should change this condition bool weAreCloseToHead = (_blockTree.Head?.Number ?? 0) + 8 >= block.Number;
public bool ValidateSeal(BlockHeader header, bool force) => | ||
_poSSwitcher.IsPostMerge(header) || _preMergeSealValidator.ValidateSeal(header, force); | ||
public bool ValidateSeal(BlockHeader header, bool force) => | ||
_poSSwitcher.GetBlockConsensusInfo(header, true).IsPostMerge || _preMergeSealValidator.ValidateSeal(header, force); |
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
qq: why we don't check the seal if it is post merge?
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
It is PoW specific thing, so we don't have it post merge
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
Isnt seal mean the hash?
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
Isnt seal mean the hash?
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
@@ -180,7 +180,8 @@ public void AddNewBlock(Block block, ISyncPeer nodeWhoSentTheBlock) | |||
|
|||
bool isBlockBeforeTheSyncPivot = block.Number < _pivotNumber; | |||
bool isBlockOlderThanMaxReorgAllows = block.Number < (_blockTree.Head?.Number ?? 0) - Sync.MaxReorgLength; | |||
bool isBlockTotalDifficultyLow = block.TotalDifficulty < _blockTree.BestSuggestedHeader.TotalDifficulty; | |||
bool isBlockTotalDifficultyLow = block.TotalDifficulty < _blockTree.BestSuggestedHeader.TotalDifficulty | |||
&& (_specProvider.TerminalTotalDifficulty == null || block.TotalDifficulty < _specProvider.TerminalTotalDifficulty); // terminal blocks with lower TTD might be useful for smooth merge transition |
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
What if there are two terminal block and the lower one appear later? I assume it must go through NP?
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
If terminal block appears later that shouldn't be block choosen by CL clients to build chain on top of it. However, in this case we will get NP(childOfTerminalBlock) we will return SYNCING and we should be able to catch up with MergeBlockDownloader
The workflow is not starting the tests, but I run them locally. |
Fix issues that we noticed during transition on GSF5