From ce026a62e6bf064cea249f391e4ab870f1cbcb61 Mon Sep 17 00:00:00 2001 From: Antoine Poinsot Date: Wed, 3 Jan 2024 18:31:48 +0100 Subject: [PATCH] Update rust-bitcoin and rust-miniscript The most notable change is rust-bitcoin's change in the serialization of transaction with no input. It now accounts for the segwit marker even for those. The base tx weight in coin selection had to be adapted to handle this. See https://gnusha.org/bitcoin-rust/2024-01-04.log for details. --- Cargo.lock | 63 +++++++++++++++++++++++-------- Cargo.toml | 2 +- src/commands/mod.rs | 71 +++++++++++++++++++---------------- src/database/mod.rs | 2 +- src/database/sqlite/mod.rs | 2 +- src/database/sqlite/schema.rs | 4 +- src/descriptors/analysis.rs | 10 +++-- src/descriptors/mod.rs | 4 +- src/jsonrpc/api.rs | 2 +- src/signer.rs | 33 ++++++++-------- src/spend.rs | 46 ++++++++++++++--------- src/testutils.rs | 4 +- 12 files changed, 144 insertions(+), 99 deletions(-) diff --git a/Cargo.lock b/Cargo.lock index 02b8fc58..7d057c72 100644 --- a/Cargo.lock +++ b/Cargo.lock @@ -56,6 +56,12 @@ version = "0.13.1" source = "registry+https://github.com/rust-lang/crates.io-index" checksum = "9e1b586273c5702936fe7b7d6896644d8be71e6314cfe09d3167c95f712589e8" +[[package]] +name = "base64" +version = "0.21.5" +source = "registry+https://github.com/rust-lang/crates.io-index" +checksum = "35636a1494ede3b646cc98f74f8e62c773a38a659ebc777a2cf26b9b74171df9" + [[package]] name = "bdk_coin_select" version = "0.1.1" @@ -64,9 +70,9 @@ checksum = "c0320167c3655e83f0415d52f39618902e449186ffc7dfb090f922f79675c316" [[package]] name = "bech32" -version = "0.9.1" +version = "0.10.0-beta" source = "registry+https://github.com/rust-lang/crates.io-index" -checksum = "d86b93f97252c47b41663388e6d155714a9d0c398b99f1005cbc5f978b29f445" +checksum = "98f7eed2b2781a6f0b5c903471d48e15f56fb4e1165df8a9a2337fd1a59d45ea" [[package]] name = "bip39" @@ -81,19 +87,29 @@ dependencies = [ [[package]] name = "bitcoin" -version = "0.30.2" +version = "0.31.0" source = "registry+https://github.com/rust-lang/crates.io-index" -checksum = "1945a5048598e4189e239d3f809b19bdad4845c4b2ba400d304d2dcf26d2c462" +checksum = "5973a027b341b462105675962214dfe3c938ad9afd395d84b28602608bdcec7b" dependencies = [ - "base64", + "base64 0.21.5", "bech32", - "bitcoin-private", - "bitcoin_hashes 0.12.0", + "bitcoin-internals", + "bitcoin_hashes 0.13.0", + "hex-conservative", "hex_lit", "secp256k1", "serde", ] +[[package]] +name = "bitcoin-internals" +version = "0.2.0" +source = "registry+https://github.com/rust-lang/crates.io-index" +checksum = "9425c3bf7089c983facbae04de54513cce73b41c7f9ff8c845b54e7bc64ebbfb" +dependencies = [ + "serde", +] + [[package]] name = "bitcoin-private" version = "0.1.0" @@ -113,6 +129,16 @@ source = "registry+https://github.com/rust-lang/crates.io-index" checksum = "5d7066118b13d4b20b23645932dfb3a81ce7e29f95726c2036fa33cd7b092501" dependencies = [ "bitcoin-private", +] + +[[package]] +name = "bitcoin_hashes" +version = "0.13.0" +source = "registry+https://github.com/rust-lang/crates.io-index" +checksum = "1930a4dabfebb8d7d9992db18ebe3ae2876f0a305fab206fd168df931ede293b" +dependencies = [ + "bitcoin-internals", + "hex-conservative", "serde", ] @@ -221,6 +247,12 @@ dependencies = [ "hashbrown", ] +[[package]] +name = "hex-conservative" +version = "0.1.1" +source = "registry+https://github.com/rust-lang/crates.io-index" +checksum = "30ed443af458ccb6d81c1e7e661545f94d3176752fb1df2f543b902a1e0f51e2" + [[package]] name = "hex_lit" version = "0.1.1" @@ -239,7 +271,7 @@ version = "0.17.0" source = "registry+https://github.com/rust-lang/crates.io-index" checksum = "a26d9104d516092f092d97448787505881fdb6518293b2d6500bf9c180c839dd" dependencies = [ - "base64", + "base64 0.13.1", "minreq", "serde", "serde_json", @@ -308,12 +340,13 @@ checksum = "523dc4f511e55ab87b694dc30d0f820d60906ef06413f93d4d7a1385599cc149" [[package]] name = "miniscript" -version = "10.0.0" +version = "11.0.0" source = "registry+https://github.com/rust-lang/crates.io-index" -checksum = "1eb102b66b2127a872dbcc73095b7b47aeb9d92f7b03c2b2298253ffc82c7594" +checksum = "86a23dd3ad145a980e231185d114399f25a0a307d2cd918010ddda6334323df9" dependencies = [ + "bech32", "bitcoin", - "bitcoin-private", + "bitcoin-internals", "serde", ] @@ -448,9 +481,9 @@ checksum = "f98d2aa92eebf49b69786be48e4477826b256916e84a57ff2a4f21923b48eb4c" [[package]] name = "secp256k1" -version = "0.27.0" +version = "0.28.0" source = "registry+https://github.com/rust-lang/crates.io-index" -checksum = "25996b82292a7a57ed3508f052cfff8640d38d32018784acd714758b43da9c8f" +checksum = "2acea373acb8c21ecb5a23741452acd2593ed44ee3d343e72baaa143bc89d0d5" dependencies = [ "bitcoin_hashes 0.12.0", "secp256k1-sys", @@ -459,9 +492,9 @@ dependencies = [ [[package]] name = "secp256k1-sys" -version = "0.8.1" +version = "0.9.1" source = "registry+https://github.com/rust-lang/crates.io-index" -checksum = "70a129b9e9efbfb223753b9163c4ab3b13cff7fd9c7f010fbac25ab4099fa07e" +checksum = "4dd97a086ec737e30053fd5c46f097465d25bb81dd3608825f65298c4c98be83" dependencies = [ "cc", ] diff --git a/Cargo.toml b/Cargo.toml index cd63dd83..21c21949 100644 --- a/Cargo.toml +++ b/Cargo.toml @@ -26,7 +26,7 @@ nonblocking_shutdown = [] [dependencies] # For managing transactions (it re-exports the bitcoin crate) -miniscript = { version = "10.0", features = ["serde", "compiler", "base64"] } +miniscript = { version = "11.0", features = ["serde", "compiler", "base64"] } # Coin selection algorithms for spend transaction creation. bdk_coin_select = { version = "0.1.0" } diff --git a/src/commands/mod.rs b/src/commands/mod.rs index 12343d37..83d8ec2c 100644 --- a/src/commands/mod.rs +++ b/src/commands/mod.rs @@ -29,7 +29,7 @@ use std::{ }; use miniscript::{ - bitcoin::{self, address, bip32, psbt::PartiallySignedTransaction as Psbt}, + bitcoin::{self, address, bip32, psbt::Psbt}, psbt::PsbtExt, }; use serde::{Deserialize, Serialize}; @@ -609,7 +609,8 @@ impl DaemonControl { // Then, broadcast it (or try to, we never know if we are not going to hit an // error at broadcast time). - let final_tx = spend_psbt.extract_tx(); + // These checks are already performed at Spend creation time. TODO: a belt-and-suspenders is still worth it though. + let final_tx = spend_psbt.extract_tx_unchecked_fee_rate(); self.bitcoin .broadcast_tx(&final_tx) .map_err(CommandError::TxBroadcast) @@ -729,7 +730,7 @@ impl DaemonControl { .expect("address already used in finalized transaction"); ( address.clone(), - bitcoin::Amount::from_sat(txo.value), + txo.value, db_conn.derivation_index_by_address(&address), ) }) @@ -1138,9 +1139,9 @@ mod tests { use bitcoin::{ bip32::{self, ChildNumber}, - blockdata::transaction::{TxIn, TxOut}, + blockdata::transaction::{TxIn, TxOut, Version as TxVersion}, locktime::absolute, - OutPoint, ScriptBuf, Sequence, Transaction, Txid, Witness, + Amount, OutPoint, ScriptBuf, Sequence, Transaction, Txid, Witness, }; use std::{collections::BTreeMap, str::FromStr}; @@ -1300,7 +1301,7 @@ mod tests { dummy_op.txid, ( bitcoin::Transaction { - version: 2, + version: TxVersion::TWO, lock_time: absolute::LockTime::Blocks(absolute::Height::ZERO), input: vec![], output: vec![], @@ -1371,18 +1372,24 @@ mod tests { assert_eq!(tx.output.len(), 2); assert_eq!( tx.output[0].script_pubkey, - dummy_addr.payload.script_pubkey() + dummy_addr.payload().script_pubkey() ); - assert_eq!(tx.output[0].value, dummy_value); + assert_eq!(tx.output[0].value.to_sat(), dummy_value); + + // NOTE: if you are wondering about the usefulness of these tests asserting arbitrary fixed + // values, that's a belt-and-suspenders check to make sure size and fee calculations do not + // change unexpectedly. For instance this specific test caught how a change in + // rust-bitcoin's serialization of transactions with no input silently affected our fee + // calculation. // Transaction is 1 in (P2WSH satisfaction), 2 outs. At 1sat/vb, it's 170 sats fees. // At 2sats/vb, it's twice that. - assert_eq!(tx.output[1].value, 89_830); + assert_eq!(tx.output[1].value.to_sat(), 89_830); let res = control .create_spend(&destinations, &[dummy_op], 2, None) .unwrap(); let tx = res.psbt.unsigned_tx; - assert_eq!(tx.output[1].value, 89_660); + assert_eq!(tx.output[1].value.to_sat(), 89_660); // A feerate of 555 won't trigger the sanity checks (they were previously not taking the // satisfaction size into account and overestimating the feerate). @@ -1414,7 +1421,7 @@ mod tests { // If we ask to create an output for an address from another network, it will fail. let invalid_addr = - bitcoin::Address::new(bitcoin::Network::Testnet, dummy_addr.payload.clone()); + bitcoin::Address::new(bitcoin::Network::Testnet, dummy_addr.payload().clone()); let invalid_destinations: HashMap, u64> = [(invalid_addr, dummy_value)].iter().cloned().collect(); assert!(matches!( @@ -1436,9 +1443,9 @@ mod tests { assert_eq!(tx.output.len(), 1); assert_eq!( tx.output[0].script_pubkey, - dummy_addr.payload.script_pubkey() + dummy_addr.payload().script_pubkey() ); - assert_eq!(tx.output[0].value, 95_000); + assert_eq!(tx.output[0].value.to_sat(), 95_000); // Now if we mark the coin as spent, we won't create another Spend transaction containing // it. @@ -1555,9 +1562,9 @@ mod tests { assert_eq!(tx_auto.output.len(), 2); assert_eq!( tx_auto.output[0].script_pubkey, - dummy_addr.payload.script_pubkey() + dummy_addr.payload().script_pubkey() ); - assert_eq!(tx_auto.output[0].value, 80_000); + assert_eq!(tx_auto.output[0].value, Amount::from_sat(80_000)); // Create a second transaction using manual coin selection. let res_manual = control @@ -1648,7 +1655,7 @@ mod tests { .unwrap(); let mut dummy_bitcoind = DummyBitcoind::new(); let dummy_tx = bitcoin::Transaction { - version: 2, + version: TxVersion::TWO, lock_time: absolute::LockTime::Blocks(absolute::Height::ZERO), input: vec![], output: vec![], @@ -1770,7 +1777,7 @@ mod tests { let mut dummy_bitcoind = DummyBitcoind::new(); // Transaction spends outpoint a. let dummy_tx_a = bitcoin::Transaction { - version: 2, + version: TxVersion::TWO, lock_time: absolute::LockTime::Blocks(absolute::Height::ZERO), input: vec![bitcoin::TxIn { previous_output: dummy_op_a, @@ -1845,7 +1852,7 @@ mod tests { ); let deposit1: Transaction = Transaction { - version: 1, + version: TxVersion::ONE, lock_time: absolute::LockTime::Blocks(absolute::Height::from_consensus(1).unwrap()), input: vec![TxIn { witness: Witness::new(), @@ -1855,12 +1862,12 @@ mod tests { }], output: vec![TxOut { script_pubkey: ScriptBuf::new(), - value: 100_000_000, + value: Amount::from_sat(100_000_000), }], }; let deposit2: Transaction = Transaction { - version: 1, + version: TxVersion::ONE, lock_time: absolute::LockTime::Blocks(absolute::Height::from_consensus(1).unwrap()), input: vec![TxIn { witness: Witness::new(), @@ -1870,12 +1877,12 @@ mod tests { }], output: vec![TxOut { script_pubkey: ScriptBuf::new(), - value: 2000, + value: Amount::from_sat(2000), }], }; let deposit3: Transaction = Transaction { - version: 1, + version: TxVersion::ONE, lock_time: absolute::LockTime::Blocks(absolute::Height::from_consensus(1).unwrap()), input: vec![TxIn { witness: Witness::new(), @@ -1885,12 +1892,12 @@ mod tests { }], output: vec![TxOut { script_pubkey: ScriptBuf::new(), - value: 3000, + value: Amount::from_sat(3000), }], }; let spend_tx: Transaction = Transaction { - version: 1, + version: TxVersion::ONE, lock_time: absolute::LockTime::Blocks(absolute::Height::from_consensus(1).unwrap()), input: vec![TxIn { witness: Witness::new(), @@ -1904,11 +1911,11 @@ mod tests { output: vec![ TxOut { script_pubkey: ScriptBuf::new(), - value: 4000, + value: Amount::from_sat(4000), }, TxOut { script_pubkey: ScriptBuf::new(), - value: 100_000_000 - 4000 - 1000, + value: Amount::from_sat(100_000_000 - 4000 - 1000), }, ], }; @@ -2072,7 +2079,7 @@ mod tests { ); let tx1: Transaction = Transaction { - version: 1, + version: TxVersion::ONE, lock_time: absolute::LockTime::Blocks(absolute::Height::from_consensus(1).unwrap()), input: vec![TxIn { witness: Witness::new(), @@ -2082,12 +2089,12 @@ mod tests { }], output: vec![TxOut { script_pubkey: ScriptBuf::new(), - value: 100_000_000, + value: Amount::from_sat(100_000_000), }], }; let tx2: Transaction = Transaction { - version: 1, + version: TxVersion::ONE, lock_time: absolute::LockTime::Blocks(absolute::Height::from_consensus(1).unwrap()), input: vec![TxIn { witness: Witness::new(), @@ -2097,12 +2104,12 @@ mod tests { }], output: vec![TxOut { script_pubkey: ScriptBuf::new(), - value: 2000, + value: Amount::from_sat(2000), }], }; let tx3: Transaction = Transaction { - version: 1, + version: TxVersion::ONE, lock_time: absolute::LockTime::Blocks(absolute::Height::from_consensus(1).unwrap()), input: vec![TxIn { witness: Witness::new(), @@ -2112,7 +2119,7 @@ mod tests { }], output: vec![TxOut { script_pubkey: ScriptBuf::new(), - value: 3000, + value: Amount::from_sat(3000), }], }; diff --git a/src/database/mod.rs b/src/database/mod.rs index 375d8ad0..9c2be7c6 100644 --- a/src/database/mod.rs +++ b/src/database/mod.rs @@ -20,7 +20,7 @@ use std::{ sync, }; -use miniscript::bitcoin::{self, bip32, psbt::PartiallySignedTransaction as Psbt, secp256k1}; +use miniscript::bitcoin::{self, bip32, psbt::Psbt, secp256k1}; pub trait DatabaseInterface: Send { fn connection(&self) -> Box; diff --git a/src/database/sqlite/mod.rs b/src/database/sqlite/mod.rs index b51b6a30..3654cc4b 100644 --- a/src/database/sqlite/mod.rs +++ b/src/database/sqlite/mod.rs @@ -39,7 +39,7 @@ use miniscript::bitcoin::{ self, bip32, consensus::encode, hashes::{sha256, Hash}, - psbt::PartiallySignedTransaction as Psbt, + psbt::Psbt, secp256k1, }; diff --git a/src/database/sqlite/schema.rs b/src/database/sqlite/schema.rs index 7ebb280b..6add975a 100644 --- a/src/database/sqlite/schema.rs +++ b/src/database/sqlite/schema.rs @@ -2,9 +2,7 @@ use crate::descriptors::LianaDescriptor; use std::{convert::TryFrom, str::FromStr}; -use miniscript::bitcoin::{ - self, address, bip32, consensus::encode, psbt::PartiallySignedTransaction as Psbt, -}; +use miniscript::bitcoin::{self, address, bip32, consensus::encode, psbt::Psbt}; pub const SCHEMA: &str = "\ CREATE TABLE version ( diff --git a/src/descriptors/analysis.rs b/src/descriptors/analysis.rs index b106a068..499a4d7e 100644 --- a/src/descriptors/analysis.rs +++ b/src/descriptors/analysis.rs @@ -72,7 +72,7 @@ fn is_single_key_or_multisig(policy: &SemanticPolicy, + keys_set: HashSet<(bip32::Xpub, descriptor::DerivPaths)>, } impl DescKeyChecker { @@ -347,7 +347,9 @@ impl PathInfo { PathInfo::Single(key) => ConcretePolicy::Key(key), PathInfo::Multi(thresh, keys) => ConcretePolicy::Threshold( thresh, - keys.into_iter().map(ConcretePolicy::Key).collect(), + keys.into_iter() + .map(|key| ConcretePolicy::Key(key).into()) + .collect(), ), } } @@ -531,9 +533,9 @@ impl LianaPolicy { .fold(primary_keys, |tl_policy, (timelock, path_info)| { let timelock = ConcretePolicy::Older(Sequence::from_height(timelock)); let keys = path_info.into_ms_policy(); - let recovery_branch = ConcretePolicy::And(vec![keys, timelock]); + let recovery_branch = ConcretePolicy::And(vec![keys.into(), timelock.into()]); // We assume the larger the timelock the less likely a branch would be used. - ConcretePolicy::Or(vec![(99, tl_policy), (1, recovery_branch)]) + ConcretePolicy::Or(vec![(99, tl_policy.into()), (1, recovery_branch.into())]) }); tl_policy diff --git a/src/descriptors/mod.rs b/src/descriptors/mod.rs index b822ac82..06f46b79 100644 --- a/src/descriptors/mod.rs +++ b/src/descriptors/mod.rs @@ -786,10 +786,10 @@ mod tests { // NOTE: The specific value is asserted because this was tested against a regtest // transaction. let stack = vec![vec![0; 65], vec![0; 0], vec![0; 33], vec![0; 72]]; - let witness_size = bitcoin::VarInt(stack.len() as u64).len() + let witness_size = bitcoin::VarInt(stack.len() as u64).size() + stack .iter() - .map(|item| bitcoin::VarInt(item.len() as u64).len() + item.len()) + .map(|item| bitcoin::VarInt(item.len() as u64).size() + item.len()) .sum::(); assert_eq!( desc.spender_input_size(), diff --git a/src/jsonrpc/api.rs b/src/jsonrpc/api.rs index 403563cf..f99ee9ef 100644 --- a/src/jsonrpc/api.rs +++ b/src/jsonrpc/api.rs @@ -10,7 +10,7 @@ use std::{ str::FromStr, }; -use miniscript::bitcoin::{self, psbt::PartiallySignedTransaction as Psbt}; +use miniscript::bitcoin::{self, psbt::Psbt}; fn create_spend(control: &DaemonControl, params: Params) -> Result { let destinations = params diff --git a/src/signer.rs b/src/signer.rs index 495a6bb5..e074e233 100644 --- a/src/signer.rs +++ b/src/signer.rs @@ -58,7 +58,7 @@ pub const MNEMONICS_FOLDER_NAME: &str = "mnemonics"; /// A signer that keeps the key on the laptop. Based on BIP39. pub struct HotSigner { mnemonic: bip39::Mnemonic, - master_xpriv: bip32::ExtendedPrivKey, + master_xpriv: bip32::Xpriv, } // TODO: instead of copying them here we could have a util module with those helpers. @@ -103,8 +103,8 @@ impl HotSigner { network: bitcoin::Network, mnemonic: bip39::Mnemonic, ) -> Result { - let master_xpriv = bip32::ExtendedPrivKey::new_master(network, &mnemonic.to_seed("")) - .map_err(SignerError::Bip32)?; + let master_xpriv = + bip32::Xpriv::new_master(network, &mnemonic.to_seed("")).map_err(SignerError::Bip32)?; Ok(Self { mnemonic, master_xpriv, @@ -212,7 +212,7 @@ impl HotSigner { &self, der_path: &bip32::DerivationPath, secp: &secp256k1::Secp256k1, - ) -> bip32::ExtendedPrivKey { + ) -> bip32::Xpriv { self.master_xpriv .derive_priv(secp, der_path) .expect("Never fails") @@ -223,9 +223,9 @@ impl HotSigner { &self, der_path: &bip32::DerivationPath, secp: &secp256k1::Secp256k1, - ) -> bip32::ExtendedPubKey { + ) -> bip32::Xpub { let xpriv = self.xpriv_at(der_path, secp); - bip32::ExtendedPubKey::from_priv(secp, &xpriv) + bip32::Xpub::from_priv(secp, &xpriv) } /// Sign all inputs of the given PSBT. @@ -254,9 +254,9 @@ impl HotSigner { .value; let sig_type = sighash::EcdsaSighashType::All; let sighash = sighash_cache - .segwit_signature_hash(i, witscript, value, sig_type) + .p2wsh_signature_hash(i, witscript, value, sig_type) .map_err(|_| SignerError::InsanePsbt)?; - let sighash = secp256k1::Message::from_slice(sighash.as_byte_array()) + let sighash = secp256k1::Message::from_digest_slice(sighash.as_byte_array()) .expect("Sighash is always 32 bytes."); // Then provide a signature for all the keys they asked for. @@ -297,7 +297,7 @@ mod tests { use super::*; use crate::{descriptors, testutils::*}; use miniscript::{ - bitcoin::{locktime::absolute, psbt::Input as PsbtIn}, + bitcoin::{locktime::absolute, psbt::Input as PsbtIn, Amount}, descriptor::{DerivPaths, DescriptorMultiXKey, DescriptorPublicKey, Wildcard}, }; use std::collections::{BTreeMap, HashSet}; @@ -429,7 +429,7 @@ mod tests { let spent_coin_desc = desc.receive_descriptor().derive(42.into(), &secp); let mut dummy_psbt = Psbt { unsigned_tx: bitcoin::Transaction { - version: 2, + version: bitcoin::transaction::Version::TWO, lock_time: absolute::LockTime::Blocks(absolute::Height::ZERO), input: vec![bitcoin::TxIn { sequence: bitcoin::Sequence::ENABLE_RBF_NO_LOCKTIME, @@ -440,12 +440,12 @@ mod tests { ..bitcoin::TxIn::default() }], output: vec![bitcoin::TxOut { - value: 18_420, + value: Amount::from_sat(18_420), script_pubkey: bitcoin::Address::from_str( "bc1qvklensptw5lk7d470ds60pcpsr0psdpgyvwepv", ) .unwrap() - .payload + .payload() .script_pubkey(), }], }, @@ -457,7 +457,7 @@ mod tests { witness_script: Some(spent_coin_desc.witness_script()), bip32_derivation: spent_coin_desc.bip32_derivations(), witness_utxo: Some(bitcoin::TxOut { - value: 19_000, + value: Amount::from_sat(19_000), script_pubkey: spent_coin_desc.script_pubkey(), }), ..PsbtIn::default() @@ -479,10 +479,7 @@ mod tests { // We can add another external output to the transaction, we can still sign without issue. // The output can be insane, we don't check it. It doesn't even need an accompanying PSBT // output. - dummy_psbt - .unsigned_tx - .output - .push(bitcoin::TxOut::default()); + dummy_psbt.unsigned_tx.output.push(bitcoin::TxOut::NULL); let psbt = dummy_psbt.clone(); assert!(psbt.inputs[0].partial_sigs.is_empty()); let psbt = prim_signer_a.sign_psbt(psbt, &secp).unwrap(); @@ -499,7 +496,7 @@ mod tests { witness_script: Some(other_spent_coin_desc.witness_script()), bip32_derivation: other_spent_coin_desc.bip32_derivations(), witness_utxo: Some(bitcoin::TxOut { - value: 19_000, + value: Amount::from_sat(19_000), script_pubkey: other_spent_coin_desc.script_pubkey(), }), ..PsbtIn::default() diff --git a/src/spend.rs b/src/spend.rs index 9a308521..091839f7 100644 --- a/src/spend.rs +++ b/src/spend.rs @@ -23,7 +23,7 @@ pub const DUST_OUTPUT_SATS: u64 = 5_000; pub const LONG_TERM_FEERATE_VB: f32 = 10.0; /// Assume that paying more than 1BTC in fee is a bug. -pub const MAX_FEE: u64 = bitcoin::blockdata::constants::COIN_VALUE; +pub const MAX_FEE: bitcoin::Amount = bitcoin::Amount::ONE_BTC; /// Assume that paying more than 1000sat/vb in feerate is a bug. pub const MAX_FEERATE: u64 = 1_000; @@ -53,7 +53,7 @@ impl fmt::Display for SpendCreationError { Self::InvalidOutputValue(amount) => write!(f, "Invalid output value '{}'.", amount), Self::InsaneFees(info) => write!( f, - "We assume transactions with a fee larger than {} sats or a feerate larger than {} sats/vb are a mistake. \ + "We assume transactions with a fee larger than {} or a feerate larger than {} sats/vb are a mistake. \ The created transaction {}.", MAX_FEE, MAX_FEERATE, @@ -81,10 +81,7 @@ impl std::error::Error for SpendCreationError {} // Sanity check the value of a transaction output. fn check_output_value(value: bitcoin::Amount) -> Result<(), SpendCreationError> { - // NOTE: the network parameter isn't used upstream - if value.to_sat() > bitcoin::blockdata::constants::MAX_MONEY - || value.to_sat() < DUST_OUTPUT_SATS - { + if value > bitcoin::Amount::MAX_MONEY || value.to_sat() < DUST_OUTPUT_SATS { Err(SpendCreationError::InvalidOutputValue(value)) } else { Ok(()) @@ -118,15 +115,16 @@ fn sanity_check_psbt( .witness_utxo .as_ref() .ok_or_else(|| SpendCreationError::SanityCheckFailure(psbt.clone()))? - .value; + .value + .to_sat(); } // Compute the output value and check the absolute fee isn't insane. - let value_out: u64 = tx.output.iter().map(|o| o.value).sum(); + let value_out: u64 = tx.output.iter().map(|o| o.value.to_sat()).sum(); let abs_fee = value_in .checked_sub(value_out) .ok_or(SpendCreationError::InsaneFees(InsaneFeeInfo::NegativeFee))?; - if abs_fee > MAX_FEE { + if abs_fee > MAX_FEE.to_sat() { return Err(SpendCreationError::InsaneFees(InsaneFeeInfo::TooHighFee( abs_fee, ))); @@ -147,7 +145,7 @@ fn sanity_check_psbt( // Check for dust outputs for txo in psbt.unsigned_tx.output.iter() { - if txo.value < txo.script_pubkey.dust_value().to_sat() { + if txo.value < txo.script_pubkey.dust_value() { return Err(SpendCreationError::SanityCheckFailure(psbt.clone())); } } @@ -257,16 +255,25 @@ fn select_coins_for_spend( max_sat_weight: u32, must_have_change: bool, ) -> Result<(Vec, bitcoin::Amount), InsufficientFunds> { - let out_value_nochange = base_tx.output.iter().map(|o| o.value).sum(); + let out_value_nochange = base_tx.output.iter().map(|o| o.value.to_sat()).sum(); // Create the coin selector from the given candidates. NOTE: the coin selector keeps track // of the original ordering of candidates so we can select any mandatory candidates using their // original indices. - let base_weight: u32 = base_tx + let mut base_weight: u32 = base_tx .weight() .to_wu() .try_into() .expect("Transaction weight must fit in u32"); + // Starting with version 0.31, rust-bitcoin now accounts for the segwit marker when serializing + // transactions with no input. But BDK's coin selector does add the segwit marker cost to the + // transaction size upon selecting the first segwit coin. To avoid accounting twice for it, + // drop it from the base weight (but only when it was added). + // NOTE: make sure to reconsider this when updating rust-bitcoin!! Behaviour may change again + // who knows. + if base_tx.input.is_empty() { + base_weight = base_weight.saturating_sub(2); + } let max_input_weight = TXIN_BASE_WEIGHT + max_sat_weight; let candidates: Vec = candidate_coins .iter() @@ -293,12 +300,15 @@ fn select_coins_for_spend( let long_term_feerate = FeeRate::from_sat_per_vb(LONG_TERM_FEERATE_VB); let drain_weights = DrainWeights { output_weight: { + // We don't reuse the above base_weight.since 2 WU may have been substracted from it. + // See comment above for details. + let nochange_weight = base_tx.weight().to_wu(); let mut tx_with_change = base_tx; tx_with_change.output.push(change_txo); tx_with_change .weight() .to_wu() - .checked_sub(base_weight.into()) + .checked_sub(nochange_weight) .expect("base_weight can't be larger") .try_into() .expect("tx size must always fit in u32") @@ -481,7 +491,7 @@ pub fn create_spend( // Create transaction with no inputs and no outputs. let mut tx = bitcoin::Transaction { - version: 2, + version: bitcoin::transaction::Version::TWO, lock_time: LockTime::Blocks(Height::ZERO), // TODO: randomized anti fee sniping input: Vec::with_capacity(candidate_coins.iter().filter(|c| c.must_select).count()), output: Vec::with_capacity(destinations.len()), @@ -493,7 +503,7 @@ pub fn create_spend( check_output_value(*amount)?; tx.output.push(bitcoin::TxOut { - value: amount.to_sat(), + value: *amount, script_pubkey: address.addr.script_pubkey(), }); // If it's an address of ours, signal it as change to signing devices by adding the @@ -520,7 +530,7 @@ pub fn create_spend( // We need to get the size of a potential change output to select coins / determine whether // we should include one, so get the change address and create a dummy txo for this purpose. let mut change_txo = bitcoin::TxOut { - value: std::u64::MAX, + value: bitcoin::Amount::MAX, script_pubkey: change_addr.addr.script_pubkey(), }; // Now select the coins necessary using the provided candidates and determine whether @@ -575,7 +585,7 @@ pub fn create_spend( }; // TODO: shuffle once we have Taproot - change_txo.value = change_amount.to_sat(); + change_txo.value = change_amount; tx.output.push(change_txo); psbt_outs.push(PsbtOut { bip32_derivation, @@ -600,7 +610,7 @@ pub fn create_spend( let coin_desc = derived_desc(secp, main_descriptor, cand); let witness_script = Some(coin_desc.witness_script()); let witness_utxo = Some(bitcoin::TxOut { - value: cand.amount.to_sat(), + value: cand.amount, script_pubkey: coin_desc.script_pubkey(), }); let non_witness_utxo = tx_getter.get_tx(&cand.outpoint.txid); diff --git a/src/testutils.rs b/src/testutils.rs index 3bd663b0..c175c156 100644 --- a/src/testutils.rs +++ b/src/testutils.rs @@ -13,9 +13,7 @@ use std::{ }; use miniscript::{ - bitcoin::{ - self, bip32, psbt::PartiallySignedTransaction as Psbt, secp256k1, Transaction, Txid, - }, + bitcoin::{self, bip32, psbt::Psbt, secp256k1, Transaction, Txid}, descriptor, };