Fix Wallet::descriptor_checksum to actually return the checksum

[evanlinjin]

Description

Wallet::descriptor_checksum should return the checksum, not the descriptor without the checksum.

Notes to the reviewers

Please merge.

Changelog notice

Fix Wallet::descriptor_checksum to actually return the descriptor checksum.

Checklists

All Submissions:

• I've signed all my commits
• I followed the contribution guidelines
• I ran cargo fmt and cargo clippy before committing

Bugfixes:

* [ ] This pull request breaks the existing API

• I've added tests to reproduce the issue which are now passing
  * [ ] I'm linking the issue being fixed by this PR

[evanlinjin]

So these lines here...

bdk/src/wallet/mod.rs

Line 208 in b14e4ee

get_checksum(&descriptor.to_string())?.as_bytes(),

bdk/src/wallet/mod.rs

Line 217 in b14e4ee

get_checksum(&change_descriptor.to_string())?.as_bytes(),

Essentially, we are getting the checksum of (descriptors+checksum)... This does not seem like wanted behavior?

This needs to be fixed, but in a way that it doesn't result in incompatible databases. I think I have a way of fixing this while being backwards compatible.

For adding new db checksums: Use checksum of (descriptor).
For verifying db checksums: Either checksum(descriptor) or checksum(descriptor+checksum) is okay.

@afilini @notmandatory Is this a good proposal?

[notmandatory]

To add another wrinkle to this discussion, there's also a non-pub function in miniscript::descriptor::checksum::desc_checksum() which is what's used in the Display impl for all the script types in the miniscript crate. I think the bdk version of get_checksum() was probably added because we couldn't use the miniscript one.

I think this is relevant in that we probably shouldn't implement our own get_checksum() and certainly not have two ways in our project to get it.

EDIT: see ACK below.
@evanlinjin I'd like to get this cleaned up, but it's not urgent right? can we move it to the 0.24 milestone so we have more time to discuss?

[notmandatory]

ACK af0b369

Even though there may be more we can do to cleanup how checksums are done, this certainly fixes a clear bug and would be nice to get into the 0.23 release.

[evanlinjin]

@notmandatory yes I agree with you. I think this PR fixes an API issue, everything else I've mentioned is really more internal.

[danielabrozzoni]

utACK af0b369

[danielabrozzoni]

Oh nevermind, it looked too fun, I had to test it! 😄

ACK af0b369 - I run the test you added with the old code, and verified that the bug was there. I then run the test (with a few more dbg!() expressions) and manually verified that the problem is fixed.