Merge bitcoindevkit/bdk#1486: refactor(chain): calculate DescriptorId as the sha256 hash of the spk at index 0

8f5b172e59b4d5e199b98777855b0d9b46742f66 test(wallet): verify wallet panics in dev mode if using keychains with duplicate spks (Steve Myers)
46c6f18cc3ff71efda2b8547aa32854f9b88762e refactor(chain): calculate DescriptorId as sha256 hash of spk at index 0 (Steve Myers)

Pull request description:

  ### Description

  Rename `DescriptorId` to `KeychainId` and `descriptor_id()` to `keychain_id()`.

  Calculate keychain ids as the hash of the spk derived from its descriptor as index 0.

  Added docs to `Wallet` and `KeychainTxOutIndex::insert_descriptor()` explaining that it's the users responsibility not to use wildcard and non-wildcard descriptors that can derive the same script pubkey. I also recommended for `Wallet` that legacy non-wildcard descriptors be added to a temporary `Wallet` and swept into a modern wildcard descriptor.

  ### Notes to the reviewers

  fixes #1483

  ### Changelog notice

  changed

  - Renamed DescriptorId to KeychainId, DescriptorExt::descriptor_id() to keychain_id().

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

  * [ ] 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:
  LLFourn:
    ACK 8f5b172e59b4d5e199b98777855b0d9b46742f66
  oleonardolima:
    Concept ACK 8f5b172e59b4d5e199b98777855b0d9b46742f66

Tree-SHA512: 07defa208d9cfcd61bc6e31ded06a287c392c51bcc4949f601ecfac635c3443e7d08c62d92618ed894dc5ef13cdcf784771a6bf8904a5397110bedb1563f52d4

crates/chain/src/descriptor_ext.rs

@@ -1,12 +1,8 @@
-use crate::{
-    alloc::{string::ToString, vec::Vec},
-    miniscript::{Descriptor, DescriptorPublicKey},
-};
+use crate::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.
+    /// Represents the unique ID of a descriptor.
     ///
     /// 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
@@ -21,8 +17,8 @@ pub trait DescriptorExt {
     /// 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.
+    /// Returns the descriptor ID, calculated as the sha256 hash of the spk derived from the
+    /// descriptor at index 0.
     fn descriptor_id(&self) -> DescriptorId;
 }
 
@@ -36,9 +32,7 @@ impl DescriptorExt for Descriptor<DescriptorPublicKey> {
     }
 
     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))
+        let spk = self.at_derivation_index(0).unwrap().script_pubkey();
+        DescriptorId(sha256::Hash::hash(spk.as_bytes()))
     }
 }

crates/chain/src/keychain/txout_index.rs

@@ -352,6 +352,13 @@ impl<K: Clone + Ord + Debug> KeychainTxOutIndex<K> {
     ///
     /// keychain <-> descriptor is a one-to-one mapping that cannot be changed. Attempting to do so
     /// will return a [`InsertDescriptorError<K>`].
+    ///
+    /// `[KeychainTxOutIndex]` will prevent you from inserting two descriptors which derive the same
+    /// script pubkey at index 0, but it's up to you to ensure that descriptors don't collide at
+    /// other indices. If they do nothing catastrophic happens at the `KeychainTxOutIndex` level
+    /// (one keychain just becomes the defacto owner of that spk arbitrarily) but this may have
+    /// subtle implications up the application stack like one UTXO being missing from one keychain
+    /// because it has been assigned to another which produces the same script pubkey.
     pub fn insert_descriptor(
         &mut self,
         keychain: K,

crates/wallet/src/wallet/mod.rs

@@ -94,6 +94,9 @@ const COINBASE_MATURITY: u32 = 100;
 /// [`ChangeSet`]s (see [`take_staged`]). Also see individual functions and example for instructions
 /// on when [`Wallet`] state needs to be persisted.
 ///
+/// The `Wallet` descriptor (external) and change descriptor (internal) must not derive the same
+/// script pubkeys. See [`KeychainTxOutIndex::insert_descriptor()`] for more details.
+///
 /// [`signer`]: crate::signer
 /// [`take_staged`]: Wallet::take_staged
 #[derive(Debug)]

crates/wallet/tests/wallet.rs

@@ -31,7 +31,16 @@ mod common;
 use common::*;
 
 fn receive_output(wallet: &mut Wallet, value: u64, height: ConfirmationTime) -> OutPoint {
-    let addr = wallet.next_unused_address(KeychainKind::External);
+    let addr = wallet.next_unused_address(KeychainKind::External).address;
+    receive_output_to_address(wallet, addr, value, height)
+}
+
+fn receive_output_to_address(
+    wallet: &mut Wallet,
+    addr: Address,
+    value: u64,
+    height: ConfirmationTime,
+) -> OutPoint {
     let tx = Transaction {
         version: transaction::Version::ONE,
         lock_time: absolute::LockTime::ZERO,
@@ -3969,6 +3978,44 @@ fn test_taproot_load_descriptor_duplicated_keys() {
     );
 }
 
+/// In dev mode this test panics, but in release mode, or if the `debug_panic` in `TxOutIndex::replenish_inner_index`
+/// is commented out, there is no panic and the balance is calculated correctly. See issue [#1483]
+/// and PR [#1486] for discussion on mixing non-wildcard and wildcard descriptors.
+///
+/// [#1483]: https://github.com/bitcoindevkit/bdk/issues/1483
+/// [#1486]: https://github.com/bitcoindevkit/bdk/pull/1486
+#[test]
+#[should_panic(
+    expected = "replenish lookahead: must not have existing spk: keychain=Internal, lookahead=25, next_store_index=0, next_reveal_index=0"
+)]
+fn test_keychains_with_overlapping_spks() {
+    // this can happen if a non-wildcard descriptor keychain derives an spk that a
+    // wildcard descriptor keychain in the same wallet also derives.
+
+    // index 1 spk overlaps with non-wildcard change descriptor
+    let wildcard_keychain = "wpkh(tprv8ZgxMBicQKsPdDArR4xSAECuVxeX1jwwSXR4ApKbkYgZiziDc4LdBy2WvJeGDfUSE4UT4hHhbgEwbdq8ajjUHiKDegkwrNU6V55CxcxonVN/*)";
+    let non_wildcard_keychain = "wpkh(tprv8ZgxMBicQKsPdDArR4xSAECuVxeX1jwwSXR4ApKbkYgZiziDc4LdBy2WvJeGDfUSE4UT4hHhbgEwbdq8ajjUHiKDegkwrNU6V55CxcxonVN/1)";
+
+    let (mut wallet, _) = get_funded_wallet_with_change(wildcard_keychain, non_wildcard_keychain);
+    assert_eq!(wallet.balance().confirmed, Amount::from_sat(50000));
+
+    let addr = wallet
+        .reveal_addresses_to(KeychainKind::External, 1)
+        .last()
+        .unwrap()
+        .address;
+    let _outpoint = receive_output_to_address(
+        &mut wallet,
+        addr,
+        8000,
+        ConfirmationTime::Confirmed {
+            height: 2000,
+            time: 0,
+        },
+    );
+    assert_eq!(wallet.balance().confirmed, Amount::from_sat(58000));
+}
+
 #[test]
 /// The wallet should re-use previously allocated change addresses when the tx using them is cancelled
 fn test_tx_cancellation() {