Merge bitcoindevkit#1476: fix(wallet)!: Simplify SignOptions and im…

…prove finalization logic

996605f fix(wallet)!: Simplify `SignOptions` and improve finalization logic (valued mammal)

Pull request description:

  Rather than comingle various `SignOptions` with the finalization step, we simply clear all fields when finalizing as per the PSBT spec in BIPs 174 and 371 which is more in line with user expectations.

  ### Notes to the reviewers

  I chose to re-implement some parts of [`finalize_input`](https://docs.rs/miniscript/latest/src/miniscript/psbt/finalizer.rs.html#434) since it's fairly straightforward, see also bitcoindevkit#1461 (comment). I had to refactor some wallet tests but didn't go out of my way to add additional tests.

  closes bitcoindevkit#1461

  ### Changelog notice

  - Removed field `remove_partial_sigs` from `SignOptions`
  - Removed field `remove_taproot_extras` from `SignOptions`

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

  * [x] This pull request breaks the existing API
  * [ ] 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:
    re-ACK 996605f

Tree-SHA512: 63e78e75c22031424e87fcc26cd6b0015c626cd57c02680256bad9d1783cef71f4048b2d7ce5d0425cd4239351e37dd0e2a626dda7e8417af7fc52cb0afe6933

crates/wallet/src/wallet/mod.rs

@@ -40,6 +40,7 @@ use bitcoin::{
 use bitcoin::{consensus::encode::serialize, transaction, BlockHash, Psbt};
 use bitcoin::{constants::genesis_block, Amount};
 use core::fmt;
+use core::mem;
 use core::ops::Deref;
 use descriptor::error::Error as DescriptorError;
 use miniscript::psbt::{PsbtExt, PsbtInputExt, PsbtInputSatisfier};
@@ -1821,7 +1822,8 @@ impl Wallet {
 
     /// Finalize a PSBT, i.e., for each input determine if sufficient data is available to pass
     /// validation and construct the respective `scriptSig` or `scriptWitness`. Please refer to
-    /// [BIP174](https://github.com/bitcoin/bips/blob/master/bip-0174.mediawiki#Input_Finalizer)
+    /// [BIP174](https://github.com/bitcoin/bips/blob/master/bip-0174.mediawiki#Input_Finalizer),
+    /// and [BIP371](https://github.com/bitcoin/bips/blob/master/bip-0371.mediawiki)
     /// for further information.
     ///
     /// Returns `true` if the PSBT could be finalized, and `false` otherwise.
@@ -1884,20 +1886,17 @@ impl Wallet {
                         ),
                     ) {
                         Ok(_) => {
+                            // Set the UTXO fields, final script_sig and witness
+                            // and clear everything else.
+                            let original = mem::take(&mut psbt.inputs[n]);
                             let psbt_input = &mut psbt.inputs[n];
-                            psbt_input.final_script_sig = Some(tmp_input.script_sig);
-                            psbt_input.final_script_witness = Some(tmp_input.witness);
-                            if sign_options.remove_partial_sigs {
-                                psbt_input.partial_sigs.clear();
+                            psbt_input.non_witness_utxo = original.non_witness_utxo;
+                            psbt_input.witness_utxo = original.witness_utxo;
+                            if !tmp_input.script_sig.is_empty() {
+                                psbt_input.final_script_sig = Some(tmp_input.script_sig);
                             }
-                            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;
+                            if !tmp_input.witness.is_empty() {
+                                psbt_input.final_script_witness = Some(tmp_input.witness);
                             }
                         }
                         Err(_) => finished = false,
@@ -1907,8 +1906,10 @@ impl Wallet {
             }
         }
 
-        if finished && sign_options.remove_taproot_extras {
+        // Clear derivation paths from outputs
+        if finished {
             for output in &mut psbt.outputs {
+                output.bip32_derivation.clear();
                 output.tap_key_origins.clear();
             }
         }

crates/wallet/src/wallet/signer.rs

@@ -801,21 +801,6 @@ pub struct SignOptions {
     /// Defaults to `false` which will only allow signing using `SIGHASH_ALL`.
     pub allow_all_sighashes: bool,
 
-    /// Whether to remove partial signatures from the PSBT inputs while finalizing PSBT.
-    ///
-    /// 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.
@@ -860,8 +845,6 @@ impl Default for SignOptions {
             trust_witness_utxo: false,
             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/wallet/tests/wallet.rs

@@ -2781,38 +2781,42 @@ fn test_signing_only_one_of_multiple_inputs() {
 }
 
 #[test]
-fn test_remove_partial_sigs_after_finalize_sign_option() {
+fn test_try_finalize_sign_option() {
     let (mut wallet, _) = get_funded_wallet("wpkh(tprv8ZgxMBicQKsPd3EupYiPRhaMooHKUHJxNsTfYuScep13go8QFfHdtkG9nRkFGb7busX4isf6X9dURGCoKgitaApQ6MupRhZMcELAxTBRJgS/*)");
 
-    for remove_partial_sigs in &[true, false] {
+    for try_finalize in &[true, false] {
         let addr = wallet.next_unused_address(KeychainKind::External);
         let mut builder = wallet.build_tx();
         builder.drain_to(addr.script_pubkey()).drain_wallet();
         let mut psbt = builder.finish().unwrap();
 
-        assert!(wallet
+        let finalized = wallet
             .sign(
                 &mut psbt,
                 SignOptions {
-                    remove_partial_sigs: *remove_partial_sigs,
+                    try_finalize: *try_finalize,
                     ..Default::default()
                 },
             )
-            .unwrap());
+            .unwrap();
 
         psbt.inputs.iter().for_each(|input| {
-            if *remove_partial_sigs {
-                assert!(input.partial_sigs.is_empty())
+            if *try_finalize {
+                assert!(finalized);
+                assert!(input.final_script_sig.is_none());
+                assert!(input.final_script_witness.is_some());
             } else {
-                assert!(!input.partial_sigs.is_empty())
+                assert!(!finalized);
+                assert!(input.final_script_sig.is_none());
+                assert!(input.final_script_witness.is_none());
             }
         });
     }
 }
 
 #[test]
-fn test_try_finalize_sign_option() {
-    let (mut wallet, _) = get_funded_wallet("wpkh(tprv8ZgxMBicQKsPd3EupYiPRhaMooHKUHJxNsTfYuScep13go8QFfHdtkG9nRkFGb7busX4isf6X9dURGCoKgitaApQ6MupRhZMcELAxTBRJgS/*)");
+fn test_taproot_try_finalize_sign_option() {
+    let (mut wallet, _) = get_funded_wallet(get_test_tr_with_taptree());
 
     for try_finalize in &[true, false] {
         let addr = wallet.next_unused_address(KeychainKind::External);
@@ -2833,14 +2837,29 @@ fn test_try_finalize_sign_option() {
         psbt.inputs.iter().for_each(|input| {
             if *try_finalize {
                 assert!(finalized);
-                assert!(input.final_script_sig.is_some());
+                assert!(input.final_script_sig.is_none());
                 assert!(input.final_script_witness.is_some());
+                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());
             } else {
                 assert!(!finalized);
                 assert!(input.final_script_sig.is_none());
                 assert!(input.final_script_witness.is_none());
             }
         });
+        psbt.outputs.iter().for_each(|output| {
+            if *try_finalize {
+                assert!(finalized);
+                assert!(output.tap_key_origins.is_empty());
+            } else {
+                assert!(!finalized);
+                assert!(!output.tap_key_origins.is_empty());
+            }
+        });
     }
 }
 
@@ -3164,32 +3183,6 @@ fn test_get_address_no_reuse() {
     });
 }
 
-#[test]
-fn test_taproot_remove_tapfields_after_finalize_sign_option() {
-    let (mut wallet, _) = get_funded_wallet(get_test_tr_with_taptree());
-
-    let addr = wallet.next_unused_address(KeychainKind::External);
-    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 (desc, change_desc) = get_test_tr_single_sig_xprv_with_change_desc();
@@ -3922,7 +3915,6 @@ fn test_fee_rate_sign_no_grinding_high_r() {
             .sign(
                 &mut psbt,
                 SignOptions {
-                    remove_partial_sigs: false,
                     try_finalize: false,
                     allow_grinding: false,
                     ..Default::default()
@@ -3941,7 +3933,6 @@ fn test_fee_rate_sign_no_grinding_high_r() {
         .sign(
             &mut psbt,
             SignOptions {
-                remove_partial_sigs: false,
                 allow_grinding: false,
                 ..Default::default()
             },
@@ -3972,7 +3963,7 @@ fn test_fee_rate_sign_grinding_low_r() {
         .sign(
             &mut psbt,
             SignOptions {
-                remove_partial_sigs: false,
+                try_finalize: false,
                 allow_grinding: true,
                 ..Default::default()
             },