Repository: wizardsardine/liana
Pull request #817 (2023-11-16 16:56:46)

Bitcoin state poller: a bug fix and a few RBF handling improvements
We start by illustrating the current logic of the poller with regard to replacements in a functional test. This exposes a bug: we could incorrectly assign a transaction which conflicts with a spend transaction for one of our coin as spending this coin whereas it in fact didn't.

After fixing this bug, we proceed to make it possible to wipe the spending status back to unspent. First when a conflict is mined, then also when it's only accepted into our mempool. This matches how we treat replacements for deposit transactions.

darosior: Pushed a new commit to avoid checking the spending status of coins whose spending transaction was dropped from our mempool, as suggested by @jp1ac4.
darosior: Addressed your comments.
jp1ac4: Should https://github.com/wizardsardine/liana/pull/817/commits/5e43f1b0518f35f386a95b4683029a5c00e1c657 be removed?
darosior: Arf my bad. Good catch.

Patch:
diff --git a/src/bitcoin/mod.rs b/src/bitcoin/mod.rs
index ed2143b9..f9369d0e 100644
--- a/src/bitcoin/mod.rs
+++ b/src/bitcoin/mod.rs
@@ -76,11 +76,16 @@ pub trait BitcoinInterface: Send {
         outpoints: &[bitcoin::OutPoint],
     ) -> Vec<(bitcoin::OutPoint, bitcoin::Txid)>;
 
-    /// Get all coins that are spent with the final spend tx txid and blocktime.
+    /// Get all coins that are spent with the final spend tx txid and blocktime. Along with the
+    /// coins for which the spending transaction "expired" (a conflicting transaction was mined and
+    /// it wasn't spending this coin).
     fn spent_coins(
         &self,
         outpoints: &[(bitcoin::OutPoint, bitcoin::Txid)],
-    ) -> Vec<(bitcoin::OutPoint, bitcoin::Txid, Block)>;
+    ) -> (
+        Vec<(bitcoin::OutPoint, bitcoin::Txid, Block)>,
+        Vec<bitcoin::OutPoint>,
+    );
 
     /// Get the common ancestor between the Bitcoin backend's tip and the given tip.
     fn common_ancestor(&self, tip: &BlockChainTip) -> Option<BlockChainTip>;
