Merge #1523: bdk_wallet: Don't reimplement descriptor checksum, use the one from rust-miniscript

70c7d6b bdk_wallet: remove unnecessary calls to calc_checksum (Antoine Poinsot)
8245fb7 bdk_wallet: don't reimplement checksum algorithm, use rust-miniscript (Antoine Poinsot)
a11ace2 bdk_wallet: descriptor: remove unnecessary call to calc_checksum_bytes (Antoine Poinsot)

Pull request description:

  Skimming through the BDK code today i noticed the descriptor checksum algorithm is re-implemented instead of using the implementation from rust-miniscript. Re-implementing it here is more code to review and maintain, more room for mistakes and overall less eyes over the code than centralizing a single implementation over at `rust-miniscript`.

  While doing this i noticed that one of the variants was completely unnecessary (`calc_checksum_bytes`), so i removed it. I realise it's breaking the API, so if we want to avoid that i can remove this part. I also added a commit to remove redundant calls to `calc_checsum`.

  Overall this is just a quick PoC as i noticed it i figured i'd send a PR, my bad if i missed anything!

ACKs for top commit:
  storopoli:
    utACK 70c7d6b
  evanlinjin:
    ACK 70c7d6b

Tree-SHA512: 89d0e582979c8260a97646abc3821a9ec1fd84b5ff1e9236efa87654bd31d5da654c9725f4e1c9871a28c9312c0cfe6e095e50742abe5fbd0490fc9a2e41c86b

Author: evanlinjin

crates/wallet/src/descriptor/checksum.rs

@@ -17,82 +17,20 @@
 use crate::descriptor::DescriptorError;
 use alloc::string::String;
 
-const INPUT_CHARSET: &[u8] = b"0123456789()[],'/*abcdefgh@:$%{}IJKLMNOPQRSTUVWXYZ&+-.;<=>?!^_|~ijklmnopqrstuvwxyzABCDEFGH`#\"\\ ";
-const CHECKSUM_CHARSET: &[u8] = b"qpzry9x8gf2tvdw0s3jn54khce6mua7l";
+use miniscript::descriptor::checksum::desc_checksum;
 
