fuzz: replace hardcoded numbers for bech32 limits #30596
Conversation
Use bech32::CharLimit::BECH32 and bech32::CHECKSUM_SIZE instead of hardcoded values. This is a follow-up fix for bitcoin#34007 (where this file was missed)
|
The following sections might be updated with supplementary metadata relevant to reviewers and maintainers. Code CoverageFor detailed information about the code coverage, see the test coverage report. ReviewsSee the guideline for information on the review process.
If your review is incorrectly listed, please react with 👎 to this comment and the bot will ignore it on the next update. |
l0rinc
left a comment
l0rinc
left a comment
There was a problem hiding this comment.
Thanks, please see if my comments would fit into the scope of this change
| if (input.size() + 3 + bech32::CHECKSUM_SIZE <= bech32::CharLimit::BECH32) { | ||
| // If it's possible to encode input in Bech32(m) without exceeding the bech32-character limit: | ||
| for (auto encoding : {bech32::Encoding::BECH32, bech32::Encoding::BECH32M}) { | ||
| const std::string encoded = bech32::Encode(encoding, "bc", input); |
There was a problem hiding this comment.
according to the bip the hrp MUST contain 1 to 83 US-ASCII characters - maybe we could extend the fuzz testing from the hard-coded "bc"
There was a problem hiding this comment.
Agree that fuzzing for more than just the hard-coded "bc" is ideal, but probably better as a separate follow-up.
There was a problem hiding this comment.
Are you working on this or should I do it?
There was a problem hiding this comment.
Up for grabs! Feel free to ping me for review.
| /** The Bech32 and Bech32m checksum size */ | ||
| constexpr size_t CHECKSUM_SIZE = 6; |
There was a problem hiding this comment.
nit: would it make sense to move these to the header as well - and make them static constexpr as well?
| /** The Bech32 and Bech32m checksum size */ | |
| constexpr size_t CHECKSUM_SIZE = 6; | |
| /** The Bech32 and Bech32m checksum size */ | |
| static constexpr size_t CHECKSUM_SIZE = 6; | |
| /** The Bech32 and Bech32m character set for encoding. */ | |
| static constexpr const char* CHARSET = "qpzry9x8gf2tvdw0s3jn54khce6mua7l"; | |
| /** The Bech32 and Bech32m character set for decoding. */ | |
| static constexpr const int8_t CHARSET_REV[128] = { | |
| -1, -1, -1, -1, -1, -1, -1, -1, -1, -1, -1, -1, -1, -1, -1, -1, | |
| -1, -1, -1, -1, -1, -1, -1, -1, -1, -1, -1, -1, -1, -1, -1, -1, | |
| -1, -1, -1, -1, -1, -1, -1, -1, -1, -1, -1, -1, -1, -1, -1, -1, | |
| 15, -1, 10, 17, 21, 20, 26, 30, 7, 5, -1, -1, -1, -1, -1, -1, | |
| -1, 29, -1, 24, 13, 25, 9, 8, 23, -1, 18, 22, 31, 27, 19, -1, | |
| 1, 0, 3, 16, 11, 28, 12, 14, 6, 4, 2, -1, -1, -1, -1, -1, | |
| -1, 29, -1, 24, 13, 25, 9, 8, 23, -1, 18, 22, 31, 27, 19, -1, | |
| 1, 0, 3, 16, 11, 28, 12, 14, 6, 4, 2, -1, -1, -1, -1, -1 | |
| }; |
There was a problem hiding this comment.
IIUC, I don't think static makes a difference here since we aren't talking about a class member? Regarding CHARSET and CHARSET_REV, what's the advantage of having them in the header file if we aren't using them outside of bech32.cpp?
There was a problem hiding this comment.
I don't think static makes a difference here
Based on other usages in the code, static constexpr in headers is good practice to prevent potential linking issues and ensure each translation unit has its own independent copy. Let me know if I'm wrong here.
what's the advantage of having them in the header file
Separating constants and definitions into the header, leaving implementations in the cpp, i.e. separation of concerns.
If you think this is outside the scope, I can accept that, thanks for considering my suggestions.
There was a problem hiding this comment.
@josibake The keyword static in C++ has two completely different meanings sadly 😠
Inside a class, a static variable/function makes it a class member rather than an instance member (shared by all instances).
Outside a class, static means that a variable/function is only accessible within its compilation unit (whose effect is identical to placing it in an anonymous namespace).
There was a problem hiding this comment.
@sipa , @paplorinc thanks for the explanation! In this case, since we are in a named namespace, it seems the static keyword would be preferable? Or doesn't matter? Will do some more reading on this cause I'm not sure what the best practice is here.
Separating constants and definitions into the header, leaving implementations in the cpp, i.e. separation of concerns.
This is another one I'm not sure about: in my mind, CHARSET is internal to bech32 hence why it makes sense to keep it in bech32.cpp. But I suppose you could also argue the char set is a definition (from BIP173), so it should be in the header. Either way, I think it's out of scope for this PR, but thought it was an interesting point.
|
ACK 59c0ece |
marcofleon
left a comment
marcofleon
left a comment
There was a problem hiding this comment.
ACK 59c0ece. Ran the test a bit to be sure, lgtm.
9 participants
Follow-up to #30047 to replace a hardcoded value that was missed in the original PR