Skip to content
New issue

Have a question about this project? Sign up for a free GitHub account to open an issue and contact its maintainers and the community.

By clicking “Sign up for GitHub”, you agree to our terms of service and privacy statement. We’ll occasionally send you account related emails.

Already on GitHub? Sign in to your account

AddressIndex improvements: LastUnused, FirstUnused, and get_batch_unused_addresses() #546

Closed
wants to merge 5 commits into from
Closed
Changes from all commits
Commits
File filter

Filter by extension

Filter by extension

Conversations
Failed to load comments.
Loading
Jump to
Jump to file
Failed to load files.
Loading
Diff view
Diff view
3 changes: 3 additions & 0 deletions CHANGELOG.md
Original file line number Diff line number Diff line change
@@ -27,6 +27,9 @@ and this project adheres to [Semantic Versioning](https://semver.org/spec/v2.0.0
- Add `get_internal_address` to allow you to get internal addresses just as you get external addresses.
- added `ensure_addresses_cached` to `Wallet` to let offline wallets load and cache addresses in their database
- Add `is_spent` field to `LocalUtxo`; when we notice that a utxo has been spent we set `is_spent` field to true instead of deleting it from the db.
- Changed `AddressIndex::LastUnused` to look back further than `current_index`, and only return a new address if all have been used.
- Add `AddressIndex::FirstUnused` to get unused addresses from the beginning of the keychain.
- Add `wallet.get_batch_unused_addresses` to return vector of N unused addresses, populating any remaining with new addresses.

### Sync API change

192 changes: 164 additions & 28 deletions src/wallet/mod.rs
Original file line number Diff line number Diff line change
@@ -115,6 +115,13 @@ pub enum AddressIndex {
/// caller is untrusted; for example when deriving donation addresses on-demand for a public
/// web page.
LastUnused,
/// Return the address for the first address in the keychain that has not been used in a received
/// transaction. Otherwise return a new address as with [`AddressIndex::New`].
///
/// Use with caution, if the wallet has not yet detected an address has been used it could
/// return an already used address. This function is primarily meant for making use of addresses earlier
/// in the keychain that were infact never used.
FirstUnused,
/// Return the address for a specific descriptor index. Does not change the current descriptor
/// index used by `AddressIndex::New` and `AddressIndex::LastUsed`.
///
@@ -255,36 +262,25 @@ where

// Return the the last previously derived address for `keychain` if it has not been used in a
// received transaction. Otherwise return a new address using [`Wallet::get_new_address`].
fn get_unused_address(&self, keychain: KeychainKind) -> Result<AddressInfo, Error> {
let current_index = self.fetch_index(keychain)?;

let derived_key = self
.get_descriptor_for_keychain(keychain)
.as_derived(current_index, &self.secp);

let script_pubkey = derived_key.script_pubkey();

let found_used = self
.list_transactions(true)?
.iter()
.flat_map(|tx_details| tx_details.transaction.as_ref())
.flat_map(|tx| tx.output.iter())
.any(|o| o.script_pubkey == script_pubkey);

if found_used {
self.get_new_address(keychain)
fn get_last_unused_address(&self, keychain: KeychainKind) -> Result<AddressInfo, Error> {
let unused_script_indexes = self.get_unused_script_indexes(keychain)?;
let current_index = &self.fetch_index(keychain)?;
if unused_script_indexes.contains(current_index) {
self.get_address(AddressIndex::Peek(*current_index))
} else {
derived_key
.address(self.network)
.map(|address| AddressInfo {
address,
index: current_index,
keychain,
})
.map_err(|_| Error::ScriptDoesntHaveAddressForm)
self.get_new_address(keychain)
}
}

// Return the the first address in the keychain which has not been used in a recieved transaction
// If they have all been used, return a new address using [`Wallet::get_new_address`].
fn get_first_unused_address(&self, keychain: KeychainKind) -> Result<AddressInfo, Error> {
self.get_unused_script_indexes(keychain)?
.get(0)
.map(|index| self.get_address(AddressIndex::Peek(*index)))
.unwrap_or_else(|| self.get_new_address(keychain))
}

// Return derived address for the descriptor of given [`KeychainKind`] at a specific index
fn peek_address(&self, index: u32, keychain: KeychainKind) -> Result<AddressInfo, Error> {
self.get_descriptor_for_keychain(keychain)
@@ -339,7 +335,8 @@ where
) -> Result<AddressInfo, Error> {
match address_index {
AddressIndex::New => self.get_new_address(keychain),
AddressIndex::LastUnused => self.get_unused_address(keychain),
AddressIndex::LastUnused => self.get_last_unused_address(keychain),
AddressIndex::FirstUnused => self.get_first_unused_address(keychain),
AddressIndex::Peek(index) => self.peek_address(index, keychain),
AddressIndex::Reset(index) => self.reset_address(index, keychain),
}
@@ -389,6 +386,41 @@ where
Ok(new_addresses_cached)
}

/// Return set of unused script indexes for the [`KeychainKind`].
pub fn get_unused_script_indexes(&self, keychain: KeychainKind) -> Result<Vec<u32>, Error> {
let script_pubkeys = self
.database
.borrow()
.iter_script_pubkeys(Some(keychain))
.unwrap_or_else(|_| vec![]);
let txs = self.list_transactions(true).unwrap_or_else(|_| vec![]);
let tx_scripts: HashSet<&Script> = txs
.iter()
.flat_map(|tx_details| tx_details.transaction.as_ref())
.flat_map(|tx| tx.output.iter())
.map(|o| &o.script_pubkey)
.collect();
let current_address_index = self.fetch_index(keychain)? as usize;

let mut scripts_not_used: Vec<u32> = script_pubkeys
.iter()
.take(current_address_index + 1)
.enumerate()
.filter_map(|(i, script_pubkey)| {
if !tx_scripts.contains(script_pubkey) {
Some(i as u32)
} else {
None
}
})
.collect();
if script_pubkeys.is_empty() {
scripts_not_used.push(0);
}

Ok(scripts_not_used)
}

/// Return whether or not a `script` is part of this wallet (either internal or external)
pub fn is_mine(&self, script: &Script) -> Result<bool, Error> {
self.database.borrow().is_mine(script)
@@ -1664,7 +1696,7 @@ pub(crate) mod test {

use super::*;
use crate::signer::{SignOptions, SignerError};
use crate::wallet::AddressIndex::{LastUnused, New, Peek, Reset};
use crate::wallet::AddressIndex::{FirstUnused, LastUnused, New, Peek, Reset};

#[test]
fn test_cache_addresses_fixed() {
@@ -3872,6 +3904,110 @@ pub(crate) mod test {
);
}

#[test]
fn test_first_unused_address() {
let descriptor = "wpkh(tpubEBr4i6yk5nf5DAaJpsi9N2pPYBeJ7fZ5Z9rmN4977iYLCGco1VyjB9tvvuvYtfZzjD5A8igzgw3HeWeeKFmanHYqksqZXYXGsw5zjnj7KM9/*)";
let descriptors = testutils!(@descriptors (descriptor));
let wallet = Wallet::new(
&descriptors.0,
None,
Network::Testnet,
MemoryDatabase::new(),
)
.unwrap();

assert_eq!(
wallet.get_address(FirstUnused).unwrap().to_string(),
"tb1q6yn66vajcctph75pvylgkksgpp6nq04ppwct9a"
);

// use the first address
crate::populate_test_db!(
wallet.database.borrow_mut(),
testutils! (@tx ( (@external descriptors, 0) => 25_000 ) (@confirmations 1)),
Some(100),
);

assert_eq!(
wallet.get_address(FirstUnused).unwrap().to_string(),
"tb1q4er7kxx6sssz3q7qp7zsqsdx4erceahhax77d7"
);
Comment on lines +3924 to +3934
Copy link
Contributor

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

I would like to see the test situation here where we extract multiple addresses, use some of them and get back a previous unused one when called again.. That would correctly test the intended behavior.. Right now its just testing the vanilla situation..

Copy link
Contributor

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

Hmm I actually don't know a better test to write than this one? With the batch unused you can write a more complicated test but with FristUnused there's not much you can do.

Copy link
Contributor

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

Something like derive a bunch of address.. Only use some of them selectively so the address gaps are simulated.. Then check if the first unused is returned correctly.. Am I missing some details why that can't work??

Copy link
Contributor

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

It can work but I don't get why the gaps would effect the algorithm that finds the first unused. I mean I don't think that this will likely find a problem with the algorithm that this test wouldn't find.

Copy link
Contributor

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

Its not that the gaps would affect the algorithm, but to confirm that the behavior we are intending here is actually happening.. And this can be checked in single test for both first and last unused.. Once the behavior is pinned, we can decide later which one to use when or to keep both..


// use the third address
crate::populate_test_db!(
wallet.database.borrow_mut(),
testutils! (@tx ( (@external descriptors, 2) => 25_000 ) (@confirmations 1)),
Some(100),
);

assert_eq!(
wallet.get_address(FirstUnused).unwrap().to_string(),
"tb1q4er7kxx6sssz3q7qp7zsqsdx4erceahhax77d7"
);
}

#[test]
fn test_get_unused_address_indexes() {
let descriptor = "wpkh(tpubEBr4i6yk5nf5DAaJpsi9N2pPYBeJ7fZ5Z9rmN4977iYLCGco1VyjB9tvvuvYtfZzjD5A8igzgw3HeWeeKFmanHYqksqZXYXGsw5zjnj7KM9/*)";
let descriptors = testutils!(@descriptors (descriptor));
let wallet = Wallet::new(
&descriptors.0,
None,
Network::Testnet,
MemoryDatabase::new(),
)
.unwrap();

assert_eq!(
wallet
.get_unused_script_indexes(KeychainKind::External)
.unwrap(),
vec![0]
);

// get four more addresses, moving index to five
for _ in 0..4 {
let _ = wallet.get_address(New);
}
assert_eq!(
wallet
.get_unused_script_indexes(KeychainKind::External)
.unwrap(),
vec![0, 1, 2, 3, 4]
);

// use the second and fifth address
crate::populate_test_db!(
wallet.database.borrow_mut(),
testutils! (@tx ( (@external descriptors, 1) => 25_000 ) (@confirmations 1)),
Some(100),
);
crate::populate_test_db!(
wallet.database.borrow_mut(),
testutils! (@tx ( (@external descriptors, 4) => 25_000 ) (@confirmations 1)),
Some(100),
);
assert_eq!(
wallet
.get_unused_script_indexes(KeychainKind::External)
.unwrap(),
vec![0, 2, 3]
);

// use the first address
crate::populate_test_db!(
wallet.database.borrow_mut(),
testutils! (@tx ( (@external descriptors, 0) => 25_000 ) (@confirmations 1)),
Some(100),
);
assert_eq!(
wallet
.get_unused_script_indexes(KeychainKind::External)
.unwrap(),
vec![2, 3]
);
}

#[test]
fn test_peek_address_at_index() {
let db = MemoryDatabase::new();