From 526a87ba6b4f20592ad3c090b82e83ecff2107fc Mon Sep 17 00:00:00 2001 From: stickies-v Date: Mon, 29 Jul 2024 17:16:54 +0100 Subject: [PATCH 1/4] test: add uint256::FromHex unittest coverage Simultaneously cover transaction_identifier::FromHex() --- src/test/uint256_tests.cpp | 56 ++++++++++++++++++++++++++++++++++++++ 1 file changed, 56 insertions(+) diff --git a/src/test/uint256_tests.cpp b/src/test/uint256_tests.cpp index d280126cde4..04f8682b69d 100644 --- a/src/test/uint256_tests.cpp +++ b/src/test/uint256_tests.cpp @@ -6,12 +6,15 @@ #include #include #include +#include +#include #include #include #include #include +#include #include BOOST_AUTO_TEST_SUITE(uint256_tests) @@ -330,6 +333,59 @@ BOOST_AUTO_TEST_CASE(parse) } } +/** + * Implemented as a templated function so it can be reused by other classes that have a FromHex() + * method that wraps base_blob::FromHex(), such as transaction_identifier::FromHex(). + */ +template +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 + const std::string valid_64char_input{"0123456789abcdef0123456789ABCDEF0123456789abcdef0123456789ABCDEF"}; + const auto valid_input{valid_64char_input.substr(0, num_chars)}; + { + // check that lower and upper case hex characters are accepted + auto valid_result{T::FromHex(valid_input)}; + BOOST_REQUIRE(valid_result); + BOOST_CHECK_EQUAL(valid_result->ToString(), ToLower(valid_input)); + } + { + // check that only strings of size num_chars are accepted + BOOST_CHECK(!T::FromHex("")); + BOOST_CHECK(!T::FromHex("0")); + BOOST_CHECK(!T::FromHex(valid_input.substr(0, num_chars / 2))); + BOOST_CHECK(!T::FromHex(valid_input.substr(0, num_chars - 1))); + BOOST_CHECK(!T::FromHex(valid_input + "0")); + } + { + // check that non-hex characters are not accepted + std::string invalid_chars{R"( !"#$%&'()*+,-./:;<=>?@GHIJKLMNOPQRSTUVWXYZ[\]^_`ghijklmnopqrstuvwxyz{|}~)"}; + for (auto c : invalid_chars) { + BOOST_CHECK(!T::FromHex(valid_input.substr(0, num_chars - 1) + c)); + } + // 0x prefixes are invalid + std::string invalid_prefix{"0x" + valid_input}; + BOOST_CHECK(!T::FromHex(std::string_view(invalid_prefix.data(), num_chars))); + BOOST_CHECK(!T::FromHex(invalid_prefix)); + } + { + // check that string_view length is respected + std::string chars_68{valid_64char_input + "0123"}; + BOOST_CHECK_EQUAL(T::FromHex(std::string_view(chars_68.data(), num_chars)).value().ToString(), ToLower(valid_input)); + BOOST_CHECK(!T::FromHex(std::string_view(chars_68.data(), num_chars - 1))); // too short + BOOST_CHECK(!T::FromHex(std::string_view(chars_68.data(), num_chars + 1))); // too long + } +} + +BOOST_AUTO_TEST_CASE(from_hex) +{ + TestFromHex(); + TestFromHex(); + TestFromHex(); + TestFromHex(); +} + BOOST_AUTO_TEST_CASE( check_ONE ) { uint256 one = uint256S("0000000000000000000000000000000000000000000000000000000000000001"); From 9a0b2a69c4d836d7382f6fe7703de40288fc7421 Mon Sep 17 00:00:00 2001 From: stickies-v Date: Mon, 29 Jul 2024 17:35:01 +0100 Subject: [PATCH 2/4] fuzz: increase FromHex() coverage --- src/test/fuzz/hex.cpp | 7 ++++++- 1 file changed, 6 insertions(+), 1 deletion(-) diff --git a/src/test/fuzz/hex.cpp b/src/test/fuzz/hex.cpp index bc46863f1df..ebe30c3c1aa 100644 --- a/src/test/fuzz/hex.cpp +++ b/src/test/fuzz/hex.cpp @@ -10,6 +10,7 @@ #include #include #include +#include #include #include @@ -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); + assert(Txid::FromHex(random_hex_string)); + assert(Wtxid::FromHex(random_hex_string)); + } (void)uint256S(random_hex_string); try { (void)HexToPubKey(random_hex_string); From 285ab50ace4c04209f331ccaf121152b977cc490 Mon Sep 17 00:00:00 2001 From: stickies-v Date: Thu, 25 Jul 2024 21:09:21 +0100 Subject: [PATCH 3/4] test: replace WtxidFromString with Wtxid::FromHex The newly introduced Wtxid::FromHex is more robust and removes the need for a WtxidFromString helper function --- src/test/txpackage_tests.cpp | 13 +++---------- 1 file changed, 3 insertions(+), 10 deletions(-) diff --git a/src/test/txpackage_tests.cpp b/src/test/txpackage_tests.cpp index f16c0cdafd4..8997b95e80e 100644 --- a/src/test/txpackage_tests.cpp +++ b/src/test/txpackage_tests.cpp @@ -43,13 +43,6 @@ inline CTransactionRef create_placeholder_tx(size_t num_inputs, size_t num_outpu } return MakeTransactionRef(mtx); } - -// Create a Wtxid from a hex string -inline Wtxid WtxidFromString(std::string_view str) -{ - return Wtxid::FromUint256(uint256S(str)); -} - BOOST_FIXTURE_TEST_CASE(package_hash_tests, TestChain100Setup) { // Random real segwit transaction @@ -74,9 +67,9 @@ BOOST_FIXTURE_TEST_CASE(package_hash_tests, TestChain100Setup) CTransactionRef ptx_3{MakeTransactionRef(tx_3)}; // It's easy to see that wtxids are sorted in lexicographical order: - Wtxid wtxid_1{WtxidFromString("0x85cd1a31eb38f74ed5742ec9cb546712ab5aaf747de28a9168b53e846cbda17f")}; - Wtxid wtxid_2{WtxidFromString("0xb4749f017444b051c44dfd2720e88f314ff94f3dd6d56d40ef65854fcd7fff6b")}; - Wtxid wtxid_3{WtxidFromString("0xe065bac15f62bb4e761d761db928ddee65a47296b2b776785abb912cdec474e3")}; + Wtxid wtxid_1{Wtxid::FromHex("85cd1a31eb38f74ed5742ec9cb546712ab5aaf747de28a9168b53e846cbda17f").value()}; + Wtxid wtxid_2{Wtxid::FromHex("b4749f017444b051c44dfd2720e88f314ff94f3dd6d56d40ef65854fcd7fff6b").value()}; + Wtxid wtxid_3{Wtxid::FromHex("e065bac15f62bb4e761d761db928ddee65a47296b2b776785abb912cdec474e3").value()}; BOOST_CHECK_EQUAL(tx_1.GetWitnessHash(), wtxid_1); BOOST_CHECK_EQUAL(tx_2.GetWitnessHash(), wtxid_2); BOOST_CHECK_EQUAL(tx_3.GetWitnessHash(), wtxid_3); From f553e6d86fe114e90585ea6d4b8e345a209cae5d Mon Sep 17 00:00:00 2001 From: stickies-v Date: Fri, 26 Jul 2024 11:51:40 +0100 Subject: [PATCH 4/4] refactor: remove TxidFromString TxidFromString was deprecated due to missing 64-character length-check and hex-check, replace it with the more robust Txid::FromHex. --- src/qt/coincontroldialog.cpp | 26 +++++++++++--------------- src/test/bloom_tests.cpp | 16 ++++++++-------- src/test/merkleblock_tests.cpp | 6 +++--- src/test/transaction_tests.cpp | 14 +++----------- src/test/txpackage_tests.cpp | 6 +++--- src/util/transaction_identifier.h | 6 ------ 6 files changed, 28 insertions(+), 46 deletions(-) diff --git a/src/qt/coincontroldialog.cpp b/src/qt/coincontroldialog.cpp index 5e412bd6b19..febf1ee82fa 100644 --- a/src/qt/coincontroldialog.cpp +++ b/src/qt/coincontroldialog.cpp @@ -174,22 +174,17 @@ void CoinControlDialog::showMenu(const QPoint &point) contextMenuItem = item; // disable some items (like Copy Transaction ID, lock, unlock) for tree roots in context menu - 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) - { + 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 m_copy_transaction_outpoint_action->setEnabled(true); - if (model->wallet().isLockedCoin(COutPoint(TxidFromString(item->data(COLUMN_ADDRESS, TxHashRole).toString().toStdString()), item->data(COLUMN_ADDRESS, VOutRole).toUInt()))) - { + if (model->wallet().isLockedCoin(COutPoint(*txid, item->data(COLUMN_ADDRESS, VOutRole).toUInt()))) { lockAction->setEnabled(false); unlockAction->setEnabled(true); - } - else - { + } else { lockAction->setEnabled(true); unlockAction->setEnabled(false); } - } - else // this means click on parent node in tree mode -> disable all - { + } else { // this means click on parent node in tree mode -> disable all m_copy_transaction_outpoint_action->setEnabled(false); lockAction->setEnabled(false); unlockAction->setEnabled(false); @@ -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()); model->wallet().lockCoin(outpt, /* write_to_db = */ true); contextMenuItem->setDisabled(true); contextMenuItem->setIcon(COLUMN_CHECKBOX, platformStyle->SingleColorIcon(":/icons/lock_closed")); @@ -250,7 +245,7 @@ void CoinControlDialog::lockCoin() // context menu action: unlock coin void CoinControlDialog::unlockCoin() { - 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()); model->wallet().unlockCoin(outpt); contextMenuItem->setDisabled(false); contextMenuItem->setIcon(COLUMN_CHECKBOX, QIcon()); @@ -340,9 +335,10 @@ void CoinControlDialog::radioListMode(bool checked) // checkbox clicked by user void CoinControlDialog::viewItemChanged(QTreeWidgetItem* item, int column) { - 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) - { - 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 + COutPoint outpt(*txid, item->data(COLUMN_ADDRESS, VOutRole).toUInt()); if (item->checkState(COLUMN_CHECKBOX) == Qt::Unchecked) m_coin_control.UnSelect(outpt); diff --git a/src/test/bloom_tests.cpp b/src/test/bloom_tests.cpp index 6699afdbfab..92ccedf6616 100644 --- a/src/test/bloom_tests.cpp +++ b/src/test/bloom_tests.cpp @@ -138,11 +138,11 @@ BOOST_AUTO_TEST_CASE(bloom_match) BOOST_CHECK_MESSAGE(filter.IsRelevantAndUpdate(tx), "Simple Bloom filter didn't match output address"); filter = CBloomFilter(10, 0.000001, 0, BLOOM_UPDATE_ALL); - filter.insert(COutPoint(TxidFromString("0x90c122d70786e899529d71dbeba91ba216982fb6ba58f3bdaab65e73b7e9260b"), 0)); + filter.insert(COutPoint(Txid::FromHex("90c122d70786e899529d71dbeba91ba216982fb6ba58f3bdaab65e73b7e9260b").value(), 0)); BOOST_CHECK_MESSAGE(filter.IsRelevantAndUpdate(tx), "Simple Bloom filter didn't match COutPoint"); filter = CBloomFilter(10, 0.000001, 0, BLOOM_UPDATE_ALL); - COutPoint prevOutPoint(TxidFromString("0x90c122d70786e899529d71dbeba91ba216982fb6ba58f3bdaab65e73b7e9260b"), 0); + COutPoint prevOutPoint(Txid::FromHex("90c122d70786e899529d71dbeba91ba216982fb6ba58f3bdaab65e73b7e9260b").value(), 0); { std::vector data(32 + sizeof(unsigned int)); memcpy(data.data(), prevOutPoint.hash.begin(), 32); @@ -160,11 +160,11 @@ BOOST_AUTO_TEST_CASE(bloom_match) BOOST_CHECK_MESSAGE(!filter.IsRelevantAndUpdate(tx), "Simple Bloom filter matched random address"); filter = CBloomFilter(10, 0.000001, 0, BLOOM_UPDATE_ALL); - filter.insert(COutPoint(TxidFromString("0x90c122d70786e899529d71dbeba91ba216982fb6ba58f3bdaab65e73b7e9260b"), 1)); + filter.insert(COutPoint(Txid::FromHex("90c122d70786e899529d71dbeba91ba216982fb6ba58f3bdaab65e73b7e9260b").value(), 1)); BOOST_CHECK_MESSAGE(!filter.IsRelevantAndUpdate(tx), "Simple Bloom filter matched COutPoint for an output we didn't care about"); filter = CBloomFilter(10, 0.000001, 0, BLOOM_UPDATE_ALL); - filter.insert(COutPoint(TxidFromString("0x000000d70786e899529d71dbeba91ba216982fb6ba58f3bdaab65e73b7e9260b"), 0)); + filter.insert(COutPoint(Txid::FromHex("000000d70786e899529d71dbeba91ba216982fb6ba58f3bdaab65e73b7e9260b").value(), 0)); BOOST_CHECK_MESSAGE(!filter.IsRelevantAndUpdate(tx), "Simple Bloom filter matched COutPoint for an output we didn't care about"); } @@ -426,9 +426,9 @@ BOOST_AUTO_TEST_CASE(merkle_block_4_test_p2pubkey_only) BOOST_CHECK(merkleBlock.header.GetHash() == block.GetHash()); // We should match the generation outpoint - BOOST_CHECK(filter.contains(COutPoint(TxidFromString("0x147caa76786596590baa4e98f5d9f48b86c7765e489f7a6ff3360fe5c674360b"), 0))); + BOOST_CHECK(filter.contains(COutPoint(Txid::FromHex("147caa76786596590baa4e98f5d9f48b86c7765e489f7a6ff3360fe5c674360b").value(), 0))); // ... but not the 4th transaction's output (its not pay-2-pubkey) - BOOST_CHECK(!filter.contains(COutPoint(TxidFromString("0x02981fa052f0481dbc5868f4fc2166035a10f27a03cfd2de67326471df5bc041"), 0))); + BOOST_CHECK(!filter.contains(COutPoint(Txid::FromHex("02981fa052f0481dbc5868f4fc2166035a10f27a03cfd2de67326471df5bc041").value(), 0))); } BOOST_AUTO_TEST_CASE(merkle_block_4_test_update_none) @@ -451,8 +451,8 @@ BOOST_AUTO_TEST_CASE(merkle_block_4_test_update_none) BOOST_CHECK(merkleBlock.header.GetHash() == block.GetHash()); // We shouldn't match any outpoints (UPDATE_NONE) - BOOST_CHECK(!filter.contains(COutPoint(TxidFromString("0x147caa76786596590baa4e98f5d9f48b86c7765e489f7a6ff3360fe5c674360b"), 0))); - BOOST_CHECK(!filter.contains(COutPoint(TxidFromString("0x02981fa052f0481dbc5868f4fc2166035a10f27a03cfd2de67326471df5bc041"), 0))); + BOOST_CHECK(!filter.contains(COutPoint(Txid::FromHex("147caa76786596590baa4e98f5d9f48b86c7765e489f7a6ff3360fe5c674360b").value(), 0))); + BOOST_CHECK(!filter.contains(COutPoint(Txid::FromHex("02981fa052f0481dbc5868f4fc2166035a10f27a03cfd2de67326471df5bc041").value(), 0))); } static std::vector RandomData() diff --git a/src/test/merkleblock_tests.cpp b/src/test/merkleblock_tests.cpp index 9c6008bdca0..73dbe337143 100644 --- a/src/test/merkleblock_tests.cpp +++ b/src/test/merkleblock_tests.cpp @@ -24,10 +24,10 @@ BOOST_AUTO_TEST_CASE(merkleblock_construct_from_txids_found) std::set txids; // Last txn in block. - Txid txhash1{TxidFromString("0x74d681e0e03bafa802c8aa084379aa98d9fcd632ddc2ed9782b586ec87451f20")}; + Txid txhash1{Txid::FromHex("74d681e0e03bafa802c8aa084379aa98d9fcd632ddc2ed9782b586ec87451f20").value()}; // Second txn in block. - Txid txhash2{TxidFromString("0xf9fc751cb7dc372406a9f8d738d5e6f8f63bab71986a39cf36ee70ee17036d07")}; + Txid txhash2{Txid::FromHex("f9fc751cb7dc372406a9f8d738d5e6f8f63bab71986a39cf36ee70ee17036d07").value()}; txids.insert(txhash1); txids.insert(txhash2); @@ -63,7 +63,7 @@ BOOST_AUTO_TEST_CASE(merkleblock_construct_from_txids_not_found) CBlock block = getBlock13b8a(); std::set txids2; - txids2.insert(TxidFromString("0xc0ffee00003bafa802c8aa084379aa98d9fcd632ddc2ed9782b586ec87451f20")); + txids2.insert(Txid::FromHex("c0ffee00003bafa802c8aa084379aa98d9fcd632ddc2ed9782b586ec87451f20").value()); CMerkleBlock merkleBlock(block, txids2); BOOST_CHECK_EQUAL(merkleBlock.header.GetHash().GetHex(), block.GetHash().GetHex()); diff --git a/src/test/transaction_tests.cpp b/src/test/transaction_tests.cpp index b152fc55844..ebd316ed4a0 100644 --- a/src/test/transaction_tests.cpp +++ b/src/test/transaction_tests.cpp @@ -223,7 +223,7 @@ BOOST_AUTO_TEST_CASE(tx_valid) fValid = false; break; } - COutPoint outpoint{TxidFromString(vinput[0].get_str()), uint32_t(vinput[1].getInt())}; + COutPoint outpoint{Txid::FromHex(vinput[0].get_str()).value(), uint32_t(vinput[1].getInt())}; mapprevOutScriptPubKeys[outpoint] = ParseScript(vinput[2].get_str()); if (vinput.size() >= 4) { @@ -311,7 +311,7 @@ BOOST_AUTO_TEST_CASE(tx_invalid) fValid = false; break; } - COutPoint outpoint{TxidFromString(vinput[0].get_str()), uint32_t(vinput[1].getInt())}; + COutPoint outpoint{Txid::FromHex(vinput[0].get_str()).value(), uint32_t(vinput[1].getInt())}; mapprevOutScriptPubKeys[outpoint] = ParseScript(vinput[2].get_str()); if (vinput.size() >= 4) { @@ -542,7 +542,7 @@ BOOST_AUTO_TEST_CASE(test_big_witness_transaction) // create a big transaction of 4500 inputs signed by the same key for(uint32_t ij = 0; ij < 4500; ij++) { uint32_t i = mtx.vin.size(); - COutPoint outpoint(TxidFromString("0000000000000000000000000000000000000000000000000000000000000100"), i); + COutPoint outpoint(Txid::FromHex("0000000000000000000000000000000000000000000000000000000000000100").value(), i); mtx.vin.resize(mtx.vin.size() + 1); mtx.vin[i].prevout = outpoint; @@ -1028,12 +1028,4 @@ BOOST_AUTO_TEST_CASE(test_IsStandard) } } -BOOST_AUTO_TEST_CASE(test_TxidFromString) -{ - // Make sure TxidFromString respects string_view length and stops reading at - // end of the substring. - BOOST_CHECK_EQUAL(TxidFromString(std::string_view("ABCD1234", 4)).ToString(), - "000000000000000000000000000000000000000000000000000000000000abcd"); -} - BOOST_AUTO_TEST_SUITE_END() diff --git a/src/test/txpackage_tests.cpp b/src/test/txpackage_tests.cpp index 8997b95e80e..ca9dc5527df 100644 --- a/src/test/txpackage_tests.cpp +++ b/src/test/txpackage_tests.cpp @@ -78,9 +78,9 @@ BOOST_FIXTURE_TEST_CASE(package_hash_tests, TestChain100Setup) BOOST_CHECK(wtxid_2.GetHex() < wtxid_3.GetHex()); // The txids are not (we want to test that sorting and hashing use wtxid, not txid): - Txid txid_1{TxidFromString("0xbd0f71c1d5e50589063e134fad22053cdae5ab2320db5bf5e540198b0b5a4e69")}; - Txid txid_2{TxidFromString("0xb4749f017444b051c44dfd2720e88f314ff94f3dd6d56d40ef65854fcd7fff6b")}; - 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()}; BOOST_CHECK_EQUAL(tx_1.GetHash(), txid_1); BOOST_CHECK_EQUAL(tx_2.GetHash(), txid_2); BOOST_CHECK_EQUAL(tx_3.GetHash(), txid_3); diff --git a/src/util/transaction_identifier.h b/src/util/transaction_identifier.h index 0e3251f4fce..81b053843df 100644 --- a/src/util/transaction_identifier.h +++ b/src/util/transaction_identifier.h @@ -72,10 +72,4 @@ using Txid = transaction_identifier; /** Wtxid commits to all transaction fields including the witness. */ using Wtxid = transaction_identifier; -/** DEPRECATED due to missing length-check and hex-check, please use the safer FromHex, or FromUint256 */ -inline Txid TxidFromString(std::string_view str) -{ - return Txid::FromUint256(uint256S(str)); -} - #endif // BITCOIN_UTIL_TRANSACTION_IDENTIFIER_H