refactor(registry): Remove the shards.is_empty condition for asserting the equality of the 2 routing table formats (#5896)

# Why

When doing invariants checks, we should always assert the equality of
the 2 routing table formats. The `is_empty` pre-condition was added
before the initial migration (where shards don't exist yet), which has
long been completed. This change also helps us identify and fix tests
where shards are not added in test setups.

# What

* Remove the `shards.is_empty` pre-conditioin for checking equality
* Update tests so that they always prepare shards when routing table is
needed.

Author: jasonz-dfinity

rs/registry/canister/src/invariants/checks.rs

@@ -190,8 +190,8 @@ mod tests {
         },
     };
     use ic_registry_keys::{
-        make_canister_migrations_record_key, make_node_operator_record_key,
-        make_routing_table_record_key,
+        make_canister_migrations_record_key, make_canister_ranges_key,
+        make_node_operator_record_key, make_routing_table_record_key,
     };
     use ic_registry_routing_table::{CanisterIdRange, CanisterMigrations, RoutingTable};
     use ic_registry_transport::{
@@ -301,21 +301,26 @@ mod tests {
         }).unwrap();
 
         let routing_table = PbRoutingTable::from(routing_table);
-        let key1 = make_routing_table_record_key();
-        let value1 = routing_table.encode_to_vec();
+        let routing_table_key = make_routing_table_record_key();
+        let routing_table_shard_key = make_canister_ranges_key(CanisterId::from(0));
+        let routing_table_value = routing_table.encode_to_vec();
 
         // The canister ID range {0x200:0x2ff} in `canister_migrations` is not hosted by any subnet in trace according to the routing table.
         let canister_migrations = CanisterMigrations::try_from(btreemap! {
             CanisterIdRange{ start: CanisterId::from(0x200), end: CanisterId::from(0x2ff) } => vec![subnet_test_id(1), subnet_test_id(2)],
         }).unwrap();
 
         let canister_migrations = PbCanisterMigrations::from(canister_migrations);
-        let key2 = make_canister_migrations_record_key();
-        let value2 = canister_migrations.encode_to_vec();
+        let canister_migrations_key = make_canister_migrations_record_key();
+        let canister_migrations_value = canister_migrations.encode_to_vec();
 
         let mutations = vec![
-            insert(key1.as_bytes(), value1),
-            insert(key2.as_bytes(), value2),
+            insert(routing_table_key.as_bytes(), &routing_table_value),
+            insert(routing_table_shard_key.as_bytes(), &routing_table_value),
+            insert(
+                canister_migrations_key.as_bytes(),
+                &canister_migrations_value,
+            ),
         ];
 
         let registry = Registry::new();
@@ -324,11 +329,12 @@ mod tests {
 
     #[test]
     fn snapshot_reflects_latest_registry_state() {
-        let key1 = make_routing_table_record_key();
-        let value1 = PbRoutingTable { entries: vec![] }.encode_to_vec();
+        let routing_table_key = make_routing_table_record_key();
+        let routing_table_shard_key = make_canister_ranges_key(CanisterId::from(0));
+        let routing_table_value = PbRoutingTable { entries: vec![] }.encode_to_vec();
 
-        let key2 = make_node_operator_record_key(*TEST_USER1_PRINCIPAL);
-        let value2 = NodeOperatorRecord {
+        let node_operator_key = make_node_operator_record_key(*TEST_USER1_PRINCIPAL);
+        let node_operator_value = NodeOperatorRecord {
             node_operator_principal_id: (*TEST_USER1_PRINCIPAL).to_vec(),
             node_allowance: 0,
             node_provider_principal_id: (*TEST_USER1_PRINCIPAL).to_vec(),
@@ -340,18 +346,19 @@ mod tests {
         .encode_to_vec();
 
         let mutations = vec![
-            insert(key1.as_bytes(), &value1),
-            insert(key2.as_bytes(), &value2),
+            insert(routing_table_key.as_bytes(), &routing_table_value),
+            insert(routing_table_shard_key.as_bytes(), &routing_table_value),
+            insert(node_operator_key.as_bytes(), &node_operator_value),
         ];
         let snapshot = Registry::new().take_latest_snapshot_with_mutations(&mutations);
 
-        let snapshot_data = snapshot.get(key1.as_bytes());
+        let snapshot_data = snapshot.get(routing_table_key.as_bytes());
         assert!(snapshot_data.is_some());
-        assert_eq!(snapshot_data.unwrap(), &value1);
+        assert_eq!(snapshot_data.unwrap(), &routing_table_value);
 
-        let snapshot_data = snapshot.get(key2.as_bytes());
+        let snapshot_data = snapshot.get(node_operator_key.as_bytes());
         assert!(snapshot_data.is_some());
-        assert_eq!(snapshot_data.unwrap(), &value2);
+        assert_eq!(snapshot_data.unwrap(), &node_operator_value);
     }
 
     #[test]

rs/registry/canister/src/invariants/routing_table.rs

@@ -35,13 +35,11 @@ fn get_routing_table(snapshot: &RegistrySnapshot) -> RoutingTable {
 
     // If there are shards, they should match the routing table record.
     let shards = get_routing_table_shards(snapshot);
-    if !shards.is_empty() {
-        let rt_from_shards = RoutingTable::try_from(shards).unwrap();
-        assert_eq!(
-            rt_from_shards, rt_from_routing_table_record,
-            "Routing tables from shards and routing table record do not match."
-        );
-    }
+    let rt_from_shards = RoutingTable::try_from(shards).unwrap();
+    assert_eq!(
+        rt_from_shards, rt_from_routing_table_record,
+        "Routing tables from shards and routing table record do not match."
+    );
 
     rt_from_routing_table_record
 }
@@ -127,20 +125,32 @@ mod tests {
     use prost::Message;
     use std::convert::TryFrom;
 
+    fn insert_routing_table_to_snapshot(
+        routing_table: RoutingTable,
+        snapshot: &mut RegistrySnapshot,
+    ) {
+        let routing_table = PbRoutingTable::from(routing_table);
+        snapshot.insert(
+            make_canister_ranges_key(CanisterId::from(0)).into_bytes(),
+            routing_table.encode_to_vec(),
+        );
+        // TODO(NNS1-3781): Remove this once we have sharded table supported by all clients, and
+        // inline the function after removal.
+        snapshot.insert(
+            make_routing_table_record_key().into_bytes(),
+            routing_table.encode_to_vec(),
+        );
+    }
+
     #[test]
     fn nonexistent_canister_migrations_can_pass_invariants_check() {
         let mut snapshot = RegistrySnapshot::new();
 
         let routing_table = RoutingTable::try_from(btreemap! {
-        CanisterIdRange{ start: CanisterId::from(0x0), end: CanisterId::from(0xff) } => subnet_test_id(1),
-        CanisterIdRange{ start: CanisterId::from(0x100), end: CanisterId::from(0x1ff) } => subnet_test_id(2),
-    }).unwrap();
-
-        let routing_table = PbRoutingTable::from(routing_table);
-        let key1 = make_routing_table_record_key();
-        let value1 = routing_table.encode_to_vec();
-
-        snapshot.insert(key1.into_bytes(), value1);
+            CanisterIdRange{ start: CanisterId::from(0x0), end: CanisterId::from(0xff) } => subnet_test_id(1),
+            CanisterIdRange{ start: CanisterId::from(0x100), end: CanisterId::from(0x1ff) } => subnet_test_id(2),
+        }).unwrap();
+        insert_routing_table_to_snapshot(routing_table, &mut snapshot);
 
         assert!(check_routing_table_invariants(&snapshot).is_ok());
         assert!(check_canister_migrations_invariants(&snapshot).is_ok());
@@ -151,19 +161,14 @@ mod tests {
         let mut snapshot = RegistrySnapshot::new();
 
         let routing_table = RoutingTable::try_from(btreemap! {
-        CanisterIdRange{ start: CanisterId::from(0x0), end: CanisterId::from(0xff) } => subnet_test_id(1),
-        CanisterIdRange{ start: CanisterId::from(0x100), end: CanisterId::from(0x1ff) } => subnet_test_id(2),
-    }).unwrap();
-
-        let routing_table = PbRoutingTable::from(routing_table);
-        let key1 = make_routing_table_record_key();
-        let value1 = routing_table.encode_to_vec();
-
-        let key2 = make_canister_migrations_record_key();
-        let value2 = PbCanisterMigrations { entries: vec![] }.encode_to_vec();
-
-        snapshot.insert(key1.into_bytes(), value1);
-        snapshot.insert(key2.into_bytes(), value2);
+            CanisterIdRange{ start: CanisterId::from(0x0), end: CanisterId::from(0xff) } => subnet_test_id(1),
+            CanisterIdRange{ start: CanisterId::from(0x100), end: CanisterId::from(0x1ff) } => subnet_test_id(2),
+        }).unwrap();
+        insert_routing_table_to_snapshot(routing_table, &mut snapshot);
+        snapshot.insert(
+            make_canister_migrations_record_key().into_bytes(),
+            PbCanisterMigrations { entries: vec![] }.encode_to_vec(),
+        );
 
         assert!(check_routing_table_invariants(&snapshot).is_ok());
         assert!(check_canister_migrations_invariants(&snapshot).is_ok());
@@ -175,15 +180,10 @@ mod tests {
 
         // The routing table before canister migration.
         let routing_table = RoutingTable::try_from(btreemap! {
-        CanisterIdRange{ start: CanisterId::from(0x0), end: CanisterId::from(0xff) } => subnet_test_id(1),
-        CanisterIdRange{ start: CanisterId::from(0x100), end: CanisterId::from(0x1ff) } => subnet_test_id(2),
-    }).unwrap();
-
-        let routing_table = PbRoutingTable::from(routing_table);
-        let key1 = make_routing_table_record_key();
-        let value1 = routing_table.encode_to_vec();
-
-        snapshot.insert(key1.into_bytes(), value1);
+            CanisterIdRange{ start: CanisterId::from(0x0), end: CanisterId::from(0xff) } => subnet_test_id(1),
+            CanisterIdRange{ start: CanisterId::from(0x100), end: CanisterId::from(0x1ff) } => subnet_test_id(2),
+        }).unwrap();
+        insert_routing_table_to_snapshot(routing_table, &mut snapshot);
         assert!(check_routing_table_invariants(&snapshot).is_ok());
         assert!(check_canister_migrations_invariants(&snapshot).is_ok());
 
@@ -193,31 +193,28 @@ mod tests {
     }).unwrap();
 
         let canister_migrations = PbCanisterMigrations::from(canister_migrations);
-        let key2 = make_canister_migrations_record_key();
-        let value2 = canister_migrations.encode_to_vec();
-
-        snapshot.insert(key2.into_bytes(), value2);
+        snapshot.insert(
+            make_canister_migrations_record_key().into_bytes(),
+            canister_migrations.encode_to_vec(),
+        );
         assert!(check_routing_table_invariants(&snapshot).is_ok());
         assert!(check_canister_migrations_invariants(&snapshot).is_ok());
 
         // Reassign ranges in routing table.
         let routing_table = RoutingTable::try_from(btreemap! {
             CanisterIdRange{ start: CanisterId::from(0x0), end: CanisterId::from(0xf) } => subnet_test_id(1),
-        CanisterIdRange{ start: CanisterId::from(0x10), end: CanisterId::from(0x1ff) } => subnet_test_id(2),
-    }).unwrap();
+            CanisterIdRange{ start: CanisterId::from(0x10), end: CanisterId::from(0x1ff) } => subnet_test_id(2),
+        }).unwrap();
 
