feat: use the signer set in the registry for picking the coordinator (#1912)

## Description

Closes #1911

## Changes

* Use the registry as the source of truth for the signer set

Note that these changes only matter if there is a signer set change that
is about to happen.

## Testing Information

I tested this branch on the usual flow on devenv, and it worked fine. I
did not do any devenv testing that puts this new logic to the test
though.

## Checklist

- [x] I have performed a self-review of my code

Author: djordon

signer/src/context/mod.rs

@@ -5,6 +5,8 @@ mod signer_context;
 mod signer_state;
 mod termination;
 
+use std::collections::BTreeSet;
+
 use tokio::sync::broadcast::error::RecvError;
 use tokio_stream::wrappers::ReceiverStream;
 
@@ -13,6 +15,7 @@ use crate::bitcoin::BitcoinInteract;
 use crate::config::Settings;
 use crate::emily_client::EmilyInteract;
 use crate::error::Error;
+use crate::keys::PublicKey;
 use crate::stacks::api::StacksInteract;
 use crate::storage::DbRead;
 use crate::storage::DbWrite;
@@ -107,4 +110,23 @@ pub trait Context: Clone + Sync + Send {
         });
         ReceiverStream::new(receiver)
     }
+
+    /// Return the signer set that is used when determining who is the
+    /// coordinator.
+    ///
+    /// # Notes
+    ///
+    /// The signer set configured as the bootstrap signing set is used for
+    /// deciding who this signer can communicate with and for whether we
+    /// need to run DKG. However, for choosing a coordinator, we prefer to
+    /// rely on what is in the registry since all signers see that at the
+    /// same time yielding a more stable and predictable coordinator
+    /// selection process.
+    fn coordinator_signer_set(&self) -> BTreeSet<PublicKey> {
+        let default_signer_set = || self.config().signer.bootstrap_signing_set.clone();
+
+        self.state()
+            .registry_signer_set_info()
+            .map_or_else(default_signer_set, |info| info.signer_set)
+    }
 }

signer/src/context/signer_state.rs

@@ -65,8 +65,7 @@ impl SignerState {
         self.registry_signing_set_info
             .read()
             .expect("BUG: Failed to acquire read lock of signer set info")
-            .as_ref()
-            .cloned()
+            .to_owned()
     }
 
     /// Get the current bitcoin chain tip.

signer/src/transaction_coordinator.rs

@@ -1731,7 +1731,12 @@ where
         S: Stream<Item = Signed<SignerMessage>>,
         Coordinator: WstsCoordinator,
     {
-        let signer_set = self.context.config().signer.bootstrap_signing_set.clone();
+        // We only allow the coordinator to send us certain kinds of
+        // messages. So later, we need to check whether the sender is the
+        // coordinator, and for that we need to use the "coordinator signer
+        // set", which may differ from the set of signers sending us
+        // messages right now.
+        let signer_set = self.context.coordinator_signer_set();
         tokio::pin!(signal_stream);
 
         // Let's get the next message from the network or the
@@ -1860,15 +1865,15 @@ where
     /// Determine if this signer is the signer set's coordinator for the
     /// specified bitcoin block hash.
     ///
-    /// The coordinator is decided using the hash of the bitcoin chain tip.
-    /// We don't use the chain tip directly because it typically starts
-    /// with a lot of leading zeros.
+    /// The coordinator is decided using the hash of the bitcoin chain tip
+    /// and signer set info from the registry if present. We don't use the
+    /// chain tip directly because it typically starts with a lot of
+    /// leading zeros.
     pub fn is_coordinator(&self, bitcoin_chain_tip: &model::BitcoinBlockHash) -> bool {
-        given_key_is_coordinator(
-            self.signer_public_key(),
-            bitcoin_chain_tip,
-            &self.context.config().signer.bootstrap_signing_set,
-        )
+        let signer_public_keys = self.context.coordinator_signer_set();
+
+        let signer_public_key = self.signer_public_key();
+        given_key_is_coordinator(signer_public_key, bitcoin_chain_tip, &signer_public_keys)
     }
 
     /// Constructs a new [`utxo::SignerBtcState`] based on the current market
@@ -2858,6 +2863,66 @@ mod tests {
             .unwrap();
     }
 
+    /// Check that is_coordinator uses the registry for figuring out who
+    /// the cooridnator is and falls back to the config if the registry has
+    /// no signer set info.
+    #[tokio::test]
+    async fn is_coordinator_uses_registry_for_coordinator() {
+        let mut rng = testing::get_rng();
+        let ctx = TestContext::builder()
+            .with_in_memory_storage()
+            .with_mocked_clients()
+            .modify_settings(|settings| {
+                settings.signer.bootstrap_signatures_required = 1;
+                settings.signer.bootstrap_signing_set =
+                    std::iter::once(settings.signer.public_key()).collect();
+            })
+            .build();
+
+        let network = WanNetwork::default();
+        let net = network.connect(&ctx);
+
+        let ev = TxCoordinatorEventLoop {
+            network: net.spawn(),
+            context: ctx.clone(),
+            context_window: 10000,
+            private_key: ctx.config().signer.private_key,
+            signing_round_max_duration: Duration::from_secs(10),
+            bitcoin_presign_request_max_duration: Duration::from_secs(10),
+            dkg_max_duration: Duration::from_secs(10),
+            is_epoch3: true,
+        };
+
+        // We should always be the coordinator since the registry is unset
+        // and we're the only signer in the bootstrap signing set.
+        assert!(ev.context.state().registry_signer_set_info().is_none());
+
+        for _ in 0..100 {
+            let chain_tip1: BitcoinBlockRef = fake::Faker.fake_with_rng(&mut rng);
+            assert!(ev.is_coordinator(&chain_tip1.block_hash));
+        }
+
+        // Now let's set the signer set in the registry to some random
+        // signer set without our key. Now we should never be the
+        // coordinator.
+        let signer_set_info = crate::stacks::api::SignerSetInfo {
+            aggregate_key: Faker.fake_with_rng(&mut rng),
+            signer_set: std::iter::repeat_with(|| Faker.fake_with_rng(&mut rng))
+                .take(2)
+                .collect(),
+            signatures_required: 2,
+        };
+
+        ctx.state().update_registry_signer_set_info(signer_set_info);
+
+        // If we were part of the signing set, there is a 2^(-128) chance
+        // that this check would pass.
+        for _ in 0..128 {
+            let chain_tip1: BitcoinBlockRef = fake::Faker.fake_with_rng(&mut rng);
+            assert!(!ev.is_coordinator(&chain_tip1.block_hash));
+        }
+    }
+
     #[tokio::test]
     async fn should_get_signer_utxo_simple() {
         test_environment().assert_get_signer_utxo_simple().await;