Skip to content

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

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

Closed
wants to merge 5 commits into from
Closed
Show file tree
Hide file tree
Changes from 4 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
Expand Up @@ -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

Expand Down
193 changes: 164 additions & 29 deletions src/wallet/mod.rs
Original file line number Diff line number Diff line change
Expand Up @@ -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`.
///
Expand Down Expand Up @@ -253,36 +260,26 @@ where
.map_err(|_| Error::ScriptDoesntHaveAddressForm)
}

// 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()
// Return whether this address has been used in a transaction
fn has_address_been_used(&self, script_pk: &Script) -> bool {
Copy link
Contributor Author

Choose a reason for hiding this comment

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

actually checking a Script..

Copy link
Contributor

Choose a reason for hiding this comment

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

This guy is better named as is_scriptpubkey_used..

let txns = self.list_transactions(true).unwrap_or_else(|_| vec![]);
txns.iter()
.flat_map(|tx_details| tx_details.transaction.as_ref())
.flat_map(|tx| tx.output.iter())
.any(|o| o.script_pubkey == script_pubkey);
.any(|o| o.script_pubkey == *script_pk)
}

if found_used {
self.get_new_address(keychain)
} else {
derived_key
.address(self.network)
.map(|address| AddressInfo {
address,
index: current_index,
keychain,
})
.map_err(|_| Error::ScriptDoesntHaveAddressForm)
}
// 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_last_unused_address(&self, keychain: KeychainKind) -> Result<AddressInfo, Error> {
let mut unused_addresses = self.get_batch_unused_addresses(1, false, keychain)?;
Ok(unused_addresses.remove(0))
}

// Return the the first address in the keychain which has not been a recipient of a transaction
fn get_first_unused_address(&self, keychain: KeychainKind) -> Result<AddressInfo, Error> {
let mut unused_addresses = self.get_batch_unused_addresses(1, true, keychain)?;
Ok(unused_addresses.remove(0))
}

// Return derived address for the descriptor of given [`KeychainKind`] at a specific index
Expand Down Expand Up @@ -339,7 +336,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),
}
Expand Down Expand Up @@ -389,6 +387,62 @@ where
Ok(new_addresses_cached)
}

/// Return vector of n unused addresses from the [`KeychainKind`].
/// If less than n unused addresses are returned, the rest will be populated by new addresses.
/// The unused addresses returned are in order of oldest in keychain first, with increasing index.
Copy link
Contributor

Choose a reason for hiding this comment

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

This is not as per impl right now.. if from_front is set false, the addresses are returned in reverse order..

pub fn get_batch_unused_addresses(
&self,
n: usize,
from_front: bool,
keychain: KeychainKind,
) -> Result<Vec<AddressInfo>, Error> {
Copy link
Contributor

Choose a reason for hiding this comment

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

I am not feeling comfortable with the API. get_batched_unused should not be concerned with fornt or back. Thats an impl detail for LastUnused or FirstUnused. And should not be exposed in public API..

This is also breaking the doc above. The order is not maintained anymore..

Better to handle the handle the first or last logic in in their respective functions itself than to handle in the batch function which is more generic.

Copy link
Contributor

Choose a reason for hiding this comment

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

The correct thing is to return a impl DoubleEndedIterator over unused addresses I think.

let script_pubkeys = self
.database
.borrow()
.iter_script_pubkeys(Some(keychain))
.unwrap_or_else(|_| vec![]);

let current_address_index = self.fetch_index(keychain)? as usize;
let check_indexes = if from_front {
(0..=current_address_index).collect::<Vec<_>>()
} else {
(0..=current_address_index).rev().collect::<Vec<_>>()
Copy link
Contributor Author

Choose a reason for hiding this comment

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

better way to do this?

Copy link
Contributor

Choose a reason for hiding this comment

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

Return a impl DoubleEndedIterator from the method instead.

};

let mut unused_addresses = vec![];
for i in check_indexes {
// if we have made a pubkey at this index, check whether the address has been used.
if i < script_pubkeys.len() {
let script_pk = &script_pubkeys[i];
if self.has_address_been_used(script_pk) {
Copy link
Contributor

Choose a reason for hiding this comment

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

Here for each spk we are iterating over the entire transaction list. For wallets with large transaction this will can cause massive overhead.

Instead a better way would be to handle Vec<Script> in the has_address_been_used function. Call list_transactions only once, filter out all the spks that haven't been used and return then as a Vec.

Copy link
Contributor Author

Choose a reason for hiding this comment

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

Yep good idea, note this would then also check more addresses than necessary (not breaking early when finding n).
If this was stored in the database it could also break early if it's just a fast read

continue;
}
}
if let Ok(unused_address) = self
.get_descriptor_for_keychain(keychain)
.as_derived(i as u32, &self.secp)
.address(self.network)
.map(|address| AddressInfo {
address,
index: i as u32,
keychain,
})
.map_err(|_| Error::ScriptDoesntHaveAddressForm)
{
unused_addresses.push(unused_address);
}

if unused_addresses.len() >= n {
break;
}
}
Copy link
Contributor

Choose a reason for hiding this comment

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

Try using some rust list comprehensions with iters and maps. Much of this code can be simplified..


for _ in 0..(n - unused_addresses.len()) {
unused_addresses.push(self.get_new_address(keychain)?)
}
Ok(unused_addresses)
}

/// 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)
Expand Down Expand Up @@ -1664,7 +1718,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() {
Expand Down Expand Up @@ -3872,6 +3926,87 @@ pub(crate) mod test {
);
}

#[test]
fn test_firstunused_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..

}

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

// get first two addresses, moving index
for _ in 0..2 {
let _ = wallet.get_address(New);
}

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

assert_eq!(
wallet
.get_batch_unused_addresses(3, true, KeychainKind::External)
.unwrap(),
vec![
AddressInfo {
index: 0,
address: Address::from_str("tb1q6yn66vajcctph75pvylgkksgpp6nq04ppwct9a")
.unwrap(),
keychain: KeychainKind::External,
},
AddressInfo {
index: 2,
address: Address::from_str("tb1qzntf2mqex4ehwkjlfdyy3ewdlk08qkvkvrz7x2")
.unwrap(),
keychain: KeychainKind::External,
},
AddressInfo {
index: 3,
address: Address::from_str("tb1q32a23q6u3yy89l8svrt80a54h06qvn7gnuvsen")
.unwrap(),
keychain: KeychainKind::External,
}
]
);
}
Copy link
Contributor

Choose a reason for hiding this comment

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

Also need to assert that FirstUnused and LastUnused are working as intended..

  • Get 5 new addresses
  • Use only index 0 and 3
  • get_batch(3) should return index 0, 2, 4. current index should still be at 4.
  • get_first_unused() should return 0
  • get_last_unused() should return 4
  • get_batch(4) should return 0,2,4,5, and current index should be at 5.

Copy link
Contributor

Choose a reason for hiding this comment

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

Adding a check of the derivation index here would be good.


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