Merge #1200: fix(bdk): Check if we're using the correct internal key before signing

e553231 fix(bdk): Check if we're using the correct... ...internal key before signing (Daniela Brozzoni)

Pull request description:

  ### Description

  Fixes #1142

  We would previously sign with whatever x_only_pubkey we had in hand, without first checking if it was the right key or not. This effectively meant that adding multiple taproot PrivateKey signers would produce unbroadcastable transactions.

  ### Changelog notice

  - Fix a bug related to taproot signing with internal keys. We would previously sign with the first private key we had, without checking if it was the correct internal key or not.

  ### 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

  #### Bugfixes:

  * [ ] This pull request breaks the existing API
  * [x] I've added tests to reproduce the issue which are now passing
  * [x] I'm linking the issue being fixed by this PR

ACKs for top commit:
  evanlinjin:
    ACK e553231

Tree-SHA512: c4abbcd27935b8ce80a70b6e0843507866e3d075939f0b01504c090929ed96b4b9c6fee599f701e69960a6c86175682cc6d7f8cc4c3fb1d08a74b7563f8ca145

Author: danielabrozzoni

crates/bdk/src/wallet/signer.rs

@@ -459,20 +459,23 @@ impl InputSigner for SignerWrapper<PrivateKey> {
         let x_only_pubkey = XOnlyPublicKey::from(pubkey.inner);
 
         if let SignerContext::Tap { is_internal_key } = self.ctx {
-            if is_internal_key
-                && psbt.inputs[input_index].tap_key_sig.is_none()
-                && sign_options.sign_with_tap_internal_key
-            {
-                let (hash, hash_ty) = Tap::sighash(psbt, input_index, None)?;
-                sign_psbt_schnorr(
-                    &self.inner,
-                    x_only_pubkey,
-                    None,
-                    &mut psbt.inputs[input_index],
-                    hash,
-                    hash_ty,
-                    secp,
-                );
+            if let Some(psbt_internal_key) = psbt.inputs[input_index].tap_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 (hash, hash_ty) = Tap::sighash(psbt, input_index, None)?;
+                    sign_psbt_schnorr(
+                        &self.inner,
+                        x_only_pubkey,
+                        None,
+                        &mut psbt.inputs[input_index],
+                        hash,
+                        hash_ty,
+                        secp,
+                    );
+                }
             }
 
             if let Some((leaf_hashes, _)) =

crates/bdk/tests/psbt.rs

@@ -156,3 +156,37 @@ fn test_psbt_fee_rate_with_missing_txout() {
     assert!(pkh_psbt.fee_amount().is_none());
     assert!(pkh_psbt.fee_rate().is_none());
 }
+
+#[test]
+fn test_psbt_multiple_internalkey_signers() {
+    use bdk::signer::{SignerContext, SignerOrdering, SignerWrapper};
+    use bdk::KeychainKind;
+    use bitcoin::{secp256k1::Secp256k1, PrivateKey};
+    use miniscript::psbt::PsbtExt;
+    use std::sync::Arc;
+
+    let secp = Secp256k1::new();
+    let (mut wallet, _) = get_funded_wallet(get_test_tr_single_sig());
+    let send_to = wallet.get_address(AddressIndex::New);
+    let mut builder = wallet.build_tx();
+    builder.add_recipient(send_to.script_pubkey(), 10_000);
+    let mut psbt = builder.finish().unwrap();
+    // Adds a signer for the wrong internal key, bdk should not use this key to sign
+    wallet.add_signer(
+        KeychainKind::External,
+        // A signerordering lower than 100, bdk will use this signer first
+        SignerOrdering(0),
+        Arc::new(SignerWrapper::new(
+            PrivateKey::from_wif("5J5PZqvCe1uThJ3FZeUUFLCh2FuK9pZhtEK4MzhNmugqTmxCdwE").unwrap(),
+            SignerContext::Tap {
+                is_internal_key: true,
+            },
+        )),
+    );
+    let _ = wallet.sign(&mut psbt, SignOptions::default()).unwrap();
+    // Checks that we signed using the right key
+    assert!(
+        psbt.finalize_mut(&secp).is_ok(),
+        "The wrong internal key was used"
+    );
+}