Merge bitcoin/bitcoin#32237: qt: Update SetHexDeprecated to FromHex

868816d refactor: Remove SetHexDeprecated (marcofleon)
6b63218 qt: Update SetHexDeprecated to FromHex (marcofleon)

Pull request description:

  This is part of bitcoin/bitcoin#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.

ACKs for top commit:
  laanwj:
    Code review ACK 868816d
  w0xlt:
    Code review ACK bitcoin/bitcoin@868816d
  BrandonOdiwuor:
    Code Review ACK 868816d
  TheCharlatan:
    ACK 868816d
  hebasto:
    ACK 868816d, I have reviewed the code and it looks OK.

Tree-SHA512: 121f149dcc7358231d0327cb3212ec96486a88410174d3c74ab8cbd61bad35185bc0a9740d534492b714811f72a6736bc7ac6eeae590c0ea1365c61cc791da37

Author: hebasto

src/qt/transactiontablemodel.cpp

@@ -276,8 +276,7 @@ void TransactionTableModel::updateAmountColumnTitle()
 
 void TransactionTableModel::updateTransaction(const QString &hash, int status, bool showTransaction)
 {
-    uint256 updated;
-    updated.SetHexDeprecated(hash.toStdString());
+    Txid updated = Txid::FromHex(hash.toStdString()).value();
 
     priv->updateWallet(walletModel->wallet(), updated, status, showTransaction);
 }

src/qt/transactionview.cpp

@@ -394,9 +394,13 @@ void TransactionView::contextualMenu(const QPoint &point)
     if (selection.empty())
         return;
 
-    // check if transaction can be abandoned, disable context menu action in case it doesn't
-    uint256 hash;
-    hash.SetHexDeprecated(selection.at(0).data(TransactionTableModel::TxHashRole).toString().toStdString());
+    // If the hash from the TxHashRole (QVariant / QString) is invalid, exit
+    QString hashQStr = selection.at(0).data(TransactionTableModel::TxHashRole).toString();
+    std::optional<Txid> maybeHash = Txid::FromHex(hashQStr.toStdString());
+    if (!maybeHash)
+        return;
+
+    Txid hash = *maybeHash;
     abandonAction->setEnabled(model->wallet().transactionCanBeAbandoned(hash));
     bumpFeeAction->setEnabled(model->wallet().transactionCanBeBumped(hash));
     copyAddressAction->setEnabled(GUIUtil::hasEntryData(transactionView, 0, TransactionTableModel::AddressRole));
@@ -414,9 +418,8 @@ void TransactionView::abandonTx()
     QModelIndexList selection = transactionView->selectionModel()->selectedRows(0);
 
     // get the hash from the TxHashRole (QVariant / QString)
-    uint256 hash;
     QString hashQStr = selection.at(0).data(TransactionTableModel::TxHashRole).toString();
-    hash.SetHexDeprecated(hashQStr.toStdString());
+    Txid hash = Txid::FromHex(hashQStr.toStdString()).value();
 
     // Abandon the wallet transaction over the walletModel
     model->wallet().abandonTransaction(hash);
@@ -429,9 +432,8 @@ void TransactionView::bumpFee([[maybe_unused]] bool checked)
     QModelIndexList selection = transactionView->selectionModel()->selectedRows(0);
 
     // get the hash from the TxHashRole (QVariant / QString)
-    uint256 hash;
     QString hashQStr = selection.at(0).data(TransactionTableModel::TxHashRole).toString();
-    hash.SetHexDeprecated(hashQStr.toStdString());
+    Txid hash = Txid::FromHex(hashQStr.toStdString()).value();
 
     // Bump tx fee over the walletModel
     uint256 newHash;

src/test/uint256_tests.cpp

@@ -147,17 +147,14 @@ BOOST_AUTO_TEST_CASE( comparison ) // <= >= < >
                    uint256{"0000000000000000000000000000000000000000000000000000000000000001"});
 }
 
-BOOST_AUTO_TEST_CASE(methods) // GetHex SetHexDeprecated FromHex begin() end() size() GetLow64 GetSerializeSize, Serialize, Unserialize
+BOOST_AUTO_TEST_CASE(methods) // GetHex FromHex begin() end() size() GetLow64 GetSerializeSize, Serialize, Unserialize
 {
     BOOST_CHECK_EQUAL(R1L.GetHex(), R1L.ToString());
     BOOST_CHECK_EQUAL(R2L.GetHex(), R2L.ToString());
     BOOST_CHECK_EQUAL(OneL.GetHex(), OneL.ToString());
     BOOST_CHECK_EQUAL(MaxL.GetHex(), MaxL.ToString());
     uint256 TmpL(R1L);
     BOOST_CHECK_EQUAL(TmpL, R1L);
-    // Verify previous values don't persist when setting to truncated string.
-    TmpL.SetHexDeprecated("21");
-    BOOST_CHECK_EQUAL(TmpL.ToString(), "0000000000000000000000000000000000000000000000000000000000000021");
     BOOST_CHECK_EQUAL(uint256::FromHex(R2L.ToString()).value(), R2L);
     BOOST_CHECK_EQUAL(uint256::FromHex(ZeroL.ToString()).value(), uint256());
 

src/uint256.cpp

@@ -17,32 +17,6 @@ std::string base_blob<BITS>::GetHex() const
     return HexStr(m_data_rev);
 }
 
