refactor: Make uint256S(const char*) consteval

transaction_identifier.h - Fixed dormant bug in TxidFromString() where the string_view length wasn't respected(!). uint256S() calls taking C-string literals are littered throughout the code and were executed at runtime to perform parsing unless a given optimizer was surprisingly efficient. While this may not be a hot spot, it's better hygiene in C++20 to store the parsed data blob directly in the binary, without any parsing at runtime. Added runtime overload taking std::string_view to avoid unnecessary creation of std::string just to get a null-terminator to feed into SetHex(). Verified that both GCC and Clang use the consteval overload of uint256S() for C-string literals by looking at produced binaries in objdump and confirming that calls to SetHex() do not remain in such a caller.
hodlinator · Jul 1, 2024 · 51c01a1 · 51c01a1
1 parent 0bd2bd1
commit 51c01a1
Show file tree

Hide file tree

Showing 6 changed files with 117 additions and 16 deletions.
diff --git a/src/test/txpackage_tests.cpp b/src/test/txpackage_tests.cpp
@@ -45,9 +45,9 @@ 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)
+consteval Wtxid WtxidFromString(const char* str)
 {
-    return Wtxid::FromUint256(uint256S(str.data()));
+    return Wtxid::FromUint256(uint256S(str));
 }
 
 BOOST_FIXTURE_TEST_CASE(package_hash_tests, TestChain100Setup)

diff --git a/src/test/uint256_tests.cpp b/src/test/uint256_tests.cpp
@@ -19,7 +19,7 @@ BOOST_AUTO_TEST_SUITE(uint256_tests)
 const unsigned char R1Array[] =
     "\x9c\x52\x4a\xdb\xcf\x56\x11\x12\x2b\x29\x12\x5e\x5d\x35\xd2\xd2"
     "\x22\x81\xaa\xb5\x33\xf0\x08\x32\xd5\x56\xb1\xf9\xea\xe5\x1d\x7d";
-const char R1ArrayHex[] = "7D1DE5EAF9B156D53208F033B5AA8122D2d2355d5e12292b121156cfdb4a529c";
+constexpr char R1ArrayHex[] = "7D1DE5EAF9B156D53208F033B5AA8122D2d2355d5e12292b121156cfdb4a529c";
 const uint256 R1L = uint256(std::vector<unsigned char>(R1Array,R1Array+32));
 const uint160 R1S = uint160(std::vector<unsigned char>(R1Array,R1Array+20));
 
@@ -299,13 +299,27 @@ BOOST_AUTO_TEST_CASE( operator_with_self )
 
 BOOST_AUTO_TEST_CASE(parse)
 {
+    // Forces compile time evaluation so we use the correct uint256S() overload.
+    auto compile_time = [] (auto input) consteval {
+        return input;
+    };
+
     {
         std::string s_12{"0000000000000000000000000000000000000000000000000000000000000012"};
-        BOOST_CHECK_EQUAL(uint256S("12\0").GetHex(), s_12);
-        BOOST_CHECK_EQUAL(uint256S(std::string{"12\0", 3}).GetHex(), s_12);
-        BOOST_CHECK_EQUAL(uint256S("0x12").GetHex(), s_12);
-        BOOST_CHECK_EQUAL(uint256S(" 0x12").GetHex(), s_12);
-        BOOST_CHECK_EQUAL(uint256S(" 12").GetHex(), s_12);
+        BOOST_CHECK_EQUAL(compile_time(uint256S(    "12\0")).GetHex(), s_12);
+        BOOST_CHECK_EQUAL(uint256S(std::string{     "12\0"}).GetHex(), s_12);
+        BOOST_CHECK_EQUAL(uint256S(std::string_view{"12\0"}).GetHex(), s_12);
+        BOOST_CHECK_EQUAL(uint256S(std::string{     "12\0", 3}).GetHex(), s_12);
+        BOOST_CHECK_EQUAL(uint256S(std::string_view{"12\0", 3}).GetHex(), s_12);
+        BOOST_CHECK_EQUAL(compile_time(uint256S(    "0x12")).GetHex(), s_12);
+        BOOST_CHECK_EQUAL(uint256S(std::string{     "0x12"}).GetHex(), s_12);
+        BOOST_CHECK_EQUAL(uint256S(std::string_view{"0x12"}).GetHex(), s_12);
+        BOOST_CHECK_EQUAL(compile_time(uint256S(    " 0x12")).GetHex(), s_12);
+        BOOST_CHECK_EQUAL(uint256S(std::string{     " 0x12"}).GetHex(), s_12);
+        BOOST_CHECK_EQUAL(uint256S(std::string_view{" 0x12"}).GetHex(), s_12);
+        BOOST_CHECK_EQUAL(compile_time(uint256S(    " 12")).GetHex(), s_12);
+        BOOST_CHECK_EQUAL(uint256S(std::string{     " 12"}).GetHex(), s_12);
+        BOOST_CHECK_EQUAL(uint256S(std::string_view{" 12"}).GetHex(), s_12);
     }
     {
         std::string s_1{uint256::ONE.GetHex()};
@@ -331,4 +345,16 @@ BOOST_AUTO_TEST_CASE( check_ONE )
     BOOST_CHECK_EQUAL(one, uint256::ONE);
 }
 
+BOOST_AUTO_TEST_CASE( check_uint256S_overloads )
+{
+    const uint256 runtime_string = uint256S(std::string{
+                 "4a5e1e4baab89f3a32518a88c31bc87f618f76673e2cc77ab2127b7afdeda33b"});
+    constexpr uint256 consteval_string = uint256S(
+             "\t0x4a5e1e4baab89f3a32518a88c31bc87f618f76673e2cc77ab2127b7afdeda33bgarbage suffix");
+    const uint256 runtime_string_view = uint256S(std::string_view{
+              " 0x4a5e1e4baab89f3a32518a88c31bc87f618f76673e2cc77ab2127b7afdeda33b yet more garbage"});
+    BOOST_CHECK_EQUAL(consteval_string, runtime_string);
+    BOOST_CHECK_EQUAL(consteval_string, runtime_string_view);
+}
+
 BOOST_AUTO_TEST_SUITE_END()
diff --git a/src/test/util/random.cpp b/src/test/util/random.cpp
@@ -18,7 +18,7 @@ static uint256 GetUintFromEnv(const std::string& env_name)
 {
     const char* num = std::getenv(env_name.c_str());
     if (!num) return {};
-    return uint256S(num);
+    return uint256S(std::string_view(num));
 }
 
 void Seed(FastRandomContext& ctx)

diff --git a/src/uint256.cpp b/src/uint256.cpp
@@ -5,8 +5,6 @@
 
 #include <uint256.h>
 
-#include <util/strencodings.h>
-
 template <unsigned int BITS>
 std::string base_blob<BITS>::GetHex() const
 {
@@ -51,6 +49,37 @@ void base_blob<BITS>::SetHex(const std::string& str)
     SetHex(str.c_str());
 }
 
+template <unsigned int BITS>
+void base_blob<BITS>::SetHex(std::string_view str)
+{
+    std::fill(m_data.begin(), m_data.end(), 0);
+
+    // skip leading spaces
+    while (!str.empty() && IsSpace(str.front()))
+        str.remove_prefix(1);
+
+    // skip 0x
+    if (str.starts_with("0x"))
+        str.remove_prefix(2);
+
+    // hex string to uint
+    size_t digits = 0;
+    for (const char c : str) {
+        if (::HexDigit(c) == -1)
+            break;
+        ++digits;
+    }
+    unsigned char* p1 = m_data.data();
+    unsigned char* pend = p1 + WIDTH;
+    while (digits > 0 && p1 < pend) {
+        *p1 = ::HexDigit(str[--digits]);
+        if (digits > 0) {
+            *p1 |= ((unsigned char)::HexDigit(str[--digits]) << 4);
+            p1++;
+        }
+    }
+}
+
 template <unsigned int BITS>
 std::string base_blob<BITS>::ToString() const
 {
@@ -62,12 +91,14 @@ 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);
diff --git a/src/uint256.h b/src/uint256.h
@@ -8,6 +8,7 @@
 
 #include <crypto/common.h>
 #include <span.h>
+#include <util/strencodings.h>
 
 #include <algorithm>
 #include <array>
@@ -60,6 +61,7 @@ class base_blob
     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(); }
