feat(resharding): flat storage resharding deltas #12271
Conversation
Codecov ReportAttention: Patch coverage is
Additional details and impacted files@@ Coverage Diff @@
## master #12271 +/- ##
==========================================
+ Coverage 71.58% 71.61% +0.02%
==========================================
Files 836 836
Lines 167470 167697 +227
Branches 167470 167697 +227
==========================================
+ Hits 119891 120096 +205
- Misses 42347 42366 +19
- Partials 5232 5235 +3
Flags with carried forward coverage won't be shown. Click here to find out more. ☔ View full report in Codecov by Sentry. |
| ); | ||
|
|
||
| // Get all the blocks from flat head to the wanted block hash. | ||
| let flat_storage = self |
There was a problem hiding this comment.
I'm a bit confused about having flat_store as an argument and getting flat_storage from runtime here. Why do you need both and how do they differ? Is there any way to make it simpler? Ideally there would only be one flat thing or they would be clearly different and both would be obtained same way.
There was a problem hiding this comment.
Yes, it's a bit weird. I need flat_store passed to the function due to lifetimes on the iterator - this prevents me from instantiating a flat_store inside the flat_storage_iterator.
And then flat_storage and flat_store, although they have similar name, are totally independent. I couldn't get one from the other
| continue; | ||
| }; | ||
| // Chain the iterators effectively adding a block worth of deltas. | ||
| // Before doing so insert a commit point to separate changes to the same key in different transactions. |
There was a problem hiding this comment.
Nice, I wouldn't have thought of that. How did you find that it's needed?
There was a problem hiding this comment.
Tests failed with transaction overwrite error 😉
| store_update.set(status.left_child_shard, key.clone(), value.clone()); | ||
| store_update.set(status.right_child_shard, key, value); |
There was a problem hiding this comment.
not related to this PR
It feels like the set method should only need a reference to the key and value. In theory it should serialize those and for that ref should be sufficient. Then the clone would not be needed.
| // Buffered receipt '1' shouldn't exist. | ||
| for child in [left_child_shard, right_child_shard] { | ||
| assert_eq!(flat_store.get(child, &buffered_receipt_1_key.to_vec()), Ok(None)); | ||
| } |
There was a problem hiding this comment.
Ultimately we should check that the state root matches that on chain (which was calculated from memtrie resharding). I'm not sure how easy would it be to do in test though. We should definitely do it when replacing the temporary memtrie with new one though.
There was a problem hiding this comment.
Yes this check should be done at least as part of memtrie replacement test
| /// necessary because otherwise deltas might set again the value of a flat storage entry inside the | ||
| /// same transaction. | ||
| enum FlatStorageAndDeltaIterItem { | ||
| Entry(Result<(Vec<u8>, Option<FlatStateValue>), FlatStorageError>), |
There was a problem hiding this comment.
nit: Is there any chance to remove the error from here? Would it be nicer?
There was a problem hiding this comment.
The only way I think would be to map the error into a panic, but that's.. brutal
|
Just out of curiosity how much of it is copy pasted from resharding V2 (RIP)? |
The idea is the same (chaining iterators). In practice though I had to code the solution differently, because resharding-v2 works on trie changes and I'm working directly on flat storage key value pairs. |
Implementation of flat storage deltas split for flat storage resharding.
Before this change, only flat storage key pairs at flat head height were split during resharding. Now flat storage deltas are part of the splitting process as well.
On a high level, it works by concatenating the iterator over flat storage and the iterator over deltas for all block heights between chain head an flat storage head.
Part of #12174