From 42d2ffeec12edb22a789d92d8f83c8b535fc0477 Mon Sep 17 00:00:00 2001 From: Antoine Poinsot Date: Wed, 16 Nov 2022 15:19:50 +0100 Subject: [PATCH 1/3] db: more flexible interface for updating our next derivation indexes. Instead of only incrementing it, allow to be able to set it to any value. This will be useful for instance to set the derivation to the latest used onchain, if another wallet is much further down the derivation tree than we are (or after a rescan). --- src/commands/mod.rs | 15 +++-- src/database/mod.rs | 38 +++++++++--- src/database/sqlite/mod.rs | 114 ++++++++++++++++++++--------------- src/database/sqlite/utils.rs | 35 +---------- src/testutils.rs | 22 ++++--- 5 files changed, 121 insertions(+), 103 deletions(-) diff --git a/src/commands/mod.rs b/src/commands/mod.rs index 05e60e53..7fbab980 100644 --- a/src/commands/mod.rs +++ b/src/commands/mod.rs @@ -219,8 +219,10 @@ impl DaemonControl { pub fn get_new_address(&self) -> GetAddressResult { let mut db_conn = self.db.connection(); let index = db_conn.receive_index(); - // TODO: should we wrap around instead of failing? - db_conn.increment_receive_index(&self.secp); + let new_index = index + .increment() + .expect("Can't get into hardened territory"); + db_conn.set_receive_index(new_index, &self.secp); let address = self .config .main_descriptor @@ -362,12 +364,17 @@ impl DaemonControl { // an added output* (for the change). if nochange_feerate_vb > feerate_vb { // Get the change address to create a dummy change txo. + let change_index = db_conn.change_index(); let change_desc = self .config .main_descriptor .change_descriptor() - .derive(db_conn.change_index(), &self.secp); - db_conn.increment_change_index(&self.secp); + .derive(change_index, &self.secp); + // Don't forget to update our next change index! + let next_index = change_index + .increment() + .expect("Must not get into hardened territory"); + db_conn.set_change_index(next_index, &self.secp); let mut change_txo = bitcoin::TxOut { value: std::u64::MAX, script_pubkey: change_desc.script_pubkey(), diff --git a/src/database/mod.rs b/src/database/mod.rs index 263f3442..095fe40f 100644 --- a/src/database/mod.rs +++ b/src/database/mod.rs @@ -45,13 +45,25 @@ pub trait DatabaseConnection { /// Update our best chain seen. fn update_tip(&mut self, tip: &BlockChainTip); + /// Get the derivation index for the next receiving address fn receive_index(&mut self) -> bip32::ChildNumber; + /// Set the derivation index for the next receiving address + fn set_receive_index( + &mut self, + index: bip32::ChildNumber, + secp: &secp256k1::Secp256k1, + ); + + /// Get the derivation index for the next change address fn change_index(&mut self) -> bip32::ChildNumber; - fn increment_receive_index(&mut self, secp: &secp256k1::Secp256k1); - - fn increment_change_index(&mut self, secp: &secp256k1::Secp256k1); + /// Set the derivation index for the next change address + fn set_change_index( + &mut self, + index: bip32::ChildNumber, + secp: &secp256k1::Secp256k1, + ); /// Get the timestamp at which to start rescaning from, if any. fn rescan_timestamp(&mut self) -> Option; @@ -131,16 +143,24 @@ impl DatabaseConnection for SqliteConn { self.db_wallet().deposit_derivation_index } + fn set_receive_index( + &mut self, + index: bip32::ChildNumber, + secp: &secp256k1::Secp256k1, + ) { + self.set_derivation_index(index, false, secp) + } + fn change_index(&mut self) -> bip32::ChildNumber { self.db_wallet().change_derivation_index } - fn increment_receive_index(&mut self, secp: &secp256k1::Secp256k1) { - self.increment_deposit_index(secp) - } - - fn increment_change_index(&mut self, secp: &secp256k1::Secp256k1) { - self.increment_change_index(secp) + fn set_change_index( + &mut self, + index: bip32::ChildNumber, + secp: &secp256k1::Secp256k1, + ) { + self.set_derivation_index(index, true, secp) } fn rescan_timestamp(&mut self) -> Option { diff --git a/src/database/sqlite/mod.rs b/src/database/sqlite/mod.rs index c942eef5..b11d8f3d 100644 --- a/src/database/sqlite/mod.rs +++ b/src/database/sqlite/mod.rs @@ -14,7 +14,7 @@ use crate::{ database::{ sqlite::{ schema::{DbAddress, DbCoin, DbSpendTransaction, DbTip, DbWallet}, - utils::{create_fresh_db, db_exec, db_query, db_tx_query, populate_address_mapping}, + utils::{create_fresh_db, db_exec, db_query, db_tx_query, LOOK_AHEAD_LIMIT}, }, Coin, }, @@ -24,8 +24,11 @@ use crate::{ use std::{cmp, convert::TryInto, fmt, io, path}; use miniscript::bitcoin::{ - self, consensus::encode, hashes::hex::ToHex, secp256k1, - util::psbt::PartiallySignedTransaction as Psbt, + self, + consensus::encode, + hashes::hex::ToHex, + secp256k1, + util::{bip32, psbt::PartiallySignedTransaction as Psbt}, }; const DB_VERSION: i64 = 0; @@ -210,61 +213,61 @@ impl SqliteConn { .expect("Database must be available") } - pub fn increment_deposit_index(&mut self, secp: &secp256k1::Secp256k1) { + /// Set the derivation index for receiving or change addresses. + /// + /// This will populate the address->deriv_index mapping with all the new entries between the + /// former and new gap limit indexes. + pub fn set_derivation_index( + &mut self, + index: bip32::ChildNumber, + change: bool, + secp: &secp256k1::Secp256k1, + ) { let network = self.db_tip().network; db_exec(&mut self.conn, |db_tx| { let db_wallet: DbWallet = db_tx_query(db_tx, "SELECT * FROM wallets", rusqlite::params![], |row| { row.try_into() - }) - .expect("Db must not fail") + })? .pop() .expect("There is always a row in the wallet table"); - let next_index: u32 = db_wallet - .deposit_derivation_index - .increment() - .expect("Must not get in hardened territory") - .into(); - // NOTE: should be updated if we ever have multi-wallet support - db_tx.execute( - "UPDATE wallets SET deposit_derivation_index = (?1)", - rusqlite::params![next_index], - )?; - if next_index > db_wallet.change_derivation_index.into() { - populate_address_mapping(db_tx, &db_wallet, next_index, network, secp)?; + // First of all set the derivation index + let index_u32: u32 = index.into(); + if change { + db_tx.execute( + "UPDATE wallets SET change_derivation_index = (?1)", + rusqlite::params![index_u32], + )?; + } else { + db_tx.execute( + "UPDATE wallets SET deposit_derivation_index = (?1)", + rusqlite::params![index_u32], + )?; } - Ok(()) - }) - .expect("Database must be available") - } + // Now if this new index is higher than the highest of our current derivation indexes, + // populate the addresses mapping for derivation indexes between our previous "gap + // limit index" and the new one. + let curr_highest_index = cmp::max( + db_wallet.deposit_derivation_index, + db_wallet.change_derivation_index, + ).into(); + if index_u32 > curr_highest_index { + let receive_desc = db_wallet.main_descriptor.receive_descriptor(); + let change_desc = db_wallet.main_descriptor.change_descriptor(); - pub fn increment_change_index(&mut self, secp: &secp256k1::Secp256k1) { - let network = self.db_tip().network; + for index in curr_highest_index + 1..=index_u32 { + let la_index = index + LOOK_AHEAD_LIMIT - 1; + let receive_addr = receive_desc.derive(la_index.into(), secp).address(network); + let change_addr = change_desc.derive(la_index.into(), secp).address(network); + db_tx.execute( + "INSERT INTO addresses (receive_address, change_address, derivation_index) VALUES (?1, ?2, ?3)", + rusqlite::params![receive_addr.to_string(), change_addr.to_string(), la_index], + )?; + } - db_exec(&mut self.conn, |db_tx| { - let db_wallet: DbWallet = - db_tx_query(db_tx, "SELECT * FROM wallets", rusqlite::params![], |row| { - row.try_into() - }) - .expect("Db must not fail") - .pop() - .expect("There is always a row in the wallet table"); - let next_index: u32 = db_wallet - .change_derivation_index - .increment() - .expect("Must not get in hardened territory") - .into(); - // NOTE: should be updated if we ever have multi-wallet support - db_tx.execute( - "UPDATE wallets SET change_derivation_index = (?1)", - rusqlite::params![next_index], - )?; - - if next_index > db_wallet.deposit_derivation_index.into() { - populate_address_mapping(db_tx, &db_wallet, next_index, network, secp)?; } Ok(()) @@ -807,7 +810,7 @@ mod tests { assert!(conn.db_address(&addr).is_none()); // But if we increment the deposit derivation index, the 200th one will be there. - conn.increment_deposit_index(&secp); + conn.set_derivation_index(1.into(), false, &secp); let db_addr = conn.db_address(&addr).unwrap(); assert_eq!(db_addr.derivation_index, 200.into()); @@ -829,11 +832,11 @@ mod tests { assert!(conn.db_address(&addr).is_none()); // If we increment the *change* derivation index to 1, it will still not be there. - conn.increment_change_index(&secp); + conn.set_derivation_index(1.into(), true, &secp); assert!(conn.db_address(&addr).is_none()); - // But doing it once again it will be there for both change and receive. - conn.increment_change_index(&secp); + // But incrementing it once again it will be there for both change and receive. + conn.set_derivation_index(2.into(), true, &secp); let db_addr = conn.db_address(&addr).unwrap(); assert_eq!(db_addr.derivation_index, 201.into()); let addr = options @@ -843,6 +846,19 @@ mod tests { .address(options.bitcoind_network); let db_addr = conn.db_address(&addr).unwrap(); assert_eq!(db_addr.derivation_index, 201.into()); + + // Now setting it to a much higher will fill all the addresses within the gap + conn.set_derivation_index(52.into(), true, &secp); + for index in 2..52 { + let look_ahead_index = 200 + index; + let addr = options + .main_descriptor + .receive_descriptor() + .derive(look_ahead_index.into(), &secp) + .address(options.bitcoind_network); + let db_addr = conn.db_address(&addr).unwrap(); + assert_eq!(db_addr.derivation_index, look_ahead_index.into()); + } } fs::remove_dir_all(&tmp_dir).unwrap(); diff --git a/src/database/sqlite/utils.rs b/src/database/sqlite/utils.rs index c091d188..87b6555d 100644 --- a/src/database/sqlite/utils.rs +++ b/src/database/sqlite/utils.rs @@ -1,11 +1,8 @@ -use crate::database::sqlite::{ - schema::{DbWallet, SCHEMA}, - FreshDbOptions, SqliteDbError, DB_VERSION, -}; +use crate::database::sqlite::{schema::SCHEMA, FreshDbOptions, SqliteDbError, DB_VERSION}; use std::{convert::TryInto, fs, path, time}; -use miniscript::bitcoin::{self, secp256k1}; +use miniscript::bitcoin::secp256k1; pub const LOOK_AHEAD_LIMIT: u32 = 200; @@ -137,31 +134,3 @@ pub fn create_fresh_db( Ok(()) } - -/// Insert the deposit and change addresses for this index in the address->index mapping table -pub fn populate_address_mapping( - db_tx: &rusqlite::Transaction, - db_wallet: &DbWallet, - next_index: u32, - network: bitcoin::Network, - secp: &secp256k1::Secp256k1, -) -> rusqlite::Result<()> { - // Update the address to derivation index mapping. - let next_la_index = next_index + LOOK_AHEAD_LIMIT - 1; - let next_receive_address = db_wallet - .main_descriptor - .receive_descriptor() - .derive(next_la_index.into(), secp) - .address(network); - let next_change_address = db_wallet - .main_descriptor - .change_descriptor() - .derive(next_la_index.into(), secp) - .address(network); - db_tx.execute( - "INSERT INTO addresses (receive_address, change_address, derivation_index) VALUES (?1, ?2, ?3)", - rusqlite::params![next_receive_address.to_string(), next_change_address.to_string(), next_la_index], - )?; - - Ok(()) -} diff --git a/src/testutils.rs b/src/testutils.rs index 7768cae9..51933dc1 100644 --- a/src/testutils.rs +++ b/src/testutils.rs @@ -145,18 +145,24 @@ impl DatabaseConnection for DummyDbConn { self.db.read().unwrap().deposit_index } + fn set_receive_index( + &mut self, + index: bip32::ChildNumber, + _: &secp256k1::Secp256k1, + ) { + self.db.write().unwrap().deposit_index = index; + } + fn change_index(&mut self) -> bip32::ChildNumber { self.db.read().unwrap().deposit_index } - fn increment_receive_index(&mut self, _: &secp256k1::Secp256k1) { - let next_index = self.db.write().unwrap().deposit_index.increment().unwrap(); - self.db.write().unwrap().deposit_index = next_index; - } - - fn increment_change_index(&mut self, _: &secp256k1::Secp256k1) { - let next_index = self.db.write().unwrap().change_index.increment().unwrap(); - self.db.write().unwrap().change_index = next_index; + fn set_change_index( + &mut self, + index: bip32::ChildNumber, + _: &secp256k1::Secp256k1, + ) { + self.db.write().unwrap().change_index = index; } fn coins(&mut self) -> HashMap { From b31673de327bc968c4046267ef5d0433b80ddd7e Mon Sep 17 00:00:00 2001 From: Antoine Poinsot Date: Wed, 16 Nov 2022 15:31:53 +0100 Subject: [PATCH 2/3] poller: update our internal derivation index also from the chain Fixes #81 --- src/bitcoin/poller/looper.rs | 30 +++++++++++++++++++++++------- 1 file changed, 23 insertions(+), 7 deletions(-) diff --git a/src/bitcoin/poller/looper.rs b/src/bitcoin/poller/looper.rs index 8aa14101..77ff6b27 100644 --- a/src/bitcoin/poller/looper.rs +++ b/src/bitcoin/poller/looper.rs @@ -9,7 +9,7 @@ use std::{ thread, time, }; -use miniscript::bitcoin; +use miniscript::bitcoin::{self, secp256k1}; #[derive(Debug, Clone)] struct UpdatedCoins { @@ -28,6 +28,7 @@ fn update_coins( db_conn: &mut Box, previous_tip: &BlockChainTip, descs: &[descriptors::InheritanceDescriptor], + secp: &secp256k1::Secp256k1, ) -> UpdatedCoins { let curr_coins = db_conn.coins(); log::debug!("Current coins: {:?}", curr_coins); @@ -35,9 +36,20 @@ fn update_coins( // Start by fetching newly received coins. let mut received = Vec::new(); for utxo in bit.received_coins(previous_tip, descs) { + // We can only really treat them if we know the derivation index that was used. if let Some((derivation_index, is_change)) = db_conn.derivation_index_by_address(&utxo.address) { + // First of if we are receiving coins that are beyond our next derivation index, + // adjust it. + if derivation_index > db_conn.receive_index() { + db_conn.set_receive_index(derivation_index, secp); + } + if derivation_index > db_conn.change_index() { + db_conn.set_change_index(derivation_index, secp); + } + + // Now record this coin as a newly received one. if !curr_coins.contains_key(&utxo.outpoint) { let UTxO { outpoint, amount, .. @@ -55,6 +67,7 @@ fn update_coins( received.push(coin); } } else { + // TODO: maybe we could try out something here? Like bruteforcing the next 200 indexes? log::error!( "Could not get derivation index for coin '{}' (address: '{}')", &utxo.outpoint, @@ -170,6 +183,7 @@ fn updates( bit: &impl BitcoinInterface, db: &impl DatabaseInterface, descs: &[descriptors::InheritanceDescriptor], + secp: &secp256k1::Secp256k1, ) { let mut db_conn = db.connection(); @@ -183,18 +197,18 @@ fn updates( // between our former chain and the new one, then restart fresh. db_conn.rollback_tip(&new_tip); log::info!("Tip was rolled back to '{}'.", new_tip); - return updates(bit, db, descs); + return updates(bit, db, descs, secp); } }; // Then check the state of our coins. Do it even if the tip did not change since last poll, as // we may have unconfirmed transactions. - let updated_coins = update_coins(bit, &mut db_conn, ¤t_tip, descs); + let updated_coins = update_coins(bit, &mut db_conn, ¤t_tip, descs, secp); // If the tip changed while we were polling our Bitcoin interface, start over. if bit.chain_tip() != latest_tip { log::info!("Chain tip changed while we were updating our state. Starting over."); - return updates(bit, db, descs); + return updates(bit, db, descs, secp); } // The chain tip did not change since we started our updates. Record them and the latest tip. @@ -217,6 +231,7 @@ fn rescan_check( bit: &impl BitcoinInterface, db: &impl DatabaseInterface, descs: &[descriptors::InheritanceDescriptor], + secp: &secp256k1::Secp256k1, ) { log::debug!("Checking the state of an ongoing rescan if there is any"); let mut db_conn = db.connection(); @@ -254,7 +269,7 @@ fn rescan_check( "Rolling back our internal tip to '{}' to update our internal state with past transactions.", rescan_tip ); - updates(bit, db, descs) + updates(bit, db, descs, secp) } else { log::debug!("No ongoing rescan."); } @@ -285,6 +300,7 @@ pub fn looper( desc.receive_descriptor().clone(), desc.change_descriptor().clone(), ]; + let secp = secp256k1::Secp256k1::verification_only(); maybe_initialize_tip(&bit, &db); @@ -316,7 +332,7 @@ pub fn looper( } } - updates(&bit, &db, &descs); - rescan_check(&bit, &db, &descs); + updates(&bit, &db, &descs, &secp); + rescan_check(&bit, &db, &descs, &secp); } } From 94d8ea6d600479d00e08992aaf153e136e5198fa Mon Sep 17 00:00:00 2001 From: Antoine Poinsot Date: Wed, 16 Nov 2022 15:42:13 +0100 Subject: [PATCH 3/3] qa: test "next derivation index" update from onchain usage --- tests/test_rpc.py | 13 +++++++++++++ 1 file changed, 13 insertions(+) diff --git a/tests/test_rpc.py b/tests/test_rpc.py index 25106ff6..1b3a576c 100644 --- a/tests/test_rpc.py +++ b/tests/test_rpc.py @@ -264,6 +264,8 @@ def test_broadcast_spend(minisafed, bitcoind): def test_start_rescan(minisafed, bitcoind): """Test we successfully retrieve all our transactions after losing state by rescanning.""" initial_timestamp = int(time.time()) + first_address = minisafed.rpc.getnewaddress() + second_address = minisafed.rpc.getnewaddress() # Some utility functions to DRY list_coins = lambda: minisafed.rpc.listcoins()["coins"] @@ -320,6 +322,9 @@ def test_start_rescan(minisafed, bitcoind): lambda: minisafed.rpc.getinfo()["blockheight"] == bitcoind.rpc.getblockcount() ) + # Receiving addresses are derived at much higher indexes now. + assert minisafed.rpc.getnewaddress() not in (first_address, second_address) + # Move time forward one day as bitcoind will rescan the last 2 hours of block upon # importing a descriptor. now = int(time.time()) @@ -334,6 +339,10 @@ def test_start_rescan(minisafed, bitcoind): minisafed.restart_fresh(bitcoind) assert len(list_coins()) == 0 + # The wallet isn't aware what derivation indexes were used. Necessarily it'll start + # from 0. + assert minisafed.rpc.getnewaddress() == first_address + # Once the rescan is done, we must have detected all previous transactions. minisafed.rpc.startrescan(initial_timestamp) rescan_progress = minisafed.rpc.getinfo()["rescan_progress"] @@ -343,3 +352,7 @@ def test_start_rescan(minisafed, bitcoind): lambda: minisafed.rpc.getinfo()["blockheight"] == bitcoind.rpc.getblockcount() ) assert coins_before == sorted_coins() + + # Now that it caught up it noticed which one were used onchain, so it won't reuse + # this derivation indexes anymore. + assert minisafed.rpc.getnewaddress() not in (first_address, second_address)