Skip to content
New issue

Have a question about this project? Sign up for a free GitHub account to open an issue and contact its maintainers and the community.

Sign up for GitHub

By clicking “Sign up for GitHub”, you agree to our terms of service and privacy statement. We’ll occasionally send you account related emails.

Already on GitHub? Sign in to your account

node: reduce unsafe uint256S usage #30569

Merged
merged 6 commits into from
Aug 27, 2024
Merged

node: reduce unsafe uint256S usage #30569

Show file tree
Hide file tree
Changes from 1 commit
Commits
Show all changes
6 commits
Select commit Hold shift + click to select a range
85b7cbf
test: unittest chainstatemanager_args
stickies-v Aug 6, 2024
70e2c87
refactor: add uint256::FromUserHex helper
stickies-v Aug 20, 2024
8a44d7d
node: use uint256::FromUserHex for -minimumchainwork parsing
stickies-v Jul 26, 2024
2e58fdb
util: remove unused IsHexNumber
stickies-v Aug 20, 2024
6819e5a
node: use uint256::FromUserHex for -assumevalid parsing
stickies-v Jul 26, 2024
18d65d2
test: use uint256::FromUserHex for RANDOM_CTX_SEED
stickies-v Jul 30, 2024
File filter

Filter by extension

Filter by extension
Conversations
Failed to load comments.
Loading
Jump to
Jump to file
Failed to load files.
Loading
Diff view
Diff view
Prev Previous commit
Next Next commit
refactor: add uint256::FromUserHex helper 
FromUserHex will be used in future commits to construct
uint256 instances from user hex input without being
unnecessarily restrictive on formatting by allowing
0x-prefixed input that is shorter than 64 characters.
  • Loading branch information
@stickies-v
stickies-v committed Aug 23, 2024
commit 70e2c87737e77ee85812cc328c4ddfaea7147533
4 changes: 4 additions & 0 deletions src/test/fuzz/hex.cpp
View file
Open in desktop
Original file line number Diff line number Diff line change
Expand Up @@ -32,6 +32,10 @@ FUZZ_TARGET(hex)
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 {
Expand Down
29 changes: 29 additions & 0 deletions src/test/uint256_tests.cpp
View file
Open in desktop
Original file line number Diff line number Diff line change
Expand Up @@ -386,6 +386,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);
stickies-v marked this conversation as resolved.
Show resolved Hide resolved
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 = uint256S("0000000000000000000000000000000000000000000000000000000000000001");
Expand Down
22 changes: 22 additions & 0 deletions src/uint256.h
View file
Open in desktop
Original file line number Diff line number Diff line change
Expand Up @@ -9,6 +9,7 @@
#include <crypto/common.h>
#include <span.h>
#include <util/strencodings.h>
#include <util/string.h>

#include <algorithm>
#include <array>
Expand All @@ -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>
Expand Down Expand Up @@ -106,6 +108,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)
Copy link
Contributor

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

Since this is a "User" centric specialization of FromHex (and not a Hex specialization of FromUser), I'd keep the FromHex prefix, i.e.: FromHexUser or perhaps FromHexLenient, since the User part is just incidental

Copy link
Contributor

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

(Thread https://github.com/bitcoin/bitcoin/pull/30569/files#r1725693175)
I prefer the current uint256::FromUserHex over uint256::FromHexUser (the latter makes me think of sorcerers). (uint256::FromHexLenient is okay to).

Copy link
Contributor Author

@stickies-v stickies-v Aug 22, 2024 •
edited
Loading

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

I have nothing against any of the suggested names, but personally I still find FromUserHex to be the most intuitive (and it seems at least 2 other reviewers like it as well), so I'm going to stick with that if that's okay?

Copy link
Contributor

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

re: #30569 (comment)
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

{
input = util::RemovePrefixView(input, "0x");
constexpr auto expected_size{uintN_t::size() * 2};
stickies-v marked this conversation as resolved.
Show resolved Hide resolved
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.
Expand All @@ -127,6 +148,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;
constexpr explicit uint256(uint8_t v) : base_blob<256>(v) {}
constexpr explicit uint256(Span<const unsigned char> vch) : base_blob<256>(vch) {}
Expand Down