Merge #1203: Include the descriptor in keychain::Changeset

86711d4 doc(chain): add section for non-recommended K to descriptor assignments (Daniela Brozzoni)
de53d72 test: Only the highest ord keychain is returned (Daniela Brozzoni)
9d8023b fix(chain): introduce keychain-variant-ranking to `KeychainTxOutIndex` (志宇)
6c87481 chore(chain): move `use` in `indexed_tx_graph.rs` so clippy is happy (志宇)
537aa03 chore(chain): update test so clippy does not complain (志宇)
ed117de test(chain): applying changesets one-by-one vs aggregate should be same (志宇)
6a3fb84 fix(chain): simplify `Append::append` impl for `keychain::ChangeSet` (志宇)
1d294b7 fix: Run tests only if the miniscript feature is.. ..enabled, enable it by default (Daniela Brozzoni)
0e3e136 doc(bdk): Add instructions for manually inserting... ...secret keys in the wallet in Wallet::load (Daniela Brozzoni)
76afccc fix(wallet): add expected descriptors as signers after creating from wallet::ChangeSet (Steve Myers)
4f05441 keychain::ChangeSet includes the descriptor (Daniela Brozzoni)
8ff99f2 ref(chain): Define test descriptors, use them... ...everywhere (Daniela Brozzoni)
b990293 ref(chain): move `keychain::ChangeSet` into `txout_index.rs` (志宇)

Pull request description:

  Fixes #1101

  - Moves keychain::ChangeSet inside `keychain/txout_index.rs` as now the `ChangeSet` depends on miniscript
  - Slightly cleans up tests by introducing some constant descriptors
  - The KeychainTxOutIndex's internal SpkIterator now uses DescriptorId
  instead of K. The DescriptorId -> K translation is made at the
  KeychainTxOutIndex level.
  - The keychain::Changeset is now a struct, which includes a map for last
  revealed indexes, and one for newly added keychains and their
  descriptor.

  ### Changelog notice

  API changes in bdk:
  - Wallet::keychains returns a `impl Iterator` instead of `BTreeMap`
  - Wallet::load doesn't take descriptors anymore, since they're stored in the db
  - Wallet::new_or_load checks if the loaded descriptor from db is the same as the provided one

  API changes in bdk_chain:
  - `ChangeSet` is now a struct, which includes a map for last revealed
        indexes, and one for keychains and descriptors.
  - `KeychainTxOutIndex::inner` returns a `SpkIterator<(DescriptorId, u32)>`
  - `KeychainTxOutIndex::outpoints` returns a `BTreeSet` instead of `&BTreeSet`
  - `KeychainTxOutIndex::keychains` returns a `impl Iterator` instead of
        `&BTreeMap`
  - `KeychainTxOutIndex::txouts` doesn't return a ExactSizeIterator anymore
  - `KeychainTxOutIndex::last_revealed_indices` returns a `BTreeMap`
        instead of `&BTreeMap`
  - `KeychainTxOutIndex::add_keychain` has been renamed to `KeychainTxOutIndex::insert_descriptor`, and now it returns a ChangeSet
  - `KeychainTxOutIndex::reveal_next_spk` returns Option
  - `KeychainTxOutIndex::next_unused_spk` returns Option
  - `KeychainTxOutIndex::unbounded_spk_iter` returns Option
  - `KeychainTxOutIndex::next_index` returns Option
  - `KeychainTxOutIndex::reveal_to_target` returns Option
  - `KeychainTxOutIndex::revealed_keychain_spks` returns Option
  - `KeychainTxOutIndex::unused_keychain_spks` returns Option
  - `KeychainTxOutIndex::last_revealed_index` returns Option
  - `KeychainTxOutIndex::keychain_outpoints` returns Option
  - `KeychainTxOutIndex::keychain_outpoints_in_range` returns Option
  - `KeychainTxOutIndex::last_used_index` returns None if the keychain has never been used, or if it doesn't exist

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

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

ACKs for top commit:
  evanlinjin:
    ACK 86711d4

Tree-SHA512: 4b1c9a31951f67b18037b7dd9837acbc35823f21de644ab833754b74d20f5373549f81e66965ecd3953ebf4f99644c9fd834812acfa65f9188950f1bda17ab60

crates/bdk/tests/wallet.rs

@@ -1,7 +1,7 @@
 use std::str::FromStr;
 
 use assert_matches::assert_matches;
