[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 <[email protected]>

---------

Co-authored-by: Daniel Jordon <[email protected]>

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());

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,

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))
     }

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

signer/src/stacks/api.rs

@@ -810,7 +810,15 @@ impl StacksClient {
             // The first block in the GET /v3/tenures/<block-id> response
             // is always the block related to the given <block-id>. 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))
         }
 

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);