Introduce get_checksum_bytes method and improvements#671
Introduce get_checksum_bytes method and improvements#671afilini merged 1 commit intobitcoindevkit:masterfrom
get_checksum_bytes method and improvements#671Conversation
7555ff6 to
a96c7da
Compare
| } | ||
|
|
||
| /// Compute the checksum of a descriptor | ||
| pub fn get_checksum(desc: &str) -> Result<String, DescriptorError> { |
There was a problem hiding this comment.
I don't know if anyone is using get_checksum() but since it's public our policy is to deprecate it for at least one release before completely removing it. We haven't been super strict about it, but we do need to be more consistent. I think you could make a simple function to call your new get_checksum_bytes and convert to a String.
There was a problem hiding this comment.
@notmandatory I haven't removed it at all! And yes, get_checksum calls get_checksum_bytes 😅 (line 79)
afilini
left a comment
There was a problem hiding this comment.
Concept ACK, just a small nit
src/descriptor/checksum.rs
Outdated
| const INPUT_CHARSET: &[u8] = "0123456789()[],'/*abcdefgh@:$%{}IJKLMNOPQRSTUVWXYZ&+-.;<=>?!^_|~ijklmnopqrstuvwxyzABCDEFGH`#\"\\ ".as_bytes(); | ||
| const CHECKSUM_CHARSET: &[u8] = "qpzry9x8gf2tvdw0s3jn54khce6mua7l".as_bytes(); |
There was a problem hiding this comment.
On both those lines I think you can use byte string literals: https://doc.rust-lang.org/reference/tokens.html#byte-string-literals
It should make the code more portable as it works on older rust versions as well (as_bytes() is a const fn only since 1.39) and it will also cause a compile error if we accidentally put a non-ascii character in there: https://play.rust-lang.org/?version=stable&mode=debug&edition=2021&gist=81fadd81957dbc1c56844a279a9d239f
There was a problem hiding this comment.
@afilini I just updated this! Hopefully the tests pass.
`get_checksum_bytes` returns a descriptor checksum as `[u8; 8]` instead of `String`, potentially improving performance and memory usage. In addition to this, since descriptors only use charaters that fit within a UTF-8 8-bit code unit, there is no need to use the `char` type (which is 4 bytes). This can also potentially bring in some performance and memory-usage benefits.
a96c7da to
6db5b4a
Compare
|
@afilini @notmandatory This is ready to go imo. 😃 |
Description
get_checksum_bytes()returns a descriptor checksum as[u8; 8]instead ofString, potentially improving performance and memory usage.In addition to this, since descriptors only use characters that fit within a UTF-8 8-bit code unit (US-ASCII), there is no need to use the
chartype (which is 4 bytes). This can also potentially bring in some performance and memory-usage benefits.Notes to the reviewers
This is useful because we will be using descriptor checksums for indexing operations in the near future (multi-descriptor wallets bitcoindevkit/bdk_wallet#188 ).
Refer to comments by @afilini :
DatabaseFactorytrait #654 (comment)Checklists
All Submissions:
cargo fmtandcargo clippybefore committingNew Features:
CHANGELOG.md