qt: Update SetHexDeprecated to FromHex #32237

marcofleon commented at 3:10 PM on April 8, 2025: contributor

This is part of #32189. I'm separating this out because it's not immediately obvious that it's just a refactor. SetHexDeprecated() doesn't do any correctness checks on the input, while FromHex() does, so it's theoretically possible that there's a behavior change.

Replaces uint256::SetHexDeprecated() calls with Txid::FromHex() in four locations:

TransactionTableModel::updateTransaction
TransactionView::contextualMenu
TransactionView::abandonTx
TransactionView::bumpFee

The input strings in these cases aren't user input, so they should only be valid hex strings from GetHex() (through TransactionRecord::getTxHash()). These conversions should be safe without additional checks.

DrahtBot commented at 3:10 PM on April 8, 2025: contributor

The following sections might be updated with supplementary metadata relevant to reviewers and maintainers.

Code Coverage & Benchmarks

For details see: https://corecheck.dev/bitcoin/bitcoin/pulls/32237.

Reviews

See the guideline for information on the review process.

Type	Reviewers
ACK	laanwj, w0xlt, BrandonOdiwuor, TheCharlatan, hebasto

If your review is incorrectly listed, please react with 👎 to this comment and the bot will ignore it on the next update.

Conflicts

Reviewers, this pull request conflicts with the following ones:

#32238 (qt, wallet: Convert uint256 to Txid by marcofleon)
#32189 (refactor: Txid type safety (parent PR) by marcofleon)

If you consider this pull request important, please also help to review the conflicting pull requests. Ideally, start with the one that should be merged first.

DrahtBot added the label GUI on Apr 8, 2025

marcofleon commented at 3:11 PM on April 8, 2025: contributor

@hebasto @furszy If you could look this over and make sure I didn't miss anything here, I would appreciate it.

instagibbs commented at 4:16 PM on April 8, 2025: member

suggestion to maybe nuke that last usage of SetHexDeprecated and then just nuke it entirely?

