refactor: remove deprecated TxidFromString() in favour of transaction

stickies-v commented at 3:02 pm on July 26, 2024: contributor

Since https://github.com/bitcoin/bitcoin/pull/30482/commits/fab6ddbee64e50d5e2f499aebca35b5911896ec4, 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 (review)

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.

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

DrahtBot commented at 3:02 pm on July 26, 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	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.

DrahtBot added the label Refactoring on Jul 26, 2024

in src/qt/coincontroldialog.cpp:238 in 2cad2ef188 outdated

234@@ -240,7 +235,7 @@ void CoinControlDialog::lockCoin()
235     if (contextMenuItem->checkState(COLUMN_CHECKBOX) == Qt::Checked)
236         contextMenuItem->setCheckState(COLUMN_CHECKBOX, Qt::Unchecked);
237 
238-    COutPoint outpt(TxidFromString(contextMenuItem->data(COLUMN_ADDRESS, TxHashRole).toString().toStdString()), contextMenuItem->data(COLUMN_ADDRESS, VOutRole).toUInt());
239+    COutPoint outpt(Txid::FromHex(contextMenuItem->data(COLUMN_ADDRESS, TxHashRole).toString().toStdString()).value(), contextMenuItem->data(COLUMN_ADDRESS, VOutRole).toUInt());

hodlinator commented at 9:31 pm on July 26, 2024:

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.

stickies-v commented at 5:15 pm on July 29, 2024:

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?

hodlinator commented at 9:37 pm on July 29, 2024:

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.

paplorinc commented at 10:04 am on August 1, 2024:

this was accessed when selecting lock from the menu: 👍

in src/test/uint256_tests.cpp:378 in 2cad2ef188 outdated

334+        auto chars_68{"0123456789abcdef0123456789abcdef0123456789abcdef0123456789abcdef0123"};
335+        std::string_view chars_64{chars_68, 64};
336+        BOOST_CHECK(!uint256::FromHex(std::string_view(chars_68, 63))); // too short
337+        BOOST_CHECK_EQUAL(uint256::FromHex(std::string_view(chars_68, 64)).value().ToString(), chars_64);
338+        BOOST_CHECK(!uint256::FromHex(std::string_view(chars_68, 65))); // too long
339+    }

hodlinator commented at 9:33 pm on July 26, 2024:

Good to see the former TxidFromString() test resurrected with even more checks!

I noticed there are no tests for FromHex returning nullopt for invalid characters, care to add?

Maybe also mix up the hex case in chars_68? 0123456789abcdef0123456789ABCDEF0123456789abcdef0123456789ABCDEF0123

stickies-v commented at 5:06 pm on July 29, 2024:

I’ve added 17ebf1ac133d26676ba7ae3e40042fdf389d146f with a bunch more unit test coverage for uint256::FromHex() and methods that wrap it, including invalid characters, 0x prefix, and mixed case. Additionally, I’ve also added a fuzz target in ed764b55abb18dacb814658d328098a77682d128

hodlinator commented at 9:48 pm on July 26, 2024: contributor

Concept ACK 2cad2ef1886d31e647d88191a2d4a9d0ce834199

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.

in src/qt/coincontroldialog.cpp:177 in 2cad2ef188 outdated

173@@ -174,22 +174,17 @@ void CoinControlDialog::showMenu(const QPoint &point)
174         contextMenuItem = item;
175 
176         // disable some items (like Copy Transaction ID, lock, unlock) for tree roots in context menu
177-        if (item->data(COLUMN_ADDRESS, TxHashRole).toString().length() == 64) // transaction hash is 64 characters (this means it is a child node, so it is not a parent node in tree mode)
178-        {
179+        auto txid{Txid::FromHex(item->data(COLUMN_ADDRESS, TxHashRole).toString().toStdString())};

paplorinc commented at 9:40 am on July 27, 2024:

can you please tell me which test covers this line? I tried checking it via the UI, but I think I would need testnet coins to be able to exercise it - is there a simpler way?

maflcko commented at 7:11 am on July 29, 2024:

can you please tell me which test covers this line?

Possibly none, as the GUI tests are sparse at best.

I tried checking it via the UI, but I think I would need testnet coins to be able to exercise it - is there a simpler way?

For the majority of testing regtest (a non-public test-only network) is more than sufficient.

stickies-v commented at 9:55 am on July 29, 2024:

regtest is indeed how I’ve been testing my changes, added some instructions to the OP.

paplorinc commented at 10:01 am on August 1, 2024:

this was triggered when right-clicking on the lines: 👍

stickies-v force-pushed on Jul 29, 2024

DrahtBot added the label CI failed on Jul 29, 2024

DrahtBot commented at 5:12 pm on July 29, 2024: contributor

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

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 commented at 5:16 pm on July 29, 2024: contributor

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.

in src/test/fuzz/uint256.cpp:23 in 30c7351491 outdated

18+
19+        // check we can also construct {T,Wt}xids from a valid 64 character hex-string input
20+        assert(Txid::FromHex(input));
21+        assert(Wtxid::FromHex(input));
22+    }
23+}

