Skip to content
New issue

Have a question about this project? Sign up for a free GitHub account to open an issue and contact its maintainers and the community.

Sign up for GitHub

By clicking “Sign up for GitHub”, you agree to our terms of service and privacy statement. We’ll occasionally send you account related emails.

Already on GitHub? Sign in to your account

fix(wallet)!: Simplify SignOptions and improve finalization logic #1476

Merged
merged 1 commit into from
Jun 20, 2024
Merged

fix(wallet)!: Simplify SignOptions and improve finalization logic #1476

Show file tree
Hide file tree
Changes from all commits
Commits
Show all changes
1 commit
Select commit
File filter

Filter by extension

Filter by extension
Conversations
Failed to load comments.
Loading
Jump to
Jump to file
Failed to load files.
Loading
Diff view
Diff view
29 changes: 15 additions & 14 deletions crates/wallet/src/wallet/mod.rs
View file
Open in desktop
Original file line number Diff line number Diff line change
Expand Up @@ -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};
Expand Down Expand Up @@ -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.
Expand Down Expand Up @@ -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,
Expand All @@ -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();
}
}
Expand Down
17 changes: 0 additions & 17 deletions crates/wallet/src/wallet/signer.rs
View file
Open in desktop
Original file line number Diff line number Diff line change
Expand Up @@ -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.
Expand Down Expand Up @@ -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,
Expand Down
71 changes: 31 additions & 40 deletions crates/wallet/tests/wallet.rs
View file
Open in desktop
Original file line number Diff line number Diff line change
Expand Up @@ -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);
Expand All @@ -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());
}
});
}
}

Expand Down Expand Up @@ -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();
Expand Down Expand Up @@ -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()
Expand All @@ -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()
},
Expand Down Expand Up @@ -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()
},
Expand Down
Loading