Remove unsafe uint256S() and test-only uint160S() #30773

stickies-v commented at 3:37 PM on August 30, 2024: contributor

Continuation of #30569.

Since https://github.com/bitcoin/bitcoin/commit/fad2991ba073de0bd1f12e42bf0fbaca4a265508, uint256S() has been deprecated because it is less robust than the base_blob::FromHex() introduced in #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 #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).
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.

DrahtBot commented at 3:37 PM on August 30, 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	l0rinc, hodlinator, ryanofsky
Concept ACK	maflcko

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

in src/rpc/util.cpp:107 in eb26946d7a outdated

 108 | -        throw JSONRPCError(RPC_INVALID_PARAMETER, strprintf("%s must be hexadecimal string (not '%s')", name, strHex));
 109 | -    return uint256S(strHex);
 110 | +    if (auto rv{uint256::FromHex(v.get_str())}) {
 111 | +        return *rv;
 112 | +    }
 113 | +    throw JSONRPCError(RPC_INVALID_PARAMETER, strprintf("%s must be string of %d hex digits (not '%s')", name, uint256::size() * 2, v.get_str()));

maflcko commented at 3:41 PM on August 30, 2024:

    throw JSONRPCError(RPC_INVALID_PARAMETER, strprintf("%s must be string of %d hex digits (not '%s' with len %s)", name, uint256::size() * 2, v.get_str(), len));

maflcko commented at 2:47 PM on September 3, 2024:

Any reason to omit the length from the error message when it was previously present?

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

we're already displaying the value itself, isn't that enough?

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

Previously we disambiguated between length errors and other errors. Since that's not longer true, I think not including the length in the error is the most elegant approach, especially since we're already showing the invalid user input as @l0rinc says. Adding the length feels like an arbitrary and not very useful addition to me.

maflcko commented at 4:47 PM on September 3, 2024:

I am mostly thinking about a user running into a copy-paste error, where they delete or add a character from a 64-length string. Returning them back the string won't be useful for them to spot the error, as they likely can already see their input. However telling them that they accidentally added or removed a character may be useful and saves them from manually counting the characters, or write a program to count for them.

maflcko commented at 10:22 AM on September 4, 2024:

For reference, the len reporting (and error handling) was added after #9040 (comment).

Also, you added len reporting in a similar place recently in #30377 (review), so I fail to see why there it is "helpful" while here it would be not.

I am happy to review and ACK either version, but I think the length can be useful and may be better to not remove from the error string.

stickies-v commented at 10:55 AM on September 4, 2024:

Also, you added len reporting in a similar place recently in #30377 (review),

I don't quite think this is similar. My suggestion in #30377 (review) was to use less code to have more accurate and helpful error reporting, triggered only when that particular requirement is breached. In this PR, we're talking about adding more code to provide context that may not be relevant (and thus make the message less useful) because the message is not specific to the error, and it is already easily resolved by the user since the necessary context (i.e. the input value) is provided (which seems to not be the case in #9040 which you linked)?

It seems we have different views on the benefit of the behaviour change. Since the behaviour change is orthogonal to the goal of this PR to remove uint256S() (I just included it as a little cleanup), I've reverted it, making the commit a refactor commit, reducing the LoC diff.

maflcko commented at 1:21 PM on September 4, 2024:

Sorry for misunderstanding, but I think an error message of

"txid must be string of 64 hex digits (not '1d999634f709c98f242f2e553bf0fdac7deb6f972c44c8c1b4d576d564acc7f')" is unclear whether the error is that there are non-hex digits or whether there are missing or extraneous characters. And it is not trivial to see from looking at it.

A proposed error message of "txid must be string of 64 hex digits (not '1d999634f709c98f242f2e553bf0fdac7deb6f972c44c8c1b4d576d564acc7f' of len $num)" whether the length was wrong or the hex-property.

But maybe I am missing something, so anything seems fine here if other reviewers disagree.

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

I tend to agree that the length is superfluous (we're usually not that user friendly with our error messages) and in case of error the user would likely copy-paste again which would likely solve the error. I'm fine with both, just don't revert anything @stickies-v :)

in src/rpc/util.cpp:103 in abcdd3898f outdated

 100 | @@ -101,12 +101,10 @@ CFeeRate ParseFeeRate(const UniValue& json)
 101 |  
 102 |  uint256 ParseHashV(const UniValue& v, std::string_view name)

l0rinc commented at 5:34 PM on August 31, 2024:

it seems to me ParseHashV doesn't have dedicated unit tests (only fuzz, without validation). If you don't think it's overkill, could you please add one before this commit - to make absolutely sure that the two behaviors are the same...

(Q: what should happen when v.isStr() is false?)

stickies-v commented at 11:54 AM on September 2, 2024:

I'm not sure if it makes sense adding it to this branch, because I think the tests should be removed after switching ParseHashV to use uint256::FromHex() so that would probably create a bit too much unnecessary churn, but here's a commit (to which you can easily add additional inputs) you can cherry-pick that verifies the parity of both functions (edit: added to PR description):

https://github.com/stickies-v/bitcoin/commit/1f2b0fa86db2ed65476b68417aa1bf4c26026a19

(Q: what should happen when v.isStr() is false?)

I don't think this commit involves v.isStr(), I presume you mean v.get_str()? It appears to me that behaviour is unchanged, and both on master as well as in this PR a type_error would be thrown if v is not a string type?

l0rinc commented at 3:22 PM on September 2, 2024:

I presume you mean v.get_str()

No, I meant when it doesn't actually store a string, what should be the error.

It appears to me that behaviour is unchanged

Yes, it's just that it's a hidden error which would be eliminated if ParseHashV accepted a string_view instead of a UniValue. I understand if it's outside the scope, just noting that there's another path that we might want to consider when evaluating the correctness of the method. Pulling the method to the parameter would make the type errors slightly more informative and would reduce the get_str calls inside ParseHashV - possibly opening this method up for other uses where we only have a string instead of a UniValue.

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

No, I meant when it doesn't actually store a string, what should be the error.

Yes, that's what the v.get_str() call checks internally, and it throws a type_error if so. Have a look at the UniValue::get_str() implementation? (Note that user input type validation happens in RPCHelpMan::HandleRequest(), but of course this can still happen through internal bugs).

I understand if it's outside the scope

Yeah it's quite orthogonal to this PR, given the number of callsites of ParseHashV(), so I'm not going to change that here. I am generally in favour of pushing UniValue user input parsing into stricter types further back to the edges, though.

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

Have a look at the UniValue::get_str() implementation

I already checked all of that, that's why I was asking if we'd rather they throw on the call site. You've already answered that you think it's orthogonal.

I am generally in favour of pushing UniValue user input parsing into stricter types further back to the edges, though.

Yes, that's what I was saying basically - though I needed 2 rounds to understand it myself why this was bothering me.

in src/test/arith_uint256_tests.cpp:26 in f7b4ab871d outdated

  22 | @@ -23,8 +23,8 @@ static inline arith_uint256 arith_uint256V(const std::vector<unsigned char>& vch
  23 |  {
  24 |      return UintToArith256(uint256(vch));
  25 |  }
  26 | -// Takes a number written in hex (with most significant digits first).
  27 | -static inline arith_uint256 arith_uint256S(std::string_view str) { return UintToArith256(uint256S(str)); }
  28 | +// Construct an arith_uint256 from a 64 hex digits string (with most significant digits first).

l0rinc commented at 5:36 PM on August 31, 2024:

I'm not a big fan of expressing code structure with dead comments - would it be possible to reorganize the code instead so that we don't need the comment? E.g. by giving a more meaningful parameter name such as std::string_view hex? The 64 char seems unnecessary here, it's a simple calculation from the return value.

stickies-v commented at 12:02 PM on September 2, 2024:

I'm not a big fan of expressing code structure with dead comments

I agree, but I'm not sure this is a dead comment? I don't think hex would be a better parameter name, because it's not validated to be a hex string yet, could be anything. The "64 hex digit" bit helps developers that want to use this utility function quickly understand how to use it, as does the "most significant digits first" bit.

I'd be okay with not having the string too, but it feels rather arbitrary so I'd rather leave as is (with slight fix).

stickies-v commented at 1:39 PM on September 2, 2024:

Resolved by removing the function altogether.

in src/test/arith_uint256_tests.cpp:101 in f7b4ab871d outdated

 110 | -    BOOST_CHECK(R1L == arith_uint256S(R1ArrayHex));
 111 | -    BOOST_CHECK(arith_uint256(R1L) == R1L);
 112 | -    BOOST_CHECK((arith_uint256(R1L^R2L)^R2L) == R1L);
 113 | -    BOOST_CHECK(arith_uint256(ZeroL) == ZeroL);
 114 | -    BOOST_CHECK(arith_uint256(OneL) == OneL);
 115 | +    BOOST_CHECK_EQUAL(arith_uint256S(R1L.ToString()), R1L);

l0rinc commented at 5:48 PM on August 31, 2024:

do all of these tests still make sense without the extra 0x prefix (and whitespace)? Seems like that's what some of them were testing here basically

stickies-v commented at 1:39 PM on September 2, 2024:

I had the same intuition, but kept them to be safe. Looking at it again, I agree with your view that they can be safely removed, so I've added a new commit 73a126f4f59470d839905d0eaaa26f689f7f2ba1 to do that and clean up the arith_uint256S() function entirely. Thanks!

l0rinc commented at 5:52 PM on August 31, 2024: contributor

Approach ACK

stickies-v force-pushed on Sep 2, 2024

stickies-v commented at 1:38 PM on September 2, 2024: contributor

Force-pushed to add commit 73a126f4f59470d839905d0eaaa26f689f7f2ba1 to remove the test-only arith_uint256S() function entirely. Since arith_uint256 does not have any string string constructors, it uses uint256 constructors, and those are already unit tested. In some cases, a string constructor can be avoid entirely, e.g. by using the arith_uint256 uint64_t constructor.

Addresses @l0rinc's review comment about unnecessary tests and unnecessary docstring.

stickies-v force-pushed on Sep 2, 2024

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

And force-pushed again to rebase on top of #30377 to avoid silent merge conflicts e.g. because of the now lowercase-only uint256 hex string constructor.

in src/test/arith_uint256_tests.cpp:104 in 5621b98fae outdated

 101 | -    BOOST_CHECK(arith_uint256S("   0x" + R1L.ToString() + "   ") == R1L);
 102 | -    BOOST_CHECK(arith_uint256S("") == ZeroL);
 103 | -    BOOST_CHECK(arith_uint256S("1") == OneL);
 104 | -    BOOST_CHECK(R1L == arith_uint256S(R1ArrayHex));
 105 | +    // Copy Constructor
 106 |      BOOST_CHECK(arith_uint256(R1L) == R1L);

l0rinc commented at 3:42 PM on September 2, 2024:

since we've already included setup_common which contains std::ostream& operator<<(std::ostream& os, const arith_uint256& num) we might as well use BOOST_CHECK_EQUAL here:

diff --git a/src/test/arith_uint256_tests.cpp b/src/test/arith_uint256_tests.cpp
index a434a1d401..9a6307c1bd 100644
--- a/src/test/arith_uint256_tests.cpp
+++ b/src/test/arith_uint256_tests.cpp
@@ -64,51 +64,50 @@ static std::string ArrayToString(const unsigned char A[], unsigned int width)
 
 BOOST_AUTO_TEST_CASE( basics ) // constructors, equality, inequality
 {
-    BOOST_CHECK(1 == 0+1);
     // constructor arith_uint256(vector<char>):
-    BOOST_CHECK(R1L.ToString() == ArrayToString(R1Array,32));
-    BOOST_CHECK(R2L.ToString() == ArrayToString(R2Array,32));
-    BOOST_CHECK(ZeroL.ToString() == ArrayToString(ZeroArray,32));
-    BOOST_CHECK(OneL.ToString() == ArrayToString(OneArray,32));
-    BOOST_CHECK(MaxL.ToString() == ArrayToString(MaxArray,32));
-    BOOST_CHECK(OneL.ToString() != ArrayToString(ZeroArray,32));
+    BOOST_CHECK_EQUAL(R1L.ToString(), ArrayToString(R1Array, 32));
+    BOOST_CHECK_EQUAL(R2L.ToString(), ArrayToString(R2Array, 32));
+    BOOST_CHECK_EQUAL(ZeroL.ToString(), ArrayToString(ZeroArray, 32));
+    BOOST_CHECK_EQUAL(OneL.ToString(), ArrayToString(OneArray, 32));
+    BOOST_CHECK_EQUAL(MaxL.ToString(), ArrayToString(MaxArray, 32));
+    BOOST_CHECK_NE(OneL.ToString(), ArrayToString(ZeroArray, 32));
 
     // == and !=
-    BOOST_CHECK(R1L != R2L);
-    BOOST_CHECK(ZeroL != OneL);
-    BOOST_CHECK(OneL != ZeroL);
-    BOOST_CHECK(MaxL != ZeroL);
-    BOOST_CHECK(~MaxL == ZeroL);
-    BOOST_CHECK( ((R1L ^ R2L) ^ R1L) == R2L);
+    BOOST_CHECK_NE(R1L, R2L);
+    BOOST_CHECK_NE(ZeroL, OneL);
+    BOOST_CHECK_NE(OneL, ZeroL);
+    BOOST_CHECK_NE(MaxL, ZeroL);
+    BOOST_CHECK_EQUAL(~MaxL, ZeroL);
+    BOOST_CHECK_EQUAL((R1L ^ R2L) ^ R1L, R2L);
 
     uint64_t Tmp64 = 0xc4dab720d9c7acaaULL;
     for (unsigned int i = 0; i < 256; ++i)
     {
-        BOOST_CHECK(ZeroL != (OneL << i));
-        BOOST_CHECK((OneL << i) != ZeroL);
-        BOOST_CHECK(R1L != (R1L ^ (OneL << i)));
-        BOOST_CHECK(((arith_uint256(Tmp64) ^ (OneL << i) ) != Tmp64 ));
+        BOOST_CHECK_NE(ZeroL, OneL << i);
+        BOOST_CHECK_NE(OneL << i, ZeroL);
+        BOOST_CHECK_NE(R1L, R1L ^ (OneL << i));
+        BOOST_CHECK_NE(arith_uint256(Tmp64) ^ (OneL << i), Tmp64);
     }
-    BOOST_CHECK(ZeroL == (OneL << 256));
+    BOOST_CHECK_EQUAL(ZeroL, OneL << 256);
 
     // 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);
+    BOOST_CHECK_EQUAL(arith_uint256(R1L), R1L);
+    BOOST_CHECK_EQUAL(arith_uint256(R1L ^ R2L) ^ R2L, R1L);
+    BOOST_CHECK_EQUAL(arith_uint256(ZeroL), ZeroL);
+    BOOST_CHECK_EQUAL(arith_uint256(OneL), OneL);
 
     // uint64_t constructor
-    BOOST_CHECK_EQUAL((R1L & arith_uint256{0xffffffffffffffff}), arith_uint256{R1LLow64});
+    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);
-    tmpL = ~OneL; BOOST_CHECK(tmpL == ~OneL);
-    tmpL = ~R1L; BOOST_CHECK(tmpL == ~R1L);
-    tmpL = ~R2L; BOOST_CHECK(tmpL == ~R2L);
-    tmpL = ~MaxL; BOOST_CHECK(tmpL == ~MaxL);
+    arith_uint256 tmpL = ~ZeroL; BOOST_CHECK_EQUAL(tmpL, ~ZeroL);
+    tmpL = ~OneL; BOOST_CHECK_EQUAL(tmpL, ~OneL);
+    tmpL = ~R1L; BOOST_CHECK_EQUAL(tmpL, ~R1L);
+    tmpL = ~R2L; BOOST_CHECK_EQUAL(tmpL, ~R2L);
+    tmpL = ~MaxL; BOOST_CHECK_EQUAL(tmpL, ~MaxL);
 }
 
 static void shiftArrayRight(unsigned char* to, const unsigned char* from, unsigned int arrayLength, unsigned int bitsToShift)

I understand of course if you think some of these are unrelated, though some of the comments are weird, e.g. // constructor arith_uint256(vector<char>) doesn't seem to test any constructors directly.

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

I'm not touching any of those lines in this PR so I'd rather keep this PR a bit narrower.

-    BOOST_CHECK_EQUAL((R1L & arith_uint256{0xffffffffffffffff}), arith_uint256{R1LLow64});
+    BOOST_CHECK_EQUAL(R1L & arith_uint256{0xffffffffffffffff}, arith_uint256{R1LLow64});

Thanks, I'll patch this if I have to force-push.

(note: you can hide long code blocks with a <details>...</details> tag.)

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

you can hide long code blocks

Ah, I don't see them anymore, Refined Github extension does it automatically. I'll do it next time.

stickies-v commented at 11:38 AM on September 3, 2024:

Thanks, I'll patch this if I have to force-push.

Done!

l0rinc approved

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

Minor nit suggestions, ACK a9d19ae1aa06f14c1892bb4e37c3f023bc61b39b

in src/test/arith_uint256_tests.cpp:567 in a9d19ae1aa outdated

 562 | +       This makes some sense for arithmetic classes, but could be considered a bug
 563 | +       elsewhere. Disable the warning here so that the code can be tested, but the
 564 | +       warning should remain on as there will likely always be a better way to
 565 | +       express this.
 566 | +    */
 567 | +

hodlinator commented at 7:40 AM on September 3, 2024:

The move of this test to this file and changes to use raw integer literals make total sense.

meganit: If you re-touch - Might just remove this empty line as the comment above applies to the preprocessor logic beneath.

in src/test/uint256_tests.cpp:92 in a9d19ae1aa outdated

  99 | +    BOOST_CHECK_EQUAL(uint256::FromHex(R1L.ToString()).value(), R1L);
 100 | +    BOOST_CHECK_EQUAL(uint256::FromHex(R2L.ToString()).value(), R2L);
 101 | +    BOOST_CHECK_EQUAL(uint256::FromHex(ZeroL.ToString()).value(), ZeroL);
 102 | +    BOOST_CHECK_EQUAL(uint256::FromHex(OneL.ToString()).value(), OneL);
 103 | +    BOOST_CHECK_EQUAL(uint256::FromHex(MaxL.ToString()).value(), MaxL);
 104 | +    BOOST_CHECK_EQUAL(R1L, uint256::FromHex(R1ArrayHex).value());

hodlinator commented at 7:52 AM on September 3, 2024:

nit: I understand this matches the order before the PR but this specific line should probably have the arguments swapped.

    BOOST_CHECK_EQUAL(uint256::FromHex(R1ArrayHex).value(), R1L);

in src/test/uint256_tests.cpp:102 in a9d19ae1aa outdated

 120 | +    BOOST_CHECK_EQUAL(uint160::FromHex(R1S.ToString()).value(), R1S);
 121 | +    BOOST_CHECK_EQUAL(uint160::FromHex(R2S.ToString()).value(), R2S);
 122 | +    BOOST_CHECK_EQUAL(uint160::FromHex(ZeroS.ToString()).value(), ZeroS);
 123 | +    BOOST_CHECK_EQUAL(uint160::FromHex(OneS.ToString()).value(), OneS);
 124 | +    BOOST_CHECK_EQUAL(uint160::FromHex(MaxS.ToString()).value(), MaxS);
 125 | +    BOOST_CHECK_EQUAL(uint160::FromHex(R1ArrayHex + 24).value(), R1S); // we want the last 40 of 64 chars so skip the first 24

hodlinator commented at 7:57 AM on September 3, 2024:

Clarify code instead of adding comment?

    BOOST_CHECK_EQUAL(uint160::FromHex(std::string_view{R1ArrayHex + 24, 40}).value(), R1S);

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

Nice, didn't quite love the docstring and this is much clearer, thanks.

in src/test/uint256_tests.cpp:259 in a9d19ae1aa outdated

 254 | @@ -274,72 +255,12 @@ BOOST_AUTO_TEST_CASE( conversion )
 255 |      BOOST_CHECK_EQUAL(UintToArith256(OneL), 1);
 256 |      BOOST_CHECK_EQUAL(ArithToUint256(0), ZeroL);
 257 |      BOOST_CHECK_EQUAL(ArithToUint256(1), OneL);
 258 | -    BOOST_CHECK_EQUAL(arith_uint256(UintToArith256(uint256S(R1L.GetHex()))), UintToArith256(R1L));
 259 | -    BOOST_CHECK_EQUAL(arith_uint256(UintToArith256(uint256S(R2L.GetHex()))), UintToArith256(R2L));
 260 | +    BOOST_CHECK_EQUAL(arith_uint256(UintToArith256(uint256::FromHex(R1L.GetHex()).value())), UintToArith256(R1L));
 261 | +    BOOST_CHECK_EQUAL(arith_uint256(UintToArith256(uint256::FromHex(R2L.GetHex()).value())), UintToArith256(R2L));

hodlinator commented at 8:17 AM on September 3, 2024:

The whole conversion-test arguably belongs in the arith_uint256_tests.cpp file, which would allow complete removal of #include <arith_uint256.h> from this file. That would mirror the code under test, as uint256 does not depend on arith_uint256 but the reverse is true.

stickies-v commented at 11:41 AM on September 3, 2024:

I was considering moving it but then thought it was quite arbitrary, but you're right - tests mirroring how code is structured is good, so I've added a commit to move the conversion tests. Thanks!

hodlinator approved

hodlinator commented at 8:25 AM on September 3, 2024: contributor

ACK a9d19ae1aa06f14c1892bb4e37c3f023bc61b39b

uint256S() be gone! :)

Changes as a consequence of removal make sense. Good to see less string parsing testing going on in arith_uint256_tests.cpp (covered in recently added From(User)?Hex-tests). Arithmetic testing that was going on in uint256_tests.cpp has also moved into arith_uint256_tests.cpp (operator_with_self), could move conversion-test too (more in specific comment).

stickies-v force-pushed on Sep 3, 2024

stickies-v commented at 11:44 AM on September 3, 2024: contributor

Force-pushed to address all review comments by @l0rinc and @hodlinator, thanks for the review! Mostly small style nits, and also added 9ef049db5ea30f60d0b72714c42c74e2d816c820 to move uint256_tests/conversion to arith_uint256_tests since that better reflects how the code is organized.

hodlinator approved

hodlinator commented at 1:22 PM on September 3, 2024: contributor

re-ACK 9ef049db5ea30f60d0b72714c42c74e2d816c820

git range-diff master a9d19ae 9ef049d showed only small expected edits + new commit to move conversion-tests.

ctest -j <cores> --test-dir build passed.

DrahtBot requested review from l0rinc on Sep 3, 2024

l0rinc commented at 2:14 PM on September 3, 2024: contributor

reACK 9ef049db5ea30f60d0b72714c42c74e2d816c820

stickies-v force-pushed on Sep 4, 2024

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

Force-pushed to remove behaviour change in commit "rpc: use uint256::FromHex for ParseHashV", addressing @maflcko's concerns over behaviour change. I don't think this is necessarily an improvement, but I appreciate that's my subjective view and the (slight) behaviour change was orthogonal the goal of the PR anyway so it seems like the most pragmatic approach to make progress. Thanks for your review!

in src/rpc/util.cpp:110 in 95b3aca0eb outdated

 110 | -    return uint256S(strHex);
 111 | +    const auto expected_len{uint256::size() * 2};
 112 | +    if (expected_len != strHex.length()) {
 113 | +        throw JSONRPCError(RPC_INVALID_PARAMETER, strprintf("%s must be of length %d (not %d, for '%s')", name, expected_len, strHex.length(), strHex));
 114 | +    }
 115 | +    if (auto rv{uint256::FromHex(strHex)}) return *rv;

maflcko commented at 12:30 PM on September 4, 2024:

style nit in 95b3aca0eb3fc49cd99b8de028a676bb04e2157f: Could have the happy-case early-return in the second line of the function, like in a prior version of this pull request.

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

Ah yes that would be better, will update this in next force-push, thanks!

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

You mean something like:

uint256 ParseHashV(const UniValue& v, std::string_view name)
{
    const std::string& strHex(v.get_str());
    if (auto rv{uint256::FromHex(strHex)}){
        return *rv;
    } else 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));
    } else {
        throw JSONRPCError(RPC_INVALID_PARAMETER, strprintf("%s must be hexadecimal string (not '%s')", name, strHex));
    }
}

?

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

Done.

in src/test/arith_uint256_tests.cpp:101 in 948bb07715 outdated

  90 | @@ -97,27 +91,17 @@ BOOST_AUTO_TEST_CASE( basics ) // constructors, equality, inequality
  91 |      }
  92 |      BOOST_CHECK(ZeroL == (OneL << 256));
  93 |  
  94 | -    // String Constructor and Copy Constructor
  95 | -    BOOST_CHECK(arith_uint256S("0x" + R1L.ToString()) == R1L);

maflcko commented at 12:50 PM on September 4, 2024:

question in 948bb07715f9ef1d90b91dfbea4f487f755498f5: Is there a reason why the tests are dropped? I read the commit message and I understand that the comment says "String constructors", but I think it meant to say UintToArith256 testing from a string, which is not a test-only function.

This is my fault, because I forgot to update the comment in commit facf629ce8ff1d1f6d254dde4e89b5018f8df60e.

I also understand that there is a BOOST_AUTO_TEST_CASE(conversion) test. However, I think it only checks that a round-trip is a no-op and not the construction from string to uint256 and then UintToArith256.

I think it would be easier to review if the tests were kept, or if the commit message said where the equivalent UintToArith256 tests are happening.

My preference would be to just keep the tests, even if they are theoretically redundant.

Personally I just find it hard to review, because seemingly equivalent code is doing the reverse:

uint256 from a string_view and span are reversed, see #30377 (review)
the GetHex member method is the reverse of the HexStr helper
base_uint::CompareTo and base_blob::Compare are also different
...

So keeping the tests makes it trivial to review and ensures that no coverage is lost accidentally.

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

Some of these were reverted since it seemed to us that it's testing prefixes and whitespaces primarily, see #30773 (review)

maflcko commented at 1:36 PM on September 4, 2024:

I agree the prefix and whitespace testing is useless and the prefix and whitespace can simply be removed in the test cases, but it is not trivially clear to me that the UintToArith256 tests can be removed.

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

is not trivially clear to me that the UintToArith256 tests can be removed

+1 for restoring them in that case

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

I'm definitely happy to put the existing tests back in if it makes review easier, but I want to make sure I understand your concern, so let me try to summarize it:

Prior to https://github.com/bitcoin/bitcoin/commit/facf629ce8ff1d1f6d254dde4e89b5018f8df60e, to construct a arith_uint256 from a hex string, one would use the arith_uint256 string constructor, and as such we had tests covering that constructor. Since https://github.com/bitcoin/bitcoin/commit/facf629ce8ff1d1f6d254dde4e89b5018f8df60e that constructor is removed, and now the de facto approach to construct a arith_uint256 from a hex string is to first construct a uint256 from a hex string, and then convert it with UintToArith256(). So, rather than removing the string construction unit tests entirely, you kept them by testing this new "roundabout" approach instead, since that's how these functions are used in practice.

Is it fair to summarize that your concern is that even though the tests I remove here are theoretically not necessary since we should have equivalent unit test coverage on the 2 underlying functions (uint256::FromHex() and UintToArith256()), you'd rather keep them because ensuring that the unit test coverage is indeed equivalent is burdensome and makes this PR harder to review?

If my summary is correct, and it seems the main concern is about UintToArith256() test coverage (and not so much uint256::FromHex() coverage, I think my latest force-push should address your concerns by vastly expanding (and at the same time simplifying) the conversion tests in 6cfa7f4a0361d9c396d1c5bd71849295baf6290d?

maflcko commented at 7:39 AM on September 5, 2024:

Is it fair to summarize that your concern is that even though the tests I remove here are theoretically not necessary since we should have equivalent unit test coverage on the 2 underlying functions (uint256::FromHex() and UintToArith256()), you'd rather keep them because ensuring that the unit test coverage is indeed equivalent is burdensome and makes this PR harder to review?

Yes. At least I had difficulty seeing that the test coverage does not decrease, and the tests run so fast that in the case where they are redundant, no one should notice.

I'd say it is mostly about review. I like this pull request, but I don't want to spend the majority of the review time here on unit test refactoring, where it is unclear what the clear benefit will be now. Also, if the unit tests are changed soon again, maybe the more involved changes can be postponed?

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

and the tests run so fast that in the case where they are redundant, no one should notice.

Absolutely, the cleanup is to avoid having tests that have unclear purpose, making them easier to maintain going forward. Performance is not the goal here.

I think it's impossible for this PR to not have significant changes to the unit tests, so I feel like since the review is happening anyway the test improvements I've introduced here make sense and add little overhead?

That said, I'm unsure if you're waiting for me to revert things or if you're happy with the current approach, so just to make it explicit could you please clarify if you'd prefer:

the conversion test changes to be reverted
the "string constructor" tests to be re-added
moving the conversion and operator_with_self to arith_uint256_tests.cpp to be reverted (not explicitly discussed, but I'm inferring from your message that this might be a concern)

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

so I feel like since the review is happening anyway the test improvements I've introduced here make sense and add little overhead?

Agree, let's leave the campground cleaner than we found it.

maflcko commented at 4:45 PM on September 5, 2024:

the "string constructor" tests to be re-added

That'd be my preference for now. Even if the tests are redundant, it seems cleaner to keep them for now to make review trivial (and maybe for symmetry with the uint256 tests).

There are many more obvious redundant and useless tests, like BOOST_CHECK(1 == 0+1); in this test case, so a lot more could be changed, but maybe removing tests can be done in a follow-up, or not at all.

But none of this is a blocker, so fully up to you. Feel free to close this thread.

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

Okay, I've re-added the "string constructor" tests. I think they can be removed later, but that's obviously not critical either. Thanks for explaining your concerns.

maflcko commented at 1:21 PM on September 4, 2024: member

Concept ACK.

I left another question in the second commit. Sorry for the incremental review.

in src/rpc/util.cpp:109 in da4377dc2a outdated

 109 | -        throw JSONRPCError(RPC_INVALID_PARAMETER, strprintf("%s must be hexadecimal string (not '%s')", name, strHex));
 110 | -    return uint256S(strHex);
 111 | +    const auto expected_len{uint256::size() * 2};
 112 | +    if (expected_len != strHex.length()) {
 113 | +        throw JSONRPCError(RPC_INVALID_PARAMETER, strprintf("%s must be of length %d (not %d, for '%s')", name, expected_len, strHex.length(), strHex));
 114 | +    }

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

Nit1: consider narrowing the scope of expected: Nit2: usually we compare the actual against the expected (switched the order):

    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));
    }

