test: fix creation of "std::string"s with \0s

[vasild]

A string literal "abc" contains a terminating \0, so that is 4
bytes. There is no need to write "abc\0" unless two terminating
\0s are necessary.

std::string objects do not internally contain a terminating \0, so
std::string("abc") creates a string with size 3 and is the same as
std::string("abc", 3).

In "\01" the 01 part is interpreted as one number (1) and that is
the same as "\1" which is a string like {1, 0} whereas "\0z" is a
string like {0, 'z', 0}. To create a string like {0, '1', 0} one
must use "\0" "1".

Adjust the tests accordingly.

[eriknylund]

Concept ACK. I browsed over these decode tests when extending the Base32 encoding tests with support for no padding for #19845 and I found them hard to comprehend. I think this is an improvement.

[vasild]

This is actually mode widespread. I checked all tests and fixed some others too.

@eriknylund, this is how stumbled on that too! :)

[practicalswift]

ACK b7f1cc6 - patch looks correct

Thanks for cleaning up these tests! :)

[maflcko]

This fails before and after, indicating that the failure has nothing to do with NUL, no?

[eriknylund]

Good point. I also don't fully understand what any of these tests are trying to achieve? Wouldn't something like

diff --git a/src/test/base32_tests.cpp b/src/test/base32_tests.cpp
index 26ed78e2c2..4a5af0fd4d 100644
--- a/src/test/base32_tests.cpp
+++ b/src/test/base32_tests.cpp
@@ -23,13 +23,13 @@ BOOST_AUTO_TEST_CASE(base32_testvectors)
 
     // Decoding strings with embedded NUL characters should fail
     bool failure;
-    (void)DecodeBase32(std::string("invalid", 8), &failure);
-    BOOST_CHECK(failure);
-    (void)DecodeBase32(std::string("AWSX3VPP"), &failure);
+    (void)DecodeBase32(std::string("my======", 8), &failure);
     BOOST_CHECK(!failure);
-    (void)DecodeBase32(std::string("AWSX3VPP\0invalid", 16), &failure);
+    (void)DecodeBase32(std::string("my======\0", 9), &failure);
+    BOOST_CHECK(failure);
+    (void)DecodeBase32(std::string("\0my======", 9), &failure);
     BOOST_CHECK(failure);
-    (void)DecodeBase32(std::string("AWSX3VPPinvalid", 16), &failure);
+    (void)DecodeBase32(std::string("my=\0=====", 9), &failure);
     BOOST_CHECK(failure);
 }

do more according to what the comment on L24 implies?

[vasild]

no?

no :-)

Before this PR DecodeBase32(std::string("invalid", 7), &failure) fails because the supplied input has invalid length (7) and is not padded to a proper length (8) with =.

With this PR the same call fails due to the ValidAsCString() check early in DecodeBase32() (this check passes before this PR). The trailing \0 upsets the ValidAsCString() check which I think was the intent.

[vasild]

Sorry, with this PR it is not "the same call", but DecodeBase32(std::string("invalid", 8), &failure).

[maflcko]

Because the failure reason is not returned, it might be good to use a valid Base32 string, which properly decodes and then make it an invalid cstr, which fails. Otherwise it is harder to guess which failure reason was the intent from just reading the test code.

[vasild]

it might be good to use a valid Base32 string, which properly decodes and then make it an invalid cstr, which fails

The tests below already do that with AWSX3VPP (length 8, valid) and AWSX3VPP\0invalid (length 16, invalid due to embedded \0).

[eriknylund]

To my understanding AWSX3VPP is not a valid base32 encoded string, but perhaps I'm missing something obvious?

[vasild]

Why not? It has proper length (8) and no embedded \0s. DecodeBase32() decodes is successfully.

[eriknylund]

Thanks, I see your point and why I misunderstood the test in this case. To me the value AWXS3VPP looks like a magic string chosen on purpose and not on random, and I expected it to actually decode into something more human readable, like the other test values in the file on L14-15.

[vasild]

Added some comments to make it clearer.

[eriknylund]

Good point. I also don't fully understand what any of these tests are trying to achieve? Wouldn't something like

diff --git a/src/test/base32_tests.cpp b/src/test/base32_tests.cpp
index 26ed78e2c2..4a5af0fd4d 100644
--- a/src/test/base32_tests.cpp
+++ b/src/test/base32_tests.cpp
@@ -23,13 +23,13 @@ BOOST_AUTO_TEST_CASE(base32_testvectors)
 
     // Decoding strings with embedded NUL characters should fail
     bool failure;
-    (void)DecodeBase32(std::string("invalid", 8), &failure);
-    BOOST_CHECK(failure);
-    (void)DecodeBase32(std::string("AWSX3VPP"), &failure);
+    (void)DecodeBase32(std::string("my======", 8), &failure);
     BOOST_CHECK(!failure);
-    (void)DecodeBase32(std::string("AWSX3VPP\0invalid", 16), &failure);
+    (void)DecodeBase32(std::string("my======\0", 9), &failure);
+    BOOST_CHECK(failure);
+    (void)DecodeBase32(std::string("\0my======", 9), &failure);
     BOOST_CHECK(failure);
-    (void)DecodeBase32(std::string("AWSX3VPPinvalid", 16), &failure);
+    (void)DecodeBase32(std::string("my=\0=====", 9), &failure);
     BOOST_CHECK(failure);
 }

do more according to what the comment on L24 implies?

[eriknylund]

ACK b7f1cc6 I've reviewed the changes and I think they are an improvement. I would consider renaming the issue to better reflect that changes that are not only base32 related.

[theStack]

Code Review ACK b7f1cc6 🗄️

[vasild]

Changed to use s-suffix strings. Tested with --enable-c++17.

[laanwj]

ACK ecc6cf1

This very much clarifies the code and works locally for me. I don't know what the travis and CI failures are about.

[practicalswift]

ACK ecc6cf1 modulo happily green CI

Needs rebase to pull in C++17 switch?