From ae59be5466d658086bc4f801e56a76d74ed2ac87 Mon Sep 17 00:00:00 2001
From: Daniel Sanchez <sanchez.quiros.daniel@gmail.com>
Date: Tue, 1 Aug 2023 09:19:29 +0200
Subject: [PATCH] Refactor parent committee methods to return optional parent
 instead of empty one (#282)

---
 consensus-engine/src/lib.rs                   | 12 +++++++---
 consensus-engine/src/overlay/flat_overlay.rs  | 12 +++++-----
 consensus-engine/src/overlay/mod.rs           |  2 +-
 .../src/overlay/tree_overlay/overlay.rs       | 15 +++++--------
 .../src/overlay/tree_overlay/tree.rs          |  7 +++---
 simulations/src/node/carnot/mod.rs            | 22 +++++++++++++------
 6 files changed, 40 insertions(+), 30 deletions(-)

diff --git a/consensus-engine/src/lib.rs b/consensus-engine/src/lib.rs
index dabbc6b0..db467c34 100644
--- a/consensus-engine/src/lib.rs
+++ b/consensus-engine/src/lib.rs
@@ -135,7 +135,10 @@ impl<O: Overlay> Carnot<O> {
         let to = if new_state.overlay.is_member_of_root_committee(new_state.id) {
             [new_state.overlay.next_leader()].into_iter().collect()
         } else {
-            new_state.overlay.parent_committee(self.id)
+            new_state
+                .overlay
+                .parent_committee(self.id)
+                .expect("Non root committee members parent should be present")
         };
         (
             new_state,
@@ -203,7 +206,10 @@ impl<O: Overlay> Carnot<O> {
         let to = if new_state.overlay.is_member_of_root_committee(new_state.id) {
             [new_state.overlay.next_leader()].into_iter().collect()
         } else {
-            new_state.overlay.parent_committee(new_state.id)
+            new_state
+                .overlay
+                .parent_committee(new_state.id)
+                .expect("Non root committee members parent should be present")
         };
         (
             new_state,
@@ -360,7 +366,7 @@ impl<O: Overlay> Carnot<O> {
         self.overlay.child_committees(self.id)
     }
 
-    pub fn parent_committee(&self) -> Committee {
+    pub fn parent_committee(&self) -> Option<Committee> {
         self.overlay.parent_committee(self.id)
     }
 
diff --git a/consensus-engine/src/overlay/flat_overlay.rs b/consensus-engine/src/overlay/flat_overlay.rs
index e0303fe2..2bfbdf82 100644
--- a/consensus-engine/src/overlay/flat_overlay.rs
+++ b/consensus-engine/src/overlay/flat_overlay.rs
@@ -63,12 +63,8 @@ where
         false
     }
 
-    fn parent_committee(&self, _id: NodeId) -> crate::Committee {
-        std::iter::once(self.next_leader()).collect()
-    }
-
-    fn node_committee(&self, _id: NodeId) -> crate::Committee {
-        self.nodes.clone().into_iter().collect()
+    fn parent_committee(&self, _id: NodeId) -> Option<crate::Committee> {
+        Some(std::iter::once(self.next_leader()).collect())
     }
 
     fn child_committees(&self, _id: NodeId) -> Vec<crate::Committee> {
@@ -79,6 +75,10 @@ where
         vec![self.root_committee()]
     }
 
+    fn node_committee(&self, _id: NodeId) -> crate::Committee {
+        self.nodes.clone().into_iter().collect()
+    }
+
     fn next_leader(&self) -> NodeId {
         self.leader.next_leader(&self.nodes)
     }
diff --git a/consensus-engine/src/overlay/mod.rs b/consensus-engine/src/overlay/mod.rs
index 75c0afb7..20b3de25 100644
--- a/consensus-engine/src/overlay/mod.rs
+++ b/consensus-engine/src/overlay/mod.rs
@@ -21,7 +21,7 @@ pub trait Overlay: Clone {
     fn is_member_of_root_committee(&self, id: NodeId) -> bool;
     fn is_member_of_leaf_committee(&self, id: NodeId) -> bool;
     fn is_child_of_root_committee(&self, id: NodeId) -> bool;
-    fn parent_committee(&self, id: NodeId) -> Committee;
+    fn parent_committee(&self, id: NodeId) -> Option<Committee>;
     fn child_committees(&self, id: NodeId) -> Vec<Committee>;
     fn leaf_committees(&self, id: NodeId) -> Vec<Committee>;
     fn node_committee(&self, id: NodeId) -> Committee;
diff --git a/consensus-engine/src/overlay/tree_overlay/overlay.rs b/consensus-engine/src/overlay/tree_overlay/overlay.rs
index a6a6ec36..2ff1ca09 100644
--- a/consensus-engine/src/overlay/tree_overlay/overlay.rs
+++ b/consensus-engine/src/overlay/tree_overlay/overlay.rs
@@ -66,7 +66,7 @@ where
     fn is_member_of_child_committee(&self, parent: NodeId, child: NodeId) -> bool {
         let child_parent = self.parent_committee(child);
         let parent = self.carnot_tree.committee_by_member_id(&parent);
-        parent.map_or(false, |p| child_parent.eq(p))
+        child_parent.as_ref() == parent
     }
 
     fn is_member_of_root_committee(&self, id: NodeId) -> bool {
@@ -81,16 +81,13 @@ where
     }
 
     fn is_child_of_root_committee(&self, id: NodeId) -> bool {
-        self.parent_committee(id) == self.root_committee()
+        self.parent_committee(id)
+            .map(|c| c == self.root_committee())
+            .unwrap_or(false)
     }
 
-    fn parent_committee(&self, id: NodeId) -> Committee {
-        let c = self.carnot_tree.parent_committee_from_member_id(&id);
-        if !c.is_empty() {
-            c
-        } else {
-            std::iter::once(self.next_leader()).collect()
-        }
+    fn parent_committee(&self, id: NodeId) -> Option<Committee> {
+        self.carnot_tree.parent_committee_from_member_id(&id)
     }
 
     fn child_committees(&self, id: NodeId) -> Vec<Committee> {
diff --git a/consensus-engine/src/overlay/tree_overlay/tree.rs b/consensus-engine/src/overlay/tree_overlay/tree.rs
index 329578de..6210faf8 100644
--- a/consensus-engine/src/overlay/tree_overlay/tree.rs
+++ b/consensus-engine/src/overlay/tree_overlay/tree.rs
@@ -78,12 +78,11 @@ impl Tree {
         }
     }
 
-    pub(super) fn parent_committee_from_member_id(&self, id: &NodeId) -> Committee {
-        let Some(committee_id) = self.committee_id_by_member_id(id) else { return Committee::new(); };
-        let Some(parent_id) = self.parent_committee(committee_id) else { return Committee::new(); };
+    pub(super) fn parent_committee_from_member_id(&self, id: &NodeId) -> Option<Committee> {
+        let committee_id = self.committee_id_by_member_id(id)?;
+        let parent_id = self.parent_committee(committee_id)?;
         self.committee_by_committee_idx(self.committee_id_to_index[parent_id])
             .cloned()
-            .unwrap_or_default()
     }
 
     pub(super) fn child_committees(
diff --git a/simulations/src/node/carnot/mod.rs b/simulations/src/node/carnot/mod.rs
index 0cb1bc81..efc8042b 100644
--- a/simulations/src/node/carnot/mod.rs
+++ b/simulations/src/node/carnot/mod.rs
@@ -72,8 +72,8 @@ pub struct CarnotState {
     last_view_timeout_qc: Option<TimeoutQc>,
     latest_committed_block: Block,
     latest_committed_view: View,
-    root_committe: Committee,
-    parent_committe: Committee,
+    root_committee: Committee,
+    parent_committee: Option<Committee>,
     child_committees: Vec<Committee>,
     committed_blocks: Vec<BlockId>,
     step_duration: Duration,
@@ -129,9 +129,9 @@ impl serde::Serialize for CarnotState {
                     LATEST_COMMITTED_VIEW => {
                         ser.serialize_field(LATEST_COMMITTED_VIEW, &self.latest_committed_view)?
                     }
-                    ROOT_COMMITTEE => ser.serialize_field(ROOT_COMMITTEE, &self.root_committe)?,
+                    ROOT_COMMITTEE => ser.serialize_field(ROOT_COMMITTEE, &self.root_committee)?,
                     PARENT_COMMITTEE => {
-                        ser.serialize_field(PARENT_COMMITTEE, &self.parent_committe)?
+                        ser.serialize_field(PARENT_COMMITTEE, &self.parent_committee)?
                     }
                     CHILD_COMMITTEES => {
                         ser.serialize_field(CHILD_COMMITTEES, &self.child_committees)?
@@ -178,8 +178,8 @@ impl<O: Overlay> From<&Carnot<O>> for CarnotState {
             node_id,
             current_view,
             local_high_qc: value.high_qc(),
-            parent_committe: value.parent_committee(),
-            root_committe: value.root_committee(),
+            parent_committee: value.parent_committee(),
+            root_committee: value.root_committee(),
             child_committees: value.child_committees(),
             latest_committed_block: value.latest_committed_block(),
             latest_committed_view: value.latest_committed_view(),
@@ -361,8 +361,16 @@ impl<L: UpdateableLeaderSelection, O: Overlay<LeaderSelection = L>> CarnotNode<O
                 }
 
                 if self.engine.overlay().is_member_of_leaf_committee(self.id) {
+                    // Check if we are also a member of the parent committee, this is a special case for the flat committee
+                    let to = if self.engine.overlay().is_child_of_root_committee(self.id) {
+                        [self.engine.overlay().next_leader()].into_iter().collect()
+                    } else {
+                        self.engine.parent_committee().expect(
+                            "Parent committee of non root committee members should be present",
+                        )
+                    };
                     output = Some(Output::Send(consensus_engine::Send {
-                        to: self.engine.parent_committee(),
+                        to,
                         payload: Payload::Vote(Vote {
                             view: self.engine.current_view(),
                             block: block.header().id,