@@ -237,9 +242,14 @@ impl BitcoinInterface for d::BitcoinD {
     fn spent_coins(
         &self,
         outpoints: &[(bitcoin::OutPoint, bitcoin::Txid)],
-    ) -> Vec<(bitcoin::OutPoint, bitcoin::Txid, Block)> {
+    ) -> (
+        Vec<(bitcoin::OutPoint, bitcoin::Txid, Block)>,
+        Vec<bitcoin::OutPoint>,
+    ) {
         // Spend coins to be returned.
         let mut spent = Vec::with_capacity(outpoints.len());
+        // Coins whose spending transaction isn't in our local mempool anymore.
+        let mut expired = Vec::new();
         // Cached calls to `gettransaction`.
         let mut tx_getter = CachedTxGetter::new(self);
 
@@ -259,16 +269,48 @@ impl BitcoinInterface for d::BitcoinD {
 
             // If a conflicting transaction was confirmed instead, replace the txid of the
             // spender for this coin with it and mark it as confirmed.
-            for txid in &res.conflicting_txs {
-                if let Some(tx) = tx_getter.get_transaction(txid) {
-                    if let Some(block) = tx.block {
-                        spent.push((*op, *txid, block))
-                    }
-                }
+            // If a conflicting transaction which doesn't spend this coin was mined or accepted in
+            // our local mempool, mark this spend as expired.
+            enum Conflict {
+                // A replacement spending transaction was confirmed.
+                Replaced((bitcoin::Txid, Block)),
+                // A transaction conflicting with the former spending transaction was confirmed or
+                // included in our local mempool.
+                Dropped,
+            }
+            let conflict = res.conflicting_txs.iter().find_map(|txid| {
+                tx_getter.get_transaction(txid).and_then(|tx| {
+                    tx.block
+                        .map(|block| {
+                            // Being part of our watchonly wallet isn't enough, as it could be a
+                            // conflicting transaction which spends a different set of coins. Make sure
+                            // it does actually spend this coin.
+                            for txin in tx.tx.input {
+                                if &txin.previous_output == op {
+                                    return Conflict::Replaced((*txid, block));
+                                }
+                            }
+                            Conflict::Dropped
+                        })
+                        .or_else(|| {
+                            // If the coin is actually being spent, but by another transaction, it
+                            // will just be set at the next poll in `spending_coins()`.
+                            if self.is_in_mempool(txid) {
+                                Some(Conflict::Dropped)
+                            } else {
+                                None
+                            }
+                        })
+                })
+            });
+            match conflict {
+                Some(Conflict::Replaced((txid, block))) => spent.push((*op, txid, block)),
+                Some(Conflict::Dropped) => expired.push(*op),
+                None => {}
             }
         }
 
-        spent
+        (spent, expired)
     }
 
     fn common_ancestor(&self, tip: &BlockChainTip) -> Option<BlockChainTip> {
@@ -372,7 +414,10 @@ impl BitcoinInterface for sync::Arc<sync::Mutex<dyn BitcoinInterface + 'static>>
     fn spent_coins(
         &self,
         outpoints: &[(bitcoin::OutPoint, bitcoin::Txid)],
-    ) -> Vec<(bitcoin::OutPoint, bitcoin::Txid, Block)> {
+    ) -> (
+        Vec<(bitcoin::OutPoint, bitcoin::Txid, Block)>,
+        Vec<bitcoin::OutPoint>,
+    ) {
         self.lock().unwrap().spent_coins(outpoints)
     }
 
diff --git a/src/bitcoin/poller/looper.rs b/src/bitcoin/poller/looper.rs
index 0d64eabe..0b984d86 100644
--- a/src/bitcoin/poller/looper.rs
+++ b/src/bitcoin/poller/looper.rs
@@ -5,6 +5,7 @@ use crate::{
 };
 
 use std::{
+    collections::HashSet,
     sync::{self, atomic},
     thread, time,
 };
@@ -17,6 +18,7 @@ struct UpdatedCoins {
     pub confirmed: Vec<(bitcoin::OutPoint, i32, u32)>,
     pub expired: Vec<bitcoin::OutPoint>,
     pub spending: Vec<(bitcoin::OutPoint, bitcoin::Txid)>,
+    pub expired_spending: Vec<bitcoin::OutPoint>,
     pub spent: Vec<(bitcoin::OutPoint, bitcoin::Txid, i32, u32)>,
 }
 
@@ -111,15 +113,20 @@ fn update_coins(
     // We need to take the newly received ones into account as well, as they may have been
     // spent within the previous tip and the current one, and we may not poll this chunk of the
     // chain anymore.
+    // NOTE: curr_coins contain the "spending" coins. So this takes care of updating the spend_txid
+    // if a coin's spending transaction gets RBF'd.
+    let expired_set: HashSet<_> = expired.iter().collect();
     let to_be_spent: Vec<bitcoin::OutPoint> = curr_coins
         .values()
         .chain(received.iter())
         .filter_map(|coin| {
             // Always check for spends when the spend tx is not confirmed as it might get RBF'd.
-            if coin.spend_txid.is_none() || coin.spend_block.is_none() {
-                Some(coin.outpoint)
-            } else {
+            if (coin.spend_txid.is_some() && coin.spend_block.is_some())
+                || expired_set.contains(&coin.outpoint)
+            {
                 None
+            } else {
+                Some(coin.outpoint)
             }
         })
         .collect();
@@ -136,8 +143,8 @@ fn update_coins(
         .map(|coin| (coin.outpoint, coin.spend_txid.expect("Coin is spending")))
         .chain(spending.iter().cloned())
         .collect();
-    let spent = bit
-        .spent_coins(spending_coins.as_slice())
+    let (spent, expired_spending) = bit.spent_coins(spending_coins.as_slice());
+    let spent = spent
         .into_iter()
         .map(|(oupoint, txid, block)| (oupoint, txid, block.height, block.time))
         .collect();
@@ -148,6 +155,7 @@ fn update_coins(
         confirmed,
         expired,
         spending,
+        expired_spending,
         spent,
     }
 }
@@ -238,6 +246,7 @@ fn updates(
     db_conn.remove_coins(&updated_coins.expired);
     db_conn.confirm_coins(&updated_coins.confirmed);
     db_conn.spend_coins(&updated_coins.spending);
+    db_conn.unspend_coins(&updated_coins.expired_spending);
     db_conn.confirm_spend(&updated_coins.spent);
     if latest_tip != current_tip {
         db_conn.update_tip(&latest_tip);
diff --git a/src/database/mod.rs b/src/database/mod.rs
index 6d275a96..375d8ad0 100644
--- a/src/database/mod.rs
+++ b/src/database/mod.rs
@@ -109,6 +109,9 @@ pub trait DatabaseConnection {
     /// Mark a set of coins as being spent by a specified txid of a pending transaction.
     fn spend_coins(&mut self, outpoints: &[(bitcoin::OutPoint, bitcoin::Txid)]);
 
+    /// Mark a set of coins as not being spent anymore.
+    fn unspend_coins(&mut self, outpoints: &[bitcoin::OutPoint]);
+
     /// Mark a set of coins as spent by a specified txid at a specified block time.
     fn confirm_spend(&mut self, outpoints: &[(bitcoin::OutPoint, bitcoin::Txid, i32, u32)]);
 
@@ -232,6 +235,10 @@ impl DatabaseConnection for SqliteConn {
         self.spend_coins(outpoints)
     }
 
+    fn unspend_coins(&mut self, outpoints: &[bitcoin::OutPoint]) {
+        self.unspend_coins(outpoints)
+    }
+
     fn confirm_spend<'a>(&mut self, outpoints: &[(bitcoin::OutPoint, bitcoin::Txid, i32, u32)]) {
         self.confirm_spend(outpoints)
     }
diff --git a/src/database/sqlite/mod.rs b/src/database/sqlite/mod.rs
index 07d5f69c..b51b6a30 100644
--- a/src/database/sqlite/mod.rs
+++ b/src/database/sqlite/mod.rs
@@ -509,6 +509,27 @@ impl SqliteConn {
         .expect("Database must be available")
     }
 
+    /// Mark a set of coins as not being spent.
+    pub fn unspend_coins<'a>(
+        &mut self,
+        outpoints: impl IntoIterator<Item = &'a bitcoin::OutPoint>,
+    ) {
+        db_exec(&mut self.conn, |db_tx| {
+            for outpoint in outpoints {
+                db_tx.execute(
+                    "UPDATE coins SET spend_txid = NULL, spend_block_height = NULL, spend_block_time = NULL WHERE txid = ?1 AND vout = ?2",
+                    rusqlite::params![
+                        outpoint.txid[..].to_vec(),
+                        outpoint.vout,
+                    ],
+                )?;
+            }
+
+            Ok(())
+        })
+        .expect("Database must be available")
+    }
+
     /// Mark the Spend transaction of a given set of coins as being confirmed at a given
     /// block.
     pub fn confirm_spend<'a>(
@@ -1350,18 +1371,27 @@ CREATE TABLE spend_transactions (
                 coin_a.outpoint,
                 bitcoin::Txid::from_slice(&[0; 32][..]).unwrap(),
             )]);
-            let coins_map: HashMap<bitcoin::OutPoint, DbCoin> = conn
-                .coins(&[], &[])
+            let coin = conn
+                .coins(&[], &[coin_a.outpoint])
                 .into_iter()
-                .map(|c| (c.outpoint, c))
-                .collect();
-            assert!(coins_map
-                .get(&coin_a.outpoint)
-                .unwrap()
-                .spend_txid
-                .is_some());
+                .next()
+                .unwrap();
+            assert!(coin.spend_txid.is_some());
+
+            // We can unspend it, if the spend transaction gets double spent.
+            conn.unspend_coins(&[coin_a.outpoint]);
+            let coin = conn
+                .coins(&[], &[coin_a.outpoint])
+                .into_iter()
+                .next()
+                .unwrap();
+            assert!(coin.spend_txid.is_none());
 
-            // We will see it as 'spending'
+            // Spend it back. We will see it as 'spending'
+            conn.spend_coins(&[(
+                coin_a.outpoint,
+                bitcoin::Txid::from_slice(&[0; 32][..]).unwrap(),
+            )]);
             let outpoints: HashSet<bitcoin::OutPoint> = conn
                 .list_spending_coins()
                 .into_iter()
@@ -1406,6 +1436,16 @@ CREATE TABLE spend_transactions (
             assert_eq!(coin.spend_block.as_ref().unwrap().time, time);
             assert_eq!(coin.spend_block.unwrap().height, height);
 
+            // If we unspend it all spend info will be wiped.
+            conn.unspend_coins(&[coin_a.outpoint]);
+            let coin = conn
+                .coins(&[], &[coin_a.outpoint])
+                .into_iter()
+                .next()
+                .unwrap();
+            assert!(coin.spend_txid.is_none());
+            assert!(coin.spend_block.is_none());
+
             // Add an immature coin. As all coins it's first registered as unconfirmed (even though
             // it's not).
             let coin_imma = Coin {


Test patch:
diff --git a/src/testutils.rs b/src/testutils.rs
index ffc75a64..4f0505b3 100644
--- a/src/testutils.rs
+++ b/src/testutils.rs
@@ -82,8 +82,11 @@ impl BitcoinInterface for DummyBitcoind {
     fn spent_coins(
         &self,
         _: &[(bitcoin::OutPoint, bitcoin::Txid)],
-    ) -> Vec<(bitcoin::OutPoint, bitcoin::Txid, Block)> {
-        Vec::new()
+    ) -> (
+        Vec<(bitcoin::OutPoint, bitcoin::Txid, Block)>,
+        Vec<bitcoin::OutPoint>,
+    ) {
+        (Vec::new(), Vec::new())
     }
 
     fn common_ancestor(&self, _: &BlockChainTip) -> Option<BlockChainTip> {
@@ -278,6 +281,16 @@ impl DatabaseConnection for DummyDatabase {
         }
     }
 
+    fn unspend_coins<'a>(&mut self, outpoints: &[bitcoin::OutPoint]) {
+        for op in outpoints {
+            let mut db = self.db.write().unwrap();
+            let spent = &mut db.coins.get_mut(op).unwrap();
+            assert!(spent.spend_txid.is_some());
+            spent.spend_txid = None;
+            spent.spend_block = None;
+        }
+    }
+
     fn confirm_spend<'a>(&mut self, outpoints: &[(bitcoin::OutPoint, bitcoin::Txid, i32, u32)]) {
         for (op, spend_txid, height, time) in outpoints {
             let mut db = self.db.write().unwrap();
diff --git a/tests/test_chain.py b/tests/test_chain.py
index b9448561..42c60788 100644
--- a/tests/test_chain.py
+++ b/tests/test_chain.py
@@ -402,3 +402,91 @@ def test_conflicting_unconfirmed_spend_txs(lianad, bitcoind):
         and get_coin(lianad, spent_coin["outpoint"])["spend_info"]["txid"]
         == txid_b.hex()
     )
+
+
+def sign_and_broadcast_psbt(lianad, psbt):
+    txid = psbt.tx.txid().hex()
+    psbt = lianad.signer.sign_psbt(psbt)
+    lianad.rpc.updatespend(psbt.to_base64())
+    lianad.rpc.broadcastspend(txid)
+    return txid
+
+
+def test_spend_replacement(lianad, bitcoind):
+    """Test we detect the new version of the unconfirmed spending transaction."""
+    # Get three coins.
+    destinations = {
+        lianad.rpc.getnewaddress()["address"]: 0.03,
+        lianad.rpc.getnewaddress()["address"]: 0.04,
+        lianad.rpc.getnewaddress()["address"]: 0.05,
+    }
+    txid = bitcoind.rpc.sendmany("", destinations)
+    bitcoind.generate_block(1, wait_for_mempool=txid)
+    wait_for(lambda: len(lianad.rpc.listcoins(["confirmed"])["coins"]) == 3)
+    coins = lianad.rpc.listcoins(["confirmed"])["coins"]
+
+    # Create three conflicting spends, the two first spend two different set of coins
+    # and the third one is just an RBF of the second one but as a send-to-self.
+    first_outpoints = [c["outpoint"] for c in coins[:2]]
+    destinations = {
+        bitcoind.rpc.getnewaddress(): 650_000,
+    }
+    first_res = lianad.rpc.createspend(destinations, first_outpoints, 1)
+    first_psbt = PSBT.from_base64(first_res["psbt"])
+    second_outpoints = [c["outpoint"] for c in coins[1:]]
+    destinations = {
+        bitcoind.rpc.getnewaddress(): 650_000,
+    }
+    second_res = lianad.rpc.createspend(destinations, second_outpoints, 2)
+    second_psbt = PSBT.from_base64(second_res["psbt"])
+    destinations = {}
+    third_res = lianad.rpc.createspend(destinations, second_outpoints, 4)
+    third_psbt = PSBT.from_base64(third_res["psbt"])
+
+    # Broadcast the first transaction. Make sure it's detected.
+    first_txid = sign_and_broadcast_psbt(lianad, first_psbt)
+    wait_for(
+        lambda: all(
+            c["spend_info"] is not None and c["spend_info"]["txid"] == first_txid
+            for c in lianad.rpc.listcoins([], first_outpoints)["coins"]
+        )
+    )
+
+    # Now RBF the first transaction by the second one. The third coin should be
+    # newly marked as spending, the second one's spend_txid should be updated and
+    # the first one's spend txid should be dropped.
+    second_txid = sign_and_broadcast_psbt(lianad, second_psbt)
+    wait_for(
+        lambda: all(
+            c["spend_info"] is not None and c["spend_info"]["txid"] == second_txid
+            for c in lianad.rpc.listcoins([], second_outpoints)["coins"]
+        )
+    )
+    wait_for(
+        lambda: lianad.rpc.listcoins([], [first_outpoints[0]])["coins"][0]["spend_info"]
+        is None
+    )
+
+    # Now RBF the second transaction with a send-to-self, just because.
+    third_txid = sign_and_broadcast_psbt(lianad, third_psbt)
+    wait_for(
+        lambda: all(
+            c["spend_info"] is not None and c["spend_info"]["txid"] == third_txid
+            for c in lianad.rpc.listcoins([], second_outpoints)["coins"]
+        )
+    )
+    assert (
+        lianad.rpc.listcoins([], [first_outpoints[0]])["coins"][0]["spend_info"] is None
+    )
+
+    # Once the RBF is mined, we detect it as confirmed and the first coin is still unspent.
+    bitcoind.generate_block(1, wait_for_mempool=third_txid)
+    wait_for(
+        lambda: all(
+            c["spend_info"] is not None and c["spend_info"]["height"] is not None
+            for c in lianad.rpc.listcoins([], second_outpoints)["coins"]
+        )
+    )
+    assert (
+        lianad.rpc.listcoins([], [first_outpoints[0]])["coins"][0]["spend_info"] is None
+    )