-template <unsigned int BITS>
-void base_blob<BITS>::SetHexDeprecated(const std::string_view str)
-{
-    std::fill(m_data.begin(), m_data.end(), 0);
-
-    const auto trimmed = util::RemovePrefixView(util::TrimStringView(str), "0x");
-
-    // Note: if we are passed a greater number of digits than would fit as bytes
-    // in m_data, we will be discarding the leftmost ones.
-    // str="12bc" in a WIDTH=1 m_data => m_data[] == "\0xbc", not "0x12".
-    size_t digits = 0;
-    for (const char c : trimmed) {
-        if (::HexDigit(c) == -1) break;
-        ++digits;
-    }
-    unsigned char* p1 = m_data.data();
-    unsigned char* pend = p1 + WIDTH;
-    while (digits > 0 && p1 < pend) {
-        *p1 = ::HexDigit(trimmed[--digits]);
-        if (digits > 0) {
-            *p1 |= ((unsigned char)::HexDigit(trimmed[--digits]) << 4);
-            p1++;
-        }
-    }
-}
-
 template <unsigned int BITS>
 std::string base_blob<BITS>::ToString() const
 {
@@ -52,12 +26,10 @@ std::string base_blob<BITS>::ToString() const
 // Explicit instantiations for base_blob<160>
 template std::string base_blob<160>::GetHex() const;
 template std::string base_blob<160>::ToString() const;
-template void base_blob<160>::SetHexDeprecated(std::string_view);
 
 // Explicit instantiations for base_blob<256>
 template std::string base_blob<256>::GetHex() const;
 template std::string base_blob<256>::ToString() const;
-template void base_blob<256>::SetHexDeprecated(std::string_view);
 
 const uint256 uint256::ZERO(0);
 const uint256 uint256::ONE(1);

src/uint256.h

@@ -69,11 +69,11 @@ class base_blob
 
     /** @name Hex representation
      *
-     * The hex representation used by GetHex(), ToString(), FromHex() and
-     * SetHexDeprecated() is unusual, since it shows bytes of the base_blob in
-     * reverse order. For example, a 4-byte blob {0x12, 0x34, 0x56, 0x78} is
-     * represented as "78563412" instead of the more typical "12345678"
-     * representation that would be shown in a hex editor or used by typical
+     * The hex representation used by GetHex(), ToString(), and FromHex()
+     * is unusual, since it shows bytes of the base_blob in reverse order.
+     * For example, a 4-byte blob {0x12, 0x34, 0x56, 0x78} is represented
+     * as "78563412" instead of the more typical "12345678" representation
+     * that would be shown in a hex editor or used by typical
      * byte-array / hex conversion functions like python's bytes.hex() and
      * bytes.fromhex().
      *
@@ -92,20 +92,6 @@ class base_blob
      *
      * @{*/
     std::string GetHex() const;
-    /** Unlike FromHex this accepts any invalid input, thus it is fragile and deprecated!
-     *
-     * - Hex numbers that don't specify enough bytes to fill the internal array
-     *   will be treated as setting the beginning of it, which corresponds to
-     *   the least significant bytes when converted to base_uint.
-     *
-     * - Hex numbers specifying too many bytes will have the numerically most
-     *   significant bytes (the beginning of the string) narrowed away.
-     *
-     * - An odd count of hex digits will result in the high bits of the leftmost
-     *   byte being zero.
-     *   "0x123" => {0x23, 0x1, 0x0, ..., 0x0}
-     */
-    void SetHexDeprecated(std::string_view str);
     std::string ToString() const;
     /**@}*/
 
@@ -158,7 +144,16 @@ std::optional<uintN_t> FromHex(std::string_view str)
 {
     if (uintN_t::size() * 2 != str.size() || !IsHex(str)) return std::nullopt;
     uintN_t rv;
-    rv.SetHexDeprecated(str);
+    unsigned char* p1 = rv.begin();
+    unsigned char* pend = rv.end();
+    size_t digits = str.size();
+    while (digits > 0 && p1 < pend) {
+        *p1 = ::HexDigit(str[--digits]);
+        if (digits > 0) {
+            *p1 |= ((unsigned char)::HexDigit(str[--digits]) << 4);
+            p1++;
+        }
+    }
     return rv;
 }
 /**