From d80f0972f0e1dbfa2d7debc1f57231b496c655cd Mon Sep 17 00:00:00 2001 From: Viktar Makouski Date: Thu, 16 Jan 2025 17:58:12 +0300 Subject: [PATCH] [feature] return a proper errors instead of debug asserts (#1165) * Enriched error message in handle_stacks_transaction_sign_request * fmt * add TweakCheck error * Enriched scripts checks in CreateDepositRequest::validate_tx * get_tenure_raw error message upgrade * remove unnecessary debug assert * review fixes * Prettify comment Co-authored-by: Daniel Jordon --------- Co-authored-by: Daniel Jordon --- sbtc/src/deposits.rs | 8 ++++++-- signer/src/error.rs | 19 +++++++++++++++++++ signer/src/keys.rs | 4 +++- signer/src/signature.rs | 4 ++-- signer/src/stacks/api.rs | 10 +++++++++- signer/src/transaction_signer.rs | 4 +++- 6 files changed, 42 insertions(+), 7 deletions(-) diff --git a/sbtc/src/deposits.rs b/sbtc/src/deposits.rs index 4f48465e1..aceff14d7 100644 --- a/sbtc/src/deposits.rs +++ b/sbtc/src/deposits.rs @@ -147,8 +147,12 @@ impl CreateDepositRequest { let deposit_script = deposit.deposit_script(); let reclaim_script = reclaim.reclaim_script(); - debug_assert_eq!(deposit_script, self.deposit_script); - debug_assert_eq!(reclaim_script, self.reclaim_script); + if deposit_script != self.deposit_script { + return Err(Error::InvalidDepositScript); + } + if reclaim_script != self.reclaim_script { + return Err(Error::InvalidReclaimScript); + } let expected_script_pubkey = to_script_pubkey(deposit_script.clone(), reclaim_script.clone()); diff --git a/signer/src/error.rs b/signer/src/error.rs index f181774ce..1ede66bc9 100644 --- a/signer/src/error.rs +++ b/signer/src/error.rs @@ -124,6 +124,12 @@ pub enum Error { #[error("observed a tenure identified by a StacksBlockId with with no blocks")] EmptyStacksTenure, + /// This happens when StacksClient::get_tenure_raw returns an array of blocks which starts + /// with a block with id {0}, while we expect it to return an array of blocks starting with + /// a block with id {1} + #[error("get_tenure_raw returned unexpected response: {0}. Expected: {1}")] + GetTenureRawMismatch(StacksBlockId, StacksBlockId), + /// Received an error in call to estimatesmartfee RPC call #[error("failed to get fee estimate from bitcoin-core for target {1}. {0}")] EstimateSmartFee(#[source] bitcoincore_rpc::Error, u16), @@ -247,6 +253,10 @@ pub enum Error { #[error("invalid tweak? seriously? {0}")] InvalidPublicKeyTweak(#[source] secp256k1::Error), + /// This happens when a tweak produced by [`XOnlyPublicKey::add_tweak`] was computed incorrectly. + #[error("Tweak was computed incorrectly.")] + InvalidPublicKeyTweakCheck, + /// This occurs when converting a byte slice to our internal public key /// type, which is a thin wrapper around the secp256k1::SecretKey. #[error("invalid private key: {0}")] @@ -465,6 +475,15 @@ pub enum Error { #[error("invalid configuration")] InvalidConfiguration, + /// We throw this when signer produced txid and coordinator produced txid differ. + #[error( + "Signer and coordinator txid mismatch. Signer produced txid {0}, but coordinator send txid {1}" + )] + SignerCoordinatorTxidMismatch( + blockstack_lib::burnchains::Txid, + blockstack_lib::burnchains::Txid, + ), + /// Observer dropped #[error("observer dropped")] ObserverDropped, diff --git a/signer/src/keys.rs b/signer/src/keys.rs index 744124363..a750d3f56 100644 --- a/signer/src/keys.rs +++ b/signer/src/keys.rs @@ -453,7 +453,9 @@ impl SignerScriptPubKey for secp256k1::XOnlyPublicKey { .add_tweak(SECP256K1, &tweak) .map_err(Error::InvalidPublicKeyTweak)?; - debug_assert!(self.tweak_add_check(SECP256K1, &output_key, parity, tweak)); + if !self.tweak_add_check(SECP256K1, &output_key, parity, tweak) { + return Err(Error::InvalidPublicKeyTweakCheck); + } let pk = secp256k1::PublicKey::from_x_only_public_key(output_key, parity); Ok(PublicKey(pk)) } diff --git a/signer/src/signature.rs b/signer/src/signature.rs index b21db8a4f..28fef4acb 100644 --- a/signer/src/signature.rs +++ b/signer/src/signature.rs @@ -127,9 +127,9 @@ impl RecoverableEcdsaSignature for RecoverableSignature { fn to_byte_array(&self) -> [u8; 65] { let (recovery_id, bytes) = self.serialize_compact(); let mut ret_bytes = [0u8; 65]; - // The recovery ID will be 0, 1, 2, or 3 + // The recovery ID will be 0, 1, 2, or 3 as described in the secp256k1 docs: + // https://docs.rs/secp256k1/0.30.0/secp256k1/ecdsa/enum.RecoveryId.html ret_bytes[0] = recovery_id.to_i32() as u8; - debug_assert!(recovery_id.to_i32() < 4); ret_bytes[1..].copy_from_slice(&bytes[..]); ret_bytes diff --git a/signer/src/stacks/api.rs b/signer/src/stacks/api.rs index 203ff6917..f7dd53ec7 100644 --- a/signer/src/stacks/api.rs +++ b/signer/src/stacks/api.rs @@ -810,7 +810,15 @@ impl StacksClient { // The first block in the GET /v3/tenures/ response // is always the block related to the given . But we // already have that block, so we can skip adding it again. - debug_assert_eq!(blocks.first().map(|b| b.block_id()), Some(last_block_id)); + + match blocks.first().map(|b| b.block_id()) { + Some(received_id) if received_id == last_block_id => {} + Some(received_id) => { + return Err(Error::GetTenureRawMismatch(received_id, last_block_id)) + } + None => return Err(Error::EmptyStacksTenure), + } + tenure_blocks.extend(blocks.into_iter().skip(1)) } diff --git a/signer/src/transaction_signer.rs b/signer/src/transaction_signer.rs index d61be7864..f3cdf5076 100644 --- a/signer/src/transaction_signer.rs +++ b/signer/src/transaction_signer.rs @@ -428,7 +428,9 @@ where let multi_sig = MultisigTx::new_tx(&request.contract_tx, &wallet, request.tx_fee); let txid = multi_sig.tx().txid(); - debug_assert_eq!(txid, request.txid); + if txid != request.txid { + return Err(Error::SignerCoordinatorTxidMismatch(txid, request.txid)); + } let signature = crate::signature::sign_stacks_tx(multi_sig.tx(), &self.signer_private_key);