feat(resharding) - congestion info computation #12581
Conversation
| @@ -213,11 +222,24 @@ impl ReshardingManager { | |||
| }; | |||
| let mem_changes = trie_changes.mem_trie_changes.as_ref().unwrap(); | |||
| let new_state_root = mem_tries.apply_memtrie_changes(block_height, mem_changes); | |||
| drop(mem_tries); | |||
There was a problem hiding this comment.
This is annoying but needed to prevent a deadlock.
| let epoch_id = block.header().epoch_id(); | ||
| let protocol_version = self.epoch_manager.get_epoch_protocol_version(epoch_id)?; | ||
|
|
||
| let trie = tries.get_trie_for_shard(new_shard_uid, new_state_root); |
There was a problem hiding this comment.
bootstrap_congestion_info requires TrieAccess - can I get it somehow from the memtries above directly?
| // TODO(resharding): set all fields of `ChunkExtra`. Consider stronger | ||
| // typing. Clarify where it should happen when `State` and | ||
| // `FlatState` update is implemented. | ||
| let mut child_chunk_extra = ChunkExtra::clone(&chunk_extra); | ||
| *child_chunk_extra.state_root_mut() = new_state_root; | ||
| // TODO(resharding) - Implement proper congestion info for | ||
| // resharding. The current implementation is very expensive. |
There was a problem hiding this comment.
While we're waiting for the bandwidth scheduler to give us an efficient way to do the same
OutgoingMetadatas are already fully functional and merged into master, I didn't plan to add anything else. What sort of functionality are you looking for?
There was a problem hiding this comment.
Ah nice! I think all I need is the total size of the buffered receipts. I'll check and come back.
|
I will do the proper version anyway so don't mind it for now please. |
wacban
force-pushed
the
waclaw-resharding
branch
2 times, most recently
from
9529c2f to
e55dae2
Compare
wacban
force-pushed
the
waclaw-resharding
branch
from
a25c85b to
31b81d0
Compare
|
nayduck with extra assertions: |
wacban
force-pushed
the
waclaw-resharding
branch
from
42b1175 to
752e804
Compare
Codecov ReportAttention: Patch coverage is
Additional details and impacted files@@ Coverage Diff @@
## master #12581 +/- ##
==========================================
- Coverage 70.49% 70.48% -0.01%
==========================================
Files 845 845
Lines 172247 172384 +137
Branches 172247 172384 +137
==========================================
+ Hits 121426 121506 +80
- Misses 45725 45761 +36
- Partials 5096 5117 +21
Flags with carried forward coverage won't be shown. Click here to find out more. ☔ View full report in Codecov by Sentry. |
wacban
changed the title
There was a problem hiding this comment.
I think it looks okay to me, but I will let @jancionear approve, since I'm not as familiar with the bandwidth scheduler/congestion code. But ping me if you need me to stamp to unblock it
| let gas = receipt_groups.total_gas(); | ||
| let gas = gas.try_into().expect("ReceiptGroup gas must fit in u64"); | ||
|
|
||
| congestion_info |
There was a problem hiding this comment.
do we not have to set total_receipts_num as well?
There was a problem hiding this comment.
Good sanity check! The total_receipts_num field is part of ReceiptGroupsQueueData which is resharded by copying to left child only. It is handled by the memtrie and flat storage split operations. MemTrie is already implemented here and this PR adds the flat storage equivalent.
Implementing the congestion info computation based on the parent congestion info and the receipt groups info from the parent trie. The seed used for the allowed shard is a bit hacky - please let me know if this makes sense.
I added assertion checking that the buffered gas in congestion info is zero iff the buffers are empty. With this in place the
test_resharding_v3_buffered_receipts_towards_splitted_shardtests fail without the updated congestion info and pass with it in place.