Add MMR-sync to consensus chain snap sync. #3206
Conversation
|
Cherry-picked #3208 for easier testing |
|
|
||
| starting_position += 1; | ||
|
|
||
| if u64::from(*position) >= target_position { |
There was a problem hiding this comment.
It took me a while to understand the logic here. I don’t think there’s an actual bug, but maybe the loop conditions could be rewritten to be clearer?
- we check
position != starting_positionand only get to this code if they’re equal, then add one tostarting_position, then compareposition >= target_position, which guaranteestarget_position <= starting_positionbelow (because that was true when they were equal, before we added one). Is there a simpler way to express this condition that makes this relationship clearer? Maybe a booleanreached_targetflag or something? - this condition is
position >= target_position, but the one below istarget_position <= starting_position. A consistent order would make it easier to see they’re the same condition.
Totally up to you, but it might also be clearer to split one or more of the inner loops into their own function.
There was a problem hiding this comment.
I'm not sure I understand your comments. There are several nested loops, and they have different conditions. I added a couple of code comments to clarify the conditions you mentioned.
|
There is definitely a bug here, it results in an attempt to query block number that was not yet imported during verification:
|
There was a problem hiding this comment.
The last commit fixes the bug. The bug was a branch from the early PoC that wasn't removed and related to the default target_block when we didn't provide the correct value. Domain sync always provided the correct target block and the incorrect code branch was never detected during the test.
|
|
||
| starting_position += 1; | ||
|
|
||
| if u64::from(*position) >= target_position { |
There was a problem hiding this comment.
I'm not sure I understand your comments. There are several nested loops, and they have different conditions. I added a couple of code comments to clarify the conditions you mentioned.
| if let Some(offchain_storage) = offchain_storage { | ||
| mmr_sync( | ||
| fork_id.map(|v| v.into()), | ||
| client.clone(), | ||
| network_request, | ||
| sync_service.clone(), | ||
| offchain_storage, | ||
| mmr_target_block, | ||
| ) | ||
| .await?; | ||
| } |
There was a problem hiding this comment.
This is not the right level to check this at. It should be checked at higher level outside of Snap sync or else it is possible for Snap sync to start, being interrupted in the middle and MMR sync to never actually happen.
There was a problem hiding this comment.
If I understood your comment correctly, "your_claim == true" even I move this check elsewhere because we don't have transactional operations in this context.
nazar-pc
force-pushed
the
mainnet-release-test-2-mmr-sync
branch
from
9059d07
to
f15e6d5
Compare
|
GitHub didn't want to trigger CI, so I just merged it directly |
This PR adds MMR-sync as part of the consensus snap sync. It will simplify future domain snap sync because the consensus chain network will contain proper local MMR data storage.
Code contributor checklist: