Skip to content

Commit

Permalink
Merge bitcoin#30773: Remove unsafe uint256S() and test-only uint160S()
Browse files Browse the repository at this point in the history 
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 bitcoin#30569._

  Since bitcoin@fad2991, `uint256S()` has been [deprecated](bitcoin@fad2991#diff-800776e2dda39116e889839f69409571a5d397de048a141da7e4003bc099e3e2R138) because it is less robust than the `base_blob::FromHex()` introduced in 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#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@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
  • Loading branch information
@ryanofsky
ryanofsky committed Sep 10, 2024
2 parents 2756797 + 43cd83b commit c66c683
Show file tree
Hide file tree
Showing 6 changed files with 82 additions and 151 deletions.
11 changes: 6 additions & 5 deletions src/rpc/util.cpp
View file
Original file line number Diff line number Diff line change
Expand Up @@ -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>
Expand Down Expand Up @@ -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)
{
Expand Down
89 changes: 63 additions & 26 deletions src/test/arith_uint256_tests.cpp
View file
Original file line number Diff line number Diff line change
Expand Up @@ -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";
Expand All @@ -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";
Expand Down Expand Up @@ -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);
Expand Down Expand Up @@ -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;
Expand Down Expand Up @@ -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);
Expand Down Expand Up @@ -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()
1 change: 0 additions & 1 deletion src/test/fuzz/hex.cpp
View file
Original file line number Diff line number Diff line change
Expand Up @@ -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&) {
Expand Down
2 changes: 1 addition & 1 deletion src/test/fuzz/integer.cpp
View file
Original file line number Diff line number Diff line change
Expand Up @@ -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);
Expand Down
120 changes: 12 additions & 108 deletions src/test/uint256_tests.cpp
View file
Original file line number Diff line number Diff line change
Expand Up @@ -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>
Expand Down Expand Up @@ -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>):
Expand All @@ -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);
Expand Down Expand Up @@ -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().
Expand Down
10 changes: 0 additions & 10 deletions src/uint256.h
View file
Original file line number Diff line number Diff line change
Expand Up @@ -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

0 comments on commit c66c683

Please sign in to comment.