From 26add29b197eb6fdf8f5f86ece4c52576700fc6f Mon Sep 17 00:00:00 2001 From: Antoine Poinsot Date: Thu, 20 Jul 2023 13:42:30 +0200 Subject: [PATCH 1/7] database: record whether a coin comes from an immature coinbase We need to keep track of such coins to: - Track their maturation - Avoid using them (for instance in coin selection) Reorg handling for coinbase deposits that become immature is not implemented (yet). That's reasonable because: 1. It would be very unlikely that we'd move back, so it's most likely gonna be mature again immediately. 2. If there's a reorg of more than 100 blocks we've got bigger problems. --- src/bitcoin/poller/looper.rs | 1 + src/commands/mod.rs | 8 +++ src/database/mod.rs | 3 + src/database/sqlite/mod.rs | 131 ++++++++++++++++++++++++++++++++-- src/database/sqlite/schema.rs | 15 ++++ src/database/sqlite/utils.rs | 24 +++++++ 6 files changed, 175 insertions(+), 7 deletions(-) diff --git a/src/bitcoin/poller/looper.rs b/src/bitcoin/poller/looper.rs index c3f246d83..8944d92ce 100644 --- a/src/bitcoin/poller/looper.rs +++ b/src/bitcoin/poller/looper.rs @@ -66,6 +66,7 @@ fn update_coins( if !curr_coins.contains_key(&utxo.outpoint) { let coin = Coin { outpoint, + is_immature: false, amount, derivation_index, is_change, diff --git a/src/commands/mod.rs b/src/commands/mod.rs index e5ca6322f..8e36ecca8 100644 --- a/src/commands/mod.rs +++ b/src/commands/mod.rs @@ -976,6 +976,7 @@ mod tests { let mut db_conn = control.db().lock().unwrap().connection(); db_conn.new_unspent_coins(&[Coin { outpoint: dummy_op, + is_immature: false, block_info: None, amount: bitcoin::Amount::from_sat(100_000), derivation_index: bip32::ChildNumber::from(13), @@ -1081,6 +1082,7 @@ mod tests { }; db_conn.new_unspent_coins(&[Coin { outpoint: dummy_op_dup, + is_immature: false, block_info: None, amount: bitcoin::Amount::from_sat(400_000), derivation_index: bip32::ChildNumber::from(42), @@ -1127,6 +1129,7 @@ mod tests { db_conn.new_unspent_coins(&[ Coin { outpoint: dummy_op_a, + is_immature: false, block_info: None, amount: bitcoin::Amount::from_sat(100_000), derivation_index: bip32::ChildNumber::from(13), @@ -1136,6 +1139,7 @@ mod tests { }, Coin { outpoint: dummy_op_b, + is_immature: false, block_info: None, amount: bitcoin::Amount::from_sat(115_680), derivation_index: bip32::ChildNumber::from(34), @@ -1303,6 +1307,7 @@ mod tests { // Deposit 1 Coin { is_change: false, + is_immature: false, outpoint: OutPoint { txid: deposit1.txid(), vout: 0, @@ -1316,6 +1321,7 @@ mod tests { // Deposit 2 Coin { is_change: false, + is_immature: false, outpoint: OutPoint { txid: deposit2.txid(), vout: 0, @@ -1329,6 +1335,7 @@ mod tests { // This coin is a change output. Coin { is_change: true, + is_immature: false, outpoint: OutPoint::new(spend_tx.txid(), 1), block_info: Some(BlockInfo { height: 3, time: 3 }), spend_block: None, @@ -1339,6 +1346,7 @@ mod tests { // Deposit 3 Coin { is_change: false, + is_immature: false, outpoint: OutPoint { txid: deposit3.txid(), vout: 0, diff --git a/src/database/mod.rs b/src/database/mod.rs index 23daf8c23..e25acb92c 100644 --- a/src/database/mod.rs +++ b/src/database/mod.rs @@ -281,6 +281,7 @@ impl From for BlockInfo { #[derive(Debug, Clone, Copy, PartialEq, Eq, Hash)] pub struct Coin { pub outpoint: bitcoin::OutPoint, + pub is_immature: bool, pub block_info: Option, pub amount: bitcoin::Amount, pub derivation_index: bip32::ChildNumber, @@ -293,6 +294,7 @@ impl std::convert::From for Coin { fn from(db_coin: DbCoin) -> Coin { let DbCoin { outpoint, + is_immature, block_info, amount, derivation_index, @@ -303,6 +305,7 @@ impl std::convert::From for Coin { } = db_coin; Coin { outpoint, + is_immature, block_info: block_info.map(BlockInfo::from), amount, derivation_index, diff --git a/src/database/sqlite/mod.rs b/src/database/sqlite/mod.rs index 2e63ec124..2cf3aa7b3 100644 --- a/src/database/sqlite/mod.rs +++ b/src/database/sqlite/mod.rs @@ -35,7 +35,7 @@ use miniscript::bitcoin::{ secp256k1, }; -const DB_VERSION: i64 = 1; +const DB_VERSION: i64 = 2; #[derive(Debug)] pub enum SqliteDbError { @@ -383,8 +383,8 @@ impl SqliteConn { for coin in coins { let deriv_index: u32 = coin.derivation_index.into(); db_tx.execute( - "INSERT INTO coins (wallet_id, txid, vout, amount_sat, derivation_index, is_change) \ - VALUES (?1, ?2, ?3, ?4, ?5, ?6)", + "INSERT INTO coins (wallet_id, txid, vout, amount_sat, derivation_index, is_change, is_immature) \ + VALUES (?1, ?2, ?3, ?4, ?5, ?6, ?7)", rusqlite::params![ WALLET_ID, coin.outpoint.txid[..].to_vec(), @@ -392,6 +392,7 @@ impl SqliteConn { coin.amount.to_sat(), deriv_index, coin.is_change, + coin.is_immature, ], )?; } @@ -593,11 +594,12 @@ impl SqliteConn { .expect("Db must not fail"); } + // TODO: mark coinbase deposits that were mature and became immature as such. /// Unconfirm all data that was marked as being confirmed *after* the given chain /// tip, and set it as our new best block seen. /// /// This includes: - /// - Coins + /// - Coins (coinbase deposits that became immature isn't currently implemented) /// - Spending transactions confirmation /// - Tip /// @@ -823,6 +825,7 @@ CREATE TABLE spend_transactions ( "6f0dc85a369b44458eba3a1f0ea5b5935d563afb6994f70f5b0094e05be1676c:1", ) .unwrap(), + is_immature: false, block_info: None, amount: bitcoin::Amount::from_sat(98765), derivation_index: bip32::ChildNumber::from_normal_idx(10).unwrap(), @@ -855,6 +858,7 @@ CREATE TABLE spend_transactions ( "61db3e276b095e5b05f1849dd6bfffb4e7e5ec1c4a4210099b98fce01571936f:12", ) .unwrap(), + is_immature: false, block_info: None, amount: bitcoin::Amount::from_sat(1111), derivation_index: bip32::ChildNumber::from_normal_idx(103).unwrap(), @@ -949,6 +953,31 @@ CREATE TABLE spend_transactions ( assert!(coin.spend_block.is_some()); assert_eq!(coin.spend_block.as_ref().unwrap().time, time); assert_eq!(coin.spend_block.unwrap().height, height); + + // Add an immature coin. As all coins it's first registered as unconfirmed (even though + // it's not). + let coin_imma = Coin { + outpoint: bitcoin::OutPoint::from_str( + "61db3e276b095e5b05f1849dd6bfffb4e7e5ec1c4a4210099b98fce01571937a:42", + ) + .unwrap(), + is_immature: true, + 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. + spend_txid: None, + spend_block: None, + }; + conn.new_unspent_coins(&[coin_imma]); + let outpoints: HashSet = conn + .coins(CoinType::All) + .into_iter() + .map(|c| c.outpoint) + .collect(); + assert!(outpoints.contains(&coin_imma.outpoint)); + let coin = conn.db_coins(&[coin_imma.outpoint]).pop().unwrap(); + assert!(coin.is_immature && !coin.is_change); } fs::remove_dir_all(tmp_dir).unwrap(); @@ -1084,12 +1113,14 @@ CREATE TABLE spend_transactions ( // - One confirmed before the rollback height but spent after // - One confirmed after the rollback height // - One spent after the rollback height + // TODO: immature deposits let coins = [ Coin { outpoint: bitcoin::OutPoint::from_str( "6f0dc85a369b44458eba3a1f0ea5b5935d563afb6994f70f5b0094e05be1676c:1", ) .unwrap(), + is_immature: false, block_info: None, amount: bitcoin::Amount::from_sat(98765), derivation_index: bip32::ChildNumber::from_normal_idx(10).unwrap(), @@ -1102,6 +1133,7 @@ CREATE TABLE spend_transactions ( "c449539458c60bee6c0d8905ba1dadb20b9187b82045d306a408b894cea492b0:2", ) .unwrap(), + is_immature: false, block_info: Some(BlockInfo { height: 101_095, time: 1_111_899, @@ -1117,6 +1149,7 @@ CREATE TABLE spend_transactions ( "f0801fd9ca8bca0624c230ab422b2e2c4c8dc995e4e1dbc6412510959cce1e4f:3", ) .unwrap(), + is_immature: false, block_info: Some(BlockInfo { height: 101_099, time: 1_121_899, @@ -1140,6 +1173,7 @@ CREATE TABLE spend_transactions ( "19f56e65069f0a7a3bfb00c6a7085cc0669e03e91befeca1ee9891c9e737b2fb:4", ) .unwrap(), + is_immature: false, block_info: Some(BlockInfo { height: 101_100, time: 1_131_899, @@ -1155,6 +1189,7 @@ CREATE TABLE spend_transactions ( "ed6c8f1af9325f84de521e785e7ddfd33dc28c9ada4d687dcd3850100bde54e9:5", ) .unwrap(), + is_immature: false, block_info: Some(BlockInfo { height: 101_102, time: 1_134_899, @@ -1303,6 +1338,7 @@ CREATE TABLE spend_transactions ( "6f0dc85a369b44458eba3a1f0ea5b5935d563afb6994f70f5b0094e05be1676c:1", ) .unwrap(), + is_immature: false, block_info: None, amount: bitcoin::Amount::from_sat(98765), derivation_index: bip32::ChildNumber::from_normal_idx(10).unwrap(), @@ -1315,6 +1351,7 @@ CREATE TABLE spend_transactions ( "c449539458c60bee6c0d8905ba1dadb20b9187b82045d306a408b894cea492b0:2", ) .unwrap(), + is_immature: false, block_info: Some(BlockInfo { height: 101_095, time: 1_121_000, @@ -1330,6 +1367,7 @@ CREATE TABLE spend_transactions ( "f0801fd9ca8bca0624c230ab422b2e2c4c8dc995e4e1dbc6412510959cce1e4f:3", ) .unwrap(), + is_immature: false, block_info: Some(BlockInfo { height: 101_099, time: 1_122_000, @@ -1353,6 +1391,7 @@ CREATE TABLE spend_transactions ( "19f56e65069f0a7a3bfb00c6a7085cc0669e03e91befeca1ee9891c9e737b2fb:4", ) .unwrap(), + is_immature: true, block_info: Some(BlockInfo { height: 101_100, time: 1_124_000, @@ -1368,6 +1407,7 @@ CREATE TABLE spend_transactions ( "ed6c8f1af9325f84de521e785e7ddfd33dc28c9ada4d687dcd3850100bde54e9:5", ) .unwrap(), + is_immature: false, block_info: Some(BlockInfo { height: 101_102, time: 1_125_000, @@ -1448,7 +1488,7 @@ CREATE TABLE spend_transactions ( } #[test] - fn v0_to_v1_migration() { + fn v0_to_v2_migration() { let secp = secp256k1::Secp256k1::verification_only(); // Create a database with version 0, using the old schema. @@ -1490,11 +1530,66 @@ CREATE TABLE spend_transactions ( store_spend_old(&mut conn, &first_psbt); } - // Migrate the DB. We should be able to insert another PSBT, to query both, and the first - // PSBT must have no associated timestamp. + // The helper that was used to store coins in previous versions of the software, stripped + // down to a single coin. + fn store_coin_old( + conn: &mut rusqlite::Connection, + outpoint: &bitcoin::OutPoint, + amount: bitcoin::Amount, + derivation_index: bip32::ChildNumber, + is_change: bool, + ) { + db_exec(conn, |db_tx| { + let deriv_index: u32 = derivation_index.into(); + db_tx.execute( + "INSERT INTO coins (wallet_id, txid, vout, amount_sat, derivation_index, is_change) \ + VALUES (?1, ?2, ?3, ?4, ?5, ?6)", + rusqlite::params![ + WALLET_ID, + outpoint.txid[..].to_vec(), + outpoint.vout, + amount.to_sat(), + deriv_index, + is_change, + ], + )?; + Ok(()) + }) + .expect("Database must be available") + } + + // Store a couple coins before the migration. + { + let mut conn = rusqlite::Connection::open(&db_path).unwrap(); + store_coin_old( + &mut conn, + &bitcoin::OutPoint::from_str( + "ed6c8f1af9325f84de521e785e7ddfd33dc28c9ada4d687dcd3850100bde54e9:5", + ) + .unwrap(), + bitcoin::Amount::from_sat(14_000), + 24.into(), + true, + ); + store_coin_old( + &mut conn, + &bitcoin::OutPoint::from_str( + "81b2f327d4c1fd67afd039374f8798fd9ff37932c6f5c221c1c569350eac5ac8:2", + ) + .unwrap(), + bitcoin::Amount::from_sat(392_093_123), + 24_567.into(), + false, + ); + } + + // Migrate the DB. maybe_apply_migration(&db_path).unwrap(); maybe_apply_migration(&db_path).unwrap(); // Migrating twice will be a no-op. let db = SqliteDb::new(db_path, None, &secp).unwrap(); + + // We should now be able to insert another PSBT, to query both, and the first PSBT must + // have no associated timestamp. { let mut conn = db.connection().unwrap(); conn.store_spend(&second_psbt); @@ -1511,6 +1606,28 @@ CREATE TABLE spend_transactions ( assert!(second_spend.updated_at.is_some()); } + // We should now be able to store an immature coin, query all of them, and the first two + // should not be immature. + { + let mut conn = db.connection().unwrap(); + conn.new_unspent_coins(&[Coin { + outpoint: bitcoin::OutPoint::from_str( + "6f0dc85a369b44458eba3a1f0ea5b5935d563afb6994f70f5b0094e05be1676c:1", + ) + .unwrap(), + is_immature: true, + block_info: None, + amount: bitcoin::Amount::from_sat(98765), + derivation_index: bip32::ChildNumber::from_normal_idx(10).unwrap(), + is_change: false, + spend_txid: None, + spend_block: None, + }]); + let coins = conn.coins(CoinType::All); + assert_eq!(coins.len(), 3); + assert_eq!(coins.iter().filter(|c| !c.is_immature).count(), 2); + } + fs::remove_dir_all(tmp_dir).unwrap(); } } diff --git a/src/database/sqlite/schema.rs b/src/database/sqlite/schema.rs index 93e101e77..b4b4b7b7a 100644 --- a/src/database/sqlite/schema.rs +++ b/src/database/sqlite/schema.rs @@ -39,6 +39,10 @@ CREATE TABLE wallets ( * * 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, @@ -53,6 +57,8 @@ CREATE TABLE coins ( 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 @@ -156,6 +162,8 @@ pub struct DbBlockInfo { pub struct DbCoin { pub id: i64, pub wallet_id: i64, + /// Whether this coin was created by a yet-to-be-mature coinbase transaction. + pub is_immature: bool, pub outpoint: bitcoin::OutPoint, pub block_info: Option, pub amount: bitcoin::Amount, @@ -201,9 +209,16 @@ impl TryFrom<&rusqlite::Row<'_>> for DbCoin { time: spend_time.expect("Must be there if height is"), }); + 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, wallet_id, + is_immature, outpoint, block_info, amount, diff --git a/src/database/sqlite/utils.rs b/src/database/sqlite/utils.rs index 073fbe7fc..a782036f8 100644 --- a/src/database/sqlite/utils.rs +++ b/src/database/sqlite/utils.rs @@ -165,6 +165,25 @@ fn migrate_v0_to_v1(conn: &mut rusqlite::Connection) -> Result<(), SqliteDbError Ok(()) } +// After Liana 1.0 we upgraded the schema to record whether a coin originated from an immature +// coinbase transaction. +fn migrate_v1_to_v2(conn: &mut rusqlite::Connection) -> Result<(), SqliteDbError> { + db_exec(conn, |tx| { + tx.execute( + "ALTER TABLE coins ADD COLUMN is_immature", + rusqlite::params![], + )?; + tx.execute( + "UPDATE coins SET is_immature = 0 WHERE is_immature IS NULL", + rusqlite::params![], + )?; + tx.execute("UPDATE version SET version = 2", rusqlite::params![])?; + Ok(()) + })?; + + 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> { @@ -183,6 +202,11 @@ pub fn maybe_apply_migration(db_path: &path::Path) -> Result<(), SqliteDbError> migrate_v0_to_v1(&mut conn)?; log::warn!("Migration from database version 0 to version 1 successful."); } + 1 => { + log::warn!("Upgrading database from version 1 to version 2."); + migrate_v1_to_v2(&mut conn)?; + log::warn!("Migration from database version 1 to version 2 successful."); + } _ => return Err(SqliteDbError::UnsupportedVersion(version)), } } From fd717123be45b73758f1e965659de8f44031fc90 Mon Sep 17 00:00:00 2001 From: Antoine Poinsot Date: Thu, 20 Jul 2023 11:19:17 +0200 Subject: [PATCH 2/7] commands: don't create spends with immature coins --- src/commands/mod.rs | 26 ++++++++++++++++++++++++++ src/jsonrpc/mod.rs | 1 + 2 files changed, 27 insertions(+) diff --git a/src/commands/mod.rs b/src/commands/mod.rs index 8e36ecca8..d1d18159b 100644 --- a/src/commands/mod.rs +++ b/src/commands/mod.rs @@ -50,6 +50,7 @@ pub enum CommandError { InvalidFeerate(/* sats/vb */ u64), UnknownOutpoint(bitcoin::OutPoint), AlreadySpent(bitcoin::OutPoint), + ImmatureCoinbase(bitcoin::OutPoint), Address(bitcoin::address::Error), InvalidOutputValue(bitcoin::Amount), InsufficientFunds( @@ -77,6 +78,7 @@ impl fmt::Display for CommandError { Self::NoOutpoint => write!(f, "No provided outpoint. Need at least one."), Self::InvalidFeerate(sats_vb) => write!(f, "Invalid feerate: {} sats/vb.", sats_vb), Self::AlreadySpent(op) => write!(f, "Coin at '{}' is already spent.", op), + Self::ImmatureCoinbase(op) => write!(f, "Coin at '{}' is from an immature coinbase transaction.", op), Self::UnknownOutpoint(op) => write!(f, "Unknown outpoint '{}'.", op), Self::Address(e) => write!( f, @@ -349,6 +351,10 @@ impl DaemonControl { if coin.is_spent() { return Err(CommandError::AlreadySpent(*op)); } + if coin.is_immature { + return Err(CommandError::ImmatureCoinbase(*op)); + } + // Fetch the transaction that created it if necessary if !spent_txs.contains_key(op) { let tx = self @@ -1097,6 +1103,26 @@ mod tests { ))) ); + // Can't create a transaction that spends an immature coinbase deposit. + let imma_op = bitcoin::OutPoint::from_str( + "4753a1d74c0af8dd0a0f3b763c14faf3bd9ed03cbdf33337a074fb0e9f6c7810:0", + ) + .unwrap(); + db_conn.new_unspent_coins(&[Coin { + outpoint: imma_op, + is_immature: true, + block_info: None, + amount: bitcoin::Amount::from_sat(100_000), + derivation_index: bip32::ChildNumber::from(13), + is_change: false, + spend_txid: None, + spend_block: None, + }]); + assert_eq!( + control.create_spend(&destinations, &[imma_op], 1_001), + Err(CommandError::ImmatureCoinbase(imma_op)) + ); + ms.shutdown(); } diff --git a/src/jsonrpc/mod.rs b/src/jsonrpc/mod.rs index 440e776b8..6af7b9e86 100644 --- a/src/jsonrpc/mod.rs +++ b/src/jsonrpc/mod.rs @@ -155,6 +155,7 @@ impl From for Error { | commands::CommandError::UnknownOutpoint(..) | commands::CommandError::InvalidFeerate(..) | commands::CommandError::AlreadySpent(..) + | commands::CommandError::ImmatureCoinbase(..) | commands::CommandError::Address(..) | commands::CommandError::InvalidOutputValue(..) | commands::CommandError::InsufficientFunds(..) From 6ab6161078af1932dd43bab813963c78887d7672 Mon Sep 17 00:00:00 2001 From: Antoine Poinsot Date: Thu, 20 Jul 2023 11:25:09 +0200 Subject: [PATCH 3/7] commands: expose whether a coin is immature in listcoins --- doc/API.md | 1 + src/commands/mod.rs | 4 ++++ 2 files changed, 5 insertions(+) diff --git a/doc/API.md b/doc/API.md index 943556cd8..68c7a1f67 100644 --- a/doc/API.md +++ b/doc/API.md @@ -96,6 +96,7 @@ This command does not take any parameter for now. | `outpoint` | string | Transaction id and output index of this coin. | | `block_height` | int or null | Block height the transaction was confirmed at, or `null`. | | `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. | ##### Spending transaction info diff --git a/src/commands/mod.rs b/src/commands/mod.rs index d1d18159b..168b1f1f0 100644 --- a/src/commands/mod.rs +++ b/src/commands/mod.rs @@ -300,6 +300,7 @@ impl DaemonControl { block_info, spend_txid, spend_block, + is_immature, .. } = coin; let spend_info = spend_txid.map(|txid| LCSpendInfo { @@ -312,6 +313,7 @@ impl DaemonControl { outpoint, block_height, spend_info, + is_immature, } }) .collect(); @@ -846,6 +848,8 @@ pub struct ListCoinsEntry { pub block_height: Option, /// Information about the transaction spending this coin. pub spend_info: Option, + /// Whether this coin was created by a coinbase transaction that is still immature. + pub is_immature: bool, } #[derive(Debug, Clone, Serialize, Deserialize)] From 6b82894614df3351d788451211d864ecff3f3544 Mon Sep 17 00:00:00 2001 From: Antoine Poinsot Date: Thu, 20 Jul 2023 13:21:33 +0200 Subject: [PATCH 4/7] bitcoin: track maturity of coinbase deposits We detect and store immature deposits (cause otherwise we won't go through them again with listsinceblock), but mark them as such and unconfirmed. We only mark them as confirmed once they've matured. It's a bit clumsy but it's not as if most of our users had coinbase deposits. --- src/bitcoin/d/mod.rs | 23 ++++++++++++++++++++++- src/bitcoin/mod.rs | 10 ++++++++++ src/bitcoin/poller/looper.rs | 5 ++++- src/database/mod.rs | 3 +++ src/database/sqlite/mod.rs | 11 ++++++++++- 5 files changed, 49 insertions(+), 3 deletions(-) diff --git a/src/bitcoin/d/mod.rs b/src/bitcoin/d/mod.rs index 9d3d670c5..160ecd0a2 100644 --- a/src/bitcoin/d/mod.rs +++ b/src/bitcoin/d/mod.rs @@ -1105,6 +1105,7 @@ pub struct LSBlockEntry { pub block_height: Option, pub address: bitcoin::Address, pub parent_descs: Vec>, + pub is_immature: bool, } impl From<&Json> for LSBlockEntry { @@ -1150,12 +1151,19 @@ impl From<&Json> for LSBlockEntry { }) .expect("bitcoind can't give invalid descriptors"); + let is_immature = json + .get("category") + .and_then(Json::as_str) + .expect("must be present") + == "immature"; + LSBlockEntry { outpoint, amount, block_height, address, parent_descs, + is_immature, } } } @@ -1183,7 +1191,7 @@ impl From for LSBlockRes { .get("category") .and_then(Json::as_str) .expect("must be present"); - if category == "receive" || category == "generate" { + if ["receive", "generate", "immature"].contains(&category) { let lsb_entry: LSBlockEntry = j.into(); Some(lsb_entry) } else { @@ -1201,6 +1209,8 @@ pub struct GetTxRes { pub conflicting_txs: Vec, pub block: Option, pub tx: bitcoin::Transaction, + pub is_coinbase: bool, + pub confirmations: i32, } impl From for GetTxRes { @@ -1238,10 +1248,21 @@ impl From for GetTxRes { let bytes = Vec::from_hex(hex).expect("bitcoind returned a wrong transaction format"); let tx: bitcoin::Transaction = bitcoin::consensus::encode::deserialize(&bytes) .expect("bitcoind returned a wrong transaction format"); + let is_coinbase = json + .get("generated") + .and_then(Json::as_bool) + .unwrap_or(false); + let confirmations = json + .get("confirmations") + .and_then(Json::as_i64) + .expect("Must be present in the response") as i32; + GetTxRes { conflicting_txs: conflicting_txs.unwrap_or_default(), block, tx, + is_coinbase, + confirmations, } } } diff --git a/src/bitcoin/mod.rs b/src/bitcoin/mod.rs index a3b9e01a4..b1d32fae5 100644 --- a/src/bitcoin/mod.rs +++ b/src/bitcoin/mod.rs @@ -14,6 +14,8 @@ use std::{fmt, sync}; use miniscript::bitcoin::{self, address}; +const COINBASE_MATURITY: i32 = 100; + /// Information about a block #[derive(Debug, Clone, Eq, PartialEq, Copy)] pub struct Block { @@ -146,6 +148,7 @@ impl BitcoinInterface for d::BitcoinD { block_height, address, parent_descs, + is_immature, } = entry; if parent_descs .iter() @@ -156,6 +159,7 @@ impl BitcoinInterface for d::BitcoinD { amount, block_height, address, + is_immature, }) } else { None @@ -184,6 +188,11 @@ impl BitcoinInterface for d::BitcoinD { // If the transaction was confirmed, mark the coin as such. if let Some(block) = res.block { + // Do not mark immature coinbase deposits as confirmed until they become mature. + if res.is_coinbase && res.confirmations < COINBASE_MATURITY { + log::debug!("Coin at '{}' comes from an immature coinbase transaction with {} confirmations. Not marking it as confirmed for now.", op, res.confirmations); + continue; + } confirmed.push((*op, block.height, block.time)); continue; } @@ -409,4 +418,5 @@ pub struct UTxO { pub amount: bitcoin::Amount, pub block_height: Option, pub address: bitcoin::Address, + pub is_immature: bool, } diff --git a/src/bitcoin/poller/looper.rs b/src/bitcoin/poller/looper.rs index 8944d92ce..a17267803 100644 --- a/src/bitcoin/poller/looper.rs +++ b/src/bitcoin/poller/looper.rs @@ -24,6 +24,8 @@ struct UpdatedCoins { // or spent. // NOTE: A coin may be updated multiple times at once. That is, a coin may be received, confirmed, // and spent in a single poll. +// NOTE: Coinbase transaction deposits are very much an afterthought here. We treat them as +// unconfirmed until the CB tx matures. fn update_coins( bit: &impl BitcoinInterface, db_conn: &mut Box, @@ -42,6 +44,7 @@ fn update_coins( outpoint, amount, address, + is_immature, .. } = utxo; // We can only really treat them if we know the derivation index that was used. @@ -66,7 +69,7 @@ fn update_coins( if !curr_coins.contains_key(&utxo.outpoint) { let coin = Coin { outpoint, - is_immature: false, + is_immature, amount, derivation_index, is_change, diff --git a/src/database/mod.rs b/src/database/mod.rs index e25acb92c..21b5a32d7 100644 --- a/src/database/mod.rs +++ b/src/database/mod.rs @@ -91,6 +91,9 @@ pub trait DatabaseConnection { fn remove_coins(&mut self, coins: &[bitcoin::OutPoint]); /// Mark a set of coins as being confirmed at a specified height and block time. + /// NOTE: if the coin comes from an immature coinbase transaction, this will mark it as mature. + /// Immature coinbase deposits must not be confirmed before they are 100 blocks deep in the + /// chain. fn confirm_coins(&mut self, outpoints: &[(bitcoin::OutPoint, i32, u32)]); /// Mark a set of coins as being spent by a specified txid of a pending transaction. diff --git a/src/database/sqlite/mod.rs b/src/database/sqlite/mod.rs index 2cf3aa7b3..d05e46639 100644 --- a/src/database/sqlite/mod.rs +++ b/src/database/sqlite/mod.rs @@ -417,6 +417,9 @@ impl SqliteConn { } /// Mark a set of coins as confirmed. + /// + /// NOTE: this will also mark the coin as mature if it originates from an immature coinbase + /// deposit. pub fn confirm_coins<'a>( &mut self, outpoints: impl IntoIterator, @@ -424,7 +427,7 @@ impl SqliteConn { db_exec(&mut self.conn, |db_tx| { for (outpoint, height, time) in outpoints { db_tx.execute( - "UPDATE coins SET blockheight = ?1, blocktime = ?2 WHERE txid = ?3 AND vout = ?4", + "UPDATE coins SET blockheight = ?1, blocktime = ?2, is_immature = 0 WHERE txid = ?3 AND vout = ?4", rusqlite::params![height, time, outpoint.txid[..].to_vec(), outpoint.vout,], )?; } @@ -978,6 +981,12 @@ CREATE TABLE spend_transactions ( assert!(outpoints.contains(&coin_imma.outpoint)); let coin = conn.db_coins(&[coin_imma.outpoint]).pop().unwrap(); assert!(coin.is_immature && !coin.is_change); + + // Confirming an immature coin marks it as mature. + let (height, time) = (424242, 424241); + conn.confirm_coins(&[(coin_imma.outpoint, height, time)]); + let coin = conn.db_coins(&[coin_imma.outpoint]).pop().unwrap(); + assert!(!coin.is_immature); } fs::remove_dir_all(tmp_dir).unwrap(); From 95dd3e52935cf7a2edb1abdee428efe26ccff36c Mon Sep 17 00:00:00 2001 From: Antoine Poinsot Date: Thu, 20 Jul 2023 13:24:15 +0200 Subject: [PATCH 5/7] qa: fix and improve the coinbase deposit functional test --- tests/test_misc.py | 40 ++++++++++++++++++++++++++++++++++------ 1 file changed, 34 insertions(+), 6 deletions(-) diff --git a/tests/test_misc.py b/tests/test_misc.py index 92394c1aa..e562a297a 100644 --- a/tests/test_misc.py +++ b/tests/test_misc.py @@ -2,7 +2,7 @@ from fixtures import * from test_framework.serializations import PSBT -from test_framework.utils import wait_for, RpcError, OLD_LIANAD_PATH, LIANAD_PATH +from test_framework.utils import wait_for, RpcError, OLD_LIANAD_PATH, LIANAD_PATH, COIN from threading import Thread @@ -182,16 +182,44 @@ def test_multipath(lianad_multipath, bitcoind): def test_coinbase_deposit(lianad, bitcoind): """Check we detect deposits from (mature) coinbase transactions.""" - # Create a new deposit in a coinbase transaction. + wait_for_sync = lambda: wait_for( + lambda: lianad.rpc.getinfo()["block_height"] == bitcoind.rpc.getblockcount() + ) + wait_for_sync() + + # Create a new deposit in a coinbase transaction. We must detect it and treat it as immature. addr = lianad.rpc.getnewaddress()["address"] bitcoind.rpc.generatetoaddress(1, addr) - assert len(lianad.rpc.listcoins()["coins"]) == 0 + wait_for_sync() + coins = lianad.rpc.listcoins()["coins"] + assert ( + len(coins) == 1 and coins[0]["is_immature"] and coins[0]["spend_info"] is None + ) - # Generate 100 blocks to make the coinbase mature. + # Generate 100 blocks to make the coinbase mature. We should detect it as such. 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 - # We must have detected a new deposit. - wait_for(lambda: len(lianad.rpc.listcoins()["coins"]) == 1) + # We must be able to spend the mature coin. + destinations = {bitcoind.rpc.getnewaddress(): int(0.999999 * COIN)} + res = lianad.rpc.createspend(destinations, [coin["outpoint"]], 42) + psbt = PSBT.from_base64(res["psbt"]) + txid = psbt.tx.txid().hex() + signed_psbt = lianad.signer.sign_psbt(psbt) + lianad.rpc.updatespend(signed_psbt.to_base64()) + lianad.rpc.broadcastspend(txid) + bitcoind.generate_block(1, wait_for_mempool=txid) + 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 + and coin["spend_info"] is not None + ) @pytest.mark.skipif( From 289f6581cccefad42afa870b382fc22eb0c5e46c Mon Sep 17 00:00:00 2001 From: Antoine Poinsot Date: Thu, 20 Jul 2023 13:41:06 +0200 Subject: [PATCH 6/7] qa: functional test createspend refuses immature outpoints --- tests/test_rpc.py | 10 ++++++++++ 1 file changed, 10 insertions(+) diff --git a/tests/test_rpc.py b/tests/test_rpc.py index 4d1cf06be..119b04c98 100644 --- a/tests/test_rpc.py +++ b/tests/test_rpc.py @@ -132,6 +132,16 @@ def test_create_spend(lianad, bitcoind): # We can sign it and broadcast it. sign_and_broadcast(lianad, bitcoind, PSBT.from_base64(res["psbt"])) + # Try creating a transaction that spends an immature coinbase deposit. + addr = lianad.rpc.getnewaddress()["address"] + bitcoind.rpc.generatetoaddress(1, addr) + wait_for( + lambda: lianad.rpc.getinfo()["block_height"] == bitcoind.rpc.getblockcount() + ) + imma_coin = next(c for c in lianad.rpc.listcoins()["coins"] if c["is_immature"]) + with pytest.raises(RpcError, match=".*is from an immature coinbase transaction."): + lianad.rpc.createspend(destinations, [imma_coin["outpoint"]], 1) + def test_list_spend(lianad, bitcoind): # Start by creating two conflicting Spend PSBTs. The first one will have a change From 097d5e71c19c0c5fc53d1e91087d91edfdd218f3 Mon Sep 17 00:00:00 2001 From: Antoine Poinsot Date: Thu, 20 Jul 2023 13:48:33 +0200 Subject: [PATCH 7/7] qa: adapt the migration functional test to also support 1.0 --- tests/test_misc.py | 7 +++++-- 1 file changed, 5 insertions(+), 2 deletions(-) diff --git a/tests/test_misc.py b/tests/test_misc.py index e562a297a..189b85caa 100644 --- a/tests/test_misc.py +++ b/tests/test_misc.py @@ -232,13 +232,16 @@ def test_migration(lianad_multisig, bitcoind): # Set the old binary and re-create the datadir. lianad.cmd_line[0] = OLD_LIANAD_PATH lianad.restart_fresh(bitcoind) - assert lianad.rpc.getinfo()["version"] == "0.3.0" + old_lianad_ver = lianad.rpc.getinfo()["version"] + assert old_lianad_ver in ["0.3.0", "1.0.0"] # Perform some transactions. On Liana v0.3 there was no "updated_at" for Spend # transaction drafts. receive_and_send(lianad, bitcoind) spend_txs = lianad.rpc.listspendtxs()["spend_txs"] - assert len(spend_txs) == 2 and all("updated_at" not in s for s in spend_txs) + assert len(spend_txs) == 2 + if old_lianad_ver == "0.3.0": + assert all("updated_at" not in s for s in spend_txs) # Set back the new binary. We should be able to read and, if necessary, upgrade # the old database and generally all files from the datadir.