-        let routing_table = PbRoutingTable::from(routing_table);
-        let key3 = make_routing_table_record_key();
-        let value3 = routing_table.encode_to_vec();
-        snapshot.insert(key3.into_bytes(), value3);
+        insert_routing_table_to_snapshot(routing_table, &mut snapshot);
         assert!(check_routing_table_invariants(&snapshot).is_ok());
         assert!(check_canister_migrations_invariants(&snapshot).is_ok());
 
         // Complete canister migrations by removing entries.
-        let key4 = make_canister_migrations_record_key();
-        let value4 = PbCanisterMigrations { entries: vec![] }.encode_to_vec();
-
-        snapshot.insert(key4.into_bytes(), value4);
+        snapshot.insert(
+            make_canister_migrations_record_key().into_bytes(),
+            PbCanisterMigrations { entries: vec![] }.encode_to_vec(),
+        );
         assert!(check_routing_table_invariants(&snapshot).is_ok());
         assert!(check_canister_migrations_invariants(&snapshot).is_ok());
     }
@@ -228,16 +225,12 @@ mod tests {
 
         // The routing table before canister migration.
         let routing_table = RoutingTable::try_from(btreemap! {
-        CanisterIdRange{ start: CanisterId::from(0x0), end: CanisterId::from(0xff) } => subnet_test_id(1),
-        CanisterIdRange{ start: CanisterId::from(0x100), end: CanisterId::from(0x1ff) } => subnet_test_id(2),
-        CanisterIdRange{ start: CanisterId::from(0x200), end: CanisterId::from(0x2ff) } => subnet_test_id(3),
-    }).unwrap();
-
-        let routing_table = PbRoutingTable::from(routing_table);
-        let key1 = make_routing_table_record_key();
-        let value1 = routing_table.encode_to_vec();
+            CanisterIdRange{ start: CanisterId::from(0x0), end: CanisterId::from(0xff) } => subnet_test_id(1),
+            CanisterIdRange{ start: CanisterId::from(0x100), end: CanisterId::from(0x1ff) } => subnet_test_id(2),
+            CanisterIdRange{ start: CanisterId::from(0x200), end: CanisterId::from(0x2ff) } => subnet_test_id(3),
+        }).unwrap();
 
