diff --git a/stacks-signer/src/cli.rs b/stacks-signer/src/cli.rs index 28ead30fee..481de71061 100644 --- a/stacks-signer/src/cli.rs +++ b/stacks-signer/src/cli.rs @@ -351,7 +351,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") @@ -359,9 +359,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/client/mod.rs b/stacks-signer/src/client/mod.rs index 87bee14750..3e1da7b20c 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( @@ -515,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/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 b89c5462dd..7d4e32c43d 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,9 +198,43 @@ 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_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 +263,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, @@ -316,12 +349,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 @@ -365,7 +397,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> { @@ -385,7 +417,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 @@ -403,23 +435,22 @@ 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); - 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, @@ -427,7 +458,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 { @@ -469,13 +500,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..."); @@ -488,9 +518,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, @@ -499,15 +528,12 @@ impl StacksClient { self.tx_version, self.chain_id, nonce, - tx_fee, - ) + )?; + Ok(unsigned_tx) } /// 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 = || { @@ -609,9 +635,13 @@ 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( + pub fn build_unsigned_contract_call_transaction( contract_addr: &StacksAddress, contract_name: ContractName, function_name: ClarityName, @@ -620,7 +650,6 @@ impl StacksClient { tx_version: TransactionVersion, chain_id: u32, nonce: u64, - tx_fee: u64, ) -> Result { let tx_payload = TransactionPayload::ContractCall(TransactionContractCall { address: *contract_addr, @@ -639,17 +668,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 @@ -683,10 +717,10 @@ 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_round_info_response, build_get_vote_for_aggregate_key_response, - build_get_weight_threshold_response, build_read_only_response, write_response, - MockServerClient, + build_get_last_round_response, build_get_medium_estimated_fee_ustx_response, + build_get_peer_info_response, build_get_pox_data_response, build_get_round_info_response, + build_get_vote_for_aggregate_key_response, build_get_weight_threshold_response, + build_read_only_response, write_response, MockServerClient, }; #[test] @@ -854,12 +888,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"), @@ -868,10 +901,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[..]); @@ -889,7 +923,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, @@ -906,7 +940,6 @@ mod tests { ); } - #[ignore] #[test] fn build_vote_for_aggregate_public_key_should_succeed() { let mock = MockServerClient::new(); @@ -917,19 +950,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(); @@ -938,28 +969,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_with_retry(&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] @@ -1133,7 +1163,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()); } @@ -1157,7 +1187,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()); } @@ -1166,7 +1196,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); } @@ -1207,7 +1237,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); } @@ -1262,4 +1292,27 @@ mod tests { write_response(mock.server, round_response.as_bytes()); assert_eq!(h.join().unwrap().unwrap(), weight as u128); } + + #[test] + fn get_medium_estimated_fee_ustx_should_succeed() { + let mock = MockServerClient::new(); + let private_key = StacksPrivateKey::new(); + let unsigned_tx = StacksClient::build_unsigned_contract_call_transaction( + &mock.client.stacks_address, + ContractName::from("contract-name"), + ClarityName::from("function-name"), + &[], + &private_key, + TransactionVersion::Testnet, + CHAIN_ID_TESTNET, + 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); + } } diff --git a/stacks-signer/src/config.rs b/stacks-signer/src/config.rs index dc48dda27a..567c61aa8c 100644 --- a/stacks-signer/src/config.rs +++ b/stacks-signer/src/config.rs @@ -34,8 +34,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)] @@ -144,8 +143,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, } @@ -177,8 +178,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 @@ -209,8 +212,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 @@ -306,6 +312,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, }) @@ -366,6 +373,7 @@ impl Display for 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, @@ -374,6 +382,8 @@ pub fn build_signer_config_tomls( password: &str, run_stamp: u16, mut port_start: usize, + max_tx_fee_ustx: Option, + tx_fee_ustx: Option, ) -> Vec { let mut signer_config_tomls = vec![]; @@ -405,7 +415,25 @@ 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} +"# + ) + } + + if let Some(tx_fee_ustx) = tx_fee_ustx { + signer_config_toml = format!( + r#" +{signer_config_toml} +tx_fee_ustx = {tx_fee_ustx} "# ) } @@ -439,12 +467,120 @@ mod tests { password, 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); } #[test] diff --git a/stacks-signer/src/coordinator.rs b/stacks-signer/src/coordinator.rs index 7469c0ff18..7fc2d238c4 100644 --- a/stacks-signer/src/coordinator.rs +++ b/stacks-signer/src/coordinator.rs @@ -91,17 +91,10 @@ impl CoordinatorSelector { } } new_index + } else if ROTATE_COORDINATORS { + self.coordinator_index.saturating_add(1) % self.coordinator_ids.len() } else { - if ROTATE_COORDINATORS { - let mut new_index = self.coordinator_index.saturating_add(1); - if new_index == self.coordinator_ids.len() { - // We have exhausted all potential coordinators. Go back to the start - new_index = 0; - } - new_index - } else { - self.coordinator_index - } + self.coordinator_index }; self.coordinator_id = *self .coordinator_ids diff --git a/stacks-signer/src/main.rs b/stacks-signer/src/main.rs index 34a9f62dc3..285dc7f7e7 100644 --- a/stacks-signer/src/main.rs +++ b/stacks-signer/src/main.rs @@ -295,6 +295,8 @@ fn handle_generate_files(args: GenerateFilesArgs) { &args.password, rand::random(), 3000, + None, + None, ); debug!("Built {:?} signer config tomls.", signer_config_tomls.len()); for (i, file_contents) in signer_config_tomls.iter().enumerate() { @@ -425,6 +427,7 @@ pub mod tests { use super::{handle_generate_stacking_signature, *}; use crate::{GenerateStackingSignatureArgs, GlobalConfig}; + #[allow(clippy::too_many_arguments)] fn call_verify_signer_sig( pox_addr: &PoxAddress, reward_cycle: u128, diff --git a/stacks-signer/src/runloop.rs b/stacks-signer/src/runloop.rs index 4491650090..17b74c2fc9 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, ) @@ -128,10 +125,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); }; @@ -213,6 +207,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(), }) } @@ -390,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, current_reward_cycle) - .map_err(backoff::Error::transient) - }) { + if let Err(e) = signer.update_dkg(&self.stacks_client) { error!("{signer}: failed to update DKG: {e}"); } } @@ -435,8 +426,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; @@ -462,4 +456,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) + )); + } } diff --git a/stacks-signer/src/signer.rs b/stacks-signer/src/signer.rs index 4d23a92c07..3da523eeaf 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; @@ -49,7 +48,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; @@ -159,8 +158,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 @@ -186,9 +188,11 @@ impl std::fmt::Display for Signer { } impl Signer { - /// Return the current coordinator. If in the active reward cycle, this is the miner, - /// so the first element of the tuple will be None (because the miner does not have a signer index). - fn get_coordinator(&self, current_reward_cycle: u64) -> (Option, PublicKey) { + /// Return the current coordinator. + /// If the current reward cycle is the active reward cycle, this is the miner, + /// so the first element of the tuple will be None (because the miner does not have a signer index). + /// Otherwise, the coordinator is the signer with the index returned by the coordinator selector. + fn get_coordinator_sign(&self, current_reward_cycle: u64) -> (Option, PublicKey) { if self.reward_cycle == current_reward_cycle { let Some(ref cur_miner) = self.miner_key else { error!( @@ -199,12 +203,18 @@ impl Signer { return (Some(selected.0), selected.1); }; // coordinator is the current miner. - (None, cur_miner.clone()) + (None, *cur_miner) } else { let selected = self.coordinator_selector.get_coordinator(); - return (Some(selected.0), selected.1); + (Some(selected.0), selected.1) } } + + /// Get the current coordinator for executing DKG + /// This will always use the coordinator selector to determine the coordinator + fn get_coordinator_dkg(&self) -> (u32, PublicKey) { + self.coordinator_selector.get_coordinator() + } } impl From for Signer { @@ -295,6 +305,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, @@ -345,11 +356,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:?}"); @@ -429,24 +436,36 @@ impl Signer { stacks_client: &StacksClient, current_reward_cycle: u64, ) { - let coordinator_id = self.get_coordinator(current_reward_cycle).0; match &self.state { State::Idle => { + let Some(command) = self.commands.front() else { + debug!("{self}: Nothing to process. Waiting for command..."); + return; + }; + let coordinator_id = if matches!(command, Command::Dkg) { + // We cannot execute a DKG command if we are not the coordinator + Some(self.get_coordinator_dkg().0) + } else { + self.get_coordinator_sign(current_reward_cycle).0 + }; if coordinator_id != Some(self.signer_id) { debug!( "{self}: Coordinator is {coordinator_id:?}. Will not process any commands...", ); return; } - if let Some(command) = self.commands.pop_front() { - self.execute_command(stacks_client, &command); - } else { - debug!("{self}: Nothing to process. Waiting for command...",); - } + let command = self + .commands + .pop_front() + .expect("BUG: Already asserted that the command queue was not empty"); + self.execute_command(stacks_client, &command); } State::OperationInProgress => { // We cannot execute the next command until the current one is finished... - debug!("{self}: Waiting for coordinator {coordinator_id:?} operation to finish. Coordinator state = {:?}", self.coordinator.state); + debug!( + "{self}: Waiting for operation to finish. Coordinator state = {:?}", + self.coordinator.state + ); } } } @@ -459,7 +478,6 @@ impl Signer { res: Sender>, current_reward_cycle: u64, ) { - let coordinator_id = self.get_coordinator(current_reward_cycle).0; let mut block_info = match block_validate_response { BlockValidateResponse::Ok(block_validate_ok) => { let signer_signature_hash = block_validate_ok.signer_signature_hash; @@ -531,32 +549,13 @@ impl Signer { sig: vec![], }; self.handle_packets(stacks_client, res, &[packet], current_reward_cycle); - } else { - if block_info.valid.unwrap_or(false) - && !block_info.signed_over - && coordinator_id == Some(self.signer_id) - { - // We are the coordinator. Trigger a signing round for this block - debug!( - "{self}: attempt to trigger a signing round for block"; - "signer_sighash" => %block_info.block.header.signer_signature_hash(), - "block_hash" => %block_info.block.header.block_hash(), - ); - self.commands.push_back(Command::Sign { - block: block_info.block.clone(), - is_taproot: false, - merkle_root: None, - }); - } else { - debug!( - "{self}: ignoring block."; - "block_hash" => block_info.block.header.block_hash(), - "valid" => block_info.valid, - "signed_over" => block_info.signed_over, - "coordinator_id" => coordinator_id, - ); - } } + debug!( + "{self}: Received a block validate response"; + "block_hash" => block_info.block.header.block_hash(), + "valid" => block_info.valid, + "signed_over" => block_info.signed_over, + ); self.signer_db .insert_block(self.reward_cycle, &block_info) .unwrap_or_else(|_| panic!("{self}: Failed to insert block in DB")); @@ -570,7 +569,6 @@ impl Signer { messages: &[SignerMessage], current_reward_cycle: u64, ) { - let coordinator_pubkey = self.get_coordinator(current_reward_cycle).1; let packets: Vec = messages .iter() .filter_map(|msg| match msg { @@ -579,6 +577,11 @@ impl Signer { | SignerMessage::Transactions(_) => None, // TODO: if a signer tries to trigger DKG and we already have one set in the contract, ignore the request. SignerMessage::Packet(packet) => { + let coordinator_pubkey = if Self::is_dkg_message(&packet.msg) { + self.get_coordinator_dkg().1 + } else { + self.get_coordinator_sign(current_reward_cycle).1 + }; self.verify_packet(stacks_client, packet.clone(), &coordinator_pubkey) } }) @@ -628,7 +631,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:?}"); }); @@ -643,6 +646,19 @@ impl Signer { } } + /// Helper function for determining if the provided message is a DKG specific message + fn is_dkg_message(msg: &Message) -> bool { + matches!( + msg, + Message::DkgBegin(_) + | Message::DkgEnd(_) + | Message::DkgEndBegin(_) + | Message::DkgPrivateBegin(_) + | Message::DkgPrivateShares(_) + | Message::DkgPublicShares(_) + ) + } + /// Process inbound packets as both a signer and a coordinator /// Will send outbound packets and operation results as appropriate fn handle_packets( @@ -770,7 +786,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:?}",); }); @@ -794,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) { @@ -857,7 +877,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 +902,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, @@ -940,7 +960,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"); @@ -1003,54 +1023,34 @@ 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(|| { - 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); - match stacks_client.build_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, - ) { + let epoch = stacks_client + .get_node_epoch() + .unwrap_or(StacksEpochId::Epoch24); + 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, @@ -1071,6 +1071,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 { + 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) + .map_err(|e| { + warn!("{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, @@ -1108,7 +1146,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()); @@ -1235,64 +1273,20 @@ 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> { - 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(()); - }; + /// Should DKG be queued to the current signer's command queue + pub fn should_queue_dkg(&mut self, stacks_client: &StacksClient) -> Result { if self.state != State::Idle - || Some(self.signer_id) != self.get_coordinator(current_reward_cycle).0 + || self.signer_id != self.get_coordinator_dkg().0 + || self.commands.front() == Some(&Command::Dkg) { - // We are not the coordinator 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 +1296,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 +1309,82 @@ 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()?; + // 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) -> 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)? { 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/stacks-signer/src/signerdb.rs b/stacks-signer/src/signerdb.rs index ea9c4eeb17..1f568ff37f 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, @@ -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) @@ -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/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) } 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() diff --git a/testnet/stacks-node/src/tests/signer.rs b/testnet/stacks-node/src/tests/signer.rs index 01024343db..867421b5c0 100644 --- a/testnet/stacks-node/src/tests/signer.rs +++ b/testnet/stacks-node/src/tests/signer.rs @@ -120,6 +120,8 @@ impl SignerTest { password, run_stamp, 3000, + Some(100_000), + None, ); let mut running_signers = Vec::new(); @@ -549,7 +551,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 +560,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 +572,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 +584,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 +602,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 +619,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 +636,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 +654,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 +666,6 @@ impl SignerTest { TransactionVersion::Testnet, CHAIN_ID_TESTNET, 0, // Old nonce - 10, ) .unwrap(); @@ -682,10 +676,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 +690,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` @@ -724,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();