fix(chain): check/sanitize descriptor has not hardened child steps before insert it

Author: f3r10

crates/chain/src/indexed_tx_graph.rs

@@ -6,7 +6,6 @@ use alloc::{sync::Arc, vec::Vec};
 use bitcoin::{Block, OutPoint, Transaction, TxOut, Txid};
 
 use crate::{
-    keychain_txout::InsertDescriptorError,
     tx_graph::{self, TxGraph},
     Anchor, BlockId, Indexer, Merge, TxPosInBlock,
 };
@@ -47,11 +46,8 @@ impl<A, I> IndexedTxGraph<A, I> {
 
 impl<A: Anchor, I: Indexer> IndexedTxGraph<A, I> {
     /// Applies the [`ChangeSet`] to the [`IndexedTxGraph`].
-    pub fn apply_changeset(
-        &mut self,
-        changeset: ChangeSet<A, I::ChangeSet>,
-    ) -> Result<(), InsertDescriptorError> {
-        self.index.apply_changeset(changeset.indexer)?;
+    pub fn apply_changeset(&mut self, changeset: ChangeSet<A, I::ChangeSet>) {
+        self.index.apply_changeset(changeset.indexer);
 
         for tx in &changeset.tx_graph.txs {
             self.index.index_tx(tx);
@@ -61,7 +57,6 @@ impl<A: Anchor, I: Indexer> IndexedTxGraph<A, I> {
         }
 
         self.graph.apply_changeset(changeset.tx_graph);
-        Ok(())
     }
 
     /// Determines the [`ChangeSet`] between `self` and an empty [`IndexedTxGraph`].

crates/chain/src/indexer.rs

@@ -23,10 +23,7 @@ pub trait Indexer {
     fn index_tx(&mut self, tx: &Transaction) -> Self::ChangeSet;
 
     /// Apply changeset to itself.
-    fn apply_changeset(
-        &mut self,
-        changeset: Self::ChangeSet,
-    ) -> Result<(), keychain_txout::InsertDescriptorError>;
+    fn apply_changeset(&mut self, changeset: Self::ChangeSet);
 
     /// Determines the [`ChangeSet`](Indexer::ChangeSet) between `self` and an empty [`Indexer`].
     fn initial_changeset(&self) -> Self::ChangeSet;

crates/chain/src/indexer/keychain_txout.rs

@@ -176,7 +176,7 @@ impl<K: Clone + Ord + Debug> Indexer for KeychainTxOutIndex<K> {
         }
     }
 
-    fn apply_changeset(&mut self, changeset: Self::ChangeSet) -> Result<(), InsertDescriptorError> {
+    fn apply_changeset(&mut self, changeset: Self::ChangeSet) {
         self.apply_changeset(changeset)
     }
 
@@ -361,6 +361,23 @@ impl<K: Clone + Ord + Debug> KeychainTxOutIndex<K> {
         keychain: K,
         descriptor: Descriptor<DescriptorPublicKey>,
     ) -> Result<bool, InsertDescriptorError<K>> {
+        // Ensure the keys don't contain any hardened derivation steps or hardened wildcards
+        let descriptor_contains_hardened_steps = descriptor.for_any_key(|k| {
+            if let DescriptorPublicKey::XPub(DescriptorXKey {
+                derivation_path,
+                wildcard,
+                ..
+            }) = k
+            {
+                return *wildcard == Wildcard::Hardened
+                    || derivation_path.into_iter().any(ChildNumber::is_hardened);
+            }
+            false
+        });
+        if descriptor_contains_hardened_steps {
+            return Err(InsertDescriptorError::HardenedDerivationXpub);
+        }
+        descriptor.sanity_check()?;
         let did = descriptor.descriptor_id();
         if !self.keychain_to_descriptor_id.contains_key(&keychain)
             && !self.descriptor_id_to_keychain.contains_key(&did)
@@ -773,32 +790,12 @@ impl<K: Clone + Ord + Debug> KeychainTxOutIndex<K> {
     }
 
     /// Applies the `ChangeSet<K>` to the [`KeychainTxOutIndex<K>`]
-    pub fn apply_changeset(&mut self, changeset: ChangeSet) -> Result<(), InsertDescriptorError> {
+    pub fn apply_changeset(&mut self, changeset: ChangeSet) {
         for (&desc_id, &index) in &changeset.last_revealed {
-            let descriptor = self.descriptors.get(&desc_id).expect("invariant");
-            // Ensure the keys don't contain any hardened derivation steps or hardened wildcards
-            let descriptor_contains_hardened_steps = descriptor.for_any_key(|k| {
-                if let DescriptorPublicKey::XPub(DescriptorXKey {
-                    derivation_path,
-                    wildcard,
-                    ..
-                }) = k
-                {
-                    return *wildcard == Wildcard::Hardened
-                        || derivation_path.into_iter().any(ChildNumber::is_hardened);
-                }
-
-                false
-            });
-            if descriptor_contains_hardened_steps {
-                return Err(InsertDescriptorError::HardenedDerivationXpub);
-            }
-            descriptor.sanity_check()?;
             let v = self.last_revealed.entry(desc_id).or_default();
             *v = index.max(*v);
             self.replenish_inner_index_did(desc_id, self.lookahead);
         }
-        Ok(())
     }
 }
 
@@ -825,7 +822,7 @@ pub enum InsertDescriptorError<K = ()> {
     HardenedDerivationXpub,
 }
 
-impl From<miniscript::Error> for InsertDescriptorError {
+impl<K> From<miniscript::Error> for InsertDescriptorError<K> {
     fn from(err: miniscript::Error) -> Self {
         InsertDescriptorError::Miniscript(err)
     }

crates/chain/src/indexer/spk_txout.rs

@@ -8,8 +8,6 @@ use crate::{
 };
 use bitcoin::{Amount, OutPoint, ScriptBuf, SignedAmount, Transaction, TxOut, Txid};
 
-use super::keychain_txout::InsertDescriptorError;
-
 /// An index storing [`TxOut`]s that have a script pubkey that matches those in a list.
 ///
 /// The basic idea is that you insert script pubkeys you care about into the index with
@@ -71,12 +69,8 @@ impl<I: Clone + Ord + core::fmt::Debug> Indexer for SpkTxOutIndex<I> {
 
     fn initial_changeset(&self) -> Self::ChangeSet {}
 
-    fn apply_changeset(
-        &mut self,
-        _changeset: Self::ChangeSet,
-    ) -> Result<(), InsertDescriptorError> {
+    fn apply_changeset(&mut self, _changeset: Self::ChangeSet) {
         // This applies nothing.
-        Ok(())
     }
 
     fn is_tx_relevant(&self, tx: &Transaction) -> bool {

crates/chain/tests/test_keychain_txout_index.rs

@@ -600,6 +600,19 @@ fn lookahead_to_target() {
     }
 }
 
+#[test]
+fn insert_descriptor_should_reject_hardened_steps() {
+    use bdk_chain::keychain_txout::KeychainTxOutIndex;
+
+    // This descriptor has hardened child steps
+    let s = "wpkh([e273fe42/84h/1h/0h/0h]tpubDEBY7DLZ5kK6pMC2qdtVvKpe6CiAmVDx1SiLtLS3V4GAGyRvsuVbCYReJ9oQz1rfMapghJyUAYLqkqJ3Xadp3GSTCtdAFcKPgzAXC1hzz8a/*h)";
+    let (desc, _) =
+        <Descriptor<DescriptorPublicKey>>::parse_descriptor(&Secp256k1::new(), s).unwrap();
+
+    let mut indexer = KeychainTxOutIndex::<&str>::new(10);
+    assert!(indexer.insert_descriptor("keychain", desc).is_err())
+}
+
 #[test]
 fn applying_changesets_one_by_one_vs_aggregate_must_have_same_result() {
     let desc = parse_descriptor(DESCRIPTORS[0]);
@@ -617,9 +630,7 @@ fn applying_changesets_one_by_one_vs_aggregate_must_have_same_result() {
         .insert_descriptor(TestKeychain::External, desc.clone())
         .expect("must insert keychain");
     for changeset in changesets {
-        indexer_a
-            .apply_changeset(changeset.clone())
-            .expect("must apply changeset");
+        indexer_a.apply_changeset(changeset.clone());
     }
 
     let mut indexer_b = KeychainTxOutIndex::<TestKeychain>::new(0);
@@ -634,9 +645,7 @@ fn applying_changesets_one_by_one_vs_aggregate_must_have_same_result() {
             agg
         })
         .expect("must aggregate changesets");
-    indexer_b
-        .apply_changeset(aggregate_changesets)
-        .expect("must apply changeset");
+    indexer_b.apply_changeset(aggregate_changesets);
 
     assert_eq!(
         indexer_a.keychains().collect::<Vec<_>>(),

crates/wallet/src/wallet/mod.rs

@@ -608,12 +608,8 @@ impl Wallet {
             .map_err(LoadError::Descriptor)?;
 
         let mut indexed_graph = IndexedTxGraph::new(index);
-        indexed_graph
-            .apply_changeset(changeset.indexer.into())
-            .map_err(|err| LoadError::Descriptor(err.into()))?;
-        indexed_graph
-            .apply_changeset(changeset.tx_graph.into())
-            .map_err(|err| LoadError::Descriptor(err.into()))?;
+        indexed_graph.apply_changeset(changeset.indexer.into());
+        indexed_graph.apply_changeset(changeset.tx_graph.into());
 
         let stage = ChangeSet::default();
 

example-crates/example_cli/src/lib.rs

@@ -827,7 +827,7 @@ pub fn init_or_load<CS: clap::Subcommand, S: clap::Args>(
                 graph.apply_changeset(indexed_tx_graph::ChangeSet {
                     tx_graph: changeset.tx_graph,
                     indexer: changeset.indexer,
-                })?;
+                });
                 graph
             });