Avoid 'ChecksumMismatch' errors

[notmandatory]

The Wallet trait currently checks given descriptor checksums against the checksums stored in the configured database. This causes an error if one uses the same descriptor but swaps the xprv and xpub keys, or uses a different master key and derivation path, even if the new descriptor will generate and/or be able to sign the same output scripts.

[notmandatory]

I think this can be solved by storing and checking the first derived address, or a hash of it for additional privacy, instead of the descriptor checksum. As long as different descriptors generate the same output scripts, which will map to the same address then I believe the wallet should treat them as equivalent.

[i5hi]

Makes sense to me. It would be a nice optimization, especially for larger scripts.

I'm looking into this within the context of wallet/mod.rs - line 129 - impl Wallet _new() calling check_descriptor_checksum at database/keyvalue.rs - line 194

Edit:
database/mod.rs -> line 98 check_descriptor_checksum which is implemented in database/keyvalue.rs for sled and memory.rs for in memory.

[i5hi]

instead it would call a check_descriptor_first_address_hash?

[i5hi]

Wondering if we could use some data extracted from the descriptors policy as an identifier, rather than the hash of the first address.

Edit:
Doesn't seem so. Just checked against a few cases and the policy id is different for each descriptor.

[afilini]

I agree that it would be nice to be able to do that, but on the other end this means committing to not having any "descriptor-specific" data in the database. Basically anything we store would have to be valid for any descriptor that produces the same addresses but that can have any combination of public/private keys, which might limit us in the future potentially.

I'm not sure if this is really worth it, because after all in the real world I'm not expecting this would happen very often: most of the times if you have different keys you also keep them on different devices.

Is there a specific use-case you had in mind @notmandatory other than just testing/debugging?

[notmandatory]

