feat!: check desc, return changeset in add_desc fn

Incorporated `check_wallet_descriptor` in `KeyRing::new` and
`KeyRing::add_descriptor` to catch multipath and hardened descriptors.

Introduced new variants to `DescriptorError` to report cases when trying
to add duplicate desc or keychain. Did not provide the other desc inside
`KeychainAlreadyExists` variant (similar other keychain inside
`DescAlreadyExists`) because introducing type parameter to
`DescriptorError` implies introducing one to `IntoWalletDescriptor`.

The add_descriptor function returns a `KeyRing::Changeset` since this
would eventually be required when we introducing APIs to add descriptors
to `Wallet.keyring`.

Author: 110CodingP

examples/multi_keychain_wallet.rs

@@ -58,11 +58,12 @@ fn main() {
         time_of_week_keychain: DayType::AnyDay,
     };
 
-    let mut keyring: KeyRing<KeychainId> = KeyRing::new(Network::Signet, keychain_1, DESC_1);
-    keyring.add_descriptor(keychain_2, DESC_2, false);
-    keyring.add_descriptor(keychain_3, DESC_3, false);
-    keyring.add_descriptor(keychain_4, DESC_4, false);
-    keyring.add_descriptor(keychain_5, DESC_5, false);
+    let mut keyring: KeyRing<KeychainId> =
+        KeyRing::new(Network::Signet, keychain_1, DESC_1).unwrap();
+    keyring.add_descriptor(keychain_2, DESC_2, false).unwrap();
+    keyring.add_descriptor(keychain_3, DESC_3, false).unwrap();
+    keyring.add_descriptor(keychain_4, DESC_4, false).unwrap();
+    keyring.add_descriptor(keychain_5, DESC_5, false).unwrap();
 
     // DESC_1 is the default keychain (the first one added to the keyring is automatically the
     // default keychain), but this can also be changed later on with the

examples/simple_keyring.rs

@@ -13,8 +13,11 @@ fn main() {
         Network::Regtest,
         KeychainKind::External,
         EXTERNAL_DESCRIPTOR,
-    );
-    keyring.add_descriptor(KeychainKind::Internal, INTERNAL_DESCRIPTOR, false);
+    )
+    .unwrap();
+    keyring
+        .add_descriptor(KeychainKind::Internal, INTERNAL_DESCRIPTOR, false)
+        .unwrap();
 
     let mut wallet = Wallet::new(keyring);
     let address = wallet.reveal_next_address(KeychainKind::External).unwrap();

src/descriptor/error.rs

@@ -28,7 +28,6 @@ pub enum Error {
     Key(crate::keys::KeyError),
     // /// Error while extracting and manipulating policies
     // Policy(crate::descriptor::policy::PolicyError),
-
     /// Invalid byte found in the descriptor checksum
     InvalidDescriptorCharacter(u8),
 
@@ -42,8 +41,10 @@ pub enum Error {
     Miniscript(miniscript::Error),
     /// Hex decoding error
     Hex(bitcoin::hex::HexToBytesError),
-    /// The provided wallet descriptors are identical
-    ExternalAndInternalAreTheSame,
+    /// The keychain exists in the `KeyRing` but mapped to a different descriptor
+    KeychainAlreadyExists,
+    /// The descriptor exists in the `KeyRing` but mapped to a different keychain
+    DescAlreadyExists,
 }
 
 impl From<crate::keys::KeyError> for Error {
@@ -81,9 +82,8 @@ impl fmt::Display for Error {
             Self::Pk(err) => write!(f, "Key-related error: {err}"),
             Self::Miniscript(err) => write!(f, "Miniscript error: {err}"),
             Self::Hex(err) => write!(f, "Hex decoding error: {err}"),
-            Self::ExternalAndInternalAreTheSame => {
-                write!(f, "External and internal descriptors are the same")
-            }
+            Self::KeychainAlreadyExists => write!(f, "Keychain already exists but corresponds to a different descriptor"),
+            Self::DescAlreadyExists => write!(f, "Descriptor already exists but corresponds to a different keychain"),
         }
     }
 }

src/keyring/mod.rs

@@ -16,7 +16,8 @@
 /// Contains `Changeset` corresponding to `KeyRing`.
 pub mod changeset;
 
