test: Add a few more corner cases to the base58 test suite #30035

l0rinc · 2024-05-03T19:42:38Z

Split out the additional tests from the base58 optimization PR as suggested #29473 (comment)

DrahtBot · 2024-05-03T19:42:42Z

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
Concept ACK	edilmedeiros
Stale ACK	tdb3

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

Conflicts

Reviewers, this pull request conflicts with the following ones:

#30571 (test: [refactor] Use g_rng/m_rng directly by maflcko)
#30377 (refactor: Replace ParseHex with consteval ArrayFromHex by hodlinator)

If you consider this pull request important, please also help to review the conflicting pull requests. Ideally, start with the one that should be merged first.

tdb3

ACK for 9431bc9
Built and ran unit tests (all passed).
Left one nit, but the nit is outside the scope of this PR, so is probably better left to a separate PR.

tdb3 · 2024-05-05T17:51:53Z

src/test/base58_tests.cpp

+        auto leadingSpaces = InsecureRandBool() ? std::string(InsecureRandRange(10), ' ') : "";
+        auto trailingSpaces = InsecureRandBool() ? std::string(InsecureRandRange(10), ' ') : "";


nit: Probably outside the scope of this PR (this PR is adding tests, so business logic changes are extraneous), but at first glance, seems like these statements could be simplified, since InsecureRandRange() can return 0, the std::string constructor can handle 0 count, and string operator+ can handle empty string addition. Maybe I'm missing something?

For example:

diff --git a/src/test/base58_tests.cpp b/src/test/base58_tests.cpp index 49ef9ff5b5..beb5ef3335 100644 --- a/src/test/base58_tests.cpp +++ b/src/test/base58_tests.cpp @@ -92,8 +92,8 @@ BOOST_AUTO_TEST_CASE(base58_random_encode_decode_with_optional_spaces) auto zeroes = InsecureRandBool() ? InsecureRandRange(len + 1) : 0; auto data = Cat(std::vector<unsigned char>(zeroes, '\000'), g_insecure_rand_ctx.randbytes(len - zeroes)); - auto leadingSpaces = InsecureRandBool() ? std::string(InsecureRandRange(10), ' ') : ""; - auto trailingSpaces = InsecureRandBool() ? std::string(InsecureRandRange(10), ' ') : ""; + auto leadingSpaces = std::string(InsecureRandRange(10), ' '); + auto trailingSpaces = std::string(InsecureRandRange(10), ' '); auto encoded = leadingSpaces + EncodeBase58Check(data) + trailingSpaces; std::vector<unsigned char> decoded;

Thanks for checking @tdb3, the results would be similar, but since I assumed the spaces are rare in reality, I gave it different odds.
In my impl the probability that we won't have any leading (or trailing) spaces was 50% + 50%*10%, in your impl it's 10%, so spaces would be in most samples.
I'm fine with both.

Thanks, that's right (higher probability of no spaces over rand range alone). I don't have a preference, just an observation.

I like all this, but it would be better to explain in the PR description why it is important/beneficial to include this change since this increases the test complexity.

Probably would be even better to have a separate test case to check for leading and trailing spaces. This test case is intended to check for leading zeros and now it checks for three concerns (leading zeros, leading spaces, and trailing spaces) instead of one.

it checks for three concerns

That's a property based test, i.e. for random valid inputs it checks that certain conditions hold - in this case a roundtrip, that decoding an encoded results in the original trimmed value.
It's meant to find corner cases that we haven't though of, that's its single concern, we have separate tests for each corner case we know about.

Added short explanations to the commit message

edilmedeiros

Concept ACK

Built and ran the unit tests.

The new messages seem to be the opposite of what the tests do, tough.

src/test/base58_tests.cpp

l0rinc · 2024-05-07T20:19:28Z

Thanks for the review @edilmedeiros, some of the existing BOOST_CHECK_MESSAGE messages are either in indicative or subjunctive grammatical moods (even in the same file, as you can see).
I'm fine with both, but if you think the subjunctive is more readable, I'll rephrase.

edilmedeiros · 2024-05-07T20:33:22Z

Thanks for the review @edilmedeiros, some of the existing BOOST_CHECK_MESSAGE messages are either in indicative or subjunctive grammatical moods (even in the same file, as you can see). I'm fine with both, but if you think the subjunctive is more readable, I'll rephrase.

I have no personal preference about it, but this is not my point.

Take for instance Mismatch for test #2: expected 626262, got 626262' has passed.

