bdk_wallet: Don't reimplement descriptor checksum, use the one from rust-miniscript#1523
Merged
evanlinjin merged 3 commits intobitcoindevkit:masterfrom Aug 5, 2024
Merged
Conversation
darosior
force-pushed
the
2407_desc_checksum
branch
2 times, most recently
from
24b935e to
413b3ce
Compare
Contributor
Author
|
CI failure looks unrelated to this PR:
EDIT: fixed in #1524. |
Member
|
Thanks for this! we'll get #1524 merged soon so you can rebase. |
It can just be a String.
This code was: 1. Removing the checksum from the descriptor string, asserting it was present 2. Re-calculating the checksum on the descriptor string absent the checksum Instead, just use the existing checksum? If we don't trust it we probably shouldn't assert its presence in the first place.
darosior
force-pushed
the
2407_desc_checksum
branch
from
413b3ce to
70c7d6b
Compare
Contributor
Author
|
Rebased on master after the merge of #1524 to fix CI. Looks like CI is failing on master though. Maybe a flaky unit test? |
storopoli
approved these changes
Jul 27, 2024
evanlinjin
reviewed
Aug 5, 2024
| // 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; |
Member
There was a problem hiding this comment.
Who is the original gansta we are referring to here? 😆
Contributor
Author
There was a problem hiding this comment.
Haha, i meant "original" of course. :)
This file contains hidden or bidirectional Unicode text that may be interpreted or compiled differently than what appears below. To review, open the file in an editor that reveals hidden Unicode characters.
Learn more about bidirectional Unicode characters
Sign up for free
to join this conversation on GitHub.
Already have an account?
Sign in to comment
4 participants
Add this suggestion to a batch that can be applied as a single commit.This suggestion is invalid because no changes were made to the code.Suggestions cannot be applied while the pull request is closed.Suggestions cannot be applied while viewing a subset of changes.Only one suggestion per line can be applied in a batch.Add this suggestion to a batch that can be applied as a single commit.Applying suggestions on deleted lines is not supported.You must change the existing code in this line in order to create a valid suggestion.Outdated suggestions cannot be applied.This suggestion has been applied or marked resolved.Suggestions cannot be applied from pending reviews.Suggestions cannot be applied on multi-line comments.Suggestions cannot be applied while the pull request is queued to merge.Suggestion cannot be applied right now. Please check back later.
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 tocalc_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!