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

paplorinc · 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.

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

paplorinc · 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>

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

paplorinc 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

paplorinc force-pushed the paplorinc/base58-tests branch 4 times, most recently from aae9616 to b1142e3 Compare May 11, 2024 20:37

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

Provide feedback

Saved searches

Use saved searches to filter your results more quickly

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

paplorinc commented May 3, 2024 •

edited

DrahtBot commented May 3, 2024 •

edited

tdb3 left a comment

tdb3 May 5, 2024

paplorinc May 5, 2024 •

edited

tdb3 May 5, 2024

edilmedeiros May 8, 2024

paplorinc May 8, 2024

paplorinc May 11, 2024

edilmedeiros left a comment

paplorinc commented May 7, 2024

edilmedeiros commented May 7, 2024

edilmedeiros left a comment

edilmedeiros May 8, 2024

paplorinc May 8, 2024

edilmedeiros May 8, 2024

paplorinc May 11, 2024

edilmedeiros May 8, 2024

paplorinc May 11, 2024

edilmedeiros May 8, 2024

paplorinc May 11, 2024 •

edited

edilmedeiros May 8, 2024

edilmedeiros May 8, 2024

paplorinc May 11, 2024

edilmedeiros May 8, 2024

paplorinc May 11, 2024

edilmedeiros May 8, 2024

paplorinc May 8, 2024

paplorinc May 11, 2024

		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

Are you sure you want to change the base?

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

Conversation

paplorinc commented May 3, 2024 • edited

DrahtBot commented May 3, 2024 • edited

Code Coverage

Reviews

tdb3 left a comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

paplorinc May 5, 2024 • edited

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

edilmedeiros left a comment

Choose a reason for hiding this comment

paplorinc commented May 7, 2024

edilmedeiros commented May 7, 2024

edilmedeiros left a comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

paplorinc May 11, 2024 • edited

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

paplorinc commented May 3, 2024 •

edited

DrahtBot commented May 3, 2024 •

edited

paplorinc May 5, 2024 •

edited

paplorinc May 11, 2024 •

edited