How can (expected) 626262 not match (got) 626262?

edilmedeiros

Gave another deep look at this, thanks again for submitting this PR.

Beside the specific code comments, please add to the commit message why are you adding new test cases, what they improve in the test logic (and how they help the work on #29473).

src/test/base58_tests.cpp

edilmedeiros · 2024-05-08T13:46:35Z

src/test/base58_tests.cpp

    }

    BOOST_CHECK(!DecodeBase58("invalid"s, result, 100));
    BOOST_CHECK(!DecodeBase58("invalid\0"s, result, 100));
    BOOST_CHECK(!DecodeBase58("\0invalid"s, result, 100));

-    BOOST_CHECK(DecodeBase58("good"s, result, 100));
+    BOOST_CHECK( DecodeBase58("good"s, result, 100));


Suggested change

BOOST_CHECK( DecodeBase58("good"s, result, 100));

BOOST_CHECK(DecodeBase58("good"s, result, 100));

I understand the rational of aligning this with surrounding context, but does seem against guidelines and will look like a typo for others.

The file was already using this format: https://github.com/bitcoin/bitcoin/blob/master/src/test/base58_tests.cpp#L74

Looks more like a typo, see lines 67 and 78.

I don't feel strongly about either, removed the space

src/test/base58_tests.cpp

edilmedeiros · 2024-05-08T14:59:23Z

src/test/base58_tests.cpp

-                    EncodeBase58(sourcedata) == base58string,
-                    strTest);
+            EncodeBase58(sourcedata) == base58string,
+            strTest << "\nEncoding `" << HexStr(Span(sourcedata)) << "` as `" << EncodeBase58(sourcedata) << "` should match `" << base58string << "`"


Suggested change

strTest << "\nEncoding `" << HexStr(Span(sourcedata)) << "` as `" << EncodeBase58(sourcedata) << "` should match `" << base58string << "`"

strTest << ": got \"" << EncodeBase58(sourcedata) << "\""

What about a little less verbose and taking advantage of the strTest string that has both input and expected outcome?

Didn't realize this, thanks!

test/base58_tests.cpp:40: info: check '["271F359E","zzzzy"]: got "zzzzy"' has passed

edilmedeiros · 2024-05-08T15:07:02Z

src/test/base58_tests.cpp