hodlinator commented at 6:25 pm on July 29, 2024:

I guess the red warning symbol is GitHub complaining about missing empty line at EOF. Best to fix in order to avoid warnings in editors, git diffs, etc.

stickies-v commented at 10:45 am on July 30, 2024:

Yeah it is, will fix in next force push, thanks.

stickies-v commented at 12:05 pm on July 30, 2024:

Resolved by removing the file entirely.

in src/test/uint256_tests.cpp:382 in 30c7351491 outdated

377+
378+BOOST_AUTO_TEST_CASE(from_hex)
379+{
380+    TestFromHex64Chars<uint256>();
381+    TestFromHex64Chars<Txid>();
382+    TestFromHex64Chars<Wtxid>();

hodlinator commented at 6:27 pm on July 29, 2024:

Very nice to see the additional coverage!

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

stickies-v commented at 10:49 am on July 30, 2024:

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?

hodlinator commented at 2:48 pm on July 30, 2024:

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.

stickies-v commented at 3:24 pm on July 30, 2024:

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.

DrahtBot removed the label CI failed on Jul 29, 2024

in src/qt/coincontroldialog.cpp:178 in 30c7351491 outdated

173@@ -174,22 +174,17 @@ void CoinControlDialog::showMenu(const QPoint &point)
174         contextMenuItem = item;
175 
176         // disable some items (like Copy Transaction ID, lock, unlock) for tree roots in context menu
177-        if (item->data(COLUMN_ADDRESS, TxHashRole).toString().length() == 64) // transaction hash is 64 characters (this means it is a child node, so it is not a parent node in tree mode)
178-        {
179+        auto txid{Txid::FromHex(item->data(COLUMN_ADDRESS, TxHashRole).toString().toStdString())};
180+        if (txid) { // transaction hash is 64 characters (this means it is a child node, so it is not a parent node in tree mode)

hodlinator commented at 8:50 pm on July 29, 2024:

Comment seems a bit off now that == 64 is removed.

0        if (txid) { // transaction hash is a valid txid (this means it is a child node, so it is not a parent node in tree mode)

Same in CoinControlDialog::viewItemChanged.

stickies-v commented at 10:54 am on July 30, 2024:

Makes sense, will update both to if (txid) { // a valid txid means this is a child node, and not a parent node in tree mode in next force push.

stickies-v commented at 12:07 pm on July 30, 2024:

Done in 24a6de35857e88d968ef90cd6fbcda0003f0e3e3

in src/test/fuzz/uint256.cpp:14 in 30c7351491 outdated

 9+
10+FUZZ_TARGET(uint256_fromhex)
11+{
12+    FuzzedDataProvider fuzzed_data_provider{buffer.data(), buffer.size()};
13+    const auto input{fuzzed_data_provider.ConsumeRandomLengthString()};
14+    const auto result{uint256::FromHex(input)};

hodlinator commented at 11:06 pm on July 29, 2024:

Strikes me as being “too far off the reservation” with the inputs. Won’t we be hitting the first condition in FromHex on str.size() being different from ::size() * 2 in >99% of cases? Maybe something closer to the below would spend fuzz-cycles a bit more wisely? Also added a loop to use more of the fuzz data. Then again I’m new to fuzzing.

 0diff --git a/src/test/fuzz/uint256.cpp b/src/test/fuzz/uint256.cpp
 1index f5c8b2e146..38bbc5e108 100644
 2--- a/src/test/fuzz/uint256.cpp
 3+++ b/src/test/fuzz/uint256.cpp
 4@@ -9,15 +9,42 @@
 5 
 6 FUZZ_TARGET(uint256_fromhex)
 7 {
 8+    enum Mode {
 9+        RandomCharsRandomLength,
10+        RandomCharsCorrectLength,
11+        ValidCharsRandomLength,
12+
13+        kMaxValue = ValidCharsRandomLength
14+    };
15+
16     FuzzedDataProvider fuzzed_data_provider{buffer.data(), buffer.size()};
17-    const auto input{fuzzed_data_provider.ConsumeRandomLengthString()};
18-    const auto result{uint256::FromHex(input)};
19-    if (result) {
20-        // we only expect a result for a 64 character hex-string
21-        assert(input.length() == 64);
22+    while (fuzzed_data_provider.remaining_bytes() > 0) {
23+        std::string input;
24+        static_assert(64 == uint256::size() * 2, "2 hex chars per byte");
25+        switch (fuzzed_data_provider.ConsumeEnum<Mode>()) {
26+        case RandomCharsRandomLength:
27+            input = fuzzed_data_provider.ConsumeRandomLengthString();
28+            break;
29+        case RandomCharsCorrectLength:
30+            input = fuzzed_data_provider.ConsumeBytesAsString(64);
31+            break;
32+        case ValidCharsRandomLength:
33+            constexpr char valid[] = "0123456789abcdefABCDEF";
34+            input.resize(64, '0');
35+            for (char& c : input) {
36+                c = valid[fuzzed_data_provider.ConsumeIntegralInRange<int>(0, sizeof(valid)-1)];
37+            }
38+            break;
39+        }
40+
41+        const auto result{uint256::FromHex(input)};
42+        if (result) {
43+            // we only expect a result for a 64 character hex-string
44+            assert(input.length() == 64);
45 
46-        // check we can also construct {T,Wt}xids from a valid 64 character hex-string input
47-        assert(Txid::FromHex(input));
48-        assert(Wtxid::FromHex(input));
49+            // check we can also construct {T,Wt}xids from a valid 64 character hex-string input
50+            assert(Txid::FromHex(input));
51+            assert(Wtxid::FromHex(input));
52+        }
53     }
54-}
55\ No newline at end of file
56+}

maflcko commented at 7:33 am on July 30, 2024:

Strikes me as being “too far off the reservation” with the inputs. Won’t we be hitting the first condition in FromHex on str.size() being different from ::size() * 2 in >99% of cases? Maybe something closer to the below would spend fuzz-cycles a bit more wisely? Also added a loop to use more of the fuzz data. Then again I’m new to fuzzing.

Some replies:

You are right that fuzz engines (the program that produces fuzz inputs) sometimes have difficulty reaching code that is hidden by complicated conditions. However, this condition isn’t complicated and a fuzz engine should be able to solve it quickly. Once it is solved, most fuzz engines will by itself mutate the passing input further to try to reach more coverage. So I think in this particular case no hand-holding in the form of enum Mode is needed. (Though, there are fuzz targets where something like it is beneficial)
You should be able to see coverage reported by the fuzz engine and see it saturate quickly as a way to check whether my first point is plausible here

About “using more fuzz data”: I’d say not using a loop here is more beneficial, because:

Generally, a shorter fuzz input corresponds to a faster runtime, allowing a fuzz engine to iterate faster when searching for new inputs
The function under test has no state (it is pure). Thus, no additional bugs can be found by running in a loop. (Though, there are fuzz targets where something like it is beneficial)
When adding fuzz inputs to the qa-assets repo, small ones will be preferred, so the long ones will be discarded anyway. Should they not be discarded (maybe due to the use of -use_value_profile=1), they will not help to increase coverage or find more bugs, but only bloat the qa-assets repo with storage and runtime overhead

stickies-v commented at 10:41 am on July 30, 2024:

I’m lacking hands-on experience with fuzzing to comment on worthwhile optimizations, so I’m going to go with consensus here. @maflcko’s reasoning looks sounds to me, so I think keeping it as is (besides moving to to fuzz/hex.cpp as discussed in another comment) is the sensible approach, but thank you for sharing the Enum approach, I hadn’t seen that yet and it looks like a useful pattern to know.

hodlinator commented at 2:59 pm on July 30, 2024:

In reply to #30532 (review):

However, this condition isn’t complicated and a fuzz engine should be able to solve it quickly. Once it is solved, most fuzz engines will by itself mutate the passing input further to try to reach more coverage.

Generally, a shorter fuzz input corresponds to a faster runtime, allowing a fuzz engine to iterate faster when searching for new inputs

Wow, wasn’t aware that fuzz engines were aware of code coverage and generated this level of non-random inputs. Thanks for the fuzz crash course @maflcko!

Sounds good to me @stickies-v.

hodlinator changes_requested

in src/test/fuzz/uint256.cpp:10 in 30c7351491 outdated

 5+#include <test/fuzz/FuzzedDataProvider.h>
 6+#include <test/fuzz/fuzz.h>
 7+#include <uint256.h>
 8+#include <util/transaction_identifier.h>
 9+
10+FUZZ_TARGET(uint256_fromhex)

maflcko commented at 7:39 am on July 30, 2024:

This would be duplicate with the src/test/fuzz/hex.cpp, which already does (void)uint256::FromHex(random_hex_string);

I’d say that hex parsing is trivial enough to put everything into one fuzz target. Otherwise the overhead of allocating a dedicated fuzz target, fuzz input folder, and fuzz resources (CPU…) may take away those resources from more meaningful fuzz targets that actually need them more than 5 minutes of initial fuzzing. (I’d be surprised if all code wasn’t covered in 5 minutes of fuzzing here, if it isn’t already covered by the existing hex fuzz inputs.

stickies-v commented at 10:32 am on July 30, 2024:

Ah yes, I missed that it’s already covered. Do you think it’s best to drop the fuzz commit altogether or move the extra checks into fuzz/hex.cpp?

 0diff --git a/src/Makefile.test.include b/src/Makefile.test.include
 1index 268d346d6b..0993a65eff 100644
 2--- a/src/Makefile.test.include
 3+++ b/src/Makefile.test.include
 4@@ -397,7 +397,6 @@ test_fuzz_fuzz_SOURCES = \
 5  test/fuzz/tx_pool.cpp \
 6  test/fuzz/txorphan.cpp \
 7  test/fuzz/txrequest.cpp \
 8- test/fuzz/uint256.cpp \
 9  test/fuzz/utxo_snapshot.cpp \
10  test/fuzz/utxo_total_supply.cpp \
11  test/fuzz/validation_load_mempool.cpp \
12diff --git a/src/test/fuzz/hex.cpp b/src/test/fuzz/hex.cpp
13index bc46863f1d..dcf2df279e 100644
14--- a/src/test/fuzz/hex.cpp
15+++ b/src/test/fuzz/hex.cpp
16@@ -27,7 +27,11 @@ FUZZ_TARGET(hex)
17         assert(ToLower(random_hex_string) == hex_data);
18     }
19     (void)IsHexNumber(random_hex_string);
20-    (void)uint256::FromHex(random_hex_string);
21+    if (uint256::FromHex(random_hex_string)) {
22+        assert(random_hex_string.length() == 64);
23+        assert(Txid::FromHex(random_hex_string));
24+        assert(Wtxid::FromHex(random_hex_string));
25+    }
26     (void)uint256S(random_hex_string);
27     try {
28         (void)HexToPubKey(random_hex_string);

I’m not 100% sure on the assert(random_hex_string.length() == 64); check since that feels like more of a unittest thing, but at the same time it’s pretty cheap check

maflcko commented at 11:14 am on July 30, 2024:

Do you think it’s best to drop the fuzz commit altogether or move the extra checks into fuzz/hex.cpp?

No opinion. Up to you. Your diff looks good.

stickies-v commented at 12:04 pm on July 30, 2024:

Thanks, I’ve applied the above diff to fcf7fe35f1805e59357bcb6e65fa66460b423f86

stickies-v force-pushed on Jul 30, 2024

stickies-v commented at 12:06 pm on July 30, 2024: contributor

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 approved

hodlinator commented at 3:18 pm on July 30, 2024: contributor

ACK 24a6de35857e88d968ef90cd6fbcda0003f0e3e3

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 (30c7351491063dab127cff236074beddff59d1a5) without success and confirmed that only comments and fuzz code have changed since then.

(Passed make check).

in src/test/transaction_tests.cpp:1035 in 17ebf1ac13 outdated

1027@@ -1028,12 +1028,4 @@ BOOST_AUTO_TEST_CASE(test_IsStandard)
1028     }
1029 }
1030 
1031-BOOST_AUTO_TEST_CASE(test_TxidFromString)
1032-{
1033-    // Make sure TxidFromString respects string_view length and stops reading at
1034-    // end of the substring.
1035-    BOOST_CHECK_EQUAL(TxidFromString(std::string_view("ABCD1234", 4)).ToString(),

maflcko commented at 9:00 am on July 31, 2024:

style nit in 17ebf1ac133d26676ba7ae3e40042fdf389d146f: Test coverage for TxidFromString is removed in the wrong commit.

stickies-v commented at 12:53 pm on July 31, 2024:

Thanks, fixed. This commit was initially just meant to move the test, but I agree your approach makes more sense, especially in the current way the commits are organized.

in src/test/uint256_tests.cpp:354 in 17ebf1ac13 outdated

344+        auto valid_result{T::FromHex(valid_64char_input)};
345+        BOOST_REQUIRE(valid_result);
346+        BOOST_CHECK_EQUAL(valid_result->ToString(), ToLower(valid_64char_input));
347+    }
348+    {
349+        // check that only strings of size 64 are accepted

maflcko commented at 9:04 am on July 31, 2024:

style nit in https://github.com/bitcoin/bitcoin/commit/17ebf1ac133d26676ba7ae3e40042fdf389d146f: Seems duplicate with the “too short”/“too long” test below?

stickies-v commented at 12:58 pm on July 31, 2024:

The “too short”/“too long” tests are a replacement of the test_TxidFromString where we’re specifically testing string_view substring input, so I don’t think they’re duplicated?

maflcko commented at 2:17 pm on July 31, 2024:

nit in d50ba708989e55d6081959acd72c7b68ed29a890: 64 -> num_chars

in src/test/uint256_tests.cpp:359 in 17ebf1ac13 outdated

354+        BOOST_CHECK(!T::FromHex("0123456789abcdef0123456789ABCDEF0123456789abcdef0123456789ABCDEF0")); // 65 chars
355+    }
356+    {
357+        // check that non-hex characters are not accepted
358+        std::string invalid_chars{R"( !"#$%&'()*+,-./:;<=>?@GHIJKLMNOPQRSTUVWXYZ[\]^_`ghijklmnopqrstuvwxyz{|}~)"};
359+        std::string valid_base{"0123456789abcdef0123456789ABCDEF0123456789abcdef0123456789ABCDE"}; // 63 chars

maflcko commented at 9:07 am on July 31, 2024:

style nit in https://github.com/bitcoin/bitcoin/commit/17ebf1ac133d26676ba7ae3e40042fdf389d146f: Could just re-use valid_64char_input.substr(0, 63)?

stickies-v commented at 12:59 pm on July 31, 2024:

Nice, taken, thanks.

in src/test/uint256_tests.cpp:372 in 17ebf1ac13 outdated

367+        BOOST_CHECK(!T::FromHex(invalid_prefix));
368+    }
369+    {
370+        // check that string_view length is respected
371+        std::string chars_68{valid_64char_input + "0123"};
372+        BOOST_CHECK(T::FromHex(std::string_view(chars_68.data(), 64)));

maflcko commented at 9:11 am on July 31, 2024:

style nit in https://github.com/bitcoin/bitcoin/commit/17ebf1ac133d26676ba7ae3e40042fdf389d146f: Could check that .value().ToString() is equal to ToLower(valid_64char_input)? Similar to test_TxidFromString.

stickies-v commented at 1:05 pm on July 31, 2024:

I initially changed it to what it is now because I wanted to keep the test focused on ensuring string_view length to be respected, but I suppose it is helpful to ensure the correct substring characters are respected too, so I’ve taken your suggestion, thanks.

in src/test/txpackage_tests.cpp:83 in 24a6de3585 outdated

77@@ -78,9 +78,9 @@ BOOST_FIXTURE_TEST_CASE(package_hash_tests, TestChain100Setup)
78     BOOST_CHECK(wtxid_2.GetHex() < wtxid_3.GetHex());
79 
80     // The txids are not (we want to test that sorting and hashing use wtxid, not txid):
81-    Txid txid_1{TxidFromString("0xbd0f71c1d5e50589063e134fad22053cdae5ab2320db5bf5e540198b0b5a4e69")};
82-    Txid txid_2{TxidFromString("0xb4749f017444b051c44dfd2720e88f314ff94f3dd6d56d40ef65854fcd7fff6b")};
83-    Txid txid_3{TxidFromString("0xee707be5201160e32c4fc715bec227d1aeea5940fb4295605e7373edce3b1a93")};
84+    Txid txid_1{Txid::FromHex("bd0f71c1d5e50589063e134fad22053cdae5ab2320db5bf5e540198b0b5a4e69").value()};
85+    Txid txid_2{Txid::FromHex("b4749f017444b051c44dfd2720e88f314ff94f3dd6d56d40ef65854fcd7fff6b").value()};
86+    Txid txid_3{Txid::FromHex("ee707be5201160e32c4fc715bec227d1aeea5940fb4295605e7373edce3b1a93").value()};

maflcko commented at 9:16 am on July 31, 2024:

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?

stickies-v commented at 1:47 pm on July 31, 2024:

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?

TheCharlatan commented at 10:14 am on August 1, 2024:

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

maflcko commented at 9:20 am on July 31, 2024: member

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

review ACK 24a6de35857e88d968ef90cd6fbcda0003f0e3e 🎨

Signature:

0untrusted 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}"
1RUTRmVTMeKV5npGrKx1nqXCw5zeVHdtdYURB/KlyA/LMFgpNCs+SkW9a8N95d+U4AP1RJMi+krxU1A3Yux4bpwZNLvVBKy0wLgM=
2trusted comment: review ACK 24a6de35857e88d968ef90cd6fbcda0003f0e3e 🎨
3myhszgeXHcG4zBfV085OKVMVT0Z23g+tNmrxR3lxWUxa1Q7mYqyLKn6T29n/APMWsREgWDZ9DtN5m9mZ+ZEHDw==

in src/test/uint256_tests.cpp:341 in 17ebf1ac13 outdated

336+ * method that wraps uint256::FromHex(), such as transaction_identifier::FromHex().
337+ */
338+template <typename T>
339+void TestFromHex64Chars()
340+{
341+    std::string valid_64char_input{"0123456789abcdef0123456789ABCDEF0123456789abcdef0123456789ABCDEF"};

maflcko commented at 9:23 am on July 31, 2024:

test-nit in: Could even template 64 with valid_input = valid_input.substr(0, T::size()) and use T::size() below as well, to allow testing of uint160 as well, but this can be done in a follow-up.

stickies-v commented at 12:56 pm on July 31, 2024:

Nice, I like it. Adopted this approach and added TestFromHex<uint160>();

stickies-v force-pushed on Jul 31, 2024

stickies-v commented at 1:56 pm on July 31, 2024: contributor

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

in src/test/uint256_tests.cpp:338 in d50ba70898 outdated

332@@ -330,6 +333,59 @@ BOOST_AUTO_TEST_CASE(parse)
333     }
334 }
335 
336+/**
337+ * Implemented as a templated function so it can be reused by other classes that have a FromHex()
338+ * method that wraps uint256::FromHex(), such as transaction_identifier::FromHex().

maflcko commented at 2:16 pm on July 31, 2024:

nit in d50ba708989e55d6081959acd72c7b68ed29a890: uint256 -> base_blob

maflcko approved

maflcko commented at 2:19 pm on July 31, 2024: member

lgtm. No real changes other than test fixups.

re-ACK 9113f6b605f9e0a3b438561837f1f4e4142ad3c7 🍷

Signature:

0untrusted 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}"
1RUTRmVTMeKV5npGrKx1nqXCw5zeVHdtdYURB/KlyA/LMFgpNCs+SkW9a8N95d+U4AP1RJMi+krxU1A3Yux4bpwZNLvVBKy0wLgM=
2trusted comment: re-ACK 9113f6b605f9e0a3b438561837f1f4e4142ad3c7 🍷
3zpDwXlcOI4l11MhTUagbDaSkMEEIAWBDTmYF6+SCff3iMIuYxOqry2UPAJrGEeZCFMmzcnvMp4iJ1MLSOdVCCA==

DrahtBot requested review from hodlinator on Jul 31, 2024

test: add uint256::FromHex unittest coverage

Simultaneously cover transaction_identifier::FromHex()

526a87ba6b

fuzz: increase FromHex() coverage 9a0b2a69c4

test: replace WtxidFromString with Wtxid::FromHex

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

285ab50ace

refactor: remove TxidFromString

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

f553e6d86f

stickies-v force-pushed on Jul 31, 2024

stickies-v commented at 3:51 pm on July 31, 2024: contributor

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

hodlinator approved

hodlinator commented at 8:46 pm on July 31, 2024: contributor

ACK f553e6d86fe114e90585ea6d4b8e345a209cae5d

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

DrahtBot requested review from maflcko on Jul 31, 2024

maflcko commented at 6:46 am on August 1, 2024: member

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 commented at 6:46 am on August 1, 2024: member

re-ACK f553e6d86fe114e90585ea6d4b8e345a209cae5d 🔻

Signature:

0untrusted 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}"
1RUTRmVTMeKV5npGrKx1nqXCw5zeVHdtdYURB/KlyA/LMFgpNCs+SkW9a8N95d+U4AP1RJMi+krxU1A3Yux4bpwZNLvVBKy0wLgM=
2trusted comment: re-ACK f553e6d86fe114e90585ea6d4b8e345a209cae5d 🔻
3uDZbcjyrvbfhAAjTYC5C+LSEI7ldXJfKAMe8JnZ3sy+CHq4+Sk0jtU11dxj46i5hk79CcicruCcb+zugYyyWDw==

in src/test/uint256_tests.cpp:344 in 526a87ba6b outdated

339+ */
340+template <typename T>
341+void TestFromHex()
342+{
343+    constexpr unsigned int num_chars{T::size() * 2};
344+    static_assert(num_chars <= 64); // this test needs to be modified to allow for more than 64 hex chars

paplorinc commented at 7:54 am on August 1, 2024:

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.

stickies-v commented at 10:27 am on August 1, 2024:

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

in src/test/uint256_tests.cpp:341 in 526a87ba6b outdated

332@@ -330,6 +333,59 @@ BOOST_AUTO_TEST_CASE(parse)
333     }
334 }
335 
336+/**
337+ * Implemented as a templated function so it can be reused by other classes that have a FromHex()
338+ * method that wraps base_blob::FromHex(), such as transaction_identifier::FromHex().
339+ */
340+template <typename T>
341+void TestFromHex()

paplorinc commented at 8:11 am on August 1, 2024:

👍 nice exhaustive test suite

in src/test/fuzz/hex.cpp:32 in 9a0b2a69c4 outdated

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

paplorinc commented at 8:11 am on August 1, 2024:

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

stickies-v commented at 10:33 am on August 1, 2024:

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?

in src/qt/coincontroldialog.cpp:336 in f553e6d86f

334@@ -340,9 +335,10 @@ void CoinControlDialog::radioListMode(bool checked)
335 // checkbox clicked by user
336 void CoinControlDialog::viewItemChanged(QTreeWidgetItem* item, int column)

paplorinc commented at 9:58 am on August 1, 2024:

opening the inputs triggers this: 👍

in src/qt/coincontroldialog.cpp:248 in f553e6d86f

244@@ -250,7 +245,7 @@ void CoinControlDialog::lockCoin()
245 // context menu action: unlock coin
246 void CoinControlDialog::unlockCoin()
247 {
248-    COutPoint outpt(TxidFromString(contextMenuItem->data(COLUMN_ADDRESS, TxHashRole).toString().toStdString()), contextMenuItem->data(COLUMN_ADDRESS, VOutRole).toUInt());
249+    COutPoint outpt(Txid::FromHex(contextMenuItem->data(COLUMN_ADDRESS, TxHashRole).toString().toStdString()).value(), contextMenuItem->data(COLUMN_ADDRESS, VOutRole).toUInt());

paplorinc commented at 10:06 am on August 1, 2024:

this was hit when selecting unlock from the menu:

in src/test/bloom_tests.cpp:141 in f553e6d86f

137@@ -138,11 +138,11 @@ BOOST_AUTO_TEST_CASE(bloom_match)
138     BOOST_CHECK_MESSAGE(filter.IsRelevantAndUpdate(tx), "Simple Bloom filter didn't match output address");
139 
140     filter = CBloomFilter(10, 0.000001, 0, BLOOM_UPDATE_ALL);
141-    filter.insert(COutPoint(TxidFromString("0x90c122d70786e899529d71dbeba91ba216982fb6ba58f3bdaab65e73b7e9260b"), 0));
142+    filter.insert(COutPoint(Txid::FromHex("90c122d70786e899529d71dbeba91ba216982fb6ba58f3bdaab65e73b7e9260b").value(), 0));

paplorinc commented at 10:07 am on August 1, 2024:

nit: might want to extract to avoid duplicating it 4 and 22 lines below (would make the bloom mistmatch slightly more obvious)

stickies-v commented at 10:30 am on August 1, 2024:

going to leave as is to minimize the diff since the bloom tests are not relevant to this PR

in src/test/uint256_tests.cpp:363 in f553e6d86f

358+        BOOST_CHECK(!T::FromHex(valid_input.substr(0, num_chars - 1)));
359+        BOOST_CHECK(!T::FromHex(valid_input + "0"));
360+    }
361+    {
362+        // check that non-hex characters are not accepted
363+        std::string invalid_chars{R"( !"#$%&'()*+,-./:;<=>?@GHIJKLMNOPQRSTUVWXYZ[\]^_`ghijklmnopqrstuvwxyz{|}~)"};

paplorinc commented at 10:11 am on August 1, 2024:

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

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

stickies-v commented at 10:39 am on August 1, 2024:

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

paplorinc commented at 11:18 am on August 1, 2024:

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

in src/qt/coincontroldialog.cpp:340 in f553e6d86f

334@@ -340,9 +335,10 @@ void CoinControlDialog::radioListMode(bool checked)
335 // checkbox clicked by user
336 void CoinControlDialog::viewItemChanged(QTreeWidgetItem* item, int column)
337 {
338-    if (column == COLUMN_CHECKBOX && item->data(COLUMN_ADDRESS, TxHashRole).toString().length() == 64) // transaction hash is 64 characters (this means it is a child node, so it is not a parent node in tree mode)
339-    {
340-        COutPoint outpt(TxidFromString(item->data(COLUMN_ADDRESS, TxHashRole).toString().toStdString()), item->data(COLUMN_ADDRESS, VOutRole).toUInt());
341+    if (column != COLUMN_CHECKBOX) return;
342+    auto txid{Txid::FromHex(item->data(COLUMN_ADDRESS, TxHashRole).toString().toStdString())};
343+    if (txid) { // a valid txid means this is a child node, and not a parent node in tree mode

TheCharlatan commented at 10:17 am on August 1, 2024:

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

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

stickies-v commented at 10:43 am on August 1, 2024:

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.

paplorinc commented at 11:19 am on August 1, 2024:

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

in src/test/uint256_tests.cpp:368 in f553e6d86f

363+        std::string invalid_chars{R"( !"#$%&'()*+,-./:;<=>?@GHIJKLMNOPQRSTUVWXYZ[\]^_`ghijklmnopqrstuvwxyz{|}~)"};
364+        for (auto c : invalid_chars) {
365+            BOOST_CHECK(!T::FromHex(valid_input.substr(0, num_chars - 1) + c));
366+        }
367+        // 0x prefixes are invalid
368+        std::string invalid_prefix{"0x" + valid_input};

paplorinc commented at 10:19 am on August 1, 2024:

this will fail because of the length requirerement, not the prefix, right?

0        std::string invalid_prefix{"0x" + valid_input.substr(0, num_chars - 2)};

stickies-v commented at 10:26 am on August 1, 2024:

Correct, this specific case is to ensure “0x” is not treated as some magic prefix that is silently trimmed. So I’m testing both the case of length 64 and 66 in the next 2 lines, so I’m going to keep this as is.

paplorinc approved

paplorinc commented at 10:20 am on August 1, 2024: contributor

ACK f553e6d86fe114e90585ea6d4b8e345a209cae5d

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

in src/test/uint256_tests.cpp:17 in f553e6d86f

12 #include <boost/test/unit_test.hpp>
13 
14 #include <iomanip>
15 #include <sstream>
16 #include <string>
17+#include <string_view>

TheCharlatan commented at 10:27 am on August 1, 2024:

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

maflcko commented at 10:43 am on August 1, 2024:

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

TheCharlatan approved

TheCharlatan commented at 10:28 am on August 1, 2024: contributor

Nice, ACK f553e6d86fe114e90585ea6d4b8e345a209cae5d

stickies-v commented at 10:44 am on August 1, 2024: contributor

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.

glozow commented at 11:02 am on August 1, 2024: member

concept ACK + lgtm

glozow merged this on Aug 1, 2024

glozow closed this on Aug 1, 2024

stickies-v deleted the branch on Aug 1, 2024

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

Testing GUI changes

Code Coverage

Reviews