-use bdk::descriptor::calc_checksum;
+use bdk::descriptor::{calc_checksum, IntoWalletDescriptor};
 use bdk::psbt::PsbtUtils;
 use bdk::signer::{SignOptions, SignerError};
 use bdk::wallet::coin_selection::{self, LargestFirstCoinSelection};
@@ -10,9 +10,11 @@ use bdk::wallet::tx_builder::AddForeignUtxoError;
 use bdk::wallet::NewError;
 use bdk::wallet::{AddressInfo, Balance, Wallet};
 use bdk::KeychainKind;
+use bdk_chain::collections::BTreeMap;
 use bdk_chain::COINBASE_MATURITY;
 use bdk_chain::{BlockId, ConfirmationTime};
 use bitcoin::hashes::Hash;
+use bitcoin::key::Secp256k1;
 use bitcoin::psbt;
 use bitcoin::script::PushBytesBuf;
 use bitcoin::sighash::{EcdsaSighashType, TapSighashType};
@@ -84,14 +86,24 @@ fn load_recovers_wallet() {
     // recover wallet
     {
         let db = bdk_file_store::Store::open(DB_MAGIC, &file_path).expect("must recover db");
-        let wallet =
-            Wallet::load(get_test_tr_single_sig_xprv(), None, db).expect("must recover wallet");
+        let wallet = Wallet::load(db).expect("must recover wallet");
         assert_eq!(wallet.network(), Network::Testnet);
-        assert_eq!(wallet.spk_index().keychains(), wallet_spk_index.keychains());
+        assert_eq!(
+            wallet.spk_index().keychains().collect::<Vec<_>>(),
+            wallet_spk_index.keychains().collect::<Vec<_>>()
+        );
         assert_eq!(
             wallet.spk_index().last_revealed_indices(),
             wallet_spk_index.last_revealed_indices()
         );
+        let secp = Secp256k1::new();
+        assert_eq!(
+            *wallet.get_descriptor_for_keychain(KeychainKind::External),
+            get_test_tr_single_sig_xprv()
+                .into_wallet_descriptor(&secp, wallet.network())
+                .unwrap()
+                .0
+        );
     }
 
     // `new` can only be called on empty db
@@ -108,12 +120,12 @@ fn new_or_load() {
     let file_path = temp_dir.path().join("store.db");
 
     // init wallet when non-existent
-    let wallet_keychains = {
+    let wallet_keychains: BTreeMap<_, _> = {
         let db = bdk_file_store::Store::open_or_create_new(DB_MAGIC, &file_path)
             .expect("must create db");
         let wallet = Wallet::new_or_load(get_test_wpkh(), None, db, Network::Testnet)
             .expect("must init wallet");
-        wallet.keychains().clone()
+        wallet.keychains().map(|(k, v)| (*k, v.clone())).collect()
     };
 
     // wrong network
@@ -162,14 +174,60 @@ fn new_or_load() {
         );
     }
 
+    // wrong external descriptor
+    {
+        let exp_descriptor = get_test_tr_single_sig();
+        let got_descriptor = get_test_wpkh()
+            .into_wallet_descriptor(&Secp256k1::new(), Network::Testnet)
+            .unwrap()
+            .0;
+
+        let db =
+            bdk_file_store::Store::open_or_create_new(DB_MAGIC, &file_path).expect("must open db");
+        let err = Wallet::new_or_load(exp_descriptor, None, db, Network::Testnet)
+            .expect_err("wrong external descriptor");
+        assert!(
+            matches!(
+                err,
+                bdk::wallet::NewOrLoadError::LoadedDescriptorDoesNotMatch { ref got, keychain }
+                if got == &Some(got_descriptor) && keychain == KeychainKind::External
+            ),
+            "err: {}",
+            err,
+        );
+    }
+
+    // wrong internal descriptor
+    {
+        let exp_descriptor = Some(get_test_tr_single_sig());
+        let got_descriptor = None;
+
+        let db =
+            bdk_file_store::Store::open_or_create_new(DB_MAGIC, &file_path).expect("must open db");
+        let err = Wallet::new_or_load(get_test_wpkh(), exp_descriptor, db, Network::Testnet)
+            .expect_err("wrong internal descriptor");
+        assert!(
+            matches!(
+                err,
+                bdk::wallet::NewOrLoadError::LoadedDescriptorDoesNotMatch { ref got, keychain }
+                if got == &got_descriptor && keychain == KeychainKind::Internal
+            ),
+            "err: {}",
+            err,
+        );
+    }
+
     // all parameters match
     {
         let db =
             bdk_file_store::Store::open_or_create_new(DB_MAGIC, &file_path).expect("must open db");
         let wallet = Wallet::new_or_load(get_test_wpkh(), None, db, Network::Testnet)
             .expect("must recover wallet");
         assert_eq!(wallet.network(), Network::Testnet);
-        assert_eq!(wallet.keychains(), &wallet_keychains);
+        assert!(wallet
+            .keychains()
+            .map(|(k, v)| (*k, v.clone()))
+            .eq(wallet_keychains));
     }
 }
 
@@ -181,7 +239,6 @@ fn test_descriptor_checksum() {
 
     let raw_descriptor = wallet
         .keychains()
-        .iter()
         .next()
         .unwrap()
         .1

crates/chain/Cargo.toml

@@ -26,6 +26,6 @@ rand = "0.8"
 proptest = "1.2.0"
 
 [features]
-default = ["std"]
-std = ["bitcoin/std", "miniscript/std"]
-serde = ["serde_crate", "bitcoin/serde"]
+default = ["std", "miniscript"]
+std = ["bitcoin/std", "miniscript?/std"]
+serde = ["serde_crate", "bitcoin/serde", "miniscript?/serde"]

crates/chain/src/descriptor_ext.rs

@@ -1,10 +1,29 @@
-use crate::miniscript::{Descriptor, DescriptorPublicKey};
+use crate::{
+    alloc::{string::ToString, vec::Vec},
+    miniscript::{Descriptor, DescriptorPublicKey},
+};
+use bitcoin::hashes::{hash_newtype, sha256, Hash};
+
+hash_newtype! {
+    /// Represents the ID of a descriptor, defined as the sha256 hash of
+    /// the descriptor string, checksum excluded.
+    ///
+    /// This is useful for having a fixed-length unique representation of a descriptor,
+    /// in particular, we use it to persist application state changes related to the
+    /// descriptor without having to re-write the whole descriptor each time.
+    ///
+    pub struct DescriptorId(pub sha256::Hash);
+}
 
 /// A trait to extend the functionality of a miniscript descriptor.
 pub trait DescriptorExt {
     /// Returns the minimum value (in satoshis) at which an output is broadcastable.
     /// Panics if the descriptor wildcard is hardened.
     fn dust_value(&self) -> u64;
+
+    /// Returns the descriptor id, calculated as the sha256 of the descriptor, checksum not
+    /// included.
+    fn descriptor_id(&self) -> DescriptorId;
 }
 
 impl DescriptorExt for Descriptor<DescriptorPublicKey> {
@@ -15,4 +34,11 @@ impl DescriptorExt for Descriptor<DescriptorPublicKey> {
             .dust_value()
             .to_sat()
     }
+
+    fn descriptor_id(&self) -> DescriptorId {
+        let desc = self.to_string();
+        let desc_without_checksum = desc.split('#').next().expect("Must be here");
+        let descriptor_bytes = <Vec<u8>>::from(desc_without_checksum.as_bytes());
+        DescriptorId(sha256::Hash::hash(&descriptor_bytes))
+    }
 }

crates/chain/src/indexed_tx_graph.rs

@@ -4,7 +4,6 @@ use alloc::vec::Vec;
 use bitcoin::{Block, OutPoint, Transaction, TxOut, Txid};
 
 use crate::{
-    keychain,
     tx_graph::{self, TxGraph},
     Anchor, AnchorFromBlockPosition, Append, BlockId,
 };
@@ -320,8 +319,9 @@ impl<A, IA: Default> From<tx_graph::ChangeSet<A>> for ChangeSet<A, IA> {
     }
 }
 
-impl<A, K> From<keychain::ChangeSet<K>> for ChangeSet<A, keychain::ChangeSet<K>> {
-    fn from(indexer: keychain::ChangeSet<K>) -> Self {
+#[cfg(feature = "miniscript")]
+impl<A, K> From<crate::keychain::ChangeSet<K>> for ChangeSet<A, crate::keychain::ChangeSet<K>> {
+    fn from(indexer: crate::keychain::ChangeSet<K>) -> Self {
         Self {
             graph: Default::default(),
             indexer,

crates/chain/src/keychain.rs

@@ -10,78 +10,12 @@
 //!
 //! [`SpkTxOutIndex`]: crate::SpkTxOutIndex
 
-use crate::{collections::BTreeMap, Append};
-
 #[cfg(feature = "miniscript")]
 mod txout_index;
 use bitcoin::Amount;
 #[cfg(feature = "miniscript")]
 pub use txout_index::*;
 
-/// Represents updates to the derivation index of a [`KeychainTxOutIndex`].
-/// It maps each keychain `K` to its last revealed index.
-///
-/// It can be applied to [`KeychainTxOutIndex`] with [`apply_changeset`]. [`ChangeSet`]s are
-/// monotone in that they will never decrease the revealed derivation index.
-///
-/// [`KeychainTxOutIndex`]: crate::keychain::KeychainTxOutIndex
-/// [`apply_changeset`]: crate::keychain::KeychainTxOutIndex::apply_changeset
-#[derive(Clone, Debug, PartialEq)]
-#[cfg_attr(
-    feature = "serde",
-    derive(serde::Deserialize, serde::Serialize),
-    serde(
-        crate = "serde_crate",
-        bound(
-            deserialize = "K: Ord + serde::Deserialize<'de>",
-            serialize = "K: Ord + serde::Serialize"
-        )
-    )
-)]
-#[must_use]
-pub struct ChangeSet<K>(pub BTreeMap<K, u32>);
-
-impl<K> ChangeSet<K> {
-    /// Get the inner map of the keychain to its new derivation index.
-    pub fn as_inner(&self) -> &BTreeMap<K, u32> {
-        &self.0
-    }
-}
-
-impl<K: Ord> Append for ChangeSet<K> {
-    /// Append another [`ChangeSet`] into self.
-    ///
-    /// If the keychain already exists, increase the index when the other's index > self's index.
-    /// If the keychain did not exist, append the new keychain.
-    fn append(&mut self, mut other: Self) {
-        self.0.iter_mut().for_each(|(key, index)| {
-            if let Some(other_index) = other.0.remove(key) {
-                *index = other_index.max(*index);
-            }
-        });
-        // We use `extend` instead of `BTreeMap::append` due to performance issues with `append`.
-        // Refer to https://github.com/rust-lang/rust/issues/34666#issuecomment-675658420
-        self.0.extend(other.0);
-    }
-
-    /// Returns whether the changeset are empty.
-    fn is_empty(&self) -> bool {
-        self.0.is_empty()
-    }
-}
-
-impl<K> Default for ChangeSet<K> {
-    fn default() -> Self {
-        Self(Default::default())
-    }
-}
-
-impl<K> AsRef<BTreeMap<K, u32>> for ChangeSet<K> {
-    fn as_ref(&self) -> &BTreeMap<K, u32> {
-        &self.0
-    }
-}
-
 /// Balance, differentiated into various categories.
 #[derive(Debug, PartialEq, Eq, Clone, Default)]
 #[cfg_attr(
@@ -137,40 +71,3 @@ impl core::ops::Add for Balance {
         }
     }
 }
-
-#[cfg(test)]
-mod test {
-    use super::*;
-
-    #[test]
-    fn append_keychain_derivation_indices() {
-        #[derive(Ord, PartialOrd, Eq, PartialEq, Clone, Debug)]
-        enum Keychain {
-            One,
-            Two,
-            Three,
-            Four,
-        }
-        let mut lhs_di = BTreeMap::<Keychain, u32>::default();
-        let mut rhs_di = BTreeMap::<Keychain, u32>::default();
-        lhs_di.insert(Keychain::One, 7);
-        lhs_di.insert(Keychain::Two, 0);
-        rhs_di.insert(Keychain::One, 3);
-        rhs_di.insert(Keychain::Two, 5);
-        lhs_di.insert(Keychain::Three, 3);
-        rhs_di.insert(Keychain::Four, 4);
-
-        let mut lhs = ChangeSet(lhs_di);
-        let rhs = ChangeSet(rhs_di);
-        lhs.append(rhs);
-
-        // Exiting index doesn't update if the new index in `other` is lower than `self`.
-        assert_eq!(lhs.0.get(&Keychain::One), Some(&7));
-        // Existing index updates if the new index in `other` is higher than `self`.
-        assert_eq!(lhs.0.get(&Keychain::Two), Some(&5));
-        // Existing index is unchanged if keychain doesn't exist in `other`.
-        assert_eq!(lhs.0.get(&Keychain::Three), Some(&3));
-        // New keychain gets added if the keychain is in `other` but not in `self`.
-        assert_eq!(lhs.0.get(&Keychain::Four), Some(&4));
-    }
-}