fix: ensure section elders info is always signed with the last chain key

src/network/mod.rs

@@ -401,7 +401,7 @@ mod tests {
 
         update_their_knowledge_and_check_proving_index(
             vec![("1", pk1), ("10", pk2)],
-            vec![("10", pk1), ("11", pk1)],
+            vec![("10", pk2), ("11", pk1)],
         )
     }
 

src/routing/tests/mod.rs

@@ -1227,7 +1227,7 @@ async fn handle_sync() -> Result<()> {
     let sk2 = bls::SecretKey::random();
     let pk2 = sk2.public_key();
     let pk2_signature = sk1_set.secret_key().sign(bincode::serialize(&pk2)?);
-    assert_eq!(chain.insert(&pk1, pk2, pk2_signature), Ok(()));
+    chain.insert(&pk1, pk2, pk2_signature)?;
 
     let old_node = nodes.remove(0);
 
@@ -1243,7 +1243,7 @@ async fn handle_sync() -> Result<()> {
         old_elders_info.prefix,
     );
     let new_elders: BTreeSet<_> = new_elders_info.elders.keys().copied().collect();
-    let proven_new_elders_info = proven(sk1_set.secret_key(), new_elders_info)?;
+    let proven_new_elders_info = proven(&sk2, new_elders_info)?;
     let new_section = Section::new(chain, proven_new_elders_info)?;
 
     // Create the `Sync` message containing the new `Section`.
@@ -1680,6 +1680,15 @@ async fn handle_demote_during_split() -> Result<()> {
 
 // TODO: add more tests here
 
+#[allow(unused)]
+fn init_log() {
+    tracing_subscriber::fmt()
+        .pretty()
+        .with_env_filter(tracing_subscriber::EnvFilter::from_default_env())
+        .with_target(false)
+        .init()
+}
+
 fn create_peer() -> Peer {
     Peer::new(rand::random(), gen_addr(), MIN_AGE)
 }

src/section/mod.rs

@@ -30,7 +30,7 @@ use crate::{
 };
 use bls_signature_aggregator::Proof;
 use serde::{Deserialize, Serialize};
-use std::{cmp::Ordering, collections::BTreeSet, convert::TryInto, iter, net::SocketAddr};
+use std::{collections::BTreeSet, convert::TryInto, iter, net::SocketAddr};
 use xor_name::{Prefix, XorName};
 
 #[derive(Clone, Debug, PartialEq, Eq, Hash, Serialize, Deserialize)]
@@ -87,20 +87,19 @@ impl Section {
     /// Try to merge this `Section` with `other`. Returns `InvalidMessage` if `other` is invalid or
     /// its chain is not compatible with the chain of `self`.
     pub fn merge(&mut self, other: Self) -> Result<()> {
-        if !other.elders_info.self_verify()
-            || &other.elders_info.proof.public_key != other.chain.last_key()
-        {
+        if !other.elders_info.self_verify() {
+            error!("can't merge sections: other elders_info failed verification");
+            return Err(Error::InvalidMessage);
+        }
+        if &other.elders_info.proof.public_key != other.chain.last_key() {
             // TODO: use more specific error variant.
+            error!("can't merge sections: other elders_info signed with incorrect key");
             return Err(Error::InvalidMessage);
         }
 
         self.chain.merge(other.chain.clone())?;
 
-        if self.chain.cmp_by_position(
-            &other.elders_info.proof.public_key,
-            &self.elders_info.proof.public_key,
-        ) == Ordering::Greater
-        {
+        if &other.elders_info.proof.public_key == self.chain.last_key() {
             self.elders_info = other.elders_info;
         }
 
@@ -142,7 +141,10 @@ impl Section {
             return false;
         }
 
-        self.elders_info = new_elders_info;
+        if &new_elders_info.proof.public_key == self.chain.last_key() {
+            self.elders_info = new_elders_info;
+        }
+
         self.members
             .prune_not_matching(&self.elders_info.value.prefix);
 

src/section/section_chain.rs

@@ -233,8 +233,13 @@ impl SectionChain {
     /// 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
     /// `Equal`.
-    pub fn cmp_by_position(&self, _lhs: &bls::PublicKey, _rhs: &bls::PublicKey) -> Ordering {
-        todo!()
+    pub fn cmp_by_position(&self, lhs: &bls::PublicKey, rhs: &bls::PublicKey) -> Ordering {
+        match (self.index_of(lhs), self.index_of(rhs)) {
+            (Some(lhs), Some(rhs)) => lhs.cmp(&rhs),
+            (Some(_), None) => Ordering::Greater,
+            (None, Some(_)) => Ordering::Less,
+            (None, None) => Ordering::Equal,
+        }
     }
 
     /// Returns the number of blocks in the chain. This is always >= 1.
@@ -855,6 +860,19 @@ mod tests {
         assert_eq!(chain.extend(&pk2, &main_chain), Ok(make_chain(pk2, vec![])));
     }
 
+    // TODO extend_unreachable_new_root_key()
+
+    #[test]
+    fn cmp_by_position() {
+        let (sk0, pk0) = gen_keypair();
+        let (sk1, pk1, sig1) = gen_signed_keypair(&sk0);
+        let (_, pk2, sig2) = gen_signed_keypair(&sk1);
+
+        let main_chain = make_chain(pk0, vec![(&pk0, pk1, sig1), (&pk1, pk2, sig2)]);
+
+        assert_eq!(main_chain.cmp_by_position(&pk0, &pk1), Ordering::Less);
+    }
+
     fn gen_keypair() -> (bls::SecretKey, bls::PublicKey) {
         let sk = bls::SecretKey::random();
         let pk = sk.public_key();