Merge bitcoin/bitcoin#30436: fix: Make TxidFromString() respect string_view length

09ce350 fix: Make TxidFromString() respect string_view length (Hodlinator)
01e314c refactor: Change base_blob::SetHex() to take std::string_view (Hodlinator)
2f5577d test: uint256 - Garbage suffixes and zero padding (Hodlinator)
f11f816 refactor: Make uint256_tests no longer use deprecated BOOST_CHECK() (Hodlinator)
f0eeee2 test: Add test for TxidFromString() behavior (Ryan Ofsky)

Pull request description:

  ### Problem

  Prior to this, `TxidFromString()` was passing `string_view::data()` into `uint256S()` which meant it would only receive the a naked `char*` pointer and potentially scan past the `string_view::length()` until it found a null terminator (or some other non-hex character).

  Appears to have been a fully dormant bug as callers were either passing a string literal or `std::string` directly to `TxidFromFromString()`, meaning a null terminator always existed at `pointer[length()]`. Bug existed since original merge of `TxidFromString()`.

  ### Solution

  Make `uint256S()` (and `base_blob::SetHex()`) take and operate on `std::string_view` instead of `const char*` and have `TxidFromString()` pass that in.

  (PR was prompted by comment in bitcoin/bitcoin#30377 (comment) (referring to bitcoin/bitcoin#28922 (comment))).

ACKs for top commit:
  maflcko:
    re-ACK 09ce350 🕓
  paplorinc:
    ACK 09ce350
  ryanofsky:
    Code review ACK 09ce350. I think the current code changes are about as small as you could make to fix the bug without introducing a string copy, and the surrounding test improvements are all very nice and welcome.

Tree-SHA512: c2c10551785fb6688d1e2492ba42a8eee4c19abbe8461bb0774d56a70c23cd6b0718d2641632890bee880c06202dee148126447dd2264eaed4f5fee7e1bcb581

Author: ryanofsky

src/test/transaction_tests.cpp

@@ -1028,4 +1028,12 @@ 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()

src/test/txpackage_tests.cpp

@@ -47,7 +47,7 @@ inline CTransactionRef create_placeholder_tx(size_t num_inputs, size_t num_outpu
 // Create a Wtxid from a hex string
 inline Wtxid WtxidFromString(std::string_view str)
 {
-    return Wtxid::FromUint256(uint256S(str.data()));
+    return Wtxid::FromUint256(uint256S(str));
 }
 
 BOOST_FIXTURE_TEST_CASE(package_hash_tests, TestChain100Setup)

src/test/uint256_tests.cpp

@@ -79,6 +79,18 @@ const std::function<std::string(const char*)> G_TRANSLATION_FUN = nullptr;
 /** Random context to get unique temp data dirs. Separate from g_insecure_rand_ctx, which can be seeded from a const env var */
 static FastRandomContext g_insecure_rand_ctx_temp_path;
 
+std::ostream& operator<<(std::ostream& os, const arith_uint256& num)
+{
+    os << ArithToUint256(num).ToString();
+    return os;
+}
+
+std::ostream& operator<<(std::ostream& os, const uint160& num)
+{
+    os << num.ToString();
+    return os;
+}
+
 std::ostream& operator<<(std::ostream& os, const uint256& num)
 {
     os << num.ToString();

src/test/util/setup_common.cpp

@@ -24,6 +24,7 @@
 #include <type_traits>
 #include <vector>
 
+class arith_uint256;
 class CFeeRate;
 class Chainstate;
 class FastRandomContext;
@@ -236,7 +237,9 @@ std::unique_ptr<T> MakeNoLogFileContext(const ChainType chain_type = ChainType::
 
 CBlock getBlock13b8a();
 
-// define an implicit conversion here so that uint256 may be used directly in BOOST_CHECK_*
+// Make types usable in BOOST_CHECK_*
+std::ostream& operator<<(std::ostream& os, const arith_uint256& num);
+std::ostream& operator<<(std::ostream& os, const uint160& num);
 std::ostream& operator<<(std::ostream& os, const uint256& num);
 
 /**

src/test/util/setup_common.h

@@ -18,39 +18,31 @@ std::string base_blob<BITS>::GetHex() const
 }
 
 template <unsigned int BITS>
-void base_blob<BITS>::SetHex(const char* psz)
+void base_blob<BITS>::SetHex(const std::string_view str)
 {
     std::fill(m_data.begin(), m_data.end(), 0);
 
-    // skip leading spaces
-    while (IsSpace(*psz))
-        psz++;
+    const auto trimmed = util::RemovePrefixView(util::TrimStringView(str), "0x");
 
-    // skip 0x
-    if (psz[0] == '0' && ToLower(psz[1]) == 'x')
-        psz += 2;
-
-    // hex string to uint
+    // 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;
-    while (::HexDigit(psz[digits]) != -1)
-        digits++;
+    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(psz[--digits]);
+        *p1 = ::HexDigit(trimmed[--digits]);
         if (digits > 0) {
-            *p1 |= ((unsigned char)::HexDigit(psz[--digits]) << 4);
+            *p1 |= ((unsigned char)::HexDigit(trimmed[--digits]) << 4);
             p1++;
         }
     }
 }
 
-template <unsigned int BITS>
-void base_blob<BITS>::SetHex(const std::string& str)
-{
-    SetHex(str.c_str());
-}
-
 template <unsigned int BITS>
 std::string base_blob<BITS>::ToString() const
 {
@@ -60,14 +52,12 @@ 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>::SetHex(const char*);
-template void base_blob<160>::SetHex(const std::string&);
+template void base_blob<160>::SetHex(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>::SetHex(const char*);
-template void base_blob<256>::SetHex(const std::string&);
+template void base_blob<256>::SetHex(std::string_view);
 
 const uint256 uint256::ZERO(0);
 const uint256 uint256::ONE(1);

src/uint256.cpp

@@ -57,9 +57,9 @@ class base_blob
     friend constexpr bool operator!=(const base_blob& a, const base_blob& b) { return a.Compare(b) != 0; }
     friend constexpr bool operator<(const base_blob& a, const base_blob& b) { return a.Compare(b) < 0; }
 
+    // Hex string representations are little-endian.
     std::string GetHex() const;
-    void SetHex(const char* psz);
-    void SetHex(const std::string& str);
+    void SetHex(std::string_view str);
     std::string ToString() const;
 
     constexpr const unsigned char* data() const { return m_data.data(); }
@@ -112,21 +112,11 @@ class uint256 : public base_blob<256> {
     static const uint256 ONE;
 };
 
-/* uint256 from const char *.
- * This is a separate function because the constructor uint256(const char*) can result
- * in dangerously catching uint256(0).
+/* uint256 from std::string_view, treated as little-endian.
+ * This is not a uint256 constructor because of historical fears of uint256(0)
+ * resolving to a NULL string and crashing.
  */
-inline uint256 uint256S(const char *str)
-{
-    uint256 rv;
-    rv.SetHex(str);
-    return rv;
-}
-/* uint256 from std::string.
- * This is a separate function because the constructor uint256(const std::string &str) can result
- * in dangerously catching uint256(0) via std::string(const char*).
- */
-inline uint256 uint256S(const std::string& str)
+inline uint256 uint256S(std::string_view str)
 {
     uint256 rv;
     rv.SetHex(str);

src/uint256.h

@@ -68,7 +68,7 @@ using Wtxid = transaction_identifier<true>;
 
 inline Txid TxidFromString(std::string_view str)
 {
-    return Txid::FromUint256(uint256S(str.data()));
+    return Txid::FromUint256(uint256S(str));
 }
 
 #endif // BITCOIN_UTIL_TRANSACTION_IDENTIFIER_H