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

Merge #93: Derivation index update from the chain

Browse Source 
94d8ea6d600479d00e08992aaf153e136e5198fa qa: test "next derivation index" update from onchain usage (Antoine Poinsot)
b31673de327bc968c4046267ef5d0433b80ddd7e poller: update our internal derivation index also from the chain (Antoine Poinsot)
42d2ffeec12edb22a789d92d8f83c8b535fc0477 db: more flexible interface for updating our next derivation indexes. (Antoine Poinsot)

Pull request description:

  Currently, we wouldn't update our "next derivation index" to derive receive/change addresses if we noticed an address from a higher derivation index was used onchain. See #81. This fixes it.

  Note that it can be tempting to set it with some leeway (for instance if we notice index `n` was used, we set our next derivation index to `n + 10`) since other wallets probably have generated addresses for more addresses than were actually used onchain.
  However, i think it could have cascading effects in a multisig situation: A would use `n`, B would set its to `n + 10` and use `n + 11`, C would set its to `n + 21` and use `n + 22`. This is clear that we could end up very far indeed down the derivation tree without having used most indexes.

  A functional test is included, demonstrating the behaviour after a rescan where we lost the database (and therefore knowledge of the previous "next derivation index"). It is in a separate commit to help reviewers attest it would fail on master and pass on this branch.

ACKs for top commit:
  darosior:
    self-ACK 94d8ea6d600479d00e08992aaf153e136e5198fa

Tree-SHA512: 67a44ac28dae391c23dd5143e9d560678e1928eddbfd2fc7ed721e02227151a8ea9571a867b6dd0e93f25ebe59adbdf846a8851c63a89144a818de870723b5da
This commit is contained in:
Antoine Poinsot 2022-11-17 14:48:09 +01:00
commit 3ce9d4f7ab
No known key found for this signature in database
GPG Key ID: E13FC145CD3F4304
7 changed files with 157 additions and 110 deletions
Show Stats Download Patch File Download Diff File Expand all files Collapse all files

30
src/bitcoin/poller/looper.rs
View File

@ -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<dyn DatabaseConnection>,
previous_tip: &BlockChainTip,
descs: &[descriptors::InheritanceDescriptor],
secp: &secp256k1::Secp256k1<secp256k1::VerifyOnly>,
) -> 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<secp256k1::VerifyOnly>,
) {
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, &current_tip, descs);
let updated_coins = update_coins(bit, &mut db_conn, &current_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<secp256k1::VerifyOnly>,
) {
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);
}
}

15
src/commands/mod.rs
View File

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

38
src/database/mod.rs
View File

@ -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<secp256k1::VerifyOnly>,
);
/// 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<secp256k1::VerifyOnly>);
fn increment_change_index(&mut self, secp: &secp256k1::Secp256k1<secp256k1::VerifyOnly>);
/// Set the derivation index for the next change address
fn set_change_index(
&mut self,
index: bip32::ChildNumber,
secp: &secp256k1::Secp256k1<secp256k1::VerifyOnly>,
);
/// Get the timestamp at which to start rescaning from, if any.
fn rescan_timestamp(&mut self) -> Option<u32>;
@ -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<secp256k1::VerifyOnly>,
) {
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<secp256k1::VerifyOnly>) {
self.increment_deposit_index(secp)
}
fn increment_change_index(&mut self, secp: &secp256k1::Secp256k1<secp256k1::VerifyOnly>) {
self.increment_change_index(secp)
fn set_change_index(
&mut self,
index: bip32::ChildNumber,
secp: &secp256k1::Secp256k1<secp256k1::VerifyOnly>,
) {
self.set_derivation_index(index, true, secp)
}
fn rescan_timestamp(&mut self) -> Option<u32> {

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

@ -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<secp256k1::VerifyOnly>) {
/// 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<secp256k1::VerifyOnly>,
) {
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<secp256k1::VerifyOnly>) {
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();

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

@ -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<secp256k1::VerifyOnly>,
) -> 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(())
}

22
src/testutils.rs
View File

@ -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<secp256k1::VerifyOnly>,
) {
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<secp256k1::VerifyOnly>) {
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<secp256k1::VerifyOnly>) {
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<secp256k1::VerifyOnly>,
) {
self.db.write().unwrap().change_index = index;
}
fn coins(&mut self) -> HashMap<bitcoin::OutPoint, Coin> {

13
tests/test_rpc.py
View File

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