No strong feelings either way, feel free to ignore if you don't like it

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

Adopted both suggestions, thanks!

l0rinc commented at 1:48 PM on September 4, 2024: contributor

ACK da4377dc2ad0f495ebb97722d6cc2a95850363fa

I didn't mind the previous version either, but maybe this preserves more of what's not strictly related to the PR. I think it's an improvement as-is, but I will gladly reack if you decide to include the other reviews as well.

DrahtBot requested review from maflcko on Sep 4, 2024

DrahtBot requested review from hodlinator on Sep 4, 2024

refactor: rpc: use uint256::FromHex for ParseHashV

uint256S() is deprecated for being unsafe, and will be removed
in a future commit.

f51b237723

stickies-v force-pushed on Sep 4, 2024

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

Force-pushed to increase uint256/arith_uint256 conversion tests (hopefully) addressing @maflcko's concern, (or at the very least just improving test coverage). To avoid changing the same lines multiple times, I've squashed the conversion move commit into 6cfa7f4a0361d9c396d1c5bd71849295baf6290d.

Also adopted two style nits on happy-path and limiting variable scope.

in src/test/arith_uint256_tests.cpp:564 in b60013ed71 outdated

 551 | @@ -571,4 +552,50 @@ BOOST_AUTO_TEST_CASE( getmaxcoverage ) // some more tests just to get 100% cover
 552 |      CHECKBITWISEOPERATOR(R1,~R2,&)
 553 |  }
 554 |  
 555 | +BOOST_AUTO_TEST_CASE(conversion)

l0rinc commented at 4:18 PM on September 4, 2024:

so these are basically roundtrip tests now, right? These are very valuable (could even be a fuzz test now), but could we add back some of the hardcoded ones to make sure they're not just self-referentially correct (i.e. circular reasoning):

    BOOST_CHECK_EQUAL(ArithToUint256(ZeroL), uint256::ZERO);
    BOOST_CHECK_EQUAL(ArithToUint256(OneL), uint256::ONE);
    BOOST_CHECK_EQUAL(UintToArith256(uint256::ZERO), ZeroL);
    BOOST_CHECK_EQUAL(UintToArith256(uint256::ONE), OneL);

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

so these are basically roundtrip tests now, right?

I think BOOST_CHECK_EQUAL(UintToArith256(ArithToUint256(arith)), arith); is the only roundtrip?

but could we add back some of the hardcoded ones to make sure they're not just self-referentially correct (i.e. circular reasoning):

Isn't that covered here? https://github.com/bitcoin/bitcoin/pull/30773/files#diff-d06d0fcd312512f190cc0d05450dd55973c60aeb8d57ed2d705e7beaa383f386R565-R568

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

Not exactly, this looks more like a trampoline than simple hard-coded examples for just ArithToUint256 and UintToArith256. Both sides depend on the same parameter, which seems circular to me-at least from a black-box perspective. However, I’m fine with this if you think it’s sufficient.

stickies-v commented at 12:24 AM on September 5, 2024:

I'm sorry, I just don't see how those hardcoded tests are any different from the ones I linked. ZeroL is the same as arith_uint256{0}, and uint256::ZERO is the same as uint256{0}. Besides these aliases being irrelevant for a conversion test, their equivalence is tested in other unit tests already. What am I missing?

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

You already know at the implementation, that's why it seems the same. The test would also pass for a simple re-cast roundtrip:

auto ArithToUint256_mock(const arith_uint256 &a) { return *reinterpret_cast<const uint256*>(&a); }
auto UintToArith256_mock(const uint256 &a) { return *reinterpret_cast<const arith_uint256*>(&a); }
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_mock(ArithToUint256_mock(arith)), arith);
        BOOST_CHECK_EQUAL(UintToArith256_mock(u256), arith);
        BOOST_CHECK_EQUAL(u256, ArithToUint256_mock(arith));
        BOOST_CHECK_EQUAL(ArithToUint256_mock(arith).GetHex(), UintToArith256_mock(u256).GetHex());
    }

    for (const uint8_t num : {0, 1, 0xff}) {
        BOOST_CHECK_EQUAL(UintToArith256_mock(uint256{num}), arith_uint256{num});
        BOOST_CHECK_EQUAL(uint256{num}, ArithToUint256_mock(arith_uint256{num}));
    }
}

