resharding: verify proof #12485

Longarithm · 2024-11-19T18:42:10Z

Completes the resharding validation flow on the chunk validator side.

The code replaces MainTransition::ShardLayoutChange with actual logic. The whole preparation is needed to call let new_root = trie.retain_split_shard(&boundary_account, retain_mode)?;, to verify it against state root provided in transition.

The main complexity is to introduce implicit transition for resharding to the right place - after applying last chunk in old shard layout, potentially missing, and before applying first chunk in the new layout. To do that, I needed to modify get_state_witness_block_range once again, and to make it readable, I rewrote it using TraversalPosition. The point of that is to change all parameters of the loop at once, instead of changing them one-by-one in the loop. Then, I generate ImplicitTransitionParams right in the loop. Finally, this allows to insert resharding transition before jumping to the previous block hash and child shard id, if needed. Note that this flow happens in reverse to chain flow.

Also unfortunately this is not triggered on production tests, because when nodes track all shards, the condition if let Ok(prev_chunk_extra) = chain.get_chunk_extra(prev_block_hash, &shard_uid) { ... } is triggered - if latest chunk extra is already computed, we skip unnecessary validations. So for now I checked the correctness by commenting this path and running tests locally.

codecov · 2024-11-19T18:59:55Z

Codecov Report

Attention: Patch coverage is 69.18239% with 49 lines in your changes missing coverage. Please review.

Project coverage is 69.96%. Comparing base (024c47c) to head (ca0c082).
Report is 14 commits behind head on master.

Files with missing lines	Patch %	Lines
...chain/src/stateless_validation/chunk_validation.rs	71.05%	27 Missing and 17 partials ⚠️
tools/replay-archive/src/cli.rs	0.00%	3 Missing ⚠️
...nt/src/stateless_validation/chunk_validator/mod.rs	33.33%	0 Missing and 2 partials ⚠️

Additional details and impacted files

@@            Coverage Diff             @@
##           master   #12485      +/-   ##
==========================================
+ Coverage   69.83%   69.96%   +0.12%     
==========================================
  Files         838      838              
  Lines      169447   169164     -283     
  Branches   169447   169164     -283     
==========================================
+ Hits       118338   118353      +15     
+ Misses      45853    45674     -179     
+ Partials     5256     5137     -119

Flag	Coverage Δ
backward-compatibility	`0.16% <0.00%> (+<0.01%)`	⬆️
db-migration	`0.16% <0.00%> (+<0.01%)`	⬆️
genesis-check	`1.28% <0.00%> (-0.01%)`	⬇️
linux	`69.27% <58.49%> (+0.11%)`	⬆️
linux-nightly	`69.50% <69.18%> (+0.08%)`	⬆️
macos	`51.08% <41.50%> (+0.07%)`	⬆️
pytests	`1.59% <0.00%> (-0.01%)`	⬇️
sanity-checks	`1.39% <0.00%> (-0.01%)`	⬇️
unittests	`69.78% <69.18%> (+0.12%)`	⬆️
upgradability	`0.20% <0.00%> (-0.01%)`	⬇️

Flags with carried forward coverage won't be shown. Click here to find out more.

☔ View full report in Codecov by Sentry.
📢 Have feedback on the report? Share it here.

🚨 Try these New Features:

Flaky Tests Detection - Detect and resolve failed and flaky tests
JS Bundle Analysis - Avoid shipping oversized bundles

Trisfald · 2024-11-20T10:16:54Z

chain/client/src/stateless_validation/chunk_validator/mod.rs

        // This usually happens because we are a chunk producer and
        // therefore have the chunk extra for the previous block saved on disk.
        // We can also skip validating the chunk state witness in this case.
+        // We don't need to switch to parent shard uid, because resharding
+        // creates chunk extra for new shard uid.


Just to double check, do the new shards' chunk extra contain the final state roots after applying the state changes on the parent and then splitting?

Yes. For the last block hash in epoch, chunk extra for parent shard id includes results of applying last chunk, and there is also chunk extra for child shard id, which also includes split result on top of that.

Trisfald · 2024-11-20T10:24:25Z

chain/chain/src/stateless_validation/chunk_validation.rs

+    /// Transition parameters **after** the last chunk, corresponding to all
+    /// state transitions from the last new chunk (exclusive) to the parent
+    /// block (inclusive).
+    implicit_transition_params: Vec<ImplicitTransitionParams>,
    /// Blocks from the last last new chunk (exclusive) to the last new chunk


