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

l0rinc commented at 1:40 PM on August 9, 2024: contributor

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 #30569 (review) 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 commented at 1:40 PM on August 9, 2024: contributor

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.

DrahtBot added the label Tests on Aug 9, 2024

l0rinc force-pushed on Aug 9, 2024

l0rinc renamed this:
~~test: TrySanitizeHexNumber fizz and unit testing coverage~~
test: TrySanitizeHexNumber FUZZ and unit testing coverage
on Aug 9, 2024

l0rinc force-pushed on Aug 9, 2024

DrahtBot added the label Needs rebase on Aug 27, 2024

l0rinc force-pushed on Aug 28, 2024

l0rinc renamed this:
~~test: TrySanitizeHexNumber FUZZ and unit testing coverage~~
test: increase FromUserHex FUZZ and unit testing coverage
on Aug 28, 2024

DrahtBot removed the label Needs rebase on Aug 28, 2024

DrahtBot added the label CI failed on Aug 29, 2024

l0rinc force-pushed on Aug 29, 2024

l0rinc marked this as ready for review on Aug 29, 2024

DrahtBot removed the label CI failed on Aug 29, 2024

DrahtBot added the label Needs rebase on Aug 31, 2024

l0rinc force-pushed on Aug 31, 2024

DrahtBot removed the label Needs rebase on Aug 31, 2024

in src/test/uint256_tests.cpp:20 in f6c0d27a3c outdated

  16 | @@ -17,6 +17,8 @@
  17 |  #include <string_view>
  18 |  #include <vector>
  19 |  
  20 | +using std::literals::string_literals::operator""s;

hodlinator commented at 7:40 PM on August 31, 2024:

Why be so specific? Found 14 prior occurrences of:

using namespace std::literals;

l0rinc commented at 1:24 PM on September 1, 2024:

done, thanks

