refactor(keychain): Fix KeychainTxOutIndex range queries

The underlying SpkTxOutIndex should not use DescriptorIds to index
because this loses the ordering relationship of the spks so queries on
subranges of keychains work.

Along with that we enforce that there is a strict 1-to-1 relationship
between descriptors and keychains. Violating this leads to an error in
insert_descriptor now.

In general I try to make the translation layer between the SpkTxOutIndex
and the KeychainTxOutIndex thinner. Ergonomics of this will be improved
in next commit.

The test from the previous commit passes.

clippy.toml

@@ -1 +1,2 @@
 msrv="1.63.0"
+type-complexity-threshold = 275

crates/chain/src/spk_client.rs

@@ -166,7 +166,7 @@ impl SyncRequest {
         self.chain_spks(
             index
                 .revealed_spks(spk_range)
-                .map(|(_, _, spk)| spk.to_owned())
+                .map(|(_, spk)| spk.to_owned())
                 .collect::<Vec<_>>(),
         )
     }

crates/chain/src/spk_txout_index.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/tests/test_indexed_tx_graph.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,
@@ -140,8 +140,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 +265,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()),
             );
 

crates/chain/tests/test_keychain_txout_index.rs

@@ -98,7 +98,7 @@ fn append_changesets_check_last_revealed() {
 }
 
 #[test]
