frost/bdk
3
0
Fork 0
Code Issues Pull Requests Actions Packages Projects Releases Wiki Activity

Merge bitcoindevkit/bdk#1476: fix(wallet)!: Simplify SignOptions and improve finalization logic

Browse Source 
996605f2bf9440dd42647123a127c038253f0247 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 https://github.com/bitcoindevkit/bdk/issues/1461#issuecomment-2171983426. I had to refactor some wallet tests but didn't go out of my way to add additional tests.

  closes #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 996605f2bf9440dd42647123a127c038253f0247

Tree-SHA512: 63e78e75c22031424e87fcc26cd6b0015c626cd57c02680256bad9d1783cef71f4048b2d7ce5d0425cd4239351e37dd0e2a626dda7e8417af7fc52cb0afe6933
This commit is contained in:
志宇 2024-06-20 12:40:24 +08:00
commit e406675f43
No known key found for this signature in database
GPG Key ID: F6345C9837C2BDE8
3 changed files with 66 additions and 91 deletions
Show Stats Download Patch File Download Diff File Expand all files Collapse all files

29
crates/wallet/src/wallet/mod.rs
View File

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

17
crates/wallet/src/wallet/signer.rs
View File

@ -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,

111
crates/wallet/tests/wallet.rs
View File

@ -2780,36 +2780,6 @@ fn test_signing_only_one_of_multiple_inputs() {
)
}
#[test]
fn test_remove_partial_sigs_after_finalize_sign_option() {
let (mut wallet, _) = get_funded_wallet("wpkh(tprv8ZgxMBicQKsPd3EupYiPRhaMooHKUHJxNsTfYuScep13go8QFfHdtkG9nRkFGb7busX4isf6X9dURGCoKgitaApQ6MupRhZMcELAxTBRJgS/*)");
for remove_partial_sigs 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
.sign(
&mut psbt,
SignOptions {
remove_partial_sigs: *remove_partial_sigs,
..Default::default()
},
)
.unwrap());
psbt.inputs.iter().for_each(|input| {
if *remove_partial_sigs {
assert!(input.partial_sigs.is_empty())
} else {
assert!(!input.partial_sigs.is_empty())
}
});
}
}
#[test]
fn test_try_finalize_sign_option() {
let (mut wallet, _) = get_funded_wallet("wpkh(tprv8ZgxMBicQKsPd3EupYiPRhaMooHKUHJxNsTfYuScep13go8QFfHdtkG9nRkFGb7busX4isf6X9dURGCoKgitaApQ6MupRhZMcELAxTBRJgS/*)");
@ -2833,7 +2803,7 @@ 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());
} else {
assert!(!finalized);
@ -2844,6 +2814,55 @@ fn test_try_finalize_sign_option() {
}
}
#[test]
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);
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 {
try_finalize: *try_finalize,
..Default::default()
},
)
.unwrap();
psbt.inputs.iter().for_each(|input| {
if *try_finalize {
assert!(finalized);
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());
}
});
}
}
#[test]
fn test_sign_nonstandard_sighash() {
let sighash = EcdsaSighashType::NonePlusAnyoneCanPay;
@ -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()
},