This is a follow-up to #30746, expanding coverage by:
100 instead of 1000 or all available input (suggested by marcofleon in #30746 (review)) since most actual usage has lengths of e.g. 21, 34, 78.
The following sections might be updated with supplementary metadata relevant to reviewers and maintainers.
For details see: https://corecheck.dev/bitcoin/bitcoin/pulls/31917.
See 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.
50 {
51- FuzzedDataProvider provider(buffer.data(), buffer.size());
52- const std::string random_string{provider.ConsumeRandomLengthString(1000)};
53- const int max_ret_len{provider.ConsumeIntegralInRange<int>(-1, 1000)};
54+ // Encode/Decode roundtrip
55+ const auto encoded{EncodeBase58Check(buffer)};
The problem actually turned out to be this. The input that times out is about 1MB so passing in the whole buffer causes the timeout.
I don’t think the change from 1000 to 100 for both random_string and max_ret_len makes a difference.
Concept ACK a63c5c1a8a012b694
I’ve reviewed the changes and have one main observation:
Limiting fuzz to the most common sizes makes sense for efficiency. However, if long strings are still expected to be supported, there should be explicit test coverage for them.
I checked the unit tests for Base58 in src/test/base58_tests.cpp, which reads test cases from test/data/base58_encode_decode.json. The longest existing test case appears on line 18, with a hex-encoded string of size 512. Given that this PR removes the long string test from the fuzz tests, it may be beneficial to add a new test case in base58_encode_decode.json to verify behaviour at 1000 characters.
After static review, I ran the fuzz tests using:
0$ cmake --preset=libfuzzer
1$ cmake --build build_fuzz -j "$(($(nproc)+1))"
2
3$ FUZZ=base58_encode_decode build_fuzz/src/test/fuzz/fuzz -max_total_time=30 -
4$ FUZZ=base58check_encode_decode build_fuzz/src/test/fuzz/fuzz -max_total_time=30
5$ FUZZ=base32_encode_decode build_fuzz/src/test/fuzz/fuzz -max_total_time=30
6$ FUZZ=base64_encode_decode build_fuzz/src/test/fuzz/fuzz -max_total_time=30
7$ FUZZ=psbt_base64_decode build_fuzz/src/test/fuzz/fuzz -max_total_time=30
All tests passed successfully within the 30-second run.
76
77 FUZZ_TARGET(base32_encode_decode)
78 {
79- const std::string random_string{buffer.begin(), buffer.end()};
80+ FuzzedDataProvider provider{buffer.data(), buffer.size()};
81+ const auto random_string{provider.ConsumeRandomLengthString(100)};
to not use all the buffer, where we had problems before.
What problems? A problem was introduced in the base58 fuzz target in the previous change, however you are changing the base32/64 here and below.
Generally, if there is no reason to change the code, I think it shouldn’t be changed. Otherwise, it will just be confusing, hard to review, and may even introduce problems.
What problems? A problem was introduced in the base58 fuzz target in the previous change, however you are changing the base32/64 here and below.
I’ve learned that using the whole buffer can be problematic for long running tasks, and since base conversion is mostly scale dependent anyway, we’d get better coverage of special values (e.g. all zeros, powers of the base, etc), by using a value that isn’t too large.
there is no reason to change the code
This PR is meant to unify the related base fuzz tests. I’d argue it’s easier to review them if they all follow the same steps. This is a follow-up to a previous PR, we’re not modifying legacy code here.
95
96 FUZZ_TARGET(base64_encode_decode)
97 {
98- const std::string random_string{buffer.begin(), buffer.end()};
99+ FuzzedDataProvider provider{buffer.data(), buffer.size()};
100+ const auto random_string{provider.ConsumeRandomLengthString(100)};
65@@ -67,9 +66,9 @@ FUZZ_TARGET(base32_encode_decode)
66 assert(encoded_string == ToLower(TrimStringView(random_string)));
67 }
68 // Encode/Decode roundtrip
69- const auto encoded{EncodeBase32(buffer)};
70+ const auto encoded{EncodeBase32(random_string)};
32- if (DecodeBase58(random_string, decoded, max_ret_len)) {
33+ if (std::vector<unsigned char> decoded; DecodeBase58(random_string, decoded, max_ret_len)) {
34 const auto encoded_string{EncodeBase58(decoded)};
35 assert(encoded_string == TrimStringView(random_string));
36- assert(encoded_string.empty() || !DecodeBase58(encoded_string, decoded, provider.ConsumeIntegralInRange<int>(0, decoded.size() - 1)));
37+ assert(decoded.empty() || !DecodeBase58(encoded_string, decoded, provider.ConsumeIntegralInRange<int>(0, decoded.size() - 1)));
encoded_string.empty() to decoded.empty() in the first commit - especially if this function is being rewritten in the next commit anyway?
(0, decoded.size() - 1) range makes sense or not - I’m correcting it in the first commit since this was a bug that could cause a fuzz failure (added explanation to commit message).
I’m reordering in the second commit so that we have single roundtrip logic instead of 2 separate ones.
decoded an encoded_string are empty. However, it should be impossible for encoded_string to be empty and decoded non-empty (and vice versa) because of how base58 works.
They seem to cause timeouts:
> Issue 397734700: bitcoin-core:base58check_encode_decode: Timeout in base58check_encode_decode
The `encoded_string.empty()` check was corrected here to `decoded.empty()` to make sure the `(0, decoded.size() - 1)` range is always valid.
Co-authored-by: maflcko <6399679+maflcko@users.noreply.github.com>
Co-authored-by: marcofleon <marleo23@proton.me>
Co-authored-by: Martin Zumsande <mzumsande@gmail.com>
In Base58 fuzz the two roundtrips are merged now, the new `decode_input` switches between a completely random input and a valid encoded one, to make sure the decoding passes more often.
The `max_ret_len` can also exceed the original length now and is being validated more thoroughly.
Co-authored-by: maflcko <6399679+maflcko@users.noreply.github.com>
Co-authored-by: marcofleon <marleo23@proton.me>
review ACK d5537c18a9034647ba4c9ed4008abd7fee33989e 🚛
Signature:
0untrusted comment: signature from minisign secret key on empty file; verify via: minisign -Vm "${path_to_any_empty_file}" -P RWTRmVTMeKV5noAMqVlsMugDDCyyTSbA3Re5AkUrhvLVln0tSaFWglOw -x "${path_to_this_whole_four_line_signature_blob}"
1RUTRmVTMeKV5npGrKx1nqXCw5zeVHdtdYURB/KlyA/LMFgpNCs+SkW9a8N95d+U4AP1RJMi+krxU1A3Yux4bpwZNLvVBKy0wLgM=
2trusted comment: review ACK d5537c18a9034647ba4c9ed4008abd7fee33989e 🚛
3lZRbdgnghqtziDGBOrJTlPhuDIsZdigxD/VS2S8f5+dWrvPvXpUWSzcRe/nWCWjUTD2bz/MArCoV2/DrCVmxDA==
