Merge #817: Bitcoin state poller: a bug fix and a few RBF handling im…

…provements

317ab96 bitcoin: add a comment about the new spend detection logic (Antoine Poinsot)
6daf7ac poller: don't check spending status of expired coins (Antoine Poinsot)
0e5634c bitcoin: poller: document where RBF is handled for spends (Antoine Poinsot)
cf7c4fb bitcoin: drop spend txid for coins whose spending tx gets RBF'd (Antoine Poinsot)
544167d bitcoin: mark coins whose spending tx got double spent as unspent again (Antoine Poinsot)
f78e831 db: make it possible to mark coins back as unspent (Antoine Poinsot)
c30bc8c bitcoin: optimize spend conflict confirmation lookup (Antoine Poinsot)
bc25add bitcoin: don't assign incorrect spend_txid on conflict tx confirmation (Antoine Poinsot)
20ab309 qa: add a test describing current poller behaviour wrt replacements (Antoine Poinsot)

Pull request description:

  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.

ACKs for top commit:
  jp1ac4:
    ACK 317ab96.

Tree-SHA512: bfad864cf03947e5d42894de12ae281a8cff10964d299df5b6b74310efbe6bbefb347349b2d421f1fb3e9470fa929fd7c2c221c9f161dd602757f75b66355ea0

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)
     }
 

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);

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)
     }

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 {

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();

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
+    )