From 63394526ea45696b7751a6ba0871d2a1b3d40833 Mon Sep 17 00:00:00 2001 From: Jacinta Ferrant Date: Mon, 25 Mar 2024 19:05:37 -0400 Subject: [PATCH 1/6] Do not attempt to treat miner as a coordinator when updating DKG Signed-off-by: Jacinta Ferrant --- stacks-signer/src/runloop.rs | 2 +- stacks-signer/src/signer.rs | 10 +++------- 2 files changed, 4 insertions(+), 8 deletions(-) diff --git a/stacks-signer/src/runloop.rs b/stacks-signer/src/runloop.rs index a056f0764e..52e606ef94 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 c5e423695b..6e0541ba50 100644 --- a/stacks-signer/src/signer.rs +++ b/stacks-signer/src/signer.rs @@ -1207,11 +1207,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 reward_cycle = self.reward_cycle; let old_dkg = self.approved_aggregate_public_key; self.approved_aggregate_public_key = @@ -1230,8 +1226,8 @@ impl Signer { } return Ok(()); }; - let coordinator_id = self.get_coordinator(current_reward_cycle).0; - if Some(self.signer_id) == coordinator_id && self.state == State::Idle { + let coordinator_id = self.coordinator_selector.get_coordinator().0; + if self.signer_id == coordinator_id && self.state == State::Idle { debug!("{self}: Checking if old vote transaction exists in StackerDB..."); // Have I already voted and have a pending transaction? Check stackerdb for the same round number and reward cycle vote transaction // Only get the account nonce of THIS signer as we only care about our own votes, not other signer votes From 8b49cfa0944c4bca86554d0e0b2d9fb4855a43ef Mon Sep 17 00:00:00 2001 From: Jacinta Ferrant Date: Wed, 27 Mar 2024 10:09:06 -0400 Subject: [PATCH 2/6] CRC: always use the signer get coordinator but pass the reward cycle optionally Signed-off-by: Jacinta Ferrant --- stacks-signer/src/signer.rs | 20 +++++++++++--------- 1 file changed, 11 insertions(+), 9 deletions(-) diff --git a/stacks-signer/src/signer.rs b/stacks-signer/src/signer.rs index 6e0541ba50..53bfb6a139 100644 --- a/stacks-signer/src/signer.rs +++ b/stacks-signer/src/signer.rs @@ -185,10 +185,12 @@ 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) { - if self.reward_cycle == current_reward_cycle { + /// 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(&self, current_reward_cycle: Option) -> (Option, PublicKey) { + if Some(self.reward_cycle) == current_reward_cycle { let Some(ref cur_miner) = self.miner_key else { error!( "Signer #{}: Could not lookup current miner while in active reward cycle", @@ -415,7 +417,7 @@ impl Signer { stacks_client: &StacksClient, current_reward_cycle: u64, ) { - let coordinator_id = self.get_coordinator(current_reward_cycle).0; + let coordinator_id = self.get_coordinator(Some(current_reward_cycle)).0; match &self.state { State::Idle => { if coordinator_id != Some(self.signer_id) { @@ -445,7 +447,7 @@ impl Signer { res: Sender>, current_reward_cycle: u64, ) { - let coordinator_id = self.get_coordinator(current_reward_cycle).0; + let coordinator_id = self.get_coordinator(Some(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; @@ -556,7 +558,7 @@ impl Signer { messages: &[SignerMessage], current_reward_cycle: u64, ) { - let coordinator_pubkey = self.get_coordinator(current_reward_cycle).1; + let coordinator_pubkey = self.get_coordinator(Some(current_reward_cycle)).1; let packets: Vec = messages .iter() .filter_map(|msg| match msg { @@ -1226,8 +1228,8 @@ impl Signer { } return Ok(()); }; - let coordinator_id = self.coordinator_selector.get_coordinator().0; - if self.signer_id == coordinator_id && self.state == State::Idle { + let coordinator_id = self.get_coordinator(None).0; + if Some(self.signer_id) == coordinator_id && self.state == State::Idle { debug!("{self}: Checking if old vote transaction exists in StackerDB..."); // Have I already voted and have a pending transaction? Check stackerdb for the same round number and reward cycle vote transaction // Only get the account nonce of THIS signer as we only care about our own votes, not other signer votes From c3cfa2aee0a242b8a5dcc3866ab4b0a9e82ffef6 Mon Sep 17 00:00:00 2001 From: Jacinta Ferrant Date: Thu, 28 Mar 2024 09:20:41 -0400 Subject: [PATCH 3/6] CRC: create a dkg and sign coordiantor selector functions and use where appropro Signed-off-by: Jacinta Ferrant --- stacks-signer/src/signer.rs | 89 +++++++++++++++++++++---------------- 1 file changed, 51 insertions(+), 38 deletions(-) diff --git a/stacks-signer/src/signer.rs b/stacks-signer/src/signer.rs index 53bfb6a139..625428ef5b 100644 --- a/stacks-signer/src/signer.rs +++ b/stacks-signer/src/signer.rs @@ -189,8 +189,8 @@ impl Signer { /// 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(&self, current_reward_cycle: Option) -> (Option, PublicKey) { - if Some(self.reward_cycle) == current_reward_cycle { + 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!( "Signer #{}: Could not lookup current miner while in active reward cycle", @@ -206,6 +206,12 @@ impl Signer { return (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 { @@ -417,24 +423,34 @@ impl Signer { stacks_client: &StacksClient, current_reward_cycle: u64, ) { - let coordinator_id = self.get_coordinator(Some(current_reward_cycle)).0; match &self.state { State::Idle => { + let Some(command) = self.commands.pop_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...", ); + // Put the command back in the queue for later processing. + self.commands.push_front(command); return; } - if let Some(command) = self.commands.pop_front() { - self.execute_command(stacks_client, &command); - } else { - debug!("{self}: Nothing to process. Waiting for command...",); - } + 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 + ); } } } @@ -447,7 +463,6 @@ impl Signer { res: Sender>, current_reward_cycle: u64, ) { - let coordinator_id = self.get_coordinator(Some(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; @@ -519,32 +534,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")); @@ -558,7 +554,6 @@ impl Signer { messages: &[SignerMessage], current_reward_cycle: u64, ) { - let coordinator_pubkey = self.get_coordinator(Some(current_reward_cycle)).1; let packets: Vec = messages .iter() .filter_map(|msg| match msg { @@ -567,6 +562,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) } }) @@ -631,6 +631,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( @@ -1228,8 +1241,8 @@ impl Signer { } return Ok(()); }; - let coordinator_id = self.get_coordinator(None).0; - if Some(self.signer_id) == coordinator_id && self.state == State::Idle { + let coordinator_id = self.get_coordinator_dkg().0; + if self.signer_id == coordinator_id && self.state == State::Idle { debug!("{self}: Checking if old vote transaction exists in StackerDB..."); // Have I already voted and have a pending transaction? Check stackerdb for the same round number and reward cycle vote transaction // Only get the account nonce of THIS signer as we only care about our own votes, not other signer votes From 07db6843691410052c0b20ee4f26bba53f70d960 Mon Sep 17 00:00:00 2001 From: Jacinta Ferrant Date: Thu, 28 Mar 2024 09:27:50 -0400 Subject: [PATCH 4/6] Clippy was driving me bananas Signed-off-by: Jacinta Ferrant --- stacks-signer/src/cli.rs | 2 +- stacks-signer/src/coordinator.rs | 18 ++++++++---------- stacks-signer/src/main.rs | 1 + stacks-signer/src/signer.rs | 18 ++++++++---------- stacks-signer/src/signerdb.rs | 18 +++++++++--------- 5 files changed, 27 insertions(+), 30 deletions(-) diff --git a/stacks-signer/src/cli.rs b/stacks-signer/src/cli.rs index 0bed4038e4..45d44dfd05 100644 --- a/stacks-signer/src/cli.rs +++ b/stacks-signer/src/cli.rs @@ -264,7 +264,7 @@ fn parse_contract(contract: &str) -> Result pub fn parse_pox_addr(pox_address_literal: &str) -> Result { PoxAddress::from_b58(pox_address_literal).map_or_else( || Err(format!("Invalid pox address: {pox_address_literal}")), - |pox_address| Ok(pox_address), + Ok, ) } diff --git a/stacks-signer/src/coordinator.rs b/stacks-signer/src/coordinator.rs index 7469c0ff18..31598cb990 100644 --- a/stacks-signer/src/coordinator.rs +++ b/stacks-signer/src/coordinator.rs @@ -91,17 +91,15 @@ impl CoordinatorSelector { } } new_index - } 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 + } 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_id = *self .coordinator_ids diff --git a/stacks-signer/src/main.rs b/stacks-signer/src/main.rs index b1d154a9fa..5bd2bfeea5 100644 --- a/stacks-signer/src/main.rs +++ b/stacks-signer/src/main.rs @@ -398,6 +398,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/signer.rs b/stacks-signer/src/signer.rs index 625428ef5b..c02a0e6fb7 100644 --- a/stacks-signer/src/signer.rs +++ b/stacks-signer/src/signer.rs @@ -200,10 +200,10 @@ 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) } } @@ -938,7 +938,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"); @@ -1001,14 +1001,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(|| { diff --git a/stacks-signer/src/signerdb.rs b/stacks-signer/src/signerdb.rs index 247bea327c..12db86ab55 100644 --- a/stacks-signer/src/signerdb.rs +++ b/stacks-signer/src/signerdb.rs @@ -32,7 +32,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, @@ -80,11 +80,11 @@ impl SignerDb { let result: Option = query_row( &self.db, "SELECT block_info FROM blocks WHERE reward_cycle = ? AND signer_signature_hash = ?", - &[&reward_cycle.to_string(), &format!("{}", hash)], + [&reward_cycle.to_string(), &format!("{}", hash)], )?; if let Some(block_info) = result { let block_info: BlockInfo = - serde_json::from_str(&block_info).map_err(|e| DBError::SerializationError(e))?; + serde_json::from_str(&block_info).map_err(DBError::SerializationError)?; Ok(Some(block_info)) } else { Ok(None) @@ -116,13 +116,13 @@ impl SignerDb { self.db .execute( "INSERT OR REPLACE INTO blocks (reward_cycle, signer_signature_hash, block_info) VALUES (?1, ?2, ?3)", - &[reward_cycle.to_string(), format!("{}", hash), block_json], + [reward_cycle.to_string(), format!("{}", hash), block_json], ) .map_err(|e| { - return DBError::Other(format!( + DBError::Other(format!( "Unable to insert block into db: {:?}", e.to_string() - )); + )) })?; Ok(()) } @@ -136,7 +136,7 @@ impl SignerDb { debug!("Deleting block_info: sighash = {hash}"); self.db.execute( "DELETE FROM blocks WHERE reward_cycle = ? AND signer_signature_hash = ?", - &[reward_cycle.to_string(), format!("{}", hash)], + [reward_cycle.to_string(), format!("{}", hash)], )?; Ok(()) @@ -147,8 +147,8 @@ impl SignerDb { 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") } From ccfba633815dfbc2f59c96b2c5727236b5a99853 Mon Sep 17 00:00:00 2001 From: Jacinta Ferrant Date: Tue, 2 Apr 2024 09:29:06 -0400 Subject: [PATCH 5/6] CRC: do not pop the command until its executed Signed-off-by: Jacinta Ferrant --- stacks-signer/src/signer.rs | 8 +++++--- 1 file changed, 5 insertions(+), 3 deletions(-) diff --git a/stacks-signer/src/signer.rs b/stacks-signer/src/signer.rs index 6ab54b692b..0ea64c70c1 100644 --- a/stacks-signer/src/signer.rs +++ b/stacks-signer/src/signer.rs @@ -439,7 +439,7 @@ impl Signer { ) { match &self.state { State::Idle => { - let Some(command) = self.commands.pop_front() else { + let Some(command) = self.commands.front() else { debug!("{self}: Nothing to process. Waiting for command..."); return; }; @@ -453,10 +453,12 @@ impl Signer { debug!( "{self}: Coordinator is {coordinator_id:?}. Will not process any commands...", ); - // Put the command back in the queue for later processing. - self.commands.push_front(command); return; } + 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 => { From 803ab477ac639350dfe9d016b48c3eb1ed149892 Mon Sep 17 00:00:00 2001 From: Jacinta Ferrant Date: Wed, 3 Apr 2024 12:38:15 -0400 Subject: [PATCH 6/6] CRC: remove duplicate logging Signed-off-by: Jacinta Ferrant --- stacks-signer/src/signer.rs | 1 - 1 file changed, 1 deletion(-) diff --git a/stacks-signer/src/signer.rs b/stacks-signer/src/signer.rs index 0ea64c70c1..e2b1eca2d5 100644 --- a/stacks-signer/src/signer.rs +++ b/stacks-signer/src/signer.rs @@ -1275,7 +1275,6 @@ impl Signer { return Ok(()); } debug!("{self}: Checking if old DKG vote transaction exists in StackerDB..."); - debug!("{self}: Checking if old DKG vote transaction exists in StackerDB..."); // Have I already voted, but the vote is still pending in StackerDB? Check stackerdb for the same round number and reward cycle vote transaction // Only get the account nonce of THIS signer as we only care about our own votes, not other signer votes let signer_address = stacks_client.get_signer_address();