in src/qt/transactiontablemodel.cpp:279 in 226454f154 outdated

 275 | @@ -276,8 +276,7 @@ void TransactionTableModel::updateAmountColumnTitle()
 276 |  
 277 |  void TransactionTableModel::updateTransaction(const QString &hash, int status, bool showTransaction)
 278 |  {
 279 | -    uint256 updated;
 280 | -    updated.SetHexDeprecated(hash.toStdString());
 281 | +    Txid updated = Txid::FromHex(hash.toStdString()).value();

laanwj commented at 7:29 AM on April 9, 2025:

This seems reasonable: updateTransaction is called from TransactionNotification::invoke, which creates the string using uint256::GetHex.

hebasto commented at 8:52 PM on April 9, 2025:

This use of SetHexDeprecated could be eliminated by changing the type of the first parameter from const QString& to const uint256&. See this commit in this branch.

in src/qt/transactionview.cpp:399 in 226454f154 outdated

 394 | @@ -395,8 +395,8 @@ void TransactionView::contextualMenu(const QPoint &point)
 395 |          return;
 396 |  
 397 |      // check if transaction can be abandoned, disable context menu action in case it doesn't
 398 | -    uint256 hash;
 399 | -    hash.SetHexDeprecated(selection.at(0).data(TransactionTableModel::TxHashRole).toString().toStdString());
 400 | +    QString hashQStr = selection.at(0).data(TransactionTableModel::TxHashRole).toString();
 401 | +    Txid hash = Txid::FromHex(hashQStr.toStdString()).value();

laanwj commented at 7:38 AM on April 9, 2025:

The string here comes from TransactionTableModel::data(TxHashRole), which is filled in from TransactionRecord::getTxHash which calls uint256::GetHex.

So far, so good, but because of potential unpredictability in internal Qt logic (eg what if the table has an incomplete row for some reason) i'm not 100% reassured that this can never return a null QVariant value, so would prefer to err on the side of "don't display the context menu" to making an hard assumption.

marcofleon commented at 3:12 PM on April 9, 2025:

Thanks for reviewing. Let me know if you had something else in mind for this check. I think the early return (not showing the menu?) is a slight behavior change, as opposed to just disabling the actions. If that works then nice, but if not then I can change it to be exactly the old behavior.

in src/qt/transactionview.cpp:422 in 226454f154 outdated

 413 | @@ -414,9 +414,8 @@ void TransactionView::abandonTx()
 414 |      QModelIndexList selection = transactionView->selectionModel()->selectedRows(0);
 415 |  
 416 |      // get the hash from the TxHashRole (QVariant / QString)
 417 | -    uint256 hash;
 418 |      QString hashQStr = selection.at(0).data(TransactionTableModel::TxHashRole).toString();
 419 | -    hash.SetHexDeprecated(hashQStr.toStdString());
 420 | +    Txid hash = Txid::FromHex(hashQStr.toStdString()).value();

laanwj commented at 7:39 AM on April 9, 2025:

Seems fine, same reasoning about .data() as before. Plus if we get here, the context menu was enabled.

in src/qt/transactionview.cpp:436 in 226454f154 outdated

 427 | @@ -429,9 +428,8 @@ void TransactionView::bumpFee([[maybe_unused]] bool checked)
 428 |      QModelIndexList selection = transactionView->selectionModel()->selectedRows(0);
 429 |  
 430 |      // get the hash from the TxHashRole (QVariant / QString)
 431 | -    uint256 hash;
 432 |      QString hashQStr = selection.at(0).data(TransactionTableModel::TxHashRole).toString();
 433 | -    hash.SetHexDeprecated(hashQStr.toStdString());
 434 | +    Txid hash = Txid::FromHex(hashQStr.toStdString()).value();

laanwj commented at 7:42 AM on April 9, 2025:

Same case as abandonTx(), this is called from the context menu only.

laanwj changes_requested

laanwj commented at 7:44 AM on April 9, 2025: member

Changes LGTM, but would prefer handling data(TxHashRole) returning a null QVariant more robustly in TransactionView::contextualMenu.

laanwj commented at 7:45 AM on April 9, 2025: member

suggestion to maybe nuke that last usage of SetHexDeprecated and then just nuke it entirely?

nuke it

hebasto commented at 8:00 AM on April 9, 2025: member

Concept ACK.

qt: Update SetHexDeprecated to FromHex

Replace `uint256::SetHexDeprecated()` calls with `Txid::FromHex()`
in four locations:
- TransactionTableModel::updateTransaction
- TransactionView::contextualMenu
- TransactionView::abandonTx
- TransactionView::bumpFee

The input strings are generally expected to be valid hex strings
from `GetHex()`. However, due to the potentially unpredictable return
value of `.data(TransactionTableModel::TxHashRole)`, check the
`Txid::FromHex` result in `contextualMenu` and return early if the
transaction hash is invalid. The other two functions, `abandonTx`
and `bumpFee` will only be called if the context menu is enabled.

6b63218ec2

refactor: Remove SetHexDeprecated

After replacing all instances of `SetHexDeprecated` in the GUI,
remove it entirely and reimplement the behavior in `FromHex`.

868816d962

marcofleon force-pushed on Apr 9, 2025

marcofleon commented at 3:15 PM on April 9, 2025: contributor

Pushed changes addressing @instagibbs and @laanwj 's suggestions. Let me know if I missed something.

in src/uint256.h:152 in 868816d962

 148 | +    unsigned char* p1 = rv.begin();
 149 | +    unsigned char* pend = rv.end();
 150 | +    size_t digits = str.size();
 151 | +    while (digits > 0 && p1 < pend) {
 152 | +        *p1 = ::HexDigit(str[--digits]);
 153 | +        if (digits > 0) {

laanwj commented at 5:32 PM on April 9, 2025:

Because the uintN_t::size() * 2 == str.size() check already makes sure there can be no underrun here, one could shorten this to

*p1 = (unsigned char)::HexDigit(str[digits - 1]) | ((unsigned char)::HexDigit(str[digits - 2]) << 4);
digits -= 2;

That said, for a refactor it seems better to move the code as obviously as possible as done here.

laanwj approved

laanwj commented at 5:37 PM on April 9, 2025: member

Code review ACK 868816d962ac55346381025e10bedc12dfcce113

DrahtBot requested review from hebasto on Apr 9, 2025

w0xlt commented at 5:59 PM on April 9, 2025: contributor

Code review ACK https://github.com/bitcoin/bitcoin/pull/32237/commits/868816d962ac55346381025e10bedc12dfcce113

BrandonOdiwuor commented at 8:16 PM on April 9, 2025: contributor

Code Review ACK 868816d962ac55346381025e10bedc12dfcce113

TheCharlatan approved

TheCharlatan commented at 8:16 AM on April 10, 2025: contributor

ACK 868816d962ac55346381025e10bedc12dfcce113

hebasto approved

hebasto commented at 11:27 AM on April 10, 2025: member

ACK 868816d962ac55346381025e10bedc12dfcce113, I have reviewed the code and it looks OK.

hebasto merged this on Apr 10, 2025

hebasto closed this on Apr 10, 2025

TheCharlatan referenced this in commit a9c46ce3c3 on Apr 24, 2025

marcofleon deleted the branch on May 22, 2025