Merge bitcoin/bitcoin#30773: Remove unsafe uint256S() and test-only uint160S()

43cd83b test: move uint256_tests/operator_with_self to arith_uint256_tests (stickies-v)
c6c994c test: remove test-only uint160S (stickies-v)
62cc465 test: remove test-only uint256S (stickies-v)
adc00ad test: remove test-only arith_uint256S (stickies-v)
f51b237 refactor: rpc: use uint256::FromHex for ParseHashV (stickies-v)

Pull request description:

  _Continuation of #30569._

  Since bitcoin/bitcoin@fad2991, `uint256S()` has been [deprecated](bitcoin/bitcoin@fad2991#diff-800776e2dda39116e889839f69409571a5d397de048a141da7e4003bc099e3e2R138) because it is less robust than the `base_blob::FromHex()` introduced in bitcoin/bitcoin#30482. Specifically, it tries to recover from length-mismatches, recover from untrimmed whitespace, 0x-prefix and garbage at the end, instead of simply requiring exactly 64 hex-only characters. (see also bitcoin/bitcoin#30532)

  This PR removes `uint256S()` (and `uint160S()`) completely, with no non-test behaviour change.

  Specifically, the main changes in this PR are:
  - the (minimal) last non-test usage of `uint256S()` in `ParseHashV()` is removed without behaviour change, which can partially be verified by cherry-picking and/or modifying [this test commit](stickies-v/bitcoin@1f2b0fa)).
  - the test usage of `uint{160,256}S()` is removed, largely replacing it with `uint{160,256}::FromHex()` where applicable, potentially modifying the test by removing non-hex characters or dropping the test entirely if removing non-hex characters makes it redundant
  - the now unused `uint{160,256}S()` functions are removed completely.
  - unit test coverage on converting `uint256` <-> `arith_uint256` through `UintToArith256()` and `ArithToUint256()` is beefed up, and `arith_uint256` tests are moved to `arith_uint256_tests.cpp`, removing the `uint256_tests.cpp` dependency on `uint256h`, mirroring how the code is structured.

  _Note:  `uint256::FromUserHex()` exists to more leniently construct uint256 from user input, allowing "0x" prefixes and too-short-input, as safer alternative to `uint256S()` where necessary._

ACKs for top commit:
  l0rinc:
    reACK 43cd83b
  hodlinator:
    re-ACK 43cd83b
  ryanofsky:
    Code review ACK 43cd83b. Only code change is a small refactoring which looks good. The rest of the PR is all test changes, which I only lightly reviewed, but seem to be positive and do what's described

Tree-SHA512: 48147a4c6af671597df0f72c1b477ae4631cd2cae4645ec54d0e327611ff302c9899e344518c81242cdde82930f6ad23a3a7e6e0b80671816e9f457b9de90a5c

Author: ryanofsky

src/rpc/util.cpp

@@ -19,6 +19,7 @@
 #include <script/signingprovider.h>
 #include <script/solver.h>
 #include <tinyformat.h>
+#include <uint256.h>
 #include <univalue.h>
 #include <util/check.h>
 #include <util/result.h>