in src/test/util/setup_common.h:58 in f6c0d27a3c outdated

  55 | +inline std::ostream& operator<<(std::ostream& os, const std::optional<T>& v)
  56 | +{
  57 | +    if (v) {
  58 | +        return os << v.value();
  59 | +    } else {
  60 | +        return os << "std::nullopt";

hodlinator commented at 7:45 PM on August 31, 2024:

Why not also bring over

inline std::ostream& operator<<(std::ostream& os, const std::nullopt_t)

from #16545 for completeness and to use here, instead of this special case?

l0rinc commented at 12:52 PM on September 1, 2024:

This seemed like a simplification to me, what would be the advantage of that?

l0rinc commented at 1:24 PM on September 1, 2024:

Done, but I don't yet see what this adds exactly

hodlinator commented at 12:01 PM on September 2, 2024:

Being able to write BOOST_CHECK_EQUALS(..., std::nullopt);, in case one wanted to.

l0rinc commented at 1:11 PM on September 2, 2024:

but we're never writing optionals on the right side here, i.e.

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

can simply be either:

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

or if empty:

BOOST_CHECK(!uint256::FromUserHex(""));

l0rinc commented at 1:15 PM on September 2, 2024:

Removed the nullopt overwrite and explained the usage in the comment, thanks for explaining your view.

stickies-v commented at 2:53 PM on September 3, 2024:

I think I agree that until we have use case (I can't think of one atm) to compare with std::nullopt, forcing the user to use BOOST_CHECK makes sense.

In commit e0a96acc82c2c918572a2c9b90431db5bca19174:

Note that we're never writing optionals on the right side, e.g. BOOST_CHECK_EQUAL(uint256::FromUserHex(""), std::make_optional(uint256::ZERO)); since they can simply be either: BOOST_CHECK_EQUAL(uint256::FromUserHex(""), uint256::ZERO); or if empty: BOOST_CHECK(!uint256::FromUserHex(""));

I think this can be dropped from the commit message? It's possible, and there are good use cases for putting an optional on the right side, e.g. here). You could add a note to the developer notes on this, but I don't think that's necessary either.

l0rinc commented at 3:23 PM on September 3, 2024:

Wouldn't

        BOOST_CHECK_EQUAL(from_hex.has_value(), parse_hash_v.has_value());
        if (!from_hex.has_value()) continue;
        BOOST_CHECK_EQUAL(from_hex.value(), parse_hash_v.value());

be simpler after this change as just directly comparing the two?

        BOOST_CHECK_EQUAL(from_hex, parse_hash_v);

?

stickies-v commented at 3:35 PM on September 3, 2024:

Yes, in which case we'd be putting an optional (variable) on the right hand side, which goes against what you're saying in the commit message. That's why I suggest removing it, it doesn't help with reviewing this commit imo. It only adds context on why it doesn't do something that was done in another commit (that's not in the tree).

l0rinc commented at 3:41 PM on September 3, 2024:

I think this can be dropped from the commit message

Done, your right, the formulation wasn't the best

in src/test/util/setup_common.h:256 in f6c0d27a3c outdated

 279 | @@ -250,11 +280,6 @@ std::unique_ptr<T> MakeNoLogFileContext(const ChainType chain_type = ChainType::
 280 |  
 281 |  CBlock getBlock13b8a();
 282 |  
 283 | -// Make types usable in BOOST_CHECK_*
 284 | -std::ostream& operator<<(std::ostream& os, const arith_uint256& num);
 285 | -std::ostream& operator<<(std::ostream& os, const uint160& num);
 286 | -std::ostream& operator<<(std::ostream& os, const uint256& num);

hodlinator commented at 9:45 PM on August 31, 2024:

Good to group the output stream operators, but better to put them down here.

They are arguably less important (enabling Boost tests to work rather than defining new types like ChainTestingSetup) and puts them next to BOOST_CHECK_EXCEPTION-related code below.

l0rinc commented at 1:24 PM on September 1, 2024:

Done

in src/test/util/setup_common.h:78 in a42afbdcfe outdated

  75 | +
  76 | +// Enable BOOST_CHECK_EQUAL for uint160
  77 | +inline std::ostream& operator<<(std::ostream& os, const uint160& num)
  78 | +{
  79 | +    return os << num.ToString();
  80 | +}

hodlinator commented at 10:03 PM on August 31, 2024:

Why:

Put these 3 inside namespace std?
inline these non-templated functions? I know one day we will have modules and be happy without .H/CPP distinction, but until then, let us try to keep headers lean. Especially when the coding style mandates minimum 4 lines per function implementation, and an #include needs adding.

l0rinc commented at 1:24 PM on September 1, 2024:

Fair, thanks, pushed

hodlinator commented at 10:16 PM on August 31, 2024: contributor

Concept ACK a42afbdcfe02b2a872fe9bfa6cd98287b1fa9a50

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

<details> <summary> Would suggest something closer to this: </summary>

diff --git a/src/test/util/setup_common.h b/src/test/util/setup_common.h
index 6c1b2a57f1..d1eae29003 100644
--- a/src/test/util/setup_common.h
+++ b/src/test/util/setup_common.h
@@ -5,7 +5,6 @@
 #ifndef BITCOIN_TEST_UTIL_SETUP_COMMON_H
 #define BITCOIN_TEST_UTIL_SETUP_COMMON_H
 
-#include <arith_uint256.h>
 #include <common/args.h> // IWYU pragma: export
 #include <kernel/context.h>
 #include <key.h>
@@ -40,44 +39,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;
 
-namespace std {
-// Enable BOOST_CHECK_EQUAL for enum class types
-template <typename T>
-inline 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);
-}
-
-// Enable BOOST_CHECK_EQUAL for std::optional
-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";
-    }
-}
-
-// Enable BOOST_CHECK_EQUAL for arith_uint256
-inline std::ostream& operator<<(std::ostream& os, const arith_uint256& num)
-{
-    return os << num.ToString();
-}
-
-// Enable BOOST_CHECK_EQUAL for uint256
-inline std::ostream& operator<<(std::ostream& os, const uint256& num)
-{
-    return os << num.ToString();
-}
-
-// Enable BOOST_CHECK_EQUAL for uint160
-inline std::ostream& operator<<(std::ostream& os, const uint160& num)
-{
-    return os << num.ToString();
-}
-} // namespace std
-
 static constexpr CAmount CENT{1000000};
 
 struct TestOpts {
@@ -280,6 +241,36 @@ std::unique_ptr<T> MakeNoLogFileContext(const ChainType chain_type = ChainType::
 
 CBlock getBlock13b8a();
 
+// Make types usable in BOOST_CHECK_* {
+namespace std {
+// Enum class types
+template <typename T>
+inline 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);
+}
+
+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;
+    }
+}
+} // namespace std
+
+std::ostream& operator<<(std::ostream& os, const arith_uint256& num);
+std::ostream& operator<<(std::ostream& os, const uint256& num);
+std::ostream& operator<<(std::ostream& os, const uint160& num);
+// Make types usable in BOOST_CHECK_* }
+
 /**
  * BOOST_CHECK_EXCEPTION predicates to check the specific validation error.
  * Use as

</details>

l0rinc force-pushed on Sep 1, 2024

l0rinc commented at 1:41 PM on September 1, 2024: contributor

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

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

l0rinc force-pushed on Sep 1, 2024

hodlinator commented at 12:06 PM on September 2, 2024: contributor

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 commented at 1:07 PM on September 2, 2024: contributor

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

<details> <summary>Details</summary>

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.

</details>

l0rinc force-pushed on Sep 2, 2024

hodlinator commented at 8:23 PM on September 2, 2024: contributor

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

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.
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 #30618 (review) 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 commented at 10:33 AM on September 3, 2024: contributor

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 commented at 12:45 PM on September 3, 2024: contributor

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.

in src/test/util/setup_common.h:254 in e0a96acc82 outdated

 247 | +// Enum class types
 248 | +template <typename T>
 249 | +inline std::ostream& operator<<(typename std::enable_if<std::is_enum<T>::value, std::ostream>::type& stream, const T& e)
 250 | +{
 251 | +    return stream << static_cast<typename std::underlying_type<T>::type>(e);
 252 | +}

stickies-v commented at 1:10 PM on September 3, 2024:

This can be cleaned up in C++20:

diff --git a/src/test/util/setup_common.h b/src/test/util/setup_common.h
index cea1481c88..0a65726395 100644
--- a/src/test/util/setup_common.h
+++ b/src/test/util/setup_common.h
@@ -243,11 +243,10 @@ CBlock getBlock13b8a();
 
 // Make types usable in BOOST_CHECK_* {
 namespace std {
-// Enum class types
-template <typename T>
-inline std::ostream& operator<<(typename std::enable_if<std::is_enum<T>::value, std::ostream>::type& stream, const T& e)
+template <typename T> requires std::is_enum_v<T>
+inline std::ostream& operator<<(std::ostream& stream, const T& e)
 {
-    return stream << static_cast<typename std::underlying_type<T>::type>(e);
+    return stream << static_cast<std::underlying_type_t<T>>(e);
 }
 
 template <typename T>

</details>

l0rinc commented at 3:45 PM on September 3, 2024:

nice! changed

in src/test/util/setup_common.h:261 in e0a96acc82 outdated

 257 | +    if (v) {
 258 | +        return os << v.value();
 259 | +    } else {
 260 | +        return os << "std::nullopt";
 261 | +    }
 262 | +}

stickies-v commented at 1:18 PM on September 3, 2024:

nit: can be onelined to return os << v ? *v : "std::nullopt";:

diff --git a/src/test/util/setup_common.h b/src/test/util/setup_common.h
index cea1481c88..22597f16c7 100644
--- a/src/test/util/setup_common.h
+++ b/src/test/util/setup_common.h
@@ -253,11 +253,7 @@ inline std::ostream& operator<<(typename std::enable_if<std::is_enum<T>::value,
 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";
-    }
+    return os << v ? *v : "std::nullopt";
 }
 } // namespace std

</details>

l0rinc commented at 3:08 PM on September 3, 2024:

Yeah, I tried that before, but getting:

error: incompatible operand types ('const value_type' (aka 'const uint256') and 'const char[13]') return os << v ? *v : "std::nullopt";

Edit: changed it to:

return os << (v ? v->ToString() : "std::nullopt");

which produces

src/test/uint256_tests.cpp:400: error: in "uint256_tests/from_user_hex": check uint256::FromUserHex("") == uint256::ONE has failed [0000000000000000000000000000000000000000000000000000000000000000 != 0000000000000000000000000000000000000000000000000000000000000001] for

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

stickies-v commented at 3:13 PM on September 3, 2024:

Sorry you're right, return v ? os << *v : os << "std::nullopt"; should work though.

l0rinc commented at 3:45 PM on September 3, 2024:

Used the version in my edited comment above

stickies-v commented at 3:51 PM on September 3, 2024:

I don't think requiring v to have implemented ToString() is a good approach, it seems unnecessarily strict. Do you see any issues with return v ? os << *v : os << "std::nullopt";?

l0rinc commented at 3:56 PM on September 3, 2024:

I'm fine with that as well, pushed.

in src/test/util/setup_common.h:244 in e0a96acc82 outdated

 240 | @@ -250,10 +241,30 @@ std::unique_ptr<T> MakeNoLogFileContext(const ChainType chain_type = ChainType::
 241 |  
 242 |  CBlock getBlock13b8a();
 243 |  
 244 | -// Make types usable in BOOST_CHECK_*
 245 | +// Make types usable in BOOST_CHECK_* {

stickies-v commented at 1:23 PM on September 3, 2024:

nit: I think the @{ notation is more common here?

diff --git a/src/test/util/setup_common.h b/src/test/util/setup_common.h
index cea1481c88..96c3b5caac 100644
--- a/src/test/util/setup_common.h
+++ b/src/test/util/setup_common.h
@@ -241,7 +241,8 @@ 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 {
 // Enum class types
 template <typename T>
@@ -264,7 +265,7 @@ inline std::ostream& operator<<(std::ostream& os, const std::optional<T>& v)
 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);
-// Make types usable in BOOST_CHECK_* }
+// @}
 
 /**
  * BOOST_CHECK_EXCEPTION predicates to check the specific validation error.

</details>

l0rinc commented at 3:44 PM on September 3, 2024:

Used the @{ and @} formats

hodlinator commented at 10:32 PM on September 3, 2024:

@{ is Doxygen grouping syntax. I was trying to make it work properly in generated documentation using @name but the namespace std parts were always being extracted, so opted for vanilla comment style.

in src/test/util/setup_common.h:257 in 50defb82c5 outdated

 250 | +{
 251 | +    return stream << static_cast<typename std::underlying_type<T>::type>(e);
 252 | +}
 253 | +
 254 | +template <typename T>
 255 | +inline std::ostream& operator<<(std::ostream& os, const std::optional<T>& v)

stickies-v commented at 1:31 PM on September 3, 2024:

nit: missing optional and ostream includes

/ci_container_base/src/test/util/setup_common.h should add these lines:
#include <stddef.h>                  // for size_t
#include <stdint.h>                  // for uint32_t
#include <exception>                 // for exception
#include <memory>                    // for make_unique, unique_ptr
#include <optional>                  // for optional
#include <ostream>                   // for ostream, operator<<, basic_ios, basic_ostream
#include <utility>                   // for pair
#include "common/args.h"
#include "consensus/amount.h"        // for CAmount, COIN
#include "node/context.h"
#include "random.h"                  // for FastRandomContext, GetRandHash
#include "util/chaintype.h"          // for ChainType (ptr only)
class uint160;
class uint256;

/ci_container_base/src/test/util/setup_common.h should remove these lines:
- #include <kernel/context.h>  // lines 9-9
- #include <pubkey.h>  // lines 14-14
- #include <util/check.h>  // lines 18-18
- #include <stdexcept>  // lines 15-15
- class FastRandomContext;  // lines 31-31
- struct CMutableTransaction;  // lines 109-109

The full include-list for /ci_container_base/src/test/util/setup_common.h:
#include <common/args.h>             // for ArgsManager
#include <key.h>                     // for CKey
#include <node/caches.h>             // for CacheSizes
#include <node/context.h>            // for NodeContext
#include <primitives/transaction.h>  // for CTransactionRef, CMutableTransaction
#include <stddef.h>                  // for size_t
#include <stdint.h>                  // for uint32_t
#include <test/util/random.h>        // for SeedRandomStateForTest, SeedRand
#include <util/chaintype.h>          // for ChainType
#include <util/fs.h>                 // for path
#include <util/signalinterrupt.h>    // for SignalInterrupt
#include <util/string.h>             // for string, basic_string<>::npos
#include <util/vector.h>             // for Cat
#include <exception>                 // for exception
#include <functional>                // for function
#include <memory>                    // for make_unique, unique_ptr
#include <optional>                  // for optional
#include <ostream>                   // for ostream, operator<<, basic_ios, basic_ostream
#include <type_traits>               // for enable_if, is_enum, underlying_type
#include <utility>                   // for pair
#include <vector>                    // for vector
#include "common/args.h"
#include "consensus/amount.h"        // for CAmount, COIN
#include "node/context.h"
#include "random.h"                  // for FastRandomContext, GetRandHash
#include "util/chaintype.h"          // for ChainType (ptr only)
class CBlock;  // lines 108-108
class CFeeRate;  // lines 29-29
class CScript;  // lines 110-110
class Chainstate;  // lines 30-30
class arith_uint256;  // lines 28-28
class uint160;
class uint256;

</details>

l0rinc commented at 3:41 PM on September 3, 2024:

The iwyu changes were a bit excessive, but kept the ones that are related to this PR, thanks!

in src/test/util/setup_common.cpp:81 in e0a96acc82 outdated

  72 | @@ -79,24 +73,6 @@ const std::function<std::string(const char*)> G_TRANSLATION_FUN = nullptr;
  73 |  /** Random context to get unique temp data dirs. Separate from m_rng, which can be seeded from a const env var */
  74 |  static FastRandomContext g_rng_temp_path;
  75 |  
  76 | -std::ostream& operator<<(std::ostream& os, const arith_uint256& num)

