Old receipts sync fix + beacon main chain marking fix #4370
Conversation
# 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
| @@ -352,8 +352,8 @@ private BlockInfo[] GetBeaconChainBranch(Block newHeadBlock, BlockInfo newHeadBl | |||
| } | |||
| BlockInfo predecessorInfo = _blockTree.GetInfo(predecessor.Number, predecessor.GetOrCalculateHash()).Info; | |||
| predecessorInfo.BlockNumber = predecessor.Number; | |||
| if (predecessorInfo.IsBeaconMainChain) break; | |||
| if (_logger.IsInfo) _logger.Info($"Reorged to beacon block ({predecessorInfo.BlockNumber}) {predecessorInfo.BlockHash} or cache rebuilt after restart"); | |||
| if (predecessorInfo.IsBeaconMainChain || !predecessorInfo.IsBeaconInfo) break; | |||
There was a problem hiding this comment.
Should we also have some depth limit?
Should we also check if something is just MainChain?
There was a problem hiding this comment.
in theory depth limit might be useful only in case of our bug, but I'm wondering what we can set here. Do you think we should break after, let's say 2000 blocks?
If something is on main chain we should break here: !predecessorInfo.IsBeaconInfo
| @@ -33,7 +33,7 @@ public enum BlockTreeLookupOptions | |||
| } | |||
|
|
|||
| [Flags] | |||
| public enum BlockTreeInsertOptions | |||
| public enum BlockTreeInsertHeaderOptions | |||
There was a problem hiding this comment.
All those options should be documented. It's getting extremely confusing what does what.
| @@ -46,6 +46,14 @@ public enum BlockTreeInsertOptions | |||
| BeaconHeaderInsert = BeaconHeaderMetadata | MoveToBeaconMainChain | NotOnMainChain | |||
| } | |||
|
|
|||
| [Flags] | |||
There was a problem hiding this comment.
All those options should be documented.
|
Another, potentially better idea is doing something similar to what is done in |
That is good idea. Maybe not for old receipts, but we can use it when we're inserting newPayloads |
Two fixes:
and later in old bodies sync: https://github.com/NethermindEth/nethermind/blob/master/src/Nethermind/Nethermind.Blockchain/BlockTree.cs#L542
so we won't insert the block and later we will fail with old receipts. In theory, it could happen in pre-merge client too