diff --git a/stacks-signer/src/coordinator.rs b/stacks-signer/src/coordinator.rs index 7469c0ff18..7fc2d238c4 100644 --- a/stacks-signer/src/coordinator.rs +++ b/stacks-signer/src/coordinator.rs @@ -91,17 +91,10 @@ impl CoordinatorSelector { } } new_index + } else if ROTATE_COORDINATORS { + self.coordinator_index.saturating_add(1) % self.coordinator_ids.len() } else { - if ROTATE_COORDINATORS { - let mut new_index = self.coordinator_index.saturating_add(1); - if new_index == self.coordinator_ids.len() { - // We have exhausted all potential coordinators. Go back to the start - new_index = 0; - } - new_index - } else { - self.coordinator_index - } + self.coordinator_index }; self.coordinator_id = *self .coordinator_ids diff --git a/stacks-signer/src/main.rs b/stacks-signer/src/main.rs index 056c5b866b..f38b14feab 100644 --- a/stacks-signer/src/main.rs +++ b/stacks-signer/src/main.rs @@ -416,6 +416,7 @@ pub mod tests { use super::{handle_generate_stacking_signature, *}; use crate::{GenerateStackingSignatureArgs, GlobalConfig}; + #[allow(clippy::too_many_arguments)] fn call_verify_signer_sig( pox_addr: &PoxAddress, reward_cycle: u128, diff --git a/stacks-signer/src/runloop.rs b/stacks-signer/src/runloop.rs index 4491650090..a29a666752 100644 --- a/stacks-signer/src/runloop.rs +++ b/stacks-signer/src/runloop.rs @@ -392,7 +392,7 @@ impl SignerRunLoop, RunLoopCommand> for RunLoop { if signer.approved_aggregate_public_key.is_none() { if let Err(e) = retry_with_exponential_backoff(|| { signer - .update_dkg(&self.stacks_client, current_reward_cycle) + .update_dkg(&self.stacks_client) .map_err(backoff::Error::transient) }) { error!("{signer}: failed to update DKG: {e}"); diff --git a/stacks-signer/src/signer.rs b/stacks-signer/src/signer.rs index 391e65ea90..6fa2b54098 100644 --- a/stacks-signer/src/signer.rs +++ b/stacks-signer/src/signer.rs @@ -185,9 +185,11 @@ impl std::fmt::Display for Signer { } impl Signer { - /// Return the current coordinator. If in the active reward cycle, this is the miner, - /// so the first element of the tuple will be None (because the miner does not have a signer index). - fn get_coordinator(&self, current_reward_cycle: u64) -> (Option, PublicKey) { + /// Return the current coordinator. + /// If the current reward cycle is the active reward cycle, this is the miner, + /// so the first element of the tuple will be None (because the miner does not have a signer index). + /// Otherwise, the coordinator is the signer with the index returned by the coordinator selector. + fn get_coordinator_sign(&self, current_reward_cycle: u64) -> (Option, PublicKey) { if self.reward_cycle == current_reward_cycle { let Some(ref cur_miner) = self.miner_key else { error!( @@ -198,12 +200,18 @@ impl Signer { return (Some(selected.0), selected.1); }; // coordinator is the current miner. - (None, cur_miner.clone()) + (None, *cur_miner) } else { let selected = self.coordinator_selector.get_coordinator(); - return (Some(selected.0), selected.1); + (Some(selected.0), selected.1) } } + + /// Get the current coordinator for executing DKG + /// This will always use the coordinator selector to determine the coordinator + fn get_coordinator_dkg(&self) -> (u32, PublicKey) { + self.coordinator_selector.get_coordinator() + } } impl From for Signer { @@ -428,24 +436,36 @@ impl Signer { stacks_client: &StacksClient, current_reward_cycle: u64, ) { - let coordinator_id = self.get_coordinator(current_reward_cycle).0; match &self.state { State::Idle => { + let Some(command) = self.commands.front() else { + debug!("{self}: Nothing to process. Waiting for command..."); + return; + }; + let coordinator_id = if matches!(command, Command::Dkg) { + // We cannot execute a DKG command if we are not the coordinator + Some(self.get_coordinator_dkg().0) + } else { + self.get_coordinator_sign(current_reward_cycle).0 + }; if coordinator_id != Some(self.signer_id) { debug!( "{self}: Coordinator is {coordinator_id:?}. Will not process any commands...", ); return; } - if let Some(command) = self.commands.pop_front() { - self.execute_command(stacks_client, &command); - } else { - debug!("{self}: Nothing to process. Waiting for command...",); - } + let command = self + .commands + .pop_front() + .expect("BUG: Already asserted that the command queue was not empty"); + self.execute_command(stacks_client, &command); } State::OperationInProgress => { // We cannot execute the next command until the current one is finished... - debug!("{self}: Waiting for coordinator {coordinator_id:?} operation to finish. Coordinator state = {:?}", self.coordinator.state); + debug!( + "{self}: Waiting for operation to finish. Coordinator state = {:?}", + self.coordinator.state + ); } } } @@ -458,7 +478,6 @@ impl Signer { res: Sender>, current_reward_cycle: u64, ) { - let coordinator_id = self.get_coordinator(current_reward_cycle).0; let mut block_info = match block_validate_response { BlockValidateResponse::Ok(block_validate_ok) => { let signer_signature_hash = block_validate_ok.signer_signature_hash; @@ -530,32 +549,13 @@ impl Signer { sig: vec![], }; self.handle_packets(stacks_client, res, &[packet], current_reward_cycle); - } else { - if block_info.valid.unwrap_or(false) - && !block_info.signed_over - && coordinator_id == Some(self.signer_id) - { - // We are the coordinator. Trigger a signing round for this block - debug!( - "{self}: attempt to trigger a signing round for block"; - "signer_sighash" => %block_info.block.header.signer_signature_hash(), - "block_hash" => %block_info.block.header.block_hash(), - ); - self.commands.push_back(Command::Sign { - block: block_info.block.clone(), - is_taproot: false, - merkle_root: None, - }); - } else { - debug!( - "{self}: ignoring block."; - "block_hash" => block_info.block.header.block_hash(), - "valid" => block_info.valid, - "signed_over" => block_info.signed_over, - "coordinator_id" => coordinator_id, - ); - } } + debug!( + "{self}: Received a block validate response"; + "block_hash" => block_info.block.header.block_hash(), + "valid" => block_info.valid, + "signed_over" => block_info.signed_over, + ); self.signer_db .insert_block(self.reward_cycle, &block_info) .unwrap_or_else(|_| panic!("{self}: Failed to insert block in DB")); @@ -569,7 +569,6 @@ impl Signer { messages: &[SignerMessage], current_reward_cycle: u64, ) { - let coordinator_pubkey = self.get_coordinator(current_reward_cycle).1; let packets: Vec = messages .iter() .filter_map(|msg| match msg { @@ -578,6 +577,11 @@ impl Signer { | SignerMessage::Transactions(_) => None, // TODO: if a signer tries to trigger DKG and we already have one set in the contract, ignore the request. SignerMessage::Packet(packet) => { + let coordinator_pubkey = if Self::is_dkg_message(&packet.msg) { + self.get_coordinator_dkg().1 + } else { + self.get_coordinator_sign(current_reward_cycle).1 + }; self.verify_packet(stacks_client, packet.clone(), &coordinator_pubkey) } }) @@ -642,6 +646,19 @@ impl Signer { } } + /// Helper function for determining if the provided message is a DKG specific message + fn is_dkg_message(msg: &Message) -> bool { + matches!( + msg, + Message::DkgBegin(_) + | Message::DkgEnd(_) + | Message::DkgEndBegin(_) + | Message::DkgPrivateBegin(_) + | Message::DkgPrivateShares(_) + | Message::DkgPublicShares(_) + ) + } + /// Process inbound packets as both a signer and a coordinator /// Will send outbound packets and operation results as appropriate fn handle_packets( @@ -939,7 +956,7 @@ impl Signer { }; self.signer_db .insert_block(self.reward_cycle, &updated_block_info) - .expect(&format!("{self}: Failed to insert block in DB")); + .unwrap_or_else(|_| panic!("{self}: Failed to insert block in DB")); let process_request = updated_block_info.vote.is_some(); if !process_request { debug!("Failed to validate nonce request"); @@ -1002,14 +1019,12 @@ impl Signer { ) { error!("{}: Failed to serialize DKGResults message for StackerDB, will continue operating.", self.signer_id; "error" => %e); - } else { - if let Err(e) = self - .stackerdb - .send_message_bytes_with_retry(&MessageSlotID::DkgResults, dkg_results_bytes) - { - error!("{}: Failed to send DKGResults message to StackerDB, will continue operating.", self.signer_id; + } else if let Err(e) = self + .stackerdb + .send_message_bytes_with_retry(&MessageSlotID::DkgResults, dkg_results_bytes) + { + error!("{}: Failed to send DKGResults message to StackerDB, will continue operating.", self.signer_id; "error" => %e); - } } let epoch = retry_with_exponential_backoff(|| { @@ -1235,13 +1250,9 @@ impl Signer { } /// Should DKG be queued to the current signer's command queue - pub fn should_queue_dkg( - &mut self, - stacks_client: &StacksClient, - current_reward_cycle: u64, - ) -> Result { + pub fn should_queue_dkg(&mut self, stacks_client: &StacksClient) -> Result { if self.state != State::Idle - || Some(self.signer_id) != self.get_coordinator(current_reward_cycle).0 + || self.signer_id != self.get_coordinator_dkg().0 || self.commands.front() == Some(&Command::Dkg) { // We are not the coordinator, we are in the middle of an operation, or we have already queued DKG. Do not attempt to queue DKG @@ -1329,11 +1340,7 @@ impl Signer { } /// Update the DKG for the provided signer info, triggering it if required - pub fn update_dkg( - &mut self, - stacks_client: &StacksClient, - current_reward_cycle: u64, - ) -> Result<(), ClientError> { + pub fn update_dkg(&mut self, stacks_client: &StacksClient) -> Result<(), ClientError> { let old_dkg = self.approved_aggregate_public_key; self.approved_aggregate_public_key = stacks_client.get_approved_aggregate_key(self.reward_cycle)?; @@ -1351,7 +1358,7 @@ impl Signer { } return Ok(()); }; - if self.should_queue_dkg(stacks_client, current_reward_cycle)? { + if self.should_queue_dkg(stacks_client)? { info!("{self} is the current coordinator and must trigger DKG. Queuing DKG command..."); self.commands.push_front(Command::Dkg); } diff --git a/stacks-signer/src/signerdb.rs b/stacks-signer/src/signerdb.rs index ea9c4eeb17..1b2fd6ca69 100644 --- a/stacks-signer/src/signerdb.rs +++ b/stacks-signer/src/signerdb.rs @@ -35,7 +35,7 @@ pub struct SignerDb { db: Connection, } -const CREATE_BLOCKS_TABLE: &'static str = " +const CREATE_BLOCKS_TABLE: &str = " CREATE TABLE IF NOT EXISTS blocks ( reward_cycle INTEGER NOT NULL, signer_signature_hash TEXT NOT NULL, @@ -170,8 +170,8 @@ where pub fn test_signer_db(db_path: &str) -> SignerDb { use std::fs; - if fs::metadata(&db_path).is_ok() { - fs::remove_file(&db_path).unwrap(); + if fs::metadata(db_path).is_ok() { + fs::remove_file(db_path).unwrap(); } SignerDb::new(db_path).expect("Failed to create signer db") }