Merge #765: Fix how descriptor checksums are calculated

648282e Update docs and tests based on review comments (Steve Myers)
60057a7 Deprecate backward compatible get_checksum_bytes, get_checksum functions (Steve Myers)
e2a4a58 Ensure backward compatibility of the  "checksum inception" bug (志宇)
fd34956 `get_checksum_bytes` now checks input data for checksum (志宇)

Pull request description:

  ### Description

  Previously, the methods `get_checksum_bytes` and `get_checksum` do not check input data to see whether the input data already has a checksum.

  This PR does the following:

  * Introduce a `exclude_hash: bool` flag for `get_checksum_bytes`, that excludes the checksum portion of the original data when calculating the checksum. In addition to this, if the calculated checksum does not match the original checksum, an error is returned for extra safety.
  * Ensure `Wallet` is still backwards compatible with databases created with the "checksum inception" bug.

  ### Notes to the reviewers

  Thank you.

  ### Changelog notice

  Fix the "checksum inception" bug, where we may accidentally calculate the checksum of a descriptor that already has a checksum.

  ### 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
  ~* [ ] I'm linking the issue being fixed by this PR~

Top commit has no ACKs.

Tree-SHA512: 7ea2721dcd56459b6996e56a3ddfc3559a0c64869a08f5312a8f0f4fcb5dbef7ac7461a4ab017acde4a62fed02d8a620c402dd384323aba85736610514fcb7e1

Author: notmandatory

src/blockchain/rpc.rs

@@ -35,7 +35,7 @@ use crate::bitcoin::hashes::hex::ToHex;
 use crate::bitcoin::{Network, OutPoint, Transaction, TxOut, Txid};
 use crate::blockchain::*;
 use crate::database::{BatchDatabase, BatchOperations, DatabaseUtils};
-use crate::descriptor::get_checksum;
+use crate::descriptor::calc_checksum;
 use crate::error::MissingCachedScripts;
 use crate::{BlockTime, Error, FeeRate, KeychainKind, LocalUtxo, TransactionDetails};
 use bitcoin::Script;
@@ -806,7 +806,7 @@ fn is_wallet_descriptor(client: &Client) -> Result<bool, Error> {
 
 fn descriptor_from_script_pubkey(script: &Script) -> String {
     let desc = format!("raw({})", script.to_hex());
-    format!("{}#{}", desc, get_checksum(&desc).unwrap())
+    format!("{}#{}", desc, calc_checksum(&desc).unwrap())
 }
 
 /// Factory of [`RpcBlockchain`] instances, implements [`BlockchainFactory`]

src/descriptor/checksum.rs

@@ -41,12 +41,24 @@ fn poly_mod(mut c: u64, val: u64) -> u64 {
     c
 }
 
-/// Computes the checksum bytes of a descriptor
-pub fn get_checksum_bytes(desc: &str) -> Result<[u8; 8], DescriptorError> {
+/// Computes the checksum bytes of a descriptor.
+/// `exclude_hash = true` ignores all data after the first '#' (inclusive).
+pub(crate) fn calc_checksum_bytes_internal(
+    mut desc: &str,
+    exclude_hash: bool,
+) -> Result<[u8; 8], DescriptorError> {
     let mut c = 1;
     let mut cls = 0;
     let mut clscount = 0;
 
+    let mut original_checksum = None;
+    if exclude_hash {
+        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()
@@ -72,37 +84,96 @@ pub fn get_checksum_bytes(desc: &str) -> Result<[u8; 8], DescriptorError> {
         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 {
+            return Err(DescriptorError::InvalidDescriptorChecksum);
+        }
+    }
+
     Ok(checksum)
 }
 
+/// Compute the checksum bytes of a descriptor, excludes any existing checksum in the descriptor string from the calculation
+pub fn calc_checksum_bytes(desc: &str) -> Result<[u8; 8], DescriptorError> {
+    calc_checksum_bytes_internal(desc, true)
+}
+
+/// 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_internal(desc, true)
+        .map(|b| unsafe { String::from_utf8_unchecked(b.to_vec()) })
+}
+
+// TODO in release 0.25.0, remove get_checksum_bytes and get_checksum
+// TODO in release 0.25.0, consolidate calc_checksum_bytes_internal into calc_checksum_bytes
+
+/// Compute the checksum bytes of a descriptor
+#[deprecated(
+    since = "0.24.0",
+    note = "Use new `calc_checksum_bytes` function which excludes any existing checksum in the descriptor string before calculating the checksum hash bytes. See https://github.com/bitcoindevkit/bdk/pull/765."
+)]
+pub fn get_checksum_bytes(desc: &str) -> Result<[u8; 8], DescriptorError> {
+    calc_checksum_bytes_internal(desc, false)
+}
+
 /// Compute the checksum of a descriptor
