From 8c78a42163dee06b640f46d74255df37dbc53873 Mon Sep 17 00:00:00 2001 From: vmammal Date: Sun, 11 Feb 2024 15:32:36 -0500 Subject: [PATCH 1/2] test(psbt): Fixup test_psbt_multiple_internalkey_signers to verify the signature of the input and ensure the right internal key is used to sign. This fixes a shortcoming of a previous version of the test that relied on the result of `finalize_psbt` but would have erroneously allowed signing with the wrong key. --- crates/bdk/tests/psbt.rs | 52 ++++++++++++++++++++++++++++++++-------- 1 file changed, 42 insertions(+), 10 deletions(-) diff --git a/crates/bdk/tests/psbt.rs b/crates/bdk/tests/psbt.rs index 3c4968bfe..b4d054c11 100644 --- a/crates/bdk/tests/psbt.rs +++ b/crates/bdk/tests/psbt.rs @@ -161,16 +161,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, @@ -183,10 +193,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"); } From 5840ce473e430de4c4e3698734e9667cc476fee4 Mon Sep 17 00:00:00 2001 From: vmammal Date: Wed, 31 Jan 2024 16:18:09 -0500 Subject: [PATCH 2/2] fix(bdk): Remove extra taproot fields when finalizing Psbt 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. test(wallet): Add test `test_taproot_remove_tapfields_after_finalize_sign_option` that checks various fields have been cleared for taproot Psbt `Input`s and `Output`s according to BIP371. --- crates/bdk/src/wallet/mod.rs | 15 +++++++++++++++ crates/bdk/src/wallet/signer.rs | 11 +++++++++++ crates/bdk/tests/wallet.rs | 26 ++++++++++++++++++++++++++ 3 files changed, 52 insertions(+) diff --git a/crates/bdk/src/wallet/mod.rs b/crates/bdk/src/wallet/mod.rs index c6c922d55..406f60e0b 100644 --- a/crates/bdk/src/wallet/mod.rs +++ b/crates/bdk/src/wallet/mod.rs @@ -1972,6 +1972,15 @@ impl Wallet { 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, } @@ -1980,6 +1989,12 @@ impl Wallet { } } + if finished && sign_options.remove_taproot_extras { + for output in &mut psbt.outputs { + output.tap_key_origins.clear(); + } + } + Ok(finished) } diff --git a/crates/bdk/src/wallet/signer.rs b/crates/bdk/src/wallet/signer.rs index 63784a3fd..38e51dfb2 100644 --- a/crates/bdk/src/wallet/signer.rs +++ b/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, diff --git a/crates/bdk/tests/wallet.rs b/crates/bdk/tests/wallet.rs index 271b87163..c2f7d40fa 100644 --- a/crates/bdk/tests/wallet.rs +++ b/crates/bdk/tests/wallet.rs @@ -2813,6 +2813,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());