From 63394526ea45696b7751a6ba0871d2a1b3d40833 Mon Sep 17 00:00:00 2001 From: Jacinta Ferrant Date: Mon, 25 Mar 2024 19:05:37 -0400 Subject: [PATCH 01/23] 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 02/23] 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 03/23] 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 04/23] 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 ee5c72665f0716a9342dac642ef4f9bd0bf42d85 Mon Sep 17 00:00:00 2001 From: Jacinta Ferrant Date: Fri, 22 Mar 2024 16:52:22 -0400 Subject: [PATCH 05/23] Remove unnecessary retries throughout code Signed-off-by: Jacinta Ferrant --- stacks-signer/src/client/stackerdb.rs | 14 +++--- stacks-signer/src/client/stacks_client.rs | 40 +++++++---------- stacks-signer/src/runloop.rs | 11 +---- stacks-signer/src/signer.rs | 54 +++++++++-------------- 4 files changed, 46 insertions(+), 73 deletions(-) diff --git a/stacks-signer/src/client/stackerdb.rs b/stacks-signer/src/client/stackerdb.rs index 691cde08cc..4b6e993bcf 100644 --- a/stacks-signer/src/client/stackerdb.rs +++ b/stacks-signer/src/client/stackerdb.rs @@ -100,7 +100,7 @@ impl StackerDB { } /// Sends message (as a raw msg ID and bytes) to the .signers stacker-db with an - /// exponential backoff retry + /// exponential backoff retry pub fn send_message_bytes_with_retry( &mut self, msg_id: &MessageSlotID, @@ -224,9 +224,7 @@ impl StackerDB { } /// Get this signer's latest transactions from stackerdb - pub fn get_current_transactions_with_retry( - &mut self, - ) -> Result, ClientError> { + pub fn get_current_transactions(&mut self) -> Result, ClientError> { let Some(transactions_session) = self .signers_message_stackerdb_sessions .get_mut(&MessageSlotID::Transactions) @@ -237,7 +235,7 @@ impl StackerDB { } /// Get the latest signer transactions from signer ids for the next reward cycle - pub fn get_next_transactions_with_retry( + pub fn get_next_transactions( &mut self, signer_ids: &[SignerSlotID], ) -> Result, ClientError> { @@ -272,7 +270,7 @@ mod tests { use crate::config::GlobalConfig; #[test] - fn get_signer_transactions_with_retry_should_succeed() { + fn get_signer_transactions_should_succeed() { let config = GlobalConfig::load_from_file("./src/tests/conf/signer-0.toml").unwrap(); let signer_config = generate_signer_config(&config, 5, 20); let mut stackerdb = StackerDB::from(&signer_config); @@ -297,7 +295,7 @@ mod tests { let message = signer_message.serialize_to_vec(); let signer_slot_ids = vec![SignerSlotID(0), SignerSlotID(1)]; - let h = spawn(move || stackerdb.get_next_transactions_with_retry(&signer_slot_ids)); + let h = spawn(move || stackerdb.get_next_transactions(&signer_slot_ids)); let mut response_bytes = b"HTTP/1.1 200 OK\n\n".to_vec(); response_bytes.extend(message); let mock_server = mock_server_from_config(&config); @@ -315,7 +313,7 @@ mod tests { } #[test] - fn send_signer_message_with_retry_should_succeed() { + fn send_signer_message_should_succeed() { let config = GlobalConfig::load_from_file("./src/tests/conf/signer-1.toml").unwrap(); let signer_config = generate_signer_config(&config, 5, 20); let mut stackerdb = StackerDB::from(&signer_config); diff --git a/stacks-signer/src/client/stacks_client.rs b/stacks-signer/src/client/stacks_client.rs index b0fb539dd1..5e5710a7fd 100644 --- a/stacks-signer/src/client/stacks_client.rs +++ b/stacks-signer/src/client/stacks_client.rs @@ -198,7 +198,7 @@ impl StacksClient { /// Determine the stacks node current epoch pub fn get_node_epoch(&self) -> Result { - let pox_info = self.get_pox_data_with_retry()?; + let pox_info = self.get_pox_data()?; let burn_block_height = self.get_burn_block_height()?; let epoch_25 = pox_info @@ -227,10 +227,7 @@ impl StacksClient { } /// Submit the block proposal to the stacks node. The block will be validated and returned via the HTTP endpoint for Block events. - pub fn submit_block_for_validation_with_retry( - &self, - block: NakamotoBlock, - ) -> Result<(), ClientError> { + pub fn submit_block_for_validation(&self, block: NakamotoBlock) -> Result<(), ClientError> { let block_proposal = NakamotoBlockProposal { block, chain_id: self.chain_id, @@ -275,12 +272,11 @@ impl StacksClient { /// Retrieve the current account nonce for the provided address pub fn get_account_nonce(&self, address: &StacksAddress) -> Result { - let account_entry = self.get_account_entry_with_retry(address)?; - Ok(account_entry.nonce) + self.get_account_entry(address).map(|entry| entry.nonce) } /// Get the current peer info data from the stacks node - pub fn get_peer_info_with_retry(&self) -> Result { + pub fn get_peer_info(&self) -> Result { debug!("Getting stacks node info..."); let send_request = || { self.stacks_node_client @@ -324,7 +320,7 @@ impl StacksClient { } /// Get the reward set signers from the stacks node for the given reward cycle - pub fn get_reward_set_signers_with_retry( + pub fn get_reward_set_signers( &self, reward_cycle: u64, ) -> Result>, ClientError> { @@ -344,7 +340,7 @@ impl StacksClient { } /// Retreive the current pox data from the stacks node - pub fn get_pox_data_with_retry(&self) -> Result { + pub fn get_pox_data(&self) -> Result { debug!("Getting pox data..."); let send_request = || { self.stacks_node_client @@ -362,13 +358,12 @@ impl StacksClient { /// Helper function to retrieve the burn tip height from the stacks node fn get_burn_block_height(&self) -> Result { - let peer_info = self.get_peer_info_with_retry()?; - Ok(peer_info.burn_block_height) + self.get_peer_info().map(|info| info.burn_block_height) } /// Get the current reward cycle info from the stacks node pub fn get_current_reward_cycle_info(&self) -> Result { - let pox_data = self.get_pox_data_with_retry()?; + let pox_data = self.get_pox_data()?; let blocks_mined = pox_data .current_burnchain_block_height .saturating_sub(pox_data.first_burnchain_block_height); @@ -386,7 +381,7 @@ impl StacksClient { } /// Helper function to retrieve the account info from the stacks node for a specific address - fn get_account_entry_with_retry( + fn get_account_entry( &self, address: &StacksAddress, ) -> Result { @@ -463,10 +458,7 @@ impl StacksClient { } /// Helper function to submit a transaction to the Stacks mempool - pub fn submit_transaction_with_retry( - &self, - tx: &StacksTransaction, - ) -> Result { + pub fn submit_transaction(&self, tx: &StacksTransaction) -> Result { let txid = tx.txid(); let tx = tx.serialize_to_vec(); let send_request = || { @@ -847,7 +839,7 @@ mod tests { + 1; let tx_clone = tx.clone(); - let h = spawn(move || mock.client.submit_transaction_with_retry(&tx_clone)); + let h = spawn(move || mock.client.submit_transaction(&tx_clone)); let request_bytes = write_response( mock.server, @@ -910,7 +902,7 @@ mod tests { nonce, ) .unwrap(); - mock.client.submit_transaction_with_retry(&tx) + mock.client.submit_transaction(&tx) }); let mock = MockServerClient::from_config(mock.config); write_response( @@ -1091,7 +1083,7 @@ mod tests { header, txs: vec![], }; - let h = spawn(move || mock.client.submit_block_for_validation_with_retry(block)); + let h = spawn(move || mock.client.submit_block_for_validation(block)); write_response(mock.server, b"HTTP/1.1 200 OK\n\n"); assert!(h.join().unwrap().is_ok()); } @@ -1115,7 +1107,7 @@ mod tests { header, txs: vec![], }; - let h = spawn(move || mock.client.submit_block_for_validation_with_retry(block)); + let h = spawn(move || mock.client.submit_block_for_validation(block)); write_response(mock.server, b"HTTP/1.1 404 Not Found\n\n"); assert!(h.join().unwrap().is_err()); } @@ -1124,7 +1116,7 @@ mod tests { fn get_peer_info_should_succeed() { let mock = MockServerClient::new(); let (response, peer_info) = build_get_peer_info_response(None, None); - let h = spawn(move || mock.client.get_peer_info_with_retry()); + let h = spawn(move || mock.client.get_peer_info()); write_response(mock.server, response.as_bytes()); assert_eq!(h.join().unwrap().unwrap(), peer_info); } @@ -1165,7 +1157,7 @@ mod tests { let stackers_response_json = serde_json::to_string(&stackers_response) .expect("Failed to serialize get stacker response"); let response = format!("HTTP/1.1 200 OK\n\n{stackers_response_json}"); - let h = spawn(move || mock.client.get_reward_set_signers_with_retry(0)); + let h = spawn(move || mock.client.get_reward_set_signers(0)); write_response(mock.server, response.as_bytes()); assert_eq!(h.join().unwrap().unwrap(), stacker_set.signers); } diff --git a/stacks-signer/src/runloop.rs b/stacks-signer/src/runloop.rs index 4491650090..32d3234464 100644 --- a/stacks-signer/src/runloop.rs +++ b/stacks-signer/src/runloop.rs @@ -128,10 +128,7 @@ impl RunLoop { reward_cycle: u64, ) -> Result, ClientError> { debug!("Getting registered signers for reward cycle {reward_cycle}..."); - let Some(signers) = self - .stacks_client - .get_reward_set_signers_with_retry(reward_cycle)? - else { + let Some(signers) = self.stacks_client.get_reward_set_signers(reward_cycle)? else { warn!("No reward set signers found for reward cycle {reward_cycle}."); return Ok(None); }; @@ -390,11 +387,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) - .map_err(backoff::Error::transient) - }) { + if let Err(e) = signer.update_dkg(&self.stacks_client, current_reward_cycle) { error!("{signer}: failed to update DKG: {e}"); } } diff --git a/stacks-signer/src/signer.rs b/stacks-signer/src/signer.rs index ea6ef534a6..5f66b48256 100644 --- a/stacks-signer/src/signer.rs +++ b/stacks-signer/src/signer.rs @@ -49,7 +49,7 @@ use wsts::state_machine::{OperationResult, SignError}; use wsts::traits::Signer as _; use wsts::v2; -use crate::client::{retry_with_exponential_backoff, ClientError, StackerDB, StacksClient}; +use crate::client::{ClientError, StackerDB, StacksClient}; use crate::config::SignerConfig; use crate::coordinator::CoordinatorSelector; use crate::signerdb::SignerDb; @@ -345,11 +345,7 @@ impl Signer { debug!("Reward cycle #{} Signer #{}: Already have an aggregate key. Ignoring DKG command.", self.reward_cycle, self.signer_id); return; } - let vote_round = match retry_with_exponential_backoff(|| { - stacks_client - .get_last_round(self.reward_cycle) - .map_err(backoff::Error::transient) - }) { + let vote_round = match stacks_client.get_last_round(self.reward_cycle) { Ok(last_round) => last_round, Err(e) => { error!("{self}: Unable to perform DKG. Failed to get last round from stacks node: {e:?}"); @@ -628,7 +624,7 @@ impl Signer { }); // Submit the block for validation stacks_client - .submit_block_for_validation_with_retry(proposal.block.clone()) + .submit_block_for_validation(proposal.block.clone()) .unwrap_or_else(|e| { warn!("{self}: Failed to submit block for validation: {e:?}"); }); @@ -770,7 +766,7 @@ impl Signer { ); let block_info = BlockInfo::new_with_request(block.clone(), nonce_request.clone()); stacks_client - .submit_block_for_validation_with_retry(block) + .submit_block_for_validation(block) .unwrap_or_else(|e| { warn!("{self}: Failed to submit block for validation: {e:?}",); }); @@ -857,7 +853,7 @@ impl Signer { ) -> Result, ClientError> { let transactions: Vec<_> = self .stackerdb - .get_current_transactions_with_retry()? + .get_current_transactions()? .into_iter() .filter_map(|tx| { if !NakamotoSigners::valid_vote_transaction(nonces, &tx, self.mainnet) { @@ -882,7 +878,7 @@ impl Signer { let account_nonces = self.get_account_nonces(stacks_client, &self.next_signer_addresses); let transactions: Vec<_> = self .stackerdb - .get_next_transactions_with_retry(&self.next_signer_slot_ids)?; + .get_next_transactions(&self.next_signer_slot_ids)?; let mut filtered_transactions = std::collections::HashMap::new(); NakamotoSigners::update_filtered_transactions( &mut filtered_transactions, @@ -1012,37 +1008,31 @@ impl Signer { "error" => %e); } } - - let epoch = retry_with_exponential_backoff(|| { - stacks_client - .get_node_epoch() - .map_err(backoff::Error::transient) - }) - .unwrap_or(StacksEpochId::Epoch24); - let tx_fee = if epoch < StacksEpochId::Epoch30 { - debug!("{self}: in pre Epoch 3.0 cycles, must set a transaction fee for the DKG vote."); - Some(self.tx_fee_ustx) - } else { - None - }; // Get our current nonce from the stacks node and compare it against what we have sitting in the stackerdb instance let signer_address = stacks_client.get_signer_address(); // Retreieve ALL account nonces as we may have transactions from other signers in our stackerdb slot that we care about let account_nonces = self.get_account_nonces(stacks_client, &self.signer_addresses); let account_nonce = account_nonces.get(signer_address).unwrap_or(&0); - let signer_transactions = retry_with_exponential_backoff(|| { - self.get_signer_transactions(&account_nonces) - .map_err(backoff::Error::transient) - }) - .map_err(|e| { - warn!("{self}: Unable to get signer transactions: {e:?}"); - }) - .unwrap_or_default(); + let signer_transactions = self + .get_signer_transactions(&account_nonces) + .map_err(|e| { + error!("{self}: Unable to get signer transactions: {e:?}."); + }) + .unwrap_or_default(); // If we have a transaction in the stackerdb slot, we need to increment the nonce hence the +1, else should use the account nonce let next_nonce = signer_transactions .first() .map(|tx| tx.get_origin_nonce().wrapping_add(1)) .unwrap_or(*account_nonce); + let epoch = stacks_client + .get_node_epoch() + .unwrap_or(StacksEpochId::Epoch24); + let tx_fee = if epoch < StacksEpochId::Epoch30 { + debug!("{self}: in pre Epoch 3.0 cycles, must set a transaction fee for the DKG vote."); + Some(self.tx_fee_ustx) + } else { + None + }; match stacks_client.build_vote_for_aggregate_public_key( self.stackerdb.get_signer_slot_id().0, self.coordinator.current_dkg_id, @@ -1108,7 +1098,7 @@ impl Signer { debug!("{self}: Received a DKG result while in epoch 3.0. Broadcast the transaction only to stackerDB."); } else if epoch == StacksEpochId::Epoch25 { debug!("{self}: Received a DKG result while in epoch 2.5. Broadcast the transaction to the mempool."); - stacks_client.submit_transaction_with_retry(&new_transaction)?; + stacks_client.submit_transaction(&new_transaction)?; info!("{self}: Submitted DKG vote transaction ({txid:?}) to the mempool"); } else { debug!("{self}: Received a DKG result, but are in an unsupported epoch. Do not broadcast the transaction ({}).", new_transaction.txid()); From 7a970ac37fa09382fe52cd5640ef6536f029b4c4 Mon Sep 17 00:00:00 2001 From: Jacinta Ferrant Date: Fri, 22 Mar 2024 17:15:47 -0400 Subject: [PATCH 06/23] Add get_medium_estimated_fee_ustx to get estimated fee from stacks node Signed-off-by: Jacinta Ferrant --- stacks-signer/src/client/mod.rs | 39 +++++++++++++ stacks-signer/src/client/stacks_client.rs | 67 ++++++++++++++++++++++- 2 files changed, 105 insertions(+), 1 deletion(-) diff --git a/stacks-signer/src/client/mod.rs b/stacks-signer/src/client/mod.rs index 15d5d256a3..52585328e4 100644 --- a/stacks-signer/src/client/mod.rs +++ b/stacks-signer/src/client/mod.rs @@ -116,6 +116,7 @@ pub(crate) mod tests { use blockstack_lib::net::api::getpoxinfo::{ RPCPoxCurrentCycleInfo, RPCPoxEpoch, RPCPoxInfoData, RPCPoxNextCycleInfo, }; + use blockstack_lib::net::api::postfeerate::{RPCFeeEstimate, RPCFeeEstimateResponse}; use blockstack_lib::util_lib::boot::boot_code_id; use clarity::vm::costs::ExecutionCost; use clarity::vm::types::TupleData; @@ -398,6 +399,44 @@ pub(crate) mod tests { format!("HTTP/1.1 200 OK\n\n{{\"okay\":true,\"result\":\"{hex}\"}}") } + /// Build a response for the get_medium_estimated_fee_ustx_response request with a specific medium estimate + pub fn build_get_medium_estimated_fee_ustx_response( + medium_estimate: u64, + ) -> (String, RPCFeeEstimateResponse) { + // Generate some random info + let fee_response = RPCFeeEstimateResponse { + estimated_cost: ExecutionCost { + write_length: thread_rng().next_u64(), + write_count: thread_rng().next_u64(), + read_length: thread_rng().next_u64(), + read_count: thread_rng().next_u64(), + runtime: thread_rng().next_u64(), + }, + estimated_cost_scalar: thread_rng().next_u64(), + cost_scalar_change_by_byte: thread_rng().next_u32() as f64, + estimations: vec![ + RPCFeeEstimate { + fee_rate: thread_rng().next_u32() as f64, + fee: thread_rng().next_u64(), + }, + RPCFeeEstimate { + fee_rate: thread_rng().next_u32() as f64, + fee: medium_estimate, + }, + RPCFeeEstimate { + fee_rate: thread_rng().next_u32() as f64, + fee: thread_rng().next_u64(), + }, + ], + }; + let fee_response_json = serde_json::to_string(&fee_response) + .expect("Failed to serialize fee estimate response"); + ( + format!("HTTP/1.1 200 OK\n\n{fee_response_json}"), + fee_response, + ) + } + /// Generate a signer config with the given number of signers and keys where the first signer is /// obtained from the provided global config pub fn generate_signer_config( diff --git a/stacks-signer/src/client/stacks_client.rs b/stacks-signer/src/client/stacks_client.rs index 5e5710a7fd..3fbc59bfe2 100644 --- a/stacks-signer/src/client/stacks_client.rs +++ b/stacks-signer/src/client/stacks_client.rs @@ -31,7 +31,9 @@ use blockstack_lib::net::api::getinfo::RPCPeerInfoData; use blockstack_lib::net::api::getpoxinfo::RPCPoxInfoData; use blockstack_lib::net::api::getstackers::GetStackersResponse; use blockstack_lib::net::api::postblock_proposal::NakamotoBlockProposal; +use blockstack_lib::net::api::postfeerate::{FeeRateEstimateRequestBody, RPCFeeEstimateResponse}; use blockstack_lib::util_lib::boot::{boot_code_addr, boot_code_id}; +use clarity::util::hash::to_hex; use clarity::vm::types::{PrincipalData, QualifiedContractIdentifier}; use clarity::vm::{ClarityName, ContractName, Value as ClarityValue}; use reqwest::header::AUTHORIZATION; @@ -196,6 +198,40 @@ impl StacksClient { } } + /// Retrieve the medium estimated transaction fee in uSTX from the stacks node for the given transaction + pub fn get_medium_estimated_fee_ustx( + &self, + tx: &StacksTransaction, + ) -> Result { + let request = FeeRateEstimateRequestBody { + estimated_len: Some(tx.tx_len()), + transaction_payload: to_hex(&tx.payload.serialize_to_vec()), + }; + let send_request = || { + self.stacks_node_client + .post(self.fees_transaction_path()) + .header("Content-Type", "application/json") + .json(&request) + .send() + .map_err(backoff::Error::transient) + }; + let response = retry_with_exponential_backoff(send_request)?; + if !response.status().is_success() { + return Err(ClientError::RequestFailure(response.status())); + } + let fee_estimate_response = response.json::()?; + let fee = fee_estimate_response + .estimations + .get(1) + .map(|estimate| estimate.fee) + .ok_or_else(|| { + ClientError::UnexpectedResponseFormat( + "RPCFeeEstimateResponse missing medium fee estimate".into(), + ) + })?; + Ok(fee) + } + /// Determine the stacks node current epoch pub fn get_node_epoch(&self) -> Result { let pox_info = self.get_pox_data()?; @@ -560,6 +596,10 @@ impl StacksClient { format!("{}/v2/stacker_set/{reward_cycle}", self.http_origin) } + fn fees_transaction_path(&self) -> String { + format!("{}/v2/fees/transaction", self.http_origin) + } + /// Helper function to create a stacks transaction for a modifying contract call #[allow(clippy::too_many_arguments)] pub fn build_signed_contract_call_transaction( @@ -634,7 +674,8 @@ mod tests { use super::*; use crate::client::tests::{ build_account_nonce_response, build_get_approved_aggregate_key_response, - build_get_last_round_response, build_get_peer_info_response, build_get_pox_data_response, + build_get_last_round_response, build_get_medium_estimated_fee_ustx_response, + build_get_peer_info_response, build_get_pox_data_response, build_get_vote_for_aggregate_key_response, build_read_only_response, write_response, MockServerClient, }; @@ -1185,4 +1226,28 @@ mod tests { write_response(mock.server, key_response.as_bytes()); assert_eq!(h.join().unwrap().unwrap(), None); } + + #[test] + fn get_medium_estimated_fee_ustx_should_succeed() { + let mock = MockServerClient::new(); + let private_key = StacksPrivateKey::new(); + let unsigned_tx = StacksClient::build_signed_contract_call_transaction( + &mock.client.stacks_address, + ContractName::from("contract-name"), + ClarityName::from("function-name"), + &[], + &private_key, + TransactionVersion::Testnet, + CHAIN_ID_TESTNET, + 10_000, + 0, + ) + .unwrap(); + + let estimate = thread_rng().next_u64(); + let response = build_get_medium_estimated_fee_ustx_response(estimate).0; + let h = spawn(move || mock.client.get_medium_estimated_fee_ustx(&unsigned_tx)); + write_response(mock.server, response.as_bytes()); + assert_eq!(h.join().unwrap().unwrap(), estimate); + } } From abfc68a3a3a7bedce9f5866b0b89344345038590 Mon Sep 17 00:00:00 2001 From: Jacinta Ferrant Date: Fri, 22 Mar 2024 17:31:40 -0400 Subject: [PATCH 07/23] Seperate unsigned and signed transaction building and update tests Signed-off-by: Jacinta Ferrant --- stacks-signer/src/client/stacks_client.rs | 73 +++++++++++------------ stacks-signer/src/signer.rs | 17 ++++-- testnet/stacks-node/src/tests/signer.rs | 38 ++++++------ 3 files changed, 66 insertions(+), 62 deletions(-) diff --git a/stacks-signer/src/client/stacks_client.rs b/stacks-signer/src/client/stacks_client.rs index 3fbc59bfe2..0f6e272217 100644 --- a/stacks-signer/src/client/stacks_client.rs +++ b/stacks-signer/src/client/stacks_client.rs @@ -459,13 +459,12 @@ impl StacksClient { } /// Helper function to create a stacks transaction for a modifying contract call - pub fn build_vote_for_aggregate_public_key( + pub fn build_unsigned_vote_for_aggregate_public_key( &self, signer_index: u32, round: u64, dkg_public_key: Point, reward_cycle: u64, - tx_fee: Option, nonce: u64, ) -> Result { debug!("Building {SIGNERS_VOTING_FUNCTION_NAME} transaction..."); @@ -478,9 +477,8 @@ impl StacksClient { ClarityValue::UInt(round as u128), ClarityValue::UInt(reward_cycle as u128), ]; - let tx_fee = tx_fee.unwrap_or(0); - Self::build_signed_contract_call_transaction( + let unsigned_tx = Self::build_unsigned_contract_call_transaction( &contract_address, contract_name, function_name, @@ -489,8 +487,8 @@ impl StacksClient { self.tx_version, self.chain_id, nonce, - tx_fee, - ) + )?; + Ok(unsigned_tx) } /// Helper function to submit a transaction to the Stacks mempool @@ -602,7 +600,7 @@ impl StacksClient { /// Helper function to create a stacks transaction for a modifying contract call #[allow(clippy::too_many_arguments)] - pub fn build_signed_contract_call_transaction( + pub fn build_unsigned_contract_call_transaction( contract_addr: &StacksAddress, contract_name: ContractName, function_name: ClarityName, @@ -611,7 +609,6 @@ impl StacksClient { tx_version: TransactionVersion, chain_id: u32, nonce: u64, - tx_fee: u64, ) -> Result { let tx_payload = TransactionPayload::ContractCall(TransactionContractCall { address: *contract_addr, @@ -630,17 +627,22 @@ impl StacksClient { ); let mut unsigned_tx = StacksTransaction::new(tx_version, tx_auth, tx_payload); - - unsigned_tx.set_tx_fee(tx_fee); unsigned_tx.set_origin_nonce(nonce); unsigned_tx.anchor_mode = TransactionAnchorMode::Any; unsigned_tx.post_condition_mode = TransactionPostConditionMode::Allow; unsigned_tx.chain_id = chain_id; + Ok(unsigned_tx) + } + /// Sign an unsigned transaction + pub fn sign_transaction( + &self, + unsigned_tx: StacksTransaction, + ) -> Result { let mut tx_signer = StacksTransactionSigner::new(&unsigned_tx); tx_signer - .sign_origin(stacks_private_key) + .sign_origin(&self.stacks_private_key) .map_err(|e| ClientError::TransactionGenerationFailure(e.to_string()))?; tx_signer @@ -845,12 +847,11 @@ mod tests { assert!(result.is_err()) } - #[ignore] #[test] fn transaction_contract_call_should_send_bytes_to_node() { let mock = MockServerClient::new(); let private_key = StacksPrivateKey::new(); - let tx = StacksClient::build_signed_contract_call_transaction( + let unsigned_tx = StacksClient::build_unsigned_contract_call_transaction( &mock.client.stacks_address, ContractName::from("contract-name"), ClarityName::from("function-name"), @@ -859,10 +860,11 @@ mod tests { TransactionVersion::Testnet, CHAIN_ID_TESTNET, 0, - 10_000, ) .unwrap(); + let tx = mock.client.sign_transaction(unsigned_tx).unwrap(); + let mut tx_bytes = [0u8; 1024]; { let mut tx_bytes_writer = BufWriter::new(&mut tx_bytes[..]); @@ -897,7 +899,6 @@ mod tests { ); } - #[ignore] #[test] fn build_vote_for_aggregate_public_key_should_succeed() { let mock = MockServerClient::new(); @@ -908,19 +909,17 @@ mod tests { let reward_cycle = thread_rng().next_u64(); let h = spawn(move || { - mock.client.build_vote_for_aggregate_public_key( + mock.client.build_unsigned_vote_for_aggregate_public_key( signer_index, round, point, reward_cycle, - None, nonce, ) }); assert!(h.join().unwrap().is_ok()); } - #[ignore] #[test] fn broadcast_vote_for_aggregate_public_key_should_succeed() { let mock = MockServerClient::new(); @@ -929,28 +928,27 @@ mod tests { let signer_index = thread_rng().next_u32(); let round = thread_rng().next_u64(); let reward_cycle = thread_rng().next_u64(); + let unsigned_tx = mock + .client + .build_unsigned_vote_for_aggregate_public_key( + signer_index, + round, + point, + reward_cycle, + nonce, + ) + .unwrap(); + let tx = mock.client.sign_transaction(unsigned_tx).unwrap(); + let tx_clone = tx.clone(); + let h = spawn(move || mock.client.submit_transaction(&tx_clone)); - let h = spawn(move || { - let tx = mock - .client - .clone() - .build_vote_for_aggregate_public_key( - signer_index, - round, - point, - reward_cycle, - None, - nonce, - ) - .unwrap(); - mock.client.submit_transaction(&tx) - }); - let mock = MockServerClient::from_config(mock.config); write_response( mock.server, - b"HTTP/1.1 200 OK\n\n4e99f99bc4a05437abb8c7d0c306618f45b203196498e2ebe287f10497124958", + format!("HTTP/1.1 200 OK\n\n{}", tx.txid()).as_bytes(), ); - assert!(h.join().unwrap().is_ok()); + let returned_txid = h.join().unwrap().unwrap(); + + assert_eq!(returned_txid, tx.txid()); } #[test] @@ -1231,7 +1229,7 @@ mod tests { fn get_medium_estimated_fee_ustx_should_succeed() { let mock = MockServerClient::new(); let private_key = StacksPrivateKey::new(); - let unsigned_tx = StacksClient::build_signed_contract_call_transaction( + let unsigned_tx = StacksClient::build_unsigned_contract_call_transaction( &mock.client.stacks_address, ContractName::from("contract-name"), ClarityName::from("function-name"), @@ -1239,7 +1237,6 @@ mod tests { &private_key, TransactionVersion::Testnet, CHAIN_ID_TESTNET, - 10_000, 0, ) .unwrap(); diff --git a/stacks-signer/src/signer.rs b/stacks-signer/src/signer.rs index 5f66b48256..7059c5e1fb 100644 --- a/stacks-signer/src/signer.rs +++ b/stacks-signer/src/signer.rs @@ -1029,19 +1029,26 @@ impl Signer { .unwrap_or(StacksEpochId::Epoch24); let tx_fee = if epoch < StacksEpochId::Epoch30 { debug!("{self}: in pre Epoch 3.0 cycles, must set a transaction fee for the DKG vote."); - Some(self.tx_fee_ustx) + self.tx_fee_ustx } else { - None + 0 }; - match stacks_client.build_vote_for_aggregate_public_key( + match stacks_client.build_unsigned_vote_for_aggregate_public_key( self.stackerdb.get_signer_slot_id().0, self.coordinator.current_dkg_id, *dkg_public_key, self.reward_cycle, - tx_fee, next_nonce, ) { - Ok(new_transaction) => { + Ok(mut unsigned_tx) => { + unsigned_tx.set_tx_fee(tx_fee); + let new_transaction = match stacks_client.sign_transaction(unsigned_tx) { + Ok(tx) => tx, + Err(e) => { + warn!("{self}: Failed to sign DKG public key vote transaction: {e:?}."); + return; + } + }; if let Err(e) = self.broadcast_dkg_vote( stacks_client, epoch, diff --git a/testnet/stacks-node/src/tests/signer.rs b/testnet/stacks-node/src/tests/signer.rs index 01024343db..7b2e938a1a 100644 --- a/testnet/stacks-node/src/tests/signer.rs +++ b/testnet/stacks-node/src/tests/signer.rs @@ -549,7 +549,7 @@ impl SignerTest { None, ), }; - let invalid_contract_address = StacksClient::build_signed_contract_call_transaction( + let invalid_contract_address = StacksClient::build_unsigned_contract_call_transaction( &StacksAddress::p2pkh(false, &StacksPublicKey::from_private(&signer_private_key)), contract_name.clone(), SIGNERS_VOTING_FUNCTION_NAME.into(), @@ -558,11 +558,10 @@ impl SignerTest { TransactionVersion::Testnet, CHAIN_ID_TESTNET, 1, - 10, ) .unwrap(); - let invalid_contract_name = StacksClient::build_signed_contract_call_transaction( + let invalid_contract_name = StacksClient::build_unsigned_contract_call_transaction( &contract_addr, "bad-signers-contract-name".into(), SIGNERS_VOTING_FUNCTION_NAME.into(), @@ -571,11 +570,10 @@ impl SignerTest { TransactionVersion::Testnet, CHAIN_ID_TESTNET, 1, - 10, ) .unwrap(); - let invalid_signers_vote_function = StacksClient::build_signed_contract_call_transaction( + let invalid_signers_vote_function = StacksClient::build_unsigned_contract_call_transaction( &contract_addr, contract_name.clone(), "some-other-function".into(), @@ -584,12 +582,11 @@ impl SignerTest { TransactionVersion::Testnet, CHAIN_ID_TESTNET, 1, - 10, ) .unwrap(); let invalid_function_arg_signer_index = - StacksClient::build_signed_contract_call_transaction( + StacksClient::build_unsigned_contract_call_transaction( &contract_addr, contract_name.clone(), SIGNERS_VOTING_FUNCTION_NAME.into(), @@ -603,11 +600,10 @@ impl SignerTest { TransactionVersion::Testnet, CHAIN_ID_TESTNET, 1, - 10, ) .unwrap(); - let invalid_function_arg_key = StacksClient::build_signed_contract_call_transaction( + let invalid_function_arg_key = StacksClient::build_unsigned_contract_call_transaction( &contract_addr, contract_name.clone(), SIGNERS_VOTING_FUNCTION_NAME.into(), @@ -621,11 +617,10 @@ impl SignerTest { TransactionVersion::Testnet, CHAIN_ID_TESTNET, 1, - 10, ) .unwrap(); - let invalid_function_arg_round = StacksClient::build_signed_contract_call_transaction( + let invalid_function_arg_round = StacksClient::build_unsigned_contract_call_transaction( &contract_addr, contract_name.clone(), SIGNERS_VOTING_FUNCTION_NAME.into(), @@ -639,12 +634,11 @@ impl SignerTest { TransactionVersion::Testnet, CHAIN_ID_TESTNET, 1, - 10, ) .unwrap(); let invalid_function_arg_reward_cycle = - StacksClient::build_signed_contract_call_transaction( + StacksClient::build_unsigned_contract_call_transaction( &contract_addr, contract_name.clone(), SIGNERS_VOTING_FUNCTION_NAME.into(), @@ -658,11 +652,10 @@ impl SignerTest { TransactionVersion::Testnet, CHAIN_ID_TESTNET, 1, - 10, ) .unwrap(); - let invalid_nonce = StacksClient::build_signed_contract_call_transaction( + let invalid_nonce = StacksClient::build_unsigned_contract_call_transaction( &contract_addr, contract_name.clone(), SIGNERS_VOTING_FUNCTION_NAME.into(), @@ -671,7 +664,6 @@ impl SignerTest { TransactionVersion::Testnet, CHAIN_ID_TESTNET, 0, // Old nonce - 10, ) .unwrap(); @@ -682,10 +674,10 @@ impl SignerTest { false, ); let invalid_signer_tx = invalid_stacks_client - .build_vote_for_aggregate_public_key(0, round, point, reward_cycle, None, 0) + .build_unsigned_vote_for_aggregate_public_key(0, round, point, reward_cycle, 0) .expect("FATAL: failed to build vote for aggregate public key"); - vec![ + let unsigned_txs = vec![ invalid_nonce, invalid_not_contract_call, invalid_contract_name, @@ -696,7 +688,15 @@ impl SignerTest { invalid_function_arg_round, invalid_function_arg_signer_index, invalid_signer_tx, - ] + ]; + unsigned_txs + .into_iter() + .map(|unsigned| { + invalid_stacks_client + .sign_transaction(unsigned) + .expect("Failed to sign transaction") + }) + .collect() } /// Kills the signer runloop at index `signer_idx` From 00fd7db2525306124aa19ccb9d6c641d129e3861 Mon Sep 17 00:00:00 2001 From: Jacinta Ferrant Date: Fri, 22 Mar 2024 17:59:06 -0400 Subject: [PATCH 08/23] Update stacks-signer to attempt to estimate the transaction fee if a user sets max_tx_fee_ustx in the config Signed-off-by: Jacinta Ferrant --- stacks-signer/src/client/mod.rs | 1 + stacks-signer/src/config.rs | 30 ++++-- stacks-signer/src/coordinator.rs | 18 ++-- stacks-signer/src/main.rs | 4 + stacks-signer/src/runloop.rs | 1 + stacks-signer/src/signer.rs | 133 ++++++++++++++---------- stacks-signer/src/signerdb.rs | 6 +- testnet/stacks-node/src/tests/signer.rs | 1 + 8 files changed, 118 insertions(+), 76 deletions(-) diff --git a/stacks-signer/src/client/mod.rs b/stacks-signer/src/client/mod.rs index 52585328e4..25492acf94 100644 --- a/stacks-signer/src/client/mod.rs +++ b/stacks-signer/src/client/mod.rs @@ -554,6 +554,7 @@ pub(crate) mod tests { nonce_timeout: config.nonce_timeout, sign_timeout: config.sign_timeout, tx_fee_ustx: config.tx_fee_ustx, + max_tx_fee_ustx: config.max_tx_fee_ustx, db_path: config.db_path.clone(), } } diff --git a/stacks-signer/src/config.rs b/stacks-signer/src/config.rs index 523536fce0..30b51ae025 100644 --- a/stacks-signer/src/config.rs +++ b/stacks-signer/src/config.rs @@ -33,8 +33,7 @@ use wsts::curve::scalar::Scalar; use crate::signer::SignerSlotID; const EVENT_TIMEOUT_MS: u64 = 5000; -// Default transaction fee in microstacks (if unspecificed in the config file) -// TODO: Use the fee estimation endpoint to get the default fee. +// Default transaction fee to use in microstacks (if unspecificed in the config file) const TX_FEE_USTX: u64 = 10_000; #[derive(thiserror::Error, Debug)] @@ -143,8 +142,10 @@ pub struct SignerConfig { pub nonce_timeout: Option, /// timeout to gather signature shares pub sign_timeout: Option, - /// the STX tx fee to use in uSTX + /// the STX tx fee to use in uSTX. pub tx_fee_ustx: u64, + /// If set, will use the estimated fee up to this amount. + pub max_tx_fee_ustx: Option, /// The path to the signer's database file pub db_path: PathBuf, } @@ -176,8 +177,10 @@ pub struct GlobalConfig { pub nonce_timeout: Option, /// timeout to gather signature shares pub sign_timeout: Option, - /// the STX tx fee to use in uSTX + /// the STX tx fee to use in uSTX. pub tx_fee_ustx: u64, + /// the max STX tx fee to use in uSTX when estimating fees + pub max_tx_fee_ustx: Option, /// the authorization password for the block proposal endpoint pub auth_password: String, /// The path to the signer's database file @@ -208,8 +211,11 @@ struct RawConfigFile { pub nonce_timeout_ms: Option, /// timeout in (millisecs) to gather signature shares pub sign_timeout_ms: Option, - /// the STX tx fee to use in uSTX + /// the STX tx fee to use in uSTX. If not set, will default to TX_FEE_USTX pub tx_fee_ustx: Option, + /// the max STX tx fee to use in uSTX when estimating fees. + /// If not set, will use tx_fee_ustx. + pub max_tx_fee_ustx: Option, /// The authorization password for the block proposal endpoint pub auth_password: String, /// The path to the signer's database file or :memory: for an in-memory database @@ -305,6 +311,7 @@ impl TryFrom for GlobalConfig { nonce_timeout, sign_timeout, tx_fee_ustx: raw_data.tx_fee_ustx.unwrap_or(TX_FEE_USTX), + max_tx_fee_ustx: raw_data.max_tx_fee_ustx, auth_password: raw_data.auth_password, db_path, }) @@ -340,6 +347,7 @@ pub fn build_signer_config_tomls( password: &str, run_stamp: u16, mut port_start: usize, + max_tx_fee_ustx: Option, ) -> Vec { let mut signer_config_tomls = vec![]; @@ -371,7 +379,16 @@ db_path = "{db_path}" signer_config_toml = format!( r#" {signer_config_toml} -event_timeout = {event_timeout_ms} +event_timeout = {event_timeout_ms} +"# + ) + } + + if let Some(max_tx_fee_ustx) = max_tx_fee_ustx { + signer_config_toml = format!( + r#" +{signer_config_toml} +max_tx_fee_ustx = {max_tx_fee_ustx} "# ) } @@ -405,6 +422,7 @@ mod tests { password, rand::random(), 3000, + None, ); let config = 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 0aedb25b86..05ac4d17e6 100644 --- a/stacks-signer/src/main.rs +++ b/stacks-signer/src/main.rs @@ -84,6 +84,7 @@ fn write_chunk_to_stdout(chunk_opt: Option>) { // Spawn a running signer and return its handle, command sender, and result receiver fn spawn_running_signer(path: &PathBuf) -> SpawnedSigner { let config = GlobalConfig::try_from(path).unwrap(); + let config_str = format!("{:?}", config); let endpoint = config.endpoint; let (cmd_send, cmd_recv) = channel(); let (res_send, res_recv) = channel(); @@ -92,6 +93,7 @@ fn spawn_running_signer(path: &PathBuf) -> SpawnedSigner { let mut signer: Signer, RunLoop, SignerEventReceiver> = Signer::new(runloop, ev, cmd_recv, res_send); let running_signer = signer.spawn(endpoint).unwrap(); + println!("Signer successfully configured: {config_str}"); SpawnedSigner { running_signer, cmd_send, @@ -294,6 +296,7 @@ fn handle_generate_files(args: GenerateFilesArgs) { &args.password, rand::random(), 3000, + None, ); debug!("Built {:?} signer config tomls.", signer_config_tomls.len()); for (i, file_contents) in signer_config_tomls.iter().enumerate() { @@ -400,6 +403,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 32d3234464..38b676106f 100644 --- a/stacks-signer/src/runloop.rs +++ b/stacks-signer/src/runloop.rs @@ -210,6 +210,7 @@ impl RunLoop { nonce_timeout: self.config.nonce_timeout, sign_timeout: self.config.sign_timeout, tx_fee_ustx: self.config.tx_fee_ustx, + max_tx_fee_ustx: self.config.max_tx_fee_ustx, db_path: self.config.db_path.clone(), }) } diff --git a/stacks-signer/src/signer.rs b/stacks-signer/src/signer.rs index 7059c5e1fb..461657fc8d 100644 --- a/stacks-signer/src/signer.rs +++ b/stacks-signer/src/signer.rs @@ -159,8 +159,11 @@ pub struct Signer { pub next_signer_addresses: Vec, /// The reward cycle this signer belongs to pub reward_cycle: u64, - /// The tx fee in uSTX to use if the epoch is pre Nakamoto (Epoch 3.0) + /// The default tx fee in uSTX to use when the epoch is pre Nakamoto (Epoch 3.0). pub tx_fee_ustx: u64, + /// If estimating the tx fee, the max tx fee in uSTX to use when the epoch is pre Nakamoto (Epoch 3.0) + /// If None, will not cap the fee. + pub max_tx_fee_ustx: Option, /// The coordinator info for the signer pub coordinator_selector: CoordinatorSelector, /// The approved key registered to the contract @@ -199,10 +202,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) } } } @@ -295,6 +298,7 @@ impl From for Signer { next_signer_addresses: vec![], reward_cycle: signer_config.reward_cycle, tx_fee_ustx: signer_config.tx_fee_ustx, + max_tx_fee_ustx: signer_config.max_tx_fee_ustx, coordinator_selector, approved_aggregate_public_key: None, miner_key: None, @@ -527,31 +531,29 @@ 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 { - 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}: ignoring block."; + "block_hash" => block_info.block.header.block_hash(), + "valid" => block_info.valid, + "signed_over" => block_info.signed_over, + "coordinator_id" => coordinator_id, + ); } self.signer_db .insert_block(self.reward_cycle, &block_info) @@ -936,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(|e| panic!("{self}: Failed to insert block in DB: {e:?}")); let process_request = updated_block_info.vote.is_some(); if !process_request { debug!("Failed to validate nonce request"); @@ -999,15 +1001,14 @@ 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); - } } + // Get our current nonce from the stacks node and compare it against what we have sitting in the stackerdb instance let signer_address = stacks_client.get_signer_address(); // Retreieve ALL account nonces as we may have transactions from other signers in our stackerdb slot that we care about @@ -1027,28 +1028,8 @@ impl Signer { let epoch = stacks_client .get_node_epoch() .unwrap_or(StacksEpochId::Epoch24); - let tx_fee = if epoch < StacksEpochId::Epoch30 { - debug!("{self}: in pre Epoch 3.0 cycles, must set a transaction fee for the DKG vote."); - self.tx_fee_ustx - } else { - 0 - }; - match stacks_client.build_unsigned_vote_for_aggregate_public_key( - self.stackerdb.get_signer_slot_id().0, - self.coordinator.current_dkg_id, - *dkg_public_key, - self.reward_cycle, - next_nonce, - ) { - Ok(mut unsigned_tx) => { - unsigned_tx.set_tx_fee(tx_fee); - let new_transaction = match stacks_client.sign_transaction(unsigned_tx) { - Ok(tx) => tx, - Err(e) => { - warn!("{self}: Failed to sign DKG public key vote transaction: {e:?}."); - return; - } - }; + match self.build_dkg_vote(stacks_client, &epoch, next_nonce, *dkg_public_key) { + Ok(new_transaction) => { if let Err(e) = self.broadcast_dkg_vote( stacks_client, epoch, @@ -1068,6 +1049,44 @@ impl Signer { } } + /// Build a signed DKG vote transaction + fn build_dkg_vote( + &mut self, + stacks_client: &StacksClient, + epoch: &StacksEpochId, + nonce: u64, + dkg_public_key: Point, + ) -> Result { + let mut unsigned_tx = stacks_client.build_unsigned_vote_for_aggregate_public_key( + self.stackerdb.get_signer_slot_id().0, + self.coordinator.current_dkg_id, + dkg_public_key, + self.reward_cycle, + nonce, + )?; + let tx_fee = if epoch < &StacksEpochId::Epoch30 { + debug!("{self}: in pre Epoch 3.0 cycles, must set a transaction fee for the DKG vote."); + let fee = if let Some(max_fee) = self.max_tx_fee_ustx { + let estimated_fee = stacks_client + .get_medium_estimated_fee_ustx(&unsigned_tx) + .map_err(|e| { + debug!("{self}: unable to estimate fee for DKG vote transaction: {e:?}"); + e + }) + .unwrap_or(self.tx_fee_ustx); + std::cmp::min(estimated_fee, max_fee) + } else { + self.tx_fee_ustx + }; + debug!("{self}: Using a fee of {fee} uSTX for DKG vote transaction."); + fee + } else { + 0 + }; + unsigned_tx.set_tx_fee(tx_fee); + stacks_client.sign_transaction(unsigned_tx) + } + // Get the account nonces for the provided list of signer addresses fn get_account_nonces( &self, 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") } diff --git a/testnet/stacks-node/src/tests/signer.rs b/testnet/stacks-node/src/tests/signer.rs index 7b2e938a1a..b2544893c6 100644 --- a/testnet/stacks-node/src/tests/signer.rs +++ b/testnet/stacks-node/src/tests/signer.rs @@ -120,6 +120,7 @@ impl SignerTest { password, run_stamp, 3000, + Some(100_000), ); let mut running_signers = Vec::new(); From a112bf510e5ea826e1989f23b68921d2359cbcd1 Mon Sep 17 00:00:00 2001 From: Jacinta Ferrant Date: Wed, 27 Mar 2024 10:24:30 -0400 Subject: [PATCH 09/23] CRC: remove dead path and update debug message to a warn Signed-off-by: Jacinta Ferrant --- stacks-signer/src/signer.rs | 26 +------------------------- 1 file changed, 1 insertion(+), 25 deletions(-) diff --git a/stacks-signer/src/signer.rs b/stacks-signer/src/signer.rs index 461657fc8d..08bb469973 100644 --- a/stacks-signer/src/signer.rs +++ b/stacks-signer/src/signer.rs @@ -459,7 +459,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; @@ -531,29 +530,6 @@ 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, - ); } self.signer_db .insert_block(self.reward_cycle, &block_info) @@ -1070,7 +1046,7 @@ impl Signer { let estimated_fee = stacks_client .get_medium_estimated_fee_ustx(&unsigned_tx) .map_err(|e| { - debug!("{self}: unable to estimate fee for DKG vote transaction: {e:?}"); + warn!("{self}: unable to estimate fee for DKG vote transaction: {e:?}."); e }) .unwrap_or(self.tx_fee_ustx); From 110d04e6120d50a72a979b38693dd05e5babf931 Mon Sep 17 00:00:00 2001 From: Jacinta Ferrant Date: Thu, 28 Mar 2024 08:49:41 -0400 Subject: [PATCH 10/23] CRC: add config tests and remove config log Signed-off-by: Jacinta Ferrant --- stacks-signer/src/cli.rs | 5 +- stacks-signer/src/config.rs | 118 ++++++++++++++++++++++++ stacks-signer/src/main.rs | 3 +- stacks-signer/src/signerdb.rs | 4 +- testnet/stacks-node/src/tests/signer.rs | 1 + 5 files changed, 124 insertions(+), 7 deletions(-) diff --git a/stacks-signer/src/cli.rs b/stacks-signer/src/cli.rs index 24f27cb58c..ae6a459fe0 100644 --- a/stacks-signer/src/cli.rs +++ b/stacks-signer/src/cli.rs @@ -345,7 +345,7 @@ mod tests { } fn clarity_tuple_version(pox_addr: &PoxAddress) -> u8 { - pox_addr + *pox_addr .as_clarity_tuple() .expect("Failed to generate clarity tuple for pox address") .get("version") @@ -353,9 +353,8 @@ mod tests { .clone() .expect_buff(1) .expect("Expected version to be a u128") - .get(0) + .first() .expect("Expected version to be a uint") - .clone() } #[test] diff --git a/stacks-signer/src/config.rs b/stacks-signer/src/config.rs index 30b51ae025..a7aed7f6cd 100644 --- a/stacks-signer/src/config.rs +++ b/stacks-signer/src/config.rs @@ -339,6 +339,7 @@ impl GlobalConfig { } /// Helper function for building a signer config for each provided signer private key +#[allow(clippy::too_many_arguments)] pub fn build_signer_config_tomls( stacks_private_keys: &[StacksPrivateKey], node_host: &str, @@ -348,6 +349,7 @@ pub fn build_signer_config_tomls( run_stamp: u16, mut port_start: usize, max_tx_fee_ustx: Option, + tx_fee_ustx: Option, ) -> Vec { let mut signer_config_tomls = vec![]; @@ -393,6 +395,15 @@ max_tx_fee_ustx = {max_tx_fee_ustx} ) } + if let Some(tx_fee_ustx) = tx_fee_ustx { + signer_config_toml = format!( + r#" +{signer_config_toml} +tx_fee_ustx = {tx_fee_ustx} +"# + ) + } + signer_config_tomls.push(signer_config_toml); } @@ -423,11 +434,118 @@ mod tests { rand::random(), 3000, None, + None, ); let config = RawConfigFile::load_from_str(&config_tomls[0]).expect("Failed to parse config file"); assert_eq!(config.auth_password, "melon"); + assert!(config.max_tx_fee_ustx.is_none()); + assert!(config.tx_fee_ustx.is_none()); + } + + #[test] + fn fee_options_should_deserialize_correctly() { + let pk = StacksPrivateKey::from_hex( + "eb05c83546fdd2c79f10f5ad5434a90dd28f7e3acb7c092157aa1bc3656b012c01", + ) + .unwrap(); + + let node_host = "localhost"; + let network = Network::Testnet; + let password = "melon"; + + // Test both max_tx_fee_ustx and tx_fee_ustx are unspecified + let config_tomls = build_signer_config_tomls( + &[pk], + node_host, + None, + &network, + password, + rand::random(), + 3000, + None, + None, + ); + + let config = + RawConfigFile::load_from_str(&config_tomls[0]).expect("Failed to parse config file"); + + assert!(config.max_tx_fee_ustx.is_none()); + assert!(config.tx_fee_ustx.is_none()); + + let config = GlobalConfig::try_from(config).expect("Failed to parse config"); + assert!(config.max_tx_fee_ustx.is_none()); + assert_eq!(config.tx_fee_ustx, TX_FEE_USTX); + + // Test both max_tx_fee_ustx and tx_fee_ustx are specified + let max_tx_fee_ustx = Some(1000); + let tx_fee_ustx = Some(2000); + let config_tomls = build_signer_config_tomls( + &[pk], + node_host, + None, + &network, + password, + rand::random(), + 3000, + max_tx_fee_ustx, + tx_fee_ustx, + ); + + let config = + RawConfigFile::load_from_str(&config_tomls[0]).expect("Failed to parse config file"); + + assert_eq!(config.max_tx_fee_ustx, max_tx_fee_ustx); + assert_eq!(config.tx_fee_ustx, tx_fee_ustx); + + // Test only max_tx_fee_ustx is specified + let max_tx_fee_ustx = Some(1000); + let config_tomls = build_signer_config_tomls( + &[pk], + node_host, + None, + &network, + password, + rand::random(), + 3000, + max_tx_fee_ustx, + None, + ); + + let config = + RawConfigFile::load_from_str(&config_tomls[0]).expect("Failed to parse config file"); + + assert_eq!(config.max_tx_fee_ustx, max_tx_fee_ustx); + assert!(config.tx_fee_ustx.is_none()); + + let config = GlobalConfig::try_from(config).expect("Failed to parse config"); + assert_eq!(config.max_tx_fee_ustx, max_tx_fee_ustx); + assert_eq!(config.tx_fee_ustx, TX_FEE_USTX); + + // Test only tx_fee_ustx is specified + let tx_fee_ustx = Some(1000); + let config_tomls = build_signer_config_tomls( + &[pk], + node_host, + None, + &network, + password, + rand::random(), + 3000, + None, + tx_fee_ustx, + ); + + let config = + RawConfigFile::load_from_str(&config_tomls[0]).expect("Failed to parse config file"); + + assert!(config.max_tx_fee_ustx.is_none()); + assert_eq!(config.tx_fee_ustx, tx_fee_ustx); + + let config = GlobalConfig::try_from(config).expect("Failed to parse config"); + assert!(config.max_tx_fee_ustx.is_none()); + assert_eq!(Some(config.tx_fee_ustx), tx_fee_ustx); } } diff --git a/stacks-signer/src/main.rs b/stacks-signer/src/main.rs index 05ac4d17e6..c46d28c059 100644 --- a/stacks-signer/src/main.rs +++ b/stacks-signer/src/main.rs @@ -84,7 +84,6 @@ fn write_chunk_to_stdout(chunk_opt: Option>) { // Spawn a running signer and return its handle, command sender, and result receiver fn spawn_running_signer(path: &PathBuf) -> SpawnedSigner { let config = GlobalConfig::try_from(path).unwrap(); - let config_str = format!("{:?}", config); let endpoint = config.endpoint; let (cmd_send, cmd_recv) = channel(); let (res_send, res_recv) = channel(); @@ -93,7 +92,6 @@ fn spawn_running_signer(path: &PathBuf) -> SpawnedSigner { let mut signer: Signer, RunLoop, SignerEventReceiver> = Signer::new(runloop, ev, cmd_recv, res_send); let running_signer = signer.spawn(endpoint).unwrap(); - println!("Signer successfully configured: {config_str}"); SpawnedSigner { running_signer, cmd_send, @@ -297,6 +295,7 @@ fn handle_generate_files(args: GenerateFilesArgs) { rand::random(), 3000, None, + None, ); debug!("Built {:?} signer config tomls.", signer_config_tomls.len()); for (i, file_contents) in signer_config_tomls.iter().enumerate() { diff --git a/stacks-signer/src/signerdb.rs b/stacks-signer/src/signerdb.rs index 1b2fd6ca69..1f568ff37f 100644 --- a/stacks-signer/src/signerdb.rs +++ b/stacks-signer/src/signerdb.rs @@ -43,7 +43,7 @@ CREATE TABLE IF NOT EXISTS blocks ( PRIMARY KEY (reward_cycle, signer_signature_hash) )"; -const CREATE_SIGNER_STATE_TABLE: &'static str = " +const CREATE_SIGNER_STATE_TABLE: &str = " CREATE TABLE IF NOT EXISTS signer_states ( reward_cycle INTEGER PRIMARY KEY, state TEXT NOT NULL @@ -88,7 +88,7 @@ impl SignerDb { let result: Option = query_row( &self.db, "SELECT state FROM signer_states WHERE reward_cycle = ?", - &[u64_to_sql(reward_cycle)?], + [u64_to_sql(reward_cycle)?], )?; try_deserialize(result) diff --git a/testnet/stacks-node/src/tests/signer.rs b/testnet/stacks-node/src/tests/signer.rs index b2544893c6..347c8cf223 100644 --- a/testnet/stacks-node/src/tests/signer.rs +++ b/testnet/stacks-node/src/tests/signer.rs @@ -121,6 +121,7 @@ impl SignerTest { run_stamp, 3000, Some(100_000), + None, ); let mut running_signers = Vec::new(); From ccfba633815dfbc2f59c96b2c5727236b5a99853 Mon Sep 17 00:00:00 2001 From: Jacinta Ferrant Date: Tue, 2 Apr 2024 09:29:06 -0400 Subject: [PATCH 11/23] 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 3c4e1246d7cf591ca0bc35bf763dd33b801d6fcf Mon Sep 17 00:00:00 2001 From: Jacinta Ferrant Date: Tue, 2 Apr 2024 12:50:27 -0400 Subject: [PATCH 12/23] Fix bad merge Signed-off-by: Jacinta Ferrant --- testnet/stacks-node/src/tests/signer.rs | 2 ++ 1 file changed, 2 insertions(+) diff --git a/testnet/stacks-node/src/tests/signer.rs b/testnet/stacks-node/src/tests/signer.rs index 347c8cf223..867421b5c0 100644 --- a/testnet/stacks-node/src/tests/signer.rs +++ b/testnet/stacks-node/src/tests/signer.rs @@ -726,6 +726,8 @@ impl SignerTest { "12345", // It worked sir, we have the combination! -Great, what's the combination? self.run_stamp, 3000 + signer_idx, + Some(100_000), + None, ) .pop() .unwrap(); From 53f2635cae085a44288ce13bf9f38fc73ca85f09 Mon Sep 17 00:00:00 2001 From: Jacinta Ferrant Date: Tue, 2 Apr 2024 15:35:45 -0400 Subject: [PATCH 13/23] Consider nonce when checking for existing vote transactions in stackerdb during update_dkg Signed-off-by: Jacinta Ferrant --- stacks-signer/src/signer.rs | 133 ++++++++++-------- .../src/chainstate/nakamoto/signer_set.rs | 2 +- 2 files changed, 79 insertions(+), 56 deletions(-) diff --git a/stacks-signer/src/signer.rs b/stacks-signer/src/signer.rs index 4d23a92c07..391e65ea90 100644 --- a/stacks-signer/src/signer.rs +++ b/stacks-signer/src/signer.rs @@ -21,7 +21,6 @@ use std::time::Instant; use blockstack_lib::chainstate::burn::ConsensusHashExtensions; use blockstack_lib::chainstate::nakamoto::signer_set::NakamotoSigners; use blockstack_lib::chainstate::nakamoto::{NakamotoBlock, NakamotoBlockVote}; -use blockstack_lib::chainstate::stacks::boot::SIGNERS_VOTING_FUNCTION_NAME; use blockstack_lib::chainstate::stacks::StacksTransaction; use blockstack_lib::net::api::postblock_proposal::BlockValidateResponse; use hashbrown::HashSet; @@ -1235,64 +1234,24 @@ impl Signer { } } - /// Update the DKG for the provided signer info, triggering it if required - pub fn update_dkg( + /// 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<(), ClientError> { - let reward_cycle = self.reward_cycle; - let old_dkg = self.approved_aggregate_public_key; - self.approved_aggregate_public_key = - stacks_client.get_approved_aggregate_key(reward_cycle)?; - if self.approved_aggregate_public_key.is_some() { - // TODO: this will never work as is. We need to have stored our party shares on the side etc for this particular aggregate key. - // Need to update state to store the necessary info, check against it to see if we have participated in the winning round and - // then overwrite our value accordingly. Otherwise, we will be locked out of the round and should not participate. - self.coordinator - .set_aggregate_public_key(self.approved_aggregate_public_key); - if old_dkg != self.approved_aggregate_public_key { - debug!( - "{self}: updated DKG value to {:?}.", - self.approved_aggregate_public_key - ); - } - return Ok(()); - }; + ) -> Result { if self.state != State::Idle || Some(self.signer_id) != self.get_coordinator(current_reward_cycle).0 + || self.commands.front() == Some(&Command::Dkg) { - // We are not the coordinator or we are in the middle of an operation. Do not attempt to queue DKG - return Ok(()); + // 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 + return Ok(false); } - 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(); - let account_nonces = self.get_account_nonces(stacks_client, &[*signer_address]); - let old_transactions = self.get_signer_transactions(&account_nonces).map_err(|e| { - warn!("{self}: Failed to get old signer transactions: {e:?}. May trigger DKG unnecessarily"); - }).unwrap_or_default(); - // Check if we have an existing vote transaction for the same round and reward cycle - for transaction in old_transactions.iter() { - let params = - NakamotoSigners::parse_vote_for_aggregate_public_key(transaction).unwrap_or_else(|| panic!("BUG: {self}: Received an invalid {SIGNERS_VOTING_FUNCTION_NAME} transaction in an already filtered list: {transaction:?}")); - if Some(params.aggregate_key) == self.coordinator.aggregate_public_key - && params.voting_round == self.coordinator.current_dkg_id - && reward_cycle == self.reward_cycle - { - debug!("{self}: Not triggering a DKG round. Already have a pending vote transaction."; - "txid" => %transaction.txid(), - "aggregate_key" => %params.aggregate_key, - "voting_round" => params.voting_round - ); - return Ok(()); - } - } if let Some(aggregate_key) = stacks_client.get_vote_for_aggregate_public_key( self.coordinator.current_dkg_id, self.reward_cycle, - *stacks_client.get_signer_address(), + *signer_address, )? { let Some(round_weight) = stacks_client .get_round_vote_weight(self.reward_cycle, self.coordinator.current_dkg_id)? @@ -1302,7 +1261,7 @@ impl Signer { "voting_round" => self.coordinator.current_dkg_id, "aggregate_key" => %aggregate_key ); - return Ok(()); + return Ok(false); }; let threshold_weight = stacks_client.get_vote_threshold_weight(self.reward_cycle)?; if round_weight < threshold_weight { @@ -1315,22 +1274,86 @@ impl Signer { "round_weight" => round_weight, "threshold_weight" => threshold_weight ); - return Ok(()); + return Ok(false); } - debug!("{self}: Vote for DKG failed. Triggering a DKG round."; + warn!("{self}: Vote for DKG failed."; "voting_round" => self.coordinator.current_dkg_id, "aggregate_key" => %aggregate_key, "round_weight" => round_weight, "threshold_weight" => threshold_weight ); } else { - debug!("{self}: Triggering a DKG round."); + // 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 account_nonce = stacks_client.get_account_nonce(signer_address).unwrap_or(0); + let old_transactions = self.stackerdb.get_current_transactions_with_retry()?; + // Check if we have an existing vote transaction for the same round and reward cycle + for transaction in old_transactions.iter() { + // We should not consider other signer transactions and should ignore invalid transaction versions + if transaction.origin_address() != *signer_address + || transaction.is_mainnet() != self.mainnet + { + continue; + } + let Some(params) = + NakamotoSigners::parse_vote_for_aggregate_public_key(transaction) + else { + continue; + }; + let Some(dkg_public_key) = self.coordinator.aggregate_public_key.clone() else { + break; + }; + if params.aggregate_key == dkg_public_key + && params.voting_round == self.coordinator.current_dkg_id + && params.reward_cycle == self.reward_cycle + { + let origin_nonce = transaction.get_origin_nonce(); + if origin_nonce < account_nonce { + // We have already voted, but our vote nonce is outdated. Resubmit vote with updated transaction + warn!("{self}: DKG vote submitted with invalid nonce ({origin_nonce} < {account_nonce}). Resubmitting vote."); + self.process_dkg(stacks_client, &dkg_public_key); + } else { + debug!("{self}: Already have a pending DKG vote in StackerDB. Waiting for it to be confirmed."; + "txid" => %transaction.txid(), + "aggregate_key" => %params.aggregate_key, + "voting_round" => params.voting_round, + "reward_cycle" => params.reward_cycle, + "nonce" => origin_nonce + ); + } + return Ok(false); + } + } } - if self.commands.front() != Some(&Command::Dkg) { + Ok(true) + } + + /// 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> { + let old_dkg = self.approved_aggregate_public_key; + self.approved_aggregate_public_key = + stacks_client.get_approved_aggregate_key(self.reward_cycle)?; + if self.approved_aggregate_public_key.is_some() { + // TODO: this will never work as is. We need to have stored our party shares on the side etc for this particular aggregate key. + // Need to update state to store the necessary info, check against it to see if we have participated in the winning round and + // then overwrite our value accordingly. Otherwise, we will be locked out of the round and should not participate. + self.coordinator + .set_aggregate_public_key(self.approved_aggregate_public_key); + if old_dkg != self.approved_aggregate_public_key { + warn!( + "{self}: updated DKG value to {:?}.", + self.approved_aggregate_public_key + ); + } + return Ok(()); + }; + if self.should_queue_dkg(stacks_client, current_reward_cycle)? { info!("{self} is the current coordinator and must trigger DKG. Queuing DKG command..."); self.commands.push_front(Command::Dkg); - } else { - debug!("{self}: DKG command already queued..."); } Ok(()) } diff --git a/stackslib/src/chainstate/nakamoto/signer_set.rs b/stackslib/src/chainstate/nakamoto/signer_set.rs index 5049286908..e776ca41db 100644 --- a/stackslib/src/chainstate/nakamoto/signer_set.rs +++ b/stackslib/src/chainstate/nakamoto/signer_set.rs @@ -514,7 +514,7 @@ impl NakamotoSigners { return false; } if origin_nonce < *account_nonce { - debug!("valid_vote_transaction: Received a transaction with an outdated nonce ({account_nonce} < {origin_nonce})."); + debug!("valid_vote_transaction: Received a transaction with an outdated nonce ({origin_nonce} < {account_nonce})."); return false; } Self::parse_vote_for_aggregate_public_key(transaction).is_some() From 0713b65350c4f61f38ea619217ba7846d4a6b4cc Mon Sep 17 00:00:00 2001 From: Hank Stoever Date: Tue, 2 Apr 2024 14:39:24 -0700 Subject: [PATCH 14/23] fix: dont warn about aggregate key in 2.5 --- stackslib/src/chainstate/nakamoto/mod.rs | 7 ++++++- 1 file changed, 6 insertions(+), 1 deletion(-) diff --git a/stackslib/src/chainstate/nakamoto/mod.rs b/stackslib/src/chainstate/nakamoto/mod.rs index 724f3681cd..2b3635660e 100644 --- a/stackslib/src/chainstate/nakamoto/mod.rs +++ b/stackslib/src/chainstate/nakamoto/mod.rs @@ -1826,6 +1826,11 @@ impl NakamotoChainState { .ok_or(ChainstateError::DBError(DBError::NotFoundError))?; let aggregate_key_block_header = Self::get_canonical_block_header(chainstate.db(), sortdb)?.unwrap(); + let epoch_id = SortitionDB::get_stacks_epoch(sortdb.conn(), block_sn.block_height)? + .ok_or(ChainstateError::InvalidStacksBlock( + "Failed to get epoch ID".into(), + ))? + .epoch_id; let aggregate_public_key = Self::load_aggregate_public_key( sortdb, @@ -1833,7 +1838,7 @@ impl NakamotoChainState { chainstate, block_sn.block_height, &aggregate_key_block_header.index_block_hash(), - true, + epoch_id >= StacksEpochId::Epoch30, )?; Ok(aggregate_public_key) } From f9efc529b78025c98809f6f6745d45175b066598 Mon Sep 17 00:00:00 2001 From: Jacinta Ferrant Date: Wed, 3 Apr 2024 09:24:17 -0400 Subject: [PATCH 15/23] Reward cycle length was being incorrectly stored as reward phase length Signed-off-by: Jacinta Ferrant --- stacks-signer/src/client/stacks_client.rs | 6 +++--- stacks-signer/src/runloop.rs | 11 ++++------- 2 files changed, 7 insertions(+), 10 deletions(-) diff --git a/stacks-signer/src/client/stacks_client.rs b/stacks-signer/src/client/stacks_client.rs index b89c5462dd..2c51c79cc9 100644 --- a/stacks-signer/src/client/stacks_client.rs +++ b/stacks-signer/src/client/stacks_client.rs @@ -413,13 +413,13 @@ impl StacksClient { let blocks_mined = pox_data .current_burnchain_block_height .saturating_sub(pox_data.first_burnchain_block_height); - let reward_phase_block_length = pox_data + let reward_cycle_length = pox_data .reward_phase_block_length .saturating_add(pox_data.prepare_phase_block_length); - let reward_cycle = blocks_mined / reward_phase_block_length; + let reward_cycle = blocks_mined / reward_cycle_length; Ok(RewardCycleInfo { reward_cycle, - reward_phase_block_length, + reward_cycle_length, prepare_phase_block_length: pox_data.prepare_phase_block_length, first_burnchain_block_height: pox_data.first_burnchain_block_height, last_burnchain_block_height: pox_data.current_burnchain_block_height, diff --git a/stacks-signer/src/runloop.rs b/stacks-signer/src/runloop.rs index 4491650090..4e1f44c2a8 100644 --- a/stacks-signer/src/runloop.rs +++ b/stacks-signer/src/runloop.rs @@ -56,8 +56,8 @@ pub enum State { pub struct RewardCycleInfo { /// The current reward cycle pub reward_cycle: u64, - /// The reward phase cycle length - pub reward_phase_block_length: u64, + /// The total reward cycle length + pub reward_cycle_length: u64, /// The prepare phase length pub prepare_phase_block_length: u64, /// The first burn block height @@ -70,10 +70,7 @@ impl RewardCycleInfo { /// Check if the provided burnchain block height is part of the reward cycle pub const fn is_in_reward_cycle(&self, burnchain_block_height: u64) -> bool { let blocks_mined = burnchain_block_height.saturating_sub(self.first_burnchain_block_height); - let reward_cycle_length = self - .reward_phase_block_length - .saturating_add(self.prepare_phase_block_length); - let reward_cycle = blocks_mined / reward_cycle_length; + let reward_cycle = blocks_mined / self.reward_cycle_length; self.reward_cycle == reward_cycle } @@ -81,7 +78,7 @@ impl RewardCycleInfo { pub fn is_in_prepare_phase(&self, burnchain_block_height: u64) -> bool { PoxConstants::static_is_in_prepare_phase( self.first_burnchain_block_height, - self.reward_phase_block_length, + self.reward_cycle_length, self.prepare_phase_block_length, burnchain_block_height, ) From 803ab477ac639350dfe9d016b48c3eb1ed149892 Mon Sep 17 00:00:00 2001 From: Jacinta Ferrant Date: Wed, 3 Apr 2024 12:38:15 -0400 Subject: [PATCH 16/23] 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(); From 3e8e519be2c8231a43867d176334848452925d3f Mon Sep 17 00:00:00 2001 From: Jacinta Ferrant Date: Wed, 3 Apr 2024 14:20:32 -0400 Subject: [PATCH 17/23] Add unit tests for is_in_prepare_phase and is_in_reward_cycle functions Signed-off-by: Jacinta Ferrant --- stacks-signer/src/runloop.rs | 78 ++++++++++++++++++++++++++++++++++++ 1 file changed, 78 insertions(+) diff --git a/stacks-signer/src/runloop.rs b/stacks-signer/src/runloop.rs index 4e1f44c2a8..5c01fd341c 100644 --- a/stacks-signer/src/runloop.rs +++ b/stacks-signer/src/runloop.rs @@ -432,8 +432,11 @@ impl SignerRunLoop, RunLoopCommand> for RunLoop { mod tests { use blockstack_lib::chainstate::stacks::boot::NakamotoSignerEntry; use libsigner::SignerEntries; + use rand::{thread_rng, Rng, RngCore}; use stacks_common::types::chainstate::{StacksPrivateKey, StacksPublicKey}; + use super::RewardCycleInfo; + #[test] fn parse_nakamoto_signer_entries_test() { let nmb_signers = 10; @@ -459,4 +462,79 @@ mod tests { (0..nmb_signers).map(|id| id as u32).collect::>() ); } + + #[test] + fn is_in_reward_cycle_info() { + let rand_byte: u8 = std::cmp::max(1, thread_rng().gen()); + let prepare_phase_block_length = rand_byte as u64; + // Ensure the reward cycle is not close to u64 Max to prevent overflow when adding prepare phase len + let reward_cycle_length = (std::cmp::max( + prepare_phase_block_length.wrapping_add(1), + thread_rng().next_u32() as u64, + )) + .wrapping_add(prepare_phase_block_length); + let reward_cycle_phase_block_length = + reward_cycle_length.wrapping_sub(prepare_phase_block_length); + let first_burnchain_block_height = std::cmp::max(1u8, thread_rng().gen()) as u64; + let last_burnchain_block_height = thread_rng().gen_range( + first_burnchain_block_height + ..first_burnchain_block_height + .wrapping_add(reward_cycle_length) + .wrapping_sub(prepare_phase_block_length), + ); + let blocks_mined = last_burnchain_block_height.wrapping_sub(first_burnchain_block_height); + let reward_cycle = blocks_mined / reward_cycle_length; + + let reward_cycle_info = RewardCycleInfo { + reward_cycle, + reward_cycle_length, + prepare_phase_block_length, + first_burnchain_block_height, + last_burnchain_block_height, + }; + assert!(reward_cycle_info.is_in_reward_cycle(first_burnchain_block_height)); + assert!(!reward_cycle_info.is_in_prepare_phase(first_burnchain_block_height)); + + assert!(reward_cycle_info.is_in_reward_cycle(last_burnchain_block_height)); + assert!(!reward_cycle_info.is_in_prepare_phase(last_burnchain_block_height)); + + assert!(!reward_cycle_info + .is_in_reward_cycle(first_burnchain_block_height.wrapping_add(reward_cycle_length))); + assert!(!reward_cycle_info + .is_in_prepare_phase(!first_burnchain_block_height.wrapping_add(reward_cycle_length))); + + assert!(reward_cycle_info.is_in_reward_cycle( + first_burnchain_block_height + .wrapping_add(reward_cycle_length) + .wrapping_sub(1) + )); + assert!(reward_cycle_info.is_in_prepare_phase( + first_burnchain_block_height + .wrapping_add(reward_cycle_length) + .wrapping_sub(1) + )); + + assert!(reward_cycle_info.is_in_reward_cycle( + first_burnchain_block_height.wrapping_add(reward_cycle_phase_block_length) + )); + assert!(!reward_cycle_info.is_in_prepare_phase( + first_burnchain_block_height.wrapping_add(reward_cycle_phase_block_length) + )); + + assert!(reward_cycle_info.is_in_reward_cycle(first_burnchain_block_height.wrapping_add(1))); + assert!( + !reward_cycle_info.is_in_prepare_phase(first_burnchain_block_height.wrapping_add(1)) + ); + + assert!(reward_cycle_info.is_in_reward_cycle( + first_burnchain_block_height + .wrapping_add(reward_cycle_phase_block_length) + .wrapping_add(1) + )); + assert!(reward_cycle_info.is_in_prepare_phase( + first_burnchain_block_height + .wrapping_add(reward_cycle_phase_block_length) + .wrapping_add(1) + )); + } } From 10281353081e9de95eac9bf7a044b992fea181c0 Mon Sep 17 00:00:00 2001 From: Jacinta Ferrant Date: Wed, 3 Apr 2024 14:27:16 -0400 Subject: [PATCH 18/23] CRC: change debug to info for pre epoch 3.0 cycles requiring a fee Signed-off-by: Jacinta Ferrant --- stacks-signer/src/signer.rs | 2 +- 1 file changed, 1 insertion(+), 1 deletion(-) diff --git a/stacks-signer/src/signer.rs b/stacks-signer/src/signer.rs index 2db9416341..a103e55f05 100644 --- a/stacks-signer/src/signer.rs +++ b/stacks-signer/src/signer.rs @@ -1040,7 +1040,7 @@ impl Signer { nonce, )?; let tx_fee = if epoch < &StacksEpochId::Epoch30 { - debug!("{self}: in pre Epoch 3.0 cycles, must set a transaction fee for the DKG vote."); + info!("{self}: in pre Epoch 3.0 cycles, must set a transaction fee for the DKG vote."); let fee = if let Some(max_fee) = self.max_tx_fee_ustx { let estimated_fee = stacks_client .get_medium_estimated_fee_ustx(&unsigned_tx) From 85eb3516090afbfb033a6f5f3e0d3ad5c01f4715 Mon Sep 17 00:00:00 2001 From: Jacinta Ferrant Date: Thu, 4 Apr 2024 07:02:08 -0400 Subject: [PATCH 19/23] Fix bad merge Signed-off-by: Jacinta Ferrant --- stacks-signer/src/signer.rs | 2 +- 1 file changed, 1 insertion(+), 1 deletion(-) diff --git a/stacks-signer/src/signer.rs b/stacks-signer/src/signer.rs index 8edf284f80..5678d28937 100644 --- a/stacks-signer/src/signer.rs +++ b/stacks-signer/src/signer.rs @@ -1317,7 +1317,7 @@ impl Signer { // 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 account_nonce = stacks_client.get_account_nonce(signer_address).unwrap_or(0); - let old_transactions = self.stackerdb.get_current_transactions_with_retry()?; + let old_transactions = self.stackerdb.get_current_transactions()?; // Check if we have an existing vote transaction for the same round and reward cycle for transaction in old_transactions.iter() { // We should not consider other signer transactions and should ignore invalid transaction versions From 76b610bf2076950966c631a4616958a806bd9188 Mon Sep 17 00:00:00 2001 From: Jacinta Ferrant Date: Thu, 4 Apr 2024 07:17:36 -0400 Subject: [PATCH 20/23] Fix bad merge 2 Signed-off-by: Jacinta Ferrant --- stacks-signer/src/runloop.rs | 6 +----- 1 file changed, 1 insertion(+), 5 deletions(-) diff --git a/stacks-signer/src/runloop.rs b/stacks-signer/src/runloop.rs index 0558681d01..17b74c2fc9 100644 --- a/stacks-signer/src/runloop.rs +++ b/stacks-signer/src/runloop.rs @@ -385,11 +385,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) - .map_err(backoff::Error::transient) - }) { + if let Err(e) = signer.update_dkg(&self.stacks_client) { error!("{signer}: failed to update DKG: {e}"); } } From 7f96d082a68c76404933fe374cd8068005e9763a Mon Sep 17 00:00:00 2001 From: Jacinta Ferrant Date: Thu, 4 Apr 2024 11:04:08 -0400 Subject: [PATCH 21/23] Bugfix: verify block transactions only when the upcoming signers approved key is not set Signed-off-by: Jacinta Ferrant --- stacks-signer/src/signer.rs | 12 ++++++++---- 1 file changed, 8 insertions(+), 4 deletions(-) diff --git a/stacks-signer/src/signer.rs b/stacks-signer/src/signer.rs index 6fa2b54098..045ca7aa10 100644 --- a/stacks-signer/src/signer.rs +++ b/stacks-signer/src/signer.rs @@ -810,10 +810,14 @@ impl Signer { stacks_client: &StacksClient, block: &NakamotoBlock, ) -> bool { - if self.approved_aggregate_public_key.is_some() { - // We do not enforce a block contain any transactions except the aggregate votes when it is NOT already set - // TODO: should be only allow special cased transactions during prepare phase before a key is set? - debug!("{self}: Already have an aggregate key. Skipping transaction verification..."); + let next_reward_cycle = self.reward_cycle.wrapping_add(1); + let approved_aggregate_public_key = stacks_client + .get_approved_aggregate_key(next_reward_cycle) + .unwrap_or(None); + if approved_aggregate_public_key.is_some() { + // We do not enforce a block contain any transactions except the aggregate votes when it is NOT already set for the upcoming signers' reward cycle + // Otherwise it is a waste of block space and time to enforce as the desired outcome has been reached. + debug!("{self}: Already have an aggregate key for the next signer set's reward cycle ({}). Skipping transaction verification...", next_reward_cycle); return true; } if let Ok(expected_transactions) = self.get_expected_transactions(stacks_client) { From b12aea34648c3c608945866ff86d6270cb48830e Mon Sep 17 00:00:00 2001 From: wileyj <2847772+wileyj@users.noreply.github.com> Date: Fri, 5 Apr 2024 11:01:18 -0700 Subject: [PATCH 22/23] removing portable flag --- .cargo/config | 7 ------- .github/actions/dockerfiles/Dockerfile.debian-source | 2 +- Dockerfile | 2 +- Dockerfile.debian | 2 +- stacks-signer/Cargo.toml | 3 --- testnet/stacks-node/Cargo.toml | 1 - 6 files changed, 3 insertions(+), 14 deletions(-) diff --git a/.cargo/config b/.cargo/config index 53130b2e6d..d59b03bf21 100644 --- a/.cargo/config +++ b/.cargo/config @@ -2,13 +2,6 @@ stacks-node = "run --package stacks-node --" fmt-stacks = "fmt -- --config group_imports=StdExternalCrate,imports_granularity=Module" -# Default to `native` -# This makes it slightly faster for running tests and locally built binaries. -# This can cause trouble when building "portable" binaries, such as for docker, -# so disable it with the "portable" feature. -[target.'cfg(not(feature = "portable"))'] -rustflags = ["-Ctarget-cpu=native"] - # Needed by perf to generate flamegraphs. #[target.x86_64-unknown-linux-gnu] #linker = "/usr/bin/clang" diff --git a/.github/actions/dockerfiles/Dockerfile.debian-source b/.github/actions/dockerfiles/Dockerfile.debian-source index c2b6df7e9b..b8da585fe2 100644 --- a/.github/actions/dockerfiles/Dockerfile.debian-source +++ b/.github/actions/dockerfiles/Dockerfile.debian-source @@ -19,7 +19,7 @@ RUN --mount=type=tmpfs,target=${BUILD_DIR} cp -R /src/. ${BUILD_DIR}/ \ && cd ${BUILD_DIR} \ && rustup target add ${TARGET} \ && rustup component add rustfmt \ - && cargo build --features monitoring_prom,slog_json,portable --release --workspace --target ${TARGET} \ + && cargo build --features monitoring_prom,slog_json --release --workspace --target ${TARGET} \ && mkdir -p /out \ && cp -R ${BUILD_DIR}/target/${TARGET}/release/. /out diff --git a/Dockerfile b/Dockerfile index a387f1b4a6..055cc3df76 100644 --- a/Dockerfile +++ b/Dockerfile @@ -12,7 +12,7 @@ RUN apk add --no-cache musl-dev RUN mkdir /out -RUN cd testnet/stacks-node && cargo build --features monitoring_prom,slog_json,portable --release +RUN cd testnet/stacks-node && cargo build --features monitoring_prom,slog_json --release RUN cp target/release/stacks-node /out diff --git a/Dockerfile.debian b/Dockerfile.debian index 2fe262807e..8b6759527e 100644 --- a/Dockerfile.debian +++ b/Dockerfile.debian @@ -10,7 +10,7 @@ COPY . . RUN mkdir /out -RUN cd testnet/stacks-node && cargo build --features monitoring_prom,slog_json,portable --release +RUN cd testnet/stacks-node && cargo build --features monitoring_prom,slog_json --release RUN cp target/release/stacks-node /out diff --git a/stacks-signer/Cargo.toml b/stacks-signer/Cargo.toml index 861e9204f2..57b2e80804 100644 --- a/stacks-signer/Cargo.toml +++ b/stacks-signer/Cargo.toml @@ -60,6 +60,3 @@ features = ["arbitrary_precision", "unbounded_depth"] [dependencies.secp256k1] version = "0.24.3" features = ["serde", "recovery"] - -[features] -portable = [] diff --git a/testnet/stacks-node/Cargo.toml b/testnet/stacks-node/Cargo.toml index f7160a1591..72cc8d2491 100644 --- a/testnet/stacks-node/Cargo.toml +++ b/testnet/stacks-node/Cargo.toml @@ -65,5 +65,4 @@ path = "src/stacks_events.rs" monitoring_prom = ["stacks/monitoring_prom"] slog_json = ["stacks/slog_json", "stacks-common/slog_json", "clarity/slog_json"] prod-genesis-chainstate = [] -portable = [] default = [] From e28fd6506aec2d1bf5a6cd00da43d7808e802135 Mon Sep 17 00:00:00 2001 From: wileyj <2847772+wileyj@users.noreply.github.com> Date: Fri, 5 Apr 2024 11:53:22 -0700 Subject: [PATCH 23/23] update per 4638 --- .cargo/config | 5 +++++ README.md | 13 +++++++++++++ 2 files changed, 18 insertions(+) diff --git a/.cargo/config b/.cargo/config index d59b03bf21..7f7e28a8b8 100644 --- a/.cargo/config +++ b/.cargo/config @@ -2,6 +2,11 @@ stacks-node = "run --package stacks-node --" fmt-stacks = "fmt -- --config group_imports=StdExternalCrate,imports_granularity=Module" +# Uncomment to improve performance slightly, at the cost of portability +# * Note that native binaries may not run on CPUs that are different from the build machine +# [build] +# rustflags = ["-Ctarget-cpu=native"] + # Needed by perf to generate flamegraphs. #[target.x86_64-unknown-linux-gnu] #linker = "/usr/bin/clang" diff --git a/README.md b/README.md index e1b79a887d..3f91b1a9f2 100644 --- a/README.md +++ b/README.md @@ -49,6 +49,19 @@ cargo build --release cargo build --profile release-lite ``` +_Note on building_: you may set `RUSTFLAGS` to build binaries for your native cpu: + +``` +RUSTFLAGS="-Ctarget-cpu=native" +``` + +or uncomment these lines in `./cargo/config`: + +``` +# [build] +# rustflags = ["-Ctarget-cpu=native"] +``` + ## Testing **Run the tests:**