From 810eb9453b6ab696c48ef3e85bd048dee1a6378a Mon Sep 17 00:00:00 2001 From: djordon Date: Thu, 3 Oct 2024 22:58:34 -0400 Subject: [PATCH 01/15] Add the transaction id to the stacks transaction sign request object --- protobufs/stacks/signer/v1/requests.proto | 10 ++++++---- signer/src/message.rs | 2 ++ signer/src/proto/generated/stacks.signer.v1.rs | 13 ++++++++----- signer/src/testing/message.rs | 2 ++ signer/src/transaction_coordinator.rs | 8 +++++--- 5 files changed, 23 insertions(+), 12 deletions(-) diff --git a/protobufs/stacks/signer/v1/requests.proto b/protobufs/stacks/signer/v1/requests.proto index 4b3b1e06..f485ce9b 100644 --- a/protobufs/stacks/signer/v1/requests.proto +++ b/protobufs/stacks/signer/v1/requests.proto @@ -26,16 +26,18 @@ message StacksTransactionSignRequest { // essentially a hash of the contract call struct, the nonce, the tx_fee // and a few other things. crypto.Uint256 digest = 4; + // The transaction ID of the associated contract call transaction. + crypto.Uint256 txid = 5; // The contract call transaction to sign. oneof contract_call { // The `complete-deposit` contract call - CompleteDeposit complete_deposit = 5; + CompleteDeposit complete_deposit = 6; // The `accept-withdrawal-request` contract call - AcceptWithdrawal accept_withdrawal = 6; + AcceptWithdrawal accept_withdrawal = 7; // The `reject-withdrawal-request` contract call - RejectWithdrawal reject_withdrawal = 7; + RejectWithdrawal reject_withdrawal = 8; // The `rotate-keys-wrapper` contract call - RotateKeys rotate_keys = 8; + RotateKeys rotate_keys = 9; } } diff --git a/signer/src/message.rs b/signer/src/message.rs index 158a70b6..9714f9f4 100644 --- a/signer/src/message.rs +++ b/signer/src/message.rs @@ -165,6 +165,8 @@ pub struct StacksTransactionSignRequest { /// It's essentially a hash of the contract call struct, the nonce, the /// tx_fee and a few other things. pub digest: [u8; 32], + /// The transaction ID of the associated contract call transaction. + pub txid: blockstack_lib::burnchains::Txid, } /// Represents a signature of a Stacks transaction. diff --git a/signer/src/proto/generated/stacks.signer.v1.rs b/signer/src/proto/generated/stacks.signer.v1.rs index f27fa6db..cf934e12 100644 --- a/signer/src/proto/generated/stacks.signer.v1.rs +++ b/signer/src/proto/generated/stacks.signer.v1.rs @@ -83,10 +83,13 @@ pub struct StacksTransactionSignRequest { /// and a few other things. #[prost(message, optional, tag = "4")] pub digest: ::core::option::Option, + /// The transaction ID of the associated contract call transaction. + #[prost(message, optional, tag = "5")] + pub txid: ::core::option::Option, /// The contract call transaction to sign. #[prost( oneof = "stacks_transaction_sign_request::ContractCall", - tags = "5, 6, 7, 8" + tags = "6, 7, 8, 9" )] pub contract_call: ::core::option::Option< stacks_transaction_sign_request::ContractCall, @@ -99,16 +102,16 @@ pub mod stacks_transaction_sign_request { #[derive(Clone, PartialEq, ::prost::Oneof)] pub enum ContractCall { /// The `complete-deposit` contract call - #[prost(message, tag = "5")] + #[prost(message, tag = "6")] CompleteDeposit(super::CompleteDeposit), /// The `accept-withdrawal-request` contract call - #[prost(message, tag = "6")] + #[prost(message, tag = "7")] AcceptWithdrawal(super::AcceptWithdrawal), /// The `reject-withdrawal-request` contract call - #[prost(message, tag = "7")] + #[prost(message, tag = "8")] RejectWithdrawal(super::RejectWithdrawal), /// The `rotate-keys-wrapper` contract call - #[prost(message, tag = "8")] + #[prost(message, tag = "9")] RotateKeys(super::RotateKeys), } } diff --git a/signer/src/testing/message.rs b/signer/src/testing/message.rs index 00dcdeb8..c6fdad30 100644 --- a/signer/src/testing/message.rs +++ b/signer/src/testing/message.rs @@ -11,6 +11,7 @@ use crate::message; use crate::stacks::contracts::ContractCall; use crate::stacks::contracts::RejectWithdrawalV1; use crate::storage::model::BitcoinBlockHash; +use crate::storage::model::StacksTxId; use crate::testing::dummy; impl message::SignerMessage { @@ -105,6 +106,7 @@ impl fake::Dummy for message::StacksTransactionSignRequest { nonce: 1, aggregate_key: PublicKey::from_private_key(&private_key), digest: config.fake_with_rng(rng), + txid: config.fake_with_rng::(rng).into(), } } } diff --git a/signer/src/transaction_coordinator.rs b/signer/src/transaction_coordinator.rs index e63228f3..d113a56f 100644 --- a/signer/src/transaction_coordinator.rs +++ b/signer/src/transaction_coordinator.rs @@ -353,13 +353,15 @@ where .await?; let multi_tx = MultisigTx::new_tx(&contract_call, wallet, tx_fee); + let tx = multi_tx.tx(); Ok(StacksTransactionSignRequest { aggregate_key: *bitcoin_aggregate_key, contract_call, - nonce: multi_tx.tx().get_origin_nonce(), - tx_fee: multi_tx.tx().get_tx_fee(), - digest: multi_tx.tx().digest(), + nonce: tx.get_origin_nonce(), + tx_fee: tx.get_tx_fee(), + digest: tx.digest(), + txid: tx.txid(), }) } From d093c95b9edde52ed155fc955574ef1c5f73af67 Mon Sep 17 00:00:00 2001 From: djordon Date: Fri, 4 Oct 2024 00:52:49 -0400 Subject: [PATCH 02/15] Make the context types static, since they are. --- signer/src/context/mod.rs | 30 +++++++++++++++--------------- signer/src/testing/context.rs | 18 +++++++++--------- 2 files changed, 24 insertions(+), 24 deletions(-) diff --git a/signer/src/context/mod.rs b/signer/src/context/mod.rs index 7c4a5071..d7fc1583 100644 --- a/signer/src/context/mod.rs +++ b/signer/src/context/mod.rs @@ -30,15 +30,15 @@ pub trait Context: Clone + Sync + Send { /// Returns a handle to the application's termination signal. fn get_termination_handle(&self) -> TerminationHandle; /// Get a read-only handle to the signer storage. - fn get_storage(&self) -> impl DbRead + Clone + Sync + Send; + fn get_storage(&self) -> impl DbRead + Clone + Sync + Send + 'static; /// Get a read-write handle to the signer storage. - fn get_storage_mut(&self) -> impl DbRead + DbWrite + Clone + Sync + Send; + fn get_storage_mut(&self) -> impl DbRead + DbWrite + Clone + Sync + Send + 'static; /// Get a handle to a Bitcoin client. - fn get_bitcoin_client(&self) -> impl BitcoinInteract + Clone; + fn get_bitcoin_client(&self) -> impl BitcoinInteract + Clone + 'static; /// Get a handler to the Stacks client. - fn get_stacks_client(&self) -> impl StacksInteract + Clone; + fn get_stacks_client(&self) -> impl StacksInteract + Clone + 'static; /// Get a handle to a Emily client. - fn get_emily_client(&self) -> impl EmilyInteract + Clone; + fn get_emily_client(&self) -> impl EmilyInteract + Clone + 'static; } /// Signer context which is passed to different components within the @@ -71,7 +71,7 @@ pub struct SignerContext { impl SignerContext where - S: DbRead + DbWrite + Clone + Sync + Send, + S: DbRead + DbWrite + Clone + Sync + Send + 'static, BC: for<'a> TryFrom<&'a [Url]> + BitcoinInteract + Clone + 'static, ST: for<'a> TryFrom<&'a Settings> + StacksInteract + Clone + Sync + Send + 'static, EM: for<'a> TryFrom<&'a [Url]> + EmilyInteract + Clone + Sync + Send + 'static, @@ -125,10 +125,10 @@ where impl Context for SignerContext where - S: DbRead + DbWrite + Clone + Sync + Send, - BC: BitcoinInteract + Clone, - ST: StacksInteract + Clone + Sync + Send, - EM: EmilyInteract + Clone + Sync + Send, + S: DbRead + DbWrite + Clone + Sync + Send + 'static, + BC: BitcoinInteract + Clone + 'static, + ST: StacksInteract + Clone + Sync + Send + 'static, + EM: EmilyInteract + Clone + Sync + Send + 'static, { fn config(&self) -> &Settings { &self.config @@ -160,23 +160,23 @@ where TerminationHandle::new(self.term_tx.clone(), self.term_tx.subscribe()) } - fn get_storage(&self) -> impl DbRead + Clone + Sync + Send { + fn get_storage(&self) -> impl DbRead + Clone + Sync + Send + 'static { self.storage.clone() } - fn get_storage_mut(&self) -> impl DbRead + DbWrite + Clone + Sync + Send { + fn get_storage_mut(&self) -> impl DbRead + DbWrite + Clone + Sync + Send + 'static { self.storage.clone() } - fn get_bitcoin_client(&self) -> impl BitcoinInteract + Clone { + fn get_bitcoin_client(&self) -> impl BitcoinInteract + Clone + 'static { self.bitcoin_client.clone() } - fn get_stacks_client(&self) -> impl StacksInteract + Clone { + fn get_stacks_client(&self) -> impl StacksInteract + Clone + 'static { self.stacks_client.clone() } - fn get_emily_client(&self) -> impl EmilyInteract + Clone { + fn get_emily_client(&self) -> impl EmilyInteract + Clone + 'static { self.emily_client.clone() } } diff --git a/signer/src/testing/context.rs b/signer/src/testing/context.rs index 0101a17f..ff88ea59 100644 --- a/signer/src/testing/context.rs +++ b/signer/src/testing/context.rs @@ -162,10 +162,10 @@ impl impl Context for TestContext where - Storage: DbRead + DbWrite + Clone + Sync + Send, - Bitcoin: BitcoinInteract + Clone + Send + Sync, - Stacks: StacksInteract + Clone + Send + Sync, - Emily: EmilyInteract + Clone + Send + Sync, + Storage: DbRead + DbWrite + Clone + Sync + Send + 'static, + Bitcoin: BitcoinInteract + Clone + Send + Sync + 'static, + Stacks: StacksInteract + Clone + Send + Sync + 'static, + Emily: EmilyInteract + Clone + Send + Sync + 'static, { fn config(&self) -> &Settings { self.inner.config() @@ -189,25 +189,25 @@ where self.inner.get_termination_handle() } - fn get_storage(&self) -> impl crate::storage::DbRead + Clone + Sync + Send { + fn get_storage(&self) -> impl crate::storage::DbRead + Clone + Sync + Send + 'static { self.inner.get_storage() } fn get_storage_mut( &self, - ) -> impl crate::storage::DbRead + crate::storage::DbWrite + Clone + Sync + Send { + ) -> impl crate::storage::DbRead + crate::storage::DbWrite + Clone + Sync + Send + 'static { self.inner.get_storage_mut() } - fn get_bitcoin_client(&self) -> impl BitcoinInteract + Clone { + fn get_bitcoin_client(&self) -> impl BitcoinInteract + Clone + 'static { self.inner.get_bitcoin_client() } - fn get_stacks_client(&self) -> impl StacksInteract + Clone { + fn get_stacks_client(&self) -> impl StacksInteract + Clone + 'static { self.inner.get_stacks_client() } - fn get_emily_client(&self) -> impl EmilyInteract + Clone { + fn get_emily_client(&self) -> impl EmilyInteract + Clone + 'static { self.inner.get_emily_client() } } From 96a928ef2e453cbdfaa6557458536598564845c2 Mon Sep 17 00:00:00 2001 From: djordon Date: Fri, 4 Oct 2024 01:02:51 -0400 Subject: [PATCH 03/15] Sign the stacks transaction in the TxSignerEventLoop --- signer/src/transaction_signer.rs | 32 ++++++++++++++++++++++---------- 1 file changed, 22 insertions(+), 10 deletions(-) diff --git a/signer/src/transaction_signer.rs b/signer/src/transaction_signer.rs index 0df95e37..958ef8ab 100644 --- a/signer/src/transaction_signer.rs +++ b/signer/src/transaction_signer.rs @@ -22,6 +22,7 @@ use crate::keys::PublicKey; use crate::message; use crate::message::StacksTransactionSignRequest; use crate::network; +use crate::signature::SighashDigest as _; use crate::stacks::contracts::AsContractCall; use crate::stacks::contracts::ContractCall; use crate::stacks::contracts::ReqContext; @@ -267,12 +268,12 @@ where } ( - message::Payload::StacksTransactionSignRequest(_request), + message::Payload::StacksTransactionSignRequest(request), true, ChainTipStatus::Canonical, ) => { - - //TODO(255): Implement + self.handle_stacks_transaction_sign_request(request, &msg.bitcoin_chain_tip) + .await?; } ( @@ -413,19 +414,26 @@ where #[tracing::instrument(skip_all)] async fn handle_stacks_transaction_sign_request( &mut self, - ctx: &impl Context, request: &StacksTransactionSignRequest, bitcoin_chain_tip: &model::BitcoinBlockHash, ) -> Result<(), Error> { - self.assert_valid_stackstransaction_sign_request(ctx, request, bitcoin_chain_tip) + self.assert_valid_stacks_tx_sign_request(request, bitcoin_chain_tip) .await?; - let wallet = SignerWallet::load(ctx, bitcoin_chain_tip).await?; + // We need to set the nonce in order to get the exact transaction + // that we need to sign. + let wallet = SignerWallet::load(&self.context, bitcoin_chain_tip).await?; wallet.set_nonce(request.nonce); let multi_sig = MultisigTx::new_tx(&request.contract_call, &wallet, request.tx_fee); let txid = multi_sig.tx().txid(); + // TODO: Make this more robust. The signer that recieves this won't + // be able to use the signature if it's over the wrong digest, so + // maybe we should error here. + debug_assert_eq!(multi_sig.tx().digest(), request.digest); + debug_assert_eq!(txid, request.txid); + let signature = crate::signature::sign_stacks_tx(multi_sig.tx(), &self.signer_private_key); let msg = message::StacksTransactionSignature { txid, signature }; @@ -435,12 +443,14 @@ where Ok(()) } - async fn assert_valid_stackstransaction_sign_request( - &mut self, - ctx: &impl Context, - request: &message::StacksTransactionSignRequest, + async fn assert_valid_stacks_tx_sign_request( + &self, + request: &StacksTransactionSignRequest, chain_tip: &model::BitcoinBlockHash, ) -> Result<(), Error> { + if true { + return Ok(()); + } // TODO(255): Finish the implementation let req_ctx = ReqContext { chain_tip: BitcoinBlockRef { @@ -457,6 +467,8 @@ where // This is wrong deployer: StacksAddress::burn_address(false), }; + // TODO: Maybe check the transaction fee in the request? + let ctx = &self.context; match &request.contract_call { ContractCall::AcceptWithdrawalV1(contract) => contract.validate(ctx, &req_ctx).await, ContractCall::CompleteDepositV1(contract) => contract.validate(ctx, &req_ctx).await, From 260edf9300d3626c16cca3ca11acda91f9fe92d9 Mon Sep 17 00:00:00 2001 From: djordon Date: Fri, 4 Oct 2024 01:04:26 -0400 Subject: [PATCH 04/15] Finish skeleton of stacks transaction sign request --- signer/src/error.rs | 5 + signer/src/stacks/api.rs | 3 +- signer/src/transaction_coordinator.rs | 132 ++++++++++++++++++++++---- 3 files changed, 122 insertions(+), 18 deletions(-) diff --git a/signer/src/error.rs b/signer/src/error.rs index 6e4bd368..79ead7d7 100644 --- a/signer/src/error.rs +++ b/signer/src/error.rs @@ -259,6 +259,11 @@ pub enum Error { #[error("failed to read migration script: {0}")] ReadSqlMigration(Cow<'static, str>), + /// An error when we exceeded the timeout when trying to sign a stacks + /// transaction. + #[error("took too long to receive enough signatures for transaction: {0}")] + SignatureTimeout(blockstack_lib::burnchains::Txid), + /// An error when attempting to generically decode bytes using the /// trait implementation. #[error("got an error wen attempting to call StacksMessageCodec::consensus_deserialize {0}")] diff --git a/signer/src/stacks/api.rs b/signer/src/stacks/api.rs index 13fc8cbe..3de67db8 100644 --- a/signer/src/stacks/api.rs +++ b/signer/src/stacks/api.rs @@ -172,7 +172,8 @@ impl GetNakamotoStartHeight for RPCPoxInfoData { /// The official documentation specifies what to expect when there is a /// rejection, and that documentation can be found here: /// https://github.com/stacks-network/stacks-core/blob/2.5.0.0.5/docs/rpc-endpoints.md -#[derive(Debug, serde::Deserialize)] +#[derive(Debug, serde::Deserialize, strum::IntoStaticStr)] +#[strum(serialize_all = "SCREAMING_SNAKE_CASE")] #[cfg_attr(feature = "testing", derive(serde::Serialize))] pub enum RejectionReason { /// From MemPoolRejection::SerializationFailure diff --git a/signer/src/transaction_coordinator.rs b/signer/src/transaction_coordinator.rs index d113a56f..5acdbe2c 100644 --- a/signer/src/transaction_coordinator.rs +++ b/signer/src/transaction_coordinator.rs @@ -7,8 +7,8 @@ use std::collections::BTreeSet; -use futures::StreamExt; -use futures::TryStreamExt; +use bitcoin::OutPoint; +use blockstack_lib::chainstate::stacks::StacksTransaction; use sha2::Digest; use crate::bitcoin::utxo; @@ -19,11 +19,13 @@ use crate::error::Error; use crate::keys::PrivateKey; use crate::keys::PublicKey; use crate::message; +use crate::message::Payload; use crate::message::StacksTransactionSignRequest; use crate::network; use crate::signature::SighashDigest; use crate::stacks::api::FeePriority; use crate::stacks::api::StacksInteract; +use crate::stacks::api::SubmitTxResponse; use crate::stacks::contracts::CompleteDepositV1; use crate::stacks::contracts::ContractCall; use crate::stacks::wallet::MultisigTx; @@ -288,17 +290,42 @@ where let account = stacks.get_account(wallet.address()).await?; wallet.set_nonce(account.nonce); - // Generate - let _sign_requests = futures::stream::iter(deposit_requests) - .then(|req| self.construct_deposit_stacks_sign_request(req, bitcoin_aggregate_key, &wallet)) - .try_collect::>() - .await?; + for deposit_req in deposit_requests { + // If this fails, we do not need to decrement the nonce. + let (sign_request, multi_tx, outpoint) = self + .construct_stacks_sign_request(deposit_req, bitcoin_aggregate_key, &wallet) + .await?; + // If we fail to sign the transaction for some reason, we + // shouldn't bail, but log the error, decrement the nonce by + // one, and try the next trasnaction. + let tx = self + .sign_stacks_transaction(sign_request, multi_tx, chain_tip, &wallet) + .await?; + + match stacks.submit_tx(&tx).await { + Ok(SubmitTxResponse::Acceptance(txid)) => tracing::info!( + %txid, + deposit_txid = %outpoint.txid, + deposit_vout = %outpoint.vout, + "successfully submitted complete-deposit stacks transaction" + ), + Ok(SubmitTxResponse::Rejection(err)) => tracing::warn!( + txid = %tx.txid(), + deposit_txid = %outpoint.txid, + deposit_vout = %outpoint.vout, + error = %Into::<&'static str>::into(err.reason), + "complete-deposit transaction rejected from stacks node" + ), + Err(error) => tracing::error!( + txid = %tx.txid(), + deposit_txid = %outpoint.txid, + deposit_vout = %outpoint.vout, + %error, + "failed to submit the stacks transaction to the stacks-node" + ), + } + } - // TODO: - // 1. Broadcast the sign requests - // 2. Gather the signatures into the transaction. - // 3. Broadcast the transaction to the stacks network. Then go home - // and relax. Ok(()) } @@ -314,7 +341,7 @@ where req: model::SweptDepositRequest, bitcoin_aggregate_key: &PublicKey, wallet: &SignerWallet, - ) -> Result { + ) -> Result<(StacksTransactionSignRequest, MultisigTx, OutPoint), Error> { let tx_info = self .context .get_bitcoin_client() @@ -355,14 +382,85 @@ where let multi_tx = MultisigTx::new_tx(&contract_call, wallet, tx_fee); let tx = multi_tx.tx(); - Ok(StacksTransactionSignRequest { + let sign_request = StacksTransactionSignRequest { aggregate_key: *bitcoin_aggregate_key, contract_call, nonce: tx.get_origin_nonce(), tx_fee: tx.get_tx_fee(), digest: tx.digest(), txid: tx.txid(), - }) + }; + + Ok((sign_request, multi_tx, outpoint)) + } + + /// Attempt to sign the stacks transaction. + async fn sign_stacks_transaction( + &mut self, + req: StacksTransactionSignRequest, + mut multi_tx: MultisigTx, + chain_tip: &model::BitcoinBlockHash, + wallet: &SignerWallet, + ) -> Result { + // First we ask for the other signers to sign our transaction + let msg = req.clone(); + self.send_message(msg, chain_tip).await?; + // Second we sign it ourselves + // + // TODO: Note that this is all pretty "loose". We haven't yet + // confirmed whether we are actually a part of the multi-sig wallet + // that we loaded. Thus, this signature could be invalid. This will + // change if we make the `SignerWallet` include the private key and + // have it verify that it is part of the signer set. This would + // make everything much more solid. + let private_key = self.context.config().signer.private_key; + let signature = crate::signature::sign_stacks_tx(multi_tx.tx(), &private_key); + multi_tx.add_signature(signature)?; + + let txid = req.txid; + let mut count = 1; + + let future = async { + while count <= wallet.signatures_required() { + let msg = self.network.receive().await?; + if !msg.verify() { + // TODO: We should track these kinds of errors. If this + // happens then something really went wrong elsewhere. + tracing::error!( + %txid, + offending_public_key = %msg.signer_pub_key, + "could not varify the received message", + ); + continue; + } + + if &msg.bitcoin_chain_tip != chain_tip { + tracing::warn!(?msg, "concurrent signing round message observed"); + continue; + } + + let sig = match msg.inner.payload { + Payload::StacksTransactionSignature(sig) if sig.txid == txid => sig, + _ => continue, + }; + + match multi_tx.add_signature(sig.signature) { + Ok(_) => count += 1, + Err(error) => tracing::warn!( + %txid, + %error, + offending_public_key = %msg.signer_pub_key, + "got an invalid signature" + ), + } + } + + Ok::<_, Error>(multi_tx.finalize_transaction()) + }; + + tokio::time::timeout(self.signing_round_max_duration, future) + .await + .map_err(|_| Error::SignatureTimeout(req.txid))? } /// Coordinate a signing round for the given request @@ -493,7 +591,7 @@ where continue; } - let message::Payload::WstsMessage(wsts_msg) = msg.inner.payload else { + let Payload::WstsMessage(wsts_msg) = msg.inner.payload else { continue; }; @@ -673,7 +771,7 @@ where #[tracing::instrument(skip(self, msg))] async fn send_message( &mut self, - msg: impl Into, + msg: impl Into, bitcoin_chain_tip: &model::BitcoinBlockHash, ) -> Result<(), Error> { let msg = msg From 9cf525b1de7a4b12bc723770b70b0714cc4e69a9 Mon Sep 17 00:00:00 2001 From: djordon Date: Fri, 4 Oct 2024 02:17:46 -0400 Subject: [PATCH 05/15] Simplify --- signer/src/error.rs | 4 ++ signer/src/stacks/api.rs | 11 +++- signer/src/storage/model.rs | 6 ++ signer/src/transaction_coordinator.rs | 91 ++++++++++++++++----------- 4 files changed, 74 insertions(+), 38 deletions(-) diff --git a/signer/src/error.rs b/signer/src/error.rs index 79ead7d7..9f6831f8 100644 --- a/signer/src/error.rs +++ b/signer/src/error.rs @@ -286,6 +286,10 @@ pub enum Error { #[error("failed to make a request to the stacks Node: {0}")] StacksNodeRequest(#[source] reqwest::Error), + /// We failed to submit the transaction to the mempool. + #[error("{0}")] + StacksTxRejection(#[from] crate::stacks::api::TxRejection), + /// Reqwest error #[error("response from stacks node did not conform to the expected schema: {0}")] UnexpectedStacksResponse(#[source] reqwest::Error), diff --git a/signer/src/stacks/api.rs b/signer/src/stacks/api.rs index 3de67db8..4473fc88 100644 --- a/signer/src/stacks/api.rs +++ b/signer/src/stacks/api.rs @@ -172,7 +172,7 @@ impl GetNakamotoStartHeight for RPCPoxInfoData { /// The official documentation specifies what to expect when there is a /// rejection, and that documentation can be found here: /// https://github.com/stacks-network/stacks-core/blob/2.5.0.0.5/docs/rpc-endpoints.md -#[derive(Debug, serde::Deserialize, strum::IntoStaticStr)] +#[derive(Debug, Clone, Copy, serde::Deserialize, strum::IntoStaticStr)] #[strum(serialize_all = "SCREAMING_SNAKE_CASE")] #[cfg_attr(feature = "testing", derive(serde::Serialize))] pub enum RejectionReason { @@ -247,6 +247,15 @@ pub struct TxRejection { pub txid: Txid, } +impl std::fmt::Display for TxRejection { + fn fmt(&self, f: &mut std::fmt::Formatter<'_>) -> std::fmt::Result { + let reason_str: &'static str = self.reason.into(); + write!(f, "transaction rejected from stacks mempool: {reason_str}") + } +} + +impl std::error::Error for TxRejection {} + /// The response from a POST /v2/transactions request /// /// The stacks node returns three types of responses, either: diff --git a/signer/src/storage/model.rs b/signer/src/storage/model.rs index ff06f2e9..52928d40 100644 --- a/signer/src/storage/model.rs +++ b/signer/src/storage/model.rs @@ -608,6 +608,12 @@ impl From<[u8; 32]> for StacksBlockHash { #[serde(transparent)] pub struct StacksTxId(blockstack_lib::burnchains::Txid); +impl std::fmt::Display for StacksTxId { + fn fmt(&self, f: &mut std::fmt::Formatter<'_>) -> std::fmt::Result { + write!(f, "{}", self.0) + } +} + impl Deref for StacksTxId { type Target = blockstack_lib::burnchains::Txid; fn deref(&self) -> &Self::Target { diff --git a/signer/src/transaction_coordinator.rs b/signer/src/transaction_coordinator.rs index 5acdbe2c..55ace6ca 100644 --- a/signer/src/transaction_coordinator.rs +++ b/signer/src/transaction_coordinator.rs @@ -7,7 +7,6 @@ use std::collections::BTreeSet; -use bitcoin::OutPoint; use blockstack_lib::chainstate::stacks::StacksTransaction; use sha2::Digest; @@ -31,6 +30,7 @@ use crate::stacks::contracts::ContractCall; use crate::stacks::wallet::MultisigTx; use crate::stacks::wallet::SignerWallet; use crate::storage::model; +use crate::storage::model::StacksTxId; use crate::storage::DbRead as _; use crate::wsts_state_machine; @@ -260,7 +260,6 @@ where bitcoin_aggregate_key: &PublicKey, ) -> Result<(), Error> { let wallet = SignerWallet::load(&self.context, chain_tip).await?; - let db = self.context.get_storage(); let stacks = self.context.get_stacks_client(); // Fetch deposit and withdrawal requests from the database where @@ -274,7 +273,9 @@ where // For withdrawals, we need to have a record of the `request_id` // associated with the bitcoin transaction's outputs. - let deposit_requests = db + let deposit_requests = self + .context + .get_storage() .get_swept_deposit_requests(chain_tip, self.context_window) .await?; @@ -290,45 +291,61 @@ where let account = stacks.get_account(wallet.address()).await?; wallet.set_nonce(account.nonce); - for deposit_req in deposit_requests { - // If this fails, we do not need to decrement the nonce. - let (sign_request, multi_tx, outpoint) = self - .construct_stacks_sign_request(deposit_req, bitcoin_aggregate_key, &wallet) - .await?; - // If we fail to sign the transaction for some reason, we - // shouldn't bail, but log the error, decrement the nonce by - // one, and try the next trasnaction. - let tx = self - .sign_stacks_transaction(sign_request, multi_tx, chain_tip, &wallet) - .await?; + for req in deposit_requests { + let sign_request_fut = + self.construct_stacks_sign_request(req, bitcoin_aggregate_key, &wallet); + + let (sign_request, multi_tx) = match sign_request_fut.await { + Ok(res) => res, + Err(error) => { + tracing::error!(%error, "could not construct a transaction completing the deposit request"); + continue; + } + }; - match stacks.submit_tx(&tx).await { - Ok(SubmitTxResponse::Acceptance(txid)) => tracing::info!( - %txid, - deposit_txid = %outpoint.txid, - deposit_vout = %outpoint.vout, - "successfully submitted complete-deposit stacks transaction" - ), - Ok(SubmitTxResponse::Rejection(err)) => tracing::warn!( - txid = %tx.txid(), - deposit_txid = %outpoint.txid, - deposit_vout = %outpoint.vout, - error = %Into::<&'static str>::into(err.reason), - "complete-deposit transaction rejected from stacks node" - ), - Err(error) => tracing::error!( - txid = %tx.txid(), - deposit_txid = %outpoint.txid, - deposit_vout = %outpoint.vout, - %error, - "failed to submit the stacks transaction to the stacks-node" - ), + // If we fail here, then we need to decrement the nonce. + let process_request_fut = + self.process_sign_request(sign_request, chain_tip, multi_tx, &wallet); + + match process_request_fut.await { + Ok(txid) => { + tracing::info!(%txid, "successfully submitted complete-deposit transaction") + } + Err(error) => { + tracing::warn!(%error, "could process the sign request for deposit request"); + wallet.set_nonce(wallet.get_nonce().saturating_sub(1)); + } } } Ok(()) } + /// Sign and broadcast the stacks transaction + /// + /// If we fail to sign the transaction for some reason, we decrement + /// the nonce by one, and try the next transaction. This is not a fatal + /// error, since we could fail to sign the transaction because someone + /// else is now the coordinator, and all of the signers are now + /// ignoring us. + async fn process_sign_request( + &mut self, + sign_request: StacksTransactionSignRequest, + chain_tip: &model::BitcoinBlockHash, + multi_tx: MultisigTx, + wallet: &SignerWallet, + ) -> Result { + let tx = self + .sign_stacks_transaction(sign_request, multi_tx, chain_tip, wallet) + .await?; + + match self.context.get_stacks_client().submit_tx(&tx).await { + Ok(SubmitTxResponse::Acceptance(txid)) => Ok(txid.into()), + Ok(SubmitTxResponse::Rejection(err)) => Err(err.into()), + Err(err) => Err(err), + } + } + /// Transform the swept deposit request into a Stacks sign request /// object. /// @@ -341,7 +358,7 @@ where req: model::SweptDepositRequest, bitcoin_aggregate_key: &PublicKey, wallet: &SignerWallet, - ) -> Result<(StacksTransactionSignRequest, MultisigTx, OutPoint), Error> { + ) -> Result<(StacksTransactionSignRequest, MultisigTx), Error> { let tx_info = self .context .get_bitcoin_client() @@ -391,7 +408,7 @@ where txid: tx.txid(), }; - Ok((sign_request, multi_tx, outpoint)) + Ok((sign_request, multi_tx)) } /// Attempt to sign the stacks transaction. From e3df04574b551c71e1e70a1d91cee3b42001a9fe Mon Sep 17 00:00:00 2001 From: djordon Date: Fri, 4 Oct 2024 02:35:37 -0400 Subject: [PATCH 06/15] minor clean up --- signer/src/transaction_coordinator.rs | 20 ++++++-------------- 1 file changed, 6 insertions(+), 14 deletions(-) diff --git a/signer/src/transaction_coordinator.rs b/signer/src/transaction_coordinator.rs index 55ace6ca..b9dcfed9 100644 --- a/signer/src/transaction_coordinator.rs +++ b/signer/src/transaction_coordinator.rs @@ -420,8 +420,8 @@ where wallet: &SignerWallet, ) -> Result { // First we ask for the other signers to sign our transaction - let msg = req.clone(); - self.send_message(msg, chain_tip).await?; + let txid = req.txid; + self.send_message(req, chain_tip).await?; // Second we sign it ourselves // // TODO: Note that this is all pretty "loose". We haven't yet @@ -434,22 +434,14 @@ where let signature = crate::signature::sign_stacks_tx(multi_tx.tx(), &private_key); multi_tx.add_signature(signature)?; - let txid = req.txid; + let mut count = 1; let future = async { while count <= wallet.signatures_required() { let msg = self.network.receive().await?; - if !msg.verify() { - // TODO: We should track these kinds of errors. If this - // happens then something really went wrong elsewhere. - tracing::error!( - %txid, - offending_public_key = %msg.signer_pub_key, - "could not varify the received message", - ); - continue; - } + // TODO: We need to verify these messages, but it is best + // to do that at the source when we receive the message. if &msg.bitcoin_chain_tip != chain_tip { tracing::warn!(?msg, "concurrent signing round message observed"); @@ -477,7 +469,7 @@ where tokio::time::timeout(self.signing_round_max_duration, future) .await - .map_err(|_| Error::SignatureTimeout(req.txid))? + .map_err(|_| Error::SignatureTimeout(txid))? } /// Coordinate a signing round for the given request From d252cf291bae5d62e420df7c8f344b4d3e7cafab Mon Sep 17 00:00:00 2001 From: djordon Date: Mon, 7 Oct 2024 15:21:53 -0400 Subject: [PATCH 07/15] Cargo fmt --- signer/src/transaction_coordinator.rs | 1 - 1 file changed, 1 deletion(-) diff --git a/signer/src/transaction_coordinator.rs b/signer/src/transaction_coordinator.rs index b9dcfed9..dc206a6b 100644 --- a/signer/src/transaction_coordinator.rs +++ b/signer/src/transaction_coordinator.rs @@ -434,7 +434,6 @@ where let signature = crate::signature::sign_stacks_tx(multi_tx.tx(), &private_key); multi_tx.add_signature(signature)?; - let mut count = 1; let future = async { From 6da81edb8b79c9cb35c9499e80e96b3c239bde1c Mon Sep 17 00:00:00 2001 From: djordon Date: Mon, 7 Oct 2024 16:14:10 -0400 Subject: [PATCH 08/15] Don't send the transaction for others to sign if you are the only signer --- signer/src/transaction_coordinator.rs | 6 ++++-- 1 file changed, 4 insertions(+), 2 deletions(-) diff --git a/signer/src/transaction_coordinator.rs b/signer/src/transaction_coordinator.rs index dc206a6b..4d965454 100644 --- a/signer/src/transaction_coordinator.rs +++ b/signer/src/transaction_coordinator.rs @@ -421,7 +421,9 @@ where ) -> Result { // First we ask for the other signers to sign our transaction let txid = req.txid; - self.send_message(req, chain_tip).await?; + if wallet.signatures_required() > 1 { + self.send_message(req, chain_tip).await?; + } // Second we sign it ourselves // // TODO: Note that this is all pretty "loose". We haven't yet @@ -437,7 +439,7 @@ where let mut count = 1; let future = async { - while count <= wallet.signatures_required() { + while count < wallet.signatures_required() { let msg = self.network.receive().await?; // TODO: We need to verify these messages, but it is best // to do that at the source when we receive the message. From dc67b02ac9e4152cd634ad2666adf4d55b4dc98b Mon Sep 17 00:00:00 2001 From: djordon Date: Mon, 7 Oct 2024 16:14:50 -0400 Subject: [PATCH 09/15] minor reordering --- signer/src/testing/transaction_coordinator.rs | 6 +++--- 1 file changed, 3 insertions(+), 3 deletions(-) diff --git a/signer/src/testing/transaction_coordinator.rs b/signer/src/testing/transaction_coordinator.rs index c7a41a29..450ece69 100644 --- a/signer/src/testing/transaction_coordinator.rs +++ b/signer/src/testing/transaction_coordinator.rs @@ -554,17 +554,17 @@ where let (aggregate_key, all_dkg_shares) = signer_set.run_dkg(bitcoin_chain_tip, dkg_txid, rng).await; + let encrypted_dkg_shares = all_dkg_shares.first().unwrap(); + signer_set .write_as_rotate_keys_tx( &self.context.get_storage_mut(), &bitcoin_chain_tip, - all_dkg_shares.first().unwrap(), + encrypted_dkg_shares, rng, ) .await; - let encrypted_dkg_shares = all_dkg_shares.first().unwrap(); - storage .write_encrypted_dkg_shares(encrypted_dkg_shares) .await From ecef0cf994fac407d01faa34bbdfa675a42e9b0d Mon Sep 17 00:00:00 2001 From: djordon Date: Mon, 7 Oct 2024 16:20:06 -0400 Subject: [PATCH 10/15] Rename event loop harnesses --- signer/src/testing/transaction_coordinator.rs | 6 +++--- signer/src/testing/transaction_signer.rs | 16 ++++++++-------- 2 files changed, 11 insertions(+), 11 deletions(-) diff --git a/signer/src/testing/transaction_coordinator.rs b/signer/src/testing/transaction_coordinator.rs index 450ece69..163c68ca 100644 --- a/signer/src/testing/transaction_coordinator.rs +++ b/signer/src/testing/transaction_coordinator.rs @@ -39,13 +39,13 @@ const EMPTY_BITCOIN_TX: bitcoin::Transaction = bitcoin::Transaction { output: vec![], }; -struct EventLoopHarness { +struct TxCoordinatorEventLoopHarness { event_loop: EventLoop, context: C, is_started: Arc, } -impl EventLoopHarness +impl TxCoordinatorEventLoopHarness where C: Context + 'static, { @@ -205,7 +205,7 @@ where let private_key = Self::select_coordinator(&bitcoin_chain_tip.block_hash, &signer_info); // Bootstrap the tx coordinator within an event loop harness. - let event_loop_harness = EventLoopHarness::create( + let event_loop_harness = TxCoordinatorEventLoopHarness::create( self.context.clone(), network.connect(), self.context_window, diff --git a/signer/src/testing/transaction_signer.rs b/signer/src/testing/transaction_signer.rs index 79139585..2da02b03 100644 --- a/signer/src/testing/transaction_signer.rs +++ b/signer/src/testing/transaction_signer.rs @@ -36,12 +36,12 @@ use tokio::time::error::Elapsed; use super::context::*; -struct EventLoopHarness { +struct TxSignerEventLoopHarness { context: Context, event_loop: EventLoop, } -impl EventLoopHarness +impl TxSignerEventLoopHarness where Ctx: Context + 'static, Rng: rand::RngCore + rand::CryptoRng + Send + Sync + 'static, @@ -161,7 +161,7 @@ where let mut network_rx = network.connect(); let mut signal_rx = self.context.get_signal_receiver(); - let event_loop_harness = EventLoopHarness::create( + let event_loop_harness = TxSignerEventLoopHarness::create( self.context.clone(), network.connect(), self.context_window, @@ -229,7 +229,7 @@ where let mut network_rx = network.connect(); let mut signal_rx = self.context.get_signal_receiver(); - let event_loop_harness = EventLoopHarness::create( + let event_loop_harness = TxSignerEventLoopHarness::create( self.context.clone(), network.connect(), self.context_window, @@ -316,7 +316,7 @@ where let mut event_loop_handles: Vec<_> = signer_info .into_iter() .map(|signer_info| { - let event_loop_harness = EventLoopHarness::create( + let event_loop_harness = TxSignerEventLoopHarness::create( build_context(), network.connect(), self.context_window, @@ -388,7 +388,7 @@ where let signer_info = testing::wsts::generate_signer_info(&mut rng, self.num_signers); let coordinator_signer_info = &signer_info.first().cloned().unwrap(); - let event_loop_harness = EventLoopHarness::create( + let event_loop_harness = TxSignerEventLoopHarness::create( self.context.clone(), network.connect(), self.context_window, @@ -503,7 +503,7 @@ where .clone() .into_iter() .map(|signer_info| { - let event_loop_harness = EventLoopHarness::create( + let event_loop_harness = TxSignerEventLoopHarness::create( build_context(), // NEED TO HAVE A NEW CONTEXT FOR EACH SIGNER network.connect(), self.context_window, @@ -584,7 +584,7 @@ where .clone() .into_iter() .map(|signer_info| { - let event_loop_harness = EventLoopHarness::create( + let event_loop_harness = TxSignerEventLoopHarness::create( build_context(), network.connect(), self.context_window, From 0f3129e8f9e915afefa9f1db0c2b0ae9d94b1e53 Mon Sep 17 00:00:00 2001 From: djordon Date: Mon, 7 Oct 2024 16:20:25 -0400 Subject: [PATCH 11/15] Start integration test [ci skip] --- signer/tests/integration/full_deposit_flow.rs | 59 +++++++++++++++++++ signer/tests/integration/main.rs | 1 + 2 files changed, 60 insertions(+) create mode 100644 signer/tests/integration/full_deposit_flow.rs diff --git a/signer/tests/integration/full_deposit_flow.rs b/signer/tests/integration/full_deposit_flow.rs new file mode 100644 index 00000000..318fb9b7 --- /dev/null +++ b/signer/tests/integration/full_deposit_flow.rs @@ -0,0 +1,59 @@ +use std::sync::atomic::Ordering; + +use blockstack_lib::types::chainstate::StacksAddress; +use rand::rngs::OsRng; +use rand::SeedableRng; + +use sbtc::testing::regtest; +use signer::testing; +use signer::testing::context::*; + +use fake::Fake; + +use crate::setup::backfill_bitcoin_blocks; +use crate::setup::TestSweepSetup; +use crate::DATABASE_NUM; + + +#[cfg_attr(not(feature = "integration-tests"), ignore)] +#[tokio::test] +async fn complete_deposit_validation_happy_path() { + // Normal: this generates the blockchain as well as deposit request + // transactions and a transaction sweeping in the deposited funds. + // This is just setup and should be essentially the same between tests. + let db_num = DATABASE_NUM.fetch_add(1, Ordering::SeqCst); + let db = testing::storage::new_test_database(db_num, true).await; + let mut rng = rand::rngs::StdRng::seed_from_u64(51); + + let (rpc, faucet) = regtest::initialize_blockchain(); + let setup = TestSweepSetup::new_setup(&rpc, &faucet, 1_000_000, &mut rng); + + // Normal: the signer follows the bitcoin blockchain and event observer + // should be getting new block events from bitcoin-core. We haven't + // hooked up our block observer, so we need to manually update the + // database with new bitcoin block headers. + backfill_bitcoin_blocks(&db, rpc, &setup.sweep_block_hash).await; + + // Normal: we take the deposit transaction as is from the test setup + // and store it in the database. This is necessary for when we fetch + // outstanding unfulfilled deposit requests. + setup.store_deposit_tx(&db).await; + + setup.store_dkg_shares(&db).await; + + + + // Create a context object for reaching out to the database and bitcoin + // core. This will create a bitcoin core client that connects to the + // bitcoin-core at the [bitcoin].endpoints[0] endpoint from the default + // toml config file. + let ctx = TestContext::builder() + .with_storage(db.clone()) + .with_first_bitcoin_core_client() + .with_mocked_stacks_client() + .with_mocked_emily_client() + .build(); + + testing::storage::drop_db(db).await; +} + diff --git a/signer/tests/integration/main.rs b/signer/tests/integration/main.rs index ffc705f4..284224ec 100644 --- a/signer/tests/integration/main.rs +++ b/signer/tests/integration/main.rs @@ -4,6 +4,7 @@ mod bitcoin_client; mod bitcoin_rpc; mod complete_deposit; mod contracts; +mod full_deposit_flow; mod postgres; mod rbf; mod setup; From 773a9dd7378d5bce9457a25f0e4723de92e96103 Mon Sep 17 00:00:00 2001 From: djordon Date: Wed, 9 Oct 2024 09:57:32 -0400 Subject: [PATCH 12/15] Updates after rebase --- signer/src/transaction_coordinator.rs | 2 +- 1 file changed, 1 insertion(+), 1 deletion(-) diff --git a/signer/src/transaction_coordinator.rs b/signer/src/transaction_coordinator.rs index 4d965454..55234602 100644 --- a/signer/src/transaction_coordinator.rs +++ b/signer/src/transaction_coordinator.rs @@ -293,7 +293,7 @@ where for req in deposit_requests { let sign_request_fut = - self.construct_stacks_sign_request(req, bitcoin_aggregate_key, &wallet); + self.construct_deposit_stacks_sign_request(req, bitcoin_aggregate_key, &wallet); let (sign_request, multi_tx) = match sign_request_fut.await { Ok(res) => res, From ca58a9967c34acd17475ff5353f11d57aeaa068d Mon Sep 17 00:00:00 2001 From: djordon Date: Wed, 9 Oct 2024 09:57:48 -0400 Subject: [PATCH 13/15] cargo fmt --- emily/handler/src/bin/emily-server.rs | 7 ++++++- emily/handler/src/context.rs | 4 +--- 2 files changed, 7 insertions(+), 4 deletions(-) diff --git a/emily/handler/src/bin/emily-server.rs b/emily/handler/src/bin/emily-server.rs index 4cce93b0..7ceae33a 100644 --- a/emily/handler/src/bin/emily-server.rs +++ b/emily/handler/src/bin/emily-server.rs @@ -57,7 +57,12 @@ async fn main() { // Get command line arguments. let Cli { server: ServerArgs { host, port }, - general: GeneralArgs { pretty_logs, log_directives, dynamodb_endpoint }, + general: + GeneralArgs { + pretty_logs, + log_directives, + dynamodb_endpoint, + }, } = Cli::parse(); // Setup logging. diff --git a/emily/handler/src/context.rs b/emily/handler/src/context.rs index de724ba3..8ebcb26a 100644 --- a/emily/handler/src/context.rs +++ b/emily/handler/src/context.rs @@ -88,9 +88,7 @@ impl EmilyContext { } /// Create a local testing instance. #[cfg(feature = "testing")] - pub async fn local_instance( - dynamodb_endpoint: &str, - ) -> Result { + pub async fn local_instance(dynamodb_endpoint: &str) -> Result { use std::collections::HashMap; // Get config that always points to the dynamodb table directly From c07d85c064988aefd8ff91a9f6f93edfd063fe03 Mon Sep 17 00:00:00 2001 From: djordon Date: Wed, 9 Oct 2024 09:58:28 -0400 Subject: [PATCH 14/15] remove the beginning of end-to-end integration test --- signer/tests/integration/full_deposit_flow.rs | 59 ------------------- signer/tests/integration/main.rs | 1 - 2 files changed, 60 deletions(-) delete mode 100644 signer/tests/integration/full_deposit_flow.rs diff --git a/signer/tests/integration/full_deposit_flow.rs b/signer/tests/integration/full_deposit_flow.rs deleted file mode 100644 index 318fb9b7..00000000 --- a/signer/tests/integration/full_deposit_flow.rs +++ /dev/null @@ -1,59 +0,0 @@ -use std::sync::atomic::Ordering; - -use blockstack_lib::types::chainstate::StacksAddress; -use rand::rngs::OsRng; -use rand::SeedableRng; - -use sbtc::testing::regtest; -use signer::testing; -use signer::testing::context::*; - -use fake::Fake; - -use crate::setup::backfill_bitcoin_blocks; -use crate::setup::TestSweepSetup; -use crate::DATABASE_NUM; - - -#[cfg_attr(not(feature = "integration-tests"), ignore)] -#[tokio::test] -async fn complete_deposit_validation_happy_path() { - // Normal: this generates the blockchain as well as deposit request - // transactions and a transaction sweeping in the deposited funds. - // This is just setup and should be essentially the same between tests. - let db_num = DATABASE_NUM.fetch_add(1, Ordering::SeqCst); - let db = testing::storage::new_test_database(db_num, true).await; - let mut rng = rand::rngs::StdRng::seed_from_u64(51); - - let (rpc, faucet) = regtest::initialize_blockchain(); - let setup = TestSweepSetup::new_setup(&rpc, &faucet, 1_000_000, &mut rng); - - // Normal: the signer follows the bitcoin blockchain and event observer - // should be getting new block events from bitcoin-core. We haven't - // hooked up our block observer, so we need to manually update the - // database with new bitcoin block headers. - backfill_bitcoin_blocks(&db, rpc, &setup.sweep_block_hash).await; - - // Normal: we take the deposit transaction as is from the test setup - // and store it in the database. This is necessary for when we fetch - // outstanding unfulfilled deposit requests. - setup.store_deposit_tx(&db).await; - - setup.store_dkg_shares(&db).await; - - - - // Create a context object for reaching out to the database and bitcoin - // core. This will create a bitcoin core client that connects to the - // bitcoin-core at the [bitcoin].endpoints[0] endpoint from the default - // toml config file. - let ctx = TestContext::builder() - .with_storage(db.clone()) - .with_first_bitcoin_core_client() - .with_mocked_stacks_client() - .with_mocked_emily_client() - .build(); - - testing::storage::drop_db(db).await; -} - diff --git a/signer/tests/integration/main.rs b/signer/tests/integration/main.rs index 284224ec..ffc705f4 100644 --- a/signer/tests/integration/main.rs +++ b/signer/tests/integration/main.rs @@ -4,7 +4,6 @@ mod bitcoin_client; mod bitcoin_rpc; mod complete_deposit; mod contracts; -mod full_deposit_flow; mod postgres; mod rbf; mod setup; From 7cdea8d577b0e2a7d7ed9335e26d38e87252a0e9 Mon Sep 17 00:00:00 2001 From: djordon Date: Wed, 9 Oct 2024 10:18:00 -0400 Subject: [PATCH 15/15] Move some comments around, add better logging --- signer/src/storage/model.rs | 10 ++++++++++ signer/src/transaction_coordinator.rs | 25 +++++++++++++------------ 2 files changed, 23 insertions(+), 12 deletions(-) diff --git a/signer/src/storage/model.rs b/signer/src/storage/model.rs index 52928d40..a70ea724 100644 --- a/signer/src/storage/model.rs +++ b/signer/src/storage/model.rs @@ -269,6 +269,16 @@ pub struct SweptDepositRequest { pub amount: u64, } +impl SweptDepositRequest { + /// The OutPoint of the actual deposit + pub fn deposit_outpoint(&self) -> bitcoin::OutPoint { + bitcoin::OutPoint { + txid: self.txid.into(), + vout: self.output_index, + } + } +} + /// Withdraw request. #[derive(Debug, Clone, Hash, PartialEq, Eq, PartialOrd, Ord, sqlx::FromRow)] #[cfg_attr(feature = "testing", derive(fake::Dummy))] diff --git a/signer/src/transaction_coordinator.rs b/signer/src/transaction_coordinator.rs index 55234602..fa9de07a 100644 --- a/signer/src/transaction_coordinator.rs +++ b/signer/src/transaction_coordinator.rs @@ -292,6 +292,7 @@ where wallet.set_nonce(account.nonce); for req in deposit_requests { + let outpoint = req.deposit_outpoint(); let sign_request_fut = self.construct_deposit_stacks_sign_request(req, bitcoin_aggregate_key, &wallet); @@ -303,7 +304,11 @@ where } }; - // If we fail here, then we need to decrement the nonce. + // If we fail to sign the transaction for some reason, we + // decrement the nonce by one, and try the next transaction. + // This is not a fatal error, since we could fail to sign the + // transaction because someone else is now the coordinator, and + // all of the signers are now ignoring us. let process_request_fut = self.process_sign_request(sign_request, chain_tip, multi_tx, &wallet); @@ -312,7 +317,12 @@ where tracing::info!(%txid, "successfully submitted complete-deposit transaction") } Err(error) => { - tracing::warn!(%error, "could process the sign request for deposit request"); + tracing::warn!( + %error, + txid = %outpoint.txid, + vout = %outpoint.vout, + "could not process the stacks sign request for a deposit" + ); wallet.set_nonce(wallet.get_nonce().saturating_sub(1)); } } @@ -322,12 +332,6 @@ where } /// Sign and broadcast the stacks transaction - /// - /// If we fail to sign the transaction for some reason, we decrement - /// the nonce by one, and try the next transaction. This is not a fatal - /// error, since we could fail to sign the transaction because someone - /// else is now the coordinator, and all of the signers are now - /// ignoring us. async fn process_sign_request( &mut self, sign_request: StacksTransactionSignRequest, @@ -368,10 +372,7 @@ where Error::BitcoinTxMissing(req.sweep_txid.into(), Some(req.sweep_block_hash.into())) })?; - let outpoint = bitcoin::OutPoint { - txid: req.txid.into(), - vout: req.output_index, - }; + let outpoint = req.deposit_outpoint(); let assessed_bitcoin_fee = tx_info .assess_input_fee(&outpoint) .ok_or_else(|| Error::OutPointMissing(outpoint))?;