+        BOOST_CHECK(DecodeBase58(base58string, result, 256));
+        BOOST_CHECK_MESSAGE(
+            result == expected,
+            strTest << "\nDecoding `" << base58string << "` as `" << HexStr(result) << "` should match `" << HexStr(expected) << "`"


Suggested change

strTest << "\nDecoding `" << base58string << "` as `" << HexStr(result) << "` should match `" << HexStr(expected) << "`"

strTest << ": got \"" << EncodeBase58(sourcedata) << "\""

Same suggestion as above.

test/base58_tests.cpp:65: info: check '["271F35A1","211112"]: got XXX "271f35a1"' has passed

src/test/base58_tests.cpp

edilmedeiros · 2024-05-08T15:22:55Z

src/test/base58_tests.cpp

+        auto leadingSpaces = InsecureRandBool() ? std::string(InsecureRandRange(10), ' ') : "";
+        auto trailingSpaces = InsecureRandBool() ? std::string(InsecureRandRange(10), ' ') : "";


I like all this, but it would be better to explain in the PR description why it is important/beneficial to include this change since this increases the test complexity.

Probably would be even better to have a separate test case to check for leading and trailing spaces. This test case is intended to check for leading zeros and now it checks for three concerns (leading zeros, leading spaces, and trailing spaces) instead of one.

edilmedeiros · 2024-05-08T15:33:56Z

src/test/base58_tests.cpp

-        auto ok = DecodeBase58Check(encoded, decoded, len + InsecureRandRange(257 - len));
-        BOOST_CHECK(ok);
-        BOOST_CHECK(data == decoded);
+        BOOST_CHECK_MESSAGE(!DecodeBase58Check(encoded, decoded, InsecureRandRange(len)), "Decoding should fail for smaller max_ret_len");


Another place where there's a random parameter that's not reported making potential debugging impossible.

Also, the text is no good since max_ret_len is the maximum return size. Better something like Decoding exceeds xxx length. where xxx prints the random parameter.

Indeed, added the values to the error message:

test/base58_tests.cpp:102: error: in "base58_tests/base58_random_encode_decode_with_optional_spaces": Decoding should fail for `invalidSmallResultLength` (61) test/base58_tests.cpp:104: error: in "base58_tests/base58_random_encode_decode_with_optional_spaces": Decoding should succeed within sufficiently large result length (134)

edilmedeiros · 2024-05-08T15:34:32Z

src/test/base58_tests.cpp

-        BOOST_CHECK(ok);
-        BOOST_CHECK(data == decoded);
+        BOOST_CHECK_MESSAGE(!DecodeBase58Check(encoded, decoded, InsecureRandRange(len)), "Decoding should fail for smaller max_ret_len");
+        BOOST_CHECK_MESSAGE( DecodeBase58Check(encoded, decoded, len + InsecureRandRange(257 - len)), "Decoding should succeed within valid length range");


Same thing about unreported random parameter in the test. This is worse yet since there's a weird logic to get the param. It's good that you submitted this PR, the original test deserved a better look already.

Done, thanks!

edilmedeiros · 2024-05-08T15:37:38Z

src/test/data/base58_encode_decode.json

Please explain in the PR first comment and in the commit message why are you adding these specific test cases, what do they improve in the existing test logic.

Thanks for your detailed review, will do that this week

Thanks, done

Add better errors for base58_EncodeBase58 and base58_DecodeBase58 to see the differing value in case of failure. Extended the base58_random_encode_decode_with_optional_spaces property based test - containing a simple roundtrip with decoding validation - to stress the leading and trailing space parsing as well. Also extended the base58_encode_decode.json file with a few corner cases - e.g. on a transition of power of 58 to check the boundaries. Co-authored-by: Edil Medeiros <jose.edil@gmail.com>

TheCharlatan · 2024-08-14T20:06:48Z

src/test/base58_tests.cpp

@@ -26,17 +26,18 @@ BOOST_AUTO_TEST_CASE(base58_EncodeBase58)
    UniValue tests = read_json(json_tests::base58_encode_decode);
    for (unsigned int idx = 0; idx < tests.size(); idx++) {
        const UniValue& test = tests[idx];
-        std::string strTest = test.write();
+        auto strTest = test.write();


Changes like adding auto instead of the actual type are mostly noise. I don't think there is precedence for merging these. Can you drop them again (here and in other places where it is the sole change on that line)?

I considered the actual type to be just noise in these cases, but you seem to have a stronger preference for minimal diff, reverted.

TheCharlatan · 2024-08-14T20:24:23Z

src/test/base58_tests.cpp

-        std::vector<unsigned char> sourcedata = ParseHex(test[0].get_str());
-        std::string base58string = test[1].get_str();
+        auto encodedSource = EncodeBase58(ParseHex(test[0].get_str()));
+        auto base58string = test[1].get_str();
        BOOST_CHECK_MESSAGE(


If you are touching this, why not just make this BOOST_CHECK_EQUAL(EncodeBase58(sourcedata), base58string); and drop all the other noisy changes here?

TheCharlatan · 2024-08-14T20:24:40Z

src/test/base58_tests.cpp

@@ -56,8 +57,12 @@ BOOST_AUTO_TEST_CASE(base58_DecodeBase58)
        }
        std::vector<unsigned char> expected = ParseHex(test[0].get_str());
        std::string base58string = test[1].get_str();
+


Unneeded whitespace change.

TheCharlatan · 2024-08-14T20:26:52Z

src/test/base58_tests.cpp

@@ -71,7 +76,7 @@ BOOST_AUTO_TEST_CASE(base58_DecodeBase58)

    // check that DecodeBase58 skips whitespace, but still fails with unexpected non-whitespace at the end.
    BOOST_CHECK(!DecodeBase58(" \t\n\v\f\r skip \r\f\v\n\t a", result, 3));
-    BOOST_CHECK( DecodeBase58(" \t\n\v\f\r skip \r\f\v\n\t ", result, 3));
+    BOOST_CHECK(DecodeBase58(" \t\n\v\f\r skip \r\f\v\n\t ", result, 3));


This whitespace was intentional such that the escape patterns can easily be compared with one another. Please leave it like it is.

This was specifically requested, but I'll revert, since I liked the spaces: #30035 (comment)

TheCharlatan · 2024-08-14T20:35:09Z

src/test/base58_tests.cpp

-        BOOST_CHECK_MESSAGE(result.size() == expected.size() && std::equal(result.begin(), result.end(), expected.begin()), strTest);
+        BOOST_CHECK_MESSAGE(
+            result == expected,
+            strTest << ": got \"" << HexStr(result) << "\""


Not sure if this change is really worth it (and the one adding more context to the case above). There is no randomness involved here and the programmer will have to debug anyway if there is a failure. The other changes for printing some context do make sense, since there is randomness involved and the failure case may not be reproduced immediately.

TheCharlatan · 2024-08-14T21:07:05Z

src/test/base58_tests.cpp

-        auto ok = DecodeBase58Check(encoded, decoded, len + InsecureRandRange(257 - len));
-        BOOST_CHECK(ok);
-        BOOST_CHECK(data == decoded);
+        auto invalidSmallResultLength = InsecureRandRange(len);


Please stick to the symbol naming conventions in https://github.com/bitcoin/bitcoin/blob/master/doc/developer-notes.md#coding-style-c - specifically use snake_case everywhere.

The rest of the code was using this style.

TheCharlatan · 2024-08-14T21:10:59Z

src/test/base58_tests.cpp

@@ -81,19 +86,26 @@ BOOST_AUTO_TEST_CASE(base58_DecodeBase58)
    BOOST_CHECK(!DecodeBase58Check("3vQB7B6MrGQZaxCuFg4oh\0" "0IOl"s, result, 100));
 }

-BOOST_AUTO_TEST_CASE(base58_random_encode_decode)
+BOOST_AUTO_TEST_CASE(base58_random_encode_decode_with_optional_spaces)


I don't think this change makes sense, since there is already other stuff added to the test data. I would just leave it as is.

l0rinc · 2024-08-15T07:01:49Z

I don't think this change makes sense

k, closing.

maflcko · 2024-08-15T07:28:27Z

src/test/data/base58_encode_decode.json

@@ -11,6 +11,13 @@
 ["ecac89cad93923c02321", "EJDM8drfXA6uyA"],
 ["10c8511e", "Rt5zm"],
 ["00000000000000000000", "1111111111"],
+["00000000000000000000000000000000000000000000000000000000000000000000000000000000", "1111111111111111111111111111111111111111"],


Seems fine to just add the new data, no?

Skipped the rest of the changes, moved these over to #30746

TheCharlatan · 2024-08-15T07:37:15Z

I would have ACKed this if it were cleaned up a bit. The new test data and better error messages in the randomized tests are good changes.

…z (and padding) tests f919d91 fuzz: Add fuzzing for max_ret_len in DecodeBase58/DecodeBase58Check (Lőrinc) 635bc58 test: Fuzz Base32/Base58/Base64 roundtrip conversions (Lőrinc) 5dd3a0d test: Extend base58_encode_decode.json with edge cases (Lőrinc) ae40cf1 test: Add padding tests for Base32/Base64 (Lőrinc) Pull request description: Added fuzzed roundtrips for `base[32|58|64]` 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 #30035 ACKs for top commit: hodlinator: re-ACK f919d91 achow101: ACK f919d91 Tree-SHA512: 6a6c63d0a659b70d42aad7a8f37ce6e372756e2c88c84e7be5c1ff1f2a7c58860ed7113acbe1a9658a7d19deb91f0abe2ec527ed660335845cd1e0a9380b4295

DrahtBot added the Tests label May 3, 2024

tdb3 reviewed May 5, 2024

View reviewed changes

edilmedeiros reviewed May 7, 2024

View reviewed changes

src/test/base58_tests.cpp Show resolved Hide resolved

src/test/base58_tests.cpp Outdated Show resolved Hide resolved

src/test/base58_tests.cpp Outdated Show resolved Hide resolved

src/test/base58_tests.cpp Outdated Show resolved Hide resolved

l0rinc force-pushed the paplorinc/base58-tests branch from 9431bc9 to fc0cc2d Compare May 7, 2024 21:24

edilmedeiros suggested changes May 8, 2024

View reviewed changes

l0rinc force-pushed the paplorinc/base58-tests branch 5 times, most recently from b1142e3 to 861ab92 Compare May 11, 2024 20:52

l0rinc force-pushed the paplorinc/base58-tests branch from 861ab92 to 9a540a7 Compare May 29, 2024 07:58

DrahtBot added CI failed and removed CI failed labels Jun 18, 2024

l0rinc mentioned this pull request Jul 1, 2024

optimization: Speed up Base58 encoding/decoding by 400%/200% via preliminary byte packing #29473

Closed

DrahtBot mentioned this pull request Jul 12, 2024

refactor: Replace ParseHex with consteval ""_hex literals #30377

Merged

DrahtBot mentioned this pull request Aug 2, 2024

test: [refactor] Use m_rng directly #30571

Merged

TheCharlatan reviewed Aug 14, 2024

View reviewed changes

l0rinc closed this Aug 15, 2024

maflcko reviewed Aug 15, 2024

View reviewed changes

maflcko mentioned this pull request Aug 23, 2024

test: add subsidy sum test, iterating every block #30699

Closed

l0rinc mentioned this pull request Aug 29, 2024

test: cover base[32|58|64] with symmetric roundtrip fuzz (and padding) tests #30746

Merged

l0rinc deleted the paplorinc/base58-tests branch August 29, 2024 17:32

l0rinc mentioned this pull request Dec 27, 2024

RPC: Add reserve member function to UniValue and use it in blockToJSON function #31179

Merged

bitcoin locked and limited conversation to collaborators Aug 29, 2025

		auto leadingSpaces = InsecureRandBool() ? std::string(InsecureRandRange(10), ' ') : "";
		auto trailingSpaces = InsecureRandBool() ? std::string(InsecureRandRange(10), ' ') : "";

	BOOST_CHECK( DecodeBase58("good"s, result, 100));
	BOOST_CHECK(DecodeBase58("good"s, result, 100));

	strTest << "\nEncoding `" << HexStr(Span(sourcedata)) << "` as `" << EncodeBase58(sourcedata) << "` should match `" << base58string << "`"
	strTest << ": got \"" << EncodeBase58(sourcedata) << "\""

	strTest << "\nDecoding `" << base58string << "` as `" << HexStr(result) << "` should match `" << HexStr(expected) << "`"
	strTest << ": got \"" << EncodeBase58(sourcedata) << "\""

test: Add a few more corner cases to the base58 test suite #30035

test: Add a few more corner cases to the base58 test suite #30035

Conversation

l0rinc commented May 3, 2024 • edited Loading Uh oh! There was an error while loading. Please reload this page.

Uh oh!

Uh oh!

DrahtBot commented May 3, 2024 • edited Loading Uh oh! There was an error while loading. Please reload this page.

Uh oh!

Code Coverage

Reviews

Conflicts

Uh oh!

tdb3 left a comment

Choose a reason for hiding this comment

Uh oh!

Choose a reason for hiding this comment

Uh oh!

l0rinc May 5, 2024 • edited Loading Uh oh! There was an error while loading. Please reload this page.

Uh oh!

Choose a reason for hiding this comment

Uh oh!

Choose a reason for hiding this comment

Uh oh!

Choose a reason for hiding this comment

Uh oh!

Choose a reason for hiding this comment

Uh oh!

Choose a reason for hiding this comment

Uh oh!

edilmedeiros left a comment

Choose a reason for hiding this comment

Uh oh!

Uh oh!

Uh oh!

Uh oh!

Uh oh!

l0rinc commented May 7, 2024

Uh oh!

edilmedeiros commented May 7, 2024

Uh oh!

edilmedeiros left a comment

Choose a reason for hiding this comment

Uh oh!

Uh oh!

Choose a reason for hiding this comment

Uh oh!

Choose a reason for hiding this comment

Uh oh!

Choose a reason for hiding this comment

Uh oh!

Choose a reason for hiding this comment

Uh oh!

Uh oh!

Choose a reason for hiding this comment

Uh oh!

Choose a reason for hiding this comment

Uh oh!

Choose a reason for hiding this comment

Uh oh!

l0rinc May 11, 2024 • edited Loading Uh oh! There was an error while loading. Please reload this page.

Uh oh!

Choose a reason for hiding this comment

Uh oh!

Uh oh!

Choose a reason for hiding this comment

Uh oh!

Choose a reason for hiding this comment

Uh oh!

Choose a reason for hiding this comment

Uh oh!

Choose a reason for hiding this comment

Uh oh!

Choose a reason for hiding this comment

Uh oh!

Choose a reason for hiding this comment

Uh oh!

Choose a reason for hiding this comment

Uh oh!

Choose a reason for hiding this comment

Uh oh!

l0rinc commented May 3, 2024 •

edited

Loading

DrahtBot commented May 3, 2024 •

edited

Loading

l0rinc May 5, 2024 •

edited

Loading

l0rinc May 11, 2024 •

edited

Loading

l0rinc Aug 15, 2024 •

edited

Loading

l0rinc Aug 29, 2024 •

edited

Loading