Introduce get_checksum_bytes method and improvements
#671
Conversation
evanlinjin
force-pushed
the
checksum_module_additions
branch
from
7555ff6
to
a96c7da
Compare
| @@ -43,15 +41,17 @@ fn poly_mod(mut c: u64, val: u64) -> u64 { | |||
| c | |||
| } | |||
|
|
|||
| /// 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)
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.
evanlinjin
force-pushed
the
checksum_module_additions
branch
from
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 #486 ).
Refer to comments by @afilini :
DatabaseFactorytrait #654 (comment)Checklists
All Submissions:
cargo fmtandcargo clippybefore committingNew Features:
CHANGELOG.md