-fn test_apply_changeset_with_different_descriptors_to_same_keychain() {
+fn when_apply_contradictory_changesets_they_are_ignored() {
     let external_descriptor = parse_descriptor(DESCRIPTORS[0]);
     let internal_descriptor = parse_descriptor(DESCRIPTORS[1]);
     let mut txout_index =
@@ -120,7 +120,7 @@ fn test_apply_changeset_with_different_descriptors_to_same_keychain() {
     assert_eq!(
         txout_index.keychains().collect::<Vec<_>>(),
         vec![
-            (&TestKeychain::External, &internal_descriptor),
+            (&TestKeychain::External, &external_descriptor),
             (&TestKeychain::Internal, &internal_descriptor)
         ]
     );
@@ -134,8 +134,8 @@ fn test_apply_changeset_with_different_descriptors_to_same_keychain() {
     assert_eq!(
         txout_index.keychains().collect::<Vec<_>>(),
         vec![
-            (&TestKeychain::External, &internal_descriptor),
-            (&TestKeychain::Internal, &external_descriptor)
+            (&TestKeychain::External, &external_descriptor),
+            (&TestKeychain::Internal, &internal_descriptor)
         ]
     );
 }
@@ -156,15 +156,15 @@ fn test_set_all_derivation_indices() {
     ]
     .into();
     assert_eq!(
-        txout_index.reveal_to_target_multi(&derive_to).1,
+        txout_index.reveal_to_target_multi(&derive_to),
         ChangeSet {
             keychains_added: BTreeMap::new(),
             last_revealed: last_revealed.clone()
         }
     );
     assert_eq!(txout_index.last_revealed_indices(), derive_to);
     assert_eq!(
-        txout_index.reveal_to_target_multi(&derive_to).1,
+        txout_index.reveal_to_target_multi(&derive_to),
         keychain::ChangeSet::default(),
         "no changes if we set to the same thing"
     );
@@ -190,7 +190,7 @@ fn test_lookahead() {
             .reveal_to_target(&TestKeychain::External, index)
             .unwrap();
         assert_eq!(
-            revealed_spks.collect::<Vec<_>>(),
+            revealed_spks,
             vec![(index, spk_at_index(&external_descriptor, index))],
         );
         assert_eq!(
@@ -241,7 +241,7 @@ fn test_lookahead() {
         .reveal_to_target(&TestKeychain::Internal, 24)
         .unwrap();
     assert_eq!(
-        revealed_spks.collect::<Vec<_>>(),
+        revealed_spks,
         (0..=24)
             .map(|index| (index, spk_at_index(&internal_descriptor, index)))
             .collect::<Vec<_>>(),
@@ -511,7 +511,7 @@ fn test_non_wildcard_derivations() {
     let (revealed_spks, revealed_changeset) = txout_index
         .reveal_to_target(&TestKeychain::External, 200)
         .unwrap();
-    assert_eq!(revealed_spks.count(), 0);
+    assert_eq!(revealed_spks.len(), 0);
     assert!(revealed_changeset.is_empty());
 
     // we check that spks_of_keychain returns a SpkIterator with just one element
@@ -591,19 +591,17 @@ fn lookahead_to_target() {
 
         let keychain_test_cases = [
             (
-                external_descriptor.descriptor_id(),
                 TestKeychain::External,
                 t.external_last_revealed,
                 t.external_target,
             ),
             (
-                internal_descriptor.descriptor_id(),
                 TestKeychain::Internal,
                 t.internal_last_revealed,
                 t.internal_target,
             ),
         ];
-        for (descriptor_id, keychain, last_revealed, target) in keychain_test_cases {
+        for (keychain, last_revealed, target) in keychain_test_cases {
             if let Some(target) = target {
                 let original_last_stored_index = match last_revealed {
                     Some(last_revealed) => Some(last_revealed + t.lookahead),
@@ -619,10 +617,10 @@ fn lookahead_to_target() {
                 let keys = index
                     .inner()
                     .all_spks()
-                    .range((descriptor_id, 0)..=(descriptor_id, u32::MAX))
-                    .map(|(k, _)| *k)
+                    .range((keychain.clone(), 0)..=(keychain.clone(), u32::MAX))
+                    .map(|(k, _)| k.clone())
                     .collect::<Vec<_>>();
-                let exp_keys = core::iter::repeat(descriptor_id)
+                let exp_keys = core::iter::repeat(keychain)
                     .zip(0_u32..=exp_last_stored_index)
                     .collect::<Vec<_>>();
                 assert_eq!(keys, exp_keys);
@@ -631,50 +629,6 @@ fn lookahead_to_target() {
     }
 }
 
-/// `::index_txout` should still index txouts with spks derived from descriptors without keychains.
-/// This includes properly refilling the lookahead for said descriptors.
-#[test]
-fn index_txout_after_changing_descriptor_under_keychain() {
-    let secp = bdk_chain::bitcoin::secp256k1::Secp256k1::signing_only();
-    let (desc_a, _) = Descriptor::<DescriptorPublicKey>::parse_descriptor(&secp, DESCRIPTORS[0])
-        .expect("descriptor 0 must be valid");
-    let (desc_b, _) = Descriptor::<DescriptorPublicKey>::parse_descriptor(&secp, DESCRIPTORS[1])
-        .expect("descriptor 1 must be valid");
-    let desc_id_a = desc_a.descriptor_id();
-
-    let mut txout_index = bdk_chain::keychain::KeychainTxOutIndex::<()>::new(10);
-
-    // Introduce `desc_a` under keychain `()` and replace the descriptor.
-    let _ = txout_index.insert_descriptor((), desc_a.clone());
-    let _ = txout_index.insert_descriptor((), desc_b.clone());
-
-    // Loop through spks in intervals of `lookahead` to create outputs with. We should always be
-    // able to index these outputs if `lookahead` is respected.
-    let spk_indices = [9, 19, 29, 39];
-    for i in spk_indices {
-        let spk_at_index = desc_a
-            .at_derivation_index(i)
-            .expect("must derive")
-            .script_pubkey();
-        let index_changeset = txout_index.index_txout(
-            // Use spk derivation index as vout as we just want an unique outpoint.
-            OutPoint::new(h!("mock_tx"), i as _),
-            &TxOut {
-                value: Amount::from_sat(10_000),
-                script_pubkey: spk_at_index,
-            },
-        );
-        assert_eq!(
-            index_changeset,
-            bdk_chain::keychain::ChangeSet {
-                keychains_added: BTreeMap::default(),
-                last_revealed: [(desc_id_a, i)].into(),
-            },
-            "must always increase last active if impl respects lookahead"
-        );
-    }
-}
-
 #[test]
 fn insert_descriptor_no_change() {
     let secp = Secp256k1::signing_only();
@@ -683,19 +637,20 @@ fn insert_descriptor_no_change() {
     let mut txout_index = KeychainTxOutIndex::<()>::default();
     assert_eq!(
         txout_index.insert_descriptor((), desc.clone()),
-        keychain::ChangeSet {
+        Ok(keychain::ChangeSet {
             keychains_added: [((), desc.clone())].into(),
             last_revealed: Default::default()
-        },
+        }),
     );
     assert_eq!(
         txout_index.insert_descriptor((), desc.clone()),
-        keychain::ChangeSet::default(),
+        Ok(keychain::ChangeSet::default()),
         "inserting the same descriptor for keychain should return an empty changeset",
     );
 }
 
 #[test]
+#[cfg(not(debug_assertions))]
 fn applying_changesets_one_by_one_vs_aggregate_must_have_same_result() {
     let desc = parse_descriptor(DESCRIPTORS[0]);
     let changesets: &[ChangeSet<TestKeychain>] = &[
@@ -743,39 +698,25 @@ fn applying_changesets_one_by_one_vs_aggregate_must_have_same_result() {
     );
 }
 
-// When the same descriptor is associated with various keychains,
-// index methods only return the highest keychain by Ord
 #[test]
-fn test_only_highest_ord_keychain_is_returned() {
+fn assigning_same_descriptor_to_multiple_keychains_should_error() {
     let desc = parse_descriptor(DESCRIPTORS[0]);
-
     let mut indexer = KeychainTxOutIndex::<TestKeychain>::new(0);
     let _ = indexer.insert_descriptor(TestKeychain::Internal, desc.clone());
-    let _ = indexer.insert_descriptor(TestKeychain::External, desc);
-
-    // reveal_next_spk will work with either keychain
-    let spk0: ScriptBuf = indexer
-        .reveal_next_spk(&TestKeychain::External)
-        .unwrap()
-        .0
-         .1
-        .into();
-    let spk1: ScriptBuf = indexer
-        .reveal_next_spk(&TestKeychain::Internal)
-        .unwrap()
-        .0
-         .1
-        .into();
+    assert!(indexer
+        .insert_descriptor(TestKeychain::External, desc)
+        .is_err())
+}
 
-    // index_of_spk will always return External
-    assert_eq!(
-        indexer.index_of_spk(&spk0),
-        Some((TestKeychain::External, 0))
-    );
-    assert_eq!(
-        indexer.index_of_spk(&spk1),
-        Some((TestKeychain::External, 1))
-    );
+#[test]
+fn reassigning_keychain_to_a_new_descriptor_should_error() {
+    let desc1 = parse_descriptor(DESCRIPTORS[0]);
+    let desc2 = parse_descriptor(DESCRIPTORS[1]);
+    let mut indexer = KeychainTxOutIndex::<TestKeychain>::new(0);
+    let _ = indexer.insert_descriptor(TestKeychain::Internal, desc1);
+    assert!(indexer
+        .insert_descriptor(TestKeychain::Internal, desc2)
+        .is_err());
 }
 
 #[test]
@@ -786,27 +727,29 @@ fn when_querying_over_a_range_of_keychains_the_utxos_should_show_up() {
     for (i, descriptor) in DESCRIPTORS.iter().enumerate() {
         let descriptor = parse_descriptor(descriptor);
         let _ = indexer.insert_descriptor(i, descriptor.clone());
-        indexer.reveal_next_spk(&i);
+        if i != 4 {
+            // skip one in the middle to see if uncovers any bugs
+            indexer.reveal_next_spk(&i);
+        }
         tx.output.push(TxOut {
             script_pubkey: descriptor.at_derivation_index(0).unwrap().script_pubkey(),
             value: Amount::from_sat(10_000),
         });
     }
 
+    let n_spks = DESCRIPTORS.len() - /*we skipped one*/ 1;
+
     let _ = indexer.index_tx(&tx);
-    assert_eq!(indexer.outpoints().count(), DESCRIPTORS.len());
+    assert_eq!(indexer.outpoints().len(), n_spks);
 
-    assert_eq!(
-        indexer.revealed_spks(0..DESCRIPTORS.len()).count(),
-        DESCRIPTORS.len()
-    );
+    assert_eq!(indexer.revealed_spks(0..DESCRIPTORS.len()).count(), n_spks);
     assert_eq!(indexer.revealed_spks(1..4).count(), 4 - 1);
     assert_eq!(
         indexer.net_value(&tx, 0..DESCRIPTORS.len()).to_sat(),
-        (10_000 * DESCRIPTORS.len()) as i64
+        (10_000 * n_spks) as i64
     );
     assert_eq!(
-        indexer.net_value(&tx, 3..5).to_sat(),
-        (10_000 * (5 - 3)) as i64
+        indexer.net_value(&tx, 3..6).to_sat(),
+        (10_000 * (6 - 3 - /*the skipped one*/ 1)) as i64
     );
 }

crates/wallet/src/descriptor/error.rs

@@ -23,7 +23,8 @@ pub enum Error {
     HardenedDerivationXpub,
     /// The descriptor contains multipath keys
     MultiPath,
-
+    /// The descriptor assigned to one keychain is the same as another
+    Duplicate,
     /// Error thrown while working with [`keys`](crate::keys)
     Key(crate::keys::KeyError),
     /// Error while extracting and manipulating policies
@@ -79,6 +80,10 @@ impl fmt::Display for Error {
             Self::Pk(err) => write!(f, "Key-related error: {}", err),
             Self::Miniscript(err) => write!(f, "Miniscript error: {}", err),
             Self::Hex(err) => write!(f, "Hex decoding error: {}", err),
+            Self::Duplicate => write!(
+                f,
+                "The descriptors for two different keychains are the same"
+            ),
         }
     }
 }