@@ -102,11 +103,11 @@ CFeeRate ParseFeeRate(const UniValue& json)
 uint256 ParseHashV(const UniValue& v, std::string_view name)
 {
     const std::string& strHex(v.get_str());
-    if (64 != strHex.length())
-        throw JSONRPCError(RPC_INVALID_PARAMETER, strprintf("%s must be of length %d (not %d, for '%s')", name, 64, strHex.length(), strHex));
-    if (!IsHex(strHex)) // Note: IsHex("") is false
-        throw JSONRPCError(RPC_INVALID_PARAMETER, strprintf("%s must be hexadecimal string (not '%s')", name, strHex));
-    return uint256S(strHex);
+    if (auto rv{uint256::FromHex(strHex)}) return *rv;
+    if (auto expected_len{uint256::size() * 2}; strHex.length() != expected_len) {
+        throw JSONRPCError(RPC_INVALID_PARAMETER, strprintf("%s must be of length %d (not %d, for '%s')", name, expected_len, strHex.length(), strHex));
+    }
+    throw JSONRPCError(RPC_INVALID_PARAMETER, strprintf("%s must be hexadecimal string (not '%s')", name, strHex));
 }
 uint256 ParseHashO(const UniValue& o, std::string_view strKey)
 {

src/test/arith_uint256_tests.cpp

@@ -23,9 +23,6 @@ static inline arith_uint256 arith_uint256V(const std::vector<unsigned char>& vch
 {
     return UintToArith256(uint256(vch));
 }
-// Takes a number written in hex (with most significant digits first).
-static inline arith_uint256 arith_uint256S(std::string_view str) { return UintToArith256(uint256S(str)); }
-
 const unsigned char R1Array[] =
     "\x9c\x52\x4a\xdb\xcf\x56\x11\x12\x2b\x29\x12\x5e\x5d\x35\xd2\xd2"
     "\x22\x81\xaa\xb5\x33\xf0\x08\x32\xd5\x56\xb1\xf9\xea\xe5\x1d\x7d";
@@ -39,8 +36,6 @@ const unsigned char R2Array[] =
     "\x13\x30\x47\xa3\x19\x2d\xda\x71\x49\x13\x72\xf0\xb4\xca\x81\xd7";
 const arith_uint256 R2L = arith_uint256V(std::vector<unsigned char>(R2Array,R2Array+32));
 
-const char R1LplusR2L[] = "549FB09FEA236A1EA3E31D4D58F1B1369288D204211CA751527CFC175767850C";
-
 const unsigned char ZeroArray[] =
     "\x00\x00\x00\x00\x00\x00\x00\x00\x00\x00\x00\x00\x00\x00\x00\x00"
     "\x00\x00\x00\x00\x00\x00\x00\x00\x00\x00\x00\x00\x00\x00\x00\x00";
@@ -97,27 +92,25 @@ BOOST_AUTO_TEST_CASE( basics ) // constructors, equality, inequality
     }
     BOOST_CHECK(ZeroL == (OneL << 256));
 
-    // String Constructor and Copy Constructor
-    BOOST_CHECK(arith_uint256S("0x" + R1L.ToString()) == R1L);
-    BOOST_CHECK(arith_uint256S("0x" + R2L.ToString()) == R2L);
-    BOOST_CHECK(arith_uint256S("0x" + ZeroL.ToString()) == ZeroL);
-    BOOST_CHECK(arith_uint256S("0x" + OneL.ToString()) == OneL);
-    BOOST_CHECK(arith_uint256S("0x" + MaxL.ToString()) == MaxL);
-    BOOST_CHECK(arith_uint256S(R1L.ToString()) == R1L);
-    BOOST_CHECK(arith_uint256S("   0x" + R1L.ToString() + "   ") == R1L);
-    BOOST_CHECK(arith_uint256S("") == ZeroL);
-    BOOST_CHECK(arith_uint256S("1") == OneL);
-    BOOST_CHECK(R1L == arith_uint256S(R1ArrayHex));
+    // Construct from hex string
+    BOOST_CHECK_EQUAL(UintToArith256(uint256::FromHex(R1L.ToString()).value()), R1L);
+    BOOST_CHECK_EQUAL(UintToArith256(uint256::FromHex(R2L.ToString()).value()), R2L);
+    BOOST_CHECK_EQUAL(UintToArith256(uint256::FromHex(ZeroL.ToString()).value()), ZeroL);
+    BOOST_CHECK_EQUAL(UintToArith256(uint256::FromHex(OneL.ToString()).value()), OneL);
+    BOOST_CHECK_EQUAL(UintToArith256(uint256::FromHex(MaxL.ToString()).value()), MaxL);
+    BOOST_CHECK_EQUAL(UintToArith256(uint256::FromHex(R1ArrayHex).value()), R1L);
+
+    // Copy constructor
     BOOST_CHECK(arith_uint256(R1L) == R1L);
     BOOST_CHECK((arith_uint256(R1L^R2L)^R2L) == R1L);
     BOOST_CHECK(arith_uint256(ZeroL) == ZeroL);
     BOOST_CHECK(arith_uint256(OneL) == OneL);
 
     // uint64_t constructor
-    BOOST_CHECK((R1L & arith_uint256S("0xffffffffffffffff")) == arith_uint256(R1LLow64));
-    BOOST_CHECK(ZeroL == arith_uint256(0));
-    BOOST_CHECK(OneL == arith_uint256(1));
-    BOOST_CHECK(arith_uint256S("0xffffffffffffffff") == arith_uint256(0xffffffffffffffffULL));
+    BOOST_CHECK_EQUAL(R1L & arith_uint256{0xffffffffffffffff}, arith_uint256{R1LLow64});
+    BOOST_CHECK_EQUAL(ZeroL, arith_uint256{0});
+    BOOST_CHECK_EQUAL(OneL, arith_uint256{1});
+    BOOST_CHECK_EQUAL(arith_uint256{0xffffffffffffffff}, arith_uint256{0xffffffffffffffffULL});
 
     // Assignment (from base_uint)
     arith_uint256 tmpL = ~ZeroL; BOOST_CHECK(tmpL == ~ZeroL);
@@ -284,15 +277,12 @@ BOOST_AUTO_TEST_CASE( comparison ) // <= >= < >
 
     BOOST_CHECK_LT(ZeroL,
                    OneL);
-    // Verify hex number representation has the most significant digits first.
-    BOOST_CHECK_LT(arith_uint256S("0000000000000000000000000000000000000000000000000000000000000001"),
-                   arith_uint256S("1000000000000000000000000000000000000000000000000000000000000000"));
 }
 
 BOOST_AUTO_TEST_CASE( plusMinus )
 {
     arith_uint256 TmpL = 0;
-    BOOST_CHECK(R1L + R2L == arith_uint256S(R1LplusR2L));
+    BOOST_CHECK_EQUAL(R1L + R2L, UintToArith256(uint256{"549fb09fea236a1ea3e31d4d58f1b1369288d204211ca751527cfc175767850c"}));
     TmpL += R1L;
     BOOST_CHECK(TmpL == R1L);
     TmpL += R2L;
@@ -356,8 +346,8 @@ BOOST_AUTO_TEST_CASE( multiply )
 
 BOOST_AUTO_TEST_CASE( divide )
 {
-    arith_uint256 D1L{arith_uint256S("AD7133AC1977FA2B7")};
-    arith_uint256 D2L{arith_uint256S("ECD751716")};
+    arith_uint256 D1L{UintToArith256(uint256{"00000000000000000000000000000000000000000000000ad7133ac1977fa2b7"})};
+    arith_uint256 D2L{UintToArith256(uint256{"0000000000000000000000000000000000000000000000000000000ecd751716"})};
     BOOST_CHECK((R1L / D1L).ToString() == "00000000000000000b8ac01106981635d9ed112290f8895545a7654dde28fb3a");
     BOOST_CHECK((R1L / D2L).ToString() == "000000000873ce8efec5b67150bad3aa8c5fcb70e947586153bf2cec7c37c57a");
     BOOST_CHECK(R1L / OneL == R1L);
@@ -571,4 +561,51 @@ BOOST_AUTO_TEST_CASE( getmaxcoverage ) // some more tests just to get 100% cover
     CHECKBITWISEOPERATOR(R1,~R2,&)
 }
 
+BOOST_AUTO_TEST_CASE(conversion)
+{
+    for (const arith_uint256& arith : {ZeroL, OneL, R1L, R2L}) {
+        const auto u256{uint256::FromHex(arith.GetHex()).value()};
+        BOOST_CHECK_EQUAL(UintToArith256(ArithToUint256(arith)), arith);
+        BOOST_CHECK_EQUAL(UintToArith256(u256), arith);
+        BOOST_CHECK_EQUAL(u256, ArithToUint256(arith));
+        BOOST_CHECK_EQUAL(ArithToUint256(arith).GetHex(), UintToArith256(u256).GetHex());
+    }
+
+    for (uint8_t num : {0, 1, 0xff}) {
+        BOOST_CHECK_EQUAL(UintToArith256(uint256{num}), arith_uint256{num});
+        BOOST_CHECK_EQUAL(uint256{num}, ArithToUint256(arith_uint256{num}));
+        BOOST_CHECK_EQUAL(UintToArith256(uint256{num}), num);
+    }
+}
+
+BOOST_AUTO_TEST_CASE(operator_with_self)
+{
+    /* Clang 16 and earlier detects v -= v and v /= v as self-assignments
+       to 0 and 1 respectively.
+       See: https://github.com/llvm/llvm-project/issues/42469
+       and the fix in commit c5302325b2a62d77cf13dd16cd5c19141862fed0 .
+
+       This makes some sense for arithmetic classes, but could be considered a bug
+       elsewhere. Disable the warning here so that the code can be tested, but the
+       warning should remain on as there will likely always be a better way to
+       express this.
+    */
+#if defined(__clang__)
+#pragma clang diagnostic push
+#pragma clang diagnostic ignored "-Wself-assign-overloaded"
+#endif
+    arith_uint256 v{2};
+    v *= v;
+    BOOST_CHECK_EQUAL(v, arith_uint256{4});
+    v /= v;
+    BOOST_CHECK_EQUAL(v, arith_uint256{1});
+    v += v;
+    BOOST_CHECK_EQUAL(v, arith_uint256{2});
+    v -= v;
+    BOOST_CHECK_EQUAL(v, arith_uint256{0});
+#if defined(__clang__)
+#pragma clang diagnostic pop
+#endif
+}
+
 BOOST_AUTO_TEST_SUITE_END()

src/test/fuzz/hex.cpp

@@ -37,7 +37,6 @@ FUZZ_TARGET(hex)
     if (const auto result{uint256::FromUserHex(random_hex_string)}) {
         assert(uint256::FromHex(result->ToString()));
     }
-    (void)uint256S(random_hex_string);
     try {
         (void)HexToPubKey(random_hex_string);
     } catch (const UniValue&) {

src/test/fuzz/integer.cpp

@@ -140,7 +140,7 @@ FUZZ_TARGET(integer, .init = initialize_integer)
 
     const arith_uint256 au256 = UintToArith256(u256);
     assert(ArithToUint256(au256) == u256);
-    assert(uint256S(au256.GetHex()) == u256);
+    assert(uint256::FromHex(au256.GetHex()).value() == u256);
     (void)au256.bits();
     (void)au256.GetCompact(/* fNegative= */ false);
     (void)au256.GetCompact(/* fNegative= */ true);

src/test/uint256_tests.cpp

@@ -2,7 +2,6 @@
 // Distributed under the MIT software license, see the accompanying
 // file COPYING or http://www.opensource.org/licenses/mit-license.php.
 
-#include <arith_uint256.h>
 #include <streams.h>
 #include <test/util/setup_common.h>
 #include <uint256.h>
@@ -61,14 +60,6 @@ static std::string ArrayToString(const unsigned char A[], unsigned int width)
     return Stream.str();
 }
 
-// Takes hex string in reverse byte order.
-inline uint160 uint160S(std::string_view str)
-{
-    uint160 rv;
-    rv.SetHexDeprecated(str);
-    return rv;
-}
-
 BOOST_AUTO_TEST_CASE( basics ) // constructors, equality, inequality
 {
     // constructor uint256(vector<char>):
@@ -92,33 +83,22 @@ BOOST_AUTO_TEST_CASE( basics ) // constructors, equality, inequality
     BOOST_CHECK_NE(MaxL, ZeroL); BOOST_CHECK_NE(MaxS, ZeroS);
 
     // String Constructor and Copy Constructor
-    BOOST_CHECK_EQUAL(uint256S("0x"+R1L.ToString()), R1L);
-    BOOST_CHECK_EQUAL(uint256S("0x"+R2L.ToString()), R2L);
-    BOOST_CHECK_EQUAL(uint256S("0x"+ZeroL.ToString()), ZeroL);
-    BOOST_CHECK_EQUAL(uint256S("0x"+OneL.ToString()), OneL);
-    BOOST_CHECK_EQUAL(uint256S("0x"+MaxL.ToString()), MaxL);
-    BOOST_CHECK_EQUAL(uint256S(R1L.ToString()), R1L);
-    BOOST_CHECK_EQUAL(uint256S("   0x"+R1L.ToString()+"   "), R1L);
-    BOOST_CHECK_EQUAL(uint256S("   0x"+R1L.ToString()+"-trash;%^&   "), R1L);
-    BOOST_CHECK_EQUAL(uint256S("\t \n  \n \f\n\r\t\v\t   0x"+R1L.ToString()+"  \t \n  \n \f\n\r\t\v\t "), R1L);
-    BOOST_CHECK_EQUAL(uint256S(""), ZeroL);
-    BOOST_CHECK_EQUAL(uint256S("1"), OneL);
-    BOOST_CHECK_EQUAL(R1L, uint256S(R1ArrayHex));
+    BOOST_CHECK_EQUAL(uint256::FromHex(R1L.ToString()).value(), R1L);
+    BOOST_CHECK_EQUAL(uint256::FromHex(R2L.ToString()).value(), R2L);
+    BOOST_CHECK_EQUAL(uint256::FromHex(ZeroL.ToString()).value(), ZeroL);
+    BOOST_CHECK_EQUAL(uint256::FromHex(OneL.ToString()).value(), OneL);
+    BOOST_CHECK_EQUAL(uint256::FromHex(MaxL.ToString()).value(), MaxL);
+    BOOST_CHECK_EQUAL(uint256::FromHex(R1ArrayHex).value(), R1L);
     BOOST_CHECK_EQUAL(uint256(R1L), R1L);
     BOOST_CHECK_EQUAL(uint256(ZeroL), ZeroL);
     BOOST_CHECK_EQUAL(uint256(OneL), OneL);
 
-    BOOST_CHECK_EQUAL(uint160S("0x"+R1S.ToString()), R1S);
-    BOOST_CHECK_EQUAL(uint160S("0x"+R2S.ToString()), R2S);
-    BOOST_CHECK_EQUAL(uint160S("0x"+ZeroS.ToString()), ZeroS);
-    BOOST_CHECK_EQUAL(uint160S("0x"+OneS.ToString()), OneS);
-    BOOST_CHECK_EQUAL(uint160S("0x"+MaxS.ToString()), MaxS);
-    BOOST_CHECK_EQUAL(uint160S(R1S.ToString()), R1S);
-    BOOST_CHECK_EQUAL(uint160S("   0x"+R1S.ToString()+"   "), R1S);
-    BOOST_CHECK_EQUAL(uint160S("   0x"+R1S.ToString()+"-trash;%^&   "), R1S);
-    BOOST_CHECK_EQUAL(uint160S(" \t \n  \n \f\n\r\t\v\t  0x"+R1S.ToString()+"   \t \n  \n \f\n\r\t\v\t"), R1S);
-    BOOST_CHECK_EQUAL(uint160S(""), ZeroS);
-    BOOST_CHECK_EQUAL(R1S, uint160S(R1ArrayHex));
+    BOOST_CHECK_EQUAL(uint160::FromHex(R1S.ToString()).value(), R1S);
+    BOOST_CHECK_EQUAL(uint160::FromHex(R2S.ToString()).value(), R2S);
+    BOOST_CHECK_EQUAL(uint160::FromHex(ZeroS.ToString()).value(), ZeroS);
+    BOOST_CHECK_EQUAL(uint160::FromHex(OneS.ToString()).value(), OneS);
+    BOOST_CHECK_EQUAL(uint160::FromHex(MaxS.ToString()).value(), MaxS);
+    BOOST_CHECK_EQUAL(uint160::FromHex(std::string_view{R1ArrayHex + 24, 40}).value(), R1S);
 
     BOOST_CHECK_EQUAL(uint160(R1S), R1S);
     BOOST_CHECK_EQUAL(uint160(ZeroS), ZeroS);
@@ -264,82 +244,6 @@ BOOST_AUTO_TEST_CASE(methods) // GetHex SetHexDeprecated FromHex begin() end() s
     ss.clear();
 }
 
-BOOST_AUTO_TEST_CASE( conversion )
-{
-    BOOST_CHECK_EQUAL(ArithToUint256(UintToArith256(ZeroL)), ZeroL);
-    BOOST_CHECK_EQUAL(ArithToUint256(UintToArith256(OneL)), OneL);
-    BOOST_CHECK_EQUAL(ArithToUint256(UintToArith256(R1L)), R1L);
-    BOOST_CHECK_EQUAL(ArithToUint256(UintToArith256(R2L)), R2L);
-    BOOST_CHECK_EQUAL(UintToArith256(ZeroL), 0);
-    BOOST_CHECK_EQUAL(UintToArith256(OneL), 1);
-    BOOST_CHECK_EQUAL(ArithToUint256(0), ZeroL);
-    BOOST_CHECK_EQUAL(ArithToUint256(1), OneL);
-    BOOST_CHECK_EQUAL(arith_uint256(UintToArith256(uint256S(R1L.GetHex()))), UintToArith256(R1L));
-    BOOST_CHECK_EQUAL(arith_uint256(UintToArith256(uint256S(R2L.GetHex()))), UintToArith256(R2L));
-    BOOST_CHECK_EQUAL(R1L.GetHex(), UintToArith256(R1L).GetHex());
-    BOOST_CHECK_EQUAL(R2L.GetHex(), UintToArith256(R2L).GetHex());
-}
-
-BOOST_AUTO_TEST_CASE( operator_with_self )
-{
-
-/* Clang 16 and earlier detects v -= v and v /= v as self-assignments
-   to 0 and 1 respectively.
-   See: https://github.com/llvm/llvm-project/issues/42469
-   and the fix in commit c5302325b2a62d77cf13dd16cd5c19141862fed0 .
-
-   This makes some sense for arithmetic classes, but could be considered a bug
-   elsewhere. Disable the warning here so that the code can be tested, but the
-   warning should remain on as there will likely always be a better way to
-   express this.
-*/
-
-#if defined(__clang__)
-#    pragma clang diagnostic push
-#    pragma clang diagnostic ignored "-Wself-assign-overloaded"
-#endif
-    arith_uint256 v = UintToArith256(uint256S("02"));
-    v *= v;
-    BOOST_CHECK_EQUAL(v, UintToArith256(uint256S("04")));
-    v /= v;
-    BOOST_CHECK_EQUAL(v, UintToArith256(uint256S("01")));
-    v += v;
-    BOOST_CHECK_EQUAL(v, UintToArith256(uint256S("02")));
-    v -= v;
-    BOOST_CHECK_EQUAL(v, UintToArith256(uint256S("0")));
-#if defined(__clang__)
-#    pragma clang diagnostic pop
-#endif
-}
-
-BOOST_AUTO_TEST_CASE(parse)
-{
-    {
-        std::string s_12{"0000000000000000000000000000000000000000000000000000000000000012"};
-        BOOST_CHECK_EQUAL(uint256S("12\0").GetHex(), s_12);
-        BOOST_CHECK_EQUAL(uint256S(std::string_view{"12\0", 3}).GetHex(), s_12);
-        BOOST_CHECK_EQUAL(uint256S("0x12").GetHex(), s_12);
-        BOOST_CHECK_EQUAL(uint256S(" 0x12").GetHex(), s_12);
-        BOOST_CHECK_EQUAL(uint256S(" 12").GetHex(), s_12);
-    }
-    {
-        std::string s_1{uint256::ONE.GetHex()};
-        BOOST_CHECK_EQUAL(uint256S("1\0").GetHex(), s_1);
-        BOOST_CHECK_EQUAL(uint256S(std::string_view{"1\0", 2}).GetHex(), s_1);
-        BOOST_CHECK_EQUAL(uint256S("0x1").GetHex(), s_1);
-        BOOST_CHECK_EQUAL(uint256S(" 0x1").GetHex(), s_1);
-        BOOST_CHECK_EQUAL(uint256S(" 1").GetHex(), s_1);
-    }
-    {
-        std::string s_0{uint256::ZERO.GetHex()};
-        BOOST_CHECK_EQUAL(uint256S("\0").GetHex(), s_0);
-        BOOST_CHECK_EQUAL(uint256S(std::string_view{"\0", 1}).GetHex(), s_0);
-        BOOST_CHECK_EQUAL(uint256S("0x").GetHex(), s_0);
-        BOOST_CHECK_EQUAL(uint256S(" 0x").GetHex(), s_0);
-        BOOST_CHECK_EQUAL(uint256S(" ").GetHex(), s_0);
-    }
-}
-
 /**
  * Implemented as a templated function so it can be reused by other classes that have a FromHex()
  * method that wraps base_blob::FromHex(), such as transaction_identifier::FromHex().

src/uint256.h

@@ -199,14 +199,4 @@ class uint256 : public base_blob<256> {
     static const uint256 ONE;
 };
 
-/* uint256 from std::string_view, containing byte-reversed hex encoding.
- * DEPRECATED. Unlike FromHex this accepts any invalid input, thus it is fragile and deprecated!
- */
-inline uint256 uint256S(std::string_view str)
-{
-    uint256 rv;
-    rv.SetHexDeprecated(str);
-    return rv;
-}
-
 #endif // BITCOIN_UINT256_H