fix: correctly handle section chain extend edge case

When the trusted key is not on the main branch, the chain cannot be extended without breaking the invariant that the last key remains intact even after the extend. We return an error now instead.

Also log more stuff, to help debugging.

src/messages/mod.rs

@@ -89,7 +89,10 @@ impl Message {
                 if let Some(proof_chain) = msg.proof_chain.as_ref() {
                     // FIXME Assumes the nodes proof last key is the one signing this message
                     if !proof_chain.last_key().verify(signature, &signed_bytes) {
-                        error!("Failed signature: {:?}", msg);
+                        error!(
+                            "Failed signature: {:?} (proof chain: {:?})",
+                            msg, proof_chain
+                        );
                         return Err(CreateError::FailedSignature);
                     }
                 }

src/routing/approved.rs

@@ -2129,10 +2129,14 @@ impl Approved {
             variant,
         };
 
-        Ok(Vote::SendMessage {
+        let vote = Vote::SendMessage {
             message: Box::new(message),
             proof_chain,
-        })
+        };
+
+        trace!("Create {:?}", vote);
+
+        Ok(vote)
     }
 
     fn create_proof_chain(

src/routing/mod.rs

@@ -405,10 +405,16 @@ async fn handle_connection_events(
 }
 
 async fn handle_message(stage: Arc<Stage>, bytes: Bytes, sender: SocketAddr) {
+    let span = {
+        let state = stage.state.lock().await;
+        trace_span!("handle_message", name = %state.node().name(), %sender)
+    };
+    let _span_guard = span.enter();
+
     let message_type = match WireMsg::deserialize(bytes) {
         Ok(message_type) => message_type,
         Err(error) => {
-            error!("Failed to deserialize message from {}: {}", sender, error);
+            error!("Failed to deserialize message: {}", error);
             return;
         }
     };
@@ -431,10 +437,7 @@ async fn handle_message(stage: Arc<Stage>, bytes: Bytes, sender: SocketAddr) {
                     let _ = task::spawn(stage.handle_commands(command));
                 }
                 Err(error) => {
-                    error!(
-                        "Error occurred when deserialising node message bytes from {}: {}",
-                        sender, error
-                    );
+                    error!("Failed to deserialize node message: {}", error);
                 }
             }
         }

src/section/mod.rs

@@ -97,8 +97,13 @@ impl Section {
             return Err(Error::InvalidMessage);
         }
 
+        let prev_last_key = *self.chain.last_key();
         self.chain.merge(other.chain.clone())?;
 
+        if !self.chain.has_key_on_main_branch(&prev_last_key) {
+            warn!("fork resolved");
+        }
+
         if &other.elders_info.proof.public_key == self.chain.last_key() {
             self.elders_info = other.elders_info;
         }
@@ -129,6 +134,8 @@ impl Section {
             return false;
         }
 
