Fix MergeBlockDownloader keep repeating last block #4390
Merged
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
# Conflicts: # src/Nethermind/Nethermind.Merge.Plugin.Test/EngineModuleTests.Setup.cs # src/Nethermind/Nethermind.Merge.Plugin.Test/Synchronization/BeaconHeadersSyncTests.cs # src/Nethermind/Nethermind.Merge.Plugin/Handlers/V1/NewPayloadV1Handler.cs # src/Nethermind/Nethermind.Merge.Plugin/Synchronization/BeaconHeadersSyncFeed.cs
# Conflicts: # src/Nethermind/Nethermind.Merge.Plugin/Synchronization/ChainLevelHelper.cs
# Conflicts: # src/Nethermind/Nethermind.JsonRpc/JsonRpcUrl.cs # src/Nethermind/Nethermind.JsonRpc/JsonRpcUrlCollection.cs # src/Nethermind/Nethermind.Merge.Plugin/Synchronization/ChainLevelHelper.cs # src/Nethermind/Nethermind.Runner/configs/catalyst.cfg # src/Nethermind/Nethermind.Runner/configs/goerli_shadowfork.cfg # src/Nethermind/Nethermind.Runner/configs/mainnet_shadowfork.cfg
MarekM25
reviewed
Aug 9, 2022
| if (_logger.IsDebug) | ||
| _logger.Debug($"Parent of block not available. Starting new beacon header. sync."); | ||
|
|
||
| StartNewBeaconHeaderSync(forkchoiceState, newHeadBlock!, requestStr); |
There was a problem hiding this comment.
I'm guessing this could trigger it. https://github.com/NethermindEth/nethermind/blob/master/src/Nethermind/Nethermind.Merge.Plugin/Handlers/V1/NewPayloadV1Handler.cs#L130
MarekM25
reviewed
Aug 9, 2022
| BlockHeader? blockParent = _blockTree.FindHeader(newHeadBlock.ParentHash!); | ||
| if (blockParent == null) | ||
| { | ||
| if (_logger.IsDebug) |
There was a problem hiding this comment.
maybe Logger.IsInfo to be able to distinguish situations for now
MarekM25
reviewed
Aug 9, 2022
| @@ -169,10 +169,10 @@ public async Task<ResultWrapper<PayloadStatusV1>> HandleAsync(ExecutionPayloadV1 | |||
|
|
|||
| BlockTreeInsertOptions insertOptions = BlockTreeInsertOptions.BeaconBlockInsert; | |||
|
|
|||
| if (_blockCacheService.ProcessDestination != null && _blockCacheService.ProcessDestination.Hash == block.ParentHash) | |||
| if (_beaconPivot.ProcessDestination != null && _beaconPivot.ProcessDestination.Hash == block.ParentHash) | |||
There was a problem hiding this comment.
What if we don't have BeaconPivot here? It could be in two cases:
- We're doing sync from PoW chain and we have parent (no need to start beacon header sync), but we haven't processed blocks yet.
- We have a timeout in newPayload(x) and we have next newPayload(x+1) in this case we don't have beacon pivot at all.
So I think if we shouldn't EnsurePivot in this if. WDYT?
There was a problem hiding this comment.
Ah great... well I'm gonna put that as out of scope.
There was a problem hiding this comment.
but it could introduce regression in such cases
There was a problem hiding this comment.
Why? This is a rename.
MarekM25
reviewed
Aug 9, 2022
| @@ -91,6 +93,10 @@ public void EnsurePivot(BlockHeader? blockHeader) | |||
| return; | |||
| } | |||
|
|
|||
| // BeaconHeaderSync actually starts from the parent of the pivot. So we need to to manually insert | |||
| // the pivot itself here. | |||
| _blockTree.Insert(blockHeader, | |||
There was a problem hiding this comment.
I thought that we're adding it in TryInsertDanglingBlock. Aren't we?
There was a problem hiding this comment.
Not if beacon header sync is not started yet.
MarekM25
reviewed
Aug 9, 2022
| return ForkchoiceUpdatedV1Result.Syncing; | ||
| } | ||
|
|
||
| BlockInfo blockInfo = _blockTree.GetInfo(newHeadBlock.Number, newHeadBlock.GetOrCalculateHash()).Info; | ||
| if (!blockInfo.WasProcessed) | ||
| { | ||
| BlockHeader? blockParent = _blockTree.FindHeader(newHeadBlock.ParentHash!); |
There was a problem hiding this comment.
do we have any unit tests for this if?
# Conflicts: # src/Nethermind/Nethermind.Merge.Plugin.Test/EngineModuleTests.Synchronization.cs # src/Nethermind/Nethermind.Merge.Plugin/Handlers/V1/NewPayloadV1Handler.cs
…oader-keep-suggesting-block
…oader-keep-suggesting-block
MarekM25
approved these changes
Aug 11, 2022
Merged
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.
Changes:
Types of changes
What types of changes does your code introduce?
Put an
xin the boxes that applyTesting
Requires testing
In case you checked yes, did you write tests??
Comments about testing , should you have some (optional)
Incremental Synchive test passes on previous test version if test is modified to not check synched via head.Sync Client Post Mergehive test passed.Only 4 test from
engine-transitionhive test failed, which is a known regression on master.Still state-syncing ropsten.
Further comments (optional)
If this is a relatively large or complex change, kick off the discussion by explaining why you chose the solution you did and what alternatives you considered, etc...