Fix how descriptor checksums are calculated

[evanlinjin]

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:

• 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

[rajarshimaitra]

Concept ACK.. Great job finding this out.. I reproduced the bug by the following method

• Created a descriptor, once with and then without checksum. Calculate checksum manually using get_checksum().
• Calculate checksum using wallet_name_from_descriptor which calls the same get_checksum() function but ensures to exclude the checksum if present in descriptor.
• Read the checksum value stored in db after running bdk-cli using w/wo checksum descriptor.

Here's the trial outputs for reference

Master:
desc w checksum: wpkh(032b0558078bec38694a84933d659303e2575dae7e91685911454115bfd64487e3)#wc9dk083

get_checksum() : arwnyfm2
wallet name : wc9dk083
get_checksum  bytes  [97, 114, 119, 110, 121, 102, 109, 50]
wallet name bytes : [119, 99, 57, 100, 107, 48, 56, 51]
checksum in db : [97, 114, 119, 110, 121, 102, 109, 50]

desc w/o checksum: wpkh(032b0558078bec38694a84933d659303e2575dae7e91685911454115bfd64487e3)

get_checksum : wc9dk083
wallet name : wc9dk083
get_checksum  bytes  [119, 99, 57, 100, 107, 48, 56, 51]
wallet name bytes : [119, 99, 57, 100, 107, 48, 56, 51]
checksum in db: [97, 114, 119, 110, 121, 102, 109, 50]

PR 765:
desc w checksum: wpkh(032b0558078bec38694a84933d659303e2575dae7e91685911454115bfd64487e3)#wc9dk083
get_checksum : wc9dk083
wallet name : wc9dk083
get_checksum  bytes  [119, 99, 57, 100, 107, 48, 56, 51]
wallet name bytes : [119, 99, 57, 100, 107, 48, 56, 51]
checksum in db : [119, 99, 57, 100, 107, 48, 56, 51]

desc w/o checksum: wpkh(032b0558078bec38694a84933d659303e2575dae7e91685911454115bfd64487e3)

get_checksum : wc9dk083
wallet name : wc9dk083
get_checksum  bytes  [119, 99, 57, 100, 107, 48, 56, 51]
wallet name bytes : [119, 99, 57, 100, 107, 48, 56, 51]
checksum in db : [119, 99, 57, 100, 107, 48, 56, 51]

Observation:

• Master is clearly storing wrong checksum in db for desc with checksum data. But it seems master is also storing the wrong checksum in case of descriptor w/o checksum. Check the Master Case 2, checksum in db value. I am not sure why this is happening. This means master will forcefully include the checksum in checksum calculation, even if its not there?

• In any case this PR seems to be working as expected, and both get_checksum and checksum stored in db is matching with wallet_name_from_descriptor, so everything seems consistent.

Few comments below on code.

[danielabrozzoni]

I reviewed only the first commit for now, but I feel like we need to tell our users about the bug in some way:

• I don't want to silently change the behavior of get_checksum - if some users used it the same way we do, they need to implement some way of switching to the new checksums - or just keep using the old ones
• On the other hand... exposing a boolean exclude_hash is a bit confusing. All the new users, and some of the old ones, don't need to know what's exclude_hash is about.

I'm thinking that we might achieve something useful with deprecating old methods, but I'm really not sure... Any ideas?

[rajarshimaitra]

Thanks @notmandatory for carrying the work.. This looks almost complete and good to go..

tACK 60057a7

Below are few nits and testing suggestions..

[evanlinjin]

ACK 60057a7

@notmandatory Thank you for moving the PR forwards. Looks good to me.

[notmandatory]

ACK 648282e

[notmandatory]

I think this one is ready to merge, but I'm going to hold off until #770 is merged since I think it will be easier to rebase this PR on that one.