refactor: Add consteval uint256 constructor #30560
Conversation
|
The following sections might be updated with supplementary metadata relevant to reviewers and maintainers. Code CoverageFor detailed information about the code coverage, see the test coverage report. ReviewsSee the guideline for information on the review process.
If your review is incorrectly listed, please react with 👎 to this comment and the bot will ignore it on the next update. ConflictsReviewers, this pull request conflicts with the following ones:
If you consider this pull request important, please also help to review the conflicting pull requests. Ideally, start with the one that should be merged first. |
Is this true? IIRC it was true for
It would be good to mention them: "Requires exactly 64 hex characters" (or so). Also, it could recap the risks of uint256S: "Previously, any invalid string was accepted silently."? |
Your memory serves you right: #30377 (comment)
Cheers, adjusted summary somewhat. |
This comment was marked as spam.
This comment was marked as spam.
Sorry, something went wrong.
This comment was marked as spam.
This comment was marked as spam.
Sorry, something went wrong.
9813cbf to
9315c1f
Compare
There was a problem hiding this comment.
Left some more nits.
ACK 9315c1f 🕵
Show signature
Signature:
untrusted comment: signature from minisign secret key on empty file; verify via: minisign -Vm "${path_to_any_empty_file}" -P RWTRmVTMeKV5noAMqVlsMugDDCyyTSbA3Re5AkUrhvLVln0tSaFWglOw -x "${path_to_this_whole_four_line_signature_blob}"
RUTRmVTMeKV5npGrKx1nqXCw5zeVHdtdYURB/KlyA/LMFgpNCs+SkW9a8N95d+U4AP1RJMi+krxU1A3Yux4bpwZNLvVBKy0wLgM=
trusted comment: ACK 9315c1f34abaa4cd78185818ed8d2df5f35eb3ce 🕵
GZQfOWL8G0spNKvjqmkS65RYUYIQBpZjUi0j+fcqLs9A5d+ffS+M/zKWMvJBpjmBad5mU4mrr/6AY/sNvV6DBg==
9315c1f to
ed9823f
Compare
ed9823f to
c0b259b
Compare
There was a problem hiding this comment.
Re-ACK c0b259b
Only changes are dropping the commit changing some strings to string_view.
|
@TheCharlatan thanks for the speedy re-ACK! My latest force-push actually dropped a tangential commit: #30560 (comment) |
There was a problem hiding this comment.
ACK c0b259b
Very nice PR, more compile-time checks and easy to review. Makes my branch removing uint256S a bunch more compact too. Non-blocking nits.
c0b259b to
be750dc
Compare
| BOOST_CHECK_EQUAL(one, uint256::ONE); | ||
| } | ||
|
|
||
| BOOST_AUTO_TEST_CASE(FromHex_vs_uint256) | ||
| { | ||
| auto runtime_uint{uint256::FromHex("4A5E1E4BAAB89F3A32518A88C31BC87F618f76673e2cc77ab2127b7afdeda33b").value()}; |
There was a problem hiding this comment.
are we sure we want to mandate that mixed-case hex strings can be parsed? I consider that an error instead
There was a problem hiding this comment.
At some point, retouching ACKed PRs becomes an attack on Bitcoin. But this is a good enough question to possibly trigger a re-touch.
Would be happy to hear opinions from others on this. But I understand if they prefer spending time on more significant things.
Supporting both cases could be considered a hangover from SetHexDeprecated, but on the other hand, mixed cases are still supported by FromHex/IsHex/HexDigit.
Checked for occurrences of uppercase hex after "uint256".
$ git grep -E 'uint256.*"[a-fA-F0-9]{64}"' |grep -E 'uint256.{0,32}".{0,63}[A-F]+.{0,63}"'
src/kernel/chainparams.cpp: uint256{"0000A000000002dc756eebf4f49723ed8d30cc28a5f108eb94b1ba88ac4f9c22"}, SCRIPT_VERIFY_NONE);
src/test/pow_tests.cpp: arith_uint256 targ_max{UintToArith256(uint256{"FFFFFFFFFFFFFFFFFFFFFFFFFFFFFFFFFFFFFFFFFFFFFFFFFFFFFFFFFFFFFFFF"})};
src/test/uint256_tests.cpp: auto runtime_uint{uint256::FromHex("4A5E1E4BAAB89F3A32518A88C31BC87F618f76673e2cc77ab2127b7afdeda33b").value()};
src/test/uint256_tests.cpp: constexpr uint256 consteval_uint{ "4a5e1e4baab89f3a32518a88c31bc87f618F76673E2CC77AB2127B7AFDEDA33B"};
src/kernel/chainparams.cpp was my modified local file. So it's only pow_tests.cpp and the current test use uppercase.
Implemented a version of this PR with lowercase enforcement (+ including assert => throw from #30560 (comment)) in 40bd3c1. It uses \L in the scripted-diff regexp to lowercase the hex number (might not be supported on Mac sed I think, but Mac is not officially supported anyways).
$ git range-diff master be750dccb08050bcca48c4236ca6b902dd911c7b 40bd3c1aa766572bf5eb8e14bbc9f9606a69299b
1: 9f4ca1f293 ! 1: 49ff7af46c refactor: Add consteval uint256(hex_str)
@@ src/test/uint256_tests.cpp: BOOST_AUTO_TEST_CASE( check_ONE )
+BOOST_AUTO_TEST_CASE(FromHex_vs_uint256)
+{
+ auto runtime_uint{uint256::FromHex("4A5E1E4BAAB89F3A32518A88C31BC87F618f76673e2cc77ab2127b7afdeda33b").value()};
-+ constexpr uint256 consteval_uint{ "4a5e1e4baab89f3a32518a88c31bc87f618F76673E2CC77AB2127B7AFDEDA33B"};
++ constexpr uint256 consteval_uint{ "4a5e1e4baab89f3a32518a88c31bc87f618f76673e2cc77ab2127b7afdeda33b"};
+ BOOST_CHECK_EQUAL(consteval_uint, runtime_uint);
+}
+
@@ src/uint256.h: public:
+ // Non-lookup table version of HexDigit().
+ auto from_hex = [](const char c) -> int8_t {
+ if (c >= '0' && c <= '9') return c - '0';
++ // Only lowercase letters are supported, for consistency.
+ if (c >= 'a' && c <= 'f') return c - 'a' + 0xA;
-+ if (c >= 'A' && c <= 'F') return c - 'A' + 0xA;
+
-+ assert(false); // Reached if ctor is called with an invalid hex digit.
++ throw "Called ctor with an invalid hex digit";
+ };
+
-+ assert(hex_str.length() == m_data.size() * 2); // 2 hex digits per byte.
++ if (hex_str.length() != m_data.size() * 2) throw "Hex string must fit exactly";
+ auto str_it = hex_str.rbegin();
+ for (auto& elem : m_data) {
+ auto lo = from_hex(*(str_it++));
2: d5e5d7eacc = 2: 5994e659c0 refactor: Hand-replace some uint256S -> uint256
3: be750dccb0 ! 3: 40bd3c1aa7 scripted-diff: Replace uint256S("str") -> uint256{"str"}
@@ Commit message
scripted-diff: Replace uint256S("str") -> uint256{"str"}
-BEGIN VERIFY SCRIPT-
- sed -i --regexp-extended -e 's/uint256S\("(0x)?([^"]{64})"\)/uint256{"\2"}/g' $(git grep -l uint256S)
+ sed -i --regexp-extended -e 's/uint256S\("(0x)?([^"]{64})"\)/uint256{"\L\2"}/g' $(git grep -l uint256S)
-END VERIFY SCRIPT-
## src/kernel/chainparams.cpp ##
@@ src/test/pow_tests.cpp: void sanity_check_chainparams(const ArgsManager& args, C
// check max target * 4*nPowTargetTimespan doesn't overflow -- see pow.cpp:CalculateNextWorkRequired()
if (!consensus.fPowNoRetargeting) {
- arith_uint256 targ_max{UintToArith256(uint256S("0xFFFFFFFFFFFFFFFFFFFFFFFFFFFFFFFFFFFFFFFFFFFFFFFFFFFFFFFFFFFFFFFF"))};
-+ arith_uint256 targ_max{UintToArith256(uint256{"FFFFFFFFFFFFFFFFFFFFFFFFFFFFFFFFFFFFFFFFFFFFFFFFFFFFFFFFFFFFFFFF"})};
++ arith_uint256 targ_max{UintToArith256(uint256{"ffffffffffffffffffffffffffffffffffffffffffffffffffffffffffffffff"})};
targ_max /= consensus.nPowTargetTimespan*4;
BOOST_CHECK(UintToArith256(consensus.powLimit) < targ_max);
}
There was a problem hiding this comment.
Thanks for considering, I'm personally fine with doing it in a different PR as well
There was a problem hiding this comment.
I don't have a strong view on this but I would prefer getting this PR over the line as is.
There was a problem hiding this comment.
Trying to not spawn yet another PR from this work but will post that as a separate PR then if this one is merged in the current state.
|
re-ACK be750dc 📩 Show signatureSignature: |
|
ACK be750dc |
be750dc to
a34ba0e
Compare
|
Rebased with minimal changes to fix merge-issues.
Edit: I realized too late that this might have been an okay time to incorporate #30560 (comment) but it is probably just as well as it could lead to further discussion and dragging out this PR. |
|
rebase re-ACK a34ba0e 🔺 Show signatureSignature: |
|
ACK a34ba0e |
|
(needs another rebase) |
Complements uint256::FromHex() nicely in that it naturally does all error checking at compile time and so doesn't need to return an std::optional. Will be used in the following 2 commits to replace many calls to uint256S(). uint256S() calls taking C-string literals are littered throughout the codebase and 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.
chainparams.cpp - workaround for MSVC bug triggering C7595 - Calling consteval constructors in initializer lists fails, but works on GCC (13.2.0) & Clang (17.0.6).
-BEGIN VERIFY SCRIPT-
sed -i --regexp-extended -e 's/\buint256S\("(0x)?([^"]{64})"\)/uint256{"\2"}/g' $(git grep -l uint256S)
-END VERIFY SCRIPT-
a34ba0e to
2d9d752
Compare
|
Checked with rebase re-cr-ACK 2d9d752 🎐 Show signatureSignature: |
|
re-ACK 2d9d752 |
Ran scripted-diff from 2d9d752. Follow-up to bitcoin#29775 which overlapped with work on bitcoin#30560 (the latter includes the scripted-diff commit).
Ran scripted-diff from 2d9d752. Follow-up to bitcoin#29775 which overlapped with work on bitcoin#30560 (the latter includes the scripted-diff commit).
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](fad2991#diff-800776e2dda39116e889839f69409571a5d397de048a141da7e4003bc099e3e2R138) because it is less robust than the `base_blob::FromHex()` introduced in [the same PR](#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
…{"str"}
49f9b64 refactor: Testnet4 - Replace uint256S("str") -> uint256{"str"} (Hodlinator)
Pull request description:
Ran scripted-diff from 2d9d752:
```
sed -i --regexp-extended -e 's/\buint256S\("(0x)?([^"]{64})"\)/uint256{"\2"}/g' $(git grep -l uint256S)
```
Follow-up to Testnet4 introduction #29775 which overlapped with work on `uint256` `consteval` ctor #30560 (the latter includes the scripted-diff commit).
Going forward `uint256{}` should be used for constants instead of `uint256S()`.
ACKs for top commit:
maflcko:
review-ACK 49f9b64 🐮
fjahr:
ACK 49f9b64
Tree-SHA512: 94fe5d9f1fb85e9ce5c3c4c5e4c31667e8cbb55ee691a4b5b3ae4172ccac38230281071023663965917f188b4c19bdf67afd4e3cdf69d89e97c65faea88af833
8756ccd scripted-diff: Replace ParseHex[<std::byte>]("str") -> "str"_hex[_u8] (Hodlinator) 9cb6873 refactor: Prepare for ParseHex -> ""_hex scripted-diff (Hodlinator) 50bc017 refactor: Hand-replace some ParseHex -> ""_hex (Hodlinator) 5b74a84 util: Add consteval ""_hex[_v][_u8] literals (l0rinc) dc5f6f6 test refactor: util_tests - parse_hex clean up (Hodlinator) 2b5e6ef refactor: Make XOnlyPubKey tolerate constexpr std::arrays (Hodlinator) 403d86f refactor: vector -> span in CCrypter (Hodlinator) bd0830b refactor: de-Hungarianize CCrypter (Hodlinator) d99c816 refactor: Improve CCrypter related lines (Hodlinator) 7e1d9a8 refactor: Enforce lowercase hex digits for consteval uint256 (Hodlinator) Pull request description: Motivation: * Validates and converts the hex string into bytes at compile time instead of at runtime like `ParseHex()`. * Eliminates runtime dependencies: #30377 (comment), #30048 (comment) * Has stricter requirements than `ParseHex()` (disallows whitespace and uppercase hex digits) and replaces it in a bunch of places. * Makes it possible to derive other compile time constants. * Minor: should shave off a few runtime CPU cycles. `""_hex` produces `std::array<std::byte>` as the momentum in the codebase is to use `std::byte` over `uint8_t`. Also makes `uint256` hex string constructor disallow uppercase hex digits. Discussed: #30560 (comment) Surprisingly does not change the size of the Guix **bitcoind** binary (on x86_64-linux-gnu) by 1 single byte. Spawned already merged PRs: #30436, #30482, #30532, #30560. ACKs for top commit: l0rinc: ACK 8756ccd stickies-v: Rebase re-ACK 8756ccd, no changes since a096215 ryanofsky: Code review ACK 8756ccd, just rebasing since last review and taking advantage of CScript constructors in #29369, also tweaking a code comment Tree-SHA512: 9b2011b7c37e0ef004c669f8601270a214b388916316458370f5902c79c2856790b1b2c7c123efa65decad04886ab5eff95644301e0d84358bb265cf1f8ec195
Motivation:
uint256.SetHexDeprecated()is called in less places).uint256S()(requiring 64 chars exactly, disallows garbage at the end) and replaces it in a bunch of places.Extracted from #30377 which diverged into exploring
constevalParseHex()solutions.