From 148fdb8d0a45ce929f14c8c0f22e8506142758c4 Mon Sep 17 00:00:00 2001 From: Thomas Eizinger Date: Thu, 12 Aug 2021 11:28:00 +1000 Subject: [PATCH 1/4] Ensure the size of our locking script never changes --- swap/Cargo.toml | 1 + swap/src/bitcoin/lock.rs | 22 ++++++++++++++-------- swap/src/lib.rs | 3 +++ swap/src/proptest.rs | 17 +++++++++++++++++ 4 files changed, 35 insertions(+), 8 deletions(-) create mode 100644 swap/src/proptest.rs diff --git a/swap/Cargo.toml b/swap/Cargo.toml index 92eeb2b8..727f8e65 100644 --- a/swap/Cargo.toml +++ b/swap/Cargo.toml @@ -80,6 +80,7 @@ get-port = "3" hyper = "0.14" monero-harness = { path = "../monero-harness" } port_check = "0.1" +proptest = "1" serde_cbor = "0.11" spectral = "0.6" tempfile = "3" diff --git a/swap/src/bitcoin/lock.rs b/swap/src/bitcoin/lock.rs index b1b2be0e..2f82ff77 100644 --- a/swap/src/bitcoin/lock.rs +++ b/swap/src/bitcoin/lock.rs @@ -7,11 +7,11 @@ use ::bitcoin::{OutPoint, TxIn, TxOut, Txid}; use anyhow::{bail, Result}; use bdk::database::BatchDatabase; use bitcoin::Script; -use ecdsa_fun::fun::Point; use miniscript::{Descriptor, DescriptorTrait}; -use rand::thread_rng; use serde::{Deserialize, Serialize}; +const SCRIPT_SIZE: usize = 34; + #[derive(Debug, Clone, Serialize, Deserialize, PartialEq)] pub struct TxLock { inner: PartiallySignedTransaction, @@ -112,12 +112,7 @@ impl TxLock { /// Calculate the size of the script used by this transaction. pub fn script_size() -> usize { - build_shared_output_descriptor( - Point::random(&mut thread_rng()), - Point::random(&mut thread_rng()), - ) - .script_pubkey() - .len() + SCRIPT_SIZE } pub fn script_pubkey(&self) -> Script { @@ -245,6 +240,17 @@ mod tests { result.expect_err("PSBT to be invalid"); } + proptest::proptest! { + #[test] + fn estimated_tx_lock_script_size_never_changes(a in crate::proptest::ecdsa_fun::point(), b in crate::proptest::ecdsa_fun::point()) { + proptest::prop_assume!(a != b); + + let computed_size = build_shared_output_descriptor(a, b).script_pubkey().len(); + + assert_eq!(computed_size, SCRIPT_SIZE); + } + } + /// Helper function that represents Bob's action of constructing the PSBT. /// /// Extracting this allows us to keep the tests concise. diff --git a/swap/src/lib.rs b/swap/src/lib.rs index 64c8ffe9..1b050ae2 100644 --- a/swap/src/lib.rs +++ b/swap/src/lib.rs @@ -32,3 +32,6 @@ pub mod tor; pub mod tracing_ext; mod monero_ext; + +#[cfg(test)] +mod proptest; diff --git a/swap/src/proptest.rs b/swap/src/proptest.rs new file mode 100644 index 00000000..0c7ee7b3 --- /dev/null +++ b/swap/src/proptest.rs @@ -0,0 +1,17 @@ +use proptest::prelude::*; + +pub mod ecdsa_fun { + use super::*; + use ::ecdsa_fun::fun::marker::{Mark, NonZero, Normal}; + use ::ecdsa_fun::fun::{Point, Scalar, G}; + + pub fn point() -> impl Strategy { + scalar().prop_map(|mut scalar| Point::from_scalar_mul(&G, &mut scalar).mark::()) + } + + pub fn scalar() -> impl Strategy { + prop::array::uniform32(0..255u8).prop_filter_map("generated the 0 element", |bytes| { + Scalar::from_bytes_mod_order(bytes).mark::() + }) + } +} From e4b5e28a93a5b3590de375a7e7b79477c879b49f Mon Sep 17 00:00:00 2001 From: Thomas Eizinger Date: Thu, 12 Aug 2021 18:08:06 +1000 Subject: [PATCH 2/4] Introduce WalletBuilder for creating test instances of wallet --- swap/src/bitcoin.rs | 7 ++- swap/src/bitcoin/lock.rs | 9 ++-- swap/src/bitcoin/wallet.rs | 90 ++++++++++++++++++++++++++------------ 3 files changed, 73 insertions(+), 33 deletions(-) diff --git a/swap/src/bitcoin.rs b/swap/src/bitcoin.rs index c44facdd..09ba21cd 100644 --- a/swap/src/bitcoin.rs +++ b/swap/src/bitcoin.rs @@ -21,6 +21,9 @@ pub use ecdsa_fun::fun::Scalar; pub use ecdsa_fun::Signature; pub use wallet::Wallet; +#[cfg(test)] +pub use wallet::WalletBuilder; + use crate::bitcoin::wallet::ScriptStatus; use ::bitcoin::hashes::hex::ToHex; use ::bitcoin::hashes::Hash; @@ -317,8 +320,8 @@ mod tests { #[tokio::test] async fn calculate_transaction_weights() { - let alice_wallet = Wallet::new_funded_default_fees(Amount::ONE_BTC.as_sat()); - let bob_wallet = Wallet::new_funded_default_fees(Amount::ONE_BTC.as_sat()); + let alice_wallet = WalletBuilder::new(Amount::ONE_BTC.as_sat()).build(); + let bob_wallet = WalletBuilder::new(Amount::ONE_BTC.as_sat()).build(); let spending_fee = Amount::from_sat(1_000); let btc_amount = Amount::from_sat(500_000); let xmr_amount = crate::monero::Amount::from_piconero(10000); diff --git a/swap/src/bitcoin/lock.rs b/swap/src/bitcoin/lock.rs index 2f82ff77..559ef1e4 100644 --- a/swap/src/bitcoin/lock.rs +++ b/swap/src/bitcoin/lock.rs @@ -183,11 +183,12 @@ impl Watchable for TxLock { mod tests { use super::*; use crate::bitcoin::wallet::StaticFeeRate; + use crate::bitcoin::WalletBuilder; #[tokio::test] async fn given_bob_sends_good_psbt_when_reconstructing_then_succeeeds() { let (A, B) = alice_and_bob(); - let wallet = Wallet::new_funded_default_fees(50000); + let wallet = WalletBuilder::new(50_000).build(); let agreed_amount = Amount::from_sat(10000); let psbt = bob_make_psbt(A, B, &wallet, agreed_amount).await; @@ -201,7 +202,7 @@ mod tests { let (A, B) = alice_and_bob(); let fees = 610; let agreed_amount = Amount::from_sat(10000); - let wallet = Wallet::new_funded_default_fees(agreed_amount.as_sat() + fees); + let wallet = WalletBuilder::new(agreed_amount.as_sat() + fees).build(); let psbt = bob_make_psbt(A, B, &wallet, agreed_amount).await; assert_eq!( @@ -217,7 +218,7 @@ mod tests { #[tokio::test] async fn given_bob_is_sending_less_than_agreed_when_reconstructing_txlock_then_fails() { let (A, B) = alice_and_bob(); - let wallet = Wallet::new_funded_default_fees(50000); + let wallet = WalletBuilder::new(50_000).build(); let agreed_amount = Amount::from_sat(10000); let bad_amount = Amount::from_sat(5000); @@ -230,7 +231,7 @@ mod tests { #[tokio::test] async fn given_bob_is_sending_to_a_bad_output_reconstructing_txlock_then_fails() { let (A, B) = alice_and_bob(); - let wallet = Wallet::new_funded_default_fees(50000); + let wallet = WalletBuilder::new(50_000).build(); let agreed_amount = Amount::from_sat(10000); let E = eve(); diff --git a/swap/src/bitcoin/wallet.rs b/swap/src/bitcoin/wallet.rs index a45e1a84..b544e650 100644 --- a/swap/src/bitcoin/wallet.rs +++ b/swap/src/bitcoin/wallet.rs @@ -547,48 +547,84 @@ impl EstimateFeeRate for StaticFeeRate { } #[cfg(test)] -impl Wallet<(), bdk::database::MemoryDatabase, StaticFeeRate> { +pub struct WalletBuilder { + utxo_amount: u64, + sats_per_vb: f32, + min_relay_fee_sats: u64, + key: bitcoin::util::bip32::ExtendedPrivKey, + num_utxos: u8, +} + +#[cfg(test)] +impl WalletBuilder { /// Creates a new, funded wallet with sane default fees. /// /// Unless you are testing things related to fees, this is likely what you /// want. - pub fn new_funded_default_fees(amount: u64) -> Self { - Self::new_funded(amount, 1.0, 1000) + pub fn new(amount: u64) -> Self { + WalletBuilder { + utxo_amount: amount, + sats_per_vb: 1.0, + min_relay_fee_sats: 1000, + key: "tprv8ZgxMBicQKsPeZRHk4rTG6orPS2CRNFX3njhUXx5vj9qGog5ZMH4uGReDWN5kCkY3jmWEtWause41CDvBRXD1shKknAMKxT99o9qUTRVC6m".parse().unwrap(), + num_utxos: 1, + } } - /// Creates a new, funded wallet that doesn't pay any fees. - /// - /// This will create invalid transactions but can be useful if you want full - /// control over the output amounts. - pub fn new_funded_zero_fees(amount: u64) -> Self { - Self::new_funded(amount, 0.0, 0) + pub fn with_zero_fees(self) -> Self { + Self { + sats_per_vb: 0.0, + min_relay_fee_sats: 0, + ..self + } + } + + pub fn with_fees(self, sats_per_vb: f32, min_relay_fee_sats: u64) -> Self { + Self { + sats_per_vb, + min_relay_fee_sats, + ..self + } } - /// Creates a new, funded wallet to be used within tests. - pub fn new_funded(amount: u64, sats_per_vb: f32, min_relay_fee_sats: u64) -> Self { + pub fn with_key(self, key: bitcoin::util::bip32::ExtendedPrivKey) -> Self { + Self { key, ..self } + } + + pub fn with_num_utxos(self, number: u8) -> Self { + Self { + num_utxos: number, + ..self + } + } + + pub fn build(self) -> Wallet<(), bdk::database::MemoryDatabase, StaticFeeRate> { use bdk::database::MemoryDatabase; use bdk::{LocalUtxo, TransactionDetails}; use bitcoin::OutPoint; use testutils::testutils; - let descriptors = testutils!(@descriptors ("wpkh(tprv8ZgxMBicQKsPeZRHk4rTG6orPS2CRNFX3njhUXx5vj9qGog5ZMH4uGReDWN5kCkY3jmWEtWause41CDvBRXD1shKknAMKxT99o9qUTRVC6m/*)")); + let descriptors = testutils!(@descriptors (&format!("wpkh({}/*)", self.key))); let mut database = MemoryDatabase::new(); - bdk::populate_test_db!( - &mut database, - testutils! { - @tx ( (@external descriptors, 0) => amount ) (@confirmations 1) - }, - Some(100) - ); + + for index in 0..self.num_utxos { + bdk::populate_test_db!( + &mut database, + testutils! { + @tx ( (@external descriptors, index as u32) => self.utxo_amount ) (@confirmations 1) + }, + Some(100) + ); + } let wallet = bdk::Wallet::new_offline(&descriptors.0, None, Network::Regtest, database).unwrap(); - Self { + Wallet { client: Arc::new(Mutex::new(StaticFeeRate { - fee_rate: FeeRate::from_sat_per_vb(sats_per_vb), - min_relay_fee: bitcoin::Amount::from_sat(min_relay_fee_sats), + fee_rate: FeeRate::from_sat_per_vb(self.sats_per_vb), + min_relay_fee: bitcoin::Amount::from_sat(self.min_relay_fee_sats), })), wallet: Arc::new(Mutex::new(wallet)), finality_confirmations: 1, @@ -1047,7 +1083,7 @@ mod tests { #[tokio::test] async fn given_no_balance_returns_amount_0() { - let wallet = Wallet::new_funded(0, 1.0, 1); + let wallet = WalletBuilder::new(0).with_fees(1.0, 1).build(); let amount = wallet.max_giveable(TxLock::script_size()).await.unwrap(); assert_eq!(amount, Amount::ZERO); @@ -1055,7 +1091,7 @@ mod tests { #[tokio::test] async fn given_balance_below_min_relay_fee_returns_amount_0() { - let wallet = Wallet::new_funded(1000, 1.0, 1001); + let wallet = WalletBuilder::new(1000).with_fees(1.0, 1001).build(); let amount = wallet.max_giveable(TxLock::script_size()).await.unwrap(); assert_eq!(amount, Amount::ZERO); @@ -1063,7 +1099,7 @@ mod tests { #[tokio::test] async fn given_balance_above_relay_fee_returns_amount_greater_0() { - let wallet = Wallet::new_funded_default_fees(10_000); + let wallet = WalletBuilder::new(10_000).build(); let amount = wallet.max_giveable(TxLock::script_size()).await.unwrap(); assert!(amount.as_sat() > 0); @@ -1083,7 +1119,7 @@ mod tests { let balance = 2000; // We don't care about fees in this test, thus use a zero fee rate - let wallet = Wallet::new_funded_zero_fees(balance); + let wallet = WalletBuilder::new(balance).with_zero_fees().build(); // sorting is only relevant for amounts that have a change output // if the change output is below dust it will be dropped by the BDK @@ -1108,7 +1144,7 @@ mod tests { #[tokio::test] async fn can_override_change_address() { - let wallet = Wallet::new_funded_default_fees(50_000); + let wallet = WalletBuilder::new(50_000).build(); let custom_change = "bcrt1q08pfqpsyrt7acllzyjm8q5qsz5capvyahm49rw" .parse::
() .unwrap(); From 475057abda233d7599cb1cbc375b4788402f0098 Mon Sep 17 00:00:00 2001 From: Thomas Eizinger Date: Thu, 12 Aug 2021 18:12:40 +1000 Subject: [PATCH 3/4] Add proptest for max_giveable and signing PSBT --- swap/proptest-regressions/bitcoin/wallet.txt | 7 +++++++ swap/src/bitcoin/wallet.rs | 17 +++++++++++++++++ swap/src/proptest.rs | 12 ++++++++++++ 3 files changed, 36 insertions(+) create mode 100644 swap/proptest-regressions/bitcoin/wallet.txt diff --git a/swap/proptest-regressions/bitcoin/wallet.txt b/swap/proptest-regressions/bitcoin/wallet.txt new file mode 100644 index 00000000..843cefb4 --- /dev/null +++ b/swap/proptest-regressions/bitcoin/wallet.txt @@ -0,0 +1,7 @@ +# Seeds for failure cases proptest has generated in the past. It is +# automatically read and these particular cases re-run before any +# novel cases are generated. +# +# It is recommended to check this file in to source control so that +# everyone who runs the test benefits from these saved cases. +cc 849f8b01f49fc9a913100203698a9151d8de8a37564e1d3b1e3b4169e192f58a # shrinks to funding_amount = 290250686, num_utxos = 3, sats_per_vb = 75.35638, key = ExtendedPrivKey { network: Regtest, depth: 0, parent_fingerprint: 00000000, child_number: Normal { index: 0 }, private_key: [private key data], chain_code: 0b7a29ca6990bbc9b9187c1d1a07e2cf68e32f5ce55d2df01edf8a4ac2ee2a4b }, alice = Point(0299a8c6a662e2e9e8ee7c6889b75a51c432812b4bf70c1d76eace63abc1bdfb1b), bob = Point(027165b1f9924030c90d38c511da0f4397766078687997ed34d6ef2743d2a7bbed) diff --git a/swap/src/bitcoin/wallet.rs b/swap/src/bitcoin/wallet.rs index b544e650..82be3dc6 100644 --- a/swap/src/bitcoin/wallet.rs +++ b/swap/src/bitcoin/wallet.rs @@ -1201,4 +1201,21 @@ DEBUG swap::bitcoin::wallet: Bitcoin transaction status changed txid=00000000000 fn confs(confirmations: u32) -> ScriptStatus { ScriptStatus::from_confirmations(confirmations) } + + proptest::proptest! { + #[test] + fn funding_never_fails_with_insufficient_funds(funding_amount in 3000u32.., num_utxos in 1..5u8, sats_per_vb in 1.0..500.0f32, key in crate::proptest::bitcoin::extended_priv_key(), alice in crate::proptest::ecdsa_fun::point(), bob in crate::proptest::ecdsa_fun::point()) { + proptest::prop_assume!(alice != bob); + + tokio::runtime::Runtime::new().unwrap().block_on(async move { + let wallet = WalletBuilder::new(funding_amount as u64).with_key(key).with_num_utxos(num_utxos).with_fees(sats_per_vb, 1000).build(); + + let amount = wallet.max_giveable(TxLock::script_size()).await.unwrap(); + let psbt: PartiallySignedTransaction = TxLock::new(&wallet, amount, PublicKey::from(alice), PublicKey::from(bob), wallet.new_address().await.unwrap()).await.unwrap().into(); + let result = wallet.sign_and_finalize(psbt).await; + + result.expect("transaction to be signed"); + }); + } + } } diff --git a/swap/src/proptest.rs b/swap/src/proptest.rs index 0c7ee7b3..41b28785 100644 --- a/swap/src/proptest.rs +++ b/swap/src/proptest.rs @@ -15,3 +15,15 @@ pub mod ecdsa_fun { }) } } + +pub mod bitcoin { + use super::*; + use ::bitcoin::util::bip32::ExtendedPrivKey; + use ::bitcoin::Network; + + pub fn extended_priv_key() -> impl Strategy { + prop::array::uniform8(0..255u8).prop_filter_map("invalid secret key generated", |bytes| { + ExtendedPrivKey::new_master(Network::Regtest, &bytes).ok() + }) + } +} From 02965091106f63cad63531f53589c12f4130e8da Mon Sep 17 00:00:00 2001 From: Thomas Eizinger Date: Thu, 12 Aug 2021 16:40:52 +1000 Subject: [PATCH 4/4] Upgrade to bdk 0.10 This fixes #546. I don't know why, but I can't reproduce the problem with the updated dependency. --- CHANGELOG.md | 4 ++++ Cargo.lock | 19 ++++++++++++++----- swap/Cargo.toml | 2 +- swap/src/bitcoin/wallet.rs | 13 ++++++++----- 4 files changed, 27 insertions(+), 11 deletions(-) diff --git a/CHANGELOG.md b/CHANGELOG.md index a472642b..3c113de3 100644 --- a/CHANGELOG.md +++ b/CHANGELOG.md @@ -7,6 +7,10 @@ and this project adheres to [Semantic Versioning](https://semver.org/spec/v2.0.0 ## [Unreleased] +### Fixed + +- An occasional error where users couldn't start a swap because of `InsufficientFunds` that were off by exactly 1 satoshi. + ## [0.8.0] - 2021-07-09 ### Added diff --git a/Cargo.lock b/Cargo.lock index 3fa58fb4..3799b5f0 100644 --- a/Cargo.lock +++ b/Cargo.lock @@ -235,11 +235,20 @@ version = "0.13.0" source = "registry+https://github.com/rust-lang/crates.io-index" checksum = "904dfeac50f3cdaba28fc6f57fdcddb75f49ed61346676a78c4ffe55877802fd" +[[package]] +name = "base64-compat" +version = "1.0.0" +source = "registry+https://github.com/rust-lang/crates.io-index" +checksum = "5a8d4d2746f89841e49230dd26917df1876050f95abafafbe34f47cb534b88d7" +dependencies = [ + "byteorder", +] + [[package]] name = "bdk" -version = "0.8.0" +version = "0.10.0" source = "registry+https://github.com/rust-lang/crates.io-index" -checksum = "05d7fee1aedf8935ba1e2c9aeee640d1b9754da1b64f30ad47e8b8e2b7904ec0" +checksum = "8f4da304c23a06c21807598a7fe3223566e84c76c6bba2cab2504370dd6f4938" dependencies = [ "async-trait", "bdk-macros", @@ -252,14 +261,13 @@ dependencies = [ "serde", "serde_json", "sled", - "tokio", ] [[package]] name = "bdk-macros" -version = "0.4.0" +version = "0.5.0" source = "registry+https://github.com/rust-lang/crates.io-index" -checksum = "b45570b78250774145859a8f85bfdb6e310663fc82640d7e159a44b1386074a2" +checksum = "c3f510015e946c5995cc169f7ed4c92ba032bbce795c0956ee0d98d82f7aff78" dependencies = [ "proc-macro2 1.0.27", "quote 1.0.9", @@ -331,6 +339,7 @@ version = "0.26.2" source = "registry+https://github.com/rust-lang/crates.io-index" checksum = "6742ec672d3f12506f4ac5c0d853926ff1f94e675f60ffd3224039972bf663f1" dependencies = [ + "base64-compat", "bech32", "bitcoin_hashes", "secp256k1", diff --git a/swap/Cargo.toml b/swap/Cargo.toml index 727f8e65..ff384dee 100644 --- a/swap/Cargo.toml +++ b/swap/Cargo.toml @@ -15,7 +15,7 @@ async-trait = "0.1" atty = "0.2" backoff = { version = "0.3", features = [ "tokio" ] } base64 = "0.13" -bdk = "0.8" +bdk = "0.10" big-bytes = "1" bitcoin = { version = "0.26", features = [ "rand", "use-serde" ] } bmrng = "0.5" diff --git a/swap/src/bitcoin/wallet.rs b/swap/src/bitcoin/wallet.rs index 82be3dc6..b2554e80 100644 --- a/swap/src/bitcoin/wallet.rs +++ b/swap/src/bitcoin/wallet.rs @@ -306,7 +306,8 @@ where .iter() .find(|tx| tx.txid == txid) .context("Could not find tx in bdk wallet when trying to determine fees")? - .fees; + .fee + .expect("fees are always present with Electrum backend"); Ok(Amount::from_sat(fees)) } @@ -394,14 +395,16 @@ where let mut tx_builder = wallet.build_tx(); let dummy_script = Script::from(vec![0u8; locking_script_size]); - tx_builder.set_single_recipient(dummy_script); - tx_builder.drain_wallet(); + tx_builder.drain_to(dummy_script); tx_builder.fee_rate(fee_rate); let response = tx_builder.finish(); match response { Ok((_, details)) => { - let max_giveable = details.sent - details.fees; + let max_giveable = details.sent + - details + .fee + .expect("fees are always present with Electrum backend"); Ok(Amount::from_sat(max_giveable)) } Err(bdk::Error::InsufficientFunds { .. }) => Ok(Amount::ZERO), @@ -600,7 +603,7 @@ impl WalletBuilder { pub fn build(self) -> Wallet<(), bdk::database::MemoryDatabase, StaticFeeRate> { use bdk::database::MemoryDatabase; - use bdk::{LocalUtxo, TransactionDetails}; + use bdk::{ConfirmationTime, LocalUtxo, TransactionDetails}; use bitcoin::OutPoint; use testutils::testutils;