Merge branch 'eichhorl/fetch-complaints' into 'master'

fix(ecdsa): CON-1039 Do not drop complaints/openings for requested transcripts

Let [A, B, C, D] be a subnet.

Let `h_r` be the height at which transcripts `t1`, `t2` are requested (i.a. their configs are written into the block and the id is added to `requested_transcripts`).

Assume `t1` and `t2` are completed at a height `h_c > h_r`. They are added to `active_transcripts` and a new config for a transcript `t'` building on `t1` and `t2` is created. 

Assume further that node A (B) was unable to support `t1` (`t2`) due to a corrupted dealing. When A (B) tries to create a dealing for `t'`, it is unable to load `t1` (`t2`), thus proceeds by sending a complaint for `t1` (`t2`).

Now assume that nodes C and D are slightly behind at a height `h` with `h_c > h >= h_r`. They receive A and B's complaints for `t1` and `t2`, but as the transcripts are not yet completed (`h_c > h`), in the current implementation, C and D will drop the adverts of the complaints and never receive them again or send the corresponding openings. Therefore, A and B are unable to create a dealing for `t'` and the transcript is stuck.

With this MR we fetch Complaints/Openings even if the corresponding transcript is requested but not yet completed. As processing Complaints/Openings without the completed transcript is impossible, we defer processing until later, when the transcript has moved from `requested` to `active`.

Additionally:
- Stop trying to create openings for own complaints. This fails when calling the crypto function and creates unnecessary logs and overhead. 

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

Author: eichhorl

rs/consensus/src/ecdsa.rs

@@ -520,7 +520,9 @@ fn compute_priority(
         | EcdsaMessageAttribute::EcdsaOpening(transcript_id) => {
             let height = transcript_id.source_height();
             if height <= args.finalized_height {
-                if args.active_transcripts.contains(transcript_id) {
+                if args.active_transcripts.contains(transcript_id)
+                    || args.requested_transcripts.contains(transcript_id)
+                {
                     Priority::Fetch
                 } else {
                     metrics
@@ -695,15 +697,18 @@ mod tests {
         let transcript_id_drop = IDkgTranscriptId::new(subnet_id, 2, Height::from(70));
         let transcript_id_fetch_2 = IDkgTranscriptId::new(subnet_id, 3, Height::from(102));
         let transcript_id_stash = IDkgTranscriptId::new(subnet_id, 4, Height::from(200));
+        let transcript_id_fetch_3 = IDkgTranscriptId::new(subnet_id, 5, Height::from(80));
 
         let metrics_registry = MetricsRegistry::new();
         let metrics = EcdsaGossipMetrics::new(metrics_registry);
 
         let mut active_transcripts = BTreeSet::new();
         active_transcripts.insert(transcript_id_fetch_1);
+        let mut requested_transcripts = BTreeSet::new();
+        requested_transcripts.insert(transcript_id_fetch_3);
         let args = EcdsaPriorityFnArgs {
             finalized_height: Height::from(100),
-            requested_transcripts: BTreeSet::new(),
+            requested_transcripts,
             requested_signatures: BTreeSet::new(),
             active_transcripts,
         };
@@ -726,6 +731,10 @@ mod tests {
                 EcdsaMessageAttribute::EcdsaComplaint(transcript_id_stash),
                 Priority::Stash,
             ),
+            (
+                EcdsaMessageAttribute::EcdsaComplaint(transcript_id_fetch_3),
+                Priority::Fetch,
+            ),
             // Openings
             (
                 EcdsaMessageAttribute::EcdsaOpening(transcript_id_fetch_1),
@@ -743,6 +752,10 @@ mod tests {
                 EcdsaMessageAttribute::EcdsaOpening(transcript_id_stash),
                 Priority::Stash,
             ),
+            (
+                EcdsaMessageAttribute::EcdsaOpening(transcript_id_fetch_3),
+                Priority::Fetch,
+            ),
         ];
 
         for (attr, expected) in tests {

rs/consensus/src/ecdsa/complaints.rs

@@ -100,6 +100,7 @@ impl EcdsaComplaintHandlerImpl {
         block_reader: &dyn EcdsaBlockReader,
     ) -> EcdsaChangeSet {
         let active_transcripts = self.active_transcripts(block_reader);
+        let requested_transcripts = self.requested_transcripts(block_reader);
 
         // Collection of validated complaints <complainer Id, transcript Id, dealer Id>
         let mut validated_complaints = BTreeSet::new();
@@ -125,6 +126,7 @@ impl EcdsaComplaintHandlerImpl {
             match Action::action(
                 block_reader,
                 &active_transcripts,
+                &requested_transcripts,
                 complaint.idkg_complaint.transcript_id.source_height(),
                 &complaint.idkg_complaint.transcript_id,
             ) {
@@ -182,6 +184,10 @@ impl EcdsaComplaintHandlerImpl {
             .validated()
             .complaints()
             .filter(|(_, signed_complaint)| {
+                // Do not try to create openings for own complaints.
+                if signed_complaint.signature.signer == self.node_id {
+                    return false;
+                }
                 let complaint = signed_complaint.get();
                 !self.has_node_issued_opening(
                     ecdsa_pool,
@@ -217,6 +223,7 @@ impl EcdsaComplaintHandlerImpl {
         block_reader: &dyn EcdsaBlockReader,
     ) -> EcdsaChangeSet {
         let active_transcripts = self.active_transcripts(block_reader);
+        let requested_transcripts = self.requested_transcripts(block_reader);
 
         // Collection of validated openings <opener Id, transcript Id, dealer Id>
         let mut validated_openings = BTreeSet::new();
@@ -240,6 +247,7 @@ impl EcdsaComplaintHandlerImpl {
             match Action::action(
                 block_reader,
                 &active_transcripts,
+                &requested_transcripts,
                 opening.idkg_opening.transcript_id.source_height(),
                 &opening.idkg_opening.transcript_id,
             ) {
@@ -749,6 +757,17 @@ impl EcdsaComplaintHandlerImpl {
             .map(|transcript_ref| (transcript_ref.transcript_id, *transcript_ref))
             .collect::<BTreeMap<_, _>>()
     }
+
+    /// Returns the requested transcript map.
+    fn requested_transcripts(
+        &self,
+        block_reader: &dyn EcdsaBlockReader,
+    ) -> BTreeSet<IDkgTranscriptId> {
+        block_reader
+            .requested_transcripts()
+            .map(|transcript_ref| transcript_ref.transcript_id)
+            .collect::<BTreeSet<_>>()
+    }
 }
 
 impl EcdsaComplaintHandler for EcdsaComplaintHandlerImpl {
@@ -922,6 +941,7 @@ impl<'a> Action<'a> {
     fn action(
         block_reader: &'a dyn EcdsaBlockReader,
         active_transcripts: &'a BTreeMap<IDkgTranscriptId, TranscriptRef>,
+        requested_transcripts: &'a BTreeSet<IDkgTranscriptId>,
         msg_height: Height,
         msg_transcript_id: &IDkgTranscriptId,
     ) -> Action<'a> {
@@ -931,10 +951,17 @@ impl<'a> Action<'a> {
             return Action::Defer;
         }
 
-        match active_transcripts.get(msg_transcript_id) {
-            Some(transcript_ref) => Action::Process(transcript_ref),
-            None => Action::Drop,
+        if let Some(transcript_ref) = active_transcripts.get(msg_transcript_id) {
+            return Action::Process(transcript_ref);
+        }
+
+        // The transcript is not yet completed on this node, process
+        // the message later when it is.
+        if requested_transcripts.contains(msg_transcript_id) {
+            return Action::Defer;
         }
+
+        Action::Drop
     }
 }
 
@@ -955,44 +982,53 @@ mod tests {
     // Tests the Action logic
     #[test]
     fn test_ecdsa_complaint_action() {
-        let (id_1, id_2, id_3, id_4) = (
+        let (id_1, id_2, id_3, id_4, id_5) = (
             create_transcript_id(1),
             create_transcript_id(2),
             create_transcript_id(3),
             create_transcript_id(4),
+            create_transcript_id(5),
         );
 
         let ref_1 = TranscriptRef::new(Height::new(10), id_1);
         let ref_2 = TranscriptRef::new(Height::new(20), id_2);
         let block_reader =
             TestEcdsaBlockReader::for_complainer_test(Height::new(100), vec![ref_1, ref_2]);
-        let mut active_transcripts = BTreeMap::new();
-        active_transcripts.insert(id_1, ref_1);
-        active_transcripts.insert(id_2, ref_2);
+        let mut active = BTreeMap::new();
+        active.insert(id_1, ref_1);
+        active.insert(id_2, ref_2);
+        let mut requested = BTreeSet::new();
+        requested.insert(id_5);
 
         // Message from a node ahead of us
         assert_eq!(
-            Action::action(&block_reader, &active_transcripts, Height::from(200), &id_3),
+            Action::action(&block_reader, &active, &requested, Height::from(200), &id_3),
             Action::Defer
         );
 
         // Messages for transcripts not currently active
         assert_eq!(
-            Action::action(&block_reader, &active_transcripts, Height::from(10), &id_3),
+            Action::action(&block_reader, &active, &requested, Height::from(10), &id_3),
             Action::Drop
         );
         assert_eq!(
-            Action::action(&block_reader, &active_transcripts, Height::from(20), &id_4),
+            Action::action(&block_reader, &active, &requested, Height::from(20), &id_4),
             Action::Drop
         );
 
+        // Messages for transcripts not currently active but requested
+        assert_eq!(
+            Action::action(&block_reader, &active, &requested, Height::from(10), &id_5),
+            Action::Defer
+        );
+
         // Messages for transcripts currently active
         assert!(matches!(
-            Action::action(&block_reader, &active_transcripts, Height::from(10), &id_1),
+            Action::action(&block_reader, &active, &requested, Height::from(10), &id_1),
             Action::Process(_)
         ));
         assert!(matches!(
-            Action::action(&block_reader, &active_transcripts, Height::from(20), &id_2),
+            Action::action(&block_reader, &active, &requested, Height::from(20), &id_2),
             Action::Process(_)
         ));
     }