mirror/bitcoin
1
0 Fork 0
Code Issues Pull Requests Actions Packages Projects Releases Wiki Activity

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

Browse Source 
f553e6d86f refactor: remove TxidFromString (stickies-v)
285ab50ace test: replace WtxidFromString with Wtxid::FromHex (stickies-v)
9a0b2a69c4 fuzz: increase FromHex() coverage (stickies-v)
526a87ba6b test: add uint256::FromHex unittest coverage (stickies-v)

Pull request description:

  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](https://github.com/bitcoin/bitcoin/pull/30482). Specifically, it tries to recover from length-mismatches, recover from untrimmed whitespace, 0x-prefix and garbage at the end, instead of simply requiring exactly 64 hex-only characters.

  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: https://github.com/bitcoin/bitcoin/pull/30482#discussion_r1691826934

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

  ### Testing GUI changes

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

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

ACKs for top commit:
  maflcko:
    re-ACK f553e6d86f 🔻
  hodlinator:
    ACK f553e6d86f
  paplorinc:
    ACK f553e6d86f
  TheCharlatan:
    Nice, ACK f553e6d86f

Tree-SHA512: c1c7e6ea4cbf05cf660ba178ffc4f35f0328f7aa6ad81872e2462fb91a6a22e4681ff64b3d0202a5a9abcb650c939561585cd309164a69ab6081c0765ee271ef
This commit is contained in:
glozow committed 2024-08-01 12:02:52 +01:00
2 parents dc605cf6e6 f553e6d86f
commit ebd82fa9fa
8 files changed
+93 -57
Download Patch File Download Diff File Expand all files Collapse all files

No files matched your search

src/qt/coincontroldialog.cpp
+11 -15
View File
@@ -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);