Merge #1463: No descriptor ids in spk txout index

8dd1744 refactor(chain): compute txid once for `KeychainTxOutIndex::index_tx` (志宇)
639d735 refactor(chain): change field names to be more sane (志宇)
5a02f40 docs(chain): fix docs (志宇)
c77e12b refactor(chain): `KeychainTxOutIndex` use `HashMap` for fields (志宇)
4d3846a chore(chain): s/replenish_lookahead/replenish_inner_index/ (LLFourn)
8779afd chore(chain): document insert_descriptor invariants better (LLFourn)
69f2a69 refactor(chain): improve replenish lookeahd internals (LLFourn)
5a584d0 chore(chain): Fix Indexed and KeychainIndexed documentaion (Lloyd Fournier)
b8ba5a0 chore(chain): Improve documentation of keychain::ChangeSet (LLFourn)
101a09a chore(chain): Standardise KeychainTxOutIndex return types (LLFourn)
bce070b chore(chain): add type IndexSpk, fix clippy type complexity warning (Steve Myers)
4d2442c chore(chain): misc docs and insert_descriptor fixes (LLFourn)
bc2a8be refactor(keychain): Fix KeychainTxOutIndex range queries (LLFourn)
3b2ff0c Write failing test for keychain range querying (LLFourn)

Pull request description:

  Fixes #1459

  This reverts part of the changes in #1203. There the `SpkTxOutIndex<(K,u32)>` was changed to `SpkTxOutIndex<(DescriptorId, u32>)`. This led to a complicated translation logic in  `KeychainTxOutIndex` (where the API is based on `K`) to transform calls to it to calls to the underlying `SpkTxOutIndex` (which now indexes by `DescriptorId`). The translation layer was broken when it came to translating range queries from the `KeychainTxOutIndex`. My solution was just to revert this part of the change and remove the need for a translation layer (almost) altogether. A thin translation layer remains to ensure that un-revealed spks are filtered out before being returned from the `KeychainTxOutIndex` methods.

  I feel like this PR could be extended to include a bunch of ergonomics improvements that are easier to implement now. But I think that's the point of #1451 so I held off and should probably go and scope creep that one instead.

  ### 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

  #### Bugfixes:

  * [x] This pull request breaks the existing API
  * [x] 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:
  evanlinjin:
    ACK 8dd1744

Tree-SHA512: 283e6b6d4218902298e2e848fe847a6c85e27af4eee3e4337e3dad6eacf9beaa08ac99b1dce7b6fb199ca53931e543ea365728a81c41567a2e510cce77b12ac0

Author: evanlinjin

clippy.toml

@@ -1 +1 @@
-msrv="1.63.0"
+msrv="1.63.0"

crates/chain/src/keychain.rs

@@ -12,7 +12,7 @@
 
 #[cfg(feature = "miniscript")]
 mod txout_index;
-use bitcoin::Amount;
+use bitcoin::{Amount, ScriptBuf};
 #[cfg(feature = "miniscript")]
 pub use txout_index::*;
 
@@ -49,6 +49,11 @@ impl Balance {
     }
 }
 