That's why I though we should cover it with hard-coded examples which cannot cheat by passing the parameter around. (note, the previous hard-coded ones also pass for the above _mock methods, we should find better ones to obviate why the mocks are incorrect)

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

You already know at the implementation

At which points are these test relying on implementation?

Thanks for providing an example. If you have a diff that shows where the "hardcoded" tests catch something that "my" tests don't, that would really help. I struggle to see how BOOST_CHECK_EQUAL(ArithToUint256(ZeroL), uint256::ZERO); is any different from BOOST_CHECK_EQUAL(ArithToUint256(arith_uint256{0}), uint256{0}); (or how that substitution is relying on implementation details, which you seem to be hinting at).

stickies-v commented at 8:03 PM on September 5, 2024:

The test would also pass for a simple re-cast roundtrip:

I'm a bit confused by the example _mock functions. I think reinterpret casting a std::array<uint8_t, 32> <-> uint32_t[8] can actually be a (unreliable and unsafe but depending on memory alignment sometimes correct) way of implementing the conversion functions, since both types are 256 contiguous bits of uint data? So depending on your platform, I think this passing the tests doesn't really provide much information?

Sorry if I'm being dim, I'm really trying to understand your concern.

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

Sorry if I'm being dim, I'm really trying to understand your concern.

I can see that and I appreciate it.

I think BOOST_CHECK_EQUAL(UintToArith256(ArithToUint256(arith)), arith); is the only roundtrip?

Aren't these roundtrip as well because of GetHex/FromHex?

const auto u256{uint256::FromHex(arith.GetHex()).value()};
BOOST_CHECK_EQUAL(UintToArith256(u256), arith);
BOOST_CHECK_EQUAL(u256, ArithToUint256(arith));
BOOST_CHECK_EQUAL(ArithToUint256(arith).GetHex(), UintToArith256(u256).GetHex());

I think this passing the tests doesn't really provide much information?

It's not trivial to find a counter example here which would still pass - but that's exactly my concern, that's it's too complicated, that if e.g. GetHex would accidentally zero out the internal state, many of these would pass, but comparing against a constant value is harder to trick, since only one side can change (easier to grasp than nested methods and parameters and states).

E.g. imagine having a trim method, and we're asserting that when it doesn't start with a space, the length is the same as the original (BOOST_CHECK_EQUAL(Trim(orig).size(), orig.size());). Would you trust this Trim method without having a concrete example like BOOST_CHECK_EQUAL(Trim("abc"), "abc"); and BOOST_CHECK_EQUAL(Trim(" abc "), "abc");, etc?

In fuzz testing the above ones are fine, since we're checking invariants (i.e. constant properties) of the method (e.g. that a trimmed method's size can never be greater than the original), but in unit testing we usually provide simple examples that serve as documentation as well.

So all in all, I think the above examples are very good property-based tests, but I'm missing simple unit tests that can document the behavior of the method in a trivial way (i.e. when they fail I want to pinpoint the reason quickly).

But I've already ACK-ed this, it's not a blocker, if this is still doesn't make sense, please mark it as resolved, it's not that important :)

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

but I'm missing simple unit tests that can document the behavior of the method in a trivial way

If we assume that GetHex() is a complete representation of each class' internal state, then feel like it's reasonable (even if suboptimal) to rely on using that method do assess the correctness of the conversion function, if GetHex() is well unit tested. Also note that we have tests that don't rely on GetHex() / FromHex() at all, i.e.:

    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);
    }

That said, I think we both agree that having trivial checks that rely as little as possible on other functions is good. I think the for (uint8_t num : {0, 1, 0xff}) { tests are a good example of that, but they could be expanded by constructing more uint256's from hardcoded arrays. I think my confusion arose from the fact that we already are doing such "hardcoded" conversion tests (for {0, 1, 0xff}), but this could be improved upon by adding more uint256s that are larger than 0xff?

While I don't object to that, I think it's not really necessary to this PR, so since it's not blocking for you I think it makes sense to leave it at this.

l0rinc approved

l0rinc commented at 4:21 PM on September 4, 2024: contributor

ACK b60013ed71b697bab98884aa475fb64ae7736c2e

in src/test/uint256_tests.cpp:274 in 6cfa7f4a03 outdated

 269 | -    BOOST_CHECK_EQUAL(ArithToUint256(UintToArith256(ZeroL)), ZeroL);
 270 | -    BOOST_CHECK_EQUAL(ArithToUint256(UintToArith256(OneL)), OneL);
 271 | -    BOOST_CHECK_EQUAL(ArithToUint256(UintToArith256(R1L)), R1L);
 272 | -    BOOST_CHECK_EQUAL(ArithToUint256(UintToArith256(R2L)), R2L);
 273 | -    BOOST_CHECK_EQUAL(UintToArith256(ZeroL), 0);
 274 | -    BOOST_CHECK_EQUAL(UintToArith256(OneL), 1);

hodlinator commented at 9:44 PM on September 5, 2024:

nit: The old conversion test compared against integer literals here. The methods test in arith_uint256_tests.cpp compares against 0, so this is covered in part.

Still, what do you think about adding something like:

diff --git a/src/test/arith_uint256_tests.cpp b/src/test/arith_uint256_tests.cpp
index f596951dac..d437b6a54b 100644
--- a/src/test/arith_uint256_tests.cpp
+++ b/src/test/arith_uint256_tests.cpp
@@ -565,6 +565,8 @@ BOOST_AUTO_TEST_CASE(conversion)
     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_CHECK_EQUAL(arith_uint256{num}, num);
     }
 }

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

