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

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

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?

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.

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

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.

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

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

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

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.

Added some comments to make it clearer.

Maybe we can wait with making this change for the C++17 switch? We'll have the s-suffix strings, so don't need to worry about specifying the length explicitly at all anymore even when there are trailing '\0' characters.

#include <iostream>
#include <string>

int main()
{
    using namespace std::string_literals;

    std::string x = "test\x00\x00\x00\x00"s;
    std::cout << x.size() << std::endl; // prints 8
}

I think this PR is good as is, but if it is still unmerged when we start allowing C++17, then I will update it to use "..."s;

then I will update it to use "..."s;

Even though this has a few ACKs, I think you can go ahead and update it to use the s-suffix strings. We'll merge this post branch off when requiring C++17.

Done

This change strikes me as odd, shouldn't it be "\01"s or did you intentionally want the other approach here to improve the test?

I guess the intention of std::string("\01", 2) was to test the string {0, 49} (ASCII code of '1' is 49). But this is not what it did because "\01" is the same as "\1" and so the resulting string is {1, 0} (1 from \1 and the terminating nul).