fix: Make leader election select from the whole Committee (MystenLabs…

…#762)

* fix: Make leader election select from the whole Committee

Fixes MystenLabs#755

* fix: Make tests of binomial result more strict

* fix: make the leader see a strict u64 (from usize)

* fix: consensus behavior under tests

narwhal/config/Cargo.toml

@@ -15,6 +15,7 @@ arc-swap = { version = "1.5.1", features = ["serde"] }
 
 crypto = { path = "../crypto" }
 workspace-hack = { version = "0.1", path = "../workspace-hack" }
+rand = "0.8.5"
 
 [dev-dependencies]
 insta = "1.17.2"

narwhal/config/src/lib.rs

@@ -11,6 +11,7 @@
 use arc_swap::ArcSwap;
 use crypto::{traits::EncodeDecodeBase64, PublicKey};
 use multiaddr::Multiaddr;
+use rand::{rngs::StdRng, seq::SliceRandom, SeedableRng};
 use serde::{de::DeserializeOwned, Deserialize, Serialize};
 use std::{
     collections::{BTreeMap, HashMap, HashSet},
@@ -377,11 +378,21 @@ impl Committee {
         (total_votes + 2) / 3
     }
 
-    /// Returns a leader node in a round-robin fashion.
-    pub fn leader(&self, seed: usize) -> PublicKey {
-        let mut keys: Vec<_> = self.authorities.keys().cloned().collect();
-        keys.sort();
-        keys[seed % self.size()].clone()
+    /// Returns a leader node as a weighted choice seeded by the provided integer
+    pub fn leader(&self, seed: u64) -> PublicKey {
+        let mut seed_bytes = [0u8; 32];
+        seed_bytes[32 - 8..].copy_from_slice(&seed.to_le_bytes());
+        let mut rng = StdRng::from_seed(seed_bytes);
+        let choices = self
+            .authorities
+            .iter()
+            .map(|(name, authority)| (name, authority.stake as f32))
+            .collect::<Vec<_>>();
+        choices
+            .choose_weighted(&mut rng, |item| item.1)
+            .expect("Weighted choice error: stake values incorrect!")
+            .0
+            .clone()
     }
 
     /// Returns the primary addresses of the target primary.

narwhal/config/tests/config_tests.rs

@@ -16,7 +16,7 @@
 // 1. Run `cargo insta test --review` under `./config`.
 // 2. Review, accept or reject changes.
 
-use std::collections::BTreeMap;
+use std::collections::{BTreeMap, HashMap};
 
 use config::{
     Committee, ConsensusAPIGrpcParameters, Epoch, Import, Parameters, PrimaryAddresses,
@@ -29,6 +29,26 @@ use std::{fs::File, io::Write};
 use tempfile::tempdir;
 use test_utils::make_authority_with_port_getter;
 
+#[test]
+fn leader_election_rotates_through_all() {
+    // this committee has equi-sized stakes
+    let committee = test_utils::committee(None);
+    let mut leader_counts = HashMap::new();
+    // We most probably will only call `leader` on even rounds, so let's check this
+    // still lets us use the whole roster of leaders.
+    let mut leader_counts_stepping_by_2 = HashMap::new();
+    for i in 0..100 {
+        let leader = committee.leader(i);
+        let leader_stepping_by_2 = committee.leader(i * 2);
+        *leader_counts.entry(leader).or_insert(0) += 1;
+        *leader_counts_stepping_by_2
+            .entry(leader_stepping_by_2)
+            .or_insert(0) += 1;
+    }
+    assert!(leader_counts.values().all(|v| *v >= 20));
+    assert!(leader_counts_stepping_by_2.values().all(|v| *v >= 20));
+}
+
 #[test]
 fn update_primary_network_info_test() {
     let committee = test_utils::committee(None);

narwhal/consensus/Cargo.toml

@@ -24,6 +24,7 @@ dag = { path = "../dag" }
 prometheus = "0.13.1"
 types = { path = "../types" }
 workspace-hack = { version = "0.1", path = "../workspace-hack" }
+cfg-if = "1.0.0"
 
 [dev-dependencies]
 bincode = "1.3.3"

narwhal/consensus/src/bullshark.rs

@@ -147,15 +147,20 @@ impl Bullshark {
         round: Round,
         dag: &'a Dag,
     ) -> Option<&'a (CertificateDigest, Certificate)> {
-        #[cfg(test)]
-        let seed = 0;
-        #[cfg(not(test))]
-        let seed = round;
-
-        // Elect the leader in a round-robin fashion.
-        let leader = committee.leader(seed as usize);
+        // Note: this function is often called with even rounds only. While we do not aim at random selection
+        // yet (see issue #10), repeated calls to this function should still pick from the whole roster of leaders.
+
+        cfg_if::cfg_if! {
+            if #[cfg(test)] {
+                // consensus tests rely on returning the same leader.
+                let leader = committee.authorities.iter().next().expect("Empty authorities table!").0;
+            } else {
+                // Elect the leader in a stake-weighted choice seeded by the round
+                let leader = &committee.leader(round);
+            }
+        }
 
         // Return its certificate and the certificate's digest.
-        dag.get(&round).and_then(|x| x.get(&leader))
+        dag.get(&round).and_then(|x| x.get(leader))
     }
 }

narwhal/consensus/src/tusk.rs

@@ -149,17 +149,22 @@ impl Tusk {
     ) -> Option<&'a (CertificateDigest, Certificate)> {
         // TODO: We should elect the leader of round r-2 using the common coin revealed at round r.
         // At this stage, we are guaranteed to have 2f+1 certificates from round r (which is enough to
-        // compute the coin). We currently just use round-robin.
-        #[cfg(test)]
-        let coin = 0;
-        #[cfg(not(test))]
-        let coin = round as usize;
-
-        // Elect the leader.
-        let leader = committee.leader(coin);
+        // compute the coin). We currently just use a stake-weighted choise seeded by the round.
+        //
+        // Note: this function is often called with even rounds only. While we do not aim at random selection
+        // yet (see issue #10), repeated calls to this function should still pick from the whole roster of leaders.
+        cfg_if::cfg_if! {
+            if #[cfg(test)] {
+                // consensus tests rely on returning the same leader.
+                let leader = committee.authorities.iter().next().expect("Empty authorities table!").0;
+            } else {
+                // Elect the leader in a stake-weighted choice seeded by the round
+                let leader = &committee.leader(round);
+            }
+        }
 
         // Return its certificate and the certificate's digest.
-        dag.get(&round).and_then(|x| x.get(&leader))
+        dag.get(&round).and_then(|x| x.get(leader))
     }
 }
 

narwhal/primary/src/proposer.rs

@@ -137,7 +137,7 @@ impl Proposer {
     // Main loop listening to incoming messages.
     /// Update the last leader certificate. This is only relevant in partial synchrony.
     fn update_leader(&mut self) -> bool {
-        let leader_name = self.committee.leader(self.round as usize);
+        let leader_name = self.committee.leader(self.round);
         self.last_leader = self
             .last_parents
             .iter()