@@ -116,10 +118,42 @@ class uint256 : public base_blob<256> {
  * This is a separate function because the constructor uint256(const char*) can result
  * in dangerously catching uint256(0).
  */
-inline uint256 uint256S(const char *str)
+consteval uint256 uint256S(const char *str)
 {
+    // Non lookup table version of HexDigit().
+    auto from_hex = [](const char c) -> int8_t {
+        if (c >= '0' && c <= '9')
+            return c - '0';
+        else if (c >= 'a' && c <= 'f')
+            return c - 'a' + 0xA;
+        else if (c >= 'A' && c <= 'F')
+            return c - 'A' + 0xA;
+        else
+            return -1;
+    };
+
+    // Skip leading spaces.
+    while (IsSpace(*str))
+        str += 1;
+    // Skip "0x" prefix.
+    if (str[0] == '0' && str[1] == 'x')
+        str += 2;
+
+    size_t digits = 0;
+    while (from_hex(str[digits]) != -1)
+        digits++;
+    // 64 = 32 bytes * 2 chars each. 32 bytes = 256 bits.
+    assert(digits <= 64);
+
     uint256 rv;
-    rv.SetHex(str);
+    uint8_t* it = rv.begin();
+    while (digits > 0) {
+        *it = from_hex(str[--digits]);
+        if (digits > 0) {
+            *it |= from_hex(str[--digits]) << 4;
+            ++it;
+        }
+    }
     return rv;
 }
 /* uint256 from std::string.
@@ -132,5 +166,15 @@ inline uint256 uint256S(const std::string& str)
     rv.SetHex(str);
     return rv;
 }
+/* uint256 from std::string_view.
+ * This is a separate function because the constructor uint256(std::string_view str) can result
+ * in dangerously catching uint256(0) via std::string_view(const char*).
+ */
+inline uint256 uint256S(std::string_view str)
+{
+    uint256 rv;
+    rv.SetHex(str);
+    return rv;
+}
 
 #endif // BITCOIN_UINT256_H
diff --git a/src/util/transaction_identifier.h b/src/util/transaction_identifier.h
@@ -13,7 +13,7 @@ class transaction_identifier
     uint256 m_wrapped;
 
     // Note: Use FromUint256 externally instead.
-    transaction_identifier(const uint256& wrapped) : m_wrapped{wrapped} {}
+    constexpr transaction_identifier(const uint256& wrapped) : m_wrapped{wrapped} {}
 
     // TODO: Comparisons with uint256 should be disallowed once we have
     // converted most of the code to using the new txid types.
@@ -37,7 +37,7 @@ class transaction_identifier
     bool operator<(const Other& other) const { return Compare(other) < 0; }
 
     const uint256& ToUint256() const LIFETIMEBOUND { return m_wrapped; }
-    static transaction_identifier FromUint256(const uint256& id) { return {id}; }
+    static constexpr transaction_identifier FromUint256(const uint256& id) { return {id}; }
 
     /** Wrapped `uint256` methods. */
     constexpr bool IsNull() const { return m_wrapped.IsNull(); }
@@ -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