diff --git a/crates/chain/src/descriptor_ext.rs b/crates/chain/src/descriptor_ext.rs index e42d291bb..eac01a684 100644 --- a/crates/chain/src/descriptor_ext.rs +++ b/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 { } fn descriptor_id(&self) -> DescriptorId { - let desc = self.to_string(); - let desc_without_checksum = desc.split('#').next().expect("Must be here"); - let descriptor_bytes = >::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())) } } diff --git a/crates/chain/src/keychain/txout_index.rs b/crates/chain/src/keychain/txout_index.rs index e4fad3d5a..758f16cbc 100644 --- a/crates/chain/src/keychain/txout_index.rs +++ b/crates/chain/src/keychain/txout_index.rs @@ -352,6 +352,13 @@ impl KeychainTxOutIndex { /// /// keychain <-> descriptor is a one-to-one mapping that cannot be changed. Attempting to do so /// will return a [`InsertDescriptorError`]. + /// + /// `[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, diff --git a/crates/wallet/src/wallet/mod.rs b/crates/wallet/src/wallet/mod.rs index 9fa9e5300..001e9f088 100644 --- a/crates/wallet/src/wallet/mod.rs +++ b/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)] diff --git a/crates/wallet/tests/wallet.rs b/crates/wallet/tests/wallet.rs index 1f8aa4510..08579396e 100644 --- a/crates/wallet/tests/wallet.rs +++ b/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() {