Merge #1310: Remove extra taproot fields when finalizing PSBT

5840ce4 fix(bdk): Remove extra taproot fields when finalizing Psbt (vmammal)
8c78a42 test(psbt): Fixup test_psbt_multiple_internalkey_signers (vmammal)

Pull request description:

  We currently allow removing `partial_sigs` from a finalized PSBT, which is relevant to non-taproot inputs, however taproot related PSBT fields were left in place despite the recommendation of BIP371 to remove them once the `final_script_witness` is constructed. This can cause confusion for parsers that encounter extra taproot metadata in an already satisfied input.

  Fix this by introducing a new member to SignOptions `remove_taproot_extras`, which when true will remove extra taproot related data from a PSBT upon successful finalization. This change makes removal of all taproot extras the default but configurable.

  fixes #1243

  ### Notes to the reviewers

  If there's a better or more descriptive name for `remove_taproot_extras`, I'm open to changing it.

  ### Changelog notice

  Fixed an [issue](#1243) finalizing taproot inputs following BIP371

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

Tree-SHA512: 69f022c6f736500590e36880dd2c845321464f89af6a2c67987d1c520f70a298328363cade0f55f270c4e7169e740bd4ada752ec2c75afa02b6a5a851f9030c3

crates/bdk/src/wallet/mod.rs

@@ -1971,6 +1971,15 @@ impl<D> Wallet<D> {
                             if sign_options.remove_partial_sigs {
                                 psbt_input.partial_sigs.clear();
                             }
+                            if sign_options.remove_taproot_extras {
+                                // We just constructed the final witness, clear these fields.
+                                psbt_input.tap_key_sig = None;
+                                psbt_input.tap_script_sigs.clear();
+                                psbt_input.tap_scripts.clear();
+                                psbt_input.tap_key_origins.clear();
+                                psbt_input.tap_internal_key = None;
+                                psbt_input.tap_merkle_root = None;
+                            }
                         }
                         Err(_) => finished = false,
                     }
@@ -1979,6 +1988,12 @@ impl<D> Wallet<D> {
             }
         }
 
+        if finished && sign_options.remove_taproot_extras {
+            for output in &mut psbt.outputs {
+                output.tap_key_origins.clear();
+            }
+        }
+
         Ok(finished)
     }
 

crates/bdk/src/wallet/signer.rs