stickies-v commented at 1:45 PM on September 3, 2024:

e0a96acc82c2c918572a2c9b90431db5bca19174: I don't really see the purpose of this ~move-only change and would rather this file can be dropped from the diff entirely (the include updates feel rather arbitrary so I'm okay with them being dropped too)

I don't think moving these functions is particularly helpful in the header either but can somewhat see the rationale since you're adding new code to it (still prefer it wouldn't move, though), but here it just feels like unnecessary churn?

l0rinc commented at 3:25 PM on September 3, 2024:

was specifically requested to avoid repetition and since they're not mission-critical code: #30618 (review)

stickies-v commented at 3:36 PM on September 3, 2024:

My comment is about the .cpp changes though (I wanted to mention I don't love, but am okay with, the header move changes).

l0rinc commented at 3:43 PM on September 3, 2024:

Pushed some changes, let me know if it's any better, or if you insist on placing them in the cpp file in a different place than the header (with some explanations)

stickies-v commented at 3:58 PM on September 3, 2024:

I don't insist on placing them in a different place than in the header file, I just don't think this is worth the noise and bigger diff - I don't think anyone uses a declaration's location in the header to find the implementation in the cpp file?

l0rinc commented at 4:01 PM on September 3, 2024:

I think it makes it slightly more aligned with the header file, while the diff isn't a lot heavier. What do you think @hodlinator?

hodlinator commented at 10:48 PM on September 3, 2024:

Was under the impression that Git had some smarts when it came to moving lines around, but at least naive git blame usage shows your name & commit on all the moved lines in the .cpp. It's straightforward code (going back in history is less necessary), and I understand your attempt to mirror the ordering from the .h file.

I think the ordering I pushed for in the .h file should stay either way, but maybe go with the more conservative approach in the .cpp as a compromise.

l0rinc commented at 1:58 PM on September 4, 2024:

Git blame without proper history is useless anyway, I always needed a bisect to see the real author, so I don't really see the advantage of optimizing for that. But mirroring the header file's structure does simplify the code a bit. Let me know if you feel strongly about it and I'll revert, but I would consider that a step back.

stickies-v commented at 4:54 PM on September 4, 2024:

No I don't feel strongly about it, I just think this change is not nearly useful enough to warrant an extra 33LoC diff. Anyway, not worth arguing over, like I said it's not blocking.

in src/test/validation_chainstatemanager_tests.cpp:25 in 50defb82c5 outdated

  19 | @@ -22,15 +20,14 @@
  20 |  #include <validation.h>
  21 |  #include <validationinterface.h>
  22 |  
  23 | -#include <tinyformat.h>

stickies-v commented at 2:00 PM on September 3, 2024:

What's the rationale behind these include updates? According to iwyu this include is still necessary. More generally, I think it's most productive to either fix all the includes, or the ones that are affected by the PR, but not a (seemingly random) subset?

l0rinc commented at 3:39 PM on September 3, 2024:

The IDE marked these as unused, but since you say there's logic behind the current structure, I've reverted them

in src/test/validation_chainstatemanager_tests.cpp:810 in 50defb82c5 outdated

 812 | -    BOOST_CHECK_EQUAL(get_valid_opts({cmd.c_str()}).assumed_valid_block.value().ToString(), cmd.substr(13, cmd.size()));
 813 | +    BOOST_CHECK(!get_valid_opts({}).assumed_valid_block);
 814 | +    BOOST_CHECK_EQUAL(get_valid_opts({"-assumevalid="}).assumed_valid_block, uint256::ZERO);
 815 | +    BOOST_CHECK_EQUAL(get_valid_opts({"-assumevalid=0"}).assumed_valid_block, uint256::ZERO);
 816 | +    BOOST_CHECK_EQUAL(get_valid_opts({"-noassumevalid"}).assumed_valid_block, uint256::ZERO);
 817 | +    BOOST_CHECK_EQUAL(get_valid_opts({"-assumevalid=0x1234"}).assumed_valid_block, uint256::FromHex("0000000000000000000000000000000000000000000000000000000000001234"));

stickies-v commented at 2:16 PM on September 3, 2024:

nit: could use the consteval uint256 hex string ctor here (+ below)

    BOOST_CHECK_EQUAL(get_valid_opts({"-assumevalid=0x1234"}).assumed_valid_block, uint256{"0000000000000000000000000000000000000000000000000000000000001234"});

diff --git a/src/test/validation_chainstatemanager_tests.cpp b/src/test/validation_chainstatemanager_tests.cpp
index 3914deb926..e9d9fee1fe 100644
--- a/src/test/validation_chainstatemanager_tests.cpp
+++ b/src/test/validation_chainstatemanager_tests.cpp
@@ -807,7 +807,7 @@ BOOST_FIXTURE_TEST_CASE(chainstatemanager_args, BasicTestingSetup)
     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=0x1234"}).assumed_valid_block, uint256::FromHex("0000000000000000000000000000000000000000000000000000000000001234"));
+    BOOST_CHECK_EQUAL(get_valid_opts({"-assumevalid=0x1234"}).assumed_valid_block, uint256{"0000000000000000000000000000000000000000000000000000000000001234"});
     auto assume_valid{"0123456789abcdef0123456789abcdef0123456789abcdef0123456789abcdef"s};
     BOOST_CHECK_EQUAL(get_valid_opts({("-assumevalid=" + assume_valid).c_str()}).assumed_valid_block, uint256::FromHex(assume_valid));
 
@@ -818,7 +818,7 @@ BOOST_FIXTURE_TEST_CASE(chainstatemanager_args, BasicTestingSetup)
     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, UintToArith256(*uint256::FromHex("0000000000000000000000000000000000000000000000000000000000001234")));
+    BOOST_CHECK_EQUAL(get_valid_opts({"-minimumchainwork=0x1234"}).minimum_chain_work, UintToArith256(uint256{"0000000000000000000000000000000000000000000000000000000000001234"}));
     auto minimum_chainwork{"0123456789abcdef0123456789abcdef0123456789abcdef0123456789abcdef"s};
     BOOST_CHECK_EQUAL(get_valid_opts({("-minimumchainwork=" + minimum_chainwork).c_str()}).minimum_chain_work, UintToArith256(*uint256::FromHex(minimum_chainwork)));

</details>

l0rinc commented at 3:39 PM on September 3, 2024:

Done, thanks!

in src/test/uint256_tests.cpp:409 in 50defb82c5 outdated

 414 | +    BOOST_CHECK_EQUAL(uint256::FromUserHex("1"), uint256::ONE);
 415 | +    BOOST_CHECK_EQUAL(uint256::FromUserHex("0x10"), uint256{0x10});
 416 | +    BOOST_CHECK_EQUAL(uint256::FromUserHex("10"), uint256{0x10});
 417 | +    BOOST_CHECK_EQUAL(uint256::FromUserHex("0xFf"), uint256{0xff});
 418 | +    BOOST_CHECK_EQUAL(uint256::FromUserHex("Ff"), uint256{0xff});
 419 | +    auto valid_hex_64{"0x0123456789abcdef0123456789abcdef0123456789ABDCEF0123456789ABCDEF"s};

stickies-v commented at 2:17 PM on September 3, 2024:

I think it's best to keep the const qualifier? (and less importantly, also here and here)

l0rinc commented at 3:27 PM on September 3, 2024:

I don't really see the value in doing that for such short-scoped test variables - but you can convince me with good arguments, it just seems like noise to me in these cases

stickies-v commented at 3:40 PM on September 3, 2024:

I don't think valid_hex_64 is so short-scoped? Sure, you can manually validate that the 9 places where it's used don't mutate it, but why not make it explicit and fool-proof with const?

l0rinc commented at 3:44 PM on September 3, 2024:

Fair, added const

in src/test/fuzz/hex.cpp:44 in 50defb82c5 outdated

  39 | +        assert(result_string.length() == 64);
  40 | +        assert(IsHex(result_string));
  41 | +        assert(TryParseHex(result_string));
  42 | +        assert(Txid::FromHex(result_string));
  43 | +        assert(Wtxid::FromHex(result_string));
  44 | +        assert(uint256::FromHex(result_string));

stickies-v commented at 2:29 PM on September 3, 2024:

I'm not sure these 3 lines are worth adding, since they're already covered higher up, so this might unnecessarily slow down the test?

l0rinc commented at 3:29 PM on September 3, 2024:

those were testing the same input, this one is testing the parsing of result->ToString()

stickies-v commented at 3:44 PM on September 3, 2024:

Isn't that just a very expensive way to do assert(random_hex_string == ToLower(result->ToString())? (and also, that's not hex testing, but uint256 testing)