I don't think BOOST_CHECK_EQUAL(arith_uint256{num}, num); is a conversion test, so I'm going to leave that out here, but I like the BOOST_CHECK_EQUAL(UintToArith256(uint256{num}), num); suggestion, thanks!

hodlinator approved

hodlinator commented at 9:47 PM on September 5, 2024: contributor

re-ACK b60013ed71b697bab98884aa475fb64ae7736c2e

git range-diff master 9ef049d b60013

Collapsed conversion test change into one commit.

New ParseHashV commit is more user-friendly.

test: remove test-only arith_uint256S

Tests that are solely testing constructing from a hex string
are dropped, others are modified to use a uint256 constructor
or the arith_uint256 uint64_t constructor.

Since an arith_uint256 can not be constructed from a string
directly, we need to ensure that test coverage on
UintToArith256(uint256::FromHex()) is not reduced.
uint256::FromHex() already has good test coverage, but
the test coverage on UintToArith256() and ArithToUint256()
is increased in this commit by upgrading the `conversion`
test case.

Moreover, since `uint256.h` does not have any dependencies
on `arith_uint256.h`, the conversion tests are moved to
`arith_uint256_tests.cpp` so the dependency can be cleaned
up entirely in a future commit.

adc00ad728

test: remove test-only uint256S

uint256S was previously deprecated for being unsafe. All non-test
usage has already been removed in earlier commits.

1. Tests now use uint256::FromHex() or other constructors wherever
possible without further modification.
2. Tests that can't use uint256::FromHex() because they use input
with non-hex digit characters are
  a) modified by dropping the non-hex digit characters if that
     provides useful test coverage.
  b) dropped if the test without non-hex digit characters does
     not provide useful test coverage, e.g. because it is now
     duplicated.

Additionally, use BOOST_CHECK_EQUAL where relevant on touched lines
to make error messages more readable.

62cc4656e2

test: remove test-only uint160S

uint160S is a test-only function, and testing input that
is not allowed in uint160::FromHex() is superfluous.

Tests that can't use uint160::FromHex() because they use input
with non-hex digit characters are
a) modified by dropping the non-hex digit characters if that
provides useful test coverage.
b) dropped if the test without non-hex digit characters does
not provide useful test coverage, e.g. because it is now
duplicated.

c6c994cb2b

test: move uint256_tests/operator_with_self to arith_uint256_tests

move/formatting-only change.

These tests do not cover uint256, so move them to the appropriate
test suite. Additionally, apply clang-format suggestions.

43cd83b0c7

stickies-v force-pushed on Sep 6, 2024

stickies-v commented at 3:56 PM on September 6, 2024: contributor

Force-pushed to add back in some previously removed unit tests to help make this PR easier to review, and added an extra conversion test. Test-only changes, so hopefully pretty straightforward to re-review. Thanks, everyone!

DrahtBot added the label CI failed on Sep 7, 2024

in src/test/fuzz/integer.cpp:143 in 43cd83b0c7

 139 | @@ -140,7 +140,7 @@ FUZZ_TARGET(integer, .init = initialize_integer)
 140 |  
 141 |      const arith_uint256 au256 = UintToArith256(u256);
 142 |      assert(ArithToUint256(au256) == u256);
 143 | -    assert(uint256S(au256.GetHex()) == u256);
 144 | +    assert(uint256::FromHex(au256.GetHex()).value() == u256);

l0rinc commented at 11:59 AM on September 7, 2024:

nit1: you might as well delete the empty (void)au256.GetHex(); a few lines below nit2: value() is redundant here (i.e. assert(uint256::FromHex(au256.GetHex()) == u256); suffices) since std::optional has an ==:

operator==(const optional<_Tp>& __x, const _Up& __v)
{
    return static_cast<bool>(__x) ? *__x == __v : false;
}

stickies-v commented at 10:06 AM on September 9, 2024:

nit1: you might as well delete the empty (void)au256.GetHex(); a few lines below

I'm not sure if that would be an improvement, though. I think having a dedicated test on each method is nice and consistent, so I think even though it's redundant keeping both calls seems sensible to me to e.g. avoid accidentally removing coverage when uint256::FromHex() is removed?

nit2: value() is redundant here

ah right, thanks, will update if I have to force-push.

l0rinc commented at 11:23 AM on September 9, 2024:

having a dedicated test on each method is nice and consistent

But it's not validating anything (except for severe exceptions), it's just an empty call to increase line coverage (i.e. kinda' fake confidence boost). But in your case even the return value is validated, so there's no point in leaving the empty one.

l0rinc approved

l0rinc commented at 12:01 PM on September 7, 2024: contributor

reACK 43cd83b0c7ba436b8ffc83d8a65e935714dffe70 Thanks for your perseverance! (note: the build failure seems unrealated)

DrahtBot requested review from hodlinator on Sep 7, 2024

hodlinator approved

hodlinator commented at 8:17 PM on September 7, 2024: contributor

re-ACK 43cd83b0c7ba436b8ffc83d8a65e935714dffe70

git range-diff 43cd83b~5 b60013 43cd83b

Added back slightly useless "Construct from hex string" section to make the PR more straight-forward to review (and commit history arguably cleaner as a consequence).
Added half of my suggestion for bringing back testing against integer literals in conversion test. :+1:

(Passed ctest locally).

Agree with l0rinc's nits though (https://github.com/bitcoin/bitcoin/pull/30773#discussion_r1748063177). Happy to re-ACK again if you incorporate them.

ryanofsky approved

ryanofsky commented at 7:40 PM on September 10, 2024: contributor

Code review ACK 43cd83b0c7ba436b8ffc83d8a65e935714dffe70. 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

ryanofsky merged this on Sep 10, 2024

ryanofsky closed this on Sep 10, 2024

stickies-v deleted the branch on Sep 10, 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 10, 2025