@@ -782,6 +782,16 @@ pub struct SignOptions {
     /// Defaults to `true` which will remove partial signatures during finalization.
     pub remove_partial_sigs: bool,
 
+    /// Whether to remove taproot specific fields from the PSBT on finalization.
+    ///
+    /// For inputs this includes the taproot internal key, merkle root, and individual
+    /// scripts and signatures. For both inputs and outputs it includes key origin info.
+    ///
+    /// Defaults to `true` which will remove all of the above mentioned fields when finalizing.
+    ///
+    /// See [`BIP371`](https://github.com/bitcoin/bips/blob/master/bip-0371.mediawiki) for details.
+    pub remove_taproot_extras: bool,
+
     /// Whether to try finalizing the PSBT after the inputs are signed.
     ///
     /// Defaults to `true` which will try finalizing PSBT after inputs are signed.
@@ -827,6 +837,7 @@ impl Default for SignOptions {
             assume_height: None,
             allow_all_sighashes: false,
             remove_partial_sigs: true,
+            remove_taproot_extras: true,
             try_finalize: true,
             tap_leaves_options: TapLeavesOptions::default(),
             sign_with_tap_internal_key: true,

crates/bdk/tests/psbt.rs

@@ -162,16 +162,26 @@ fn test_psbt_fee_rate_with_missing_txout() {
 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 bitcoin::key::TapTweak;
+    use bitcoin::secp256k1::{schnorr, KeyPair, Message, Secp256k1, XOnlyPublicKey};
+    use bitcoin::sighash::{Prevouts, SighashCache, TapSighashType};
+    use bitcoin::{PrivateKey, TxOut};
     use std::sync::Arc;
 
     let secp = Secp256k1::new();
-    let (mut wallet, _) = get_funded_wallet(get_test_tr_single_sig());
+    let wif = "cNJmN3fH9DDbDt131fQNkVakkpzawJBSeybCUNmP1BovpmGQ45xG";
+    let desc = format!("tr({})", wif);
+    let prv = PrivateKey::from_wif(wif).unwrap();
+    let keypair = KeyPair::from_secret_key(&secp, &prv.inner);
+
+    let (mut wallet, _) = get_funded_wallet(&desc);
+    let to_spend = wallet.get_balance().total();
     let send_to = wallet.get_address(AddressIndex::New);
     let mut builder = wallet.build_tx();
-    builder.add_recipient(send_to.script_pubkey(), 10_000);
+    builder.drain_to(send_to.script_pubkey()).drain_wallet();
     let mut psbt = builder.finish().unwrap();
+    let unsigned_tx = psbt.unsigned_tx.clone();
+
     // Adds a signer for the wrong internal key, bdk should not use this key to sign
     wallet.add_signer(
         KeychainKind::External,
@@ -184,10 +194,32 @@ fn test_psbt_multiple_internalkey_signers() {
             },
         )),
     );
-    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"
-    );
+    let finalized = wallet.sign(&mut psbt, SignOptions::default()).unwrap();
+    assert!(finalized);
+
+    // To verify, we need the signature, message, and pubkey
+    let witness = psbt.inputs[0].final_script_witness.as_ref().unwrap();
+    assert!(!witness.is_empty());
+    let signature = schnorr::Signature::from_slice(witness.iter().next().unwrap()).unwrap();
+
+    // the prevout we're spending
+    let prevouts = &[TxOut {
+        script_pubkey: send_to.script_pubkey(),
+        value: to_spend,
+    }];
+    let prevouts = Prevouts::All(prevouts);
+    let input_index = 0;
+    let mut sighash_cache = SighashCache::new(unsigned_tx);
+    let sighash = sighash_cache
+        .taproot_key_spend_signature_hash(input_index, &prevouts, TapSighashType::Default)
+        .unwrap();
+    let message = Message::from(sighash);
+
+    // add tweak. this was taken from `signer::sign_psbt_schnorr`
+    let keypair = keypair.tap_tweak(&secp, None).to_inner();
+    let (xonlykey, _parity) = XOnlyPublicKey::from_keypair(&keypair);
+
+    // Must verify if we used the correct key to sign
+    let verify_res = secp.verify_schnorr(&signature, &message, &xonlykey);
+    assert!(verify_res.is_ok(), "The wrong internal key was used");
 }

crates/bdk/tests/wallet.rs

@@ -2831,6 +2831,32 @@ fn test_get_address_no_reuse_single_descriptor() {
     });
 }
 
+#[test]
+fn test_taproot_remove_tapfields_after_finalize_sign_option() {
+    let (mut wallet, _) = get_funded_wallet(get_test_tr_with_taptree());
+
+    let addr = wallet.get_address(New);
+    let mut builder = wallet.build_tx();
+    builder.drain_to(addr.script_pubkey()).drain_wallet();
+    let mut psbt = builder.finish().unwrap();
+    let finalized = wallet.sign(&mut psbt, SignOptions::default()).unwrap();
+    assert!(finalized);
+
+    // removes tap_* from inputs
+    for input in &psbt.inputs {
+        assert!(input.tap_key_sig.is_none());
+        assert!(input.tap_script_sigs.is_empty());
+        assert!(input.tap_scripts.is_empty());
+        assert!(input.tap_key_origins.is_empty());
+        assert!(input.tap_internal_key.is_none());
+        assert!(input.tap_merkle_root.is_none());
+    }
+    // removes key origins from outputs
+    for output in &psbt.outputs {
+        assert!(output.tap_key_origins.is_empty());
+    }
+}
+
 #[test]
 fn test_taproot_psbt_populate_tap_key_origins() {
     let (mut wallet, _) = get_funded_wallet(get_test_tr_single_sig_xprv());