Merge #643

643: Upgrade to latest bdk to fix occasional `InsufficientFunds` error r=thomaseizinger a=thomaseizinger

Fixes #546.
Please review patch-by-patch.

Co-authored-by: Thomas Eizinger <thomas@eizinger.io>

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

Cargo.lock

@@ -236,10 +236,19 @@ source = "registry+https://github.com/rust-lang/crates.io-index"
 checksum = "904dfeac50f3cdaba28fc6f57fdcddb75f49ed61346676a78c4ffe55877802fd"
 
 [[package]]
-name = "bdk"
+name = "base64-compat"
-version = "0.8.0"
+version = "1.0.0"
 source = "registry+https://github.com/rust-lang/crates.io-index"
-checksum = "05d7fee1aedf8935ba1e2c9aeee640d1b9754da1b64f30ad47e8b8e2b7904ec0"
+checksum = "5a8d4d2746f89841e49230dd26917df1876050f95abafafbe34f47cb534b88d7"
+dependencies = [
+ "byteorder",
+]
+
+[[package]]
+name = "bdk"
+version = "0.10.0"
+source = "registry+https://github.com/rust-lang/crates.io-index"
+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",

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"
@@ -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"

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<Normal,Public,NonZero>(0299a8c6a662e2e9e8ee7c6889b75a51c432812b4bf70c1d76eace63abc1bdfb1b), bob = Point<Normal,Public,NonZero>(027165b1f9924030c90d38c511da0f4397766078687997ed34d6ef2743d2a7bbed)

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 alice_wallet = WalletBuilder::new(Amount::ONE_BTC.as_sat()).build();
-        let bob_wallet = Wallet::new_funded_default_fees(Amount::ONE_BTC.as_sat());
+        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);

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(
+        SCRIPT_SIZE
-            Point::random(&mut thread_rng()),
-            Point::random(&mut thread_rng()),
-        )
-        .script_pubkey()
-        .len()
     }
 
     pub fn script_pubkey(&self) -> Script {
@@ -188,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;
@@ -206,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!(
@@ -222,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);
@@ -235,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();
@@ -245,6 +241,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.

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_to(dummy_script);
-        tx_builder.drain_wallet();
         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),
@@ -547,48 +550,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 {
+    pub fn new(amount: u64) -> Self {
-        Self::new_funded(amount, 1.0, 1000)
+        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.
+    pub fn with_zero_fees(self) -> Self {
-    ///
+        Self {
-    /// This will create invalid transactions but can be useful if you want full
+            sats_per_vb: 0.0,
-    /// control over the output amounts.
+            min_relay_fee_sats: 0,
-    pub fn new_funded_zero_fees(amount: u64) -> Self {
+            ..self
-        Self::new_funded(amount, 0.0, 0)
+        }
     }
 
-    /// Creates a new, funded wallet to be used within tests.
+    pub fn with_fees(self, sats_per_vb: f32, min_relay_fee_sats: u64) -> Self {
-    pub fn new_funded(amount: u64, sats_per_vb: f32, min_relay_fee_sats: u64) -> Self {
+        Self {
+            sats_per_vb,
+            min_relay_fee_sats,
+            ..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 bdk::{ConfirmationTime, 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();
+
+        for index in 0..self.num_utxos {
             bdk::populate_test_db!(
                 &mut database,
                 testutils! {
-                @tx ( (@external descriptors, 0) => amount ) (@confirmations 1)
+                    @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),
+                fee_rate: FeeRate::from_sat_per_vb(self.sats_per_vb),
-                min_relay_fee: bitcoin::Amount::from_sat(min_relay_fee_sats),
+                min_relay_fee: bitcoin::Amount::from_sat(self.min_relay_fee_sats),
             })),
             wallet: Arc::new(Mutex::new(wallet)),
             finality_confirmations: 1,
@@ -1047,7 +1086,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 +1094,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 +1102,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 +1122,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 +1147,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::<Address>()
             .unwrap();
@@ -1165,4 +1204,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");
+            });
+        }
+    }
 }

swap/src/lib.rs

@@ -32,3 +32,6 @@ pub mod tor;
 pub mod tracing_ext;
 
 mod monero_ext;
+
+#[cfg(test)]
+mod proptest;

swap/src/proptest.rs

@@ -0,0 +1,29 @@
+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<Value = Point> {
+        scalar().prop_map(|mut scalar| Point::from_scalar_mul(&G, &mut scalar).mark::<Normal>())
+    }
+
+    pub fn scalar() -> impl Strategy<Value = Scalar> {
+        prop::array::uniform32(0..255u8).prop_filter_map("generated the 0 element", |bytes| {
+            Scalar::from_bytes_mod_order(bytes).mark::<NonZero>()
+        })
+    }
+}
+
+pub mod bitcoin {
+    use super::*;
+    use ::bitcoin::util::bip32::ExtendedPrivKey;
+    use ::bitcoin::Network;
+
+    pub fn extended_priv_key() -> impl Strategy<Value = ExtendedPrivKey> {
+        prop::array::uniform8(0..255u8).prop_filter_map("invalid secret key generated", |bytes| {
+            ExtendedPrivKey::new_master(Network::Regtest, &bytes).ok()
+        })
+    }
+}