l0rinc commented at 3:48 PM on September 3, 2024:

isn't the point of these tests that we shouldn't have know the internals (i.e. black box)? (though in this case that wouldn't even be true since random_hex_string can start with 0x for FromUserHex which isn't added back via ToString)

stickies-v commented at 11:45 AM on September 4, 2024:

(though in this case that wouldn't even be true since random_hex_string can start with 0x for FromUserHex which isn't added back via ToString)

Ah yes good point thanks, perhaps that would be good to clarify in a docstring?

isn't the point of these tests that we shouldn't have know the internals (i.e. black box)?

I'm out of my depth here, so will probably refrain from commenting further and hopefully someone with more fuzzing experience can chime in. I think the point of the fuzz tests is to find bugs, and my understanding is that we often tweak them (e.g. based on knowledge about the implementation) to make them more effective at doing that. As such, not testing the {Txid,Wtxid,uint256}::FromHex() asserts twice seems like a reasonable improvement, but I'm not sure.

l0rinc commented at 1:54 PM on September 4, 2024:

If we think this takes a lot of time, we can optimize it by removing cases that we think are unlikely to result in revealing failures. We can also do it in separate steps, if we see that it's stable for months, remove the excess.

marcofleon commented at 2:49 PM on September 4, 2024:

On my machine, running

