Merge branch 'alin/MR-441-retain-subnet-messages' into 'master'

fix: [MR-441] Also retain ingress messages addressed to subnet in `after_split()`

In addition to ingress messages in terminal states; and ingress messages
addressed to local canisters; also retain ingress messages addressed to either
`IC_00` or to the actual subnet ID on subnet A'.

Closes MR-441 

Closes MR-441

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

Author: alin-at-dfinity

rs/replicated_state/src/metadata_state.rs

@@ -31,7 +31,7 @@ use ic_registry_subnet_type::SubnetType;
 use ic_types::{
     crypto::CryptoHash,
     ingress::{IngressState, IngressStatus},
-    messages::{MessageId, RequestOrResponse},
+    messages::{is_subnet_id, MessageId, RequestOrResponse},
     node_id_into_protobuf, node_id_try_from_option,
     nominal_cycles::NominalCycles,
     state_sync::{StateSyncVersion, CURRENT_STATE_SYNC_VERSION},
@@ -810,7 +810,7 @@ impl SystemMetadata {
     /// with canisters split among the two subnets according to the routing table.
     /// Because subnet A' retains the subnet ID of subnet A, it is identified by
     /// having `self.split_from == Some(self.own_subnet_id)`. Conversely, subnet B
-    /// has `self.split_from == Some(self.own_subnet_id)`.
+    /// has `self.split_from != Some(self.own_subnet_id)`.
     ///
     /// In the first phase (see [`Self::split()`]), the ingress history was left
     /// untouched on both subnets, in order to make it trivial to verify that no
@@ -866,13 +866,18 @@ impl SystemMetadata {
             bitcoin_get_successors_follow_up_responses,
         } = self;
 
-        split_from.expect("Not a state resulting from a subnet split");
+        let split_from = split_from.expect("Not a state resulting from a subnet split");
 
         assert_eq!(0, heap_delta_estimate.get());
         assert!(expected_compiled_wasms.is_empty());
 
         // Prune the ingress history.
-        ingress_history = ingress_history.prune_after_split(is_local_canister);
+        ingress_history = ingress_history.prune_after_split(|canister_id: &CanisterId| {
+            // An actual local canister.
+            is_local_canister(canister_id)
+                // Or this is subnet A' and message is addressed to the management canister.
+                || split_from == own_subnet_id && is_subnet_id(*canister_id, own_subnet_id)
+        });
 
         SystemMetadata {
             ingress_history,
@@ -1643,9 +1648,9 @@ impl IngressHistoryState {
     /// Prunes the ingress history (as part of subnet splitting phase 2), retaining:
     ///
     ///  * all terminal states (since they are immutable and will get pruned); and
-    ///  * all non-terminal states for ingress messages addressed to local canisters
-    ///    (as determined by the provided predicate).
-    fn prune_after_split<F>(self, is_local_canister: F) -> Self
+    ///  * all non-terminal states for ingress messages addressed to local receivers
+    ///    (canisters or subnet; as determined by the provided predicate).
+    fn prune_after_split<F>(self, is_local_receiver: F) -> Self
     where
         F: Fn(&CanisterId) -> bool,
     {
@@ -1662,7 +1667,7 @@ impl IngressHistoryState {
         let should_retain = |status: &IngressStatus| match status {
             IngressStatus::Known {
                 receiver, state, ..
-            } => state.is_terminal() || is_local_canister(&CanisterId::new(*receiver).unwrap()),
+            } => state.is_terminal() || is_local_receiver(&CanisterId::new(*receiver).unwrap()),
             IngressStatus::Unknown => false,
         };
 

rs/replicated_state/src/metadata_state/tests.rs

@@ -2,7 +2,7 @@ use super::*;
 use crate::metadata_state::subnet_call_context_manager::SubnetCallContextManager;
 use ic_constants::MAX_INGRESS_TTL;
 use ic_error_types::{ErrorCode, UserError};
-use ic_ic00_types::EcdsaCurve;
+use ic_ic00_types::{EcdsaCurve, IC_00};
 use ic_registry_routing_table::CanisterIdRange;
 use ic_test_utilities::{
     mock_time,
@@ -456,15 +456,27 @@ fn system_metadata_split() {
     const SUBNET_B: SubnetId = SUBNET_1;
     const SUBNET_C: SubnetId = SUBNET_2;
 
-    // Ingress history with 2 Received messages, addressed to canisters 1 and 2.
+    // 2 canisters: we will retain `CANISTER_1` on `SUBNET_A` and split off
+    // `CANISTER_2` to `SUBNET_B`.
+    const CANISTER_1: CanisterId = CanisterId::from_u64(1);
+    const CANISTER_2: CanisterId = CanisterId::from_u64(2);
+
+    // Ingress history with 4 Received messages, addressed to canisters 1 and 2;
+    // `IC_00`; and respectively `SUBNET_A`.
     let mut ingress_history = IngressHistoryState::new();
     let time = mock_time();
-    for i in (1..=2u64).rev() {
+    let receivers = [
+        CANISTER_1.get(),
+        CANISTER_2.get(),
+        IC_00.get(),
+        SUBNET_A.get(),
+    ];
+    for (i, receiver) in receivers.into_iter().enumerate().rev() {
         ingress_history.insert(
-            message_test_id(i),
+            message_test_id(i as u64),
             IngressStatus::Known {
-                receiver: canister_test_id(i).get(),
-                user_id: user_test_id(i),
+                receiver,
+                user_id: user_test_id(i as u64),
                 time,
                 state: IngressState::Received,
             },
@@ -473,9 +485,17 @@ fn system_metadata_split() {
         );
     }
 
+    // `CANISTER_1` remains on `SUBNET_A`.
+    let is_canister_on_subnet_a = |canister_id: &CanisterId| *canister_id == CANISTER_1;
+    // All ingress messages except the one addressed to `CANISTER_2` (including the
+    // ones for `IC_00` and `SUBNET_A`) should remain on `SUBNET_A` after the split.
+    let is_receiver_on_subnet_a = |canister_id: &CanisterId| *canister_id != CANISTER_2;
+    // Only ingress messages for `CANISTER_2` should be retained on `SUBNET_B`.
+    let is_canister_on_subnet_b = |canister_id: &CanisterId| *canister_id == CANISTER_2;
+
     let streams = Streams {
         streams: btreemap! { SUBNET_C => Stream::new(StreamIndexedQueue::with_begin(13.into()), 14.into()) },
-        responses_size_bytes: btreemap! { canister_test_id(1) => 169 },
+        responses_size_bytes: btreemap! { CANISTER_1 => 169 },
     };
 
     // Use uncommon `SubnetType::VerifiedApplication` to make it more likely to
@@ -503,14 +523,12 @@ fn system_metadata_split() {
     // 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);
 
-    // Expect same metadata, but with pruned ingress history, no previous hash and
-    // no split marker.
+    // Expect same metadata, but with pruned ingress history and no split marker.
     expected.ingress_history = expected
         .ingress_history
-        .prune_after_split(is_canister_on_subnet_a);
+        .prune_after_split(is_receiver_on_subnet_a);
     expected.split_from = None;
     assert_eq!(expected, metadata_a_phase_2);
 
@@ -528,10 +546,9 @@ fn system_metadata_split() {
     // 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);
 
-    // Expect pruned ingress history and no previous hash or split marker.
+    // Expect pruned ingress history and no split marker.
     expected.split_from = None;
     expected.ingress_history = expected
         .ingress_history

rs/types/types/src/messages.rs

@@ -22,7 +22,8 @@ use ic_protobuf::proxy::{try_from_option_field, ProxyDecodeError};
 use ic_protobuf::state::canister_state_bits::v1 as pb;
 use ic_protobuf::types::v1 as pb_types;
 pub use ingress_messages::{
-    extract_effective_canister_id, Ingress, ParseIngressError, SignedIngress, SignedIngressContent,
+    extract_effective_canister_id, is_subnet_id, Ingress, ParseIngressError, SignedIngress,
+    SignedIngressContent,
 };
 pub use inter_canister::{
     CallContextId, CallbackId, Payload, RejectContext, Request, RequestOrResponse, Response,

rs/types/types/src/messages/ingress_messages.rs

@@ -12,7 +12,7 @@ use crate::{
 use ic_error_types::{ErrorCode, UserError};
 use ic_ic00_types::{
     CanisterIdRecord, CanisterInfoRequest, InstallCodeArgs, Method, Payload, SetControllerArgs,
-    UpdateSettingsArgs,
+    UpdateSettingsArgs, IC_00,
 };
 use ic_protobuf::{
     log::ingress_message_log_entry::v1::IngressMessageLogEntry,
@@ -554,6 +554,7 @@ mod test {
     }
 }
 
-fn is_subnet_id(canister_id: CanisterId, own_subnet_id: SubnetId) -> bool {
-    canister_id == CanisterId::ic_00() || canister_id.get_ref() == own_subnet_id.get_ref()
+/// Checks whether the given canister ID refers to the subnet (directly or as `IC_00`).
+pub fn is_subnet_id(canister_id: CanisterId, own_subnet_id: SubnetId) -> bool {
+    canister_id == IC_00 || canister_id.get_ref() == own_subnet_id.get_ref()
 }