Merge branch 'alin/MR-444-retain-prev_state_hash' into 'master'

fix: [MR-444] Do not reset `SystemMetadata::prev_state_hash`

`SystemMetadata::after_split()` used to reset `SystemMetadata::prev_state_hash`,
on account of the post-split state being a genesis state. However, by the time
`after_split()` is called, this is already the tip (the mutable state that the
round will execute on) and its `prev_state_hash` has already been set to the
checkpoint/CUP hash. Resetting it to None will cause `commit_and_certify()` to
panic.

Closes MR-444 

Closes MR-444

See merge request dfinity-lab/public/ic!12656

Author: alin-at-dfinity

rs/replicated_state/src/metadata_state.rs

@@ -821,6 +821,8 @@ impl SystemMetadata {
     /// in terminal states and messages addressed to local canisters.
     ///
     /// Notes:
+    ///  * `prev_state_hash` has just been set by `take_tip()` to the checkpoint
+    ///    hash (checked against the hash in the CUP). It must be preserved.
     ///  * `own_subnet_type` has just been set during `load_checkpoint()`, based on
     ///    the registry subnet record of the subnet that this node is part of.
     ///  * `batch_time`, `network_topology` and `own_subnet_features` will be set
@@ -841,7 +843,7 @@ impl SystemMetadata {
             streams,
             canister_allocation_ranges,
             last_generated_canister_id,
-            prev_state_hash: _,
+            prev_state_hash,
             // Overwritten as soon as the round begins, no explicit action needed.
             batch_time,
             // Overwritten as soon as the round begins, no explicit action needed.
@@ -872,9 +874,6 @@ impl SystemMetadata {
         // Prune the ingress history.
         ingress_history = ingress_history.prune_after_split(is_local_canister);
 
-        // This is a genesis state, there is no previous state hash.
-        let prev_state_hash = None;
-
         SystemMetadata {
             ingress_history,
             streams,

rs/replicated_state/src/metadata_state/tests.rs

@@ -499,6 +499,10 @@ fn system_metadata_split() {
     assert_eq!(expected, metadata_a_phase_1);
 
     // Split off subnet A', phase 2.
+    //
+    // Technically some parts of the `SystemMetadata` (such as `prev_state_hash` and
+    // `own_subnet_type`) would be replaced during loading. However, we only care
+    // that `after_split()` does not touch them.
     let is_canister_on_subnet_a = |canister_id: &CanisterId| *canister_id == canister_test_id(0);
     let metadata_a_phase_2 = metadata_a_phase_1.after_split(is_canister_on_subnet_a);
 
@@ -507,7 +511,6 @@ fn system_metadata_split() {
     expected.ingress_history = expected
         .ingress_history
         .prune_after_split(is_canister_on_subnet_a);
-    expected.prev_state_hash = None;
     expected.split_from = None;
     assert_eq!(expected, metadata_a_phase_2);
 
@@ -521,6 +524,10 @@ fn system_metadata_split() {
     assert_eq!(expected, metadata_b_phase_1);
 
     // Split off subnet B, phase 2.
+    //
+    // Technically some parts of the `SystemMetadata` (such as `prev_state_hash` and
+    // `own_subnet_type`) would be replaced during loading. However, we only care
+    // that `after_split()` does not touch them.
     let is_canister_on_subnet_b = |canister_id: &CanisterId| !is_canister_on_subnet_a(canister_id);
     let metadata_b_phase_2 = metadata_b_phase_1.after_split(is_canister_on_subnet_b);
 
@@ -529,7 +536,6 @@ fn system_metadata_split() {
     expected.ingress_history = expected
         .ingress_history
         .prune_after_split(is_canister_on_subnet_b);
-    expected.prev_state_hash = None;
     assert_eq!(expected, metadata_b_phase_2);
 }