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

l0rinc commented at 7:42 pm on May 3, 2024: contributor

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

DrahtBot commented at 7:42 pm on May 3, 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
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.

DrahtBot added the label Tests on May 3, 2024

in src/test/base58_tests.cpp:97 in 9431bc94a1 outdated

 95+        auto zeroes = InsecureRandBool() ? InsecureRandRange(len + 1) : 0;
 96         auto data = Cat(std::vector<unsigned char>(zeroes, '\000'), g_insecure_rand_ctx.randbytes(len - zeroes));
 97-        auto encoded = EncodeBase58Check(data);
 98+
 99+        auto leadingSpaces = InsecureRandBool() ? std::string(InsecureRandRange(10), ' ') : "";
100+        auto trailingSpaces = InsecureRandBool() ? std::string(InsecureRandRange(10), ' ') : "";

tdb3 commented at 5:51 pm on May 5, 2024:

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:

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

l0rinc commented at 6:23 pm on May 5, 2024:

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.

tdb3 commented at 6:33 pm on May 5, 2024:

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

edilmedeiros commented at 3:22 pm on May 8, 2024:

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.

l0rinc commented at 8:32 pm on May 8, 2024:

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.

l0rinc commented at 8:53 pm on May 11, 2024:

Added short explanations to the commit message

tdb3 commented at 5:53 pm on May 5, 2024: contributor

ACK for 9431bc94a19837a0b053fbbde9f5367543a7264e 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.

in src/test/base58_tests.cpp:99 in 9431bc94a1 outdated

 97-        auto encoded = EncodeBase58Check(data);
 98+
 99+        auto leadingSpaces = InsecureRandBool() ? std::string(InsecureRandRange(10), ' ') : "";
100+        auto trailingSpaces = InsecureRandBool() ? std::string(InsecureRandRange(10), ' ') : "";
101+        auto encoded = leadingSpaces + EncodeBase58Check(data) + trailingSpaces;
102+

edilmedeiros commented at 6:31 pm on May 7, 2024:

Since you are proposing random input vectors, what do you think about adding a log message with the generated test vector?

BOOST_TEST_MESSAGE("Test input: '" << encoded << "'");

looks good on my terminal. You probably can figure out a better message.

l0rinc commented at 9:26 pm on May 7, 2024:

Good idea, done

in src/test/base58_tests.cpp:40 in 9431bc94a1 outdated

34@@ -35,8 +35,9 @@ BOOST_AUTO_TEST_CASE(base58_EncodeBase58)
35         std::vector<unsigned char> sourcedata = ParseHex(test[0].get_str());
36         std::string base58string = test[1].get_str();
37         BOOST_CHECK_MESSAGE(
38-                    EncodeBase58(sourcedata) == base58string,
39-                    strTest);
40+            EncodeBase58(sourcedata) == base58string,
41+            strTest << "\nEncoding failed for test #" << idx << ": expected " << base58string << ", got " << EncodeBase58(sourcedata)
42+        );

edilmedeiros commented at 6:46 pm on May 7, 2024:

Something sus happened here: the tests seem to pass, but the message indicates encoding failure. Next is the beginning of the relevant log. It happens for all input vectors.

