refactor(crypto): CRP-1447 make IDkgReceivers::position() private

Author: altkdf

rs/consensus/src/ecdsa/pre_signer.rs

@@ -278,8 +278,7 @@ impl EcdsaPreSignerImpl {
                     self.resolve_ref(transcript_params_ref, block_reader, "send_dealing_support")
                         .and_then(|transcript_params| {
                             transcript_params
-                                .receivers()
-                                .position(self.node_id)
+                                .receiver_index(self.node_id)
                                 .map(|_| (id, transcript_params, signed_dealing))
                         })
                 } else {
@@ -404,10 +403,9 @@ impl EcdsaPreSignerImpl {
                         }
                     };
 
-                    if transcript_params
+                    if !transcript_params
                         .receivers()
-                        .position(support.sig_share.signer)
-                        .is_none()
+                        .contains(support.sig_share.signer)
                     {
                         // The node is not in the receiver list for this transcript,
                         // support share is not expected from it

rs/crypto/src/sign/canister_threshold_sig/ecdsa.rs

@@ -17,7 +17,7 @@ use ic_types::crypto::canister_threshold_sig::{
 use ic_types::crypto::AlgorithmId;
 use ic_types::{NodeId, NodeIndex};
 use serde::{Deserialize, Serialize};
-use std::collections::{BTreeMap, BTreeSet};
+use std::collections::BTreeMap;
 use std::convert::TryFrom;
 
 #[cfg(test)]
@@ -43,7 +43,7 @@ pub fn sign_share<C: CspThresholdEcdsaSigner>(
     self_node_id: &NodeId,
     inputs: &ThresholdEcdsaSigInputs,
 ) -> Result<ThresholdEcdsaSigShare, ThresholdEcdsaSignShareError> {
-    ensure_self_was_receiver(self_node_id, inputs.receivers().get())?;
+    ensure_self_was_receiver(self_node_id, inputs.receivers())?;
 
     let internal_sig_share = csp_client.ecdsa_sign_share(inputs)?;
 
@@ -166,7 +166,7 @@ pub fn combine_sig_shares<C: CspThresholdEcdsaSigVerifier>(
     let kappa_unmasked =
         IDkgTranscriptInternal::deserialize(kappa_transcript).map_err(conv_error)?;
 
-    let internal_shares = internal_sig_shares_by_index_from_sig_shares(shares, inputs.receivers())?;
+    let internal_shares = internal_sig_shares_by_index_from_sig_shares(shares, inputs)?;
 
     let key = IDkgTranscriptInternal::try_from(inputs.key_transcript()).map_err(conv_error)?;
 
@@ -219,9 +219,9 @@ pub fn get_tecdsa_master_public_key(
 
 fn ensure_self_was_receiver(
     self_node_id: &NodeId,
-    receivers: &BTreeSet<NodeId>,
+    receivers: &IDkgReceivers,
 ) -> Result<(), ThresholdEcdsaSignShareError> {
-    if receivers.contains(self_node_id) {
+    if receivers.contains(*self_node_id) {
         Ok(())
     } else {
         Err(ThresholdEcdsaSignShareError::NotAReceiver)
@@ -248,14 +248,14 @@ fn ensure_sufficient_sig_shares_collected(
 /// and map them by signer index (rather than signer Id).
 fn internal_sig_shares_by_index_from_sig_shares(
     shares: &BTreeMap<NodeId, ThresholdEcdsaSigShare>,
-    receivers: &IDkgReceivers,
+    inputs: &ThresholdEcdsaSigInputs,
 ) -> Result<BTreeMap<NodeIndex, ThresholdEcdsaSigShareInternal>, ThresholdEcdsaCombineSigSharesError>
 {
     shares
         .iter()
         .map(|(&id, share)| {
-            let index = receivers
-                .position(id)
+            let index = inputs
+                .index_for_signer_id(id)
                 .ok_or(ThresholdEcdsaCombineSigSharesError::SignerNotAllowed { node_id: id })?;
             let internal_share = ThresholdEcdsaSigShareInternal::deserialize(&share.sig_share_raw)
                 .map_err(

rs/crypto/src/sign/canister_threshold_sig/idkg/complaint.rs

@@ -49,7 +49,7 @@ fn index_of_complainer(
     complainer_id: NodeId,
     transcript: &IDkgTranscript,
 ) -> Result<NodeIndex, IDkgVerifyComplaintError> {
-    transcript.receivers.position(complainer_id).ok_or(
+    transcript.index_for_signer_id(complainer_id).ok_or(
         IDkgVerifyComplaintError::InvalidArgumentMissingComplainerInTranscript { complainer_id },
     )
 }

rs/crypto/src/sign/canister_threshold_sig/idkg/transcript.rs

@@ -148,7 +148,7 @@ pub fn load_transcript<C: CspIDkgProtocol>(
     registry: &dyn RegistryClient,
     transcript: &IDkgTranscript,
 ) -> Result<Vec<IDkgComplaint>, IDkgLoadTranscriptError> {
-    let self_index = match transcript.receivers.position(*self_node_id) {
+    let self_index = match transcript.index_for_signer_id(*self_node_id) {
         Some(index) => index,
         None => {
             return Ok(vec![]); // This is not a receiver: nothing to do.
@@ -180,7 +180,7 @@ pub fn load_transcript_with_openings<C: CspIDkgProtocol>(
     transcript: &IDkgTranscript,
     openings: &BTreeMap<IDkgComplaint, BTreeMap<NodeId, IDkgOpening>>,
 ) -> Result<(), IDkgLoadTranscriptError> {
-    let self_index = match transcript.receivers.position(*self_node_id) {
+    let self_index = match transcript.index_for_signer_id(*self_node_id) {
         Some(index) => index,
         None => {
             return Ok(()); // This is not a receiver: nothing to do.
@@ -199,7 +199,7 @@ pub fn load_transcript_with_openings<C: CspIDkgProtocol>(
     for (complaint, openings_by_opener_id) in openings {
         let mut internal_openings_by_opener_index = BTreeMap::new();
         for (opener_id, opening) in openings_by_opener_id {
-            let opener_index = transcript.receivers.position(*opener_id).ok_or_else(|| {
+            let opener_index = transcript.index_for_signer_id(*opener_id).ok_or_else(|| {
                 IDkgLoadTranscriptError::InvalidArguments {
                     internal_error: format!(
                         "invalid opener: node with ID {:?} is not a receiver",
@@ -266,7 +266,7 @@ pub fn open_transcript<C: CspIDkgProtocol>(
     let (dealer_index, signed_dealing) =
         index_and_batch_signed_dealing_of_dealer(complaint.dealer_id, transcript)?;
     let context_data = transcript.context_data();
-    let opener_index = match transcript.receivers.position(*self_node_id) {
+    let opener_index = match transcript.index_for_signer_id(*self_node_id) {
         None => {
             return Err(IDkgOpenTranscriptError::InternalError {
                 internal_error: "This node is not a receiver of the given transcript".to_string(),
@@ -317,8 +317,7 @@ pub fn verify_opening<C: CspIDkgProtocol>(
     // Extract the accused dealing from the transcript
     let (_, internal_dealing) = index_and_dealing_of_dealer(complaint.dealer_id, transcript)?;
     let opener_index = transcript
-        .receivers
-        .position(opener_id)
+        .index_for_signer_id(opener_id)
         .ok_or(IDkgVerifyOpeningError::MissingOpenerInReceivers { opener_id })?;
     let internal_opening = CommitmentOpening::try_from(opening).map_err(|e| {
         IDkgVerifyOpeningError::InternalError {
@@ -362,7 +361,7 @@ fn ensure_signers_allowed_by_params(
 ) -> Result<(), IDkgCreateTranscriptError> {
     for dealing in dealings {
         for signer in dealing.signers() {
-            if !params.receivers().get().contains(&signer) {
+            if !params.receivers().contains(signer) {
                 return Err(IDkgCreateTranscriptError::SignerNotAllowed { node_id: signer });
             }
         }

rs/crypto/test_utils/canister_threshold_sigs/src/lib.rs

@@ -751,15 +751,15 @@ pub mod node {
         ) -> impl Iterator<Item = Node> + 'a {
             self.nodes
                 .into_iter()
-                .filter(move |node| idkg_receivers.as_ref().get().contains(&node.id))
+                .filter(move |node| idkg_receivers.as_ref().contains(node.id))
         }
 
         pub fn receivers<'a, T: AsRef<IDkgReceivers> + 'a>(
             &'a self,
             idkg_receivers: T,
         ) -> impl Iterator<Item = &Node> + 'a {
             self.iter()
-                .filter(move |node| idkg_receivers.as_ref().get().contains(&node.id))
+                .filter(move |node| idkg_receivers.as_ref().contains(node.id))
         }
 
         pub fn dealers<'a, T: AsRef<IDkgDealers> + 'a>(
@@ -1456,7 +1456,7 @@ pub fn corrupt_signed_idkg_dealing<R: CryptoRng + RngCore, T: BasicSigner<IDkgDe
     let receiver =
         random_receiver_id_excluding_set(transcript_params.receivers(), excluded_receivers, rng)
             .ok_or(CorruptSignedIDkgDealingError::NoReceivers)?;
-    let node_index = transcript_params.receivers().position(*receiver).unwrap();
+    let node_index = transcript_params.receiver_index(*receiver).unwrap();
 
     Ok(idkg_dealing
         .into_builder()

rs/crypto/tests/canister_threshold_sigs.rs

@@ -3737,7 +3737,7 @@ mod verify_opening {
             let verifier = env.nodes.random_receiver(&transcript.receivers, rng);
             let wrong_opener_id = node_id(123456789);
             assert!(
-                !transcript.receivers.get().contains(&wrong_opener_id),
+                !transcript.receivers.contains(wrong_opener_id),
                 "Wrong opener_id unexpectedly in receivers"
             );
             let result =

rs/types/types/src/crypto/canister_threshold_sig.rs

@@ -6,7 +6,8 @@ use crate::crypto::canister_threshold_sig::idkg::{
 use crate::crypto::AlgorithmId;
 use crate::{NumberOfNodes, Randomness};
 use core::fmt;
-use ic_base_types::PrincipalId;
+use ic_base_types::{NodeId, PrincipalId};
+use ic_crypto_internal_types::NodeIndex;
 #[cfg(test)]
 use ic_exhaustive_derive::ExhaustiveSet;
 use serde::{Deserialize, Serialize};
@@ -481,6 +482,10 @@ impl ThresholdEcdsaSigInputs {
             ),
         }
     }
+
+    pub fn index_for_signer_id(&self, node_id: NodeId) -> Option<NodeIndex> {
+        self.key_transcript().index_for_signer_id(node_id)
+    }
 }
 
 /// A single threshold ECDSA signature share.

rs/types/types/src/crypto/canister_threshold_sig/idkg.rs

@@ -163,13 +163,21 @@ impl IDkgReceivers {
 
     /// Returns the position of the given `node_id` in the receivers. Returns
     /// `None` if the `node_id` is not a receiver.
-    pub fn position(&self, node_id: NodeId) -> Option<NodeIndex> {
+    ///
+    /// This method is intended to be PRIVATE. For public methods for obtaining
+    /// a receiver's index, the methods of the objects like [`IDkgTranscript`],
+    /// [`IDkgTranscriptParams`], or [`ThresholdEcdsaSigInputs`] should be used.
+    fn position(&self, node_id: NodeId) -> Option<NodeIndex> {
         self.receivers
             .iter()
             .position(|receiver| node_id == *receiver)
             .map(|index| NodeIndex::try_from(index).expect("node index overflow"))
     }
 
+    pub fn contains(&self, node_id: NodeId) -> bool {
+        self.receivers.contains(&node_id)
+    }
+
     pub fn get(&self) -> &BTreeSet<NodeId> {
         &self.receivers
     }
@@ -407,15 +415,15 @@ impl IDkgTranscriptParams {
         match &self.operation_type {
             IDkgTranscriptOperation::Random => Some(index),
             IDkgTranscriptOperation::ReshareOfMasked(transcript) => {
-                transcript.receivers.position(node_id)
+                transcript.index_for_signer_id(node_id)
             }
             IDkgTranscriptOperation::ReshareOfUnmasked(transcript) => {
-                transcript.receivers.position(node_id)
+                transcript.index_for_signer_id(node_id)
             }
             IDkgTranscriptOperation::UnmaskedTimesMasked(transcript_1, _transcript_2) => {
                 // transcript_1.receivers == transcript_2.receivers already checked by
                 // IDkgTranscriptParams::new
-                transcript_1.receivers.position(node_id)
+                transcript_1.index_for_signer_id(node_id)
             }
         }
     }
@@ -936,7 +944,7 @@ impl IDkgTranscript {
 
     /// Checks if the specified `NodeId` is a receiver of the transcript.
     pub fn has_receiver(&self, receiver_id: NodeId) -> bool {
-        self.receivers.position(receiver_id).is_some()
+        self.receivers.contains(receiver_id)
     }
 
     /// Returns a copy of the raw internal transcript.

rs/types/types/src/crypto/canister_threshold_sig/idkg/tests/receivers.rs

@@ -91,6 +91,30 @@ fn should_return_none_position_if_node_id_not_in_receivers() {
     assert!(receivers.position(not_a_receiver_node).is_none());
 }
 
+#[test]
+fn should_return_contains_false_if_node_id_not_in_receivers() {
+    let not_a_receiver_node = node_id(NODE_3);
+    let mut receivers = BTreeSet::new();
+    receivers.insert(node_id(NODE_1));
+    receivers.insert(node_id(NODE_2));
+    let receivers = IDkgReceivers::new(receivers).unwrap();
+
+    assert!(!receivers.contains(not_a_receiver_node));
+}
+
+#[test]
+fn should_return_contains_true_if_node_id_in_receivers() {
+    let receiver_node_1 = node_id(NODE_1);
+    let receiver_node_2 = node_id(NODE_2);
+    let mut receivers = BTreeSet::new();
+    receivers.insert(receiver_node_1);
+    receivers.insert(receiver_node_2);
+    let receivers = IDkgReceivers::new(receivers).unwrap();
+
+    assert!(receivers.contains(receiver_node_1));
+    assert!(receivers.contains(receiver_node_2));
+}
+
 #[test]
 fn should_return_correct_reconstruction_threshold() {
     let mut receivers = BTreeSet::new();

rs/types/types/src/crypto/canister_threshold_sig/idkg/tests/verify_transcript_params.rs

@@ -211,7 +211,7 @@ fn should_fail_on_ineligible_signer() {
     let rng = &mut reproducible_rng();
     let (mut transcript, params) = valid_transcript_and_params(rng);
     let non_receiver = node_id(99999);
-    assert!(!params.receivers.get().contains(&non_receiver));
+    assert!(!params.receivers.contains(non_receiver));
     let first_dealer_index = *transcript.verified_dealings.keys().next().unwrap();
     transcript
         .verified_dealings

rs/types/types/src/crypto/canister_threshold_sig/tests.rs

@@ -5,7 +5,7 @@ use crate::crypto::canister_threshold_sig::error::{
 use crate::crypto::canister_threshold_sig::idkg::IDkgTranscriptId;
 use crate::{Height, NodeId, RegistryVersion, SubnetId};
 use assert_matches::assert_matches;
-use ic_crypto_test_utils_canister_threshold_sigs::set_of_nodes;
+use ic_crypto_test_utils_canister_threshold_sigs::{node_id, set_of_nodes};
 use ic_crypto_test_utils_reproducible_rng::reproducible_rng;
 use rand::{CryptoRng, Rng};
 use std::collections::{BTreeMap, BTreeSet};
@@ -486,6 +486,42 @@ fn serde_cbor_deserialization_of_extended_derivation_path_is_backward_compatible
     assert_eq!(computed_derivation_path, expected_derivation_path);
 }
 
+#[test]
+fn should_return_correct_index_for_signer_id_from_threshold_ecdsa_sig_inputs() {
+    let rng = &mut reproducible_rng();
+    let common_receivers = set_of_nodes(&[42, 43, 45, 128]);
+    let (kappa_unmasked, lambda_masked, kappa_times_lambda, key_times_lambda, key_transcript) =
+        transcripts_for_ecdsa_inputs(common_receivers, rng);
+
+    let quadruple = PreSignatureQuadruple::new(
+        kappa_unmasked,
+        lambda_masked,
+        kappa_times_lambda,
+        key_times_lambda,
+    );
+    assert!(quadruple.is_ok());
+
+    let derivation_path = derivation_path();
+    let hashed_message = hashed_message();
+    let nonce = nonce();
+    let quadruple = quadruple.unwrap();
+    let inputs = ThresholdEcdsaSigInputs::new(
+        &derivation_path,
+        &hashed_message,
+        nonce,
+        quadruple.clone(),
+        key_transcript.clone(),
+    )
+    .expect("failed to create ThresholdEcdsaSigInputs");
+
+    assert_eq!(inputs.index_for_signer_id(node_id(42)), Some(0));
+    assert_eq!(inputs.index_for_signer_id(node_id(43)), Some(1));
+    assert_eq!(inputs.index_for_signer_id(node_id(44)), None);
+    assert_eq!(inputs.index_for_signer_id(node_id(45)), Some(2));
+    assert_eq!(inputs.index_for_signer_id(node_id(46)), None);
+    assert_eq!(inputs.index_for_signer_id(node_id(128)), Some(3));
+}
+
 // A randomized way to get non-repeating IDs.
 pub fn random_transcript_id<R: Rng + CryptoRng>(rng: &mut R) -> IDkgTranscriptId {
     let id = rng.gen();