Skip to content

Commit

Permalink
refactor: remove TxidFromString
Browse files Browse the repository at this point in the history 
TxidFromString was deprecated due to missing 64-character length-check
and hex-check, replace it with the more robust Txid::FromHex.
  • Loading branch information
@stickies-v
stickies-v committed Jul 31, 2024
1 parent 285ab50 commit f553e6d
Show file tree
Hide file tree
Showing 6 changed files with 28 additions and 46 deletions.
26 changes: 11 additions & 15 deletions src/qt/coincontroldialog.cpp
View file
Original file line number Diff line number Diff line change
Expand Up @@ -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);
Expand Down Expand Up @@ -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"));
Expand All @@ -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());
Expand Down Expand Up @@ -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);
Expand Down
16 changes: 8 additions & 8 deletions src/test/bloom_tests.cpp
View file
Original file line number Diff line number Diff line change
Expand Up @@ -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<unsigned char> data(32 + sizeof(unsigned int));
memcpy(data.data(), prevOutPoint.hash.begin(), 32);
Expand All @@ -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");
}

Expand Down Expand Up @@ -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)
Expand All @@ -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<unsigned char> RandomData()
Expand Down
6 changes: 3 additions & 3 deletions src/test/merkleblock_tests.cpp
View file
Original file line number Diff line number Diff line change
Expand Up @@ -24,10 +24,10 @@ BOOST_AUTO_TEST_CASE(merkleblock_construct_from_txids_found)
std::set<Txid> 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);
Expand Down Expand Up @@ -63,7 +63,7 @@ BOOST_AUTO_TEST_CASE(merkleblock_construct_from_txids_not_found)
CBlock block = getBlock13b8a();

std::set<Txid> 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());
Expand Down
14 changes: 3 additions & 11 deletions src/test/transaction_tests.cpp
View file
Original file line number Diff line number Diff line change
Expand Up @@ -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<int>())};
COutPoint outpoint{Txid::FromHex(vinput[0].get_str()).value(), uint32_t(vinput[1].getInt<int>())};
mapprevOutScriptPubKeys[outpoint] = ParseScript(vinput[2].get_str());
if (vinput.size() >= 4)
{
Expand Down Expand Up @@ -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<int>())};
COutPoint outpoint{Txid::FromHex(vinput[0].get_str()).value(), uint32_t(vinput[1].getInt<int>())};
mapprevOutScriptPubKeys[outpoint] = ParseScript(vinput[2].get_str());
if (vinput.size() >= 4)
{
Expand Down Expand Up @@ -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;
Expand Down Expand Up @@ -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()
6 changes: 3 additions & 3 deletions src/test/txpackage_tests.cpp
View file
Original file line number Diff line number Diff line change
Expand Up @@ -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);
Expand Down
6 changes: 0 additions & 6 deletions src/util/transaction_identifier.h
View file
Original file line number Diff line number Diff line change
Expand Up @@ -72,10 +72,4 @@ using Txid = transaction_identifier<false>;
/** Wtxid commits to all transaction fields including the witness. */
using Wtxid = transaction_identifier<true>;

/** 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

0 comments on commit f553e6d

Please sign in to comment.