-use crate::descriptor::IntoWalletDescriptor;
+use crate::descriptor::check_wallet_descriptor;
+use crate::descriptor::{DescriptorError, IntoWalletDescriptor};
 use crate::keyring::changeset::ChangeSet;
 use alloc::collections::BTreeMap;
 use bitcoin::secp256k1::{All, Secp256k1};
@@ -38,46 +39,65 @@ where
 {
     /// Construct a new [`KeyRing`] with the provided `network` and a descriptor. This descriptor
     /// will automatically become your default keychain. You can change your default keychain
-    /// upon adding new ones with [`KeyRing::add_descriptor`]. Note that you cannot use a
-    /// multipath descriptor here.
-    pub fn new(network: Network, keychain: K, descriptor: impl IntoWalletDescriptor) -> Self {
+    /// upon adding new ones with [`KeyRing::add_descriptor`].
+    ///
+    /// This method returns [`DescriptorError`] if the provided descriptor is multipath (in case of
+    /// public descriptors), contains hardened derivation steps or fails miniscripts sanity
+    /// checks.
+    pub fn new(
+        network: Network,
+        keychain: K,
+        descriptor: impl IntoWalletDescriptor,
+    ) -> Result<Self, DescriptorError> {
         let secp = Secp256k1::new();
-        let descriptor = descriptor
-            .into_wallet_descriptor(&secp, network)
-            .expect("err: invalid descriptor")
-            .0;
-        assert!(
-            !descriptor.is_multipath(),
-            "err: Use `add_multipath_descriptor` instead"
-        );
-        Self {
+        let descriptor = descriptor.into_wallet_descriptor(&secp, network)?.0;
+        check_wallet_descriptor(&descriptor)?;
+        Ok(Self {
             secp: Secp256k1::new(),
             network,
             descriptors: BTreeMap::from([(keychain.clone(), descriptor)]),
             default_keychain: keychain.clone(),
-        }
+        })
     }
 
     /// Add a descriptor. Must not be [multipath](miniscript::Descriptor::is_multipath).
+    /// This method returns [`DescriptorError`] if the provided descriptor is multipath, contains
+    /// hardened derivation steps (in case of public descriptors) or fails miniscripts sanity
+    /// checks. It also returns the error when exactly one of `keychain` or `descriptor` is
+    /// already in the keyring.
     pub fn add_descriptor(
         &mut self,
         keychain: K,
         descriptor: impl IntoWalletDescriptor,
         default: bool,
-    ) {
+    ) -> Result<ChangeSet<K>, DescriptorError> {
         let descriptor = descriptor
-            .into_wallet_descriptor(&self.secp, self.network)
-            .expect("err: invalid descriptor")
+            .into_wallet_descriptor(&self.secp, self.network)?
             .0;
-        assert!(
-            !descriptor.is_multipath(),
-            "err: Use `add_multipath_descriptor` instead"
-        );
+        check_wallet_descriptor(&descriptor)?;
+
+        // if the descriptor or keychain already exist
+        for (keychain_old, desc) in self.descriptors.iter() {
+            if (desc == &descriptor) && (keychain_old != &keychain) {
+                return Err(DescriptorError::DescAlreadyExists);
+            }
+            if (keychain_old == &keychain) && (desc != &descriptor) {
+                return Err(DescriptorError::KeychainAlreadyExists);
+            }
+        }
+
+        self.descriptors
+            .insert(keychain.clone(), descriptor.clone());
+
+        let mut changeset = ChangeSet::default();
+        changeset.descriptors.insert(keychain.clone(), descriptor);
 
         if default {
             self.default_keychain = keychain.clone();
+            changeset.default_keychain = Some(keychain);
         }
-        self.descriptors.insert(keychain, descriptor);
+
+        Ok(changeset)
     }
 
     /// Returns the specified default keychain on the KeyRing.

src/wallet/mod.rs

@@ -59,12 +59,15 @@ pub mod tx_builder;
 pub(crate) mod utils;
 
 use crate::descriptor::{
-    check_wallet_descriptor, error::Error as DescriptorError, 
+    check_wallet_descriptor,
+    error::Error as DescriptorError,
     // policy::BuildSatisfaction,
-    DerivedDescriptor, DescriptorMeta, ExtendedDescriptor, 
-    // ExtractPolicy, 
+    DerivedDescriptor,
+    DescriptorMeta,
+    ExtendedDescriptor,
+    // ExtractPolicy,
     IntoWalletDescriptor,
-    // Policy, 
+    // Policy,
     XKeyUtils,
 };
 use crate::keyring::KeyRing;
@@ -3002,10 +3005,10 @@ mod test {
     fn test_keyring(desc_strs: impl IntoIterator<Item = &'static str>) -> KeyRing<DescriptorId> {
         let mut desc_strs = desc_strs.into_iter();
         let desc = parse_descriptor(desc_strs.next().unwrap());
-        let mut keyring = KeyRing::new(Network::Testnet4, desc.descriptor_id(), desc);
+        let mut keyring = KeyRing::new(Network::Testnet4, desc.descriptor_id(), desc).unwrap();
         for desc_str in desc_strs {
             let desc = parse_descriptor(desc_str);
-            keyring.add_descriptor(desc.descriptor_id(), desc, false);
+            let _ = keyring.add_descriptor(desc.descriptor_id(), desc, false);
         }
         keyring
     }

tests/keyring.rs

@@ -1,17 +1,27 @@
+use bdk_wallet::chain::DescriptorExt;
+use bdk_wallet::descriptor::DescriptorError;
 use bdk_wallet::keyring::KeyRing;
 use bdk_wallet::KeychainKind;
+use bitcoin::secp256k1::Secp256k1;
 use bitcoin::Network;
+use miniscript::{Descriptor, DescriptorPublicKey};
 
 // From the mnemonic "awesome awesome awesome awesome awesome awesome awesome awesome awesome
 // awesome awesome awesome"
 const DESC_1: &str = "tr(tprv8ZgxMBicQKsPdWAHbugK2tjtVtRjKGixYVZUdL7xLHMgXZS6BFbFi1UDb1CHT25Z5PU1F9j7wGxwUiRhqz9E3nZRztikGUV6HoRDYcqPhM4/86'/1'/0'/0/*)";
 
+pub fn get_descriptor(desc_str: &str) -> Descriptor<DescriptorPublicKey> {
+    Descriptor::parse_descriptor(&Secp256k1::new(), desc_str)
+        .unwrap()
+        .0
+}
+
 #[test]
 fn test_simple_keyring() {
     let network = Network::Regtest;
     let keychain_id = KeychainKind::External;
 
-    let keyring = KeyRing::new(network, keychain_id, DESC_1);
+    let keyring = KeyRing::new(network, keychain_id, DESC_1).unwrap();
 
     assert_eq!(keyring.default_keychain(), keychain_id);
     assert_eq!(keyring.list_keychains().len(), 1);
@@ -28,15 +38,63 @@ fn test_8_keychains_keyring() {
     const DESC_7: &str = "tr(tprv8ZgxMBicQKsPdWAHbugK2tjtVtRjKGixYVZUdL7xLHMgXZS6BFbFi1UDb1CHT25Z5PU1F9j7wGxwUiRhqz9E3nZRztikGUV6HoRDYcqPhM4/86'/1'/0'/6/*)";
     const DESC_8: &str = "tr(tprv8ZgxMBicQKsPdWAHbugK2tjtVtRjKGixYVZUdL7xLHMgXZS6BFbFi1UDb1CHT25Z5PU1F9j7wGxwUiRhqz9E3nZRztikGUV6HoRDYcqPhM4/86'/1'/0'/7/*)";
 
-    let mut keyring = KeyRing::new(Network::Regtest, 1, DESC_1);
-    keyring.add_descriptor(2, DESC_2, false);
-    keyring.add_descriptor(3, DESC_3, false);
-    keyring.add_descriptor(4, DESC_4, false);
-    keyring.add_descriptor(5, DESC_5, false);
-    keyring.add_descriptor(6, DESC_6, false);
-    keyring.add_descriptor(7, DESC_7, false);
-    keyring.add_descriptor(8, DESC_8, false);
+    let mut keyring = KeyRing::new(Network::Regtest, 1, DESC_1).unwrap();
+    keyring.add_descriptor(2, DESC_2, false).unwrap();
+    keyring.add_descriptor(3, DESC_3, false).unwrap();
+    keyring.add_descriptor(4, DESC_4, false).unwrap();
+    keyring.add_descriptor(5, DESC_5, false).unwrap();
+    keyring.add_descriptor(6, DESC_6, false).unwrap();
+    keyring.add_descriptor(7, DESC_7, false).unwrap();
+    keyring.add_descriptor(8, DESC_8, false).unwrap();
 
     assert_eq!(keyring.default_keychain(), 1);
     assert_eq!(keyring.list_keychains().len(), 8);
 }
+
+#[test]
+fn err_on_hardened_derivation_path() {
+    let err =  KeyRing::new(Network::Regtest, KeychainKind::External, "tr(tpubD6NzVbkrYhZ4WyC5VZLuSJQ14uwfUbus7oAFurAFkZA5N3groeQqtW65m8pG1TT1arPpfWu9RbBsc5rSBncrX2d84BAwJJHQfaRjnMCQwuT/86h/1h/0h/0/*)").err();
+    assert_eq!(err, Some(DescriptorError::HardenedDerivationXpub));
+    let mut keyring = KeyRing::new(Network::Regtest, KeychainKind::External, DESC_1).unwrap();
+    let res = keyring.add_descriptor(KeychainKind::Internal,"tr([738b4dbd/86h/1h/0h]tpubDDQsJyQKuP6jCCSZ75Y8zpBAnXsvAN6BWpp6ZoczfxKBDBWnY8XGbC7AMMSyXAcQPNgppkCBmv3hkCLZSaQ4VvSTGsstuTrXuDadMaB7E45/0'/*)", false).err();
+    assert_eq!(res, Some(DescriptorError::HardenedDerivationXpub));
+}
+
+#[test]
+fn err_on_multipath() {
+    let err =  KeyRing::new(Network::Regtest, KeychainKind::External, "pkh(tpubD6NzVbkrYhZ4WaWSyoBvQwbpLkojyoTZPRsgXELWz3Popb3qkjcJyJUGLnL4qHHoQvao8ESaAstxYSnhyswJ76uZPStJRJCTKvosUCJZL5B/1/1/<0;1>)").err();
+    assert_eq!(
+        err,
+        Some(DescriptorError::Miniscript(
+            miniscript::Error::BadDescriptor(
+                "`check_wallet_descriptor` must not contain multipath keys".to_string(),
+            )
+        ))
+    );
+    let mut keyring = KeyRing::new(Network::Regtest, KeychainKind::External, DESC_1).unwrap();
+    let res = keyring.add_descriptor(KeychainKind::Internal, "tr(tpubD6NzVbkrYhZ4WyC5VZLuSJQ14uwfUbus7oAFurAFkZA5N3groeQqtW65m8pG1TT1arPpfWu9RbBsc5rSBncrX2d84BAwJJHQfaRjnMCQwuT/86/1/0/<0;1>/*)" , false).err();
+    assert_eq!(
+        res,
+        Some(DescriptorError::Miniscript(
+            miniscript::Error::BadDescriptor(
+                "`check_wallet_descriptor` must not contain multipath keys".to_string(),
+            )
+        ))
+    );
+}
+
+#[test]
+fn duplicate_desc_keychain() {
+    let desc1 = get_descriptor(DESC_1);
+    let mut keyring = KeyRing::new(Network::Regtest, desc1.descriptor_id(), desc1.clone()).unwrap();
+    let desc2 = get_descriptor("tr(tprv8ZgxMBicQKsPdWAHbugK2tjtVtRjKGixYVZUdL7xLHMgXZS6BFbFi1UDb1CHT25Z5PU1F9j7wGxwUiRhqz9E3nZRztikGUV6HoRDYcqPhM4/86'/1'/0'/1/*)");
+    let res1 = keyring
+        .add_descriptor(desc2.descriptor_id(), desc1.clone(), false)
+        .err();
+    assert_eq!(res1, Some(DescriptorError::DescAlreadyExists));
+
+    let res2 = keyring
+        .add_descriptor(desc1.descriptor_id(), desc2, false)
+        .err();
+    assert_eq!(res2, Some(DescriptorError::KeychainAlreadyExists));
+}