diff --git a/src/bitcoin/mod.rs b/src/bitcoin/mod.rs index ed2143b98..f9369d0e4 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, + ); /// Get the common ancestor between the Bitcoin backend's tip and the given tip. fn common_ancestor(&self, tip: &BlockChainTip) -> Option; @@ -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, + ) { // 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 { @@ -372,7 +414,10 @@ impl BitcoinInterface for sync::Arc> fn spent_coins( &self, outpoints: &[(bitcoin::OutPoint, bitcoin::Txid)], - ) -> Vec<(bitcoin::OutPoint, bitcoin::Txid, Block)> { + ) -> ( + Vec<(bitcoin::OutPoint, bitcoin::Txid, Block)>, + Vec, + ) { self.lock().unwrap().spent_coins(outpoints) } diff --git a/src/bitcoin/poller/looper.rs b/src/bitcoin/poller/looper.rs index 0d64eabef..0b984d86c 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, pub spending: Vec<(bitcoin::OutPoint, bitcoin::Txid)>, + pub expired_spending: Vec, 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 = 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 6d275a96d..375d8ad07 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 07d5f69cd..b51b6a306 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, + ) { + 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 = 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 = 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 { diff --git a/src/testutils.rs b/src/testutils.rs index ffc75a64c..4f0505b3c 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, + ) { + (Vec::new(), Vec::new()) } fn common_ancestor(&self, _: &BlockChainTip) -> Option { @@ -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 b94485611..42c607882 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 + )