-        snapshot.insert(key1.into_bytes(), value1);
+        insert_routing_table_to_snapshot(routing_table, &mut snapshot);
         assert!(check_routing_table_invariants(&snapshot).is_ok());
         assert!(check_canister_migrations_invariants(&snapshot).is_ok());
 
@@ -249,10 +242,10 @@ mod tests {
     }).unwrap();
 
         let canister_migrations = PbCanisterMigrations::from(canister_migrations);
-        let key2 = make_canister_migrations_record_key();
-        let value2 = canister_migrations.encode_to_vec();
-
-        snapshot.insert(key2.into_bytes(), value2);
+        snapshot.insert(
+            make_canister_migrations_record_key().into_bytes(),
+            canister_migrations.encode_to_vec(),
+        );
 
         assert!(check_routing_table_invariants(&snapshot).is_ok());
         assert!(check_canister_migrations_invariants(&snapshot).is_err());
@@ -269,61 +262,60 @@ mod tests {
         CanisterIdRange{ start: CanisterId::from(0x200), end: CanisterId::from(0x2ff) } => subnet_test_id(3),
     }).unwrap();
 
-        let routing_table = PbRoutingTable::from(routing_table);
-        let key1 = make_routing_table_record_key();
-        let value1 = routing_table.encode_to_vec();
-
         // The canister migrations after preparation.
         let canister_migrations = CanisterMigrations::try_from(btreemap! {
         CanisterIdRange{ start: CanisterId::from(0x0), end: CanisterId::from(0xff) } => vec![subnet_test_id(1), subnet_test_id(2)],
     }).unwrap();
 
         let canister_migrations = PbCanisterMigrations::from(canister_migrations);