Is the "last last new chunk" the "previous last new chunk"? Like.. the last new chunk of the previous state witness?

Trisfald · 2024-11-20T10:51:42Z

If I understood correctly , we must fix resharding_v3 test loop to work without tracking all shards, then this code will be tested? Do you plan to address this in another PR?

Also, should we add tests where a wrong split proof is sent and we validate the chunk is not endorsed?

wacban

LGTM
Nice, it all comes together!

wacban · 2024-11-20T13:35:36Z

chain/chain/src/stateless_validation/chunk_validation.rs

+pub enum ImplicitTransitionParams {
+    ApplyChunk(ApplyChunkBlockContext, ShardContext),
+    Resharding(AccountId, RetainMode, ShardUId),
+}


❤️

nit: Can you add a comment please?

wacban · 2024-11-20T13:37:55Z

chain/chain/src/stateless_validation/chunk_validation.rs

+        /// Number of chunks seen during traversal.
+        chunks_seen: u32,


Is that only for new chunks or any chunks?

I think it's new chunks considering position.chunks_seen + if is_new_chunk { 1 } else { 0 };
Perhaps num_unique_chunks_seen or something similar might help?

wacban · 2024-11-20T13:38:36Z

chain/chain/src/stateless_validation/chunk_validation.rs

+        /// Below, supposed id and index of last chunk before the chunk being
+        /// validated.


nit: I can't quite parse this sentence, can you put in different words? The "below" in particular is confusing.

wacban · 2024-11-20T13:45:38Z

chain/chain/src/stateless_validation/chunk_validation.rs

+            last_chunk_shard_id: if position.chunks_seen == 0 {
+                prev_shard_id
+            } else {
+                position.last_chunk_shard_id
+            },
+            last_chunk_shard_index: if position.chunks_seen == 0 {
+                prev_shard_index
+            } else {
+                position.last_chunk_shard_index
+            },


nit: Maybe move this to a separate line?

wacban · 2024-11-20T13:47:31Z

chain/chain/src/stateless_validation/chunk_validation.rs

+    let Some(ReshardingEventType::SplitShard(params)) =
+        ReshardingEventType::from_shard_layout(shard_layout, *prev_hash)?
+    else {
+        return Ok(None);
+    };


mini nit: I think a match statement on the ReshardingEventType will be more future proof so that the compiler warns that a new event is not handled here. let else return is fine for handling Option though.

wacban · 2024-11-20T13:50:00Z

chain/chain/src/stateless_validation/chunk_validation.rs

+        )));
+    }
+
+    for (implicit_transition_params, transition) in pre_validation_output


Just for my education - what exactly is the length of the params, the pre validation and why are they always equal?

ChunkStateWitness is expected to provide all implicit state transitions, but it must be validated.
Chunk validator can retrieve number of implicit transitions by chain traversal itself. It is always number of missing chunks plus resharding transition if needed. If the loop completes, it means that implicit transitions were provided correctly.

wacban · 2024-11-20T13:52:36Z

What is your plan for tests for this logic?

shreyan-gupta

Looks great!

shreyan-gupta · 2024-11-20T15:35:48Z

chain/chain/src/stateless_validation/chunk_validation.rs

+                    old_root,
+                    true,
+                );
+                let new_root = trie.retain_split_shard(&boundary_account, retain_mode)?;


shreyan-gupta · 2024-11-20T15:47:21Z

chain/chain/src/stateless_validation/chunk_validation.rs

+        /// Number of chunks seen during traversal.
+        chunks_seen: u32,


I think it's new chunks considering position.chunks_seen + if is_new_chunk { 1 } else { 0 };
Perhaps num_unique_chunks_seen or something similar might help?

shreyan-gupta · 2024-11-20T15:48:23Z

chain/chain/src/stateless_validation/chunk_validation.rs

+        /// currently observed block.
+        shard_id: ShardId,
+        /// Hash of the previous block.
+        prev_hash: CryptoHash,


Are we maintaining both prev_hash and prev_block mainly for readability? prev_hash can be easily got from prev_block.hash() right?

shreyan-gupta · 2024-11-20T15:57:49Z

chain/chain/src/stateless_validation/chunk_validation.rs

+        let prev_prev_hash = *position.prev_block.header().prev_hash();
+        let epoch_id = epoch_manager.get_epoch_id_from_prev_block(&position.prev_hash)?;
+        let shard_layout = epoch_manager.get_shard_layout(&epoch_id)?;
+        let shard_uid = ShardUId::from_shard_id_and_layout(position.shard_id, &shard_layout);


