qt: Update SetHexDeprecated to FromHex #32237

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

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

3. DrahtBot added the label GUI on Apr 8, 2025
4. 
   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.
5. 
   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?
6. 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.
7. 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.
8. 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.
9. 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.
10. laanwj changes_requested
11. 
    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.
12. 
    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

13. 
    hebasto commented at 8:00 am on April 9, 2025: member

    Concept ACK.
14. 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
15. refactor: Remove SetHexDeprecated

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

    868816d962
16. marcofleon force-pushed on Apr 9, 2025
17. 
    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.
18. 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

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

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

19. laanwj approved
20. 
    laanwj commented at 5:37 pm on April 9, 2025: member

    Code review ACK 868816d962ac55346381025e10bedc12dfcce113
21. DrahtBot requested review from hebasto on Apr 9, 2025
22. 
    w0xlt commented at 5:59 pm on April 9, 2025: contributor

    Code review ACK https://github.com/bitcoin/bitcoin/pull/32237/commits/868816d962ac55346381025e10bedc12dfcce113
23. 
    BrandonOdiwuor commented at 8:16 pm on April 9, 2025: contributor

    Code Review ACK 868816d962ac55346381025e10bedc12dfcce113
24. TheCharlatan approved
25. 
    TheCharlatan commented at 8:16 am on April 10, 2025: contributor

    ACK 868816d962ac55346381025e10bedc12dfcce113
26. hebasto approved
27. 
    hebasto commented at 11:27 am on April 10, 2025: member

    ACK 868816d962ac55346381025e10bedc12dfcce113, I have reviewed the code and it looks OK.
28. hebasto merged this on Apr 10, 2025
29. hebasto closed this on Apr 10, 2025

    ---

Contributors
marcofleon DrahtBot instagibbs laanwj hebasto w0xlt BrandonOdiwuor TheCharlatan

Labels
GUI