-        let key2 = make_canister_migrations_record_key();
-        let value2 = canister_migrations.encode_to_vec();
 
-        snapshot.insert(key1.into_bytes(), value1);
-        snapshot.insert(key2.into_bytes(), value2);
+        insert_routing_table_to_snapshot(routing_table, &mut snapshot);
+        snapshot.insert(
+            make_canister_migrations_record_key().into_bytes(),
+            canister_migrations.encode_to_vec(),
+        );
         assert!(check_routing_table_invariants(&snapshot).is_ok());
         assert!(check_canister_migrations_invariants(&snapshot).is_ok());
 
         // The new routing table after reassigning ranges.
 
         // Case 1: cannot find the entry containing `range.start` of canister migrations when looking up entries in the routing table,
-        let mut new_snapshot = snapshot.clone();
         let new_routing_table_1 = RoutingTable::try_from(btreemap! {
         CanisterIdRange{ start: CanisterId::from(0x1), end: CanisterId::from(0xff) } => subnet_test_id(1),
         CanisterIdRange{ start: CanisterId::from(0x100), end: CanisterId::from(0x1ff) } => subnet_test_id(2),
         CanisterIdRange{ start: CanisterId::from(0x200), end: CanisterId::from(0x2ff) } => subnet_test_id(3),
     }).unwrap();
 