+/// A tuple of keychain index and `T` representing the indexed value.
+pub type Indexed<T> = (u32, T);
+/// A tuple of keychain `K`, derivation index (`u32`) and a `T` associated with them.
+pub type KeychainIndexed<K, T> = ((K, u32), T);
+
 impl core::fmt::Display for Balance {
     fn fmt(&self, f: &mut core::fmt::Formatter<'_>) -> core::fmt::Result {
         write!(

crates/chain/src/keychain/txout_index.rs

@@ -28,6 +28,7 @@ pub use chain_data::*;
 pub mod indexed_tx_graph;
 pub use indexed_tx_graph::IndexedTxGraph;
 pub mod keychain;
+pub use keychain::{Indexed, KeychainIndexed};
 pub mod local_chain;
 mod tx_data_traits;
 pub mod tx_graph;

crates/chain/src/lib.rs

@@ -1,7 +1,8 @@
 //! Helper types for spk-based blockchain clients.
 
 use crate::{
-    collections::BTreeMap, local_chain::CheckPoint, ConfirmationTimeHeightAnchor, TxGraph,
+    collections::BTreeMap, keychain::Indexed, local_chain::CheckPoint,
+    ConfirmationTimeHeightAnchor, TxGraph,
 };
 use alloc::{boxed::Box, vec::Vec};
 use bitcoin::{OutPoint, Script, ScriptBuf, Txid};
@@ -166,7 +167,7 @@ impl SyncRequest {
         self.chain_spks(
             index
                 .revealed_spks(spk_range)
-                .map(|(_, _, spk)| spk.to_owned())
+                .map(|(_, spk)| spk.to_owned())
                 .collect::<Vec<_>>(),
         )
     }
@@ -195,7 +196,7 @@ pub struct FullScanRequest<K> {
     /// [`LocalChain::tip`]: crate::local_chain::LocalChain::tip
     pub chain_tip: CheckPoint,
     /// Iterators of script pubkeys indexed by the keychain index.
-    pub spks_by_keychain: BTreeMap<K, Box<dyn Iterator<Item = (u32, ScriptBuf)> + Send>>,
+    pub spks_by_keychain: BTreeMap<K, Box<dyn Iterator<Item = Indexed<ScriptBuf>> + Send>>,
 }
 
 impl<K: Ord + Clone> FullScanRequest<K> {
@@ -238,7 +239,7 @@ impl<K: Ord + Clone> FullScanRequest<K> {
     pub fn set_spks_for_keychain(
         mut self,
         keychain: K,
-        spks: impl IntoIterator<IntoIter = impl Iterator<Item = (u32, ScriptBuf)> + Send + 'static>,
+        spks: impl IntoIterator<IntoIter = impl Iterator<Item = Indexed<ScriptBuf>> + Send + 'static>,
     ) -> Self {
         self.spks_by_keychain
             .insert(keychain, Box::new(spks.into_iter()));
@@ -252,7 +253,7 @@ impl<K: Ord + Clone> FullScanRequest<K> {
     pub fn chain_spks_for_keychain(
         mut self,
         keychain: K,
-        spks: impl IntoIterator<IntoIter = impl Iterator<Item = (u32, ScriptBuf)> + Send + 'static>,
+        spks: impl IntoIterator<IntoIter = impl Iterator<Item = Indexed<ScriptBuf>> + Send + 'static>,
     ) -> Self {
         match self.spks_by_keychain.remove(&keychain) {
             // clippy here suggests to remove `into_iter` from `spks.into_iter()`, but doing so

crates/chain/src/spk_client.rs

@@ -1,5 +1,6 @@
 use crate::{
     bitcoin::{secp256k1::Secp256k1, ScriptBuf},
+    keychain::Indexed,
     miniscript::{Descriptor, DescriptorPublicKey},
 };
 use core::{borrow::Borrow, ops::Bound, ops::RangeBounds};
@@ -97,7 +98,7 @@ impl<D> Iterator for SpkIterator<D>
 where
     D: Borrow<Descriptor<DescriptorPublicKey>>,
 {
-    type Item = (u32, ScriptBuf);
+    type Item = Indexed<ScriptBuf>;
 
     fn next(&mut self) -> Option<Self::Item> {
         // For non-wildcard descriptors, we expect the first element to be Some((0, spk)), then None after.
@@ -158,8 +159,12 @@ mod test {
         let (external_descriptor,_) = Descriptor::<DescriptorPublicKey>::parse_descriptor(&secp, "tr([73c5da0a/86'/0'/0']xprv9xgqHN7yz9MwCkxsBPN5qetuNdQSUttZNKw1dcYTV4mkaAFiBVGQziHs3NRSWMkCzvgjEe3n9xV8oYywvM8at9yRqyaZVz6TYYhX98VjsUk/0/*)").unwrap();
         let (internal_descriptor,_) = Descriptor::<DescriptorPublicKey>::parse_descriptor(&secp, "tr([73c5da0a/86'/0'/0']xprv9xgqHN7yz9MwCkxsBPN5qetuNdQSUttZNKw1dcYTV4mkaAFiBVGQziHs3NRSWMkCzvgjEe3n9xV8oYywvM8at9yRqyaZVz6TYYhX98VjsUk/1/*)").unwrap();
 
-        let _ = txout_index.insert_descriptor(TestKeychain::External, external_descriptor.clone());
-        let _ = txout_index.insert_descriptor(TestKeychain::Internal, internal_descriptor.clone());
+        let _ = txout_index
+            .insert_descriptor(TestKeychain::External, external_descriptor.clone())
+            .unwrap();
+        let _ = txout_index
+            .insert_descriptor(TestKeychain::Internal, internal_descriptor.clone())
+            .unwrap();
 
         (txout_index, external_descriptor, internal_descriptor)
     }

crates/chain/src/spk_iter.rs

@@ -52,7 +52,7 @@ impl<I> Default for SpkTxOutIndex<I> {
     }
 }
 
-impl<I: Clone + Ord> Indexer for SpkTxOutIndex<I> {
+impl<I: Clone + Ord + core::fmt::Debug> Indexer for SpkTxOutIndex<I> {
     type ChangeSet = ();
 
     fn index_txout(&mut self, outpoint: OutPoint, txout: &TxOut) -> Self::ChangeSet {
@@ -76,7 +76,7 @@ impl<I: Clone + Ord> Indexer for SpkTxOutIndex<I> {
     }
 }
 
-impl<I: Clone + Ord> SpkTxOutIndex<I> {
+impl<I: Clone + Ord + core::fmt::Debug> SpkTxOutIndex<I> {
     /// Scans a transaction's outputs for matching script pubkeys.
     ///
     /// Typically, this is used in two situations:

crates/chain/src/spk_txout_index.rs

@@ -10,7 +10,7 @@ use bdk_chain::{
     indexed_tx_graph::{self, IndexedTxGraph},
     keychain::{self, Balance, KeychainTxOutIndex},
     local_chain::LocalChain,
-    tx_graph, ChainPosition, ConfirmationHeightAnchor, DescriptorExt,
+    tx_graph, Append, ChainPosition, ConfirmationHeightAnchor, DescriptorExt,
 };
 use bitcoin::{
     secp256k1::Secp256k1, Amount, OutPoint, Script, ScriptBuf, Transaction, TxIn, TxOut,
@@ -34,7 +34,10 @@ fn insert_relevant_txs() {
     let mut graph = IndexedTxGraph::<ConfirmationHeightAnchor, KeychainTxOutIndex<()>>::new(
         KeychainTxOutIndex::new(10),
     );
-    let _ = graph.index.insert_descriptor((), descriptor.clone());
+    let _ = graph
+        .index
+        .insert_descriptor((), descriptor.clone())
+        .unwrap();
 
     let tx_a = Transaction {
         output: vec![
@@ -140,8 +143,16 @@ fn test_list_owned_txouts() {
         KeychainTxOutIndex::new(10),
     );
 
-    let _ = graph.index.insert_descriptor("keychain_1".into(), desc_1);
-    let _ = graph.index.insert_descriptor("keychain_2".into(), desc_2);
+    assert!(!graph
+        .index
+        .insert_descriptor("keychain_1".into(), desc_1)
+        .unwrap()
+        .is_empty());
+    assert!(!graph
+        .index
+        .insert_descriptor("keychain_2".into(), desc_2)
+        .unwrap()
+        .is_empty());
 
     // Get trusted and untrusted addresses
 
@@ -257,18 +268,26 @@ fn test_list_owned_txouts() {
                 .unwrap_or_else(|| panic!("block must exist at {}", height));
             let txouts = graph
                 .graph()
-                .filter_chain_txouts(&local_chain, chain_tip, graph.index.outpoints())
+                .filter_chain_txouts(
+                    &local_chain,
+                    chain_tip,
+                    graph.index.outpoints().iter().cloned(),
+                )
                 .collect::<Vec<_>>();
 
             let utxos = graph
                 .graph()
-                .filter_chain_unspents(&local_chain, chain_tip, graph.index.outpoints())
+                .filter_chain_unspents(
+                    &local_chain,
+                    chain_tip,
+                    graph.index.outpoints().iter().cloned(),
+                )
                 .collect::<Vec<_>>();
 
             let balance = graph.graph().balance(
                 &local_chain,
                 chain_tip,
-                graph.index.outpoints(),
+                graph.index.outpoints().iter().cloned(),
                 |_, spk: &Script| trusted_spks.contains(&spk.to_owned()),
             );