From b5e95898032258ddce6a4b11a5f378d350d1795e Mon Sep 17 00:00:00 2001 From: Alekos Filini Date: Mon, 19 Apr 2021 14:16:39 +0200 Subject: [PATCH] [signer] Adjust signing behavior with `SignOptions` --- CHANGELOG.md | 1 + README.md | 4 +-- src/lib.rs | 1 + src/wallet/mod.rs | 55 ++++++++++++++++++------------ src/wallet/signer.rs | 68 +++++++++++++++++++++++++++++++++---- testutils-macros/src/lib.rs | 22 ++++++------ 6 files changed, 110 insertions(+), 41 deletions(-) diff --git a/CHANGELOG.md b/CHANGELOG.md index fecd885e..a0a8c964 100644 --- a/CHANGELOG.md +++ b/CHANGELOG.md @@ -16,6 +16,7 @@ Timelocks are considered (optionally) in building the `satisfaction` field ### Wallet - Changed `Wallet::{sign, finalize_psbt}` now take a `&mut psbt` rather than consuming it. +- Require and validate `non_witness_utxo` for SegWit signatures by default, can be adjusted with `SignOptions` ## [v0.6.0] - [v0.5.1] diff --git a/README.md b/README.md index fac0608e..2d8c3c21 100644 --- a/README.md +++ b/README.md @@ -130,7 +130,7 @@ fn main() -> Result<(), bdk::Error> { ### Sign a transaction ```rust,no_run -use bdk::{Wallet, database::MemoryDatabase}; +use bdk::{Wallet, SignOptions, database::MemoryDatabase}; use bitcoin::consensus::deserialize; @@ -145,7 +145,7 @@ fn main() -> Result<(), bdk::Error> { let psbt = "..."; let mut psbt = deserialize(&base64::decode(psbt).unwrap())?; - let finalized = wallet.sign(&mut psbt, None)?; + let finalized = wallet.sign(&mut psbt, SignOptions::default())?; Ok(()) } diff --git a/src/lib.rs b/src/lib.rs index 917d6a8f..f5323b1a 100644 --- a/src/lib.rs +++ b/src/lib.rs @@ -256,6 +256,7 @@ pub use error::Error; pub use types::*; pub use wallet::address_validator; pub use wallet::signer; +pub use wallet::signer::SignOptions; pub use wallet::tx_builder::TxBuilder; pub use wallet::Wallet; diff --git a/src/wallet/mod.rs b/src/wallet/mod.rs index 6ae397a6..ef945a13 100644 --- a/src/wallet/mod.rs +++ b/src/wallet/mod.rs @@ -46,7 +46,7 @@ pub use utils::IsDust; use address_validator::AddressValidator; use coin_selection::DefaultCoinSelectionAlgorithm; -use signer::{Signer, SignerOrdering, SignersContainer}; +use signer::{SignOptions, Signer, SignerOrdering, SignersContainer}; use tx_builder::{BumpFee, CreateTx, FeePolicy, TxBuilder, TxParams}; use utils::{check_nlocktime, check_nsequence_rbf, After, Older, SecpCtx, DUST_LIMIT_SATOSHI}; @@ -706,7 +706,7 @@ where /// .enable_rbf(); /// builder.finish()? /// }; - /// let _ = wallet.sign(&mut psbt, None)?; + /// let _ = wallet.sign(&mut psbt, SignOptions::default())?; /// let tx = psbt.extract_tx(); /// // broadcast tx but it's taking too long to confirm so we want to bump the fee /// let (mut psbt, _) = { @@ -716,7 +716,7 @@ where /// builder.finish()? /// }; /// - /// let _ = wallet.sign(&mut psbt, None)?; + /// let _ = wallet.sign(&mut psbt, SignOptions::default())?; /// let fee_bumped_tx = psbt.extract_tx(); /// // broadcast fee_bumped_tx to replace original /// # Ok::<(), bdk::Error>(()) @@ -833,6 +833,11 @@ where /// Sign a transaction with all the wallet's signers, in the order specified by every signer's /// [`SignerOrdering`] /// + /// The [`SignOptions`] can be used to tweak the behavior of the software signers, and the way + /// the transaction is finalized at the end. Note that it can't be guaranteed that *every* + /// signers will follow the options, but the "software signers" (WIF keys and `xprv`) defined + /// in this library will. + /// /// ## Example /// /// ``` @@ -848,13 +853,23 @@ where /// builder.add_recipient(to_address.script_pubkey(), 50_000); /// builder.finish()? /// }; - /// let finalized = wallet.sign(&mut psbt, None)?; + /// let finalized = wallet.sign(&mut psbt, SignOptions::default())?; /// assert!(finalized, "we should have signed all the inputs"); /// # Ok::<(), bdk::Error>(()) - pub fn sign(&self, psbt: &mut PSBT, assume_height: Option) -> Result { + pub fn sign(&self, psbt: &mut PSBT, sign_options: SignOptions) -> Result { // this helps us doing our job later self.add_input_hd_keypaths(psbt)?; + // If we aren't allowed to use `witness_utxo`, ensure that every input has the + // `non_witness_utxo` + if !sign_options.trust_witness_utxo { + for input in &psbt.inputs { + if input.non_witness_utxo.is_none() { + return Err(Error::Signer(signer::SignerError::MissingNonWitnessUtxo)); + } + } + } + for signer in self .signers .signers() @@ -871,7 +886,7 @@ where } // attempt to finalize - self.finalize_psbt(psbt, assume_height) + self.finalize_psbt(psbt, sign_options) } /// Return the spending policies for the wallet's descriptor @@ -907,11 +922,9 @@ where } /// Try to finalize a PSBT - pub fn finalize_psbt( - &self, - psbt: &mut PSBT, - assume_height: Option, - ) -> Result { + /// + /// The [`SignOptions`] can be used to tweak the behavior of the finalizer. + pub fn finalize_psbt(&self, psbt: &mut PSBT, sign_options: SignOptions) -> Result { let tx = &psbt.global.unsigned_tx; let mut finished = true; @@ -927,7 +940,7 @@ where .borrow() .get_tx(&input.previous_output.txid, false)? .map(|tx| tx.height.unwrap_or(std::u32::MAX)); - let current_height = assume_height.or(self.current_height); + let current_height = sign_options.assume_height.or(self.current_height); debug!( "Input #{} - {}, using `create_height` = {:?}, `current_height` = {:?}", @@ -2440,14 +2453,14 @@ mod test { "foreign_utxo should be in there" ); - let finished = wallet1.sign(&mut psbt, None).unwrap(); + let finished = wallet1.sign(&mut psbt, Default::default()).unwrap(); assert!( !finished, "only one of the inputs should have been signed so far" ); - let finished = wallet2.sign(&mut psbt, None).unwrap(); + let finished = wallet2.sign(&mut psbt, Default::default()).unwrap(); assert!(finished, "all the inputs should have been signed now"); } @@ -3468,7 +3481,7 @@ mod test { .drain_wallet(); let (mut psbt, _) = builder.finish().unwrap(); - let finalized = wallet.sign(&mut psbt, None).unwrap(); + let finalized = wallet.sign(&mut psbt, Default::default()).unwrap(); assert_eq!(finalized, true); let extracted = psbt.extract_tx(); @@ -3485,7 +3498,7 @@ mod test { .drain_wallet(); let (mut psbt, _) = builder.finish().unwrap(); - let finalized = wallet.sign(&mut psbt, None).unwrap(); + let finalized = wallet.sign(&mut psbt, Default::default()).unwrap(); assert_eq!(finalized, true); let extracted = psbt.extract_tx(); @@ -3502,7 +3515,7 @@ mod test { .drain_wallet(); let (mut psbt, _) = builder.finish().unwrap(); - let finalized = wallet.sign(&mut psbt, None).unwrap(); + let finalized = wallet.sign(&mut psbt, Default::default()).unwrap(); assert_eq!(finalized, true); let extracted = psbt.extract_tx(); @@ -3519,7 +3532,7 @@ mod test { .drain_wallet(); let (mut psbt, _) = builder.finish().unwrap(); - let finalized = wallet.sign(&mut psbt, None).unwrap(); + let finalized = wallet.sign(&mut psbt, Default::default()).unwrap(); assert_eq!(finalized, true); let extracted = psbt.extract_tx(); @@ -3537,7 +3550,7 @@ mod test { .drain_wallet(); let (mut psbt, _) = builder.finish().unwrap(); - let finalized = wallet.sign(&mut psbt, None).unwrap(); + let finalized = wallet.sign(&mut psbt, Default::default()).unwrap(); assert_eq!(finalized, true); let extracted = psbt.extract_tx(); @@ -3557,7 +3570,7 @@ mod test { psbt.inputs[0].bip32_derivation.clear(); assert_eq!(psbt.inputs[0].bip32_derivation.len(), 0); - let finalized = wallet.sign(&mut psbt, None).unwrap(); + let finalized = wallet.sign(&mut psbt, Default::default()).unwrap(); assert_eq!(finalized, true); let extracted = psbt.extract_tx(); @@ -3606,7 +3619,7 @@ mod test { psbt.inputs.push(dud_input); psbt.global.unsigned_tx.input.push(bitcoin::TxIn::default()); - let is_final = wallet.sign(&mut psbt, None).unwrap(); + let is_final = wallet.sign(&mut psbt, Default::default()).unwrap(); assert!( !is_final, "shouldn't be final since we can't sign one of the inputs" diff --git a/src/wallet/signer.rs b/src/wallet/signer.rs index 45ab6bf4..3198a61f 100644 --- a/src/wallet/signer.rs +++ b/src/wallet/signer.rs @@ -427,6 +427,43 @@ impl SignersContainer { } } +/// Options for a software signer +/// +/// Adjust the behavior of our software signers and the way a transaction is finalized +#[derive(Debug, Clone)] +pub struct SignOptions { + /// Whether the signer should trust the `witness_utxo`, if the `non_witness_utxo` hasn't been + /// provided + /// + /// Defaults to `false` to mitigate the "SegWit bug" which chould trick the wallet into + /// paying a fee larger than expected. + /// + /// Some wallets, especially if relatively old, might not provide the `non_witness_utxo` for + /// SegWit transactions in the PSBT they generate: in those cases setting this to `true` + /// should correctly produce a signature, at the expense of an increased trust in the creator + /// of the PSBT. + /// + /// For more details see: + pub trust_witness_utxo: bool, + + /// Whether the wallet should assume a specific height has been reached when trying to finalize + /// a transaction + /// + /// The wallet will only "use" a timelock to satisfy the spending policy of an input if the + /// timelock height has already been reached. This option allows overriding the "current height" to let the + /// wallet use timelocks in the future to spend a coin. + pub assume_height: Option, +} + +impl Default for SignOptions { + fn default() -> Self { + SignOptions { + trust_witness_utxo: false, + assume_height: None, + } + } +} + pub(crate) trait ComputeSighash { fn sighash( psbt: &psbt::PartiallySignedTransaction, @@ -492,20 +529,37 @@ impl ComputeSighash for Segwitv0 { } let psbt_input = &psbt.inputs[input_index]; + let tx_input = &psbt.global.unsigned_tx.input[input_index]; let sighash = psbt_input.sighash_type.unwrap_or(SigHashType::All); - let witness_utxo = psbt_input - .witness_utxo - .as_ref() - .ok_or(SignerError::MissingWitnessUtxo)?; - let value = witness_utxo.value; + // Always try first with the non-witness utxo + let utxo = if let Some(prev_tx) = &psbt_input.non_witness_utxo { + // Check the provided prev-tx + if prev_tx.txid() != tx_input.previous_output.txid { + return Err(SignerError::InvalidNonWitnessUtxo); + } + + // The output should be present, if it's missing the `non_witness_utxo` is invalid + prev_tx + .output + .get(tx_input.previous_output.vout as usize) + .ok_or(SignerError::InvalidNonWitnessUtxo)? + } else if let Some(witness_utxo) = &psbt_input.witness_utxo { + // Fallback to the witness_utxo. If we aren't allowed to use it, signing should fail + // before we get to this point + witness_utxo + } else { + // Nothing has been provided + return Err(SignerError::MissingNonWitnessUtxo); + }; + let value = utxo.value; let script = match psbt_input.witness_script { Some(ref witness_script) => witness_script.clone(), None => { - if witness_utxo.script_pubkey.is_v0_p2wpkh() { - p2wpkh_script_code(&witness_utxo.script_pubkey) + if utxo.script_pubkey.is_v0_p2wpkh() { + p2wpkh_script_code(&utxo.script_pubkey) } else if psbt_input .redeem_script .as_ref() diff --git a/testutils-macros/src/lib.rs b/testutils-macros/src/lib.rs index 2d42db14..db01e1ed 100644 --- a/testutils-macros/src/lib.rs +++ b/testutils-macros/src/lib.rs @@ -298,7 +298,7 @@ pub fn bdk_blockchain_tests(attr: TokenStream, item: TokenStream) -> TokenStream let mut builder = wallet.build_tx(); builder.add_recipient(node_addr.script_pubkey(), 25_000); let (mut psbt, details) = builder.finish().unwrap(); - let finalized = wallet.sign(&mut psbt, None).unwrap(); + let finalized = wallet.sign(&mut psbt, Default::default()).unwrap(); assert!(finalized, "Cannot finalize transaction"); let tx = psbt.extract_tx(); println!("{}", bitcoin::consensus::encode::serialize_hex(&tx)); @@ -327,7 +327,7 @@ pub fn bdk_blockchain_tests(attr: TokenStream, item: TokenStream) -> TokenStream let mut builder = wallet.build_tx(); builder.add_recipient(node_addr.script_pubkey(), 25_000); let (mut psbt, details) = builder.finish().unwrap(); - let finalized = wallet.sign(&mut psbt, None).unwrap(); + let finalized = wallet.sign(&mut psbt, Default::default()).unwrap(); assert!(finalized, "Cannot finalize transaction"); let sent_txid = wallet.broadcast(psbt.extract_tx()).unwrap(); @@ -368,7 +368,7 @@ pub fn bdk_blockchain_tests(attr: TokenStream, item: TokenStream) -> TokenStream let mut builder = wallet.build_tx(); builder.add_recipient(node_addr.script_pubkey(), 5_000); let (mut psbt, details) = builder.finish().unwrap(); - let finalized = wallet.sign(&mut psbt, None).unwrap(); + let finalized = wallet.sign(&mut psbt, Default::default()).unwrap(); assert!(finalized, "Cannot finalize transaction"); wallet.broadcast(psbt.extract_tx()).unwrap(); @@ -402,7 +402,7 @@ pub fn bdk_blockchain_tests(attr: TokenStream, item: TokenStream) -> TokenStream let mut builder = wallet.build_tx(); builder.add_recipient(node_addr.script_pubkey().clone(), 5_000).enable_rbf(); let (mut psbt, details) = builder.finish().unwrap(); - let finalized = wallet.sign(&mut psbt, None).unwrap(); + let finalized = wallet.sign(&mut psbt, Default::default()).unwrap(); assert!(finalized, "Cannot finalize transaction"); wallet.broadcast(psbt.extract_tx()).unwrap(); wallet.sync(noop_progress(), None).unwrap(); @@ -412,7 +412,7 @@ pub fn bdk_blockchain_tests(attr: TokenStream, item: TokenStream) -> TokenStream let mut builder = wallet.build_fee_bump(details.txid).unwrap(); builder.fee_rate(FeeRate::from_sat_per_vb(2.1)); let (mut new_psbt, new_details) = builder.finish().unwrap(); - let finalized = wallet.sign(&mut new_psbt, None).unwrap(); + let finalized = wallet.sign(&mut new_psbt, Default::default()).unwrap(); assert!(finalized, "Cannot finalize transaction"); wallet.broadcast(new_psbt.extract_tx()).unwrap(); wallet.sync(noop_progress(), None).unwrap(); @@ -438,7 +438,7 @@ pub fn bdk_blockchain_tests(attr: TokenStream, item: TokenStream) -> TokenStream let mut builder = wallet.build_tx(); builder.add_recipient(node_addr.script_pubkey().clone(), 49_000).enable_rbf(); let (mut psbt, details) = builder.finish().unwrap(); - let finalized = wallet.sign(&mut psbt, None).unwrap(); + let finalized = wallet.sign(&mut psbt, Default::default()).unwrap(); assert!(finalized, "Cannot finalize transaction"); wallet.broadcast(psbt.extract_tx()).unwrap(); wallet.sync(noop_progress(), None).unwrap(); @@ -448,7 +448,7 @@ pub fn bdk_blockchain_tests(attr: TokenStream, item: TokenStream) -> TokenStream let mut builder = wallet.build_fee_bump(details.txid).unwrap(); builder.fee_rate(FeeRate::from_sat_per_vb(5.0)); let (mut new_psbt, new_details) = builder.finish().unwrap(); - let finalized = wallet.sign(&mut new_psbt, None).unwrap(); + let finalized = wallet.sign(&mut new_psbt, Default::default()).unwrap(); assert!(finalized, "Cannot finalize transaction"); wallet.broadcast(new_psbt.extract_tx()).unwrap(); wallet.sync(noop_progress(), None).unwrap(); @@ -474,7 +474,7 @@ pub fn bdk_blockchain_tests(attr: TokenStream, item: TokenStream) -> TokenStream let mut builder = wallet.build_tx(); builder.add_recipient(node_addr.script_pubkey().clone(), 49_000).enable_rbf(); let (mut psbt, details) = builder.finish().unwrap(); - let finalized = wallet.sign(&mut psbt, None).unwrap(); + let finalized = wallet.sign(&mut psbt, Default::default()).unwrap(); assert!(finalized, "Cannot finalize transaction"); wallet.broadcast(psbt.extract_tx()).unwrap(); wallet.sync(noop_progress(), None).unwrap(); @@ -484,7 +484,7 @@ pub fn bdk_blockchain_tests(attr: TokenStream, item: TokenStream) -> TokenStream let mut builder = wallet.build_fee_bump(details.txid).unwrap(); builder.fee_rate(FeeRate::from_sat_per_vb(10.0)); let (mut new_psbt, new_details) = builder.finish().unwrap(); - let finalized = wallet.sign(&mut new_psbt, None).unwrap(); + let finalized = wallet.sign(&mut new_psbt, Default::default()).unwrap(); assert!(finalized, "Cannot finalize transaction"); wallet.broadcast(new_psbt.extract_tx()).unwrap(); wallet.sync(noop_progress(), None).unwrap(); @@ -508,7 +508,7 @@ pub fn bdk_blockchain_tests(attr: TokenStream, item: TokenStream) -> TokenStream let mut builder = wallet.build_tx(); builder.add_recipient(node_addr.script_pubkey().clone(), 49_000).enable_rbf(); let (mut psbt, details) = builder.finish().unwrap(); - let finalized = wallet.sign(&mut psbt, None).unwrap(); + let finalized = wallet.sign(&mut psbt, Default::default()).unwrap(); assert!(finalized, "Cannot finalize transaction"); wallet.broadcast(psbt.extract_tx()).unwrap(); wallet.sync(noop_progress(), None).unwrap(); @@ -520,7 +520,7 @@ pub fn bdk_blockchain_tests(attr: TokenStream, item: TokenStream) -> TokenStream let (mut new_psbt, new_details) = builder.finish().unwrap(); println!("{:#?}", new_details); - let finalized = wallet.sign(&mut new_psbt, None).unwrap(); + let finalized = wallet.sign(&mut new_psbt, Default::default()).unwrap(); assert!(finalized, "Cannot finalize transaction"); wallet.broadcast(new_psbt.extract_tx()).unwrap(); wallet.sync(noop_progress(), None).unwrap();