Fix/snap sync edge cases #7708
Fix/snap sync edge cases #7708
Conversation
LukaszRozmej
requested review from
LukaszRozmej,
benaadams and
damian-orzechowski
| if (accountRangePartitionCount < 1 || accountRangePartitionCount > int.MaxValue) | ||
| throw new ArgumentException($"Account range partition must be between 1 to {int.MaxValue}."); |
There was a problem hiding this comment.
this if now doesn't make sense, basically you need to check if accountRangePartitionCount > 0 (but maybe you can assume then it is 1? Or maybe change accountRangePartitionCount to be uint?
There was a problem hiding this comment.
True, its useless now.
| Hash256 startingPath = new Hash256(Keccak.Zero.Bytes); | ||
| startingPath.Bytes[0] = curStartingPath; | ||
| BinaryPrimitives.WriteUInt32BigEndian(startingPath.Bytes, curStartingPath); |
There was a problem hiding this comment.
why not write directly to partition.NextAccountPath and not create Hash256 at all?
| @@ -112,7 +110,7 @@ private void SetupAccountRangePartition() | |||
| else | |||
| { | |||
| limitPath = new Hash256(Keccak.Zero.Bytes); | |||
| limitPath.Bytes[0] = curStartingPath; | |||
| BinaryPrimitives.WriteUInt32BigEndian(limitPath.Bytes, curStartingPath); | |||
| // The server will always give one node extra after limitpath if it can fit in the response. | ||
| // When we have extra storage, the extra storage must not be re-stored as it may have already been set | ||
| // by another top level partition. If the sync pivot moved and the storage was modified, it must not be saved | ||
| // here along with updated ancestor so that healing can detect that the storage need to be healed. | ||
| // | ||
| // Unfortunately, without introducing large change to the tree, the easiest way to | ||
| // exclude the extra storage is to just rebuild the whole tree and also skip stitching. | ||
| // Fortunately, this should only happen n-1 time where n is the number of top level | ||
| // partition count. | ||
|
|
||
| tree.RootHash = Keccak.EmptyTreeHash; | ||
| for (var index = 0; index < accounts.Count; index++) | ||
| { | ||
| PathWithAccount account = accounts[index]; | ||
| if (account.Path >= limitHash || account.Path < startingHash) continue; | ||
| _ = tree.Set(account.Path, account.Account); | ||
| } |
There was a problem hiding this comment.
Can we just detect and ignore that one extra node, instead of rebuilding the whole tree? Otherwise partitioning storage tree's is useless?
There was a problem hiding this comment.
Its not just the node, but the parents that touches the node also. Partitionning is not useless, this only happens 16 time on mainnet.
There was a problem hiding this comment.
Probably a super minor optimisation, but could check hasExtraStorage in the 1st loop and not make changes to the tree, if we will still will rebuild it.
There was a problem hiding this comment.
Also - if we skip the extra node, why then skip stitching as well?
There was a problem hiding this comment.
The first loop is needed to verify the hash. Not sure aboutt stitching...
| proofNodesToProcess.Push((child, childPath)); | ||
| sortedBoundaryList.Add((child, childPath)); |
There was a problem hiding this comment.
out of scope of this PR: but I always was curious if we can use some memory pooling on snap syncing
TreePathinstead of array.Types of changes
What types of changes does your code introduce?
Testing
Requires testing
If yes, did you write tests?
Notes on testing