base32/base58/base64 encoding to make sure encoding/decoding are symmetric.
Note that if we omit the padding in EncodeBase32 we won’t be able to decode it with DecodeBase32.
Added dedicated padding tests to cover failure behavior
Also moved over the Base58 json test edge cases from https://github.com/bitcoin/bitcoin/pull/30035
The following sections might be updated with supplementary metadata relevant to reviewers and maintainers.
For details see: https://corecheck.dev/bitcoin/bitcoin/pulls/30746.
See the guideline for information on the review process.
| Type | Reviewers |
|---|---|
| ACK | hodlinator |
| Stale ACK | marcofleon |
If your review is incorrectly listed, please react with 👎 to this comment and the bot will ignore it on the next update.
56+ BOOST_CHECK( DecodeBase32("aaaaa==="s));
57+ BOOST_CHECK(!DecodeBase32("aaaaaa=="s));
58+ BOOST_CHECK( DecodeBase32("aaaaaaa="s));
59+}
60+
61+BOOST_AUTO_TEST_CASE(base32_roundtrip) {
62+ BOOST_CHECK(!DecodeBase64("a==="s));
63+ BOOST_CHECK( DecodeBase64("YQ=="s));
64+ BOOST_CHECK( DecodeBase64("YWE="s));
65+}
66+
67+BOOST_AUTO_TEST_CASE(base64_roundtrip) {
@maflcko, the Fuzz tests seem to be timing out now, is the new setup too heavy, should I maybe add them to different suites?
Did you run them locally, and can point out which commit makes them time out? Is there a flame graph, or some other kind of benchmark or performance debugging that would help tack this down?
Absent that, it seems odd that the fuzz tests would time out here.
it seems odd that the fuzz tests would time out here
Split the big fuzz target into separate base58_encode_decode / base58check_encode_decode / base32_encode_decode / base64_encode_decode / psbt_base64_decode - it’s cleaner like this anyway.
Edit: seems this fixed the timeout
79+ if (auto result{DecodeBase32(random_string)}) {
80+ auto encoded_string{EncodeBase32(*result)};
81+ assert(encoded_string == ToLower(TrimString(random_string)));
82 }
83+ // Encode/Decode roundtrip
84+ auto encoded{EncodeBase32(random_data)};
Reusing the same data from the fuzz input for two different things (e.g. decode/encode and encode/decode) is usually not a good idea.
How about adding a decode step after the encoding above (i.e. decode -> encode -> decode)? Or alternatively split this second round-trip into its own harness as well?
Thanks for the review!
is usually not a good idea
Can you please expand on that? I don’t mind splitting, as you’ve recommended, but I have to understand what the current drawbacks are.
There is also an overhead that we probably don’t want a separate fuzz target for every trivial function, because each fuzz target may be put into a separate binary and is allocated a separate folder. So in very trivial cases, I think it is fine to combine several trivial function calls into one fuzz target.
Encode/Decode of a base seems to be trivial enough. I think the previous CI timeout may be unrelated #30746 (comment) , because there are general CI issues right now. Generally, it is best to not rely on CI and separately double-check and confirm any result locally.
If you want to combine the test cases, but not want to always exclude both test branches, the CallOneOf helper can be used:
0 CallOneOf(
1 fuzzed_data_provider,
2 [&] {
3 Branch1(fuzzed_data_provider);
4 },
5 [&] {
6 Branch2(fuzzed_data_provider);
7 });
There is also an overhead that we probably don’t want a separate fuzz target for every trivial function, because each fuzz target may be put into a separate binary and is allocated a separate folder.
I don’t think the binary sizes would be a problem after we merge something like #30882 and the extra folder for the corpus is also not a huge cost (they compress really nicely).
I’d prefer to follow best practices over having to weigh triviality against some external overhead of running these tests. “Put separate tests into separate harnesses” is easier to follow than “aggregate trivial functions in one harness” because it is entirely subjective. What is trivial? How many unrelated trivial functions are allowed in one harness? I think this creates confusion for devs that aren’t experts on fuzzing.
would a CallOneOf based solution satisfy your needs?
I don’t think this solves the issue, e.g. mutations that change the calloneof branch result in completely changing the meaning of the fuzz input (i.e. switching between the different tests).
I don’t mean to derail this PR, it’s already improving things by splitting up base_encode_decode. Feel free to leave it as is.
I don’t think the binary sizes would be a problem after we merge something like #30882 and the extra folder for the corpus is also not a huge cost (they compress really nicely).
Are you sure? IIUC 30882 creates a separate binary for each fuzz target, so depending on the compile/link options (static, without lto, …), each fuzz target may or may not be heavy. For example, the OSS-Fuzzing already went down due to storage limitations (https://github.com/google/oss-fuzz/pull/12232). I am not saying that OSS-Fuzz should be considered a blocker, given that there are now several in-house backups, but I wanted to mention it for context.
I’d prefer to follow best practices over having to weigh triviality against some external overhead of running these tests. “Put separate tests into separate harnesses” is easier to follow than “aggregate trivial functions in one harness” because it is entirely subjective. What is trivial? How many unrelated trivial functions are allowed in one harness? I think this creates confusion for devs that aren’t experts on fuzzing.
I think I agree that it would be ideal to have an easy/trivial guideline to follow. However, I still think there is some threshold where a separate fuzz target (or maybe even a fuzz target at all) may not be the best choice. There are many pure util functions that (in theory) could go into a separate fuzz target. For example, StringForFeeEstimateHorizon https://github.com/bitcoin/bitcoin/blob/e43ce250c6fb65cc0c8903296c8ab228539d2204/src/test/fuzz/kitchen_sink.cpp#L45 should (in theory) be moved to a separate target. However, it is such a trivial function (a single switch-case) that linking the binary and starting it takes longer than reaching full coverage from an empty initial seed corpus. Maybe the function is too trivial to be put into a fuzz target at all, but I think there are plenty other pure util functions, such that there could easily be 500-1500 fuzz targets with similar properties. Of course there is nothing inherently wrong with having so many fuzz targets, but I could imagine that some tooling (introspector, coverage reports per fuzz target) or humans consuming the tool’s output could be overwhelmed by the number of trivial fuzz targets and miss the actually important ones.
Thanks for the directions @dergoegge, @maflcko.
I don’t mean to derail this PR, it’s already improving things by splitting up base_encode_decode. Feel free to leave it as is.
I’m all for separation of concerns, that’s what I’ve done here as well. If there’s a simple and non-controvertial way to make it even better, let me know!
(My instinct would be to go with the following to avoid rocking the boat when it comes to seed corpus naming etc, but I’m too new to this aspect of fuzzing to carry much weight).
0void base58_encode_decode(FuzzBufferType buffer);
1void base58check_encode_decode(FuzzBufferType buffer);
2void base32_encode_decode(FuzzBufferType buffer);
3void base64_encode_decode(FuzzBufferType buffer);
4void psbt_base64_decode(FuzzBufferType buffer);
5
6FUZZ_TARGET(base_encode_decode)
7{
8 base58_encode_decode(buffer);
9 base58check_encode_decode(buffer);
10 base32_encode_decode(buffer);
11 base64_encode_decode(buffer);
12 psbt_base64_decode(buffer);
13}
30- const std::string encoded_string = EncodeBase58(decoded);
31- assert(encoded_string == TrimStringView(encoded_string));
32- assert(ToLower(encoded_string) == ToLower(TrimString(random_encoded_string)));
33+ if (DecodeBase58(random_string, decoded, 100)) {
34+ auto encoded_string{EncodeBase58(decoded)};
35+ assert(encoded_string == TrimString(random_string));
Should keep using TrimStringView here to avoid allocating extra std::string instances. Fuzzing code is re-run at high frequency so optimization (without compromising code coverage) is important.
0 assert(encoded_string == TrimStringView(random_string));
20-FUZZ_TARGET(base_encode_decode)
21+FUZZ_TARGET(base58_encode_decode)
22 {
23- const std::string random_encoded_string(buffer.begin(), buffer.end());
24+ std::string random_string(buffer.begin(), buffer.end());
25+ std::vector<unsigned char> random_data{ToByteVector(random_string)};
I understand the change to use random_string as something that might either be encoded already, or as something to be encoded in the later half of these functions, so the rename makes sense. Would avoid stripping const for these slightly longer lived variables, just for peace of mind.
0 const std::string random_string{buffer.begin(), buffer.end()};
1 const std::vector<unsigned char> random_data{ToByteVector(random_string)};
Another aspect is that buffer has the type using FuzzBufferType = std::span<const uint8_t> already, so it could potentially be used directly instead of introducing and allocating random_data. (Not super happy about the memcmp though).
0 FUZZ_TARGET(base58_encode_decode)
1 {
2- std::string random_string(buffer.begin(), buffer.end());
3- std::vector<unsigned char> random_data{ToByteVector(random_string)};
4+ const std::string random_string{buffer.begin(), buffer.end()};
5
6 // Decode/Encode roundtrip
7 std::vector<unsigned char> decoded;
8@@ -27,11 +28,12 @@ FUZZ_TARGET(base58_encode_decode)
9 assert(encoded_string == TrimString(random_string));
10 assert(encoded_string.empty() || !DecodeBase58(encoded_string, decoded, decoded.size() - 1));
11 }
12
13 // Encode/Decode roundtrip
14- auto encoded{EncodeBase58(random_data)};
15+ auto encoded{EncodeBase58(buffer)};
16 std::vector<unsigned char> roundtrip_decoded;
17 assert(DecodeBase58(encoded, roundtrip_decoded, random_string.size())
18- && roundtrip_decoded == random_data);
19+ && !std::memcmp(roundtrip_decoded.data(), buffer.data(), buffer.size()));
20 }
Would avoid stripping const for these slightly longer lived variables, just for peace of mind.
Done, thanks.
Not super happy about the memcmp though
agree, memcmp is ugly, but std::ranges::equal should work here.
112+ std::string random_string(buffer.begin(), buffer.end());
113
114 PartiallySignedTransaction psbt;
115 std::string error;
116- (void)DecodeBase64PSBT(psbt, random_encoded_string, error);
117+ (void) DecodeBase64PSBT(psbt, random_string, error);
Why add the space?
Could also verify error is returned correctly.
0 assert(DecodeBase64PSBT(psbt, random_string, error) == error.empty());
27+ // Decode/Encode roundtrip
28 std::vector<unsigned char> decoded;
29- if (DecodeBase58(random_encoded_string, decoded, 100)) {
30- const std::string encoded_string = EncodeBase58(decoded);
31- assert(encoded_string == TrimStringView(encoded_string));
32- assert(ToLower(encoded_string) == ToLower(TrimString(random_encoded_string)));
31- assert(encoded_string == TrimStringView(encoded_string));
32- assert(ToLower(encoded_string) == ToLower(TrimString(random_encoded_string)));
33+ if (DecodeBase58(random_string, decoded, 100)) {
34+ auto encoded_string{EncodeBase58(decoded)};
35+ assert(encoded_string == TrimString(random_string));
36+ assert(encoded_string.empty() || !DecodeBase58(encoded_string, decoded, decoded.size() - 1));
Could test slightly more aggressively in line with the removed test: https://github.com/bitcoin/bitcoin/blob/a5b4e42a575b4680a9eedb6ab2275d2a723cc20d/src/test/base58_tests.cpp#L93-L94
0 assert(encoded_string.empty() || !DecodeBase58(encoded_string, decoded, fuzzed_data_provider.ConsumeIntegralInRange(0, decoded.size() - 1)));
FuzzedDataProvider anyway
Concept ACK ad917fb105b68fb369455ec7c42292a7673db1ad
base58_random_encode_decode unit test, but everything seems to be carried over to the modified fuzz test (except for testing for a variable count of insufficient result buffer sizes).22-FUZZ_TARGET(base_encode_decode)
23+FUZZ_TARGET(base58_encode_decode)
24 {
25- const std::string random_encoded_string(buffer.begin(), buffer.end());
26+ FuzzedDataProvider provider(buffer.data(), buffer.size());
27+ const std::string random_string{provider.ConsumeRemainingBytesAsString()};
I’m afraid FuzzedDataProvider::remaining_bytes_ becomes zero after this line in the current version, meaning provider.ConsumeIntegralInRange<int>(0, decoded.size() - 1) below will always return the minimum value if I’m reading the ConsumeIntegralInRange-implementation correctly.
0 const std::string random_string{provider.ConsumeRandomLengthString(101)};
Could maybe (in another PR) add something like bool FuzzedDataProvider::consume_remaining_called_{false} which is set by ConsumeRemainingBytes*() and then assert against it in the various consume methods, to catch cases like these.
Could maybe (in another PR)
In theory it could also be caught at compile time with https://clang.llvm.org/docs/AttributeReference.html#consumed-annotation-checking
FuzzedDataProvider to trigger asserts when calls like ConsumeIntegralInRange<int>(0, decoded.size() - 1) are made after calls to ConsumeRemainingBytes*()? Maybe such an improvement could be upstreamed even (possibly with a flag to turn it off for sloppy tests).
agree on preprocessor logic
Maybe I am missing something, but it should be pretty standard, similar to
https://github.com/bitcoin/bitcoin/blob/f07a533dfcb172321972e5afb3b38a4bd24edb87/src/span.h#L22-L30
?
FuzzedDataProvider impl stays in sync.
I tried to implement this, but (compile-time) annotations are either useless, because they are not applied globally, or they are too verbose for this, because they need to be applied globally/virally.
I think doing the check at runtime with a bool is probably good enough. Though that requires modifying the header file in some way or another.
An alternative could be to forbid the direct use of the depleting member functions (with a linter) and add a new helper which eats a FuzzedDataProvider&& and returns the bytes in it. This way, the existing use-after-move checks can be used.
Great that you made time to look into this!
An alternative could be to forbid the direct use of the depleting member functions (with a linter) and add a new helper which eats a
FuzzedDataProvider&&and returns the bytes in it. This way, the existing use-after-move checks can be used.
So you mean disallowing direct calls to ConsumeRemainingBytes*-family of methods only, and provide a helper that does only that part instead after swallowing up FuzzedDataProvider&& so it should no longer be used. Sounds like it’s worth a try to me!
git grep in the linter, similar to https://github.com/bitcoin/bitcoin/blob/dbc8ba12f3b3548dd6955293c5d650320ca39c5b/test/lint/test_runner/src/main.rs#L270
ACK 6fd185c035c1cc4dd961cf14a2087e97fb069440
range-diff shows all notable changes since my Concept ACK are in response to my suggestions.
Ran each new fuzz target with:
0₿ FUZZ=<target name> build_fuzz/src/test/fuzz/fuzz -max_total_time=30 -jobs=8
Also ran:
0₿ ctest -j<X> --test-dir build --output-on-failure -R base
Code review ACK 6fd185c035c1cc4dd961cf14a2087e97fb069440
Nice improvements to the fuzz test. The separation into different targets for each encoding looks good to me. I guess in principle, each roundtrip should be separated as well. But seems fine as is for these simple targets.
re-ACK 6fd185c035c1cc4dd961cf14a2087e97fb069440
(Hoping Drahtbot will react properly to this comment.. already ACKed the same PR version above).
42- BOOST_CHECK(!DecodeBase64("nQB/pZw=\0invalid"s));
43- BOOST_CHECK(!DecodeBase64("nQB/pZw=invalid\0"s));
44+ BOOST_CHECK(!DecodeBase64("invalid\0"s)); // correct size, invalid due to \0
45+ BOOST_CHECK( DecodeBase64("nQB/pZw="s)); // valid
46+ BOOST_CHECK(!DecodeBase64("nQB/pZw=\0invalid"s)); // correct size, invalid due to \0
47+ BOOST_CHECK(!DecodeBase64("nQB/pZw=invalid\0"s)); // invalid size
= and \0.
=, not the \0 - updated them
They are invalid, but still worth testing regardless of why precisely the current implementation errors out?
0BOOST_CHECK(!DecodeBase64("nQB/pZw=\0invalid"); // padding and NUL followed by invalid data
1BOOST_CHECK(!DecodeBase64("nQB/pZw=invalid\0"); // padding followed by invalid data and NUL
\0 and non-trailing =, what else do these ones add?
What’s the advantage of the comment stating exactly what the code already states?
Valid point. Better to describe what is wrong according to the Base64 standard (+ \0 I guess).
We’re already testing
\0and non-trailing=, what else do these ones add?
They cover slightly different cases, mixing of \0 and =. They may not test a unique error-handling path with the current implementation, but if they can uncover issues with a potential future implementation I think it was a mistake to remove them (@maflcko was just stating the comments were invalid as I understand it).
(@maflcko was just stating the comments were invalid as I understand it).
Yes, it seemed odd to touch a line that doesn’t need to be touched only to add a comment that is wrong. It seems better to not touch the line in this case.
41+{
42+ // Is valid without padding
43+ BOOST_CHECK_EQUAL(EncodeBase32("fooba"), "mzxw6ytb");
44+
45+ // Valid size
46+ BOOST_CHECK(!DecodeBase32("========"s));
nit: Could use ""sv or simply "" to avoid string allocations here and in base64_padding.
0 BOOST_CHECK(!DecodeBase32("========"));
"sv only for the values with \0 in them
41- BOOST_CHECK(DecodeBase64("nQB/pZw="s));
42- BOOST_CHECK(!DecodeBase64("nQB/pZw=\0invalid"s));
43- BOOST_CHECK(!DecodeBase64("nQB/pZw=invalid\0"s));
44+ BOOST_CHECK(!DecodeBase64("invalid\0"s)); // correct size, invalid due to \0
45+ BOOST_CHECK( DecodeBase64("nQB/pZw="s)); // valid
46+ BOOST_CHECK(!DecodeBase64("nQB/pZw=invalid"s)); // invalid, padding only allowed at the end
Last line no longer has an embedded NUL character as specified in the comment above this code block (// Decoding strings with embedded NUL characters should fail). Might belong more in base64_padding or at least in its own newline-separated block here:
0 BOOST_CHECK(!DecodeBase64("invalid\0"s)); // correct size, invalid due to \0
1 BOOST_CHECK( DecodeBase64("nQB/pZw="s)); // valid
2
3 BOOST_CHECK(!DecodeBase64("nQB/pZw=invalid"s)); // invalid, padding only allowed at the end
\0 above the comment in 32 and 64 tests, thanks!
Thanks @maflcko & @hodlinator for the comments, I’ve rebased the PR in the first push without any changes and addresses your concerns in the second one - let me know if there’s anything left:
\0 tests from length validation tests\0 with "sv, kept the rest as normal strings=\0 case
re-ACK 3b66b7e20ddba94d251c97818700d46030b14cc5
23+FUZZ_TARGET(base58_encode_decode)
24 {
25- const std::string random_encoded_string(buffer.begin(), buffer.end());
26+ FuzzedDataProvider provider(buffer.data(), buffer.size());
27+ const std::string random_string{provider.ConsumeRandomLengthString(1000)};
28+ const auto max_ret_len{provider.ConsumeIntegralInRange<int>(1, 300)};
nit: As random_string is currently 0-1000 long, and max_ret_len is 1-300, most of the time DecodeBase58() is just returning false since max_ret_len is too small. But maybe that is intentional to stress that aspect?
Might be better to have more similar lengths, and also to include 0 as max_ret_len since it might occur in places where that parameter is calculated.
0 const std::string random_string{provider.ConsumeRandomLengthString(1000)};
1 const auto max_ret_len{provider.ConsumeIntegralInRange<int>(0, 1000)};
re-ACK a32002f2d5cff72639c9782d70ae52ec162d9ef8
DecodeBase58&DecodeBase58Check.Re-ran the 2 modified fuzz-targets 30 seconds each.
re-ACK 6983d82c84d4ca351a7cd9d1e0e20ab878da6475
Responding to my previous feedback + testing negative max return values.
Retested modified targets.
Added edge cases, such as transitions at powers of 58, to verify boundary behavior.
This commit introduces symmetric encode-decode roundtrips for all bases.
Minor refactors were also included:
• Split each base into a separate fuzz target.
• Added symmetric encode-decode roundtrip tests for all bases.
• Removed trim testing for encoded_string, as Base58 does not use whitespace padding.
• Made comparisons stricter by removing unnecessary lowercase conversions for bases that have mixed-case alphabets.
Co-authored-by: Hodlinator <172445034+hodlinator@users.noreply.github.com>
Different values are used for max_ret_len throughout the codebase (e.g., 21, 34, 78).
Theoretically, negative and zero values are also permitted. Let's stress-test those as well.
Co-authored-by: brunoerg <brunoely.gc@gmail.com>
81@@ -82,20 +82,4 @@ BOOST_AUTO_TEST_CASE(base58_DecodeBase58)
82 BOOST_CHECK(!DecodeBase58Check("3vQB7B6MrGQZaxCuFg4oh\0" "0IOl"s, result, 100));
In commit 3b66b7e20ddba94d251c97818700d46030b14cc5
s/Spit/Split in commit message.
Hah, indeed, proofread all other commit messages:
0git range-diff 836d733a6788c7f7e66da6f8007fb62f15161429..6983d82c84d4ca351a7cd9d1e0e20ab878da6475 ae40cf1a8e16462a8b9dfd076d440bc8ec796c2b..f919d919eb8425ef2bb25aa0ebf61c90ab9b07fa
re-ACK f919d919eb8425ef2bb25aa0ebf61c90ab9b07fa
Only commit message improvements since last ACK.
PR summary nit:
Added dedicated padding tests to hardcode the failure behavior.
Not sure “hardcode” is the best verb there, how about:
“Added dedicated padding tests to cover failure behavior.”?
