feat(resharding): Full forks support in flat storage #12727
Conversation
Codecov ReportAttention: Patch coverage is
Additional details and impacted files@@ Coverage Diff @@
## master #12727 +/- ##
==========================================
+ Coverage 70.64% 70.73% +0.09%
==========================================
Files 848 848
Lines 173878 174207 +329
Branches 173878 174207 +329
==========================================
+ Hits 122836 123229 +393
+ Misses 45903 45835 -68
- Partials 5139 5143 +4
Flags with carried forward coverage won't be shown. Click here to find out more. ☔ View full report in Codecov by Sentry. |
| @@ -181,6 +181,22 @@ impl FlatStorageResharder { | |||
| info!(target: "resharding", ?split_params, "initiating flat storage shard split"); | |||
There was a problem hiding this comment.
Can you add a comment to the method about the fork handling?
github doesn't let me comment there but I mean split_shard a few lines above here.
| if parent_shard == parent | ||
| && params.left_child_shard == left_child_shard | ||
| && params.right_child_shard == right_child_shard | ||
| && params.shard_layout == *shard_layout |
There was a problem hiding this comment.
Why do you need those checks?
There was a problem hiding this comment.
These are not strictly necessary. The checks are a safeguard to discard the old candidate resharding block if the shard layout of the new resharding is different. It can't really happen today since shard layout is fixed.
| let (resharding_blocks, parent_shard, split_params, execution_status) = if let Some(event) = | ||
| event_lock_guard.as_mut() |
There was a problem hiding this comment.
nit: Would the following work?
let Some(event) = event_lock_guard.as_mut() else {
info!(target: "resharding", "flat storage shard split task cancelled: no resharding event");
return SplitShardSchedulingStatus::Cancelled;
}
...
| if event.has_started() { | ||
| info!(target: "resharding", "flat storage shard split task cancelled: resharding already in progress"); | ||
| return SplitShardSchedulingStatus::Cancelled; | ||
| } |
There was a problem hiding this comment.
Is this expected to happen under normal circumstances, under fork or under error?
There was a problem hiding this comment.
Only if there are forks involved
| for block_info in resharding_blocks { | ||
| match self.compute_scheduled_task_status_at_block(&block_info, chain_store) { | ||
| CanStart(block_info) => return CanStart(block_info), | ||
| Failed => {} // Nothing to do on failure. |
There was a problem hiding this comment.
Can you mention that this may happen during forks and isn't actually "the end of the world"?
| // TODO(resharding): duplicate this test so that in one case resharding is performed on block | ||
| // B(height=13) and in another case resharding is performed on block B'(height=13). | ||
| // In the current scenario the real resharding happens on block B'. Low priority TODO | ||
| // since it's a very rare corner case. |
There was a problem hiding this comment.
Yes this must be done still. split_shard_in_forks_when_older_branch_is_finalized is doing that but only for flat storage unit testing; it would be nice to have the test loop version with all the components.
Is the first option worth considering as a better engineering or good first issue project for later? |
It's an alternative with different tradeoffs. It makes easier handling the state, since instead of block hashes we just need one epoch_id. On the other hand, it makes resharder less decoupled because of the dependency on epochs. TL;DR Not a straight, simple improvement IMO |
| @@ -709,8 +709,6 @@ fn test_resharding_v3_drop_chunks_all() { | |||
| } | |||
|
|
|||
| #[test] | |||
| // TODO(resharding): fix nearcore and un-ignore this test | |||
| #[ignore] | |||
This PR fixes the remaining issue in
FlatStorageResharderthat prevented the chain from progressing at resharding boundary in some corner cases.Two possible solutions have been considered:
I've decided to proceed with the second option, as it was building on existing code, and the changes were limited in scope to the inner impl of
FlatStorageResharder. In this way the resharder remains entirely decoupled from epoch manager.Also in this PR:
Now the existing tests for forks in
resharding_v3have been finallyunignored💪