From 3d16785a2cd46f182cf7407e4a057677d453c88a Mon Sep 17 00:00:00 2001 From: Francesco Leacche Date: Thu, 6 Feb 2025 19:40:45 +0000 Subject: [PATCH] rotate key - wait for all signers, but sign only required --- signer/src/stacks/api.rs | 42 ++++------------- signer/src/stacks/wallet.rs | 45 +++++++------------ signer/src/testing/block_observer.rs | 8 +--- signer/src/testing/context.rs | 3 +- signer/src/testing/transaction_coordinator.rs | 2 +- signer/src/transaction_coordinator.rs | 31 ++++++++----- signer/tests/integration/contracts.rs | 7 +-- .../integration/transaction_coordinator.rs | 39 ++++++++-------- 8 files changed, 68 insertions(+), 109 deletions(-) diff --git a/signer/src/stacks/api.rs b/signer/src/stacks/api.rs index efe6d5973..f7dd53ec7 100644 --- a/signer/src/stacks/api.rs +++ b/signer/src/stacks/api.rs @@ -194,7 +194,6 @@ pub trait StacksInteract: Send + Sync { wallet: &SignerWallet, payload: &T, priority: FeePriority, - require_all_signatures: bool, ) -> impl Future> + Send where T: AsTxPayload + Send + Sync; @@ -1180,13 +1179,11 @@ impl StacksInteract for StacksClient { wallet: &SignerWallet, payload: &T, priority: FeePriority, - require_all_signatures: bool, ) -> Result where T: AsTxPayload + Send + Sync, { - let transaction_size = - super::wallet::get_full_tx_size(payload, wallet, require_all_signatures)?; + let transaction_size = super::wallet::get_full_tx_size(payload, wallet)?; // In Stacks core, the minimum fee is 1 mSTX per byte, so take the // transaction size and multiply it by the TX_FEE_TX_SIZE_MULTIPLIER @@ -1369,15 +1366,12 @@ impl StacksInteract for ApiFallbackClient { wallet: &SignerWallet, payload: &T, priority: FeePriority, - require_all_signatures: bool, ) -> Result where T: AsTxPayload + Send + Sync, { - self.exec(|client, _| { - StacksClient::estimate_fees(client, wallet, payload, priority, require_all_signatures) - }) - .await + self.exec(|client, _| StacksClient::estimate_fees(client, wallet, payload, priority)) + .await } async fn get_pox_info(&self) -> Result { @@ -1927,16 +1921,11 @@ mod tests { let client = StacksClient::new(url::Url::parse(stacks_node_server.url().as_str()).unwrap()).unwrap(); - let expected_fee = get_full_tx_size(&DUMMY_STX_TRANSFER_PAYLOAD, &wallet, false).unwrap() + let expected_fee = get_full_tx_size(&DUMMY_STX_TRANSFER_PAYLOAD, &wallet).unwrap() * TX_FEE_TX_SIZE_MULTIPLIER; let resp = client - .estimate_fees( - &wallet, - &DUMMY_STX_TRANSFER_PAYLOAD, - FeePriority::High, - false, - ) + .estimate_fees(&wallet, &DUMMY_STX_TRANSFER_PAYLOAD, FeePriority::High) .await .unwrap(); @@ -1992,34 +1981,19 @@ mod tests { // Now lets check that the interface function returns the requested // priority fees. let fee = client - .estimate_fees( - &wallet, - &DUMMY_STX_TRANSFER_PAYLOAD, - FeePriority::Low, - false, - ) + .estimate_fees(&wallet, &DUMMY_STX_TRANSFER_PAYLOAD, FeePriority::Low) .await .unwrap(); assert_eq!(fee, 7679); let fee = client - .estimate_fees( - &wallet, - &DUMMY_STX_TRANSFER_PAYLOAD, - FeePriority::Medium, - false, - ) + .estimate_fees(&wallet, &DUMMY_STX_TRANSFER_PAYLOAD, FeePriority::Medium) .await .unwrap(); assert_eq!(fee, 7680); let fee = client - .estimate_fees( - &wallet, - &DUMMY_STX_TRANSFER_PAYLOAD, - FeePriority::High, - false, - ) + .estimate_fees(&wallet, &DUMMY_STX_TRANSFER_PAYLOAD, FeePriority::High) .await .unwrap(); assert_eq!(fee, 25505); diff --git a/signer/src/stacks/wallet.rs b/signer/src/stacks/wallet.rs index 24a1ae456..8f823b75c 100644 --- a/signer/src/stacks/wallet.rs +++ b/signer/src/stacks/wallet.rs @@ -345,6 +345,11 @@ impl MultisigTx { &self.tx } + /// Return the message digest associated with the transaction + pub fn digest(&self) -> &Message { + &self.digest + } + /// Return the total number of signatures that have been received so /// far for this transaction. pub fn num_signatures(&self) -> u16 { @@ -402,11 +407,7 @@ impl MultisigTx { /// given payload. /// /// This function is very unlikely to fail in practice. -pub fn get_full_tx_size( - payload: &T, - wallet: &SignerWallet, - require_all_signatures: bool, -) -> Result +pub fn get_full_tx_size(payload: &T, wallet: &SignerWallet) -> Result where T: AsTxPayload, { @@ -436,14 +437,11 @@ where 0, )?; - let sigantures_required = if require_all_signatures { - wallet.num_signers() - } else { - wallet.signatures_required - } as usize; - let mut multisig_tx = MultisigTx::new_tx(payload, &wallet, 0); - for private_key in private_keys.iter().take(sigantures_required) { + for private_key in private_keys + .iter() + .take(wallet.signatures_required as usize) + { let signature = crate::signature::sign_stacks_tx(multisig_tx.tx(), private_key); // This won't fail, since this is a proper signature multisig_tx.add_signature(signature)?; @@ -759,13 +757,10 @@ mod tests { SignerWallet::load_boostrap_wallet(&ctx.config().signer).unwrap(); } - #[test_case(1, 1, true)] - #[test_case(1, 1, false)] - #[test_case(2, 3, true)] - #[test_case(2, 3, false)] - #[test_case(11, 15, true)] - #[test_case(11, 15, false)] - fn can_get_full_tx_size(signatures_required: u16, num_keys: u16, require_all_signatures: bool) { + #[test_case(1, 1)] + #[test_case(2, 3)] + #[test_case(11, 15)] + fn can_get_full_tx_size(signatures_required: u16, num_keys: u16) { const BASE_TX_SIZE: u64 = 55; const SIGNATURE_SIZE: u64 = 66; const PUBKEY_SIZE: u64 = 34; @@ -783,18 +778,12 @@ mod tests { let payload_size = payload.tx_payload().serialize_to_vec().len() as u64; - let signatures_used = if require_all_signatures { - num_keys - } else { - signatures_required - }; - let expected_size = BASE_TX_SIZE + payload_size - + (signatures_used as u64 * SIGNATURE_SIZE) - + ((num_keys - signatures_used) as u64 * PUBKEY_SIZE); + + (signatures_required as u64 * SIGNATURE_SIZE) + + ((num_keys - signatures_required) as u64 * PUBKEY_SIZE); - let size = get_full_tx_size(&payload, &wallet, require_all_signatures).unwrap(); + let size = get_full_tx_size(&payload, &wallet).unwrap(); assert_eq!(size, expected_size); } diff --git a/signer/src/testing/block_observer.rs b/signer/src/testing/block_observer.rs index f37574c78..2dbdfe4cf 100644 --- a/signer/src/testing/block_observer.rs +++ b/signer/src/testing/block_observer.rs @@ -407,13 +407,7 @@ impl StacksInteract for TestHarness { }) } - async fn estimate_fees( - &self, - _: &SignerWallet, - _: &T, - _: FeePriority, - _: bool, - ) -> Result + async fn estimate_fees(&self, _: &SignerWallet, _: &T, _: FeePriority) -> Result where T: crate::stacks::contracts::AsTxPayload, { diff --git a/signer/src/testing/context.rs b/signer/src/testing/context.rs index e66b7081c..782ab0a4b 100644 --- a/signer/src/testing/context.rs +++ b/signer/src/testing/context.rs @@ -438,7 +438,6 @@ impl StacksInteract for WrappedMock { wallet: &SignerWallet, payload: &T, priority: FeePriority, - require_all_signatures: bool, ) -> Result where T: AsTxPayload + Send + Sync, @@ -446,7 +445,7 @@ impl StacksInteract for WrappedMock { self.inner .lock() .await - .estimate_fees(wallet, payload, priority, require_all_signatures) + .estimate_fees(wallet, payload, priority) .await } diff --git a/signer/src/testing/transaction_coordinator.rs b/signer/src/testing/transaction_coordinator.rs index aafdf0f27..7737d103d 100644 --- a/signer/src/testing/transaction_coordinator.rs +++ b/signer/src/testing/transaction_coordinator.rs @@ -513,7 +513,7 @@ where client .expect_estimate_fees() .times(5) - .returning(|_, _, _, _| Box::pin(async { Ok(123000) })); + .returning(|_, _, _| Box::pin(async { Ok(123000) })); client.expect_get_account().times(5).returning(|_| { Box::pin(async { diff --git a/signer/src/transaction_coordinator.rs b/signer/src/transaction_coordinator.rs index 896aa7b52..4d224bdff 100644 --- a/signer/src/transaction_coordinator.rs +++ b/signer/src/transaction_coordinator.rs @@ -43,6 +43,7 @@ use crate::metrics::Metrics; use crate::metrics::BITCOIN_BLOCKCHAIN; use crate::metrics::STACKS_BLOCKCHAIN; use crate::network; +use crate::signature::RecoverableEcdsaSignature as _; use crate::signature::TaprootSignature; use crate::stacks::api::FeePriority; use crate::stacks::api::GetNakamotoStartHeight; @@ -719,7 +720,7 @@ where let tx_fee = self .context .get_stacks_client() - .estimate_fees(wallet, &contract_call, FeePriority::High, true) + .estimate_fees(wallet, &contract_call, FeePriority::High) .await?; let multi_tx = MultisigTx::new_tx(&contract_call, wallet, tx_fee); @@ -872,7 +873,7 @@ where let tx_fee = self .context .get_stacks_client() - .estimate_fees(wallet, &contract_call, FeePriority::High, false) + .estimate_fees(wallet, &contract_call, FeePriority::High) .await?; let multi_tx = MultisigTx::new_tx(&contract_call, wallet, tx_fee); @@ -914,7 +915,7 @@ where wallet.signatures_required() } else { wallet.num_signers() - }; + } as usize; // We ask for the signers to sign our transaction (including // ourselves, via our tx signer event loop) @@ -929,7 +930,9 @@ where tokio::pin!(signal_stream); let future = async { - while multi_tx.num_signatures() < signatures_required { + let mut pending_signer = wallet.public_keys().clone(); + + while pending_signer.len() < signatures_required { // If signal_stream.next() returns None then one of the // underlying streams has closed. That means either the // network stream, the internal message stream, or the @@ -954,6 +957,19 @@ where _ => continue, }; + let enough_signatures = multi_tx.num_signatures() >= wallet.signatures_required(); + let recovered_key = sig.signature.recover_ecdsa(multi_tx.digest()); + + if let Ok(key) = recovered_key { + pending_signer.remove(&key); + } + + // Stop collecting signatures once we have enough, but keep tracking responses + // from remaining signers for key rotation transactions + if enough_signatures { + continue; + } + if let Err(error) = multi_tx.add_signature(sig.signature) { tracing::warn!( %txid, @@ -1552,12 +1568,7 @@ where let tx_fee = self .context .get_stacks_client() - .estimate_fees( - wallet, - &contract_deploy.tx_payload(), - FeePriority::High, - false, - ) + .estimate_fees(wallet, &contract_deploy.tx_payload(), FeePriority::High) .await?; let multi_tx = MultisigTx::new_tx(&contract_deploy, wallet, tx_fee); let tx = multi_tx.tx(); diff --git a/signer/tests/integration/contracts.rs b/signer/tests/integration/contracts.rs index 87bc713c5..c784cb9b5 100644 --- a/signer/tests/integration/contracts.rs +++ b/signer/tests/integration/contracts.rs @@ -245,12 +245,7 @@ async fn estimate_tx_fees() { // transaction. let fee = client - .estimate_fees( - &testing::wallet::WALLET.0, - &payload, - FeePriority::Medium, - false, - ) + .estimate_fees(&testing::wallet::WALLET.0, &payload, FeePriority::Medium) .await .unwrap(); more_asserts::assert_gt!(fee, 0); diff --git a/signer/tests/integration/transaction_coordinator.rs b/signer/tests/integration/transaction_coordinator.rs index 843dda341..a91114df4 100644 --- a/signer/tests/integration/transaction_coordinator.rs +++ b/signer/tests/integration/transaction_coordinator.rs @@ -229,7 +229,7 @@ fn mock_deploy_all_contracts( // deployed and 1 for the deposit tx client .expect_estimate_fees() - .returning(|_, _, _, _| Box::pin(async { Ok(100) })); + .returning(|_, _, _| Box::pin(async { Ok(100) })); client.expect_get_account().returning(move |_| { Box::pin(async move { @@ -296,7 +296,7 @@ fn mock_deploy_remaining_contracts_when_some_already_deployed( client .expect_estimate_fees() .times(3) - .returning(|_, _, _, _| Box::pin(async { Ok(100) })); + .returning(|_, _, _| Box::pin(async { Ok(100) })); client.expect_get_account().times(3).returning(move |_| { Box::pin(async move { @@ -344,21 +344,18 @@ fn mock_recover_and_deploy_all_contracts_after_failure( client .expect_estimate_fees() .once() - .returning(|_, _, _, _| Box::pin(async { Ok(100) })); + .returning(|_, _, _| Box::pin(async { Ok(100) })); // In the process of deploying the second contract, the coordinator // will fail to estimate fees and it will abort the deployment It // will try again from scratch when It'll receive a second signal. - client - .expect_estimate_fees() - .times(1) - .returning(|_, _, _, _| { - Box::pin(async { - Err(Error::UnexpectedStacksResponse( - mock_reqwests_status_code_error(500).await, - )) - }) - }); + client.expect_estimate_fees().times(1).returning(|_, _, _| { + Box::pin(async { + Err(Error::UnexpectedStacksResponse( + mock_reqwests_status_code_error(500).await, + )) + }) + }); // The coordinator should try again from scratch. For the first // contract we will return the contract source as if it was already @@ -393,7 +390,7 @@ fn mock_recover_and_deploy_all_contracts_after_failure( client .expect_estimate_fees() .times(4) - .returning(|_, _, _, _| Box::pin(async { Ok(100) })); + .returning(|_, _, _| Box::pin(async { Ok(100) })); // `get_account` will be called 6 times, 2 for the first try to // deploy the contracts, 4 for the second try @@ -472,7 +469,7 @@ async fn process_complete_deposit() { client .expect_estimate_fees() .once() - .returning(move |_, _, _, _| Box::pin(async move { Ok(25505) })); + .returning(move |_, _, _| Box::pin(async move { Ok(25505) })); }) .await; @@ -863,7 +860,7 @@ async fn run_dkg_from_scratch() { ctx.with_stacks_client(|client| { client .expect_estimate_fees() - .returning(|_, _, _, _| Box::pin(async { Ok(123000) })); + .returning(|_, _, _| Box::pin(async { Ok(123000) })); client.expect_get_account().returning(|_| { Box::pin(async { @@ -1113,7 +1110,7 @@ async fn run_subsequent_dkg() { ctx.with_stacks_client(|client| { client .expect_estimate_fees() - .returning(|_, _, _, _| Box::pin(async { Ok(123000) })); + .returning(|_, _, _| Box::pin(async { Ok(123000) })); client.expect_get_account().returning(|_| { Box::pin(async { @@ -1400,7 +1397,7 @@ async fn sign_bitcoin_transaction() { client .expect_estimate_fees() - .returning(|_, _, _, _| Box::pin(std::future::ready(Ok(25)))); + .returning(|_, _, _| Box::pin(std::future::ready(Ok(25)))); // The coordinator will try to further process the deposit to submit // the stacks tx, but we are not interested (for the current test iteration). @@ -1831,7 +1828,7 @@ async fn sign_bitcoin_transaction_multiple_locking_keys() { client .expect_estimate_fees() - .returning(|_, _, _, _| Box::pin(std::future::ready(Ok(25)))); + .returning(|_, _, _| Box::pin(std::future::ready(Ok(25)))); // The coordinator will try to further process the deposit to submit // the stacks tx, but we are not interested (for the current test iteration). @@ -2421,7 +2418,7 @@ async fn skip_smart_contract_deployment_and_key_rotation_if_up_to_date() { client .expect_estimate_fees() - .returning(|_, _, _, _| Box::pin(std::future::ready(Ok(25)))); + .returning(|_, _, _| Box::pin(std::future::ready(Ok(25)))); // The coordinator will try to further process the deposit to submit // the stacks tx, but we are not interested (for the current test iteration). @@ -3201,7 +3198,7 @@ async fn test_conservative_initial_sbtc_limits() { client .expect_estimate_fees() - .returning(|_, _, _, _| Box::pin(std::future::ready(Ok(25)))); + .returning(|_, _, _| Box::pin(std::future::ready(Ok(25)))); // The coordinator will try to further process the deposit to submit // the stacks tx, but we are not interested (for the current test iteration).