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); } } 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 { 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)