fuzz: replace hardcoded numbers for bech32 limits

josibake commented at 9:20 am on August 6, 2024: member

Follow-up to #30047 to replace a hardcoded value that was missed in the original PR

Use bech32::CharLimit::BECH32 and bech32::CHECKSUM_SIZE instead of
hardcoded values. This is a follow-up fix for #34007
(where this file was missed)

59c0ece0a7

DrahtBot commented at 9:20 am on August 6, 2024: contributor

The following sections might be updated with supplementary metadata relevant to reviewers and maintainers.

Code Coverage

For detailed information about the code coverage, see the test coverage report.

Reviews

See the guideline for information on the review process.

Type	Reviewers
ACK	dergoegge, paplorinc, marcofleon, brunoerg

If your review is incorrectly listed, please react with 👎 to this comment and the bot will ignore it on the next update.

DrahtBot added the label Tests on Aug 6, 2024

dergoegge approved

dergoegge commented at 10:32 am on August 6, 2024: member

utACK 59c0ece0a785ce9e22fbfefce9ca228d85e5d43d

in src/test/fuzz/bech32.cpp:33 in 59c0ece0a7

28@@ -29,8 +29,9 @@ FUZZ_TARGET(bech32)
29     std::vector<unsigned char> input;
30     ConvertBits<8, 5, true>([&](unsigned char c) { input.push_back(c); }, buffer.begin(), buffer.end());
31 
32-    if (input.size() + 3 + 6 <= 90) {
33-        // If it's possible to encode input in Bech32(m) without exceeding the 90-character limit:
34+    // Input data part + 3 characters for the HRP and separator (bc1) + the checksum characters
35+    if (input.size() + 3 + bech32::CHECKSUM_SIZE <= bech32::CharLimit::BECH32) {

paplorinc commented at 11:15 am on August 6, 2024:

nit: According to https://en.bitcoin.it/wiki/BIP_0173 bc + 1 is human-readable part + separator, if we’re splitting these up, maybe this would be a better documentation:

0    const auto hrp_size = 2, separator_size = 1;
1    if (input.size() + hrp_size + separator_size + bech32::CHECKSUM_SIZE <= bech32::CharLimit::BECH32) {

josibake commented at 12:19 pm on August 6, 2024:

Per #30596 (review), it would be nice to not hardcode the HRP at all (or it’s size) but I think it should be a separate and larger change. For the separator, I don’t think it makes much difference if we use a variable or comment to mention what the value is for.

paplorinc commented at 12:21 pm on August 6, 2024:

I’m fine either way

in src/test/fuzz/bech32.cpp:36 in 59c0ece0a7

33-        // If it's possible to encode input in Bech32(m) without exceeding the 90-character limit:
34+    // Input data part + 3 characters for the HRP and separator (bc1) + the checksum characters
35+    if (input.size() + 3 + bech32::CHECKSUM_SIZE <= bech32::CharLimit::BECH32) {
36+        // If it's possible to encode input in Bech32(m) without exceeding the bech32-character limit:
37         for (auto encoding : {bech32::Encoding::BECH32, bech32::Encoding::BECH32M}) {
38             const std::string encoded = bech32::Encode(encoding, "bc", input);

paplorinc commented at 11:29 am on August 6, 2024:

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”

josibake commented at 12:16 pm on August 6, 2024:

Agree that fuzzing for more than just the hard-coded “bc” is ideal, but probably better as a separate follow-up.

paplorinc commented at 12:29 pm on August 7, 2024:

Are you working on this or should I do it?

josibake commented at 12:58 pm on August 7, 2024:

Up for grabs! Feel free to ping me for review.

paplorinc commented at 7:58 am on August 10, 2024:

Done in #30623 (comment)

in src/bech32.h:25 in 59c0ece0a7

20@@ -21,6 +21,9 @@
21 namespace bech32
22 {
23 
24+/** The Bech32 and Bech32m checksum size */
25+constexpr size_t CHECKSUM_SIZE = 6;

paplorinc commented at 11:31 am on August 6, 2024:

nit: would it make sense to move these to the header as well - and make them static constexpr as well?

 0/** The Bech32 and Bech32m checksum size */
 1static constexpr size_t CHECKSUM_SIZE = 6;
 2
 3/** The Bech32 and Bech32m character set for encoding. */
 4static constexpr const char* CHARSET = "qpzry9x8gf2tvdw0s3jn54khce6mua7l";
 5
 6/** The Bech32 and Bech32m character set for decoding. */
 7static constexpr const int8_t CHARSET_REV[128] = {
 8    -1, -1, -1, -1, -1, -1, -1, -1, -1, -1, -1, -1, -1, -1, -1, -1,
 9    -1, -1, -1, -1, -1, -1, -1, -1, -1, -1, -1, -1, -1, -1, -1, -1,
10    -1, -1, -1, -1, -1, -1, -1, -1, -1, -1, -1, -1, -1, -1, -1, -1,
11    15, -1, 10, 17, 21, 20, 26, 30,  7,  5, -1, -1, -1, -1, -1, -1,
12    -1, 29, -1, 24, 13, 25,  9,  8, 23, -1, 18, 22, 31, 27, 19, -1,
13    1,  0,  3, 16, 11, 28, 12, 14,  6,  4,  2, -1, -1, -1, -1, -1,
14    -1, 29, -1, 24, 13, 25,  9,  8, 23, -1, 18, 22, 31, 27, 19, -1,
15    1,  0,  3, 16, 11, 28, 12, 14,  6,  4,  2, -1, -1, -1, -1, -1
16};

josibake commented at 12:52 pm on August 6, 2024:

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?

paplorinc commented at 1:15 pm on August 6, 2024:

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.

sipa commented at 2:03 pm on August 6, 2024:

@josibake The keyword static in C++ has two completely different meanings sadly :angry:

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).

josibake commented at 2:46 pm on August 6, 2024:

@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.

paplorinc commented at 7:59 am on August 10, 2024:

Should I add this to #30623 (comment) as well?

paplorinc commented at 11:33 am on August 6, 2024: contributor

Thanks, please see if my comments would fit into the scope of this change

paplorinc commented at 1:16 pm on August 6, 2024: contributor

ACK 59c0ece0a785ce9e22fbfefce9ca228d85e5d43d

marcofleon commented at 1:27 pm on August 6, 2024: contributor

ACK 59c0ece0a785ce9e22fbfefce9ca228d85e5d43d. Ran the test a bit to be sure, lgtm.

brunoerg approved

brunoerg commented at 1:56 pm on August 6, 2024: contributor

utACK 59c0ece0a785ce9e22fbfefce9ca228d85e5d43d

glozow merged this on Aug 6, 2024

glozow closed this on Aug 6, 2024

Theschorpioen approved

fuzz: replace hardcoded numbers for bech32 limits #30596

Code Coverage

Reviews