Merge bitcoin/bitcoin#30618: test: support std::optional in BOOST_CHECK_* and increase FromUserHex fuzz feature coverage

ryanofsky · ryanofsky · commit be768dbd185b · 2024-09-12T12:36:37.000-04:00
1eac96a Compare FromUserHex result against other hex validators and parsers (Lőrinc) 1994786 Use BOOST_CHECK_EQUAL for optional, arith_uint256, uint256, uint160 (Lőrinc) 743ac30 Add std::optional support to Boost's equality check (Lőrinc) Pull request description: 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 bitcoin/bitcoin#30569 (comment) 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] ACKs for top commit: stickies-v: re-ACK 1eac96a ryanofsky: Code review ACK 1eac96a. Only changes since last review were auto type and fuzz test tweaks. hodlinator: ACK 1eac96a Tree-SHA512: f1d2c65f0ee4e97830700be5b330189207b11ed0c89a8cebf0f97d43308402a6b3732e10130c79a0c044f7d2eeabfb5359990825aadf02c4ec19428dcd982b00
diff --git a/src/test/fuzz/hex.cpp b/src/test/fuzz/hex.cpp
@@ -35,7 +35,13 @@ FUZZ_TARGET(hex)
         assert(uint256::FromUserHex(random_hex_string));
     }
     if (const auto result{uint256::FromUserHex(random_hex_string)}) {
-        assert(uint256::FromHex(result->ToString()));
+        const auto result_string{result->ToString()}; // ToString() returns a fixed-length string without "0x" prefix
+        assert(result_string.length() == 64);
+        assert(IsHex(result_string));
+        assert(TryParseHex(result_string));
+        assert(Txid::FromHex(result_string));
+        assert(Wtxid::FromHex(result_string));
+        assert(uint256::FromHex(result_string));
     }
     try {
         (void)HexToPubKey(random_hex_string);
diff --git a/src/test/uint256_tests.cpp b/src/test/uint256_tests.cpp
@@ -299,15 +299,15 @@ BOOST_AUTO_TEST_CASE(from_hex)
 
 BOOST_AUTO_TEST_CASE(from_user_hex)
 {
-    BOOST_CHECK_EQUAL(uint256::FromUserHex("").value(), uint256::ZERO);
-    BOOST_CHECK_EQUAL(uint256::FromUserHex("0x").value(), uint256::ZERO);
-    BOOST_CHECK_EQUAL(uint256::FromUserHex("0").value(), uint256::ZERO);
-    BOOST_CHECK_EQUAL(uint256::FromUserHex("00").value(), uint256::ZERO);
-    BOOST_CHECK_EQUAL(uint256::FromUserHex("1").value(), uint256::ONE);
-    BOOST_CHECK_EQUAL(uint256::FromUserHex("0x10").value(), uint256{0x10});
-    BOOST_CHECK_EQUAL(uint256::FromUserHex("10").value(), uint256{0x10});
-    BOOST_CHECK_EQUAL(uint256::FromUserHex("0xFf").value(), uint256{0xff});
-    BOOST_CHECK_EQUAL(uint256::FromUserHex("Ff").value(), uint256{0xff});
+    BOOST_CHECK_EQUAL(uint256::FromUserHex(""), uint256::ZERO);
+    BOOST_CHECK_EQUAL(uint256::FromUserHex("0x"), uint256::ZERO);
+    BOOST_CHECK_EQUAL(uint256::FromUserHex("0"), uint256::ZERO);
+    BOOST_CHECK_EQUAL(uint256::FromUserHex("00"), uint256::ZERO);
+    BOOST_CHECK_EQUAL(uint256::FromUserHex("1"), uint256::ONE);
+    BOOST_CHECK_EQUAL(uint256::FromUserHex("0x10"), uint256{0x10});
+    BOOST_CHECK_EQUAL(uint256::FromUserHex("10"), uint256{0x10});
+    BOOST_CHECK_EQUAL(uint256::FromUserHex("0xFf"), uint256{0xff});
+    BOOST_CHECK_EQUAL(uint256::FromUserHex("Ff"), uint256{0xff});
     const std::string valid_hex_64{"0x0123456789abcdef0123456789abcdef0123456789ABDCEF0123456789ABCDEF"};
     BOOST_REQUIRE_EQUAL(valid_hex_64.size(), 2 + 64); // 0x prefix and 64 hex digits
     BOOST_CHECK_EQUAL(uint256::FromUserHex(valid_hex_64.substr(2)).value().ToString(), ToLower(valid_hex_64.substr(2)));
@@ -334,7 +334,7 @@ BOOST_AUTO_TEST_CASE( check_ONE )
 
 BOOST_AUTO_TEST_CASE(FromHex_vs_uint256)
 {
-    auto runtime_uint{uint256::FromHex("4A5E1E4BAAB89F3A32518A88C31BC87F618f76673e2cc77ab2127b7afdeda33b").value()};
+    auto runtime_uint{uint256::FromHex("4A5E1E4BAAB89F3A32518A88C31BC87F618f76673e2cc77ab2127b7afdeda33b")};
     constexpr uint256 consteval_uint{  "4a5e1e4baab89f3a32518a88c31bc87f618f76673e2cc77ab2127b7afdeda33b"};
     BOOST_CHECK_EQUAL(consteval_uint, runtime_uint);
 }
diff --git a/src/test/util/setup_common.cpp b/src/test/util/setup_common.cpp
@@ -78,24 +78,6 @@ constexpr inline auto TEST_DIR_PATH_ELEMENT{"test_common bitcoin"}; // Includes
 /** Random context to get unique temp data dirs. Separate from m_rng, which can be seeded from a const env var */
 static FastRandomContext g_rng_temp_path;
 
-std::ostream& operator<<(std::ostream& os, const arith_uint256& num)
-{
-    os << num.ToString();
-    return os;
-}
-
-std::ostream& operator<<(std::ostream& os, const uint160& num)
-{
-    os << num.ToString();
-    return os;
-}
-
-std::ostream& operator<<(std::ostream& os, const uint256& num)
-{
-    os << num.ToString();
-    return os;
-}
-
 struct NetworkSetup
 {
     NetworkSetup()
@@ -606,3 +588,18 @@ CBlock getBlock13b8a()
     stream >> TX_WITH_WITNESS(block);
     return block;
 }
+
+std::ostream& operator<<(std::ostream& os, const arith_uint256& num)
+{
+    return os << num.ToString();
+}
+
+std::ostream& operator<<(std::ostream& os, const uint160& num)
+{
+    return os << num.ToString();
+}
+
+std::ostream& operator<<(std::ostream& os, const uint256& num)
+{
+    return os << num.ToString();
+}
diff --git a/src/test/util/setup_common.h b/src/test/util/setup_common.h
@@ -10,6 +10,8 @@
 #include <key.h>
 #include <node/caches.h>
 #include <node/context.h> // IWYU pragma: export
+#include <optional>
+#include <ostream>
 #include <primitives/transaction.h>
 #include <pubkey.h>
 #include <stdexcept>
@@ -29,6 +31,8 @@ class arith_uint256;
 class CFeeRate;
 class Chainstate;
 class FastRandomContext;
+class uint160;
+class uint256;
 
 /** This is connected to the logger. Can be used to redirect logs to any other log */
 extern const std::function<void(const std::string&)> G_TEST_LOG_FUN;
@@ -39,15 +43,6 @@ extern const std::function<std::vector<const char*>()> G_TEST_COMMAND_LINE_ARGUM
 /** Retrieve the unit test name. */
 extern const std::function<std::string()> G_TEST_GET_FULL_NAME;
 
-// Enable BOOST_CHECK_EQUAL for enum class types
-namespace std {
-template <typename T>
-std::ostream& operator<<(typename std::enable_if<std::is_enum<T>::value, std::ostream>::type& stream, const T& e)
-{
-    return stream << static_cast<typename std::underlying_type<T>::type>(e);
-}
-} // namespace std
-
 static constexpr CAmount CENT{1000000};
 
 struct TestOpts {
@@ -250,10 +245,26 @@ std::unique_ptr<T> MakeNoLogFileContext(const ChainType chain_type = ChainType::
 
 CBlock getBlock13b8a();
 
-// Make types usable in BOOST_CHECK_*
+// Make types usable in BOOST_CHECK_* @{
+namespace std {
+template <typename T> requires std::is_enum_v<T>
+inline std::ostream& operator<<(std::ostream& os, const T& e)
+{
+    return os << static_cast<std::underlying_type_t<T>>(e);
+}
+
+template <typename T>
+inline std::ostream& operator<<(std::ostream& os, const std::optional<T>& v)
+{
+    return v ? os << *v
+             : os << "std::nullopt";
+}
+} // namespace std
+
 std::ostream& operator<<(std::ostream& os, const arith_uint256& num);
 std::ostream& operator<<(std::ostream& os, const uint160& num);
 std::ostream& operator<<(std::ostream& os, const uint256& num);
+// @}
 
 /**
  * BOOST_CHECK_EXCEPTION predicates to check the specific validation error.
diff --git a/src/test/validation_chainstatemanager_tests.cpp b/src/test/validation_chainstatemanager_tests.cpp
@@ -806,22 +806,26 @@ BOOST_FIXTURE_TEST_CASE(chainstatemanager_args, BasicTestingSetup)
     };
 
     // test -assumevalid
-    BOOST_CHECK(!get_valid_opts({}).assumed_valid_block.has_value());
-    BOOST_CHECK(get_valid_opts({"-assumevalid="}).assumed_valid_block.value().IsNull());
-    BOOST_CHECK(get_valid_opts({"-assumevalid=0"}).assumed_valid_block.value().IsNull());
-    BOOST_CHECK(get_valid_opts({"-noassumevalid"}).assumed_valid_block.value().IsNull());
-    BOOST_CHECK_EQUAL(get_valid_opts({"-assumevalid=0x1234"}).assumed_valid_block.value().ToString(), std::string(60, '0') + "1234");
-    const std::string cmd{"-assumevalid=0123456789abcdef0123456789abcdef0123456789abcdef0123456789abcdef"};
-    BOOST_CHECK_EQUAL(get_valid_opts({cmd.c_str()}).assumed_valid_block.value().ToString(), cmd.substr(13, cmd.size()));
+    BOOST_CHECK(!get_valid_opts({}).assumed_valid_block);
+    BOOST_CHECK_EQUAL(get_valid_opts({"-assumevalid="}).assumed_valid_block, uint256::ZERO);
+    BOOST_CHECK_EQUAL(get_valid_opts({"-assumevalid=0"}).assumed_valid_block, uint256::ZERO);
+    BOOST_CHECK_EQUAL(get_valid_opts({"-noassumevalid"}).assumed_valid_block, uint256::ZERO);
+    BOOST_CHECK_EQUAL(get_valid_opts({"-assumevalid=0x12"}).assumed_valid_block, uint256{0x12});
+
+    std::string assume_valid{"0123456789abcdef0123456789abcdef0123456789abcdef0123456789abcdef"};
+    BOOST_CHECK_EQUAL(get_valid_opts({("-assumevalid=" + assume_valid).c_str()}).assumed_valid_block, uint256::FromHex(assume_valid));
 
     BOOST_CHECK(!get_opts({"-assumevalid=xyz"}));                                                               // invalid hex characters
     BOOST_CHECK(!get_opts({"-assumevalid=01234567890123456789012345678901234567890123456789012345678901234"})); // > 64 hex chars
 
     // test -minimumchainwork
-    BOOST_CHECK(!get_valid_opts({}).minimum_chain_work.has_value());
-    BOOST_CHECK_EQUAL(get_valid_opts({"-minimumchainwork=0"}).minimum_chain_work.value().GetCompact(), 0U);
-    BOOST_CHECK_EQUAL(get_valid_opts({"-nominimumchainwork"}).minimum_chain_work.value().GetCompact(), 0U);
-    BOOST_CHECK_EQUAL(get_valid_opts({"-minimumchainwork=0x1234"}).minimum_chain_work.value().GetCompact(), 0x02123400U);
+    BOOST_CHECK(!get_valid_opts({}).minimum_chain_work);
+    BOOST_CHECK_EQUAL(get_valid_opts({"-minimumchainwork=0"}).minimum_chain_work, arith_uint256());
+    BOOST_CHECK_EQUAL(get_valid_opts({"-nominimumchainwork"}).minimum_chain_work, arith_uint256());
+    BOOST_CHECK_EQUAL(get_valid_opts({"-minimumchainwork=0x1234"}).minimum_chain_work, arith_uint256{0x1234});
+
+    std::string minimum_chainwork{"0123456789abcdef0123456789abcdef0123456789abcdef0123456789abcdef"};
+    BOOST_CHECK_EQUAL(get_valid_opts({("-minimumchainwork=" + minimum_chainwork).c_str()}).minimum_chain_work, UintToArith256(uint256::FromHex(minimum_chainwork).value()));
 
     BOOST_CHECK(!get_opts({"-minimumchainwork=xyz"}));                                                               // invalid hex characters
     BOOST_CHECK(!get_opts({"-minimumchainwork=01234567890123456789012345678901234567890123456789012345678901234"})); // > 64 hex chars
