refactor: remove deprecated TxidFromString() in favour of transaction_identifier::FromHex() #30532

stickies-v · 2024-07-26T15:02:08Z

Since fab6ddb, TxidFromString() has been deprecated because it is less robust than the transaction_identifier::FromHex() introduced in the same PR. 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.

In this PR, TxidFromString is removed completely to clean up the code and prevent further unsafe usage. Unit and fuzz test coverage on uint256::FromHex() and functions that wrap it is increased.

Note: TxidFromSring allowed users to prefix strings with "0x", this is no longer allowed for transaction_identifier::FromHex(), so a helper function for input validation may prove helpful in the future (this overlaps with the uint256::uint256S() vs uint256::FromHex() future cleanup). It is not relevant to this PR, though, besides the fact that this unused (except for in tests) functionality is removed.

The only users of TxidFromString are:

test, where it is straightforward to drop in the new FromHex() methods without much further concern
qt coincontrol. There is no need for input validation here, but txids are not guaranteed to be 64 characters. This is already handled by the existing code, so again, using FromHex() here seems quite straightforward.

Addresses @maflcko's suggestion: #30482 (comment)

Also removes WtxidFromString(), which is a test-only helper function.

Testing GUI changes

To test the GUI coincontrol affected lines, regtest is probably the easiest way to quickly get some test coins, you can use e.g.

alias cli="./src/bitcoin-cli -regtest"
cli createwallet "coincontrol"
# generate 10 spendable outputs on 1 address
cli generatetoaddress 10 $(cli -rpcwallet=coincontrol getnewaddress)
# generate 10 spendable outputs on another address
cli generatetoaddress 10 $(cli -rpcwallet=coincontrol getnewaddress)
# make previous outputs spendable
cli generatetoaddress 100 $(cli -rpcwallet=coincontrol getnewaddress)

DrahtBot · 2024-07-26T15:02:11Z

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	hodlinator, maflcko, paplorinc, TheCharlatan
Concept ACK	glozow

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

hodlinator

Concept ACK 2cad2ef

nit regarding PR summary:
Specifically, it is lacking length checks to ensure input is exactly 64 characters.
->
Specifically, it does it's best 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.

hodlinator · 2024-07-26T21:31:01Z

src/qt/coincontroldialog.cpp

@@ -240,7 +235,7 @@ void CoinControlDialog::lockCoin()
    if (contextMenuItem->checkState(COLUMN_CHECKBOX) == Qt::Checked)
        contextMenuItem->setCheckState(COLUMN_CHECKBOX, Qt::Unchecked);

-    COutPoint outpt(TxidFromString(contextMenuItem->data(COLUMN_ADDRESS, TxHashRole).toString().toStdString()), contextMenuItem->data(COLUMN_ADDRESS, VOutRole).toUInt());
+    COutPoint outpt(Txid::FromHex(contextMenuItem->data(COLUMN_ADDRESS, TxHashRole).toString().toStdString()).value(), contextMenuItem->data(COLUMN_ADDRESS, VOutRole).toUInt());


This will now throw std::bad_optional_access instead of.. trying to make sense of a potentially non-conforming hex string and continuing. I guess that's fair. :) Not sure on the guarantees of a valid hex-string here.

There's no user input here, this dialog just shows coins that are already in the wallet (and thus validated), so I think this is a safe change?

Probably safe. I clicked around a bunch in the Coin Control dialog in both tree & list nodes. Wasn't able to have the debugger detect anything that failed to parse as txid's in any of these functions except for when right-clicking the parent tree item to have the menu be shown, correctly disabling lock/unlock actions.

src/test/uint256_tests.cpp

src/qt/coincontroldialog.cpp

DrahtBot · 2024-07-29T17:12:18Z

