Merge bitcoindevkit/bdk#1424: Remove trait ComputeSighash

55a17293a455435c868f60f0c9f06ba80f2f0e4c ref(signer): Use `Psbt::sighash_ecdsa` for computing sighashes (valued mammal)
f2a2dae84cd04df4301f91745efa137975eeb8e4 refactor(signer): Remove trait ComputeSighash (valued mammal)

Pull request description:

  This PR does some cleanup of the `bdk_wallet` signer module most notably by removing the internal trait `ComputeSighash` and replacing old code for computing the sighash (for legacy and segwit context) with a single method [`Psbt::sighash_ecdsa`](https://docs.rs/bitcoin/0.31.2/bitcoin/psbt/struct.Psbt.html#method.sighash_ecdsa). The logic for computing the taproot sighash is unchanged and extracted to a new helper function `compute_tap_sighash`.

  - [x] Unimplement `ComputeSighash`
  - [x] Try de-duplicating code by using `Psbt::sighash_ecdsa`. see https://github.com/bitcoindevkit/bdk/pull/1023#discussion_r1263140218
  - Not done in this PR: Consider removing unused `SignerError` variants

  fixes #1038

  ### Notes to the reviewers

  ### Changelog notice

  ### Checklists

  #### All Submissions:

  * [x] I've signed all my commits
  * [x] I followed the [contribution guidelines](https://github.com/bitcoindevkit/bdk/blob/master/CONTRIBUTING.md)
  * [x] I ran `cargo fmt` and `cargo clippy` before committing

Top commit has no ACKs.

Tree-SHA512: 56af3c9c463513ca3bae5480aa5b90d78de119c3c09c824a7220eb6832d5f403b172afc8168228918ea1adabb4bf8fca858790adfebf84fc334b4fc1cc99d3cd

crates/wallet/src/wallet/signer.rs

@@ -91,7 +91,7 @@ use bitcoin::bip32::{ChildNumber, DerivationPath, Fingerprint, Xpriv};
 use bitcoin::hashes::hash160;
 use bitcoin::secp256k1::Message;
 use bitcoin::sighash::{EcdsaSighashType, TapSighash, TapSighashType};
-use bitcoin::{ecdsa, psbt, sighash, taproot, transaction};
+use bitcoin::{ecdsa, psbt, sighash, taproot};
 use bitcoin::{key::TapTweak, key::XOnlyPublicKey, secp256k1};
 use bitcoin::{PrivateKey, Psbt, PublicKey};
 
@@ -99,7 +99,7 @@ use miniscript::descriptor::{
     Descriptor, DescriptorMultiXKey, DescriptorPublicKey, DescriptorSecretKey, DescriptorXKey,
     InnerXKey, KeyMap, SinglePriv, SinglePubKey,
 };
-use miniscript::{Legacy, Segwitv0, SigType, Tap, ToPublicKey};
+use miniscript::{SigType, ToPublicKey};
 
 use super::utils::SecpCtx;
 use crate::descriptor::{DescriptorMeta, XKeyUtils};
@@ -159,12 +159,10 @@ pub enum SignerError {
     NonStandardSighash,
     /// Invalid SIGHASH for the signing context in use
     InvalidSighash,
-    /// Error while computing the hash to sign a P2WPKH input.
-    SighashP2wpkh(sighash::P2wpkhError),
     /// Error while computing the hash to sign a Taproot input.
     SighashTaproot(sighash::TaprootError),
-    /// Error while computing the hash, out of bounds access on the transaction inputs.
+    /// PSBT sign error.
-    TxInputsIndexError(transaction::InputsIndexError),
+    Psbt(psbt::SignError),
     /// Miniscript PSBT error
     MiniscriptPsbt(MiniscriptPsbtError),
     /// To be used only by external libraries implementing [`InputSigner`] or
@@ -173,24 +171,6 @@ pub enum SignerError {
     External(String),
 }
 
-impl From<transaction::InputsIndexError> for SignerError {
-    fn from(v: transaction::InputsIndexError) -> Self {
-        Self::TxInputsIndexError(v)
-    }
-}
-
-impl From<sighash::P2wpkhError> for SignerError {
-    fn from(e: sighash::P2wpkhError) -> Self {
-        Self::SighashP2wpkh(e)
-    }
-}
-
-impl From<sighash::TaprootError> for SignerError {
-    fn from(e: sighash::TaprootError) -> Self {
-        Self::SighashTaproot(e)
-    }
-}
-
 impl fmt::Display for SignerError {
     fn fmt(&self, f: &mut fmt::Formatter<'_>) -> fmt::Result {
         match self {
@@ -205,9 +185,8 @@ impl fmt::Display for SignerError {
             Self::MissingHdKeypath => write!(f, "Missing fingerprint and derivation path"),
             Self::NonStandardSighash => write!(f, "The psbt contains a non standard sighash"),
             Self::InvalidSighash => write!(f, "Invalid SIGHASH for the signing context in use"),
-            Self::SighashP2wpkh(err) => write!(f, "Error while computing the hash to sign a P2WPKH input: {}", err),
             Self::SighashTaproot(err) => write!(f, "Error while computing the hash to sign a Taproot input: {}", err),
-            Self::TxInputsIndexError(err) => write!(f, "Error while computing the hash, out of bounds access on the transaction inputs: {}", err),
+            Self::Psbt(err) => write!(f, "Error computing the sighash: {}", err),
             Self::MiniscriptPsbt(err) => write!(f, "Miniscript PSBT error: {}", err),
             Self::External(err) => write!(f, "{}", err),
         }
@@ -472,93 +451,88 @@ impl InputSigner for SignerWrapper<PrivateKey> {
         }
 
         let pubkey = PublicKey::from_private_key(secp, self);
-        let x_only_pubkey = XOnlyPublicKey::from(pubkey.inner);
 
-        if let SignerContext::Tap { is_internal_key } = self.ctx {
+        match self.ctx {
-            if let Some(psbt_internal_key) = psbt.inputs[input_index].tap_internal_key {
+            SignerContext::Tap { is_internal_key } => {
-                if is_internal_key
+                let x_only_pubkey = XOnlyPublicKey::from(pubkey.inner);
-                    && psbt.inputs[input_index].tap_key_sig.is_none()
+
-                    && sign_options.sign_with_tap_internal_key
+                if let Some(psbt_internal_key) = psbt.inputs[input_index].tap_internal_key {
-                    && x_only_pubkey == psbt_internal_key
+                    if is_internal_key
+                        && psbt.inputs[input_index].tap_key_sig.is_none()
+                        && sign_options.sign_with_tap_internal_key
+                        && x_only_pubkey == psbt_internal_key
+                    {
+                        let (sighash, sighash_type) = compute_tap_sighash(psbt, input_index, None)?;
+                        sign_psbt_schnorr(
+                            &self.inner,
+                            x_only_pubkey,
+                            None,
+                            &mut psbt.inputs[input_index],
+                            sighash,
+                            sighash_type,
+                            secp,
+                        );
+                    }
+                }
+
+                if let Some((leaf_hashes, _)) =
+                    psbt.inputs[input_index].tap_key_origins.get(&x_only_pubkey)
                 {
-                    let (hash, hash_ty) = Tap::sighash(psbt, input_index, None)?;
+                    let leaf_hashes = leaf_hashes
-                    sign_psbt_schnorr(
+                        .iter()
-                        &self.inner,
+                        .filter(|lh| {
-                        x_only_pubkey,
+                            // Removing the leaves we shouldn't sign for
-                        None,
+                            let should_sign = match &sign_options.tap_leaves_options {
-                        &mut psbt.inputs[input_index],
+                                TapLeavesOptions::All => true,
-                        hash,
+                                TapLeavesOptions::Include(v) => v.contains(lh),
-                        hash_ty,
+                                TapLeavesOptions::Exclude(v) => !v.contains(lh),
-                        secp,
+                                TapLeavesOptions::None => false,
-                    );
+                            };
+                            // Filtering out the leaves without our key
+                            should_sign
+                                && !psbt.inputs[input_index]
+                                    .tap_script_sigs
+                                    .contains_key(&(x_only_pubkey, **lh))
+                        })
+                        .cloned()
+                        .collect::<Vec<_>>();
+                    for lh in leaf_hashes {
+                        let (sighash, sighash_type) =
+                            compute_tap_sighash(psbt, input_index, Some(lh))?;
+                        sign_psbt_schnorr(
+                            &self.inner,
+                            x_only_pubkey,
+                            Some(lh),
+                            &mut psbt.inputs[input_index],
+                            sighash,
+                            sighash_type,
+                            secp,
+                        );
+                    }
                 }
             }
-
+            SignerContext::Segwitv0 | SignerContext::Legacy => {
-            if let Some((leaf_hashes, _)) =
+                if psbt.inputs[input_index].partial_sigs.contains_key(&pubkey) {
-                psbt.inputs[input_index].tap_key_origins.get(&x_only_pubkey)
+                    return Ok(());
-            {
-                let leaf_hashes = leaf_hashes
-                    .iter()
-                    .filter(|lh| {
-                        // Removing the leaves we shouldn't sign for
-                        let should_sign = match &sign_options.tap_leaves_options {
-                            TapLeavesOptions::All => true,
-                            TapLeavesOptions::Include(v) => v.contains(lh),
-                            TapLeavesOptions::Exclude(v) => !v.contains(lh),
-                            TapLeavesOptions::None => false,
-                        };
-                        // Filtering out the leaves without our key
-                        should_sign
-                            && !psbt.inputs[input_index]
-                                .tap_script_sigs
-                                .contains_key(&(x_only_pubkey, **lh))
-                    })
-                    .cloned()
-                    .collect::<Vec<_>>();
-                for lh in leaf_hashes {
-                    let (hash, hash_ty) = Tap::sighash(psbt, input_index, Some(lh))?;
-                    sign_psbt_schnorr(
-                        &self.inner,
-                        x_only_pubkey,
-                        Some(lh),
-                        &mut psbt.inputs[input_index],
-                        hash,
-                        hash_ty,
-                        secp,
-                    );
                 }
-            }
 
-            return Ok(());
+                let mut sighasher = sighash::SighashCache::new(psbt.unsigned_tx.clone());
+                let (msg, sighash_type) = psbt
+                    .sighash_ecdsa(input_index, &mut sighasher)
+                    .map_err(SignerError::Psbt)?;
+
+                sign_psbt_ecdsa(
+                    &self.inner,
+                    pubkey,
+                    &mut psbt.inputs[input_index],
+                    &msg,
+                    sighash_type,
+                    secp,
+                    sign_options.allow_grinding,
+                );
+            }
         }
 
-        if psbt.inputs[input_index].partial_sigs.contains_key(&pubkey) {
-            return Ok(());
-        }
-
-        let (hash, hash_ty) = match self.ctx {
-            SignerContext::Segwitv0 => {
-                let (h, t) = Segwitv0::sighash(psbt, input_index, ())?;
-                let h = h.to_raw_hash();
-                (h, t)
-            }
-            SignerContext::Legacy => {
-                let (h, t) = Legacy::sighash(psbt, input_index, ())?;
-                let h = h.to_raw_hash();
-                (h, t)
-            }
-            _ => return Ok(()), // handled above
-        };
-        sign_psbt_ecdsa(
-            &self.inner,
-            pubkey,
-            &mut psbt.inputs[input_index],
-            hash,
-            hash_ty,
-            secp,
-            sign_options.allow_grinding,
-        );
-
         Ok(())
     }
 }
@@ -567,12 +541,11 @@ fn sign_psbt_ecdsa(
     secret_key: &secp256k1::SecretKey,
     pubkey: PublicKey,
     psbt_input: &mut psbt::Input,
-    hash: impl bitcoin::hashes::Hash<Bytes = [u8; 32]>,
+    msg: &Message,
     sighash_type: EcdsaSighashType,
     secp: &SecpCtx,
     allow_grinding: bool,
 ) {
-    let msg = &Message::from_digest(hash.to_byte_array());
     let signature = if allow_grinding {
         secp.sign_ecdsa_low_r(msg, secret_key)
     } else {
@@ -594,7 +567,7 @@ fn sign_psbt_schnorr(
     pubkey: XOnlyPublicKey,
     leaf_hash: Option<taproot::TapLeafHash>,
     psbt_input: &mut psbt::Input,
-    hash: TapSighash,
+    sighash: TapSighash,
     sighash_type: TapSighashType,
     secp: &SecpCtx,
 ) {
@@ -606,7 +579,7 @@ fn sign_psbt_schnorr(
         Some(_) => keypair, // no tweak for script spend
     };
 
-    let msg = &Message::from(hash);
+    let msg = &Message::from(sighash);
     let signature = secp.sign_schnorr_no_aux_rand(msg, &keypair);
     secp.verify_schnorr(&signature, msg, &XOnlyPublicKey::from_keypair(&keypair).0)
         .expect("invalid or corrupted schnorr signature");
@@ -853,198 +826,53 @@ impl Default for SignOptions {
     }
 }
 
-pub(crate) trait ComputeSighash {
+/// Computes the taproot sighash.
-    type Extra;
+fn compute_tap_sighash(
-    type Sighash;
+    psbt: &Psbt,
-    type SighashType;
+    input_index: usize,
-
+    extra: Option<taproot::TapLeafHash>,
-    fn sighash(
+) -> Result<(sighash::TapSighash, TapSighashType), SignerError> {
-        psbt: &Psbt,
+    if input_index >= psbt.inputs.len() || input_index >= psbt.unsigned_tx.input.len() {
-        input_index: usize,
+        return Err(SignerError::InputIndexOutOfRange);
-        extra: Self::Extra,
-    ) -> Result<(Self::Sighash, Self::SighashType), SignerError>;
-}
-
-impl ComputeSighash for Legacy {
-    type Extra = ();
-    type Sighash = sighash::LegacySighash;
-    type SighashType = EcdsaSighashType;
-
-    fn sighash(
-        psbt: &Psbt,
-        input_index: usize,
-        _extra: (),
-    ) -> Result<(Self::Sighash, Self::SighashType), SignerError> {
-        if input_index >= psbt.inputs.len() || input_index >= psbt.unsigned_tx.input.len() {
-            return Err(SignerError::InputIndexOutOfRange);
-        }
-
-        let psbt_input = &psbt.inputs[input_index];
-        let tx_input = &psbt.unsigned_tx.input[input_index];
-
-        let sighash = psbt_input
-            .sighash_type
-            .unwrap_or_else(|| EcdsaSighashType::All.into())
-            .ecdsa_hash_ty()
-            .map_err(|_| SignerError::InvalidSighash)?;
-        let script = match psbt_input.redeem_script {
-            Some(ref redeem_script) => redeem_script.clone(),
-            None => {
-                let non_witness_utxo = psbt_input
-                    .non_witness_utxo
-                    .as_ref()
-                    .ok_or(SignerError::MissingNonWitnessUtxo)?;
-                let prev_out = non_witness_utxo
-                    .output
-                    .get(tx_input.previous_output.vout as usize)
-                    .ok_or(SignerError::InvalidNonWitnessUtxo)?;
-
-                prev_out.script_pubkey.clone()
-            }
-        };
-
-        Ok((
-            sighash::SighashCache::new(&psbt.unsigned_tx).legacy_signature_hash(
-                input_index,
-                &script,
-                sighash.to_u32(),
-            )?,
-            sighash,
-        ))
     }
-}
 
-impl ComputeSighash for Segwitv0 {
+    let psbt_input = &psbt.inputs[input_index];
-    type Extra = ();
-    type Sighash = sighash::SegwitV0Sighash;
-    type SighashType = EcdsaSighashType;
 
-    fn sighash(
+    let sighash_type = psbt_input
-        psbt: &Psbt,
+        .sighash_type
-        input_index: usize,
+        .unwrap_or_else(|| TapSighashType::Default.into())
-        _extra: (),
+        .taproot_hash_ty()
-    ) -> Result<(Self::Sighash, Self::SighashType), SignerError> {
+        .map_err(|_| SignerError::InvalidSighash)?;
-        if input_index >= psbt.inputs.len() || input_index >= psbt.unsigned_tx.input.len() {
+    let witness_utxos = (0..psbt.inputs.len())
-            return Err(SignerError::InputIndexOutOfRange);
+        .map(|i| psbt.get_utxo_for(i))
-        }
+        .collect::<Vec<_>>();
+    let mut all_witness_utxos = vec![];
 
-        let psbt_input = &psbt.inputs[input_index];
+    let mut cache = sighash::SighashCache::new(&psbt.unsigned_tx);
-        let tx_input = &psbt.unsigned_tx.input[input_index];
+    let is_anyone_can_pay = psbt::PsbtSighashType::from(sighash_type).to_u32() & 0x80 != 0;
+    let prevouts = if is_anyone_can_pay {
+        sighash::Prevouts::One(
+            input_index,
+            witness_utxos[input_index]
+                .as_ref()
+                .ok_or(SignerError::MissingWitnessUtxo)?,
+        )
+    } else if witness_utxos.iter().all(Option::is_some) {
+        all_witness_utxos.extend(witness_utxos.iter().filter_map(|x| x.as_ref()));
+        sighash::Prevouts::All(&all_witness_utxos)
+    } else {
+        return Err(SignerError::MissingWitnessUtxo);
+    };
 
-        let sighash_type = psbt_input
+    // Assume no OP_CODESEPARATOR
-            .sighash_type
+    let extra = extra.map(|leaf_hash| (leaf_hash, 0xFFFFFFFF));
-            .unwrap_or_else(|| EcdsaSighashType::All.into())
-            .ecdsa_hash_ty()
-            .map_err(|_| SignerError::InvalidSighash)?;
 
-        // Always try first with the non-witness utxo
+    Ok((
-        let utxo = if let Some(prev_tx) = &psbt_input.non_witness_utxo {
+        cache
-            // Check the provided prev-tx
+            .taproot_signature_hash(input_index, &prevouts, None, extra, sighash_type)
-            if prev_tx.compute_txid() != tx_input.previous_output.txid {
+            .map_err(SignerError::SighashTaproot)?,
-                return Err(SignerError::InvalidNonWitnessUtxo);
+        sighash_type,
-            }
+    ))
-
-            // The output should be present, if it's missing the `non_witness_utxo` is invalid
-            prev_tx
-                .output
-                .get(tx_input.previous_output.vout as usize)
-                .ok_or(SignerError::InvalidNonWitnessUtxo)?
-        } else if let Some(witness_utxo) = &psbt_input.witness_utxo {
-            // Fallback to the witness_utxo. If we aren't allowed to use it, signing should fail
-            // before we get to this point
-            witness_utxo
-        } else {
-            // Nothing has been provided
-            return Err(SignerError::MissingNonWitnessUtxo);
-        };
-        let value = utxo.value;
-
-        let mut sighasher = sighash::SighashCache::new(&psbt.unsigned_tx);
-
-        let sighash = match psbt_input.witness_script {
-            Some(ref witness_script) => {
-                sighasher.p2wsh_signature_hash(input_index, witness_script, value, sighash_type)?
-            }
-            None => {
-                if utxo.script_pubkey.is_p2wpkh() {
-                    sighasher.p2wpkh_signature_hash(
-                        input_index,
-                        &utxo.script_pubkey,
-                        value,
-                        sighash_type,
-                    )?
-                } else if psbt_input
-                    .redeem_script
-                    .as_ref()
-                    .map(|s| s.is_p2wpkh())
-                    .unwrap_or(false)
-                {
-                    let script_pubkey = psbt_input.redeem_script.as_ref().unwrap();
-                    sighasher.p2wpkh_signature_hash(
-                        input_index,
-                        script_pubkey,
-                        value,
-                        sighash_type,
-                    )?
-                } else {
-                    return Err(SignerError::MissingWitnessScript);
-                }
-            }
-        };
-        Ok((sighash, sighash_type))
-    }
-}
-
-impl ComputeSighash for Tap {
-    type Extra = Option<taproot::TapLeafHash>;
-    type Sighash = TapSighash;
-    type SighashType = TapSighashType;
-
-    fn sighash(
-        psbt: &Psbt,
-        input_index: usize,
-        extra: Self::Extra,
-    ) -> Result<(Self::Sighash, TapSighashType), SignerError> {
-        if input_index >= psbt.inputs.len() || input_index >= psbt.unsigned_tx.input.len() {
-            return Err(SignerError::InputIndexOutOfRange);
-        }
-
-        let psbt_input = &psbt.inputs[input_index];
-
-        let sighash_type = psbt_input
-            .sighash_type
-            .unwrap_or_else(|| TapSighashType::Default.into())
-            .taproot_hash_ty()
-            .map_err(|_| SignerError::InvalidSighash)?;
-        let witness_utxos = (0..psbt.inputs.len())
-            .map(|i| psbt.get_utxo_for(i))
-            .collect::<Vec<_>>();
-        let mut all_witness_utxos = vec![];
-
-        let mut cache = sighash::SighashCache::new(&psbt.unsigned_tx);
-        let is_anyone_can_pay = psbt::PsbtSighashType::from(sighash_type).to_u32() & 0x80 != 0;
-        let prevouts = if is_anyone_can_pay {
-            sighash::Prevouts::One(
-                input_index,
-                witness_utxos[input_index]
-                    .as_ref()
-                    .ok_or(SignerError::MissingWitnessUtxo)?,
-            )
-        } else if witness_utxos.iter().all(Option::is_some) {
-            all_witness_utxos.extend(witness_utxos.iter().filter_map(|x| x.as_ref()));
-            sighash::Prevouts::All(&all_witness_utxos)
-        } else {
-            return Err(SignerError::MissingWitnessUtxo);
-        };
-
-        // Assume no OP_CODESEPARATOR
-        let extra = extra.map(|leaf_hash| (leaf_hash, 0xFFFFFFFF));
-
-        Ok((
-            cache.taproot_signature_hash(input_index, &prevouts, None, extra, sighash_type)?,
-            sighash_type,
-        ))
-    }
 }
 
 impl PartialOrd for SignersContainerKey {