Skip to content

Commit

Permalink
Merge #1269: Revamp KeychainTxOutIndex API to be safer
Browse files Browse the repository at this point in the history 
71fff16 feat(chain): add txout methods to `KeychainTxOutIndex` (志宇)
83e7b7e docs(chain): improve `KeychainTxOutIndex` docs (志宇)
9294e30 docs(wallet): improve docs for unbounded spk iterator methods (志宇)
b74c2e2 fix(wallet): use efficient peek address logic (志宇)
81aeaba feat(chain): add `SpkIterator::descriptor` method (志宇)
c7b47af refactor(chain)!: revamp `KeychainTxOutIndex` API (志宇)
705690e feat(chain): make output of `SpkTxOutIndex::unused_spks` cloneable (志宇)

Pull request description:

  Closes #1268

  ### Description

  Previously `SpkTxOutIndex` methods can be called from `KeychainTxOutIndex` due to the `DeRef` implementation. However, the internal `SpkTxOut` will also contain lookahead spks resulting in an error-prone API.

  `SpkTxOutIndex` methods are now not directly callable from `KeychainTxOutIndex`. Methods of `KeychainTxOutIndex` are renamed for clarity. I.e. methods that return an unbounded spk iter are prefixed with `unbounded`.

  In addition to this, I also optimized the peek-address logic of `bdk::Wallet` using the optimized `<SpkIterator as Iterator>::nth` implementation.

  ### Notes to the reviewers

  This is mostly refactoring, but can also be considered a bug-fix (as the API before was very problematic).

  ### Changelog notice

  Changed
  * Wallet's peek-address logic is optimized by making use of `<SpkIterator as Iterator>::nth`.
  * `KeychainTxOutIndex` API is refactored to better differentiate between methods that return unbounded vs stored spks.
  * `KeychainTxOutIndex` no longer directly exposes `SpkTxOutIndex` methods via `DeRef`. This was problematic because `SpkTxOutIndex` also contains lookahead spks which we want to hide.

  Added
  * `SpkIterator::descriptor` method which gets a reference to the internal descriptor.

  ### Checklists

  #### All Submissions:

  * [x] I've signed all my commits
  * [x] I followed the [contribution guidelines](https://github.com/bitcoindevkit/bdk/blob/master/CONTRIBUTING.md)
  * [x] I ran `cargo fmt` and `cargo clippy` before committing

  #### New Features:

  ~* [ ] I've added tests for the new feature~
  * [x] I've added docs for the new feature

  #### Bugfixes:

  * [x] This pull request breaks the existing API
  * [ ] I've added tests to reproduce the issue which are now passing
  * [x] I'm linking the issue being fixed by this PR

ACKs for top commit:
  danielabrozzoni:
    ACK 71fff16

Tree-SHA512: f29c7d2311d0e81c4fe29b8f57c219c24db958194fad5de82bb6d42d562d37fd5d152be7ee03a3f00843be5760569ad29b848250267a548d7d15320fd5292a8f
  • Loading branch information
@danielabrozzoni
danielabrozzoni committed Jan 18, 2024
2 parents 40f0765 + 71fff16 commit 60abd87
Show file tree
Hide file tree
Showing 12 changed files with 345 additions and 172 deletions.
79 changes: 53 additions & 26 deletions crates/bdk/src/wallet/mod.rs
View file
Original file line number Diff line number Diff line change
Expand Up @@ -262,6 +262,11 @@ where
/// Infallibly return a derived address using the external descriptor, see [`AddressIndex`] for
/// available address index selection strategies. If none of the keys in the descriptor are derivable
/// (i.e. does not end with /*) then the same address will always be returned for any [`AddressIndex`].
///
/// # Panics
///
/// This panics when the caller requests for an address of derivation index greater than the
/// BIP32 max index.
pub fn get_address(&mut self, address_index: AddressIndex) -> AddressInfo {
self.try_get_address(address_index).unwrap()
}
Expand All @@ -273,6 +278,11 @@ where
/// see [`AddressIndex`] for available address index selection strategies. If none of the keys
/// in the descriptor are derivable (i.e. does not end with /*) then the same address will always
/// be returned for any [`AddressIndex`].
///
/// # Panics
///
/// This panics when the caller requests for an address of derivation index greater than the
/// BIP32 max index.
pub fn get_internal_address(&mut self, address_index: AddressIndex) -> AddressInfo {
self.try_get_internal_address(address_index).unwrap()
}
Expand Down Expand Up @@ -649,6 +659,11 @@ impl<D> Wallet<D> {
///
/// A `PersistBackend<ChangeSet>::WriteError` will result if unable to persist the new address
/// to the `PersistBackend`.
///
/// # Panics
///
/// This panics when the caller requests for an address of derivation index greater than the
/// BIP32 max index.
pub fn try_get_address(
&mut self,
address_index: AddressIndex,
Expand All @@ -669,6 +684,11 @@ impl<D> Wallet<D> {
/// see [`AddressIndex`] for available address index selection strategies. If none of the keys
/// in the descriptor are derivable (i.e. does not end with /*) then the same address will always
/// be returned for any [`AddressIndex`].
///
/// # Panics
///
/// This panics when the caller requests for an address of derivation index greater than the
/// BIP32 max index.
pub fn try_get_internal_address(
&mut self,
address_index: AddressIndex,
Expand All @@ -691,6 +711,11 @@ impl<D> Wallet<D> {
/// See [`AddressIndex`] for available address index selection strategies. If none of the keys
/// in the descriptor are derivable (i.e. does not end with /*) then the same address will
/// always be returned for any [`AddressIndex`].
///
/// # Panics
///
/// This panics when the caller requests for an address of derivation index greater than the
/// BIP32 max index.
fn _get_address(
&mut self,
keychain: KeychainKind,
Expand All @@ -710,12 +735,14 @@ impl<D> Wallet<D> {
let ((index, spk), index_changeset) = txout_index.next_unused_spk(&keychain);
(index, spk.into(), Some(index_changeset))
}
AddressIndex::Peek(index) => {
let (index, spk) = txout_index
.spks_of_keychain(&keychain)
.take(index as usize + 1)
.last()
.unwrap();
AddressIndex::Peek(mut peek_index) => {
let mut spk_iter = txout_index.unbounded_spk_iter(&keychain);
if !spk_iter.descriptor().has_wildcard() {
peek_index = 0;
}
let (index, spk) = spk_iter
.nth(peek_index as usize)
.expect("derivation index is out of bounds");
(index, spk, None)
}
};
Expand Down Expand Up @@ -745,7 +772,7 @@ impl<D> Wallet<D> {
///
/// Will only return `Some(_)` if the wallet has given out the spk.
pub fn derivation_of_spk(&self, spk: &Script) -> Option<(KeychainKind, u32)> {
self.indexed_graph.index.index_of_spk(spk).copied()
self.indexed_graph.index.index_of_spk(spk)
}

/// Return the list of unspent outputs of this wallet
Expand Down Expand Up @@ -784,44 +811,44 @@ impl<D> Wallet<D> {
self.chain.tip()
}

/// Returns a iterators of all the script pubkeys for the `Internal` and External` variants in `KeychainKind`.
/// Get unbounded script pubkey iterators for both `Internal` and `External` keychains.
///
/// This is intended to be used when doing a full scan of your addresses (e.g. after restoring
/// from seed words). You pass the `BTreeMap` of iterators to a blockchain data source (e.g.
/// electrum server) which will go through each address until it reaches a *stop gap*.
///
/// Note carefully that iterators go over **all** script pubkeys on the keychains (not what
/// script pubkeys the wallet is storing internally).
pub fn spks_of_all_keychains(
pub fn all_unbounded_spk_iters(
&self,
) -> BTreeMap<KeychainKind, impl Iterator<Item = (u32, ScriptBuf)> + Clone> {
self.indexed_graph.index.spks_of_all_keychains()
self.indexed_graph.index.all_unbounded_spk_iters()
}

/// Gets an iterator over all the script pubkeys in a single keychain.
/// Get an unbounded script pubkey iterator for the given `keychain`.
///
/// See [`spks_of_all_keychains`] for more documentation
/// See [`all_unbounded_spk_iters`] for more documentation
///
/// [`spks_of_all_keychains`]: Self::spks_of_all_keychains
pub fn spks_of_keychain(
/// [`all_unbounded_spk_iters`]: Self::all_unbounded_spk_iters
pub fn unbounded_spk_iter(
&self,
keychain: KeychainKind,
) -> impl Iterator<Item = (u32, ScriptBuf)> + Clone {
self.indexed_graph.index.spks_of_keychain(&keychain)
self.indexed_graph.index.unbounded_spk_iter(&keychain)
}

/// Returns the utxo owned by this wallet corresponding to `outpoint` if it exists in the
/// wallet's database.
pub fn get_utxo(&self, op: OutPoint) -> Option<LocalOutput> {
let (&spk_i, _) = self.indexed_graph.index.txout(op)?;
let (keychain, index, _) = self.indexed_graph.index.txout(op)?;
self.indexed_graph
.graph()
.filter_chain_unspents(
&self.chain,
self.chain.tip().block_id(),
core::iter::once((spk_i, op)),
core::iter::once(((), op)),
)
.map(|((k, i), full_txo)| new_local_utxo(k, i, full_txo))
.map(|(_, full_txo)| new_local_utxo(keychain, index, full_txo))
.next()
}

Expand Down Expand Up @@ -1459,7 +1486,7 @@ impl<D> Wallet<D> {
let ((index, spk), index_changeset) =
self.indexed_graph.index.next_unused_spk(&change_keychain);
let spk = spk.into();
self.indexed_graph.index.mark_used(&change_keychain, index);
self.indexed_graph.index.mark_used(change_keychain, index);
self.persist
.stage(ChangeSet::from(indexed_tx_graph::ChangeSet::from(
index_changeset,
Expand Down Expand Up @@ -1640,7 +1667,7 @@ impl<D> Wallet<D> {
.into();

let weighted_utxo = match txout_index.index_of_spk(&txout.script_pubkey) {
Some(&(keychain, derivation_index)) => {
Some((keychain, derivation_index)) => {
#[allow(deprecated)]
let satisfaction_weight = self
.get_descriptor_for_keychain(keychain)
Expand Down Expand Up @@ -1684,7 +1711,7 @@ impl<D> Wallet<D> {
for (index, txout) in tx.output.iter().enumerate() {
let change_type = self.map_keychain(KeychainKind::Internal);
match txout_index.index_of_spk(&txout.script_pubkey) {
Some(&(keychain, _)) if keychain == change_type => change_index = Some(index),
Some((keychain, _)) if keychain == change_type => change_index = Some(index),
_ => {}
}
}
Expand Down Expand Up @@ -1939,10 +1966,10 @@ impl<D> Wallet<D> {
pub fn cancel_tx(&mut self, tx: &Transaction) {
let txout_index = &mut self.indexed_graph.index;
for txout in &tx.output {
if let Some(&(keychain, index)) = txout_index.index_of_spk(&txout.script_pubkey) {
if let Some((keychain, index)) = txout_index.index_of_spk(&txout.script_pubkey) {
// NOTE: unmark_used will **not** make something unused if it has actually been used
// by a tx in the tracker. It only removes the superficial marking.
txout_index.unmark_used(&keychain, index);
txout_index.unmark_used(keychain, index);
}
}
}
Expand All @@ -1958,7 +1985,7 @@ impl<D> Wallet<D> {
}

fn get_descriptor_for_txout(&self, txout: &TxOut) -> Option<DerivedDescriptor> {
let &(keychain, child) = self
let (keychain, child) = self
.indexed_graph
.index
.index_of_spk(&txout.script_pubkey)?;
Expand Down Expand Up @@ -2172,7 +2199,7 @@ impl<D> Wallet<D> {
{
// Try to find the prev_script in our db to figure out if this is internal or external,
// and the derivation index
let &(keychain, child) = self
let (keychain, child) = self
.indexed_graph
.index
.index_of_spk(&utxo.txout.script_pubkey)
Expand Down Expand Up @@ -2226,7 +2253,7 @@ impl<D> Wallet<D> {

// Try to figure out the keychain and derivation for every input and output
for (is_input, index, out) in utxos.into_iter() {
if let Some(&(keychain, child)) =
if let Some((keychain, child)) =
self.indexed_graph.index.index_of_spk(&out.script_pubkey)
{
let desc = self.get_descriptor_for_keychain(keychain);
Expand Down
Loading

0 comments on commit 60abd87

Please sign in to comment.