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

Merge #1001: commands: exclude immature coins from auto-selection

Browse Source 
f9bae9cc0814d909a72a800d9f6d7ced2f99f06d database: add migration from db version 3 to 4 (jp1ac4)
2c96ef57bd606524223da037375ef351ea69e949 commands: exclude immature coins from coin selection (jp1ac4)
3a7c151674c487dd11f4f2d1db78d468f5f087a9 database: allow for coinbase transactions to change addresses (jp1ac4)

Pull request description:

  As a follow-up to #873, this ~~adds a comment to make clear that immature coins are not included as candidates for auto-selection~~ excludes immature coins from auto-selection.

  An unconfirmed coin could be both immature and marked as change, as the latter only depends on whether the address is derived from the wallet's change descriptor. I've also removed a corresponding assertion that may not always hold.

ACKs for top commit:
  darosior:
    ACK f9bae9cc0814d909a72a800d9f6d7ced2f99f06d -- this is very nice. Again, great catch. And thanks for adding a more extensive unit test.

Tree-SHA512: 12abe3dd18723db58ff701f664c6085e7fd29d39fefa7206e3e00fa5fb3e3b4320720c183a9719a3a0f3124896347ac74571b45a54bd504d674f779913466c16
This commit is contained in:
Antoine Poinsot 2024-03-25 16:16:03 +01:00
commit 62efd33342
No known key found for this signature in database
GPG Key ID: E13FC145CD3F4304
5 changed files with 376 additions and 14 deletions
Show Stats Download Patch File Download Diff File Expand all files Collapse all files

42
src/commands/mod.rs
View File

@ -472,7 +472,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((
@ -1704,7 +1704,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,
@ -1713,7 +1713,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),
@ -1795,6 +1796,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(

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

@ -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 {
@ -1457,7 +1543,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,
};
@ -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::<Vec<_>>()
.len(),
1
);
assert_eq!(
coins_pre
.iter()
.filter(|c| c.is_change)
.collect::<Vec<_>>()
.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();

9
src/database/sqlite/schema.rs
View File

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

40
src/database/sqlite/utils.rs
View File

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

14
tests/test_misc.py
View File

@ -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."