fix(consensus): [CON-1387] Use correct signer id in make_next_block_w…

…ith_rank (#644)

Currently, the test utility function `make_next_block_with_rank` creates
block proposals with an incorrect signer id (it defaults to
`node_test_id(0)`). This was hiding a few errors in our unit tests.

This PR assigns the correct signer id in proposals, and fixes several
issues in our unit tests.

rs/artifact_pool/src/consensus_pool.rs

@@ -2190,18 +2190,18 @@ mod tests {
             check_iterator(&pool, pool.as_cache().finalized_block(), vec![2, 1, 0]);
 
             // Two notarized rounds added
+            pool.insert_validated(pool.make_next_beacon());
             let block = pool.make_next_block();
             pool.insert_validated(block.clone());
             pool.notarize(&block);
 
+            pool.insert_validated(pool.make_next_beacon());
             let block = pool.make_next_block();
             pool.insert_validated(block.clone());
             pool.notarize(&block);
             check_iterator(&pool, block.clone().into(), vec![4, 3, 2, 1, 0]);
 
             pool.finalize(&block);
-            pool.insert_validated(pool.make_next_beacon());
-            pool.insert_validated(pool.make_next_beacon());
             pool.insert_validated(pool.make_next_tape());
             pool.insert_validated(pool.make_next_tape());
             check_iterator(&pool, block.into(), vec![4, 3, 2, 1, 0]);

rs/consensus/src/consensus/priority.rs

@@ -203,6 +203,7 @@ mod tests {
             let expected_batch_height = Height::from(1);
             let priority = get_priority_function(&pool, expected_batch_height);
             // New block ==> FetchNow
+            pool.insert_validated(pool.make_next_beacon());
             let block = pool.make_next_block();
             assert_eq!(priority(&block.get_id(), &()), FetchNow);
 
@@ -267,6 +268,7 @@ mod tests {
 
             // Add notarizations until we reach finalized_height + LOOK_AHEAD.
             for _ in 0..LOOK_AHEAD {
+                pool.insert_validated(pool.make_next_beacon());
                 let block = pool.make_next_block();
                 pool.insert_validated(block.clone());
                 let notarization = Notarization::fake(NotarizationContent::new(
@@ -276,6 +278,7 @@ mod tests {
                 pool.insert_validated(notarization.clone());
             }
             // Insert one more block
+            pool.insert_validated(pool.make_next_beacon());
             let block = pool.make_next_block();
             pool.insert_validated(block.clone());
             let notarization = Notarization::fake(NotarizationContent::new(

rs/consensus/src/consensus/purger.rs

@@ -638,7 +638,7 @@ mod tests {
                 replica_config,
                 registry,
                 ..
-            } = dependencies(pool_config, 1);
+            } = dependencies(pool_config, 10);
             state_manager
                 .get_mut()
                 .expect_latest_state_height()
@@ -658,13 +658,15 @@ mod tests {
 
             // Height 1 - two block proposals, one notarization, one finalization.
             // We will later instruct purger not to consider this height.
+            pool.insert_validated(pool.make_next_beacon());
             let finalized_block_proposal_1 = pool.make_next_block_with_rank(Rank(0));
             let non_finalized_block_proposal_1 = pool.make_next_block_with_rank(Rank(1));
             pool.insert_validated(finalized_block_proposal_1.clone());
             pool.insert_validated(non_finalized_block_proposal_1.clone());
             pool.notarize(&finalized_block_proposal_1);
             pool.finalize(&finalized_block_proposal_1);
             // Height 2 - three block proposals, two notarizations, one finalization
+            pool.insert_validated(pool.make_next_beacon());
             let finalized_block_proposal_2 = pool.make_next_block_with_rank(Rank(0));
             let non_finalized_block_proposal_2_0 = pool.make_next_block_with_rank(Rank(1));
             let non_finalized_block_proposal_2_1 = pool.make_next_block_with_rank(Rank(2));
@@ -676,6 +678,7 @@ mod tests {
             pool.finalize(&finalized_block_proposal_2);
             // Height 3 - two block proposals, two notarizations, no finalizations.
             // The purger should not consider this height as it hasn't been finalized yet.
+            pool.insert_validated(pool.make_next_beacon());
             let finalized_block_proposal_3_0 = pool.make_next_block_with_rank(Rank(0));
             let finalized_block_proposal_3_1 = pool.make_next_block_with_rank(Rank(1));
             pool.insert_validated(finalized_block_proposal_3_0.clone());

rs/consensus/utils/src/pool_reader.rs

@@ -736,6 +736,7 @@ pub mod test {
         ic_test_utilities::artifact_pool_config::with_test_pool_config(|pool_config| {
             let Dependencies { mut pool, .. } = dependencies(pool_config, 1);
             let start = pool.make_next_block();
+            pool.insert_beacon_chain(&pool.make_next_beacon(), Height::from(10));
             pool.insert_block_chain_with(start.clone(), Height::from(10));
             let ten_block = pool
                 .validated()
@@ -791,19 +792,22 @@ pub mod test {
     // sorted in ascending order.
     fn test_get_by_height_range() {
         ic_test_utilities::artifact_pool_config::with_test_pool_config(|pool_config| {
-            let Dependencies { mut pool, .. } = dependencies(pool_config, 1);
-            // Let's generate 4 proposals so that 2 of them end up in the
-            // unvalidated pool and have the same height.
             let rounds = 30;
-            let replicas = 3;
+            let replicas = 10;
+            let f = 3;
+            let Dependencies { mut pool, .. } = dependencies(pool_config, replicas);
+
+            // Because `TestConsensusPool::advance_round` alternates between
+            // putting blocks in validated and unvalidated pools for each rank,
+            // we expect (f+1)/2 blocks in the unvalidated pool per round.
             let mut round = pool
                 .prepare_round()
-                .with_replicas(replicas)
-                .with_new_block_proposals(replicas + 1)
-                .with_random_beacon_shares(replicas)
-                .with_notarization_shares(replicas)
-                .with_finalization_shares(replicas);
-            // Grow the artifact pool for `rounds` mimicking a subnet with 3 replicas.
+                .with_replicas(replicas as u32)
+                .with_new_block_proposals(f + 1)
+                .with_random_beacon_shares(replicas as u32)
+                .with_notarization_shares(replicas as u32)
+                .with_finalization_shares(replicas as u32);
+            // Grow the artifact pool for `rounds`.
             for _ in 0..rounds {
                 round.advance();
             }
@@ -828,9 +832,9 @@ pub mod test {
                     Height::from((rounds * 2) as u64),
                 ))
                 .collect::<Vec<_>>();
-            // We expect to see `2*rounds` unvalidated block proposals sorted by
+            // We expect to see `rounds * ((f+1)/2)` unvalidated block proposals sorted by
             // height in ascending order.
-            assert_eq!(artifacts.len(), 2 * rounds);
+            assert_eq!(artifacts.len(), rounds * ((f as usize + 1) / 2));
             for i in 0..artifacts.len() - 1 {
                 // Heights are ascending, but NOT unique.
                 assert!(artifacts[i].content.height() <= artifacts[i + 1].content.height());

rs/test_utilities/artifact_pool/src/consensus_pool.rs

@@ -256,7 +256,8 @@ impl TestConsensusPool {
         block.context.registry_version = registry_version;
         let dkg_payload = (self.dkg_payload_builder)(self, parent.clone(), &block.context);
         block.payload = Payload::new(ic_types::crypto::crypto_hash, dkg_payload.into());
-        BlockProposal::fake(block, node_test_id(0))
+        let signer = self.get_block_maker_by_rank(block.height(), rank);
+        BlockProposal::fake(block, signer)
     }
 
     pub fn make_next_beacon(&self) -> RandomBeacon {