test: produce chunks in resharding v3 test #12196
Conversation
Codecov ReportAttention: Patch coverage is
Additional details and impacted files@@ Coverage Diff @@
## master #12196 +/- ##
==========================================
+ Coverage 71.69% 71.76% +0.06%
==========================================
Files 825 825
Lines 165834 165747 -87
Branches 165834 165747 -87
==========================================
+ Hits 118902 118952 +50
+ Misses 41751 41608 -143
- Partials 5181 5187 +6
Flags with carried forward coverage won't be shown. Click here to find out more. ☔ View full report in Codecov by Sentry. |
| let next_block_has_new_shard_layout = | ||
| self.epoch_manager.will_shard_layout_change(prev_hash)? | ||
| && self.epoch_manager.is_next_block_epoch_start(block.hash())?; |
| // state transition covered by state witness. Won't happen in production | ||
| // until resharding release. Must be removed and replaced with proper | ||
| // resharding state transition validation. | ||
| ShardLayoutChange, |
There was a problem hiding this comment.
My thinking was that the resharding would be in addition to the existing MainTransition. I think we will first need to apply the chunk (if present) and only then validate resharding.
There was a problem hiding this comment.
I guess I mean that we can have both new chunk and resharding at the same time
There was a problem hiding this comment.
Is that what you mean by temporary? If so I'm happy to approve to unblock the progress, but still I'm curious about your thought of the proper structure for this logic.
There was a problem hiding this comment.
Yes, this is very temporary. It is just the easiest way to skip everything related to chunk validation in our case. Made a comment.
| @@ -130,16 +130,30 @@ pub fn pre_validate_chunk_state_witness( | |||
| // Blocks from the last last new chunk (exclusive) to the last new chunk (inclusive). | |||
| let mut blocks_after_last_last_chunk = Vec::new(); | |||
|
|
|||
| { | |||
| let (last_chunk_shard_id, last_chunk_shard_index) = { | |||
There was a problem hiding this comment.
nit: Can this be moved to a helper method?
| (current_shard_id, current_shard_index) = | ||
| epoch_manager.get_prev_shard_id(&block_hash, current_shard_id)?; | ||
|
|
||
| let chunks = block.chunks(); | ||
| let Some(chunk) = chunks.get(shard_index) else { | ||
| let Some(chunk) = chunks.get(current_shard_index) else { |
There was a problem hiding this comment.
Is that off by one block?
the shard_id and shard_index are from the prev block (that's what get_prev_shard_id gives)
the block.chunks() are from the current block
There was a problem hiding this comment.
I think it was correct. However, I made it more intuitive - by changing current_shard_id simultaneously with going to prev block.
| (last_chunk_shard_id, last_chunk_shard_index) | ||
| }; | ||
|
|
||
| let last_chunk_block = blocks_after_last_last_chunk.first().ok_or_else(|| { |
There was a problem hiding this comment.
sanity check for myself
- the blocks in blocks_after_last_last_chunk are in reverse chronological order
- the blocks include the last chunk block (it's inclusive)
| state_witness: &ChunkStateWitness, | ||
| store: &ChainStore, | ||
| epoch_manager: &dyn EpochManagerAdapter, |
There was a problem hiding this comment.
min nit: It's nice to keep the arguments from most general to most specific. In this case I would make it
store,
epoch manager,
state witness,
It's nice to have a convention and this one is as good as any I guess.
Improve the test by checking that all chunks are included, and the chain passes through one epoch after resharding.
For that, I need to properly skip chunk validation if resharding happened in the middle, as this is not a part of early MVP. One more useful and necessary change: on iterating over block range of state transition, I need to move from shard_id to prev_shard_id, so that we query proper chunk in a block.
Checked offline that without validation skip, chunks for shards 1-2 are not included.