-fn poly_mod(mut c: u64, val: u64) -> u64 {
-    let c0 = c >> 35;
-    c = ((c & 0x7ffffffff) << 5) ^ val;
-    if c0 & 1 > 0 {
-        c ^= 0xf5dee51989
-    };
-    if c0 & 2 > 0 {
-        c ^= 0xa9fdca3312
-    };
-    if c0 & 4 > 0 {
-        c ^= 0x1bab10e32d
-    };
-    if c0 & 8 > 0 {
-        c ^= 0x3706b1677a
-    };
-    if c0 & 16 > 0 {
-        c ^= 0x644d626ffd
-    };
-
-    c
-}
-
-/// Compute the checksum bytes of a descriptor, excludes any existing checksum in the descriptor string from the calculation
-pub fn calc_checksum_bytes(mut desc: &str) -> Result<[u8; 8], DescriptorError> {
-    let mut c = 1;
-    let mut cls = 0;
-    let mut clscount = 0;
-
-    let mut original_checksum = None;
+/// Compute the checksum of a descriptor, excludes any existing checksum in the descriptor string from the calculation
+pub fn calc_checksum(desc: &str) -> Result<String, DescriptorError> {
     if let Some(split) = desc.split_once('#') {
-        desc = split.0;
-        original_checksum = Some(split.1);
-    }
-
-    for ch in desc.as_bytes() {
-        let pos = INPUT_CHARSET
-            .iter()
-            .position(|b| b == ch)
-            .ok_or(DescriptorError::InvalidDescriptorCharacter(*ch))? as u64;
-        c = poly_mod(c, pos & 31);
-        cls = cls * 3 + (pos >> 5);
-        clscount += 1;
-        if clscount == 3 {
-            c = poly_mod(c, cls);
-            cls = 0;
-            clscount = 0;
-        }
-    }
-    if clscount > 0 {
-        c = poly_mod(c, cls);
-    }
-    (0..8).for_each(|_| c = poly_mod(c, 0));
-    c ^= 1;
-
-    let mut checksum = [0_u8; 8];
-    for j in 0..8 {
-        checksum[j] = CHECKSUM_CHARSET[((c >> (5 * (7 - j))) & 31) as usize];
-    }
-
-    // if input data already had a checksum, check calculated checksum against original checksum
-    if let Some(original_checksum) = original_checksum {
-        if original_checksum.as_bytes() != checksum {
+        let og_checksum = split.1;
+        let checksum = desc_checksum(split.0)?;
+        if og_checksum != checksum {
             return Err(DescriptorError::InvalidDescriptorChecksum);
         }
+        Ok(checksum)
+    } else {
+        Ok(desc_checksum(desc)?)
     }
-
-    Ok(checksum)
-}
-
-/// Compute the checksum of a descriptor, excludes any existing checksum in the descriptor string from the calculation
-pub fn calc_checksum(desc: &str) -> Result<String, DescriptorError> {
-    // unsafe is okay here as the checksum only uses bytes in `CHECKSUM_CHARSET`
-    calc_checksum_bytes(desc).map(|b| unsafe { String::from_utf8_unchecked(b.to_vec()) })
 }
 
 #[cfg(test)]
@@ -141,7 +79,7 @@ mod test {
 
         assert_matches!(
             calc_checksum(&invalid_desc),
-            Err(DescriptorError::InvalidDescriptorCharacter(invalid_char)) if invalid_char == sparkle_heart.as_bytes()[0]
+            Err(DescriptorError::Miniscript(miniscript::Error::BadDescriptor(e))) if e == format!("Invalid character in checksum: '{}'", sparkle_heart)
         );
     }
 }

crates/wallet/src/descriptor/mod.rs

@@ -42,7 +42,6 @@ pub mod policy;
 pub mod template;
 
 pub use self::checksum::calc_checksum;
-use self::checksum::calc_checksum_bytes;
 pub use self::error::Error as DescriptorError;
 pub use self::policy::Policy;
 use self::template::DescriptorTemplateOut;
@@ -88,8 +87,8 @@ impl IntoWalletDescriptor for &str {
     ) -> Result<(ExtendedDescriptor, KeyMap), DescriptorError> {
         let descriptor = match self.split_once('#') {
             Some((desc, original_checksum)) => {
-                let checksum = calc_checksum_bytes(desc)?;
-                if original_checksum.as_bytes() != checksum {
+                let checksum = calc_checksum(desc)?;
+                if original_checksum != checksum {
                     return Err(DescriptorError::InvalidDescriptorChecksum);
                 }
                 desc

crates/wallet/src/wallet/mod.rs

@@ -79,7 +79,7 @@ use utils::{check_nsequence_rbf, After, Older, SecpCtx};
 
 use crate::descriptor::policy::BuildSatisfaction;
 use crate::descriptor::{
-    self, calc_checksum, DerivedDescriptor, DescriptorMeta, ExtendedDescriptor, ExtractPolicy,
+    self, DerivedDescriptor, DescriptorMeta, ExtendedDescriptor, ExtractPolicy,
     IntoWalletDescriptor, Policy, XKeyUtils,
 };
 use crate::psbt::PsbtUtils;
@@ -2360,15 +2360,13 @@ where
         .into_wallet_descriptor(secp, network)?
         .0
         .to_string();
-    let mut wallet_name = calc_checksum(&descriptor[..descriptor.find('#').unwrap()])?;
+    let mut wallet_name = descriptor.split_once('#').unwrap().1.to_string();
     if let Some(change_descriptor) = change_descriptor {
         let change_descriptor = change_descriptor
             .into_wallet_descriptor(secp, network)?
             .0
             .to_string();
-        wallet_name.push_str(
-            calc_checksum(&change_descriptor[..change_descriptor.find('#').unwrap()])?.as_str(),
-        );
+        wallet_name.push_str(change_descriptor.split_once('#').unwrap().1);
     }
 
     Ok(wallet_name)