+#[deprecated(
+    since = "0.24.0",
+    note = "Use new `calc_checksum` function which excludes any existing checksum in the descriptor string before calculating the checksum hash. See https://github.com/bitcoindevkit/bdk/pull/765."
+)]
 pub fn get_checksum(desc: &str) -> Result<String, DescriptorError> {
     // unsafe is okay here as the checksum only uses bytes in `CHECKSUM_CHARSET`
-    get_checksum_bytes(desc).map(|b| unsafe { String::from_utf8_unchecked(b.to_vec()) })
+    calc_checksum_bytes_internal(desc, false)
+        .map(|b| unsafe { String::from_utf8_unchecked(b.to_vec()) })
 }
 
 #[cfg(test)]
 mod test {
     use super::*;
-    use crate::descriptor::get_checksum;
+    use crate::descriptor::calc_checksum;
 
-    // test get_checksum() function; it should return the same value as Bitcoin Core
+    // test calc_checksum() function; it should return the same value as Bitcoin Core
     #[test]
-    fn test_get_checksum() {
+    fn test_calc_checksum() {
         let desc = "wpkh(tprv8ZgxMBicQKsPdpkqS7Eair4YxjcuuvDPNYmKX3sCniCf16tHEVrjjiSXEkFRnUH77yXc6ZcwHHcLNfjdi5qUvw3VDfgYiH5mNsj5izuiu2N/1/2/*)";
-        assert_eq!(get_checksum(desc).unwrap(), "tqz0nc62");
+        assert_eq!(calc_checksum(desc).unwrap(), "tqz0nc62");
 
         let desc = "pkh(tpubD6NzVbkrYhZ4XHndKkuB8FifXm8r5FQHwrN6oZuWCz13qb93rtgKvD4PQsqC4HP4yhV3tA2fqr2RbY5mNXfM7RxXUoeABoDtsFUq2zJq6YK/44'/1'/0'/0/*)";
-        assert_eq!(get_checksum(desc).unwrap(), "lasegmfs");
+        assert_eq!(calc_checksum(desc).unwrap(), "lasegmfs");
+    }
+
+    // test calc_checksum() function; it should return the same value as Bitcoin Core even if the
+    // descriptor string includes a checksum hash
+    #[test]
+    fn test_calc_checksum_with_checksum_hash() {
+        let desc = "wpkh(tprv8ZgxMBicQKsPdpkqS7Eair4YxjcuuvDPNYmKX3sCniCf16tHEVrjjiSXEkFRnUH77yXc6ZcwHHcLNfjdi5qUvw3VDfgYiH5mNsj5izuiu2N/1/2/*)#tqz0nc62";
+        assert_eq!(calc_checksum(desc).unwrap(), "tqz0nc62");
+
+        let desc = "pkh(tpubD6NzVbkrYhZ4XHndKkuB8FifXm8r5FQHwrN6oZuWCz13qb93rtgKvD4PQsqC4HP4yhV3tA2fqr2RbY5mNXfM7RxXUoeABoDtsFUq2zJq6YK/44'/1'/0'/0/*)#lasegmfs";
+        assert_eq!(calc_checksum(desc).unwrap(), "lasegmfs");
+
+        let desc = "wpkh(tprv8ZgxMBicQKsPdpkqS7Eair4YxjcuuvDPNYmKX3sCniCf16tHEVrjjiSXEkFRnUH77yXc6ZcwHHcLNfjdi5qUvw3VDfgYiH5mNsj5izuiu2N/1/2/*)#tqz0nc26";
+        assert!(matches!(
+            calc_checksum(desc).err(),
+            Some(DescriptorError::InvalidDescriptorChecksum)
+        ));
+
+        let desc = "pkh(tpubD6NzVbkrYhZ4XHndKkuB8FifXm8r5FQHwrN6oZuWCz13qb93rtgKvD4PQsqC4HP4yhV3tA2fqr2RbY5mNXfM7RxXUoeABoDtsFUq2zJq6YK/44'/1'/0'/0/*)#lasegmsf";
+        assert!(matches!(
+            calc_checksum(desc).err(),
+            Some(DescriptorError::InvalidDescriptorChecksum)
+        ));
     }
 
     #[test]
-    fn test_get_checksum_invalid_character() {
+    fn test_calc_checksum_invalid_character() {
         let sparkle_heart = unsafe { std::str::from_utf8_unchecked(&[240, 159, 146, 150]) };
         let invalid_desc = format!("wpkh(tprv8ZgxMBicQKsPdpkqS7Eair4YxjcuuvDPNYmKX3sCniCf16tHEVrjjiSXEkFRnUH77yXc6ZcwHHcL{}fjdi5qUvw3VDfgYiH5mNsj5izuiu2N/1/2/*)", sparkle_heart);
 
         assert!(matches!(
-            get_checksum(&invalid_desc).err(),
+            calc_checksum(&invalid_desc).err(),
             Some(DescriptorError::InvalidDescriptorCharacter(invalid_char)) if invalid_char == sparkle_heart.as_bytes()[0]
         ));
     }

src/descriptor/mod.rs

@@ -37,7 +37,8 @@ pub mod error;
 pub mod policy;
 pub mod template;
 
-pub use self::checksum::get_checksum;
+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;
@@ -81,19 +82,15 @@ impl IntoWalletDescriptor for &str {
         secp: &SecpCtx,
         network: Network,
     ) -> Result<(ExtendedDescriptor, KeyMap), DescriptorError> {
-        let descriptor = if self.contains('#') {
-            let parts: Vec<&str> = self.splitn(2, '#').collect();
-            if !get_checksum(parts[0])
-                .ok()
-                .map(|computed| computed == parts[1])
-                .unwrap_or(false)
-            {
-                return Err(DescriptorError::InvalidDescriptorChecksum);
+        let descriptor = match self.split_once('#') {
+            Some((desc, original_checksum)) => {
+                let checksum = calc_checksum_bytes(desc)?;
+                if original_checksum.as_bytes() != checksum {
+                    return Err(DescriptorError::InvalidDescriptorChecksum);
+                }
+                desc
             }
-
-            parts[0]
-        } else {
-            self
+            None => self,
         };
 
         ExtendedDescriptor::parse_descriptor(secp, descriptor)?

src/descriptor/policy.rs

@@ -63,7 +63,7 @@ use crate::keys::ExtScriptContext;
 use crate::wallet::signer::{SignerId, SignersContainer};
 use crate::wallet::utils::{After, Older, SecpCtx};
 
-use super::checksum::get_checksum;
+use super::checksum::calc_checksum;
 use super::error::Error;
 use super::XKeyUtils;
 use bitcoin::util::psbt::{Input as PsbtInput, PartiallySignedTransaction as Psbt};
@@ -168,7 +168,7 @@ impl SatisfiableItem {
 
     /// Returns a unique id for the [`SatisfiableItem`]
     pub fn id(&self) -> String {
-        get_checksum(&serde_json::to_string(self).expect("Failed to serialize a SatisfiableItem"))
+        calc_checksum(&serde_json::to_string(self).expect("Failed to serialize a SatisfiableItem"))
             .expect("Failed to compute a SatisfiableItem id")
     }
 }

src/wallet/mod.rs

@@ -59,9 +59,10 @@ use utils::{check_nsequence_rbf, After, Older, SecpCtx};
 use crate::blockchain::{GetHeight, NoopProgress, Progress, WalletSync};
 use crate::database::memory::MemoryDatabase;
 use crate::database::{AnyDatabase, BatchDatabase, BatchOperations, DatabaseUtils, SyncTime};
+use crate::descriptor::checksum::calc_checksum_bytes_internal;
 use crate::descriptor::policy::BuildSatisfaction;
 use crate::descriptor::{
-    get_checksum, into_wallet_descriptor_checked, DerivedDescriptor, DescriptorMeta,
+    calc_checksum, into_wallet_descriptor_checked, DerivedDescriptor, DescriptorMeta,
     ExtendedDescriptor, ExtractPolicy, IntoWalletDescriptor, Policy, XKeyUtils,
 };
 use crate::error::{Error, MiniscriptPsbtError};
@@ -193,28 +194,27 @@ where
         let secp = Secp256k1::new();
 
         let (descriptor, keymap) = into_wallet_descriptor_checked(descriptor, &secp, network)?;
-        database.check_descriptor_checksum(
+        Self::db_checksum(
+            &mut database,
+            &descriptor.to_string(),
             KeychainKind::External,
-            get_checksum(&descriptor.to_string())?.as_bytes(),
         )?;
         let signers = Arc::new(SignersContainer::build(keymap, &descriptor, &secp));
         let (change_descriptor, change_signers) = match change_descriptor {
             Some(desc) => {
                 let (change_descriptor, change_keymap) =
                     into_wallet_descriptor_checked(desc, &secp, network)?;
-                database.check_descriptor_checksum(
+                Self::db_checksum(
+                    &mut database,
+                    &change_descriptor.to_string(),
                     KeychainKind::Internal,
-                    get_checksum(&change_descriptor.to_string())?.as_bytes(),
                 )?;
 
                 let change_signers = Arc::new(SignersContainer::build(
                     change_keymap,
                     &change_descriptor,
                     &secp,
                 ));
-                // if !parsed.same_structure(descriptor.as_ref()) {
-                //     return Err(Error::DifferentDescriptorStructure);
-                // }
 
                 (Some(change_descriptor), change_signers)
             }
@@ -232,6 +232,19 @@ where
         })
     }
 
+    /// This checks the checksum within [`BatchDatabase`] twice (if needed). The first time with the
+    /// actual checksum, and the second time with the checksum of `descriptor+checksum`. The second
+    /// check is necessary for backwards compatibility of a checksum-inception bug.
+    fn db_checksum(db: &mut D, desc: &str, kind: KeychainKind) -> Result<(), Error> {
+        let checksum = calc_checksum_bytes_internal(desc, true)?;
+        if db.check_descriptor_checksum(kind, checksum).is_ok() {
+            return Ok(());
+        }
+
+        let checksum_inception = calc_checksum_bytes_internal(desc, false)?;
+        db.check_descriptor_checksum(kind, checksum_inception)
+    }
+
     /// Get the Bitcoin network the wallet is using.
     pub fn network(&self) -> Network {
         self.network
@@ -1781,14 +1794,14 @@ where
         .into_wallet_descriptor(secp, network)?
         .0
         .to_string();
-    let mut wallet_name = get_checksum(&descriptor[..descriptor.find('#').unwrap()])?;
+    let mut wallet_name = calc_checksum(&descriptor[..descriptor.find('#').unwrap()])?;
     if let Some(change_descriptor) = change_descriptor {
         let change_descriptor = change_descriptor
             .into_wallet_descriptor(secp, network)?
             .0
             .to_string();
         wallet_name.push_str(
-            get_checksum(&change_descriptor[..change_descriptor.find('#').unwrap()])?.as_str(),
+            calc_checksum(&change_descriptor[..change_descriptor.find('#').unwrap()])?.as_str(),
         );
     }
 
@@ -1860,15 +1873,38 @@ pub(crate) mod test {
         let (wallet, _, _) = get_funded_wallet(get_test_wpkh());
         let checksum = wallet.descriptor_checksum(KeychainKind::External);
         assert_eq!(checksum.len(), 8);
+        assert_eq!(
+            calc_checksum(&wallet.descriptor.to_string()).unwrap(),
+            checksum
+        );
+    }
 
-        let raw_descriptor = wallet
-            .descriptor
-            .to_string()
-            .split_once('#')
-            .unwrap()
-            .0
-            .to_string();
-        assert_eq!(get_checksum(&raw_descriptor).unwrap(), checksum);
+    #[test]
+    fn test_db_checksum() {
+        let (wallet, _, _) = get_funded_wallet(get_test_wpkh());
+        let desc = wallet.descriptor.to_string();
+
+        let checksum = calc_checksum_bytes_internal(&desc, true).unwrap();
+        let checksum_inception = calc_checksum_bytes_internal(&desc, false).unwrap();
+        let checksum_invalid = [b'q'; 8];
+
+        let mut db = MemoryDatabase::new();
+        db.check_descriptor_checksum(KeychainKind::External, checksum)
+            .expect("failed to save actual checksum");
+        Wallet::db_checksum(&mut db, &desc, KeychainKind::External)
+            .expect("db that uses actual checksum should be supported");
+
+        let mut db = MemoryDatabase::new();
+        db.check_descriptor_checksum(KeychainKind::External, checksum_inception)
+            .expect("failed to save checksum inception");
+        Wallet::db_checksum(&mut db, &desc, KeychainKind::External)
+            .expect("db that uses checksum inception should be supported");
+
+        let mut db = MemoryDatabase::new();
+        db.check_descriptor_checksum(KeychainKind::External, checksum_invalid)
+            .expect("failed to save invalid checksum");
+        Wallet::db_checksum(&mut db, &desc, KeychainKind::External)
+            .expect_err("db that uses invalid checksum should fail");
     }
 
     #[test]