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 <[email protected]>
comit-network · Aug 16, 2021 · b2c3770 · b2c3770
2 parents 4405dbc + 0296509
commit b2c3770
Show file tree

Hide file tree

Showing 9 changed files with 171 additions and 52 deletions.
diff --git 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
diff --git 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"
@@ -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/proptest-regressions/bitcoin/wallet.txt 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<Normal,Public,NonZero>(0299a8c6a662e2e9e8ee7c6889b75a51c432812b4bf70c1d76eace63abc1bdfb1b), bob = Point<Normal,Public,NonZero>(027165b1f9924030c90d38c511da0f4397766078687997ed34d6ef2743d2a7bbed)
diff --git 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
@@ -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 {
@@ -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.

diff --git 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),
@@ -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 {
-        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
+        }
     }
 
-    /// 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_fees(self, 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();
-        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,23 +1086,23 @@ 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);
     }
 
     #[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);
     }
 
     #[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");
+            });
+        }
+    }
 }