nik/liana
1
0
Fork 0
Code Issues Pull Requests Actions Packages Projects Releases Wiki Activity

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

Browse Source 
317ab964f707f049e51bbb4be7ae32975ca3da45 bitcoin: add a comment about the new spend detection logic (Antoine Poinsot)
6daf7ac2603a4a4f588e6106d3c4398228113b97 poller: don't check spending status of expired coins (Antoine Poinsot)
0e5634ce5986d70ab703f639882e35bba6ee571f bitcoin: poller: document where RBF is handled for spends (Antoine Poinsot)
cf7c4fbac96c421c311fd86255cdea26394954e3 bitcoin: drop spend txid for coins whose spending tx gets RBF'd (Antoine Poinsot)
544167dee4c631b324848d9f558ba48504582018 bitcoin: mark coins whose spending tx got double spent as unspent again (Antoine Poinsot)
f78e831c39311cd5d758ea81d6e855a85d13fa5c db: make it possible to mark coins back as unspent (Antoine Poinsot)
c30bc8cd18ac77f64179a8794a26770c68eb4916 bitcoin: optimize spend conflict confirmation lookup (Antoine Poinsot)
bc25addf342d5f570eb20f97b0413bb7109f0be9 bitcoin: don't assign incorrect spend_txid on conflict tx confirmation (Antoine Poinsot)
20ab30924fbdf62bb3886b2da75fe8c57555d2b6 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 317ab964f7.

Tree-SHA512: bfad864cf03947e5d42894de12ae281a8cff10964d299df5b6b74310efbe6bbefb347349b2d421f1fb3e9470fa929fd7c2c221c9f161dd602757f75b66355ea0
This commit is contained in:
Antoine Poinsot 2023-11-17 14:25:18 +01:00
commit 38b851e188
No known key found for this signature in database
GPG Key ID: E13FC145CD3F4304
6 changed files with 230 additions and 28 deletions
Show Stats Download Patch File Download Diff File Expand all files Collapse all files

67
src/bitcoin/mod.rs
View File

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

19
src/bitcoin/poller/looper.rs
View File

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

7
src/database/mod.rs
View File

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

60
src/database/sqlite/mod.rs
View File

@ -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 will see it as 'spending'
// 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());
// 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 {

17
src/testutils.rs
View File

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

88
tests/test_chain.py
View File

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