@afilini my original motivation is I believe you mentioned once that it'd be nice to have a better way to hash the descriptor that wasn't based only on the text but captured the semantic meaning of the descriptor. One way I can see this still being useful (beyond only for testing) is if you are using the same key but derived from a different origin key path, ie. if wpkh([abcdef/84'/1'/1'/0]tpubXXX/*) and wpkh([abcdef/84'/1'/1']tpubYYY/0/*) both produce the same scripts, then they should be valid for the same wallet and not produce a checksum error.

[notmandatory]

One other case I thought of after reading about the recent hardened path issue rust-bitcoin/rust-bitcoin#608 with ' vs h , based on text checksum two identical scripts will fail vs this approach where they would produce the same address and not fail. But I consider this a low priority issue, it was just one of those things I thought I had a good solution for and wanted to get it out there. 🙂

[afilini]

Yeah, it can be useful to have the same checksum for descriptors that encode essentially the same script, the only thing is that it might complicate things if we use that "generalized" checksum for our db for the reason I was explaining above.

One place where we could use this though is the rpc backend, because in that context we never import private keys into the node, so we don't have to worry about two descriptors potentially containing different secrets inside.

[LLFourn]

Why not load the descriptor from the database itself to avoid this error?
i.e. you can either pass in a descriptor in which case we are initializing a new database or you just pass in a database and the wallet loads itself from there.

[afilini]

That requires changing some of the assumptions about the db, like the fact that it can completely be recreated with a sync or that it doesn't contain any secrets

[johncantrell97]

I'll add what I thought would have been an extremely common use case where I ran into this problem.

As part of a hot wallet you would want to keep the xprv encrypted as much as possible and only decrypt if you actually need it (to sign). This means I was building and saving a Wallet with xpub descriptor which lets me generate receive addresses and monitor balance (what user is doing most of the time). When they go to sign I would prompt for pin/password to decrypt xprv. I would then want to build a Wallet instance with the xprv swapped in only for signing the tx.

[afilini]

This is a good point, I had not considered this potential use-case. Let me work on #359 first because this change will probably need to touch the database, and then we can think about implementing it

[johncantrell97]

So oddly enough I guess in some cases swapping xpub/xprv in the descriptor does not cause a problem with the checksum? I can instantiate and sync a wallet with wpkh([fingerprint/84h/0h/0h]xpub/0/) and then when it's time to build a transaction, sign, and broadcast if I build it with wpkh([fingerprint/84h/0h/0h]xprv/0/) it lets me do that without any issues.

Maybe this always wouldn't work depending on the descriptor and I'm just getting lucky? With that said, I haven't looked at how the checksum is calculated so maybe it's expected.

[afilini]

Uhm, I don't remember how the checksum works exactly, but if I had to guess I would say that this shouldn't happen accidentally. I'm more inclined to think that this is a bug in BDK that doesn't properly compare the checksum, rather than a collision on the checksum.

If you want to double check if this is really a collision you can use the getdescriptorinfo RPC call to let Core compute the checksum.

[johncantrell97]

Oh yeah good point. I have a feeling then it's probably a bug with my sqlite backend I'm using. I'll investigate it today and report back

[johncantrell97]

okay, did some research and figured out what's going on.

If I start with these two descriptors (same descriptor just substituting the xpub and xprv):

let xprv_descriptor = "wpkh([aa98cf8e/84h/1h/0h]tprv8gfpa5FSmqGCx3gjtpwvk3W44aCEQjGVBBzHLNwJujNzoezLMSZ7NC44wpnTheqkh9DH4Mpy5PPXUw4nrRVY4XvVb7Q3XGWnxDoQYeVXhxP/0/*)";

let xpub_descriptor = "wpkh([aa98cf8e/84h/1h/0h]tpubDDMriVHgvCwsqWiXnUcX9TAAdbiAa4TPkVb4ctycL1BPe9F6yqNhYgfw7yoPG3HppgezcvEutwDPTyE4vq5VRNkAghEea5fGS8WzdgVx91P/0/*)";

if I do this:

use bdk::descriptor::checksum::get_checksum;

println!("{:?}", get_checksum(xpub_descriptor));
println!("{:?}", get_checksum(xprv_descriptor));

I get Ok("fktxmgw3") and Ok("w5xn79sp") showing that if I calculate the checksum directly on these descriptor strings they produce different checksums (as we expected).

However, in Wallet::_new we first convert the IntoWalletDescriptor String to an DescriptorPublicKey using let (descriptor, keymap) = into_wallet_descriptor_checked(descriptor, &secp, network)?;

We then pass in &descriptor.to_string() to get_checksum and not the raw descriptor string.

The into_wallet_descriptor code ends up using a miniscript func called parse_descriptor that has the following comment:

/// Parse a descriptor that may contain secret keys
///
/// Internally turns every secret key found into the corresponding public key and then returns a
/// a descriptor that only contains public keys and a map to lookup the secret key given a public key.

So the descriptor returned from into_wallet_descriptor is always a DescriptorPublicKey and is why what I am doing actually works.

So I guess the use case for swapping xpub/xrpv in descriptors is not relevant for this issue.

[notmandatory]

Oh wow I didn't know this, I think we can close this issue. I did a test also with multikey scripts and as you found it's normalizing the keys to pubkeys for the checksum so will work even when substituting XPRV for an XPUB. We will still get a checksum issue if different root / derivation paths are used that generate the same final keys, but that isn't a typical situation.

PRV1="[e78ead63/84'/1'/0'/0]tprv8hyCZ1Fkk2YWNAJpeypg7iDjqwjVVaepcUUe2Em3hGnsWWDNR5xuTZYjVxcsavdHL1jMDm5un1BWDS2zg1YiHZQtPjYSdTwQg6EiMpNeHri/*"
PUB1="[e78ead63/84'/1'/0'/0]tpubDEfEhRHztQEBFdLcYdVGX7srQyFReuqjBn5RJkoM7YbGLzU93UnVe4Abg6AibKQj89WkG4BBGuLzQw3QY7CZzNpEwHF9Ut5TK9NeGSHW9Jo/*"
PRV2="[6a331c6a/84'/1'/0'/0]tprv8hPxJFYHkK13Mb3EcmtaaorCQB73T1n2nibdYEpE45m55huCLnKshoaiHQ5hePQEENc7M89n9boHW9pR1iq5e6Yw4YT2gEycfrd5UmYdz99/*"
PUB2="[6a331c6a/84'/1'/0'/0]tpubDE5zSfaXtggiF452WRZAzDWJyCcycLxwN2CQpkrXUMZTvC9xyB9TtJCaTZc6GqAKMBPC19PCDE71nLzyNMRCtb4xr3pxNG8PNG2b2TQ8YTn/*"

# all these variations sync fine with no checksum issue, but changing the script in any other way fails as it should
bdk-cli wallet -w wallet1 -d "wsh(multi(2,$PUB1,$PUB2))" sync
{}
bdk-cli wallet -w wallet1 -d "wsh(multi(2,$PRV1,$PUB2))" sync
{}
bdk-cli wallet -w wallet1 -d "wsh(multi(2,$PUB1,$PRV2))" sync
{}
bdk-cli wallet -w wallet1 -d "wsh(multi(2,$PRV1,$PRV2))" sync 
{}

[afilini]

That's interesting, I think I actually wrote that part of rust-miniscript but apparently I did not fully understand all the implications it has on BDK.

I guess this:

Basically anything we store would have to be valid for any descriptor that produces the same addresses but that can have any combination of public/private keys, which might limit us in the future potentially.

is already true then. Good to know!