Merge bitcoin/bitcoin#30569: node: reduce unsafe uint256S usage

18d65d2772 test: use uint256::FromUserHex for RANDOM_CTX_SEED (stickies-v)
6819e5a329 node: use uint256::FromUserHex for -assumevalid parsing (stickies-v)
2e58fdb544 util: remove unused IsHexNumber (stickies-v)
8a44d7d3c1 node: use uint256::FromUserHex for -minimumchainwork parsing (stickies-v)
70e2c87737 refactor: add uint256::FromUserHex helper (stickies-v)
85b7cbfcbe test: unittest chainstatemanager_args (stickies-v)

Pull request description:

  Since fad2991ba0, `uint256S` has been [deprecated](fad2991ba0 (diff-800776e2dda39116e889839f69409571a5d397de048a141da7e4003bc099e3e2R138)) because it is less robust than the `base_blob::FromHex()` introduced in [the same PR](https://github.com/bitcoin/bitcoin/pull/30482). Specifically, it tries to recover from length-mismatches, recover from untrimmed whitespace, 0x-prefix and garbage at the end, instead of simply requiring exactly 64 hex-only characters. _(see also #30532)_

  This PR carves out the few `uint256S` callsites that may potentially prove a bit more controversial to change because they deal with user input and backwards incompatible behaviour change.

  The main behaviour change introduced in this PR is:
  - `-minimumchainwork` will raise an error when input is longer than 64 hex digits
  - `-assumevalid` will raise an error when input contains invalid hex characters, or when it is longer than 64 hex digits
  - test: the optional RANDOM_CTX_SEED env var will now cause tests to abort when it contains invalid hex characters, or when it is longer than 64 hex digits

  After this PR, the remaining work to remove `uint256S` completely is almost entirely mechanical and/or test related. I will open that PR once #30560 is merged because it builds on that.

ACKs for top commit:
  hodlinator:
    re-ACK 18d65d2772
  l0rinc:
    ACK 18d65d2772
  achow101:
    ACK 18d65d2772
  ryanofsky:
    Code review ACK 18d65d2772. Very nice change that cleans up the API, adds checking for invalid values, makes parsing of values more consistent, and adds test coverage.

Tree-SHA512: ec118ea3d56e1dfbc4c79acdbfc797f65c4d2107b0ca9577c848b4ab9b7cb8d05db9f3c7fe8441a09291aca41bf671572431f4eddc59be8fb53abbea76bbbf86

src/node/chainstatemanager_args.cpp

@@ -32,13 +32,20 @@ util::Result<void> ApplyArgsManOptions(const ArgsManager& args, ChainstateManage
     if (auto value{args.GetBoolArg("-checkpoints")}) opts.checkpoints_enabled = *value;
 
     if (auto value{args.GetArg("-minimumchainwork")}) {
-        if (!IsHexNumber(*value)) {
+        if (auto min_work{uint256::FromUserHex(*value)}) {
-            return util::Error{strprintf(Untranslated("Invalid non-hex (%s) minimum chain work value specified"), *value)};
+            opts.minimum_chain_work = UintToArith256(*min_work);
+        } else {
+            return util::Error{strprintf(Untranslated("Invalid minimum work specified (%s), must be up to %d hex digits"), *value, uint256::size() * 2)};
         }
-        opts.minimum_chain_work = UintToArith256(uint256S(*value));
     }
 
-    if (auto value{args.GetArg("-assumevalid")}) opts.assumed_valid_block = uint256S(*value);
+    if (auto value{args.GetArg("-assumevalid")}) {
+        if (auto block_hash{uint256::FromUserHex(*value)}) {
+            opts.assumed_valid_block = *block_hash;
+        } else {
+            return util::Error{strprintf(Untranslated("Invalid assumevalid block hash specified (%s), must be up to %d hex digits (or 0 to disable)"), *value, uint256::size() * 2)};
+        }
+    }
 
     if (auto value{args.GetIntArg("-maxtipage")}) opts.max_tip_age = std::chrono::seconds{*value};
 

src/test/fuzz/hex.cpp

@@ -27,11 +27,14 @@ FUZZ_TARGET(hex)
     if (IsHex(random_hex_string)) {
         assert(ToLower(random_hex_string) == hex_data);
     }
-    (void)IsHexNumber(random_hex_string);
     if (uint256::FromHex(random_hex_string)) {
         assert(random_hex_string.length() == 64);
         assert(Txid::FromHex(random_hex_string));
         assert(Wtxid::FromHex(random_hex_string));
+        assert(uint256::FromUserHex(random_hex_string));
+    }
+    if (const auto result{uint256::FromUserHex(random_hex_string)}) {
+        assert(uint256::FromHex(result->ToString()));
     }
     (void)uint256S(random_hex_string);
     try {

src/test/uint256_tests.cpp

@@ -393,6 +393,35 @@ BOOST_AUTO_TEST_CASE(from_hex)
     TestFromHex<Wtxid>();
 }
 
+BOOST_AUTO_TEST_CASE(from_user_hex)
+{
+    BOOST_CHECK_EQUAL(uint256::FromUserHex("").value(), uint256::ZERO);
+    BOOST_CHECK_EQUAL(uint256::FromUserHex("0x").value(), uint256::ZERO);
+    BOOST_CHECK_EQUAL(uint256::FromUserHex("0").value(), uint256::ZERO);
+    BOOST_CHECK_EQUAL(uint256::FromUserHex("00").value(), uint256::ZERO);
+    BOOST_CHECK_EQUAL(uint256::FromUserHex("1").value(), uint256::ONE);
+    BOOST_CHECK_EQUAL(uint256::FromUserHex("0x10").value(), uint256{0x10});
+    BOOST_CHECK_EQUAL(uint256::FromUserHex("10").value(), uint256{0x10});
+    BOOST_CHECK_EQUAL(uint256::FromUserHex("0xFf").value(), uint256{0xff});
+    BOOST_CHECK_EQUAL(uint256::FromUserHex("Ff").value(), uint256{0xff});
+    const std::string valid_hex_64{"0x0123456789abcdef0123456789abcdef0123456789ABDCEF0123456789ABCDEF"};
+    BOOST_REQUIRE_EQUAL(valid_hex_64.size(), 2 + 64); // 0x prefix and 64 hex digits
+    BOOST_CHECK_EQUAL(uint256::FromUserHex(valid_hex_64.substr(2)).value().ToString(), ToLower(valid_hex_64.substr(2)));
+    BOOST_CHECK_EQUAL(uint256::FromUserHex(valid_hex_64.substr(0)).value().ToString(), ToLower(valid_hex_64.substr(2)));
+
+    BOOST_CHECK(!uint256::FromUserHex("0x0 "));                       // no spaces at end,
+    BOOST_CHECK(!uint256::FromUserHex(" 0x0"));                       // or beginning,
+    BOOST_CHECK(!uint256::FromUserHex("0x 0"));                       // or middle,
+    BOOST_CHECK(!uint256::FromUserHex(" "));                          // etc.
+    BOOST_CHECK(!uint256::FromUserHex("0x0ga"));                      // invalid character
+    BOOST_CHECK(!uint256::FromUserHex("x0"));                         // broken prefix
+    BOOST_CHECK(!uint256::FromUserHex("0x0x00"));                     // two prefixes not allowed
+    BOOST_CHECK(!uint256::FromUserHex(valid_hex_64.substr(2) + "0")); // 1 hex digit too many
+    BOOST_CHECK(!uint256::FromUserHex(valid_hex_64 + "a"));           // 1 hex digit too many
+    BOOST_CHECK(!uint256::FromUserHex(valid_hex_64 + " "));           // whitespace after max length
+    BOOST_CHECK(!uint256::FromUserHex(valid_hex_64 + "z"));           // invalid character after max length
+}
+
 BOOST_AUTO_TEST_CASE( check_ONE )
 {
     uint256 one = uint256{"0000000000000000000000000000000000000000000000000000000000000001"};

src/test/util/random.cpp

@@ -7,9 +7,10 @@
 #include <logging.h>
 #include <random.h>
 #include <uint256.h>
+#include <util/check.h>
 
 #include <cstdlib>
-#include <string>
+#include <iostream>
 
 FastRandomContext g_insecure_rand_ctx;
 
@@ -17,7 +18,7 @@ extern void MakeRandDeterministicDANGEROUS(const uint256& seed) noexcept;
 
 void SeedRandomForTest(SeedRand seedtype)
 {
-    static const std::string RANDOM_CTX_SEED{"RANDOM_CTX_SEED"};
+    constexpr auto RANDOM_CTX_SEED{"RANDOM_CTX_SEED"};
 
     // Do this once, on the first call, regardless of seedtype, because once
     // MakeRandDeterministicDANGEROUS is called, the output of GetRandHash is
@@ -25,14 +26,20 @@ void SeedRandomForTest(SeedRand seedtype)
     // process.
     static const uint256 ctx_seed = []() {
         // If RANDOM_CTX_SEED is set, use that as seed.
-        const char* num = std::getenv(RANDOM_CTX_SEED.c_str());
+        if (const char* num{std::getenv(RANDOM_CTX_SEED)}) {
-        if (num) return uint256S(num);
+            if (auto num_parsed{uint256::FromUserHex(num)}) {
+                return *num_parsed;
+            } else {
+                std::cerr << RANDOM_CTX_SEED << " must consist of up to " << uint256::size() * 2 << " hex digits (\"0x\" prefix allowed), it was set to: '" << num << "'.\n";
+                std::abort();
+            }
+        }
         // Otherwise use a (truly) random value.
         return GetRandHash();
     }();
 
     const uint256& seed{seedtype == SeedRand::SEED ? ctx_seed : uint256::ZERO};
-    LogPrintf("%s: Setting random seed for current tests to %s=%s\n", __func__, RANDOM_CTX_SEED, seed.GetHex());
+    LogInfo("Setting random seed for current tests to %s=%s\n", RANDOM_CTX_SEED, seed.GetHex());
     MakeRandDeterministicDANGEROUS(seed);
     g_insecure_rand_ctx.Reseed(GetRandHash());
 }

src/test/util_tests.cpp

@@ -432,31 +432,6 @@ BOOST_AUTO_TEST_CASE(util_IsHex)
     BOOST_CHECK(!IsHex("0x0000"));
 }
 
-BOOST_AUTO_TEST_CASE(util_IsHexNumber)
-{
-    BOOST_CHECK(IsHexNumber("0x0"));
-    BOOST_CHECK(IsHexNumber("0"));
-    BOOST_CHECK(IsHexNumber("0x10"));
-    BOOST_CHECK(IsHexNumber("10"));
-    BOOST_CHECK(IsHexNumber("0xff"));
-    BOOST_CHECK(IsHexNumber("ff"));
-    BOOST_CHECK(IsHexNumber("0xFfa"));
-    BOOST_CHECK(IsHexNumber("Ffa"));
-    BOOST_CHECK(IsHexNumber("0x00112233445566778899aabbccddeeffAABBCCDDEEFF"));
-    BOOST_CHECK(IsHexNumber("00112233445566778899aabbccddeeffAABBCCDDEEFF"));
-
-    BOOST_CHECK(!IsHexNumber(""));   // empty string not allowed
-    BOOST_CHECK(!IsHexNumber("0x")); // empty string after prefix not allowed
-    BOOST_CHECK(!IsHexNumber("0x0 ")); // no spaces at end,
-    BOOST_CHECK(!IsHexNumber(" 0x0")); // or beginning,
-    BOOST_CHECK(!IsHexNumber("0x 0")); // or middle,
-    BOOST_CHECK(!IsHexNumber(" "));    // etc.
-    BOOST_CHECK(!IsHexNumber("0x0ga")); // invalid character
-    BOOST_CHECK(!IsHexNumber("x0"));    // broken prefix
-    BOOST_CHECK(!IsHexNumber("0x0x00")); // two prefixes not allowed
-
-}
-
 BOOST_AUTO_TEST_CASE(util_seed_insecure_rand)
 {
     SeedRandomForTest(SeedRand::ZEROS);

src/test/validation_chainstatemanager_tests.cpp

@@ -5,6 +5,7 @@
 #include <chainparams.h>
 #include <consensus/validation.h>
 #include <kernel/disconnected_transactions.h>
+#include <node/chainstatemanager_args.h>
 #include <node/kernel_notifications.h>
 #include <node/utxo_snapshot.h>
 #include <random.h>
@@ -16,6 +17,8 @@
 #include <test/util/setup_common.h>
 #include <test/util/validation.h>
 #include <uint256.h>
+#include <util/result.h>
+#include <util/vector.h>
 #include <validation.h>
 #include <validationinterface.h>
 
@@ -769,4 +772,59 @@ BOOST_FIXTURE_TEST_CASE(chainstatemanager_snapshot_completion_hash_mismatch, Sna
     }
 }
 
+/** Helper function to parse args into args_man and return the result of applying them to opts */
+template <typename Options>
+util::Result<Options> SetOptsFromArgs(ArgsManager& args_man, Options opts,
+                                      const std::vector<const char*>& args)
+{
+    const auto argv{Cat({"ignore"}, args)};
+    std::string error{};
+    if (!args_man.ParseParameters(argv.size(), argv.data(), error)) {
+        return util::Error{Untranslated("ParseParameters failed with error: " + error)};
+    }
+    const auto result{node::ApplyArgsManOptions(args_man, opts)};
+    if (!result) return util::Error{util::ErrorString(result)};
+    return opts;
+}
+
+BOOST_FIXTURE_TEST_CASE(chainstatemanager_args, BasicTestingSetup)
+{
+    //! Try to apply the provided args to a ChainstateManager::Options
+    auto get_opts = [&](const std::vector<const char*>& args) {
+        static kernel::Notifications notifications{};
+        static const ChainstateManager::Options options{
+            .chainparams = ::Params(),
+            .datadir = {},
+            .notifications = notifications};
+        return SetOptsFromArgs(*this->m_node.args, options, args);
+    };
+    //! Like get_opts, but requires the provided args to be valid and unwraps the result
+    auto get_valid_opts = [&](const std::vector<const char*>& args) {
+        const auto result{get_opts(args)};
+        BOOST_REQUIRE_MESSAGE(result, util::ErrorString(result).original);
+        return *result;
+    };
+
+    // test -assumevalid
+    BOOST_CHECK(!get_valid_opts({}).assumed_valid_block.has_value());
+    BOOST_CHECK(get_valid_opts({"-assumevalid="}).assumed_valid_block.value().IsNull());
+    BOOST_CHECK(get_valid_opts({"-assumevalid=0"}).assumed_valid_block.value().IsNull());
+    BOOST_CHECK(get_valid_opts({"-noassumevalid"}).assumed_valid_block.value().IsNull());
+    BOOST_CHECK_EQUAL(get_valid_opts({"-assumevalid=0x1234"}).assumed_valid_block.value().ToString(), std::string(60, '0') + "1234");
+    const std::string cmd{"-assumevalid=0123456789abcdef0123456789abcdef0123456789abcdef0123456789abcdef"};
+    BOOST_CHECK_EQUAL(get_valid_opts({cmd.c_str()}).assumed_valid_block.value().ToString(), cmd.substr(13, cmd.size()));
+
+    BOOST_CHECK(!get_opts({"-assumevalid=xyz"}));                                                               // invalid hex characters
+    BOOST_CHECK(!get_opts({"-assumevalid=01234567890123456789012345678901234567890123456789012345678901234"})); // > 64 hex chars
+
+    // test -minimumchainwork
+    BOOST_CHECK(!get_valid_opts({}).minimum_chain_work.has_value());
+    BOOST_CHECK_EQUAL(get_valid_opts({"-minimumchainwork=0"}).minimum_chain_work.value().GetCompact(), 0U);
+    BOOST_CHECK_EQUAL(get_valid_opts({"-nominimumchainwork"}).minimum_chain_work.value().GetCompact(), 0U);
+    BOOST_CHECK_EQUAL(get_valid_opts({"-minimumchainwork=0x1234"}).minimum_chain_work.value().GetCompact(), 0x02123400U);
+
+    BOOST_CHECK(!get_opts({"-minimumchainwork=xyz"}));                                                               // invalid hex characters
+    BOOST_CHECK(!get_opts({"-minimumchainwork=01234567890123456789012345678901234567890123456789012345678901234"})); // > 64 hex chars
+}
+
 BOOST_AUTO_TEST_SUITE_END()

src/uint256.h

@@ -9,6 +9,7 @@
 #include <crypto/common.h>
 #include <span.h>
 #include <util/strencodings.h>
+#include <util/string.h>
 
 #include <algorithm>
 #include <array>
@@ -17,6 +18,7 @@
 #include <cstring>
 #include <optional>
 #include <string>
+#include <string_view>
 
 /** Template base class for fixed-sized opaque blobs. */
 template<unsigned int BITS>
@@ -157,6 +159,25 @@ std::optional<uintN_t> FromHex(std::string_view str)
     rv.SetHexDeprecated(str);
     return rv;
 }
+/**
+ * @brief Like FromHex(std::string_view str), but allows an "0x" prefix
+ *        and pads the input with leading zeroes if it is shorter than
+ *        the expected length of uintN_t::size()*2.
+ *
+ *        Designed to be used when dealing with user input.
+ */
+template <class uintN_t>
+std::optional<uintN_t> FromUserHex(std::string_view input)
+{
+    input = util::RemovePrefixView(input, "0x");
+    constexpr auto expected_size{uintN_t::size() * 2};
+    if (input.size() < expected_size) {
+        auto padded = std::string(expected_size, '0');
+        std::copy(input.begin(), input.end(), padded.begin() + expected_size - input.size());
+        return FromHex<uintN_t>(padded);
+    }
+    return FromHex<uintN_t>(input);
+}
 } // namespace detail
 
 /** 160-bit opaque blob.
@@ -178,6 +199,7 @@ public:
 class uint256 : public base_blob<256> {
 public:
     static std::optional<uint256> FromHex(std::string_view str) { return detail::FromHex<uint256>(str); }
+    static std::optional<uint256> FromUserHex(std::string_view str) { return detail::FromUserHex<uint256>(str); }
     constexpr uint256() = default;
     consteval explicit uint256(std::string_view hex_str) : base_blob<256>(hex_str) {}
     constexpr explicit uint256(uint8_t v) : base_blob<256>(v) {}

src/util/strencodings.cpp

@@ -46,16 +46,6 @@ bool IsHex(std::string_view str)
     return (str.size() > 0) && (str.size()%2 == 0);
 }
 
-bool IsHexNumber(std::string_view str)
-{
-    if (str.substr(0, 2) == "0x") str.remove_prefix(2);
-    for (char c : str) {
-        if (HexDigit(c) < 0) return false;
-    }
-    // Return false for empty string or "0x".
-    return str.size() > 0;
-}
-
 template <typename Byte>
 std::optional<std::vector<Byte>> TryParseHex(std::string_view str)
 {

src/util/strencodings.h

@@ -70,10 +70,6 @@ std::vector<Byte> ParseHex(std::string_view hex_str)
 /* Returns true if each character in str is a hex character, and has an even
  * number of hex digits.*/
 bool IsHex(std::string_view str);
-/**
-* Return true if the string is a hex number, optionally prefixed with "0x"
-*/
-bool IsHexNumber(std::string_view str);
 std::optional<std::vector<unsigned char>> DecodeBase64(std::string_view str);
 std::string EncodeBase64(Span<const unsigned char> input);
 inline std::string EncodeBase64(Span<const std::byte> input) { return EncodeBase64(MakeUCharSpan(input)); }

test/functional/feature_assumevalid.py

@@ -139,8 +139,8 @@ class AssumeValidTest(BitcoinTestFramework):
             height += 1
 
         # Start node1 and node2 with assumevalid so they accept a block with a bad signature.
-        self.start_node(1, extra_args=["-assumevalid=" + hex(block102.sha256)])
+        self.start_node(1, extra_args=["-assumevalid=" + block102.hash])
-        self.start_node(2, extra_args=["-assumevalid=" + hex(block102.sha256)])
+        self.start_node(2, extra_args=["-assumevalid=" + block102.hash])
 
         p2p0 = self.nodes[0].add_p2p_connection(BaseNode())
         p2p0.send_header_for_blocks(self.blocks[0:2000])

test/functional/feature_minchainwork.py

@@ -110,7 +110,7 @@ class MinimumChainWorkTest(BitcoinTestFramework):
         self.stop_node(0)
         self.nodes[0].assert_start_raises_init_error(
             ["-minimumchainwork=test"],
-            expected_msg='Error: Invalid non-hex (test) minimum chain work value specified',
+            expected_msg='Error: Invalid minimum work specified (test), must be up to 64 hex digits',
         )