+        let prev_last_key = *self.chain.last_key();
+
         if let Err(error) = self.chain.insert(
             &new_key_proof.public_key,
             new_elders_info.proof.public_key,
@@ -141,6 +148,10 @@ impl Section {
             return false;
         }
 
+        if !self.chain.has_key_on_main_branch(&prev_last_key) {
+            warn!("fork resolved");
+        }
+
         if &new_elders_info.proof.public_key == self.chain.last_key() {
             self.elders_info = new_elders_info;
         }

src/section/section_chain.rs

@@ -203,11 +203,7 @@ impl SectionChain {
     /// Returns `Error::KeyNotFound` if either `trusted_key` or `self.last_key()` is not present in
     /// `super_chain`.
     ///
-    /// If `trusted_key` is not reachable from `self.last_key()` then the only way to make the
-    /// resulting chain trusted is to extend it to some trusted common ancestor of both
-    /// `trusted_key` and `self.last_key()`. The only such trusted ancestor, based on the
-    /// information available to this function, is the genesis key which we assume is the root key
-    /// of `super_chain`. Thus a copy of `super_chain` is returned in this case.
+    /// Returns `Error::KeyOnWrongBranch` if `trusted_key` is not reachable from `self.last_key()`.
     pub fn extend(&self, trusted_key: &bls::PublicKey, super_chain: &Self) -> Result<Self, Error> {
         let trusted_key_index = super_chain
             .index_of(trusted_key)
@@ -219,7 +215,7 @@ impl SectionChain {
         if super_chain.is_ancestor(trusted_key_index, last_key_index) {
             super_chain.minimize(vec![trusted_key, self.last_key()])
         } else {
-            Ok(super_chain.clone())
+            Err(Error::KeyOnWrongBranch)
         }
     }
 
@@ -247,6 +243,14 @@ impl SectionChain {
         self.keys().any(|existing_key| existing_key == key)
     }
 
+    /// Returns whether the given key is on the main branch - that is, it is reachable from the
+    /// current last key by following the parent links.
+    pub fn has_key_on_main_branch(&self, key: &bls::PublicKey) -> bool {
+        self.index_of(key)
+            .map(|index| self.is_ancestor(index, self.tree.len()))
+            .unwrap_or(false)
+    }
+
     /// Compare the two keys by their position in the chain. The key that is higher (closer to the
     /// last key) is considered `Greater`. If exactly one of the keys is not in the chain, the other
     /// one is implicitly considered `Greater`. If none are in the chain, they are considered
@@ -376,6 +380,8 @@ pub enum Error {
     FailedSignature,
     #[error("key not found in the chain")]
     KeyNotFound,
+    #[error("key not on the main branch of the chain")]
+    KeyOnWrongBranch,
     #[error("chain doesn't contain any trusted keys")]
     Untrusted,
     #[error("chains are incompatible")]
@@ -922,7 +928,10 @@ mod tests {
         let chain = make_chain(pk1, vec![(&pk1, pk2, sig2.clone())]);
         assert_eq!(
             chain.extend(&pk0, &main_chain),
-            Ok(make_chain(pk0, vec![(&pk0, pk1, sig1), (&pk1, pk2, sig2)]))
+            Ok(make_chain(
+                pk0,
+                vec![(&pk0, pk1, sig1.clone()), (&pk1, pk2, sig2)]
+            ))
         );
 
         // in:      2->3
@@ -932,22 +941,27 @@ mod tests {
         let chain = make_chain(pk2, vec![(&pk2, pk3, sig3)]);
         assert_eq!(chain.extend(&pk1, &main_chain), Err(Error::KeyNotFound));
 
-        // in:       2
-        // new_root: 2
-        // out:      2
+        // in:      2
+        // trusted: 2
+        // out:     2
         let chain = make_chain(pk2, vec![]);
         assert_eq!(chain.extend(&pk2, &main_chain), Ok(make_chain(pk2, vec![])));
+
+        // in:      1
+        // trusted: 0
+        // out:     0->1
+        let chain = make_chain(pk1, vec![]);
+        assert_eq!(
+            chain.extend(&pk0, &main_chain),
+            Ok(make_chain(pk0, vec![(&pk0, pk1, sig1)]))
+        );
     }
 
     #[test]
     fn extend_unreachable_trusted_key() {
         // main:    0->1->2->3
         //             |
         //             +->4
-        //
-        // in:      1->2->3
-        // trusted: 4
-        // out:     Error::Unreachable
         let (sk0, pk0) = gen_keypair();
         let (sk1, pk1, sig1) = gen_signed_keypair(&sk0);
         let (sk2, pk2, sig2) = gen_signed_keypair(&sk1);
@@ -964,8 +978,14 @@ mod tests {
             ],
         );
 
+        // in:      1->2->3
+        // trusted: 4
+        // out:     Error
         let chain = make_chain(pk1, vec![(&pk1, pk2, sig2), (&pk2, pk3, sig3)]);
-        assert_eq!(chain.extend(&pk4, &main_chain), Ok(main_chain));
+        assert_eq!(
+            chain.extend(&pk4, &main_chain),
+            Err(Error::KeyOnWrongBranch)
+        );
     }
 
     #[test]