test fixes, some cleanup

Author: kantai

stacks-signer/src/lib.rs

@@ -72,7 +72,7 @@ pub trait Signer<T: SignerEventTrait>: Debug + Display {
         stacks_client: &StacksClient,
         sortition_state: &mut Option<SortitionsView>,
         event: Option<&SignerEvent<T>>,
-        res: &Sender<Vec<SignerResult>>,
+        res: &Sender<SignerResult>,
         current_reward_cycle: u64,
     );
     /// Check if the signer is in the middle of processing blocks
@@ -82,18 +82,18 @@ pub trait Signer<T: SignerEventTrait>: Debug + Display {
 }
 
 /// A wrapper around the running signer type for the signer
-pub type RunningSigner<T> = libsigner::RunningSigner<SignerEventReceiver<T>, Vec<SignerResult>, T>;
+pub type RunningSigner<T> = libsigner::RunningSigner<SignerEventReceiver<T>, SignerResult, T>;
 
 /// The wrapper for the runloop signer type
 type RunLoopSigner<S, T> =
-    libsigner::Signer<Vec<SignerResult>, RunLoop<S, T>, SignerEventReceiver<T>, T>;
+    libsigner::Signer<SignerResult, RunLoop<S, T>, SignerEventReceiver<T>, T>;
 
 /// The spawned signer
 pub struct SpawnedSigner<S: Signer<T> + Send, T: SignerEventTrait> {
     /// The underlying running signer thread handle
     running_signer: RunningSigner<T>,
     /// The result receiver for interacting with the running signer
-    pub res_recv: Receiver<Vec<SignerResult>>,
+    pub res_recv: Receiver<SignerResult>,
     /// The spawned signer's config
     pub config: GlobalConfig,
     /// Phantom data for the signer type
@@ -102,12 +102,12 @@ pub struct SpawnedSigner<S: Signer<T> + Send, T: SignerEventTrait> {
 
 impl<S: Signer<T> + Send, T: SignerEventTrait> SpawnedSigner<S, T> {
     /// Stop the signer thread and return the final state
-    pub fn stop(self) -> Option<Vec<SignerResult>> {
+    pub fn stop(self) -> Option<SignerResult> {
         self.running_signer.stop()
     }
 
     /// Wait for the signer to terminate, and get the final state. WARNING: This will hang forever if the event receiver stop signal was never sent/no error occurred.
-    pub fn join(self) -> Option<Vec<SignerResult>> {
+    pub fn join(self) -> Option<SignerResult> {
         self.running_signer.join()
     }
 }

stacks-signer/src/runloop.rs

@@ -481,7 +481,7 @@ impl<Signer: SignerTrait<T>, T: StacksMessageCodec + Clone + Send + Debug> RunLo
 }
 
 impl<Signer: SignerTrait<T>, T: StacksMessageCodec + Clone + Send + Debug>
-    SignerRunLoop<Vec<SignerResult>, T> for RunLoop<Signer, T>
+    SignerRunLoop<SignerResult, T> for RunLoop<Signer, T>
 {
     fn set_event_timeout(&mut self, timeout: Duration) {
         self.config.event_timeout = timeout;
@@ -494,8 +494,8 @@ impl<Signer: SignerTrait<T>, T: StacksMessageCodec + Clone + Send + Debug>
     fn run_one_pass(
         &mut self,
         event: Option<SignerEvent<T>>,
-        res: &Sender<Vec<SignerResult>>,
-    ) -> Option<Vec<SignerResult>> {
+        res: &Sender<SignerResult>,
+    ) -> Option<SignerResult> {
         debug!(
             "Running one pass for the signer. state={:?}, event={event:?}",
             self.state
@@ -536,8 +536,7 @@ impl<Signer: SignerTrait<T>, T: StacksMessageCodec + Clone + Send + Debug>
 
         // This is the only event that we respond to from the outer signer runloop
         if let Some(SignerEvent::StatusCheck) = event {
-            info!("Signer status check requested: {:?}.", self.state);
-            if let Err(e) = res.send(vec![StateInfo {
+            let state_info = StateInfo {
                 runloop_state: self.state,
                 reward_cycle_info: self.current_reward_cycle_info,
                 running_signers: self
@@ -558,9 +557,10 @@ impl<Signer: SignerTrait<T>, T: StacksMessageCodec + Clone + Send + Debug>
                         )
                     })
                     .collect(),
-            }
-            .into()])
-            {
+            };
+            info!("Signer status check requested: {state_info:?}");
+
+            if let Err(e) = res.send(state_info.into()) {
                 error!("Failed to send status check result: {e}.");
             }
         }

stacks-signer/src/v0/signer.rs

@@ -176,7 +176,7 @@ impl SignerTrait<SignerMessage> for Signer {
         stacks_client: &StacksClient,
         sortition_state: &mut Option<SortitionsView>,
         event: Option<&SignerEvent<SignerMessage>>,
-        _res: &Sender<Vec<SignerResult>>,
+        _res: &Sender<SignerResult>,
         current_reward_cycle: u64,
     ) {
         let event_parity = match event {

testnet/stacks-node/src/tests/signer/mod.rs

@@ -214,13 +214,11 @@ impl<S: Signer<T> + Send + 'static, T: SignerEventTrait + 'static> SignerTest<Sp
 
     /// Send a status request to each spawned signer
     pub fn send_status_request(&self, exclude: &HashSet<usize>) {
-        for signer_ix in 0..self.spawned_signers.len() {
+        for (signer_ix, signer_config) in self.signer_configs.iter().enumerate() {
             if exclude.contains(&signer_ix) {
                 continue;
             }
-            let port = 3000 + signer_ix;
-            let endpoint = format!("http://localhost:{port}");
-            let path = format!("{endpoint}/status");
+            let path = format!("http://{}/status", signer_config.endpoint);
 
             debug!("Issue status request to {path}");
             let client = reqwest::blocking::Client::new();
@@ -471,7 +469,14 @@ impl<S: Signer<T> + Send + 'static, T: SignerEventTrait + 'static> SignerTest<Sp
                         }
                     })
                 else {
-                    warn!("Local state machine for signer #{ix} not set for reward cycle ${current_rc} yet");
+                    let rcs_set: Vec<_> = signer_state.signer_state_machines.iter().map(|(rc, state)| {
+                          (rc, state.is_some())
+                        }).collect();
+                    warn!(
+                        "Local state machine for signer #{ix} not set for reward cycle #{current_rc} yet";
+                        "burn_block_height" => info_cur.burn_block_height,
+                        "rcs_set" => ?rcs_set
+                    );
                     return false;
                 };
 
@@ -702,6 +707,24 @@ impl<S: Signer<T> + Send + 'static, T: SignerEventTrait + 'static> SignerTest<Sp
             .collect()
     }
 
+    /// Replace the test's configured signer st
+    pub fn replace_signers(
+        &mut self,
+        new_signers: Vec<SpawnedSigner<S, T>>,
+        new_signers_sks: Vec<StacksPrivateKey>,
+        new_signer_configs: Vec<SignerConfig>,
+    ) -> (
+        Vec<SpawnedSigner<S, T>>,
+        Vec<StacksPrivateKey>,
+        Vec<SignerConfig>,
+    ) {
+        let old_signers = std::mem::replace(&mut self.spawned_signers, new_signers);
+        let old_signers_sks =
+            std::mem::replace(&mut self.signer_stacks_private_keys, new_signers_sks);
+        let old_signers_confs = std::mem::replace(&mut self.signer_configs, new_signer_configs);
+        (old_signers, old_signers_sks, old_signers_confs)
+    }
+
     /// Get status check results (if returned) from each signer without blocking
     /// Returns Some() or None() for each signer, in order of `self.spawned_signers`
     pub fn get_states(&mut self, exclude: &HashSet<usize>) -> Vec<Option<StateInfo>> {
@@ -711,17 +734,14 @@ impl<S: Signer<T> + Send + 'static, T: SignerEventTrait + 'static> SignerTest<Sp
                 output.push(None);
                 continue;
             }
-            let Ok(mut results) = signer.res_recv.try_recv() else {
-                debug!("Could not receive latest state from signer #{ix}");
-                output.push(None);
-                continue;
-            };
-            assert!(results.len() <= 1, "Received multiple states from the signer receiver: this test function assumes it should only ever receive 1");
-            let Some(SignerResult::StatusCheck(state_info)) = results.pop() else {
-                debug!("Could not receive latest state from signer #{ix}");
+            let Ok(results) = signer.res_recv.try_recv() else {
+                info!("Could not receive latest state from signer #{ix}");
                 output.push(None);
                 continue;
             };
+            // Note: if we ever add more signer result enum variants, this function
+            //  should push None and continue for non-StatusCheck variants
+            let SignerResult::StatusCheck(state_info) = results;
             output.push(Some(state_info));
         }
         output

testnet/stacks-node/src/tests/signer/v0.rs

@@ -4648,12 +4648,16 @@ fn signer_set_rollover() {
         None,
     );
 
+    let new_signer_configs: Vec<_> = new_signer_configs
+        .iter()
+        .map(|conf_str| SignerConfig::load_from_str(conf_str).unwrap())
+        .collect();
+
     let new_spawned_signers: Vec<_> = new_signer_configs
         .iter()
-        .map(|conf| {
+        .map(|signer_config| {
             info!("spawning signer");
-            let signer_config = SignerConfig::load_from_str(conf).unwrap();
-            SpawnedSigner::new(signer_config)
+            SpawnedSigner::new(signer_config.clone())
         })
         .collect();
 
@@ -4663,8 +4667,7 @@ fn signer_set_rollover() {
         initial_balances,
         |_| {},
         |naka_conf| {
-            for toml in new_signer_configs.clone() {
-                let signer_config = SignerConfig::load_from_str(&toml).unwrap();
+            for signer_config in new_signer_configs.clone() {
                 info!(
                     "---- Adding signer endpoint to naka conf ({}) ----",
                     signer_config.endpoint
@@ -4697,9 +4700,8 @@ fn signer_set_rollover() {
     let short_timeout = Duration::from_secs(20);
 
     // Verify that naka_conf has our new signer's event observers
-    for toml in &new_signer_configs {
-        let signer_config = SignerConfig::load_from_str(toml).unwrap();
-        let endpoint = format!("{}", signer_config.endpoint);
+    for signer_config in &new_signer_configs {
+        let endpoint = signer_config.endpoint.to_string();
         assert!(signer_test
             .running_nodes
             .conf
@@ -4843,7 +4845,20 @@ fn signer_set_rollover() {
         assert!(new_signer_public_keys.contains(&signer.signing_key.to_vec()));
     }
 
-    info!("---- Mining to the next reward cycle (block {next_cycle_height}) -----",);
+    info!("---- Mining to just before the next reward cycle (block {next_cycle_height}) -----",);
+    signer_test.run_until_burnchain_height_nakamoto(
+        Duration::from_secs(60),
+        next_cycle_height.saturating_sub(1),
+        new_num_signers,
+    );
+
+    let (old_spawned_signers, _, _) = signer_test.replace_signers(
+        new_spawned_signers,
+        new_signer_private_keys,
+        new_signer_configs,
+    );
+
+    info!("---- Mining into the next reward cycle (block {next_cycle_height}) -----",);
     signer_test.run_until_burnchain_height_nakamoto(
         Duration::from_secs(60),
         next_cycle_height,
@@ -4887,7 +4902,7 @@ fn signer_set_rollover() {
     }
 
     signer_test.shutdown();
-    for signer in new_spawned_signers {
+    for signer in old_spawned_signers {
         assert!(signer.stop().is_none());
     }
 }
@@ -12308,6 +12323,7 @@ fn signer_can_accept_rejected_block() {
     info!("Submitted transfer tx and waiting for block proposal");
     let block = wait_for_block_proposal(30, block_height_before + 1, &miner_pk)
         .expect("Timed out waiting for block proposal");
+    let expected_block_height = block.header.chain_length;
 
     // Wait for signer[0] to reject the block
     wait_for_block_rejections(30, block.header.signer_signature_hash(), 1)
@@ -12337,18 +12353,18 @@ fn signer_can_accept_rejected_block() {
     wait_for(60, || {
         let blocks = test_observer::get_blocks();
 
-        // Look for a block with height `block_height_before + 1`
-        if let Some(block) = blocks
+        // Look for a block with expected height
+        let Some(block) = blocks
             .iter()
-            .find(|block| block["block_height"].as_u64() == Some(block_height_before + 1))
-        {
-            if transfers_in_block(block) == 1 {
-                Ok(true) // Success: found the block with exactly 1 transfer
-            } else {
-                Err("Transfer included in block".into()) // Found the block, but it has the wrong number of transfers
-            }
+            .find(|block| block["block_height"].as_u64() == Some(expected_block_height)) else {
+                return Ok(false) // Keep waiting if the block hasn't appeared yet
+        };
+
+        let transfers_included_in_block = transfers_in_block(block);
+        if transfers_included_in_block == 1 {
+            Ok(true) // Success: found the block with exactly 1 transfer
         } else {
-            Ok(false) // Keep waiting if the block hasn't appeared yet
+            Err(format!("Unexpected amount of transfers included in block. Found: {transfers_included_in_block}"))
         }
     })
     .expect("Timed out waiting for block");