From bde3299db1c38eb6b0b09d7da2a73c43e40181b9 Mon Sep 17 00:00:00 2001 From: Michael Mallan Date: Tue, 19 Nov 2024 16:31:58 +0000 Subject: [PATCH 1/7] sqlite: refactor migration tests Some migration tests used structs and methods that are expected to change and so will not be backward compatible. This change replaces those with structs and methods specific to the migration tests so that the tests will be unaffected by DB schema changes. At the same time, these new structs and methods simplify some of the setup by allowing to store new coins, including their confirmation and spend status, in a single DB operation. --- lianad/src/database/sqlite/mod.rs | 255 +++++++++++++++++------------- 1 file changed, 145 insertions(+), 110 deletions(-) diff --git a/lianad/src/database/sqlite/mod.rs b/lianad/src/database/sqlite/mod.rs index 27676862..ae227a07 100644 --- a/lianad/src/database/sqlite/mod.rs +++ b/lianad/src/database/sqlite/mod.rs @@ -1114,6 +1114,65 @@ CREATE TABLE labels ( (tmp_dir, options, secp, db) } + // All values required to store a coin in the V3 schema DB (including `id` column). + #[derive(Debug, Clone, Copy, PartialEq, Eq, Hash)] + pub struct DbCoinV3 { + pub id: i64, + pub wallet_id: i64, + pub outpoint: bitcoin::OutPoint, + pub is_immature: bool, + pub block_info: Option, + pub amount: bitcoin::Amount, + pub derivation_index: bip32::ChildNumber, + pub is_change: bool, + pub spend_txid: Option, + pub spend_block: Option, + } + + // Helper to store coins in a V3 schema database (including `id` column). + fn store_coins_v3(conn: &mut SqliteConn, coins: &[DbCoinV3]) { + db_exec(&mut conn.conn, |db_tx| { + for coin in coins { + let deriv_index: u32 = coin.derivation_index.into(); + db_tx.execute( + "INSERT INTO coins ( + id, + wallet_id, + blockheight, + blocktime, + txid, + vout, + amount_sat, + derivation_index, + is_change, + spend_txid, + spend_block_height, + spend_block_time, + is_immature + ) \ + VALUES (?1, ?2, ?3, ?4, ?5, ?6, ?7, ?8, ?9, ?10, ?11, ?12, ?13)", + rusqlite::params![ + coin.id, + coin.wallet_id, + coin.block_info.map(|block| block.height), + coin.block_info.map(|block| block.time), + coin.outpoint.txid[..].to_vec(), + coin.outpoint.vout, + coin.amount.to_sat(), + deriv_index, + coin.is_change, + coin.spend_txid.map(|txid| txid[..].to_vec()), + coin.spend_block.map(|block| block.height), + coin.spend_block.map(|block| block.time), + coin.is_immature, + ], + )?; + } + Ok(()) + }) + .expect("Database must be available") + } + #[test] fn db_startup_sanity_checks() { let tmp_dir = tmp_dir(); @@ -2606,53 +2665,80 @@ CREATE TABLE labels ( }) .collect(); - // The following coins will be inserted into the DB as unconfirmed and then - // some of them will be subsequently confirmed and spent. - // Note that `block_info`, `spend_txid` and `spend_block` will all be set to - // NULL in the DB by the `new_unspent_coins` method, but are set to `None` - // here anyway. - let coin_a = Coin { + // The state of the coins will be: + // - coin_a is spent. + // - coin_b is confirmed and spending. + // - coin_c is confirmed. + // - coin_d is the unconfirmed output of coin_b's spend and is spending. + // - coin_e is the unconfirmed output of coin_d's spend. + // - coin_imma_a is confirmed. + // - coin_imma_b is still immature. + let coin_d_outpoint = bitcoin::OutPoint::new(bitcoin_txs.get(3).unwrap().txid(), 1456); + let coin_e_outpoint = bitcoin::OutPoint::new(bitcoin_txs.get(4).unwrap().txid(), 4633); + let coin_a = DbCoinV3 { + id: 1, + wallet_id: WALLET_ID, outpoint: bitcoin::OutPoint::new(bitcoin_txs.first().unwrap().txid(), 1), is_immature: false, amount: bitcoin::Amount::from_sat(1231001), derivation_index: bip32::ChildNumber::from_normal_idx(101).unwrap(), is_change: false, - block_info: None, - spend_txid: None, - spend_block: None, + block_info: Some(DbBlockInfo { + height: 175500, + time: 1755001001, + }), + spend_txid: Some(bitcoin_txs.get(7).unwrap().txid()), + spend_block: Some(DbBlockInfo { + height: 245500, + time: 1755003000, + }), }; - let coin_b = Coin { + let coin_b = DbCoinV3 { + id: 2, + wallet_id: WALLET_ID, outpoint: bitcoin::OutPoint::new(bitcoin_txs.get(1).unwrap().txid(), 19234), is_immature: false, amount: bitcoin::Amount::from_sat(23145), derivation_index: bip32::ChildNumber::from_normal_idx(10).unwrap(), is_change: false, - block_info: None, - spend_txid: None, + block_info: Some(DbBlockInfo { + height: 175502, + time: 1755001032, + }), + spend_txid: Some(coin_d_outpoint.txid), spend_block: None, }; - let coin_c = Coin { + let coin_c = DbCoinV3 { + id: 3, + wallet_id: WALLET_ID, outpoint: bitcoin::OutPoint::new(bitcoin_txs.get(2).unwrap().txid(), 932), is_immature: false, amount: bitcoin::Amount::from_sat(354764), derivation_index: bip32::ChildNumber::from_normal_idx(3401).unwrap(), is_change: true, - block_info: None, + block_info: Some(DbBlockInfo { + height: 175504, + time: 1755005032, + }), spend_txid: None, spend_block: None, }; - let coin_d = Coin { - outpoint: bitcoin::OutPoint::new(bitcoin_txs.get(3).unwrap().txid(), 1456), + let coin_d = DbCoinV3 { + id: 4, + wallet_id: WALLET_ID, + outpoint: coin_d_outpoint, is_immature: false, amount: bitcoin::Amount::from_sat(23200), derivation_index: bip32::ChildNumber::from_normal_idx(4793235).unwrap(), is_change: true, block_info: None, - spend_txid: None, + spend_txid: Some(coin_e_outpoint.txid), spend_block: None, }; - let coin_e = Coin { - outpoint: bitcoin::OutPoint::new(bitcoin_txs.get(4).unwrap().txid(), 4633), + let coin_e = DbCoinV3 { + id: 5, + wallet_id: WALLET_ID, + outpoint: coin_e_outpoint, is_immature: false, amount: bitcoin::Amount::from_sat(675000), derivation_index: bip32::ChildNumber::from_normal_idx(3).unwrap(), @@ -2661,17 +2747,24 @@ CREATE TABLE labels ( spend_txid: None, spend_block: None, }; - let coin_imma_a = Coin { + let coin_imma_a = DbCoinV3 { + id: 6, + wallet_id: WALLET_ID, outpoint: bitcoin::OutPoint::new(bitcoin_txs.get(5).unwrap().txid(), 5), is_immature: true, amount: bitcoin::Amount::from_sat(4564347), derivation_index: bip32::ChildNumber::from_normal_idx(453).unwrap(), is_change: false, - block_info: None, + block_info: Some(DbBlockInfo { + height: 176001, + time: 1755001004, + }), spend_txid: None, spend_block: None, }; - let coin_imma_b = Coin { + let coin_imma_b = DbCoinV3 { + id: 7, + wallet_id: WALLET_ID, outpoint: bitcoin::OutPoint::new(bitcoin_txs.get(6).unwrap().txid(), 19234), is_immature: true, amount: bitcoin::Amount::from_sat(731453), @@ -2681,15 +2774,7 @@ CREATE TABLE labels ( spend_txid: None, spend_block: None, }; - // After the following operations, the state of the coins will be: - // - coin_a is spent. - // - coin_b is confirmed and spending. - // - coin_c is confirmed. - // - coin_d is the unconfirmed output of coin_b's spend and is spending. - // - coin_e is the unconfirmed output of coin_d's spend. - // - coin_imma_a is confirmed. - // - coin_imma_b is still immature. - conn.new_unspent_coins(&[ + let coins_pre = vec![ coin_a, coin_b, coin_c, @@ -2697,46 +2782,8 @@ CREATE TABLE labels ( coin_e, coin_imma_a, coin_imma_b, - ]); - conn.confirm_coins(&[ - (coin_a.outpoint, 175500, 1755001001), - (coin_b.outpoint, 175502, 1755001032), - (coin_c.outpoint, 175504, 1755005032), - (coin_imma_a.outpoint, 176001, 1755001004), - ]); - conn.spend_coins(&[ - (coin_a.outpoint, bitcoin_txs.get(7).unwrap().txid()), - (coin_b.outpoint, coin_d.outpoint.txid), - (coin_d.outpoint, coin_e.outpoint.txid), - ]); - conn.confirm_spend(&[( - coin_a.outpoint, - bitcoin_txs.get(7).unwrap().txid(), - 245500, - 1755003000, - )]); - assert_eq!(conn.coins(&[CoinStatus::Unconfirmed], &[]).len(), 2); - assert_eq!(conn.coins(&[CoinStatus::Confirmed], &[]).len(), 2); - assert_eq!(conn.coins(&[CoinStatus::Spending], &[]).len(), 2); - assert_eq!(conn.coins(&[CoinStatus::Spent], &[]).len(), 1); - let coins_pre = conn.coins(&[], &[]); - assert_eq!(coins_pre.len(), 7); - assert_eq!( - coins_pre - .iter() - .filter(|c| c.is_immature) - .collect::>() - .len(), - 1 - ); - assert_eq!( - coins_pre - .iter() - .filter(|c| c.is_change) - .collect::>() - .len(), - 2 - ); + ]; + store_coins_v3(&mut conn, &coins_pre); // Migrate the DB. maybe_apply_migration(&db_path, &bitcoin_txs).unwrap(); @@ -2744,8 +2791,25 @@ CREATE TABLE labels ( // Migrating twice will be a no-op. No need to pass `bitcoin_txs` second time. maybe_apply_migration(&db_path, &[]).unwrap(); assert!(conn.db_version() == 6); + + // Compare the `DbCoin`s with the expected values. let coins_post = conn.coins(&[], &[]); - assert_eq!(coins_pre, coins_post); + assert_eq!(coins_pre.len(), coins_post.len()); + for c_post in coins_post { + let c_pre = coins_pre + .iter() + .find(|c| c.outpoint == c_post.outpoint) + .unwrap(); + assert_eq!(c_post.id, c_pre.id); + assert_eq!(c_post.wallet_id, c_pre.wallet_id); + assert_eq!(c_post.is_immature, c_pre.is_immature); + assert_eq!(c_post.block_info, c_pre.block_info); + assert_eq!(c_post.amount, c_pre.amount); + assert_eq!(c_post.derivation_index, c_pre.derivation_index); + assert_eq!(c_post.is_change, c_pre.is_change); + assert_eq!(c_post.spend_txid, c_pre.spend_txid); + assert_eq!(c_post.spend_block, c_pre.spend_block); + } } fs::remove_dir_all(tmp_dir).unwrap(); @@ -2785,7 +2849,7 @@ CREATE TABLE labels ( output: Vec::new(), }, if i % 2 == 0 { - Some(BlockInfo { + Some(DbBlockInfo { height: (i % 5) as i32 * 2_000, time: 1722488619 + (i % 5) * 84_999, }) @@ -2795,11 +2859,15 @@ CREATE TABLE labels ( ) }) .collect(); - let coins: Vec = bitcoin_txs + // We can use `DbCoinV3` to store coin in v4 database + // (fields are the same, only a constraint changed). + let coins: Vec = bitcoin_txs .iter() .chain(bitcoin_txs.iter()) // We do this to have coins which originate from the same tx. .enumerate() - .map(|(i, tx)| Coin { + .map(|(i, tx)| DbCoinV3 { + id: i.try_into().unwrap(), + wallet_id: WALLET_ID, outpoint: bitcoin::OutPoint { txid: tx.txid(), vout: i as u32, @@ -2809,7 +2877,7 @@ CREATE TABLE labels ( derivation_index: bip32::ChildNumber::from_normal_idx(i as u32 * 100).unwrap(), is_change: (i % 4) == 0, block_info: if i & 2 == 0 { - Some(BlockInfo { + Some(DbBlockInfo { height: (i % 100) as i32 * 1_000, time: 1722408619 + (i % 100) as u32 * 42_000, }) @@ -2834,40 +2902,7 @@ CREATE TABLE labels ( let mut conn = db.connection().unwrap(); // Insert all these coins into database. - conn.new_unspent_coins(&coins); - - // Confirm those which are supposed to be. - let confirmed_coins: Vec<_> = coins - .iter() - .filter_map(|coin| { - coin.block_info - .map(|blk| (coin.outpoint, blk.height, blk.time)) - }) - .collect(); - conn.confirm_coins(&confirmed_coins); - - // Spend those which are supposed to be. - let spent_coins: Vec<_> = coins - .iter() - .filter_map(|coin| coin.spend_txid.map(|txid| (coin.outpoint, txid))) - .collect(); - conn.spend_coins(&spent_coins); - - // Mark the spend as confirmed for those which are supposed to be. - let confirmed_spent_coins: Vec<_> = coins - .iter() - .filter_map(|coin| { - coin.spend_block.map(|blk| { - ( - coin.outpoint, - coin.spend_txid.expect("always set when spend block is"), - blk.height, - blk.time, - ) - }) - }) - .collect(); - conn.confirm_spend(&confirmed_spent_coins); + store_coins_v3(&mut conn, &coins); } // Trying to migrate without specifying the transactions will fail. From f2c910f6eb9f2b74329fa026c93f7e813f3baa53 Mon Sep 17 00:00:00 2001 From: Michael Mallan Date: Thu, 28 Nov 2024 08:56:08 +0000 Subject: [PATCH 2/7] sqlite: refactor test to not depend on order of coins I found that making changes to the transactions used in the test can affect the order in which coins are returned, probably due to the txid changing. --- lianad/src/database/sqlite/mod.rs | 37 ++++++++++++++++++------------- 1 file changed, 21 insertions(+), 16 deletions(-) diff --git a/lianad/src/database/sqlite/mod.rs b/lianad/src/database/sqlite/mod.rs index ae227a07..a25a9c13 100644 --- a/lianad/src/database/sqlite/mod.rs +++ b/lianad/src/database/sqlite/mod.rs @@ -1373,9 +1373,10 @@ CREATE TABLE labels ( conn.db_coins(&[outpoint_a, outpoint_b]), ] .iter() - .all(|c| c.len() == 2 - && c[0].outpoint == coin_a.outpoint - && c[1].outpoint == coin_b.outpoint)); + .all(|coins| coins.len() == 2 + && coins + .iter() + .all(|c| [coin_a.outpoint, coin_b.outpoint].contains(&c.outpoint)))); // We can filter for just the first coin. assert!([ conn.coins(&[CoinStatus::Unconfirmed], &[outpoint_a]), @@ -1425,9 +1426,10 @@ CREATE TABLE labels ( conn.db_coins(&[outpoint_a, outpoint_b]), ] .iter() - .all(|c| c.len() == 2 - && c[0].outpoint == coin_a.outpoint - && c[1].outpoint == coin_b.outpoint)); + .all(|coins| coins.len() == 2 + && coins + .iter() + .all(|c| [coin_a.outpoint, coin_b.outpoint].contains(&c.outpoint)))); // Now if we spend one, it'll be marked as such. conn.spend_coins(&[(coin_a.outpoint, txs.get(2).unwrap().txid())]); @@ -1478,9 +1480,10 @@ CREATE TABLE labels ( conn.db_coins(&[outpoint_a, outpoint_b]), ] .iter() - .all(|c| c.len() == 2 - && c[0].outpoint == coin_a.outpoint - && c[1].outpoint == coin_b.outpoint)); + .all(|coins| coins.len() == 2 + && coins + .iter() + .all(|c| [coin_a.outpoint, coin_b.outpoint].contains(&c.outpoint)))); // Add a third and fourth coin. let outpoint_c = bitcoin::OutPoint::new(txs.get(3).unwrap().txid(), 42); @@ -1518,10 +1521,11 @@ CREATE TABLE labels ( conn.db_coins(&[outpoint_b, outpoint_c, outpoint_d]), ] .iter() - .all(|coin| coin.len() == 3 - && coin[0].outpoint == coin_b.outpoint - && coin[1].outpoint == coin_c.outpoint - && coin[2].outpoint == coin_d.outpoint)); + .all(|coins| coins.len() == 3 + && coins + .iter() + .all(|c| [coin_b.outpoint, coin_c.outpoint, coin_d.outpoint] + .contains(&c.outpoint)))); // We can also get two of the three unconfirmed coins by filtering for their outpoints. assert!([ @@ -1530,9 +1534,10 @@ CREATE TABLE labels ( conn.db_coins(&[outpoint_b, outpoint_c]), ] .iter() - .all(|coin| coin.len() == 2 - && coin[0].outpoint == coin_b.outpoint - && coin[1].outpoint == coin_c.outpoint)); + .all(|coins| coins.len() == 2 + && coins + .iter() + .all(|c| [coin_b.outpoint, coin_c.outpoint].contains(&c.outpoint)))); // Now spend second coin, even though it is still unconfirmed. conn.spend_coins(&[(coin_b.outpoint, txs.get(5).unwrap().txid())]); From 4f6dcbfdfd7eaf471a38badc0c9ce48aefac814c Mon Sep 17 00:00:00 2001 From: Michael Mallan Date: Thu, 28 Nov 2024 09:03:08 +0000 Subject: [PATCH 3/7] sqlite: add columns to transactions and coins tables The new columns in the transactions table will be populated for new transactions by the poller, while existing rows will be updated in a subsequent migration. New and existing coins will all have `is_from_self` set to false due to the default column value. None of these columns will be used by the wallet at this stage. --- lianad/src/database/sqlite/mod.rs | 57 ++++++++++++++++------------ lianad/src/database/sqlite/schema.rs | 23 ++++++++++- lianad/src/database/sqlite/utils.rs | 23 +++++++++++ 3 files changed, 77 insertions(+), 26 deletions(-) diff --git a/lianad/src/database/sqlite/mod.rs b/lianad/src/database/sqlite/mod.rs index a25a9c13..59ba5c9a 100644 --- a/lianad/src/database/sqlite/mod.rs +++ b/lianad/src/database/sqlite/mod.rs @@ -43,7 +43,7 @@ use miniscript::bitcoin::{ secp256k1, }; -const DB_VERSION: i64 = 6; +const DB_VERSION: i64 = 7; /// Last database version for which Bitcoin transactions were not stored in database. In practice /// this meant we relied on the bitcoind watchonly wallet to store them for us. @@ -733,9 +733,16 @@ impl SqliteConn { let txid = &tx.txid()[..].to_vec(); let tx_ser = bitcoin::consensus::serialize(tx); db_tx.execute( - "INSERT INTO transactions (txid, tx) VALUES (?1, ?2) \ + "INSERT INTO transactions (txid, tx, num_inputs, num_outputs, is_coinbase) \ + VALUES (?1, ?2, ?3, ?4, ?5) \ ON CONFLICT DO NOTHING", - rusqlite::params![txid, tx_ser,], + rusqlite::params![ + txid, + tx_ser, + tx.input.len(), + tx.output.len(), + tx.is_coinbase() + ], )?; } Ok(()) @@ -840,7 +847,7 @@ mod tests { str::FromStr, }; - use bitcoin::bip32; + use bitcoin::{bip32, ScriptBuf}; // The database schema used by the first versions of Liana (database version 0). Used to test // migrations starting from the first version. @@ -1299,8 +1306,8 @@ CREATE TABLE labels ( .map(|i| bitcoin::Transaction { version: bitcoin::transaction::Version::TWO, lock_time: bitcoin::absolute::LockTime::from_height(i).unwrap(), - input: Vec::new(), - output: Vec::new(), + input: vec![bitcoin::TxIn::default()], // a single input + output: vec![bitcoin::TxOut::minimal_non_dust(ScriptBuf::default())], // a single output, }) .collect(); conn.new_txs(&txs); @@ -1602,8 +1609,8 @@ CREATE TABLE labels ( .map(|i| bitcoin::Transaction { version: bitcoin::transaction::Version::TWO, lock_time: bitcoin::absolute::LockTime::from_height(i).unwrap(), - input: Vec::new(), - output: Vec::new(), + input: vec![bitcoin::TxIn::default()], // a single input + output: vec![bitcoin::TxOut::minimal_non_dust(ScriptBuf::default())], // a single output, }) .collect(); conn.new_txs(&txs); @@ -1935,8 +1942,8 @@ CREATE TABLE labels ( .map(|i| bitcoin::Transaction { version: bitcoin::transaction::Version::TWO, lock_time: bitcoin::absolute::LockTime::from_height(i).unwrap(), - input: Vec::new(), - output: Vec::new(), + input: vec![bitcoin::TxIn::default()], // a single input + output: vec![bitcoin::TxOut::minimal_non_dust(ScriptBuf::default())], // a single output, }) .collect(); conn.new_txs(&txs); @@ -2144,8 +2151,8 @@ CREATE TABLE labels ( .map(|i| bitcoin::Transaction { version: bitcoin::transaction::Version::TWO, lock_time: bitcoin::absolute::LockTime::from_height(i).unwrap(), - input: Vec::new(), - output: Vec::new(), + input: vec![bitcoin::TxIn::default()], // a single input + output: vec![bitcoin::TxOut::minimal_non_dust(ScriptBuf::default())], // a single output, }) .collect(); conn.new_txs(&txs); @@ -2263,8 +2270,8 @@ CREATE TABLE labels ( .map(|i| bitcoin::Transaction { version: bitcoin::transaction::Version::TWO, lock_time: bitcoin::absolute::LockTime::from_height(i).unwrap(), - input: Vec::new(), - output: Vec::new(), + input: vec![bitcoin::TxIn::default()], // a single input + output: vec![bitcoin::TxOut::minimal_non_dust(ScriptBuf::default())], // a single output, }) .collect(); conn.new_txs(&txs); @@ -2291,8 +2298,8 @@ CREATE TABLE labels ( .map(|i| bitcoin::Transaction { version: bitcoin::transaction::Version::TWO, lock_time: bitcoin::absolute::LockTime::from_height(i).unwrap(), - input: Vec::new(), - output: Vec::new(), + input: vec![bitcoin::TxIn::default()], // a single input + output: vec![bitcoin::TxOut::minimal_non_dust(ScriptBuf::default())], // a single output, }) .collect(); let spend_txs: Vec<_> = (0..10) @@ -2301,8 +2308,8 @@ CREATE TABLE labels ( bitcoin::Transaction { version: bitcoin::transaction::Version::TWO, lock_time: bitcoin::absolute::LockTime::from_height(1_234 + i).unwrap(), - input: Vec::new(), - output: Vec::new(), + input: vec![bitcoin::TxIn::default()], // a single input + output: vec![bitcoin::TxOut::minimal_non_dust(ScriptBuf::default())], // a single output, }, if i % 2 == 0 { Some(BlockInfo { @@ -2462,7 +2469,7 @@ CREATE TABLE labels ( } #[test] - fn v0_to_v6_migration() { + fn v0_to_v7_migration() { let secp = secp256k1::Secp256k1::verification_only(); // Create a database with version 0, using the old schema. @@ -2568,7 +2575,7 @@ CREATE TABLE labels ( { let mut conn = db.connection().unwrap(); let version = conn.db_version(); - assert_eq!(version, 6); + assert_eq!(version, 7); } // We should now be able to insert another PSBT, to query both, and the first PSBT must // have no associated timestamp. @@ -2595,8 +2602,8 @@ CREATE TABLE labels ( let tx = bitcoin::Transaction { version: bitcoin::transaction::Version::TWO, lock_time: bitcoin::absolute::LockTime::from_height(2).unwrap(), - input: Vec::new(), - output: Vec::new(), + input: vec![bitcoin::TxIn::default()], // a single input + output: vec![bitcoin::TxOut::minimal_non_dust(ScriptBuf::default())], // a single output, }; conn.new_txs(&[tx.clone()]); conn.new_unspent_coins(&[Coin { @@ -2642,7 +2649,7 @@ CREATE TABLE labels ( } #[test] - fn v3_to_v6_migration() { + fn v3_to_v7_migration() { let secp = secp256k1::Secp256k1::verification_only(); // Create a database with version 3, using the old schema. @@ -2792,10 +2799,10 @@ CREATE TABLE labels ( // Migrate the DB. maybe_apply_migration(&db_path, &bitcoin_txs).unwrap(); - assert_eq!(conn.db_version(), 6); + assert_eq!(conn.db_version(), 7); // Migrating twice will be a no-op. No need to pass `bitcoin_txs` second time. maybe_apply_migration(&db_path, &[]).unwrap(); - assert!(conn.db_version() == 6); + assert!(conn.db_version() == 7); // Compare the `DbCoin`s with the expected values. let coins_post = conn.coins(&[], &[]); diff --git a/lianad/src/database/sqlite/schema.rs b/lianad/src/database/sqlite/schema.rs index 52a83a0b..0f4ec7a8 100644 --- a/lianad/src/database/sqlite/schema.rs +++ b/lianad/src/database/sqlite/schema.rs @@ -4,6 +4,11 @@ use std::{convert::TryFrom, str::FromStr}; use miniscript::bitcoin::{self, address, bip32, consensus::encode, psbt::Psbt}; +// Due to limitations of Sqlite's ALTER TABLE command and in order not to recreate +// tables during migration: +// - Columns `num_inputs` and `num_outputs` of the transactions table remain nullable. +// - There is no CHECK constraint to prevent both `is_immature` and `is_from_self` +// being true in the coins table. pub const SCHEMA: &str = "\ CREATE TABLE version ( version INTEGER NOT NULL @@ -42,6 +47,10 @@ CREATE TABLE wallets ( * The 'is_immature' field is for coinbase deposits that are not yet buried under 100 * blocks. Note coinbase deposits can't technically be unconfirmed but we keep them * as such until they become mature. + * + * The `is_from_self` field indicates if the coin is the output of a transaction whose + * inputs are all from the same wallet as the coin. For an unconfirmed coin, this also + * means that all unconfirmed ancestors, if any, are from self. */ CREATE TABLE coins ( id INTEGER PRIMARY KEY NOT NULL, @@ -57,6 +66,7 @@ CREATE TABLE coins ( spend_block_height INTEGER, spend_block_time INTEGER, is_immature BOOLEAN NOT NULL CHECK (is_immature IN (0,1)), + is_from_self BOOLEAN NOT NULL DEFAULT 0 CHECK (is_from_self IN (0,1)), UNIQUE (txid, vout), FOREIGN KEY (wallet_id) REFERENCES wallets (id) ON UPDATE RESTRICT @@ -82,7 +92,10 @@ CREATE TABLE addresses ( CREATE TABLE transactions ( id INTEGER PRIMARY KEY NOT NULL, txid BLOB UNIQUE NOT NULL, - tx BLOB UNIQUE NOT NULL + tx BLOB UNIQUE NOT NULL, + num_inputs INTEGER CHECK (num_inputs IS NULL OR num_inputs > 0), + num_outputs INTEGER CHECK (num_outputs IS NULL OR num_outputs > 0), + is_coinbase BOOLEAN NOT NULL DEFAULT 0 CHECK (is_coinbase IN (0,1)) ); /* Transactions we created that spend some of our coins. */ @@ -195,6 +208,12 @@ pub struct DbCoin { pub is_change: bool, pub spend_txid: Option, pub spend_block: Option, + /// A coin is from self if it is the output of a transaction whose + /// inputs are all from this wallet. For unconfirmed coins, we + /// further require that all unconfirmed ancestors, if any, also + /// be from self, as otherwise they will depend on an unconfirmed + /// external transaction. + pub is_from_self: bool, } impl TryFrom<&rusqlite::Row<'_>> for DbCoin { @@ -234,6 +253,7 @@ impl TryFrom<&rusqlite::Row<'_>> for DbCoin { }); let is_immature: bool = row.get(12)?; + let is_from_self: bool = row.get(13)?; Ok(DbCoin { id, @@ -246,6 +266,7 @@ impl TryFrom<&rusqlite::Row<'_>> for DbCoin { is_change, spend_txid, spend_block, + is_from_self, }) } } diff --git a/lianad/src/database/sqlite/utils.rs b/lianad/src/database/sqlite/utils.rs index 95fc984b..5847a6c6 100644 --- a/lianad/src/database/sqlite/utils.rs +++ b/lianad/src/database/sqlite/utils.rs @@ -307,6 +307,24 @@ fn migrate_v5_to_v6(conn: &mut rusqlite::Connection) -> Result<(), SqliteDbError Ok(()) } +fn migrate_v6_to_v7(conn: &mut rusqlite::Connection) -> Result<(), SqliteDbError> { + db_exec(conn, |db_tx| { + db_tx.execute_batch( + " + ALTER TABLE transactions ADD COLUMN num_inputs INTEGER CHECK (num_inputs IS NULL OR num_inputs > 0); + ALTER TABLE transactions ADD COLUMN num_outputs INTEGER CHECK (num_outputs IS NULL OR num_outputs > 0); + ALTER TABLE transactions ADD COLUMN is_coinbase BOOLEAN NOT NULL DEFAULT 0 CHECK (is_coinbase IN (0,1)); + + ALTER TABLE coins ADD COLUMN is_from_self BOOLEAN NOT NULL DEFAULT 0 CHECK (is_from_self IN (0,1)); + + UPDATE version SET version = 7; + ", + )?; + Ok(()) + })?; + Ok(()) +} + /// Check the database version and if necessary apply the migrations to upgrade it to the current /// one. The `bitcoin_txs` parameter is here for the migration from versions 4 and earlier, which /// did not store the Bitcoin transactions in database, to versions 5 and later, which do. For a @@ -360,6 +378,11 @@ pub fn maybe_apply_migration( migrate_v5_to_v6(&mut conn)?; log::warn!("Migration from database version 5 to version 6 successful."); } + 6 => { + log::warn!("Upgrading database from version 6 to version 7."); + migrate_v6_to_v7(&mut conn)?; + log::warn!("Migration from database version 6 to version 7 successful."); + } _ => return Err(SqliteDbError::UnsupportedVersion(version)), } } From da185361f99eb319554ec78daa2fccdcd72eb298 Mon Sep 17 00:00:00 2001 From: Michael Mallan Date: Thu, 28 Nov 2024 12:11:21 +0000 Subject: [PATCH 4/7] sqlite: add helper to query single row --- lianad/src/database/sqlite/utils.rs | 15 +++++++++++++++ 1 file changed, 15 insertions(+) diff --git a/lianad/src/database/sqlite/utils.rs b/lianad/src/database/sqlite/utils.rs index 5847a6c6..f26f4f0e 100644 --- a/lianad/src/database/sqlite/utils.rs +++ b/lianad/src/database/sqlite/utils.rs @@ -50,6 +50,21 @@ where .collect::>>() } +/// Internal helper for queries boilerplate +pub fn db_query_row( + conn: &mut rusqlite::Connection, + stmt_str: &str, + params: P, + f: F, +) -> Result +where + P: IntoIterator + rusqlite::Params, + P::Item: rusqlite::ToSql, + F: FnMut(&rusqlite::Row<'_>) -> rusqlite::Result, +{ + conn.prepare(stmt_str)?.query_row(params, f) +} + /// The current time as the number of seconds since the UNIX epoch, truncated to u32 since SQLite /// only supports i64 integers. pub fn curr_timestamp() -> u32 { From 6dd92052af0afc37bddb67493a64af396e7964c9 Mon Sep 17 00:00:00 2001 From: Michael Mallan Date: Thu, 28 Nov 2024 12:12:24 +0000 Subject: [PATCH 5/7] database: track whether coin is from self This populates the new columns from the previous migration for existing rows and then maintains them in the poller moving forward. --- lianad/src/bitcoin/poller/looper.rs | 3 + lianad/src/database/mod.rs | 9 + lianad/src/database/sqlite/mod.rs | 459 ++++++++++++++++++++++++++-- lianad/src/database/sqlite/utils.rs | 147 +++++++++ lianad/src/testutils.rs | 4 + 5 files changed, 605 insertions(+), 17 deletions(-) diff --git a/lianad/src/bitcoin/poller/looper.rs b/lianad/src/bitcoin/poller/looper.rs index baa95c2c..181d3c9d 100644 --- a/lianad/src/bitcoin/poller/looper.rs +++ b/lianad/src/bitcoin/poller/looper.rs @@ -315,6 +315,9 @@ fn updates( db_conn.unspend_coins(&updated_coins.expired_spending); db_conn.spend_coins(&updated_coins.spending); db_conn.confirm_spend(&updated_coins.spent); + // Update info about which coins are from self only after + // coins have been inserted & updated above. + db_conn.update_coins_from_self(current_tip.height); if latest_tip != current_tip { db_conn.update_tip(&latest_tip); log::debug!("New tip: '{}'", latest_tip); diff --git a/lianad/src/database/mod.rs b/lianad/src/database/mod.rs index 6ffce1e3..614d4c82 100644 --- a/lianad/src/database/mod.rs +++ b/lianad/src/database/mod.rs @@ -181,6 +181,10 @@ pub trait DatabaseConnection { /// Store transactions in database, ignoring any that already exist. fn new_txs(&mut self, txs: &[bitcoin::Transaction]); + /// For all unconfirmed coins and those confirmed after `prev_tip_height`, + /// update whether the coin is from self or not. + fn update_coins_from_self(&mut self, prev_tip_height: i32); + /// Retrieve a list of transactions and their corresponding block heights and times. fn list_wallet_transactions( &mut self, @@ -379,6 +383,11 @@ impl DatabaseConnection for SqliteConn { self.new_txs(txs) } + fn update_coins_from_self(&mut self, prev_tip_height: i32) { + self.update_coins_from_self(prev_tip_height) + .expect("must not fail") + } + fn list_wallet_transactions( &mut self, txids: &[bitcoin::Txid], diff --git a/lianad/src/database/sqlite/mod.rs b/lianad/src/database/sqlite/mod.rs index 59ba5c9a..2084c86c 100644 --- a/lianad/src/database/sqlite/mod.rs +++ b/lianad/src/database/sqlite/mod.rs @@ -43,7 +43,7 @@ use miniscript::bitcoin::{ secp256k1, }; -const DB_VERSION: i64 = 7; +const DB_VERSION: i64 = 8; /// Last database version for which Bitcoin transactions were not stored in database. In practice /// this meant we relied on the bitcoind watchonly wallet to store them for us. @@ -750,6 +750,91 @@ impl SqliteConn { .expect("Database must be available") } + /// Update `is_from_self` in coins table for all unconfirmed coins + /// and those confirmed after `prev_tip_height`. + /// + /// This only sets the value to true as we do not expect the value + /// to change from true to false. In case of a reorg, the value + /// for all unconfirmed coins should be set to false before this + /// method is called. + pub fn update_coins_from_self(&mut self, prev_tip_height: i32) -> Result<(), rusqlite::Error> { + db_exec(&mut self.conn, |db_tx| { + // Given the requirement for unconfirmed coins that all ancestors + // be from self, we perform the update in a loop until no further + // rows are updated in order to iterate over the unconfirmed coins. + // Although we don't expect any unconfirmed transaction to have + // more than 25 in-mempool descendants including itself, there + // could be more descendants in the DB following a reorg and a + // rollback of the tip. The max number of iterations would be + // one per unconfirmed coin not from self plus one for all + // confirmed coins. + // In any case, the query only sets `is_from_self` to 1 for + // those coins with value 0 and so the number of rows affected + // by each iteration must become 0. + let max_iterations = { + let num_unconfirmed: u64 = db_tx.query_row( + "SELECT COUNT(*) FROM coins + WHERE blockheight IS NULL AND is_from_self = 0", + [], + |row| row.get(0), + )?; + // Add 1 for the confirmed coins, which will all + // be updated in the first iteration, and another 1 + // as a final check there's nothing left to update. + num_unconfirmed.checked_add(2).expect("must fit") + }; + log::debug!( + "Updating is_from_self in up to {} iterations..", + max_iterations + ); + let mut updated = 0; + for i in 0..max_iterations { + updated = db_tx.execute( + " + UPDATE coins + SET is_from_self = 1 + FROM transactions t + INNER JOIN ( + SELECT + spend_txid, + SUM( + CASE + WHEN blockheight IS NOT NULL THEN 1 + -- If the spending coin is unconfirmed, only count + -- it as an input coin if it is from self. + WHEN blockheight IS NULL AND is_from_self = 1 THEN 1 + ELSE 0 + END + ) AS cnt + FROM coins + WHERE spend_txid IS NOT NULL + -- We only need to consider spend transactions that are + -- unconfirmed or confirmed after `prev_tip_height + -- as only these transactions will affect the coins that + -- we are updating. + AND (spend_block_height IS NULL OR spend_block_height > ?1) + GROUP BY spend_txid + ) spends + ON t.txid = spends.spend_txid AND t.num_inputs = spends.cnt + WHERE coins.txid = t.txid + AND (coins.blockheight IS NULL OR coins.blockheight > ?1) + AND coins.is_from_self = 0 + ", + [prev_tip_height], + )?; + if updated == 0 { + log::debug!("Finished updating is_from_self in {} iterations.", i + 1); + break; + } + } + assert_eq!( + updated, 0, + "no rows expected to be updated on final iteration while updating is_from_self", + ); + Ok(()) + }) + } + pub fn list_wallet_transactions( &mut self, txids: &[bitcoin::Txid], @@ -814,6 +899,10 @@ impl SqliteConn { /// - Spending transactions confirmation /// - Tip /// + /// The `is_from_self` value for all unconfirmed coins following the rollback is + /// set to false. This is because this value depends on the confirmation status + /// of ancestor coins and so will need to be re-evaluated. + /// /// This will have to be updated if we are to add new fields based on block data /// in the database eventually. pub fn rollback_tip(&mut self, new_tip: &BlockChainTip) { @@ -826,6 +915,12 @@ impl SqliteConn { "UPDATE coins SET spend_block_height = NULL, spend_block_time = NULL WHERE spend_block_height > ?1", rusqlite::params![new_tip.height], )?; + // This statement must be run after updating `blockheight` above so that it includes coins + // that become unconfirmed following the rollback. + db_tx.execute( + "UPDATE coins SET is_from_self = 0 WHERE blockheight IS NULL", + rusqlite::params![], + )?; db_tx.execute( "UPDATE tip SET blockheight = (?1), blockhash = (?2)", rusqlite::params![new_tip.height, new_tip.hash[..].to_vec()], @@ -847,7 +942,7 @@ mod tests { str::FromStr, }; - use bitcoin::{bip32, ScriptBuf}; + use bitcoin::{bip32, BlockHash, ScriptBuf, TxIn}; // The database schema used by the first versions of Liana (database version 0). Used to test // migrations starting from the first version. @@ -2469,7 +2564,332 @@ CREATE TABLE labels ( } #[test] - fn v0_to_v7_migration() { + fn sqlite_update_coins_from_self() { + let (tmp_dir, _, _, db) = dummy_db(); + + // Helper to create a dummy transaction. + // Varying `lock_time_height` allows to obtain a unique txid for the given `num_inputs`. + fn dummy_tx(num_inputs: u32, lock_time_height: u32) -> bitcoin::Transaction { + bitcoin::Transaction { + version: bitcoin::transaction::Version::TWO, + lock_time: bitcoin::absolute::LockTime::from_height(lock_time_height).unwrap(), + input: (0..num_inputs).map(|_| TxIn::default()).collect(), + output: vec![bitcoin::TxOut::minimal_non_dust(ScriptBuf::default())], // a single output, + } + } + + { + let mut conn = db.connection().unwrap(); + + // Deposit two coins from two different external transactions. + let tx_a = dummy_tx(1, 0); + let tx_b = dummy_tx(1, 1); + let coin_tx_a: Coin = Coin { + outpoint: bitcoin::OutPoint::new(tx_a.txid(), 0), + is_immature: false, + amount: bitcoin::Amount::from_sat(1_000_000), + derivation_index: bip32::ChildNumber::from_normal_idx(0).unwrap(), + is_change: false, + block_info: None, + spend_txid: None, + spend_block: None, + }; + let coin_tx_b: Coin = Coin { + outpoint: bitcoin::OutPoint::new(tx_b.txid(), 0), + is_immature: false, + amount: bitcoin::Amount::from_sat(1_000_000), + derivation_index: bip32::ChildNumber::from_normal_idx(1).unwrap(), + is_change: false, + block_info: None, + spend_txid: None, + spend_block: None, + }; + conn.new_txs(&[tx_a, tx_b]); + conn.new_unspent_coins(&[coin_tx_a, coin_tx_b]); + + // The coins are not from self. + assert!(conn.coins(&[], &[]).iter().all(|c| !c.is_from_self)); + // Update from self info. + conn.update_coins_from_self(0).unwrap(); + // As expected, the coins are still not marked as from self. + assert!(conn.coins(&[], &[]).iter().all(|c| !c.is_from_self)); + + // Spend `coin_tx_a` in `tx_c` with change `coin_tx_c`. + let tx_c = dummy_tx(1, 2); + let coin_tx_c: Coin = Coin { + outpoint: bitcoin::OutPoint::new(tx_c.txid(), 0), + is_immature: false, + amount: bitcoin::Amount::from_sat(1_000_000), + derivation_index: bip32::ChildNumber::from_normal_idx(2).unwrap(), + is_change: true, + block_info: None, + spend_txid: None, + spend_block: None, + }; + conn.new_txs(&[tx_c.clone()]); + conn.spend_coins(&[(coin_tx_a.outpoint, tx_c.txid())]); + conn.new_unspent_coins(&[coin_tx_c]); + + // Although `coin_tx_c` has only one parent, `coin_tx_a` is + // unconfirmed and not from self. So all our coins are still + // not marked as from self. + assert!(conn.coins(&[], &[]).iter().all(|c| !c.is_from_self)); + conn.update_coins_from_self(0).unwrap(); + assert!(conn.coins(&[], &[]).iter().all(|c| !c.is_from_self)); + + // Now refresh `coin_tx_c` in `tx_d`, creating `coin_tx_d`. + let tx_d = dummy_tx(1, 3); + let coin_tx_d: Coin = Coin { + outpoint: bitcoin::OutPoint::new(tx_d.txid(), 0), + is_immature: false, + amount: bitcoin::Amount::from_sat(1_000_000), + derivation_index: bip32::ChildNumber::from_normal_idx(3).unwrap(), + is_change: true, + block_info: None, + spend_txid: None, + spend_block: None, + }; + conn.new_txs(&[tx_d.clone()]); + conn.spend_coins(&[(coin_tx_c.outpoint, tx_d.txid())]); + conn.new_unspent_coins(&[coin_tx_d]); + + // All coins are unconfirmed and none are from self. + assert!(conn.coins(&[], &[]).iter().all(|c| !c.is_from_self)); + conn.update_coins_from_self(0).unwrap(); + assert!(conn.coins(&[], &[]).iter().all(|c| !c.is_from_self)); + + // Spend the deposited coin `coin_tx_b` and the refreshed coin `coin_tx_d` + // together in `tx_e`, creating `coin_tx_e`. + let tx_e = dummy_tx(2, 4); // 2 inputs + let coin_tx_e: Coin = Coin { + outpoint: bitcoin::OutPoint::new(tx_e.txid(), 0), + is_immature: false, + amount: bitcoin::Amount::from_sat(1_000_000), + derivation_index: bip32::ChildNumber::from_normal_idx(4).unwrap(), + is_change: false, + block_info: None, + spend_txid: None, + spend_block: None, + }; + conn.new_txs(&[tx_e.clone()]); + conn.spend_coins(&[ + (coin_tx_b.outpoint, tx_e.txid()), + (coin_tx_d.outpoint, tx_e.txid()), + ]); + conn.new_unspent_coins(&[coin_tx_e]); + + // Still there are no confirmed coins, so everything remains as not from self. + assert!(conn.coins(&[], &[]).iter().all(|c| !c.is_from_self)); + conn.update_coins_from_self(0).unwrap(); + assert!(conn.coins(&[], &[]).iter().all(|c| !c.is_from_self)); + + // Finally, refresh `coin_tx_e` in transaction `tx_f`, creating `coin_tx_f`. + let tx_f = dummy_tx(1, 5); + let coin_tx_f: Coin = Coin { + outpoint: bitcoin::OutPoint::new(tx_f.txid(), 0), + is_immature: false, + amount: bitcoin::Amount::from_sat(1_000_000), + derivation_index: bip32::ChildNumber::from_normal_idx(5).unwrap(), + is_change: true, + block_info: None, + spend_txid: None, + spend_block: None, + }; + conn.new_txs(&[tx_f.clone()]); + conn.spend_coins(&[(coin_tx_e.outpoint, tx_f.txid())]); + conn.new_unspent_coins(&[coin_tx_f]); + + // Still no coins are from self. + assert!(conn.coins(&[], &[]).iter().all(|c| !c.is_from_self)); + conn.update_coins_from_self(0).unwrap(); + assert!(conn.coins(&[], &[]).iter().all(|c| !c.is_from_self)); + + // Now confirm `tx_a` and `tx_c` in successive blocks. + conn.confirm_coins(&[ + (coin_tx_a.outpoint, 100, 1_000), + (coin_tx_c.outpoint, 101, 1_001), + ]); + conn.confirm_spend(&[(coin_tx_a.outpoint, tx_c.txid(), 101, 1_001)]); + // Coins are still not marked as from self. + assert!(conn.coins(&[], &[]).iter().all(|c| !c.is_from_self)); + // Now update from self for coins confirmed after 101, which excludes the two coins above. + // Only `coin_tx_d` is from self, because it's unconfirmed and its parent is a confirmed coin. + // `coin_tx_e` still depends on `coin_tx_b` which is an unconfirmed deposit. + conn.update_coins_from_self(101).unwrap(); + assert!(conn + .coins(&[], &[coin_tx_d.outpoint]) + .iter() + .all(|c| c.is_from_self)); + assert!(conn + .coins( + &[], + &[ + coin_tx_a.outpoint, + coin_tx_b.outpoint, + coin_tx_c.outpoint, + coin_tx_e.outpoint, + coin_tx_f.outpoint + ] + ) + .iter() + .all(|c| !c.is_from_self)); + + // Now run the update for coins confirmed after 100. + conn.update_coins_from_self(100).unwrap(); + // `coin_tx_c` is now marked as from self as it has a single parent + // that is confirmed (even though its parent is an external deposit). + assert!(conn + .coins(&[], &[coin_tx_c.outpoint, coin_tx_d.outpoint]) + .iter() + .all(|c| c.is_from_self)); + assert!(conn + .coins( + &[], + &[ + coin_tx_a.outpoint, + coin_tx_b.outpoint, + coin_tx_e.outpoint, + coin_tx_f.outpoint + ] + ) + .iter() + .all(|c| !c.is_from_self)); + + // Even if we run the update for coins confirmed after height 99, + // `coin_tx_a` will not be marked as from self as it's an external deposit. + conn.update_coins_from_self(99).unwrap(); + assert!(conn + .coins(&[], &[coin_tx_c.outpoint, coin_tx_d.outpoint]) + .iter() + .all(|c| c.is_from_self)); + assert!(conn + .coins( + &[], + &[ + coin_tx_a.outpoint, + coin_tx_b.outpoint, + coin_tx_e.outpoint, + coin_tx_f.outpoint + ] + ) + .iter() + .all(|c| !c.is_from_self)); + + // Now confirm the other external deposit coin. + conn.confirm_coins(&[(coin_tx_b.outpoint, 102, 1_002)]); + // If we run the update, it doesn't matter if we use a later height + // as there are only unconfirmed coins that need to be updated. + conn.update_coins_from_self(110).unwrap(); + // `coin_tx_e` and `coin_tx_f` are also now from self. + assert!(conn + .coins( + &[], + &[ + coin_tx_c.outpoint, + coin_tx_d.outpoint, + coin_tx_e.outpoint, + coin_tx_f.outpoint + ] + ) + .iter() + .all(|c| c.is_from_self)); + assert!(conn + .coins(&[], &[coin_tx_a.outpoint, coin_tx_b.outpoint,]) + .iter() + .all(|c| !c.is_from_self)); + + // Even if we now run the update with an earlier height, + // `coin_tx_b` will not be marked as from self. + conn.update_coins_from_self(101).unwrap(); + // No changes from above. + assert!(conn + .coins( + &[], + &[ + coin_tx_c.outpoint, + coin_tx_d.outpoint, + coin_tx_e.outpoint, + coin_tx_f.outpoint + ] + ) + .iter() + .all(|c| c.is_from_self)); + assert!(conn + .coins(&[], &[coin_tx_a.outpoint, coin_tx_b.outpoint,]) + .iter() + .all(|c| !c.is_from_self)); + + // Now we will roll the tip back earlier than some of our confirmed coins. + let new_tip = { + // It doesn't matter what this hash value is as we only care about the height. + let hash = BlockHash::from_str( + "000000000019d6689c085ae165831e934ff763ae46a2a6c172b3f1b60a8ce26f", + ) + .unwrap(); + &BlockChainTip { height: 101, hash } + }; + conn.rollback_tip(new_tip); + + // Only `coin_tx_a` and `coin_tx_c` are still confirmed. + assert_eq!( + conn.coins(&[], &[]) + .iter() + .filter_map(|c| if c.block_info.is_some() { + Some(c.outpoint) + } else { + None + }) + .collect::>(), + vec![coin_tx_a.outpoint, coin_tx_c.outpoint] + ); + // Rolling back sets all unconfirmed coins as not from self so only + // `coin_tx_c` is still marked as from self. + assert!(conn + .coins(&[], &[coin_tx_c.outpoint]) + .iter() + .all(|c| c.is_from_self)); + assert!(conn + .coins( + &[], + &[ + coin_tx_a.outpoint, + coin_tx_b.outpoint, + coin_tx_d.outpoint, + coin_tx_e.outpoint, + coin_tx_f.outpoint + ] + ) + .iter() + .all(|c| !c.is_from_self)); + + // Now run the update from the current tip height of 101. + conn.update_coins_from_self(101).unwrap(); + // `coin_tx_d` is now marked as from self as its parent `coin_tx_c` + // is confirmed. Coins `coin_tx_e` and `coin_tx_f` depend on the + // unconfirmed `tx_coin_b` and so remain as not from self. + assert!(conn + .coins(&[], &[coin_tx_c.outpoint, coin_tx_d.outpoint]) + .iter() + .all(|c| c.is_from_self)); + assert!(conn + .coins( + &[], + &[ + coin_tx_a.outpoint, + coin_tx_b.outpoint, + coin_tx_e.outpoint, + coin_tx_f.outpoint, + ] + ) + .iter() + .all(|c| !c.is_from_self)); + } + + fs::remove_dir_all(tmp_dir).unwrap(); + } + + #[test] + fn v0_to_v8_migration() { let secp = secp256k1::Secp256k1::verification_only(); // Create a database with version 0, using the old schema. @@ -2492,8 +2912,8 @@ CREATE TABLE labels ( .map(|i| bitcoin::Transaction { version: bitcoin::transaction::Version::TWO, lock_time: bitcoin::absolute::LockTime::from_height(i).unwrap(), - input: Vec::new(), - output: Vec::new(), + input: vec![bitcoin::TxIn::default()], // a single input + output: vec![bitcoin::TxOut::minimal_non_dust(ScriptBuf::default())], // a single output, }) .collect(); // The helper that was used to store Spend transaction in previous versions of the software @@ -2575,7 +2995,7 @@ CREATE TABLE labels ( { let mut conn = db.connection().unwrap(); let version = conn.db_version(); - assert_eq!(version, 7); + assert_eq!(version, 8); } // We should now be able to insert another PSBT, to query both, and the first PSBT must // have no associated timestamp. @@ -2649,7 +3069,7 @@ CREATE TABLE labels ( } #[test] - fn v3_to_v7_migration() { + fn v3_to_v8_migration() { let secp = secp256k1::Secp256k1::verification_only(); // Create a database with version 3, using the old schema. @@ -2672,8 +3092,8 @@ CREATE TABLE labels ( .map(|i| bitcoin::Transaction { version: bitcoin::transaction::Version::TWO, lock_time: bitcoin::absolute::LockTime::from_height(i).unwrap(), - input: Vec::new(), - output: Vec::new(), + input: vec![bitcoin::TxIn::default()], // a single input + output: vec![bitcoin::TxOut::minimal_non_dust(ScriptBuf::default())], // a single output, }) .collect(); @@ -2799,10 +3219,10 @@ CREATE TABLE labels ( // Migrate the DB. maybe_apply_migration(&db_path, &bitcoin_txs).unwrap(); - assert_eq!(conn.db_version(), 7); + assert_eq!(conn.db_version(), 8); // Migrating twice will be a no-op. No need to pass `bitcoin_txs` second time. maybe_apply_migration(&db_path, &[]).unwrap(); - assert!(conn.db_version() == 7); + assert!(conn.db_version() == 8); // Compare the `DbCoin`s with the expected values. let coins_post = conn.coins(&[], &[]); @@ -2821,6 +3241,11 @@ CREATE TABLE labels ( assert_eq!(c_post.is_change, c_pre.is_change); assert_eq!(c_post.spend_txid, c_pre.spend_txid); assert_eq!(c_post.spend_block, c_pre.spend_block); + // only coins D and E are from self. + assert_eq!( + c_post.is_from_self, + [coin_d_outpoint, coin_e_outpoint].contains(&c_pre.outpoint) + ); } } @@ -2847,8 +3272,8 @@ CREATE TABLE labels ( .map(|i| bitcoin::Transaction { version: bitcoin::transaction::Version::TWO, lock_time: bitcoin::absolute::LockTime::from_height(i).unwrap(), - input: Vec::new(), - output: Vec::new(), + input: vec![bitcoin::TxIn::default()], // a single input + output: vec![bitcoin::TxOut::minimal_non_dust(ScriptBuf::default())], // a single output, }) .collect(); let spend_txs: Vec<_> = (0..10) @@ -2857,12 +3282,12 @@ CREATE TABLE labels ( bitcoin::Transaction { version: bitcoin::transaction::Version::TWO, lock_time: bitcoin::absolute::LockTime::from_height(1_234 + i).unwrap(), - input: Vec::new(), - output: Vec::new(), + input: vec![bitcoin::TxIn::default()], // a single input + output: vec![bitcoin::TxOut::minimal_non_dust(ScriptBuf::default())], // a single output, }, if i % 2 == 0 { Some(DbBlockInfo { - height: (i % 5) as i32 * 2_000, + height: 1 + (i % 5) as i32 * 2_000, time: 1722488619 + (i % 5) * 84_999, }) } else { @@ -2890,7 +3315,7 @@ CREATE TABLE labels ( is_change: (i % 4) == 0, block_info: if i & 2 == 0 { Some(DbBlockInfo { - height: (i % 100) as i32 * 1_000, + height: 1 + (i % 100) as i32 * 1_000, time: 1722408619 + (i % 100) as u32 * 42_000, }) } else { diff --git a/lianad/src/database/sqlite/utils.rs b/lianad/src/database/sqlite/utils.rs index f26f4f0e..ad7aab6b 100644 --- a/lianad/src/database/sqlite/utils.rs +++ b/lianad/src/database/sqlite/utils.rs @@ -340,6 +340,148 @@ fn migrate_v6_to_v7(conn: &mut rusqlite::Connection) -> Result<(), SqliteDbError Ok(()) } +fn migrate_v7_to_v8(conn: &mut rusqlite::Connection) -> Result<(), SqliteDbError> { + // This migration is done as several database transactions in order not to + // have a very large database transaction containing all rows from the + // transactions table. + const TXIDS_BATCH_SIZE: u32 = 100; + loop { + let txids = db_query( + conn, + "SELECT txid FROM transactions WHERE num_inputs IS NULL LIMIT ?1", + rusqlite::params![TXIDS_BATCH_SIZE], + |row| { + let txid: Vec = row.get(0)?; + let txid: bitcoin::Txid = bitcoin::consensus::encode::deserialize(&txid) + .expect("We only store valid txids"); + Ok(txid) + }, + )?; + if txids.is_empty() { + break; + } + for txid in &txids { + let tx = db_query_row( + conn, + "SELECT tx FROM transactions WHERE txid = ?1", + rusqlite::params![txid[..].to_vec()], + |row| { + let tx: Vec = row.get(0)?; + let tx: bitcoin::Transaction = bitcoin::consensus::encode::deserialize(&tx) + .expect("We only store valid transactions"); + Ok(tx) + }, + )?; + db_exec(conn, |db_tx| { + let updated = db_tx.execute( + "UPDATE transactions SET num_inputs = ?1, num_outputs = ?2, is_coinbase = ?3 WHERE txid = ?4", + rusqlite::params![tx.input.len(), tx.output.len(), tx.is_coinbase(), txid[..].to_vec()], + )?; + assert_eq!(updated, 1); + Ok(()) + })?; + } + } + + // Update the `is_from_self` column for all unconfirmed coins and those + // confirmed after height 0, i.e. this will act on all coins. + let prev_tip_height = 0; + // As part of the same db_tx, first make sure that all rows of the + // transactions table have been updated. + db_exec(conn, |db_tx| { + let num_txs_to_update: u32 = db_tx.query_row( + "SELECT count(txid) FROM transactions WHERE num_inputs IS NULL OR num_outputs IS NULL", + [], + |row| row.get(0), + )?; + assert_eq!(num_txs_to_update, 0); + + // This is a copy of `SqliteConn::update_coins_from_self` as of + // the time of writing this migration. We don't use that method + // directly in case the schema changes in future and it no longer + // works on the V7 schema. + + // Given the requirement for unconfirmed coins that all ancestors + // be from self, we perform the update in a loop until no further + // rows are updated in order to iterate over the unconfirmed coins. + // Although we don't expect any unconfirmed transaction to have + // more than 25 in-mempool descendants including itself, there + // could be more descendants in the DB following a reorg and a + // rollback of the tip. The max number of iterations would be + // one per unconfirmed coin not from self plus one for all + // confirmed coins. + // In any case, the query only sets `is_from_self` to 1 for + // those coins with value 0 and so the number of rows affected + // by each iteration must become 0. + let max_iterations = { + let num_unconfirmed: u64 = db_tx.query_row( + "SELECT COUNT(*) FROM coins + WHERE blockheight IS NULL AND is_from_self = 0", + [], + |row| row.get(0), + )?; + // Add 1 for the confirmed coins, which will all + // be updated in the first iteration, and another 1 + // as a final check there's nothing left to update. + num_unconfirmed.checked_add(2).expect("must fit") + }; + log::debug!( + "Updating is_from_self in up to {} iterations..", + max_iterations + ); + let mut updated = 0; + for i in 0..max_iterations { + updated = db_tx.execute( + " + UPDATE coins + SET is_from_self = 1 + FROM transactions t + INNER JOIN ( + SELECT + spend_txid, + SUM( + CASE + WHEN blockheight IS NOT NULL THEN 1 + -- If the spending coin is unconfirmed, only count + -- it as an input coin if it is from self. + WHEN blockheight IS NULL AND is_from_self = 1 THEN 1 + ELSE 0 + END + ) AS cnt + FROM coins + WHERE spend_txid IS NOT NULL + -- We only need to consider spend transactions that are + -- unconfirmed or confirmed after `prev_tip_height + -- as only these transactions will affect the coins that + -- we are updating. + AND (spend_block_height IS NULL OR spend_block_height > ?1) + GROUP BY spend_txid + ) spends + ON t.txid = spends.spend_txid AND t.num_inputs = spends.cnt + WHERE coins.txid = t.txid + AND (coins.blockheight IS NULL OR coins.blockheight > ?1) + AND coins.is_from_self = 0 + ", + [prev_tip_height], + )?; + if updated == 0 { + log::debug!("Finished updating is_from_self in {} iterations.", i + 1); + break; + } + } + assert_eq!( + updated, 0, + "no rows expected to be updated on final iteration while updating is_from_self", + ); + + // Finally update the DB version. + db_tx.execute("UPDATE version SET version = 8", [])?; + Ok(()) + })?; + + Ok(()) +} + /// Check the database version and if necessary apply the migrations to upgrade it to the current /// one. The `bitcoin_txs` parameter is here for the migration from versions 4 and earlier, which /// did not store the Bitcoin transactions in database, to versions 5 and later, which do. For a @@ -398,6 +540,11 @@ pub fn maybe_apply_migration( migrate_v6_to_v7(&mut conn)?; log::warn!("Migration from database version 6 to version 7 successful."); } + 7 => { + log::warn!("Upgrading database from version 7 to version 8."); + migrate_v7_to_v8(&mut conn)?; + log::warn!("Migration from database version 7 to version 8 successful."); + } _ => return Err(SqliteDbError::UnsupportedVersion(version)), } } diff --git a/lianad/src/testutils.rs b/lianad/src/testutils.rs index 04ae6820..75aa5ef1 100644 --- a/lianad/src/testutils.rs +++ b/lianad/src/testutils.rs @@ -484,6 +484,10 @@ impl DatabaseConnection for DummyDatabase { } } + fn update_coins_from_self(&mut self, _prev_tip_height: i32) { + // noop + } + fn list_wallet_transactions( &mut self, txids: &[bitcoin::Txid], From c1e55716e619249c264193776d36c640df9bc946 Mon Sep 17 00:00:00 2001 From: Michael Mallan Date: Mon, 25 Nov 2024 10:46:24 +0000 Subject: [PATCH 6/7] database: add is_from_self to Coin --- lianad/src/bitcoin/poller/looper.rs | 1 + lianad/src/commands/mod.rs | 14 ++++++++++++++ lianad/src/database/mod.rs | 3 +++ lianad/src/database/sqlite/mod.rs | 25 +++++++++++++++++++++++++ 4 files changed, 43 insertions(+) diff --git a/lianad/src/bitcoin/poller/looper.rs b/lianad/src/bitcoin/poller/looper.rs index 181d3c9d..74c9ba8e 100644 --- a/lianad/src/bitcoin/poller/looper.rs +++ b/lianad/src/bitcoin/poller/looper.rs @@ -91,6 +91,7 @@ fn update_coins( block_info: None, spend_txid: None, spend_block: None, + is_from_self: false, }; received.push(coin); } diff --git a/lianad/src/commands/mod.rs b/lianad/src/commands/mod.rs index eba110d7..9afa5c2c 100644 --- a/lianad/src/commands/mod.rs +++ b/lianad/src/commands/mod.rs @@ -1491,6 +1491,7 @@ mod tests { is_change: false, spend_txid: None, spend_block: None, + is_from_self: false, }]); // If we try to use coin selection, the unconfirmed non-change coin will not be used // as a candidate and so we get a coin selection error due to insufficient funds. @@ -1733,6 +1734,7 @@ mod tests { is_change: false, spend_txid: None, spend_block: None, + is_from_self: false, }]); assert_eq!( control.create_spend(&destinations, &[dummy_op_dup], 1_001, None), @@ -1755,6 +1757,7 @@ mod tests { is_change: true, spend_txid: None, spend_block: None, + is_from_self: false, }; db_conn.new_unspent_coins(&[unconfirmed_coin]); // Coin selection error due to insufficient funds. @@ -1786,6 +1789,7 @@ mod tests { is_change: false, spend_txid: None, spend_block: None, + is_from_self: false, }]); // First, create a transaction using auto coin selection. let psbt = if let CreateSpendResult::Success { psbt, .. } = @@ -1921,6 +1925,7 @@ mod tests { is_change: false, spend_txid: None, spend_block: None, + is_from_self: false, }]); let empty_dest = &HashMap::, u64>::new(); assert!(matches!( @@ -1960,6 +1965,7 @@ mod tests { is_change: false, spend_txid: None, spend_block: None, + is_from_self: false, }]); assert_eq!( control.create_spend(&destinations, &[imma_op], 1_001, None), @@ -2005,6 +2011,7 @@ mod tests { is_change: false, spend_txid: None, spend_block: None, + is_from_self: false, }, Coin { outpoint: dummy_op_b, @@ -2015,6 +2022,7 @@ mod tests { is_change: false, spend_txid: None, spend_block: None, + is_from_self: false, }, ]); @@ -2162,6 +2170,7 @@ mod tests { height: 184500, time: 184500, }), + is_from_self: false, }]); // The coin is spent so we cannot RBF. assert_eq!( @@ -2273,6 +2282,7 @@ mod tests { derivation_index: ChildNumber::from(0), amount: bitcoin::Amount::from_sat(100_000_000), spend_txid: Some(spend_tx.txid()), + is_from_self: false, }, // Deposit 2 Coin { @@ -2287,6 +2297,7 @@ mod tests { derivation_index: ChildNumber::from(1), amount: bitcoin::Amount::from_sat(2000), spend_txid: None, + is_from_self: false, }, // This coin is a change output. Coin { @@ -2298,6 +2309,7 @@ mod tests { derivation_index: ChildNumber::from(2), amount: bitcoin::Amount::from_sat(100_000_000 - 4000 - 1000), spend_txid: None, + is_from_self: false, }, // Deposit 3 Coin { @@ -2312,6 +2324,7 @@ mod tests { derivation_index: ChildNumber::from(3), amount: bitcoin::Amount::from_sat(3000), spend_txid: None, + is_from_self: false, }, ]); @@ -2532,6 +2545,7 @@ mod tests { is_change: false, spend_txid: None, spend_block: None, + is_from_self: false, }]); } } diff --git a/lianad/src/database/mod.rs b/lianad/src/database/mod.rs index 614d4c82..086939b0 100644 --- a/lianad/src/database/mod.rs +++ b/lianad/src/database/mod.rs @@ -430,6 +430,7 @@ pub struct Coin { pub is_change: bool, pub spend_txid: Option, pub spend_block: Option, + pub is_from_self: bool, } impl std::convert::From for Coin { @@ -443,6 +444,7 @@ impl std::convert::From for Coin { is_change, spend_txid, spend_block, + is_from_self, .. } = db_coin; Coin { @@ -454,6 +456,7 @@ impl std::convert::From for Coin { is_change, spend_txid, spend_block: spend_block.map(BlockInfo::from), + is_from_self, } } } diff --git a/lianad/src/database/sqlite/mod.rs b/lianad/src/database/sqlite/mod.rs index 2084c86c..1eed24f2 100644 --- a/lianad/src/database/sqlite/mod.rs +++ b/lianad/src/database/sqlite/mod.rs @@ -1418,6 +1418,7 @@ CREATE TABLE labels ( is_change: false, spend_txid: None, spend_block: None, + is_from_self: false, }; conn.new_unspent_coins(&[coin_a]); // We can query by status and/or outpoint. @@ -1464,6 +1465,7 @@ CREATE TABLE labels ( is_change: true, spend_txid: None, spend_block: None, + is_from_self: false, }; conn.new_unspent_coins(&[coin_b]); // Both coins are unconfirmed. @@ -1598,6 +1600,7 @@ CREATE TABLE labels ( is_change: false, spend_txid: None, spend_block: None, + is_from_self: false, }; let outpoint_d = bitcoin::OutPoint::new(txs.get(4).unwrap().txid(), 43); let coin_d = Coin { @@ -1609,6 +1612,7 @@ CREATE TABLE labels ( is_change: false, spend_txid: None, spend_block: None, + is_from_self: false, }; conn.new_unspent_coins(&[coin_c, coin_d]); @@ -1723,6 +1727,7 @@ CREATE TABLE labels ( is_change: false, spend_txid: None, spend_block: None, + is_from_self: false, }; conn.new_unspent_coins(&[coin_a]); assert_eq!(conn.coins(&[], &[])[0].outpoint, coin_a.outpoint); @@ -1765,6 +1770,7 @@ CREATE TABLE labels ( is_change: true, spend_txid: None, spend_block: None, + is_from_self: false, }; conn.new_unspent_coins(&[coin_b]); let outpoints: HashSet = conn @@ -1888,6 +1894,7 @@ CREATE TABLE labels ( is_change: false, spend_txid: None, spend_block: None, + is_from_self: false, }; conn.new_unspent_coins(&[coin_imma]); let outpoints: HashSet = conn @@ -2059,6 +2066,7 @@ CREATE TABLE labels ( is_change: false, spend_txid: None, spend_block: None, + is_from_self: false, }, Coin { outpoint: bitcoin::OutPoint::new(txs.get(1).unwrap().txid(), 2), @@ -2072,6 +2080,7 @@ CREATE TABLE labels ( is_change: false, spend_txid: None, spend_block: None, + is_from_self: false, }, Coin { outpoint: bitcoin::OutPoint::new(txs.get(2).unwrap().txid(), 3), @@ -2088,6 +2097,7 @@ CREATE TABLE labels ( height: 101_199, time: 1_231_678, }), + is_from_self: false, }, Coin { outpoint: bitcoin::OutPoint::new(txs.get(4).unwrap().txid(), 4), @@ -2101,6 +2111,7 @@ CREATE TABLE labels ( is_change: false, spend_txid: None, spend_block: None, + is_from_self: false, }, Coin { outpoint: bitcoin::OutPoint::new(txs.get(5).unwrap().txid(), 5), @@ -2117,6 +2128,7 @@ CREATE TABLE labels ( height: 101_105, time: 1_201_678, }), + is_from_self: false, }, ]; conn.new_unspent_coins(&coins); @@ -2262,6 +2274,7 @@ CREATE TABLE labels ( is_change: false, spend_txid: None, spend_block: None, + is_from_self: false, }, Coin { outpoint: bitcoin::OutPoint::new(txs.get(1).unwrap().txid(), 2), @@ -2275,6 +2288,7 @@ CREATE TABLE labels ( is_change: false, spend_txid: None, spend_block: None, + is_from_self: false, }, Coin { outpoint: bitcoin::OutPoint::new(txs.get(2).unwrap().txid(), 3), @@ -2291,6 +2305,7 @@ CREATE TABLE labels ( height: 101_199, time: 1_123_000, }), + is_from_self: false, }, Coin { outpoint: bitcoin::OutPoint::new(txs.get(4).unwrap().txid(), 4), @@ -2304,6 +2319,7 @@ CREATE TABLE labels ( is_change: false, spend_txid: None, spend_block: None, + is_from_self: false, }, Coin { outpoint: bitcoin::OutPoint::new(txs.get(5).unwrap().txid(), 5), @@ -2320,6 +2336,7 @@ CREATE TABLE labels ( height: 101_105, time: 1_126_000, }), + is_from_self: false, }, ]; conn.new_unspent_coins(&coins); @@ -2448,6 +2465,7 @@ CREATE TABLE labels ( } else { None }, + is_from_self: false, }) .collect(); @@ -2593,6 +2611,7 @@ CREATE TABLE labels ( block_info: None, spend_txid: None, spend_block: None, + is_from_self: false, }; let coin_tx_b: Coin = Coin { outpoint: bitcoin::OutPoint::new(tx_b.txid(), 0), @@ -2603,6 +2622,7 @@ CREATE TABLE labels ( block_info: None, spend_txid: None, spend_block: None, + is_from_self: false, }; conn.new_txs(&[tx_a, tx_b]); conn.new_unspent_coins(&[coin_tx_a, coin_tx_b]); @@ -2625,6 +2645,7 @@ CREATE TABLE labels ( block_info: None, spend_txid: None, spend_block: None, + is_from_self: false, }; conn.new_txs(&[tx_c.clone()]); conn.spend_coins(&[(coin_tx_a.outpoint, tx_c.txid())]); @@ -2648,6 +2669,7 @@ CREATE TABLE labels ( block_info: None, spend_txid: None, spend_block: None, + is_from_self: false, }; conn.new_txs(&[tx_d.clone()]); conn.spend_coins(&[(coin_tx_c.outpoint, tx_d.txid())]); @@ -2670,6 +2692,7 @@ CREATE TABLE labels ( block_info: None, spend_txid: None, spend_block: None, + is_from_self: false, }; conn.new_txs(&[tx_e.clone()]); conn.spend_coins(&[ @@ -2694,6 +2717,7 @@ CREATE TABLE labels ( block_info: None, spend_txid: None, spend_block: None, + is_from_self: false, }; conn.new_txs(&[tx_f.clone()]); conn.spend_coins(&[(coin_tx_e.outpoint, tx_f.txid())]); @@ -3035,6 +3059,7 @@ CREATE TABLE labels ( is_change: false, spend_txid: None, spend_block: None, + is_from_self: false, }]); let coins = conn.coins(&[], &[]); assert_eq!(coins.len(), 3); From 1c0338610f76419287157c14a6647a362160f9e4 Mon Sep 17 00:00:00 2001 From: Michael Mallan Date: Tue, 26 Nov 2024 09:31:55 +0000 Subject: [PATCH 7/7] commands: add is_from_self to listcoins response --- doc/API.md | 1 + liana-gui/src/app/state/coins.rs | 4 ++ liana-gui/src/app/state/psbt.rs | 1 + liana-gui/src/lianalite/client/backend/mod.rs | 3 + lianad/src/commands/mod.rs | 6 ++ tests/test_chain.py | 60 +++++++++++++++---- tests/test_misc.py | 20 +++++-- tests/test_rpc.py | 50 ++++++++++++++++ 8 files changed, 130 insertions(+), 15 deletions(-) diff --git a/doc/API.md b/doc/API.md index c89d206a..0bdadcc4 100644 --- a/doc/API.md +++ b/doc/API.md @@ -135,6 +135,7 @@ A coin may have one of the following four statuses: | `spend_info` | object | Information about the transaction spending this coin. See [Spending transaction info](#spending_transaction_info). | | `is_immature` | bool | Whether this coin was created by a coinbase transaction that is still immature. | | `is_change` | bool | Whether the coin deposit address was derived from the change descriptor. | +| `is_from_self` | bool | Whether the coin and all its unconfirmed ancestors, if any, are outputs of transactions from this wallet. | ##### Spending transaction info diff --git a/liana-gui/src/app/state/coins.rs b/liana-gui/src/app/state/coins.rs index c7efb89e..054241d1 100644 --- a/liana-gui/src/app/state/coins.rs +++ b/liana-gui/src/app/state/coins.rs @@ -226,6 +226,7 @@ mod tests { address: dummy_address.clone(), derivation_index: 0.into(), is_change: false, + is_from_self: false, }, Coin { outpoint: bitcoin::OutPoint { txid, vout: 3 }, @@ -236,6 +237,7 @@ mod tests { address: dummy_address.clone(), derivation_index: 1.into(), is_change: false, + is_from_self: false, }, Coin { outpoint: bitcoin::OutPoint { txid, vout: 0 }, @@ -246,6 +248,7 @@ mod tests { address: dummy_address.clone(), derivation_index: 2.into(), is_change: false, + is_from_self: false, }, Coin { outpoint: bitcoin::OutPoint { txid, vout: 1 }, @@ -256,6 +259,7 @@ mod tests { address: dummy_address, derivation_index: 3.into(), is_change: false, + is_from_self: false, }, ]); diff --git a/liana-gui/src/app/state/psbt.rs b/liana-gui/src/app/state/psbt.rs index b736219b..c133865c 100644 --- a/liana-gui/src/app/state/psbt.rs +++ b/liana-gui/src/app/state/psbt.rs @@ -793,6 +793,7 @@ mod tests { "derivation_index": 0, "is_immature": false, "is_change": false, + "is_from_self": false, }]})), ), diff --git a/liana-gui/src/lianalite/client/backend/mod.rs b/liana-gui/src/lianalite/client/backend/mod.rs index 554df554..1118c686 100644 --- a/liana-gui/src/lianalite/client/backend/mod.rs +++ b/liana-gui/src/lianalite/client/backend/mod.rs @@ -649,6 +649,7 @@ impl Daemon for BackendWalletClient { txid: info.txid, height: info.height, }), + is_from_self: false, // FIXME: use value from backend }) .collect(), }) @@ -1133,6 +1134,7 @@ fn history_tx_from_api(value: api::Transaction, network: Network) -> HistoryTran txid: info.txid, height: info.height, }), + is_from_self: false, // FIXME: use value from backend }); } } @@ -1188,6 +1190,7 @@ fn spend_tx_from_api( txid: info.txid, height: info.height, }), + is_from_self: false, // FIXME: use value from backend }); } } diff --git a/lianad/src/commands/mod.rs b/lianad/src/commands/mod.rs index 9afa5c2c..1669da53 100644 --- a/lianad/src/commands/mod.rs +++ b/lianad/src/commands/mod.rs @@ -425,6 +425,7 @@ impl DaemonControl { spend_block, is_immature, is_change, + is_from_self, derivation_index, .. } = coin; @@ -445,6 +446,7 @@ impl DaemonControl { spend_info, is_immature, is_change, + is_from_self, } }) .collect(); @@ -1229,6 +1231,10 @@ pub struct ListCoinsEntry { pub is_immature: bool, /// Whether the coin deposit address was derived from the change descriptor. pub is_change: bool, + /// Whether the coin is the output of a transaction whose inputs are all from + /// this same wallet. If the coin is unconfirmed, it also means that all its + /// unconfirmed ancestors, if any, are also from self. + pub is_from_self: bool, } #[derive(Debug, Clone, Serialize, Deserialize)] diff --git a/tests/test_chain.py b/tests/test_chain.py index 30027bbb..87d01082 100644 --- a/tests/test_chain.py +++ b/tests/test_chain.py @@ -75,14 +75,36 @@ def test_reorg_exclusion(lianad, bitcoind): coin_b = get_coin(lianad, txid) b_spend_tx = spend_coins(lianad, bitcoind, [coin_b]) + # These are external deposits so not from self. + assert coin_a["is_from_self"] is False + assert coin_b["is_from_self"] is False + # A confirmed and spent coin addr = lianad.rpc.getnewaddress()["address"] - txid = bitcoind.rpc.sendtoaddress(addr, 3) - bitcoind.generate_block(1, wait_for_mempool=txid) + txid_c = bitcoind.rpc.sendtoaddress(addr, 3) wait_for(lambda: len(lianad.rpc.listcoins()["coins"]) == 3) - coin_c = get_coin(lianad, txid) - c_spend_tx = spend_coins(lianad, bitcoind, [coin_c]) - bitcoind.generate_block(1, wait_for_mempool=1) + # Now refresh this coin while it is unconfirmed. + res = lianad.rpc.createspend({}, [get_coin(lianad, txid_c)["outpoint"]], 1) + c_spend_psbt = PSBT.from_base64(res["psbt"]) + txid_d = sign_and_broadcast_psbt(lianad, c_spend_psbt) + wait_for(lambda: len(lianad.rpc.listcoins()["coins"]) == 4) + coin_c = get_coin(lianad, txid_c) + coin_d = get_coin(lianad, txid_d) + assert coin_c["is_from_self"] is False + assert coin_c["block_height"] is None + # Even though coin_d is from a self-send, coin_c is still unconfirmed + # and is not from self. Therefore, coin_d is not from self either. + assert coin_d["is_from_self"] is False + + bitcoind.generate_block(1) + # Wait for confirmation to be detected. + wait_for(lambda: get_coin(lianad, txid_d)["block_height"] is not None) + coin_c = get_coin(lianad, txid_c) + coin_d = get_coin(lianad, txid_d) + assert coin_c["is_from_self"] is False + assert coin_c["block_height"] is not None + assert coin_d["is_from_self"] is True + assert coin_d["block_height"] is not None # Make sure the transaction were confirmed >10 blocks ago, so bitcoind won't update the # mempool during the reorg to the initial height. @@ -108,7 +130,7 @@ def test_reorg_exclusion(lianad, bitcoind): tx = bitcoind.rpc.gettransaction(txid)["hex"] bitcoind.rpc.sendrawtransaction(tx) bitcoind.rpc.sendrawtransaction(b_spend_tx) - bitcoind.rpc.sendrawtransaction(c_spend_tx) + sign_and_broadcast_psbt(lianad, c_spend_psbt) bitcoind.generate_block(1, wait_for_mempool=5) new_height = bitcoind.rpc.getblockcount() wait_for(lambda: lianad.rpc.getinfo()["block_height"] == new_height) @@ -128,9 +150,11 @@ def test_reorg_exclusion(lianad, bitcoind): for c in lianad.rpc.listcoins()["coins"] if coin_c["outpoint"] == c["outpoint"] ) - c_spend_txid = get_txid(c_spend_tx) - assert new_coin_c["spend_info"]["txid"] == c_spend_txid + assert new_coin_c["spend_info"]["txid"] == txid_d assert new_coin_c["spend_info"]["height"] == new_height + new_coin_d = get_coin(lianad, txid_d) + assert new_coin_d["is_from_self"] is True + assert new_coin_d["block_height"] == new_height # TODO: maybe test with some malleation for the deposit and spending txs? @@ -162,17 +186,26 @@ def test_reorg_status_recovery(lianad, bitcoind): assert initial_height > 100 wait_for(lambda: lianad.rpc.getinfo()["block_height"] == initial_height) - # Both coins are confirmed. Spend the second one then get their infos. + # Both coins are confirmed. Refresh the second one then get their infos. wait_for(lambda: len(list_coins()) == 2) wait_for(lambda: all(c["block_height"] is not None for c in list_coins())) coin_b = get_coin(lianad, txids[1]) - tx = spend_coins(lianad, bitcoind, [coin_b]) - locktime = bitcoind.rpc.decoderawtransaction(tx)["locktime"] + # Refresh coin_b. + res = lianad.rpc.createspend({}, [coin_b["outpoint"]], 1) + b_spend_psbt = PSBT.from_base64(res["psbt"]) + txid = sign_and_broadcast_psbt(lianad, b_spend_psbt) + coin_c = get_coin(lianad, txid) + # coin_c is unconfirmed and marked as from self as its parent is confirmed. + assert coin_c["block_height"] is None + assert coin_c["is_from_self"] is True + + locktime = b_spend_psbt.tx.nLockTime assert initial_height - 100 <= locktime <= initial_height bitcoind.generate_block(1, wait_for_mempool=1) wait_for(lambda: spend_confirmed_noticed(lianad, coin_b["outpoint"])) coin_a = get_coin(lianad, txids[0]) coin_b = get_coin(lianad, txids[1]) + coin_c = get_coin(lianad, txid) # Reorg the chain down to the initial height without shifting nor malleating # any transaction. The coin info should be identical (except the spend info @@ -187,9 +220,14 @@ def test_reorg_status_recovery(lianad, bitcoind): if locktime == initial_height: # Cannot be mined until next block (initial_height + 1). coin_b["spend_info"] = None + # coin_c no longer exists. + with pytest.raises(StopIteration): + get_coin(lianad, coin_c["outpoint"]) else: # Otherwise, the tx will be mined at the height the reorg happened. coin_b["spend_info"]["height"] = initial_height + new_coin_c = get_coin(lianad, coin_c["outpoint"]) + assert new_coin_c["is_from_self"] is True assert new_coin_b == coin_b diff --git a/tests/test_misc.py b/tests/test_misc.py index 7cd1f95b..52f5e7a3 100644 --- a/tests/test_misc.py +++ b/tests/test_misc.py @@ -213,14 +213,22 @@ def test_coinbase_deposit(lianad, bitcoind): wait_for_sync() coins = lianad.rpc.listcoins()["coins"] assert ( - len(coins) == 1 and coins[0]["is_immature"] and coins[0]["spend_info"] is None + len(coins) == 1 + and coins[0]["is_immature"] + and coins[0]["spend_info"] is None + and not coins[0]["is_from_self"] ) # Generate 100 blocks to make the coinbase mature. We should detect it as such. + # It remains as not from self. bitcoind.generate_block(100) wait_for_sync() coin = lianad.rpc.listcoins()["coins"][0] - assert not coin["is_immature"] and coin["block_height"] is not None + assert ( + not coin["is_immature"] + and coin["block_height"] is not None + and not coins[0]["is_from_self"] + ) # We must be able to spend the mature coin. destinations = {bitcoind.rpc.getnewaddress(): int(0.999999 * COIN)} @@ -249,13 +257,17 @@ def test_coinbase_deposit(lianad, bitcoind): bitcoind.rpc.generatetoaddress(1, change_addr) wait_for(lambda: any(c["is_immature"] for c in lianad.rpc.listcoins()["coins"])) coin = next(c for c in lianad.rpc.listcoins()["coins"] if c["is_immature"]) - assert coin["is_change"] + assert coin["is_change"] and not coin["is_from_self"] bitcoind.generate_block(100) wait_for_sync() coin = next( c for c in lianad.rpc.listcoins()["coins"] if c["outpoint"] == coin["outpoint"] ) - assert not coin["is_immature"] and coin["block_height"] is not None + assert ( + not coin["is_immature"] + and coin["block_height"] is not None + and not coin["is_from_self"] + ) @pytest.mark.skipif( diff --git a/tests/test_rpc.py b/tests/test_rpc.py index 0a118506..92084c27 100644 --- a/tests/test_rpc.py +++ b/tests/test_rpc.py @@ -100,6 +100,7 @@ def test_listcoins(lianad, bitcoind): assert res[0]["is_change"] == False assert res[0]["block_height"] is None assert res[0]["spend_info"] is None + assert res[0]["is_from_self"] is False assert len(lianad.rpc.listcoins(["confirmed", "spent", "spending"])["coins"]) == 0 assert ( @@ -131,6 +132,8 @@ def test_listcoins(lianad, bitcoind): == lianad.rpc.listcoins(["spent", "unconfirmed", "confirmed"], [outpoint_a]) ) + assert lianad.rpc.listcoins()["coins"][0]["is_from_self"] is False + # Same if the coin gets spent. spend_tx = spend_coins(lianad, bitcoind, (res[0],)) spend_txid = get_txid(spend_tx) @@ -140,6 +143,7 @@ def test_listcoins(lianad, bitcoind): assert spend_info["height"] is None assert len(lianad.rpc.listcoins(["spent"])["coins"]) == 0 assert len(lianad.rpc.listcoins(["spending"])["coins"]) == 1 + assert lianad.rpc.listcoins(["spending"])["coins"][0]["is_from_self"] is False # And if this spending tx gets confirmed. bitcoind.generate_block(1, wait_for_mempool=spend_txid) @@ -154,6 +158,8 @@ def test_listcoins(lianad, bitcoind): == lianad.rpc.listcoins(["spent"]) == lianad.rpc.listcoins(["spent", "unconfirmed", "confirmed"]) ) + assert len(lianad.rpc.listcoins()["coins"]) == 1 + assert lianad.rpc.listcoins()["coins"][0]["is_from_self"] is False # Add a second coin. addr_b = lianad.rpc.getnewaddress()["address"] @@ -161,6 +167,7 @@ def test_listcoins(lianad, bitcoind): wait_for(lambda: len(lianad.rpc.listcoins()["coins"]) == 2) res = lianad.rpc.listcoins(["unconfirmed"], [])["coins"] outpoint_b = res[0]["outpoint"] + assert res[0]["is_from_self"] is False # We have one unconfirmed coin and one spent coin. assert ( @@ -182,6 +189,7 @@ def test_listcoins(lianad, bitcoind): == lianad.rpc.listcoins(["spending", "unconfirmed", "confirmed"]) == lianad.rpc.listcoins(["spending", "unconfirmed", "confirmed"], [outpoint_b]) ) + assert lianad.rpc.listcoins([], [outpoint_b])["coins"][0]["is_from_self"] is False # Now confirm the second coin. bitcoind.generate_block(1, wait_for_mempool=txid_b) @@ -190,6 +198,7 @@ def test_listcoins(lianad, bitcoind): lambda: lianad.rpc.listcoins([], [outpoint_b])["coins"][0]["block_height"] == block_height ) + assert lianad.rpc.listcoins([], [outpoint_b])["coins"][0]["is_from_self"] is False # We have one confirmed coin and one spent coin. assert ( @@ -1079,6 +1088,7 @@ def test_rbfpsbt_bump_fee(lianad, bitcoind): bitcoind.generate_block(1, wait_for_mempool=txid) wait_for(lambda: len(lianad.rpc.listcoins(["confirmed"])["coins"]) == 3) coins = lianad.rpc.listcoins(["confirmed"])["coins"] + assert all(c["is_from_self"] is False for c in coins) # Create a spend that will later be replaced. first_outpoints = [c["outpoint"] for c in coins[:2]] @@ -1113,6 +1123,13 @@ def test_rbfpsbt_bump_fee(lianad, bitcoind): for c in lianad.rpc.listcoins([], first_outpoints)["coins"] ) ) + # The change output is from self as its parent is confirmed. + lc_res = lianad.rpc.listcoins(["unconfirmed"], [])["coins"] + assert ( + len(lc_res) == 1 + and first_txid in lc_res[0]["outpoint"] + and lc_res[0]["is_from_self"] is True + ) # We can now use RBF, but the feerate must be higher than that of the first transaction. with pytest.raises(RpcError, match=f"Feerate 1 too low for minimum feerate 2."): lianad.rpc.rbfpsbt(first_txid, False, 1) @@ -1145,6 +1162,13 @@ def test_rbfpsbt_bump_fee(lianad, bitcoind): for c in lianad.rpc.listcoins([], first_outpoints)["coins"] ) ) + # The change output of the replacement is also from self as its parent is confirmed. + lc_res = lianad.rpc.listcoins(["unconfirmed"], [])["coins"] + assert ( + len(lc_res) == 1 + and rbf_1_txid in lc_res[0]["outpoint"] + and lc_res[0]["is_from_self"] is True + ) mempool_rbf_1 = bitcoind.rpc.getmempoolentry(rbf_1_txid) # Note that in the mempool entry, "ancestor" includes rbf_1_txid itself. rbf_1_feerate = ( @@ -1178,6 +1202,12 @@ def test_rbfpsbt_bump_fee(lianad, bitcoind): for c in lianad.rpc.listcoins([], desc_1_outpoints)["coins"] ) ) + lc_res = [c for c in lianad.rpc.listcoins(["unconfirmed"], [])["coins"]] + assert ( + len(lc_res) == 1 + and desc_1_txid in lc_res[0]["outpoint"] + and lc_res[0]["is_from_self"] is True + ) # Add a new transaction spending the change from the first descendant. desc_2_destinations = { bitcoind.rpc.getnewaddress(): 25_000, @@ -1194,6 +1224,12 @@ def test_rbfpsbt_bump_fee(lianad, bitcoind): for c in lianad.rpc.listcoins([], desc_2_outpoints)["coins"] ) ) + lc_res = [c for c in lianad.rpc.listcoins(["unconfirmed"], [])["coins"]] + assert ( + len(lc_res) == 1 + and desc_2_txid in lc_res[0]["outpoint"] + and lc_res[0]["is_from_self"] is True + ) # Now replace the first RBF, which will also remove its descendants. rbf_2_res = lianad.rpc.rbfpsbt(rbf_1_txid, False, feerate) rbf_2_psbt = PSBT.from_base64(rbf_2_res["psbt"]) @@ -1216,6 +1252,12 @@ def test_rbfpsbt_bump_fee(lianad, bitcoind): for c in lianad.rpc.listcoins([], first_outpoints)["coins"] ) ) + lc_res = [c for c in lianad.rpc.listcoins(["unconfirmed"], [])["coins"]] + assert ( + len(lc_res) == 1 + and rbf_2_txid in lc_res[0]["outpoint"] + and lc_res[0]["is_from_self"] is True + ) # The unconfirmed coins used in the descendant transactions have been removed so that # only one of the input coins remains, and its spend info has been wiped so that it is as before. assert lianad.rpc.listcoins([], desc_1_outpoints + desc_2_outpoints)["coins"] == [ @@ -1230,6 +1272,14 @@ def test_rbfpsbt_bump_fee(lianad, bitcoind): for c in lianad.rpc.listcoins([], first_outpoints)["coins"] ) ) + final_coins = lianad.rpc.listcoins()["coins"] + # We have the three original coins plus the change output from the last RBF. + assert len(final_coins) == 4 + assert len(coins + lc_res) == 4 + for fc in final_coins: + assert fc["outpoint"] in [c["outpoint"] for c in coins + lc_res] + # Original coins are not from self, but RBF change output is. + assert fc["is_from_self"] is (fc["outpoint"] in [c["outpoint"] for c in lc_res]) def test_rbfpsbt_insufficient_funds(lianad, bitcoind):