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

18d65d2 test: use uint256::FromUserHex for RANDOM_CTX_SEED (stickies-v)
6819e5a node: use uint256::FromUserHex for -assumevalid parsing (stickies-v)
2e58fdb util: remove unused IsHexNumber (stickies-v)
8a44d7d node: use uint256::FromUserHex for -minimumchainwork parsing (stickies-v)
70e2c87 refactor: add uint256::FromUserHex helper (stickies-v)
85b7cbf test: unittest chainstatemanager_args (stickies-v)

Pull request description:

  Since fad2991, `uint256S` has been [deprecated](bitcoin/bitcoin@fad2991#diff-800776e2dda39116e889839f69409571a5d397de048a141da7e4003bc099e3e2R138) because it is less robust than the `base_blob::FromHex()` introduced in [the same PR](bitcoin/bitcoin#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 18d65d2
  l0rinc:
    ACK 18d65d2
  achow101:
    ACK 18d65d2
  ryanofsky:
    Code review ACK 18d65d2. 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

Author: achow101

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)) {
-            return util::Error{strprintf(Untranslated("Invalid non-hex (%s) minimum chain work value specified"), *value)};
+        if (auto min_work{uint256::FromUserHex(*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,32 +7,39 @@
 #include <logging.h>
 #include <random.h>
 #include <uint256.h>
+#include <util/check.h>
 
 #include <cstdlib>
-#include <string>
+#include <iostream>
 
 FastRandomContext g_insecure_rand_ctx;
 
 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
     // no longer truly random. It should be enough to get the seed once for the
     // 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 (num) return uint256S(num);
+        if (const char* num{std::getenv(RANDOM_CTX_SEED)}) {
+            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 @@ class uint160 : public base_blob<160> {
 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 @@ def run_test(self):
             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(2, extra_args=["-assumevalid=" + hex(block102.sha256)])
+        self.start_node(1, extra_args=["-assumevalid=" + block102.hash])
+        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])