node: reduce unsafe uint256S usage #30569

I wonder if it makes sense to add an option to FromHex to allow parsing of “hex numbers”, which would remove the prefix and only reject too large or junk input?

In any case, you can remove IsHexNumber?

Thanks for the suggestion. I’ve been exploring different options here. Adding a bool is_num optional param to uint256::FromHex() is an option, but felt a bit awkward because it’d be a uint256-specific parameter (doesn’t make sense for transaction_identifier).

Since we only need this for user input validation, I’ve now taken a different approach where I’ve repurposed IsHexNumber() -> TrySanitizeHexNumber(). I think this minimizes churn (e.g. the test suite can largely be kept as is), isolates user input validation from code that’s mostly used internally and benefits from being strict and simple.

I presume this will break, when someone sets -noassumevalid?

0Error: Invalid assumevalid block hash specified (0), must be 64 character hex

Ah thanks, didn’t think of that. Will add a “0” check:

 0diff --git a/src/node/chainstatemanager_args.cpp b/src/node/chainstatemanager_args.cpp
 1index 8085aa65dc..60cee3dd78 100644
 2--- a/src/node/chainstatemanager_args.cpp
 3+++ b/src/node/chainstatemanager_args.cpp
 4@@ -31,7 +31,7 @@ util::Result<void> ApplyArgsManOptions(const ArgsManager& args, ChainstateManage
 5 
 6     if (auto value{args.GetBoolArg("-checkpoints")}) opts.checkpoints_enabled = *value;
 7 
 8-    if (auto value{args.GetArg("-minimumchainwork")}) {
 9+    if (auto value{args.GetArg("-minimumchainwork")}; value && *value != "0") {
10         if (auto min_work{uint256::FromHex(util::RemovePrefixView(*value, "0x"))}) {
11             opts.minimum_chain_work = UintToArith256(*min_work);
12         } else {
13@@ -39,7 +39,7 @@ util::Result<void> ApplyArgsManOptions(const ArgsManager& args, ChainstateManage
14         }
15     }
16 
17-    if (auto value{args.GetArg("-assumevalid")}) {
18+    if (auto value{args.GetArg("-assumevalid")}; value && *value != "0") {
19         if (auto block_hash{uint256::FromHex(util::RemovePrefixView(*value, "0x"))}) {
20             opts.assumed_valid_block = *block_hash;
21         } else {

To me asserts are for enforcing logical consistency of the code, NOT for handling invalid values of environment variables.

A slight convention seems to be using abort() in these circumstances.

0            if (auto num_parsed{uint256::FromHex(num)}) {
1                return *num_parsed;
2            } else {
3                std::cerr << RANDOM_CTX_SEED << " must be a " << uint256::size() * 2 << " char hex string without '0x'-prefix, was set to: '" << num << "'." << std::endl;
4                std::abort();
5            }

(I also tried BOOST_TEST_FAIL but ran into linker errors for the test_util library so would rather avoid adding a dependency).

Gives output:

0$ RANDOM_CTX_SEED=123 src/test/test_bitcoin
1Running 621 test cases...
2RANDOM_CTX_SEED must be a 64 char hex string without '0x'-prefix, was set to: '123'.
3unknown location(0): fatal error: in "addrman_tests/addrman_simple": signal: SIGABRT (application abort requested)
4test/addrman_tests.cpp(64): last checkpoint: "addrman_simple" fixture ctor
5test_bitcoin: common/args.cpp:576: void ArgsManager::AddArg(const std::string&, const std::string&, unsigned int, const OptionsCategory&): Assertion `ret.second' failed.
6unknown location(0): fatal error: in "addrman_tests/addrman_ports": signal: SIGABRT (application abort requested)
7test/addrman_tests.cpp(108): last checkpoint: "addrman_ports" fixture ctor
8...

To me asserts are for enforcing logical consistency of the code, NOT for handling invalid values of environment variables.

Correct. This rule must be applied to real production code. However, in the tests, personally I like to use it for brevity. No objection to using something else.

However, in the tests, personally I like to use it for brevity.

I agree. The tests fail gracefully, and the error message is helpful, including the location of the assertion which documents that we’re expecting a 64 char hex string. Going to leave as is.

0Running 1 test case...
1test/util/random.cpp:29 operator(): Assertion `uint256::FromHex(num)' failed.
2unknown location:0: fatal error: in "timeoffsets_tests/timeoffsets_warning": signal: SIGABRT (application abort requested)
3test/timeoffsets_tests.cpp:62: last checkpoint: "timeoffsets_warning" fixture ctor
4
5*** 1 failure is detected in the test module "Bitcoin Core Test Suite"

The inclination to use Assert for any fatal error is understandable, but as I said in my book they are for catching logic errors - not for validating user environment. My non-Bitcoin experience tells me that policies around what should be asserts require upkeep, so I took the time to provide an alternate version.

Was reading https://bitcoincore.academy/bin/onboarding-to-bitcoin-core.html today and came across

We take extra care during the encryption phase to either complete atomically or fail. This includes database writes where we don’t want to write half and crash, for example. Therefore we will throw an assertion if the write fails.

Not sure if the actual code really uses assertions to stop the process if the database reports write failure. Maybe Bitcoin Core has a looser policy than I’m used to, and at least @maflcko is okay with using it in tests.

From a purely functional angle, the std::cerr output provides more immediate and actionable information than the assert. test/util/random.cpp:29 operator(): Assertion `uint256::FromHex(num)' failed. vs RANDOM_CTX_SEED must be a 64 char hex string without '0x'-prefix, was set to: '123'.

I’m happy you incorporated some of my other suggestions but I pointed out this as my major gripe with the PR.

Staying true to my principles and giving this an Approach A-C-K generates cognitive dissonance. But maybe I could be bribed by promises of likely support for a future PR from me cleaning up incorrect Assert usage in tests in general.

Therefore we will throw an assertion if the write fails.

Not sure if the actual code really uses assertions to stop the process if the database reports write failure. Maybe Bitcoin Core has a looser policy than I’m used to, and at least @maflcko is okay with using it in tests.

I haven’t checked the code, but “throw assertion” could mean “throw assertion error”, which seems fine to use in this context the quote was taken from.

Not to be confused with other context, where aborting the program on (some) hardware corruption is actually intentional, and I think also fine. When your hardware runs into a corrupt state, aborting seems preferable, when otherwise execution can’t continue meaningfully anyway. When it happens, the user will usually have to wipe their state or replace their hardware anyway. It is also not external user-input that is passed to the assert, in this case (at least it can be assumed to).

But maybe I could be bribed by promises of likely support for a future PR from me cleaning up incorrect Assert usage in tests in general.

Heh, I am generally happy to review any test-related pull requests.

Using assertions to prevent the process from continuing in an unsupported state is certainly better than nothing at all.

Once the risk of unsupported state due to hardware failure or environment variable misconfiguration becomes known, one should implement non-assert error handling code, as the unsupported state is now expected in some circumstances.

I concede that in this case the environment variable is usually supposed to be set by Python functional tests, so there is a weak case to be made that it is a logic error. But if a problematic seed value is found and incorrectly copied like in my “123” example above, skipping over the Python layer, it no longer is a code-level logic error.

in this case the environment variable is usually supposed to be set by Python functional tests, so there is a weak case to be made that it is a logic error.

Going back to this case, I don’t think RANDOM_CTX_SEED is set by python at all. It should only be set by a dev (or user), or not at all.

At least when I locally call git grep RANDOM_CTX_SEED, I don’t see it.

Thanks for calling out my assumption that test_runner.py --randomseed=X could trickle down into unit tests.

Makes my case for it not being a logic error slightly stronger.

since we’ve checked > before, maybe it would be more self-explanatory to check whether we’re equal or not, since it’s a stronger guarantee:

0    if (input.size() != result_size) {

ok, the number part can make sense, I’m still not sure about the Try part - seems like https://en.wikipedia.org/wiki/Hungarian_notation, which (as a Hungarian) I’m very much against :p

Return types are not immediately visible on in the callsite

It’s part of the method’s signature, I don’t see how repeating it in the method’s name helps. The parameters also aren’t visible on the call site, yet we’re not encoding them in the method’s name.

If you insist, I won’t block you on this of course, but want to make sure that my arguments against it are clear.

Ugh, you’re right, thanks a lot for catching this. This approach would’ve silently ignored -noassumevalid and -assumevalid=0 by using the defaultAssumeValid value instead. I think it’s not great that this bug didn’t cause any tests to fail. feature_assumevalid.py doesn’t catch this because defaultAssumeValid on regtest is null (and I don’t think there’s a way around this), and we don’t seem to have any unittesting on applying args to options.

I’ve added 1c909b2ccfaee902637b28f890db8dbe168ea926 as a first commit (fails on b38a259d1e2749876d4e3a15f218b4f257049320) to add unittesting on -assumevalid and -minimumchainwork.

I couldn’t see RANDOM_CTX_SEED be documented anywhere as having to be a 64 char hex string, so I thought trying to be lenient would be good for backwards compatibility, but I don’t have a view on how important that is so happy to go with your simplification. Adopted in latest force push.

Thanks.

For historic context, it was added in fae43a97ca947cd0802392e9bb86d9d0572c0fba, where it is only printed. Thus, the only source should be to copy-paste from a printed value.

If not, and someone were to provide the seed from “outside”, I think it is reasonable and clearer to require them to provide it in the exact same hex format. Otherwise, it seems fragile.

Thanks for covering these scenarios!

I checked to see how this fails for the previous error:

unknown location:0: fatal error: in “validation_chainstatemanager_tests/chainstatemanager_args”: std::bad_optional_access: bad_optional_access

i.e. it doesn’t get to the IsNull check since it can’t call value() on a null optional, right?

I think we can work around that by comparing the dereferenced optional instead:

0    BOOST_CHECK_EQUAL(*set_opts({"-assumevalid=0"}).assumed_valid_block, uint256::ZERO);

which gives this error for the previously invalid setup:

src/test/validation_chainstatemanager_tests.cpp:801: error: in “validation_chainstatemanager_tests/chainstatemanager_args”: check *set_opts({"-assumevalid=0"}).assumed_valid_block == uint256::ZERO has failed [000000016bcc2e20000000016bcc2e20000000016bcc29a03b66800104140f00 != 0000000000000000000000000000000000000000000000000000000000000000]

Which I find a lot more revealing.

has failed [000000016bcc2e20000000016bcc2e20000000016bcc29a03b66800104140f00 != 0000000000000000000000000000000000000000000000000000000000000000]

Which I find a lot more revealing.

Can you run this in valgrind? Dereferencing a nullopt is UB (undefined behavior), which must be avoided in production (obviously), but also in tests.

If you want to learn which methods expose undefined behavior, I find [https://en.cppreference.com/w/cpp/utility/optional/operator*] a good resource.

Can you run this in valgrind?

Thanks for the context @maflcko! Valgrind currently isn’t really compatible with ARM-based Macs.

Dereferencing a nullopt is UB

And we can’t use BOOST_CHECK_EQUAL(set_opts({"-assumevalid=0"}).assumed_valid_block, std::make_optional(uint256::ZERO)) since there’s no standard way to display optionals, right?

Ok, in that case just consider the test fix validated, it failed before the fix, passes now.

Can you run this in valgrind?

Thanks for the context @maflcko! Valgrind currently isn’t really compatible with ARM-based Macs.

An alternative on macos may be Asan (or possibly Ubsan), but they require re-compiling the binary.

And we can’t use BOOST_CHECK_EQUAL(set_opts({"-assumevalid=0"}).assumed_valid_block, std::make_optional(uint256::ZERO)) since there’s no standard way to display optionals, right?

Would this fit your definition of displaying optionals?

https://github.com/bitcoin/bitcoin/pull/16545/files#diff-d4a2fb26adedc27f16bd3778424fa94c473342a695b228220a1810119028be5bR48-R64

nit: my first reaction was here - “I wonder if there’s an off-by-one error here”. If you think it makes sense, consider this alternative:

0    if (result_size < 0) result_size = std::numeric_limits<int>::max(); // negative result_size disables the size check

I don’t particularly like the alternative, but the developer notes state:

0- Use a named cast or functional cast, not a C-Style cast. When casting
1    between integer types, use functional casts such as `int(x)` or `int{x}`
2    instead of `(int) x`. When casting between more complex types, use `static_cast`.

That might not work, but extracting might:

0    auto final_size = (result_size < 0) ? input.size() : static_cast<size_t>(result_size);

Do I understand it correctly that the point of this kind of testing (i.e. just calling the method with random values without validating the result) is to make sure we don’t have memory problems, don’t throw unexpected exceptions, etc?

Since we seem to have many hex related methods, would it make sense to compare their outputs, to make sure we at least have internal consistency?

0    auto sanitized_hex = TrySanitizeHexNumber(random_hex_string, result_size);
1    if (sanitized_hex) {
2        assert(IsHex(*sanitized_hex));
3        assert(result_size < 0 || sanitized_hex->length() == static_cast<size_t>(result_size));
4    }

is to make sure we don’t have memory problems, don’t throw unexpected exceptions, etc?

That’s my understanding too.

would it make sense to compare their outputs, to make sure we at least have internal consistency?

Sounds reasonable, but I think I’ll leave that for another PR.

could we use IsHex here instead, i.e. merge it with previous line:

0    if (input.empty() || ((int)input.size() > result_size) || !IsHex(input)) return std::nullopt;

though this contains an extra evennes requirement which we may not want here.

though this contains an extra evennes requirement which we may not want here.

Exactly

nit: if you think it’s more readable, here’s an alternative which avoids reparsing and duplicating uint256::size() * 2 (but assigns twice, so you may not like it):

0    if (auto value{args.GetArg("-assumevalid")}) {
1        if (*value == "0") { // handle -noassumevalid
2            opts.assumed_valid_block = uint256{};
3        } else if (auto block_hash{uint256::FromHex(*value)}) {
4            opts.assumed_valid_block = *block_hash;
5        } else {
6            return util::Error{strprintf(Untranslated("Invalid assumevalid block hash specified (%s), must be %d character hex (or 0 to disable)"), *value, uint256::size() * 2)};
7        }
8    }

Don’t think this is worth spending too much time on but given that the only non-test call is quite likely to require padding, I’ve updated it to:

 0diff --git a/src/util/strencodings.cpp b/src/util/strencodings.cpp
 1index 4e0317cd0e..be946af269 100644
 2--- a/src/util/strencodings.cpp
 3+++ b/src/util/strencodings.cpp
 4@@ -8,6 +8,7 @@
 5 #include <crypto/hex_base.h>
 6 #include <span.h>
 7 
 8+#include <algorithm>
 9 #include <array>
10 #include <cassert>
11 #include <cstring>
12@@ -54,10 +55,8 @@ std::optional<std::string> TrySanitizeHexNumber(std::string_view input, int resu
13     for (char c : input) {
14         if (HexDigit(c) < 0) return std::nullopt;
15     }
16-    std::string result{input};
17-    if (auto padding{final_size - input.size()}; padding > 0) {
18-        result = std::string(padding, '0') + result;
19-    }
20+    std::string result(final_size, '0');
21+    std::copy(input.begin(), input.end(), result.begin() + (final_size - input.size()));
22     return result;
23 }
24 

nit:

0    BOOST_REQUIRE(args_man.ParseParameters(argv.size(), argv.data(), error));
1    BOOST_REQUIRE(error.empty());

Given that:

• this is only used in a single place in prod (which accept std::string_view as well and has an std::string input)
• we’re always copying the result before returning
• the overwhelming majority of inputs will probably have the correct size already, i.e. don’t need change.
• the performance of the method seems to be important

Could we either return an std::optional<std::string_view> instead or take an std::string& input to be able to return it?

i.e.

 0std::optional<std::string> TrySanitizeHexNumber(std::string input, int result_size)
 1{
 2    input = util::RemovePrefixView(input, "0x");
 3    const auto final_size{(result_size < 0) ? input.size() : static_cast<size_t>(result_size)};
 4    if (input.empty() || (input.size() > final_size)) [[unlikely]] return std::nullopt;
 5    for (char c : input) {
 6        if (HexDigit(c) < 0) [[unlikely]] return std::nullopt;
 7    }
 8    if (input.size() == final_size) [[likely]] return input;
 9
10    std::string result(final_size, '0');
11    std::copy(input.begin(), input.end(), result.begin() + (final_size - input.size()));
12    return result;
13}

I agree taking input as std::string works better with the return type.

Instead of introducing a 2nd string at the end, one could simply:

0    input.insert(0, final_size - input.size(), '0');
1    return input;

input.insert(0, final_size - input.size(), ‘0’); return input;

my functional programming past screams at the though of mutating parameters and also returning them

the performance of the method seems to be important

How so? There is one callsite, which can be called at most once, to parse one optional debug-only option.

Could we either return an std::optionalstd::string_view

How would that work? std::string_view is a non-owning view.

we’re always copying the result before returning

In 802374b4355bd1dec7a88bba6287c55f935699fe, we’re always allocating exactly once (assuming RVO). In your example, we’d be allocating at least once, and twice if we need padding. That doesn’t look like an improvement to me.

the overwhelming majority of inputs will probably have the correct size already, i.e. don’t need change.

Given that it relies on user input, that seems hard to substantiate. In the functional tests, for example, we’re using quite a bit of non-64 char hex input, which is reasonable given that we’re doing all this work to accept number input.

(Edit: this was regarding the suggestion in #30569 (review)).

0input = util::RemovePrefixView(input, "0x");

This line gives me the heebie-jeebies if input is std::string. It might be okay as we are assigning a shorter string, but worst case, the assignment operator could free it’s prior heap buffer and allocate a shorter buffer, before attempting to copy from the string_view which now might reference the deallocated buffer. Using RemovePrefix() should alleviate the problem.

Will let you decide, there are multiple ways to do this, each would be slightly better in a different scenario. Whichever produces the cleanest code, choose that :)

edit: the example I gave didn’t actually use the std::string& input I’ve suggested

nit: An Assume() or Assert() around the FromHex() result before the *-deref might be better here, even though it should work :tm:.

Maybe linking TrySanitizeHexNumber and FromHex together in the fuzz-test would be good for enforcing that the former only allows through something acceptable to the latter (although result_size must also be correct).

Edit: An alternative to Assert/Assume would be to call FromHex(..).value() and have a std::bad_optional_access be thrown should it ever not have a value.

(In the follow-up you can also adjust the error message to explain the new max-length check. Otherwise, this could be confusing:

Error: Invalid non-hex (0x1234ffffffffffffffffffffffffffffffffffffffffffffffffffffffffffffffffffffffffffffffffffffffffffffff) minimum chain work value specified)

blindly dereferencing it is not good practice

In C++23 we’ll be able to chain them properly via something like auto hex_value = sanitized_hex.and_then(uint256::FromHex); If there’s a C++23 checklist somewhere (haven’t seen any), we could just leave the dereference as is and fix it after we migrate to C++23 instead.

In commit “node: use uint256::FromHex for -minimumchainwork parsing” (3c794b537fe8fe000d3545704afc17d1d63bdb5f)

This makes parsing more strict, by explicitly returning an error when the input is longer than 64 hex characters.

Commit message is incomplete. The number is not just rejected when it is too long but also if it has whitespace prefixes or suffixes or trailing non-hex characters. These would have been ignored previously.

The number is not just rejected when it is too long but also if it has whitespace prefixes or suffixes or trailing non-hex characters. These would have been ignored previously.

Is that so? It seems to me that IsHexNumber() would have returned false for all of those cases, which is why 802374b4355bd1dec7a88bba6287c55f935699fe is a refactor commit?

The number is not just rejected when it is too long but also if it has whitespace prefixes or suffixes or trailing non-hex characters. These would have been ignored previously.

Is that so? It seems to me that IsHexNumber() would have returned false for all of those cases, which is why 802374b is a refactor commit?

Oh, sorry I forgot about the previous call. So this commit message is perfectly accurate as it is.

(In the follow-up you can also adjust the error message to explain the new max-length check.

Done.

Will use .value() either in next force-push

Resolved by using FromUserHex() instead.

In commit “test: unittest chainstatemanager_args” (1c909b2ccfaee902637b28f890db8dbe168ea926)

I don’t think this should be a standalone function when it is only called one place. IMO it would be better to drop it and move the code into the set_opts lambda, which is the only place using it. Getting rid of this function would also avoid unnecessary complexity of converting a char* array to a std::string array and back to a char* array again, and it could avoid introducing an inconsistently named “args_man” variable (majority of code uses args to refer to the ArgsManager instance, and some older code uses argsman, but args_man would be new).

I don’t think this should be a standalone function when it is only called one place.

The reason I organized it this way is because I was quite surprised to see that we don’t have unit testing on setting options from arguments, and SetOptsFromArgs() can trivially (when templated) be reused for any other options setting test, avoiding (non-move) changes to this file. With that said, would you still prefer not having a separate function, or e.g. templating it (prematurely) and moving it into test/util/?

re: #30569 (review)

With that said, would you still prefer not having a separate function, or e.g. templating it (prematurely) and moving it into test/util/?

Thanks for pointing out that it’s a pretty generic function. Either way seems fine, since it does seem like something that could be reused.

In commit “util: refactor: change IsHexNumber to TrySanitizeHexNumber” (802374b4355bd1dec7a88bba6287c55f935699fe)

Would be good to change “Size of” to “Desired size of”. When I first read this I was confused why this would ask for the size to specified.

Would be good to change “Size of” to “Desired size of”.

“Desired” to me sounds like the result string may not actually be of the requested size, when it is actually guaranteed. I’m not sure that would be an improvement?

re: #30569 (review)

“Desired” to me sounds like the result string may not actually be of the requested size, when it is actually guaranteed. I’m not sure that would be an improvement?

Makes sense, I think “Requested” would be better than “Desired” in that case, since the function can still fail and return nullopt.

In commit “util: refactor: change IsHexNumber to TrySanitizeHexNumber” (802374b4355bd1dec7a88bba6287c55f935699fe)

This is a somewhat odd function doing a combination of arbitrary things:

• Trimming 0x prefix
• Returning an error the string is too long
• Padding with 0 if the string is too short

And not doing other things that you might expect a sanitize function to do like trimming whitespace or normalizing the case.

I think instead of saying this “sanitizes” the string and pretending like that word means something it would be better to just say this strips 0x prefixes and fixes the length of the string so it can be passed to the base_blob::ParseHex() function.

In commit “util: refactor: change IsHexNumber to TrySanitizeHexNumber” (802374b4355bd1dec7a88bba6287c55f935699fe)

std::nullopt returned if input is longer than result_size

Implementation seems to return null when string is empty, not just when string is too long.

I think instead of saying this “sanitizes” the string and pretending like that word means something it would be better to just say this strips 0x prefixes and fixes the length of the string so it can be passed to the base_blob::ParseHex() function.

Yeah, I felt a bit uneasy about the arbitrariness of what the function is doing and how it’s named. I can’t really come up with a better name, though, but I’m open to suggestions?

An alternative would be to, as @maflcko initially suggested, just have this be a parameter of base_blob::FromHex(), but that feels less elegant to me, still. What do you think?

re: #30569 (review)

An alternative would be to, as @maflcko initially suggested, just have this be a parameter of base_blob::FromHex(), but that feels less elegant to me, still. What do you think?

Yes I think that suggestion would be a lot better! It’s hard to imagine this function being used for anything other than as a preprocessor to FromHex. And I think it would be great if FromHex had an option to parse numbers strictly, or in a nonstrict way that is still safe, with protection from overflows and garbage characters.

It’s hard to imagine this function being used for anything other than as a preprocessor to FromHex

The reason I carved it out instead of having it be a FromHex() parameter is not for reusability but to not bloat FromHex() with an explosion of input validation options (which also leaks into functions wrapping base_blob::FromHex(), such as transaction_identifier::FromHex()). I don’t think there is a single answer to what we’ll ever need for e.g. the below options, it can very much vary for each callsite and context:

• do we accept leading and/or trailing whitespaces
• do we require fixed size input
• do we forbid, allow or require a 0x prefix
• is an empty string forbidden or equivalent to 0

I think having an opinionated input validation function (and potentially more in the future, when the need arises) and a pristine FromHex() that does not concern itself with input validation is a much more maintainable and testable approach. For that reason, I’d prefer not parameterizing FromHex().

Would an approach like the below (moved into the uint256.h header), alleviate your concern?

 0/**
 1 * [@brief](/bitcoin-bitcoin/contributor/brief/) Helper function for \ref base_blob::FromHex. Returns a copy of
 2 *        the input without (optional) "0x" prefix, and padded with
 3 *        leading zeroes if it is shorter than what T::FromHex expects.
 4 *        If input contains invalid hex characters, including
 5 *        whitespace, or if it is longer than what T::FromHex expects, a
 6 *        std::nullopt is returned.
 7 *
 8 *        If a value is returned, it will be accepted by T::FromHex().
 9 *
10 * [@param](/bitcoin-bitcoin/contributor/param/) input Hex encoding of a number, optionally prefixed with 0x.
11 */
12template <typename T>
13std::optional<std::string> ValidateAndPadHexNumber(std::string_view input) 
14    requires std::is_base_of_v<base_blob<T::size() * 8>, T>;

I think I would probably add base_blob::FromUserHex() function wrapping base_blob::FromHex(). FromHex() would require strict formatting with fixed width strings and no extraneous characters. But when provided with user input, FromUserHex() would allow 0x prefixes, and not require numbers to be padded to a fixed number of digits. Either way, numbers that are too big to fit in the blob or contained unrecognized characters would be errors.

There is no need for either of these functions to trim whitespace as we already have convenient functions to do that, and decision on whether to allow whitespace depends more on how the string arrived (from a file, or over the command line or RPC) than on what produced it.

I think a FromUserHex function would be better than a sidecar ValidateAndPadHexNumber function because it would be easier to use, and more discoverable. The more it was used, the more it would provide a consistent user interface that did not arbitrarily reject things some places which are accepted other places.

I think I would probably add base_blob::FromUserHex() function wrapping base_blob::FromHex().

Thanks for the suggestion, I’ve adopted this approach.

~In commit “node: use uint256::FromHex for -minimumchainwork parsing” (3c794b537fe8fe000d3545704afc17d1d63bdb5f)~ In commit “node: use uint256::FromHex for -assumevalid parsing” (19650768575e3d76d0e06c186896b00ad3e26e57)

This is also now going to reject assumevalid numbers that begin with 0x which isn’t mentioned in the commit message, and also reject numbers with whitespace prefixes and suffixes and trailing non-hex characters.

I think it would be better to just use TrySanitizeHexNumber here to avoid breaking things that work without a clear reason. I can see an argument for rejecting extra whitespace and spurious characters, but 0x prefix should be harmless and it also seems ok to not require numbers to be padded with leading 0’s.

Using TrySanitizeHexNumber here would also allow simplifying the code by dropping the == “0” special case and avoiding the need to modify feature_assumevalid.py.

0x which isn’t mentioned in the commit message, and also reject numbers with whitespace prefixes and suffixes and trailing non-hex characters.

x, whitespace, and trailing non-hex characters are all non-hex characters, so arguably it is mentioned in the commit message, but I agree it wouldn’t hurt to be explicit about the 0x prefix and whitespace.

and it also seems ok to not require numbers to be padded with leading 0’s.

I’m a bit confused about this comment being in the -assumevalid code block, which specifically should only accept hashes and not numbers

Using TrySanitizeHexNumber here would also allow simplifying the code by dropping the == “0” special case and avoiding the need to modify feature_assumevalid.py.

I don’t think we should allow anything besides 0 or a (potentially) valid block hash, that feels brittle to me. But again, given that you’re commenting on -assumedvalid code in a -minimumchainwork commit I think there might be some misunderstanding?

re: #30569 (review)

Thanks for the response, but I think there are two things that could be improved here:

• I think it would be better to call TrySanitizeHexNumber so this continues to accept block hashes with optional 0x prefixes and without leading 0’s. This behavior worked previously and seems safe, so I don’t see a reason to break it. If we do want to break, it would be nice to state some reasoning for doing that in the commit message. I don’t think there is a problem with treating block hashes like numbers since our own code does it and it’s how bitcoin determines block difficulty.

• I don’t think current commit message is accurate unless you drop the word “explicitly” because this change is not just reporting errors that weren’t previously reported, it is making things that use to work and were not errors into errors. The commit message is also incomplete because it is not mentioning the things which used to work.

To be clear, IMO:

• It’s good this change is making it an error to include trailing non-hex characters instead of silently ignoring them.
• It’s good this change is making it an error to specify hex strings that are too long.
• It’s probably ok this change makes it an error to include leading and trailing whitespace, instead of trimming it. Maybe that could be safer and prevent configuration bugs.
• It’s not good but probably ok this change makes it an error to specify an empty string (-assumedvalid="") to disable the assumevalid feature. That seemed like a useful way to clear the setting.
• It’s not good this change makes it an error to include a 0x prefix or write numbers not padded with 0’s. That just seems like being arbitrarily strict and breaking backwards compatibility for no reason.

Thank you for summarizing your concerns @ryanofsky. I believe the latest force-push addresses all of them.

It’s good this change is making it an error to include trailing non-hex characters instead of silently ignoring them. It’s good this change is making it an error to specify hex strings that are too long. It’s probably ok this change makes it an error to include leading and trailing whitespace, instead of trimming it. Maybe that could be safer and prevent configuration bugs.

Relevant behaviour unchanged in latest force-push.

It’s not good but probably ok this change makes it an error to specify an empty string (-assumedvalid="") to disable the assumevalid feature. That seemed like a useful way to clear the setting. It’s not good this change makes it an error to include a 0x prefix or write numbers not padded with 0’s. That just seems like being arbitrarily strict and breaking backwards compatibility for no reason.

Latest force-push continues to allow -assumevalid="" as well as 0x-prefixed and too-short-input (and unit test cases are added to test: unittest chainstatemanager_args), keeping this behaviour the same as it currently is on master.

In commit “test: Only accept a fully valid RANDOM_CTX_SEED” (855784d3a0026414159acc42fceeb271f8a28133)

I don’t understand the motivation for being so strict here. It seems better to call TrySanitizeHexNumber and keep accepting small numbers. The seed just needs to be any number, and I don’t see why numbers that have 64 digits are better than other numbers, or why somebody manually setting the environment variable manually should need to pad the number and make sure it has 64 digits.

In commit “test: Only accept a fully valid RANDOM_CTX_SEED” (855784d)

I don’t understand the motivation for being so strict here.

Just for reference, you approved the exact same commit 2 days prior in #30571#pullrequestreview-2222184242 .

Maybe my comment from #30569 (review) can be moved into the commit message for clarity?

re: #30569 (review)

Thanks for the links. I don’t think I realized this change was made in the other PR, and I think it is not a good change.

I don’t see a reason somebody shouldn’t be able to run RANDOM_CTX_SEED=12345 ./test_bitcoin conveniently from a command line or script without padding the seed with 59 0’s. It seems like this makes it pointlessly difficult to specify a random seed and I don’t get the motivation behind it.

If we want to go ahead with change, I think it should be described accurately as making something that used to work no longer work, not just as adding error checking and implying that anything this now rejects was previously in error.

Thanks for the links. I don’t think I realized this change was made in the other PR, and I think it is not a good change.

I don’t see a reason somebody shouldn’t be able to run RANDOM_CTX_SEED=12345 ./test_bitcoin conveniently from a command line or script without padding the seed with 59 0’s. It seems like this makes it pointlessly difficult to specify a random seed and I don’t get the motivation behind it.

No strong opinion, but my thinking was that the seed would at most be copy-pasted from previous logs of failing tests. If someone really wanted to provide a seed themselves, they could easily run echo 12355 | sha256sum and use the result of that.

But this is test-only, so anything is probably fine here.

It seems like this makes it pointlessly difficult to specify a random seed and I don’t get the motivation behind it.

I definitely didn’t mean to introduce user friction with this PR - the philosophy was more to not introduce unnecessary code complexity for features that aren’t used, but since it seems like this might be breaking existing/valuable use cases, I’ve now updated RANDOM_CTX_SEED to use the more lenient uint256::FromUserHex() to still allow 0x-prefixes and shorter input, and it doesn’t add code complexity anyway.

Nit (feel free to disregard): Based on the description (Minimum work assumed to exist on a valid chain in hex), “chain” and “value” might be superfluous here

0            return util::Error{strprintf(Untranslated("Invalid minimum work specified (%s), must be up to %d characters hex"), *value, uint256::size() * 2)};

re: #30569 (review) re: #30569 (comment)

Maybe I have a slightly different perspective because I think it can be good to name functions after how they are intended to be used, instead of what they do, depending on what’s more relevant to callers. In this case, though, I do think the name describes what it does: parse hex input from a user. The assumption is that callers should not make idiosyncratic choices about what user input to accept, and there should be a common standard.

I don’t like name FromHexLenient so much because it sounds like a negative thing that should be avoided, and doesn’t provide much more information about what the function does either.

If we do want the function name to more literally describe what it does, I think FromHexNumber could work because it’s conventional for hex numbers to have 0x prefixes and not need to be padded with 0’s.

But my vote would be for FromUserHex over FromHexNumber over FromHexLenient

I’m open to the idea, but I don’t immediately see a super elegant solution. Thoughts:

• a lot of the ::size() * 2 uses are templated (such as in the referenced example here), so constants wouldn’t really work. Adding a base_blob::hex_size() seems inappropriate to me too, that’s not really a base_blob concern.
• non-templated use is not very frequent, and mostly in tests. Potentially a constexpr auto UINT256_HEX_SIZE{uint256::size() * 2} could work, but I’m not really convinced that’s worth it either tbh

nit: I was wondering if it accepts 00 as well (since numbers usually trim leading zeros) and if it accepts 1 (since other implementations require an even number of digits), consider extending the examples:

0    BOOST_CHECK_EQUAL(uint256::FromUserHex("00").value(), uint256::ZERO);
1    BOOST_CHECK_EQUAL(uint256::FromUserHex("1").value(), uint256::ONE);

nit, if you touch again, consider:

0                std::cerr << RANDOM_CTX_SEED << " must consist of up to " << uint256::size() * 2 << " hex digits (\"0x\" prefix allowed), it was set to: '" << num << "'.\n";

In commit “refactor: add uint256::FromUserHex helper” (4e05d9459ad275d5929bab8f55e7edcd0be9dca9)

Not important but could also add a test that this does not allow spaces after a 64 digit string (not just a short string).

In commit “refactor: add uint256::FromUserHex helper” (4e05d9459ad275d5929bab8f55e7edcd0be9dca9)

I found these two tests and especially the comment above “a value is expected when the last (65th) character is trimmed off valid_hex_65” confusing because they make it sound like the function is supposed to trim off the 65th hex character and ignore it.

Would change the comment above to plainly describe the string: // 65 digit hex number with 0x prefix

And add a comment to explain the thing these two tests are actually checking: // Pass views of first 64 hex digits to verify string_view size is respected.

In commit “refactor: add uint256::FromUserHex helper” (4e05d9459ad275d5929bab8f55e7edcd0be9dca9)

We seem to be dropping the mixed case checks from the previous code:

0    BOOST_CHECK(IsHexNumber("0xFfa"));
1    BOOST_CHECK(IsHexNumber("Ffa"));
2    BOOST_CHECK(IsHexNumber("0x00112233445566778899aabbccddeeffAABBCCDDEEFF"));
3    BOOST_CHECK(IsHexNumber("00112233445566778899aabbccddeeffAABBCCDDEEFF"));

I tend to think it would be good to keep these, so code is changing less. But if these are intentionally being dropped, it could be good to add a test comment saying what this test is intentionally not checking, and pointing to other tests where the checks are.

Hmm, fair point. I think I dropped it because of #30377 dropping mixed case support for the consteval ctor, but that’s unrelated to this pull of course so keeping the test is the right approach. I’ve added mixed case into 4 existing checks, but I’d still like to drop the Ffa tests since we can’t use the uint8_t ctor which would make an equality test quite verbose for I think nothing that’s not already tested in the other lines?

0    BOOST_CHECK_EQUAL(uint256::FromUserHex("0xFf").value(), uint256{0xff});
1    BOOST_CHECK_EQUAL(uint256::FromUserHex("Ff").value(), uint256{0xff});
2    const std::string valid_hex_64{"0x0123456789abcdef0123456789abcdef0123456789ABDCEF0123456789ABCDEF"};
3    BOOST_REQUIRE_EQUAL(valid_hex_64.size(), 2 + 64); // 0x prefix and 64 hex digits
4    BOOST_CHECK_EQUAL(uint256::FromUserHex(valid_hex_64.substr(2)).value().ToString(), ToLower(valid_hex_64.substr(2)));
5    BOOST_CHECK_EQUAL(uint256::FromUserHex(valid_hex_64.substr(0)).value().ToString(), ToLower(valid_hex_64.substr(2)));