From 3a7c151674c487dd11f4f2d1db78d468f5f087a9 Mon Sep 17 00:00:00 2001 From: jp1ac4 <121959000+jp1ac4@users.noreply.github.com> Date: Tue, 19 Mar 2024 10:45:32 +0000 Subject: [PATCH 1/3] database: allow for coinbase transactions to change addresses `is_change` is `true` for a coin if its address is derived from our change descriptor and could in principle be used for a coinbase transaction. The functional test was provided by darosior in a PR comment: https://github.com/wizardsardine/liana/pull/1001#pullrequestreview-1948564150 --- src/database/sqlite/mod.rs | 2 +- src/database/sqlite/schema.rs | 9 ++------- tests/test_misc.py | 14 ++++++++++++++ 3 files changed, 17 insertions(+), 8 deletions(-) diff --git a/src/database/sqlite/mod.rs b/src/database/sqlite/mod.rs index 0433c42e..52e660ad 100644 --- a/src/database/sqlite/mod.rs +++ b/src/database/sqlite/mod.rs @@ -1457,7 +1457,7 @@ CREATE TABLE spend_transactions ( block_info: None, amount: bitcoin::Amount::from_sat(424242), derivation_index: bip32::ChildNumber::from_normal_idx(4103).unwrap(), - is_change: false, // Cannot be both a coinbase deposit and change. + is_change: false, spend_txid: None, spend_block: None, }; diff --git a/src/database/sqlite/schema.rs b/src/database/sqlite/schema.rs index 6add975a..ac101bcd 100644 --- a/src/database/sqlite/schema.rs +++ b/src/database/sqlite/schema.rs @@ -39,8 +39,8 @@ CREATE TABLE wallets ( * transaction for this coin exists and was confirmed. * * The 'is_immature' field is for coinbase deposits that are not yet buried under 100 - * blocks. Note coinbase deposits can't be change. They also technically can't be - * unconfirmed but we keep them as such until they become mature. + * blocks. Note coinbase deposits can't technically be unconfirmed but we keep them + * as such until they become mature. */ CREATE TABLE coins ( id INTEGER PRIMARY KEY NOT NULL, @@ -56,7 +56,6 @@ CREATE TABLE coins ( spend_block_height INTEGER, spend_block_time INTEGER, is_immature BOOLEAN NOT NULL CHECK (is_immature IN (0,1)), - CHECK (is_change IS 0 OR is_immature IS 0), UNIQUE (txid, vout), FOREIGN KEY (wallet_id) REFERENCES wallets (id) ON UPDATE RESTRICT @@ -217,10 +216,6 @@ impl TryFrom<&rusqlite::Row<'_>> for DbCoin { }); let is_immature: bool = row.get(12)?; - assert!( - !is_immature || !is_change, - "A coin cannot be both created in a coinbase and be change" - ); Ok(DbCoin { id, diff --git a/tests/test_misc.py b/tests/test_misc.py index f4a81c5f..c7f29a6d 100644 --- a/tests/test_misc.py +++ b/tests/test_misc.py @@ -239,6 +239,20 @@ def test_coinbase_deposit(lianad, bitcoind): and coin["spend_info"] is not None ) + # We must also properly detect coinbase deposits to a change address. We used to have + # an assertion that a coin cannot both be change and a coinbase deposit. Since change + # is determined by the address... Technically we can. + change_desc = lianad.multi_desc.singlepath_descriptors()[1] + change_addr = bitcoind.rpc.deriveaddresses(str(change_desc), [0, 0])[0] + 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"] + 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 + @pytest.mark.skipif( OLD_LIANAD_PATH is None or USE_TAPROOT, reason="Need the old lianad binary to create the datadir." From 2c96ef57bd606524223da037375ef351ea69e949 Mon Sep 17 00:00:00 2001 From: jp1ac4 <121959000+jp1ac4@users.noreply.github.com> Date: Mon, 11 Mar 2024 16:55:56 +0000 Subject: [PATCH 2/3] commands: exclude immature coins from coin selection An immature coin could in principle be marked as change as this depends only on whether the address is derived from the wallet's change descriptor. --- src/commands/mod.rs | 42 +++++++++++++++++++++++++++++++++++++++--- 1 file changed, 39 insertions(+), 3 deletions(-) diff --git a/src/commands/mod.rs b/src/commands/mod.rs index 6cb372d7..80c9f8bd 100644 --- a/src/commands/mod.rs +++ b/src/commands/mod.rs @@ -470,7 +470,7 @@ impl DaemonControl { .filter_map(|(op, c)| { if c.block_info.is_some() { Some((c, None)) // confirmed coins have no ancestor info - } else if c.is_change { + } else if c.is_change && !c.is_immature { // In case the mempool_entry is None, the coin will be included without // any ancestor info. Some(( @@ -1691,7 +1691,7 @@ mod tests { txid: dummy_op.txid, vout: dummy_op.vout + 100, }; - db_conn.new_unspent_coins(&[Coin { + let unconfirmed_coin = Coin { outpoint: confirmed_op_1, is_immature: false, block_info: None, @@ -1700,7 +1700,8 @@ mod tests { is_change: true, spend_txid: None, spend_block: None, - }]); + }; + db_conn.new_unspent_coins(&[unconfirmed_coin]); // Coin selection error due to insufficient funds. assert!(matches!( control.create_spend(&destinations, &[], 1, None), @@ -1782,6 +1783,41 @@ mod tests { manual_input.sort(); assert_eq!(auto_input, manual_input); + // Now check that the spend created above using auto-selection only works when the unconfirmed coin + // is change and not immature. + // 1. not change and not immature + db_conn.remove_coins(&[unconfirmed_coin.outpoint]); + let mut unconfirmed_coin_2 = unconfirmed_coin; + unconfirmed_coin_2.is_change = false; + unconfirmed_coin_2.is_immature = false; // (this is already the case) + db_conn.new_unspent_coins(&[unconfirmed_coin_2]); + assert!(matches!( + control.create_spend(&destinations, &[], 1, None), + Ok(CreateSpendResult::InsufficientFunds { .. }), + )); + // 2. change and immature + db_conn.remove_coins(&[unconfirmed_coin_2.outpoint]); + unconfirmed_coin_2.is_change = true; + unconfirmed_coin_2.is_immature = true; + db_conn.new_unspent_coins(&[unconfirmed_coin_2]); + assert!(matches!( + control.create_spend(&destinations, &[], 1, None), + Ok(CreateSpendResult::InsufficientFunds { .. }), + )); + // 3. not change and immature + db_conn.remove_coins(&[unconfirmed_coin_2.outpoint]); + unconfirmed_coin_2.is_change = false; + unconfirmed_coin_2.is_immature = true; + db_conn.new_unspent_coins(&[unconfirmed_coin_2]); + assert!(matches!( + control.create_spend(&destinations, &[], 1, None), + Ok(CreateSpendResult::InsufficientFunds { .. }), + )); + + // Re-insert the original unconfirmed coin again. + db_conn.remove_coins(&[unconfirmed_coin_2.outpoint]); + db_conn.new_unspent_coins(&[unconfirmed_coin]); + // Now do the same again, but this time specifying the change address to be the same // as for the auto spend. let change_address = bitcoin::Address::from_script( From f9bae9cc0814d909a72a800d9f6d7ced2f99f06d Mon Sep 17 00:00:00 2001 From: jp1ac4 <121959000+jp1ac4@users.noreply.github.com> Date: Wed, 20 Mar 2024 12:05:00 +0000 Subject: [PATCH 3/3] database: add migration from db version 3 to 4 --- src/database/sqlite/mod.rs | 283 ++++++++++++++++++++++++++++++++++- src/database/sqlite/utils.rs | 40 +++++ 2 files changed, 320 insertions(+), 3 deletions(-) diff --git a/src/database/sqlite/mod.rs b/src/database/sqlite/mod.rs index 52e660ad..ad99ac55 100644 --- a/src/database/sqlite/mod.rs +++ b/src/database/sqlite/mod.rs @@ -43,7 +43,7 @@ use miniscript::bitcoin::{ secp256k1, }; -const DB_VERSION: i64 = 3; +const DB_VERSION: i64 = 4; #[derive(Debug)] pub enum SqliteDbError { @@ -807,6 +807,92 @@ CREATE TABLE spend_transactions ( psbt BLOB UNIQUE NOT NULL, txid BLOB UNIQUE NOT NULL ); +"; + + const V3_SCHEMA: &str = "\ +CREATE TABLE version ( + version INTEGER NOT NULL +); + +/* About the Bitcoin network. */ +CREATE TABLE tip ( + network TEXT NOT NULL, + blockheight INTEGER, + blockhash BLOB +); + +/* This stores metadata about our wallet. We only support single wallet for + * now (and the foreseeable future). + * + * The 'timestamp' field is the creation date of the wallet. We guarantee to have seen all + * information related to our descriptor(s) that occured after this date. + * The optional 'rescan_timestamp' field is a the timestamp we need to rescan the chain + * for events related to our descriptor(s) from. + */ +CREATE TABLE wallets ( + id INTEGER PRIMARY KEY NOT NULL, + timestamp INTEGER NOT NULL, + main_descriptor TEXT NOT NULL, + deposit_derivation_index INTEGER NOT NULL, + change_derivation_index INTEGER NOT NULL, + rescan_timestamp INTEGER +); + +/* Our (U)TxOs. + * + * The 'spend_block_height' and 'spend_block.time' are only present if the spending + * transaction for this coin exists and was confirmed. + * + * The 'is_immature' field is for coinbase deposits that are not yet buried under 100 + * blocks. Note coinbase deposits can't be change. They also technically can't be + * unconfirmed but we keep them as such until they become mature. + */ +CREATE TABLE coins ( + id INTEGER PRIMARY KEY NOT NULL, + wallet_id INTEGER NOT NULL, + blockheight INTEGER, + blocktime INTEGER, + txid BLOB NOT NULL, + vout INTEGER NOT NULL, + amount_sat INTEGER NOT NULL, + derivation_index INTEGER NOT NULL, + is_change BOOLEAN NOT NULL CHECK (is_change IN (0,1)), + spend_txid BLOB, + spend_block_height INTEGER, + spend_block_time INTEGER, + is_immature BOOLEAN NOT NULL CHECK (is_immature IN (0,1)), + CHECK (is_change IS 0 OR is_immature IS 0), + UNIQUE (txid, vout), + FOREIGN KEY (wallet_id) REFERENCES wallets (id) + ON UPDATE RESTRICT + ON DELETE RESTRICT +); + +/* A mapping from descriptor address to derivation index. Necessary until + * we can get the derivation index from the parent descriptor from bitcoind. + */ +CREATE TABLE addresses ( + receive_address TEXT NOT NULL UNIQUE, + change_address TEXT NOT NULL UNIQUE, + derivation_index INTEGER NOT NULL UNIQUE +); + +/* Transactions we created that spend some of our coins. */ +CREATE TABLE spend_transactions ( + id INTEGER PRIMARY KEY NOT NULL, + psbt BLOB UNIQUE NOT NULL, + txid BLOB UNIQUE NOT NULL, + updated_at INTEGER +); + +/* Labels applied on addresses (0), outpoints (1), txids (2) */ +CREATE TABLE labels ( + id INTEGER PRIMARY KEY NOT NULL, + wallet_id INTEGER NOT NULL, + item_kind INTEGER NOT NULL CHECK (item_kind IN (0,1,2)), + item TEXT UNIQUE NOT NULL, + value TEXT NOT NULL +); "; fn psbt_from_str(psbt_str: &str) -> Psbt { @@ -2130,7 +2216,198 @@ CREATE TABLE spend_transactions ( } #[test] - fn v0_to_v3_migration() { + fn v3_to_v4_migration() { + let secp = secp256k1::Secp256k1::verification_only(); + + // Create a database with version 3, using the old schema. + let tmp_dir = tmp_dir(); + fs::create_dir_all(&tmp_dir).unwrap(); + let db_path: path::PathBuf = [tmp_dir.as_path(), path::Path::new("lianad_v3.sqlite3")] + .iter() + .collect(); + let mut options = dummy_options(); + options.schema = V3_SCHEMA; + options.version = 3; + create_fresh_db(&db_path, options, &secp).unwrap(); + + { + // Don't use SqliteDb::new() in order not to apply migration. + let db = SqliteDb { + db_path: db_path.clone(), + }; + let mut conn = db.connection().unwrap(); + assert!(conn.db_version() == 3); + + // 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 { + outpoint: bitcoin::OutPoint::from_str( + "6f0dc85a369b44458eba3a1f0ea5b5935d563afb6994f70f5b0094e05be1676c:1", + ) + .unwrap(), + 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, + }; + let coin_b = Coin { + outpoint: bitcoin::OutPoint::from_str( + "81b2f327d4c1fd67afd039374f8798fd9ff37932c6f5c221c1c569350eac5ac8:19234", + ) + .unwrap(), + 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, + spend_block: None, + }; + let coin_c = Coin { + outpoint: bitcoin::OutPoint::from_str( + "7477017f992cdc7ba08acafb77cb3b5bc0f42ac340d3e1e1da0785bdda20d5f6:932", + ) + .unwrap(), + is_immature: false, + amount: bitcoin::Amount::from_sat(354764), + derivation_index: bip32::ChildNumber::from_normal_idx(3401).unwrap(), + is_change: true, + block_info: None, + spend_txid: None, + spend_block: None, + }; + let coin_d = Coin { + outpoint: bitcoin::OutPoint::from_str( + "ed6c8f1af9325f84de521e785e7ddfd33dc28c9ada4d687dcd3850100bde54e9:1456", + ) + .unwrap(), + 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_block: None, + }; + let coin_e = Coin { + outpoint: bitcoin::OutPoint::from_str( + "4753a1d74c0af8dd0a0f3b763c14faf3bd9ed03cbdf33337a074fb0e9f6c7810:4633", + ) + .unwrap(), + is_immature: false, + amount: bitcoin::Amount::from_sat(675000), + derivation_index: bip32::ChildNumber::from_normal_idx(3).unwrap(), + is_change: false, + block_info: None, + spend_txid: None, + spend_block: None, + }; + let coin_imma_a = Coin { + outpoint: bitcoin::OutPoint::from_str( + "c449539458c60bee6c0d8905ba1dadb20b9187b82045d306a408b894cea492b0:5", + ) + .unwrap(), + is_immature: true, + amount: bitcoin::Amount::from_sat(4564347), + derivation_index: bip32::ChildNumber::from_normal_idx(453).unwrap(), + is_change: false, + block_info: None, + spend_txid: None, + spend_block: None, + }; + let coin_imma_b = Coin { + outpoint: bitcoin::OutPoint::from_str( + "f0801fd9ca8bca0624c230ab422b2e2c4c8dc995e4e1dbc6412510959cce1e4f:19234", + ) + .unwrap(), + is_immature: true, + amount: bitcoin::Amount::from_sat(731453), + derivation_index: bip32::ChildNumber::from_normal_idx(98).unwrap(), + is_change: false, + block_info: None, + 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(&[ + coin_a, + coin_b, + coin_c, + coin_d, + 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::Txid::from_slice(&[1; 32][..]).unwrap(), + ), + (coin_b.outpoint, coin_d.outpoint.txid), + (coin_d.outpoint, coin_e.outpoint.txid), + ]); + conn.confirm_spend(&[( + coin_a.outpoint, + bitcoin::Txid::from_slice(&[1; 32][..]).unwrap(), + 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 + ); + + // Migrate the DB. + maybe_apply_migration(&db_path).unwrap(); + assert!(conn.db_version() == 4); + maybe_apply_migration(&db_path).unwrap(); // Migrating twice will be a no-op. + assert!(conn.db_version() == 4); + let coins_post = conn.coins(&[], &[]); + assert_eq!(coins_pre, coins_post); + } + + fs::remove_dir_all(tmp_dir).unwrap(); + } + + #[test] + fn v0_to_v4_migration() { let secp = secp256k1::Secp256k1::verification_only(); // Create a database with version 0, using the old schema. @@ -2150,7 +2427,7 @@ CREATE TABLE spend_transactions ( { let mut conn = db.connection().unwrap(); let version = conn.db_version(); - assert_eq!(version, 3); + assert_eq!(version, 4); let txid_str = "0c62a990d20d54429e70859292e82374ba6b1b951a3ab60f26bb65fee5724ff7"; let txid = LabelItem::from_str(txid_str, bitcoin::Network::Bitcoin).unwrap(); diff --git a/src/database/sqlite/utils.rs b/src/database/sqlite/utils.rs index efe92644..e1981ee4 100644 --- a/src/database/sqlite/utils.rs +++ b/src/database/sqlite/utils.rs @@ -194,6 +194,41 @@ fn migrate_v2_to_v3(conn: &mut rusqlite::Connection) -> Result<(), SqliteDbError Ok(()) } +fn migrate_v3_to_v4(conn: &mut rusqlite::Connection) -> Result<(), SqliteDbError> { + db_exec(conn, |tx| { + tx.execute_batch( + "CREATE TABLE coins_new ( + id INTEGER PRIMARY KEY NOT NULL, + wallet_id INTEGER NOT NULL, + blockheight INTEGER, + blocktime INTEGER, + txid BLOB NOT NULL, + vout INTEGER NOT NULL, + amount_sat INTEGER NOT NULL, + derivation_index INTEGER NOT NULL, + is_change BOOLEAN NOT NULL CHECK (is_change IN (0,1)), + spend_txid BLOB, + spend_block_height INTEGER, + spend_block_time INTEGER, + is_immature BOOLEAN NOT NULL CHECK (is_immature IN (0,1)), + UNIQUE (txid, vout), + FOREIGN KEY (wallet_id) REFERENCES wallets (id) + ON UPDATE RESTRICT + ON DELETE RESTRICT + ); + + INSERT INTO coins_new SELECT * FROM coins; + + DROP TABLE coins; + + ALTER TABLE coins_new RENAME TO coins; + + UPDATE version SET version = 4;", + ) + })?; + Ok(()) +} + /// Check the database version and if necessary apply the migrations to upgrade it to the current /// one. pub fn maybe_apply_migration(db_path: &path::Path) -> Result<(), SqliteDbError> { @@ -222,6 +257,11 @@ pub fn maybe_apply_migration(db_path: &path::Path) -> Result<(), SqliteDbError> migrate_v2_to_v3(&mut conn)?; log::warn!("Migration from database version 2 to version 3 successful."); } + 3 => { + log::warn!("Upgrading database from version 3 to version 4."); + migrate_v3_to_v4(&mut conn)?; + log::warn!("Migration from database version 3 to version 4 successful."); + } _ => return Err(SqliteDbError::UnsupportedVersion(version)), } }