-        let routing_table = PbRoutingTable::from(new_routing_table_1);
-        let key3 = make_routing_table_record_key();
-        let value3 = routing_table.encode_to_vec();
-        new_snapshot.insert(key3.into_bytes(), value3);
+        insert_routing_table_to_snapshot(new_routing_table_1, &mut snapshot);
 
-        assert!(check_routing_table_invariants(&new_snapshot).is_ok());
-        assert!(check_canister_migrations_invariants(&new_snapshot).is_err());
+        assert!(check_routing_table_invariants(&snapshot).is_ok());
+        assert!(check_canister_migrations_invariants(&snapshot).is_err());
 
         // Case 2: find the entry containing `range.start` but the entry cannot fully cover the canister migration range {0x0:0xff}.
         let new_routing_table_2 = RoutingTable::try_from(btreemap! {
-        CanisterIdRange{ start: CanisterId::from(0x0), end: CanisterId::from(0x1) } => subnet_test_id(1),
-        CanisterIdRange{ start: CanisterId::from(0x100), end: CanisterId::from(0x1ff) } => subnet_test_id(2),
-        CanisterIdRange{ start: CanisterId::from(0x200), end: CanisterId::from(0x2ff) } => subnet_test_id(3),
-    }).unwrap();
+            CanisterIdRange{ start: CanisterId::from(0x0), end: CanisterId::from(0x1) } => subnet_test_id(1),
+            CanisterIdRange{ start: CanisterId::from(0x100), end: CanisterId::from(0x1ff) } => subnet_test_id(2),
+            CanisterIdRange{ start: CanisterId::from(0x200), end: CanisterId::from(0x2ff) } => subnet_test_id(3),
+        }).unwrap();
 
         let routing_table = PbRoutingTable::from(new_routing_table_2);
-        let key4 = make_routing_table_record_key();
-        let value4 = routing_table.encode_to_vec();
-        snapshot.insert(key4.into_bytes(), value4);
+        snapshot.insert(
+            make_routing_table_record_key().into_bytes(),
+            routing_table.encode_to_vec(),
+        );
+        snapshot.insert(
+            make_canister_ranges_key(CanisterId::from(0)).into_bytes(),
+            routing_table.encode_to_vec(),
+        );
 
         assert!(check_routing_table_invariants(&snapshot).is_ok());
         assert!(check_canister_migrations_invariants(&snapshot).is_err());
     }
 
+    // TODO(NNS1-3781): Remove this test once we have sharded table supported by all clients.
     #[test]
     #[should_panic(expected = "Routing tables from shards and routing table record do not match.")]
-    fn if_sharded_ranges_they_must_match_original_routing_table() {
+    fn sharded_ranges_must_match_original_routing_table() {
         let mut snapshot = RegistrySnapshot::new();
 
         // The routing table before canister migration.
@@ -334,20 +326,22 @@ mod tests {
          }).unwrap();
 
         let routing_table = PbRoutingTable::from(routing_table);
-        let key1 = make_routing_table_record_key();
-        let value1 = routing_table.encode_to_vec();
 
         let rt_shard = RoutingTable::try_from(btreemap! {
             CanisterIdRange{ start: CanisterId::from(0x0), end: CanisterId::from(0xff) } => subnet_test_id(1),
             CanisterIdRange{ start: CanisterId::from(0x100), end: CanisterId::from(0x1ff) } => subnet_test_id(2),
         }).unwrap();
 
         let rt_shard = PbRoutingTable::from(rt_shard);
-        let key2 = make_canister_ranges_key(CanisterId::from_u64(0x0));
-        let value2 = rt_shard.encode_to_vec();
 
-        snapshot.insert(key1.into_bytes(), value1);
-        snapshot.insert(key2.into_bytes(), value2);
+        snapshot.insert(
+            make_routing_table_record_key().into_bytes(),
+            routing_table.encode_to_vec(),
+        );
+        snapshot.insert(
+            make_canister_ranges_key(CanisterId::from(0)).into_bytes(),
+            rt_shard.encode_to_vec(),
+        );
 
         check_routing_table_invariants(&snapshot).unwrap();
     }