We have a nice function epoch_manager.shard_id_to_uid(..) if that seems more legible. Also seems like fetching the shard_layout can be pushed to within check_shard_layout_transition

shreyan-gupta · 2024-11-20T16:24:33Z

chain/chain/src/stateless_validation/chunk_validation.rs

+/// Checks if chunk validation requires a transition to new shard layout in the
+/// block with `prev_hash`, with a split resulting in the `shard_uid`, and if
+/// so, returns the corresponding resharding transition parameters.
+fn check_shard_layout_transition(


Seems like this function is specifically to get the resharding transition, could we rename it to reflect that, say get_resharding_transition?

shreyan-gupta · 2024-11-20T17:11:26Z

chain/chain/src/update_shard.rs

@@ -70,10 +70,6 @@ pub enum ShardUpdateReason {
 /// Information about shard to update.
 pub struct ShardContext {
    pub shard_uid: ShardUId,
-    /// Whether node cares about shard in this epoch.
-    pub cares_about_shard_this_epoch: bool,


Seems like we don't care about shard this epoch any more, lol! Was this and will_shard_layout_change not being used anywhere?

It was part of resharding v2 logic.

Longarithm · 2024-11-22T23:20:34Z

If I understood correctly , we must fix resharding_v3 test loop to work without tracking all shards, then this code will be tested? Do you plan to address this in another PR?

Yeah. I believe @marcelo-gonzalez is working on single shard tracking.

As for tests, the current plan was to resharding_v3 test should not fail, I will add unit tests in the next PR.

Longarithm added 4 commits November 19, 2024 22:18

verify

fbebd79

Merge branch 'master' into rv3-verify-proof

50bbcb3

fix

d302e6a

fix

12a1e13

Longarithm requested review from wacban, pugachAG, Trisfald and shreyan-gupta November 19, 2024 18:42

Longarithm requested a review from a team as a code owner November 19, 2024 18:42

fix

25d248f

Longarithm linked an issue Nov 19, 2024 that may be closed by this pull request

[ReshardingV3] State Witness #12019

Closed

Trisfald reviewed Nov 20, 2024

View reviewed changes

wacban approved these changes Nov 20, 2024

View reviewed changes

shreyan-gupta approved these changes Nov 20, 2024

View reviewed changes

feedback

ca0c082

Longarithm enabled auto-merge November 22, 2024 23:19

Longarithm added this pull request to the merge queue Nov 22, 2024

github-merge-queue bot removed this pull request from the merge queue due to failed status checks Nov 23, 2024

Longarithm added this pull request to the merge queue Nov 23, 2024

Merged via the queue into near:master with commit af6559a Nov 23, 2024
26 of 28 checks passed

Longarithm deleted the rv3-verify-proof branch November 23, 2024 09:22

Provide feedback

Saved searches

Use saved searches to filter your results more quickly

resharding: verify proof #12485

resharding: verify proof #12485

Longarithm commented Nov 19, 2024

codecov bot commented Nov 19, 2024 •

edited

Loading

Trisfald Nov 20, 2024

Longarithm Nov 20, 2024

Trisfald Nov 20, 2024

Longarithm Nov 20, 2024

Trisfald commented Nov 20, 2024

wacban left a comment

wacban Nov 20, 2024

wacban Nov 20, 2024

shreyan-gupta Nov 20, 2024

wacban Nov 20, 2024

wacban Nov 20, 2024

wacban Nov 20, 2024

wacban Nov 20, 2024

Longarithm Nov 22, 2024

wacban commented Nov 20, 2024

shreyan-gupta left a comment

shreyan-gupta Nov 20, 2024

shreyan-gupta Nov 20, 2024

shreyan-gupta Nov 20, 2024

shreyan-gupta Nov 20, 2024

shreyan-gupta Nov 20, 2024

shreyan-gupta Nov 20, 2024

Longarithm Nov 22, 2024

Longarithm commented Nov 22, 2024

		/// Number of chunks seen during traversal.
		chunks_seen: u32,

		/// Below, supposed id and index of last chunk before the chunk being
		/// validated.

resharding: verify proof #12485

resharding: verify proof #12485

Conversation

Longarithm commented Nov 19, 2024

codecov bot commented Nov 19, 2024 • edited Loading

Codecov Report

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Trisfald commented Nov 20, 2024

wacban left a comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

wacban commented Nov 20, 2024

shreyan-gupta left a comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Longarithm commented Nov 22, 2024

codecov bot commented Nov 19, 2024 •

edited

Loading