test: support std::optional in BOOST_CHECK_* and increase FromUserHex fuzz feature coverage

[l0rinc]

trafficstars

Enhanced FromUserHex coverage by:

• Added std::optional support to BOOST_CHECK_EQUAL, allowing direct comparisons of std::optional<T> with other T expected values.
• Increased fuzz testing for hex parsing to validate against other hex validators and parsers.

---

• Use BOOST_CHECK_EQUAL for https://github.com/bitcoin/bitcoin/pull/30569#discussion_r1706637780 arith_uint256, uint256, uint160

Example error before:

unknown location:0: fatal error: in "validation_chainstatemanager_tests/chainstatemanager_args": std::bad_optional_access: bad_optional_access test/validation_chainstatemanager_tests.cpp:781: last checkpoint

after:

test/validation_chainstatemanager_tests.cpp:801: error: in "validation_chainstatemanager_tests/chainstatemanager_args": check set_opts({"-assumevalid=0"}).assumed_valid_block == uint256::ZERO has failed [std::nullopt != 0000000000000000000000000000000000000000000000000000000000000000]

[DrahtBot]

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
ACK	ryanofsky, hodlinator, stickies-v

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

Conflicts

No conflicts as of last run.

[l0rinc]

TIL: one can compare optional against a value without .value() (C++17 addition).

Yeah, but I don't think that's used here

[hodlinator]

TIL: one can compare optional against a value without .value() (C++17 addition).

Yeah, but I don't think that's used here

It is used in your change from

BOOST_CHECK_EQUAL(uint256::FromUserHex("").value(), uint256::ZERO);

to

BOOST_CHECK_EQUAL(uint256::FromUserHex(""), uint256::ZERO);

It was my obscure way of saying thank you for making me discover that. :)

[l0rinc]

It is used in your change from BOOST_CHECK_EQUAL(uint256::FromUserHex("").value(), uint256::ZERO);

Details

I'm not sure it's related, commenting out:

-inline std::ostream& operator<<(std::ostream& os, const std::nullopt_t)
-{
-    return os << "std::nullopt";
-}
-
-template <typename T>
-inline std::ostream& operator<<(std::ostream& os, const std::optional<T>& v)
-{
-    if (v) {
-        return os << v.value();
-    } else {
-        return os << std::nullopt;
-    }
-}
+//inline std::ostream& operator<<(std::ostream& os, const std::nullopt_t)
+//{
+//    return os << "std::nullopt";
+//}
+//
+//template <typename T>
+//inline std::ostream& operator<<(std::ostream& os, const std::optional<T>& v)
+//{
+//    if (v) {
+//        return os << v.value();
+//    } else {
+//        return os << std::nullopt;
+//    }
+//}

and running

cmake -B build && cmake --build build -j10 && build/src/test/test_bitcoin --run_test=uint256_tests/from_user_hex gives me:

/Applications/Xcode.app/Contents/Developer/Platforms/MacOSX.platform/Developer/SDKs/MacOSX14.5.sdk/usr/include/c++/v1/ostream:268:20: note: candidate function not viable: no known conversion from 'const std::optional<arith_uint256>' to 'nullptr_t' (aka 'std::nullptr_t') for 1st argument
    basic_ostream& operator<<(nullptr_t)
                   ^
4 errors generated.

[hodlinator]

Maybe my communication was not clear enough. There are 2 separate things that you might be conflating?

1. I like that one can compare optional<T> to T as per https://en.cppreference.com/w/cpp/utility/optional/operator_cmp (21-33). That's the BOOST_CHECK_EQUAL(uint256::FromUserHex(""), uint256::ZERO);-example.

2. I don't see why we want to prevent being able to do: BOOST_CHECK_EQUAL(uint256::FromUserHex("Not valid"), std::nullopt); one could do the following as an alternative, but BOOST_CHECK is deprecated: BOOST_CHECK(!uint256::FromUserHex("Not valid"));

Not sure where your nullptr_t error is coming from. I misspelled std::nullopt as nullptr once while attempting to verify 2).

Edit: Hm.. didn't see your subthread comment in https://github.com/bitcoin/bitcoin/pull/30618#discussion_r1740911993 before writing the above. Still don't understand the nullptr_t error but I'm okay with you not breaking out a function to handle nullopt_t until we find it necessary.

[l0rinc]

2. but BOOST_CHECK is deprecated

I don't see the deprecation, I think it's still useful for truthy checks (e.g. empty optionals).

one can compare optional against a value without .value() (C++17 addition).

Yes, you're right here, I wrongly assumed some BOOST magic was done in this case instead.

[hodlinator]

2. but BOOST_CHECK is deprecated

I don't see the deprecation, I think it's still useful for truthy checks (e.g. empty optionals).

It seems to be poorly documented, but if you search the header for "DEPRECATED TOOLS" you'll find it.

[l0rinc]

Thanks for the thorough review @hodlinator and @stickies-v, I pushed another batch (first an empty rebase), followed by https://github.com/bitcoin/bitcoin/compare/c6f9add458bb8e142d8ccfe5c661ec56143e3f31..6fa4c92e4c9bf5ea90e25d41b525c88f9c2d834d

Edit:

we're not increasing FromUserHex fuzz coverage imo

The line coverage may be the same, but it's compared against other ones, so it's stricter, i.e. more features are covered. I've update the PR description and title, thanks.

helpful but outdated (set_opts doesn't exist)

Not sure that's important, but if I edit again, I'll update it

[l0rinc]

Could add something like this, but no strong view

Done, thanks!

[stickies-v]

re-ACK 4ac1981d338f89ed2b814aedaebe81783293e6f5

[stickies-v]

rebase re-ACK 6da47f6ba5450e7f2b2ab2a64c7cfd354f67a1a9, CI timeout failure seems unrelated

[stickies-v]

re-ACK 1eac96a503b6bac3eaf5d0eb3d23ffde3bfbf9aa

CI timeout issues seem unrelated.