🚧 At least one of the CI tasks failed.
_{Debug: https://github.com/bitcoin/bitcoin/runs/28061801420}

Hints

Make sure to run all tests locally, according to the documentation.

The failure may happen due to a number of reasons, for example:

Possibly due to a silent merge conflict (the changes in this pull request being
incompatible with the current code in the target branch). If so, make sure to rebase on the latest
commit of the target branch.
A sanitizer issue, which can only be found by compiling with the sanitizer and running the
affected test.
An intermittent issue.

Leave a comment here, if you need help tracking down a confusing failure.

stickies-v · 2024-07-29T17:16:16Z

Force pushed to:

increase unittest coverage on uint256::FromHex(), as well as methods that wrap it (Txid::FromHex() and Wtxid::FromHex())
add fuzz target uint256_fromhex

nit regarding PR summary:

Thanks, updated PR description.

src/test/fuzz/uint256.cpp

hodlinator · 2024-07-29T18:27:36Z

src/test/uint256_tests.cpp

+    TestFromHex64Chars<uint256>();
+    TestFromHex64Chars<Txid>();
+    TestFromHex64Chars<Wtxid>();


Very nice to see the additional coverage!

IMHO it is overkill to be testing the Txid/Wtxid clients of FromHex here.

Would you prefer we don't test {T, Wt}xid::FromHex() at all, or in a different way? At the moment these won't catch anything as {T, Wt}xid::FromHex() is just wrapping the uint256 one, but I thought it'd be prudent to add the tests already for when the wrappers are reimplemented in the future for whatever reason, and this approach doesn't add much overhead? So, just a way to enforce the interface remains stable?

In reply to https://github.com/bitcoin/bitcoin/pull/30532/files#r1696752519:

I'm not too worried about {T, Wt}xid::FromHex diverging in behavior. But maybe someone else can chime in to tie-break.

Welcoming more input here, yeah. To draw a parallel: TxidFromString was also "just a wrapper".

Edit: my biggest concern with this code is actually that we're testing transaction_identifier logic in uint256 tests, but creating a header just for this seems like too much of a hassle atm.

src/qt/coincontroldialog.cpp

hodlinator · 2024-07-29T21:37:47Z

src/qt/coincontroldialog.cpp

@@ -240,7 +235,7 @@ void CoinControlDialog::lockCoin()
    if (contextMenuItem->checkState(COLUMN_CHECKBOX) == Qt::Checked)
        contextMenuItem->setCheckState(COLUMN_CHECKBOX, Qt::Unchecked);

-    COutPoint outpt(TxidFromString(contextMenuItem->data(COLUMN_ADDRESS, TxHashRole).toString().toStdString()), contextMenuItem->data(COLUMN_ADDRESS, VOutRole).toUInt());
+    COutPoint outpt(Txid::FromHex(contextMenuItem->data(COLUMN_ADDRESS, TxHashRole).toString().toStdString()).value(), contextMenuItem->data(COLUMN_ADDRESS, VOutRole).toUInt());


Probably safe. I clicked around a bunch in the Coin Control dialog in both tree & list nodes. Wasn't able to have the debugger detect anything that failed to parse as txid's in any of these functions except for when right-clicking the parent tree item to have the menu be shown, correctly disabling lock/unlock actions.

src/test/fuzz/uint256.cpp

stickies-v · 2024-07-30T12:06:42Z

Force pushed to address review comments by removing the new fuzz target and updating the existing one instead, and updating a docstring in coincontroldialog.cpp. No other changes.

hodlinator

ACK 24a6de3

Minor reservation, but not blocking: https://github.com/bitcoin/bitcoin/pull/30532/files#r1695679909

Tried to break bitcoin-qt Coin Control dialog on the previous revision (30c7351) without success and confirmed that only comments and fuzz code have changed since then.

(Passed make check).

hodlinator · 2024-07-30T14:48:53Z

src/test/uint256_tests.cpp

+    TestFromHex64Chars<uint256>();
+    TestFromHex64Chars<Txid>();
+    TestFromHex64Chars<Wtxid>();


In reply to https://github.com/bitcoin/bitcoin/pull/30532/files#r1696752519:

I'm not too worried about {T, Wt}xid::FromHex diverging in behavior. But maybe someone else can chime in to tie-break.

src/test/fuzz/uint256.cpp

maflcko

Only left some test-only style-nits. Feel free to ignore.

review ACK 24a6de3 🎨

Show signature

Signature:

untrusted comment: signature from minisign secret key on empty file; verify via: minisign -Vm "${path_to_any_empty_file}" -P RWTRmVTMeKV5noAMqVlsMugDDCyyTSbA3Re5AkUrhvLVln0tSaFWglOw -x "${path_to_this_whole_four_line_signature_blob}"
RUTRmVTMeKV5npGrKx1nqXCw5zeVHdtdYURB/KlyA/LMFgpNCs+SkW9a8N95d+U4AP1RJMi+krxU1A3Yux4bpwZNLvVBKy0wLgM=
trusted comment: review ACK 24a6de35857e88d968ef90cd6fbcda0003f0e3e 🎨
myhszgeXHcG4zBfV085OKVMVT0Z23g+tNmrxR3lxWUxa1Q7mYqyLKn6T29n/APMWsREgWDZ9DtN5m9mZ+ZEHDw==

src/test/transaction_tests.cpp

src/test/uint256_tests.cpp

maflcko · 2024-07-31T09:16:15Z

src/test/txpackage_tests.cpp

-    Txid txid_3{TxidFromString("0xee707be5201160e32c4fc715bec227d1aeea5940fb4295605e7373edce3b1a93")};
+    Txid txid_1{Txid::FromHex("bd0f71c1d5e50589063e134fad22053cdae5ab2320db5bf5e540198b0b5a4e69").value()};
+    Txid txid_2{Txid::FromHex("b4749f017444b051c44dfd2720e88f314ff94f3dd6d56d40ef65854fcd7fff6b").value()};
+    Txid txid_3{Txid::FromHex("ee707be5201160e32c4fc715bec227d1aeea5940fb4295605e7373edce3b1a93").value()};


style nit in the last commit: Could combine all test changes of this form into the previous test-only commit, so that similar test-only changes are grouped together?

The reason I organized it like this was to keep TxidFromString() test coverage until the same commit it was removed, whereas WtxidFromString is a test-only function and could be safely removed beforehand. Gonna leave as is for now since I think it's quite reviewable in the current shape, too?

Would have preferred the test changes in one commit too, but as you said, it is very reviewable already.

src/test/uint256_tests.cpp

stickies-v

Force-pushed to address test-only style nits, and generalizing TestFromHex() further to allow for testinguint160 too.

src/test/transaction_tests.cpp

src/test/uint256_tests.cpp

stickies-v · 2024-07-31T13:47:47Z

src/test/txpackage_tests.cpp

-    Txid txid_3{TxidFromString("0xee707be5201160e32c4fc715bec227d1aeea5940fb4295605e7373edce3b1a93")};
+    Txid txid_1{Txid::FromHex("bd0f71c1d5e50589063e134fad22053cdae5ab2320db5bf5e540198b0b5a4e69").value()};
+    Txid txid_2{Txid::FromHex("b4749f017444b051c44dfd2720e88f314ff94f3dd6d56d40ef65854fcd7fff6b").value()};
+    Txid txid_3{Txid::FromHex("ee707be5201160e32c4fc715bec227d1aeea5940fb4295605e7373edce3b1a93").value()};


The reason I organized it like this was to keep TxidFromString() test coverage until the same commit it was removed, whereas WtxidFromString is a test-only function and could be safely removed beforehand. Gonna leave as is for now since I think it's quite reviewable in the current shape, too?

maflcko

lgtm. No real changes other than test fixups.

re-ACK 9113f6b 🍷

Show signature

Signature:

untrusted comment: signature from minisign secret key on empty file; verify via: minisign -Vm "${path_to_any_empty_file}" -P RWTRmVTMeKV5noAMqVlsMugDDCyyTSbA3Re5AkUrhvLVln0tSaFWglOw -x "${path_to_this_whole_four_line_signature_blob}"
RUTRmVTMeKV5npGrKx1nqXCw5zeVHdtdYURB/KlyA/LMFgpNCs+SkW9a8N95d+U4AP1RJMi+krxU1A3Yux4bpwZNLvVBKy0wLgM=
trusted comment: re-ACK 9113f6b605f9e0a3b438561837f1f4e4142ad3c7 🍷
zpDwXlcOI4l11MhTUagbDaSkMEEIAWBDTmYF6+SCff3iMIuYxOqry2UPAJrGEeZCFMmzcnvMp4iJ1MLSOdVCCA==

src/test/uint256_tests.cpp

Simultaneously cover transaction_identifier::FromHex()

The newly introduced Wtxid::FromHex is more robust and removes the need for a WtxidFromString helper function

TxidFromString was deprecated due to missing 64-character length-check and hex-check, replace it with the more robust Txid::FromHex.

stickies-v · 2024-07-31T15:51:49Z

Force pushed to resolve all nits. Test-only doc changes so should be a trivial re-review.

hodlinator

ACK f553e6d

Compared to see only test code changed with fairly reasonable changes. Passed make check.

maflcko · 2024-08-01T06:46:37Z

Compared to see only test code changed with fairly reasonable changes. Passed make check.

Just as a note, the GitHub compare between A and B (or similarly, git diff A B) is insufficient if the pull request has more then one commit. It can easily miss a code issue that was added in an earlier commit and then removed in a later commit in the same pull request. The overall diff for the added code issue will not be visible, but when running git bisect or otherwise walking over the commits, it may become apparent.

maflcko · 2024-08-01T06:46:45Z

re-ACK f553e6d 🔻

Show signature

Signature:

untrusted comment: signature from minisign secret key on empty file; verify via: minisign -Vm "${path_to_any_empty_file}" -P RWTRmVTMeKV5noAMqVlsMugDDCyyTSbA3Re5AkUrhvLVln0tSaFWglOw -x "${path_to_this_whole_four_line_signature_blob}"
RUTRmVTMeKV5npGrKx1nqXCw5zeVHdtdYURB/KlyA/LMFgpNCs+SkW9a8N95d+U4AP1RJMi+krxU1A3Yux4bpwZNLvVBKy0wLgM=
trusted comment: re-ACK f553e6d86fe114e90585ea6d4b8e345a209cae5d 🔻
uDZbcjyrvbfhAAjTYC5C+LSEI7ldXJfKAMe8JnZ3sy+CHq4+Sk0jtU11dxj46i5hk79CcicruCcb+zugYyyWDw==

l0rinc

ACK f553e6d

Left a few nits, will glady re-ack, if needed.

l0rinc · 2024-08-01T07:54:15Z

src/test/uint256_tests.cpp

+void TestFromHex()
+{
+    constexpr unsigned int num_chars{T::size() * 2};
+    static_assert(num_chars <= 64); // this test needs to be modified to allow for more than 64 hex chars


nit: what is the purpose of the comment? Sounds like a todo, but I guess it's just a warning - which the assert already states clearly.

it's just a brief explanation on why this assertion is here, happy to remove if you feel strongly about it but i think it's fine as is

src/test/uint256_tests.cpp

l0rinc · 2024-08-01T08:11:40Z

src/test/fuzz/hex.cpp

@@ -27,7 +28,11 @@
10000
 FUZZ_TARGET(hex)
        assert(ToLower(random_hex_string) == hex_data);
    }
    (void)IsHexNumber(random_hex_string);
-    (void)uint256::FromHex(random_hex_string);
+    if (uint256::FromHex(random_hex_string)) {
+        assert(random_hex_string.length() == 64);


would it make sense to deduplicate (and therefore document) all the 64 constants used in the PR

I'm not sure, internally we're using base_blob::size() quite a bit already but I think for this test it makes sense to hardcode 64 since this function deals with user input, so we really do want to make sure this keeps expecting 64 characters. I don't think there are any other 64 constants in this PR that would benefit from being deduplicated or documented further?

src/qt/coincontroldialog.cpp

src/test/bloom_tests.cpp

l0rinc · 2024-08-01T10:11:44Z

src/test/uint256_tests.cpp

+    }
+    {
+        // check that non-hex characters are not accepted
+        std::string invalid_chars{R"( !"#$%&'()*+,-./:;<=>?@GHIJKLMNOPQRSTUVWXYZ[\]^_`ghijklmnopqrstuvwxyz{|}~)"};


nit: let's add some multi-byte unicode values here:

Suggested change

std::string invalid_chars{R"( !"#$%&'()*+,-./:;<=>?@GHIJKLMNOPQRSTUVWXYZ[\]^_`ghijklmnopqrstuvwxyz{|}~)"};

std::string invalid_chars{R"( !"#$%&'()*+,-./:;<=>?@GHIJKLMNOPQRSTUVWXYZ[\]^_`ghijklmnopqrstuvwxyz{|}~ő💣)"};

good idea, will add if I have to force push, thanks

these are just nits, it's fine if we do it next time we touch it, of course

src/test/uint256_tests.cpp

TheCharlatan

Nice, ACK f553e6d

TheCharlatan · 2024-08-01T10:14:28Z

src/test/txpackage_tests.cpp

-    Txid txid_3{TxidFromString("0xee707be5201160e32c4fc715bec227d1aeea5940fb4295605e7373edce3b1a93")};
+    Txid txid_1{Txid::FromHex("bd0f71c1d5e50589063e134fad22053cdae5ab2320db5bf5e540198b0b5a4e69").value()};
+    Txid txid_2{Txid::FromHex("b4749f017444b051c44dfd2720e88f314ff94f3dd6d56d40ef65854fcd7fff6b").value()};
+    Txid txid_3{Txid::FromHex("ee707be5201160e32c4fc715bec227d1aeea5940fb4295605e7373edce3b1a93").value()};


Would have preferred the test changes in one commit too, but as you said, it is very reviewable already.

TheCharlatan · 2024-08-01T10:17:02Z

src/qt/coincontroldialog.cpp

-        COutPoint outpt(TxidFromString(item->data(COLUMN_ADDRESS, TxHashRole).toString().toStdString()), item->data(COLUMN_ADDRESS, VOutRole).toUInt());
+    if (column != COLUMN_CHECKBOX) return;
+    auto txid{Txid::FromHex(item->data(COLUMN_ADDRESS, TxHashRole).toString().toStdString())};
+    if (txid) { // a valid txid means this is a child node, and not a parent node in tree mode


Just a comment: Still not sure what ends up being more readable. This, or:

if (auto txid{...}; txid) { ...

Usually I'd have preferred the if (auto txid{...}) { approach (no need for the ; txid I think), but since we have the docstring anyway I thought the current approach was slightly more readable.

Though of the same, both are fine with me as well

TheCharlatan · 2024-08-01T10:27:02Z

src/test/uint256_tests.cpp


 #include <boost/test/unit_test.hpp>

 #include <iomanip>
 #include <sstream>
 #include <string>
+#include <string_view>


iwyu reports this include as unneeded, but it is obviously used: https://cirrus-ci.com/task/6533090686795776?logs=ci#L17964

I made util/string.h to export them, but this may be confusing? (Should be harmless either way)

stickies-v

Thanks for the reviews everyone. Going to hold off on addressing nits given that I think this PR is now RFM, but will happily incorporate them when I have to force push for other reasons.

src/test/uint256_tests.cpp

stickies-v · 2024-08-01T10:27:46Z

src/test/uint256_tests.cpp

+void TestFromHex()
+{
+    constexpr unsigned int num_chars{T::size() * 2};
+    static_assert(num_chars <= 64); // this test needs to be modified to allow for more than 64 hex chars


it's just a brief explanation on why this assertion is here, happy to remove if you feel strongly about it but i think it's fine as is

src/test/bloom_tests.cpp

stickies-v · 2024-08-01T10:33:57Z

src/test/fuzz/hex.cpp

@@ -27,7 +28,11 @@ FUZZ_TARGET(hex)
        assert(ToLower(random_hex_string) == hex_data);
    }
    (void)IsHexNumber(random_hex_string);
-    (void)uint256::FromHex(random_hex_string);
+    if (uint256::FromHex(random_hex_string)) {
+        assert(random_hex_string.length() == 64);


I'm not sure, internally we're using base_blob::size() quite a bit already but I think for this test it makes sense to hardcode 64 since this function deals with user input, so we really do want to make sure this keeps expecting 64 characters. I don't think there are any other 64 constants in this PR that would benefit from being deduplicated or documented further?

stickies-v · 2024-08-01T10:39:37Z

src/test/uint256_tests.cpp

+    }
+    {
+        // check that non-hex characters are not accepted
+        std::string invalid_chars{R"( !"#$%&'()*+,-./:;<=>?@GHIJKLMNOPQRSTUVWXYZ[\]^_`ghijklmnopqrstuvwxyz{|}~)"};


good idea, will add if I have to force push, thanks

stickies-v · 2024-08-01T10:43:57Z

src/qt/coincontroldialog.cpp

-        COutPoint outpt(TxidFromString(item->data(COLUMN_ADDRESS, TxHashRole).toString().toStdString()), item->data(COLUMN_ADDRESS, VOutRole).toUInt());
+    if (column != COLUMN_CHECKBOX) return;
+    auto txid{Txid::FromHex(item->data(COLUMN_ADDRESS, TxHashRole).toString().toStdString())};
+    if (txid) { // a valid txid means this is a child node, and not a parent node in tree mode


Usually I'd have preferred the if (auto txid{...}) { approach (no need for the ; txid I think), but since we have the docstring anyway I thought the current approach was slightly more readable.

glozow

concept ACK + lgtm

18d65d2 test: use uint256::FromUserHex for RANDOM_CTX_SEED (stickies-v) 6819e5a node: use uint256::FromUserHex for -assumevalid parsing (stickies-v) 2e58fdb util: remove unused IsHexNumber (stickies-v) 8a44d7d node: use uint256::FromUserHex for -minimumchainwork parsing (stickies-v) 70e2c87 refactor: add uint256::FromUserHex helper (stickies-v) 85b7cbf test: unittest chainstatemanager_args (stickies-v) Pull request description: Since fad2991, `uint256S` has been [deprecated](fad2991#diff-800776e2dda39116e889839f69409571a5d397de048a141da7e4003bc099e3e2R138) because it is less robust than the `base_blob::FromHex()` introduced in [the same PR](#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 carves out the few `uint256S` callsites that may potentially prove a bit more controversial to change because they deal with user input and backwards incompatible behaviour change. The main behaviour change introduced in this PR is: - `-minimumchainwork` will raise an error when input is longer than 64 hex digits - `-assumevalid` will raise an error when input contains invalid hex characters, or when it is longer than 64 hex digits - test: the optional RANDOM_CTX_SEED env var will now cause tests to abort when it contains invalid hex characters, or when it is longer than 64 hex digits After this PR, the remaining work to remove `uint256S` completely is almost entirely mechanical and/or test related. I will open that PR once #30560 is merged because it builds on that. ACKs for top commit: hodlinator: re-ACK 18d65d2 l0rinc: ACK 18d65d2 achow101: ACK 18d65d2 ryanofsky: Code review ACK 18d65d2. Very nice change that cleans up the API, adds checking for invalid values, makes parsing of values more consistent, and adds test coverage. Tree-SHA512: ec118ea3d56e1dfbc4c79acdbfc797f65c4d2107b0ca9577c848b4ab9b7cb8d05db9f3c7fe8441a09291aca41bf671572431f4eddc59be8fb53abbea76bbbf86

8756ccd scripted-diff: Replace ParseHex[<std::byte>]("str") -> "str"_hex[_u8] (Hodlinator) 9cb6873 refactor: Prepare for ParseHex -> ""_hex scripted-diff (Hodlinator) 50bc017 refactor: Hand-replace some ParseHex -> ""_hex (Hodlinator) 5b74a84 util: Add consteval ""_hex[_v][_u8] literals (l0rinc) dc5f6f6 test refactor: util_tests - parse_hex clean up (Hodlinator) 2b5e6ef refactor: Make XOnlyPubKey tolerate constexpr std::arrays (Hodlinator) 403d86f refactor: vector -> span in CCrypter (Hodlinator) bd0830b refactor: de-Hungarianize CCrypter (Hodlinator) d99c816 refactor: Improve CCrypter related lines (Hodlinator) 7e1d9a8 refactor: Enforce lowercase hex digits for consteval uint256 (Hodlinator) Pull request description: Motivation: * Validates and converts the hex string into bytes at compile time instead of at runtime like `ParseHex()`. * Eliminates runtime dependencies: #30377 (comment), #30048 (comment) * Has stricter requirements than `ParseHex()` (disallows whitespace and uppercase hex digits) and replaces it in a bunch of places. * Makes it possible to derive other compile time constants. * Minor: should shave off a few runtime CPU cycles. `""_hex` produces `std::array<std::byte>` as the momentum in the codebase is to use `std::byte` over `uint8_t`. Also makes `uint256` hex string constructor disallow uppercase hex digits. Discussed: #30560 (comment) Surprisingly does not change the size of the Guix **bitcoind** binary (on x86_64-linux-gnu) by 1 single byte. Spawned already merged PRs: #30436, #30482, #30532, #30560. ACKs for top commit: l0rinc: ACK 8756ccd stickies-v: Rebase re-ACK 8756ccd, no changes since a096215 ryanofsky: Code review ACK 8756ccd, just rebasing since last review and taking advantage of CScript constructors in #29369, also tweaking a code comment Tree-SHA512: 9b2011b7c37e0ef004c669f8601270a214b388916316458370f5902c79c2856790b1b2c7c123efa65decad04886ab5eff95644301e0d84358bb265cf1f8ec195

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 fad2991, `uint256S()` has been [deprecated](fad2991#diff-800776e2dda39116e889839f69409571a5d397de048a141da7e4003bc099e3e2R138) 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](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

DrahtBot added the Refactoring label Jul 26, 2024

stickies-v mentioned this pull request Jul 26, 2024

rest: Reject truncated hex txid early in getutxos parsing #30482

Merged

hodlinator reviewed Jul 26, 2024

View reviewed changes

l0rinc reviewed Jul 28, 2024

View reviewed changes

src/qt/coincontroldialog.cpp Show resolved Hide resolved

stickies-v force-pushed the 2024-07/rm-txidfromstring branch 2 times, most recently from b6080f2 to 30c7351 Compare July 29, 2024 17:12

DrahtBot added the CI failed label Jul 29, 2024

DrahtBot removed the CI failed label Jul 29, 2024

hodlinator suggested changes Jul 29, 2024

View reviewed changes

maflcko reviewed Jul 30, 2024

View reviewed changes

src/test/fuzz/uint256.cpp Outdated Show resolved Hide resolved

stickies-v force-pushed the 2024-07/rm-txidfromstring branch from 30c7351 to 24a6de3 Compare July 30, 2024 12:02

hodlinator approved these changes Jul 30, 2024

View reviewed changes

maflcko reviewed Jul 31, 2024

View reviewed changes

src/test/uint256_tests.cpp Outdated Show resolved Hide resolved

stickies-v force-pushed the 2024-07/rm-txidfromstring branch from 24a6de3 to 9113f6b Compare July 31, 2024 13:53

stickies-v commented Jul 31, 2024

View reviewed changes

maflcko approved these changes Jul 31, 2024

View reviewed changes

src/test/uint256_tests.cpp Outdated Show resolved Hide resolved

src/test/uint256_tests.cpp Outdated Show resolved Hide resolved

DrahtBot requested a review from hodlinator July 31, 2024 14:19

stickies-v added 4 commits July 31, 2024 16:47

test: add uint256::FromHex unittest coverage

526a87b

Simultaneously cover transaction_identifier::FromHex()

fuzz: increase FromHex() coverage

9a0b2a6

test: replace WtxidFromString with Wtxid::FromHex

285ab50

The newly introduced Wtxid::FromHex is more robust and removes the need for a WtxidFromString helper function

refactor: remove TxidFromString

f553e6d

TxidFromString was deprecated due to missing 64-character length-check and hex-check, replace it with the more robust Txid::FromHex.

stickies-v force-pushed the 2024-07/rm-txidfromstring branch from 9113f6b to f553e6d Compare July 31, 2024 15:50

hodlinator approved these changes Jul 31, 2024

View reviewed changes

DrahtBot requested a review from maflcko July 31, 2024 20:46

l0rinc approved these changes Aug 1, 2024

View reviewed changes

TheCharlatan approved these changes Aug 1, 2024

View reviewed changes

stickies-v commented Aug 1, 2024

View reviewed changes

glozow reviewed Aug 1, 2024

View reviewed changes

glozow merged commit ebd82fa into bitcoin:master Aug 1, 2024
16 checks passed

stickies-v deleted the 2024-07/rm-txidfromstring branch August 1, 2024 11:17

stickies-v mentioned this pull request Aug 1, 2024

node: reduce unsafe uint256S usage #30569

Merged

hodlinator mentioned this pull request Aug 13, 2024

refactor: Replace ParseHex with consteval ""_hex literals #30377

Merged

stickies-v mentioned this pull request Aug 30, 2024

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

Merged

	std::string invalid_chars{R"( !"#$%&'()*+,-./:;<=>?@GHIJKLMNOPQRSTUVWXYZ[\]^_`ghijklmnopqrstuvwxyz{\|}~)"};
	std::string invalid_chars{R"( !"#$%&'()*+,-./:;<=>?@GHIJKLMNOPQRSTUVWXYZ[\]^_`ghijklmnopqrstuvwxyz{\|}~ő💣)"};

refactor: remove deprecated TxidFromString() in favour of transaction_identifier::FromHex() #30532

refactor: remove deprecated TxidFromString() in favour of transaction_identifier::FromHex() #30532

Uh oh!

Conversation

Uh oh!

Testing GUI changes

Uh oh!

Uh oh!

Code Coverage

Reviews

Uh oh!

Choose a reason for hiding this comment

Uh oh!

Choose a reason for hiding this comment

Uh oh!

Choose a reason for hiding this comment

Uh oh!

Choose a reason for hiding this comment

Uh oh!

Uh oh!

Uh oh!

Uh oh!

Uh oh!

Uh oh!

Uh oh!

Choose a reason for hiding this comment

Uh oh!

Uh oh!

Choose a reason for hiding this comment

Uh oh!

Choose a reason for hiding this comment

Uh oh!

Uh oh!

Choose a reason for hiding this comment

Uh oh!

Uh oh!

Choose a reason for hiding this comment

Uh oh!

Uh oh!

Uh oh!

Uh oh!

Choose a reason for hiding this comment

Uh oh!

Choose a reason for hiding this comment

Uh oh!

Uh oh!

Choose a reason for hiding this comment

Uh oh!

Uh oh!

Uh oh!

Uh oh!

Uh oh!

Choose a reason for hiding this comment

Uh oh!

Choose a reason for hiding this comment

Uh oh!

Choose a reason for hiding this comment

Uh oh!

Uh oh!

Choose a reason for hiding this comment

Uh oh!

Uh oh!

Uh oh!

Uh oh!

Uh oh!

Uh oh!

Choose a reason for hiding this comment

Uh oh!

Choose a reason for hiding this comment

Uh oh!

Uh oh!

Uh oh!

Uh oh!

Choose a reason for hiding this comment

Uh oh!

Uh oh!

Uh oh!

Choose a reason for hiding this comment

Uh oh!

Choose a reason for hiding this comment