fix(forknet): fix account shard assignment for forknet v2 #12837
Conversation
When iterating over a whole mainnet trie, this might actually be pretty unnecessary
We already have the key we want to remove, so constructing the same trie key and then serializing it is not necessary. Also, this was incorrect for delayed receipts, where the indices in the source DB probably did not start from zero, so we were deleting the wrong trie key
|
this PR is on top of the same branch as #12798, so it's not quite as big as it looks. Will look to merge this one after that one is merged, but opening it now since I think it should be good to go |
|
ok other one is merged, so the diff is just from this change now |
Codecov ReportAttention: Patch coverage is
Additional details and impacted files@@ Coverage Diff @@
## master #12837 +/- ##
==========================================
- Coverage 70.39% 70.30% -0.09%
==========================================
Files 848 848
Lines 174091 174287 +196
Branches 174091 174287 +196
==========================================
- Hits 122548 122536 -12
- Misses 46296 46501 +205
- Partials 5247 5250 +3
Flags with carried forward coverage won't be shown. Click here to find out more. ☔ View full report in Codecov by Sentry. |
| let new_shard_id = shard_layout.account_id_to_shard_id(&new_account_id); | ||
| let new_shard_idx = shard_layout.get_shard_index(new_shard_id).unwrap(); |
There was a problem hiding this comment.
I don't understand why in some places we check if account is NearImplicitAccount and in others we don't. Could be great to deduplicate logic or comment in which cases we need to recompute shard id.
There was a problem hiding this comment.
I think it would all work even if you removed those checks, but the reason for them is that map_account() only changes implicit accounts, and for every other account it just clones it. So the if account is NearImplicitAccount { } just allows us to skip the unnecessary clone.
In this case we don't check it because we know we have to map the key and rewrite the value in the trie anyway, so I guess we might as well just omit the account type check.
But in any case I think this shows that map_account() should just return a Cow so we dont have to worry about calling it on every single key in the trie. will add a TODO to do that
| /// The fake block height is used to allow memtries to garbage collect. | ||
| /// Otherwise it would take significantly more memory holding old nodes. |
| { | ||
| let source_shard_id = shard_layout.get_shard_id(source_shard_idx).unwrap(); | ||
|
|
||
| remove_source_receipt_index(&mut trie_updates[source_shard_idx], index); |
There was a problem hiding this comment.
Why you may need to remove receipt from trie_updates? Looks like the logic is to take all receipts from trackers and commit them to storage, so the trie_updates is append only.
There was a problem hiding this comment.
This is for something like this case:
Suppose there are these delayed receipts (receipt_index, target_shard -> ($receipt_index: $target_shard)):
source shard 0: [(0: 0), (1: 1)]
source shard 1: [(0: 1)]
source shard 2: [(0: 0)]
Then suppose we iterate over the ones in source shard 0 first. Then for the first one (0: 0), this remove_source_receipt_index() is kind of unnecessary but won't really do anything bad. But then when we process the next one (1: 1), we add it as a receipt with index 0 in shard 1, and now there is no longer a receipt with index 1 in shard 0. Then we process shard 1 receipts and then shard 2 receipts. When we get to shard 2, we add that one receipt to shard 0, and it will have index 1, which was the one we deleted when processing shard 0 receipts. But what if that shard 2 receipt was actually (0: 2)? Then there would be no receipt with index 1 in shard 0 at the end, and we would want to just delete that index from the trie
| // Keeps track of the mapping of delayed receipts from a source shard to the | ||
| // shards their receivers will belong to in the forked chain |
There was a problem hiding this comment.
It is worth mentioning that we don't store Receipts directly due to RAM concerns. At least that's my impression.
There was a problem hiding this comment.
Oh yea that was why. Idk how important it will be in practice but I guess at least in theory it could get big. Actually now that I think about it, I should add a todo in write_delayed_receipts() because in that function we are storing everything in memory anyway :/ we should commit them periodically
|
|
||
| struct InProgressRoot { |
There was a problem hiding this comment.
Please comment what it is for. Maybe part of comment for ShardUpdateState::new can go here.
I forgot to "git add" this when applying suggestions from near#12837
I forgot to "git add" this when applying suggestions from #12837
the
fork-network amend-validatorscommand changes the accounts and keys for use with themirrortx sending tool. This will change all the implicit accounts so that we can control them in the forked chain. But after this mapping, the account might not be in the same shard as it was before, in which case it's left in the trie for the wrong shard.Here we fix it by keeping a list of updates for every shard in each
StorageMutator, and then writing updates to the right shard's trie whencommit()is called. To synchronize this across the threads that are each iterating over different shards, we put the ongoing state roots in aMutexthat we lock whenever we want to commit changes to the shard.We also introduce a new type
DelayedReceiptTrackerthat will remember the mapping from(source shard, receipt index)totarget shard, and then after all threads have finished iterating over the whole trie in their shards, we write all delayed receipts in one thread withwrite_delayed_receipts()One other small change we make here is passing the same height to
MemTries::delete_until_height()as we did to the previous call toapply_memtrie_changes(), because the garbage collection condition in that function gets rid of anything below the specified height. So with the previous code, we were keeping one extra update's worth of unnecessary nodes