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

pull stickies-v wants to merge 4 commits into bitcoin:master from stickies-v:2024-07/rm-txidfromstring changing 8 files +93 −57
  1. 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)
    
  2. 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.

  3. DrahtBot added the label Refactoring on Jul 26, 2024
  4. 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: 👍
  5. 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
  6. 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.

  7. 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: 👍
  8. stickies-v force-pushed on Jul 29, 2024
  9. stickies-v force-pushed on Jul 29, 2024
  10. DrahtBot added the label CI failed on Jul 29, 2024
  11. 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.

  12. 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.

  13. 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:
  14. 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.

  15. DrahtBot removed the label CI failed on Jul 29, 2024
  16. 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
  17. 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.

  18. hodlinator changes_requested
  19. 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
  20. stickies-v force-pushed on Jul 30, 2024
  21. stickies-v commented at 12:06 pm on July 30, 2024: contributor
  22. hodlinator approved
  23. 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).

  24. 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.
  25. 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
  26. 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.
  27. 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.
  28. 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.
  29. 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==
    
  30. 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>();
  31. stickies-v force-pushed on Jul 31, 2024
  32. 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.
  33. 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
  34. maflcko approved
  35. 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==
    
  36. DrahtBot requested review from hodlinator on Jul 31, 2024
  37. test: add uint256::FromHex unittest coverage
    Simultaneously cover transaction_identifier::FromHex()
    526a87ba6b
  38. fuzz: increase FromHex() coverage 9a0b2a69c4
  39. test: replace WtxidFromString with Wtxid::FromHex
    The newly introduced Wtxid::FromHex is more robust and removes
    the need for a WtxidFromString helper function
    285ab50ace
  40. 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
  41. stickies-v force-pushed on Jul 31, 2024
  42. 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.
  43. hodlinator approved
  44. 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.

  45. DrahtBot requested review from maflcko on Jul 31, 2024
  46. 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.

  47. 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==
    
  48. 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
  49. 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
  50. 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?
  51. 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: 👍
  52. 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:
  53. 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
  54. 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
  55. 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
  56. 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.
  57. paplorinc approved
  58. paplorinc commented at 10:20 am on August 1, 2024: contributor

    ACK f553e6d86fe114e90585ea6d4b8e345a209cae5d

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

  59. 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)
  60. TheCharlatan approved
  61. TheCharlatan commented at 10:28 am on August 1, 2024: contributor
    Nice, ACK f553e6d86fe114e90585ea6d4b8e345a209cae5d
  62. 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.
  63. glozow commented at 11:02 am on August 1, 2024: member
    concept ACK + lgtm
  64. glozow merged this on Aug 1, 2024
  65. glozow closed this on Aug 1, 2024

  66. stickies-v deleted the branch on Aug 1, 2024

github-metadata-mirror

This is a metadata mirror of the GitHub repository bitcoin/bitcoin. This site is not affiliated with GitHub. Content is generated from a GitHub metadata backup.
generated: 2024-10-18 04:12 UTC

This site is hosted by @0xB10C
More mirrored repositories can be found on mirror.b10c.me