0test/base58_tests.cpp:21: Entering test suite "base58_tests"
1test/base58_tests.cpp:24: Entering test case "base58_EncodeBase58"
2test/base58_tests.cpp:40: info: check '["",""]
3Encoding failed for test [#0](/bitcoin-bitcoin/0/): expected , got ' has passed
4test/base58_tests.cpp:40: info: check '["61","2g"]
5Encoding failed for test [#1](/bitcoin-bitcoin/1/): expected 2g, got 2g' has passed
6test/base58_tests.cpp:40: info: check '["626262","a3gV"]
7Encoding failed for test [#2](/bitcoin-bitcoin/2/): expected a3gV, got a3gV' has passed
8...

l0rinc commented at 9:26 pm on May 7, 2024:

Fixed, thanks! Added you as a Co-author, please check that the email is correct.

edilmedeiros commented at 3:42 pm on May 8, 2024:

Fixed, thanks! Added you as a Co-author, please check that the email is correct.

Not needed, but highly appreciated. Thanks.

in src/test/base58_tests.cpp:101 in 9431bc94a1 outdated

101+        auto encoded = leadingSpaces + EncodeBase58Check(data) + trailingSpaces;
102+
103         std::vector<unsigned char> decoded;
104         auto ok_too_small = DecodeBase58Check(encoded, decoded, InsecureRandRange(len));
105-        BOOST_CHECK(!ok_too_small);
106+        BOOST_CHECK_MESSAGE(!ok_too_small, "Decoding should fail for smaller maxRetLen");

edilmedeiros commented at 7:14 pm on May 7, 2024:

I like this change, but the message doesn’t seem so good. I greped the repo but couldn’t find a maxRetLen symbol, thus it sounds very confusing to me.

l0rinc commented at 9:25 pm on May 7, 2024:

Thanks, I’ve cherry picked this, in the current impl it’s called max_ret_len - fixed

in src/test/base58_tests.cpp:65 in 9431bc94a1 outdated

61-        BOOST_CHECK_MESSAGE(result.size() == expected.size() && std::equal(result.begin(), result.end(), expected.begin()), strTest);
62+        BOOST_CHECK_MESSAGE(DecodeBase58(base58string, result, 256), strTest << "\nDecoding failed for test #" << idx << ": " << base58string);
63+        BOOST_CHECK_MESSAGE(
64+            result.size() == expected.size() && std::equal(result.begin(), result.end(), expected.begin()),
65+            strTest << "\nMismatch for test #" << idx << ": expected " << HexStr(expected) << ", got " << HexStr(result)
66+        );

edilmedeiros commented at 7:21 pm on May 7, 2024:

Same thing as above.

 0test/base58_tests.cpp:45: Entering test case "base58_DecodeBase58"
 1test/base58_tests.cpp:60: info: check '["",""]
 2Decoding failed for test [#0](/bitcoin-bitcoin/0/): ' has passed
 3test/base58_tests.cpp:64: info: check '["",""]
 4Mismatch for test [#0](/bitcoin-bitcoin/0/): expected , got ' has passed
 5test/base58_tests.cpp:60: info: check '["61","2g"]
 6Decoding failed for test [#1](/bitcoin-bitcoin/1/): 2g' has passed
 7test/base58_tests.cpp:64: info: check '["61","2g"]
 8Mismatch for test [#1](/bitcoin-bitcoin/1/): expected 61, got 61' has passed
 9test/base58_tests.cpp:60: info: check '["626262","a3gV"]
10Decoding failed for test [#2](/bitcoin-bitcoin/2/): a3gV' has passed
11test/base58_tests.cpp:64: info: check '["626262","a3gV"]
12Mismatch for test [#2](/bitcoin-bitcoin/2/): expected 626262, got 626262' has passed
13...

l0rinc commented at 9:24 pm on May 7, 2024:

Ah, now I get what you meant - I’ve changed the messages to shoulds, thanks for the hint! Running it with make && src/test/test_bitcoin --run_test=base58_tests --log_level=all -- -printtoconsole=1 now prints:

0test/base58_tests.cpp:108: info: check 'Decoding `111111111111111J4kMuvqbHmtTVvULAvzkZCa3m2n9zHaYBL3KCE5y7phvY7737aYsgLjg5343Awj7jbBuYvioVwvkP4HvWsb3T6F8w3WEax4HkrTx8GXbZc1R1m4BWT48R8d2rU3557aNGpQ5pFyaFHYzTzDiNRTVmoYG7pns87AuFahyrTUNRufZe4gLf5Mb1JhWDCd438qHu8pBFWsVJrGkw` as `0000000000000000000000000000008b34460016ad99c0bc7832c852b1466044ad61e39127e2c55a0de4edf834261031c53bdbe06ab56df531b108229e2a2c9e12b9ebe047f8628407efb6e5d4664072d7c5759037f01cae87c4a15d6bd260dddd678cd9b5ded2202067b0538e572dba1a91d88e86fabfefe8648388af51d3c4c5a2732e66ff5c924f3460cca68bcf336e50b1350d16bc8590456a4e2477a3f2d2` should match `0000000000000000000000000000008b34460016ad99c0bc7832c852b1466044ad61e39127e2c55a0de4edf834261031c53bdbe06ab56df531b108229e2a2c9e12b9ebe047f8628407efb6e5d4664072d7c5759037f01cae87c4a15d6bd260dddd678cd9b5ded2202067b0538e572dba1a91d88e86fabfefe8648388af51d3c4c5a2732e66ff5c924f3460cca68bcf336e50b1350d16bc8590456a4e2477a3f2d2`' has passed

edilmedeiros commented at 8:18 pm on May 7, 2024: contributor

Concept ACK

Built and ran the unit tests.

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

l0rinc commented at 8:19 pm on May 7, 2024: contributor

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 commented at 8:33 pm on May 7, 2024: contributor

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](/bitcoin-bitcoin/2/): expected 626262, got 626262' has passed.

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

l0rinc force-pushed on May 7, 2024

in src/test/base58_tests.cpp:60 in fc0cc2dbf9 outdated

56@@ -56,15 +57,20 @@ BOOST_AUTO_TEST_CASE(base58_DecodeBase58)
57         }
58         std::vector<unsigned char> expected = ParseHex(test[0].get_str());
59         std::string base58string = test[1].get_str();
60-        BOOST_CHECK_MESSAGE(DecodeBase58(base58string, result, 256), strTest);
61-        BOOST_CHECK_MESSAGE(result.size() == expected.size() && std::equal(result.begin(), result.end(), expected.begin()), strTest);
62+        BOOST_TEST_MESSAGE("Test input: `" << base58string << "`");
63+

edilmedeiros commented at 1:44 pm on May 8, 2024:

Let’s remove this since the messages now bring the test vectors themselves, this is duplicating purpose.

in src/test/base58_tests.cpp:72 in fc0cc2dbf9 outdated

71     BOOST_CHECK(!DecodeBase58("invalid"s, result, 100));
72     BOOST_CHECK(!DecodeBase58("invalid\0"s, result, 100));
73     BOOST_CHECK(!DecodeBase58("\0invalid"s, result, 100));
74 
75-    BOOST_CHECK(DecodeBase58("good"s, result, 100));
76+    BOOST_CHECK( DecodeBase58("good"s, result, 100));

edilmedeiros commented at 1:46 pm on May 8, 2024:

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

l0rinc commented at 8:36 pm on May 8, 2024:

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

edilmedeiros commented at 9:18 pm on May 8, 2024:

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

l0rinc commented at 8:01 pm on May 11, 2024:

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

in src/test/base58_tests.cpp:83 in fc0cc2dbf9 outdated

80@@ -75,25 +81,31 @@ BOOST_AUTO_TEST_CASE(base58_DecodeBase58)
81     std::vector<unsigned char> expected = ParseHex("971a55");
82     BOOST_CHECK_EQUAL_COLLECTIONS(result.begin(), result.end(), expected.begin(), expected.end());
83 
84-    BOOST_CHECK(DecodeBase58Check("3vQB7B6MrGQZaxCuFg4oh"s, result, 100));
85+    BOOST_CHECK( DecodeBase58Check("3vQB7B6MrGQZaxCuFg4oh"s, result, 100));

edilmedeiros commented at 1:46 pm on May 8, 2024:

0    BOOST_CHECK(DecodeBase58Check("3vQB7B6MrGQZaxCuFg4oh"s, result, 100));

Same as above.

in src/test/base58_tests.cpp:39 in fc0cc2dbf9 outdated

34@@ -35,8 +35,9 @@ BOOST_AUTO_TEST_CASE(base58_EncodeBase58)
35         std::vector<unsigned char> sourcedata = ParseHex(test[0].get_str());
36         std::string base58string = test[1].get_str();
37         BOOST_CHECK_MESSAGE(
38-                    EncodeBase58(sourcedata) == base58string,
39-                    strTest);
40+            EncodeBase58(sourcedata) == base58string,
41+            strTest << "\nEncoding `" << HexStr(Span(sourcedata)) << "` as `" << EncodeBase58(sourcedata) << "` should match `" << base58string << "`"

edilmedeiros commented at 2:59 pm on May 8, 2024:

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

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

l0rinc commented at 8:36 pm on May 11, 2024:

Didn’t realize this, thanks!

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

in src/test/base58_tests.cpp:64 in fc0cc2dbf9 outdated

62+        BOOST_TEST_MESSAGE("Test input: `" << base58string << "`");
63+
64+        BOOST_CHECK(DecodeBase58(base58string, result, 256));
65+        BOOST_CHECK_MESSAGE(
66+            result == expected,
67+            strTest << "\nDecoding `" << base58string << "` as `" << HexStr(result) << "` should match `" << HexStr(expected) << "`"

edilmedeiros commented at 3:07 pm on May 8, 2024:

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

Same suggestion as above.

l0rinc commented at 8:36 pm on May 11, 2024:

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

in src/test/base58_tests.cpp:62 in fc0cc2dbf9 outdated

56@@ -56,15 +57,20 @@ BOOST_AUTO_TEST_CASE(base58_DecodeBase58)
57         }
58         std::vector<unsigned char> expected = ParseHex(test[0].get_str());
59         std::string base58string = test[1].get_str();
60-        BOOST_CHECK_MESSAGE(DecodeBase58(base58string, result, 256), strTest);
61-        BOOST_CHECK_MESSAGE(result.size() == expected.size() && std::equal(result.begin(), result.end(), expected.begin()), strTest);
62+        BOOST_TEST_MESSAGE("Test input: `" << base58string << "`");
63+
64+        BOOST_CHECK(DecodeBase58(base58string, result, 256));

edilmedeiros commented at 3:09 pm on May 8, 2024:

0        BOOST_CHECK_MESSAGE(DecodeBase58(base58string, result, 256), strTest);

Testing for the same thing as before, but removing the test vector from the message don’t seem to be an improvement. Potentially makes debugging more difficult.

in src/test/base58_tests.cpp:63 in fc0cc2dbf9 outdated

61-        BOOST_CHECK_MESSAGE(result.size() == expected.size() && std::equal(result.begin(), result.end(), expected.begin()), strTest);
62+        BOOST_TEST_MESSAGE("Test input: `" << base58string << "`");
63+
64+        BOOST_CHECK(DecodeBase58(base58string, result, 256));
65+        BOOST_CHECK_MESSAGE(
66+            result == expected,

edilmedeiros commented at 3:14 pm on May 8, 2024:

I was in doubt this would check if the vectors contents are equal, but it does indeed. Nice catch.

l0rinc commented at 8:39 pm on May 8, 2024:

Valid concern, I also testes it to understand why it was done differently

in src/test/base58_tests.cpp:100 in fc0cc2dbf9 outdated

100-        auto encoded = EncodeBase58Check(data);
101+
102+        auto leadingSpaces = InsecureRandBool() ? std::string(InsecureRandRange(10), ' ') : "";
103+        auto trailingSpaces = InsecureRandBool() ? std::string(InsecureRandRange(10), ' ') : "";
104+        auto encoded = leadingSpaces + EncodeBase58Check(data) + trailingSpaces;
105+        BOOST_TEST_MESSAGE("Test input: `" << encoded << "`");

edilmedeiros commented at 3:18 pm on May 8, 2024:

Don’t need this since the test messages now bring the input vector.

in src/test/base58_tests.cpp:101 in fc0cc2dbf9 outdated

108-        auto ok_too_small = DecodeBase58Check(encoded, decoded, InsecureRandRange(len));
109-        BOOST_CHECK(!ok_too_small);
110-        auto ok = DecodeBase58Check(encoded, decoded, len + InsecureRandRange(257 - len));
111-        BOOST_CHECK(ok);
112-        BOOST_CHECK(data == decoded);
113+        BOOST_CHECK_MESSAGE(!DecodeBase58Check(encoded, decoded, InsecureRandRange(len)), "Decoding should fail for smaller max_ret_len");

edilmedeiros commented at 3:33 pm on May 8, 2024:

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.

l0rinc commented at 8:52 pm on May 11, 2024:

Indeed, added the values to the error message:

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

in src/test/base58_tests.cpp:102 in fc0cc2dbf9 outdated

109-        BOOST_CHECK(!ok_too_small);
110-        auto ok = DecodeBase58Check(encoded, decoded, len + InsecureRandRange(257 - len));
111-        BOOST_CHECK(ok);
112-        BOOST_CHECK(data == decoded);
113+        BOOST_CHECK_MESSAGE(!DecodeBase58Check(encoded, decoded, InsecureRandRange(len)), "Decoding should fail for smaller max_ret_len");
114+        BOOST_CHECK_MESSAGE( DecodeBase58Check(encoded, decoded, len + InsecureRandRange(257 - len)), "Decoding should succeed within valid length range");

edilmedeiros commented at 3:34 pm on May 8, 2024:

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.

l0rinc commented at 8:52 pm on May 11, 2024:

Done, thanks!

in src/test/data/base58_encode_decode.json:1 in fc0cc2dbf9

edilmedeiros commented at 3:37 pm on May 8, 2024:

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.

l0rinc commented at 8:40 pm on May 8, 2024:

Thanks for your detailed review, will do that this week

l0rinc commented at 8:54 pm on May 11, 2024:

Thanks, done

edilmedeiros changes_requested

edilmedeiros commented at 3:41 pm on May 8, 2024: contributor

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

l0rinc force-pushed on May 8, 2024

l0rinc force-pushed on May 11, 2024

Add a few more corner cases to the base58 test suite

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>

9a540a7a79

l0rinc force-pushed on May 29, 2024

DrahtBot added the label CI failed on Jun 18, 2024

DrahtBot removed the label CI failed on Jun 18, 2024

in src/test/base58_tests.cpp:29 in 9a540a7a79

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

TheCharlatan commented at 8:06 pm on August 14, 2024:

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

l0rinc commented at 6:57 am on August 15, 2024:

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

in src/test/base58_tests.cpp:37 in 9a540a7a79

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

TheCharlatan commented at 8:24 pm on August 14, 2024:

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

in src/test/base58_tests.cpp:60 in 9a540a7a79

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

TheCharlatan commented at 8:24 pm on August 14, 2024:

Unneeded whitespace change.

in src/test/base58_tests.cpp:79 in 9a540a7a79

75@@ -71,7 +76,7 @@ BOOST_AUTO_TEST_CASE(base58_DecodeBase58)
76 
77     // check that DecodeBase58 skips whitespace, but still fails with unexpected non-whitespace at the end.
78     BOOST_CHECK(!DecodeBase58(" \t\n\v\f\r skip \r\f\v\n\t a", result, 3));
79-    BOOST_CHECK( DecodeBase58(" \t\n\v\f\r skip \r\f\v\n\t ", result, 3));
80+    BOOST_CHECK(DecodeBase58(" \t\n\v\f\r skip \r\f\v\n\t ", result, 3));

TheCharlatan commented at 8:26 pm on August 14, 2024:

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

l0rinc commented at 6:49 am on August 15, 2024:

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

in src/test/base58_tests.cpp:64 in 9a540a7a79

56@@ -56,8 +57,12 @@ BOOST_AUTO_TEST_CASE(base58_DecodeBase58)
57         }
58         std::vector<unsigned char> expected = ParseHex(test[0].get_str());
59         std::string base58string = test[1].get_str();
60+
61         BOOST_CHECK_MESSAGE(DecodeBase58(base58string, result, 256), strTest);
62-        BOOST_CHECK_MESSAGE(result.size() == expected.size() && std::equal(result.begin(), result.end(), expected.begin()), strTest);
63+        BOOST_CHECK_MESSAGE(
64+            result == expected,
65+            strTest << ": got \"" << HexStr(result) << "\""

TheCharlatan commented at 8:35 pm on August 14, 2024:

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.

in src/test/base58_tests.cpp:101 in 9a540a7a79

105-        auto ok_too_small = DecodeBase58Check(encoded, decoded, InsecureRandRange(len));
106-        BOOST_CHECK(!ok_too_small);
107-        auto ok = DecodeBase58Check(encoded, decoded, len + InsecureRandRange(257 - len));
108-        BOOST_CHECK(ok);
109-        BOOST_CHECK(data == decoded);
110+        auto invalidSmallResultLength = InsecureRandRange(len);

TheCharlatan commented at 9:07 pm on August 14, 2024:

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.

l0rinc commented at 7:00 am on August 15, 2024:

The rest of the code was using this style.

in src/test/base58_tests.cpp:89 in 9a540a7a79

85@@ -81,19 +86,26 @@ BOOST_AUTO_TEST_CASE(base58_DecodeBase58)
86     BOOST_CHECK(!DecodeBase58Check("3vQB7B6MrGQZaxCuFg4oh\0" "0IOl"s, result, 100));
87 }
88 
89-BOOST_AUTO_TEST_CASE(base58_random_encode_decode)
90+BOOST_AUTO_TEST_CASE(base58_random_encode_decode_with_optional_spaces)

TheCharlatan commented at 9:10 pm on August 14, 2024:

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 commented at 7:01 am on August 15, 2024: contributor

I don’t think this change makes sense

k, closing.

l0rinc closed this on Aug 15, 2024

in src/test/data/base58_encode_decode.json:14 in 9a540a7a79

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

maflcko commented at 7:28 am on August 15, 2024:

Seems fine to just add the new data, no?

l0rinc commented at 5:27 pm on August 29, 2024:

Skipped the rest of the changes, moved these over to https://github.com/bitcoin/bitcoin/pull/30746

TheCharlatan commented at 7:37 am on August 15, 2024: contributor

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.

l0rinc deleted the branch on Aug 29, 2024

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

Code Coverage

Reviews

Conflicts