FUZZ=hex build_fuzz/src/test/fuzz/fuzz ../qa-assets/fuzz_seed_corpus/hex -runs=1000000

gives 681 seconds with those three lines and 533 seconds without them. Exec/s are reasonable (>1000) for both versions of the harness. The slow down isn't too drastic, so if these added checks are useful in any way then I think it's fine to keep them.

stickies-v commented at 4:51 PM on September 4, 2024:

Thanks for looking into it @marcofleon! If there are no operational concerns adding the test cases, I'm happy with it.

I guess if we keep this, the uint256::FromHex() asserts have become superfluous and could be removed. Shouldn't make a meaningful performance impact (since they're called much less often than the FromUserHex() asserts, so it's more of a thing to keep the test code lean.

diff --git a/src/test/fuzz/hex.cpp b/src/test/fuzz/hex.cpp
index 1109186a8f..71518f626a 100644
--- a/src/test/fuzz/hex.cpp
+++ b/src/test/fuzz/hex.cpp
@@ -28,12 +28,7 @@ FUZZ_TARGET(hex)
     if (IsHex(random_hex_string)) {
         assert(ToLower(random_hex_string) == hex_data);
     }
-    if (uint256::FromHex(random_hex_string)) {
-        assert(random_hex_string.length() == 64);
-        assert(Txid::FromHex(random_hex_string));
-        assert(Wtxid::FromHex(random_hex_string));
-        assert(uint256::FromUserHex(random_hex_string));
-    }
+    (void)uint256::FromHex(random_hex_string);
     if (auto result{uint256::FromUserHex(random_hex_string)}) {
         const auto result_string{result->ToString()};
         assert(result_string.length() == 64);

</details>

l0rinc commented at 5:44 PM on September 4, 2024:

I don’t fully understand why we’re focusing on optimization at this stage. Why not keep both options for now, and if we later find that performance is an issue, remove the redundant ones based on their actual implementations? In my opinion, we’re still in the discovery phase, where having as much redundancy as we can afford is beneficial. Since fuzzing—and property-based testing in general—is black-box testing, we acknowledge that we don’t know the full input space. Our goal is to ensure that certain properties are upheld, even though we might not fully understand how those properties could be violated.

stickies-v commented at 1:24 PM on September 5, 2024:

I agree, we shouldn't overoptimize. My last suggestion was not about performance, just about making the test more maintainable by removing tests we know don't add anything. We seem to disagree, but I'm okay with keeping the duplication if you think it's better, there's no big cost to it. You can mark this as resolved.

l0rinc commented at 1:43 PM on September 5, 2024:

We seem to disagree

We don't have to, if the above discussion didn't change your mind I'll gladly remove them.

l0rinc commented at 1:50 PM on September 5, 2024:

Done

stickies-v commented at 2:17 PM on September 5, 2024:

I'm hesitant to add more to the back-and-forth, but I think we may have been talking past each other. In my latest comment I suggested updating the FromHex() tests, where based on the latest force-push it seems you were talking about the FromUserHex() tests, which I agreed I was okay with based on @marcofleon's testing. @marcofleon also showed me (and I agree with) that removing assert(uint256::FromHex(result_string)); is a slight regression since it's not a duplicate (and I specifically added it), so that could be added back in? Sorry for not highlighting that earlier, I thought we'd moved past keeping all the FromUserHex() tests already so I didn't want to add noise.

l0rinc commented at 8:05 PM on September 5, 2024:

I'm hesitant to add more to the back-and-forth

I don't mind jousting until we find something that is better than what either of us had in mind. Happens often, keep insisting on parts that you feel strongly about! Let's see what you think of my latest push:

FromHex correctness assumes FromUserHex parseability
FromUserHex correctness assumes that if reconverted to hex, it should be accepted by FromHex

Pushed (first a separate rebase, and the change)

stickies-v commented at 9:20 AM on September 6, 2024:

Let's see what you think of my latest push:

Yeah I like the current state of it, thanks for addressing this and can be marked as resolved now!

in src/test/fuzz/hex.cpp:36 in 50defb82c5 outdated

  31 | @@ -34,8 +32,14 @@ FUZZ_TARGET(hex)
  32 |          assert(Wtxid::FromHex(random_hex_string));
  33 |          assert(uint256::FromUserHex(random_hex_string));
  34 |      }
  35 | -    if (const auto result{uint256::FromUserHex(random_hex_string)}) {
  36 | -        assert(uint256::FromHex(result->ToString()));
  37 | +    if (auto result{uint256::FromUserHex(random_hex_string)}) {
  38 | +        auto result_string = result->ToString();

stickies-v commented at 2:30 PM on September 3, 2024:

nit

        const auto result_string{result->ToString()};

l0rinc commented at 3:40 PM on September 3, 2024:

Done (without the const part, which just seems like noise in this context to me, but am open to changing my mind about it)

stickies-v commented at 3:47 PM on September 3, 2024:

Again, here, I don't think the const is noise. This variable is passed to multiple functions, and verifying no mutation happens is an unnecessary burden on the developer. Why not just lean on the compiler?

l0rinc commented at 3:49 PM on September 3, 2024:

K, I don't feel strongly about it, pushed.

stickies-v commented at 2:57 PM on September 3, 2024: contributor

Approach ACK 50defb82c5c52f9cca00de53f76064c376220329

Being able to compare std::optional directly in BOOST_CHECK_* macros is very helpful, thank you for picking this up. The added fuzz coverage seems helpful too but I'm lacking fuzzing experience to comment in detail here.

A couple of suggested improvements besides the comments left:

The PR description would benefit from a bit more context/rationale on what this PR does and why it is helpful (TrySanitizeHexNumber is also no longer a thing).
The example in the f509948e1e17d244c40e3e3b38c755ec5b5f5d08 commit message is helpful but outdated (set_opts doesn't exist)
I think the PR title is not very accurate, would suggest e.g. test: support std::optional in BOOST_CHECK_* and increase hex fuzz coverage (we're not increasing FromUserHex fuzz coverage imo)

l0rinc force-pushed on Sep 3, 2024

l0rinc commented at 3:56 PM on September 3, 2024: contributor

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 renamed this:
~~test: increase FromUserHex FUZZ and unit testing coverage~~
test: support std::optional in BOOST_CHECK_* and increase FromUserHex fuzz feature coverage
on Sep 3, 2024

in src/test/validation_chainstatemanager_tests.cpp:825 in 6fa4c92e4c outdated

 831 | -    BOOST_CHECK_EQUAL(get_valid_opts({"-nominimumchainwork"}).minimum_chain_work.value().GetCompact(), 0U);
 832 | -    BOOST_CHECK_EQUAL(get_valid_opts({"-minimumchainwork=0x1234"}).minimum_chain_work.value().GetCompact(), 0x02123400U);
 833 | +    BOOST_CHECK(!get_valid_opts({}).minimum_chain_work);
 834 | +    BOOST_CHECK_EQUAL(get_valid_opts({"-minimumchainwork=0"}).minimum_chain_work, arith_uint256());
 835 | +    BOOST_CHECK_EQUAL(get_valid_opts({"-nominimumchainwork"}).minimum_chain_work, arith_uint256());
 836 | +    BOOST_CHECK_EQUAL(get_valid_opts({"-minimumchainwork=0x1234"}).minimum_chain_work, UintToArith256(uint256{"0000000000000000000000000000000000000000000000000000000000001234"}));

stickies-v commented at 11:37 AM on September 4, 2024:

simplification nit

    BOOST_CHECK_EQUAL(get_valid_opts({"-minimumchainwork=0x1234"}).minimum_chain_work, arith_uint256{0x1234});

l0rinc commented at 2:00 PM on September 4, 2024:

nice, if I retouch I'll apply it!

l0rinc commented at 1:50 PM on September 5, 2024:

Done. Also applied it to the assumevalid example:

BOOST_CHECK_EQUAL(get_valid_opts({"-assumevalid=0x12"}).assumed_valid_block, uint256{0x12});

stickies-v approved

stickies-v commented at 11:51 AM on September 4, 2024: contributor

ACK 6fa4c92e4c9bf5ea90e25d41b525c88f9c2d834d , modulo my lack of fuzzing experience to have a good view on whether the fuzz/hex.cpp changes indeed make the fuzz test better (the code looks correct and safe to me), as per https://github.com/bitcoin/bitcoin/pull/30618/files#r1742175853

I maintain that the setup_common.cpp changes should be dropped from dec011bdaaeb000a9f22a31e3fe9415ec7bf97dc, but it is not a blocker.

DrahtBot requested review from hodlinator on Sep 4, 2024

l0rinc commented at 2:00 PM on September 4, 2024: contributor

good point thanks, perhaps that would be good to clarify in a docstring?

Do you think it's important in this case? We're not using the random_hex_string as input here at all, only the parsed output, reconverted to hex. If you do, please give me a diff and I'll apply it.

l0rinc force-pushed on Sep 5, 2024

DrahtBot added the label CI failed on Sep 6, 2024

l0rinc force-pushed on Sep 6, 2024

in src/test/validation_chainstatemanager_tests.cpp:827 in 3437d4e1bd outdated

 833 | +    BOOST_CHECK(!get_valid_opts({}).minimum_chain_work);
 834 | +    BOOST_CHECK_EQUAL(get_valid_opts({"-minimumchainwork=0"}).minimum_chain_work, arith_uint256());
 835 | +    BOOST_CHECK_EQUAL(get_valid_opts({"-nominimumchainwork"}).minimum_chain_work, arith_uint256());
 836 | +    BOOST_CHECK_EQUAL(get_valid_opts({"-minimumchainwork=0x1234"}).minimum_chain_work, arith_uint256{0x1234});
 837 | +    auto minimum_chainwork{"0123456789abcdef0123456789abcdef0123456789abcdef0123456789abcdef"s};
 838 | +    BOOST_CHECK_EQUAL(get_valid_opts({("-minimumchainwork=" + minimum_chainwork).c_str()}).minimum_chain_work, UintToArith256(*uint256::FromHex(minimum_chainwork)));

stickies-v commented at 9:19 AM on September 6, 2024:

Dereferencing could be UB when FromHex() is buggy, so for robustness would use .value() here.

    BOOST_CHECK_EQUAL(get_valid_opts({("-minimumchainwork=" + minimum_chainwork).c_str()}).minimum_chain_work, UintToArith256(uint256::FromHex(minimum_chainwork).value()));

l0rinc commented at 10:18 AM on September 6, 2024:

Thanks, fixed

stickies-v approved

stickies-v commented at 10:00 AM on September 6, 2024: contributor

ACK 3437d4e1bdec7963d3b49bcc396838602a5b00fc

Do you think it's important in this case?

No it's fine as-is too, it's just that we start from a string so producing another, seemingly similar one might be unintuitive to someone less familiar with the code. Could add something like this, but no strong view

diff --git a/src/test/fuzz/hex.cpp b/src/test/fuzz/hex.cpp
index 6a85fd9fdb..6a4a381588 100644
--- a/src/test/fuzz/hex.cpp
+++ b/src/test/fuzz/hex.cpp
@@ -32,6 +32,7 @@ FUZZ_TARGET(hex)
         assert(uint256::FromUserHex(random_hex_string));
     }
     if (const auto result{uint256::FromUserHex(random_hex_string)}) {
+        // ToString() returns a fixed-length string without "0x" prefix
         const auto result_string{result->ToString()};
         assert(result_string.length() == 64);
         assert(IsHex(result_string));

</details>

l0rinc force-pushed on Sep 6, 2024

l0rinc commented at 10:18 AM on September 6, 2024: contributor

Could add something like this, but no strong view

Done, thanks!

stickies-v commented at 10:43 AM on September 6, 2024: contributor

re-ACK 4ac1981d338f89ed2b814aedaebe81783293e6f5

l0rinc force-pushed on Sep 9, 2024

Add std::optional support to Boost's equality check

Also moved the operators to the bottom of the file since they're less important and to group them together.

Co-authored-by: Hodlinator <172445034+hodlinator@users.noreply.github.com>
Co-authored-by: stickies-v <stickies-v@protonmail.com>

743ac30e34

l0rinc force-pushed on Sep 9, 2024

stickies-v commented at 6:58 AM on September 10, 2024: contributor

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

in src/test/uint256_tests.cpp:409 in 57c1f9fb95 outdated

 414 | +    BOOST_CHECK_EQUAL(uint256::FromUserHex("1"), uint256::ONE);
 415 | +    BOOST_CHECK_EQUAL(uint256::FromUserHex("0x10"), uint256{0x10});
 416 | +    BOOST_CHECK_EQUAL(uint256::FromUserHex("10"), uint256{0x10});
 417 | +    BOOST_CHECK_EQUAL(uint256::FromUserHex("0xFf"), uint256{0xff});
 418 | +    BOOST_CHECK_EQUAL(uint256::FromUserHex("Ff"), uint256{0xff});
 419 | +    const auto valid_hex_64{"0x0123456789abcdef0123456789abcdef0123456789ABDCEF0123456789ABCDEF"s};

ryanofsky commented at 12:39 PM on September 11, 2024:

In commit "Use BOOST_CHECK_EQUAL for optional, arith_uint256, uint256, uint160" (57c1f9fb9580456f98c239e1a8aa9161c7014fc6)

In general I like using auto to avoid verbosity and repetition when types are obvious. But in this case I think using std::string makes code easier to read because "0x123..." literals in our codebase are more commonly used to create different types like c strings, arrays, vectors, and blobs. Using auto is less clear because it makes std::string type a surprise until you notice "s}; at the end.

l0rinc commented at 1:42 PM on September 11, 2024:

I'm full of surprises - done

in src/test/validation_chainstatemanager_tests.cpp:816 in 57c1f9fb95 outdated

 818 | +    BOOST_CHECK_EQUAL(get_valid_opts({"-assumevalid="}).assumed_valid_block, uint256::ZERO);
 819 | +    BOOST_CHECK_EQUAL(get_valid_opts({"-assumevalid=0"}).assumed_valid_block, uint256::ZERO);
 820 | +    BOOST_CHECK_EQUAL(get_valid_opts({"-noassumevalid"}).assumed_valid_block, uint256::ZERO);
 821 | +    BOOST_CHECK_EQUAL(get_valid_opts({"-assumevalid=0x12"}).assumed_valid_block, uint256{0x12});
 822 | +
 823 | +    auto assume_valid{"0123456789abcdef0123456789abcdef0123456789abcdef0123456789abcdef"s};

ryanofsky commented at 12:47 PM on September 11, 2024:

In commit "Use BOOST_CHECK_EQUAL for optional, arith_uint256, uint256, uint160" (57c1f9fb9580456f98c239e1a8aa9161c7014fc6)

Here and below I also think const std::string would be more helpful than auto

l0rinc commented at 1:42 PM on September 11, 2024:

Done

l0rinc commented at 1:45 PM on September 11, 2024:

I didn't const them since they're only used in the next line (i.e. very narrow scoped), seems like noise in this case - but if you insist, I'll add it

ryanofsky commented at 2:15 PM on September 11, 2024:

re: #30618 (review)

if you insist

I don't insist. Definitely follow your own judgement, I'm just throwing out suggestions.

l0rinc commented at 3:01 PM on September 11, 2024:

Appreciate all your reviews and valuable feedback, thanks for taking the time!

in src/test/fuzz/hex.cpp:34 in 6da47f6ba5 outdated

  28 | @@ -29,13 +29,16 @@ FUZZ_TARGET(hex)
  29 |          assert(ToLower(random_hex_string) == hex_data);
  30 |      }
  31 |      if (uint256::FromHex(random_hex_string)) {
  32 | -        assert(random_hex_string.length() == 64);
  33 | -        assert(Txid::FromHex(random_hex_string));
  34 | -        assert(Wtxid::FromHex(random_hex_string));

ryanofsky commented at 1:00 PM on September 11, 2024:

In commit "Compare FromUserHex result against other hex validators and parsers" (6da47f6ba5450e7f2b2ab2a64c7cfd354f67a1a9)

The three lines removed here are not equivalent to the similar lines added below because these are checking random_hex_string, while lines below are checking result->ToString(), these strings are not the same because random_hex_string can have mixed case. Would suggest not dropping these lines.

stickies-v commented at 1:32 PM on September 11, 2024:

This change was made because of my suggestion but you're right there can be a case difference which I did not consider. Sorry for that @l0rinc .

l0rinc commented at 1:42 PM on September 11, 2024:

Done, thanks both for the suggestions (this is how we get to the best outcome).

ryanofsky approved

ryanofsky commented at 1:03 PM on September 11, 2024: contributor

Code review ACK 6da47f6ba5450e7f2b2ab2a64c7cfd354f67a1a9. Nice change to make tests easier to read and setup_common code better organized

Use BOOST_CHECK_EQUAL for optional, 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]

Also added extra minimum_chainwork test to make it symmetric with assumevalid

Co-authored-by: Ryan Ofsky <ryan@ofsky.org>
Co-authored-by: Hodlinator <172445034+hodlinator@users.noreply.github.com>

19947863e1

Compare FromUserHex result against other hex validators and parsers 1eac96a503

l0rinc force-pushed on Sep 11, 2024

ryanofsky approved

ryanofsky commented at 2:21 PM on September 11, 2024: contributor

Code review ACK 1eac96a503b6bac3eaf5d0eb3d23ffde3bfbf9aa. Only changes since last review were auto type and fuzz test tweaks.

DrahtBot requested review from stickies-v on Sep 11, 2024

hodlinator approved

hodlinator commented at 10:17 PM on September 11, 2024: contributor

ACK 1eac96a503b6bac3eaf5d0eb3d23ffde3bfbf9aa

Provides better error messages for tests.

nit: 1eac96a503b6bac3eaf5d0eb3d23ffde3bfbf9aa - "Compare FromUserHex result against other hex validators and parsers" is not dependent on the other changes, so could be moved first instead of last if you re-touch.

stickies-v commented at 12:55 PM on September 12, 2024: contributor

re-ACK 1eac96a503b6bac3eaf5d0eb3d23ffde3bfbf9aa

CI timeout issues seem unrelated.

ryanofsky merged this on Sep 12, 2024

ryanofsky closed this on Sep 12, 2024

l0rinc deleted the branch on Sep 12, 2024

TheCharlatan referenced this in commit 69282950aa on Sep 16, 2024

TheCharlatan referenced this in commit dfe0cd4ec5 on Sep 16, 2024

bitcoin locked this on Sep 12, 2025