Merge bitcoindevkit/bdk#761: Remove redundant duplicated keys check

e2bf9734b16f5a0f419f1c1fc39c9773fd7e1128 Remove redundant duplicated keys check (Alekos Filini)

Pull request description:

  ### Description

  This check is redundant since it's already performed by miniscript (see https://docs.rs/miniscript/7.0.0/miniscript/miniscript/analyzable/enum.AnalysisError.html#variant.RepeatedPubkeys) and it was incorrectly failing on tr descriptors that contain duplicated keys across different taproot leaves

  Fixes #760

  ### Changelog notice

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

  * [x] This pull request breaks the existing API
  * [x] 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:
  danielabrozzoni:
    Code review ACK e2bf9734b16f5a0f419f1c1fc39c9773fd7e1128 - the code looks good to me, but I didn't test manually

Tree-SHA512: 3c557d741e34abf6336c7e257867f2c6f91a4be0024317af834f08ba7c0c64557bd74b643005254c9f04e953350b104b0d4d287f0d0528134b357a4adf580f87

src/descriptor/error.rs

@@ -20,8 +20,6 @@ pub enum Error {
     InvalidDescriptorChecksum,
     /// The descriptor contains hardened derivation steps on public extended keys
     HardenedDerivationXpub,
-    /// The descriptor contains multiple keys with the same BIP32 fingerprint
-    DuplicatedKeys,
 
     /// Error thrown while working with [`keys`](crate::keys)
     Key(crate::keys::KeyError),

src/descriptor/mod.rs

@@ -14,7 +14,7 @@
 //! This module contains generic utilities to work with descriptors, plus some re-exported types
 //! from [`miniscript`].
 
-use std::collections::{BTreeMap, HashSet};
+use std::collections::BTreeMap;
 use std::ops::Deref;
 
 use bitcoin::util::bip32::{ChildNumber, DerivationPath, ExtendedPubKey, Fingerprint, KeySource};
@@ -222,23 +222,9 @@ pub(crate) fn into_wallet_descriptor_checked<T: IntoWalletDescriptor>(
         return Err(DescriptorError::HardenedDerivationXpub);
     }
 
-    // Ensure that there are no duplicated keys
-    let mut found_keys = HashSet::new();
-    let descriptor_contains_duplicated_keys = descriptor.for_any_key(|k| {
-        if let DescriptorPublicKey::XPub(xkey) = k.as_key() {
-            let fingerprint = xkey.root_fingerprint(secp);
-            if found_keys.contains(&fingerprint) {
-                return true;
-            }
-
-            found_keys.insert(fingerprint);
-        }
-
-        false
-    });
-    if descriptor_contains_duplicated_keys {
-        return Err(DescriptorError::DuplicatedKeys);
-    }
+    // Run miniscript's sanity check, which will look for duplicated keys and other potential
+    // issues
+    descriptor.sanity_check()?;
 
     Ok((descriptor, keymap))
 }
@@ -923,14 +909,10 @@ mod test {
             DescriptorError::HardenedDerivationXpub
         ));
 
-        let descriptor = "wsh(multi(2,tpubD6NzVbkrYhZ4XHndKkuB8FifXm8r5FQHwrN6oZuWCz13qb93rtgKvD4PQsqC4HP4yhV3tA2fqr2RbY5mNXfM7RxXUoeABoDtsFUq2zJq6YK/0/*,tpubD6NzVbkrYhZ4XHndKkuB8FifXm8r5FQHwrN6oZuWCz13qb93rtgKvD4PQsqC4HP4yhV3tA2fqr2RbY5mNXfM7RxXUoeABoDtsFUq2zJq6YK/1/*))";
+        let descriptor = "wsh(multi(2,tpubD6NzVbkrYhZ4XHndKkuB8FifXm8r5FQHwrN6oZuWCz13qb93rtgKvD4PQsqC4HP4yhV3tA2fqr2RbY5mNXfM7RxXUoeABoDtsFUq2zJq6YK/0/*,tpubD6NzVbkrYhZ4XHndKkuB8FifXm8r5FQHwrN6oZuWCz13qb93rtgKvD4PQsqC4HP4yhV3tA2fqr2RbY5mNXfM7RxXUoeABoDtsFUq2zJq6YK/0/*))";
         let result = into_wallet_descriptor_checked(descriptor, &secp, Network::Testnet);
 
         assert!(result.is_err());
-        assert!(matches!(
-            result.unwrap_err(),
-            DescriptorError::DuplicatedKeys
-        ));
     }
 
     #[test]

src/wallet/mod.rs

@@ -2077,6 +2077,10 @@ pub(crate) mod test {
         "tr(cNJmN3fH9DDbDt131fQNkVakkpzawJBSeybCUNmP1BovpmGQ45xG,{pk(tprv8ZgxMBicQKsPdDArR4xSAECuVxeX1jwwSXR4ApKbkYgZiziDc4LdBy2WvJeGDfUSE4UT4hHhbgEwbdq8ajjUHiKDegkwrNU6V55CxcxonVN/*),pk(8aee2b8120a5f157f1223f72b5e62b825831a27a9fdf427db7cc697494d4a642)})"
     }
 
+    pub(crate) fn get_test_tr_dup_keys() -> &'static str {
+        "tr(cNJmN3fH9DDbDt131fQNkVakkpzawJBSeybCUNmP1BovpmGQ45xG,{pk(8aee2b8120a5f157f1223f72b5e62b825831a27a9fdf427db7cc697494d4a642),pk(8aee2b8120a5f157f1223f72b5e62b825831a27a9fdf427db7cc697494d4a642)})"
+    }
+
     macro_rules! assert_fee_rate {
         ($psbt:expr, $fees:expr, $fee_rate:expr $( ,@dust_change $( $dust_change:expr )* )* $( ,@add_signature $( $add_signature:expr )* )* ) => ({
             let psbt = $psbt.clone();
@@ -5554,4 +5558,19 @@ pub(crate) mod test {
         let finalized = wallet.sign(&mut psbt, Default::default()).unwrap();
         assert!(finalized);
     }
+
+    #[test]
+    fn test_taproot_load_descriptor_duplicated_keys() {
+        // Added after issue https://github.com/bitcoindevkit/bdk/issues/760
+        //
+        // Having the same key in multiple taproot leaves is safe and should be accepted by BDK
+
+        let (wallet, _, _) = get_funded_wallet(get_test_tr_dup_keys());
+        let addr = wallet.get_address(New).unwrap();
+
+        assert_eq!(
+            addr.to_string(),
+            "bcrt1pvysh4nmh85ysrkpwtrr8q8gdadhgdejpy6f9v424a8v9htjxjhyqw9c5s5"
+        );
+    }
 }