refactor: Replace ParseHex with consteval ""_hex literals by hodlinator · Pull Request #30377 · bitcoin/bitcoin

hodlinator · 2024-07-02T10:50:08Z

Motivation:

Validates and converts the hex string into bytes at compile time instead of at runtime like ParseHex().
Eliminates runtime dependencies: refactor: Replace ParseHex with consteval ""_hex literals #30377 (comment), crypto: add NUMS_H const #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.

DrahtBot · 2024-07-02T10:50:10Z

The following sections might be updated with supplementary metadata relevant to reviewers and maintainers.

Code Coverage

For detailed information about the code coverage, see the test coverage report.

Reviews

See the guideline for information on the review process.

Type	Reviewers
ACK	l0rinc, ryanofsky, stickies-v
Stale ACK	maflcko

If your review is incorrectly listed, please react with 👎 to this comment and the bot will ignore it on the next update.

Conflicts

Reviewers, this pull request conflicts with the following ones:

#30765 (refactor: Extend CScript with operator<< for std::array by l0rinc)
#30618 (test: increase FromUserHex FUZZ and unit testing coverage by l0rinc)
#28710 (Remove the legacy wallet and BDB dependency by achow101)
#26593 (tracing: Only prepare tracepoint arguments when actually tracing by 0xB10C)

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.

maflcko · 2024-07-04T12:29:51Z

transaction_identifier.h - Fixed dormant bug in TxidFromString() where the string_view length wasn't respected(!).

This is known, see #28922 (comment). Thanks for picking it up!

Maybe submit the fix first?

Realizing my GH history has minimum proof of work, I might have delayed creating a PR, but it felt timely as Testnet 4 is being worked on.

Not sure what this has to do with testnet 4. May be best to remove unrelated non-technical details from the commits and pull request description. (You can still share them in later comments, if you really want)

Concept ACK. The same should be done to ParseHex: #30048 (comment)

hodlinator · 2024-07-05T22:00:08Z

transaction_identifier.h - Fixed dormant bug in TxidFromString() where the string_view length wasn't respected(!).

This is known, see #28922 (comment). Thanks for picking it up!

Maybe submit the fix first?

Aha, good that you are tracking it! I see at least 4 possible fixes:

Switch back to Txid TxidFromString(const std::string& str)
Make TxidFromString() convert from std::string_view back to std::string internally before calling uint256S() to introduce a null-terminator.
Carry over the full SetHex(std::string_view) implementation from this PR without touching the SetHex(const char*) implementation.
Like 3 but implement SetHex(const char* str) by calling the std::string_view version.

Which do you recommend?

Realizing my GH history has minimum proof of work, I might have delayed creating a PR, but it felt timely as Testnet 4 is being worked on.

Not sure what this has to do with testnet 4. May be best to remove unrelated non-technical details from the commits and pull request description. (You can still share them in later comments, if you really want)

Thanks for the feedback. The Testnet 4 PR from this weeks review club introduces new hash-literals to the code, but I concede that it's a weak argument.

Concept ACK. The same should be done to ParseHex: #30048 (comment)

Thanks for having a look and the pointer to ParseHex! It was on my radar momentarily but I didn't reconsider it after reaching the current solution for uint256S(). Should probably introduce a consteval ParseHex(const char*) implementation as part of this PR. Moving to draft for now.

maflcko · 2024-07-08T06:47:17Z

Like 3 but implement SetHex(const char* str) by calling the std::string_view version.

I don't think const char* overloads will need to be provided when string_view exists. Seems fine to just have a single sting_view function (and call it a fix at the same time).

ryanofsky · 2024-07-08T15:22:15Z

Concept ACK, and would be very nice for this to cover ParseHex. If it did, it seems like it would fix the unexpected consensus library dependency on the util library that hebasto reported in #29015 (comment):

bitcoin/src/pubkey.cpp

Line 193 in a83f050

    
           static const std::vector<unsigned char> NUMS_H_DATA{ParseHex("50929b74c1a04954b78b4b6035e97a5e078a5a0f28ec96d547bfee9ace803ac0")};

hodlinator · 2024-07-12T09:09:21Z

transaction_identifier.h - Fixed dormant bug in TxidFromString() where the string_view length wasn't respected(!).

This is known, see #28922 (comment). Thanks for picking it up!

Maybe submit the fix first?

PR up now: #30436

DrahtBot · 2024-07-16T21:51:26Z

🚧 At least one of the CI tasks failed.
_{Debug: https://github.com/bitcoin/bitcoin/runs/27531471253}

Hints

Make sure to run all tests locally, according to the documentation.

The failure may happen due to a number of reasons, for example:

Possibly due to a silent merge conflict (the changes in this pull request being
incompatible with the current code in the target branch). If so, make sure to rebase on the latest
commit of the target branch.
A sanitizer issue, which can only be found by compiling with the sanitizer and running the
affected test.
An intermittent issue.

Leave a comment here, if you need help tracking down a confusing failure.

l0rinc · 2024-08-24T19:30:38Z

~~I'm not sure what's with the Instruction clone failed CI failures, but the change looks good to me~~ the build fixed itself

ACK 92e599d57c507f35d889c3a804d2a7020dcc6b1d

ryanofsky

Code review ACK 92e599d57c507f35d889c3a804d2a7020dcc6b1d, just small suggested tweaks since last review (comments, constexpr, MakeByteSpan)

hodlinator · 2024-08-28T07:01:09Z

Rebased due to conflict. Also took advantage of now merged #29369 that fully resolves #30377 (comment) and #30377 (comment).

Changes are:

3 out of 5 replacements in miniscript_tests.cpp are moved to the scripted-diff commit.
The new ToScript in script_tests.cpp can now use the more natural {span.begin(), span.end()} instead of {span.data(), span.data() + span.size()}.

stickies-v

re-ACK dae42d474d04168fba077f30bf5113721e9fc108

stickies-v · 2024-08-28T11:59:12Z

re-ACK a096215c9c71a2ec03e76f1fd0bcdda0727996e0

l0rinc · 2024-08-28T16:00:32Z

Changes since 92e599d57c507f35d889c3a804d2a7020dcc6b1d:

diff --git a/src/test/miniscript_tests.cpp b/src/test/miniscript_tests.cpp
index 452baebff3..e9b0d0ec1d 100644
--- a/src/test/miniscript_tests.cpp
+++ b/src/test/miniscript_tests.cpp
@@ -596,11 +596,11 @@ BOOST_AUTO_TEST_CASE(fixed_tests)
     //  - no pubkey before the CHECKSIG
     constexpr KeyConverter tap_converter{miniscript::MiniscriptContext::TAPSCRIPT};
     constexpr KeyConverter wsh_converter{miniscript::MiniscriptContext::P2WSH};
-    const auto no_pubkey{"ac519c"_hex_v_u8};
+    const auto no_pubkey{"ac519c"_hex_u8};
     BOOST_CHECK(miniscript::FromScript({no_pubkey.begin(), no_pubkey.end()}, tap_converter) == nullptr);
-    const auto incomplete_multi_a{"ba20c6047f9441ed7d6d3045406e95c07cd85c778e4b8cef3ca7abac09b95c709ee5ba519c"_hex_v_u8};
+    const auto incomplete_multi_a{"ba20c6047f9441ed7d6d3045406e95c07cd85c778e4b8cef3ca7abac09b95c709ee5ba519c"_hex_u8};
     BOOST_CHECK(miniscript::FromScript({incomplete_multi_a.begin(), incomplete_multi_a.end()}, tap_converter) == nullptr);
-    const auto incomplete_multi_a_2{"ac2079be667ef9dcbbac55a06295ce870b07029bfcdb2dce28d959f2815b16f81798ac20c6047f9441ed7d6d3045406e95c07cd85c778e4b8cef3ca7abac09b95c709ee5ba519c"_hex_v_u8};
+    const auto incomplete_multi_a_2{"ac2079be667ef9dcbbac55a06295ce870b07029bfcdb2dce28d959f2815b16f81798ac20c6047f9441ed7d6d3045406e95c07cd85c778e4b8cef3ca7abac09b95c709ee5ba519c"_hex_u8};
     BOOST_CHECK(miniscript::FromScript({incomplete_multi_a_2.begin(), incomplete_multi_a_2.end()}, tap_converter) == nullptr);
     // Can use multi_a under Tapscript but not P2WSH.
     Test("and_v(v:multi_a(2,03d01115d548e7561b15c38f004d734633687cf4419620095bc5b0f47070afe85a,025601570cb47f238d2b0286db4a990fa0f3ba28d1a319f5e7cf55c2a2444da7cc),after(1231488000))", "?", "20d01115d548e7561b15c38f004d734633687cf4419620095bc5b0f47070afe85aac205601570cb47f238d2b0286db4a990fa0f3ba28d1a319f5e7cf55c2a2444da7ccba529d0400046749b1", TESTMODE_VALID | TESTMODE_NONMAL | TESTMODE_NEEDSIG | TESTMODE_P2WSH_INVALID, 4, 2, {}, {}, 3);
@@ -645,12 +645,12 @@ BOOST_AUTO_TEST_CASE(fixed_tests)
 
     // Misc unit tests
     // A Script with a non minimal push is invalid
-    const std::vector<unsigned char> nonminpush{"0000210232780000feff00ffffffffffff21ff005f00ae21ae00000000060602060406564c2102320000060900fe00005f00ae21ae00100000060606060606000000000000000000000000000000000000000000000000000000000000000000"_hex_v_u8};
+    constexpr auto nonminpush{"0000210232780000feff00ffffffffffff21ff005f00ae21ae00000000060602060406564c2102320000060900fe00005f00ae21ae00100000060606060606000000000000000000000000000000000000000000000000000000000000000000"_hex_u8};
     const CScript nonminpush_script(nonminpush.begin(), nonminpush.end());
     BOOST_CHECK(miniscript::FromScript(nonminpush_script, wsh_converter) == nullptr);
     BOOST_CHECK(miniscript::FromScript(nonminpush_script, tap_converter) == nullptr);
     // A non-minimal VERIFY (<key> CHECKSIG VERIFY 1)
-    const std::vector<unsigned char> nonminverify{"2103a0434d9e47f3c86235477c7b1ae6ae5d3442d49b1943c2b752a68e2a47e247c7ac6951"_hex_v_u8};
+    constexpr auto nonminverify{"2103a0434d9e47f3c86235477c7b1ae6ae5d3442d49b1943c2b752a68e2a47e247c7ac6951"_hex_u8};
     const CScript nonminverify_script(nonminverify.begin(), nonminverify.end());
     BOOST_CHECK(miniscript::FromScript(nonminverify_script, wsh_converter) == nullptr);
     BOOST_CHECK(miniscript::FromScript(nonminverify_script, tap_converter) == nullptr);
diff --git a/src/test/script_tests.cpp b/src/test/script_tests.cpp
index ef6ca8dc0d..5825684947 100644
--- a/src/test/script_tests.cpp
+++ b/src/test/script_tests.cpp
@@ -1358,7 +1358,7 @@ template <typename T>
 CScript ToScript(const T& byte_container)
 {
     auto span{MakeUCharSpan(byte_container)};
-    return {span.data(), span.data() + span.size()};
+    return {span.begin(), span.end()};
 }
 
 static CScript ScriptFromHex(const std::string& str)
diff --git a/src/util/strencodings.h b/src/util/strencodings.h
index f9076de047..1543de03ab 100644
--- a/src/util/strencodings.h
+++ b/src/util/strencodings.h
@@ -396,7 +396,7 @@ consteval uint8_t ConstevalHexDigit(const char c)
  *   length and serialized with no prefix.
  *
  * @warning It may be preferable to use vector variants to save stack space when
- *   declaring local variables if hex strings are large. Alternately variables
+ *   declaring local variables if hex strings are large. Alternatively variables
  *   could be declared constexpr to avoid using stack space.
  *
  * @warning Avoid `uint8_t` variants when not necessary, as the codebase

ACK a096215c9c71a2ec03e76f1fd0bcdda0727996e0

Also changes compile-time asserts with comments into throws.

Lines will be touched in next 2 commits.

Beyond renaming it also adjusts whitespace and adds braces to conform to current doc/developer-notes.md. TestEncrypt: Change iterator type to auto in ahead of vector -> span conversion. Only touches functions that will be modified in next commit.

TestEncryptSingle: Remove no longer needed plaintext2-variable that existed because vectors had different allocators.

Length was already asserted inside of base_blob-ctor.

* Use BOOST_CHECK_EQUAL_COLLECTIONS and BOOST_CHECK_EQUAL instead of deprecated BOOST_CHECK. * Avoid repeating expected values. * Break out repeated HEX_PARSE_INPUT and rename ParseHex_expected to HEX_PARSE_OUTPUT. Done in preparation for adding a couple more tests in the next commit. Co-Authored-By: l0rinc <pap.lorinc@gmail.com>

""_hex is a compile-time user-defined literal returning std::array<std::byte>, equivalent of ParseHex. Variants: - ""_hex_v returns std::vector<std::byte> - ""_hex_u8 returns std::array<uint8_t> - ""_hex_v_u8 returns std::vector<uint8_t> - Directly serializable as a size-prefixed OP_PUSH CScript payload using operator<<. Also extracts from_hex into shared util::ConstevalHexDigit function. Co-Authored-By: hodlinator <172445034+hodlinator@users.noreply.github.com> Co-Authored-By: MarcoFalke <*~=`'#}+{/-|&$^_@721217.xyz> Co-Authored-By: Ryan Ofsky <ryan@ofsky.org> Co-Authored-By: stickies-v <stickies-v@protonmail.com>

The following scripted-diff commit will replace ParseHex("...") with "..."_hex_u8, but this replacement will not work in cases where vectors are needed instead of arrays, and is not ideal in cases where std::byte is accepted. For example, it is currently necessary to use _hex_v_u8 when calling CScript operator<< because that operator does not currently support std::array or std::byte. Conversely, it is incorrect to use _hex_v instead of _hex in net_processing.cpp for the MakeAndPushMessage argument, because if the argument is a std::vector it is considered variable-length and serialized with a size prefix, but if the argument is a std::array or Span is it considered fixed length and serialized without a prefix. By the same logic, it is also safe to change the NUMS_H constant in pubkey.cpp from a std::vector to std::array because it is never serialized.

- Adds using namespace. - Extracts ToScript helper function from ScriptFromHex, to be used heavily in the next commit. - Changes ScriptFromHex from using ParseHex to TryParseHex, now asserting the string is valid. - Use even number of hex digits in comment (and apply replacement from next commit to only touch line once).

Ideally all call sites should accept std::byte instead of uint8_t but those transformations are left to future PRs. -BEGIN VERIFY SCRIPT- sed -i --regexp-extended 's/\bParseHex$("[^"]*")$/\1_hex_u8/g' $(git grep -l ParseHex -- :src ':(exclude)src/test/util_tests.cpp') sed -i --regexp-extended 's/\bParseHex<std::byte>$("[^"]*")$/\1_hex/g' $(git grep -l ParseHex -- :src ':(exclude)src/test/util_tests.cpp') sed -i --regexp-extended 's/\bScriptFromHex$("[^"]*")$/ToScript(\1_hex)/g' src/test/script_tests.cpp -END VERIFY SCRIPT- Co-Authored-By: MarcoFalke <*~=`'#}+{/-|&$^_@721217.xyz> Co-Authored-By: Ryan Ofsky <ryan@ofsky.org>

l0rinc · 2024-08-28T21:43:38Z

Non-trivial rebase with conflicts in bloom_tests, crypto_tests and txpackage_tests.
The resolutions seem correct, all 3 are passing locally.
ACK 8756ccd

ryanofsky

Code review ACK 8756ccd, just rebasing since last review and taking advantage of CScript constructors in #29369, also tweaking a code comment

stickies-v · 2024-08-29T09:43:31Z

Rebase re-ACK 8756ccd, no changes since a096215c9c71a2ec03e76f1fd0bcdda0727996e0

maflcko · 2024-09-04T10:34:50Z

macOS follow-up idea: Someone could check on macOS 13, if compilation with XCode still works. If not, build-osx.md could be updated (Ref: #30377 (comment))

edit: i guess this is already checked by CI?

DrahtBot added the Refactoring label Jul 2, 2024

DrahtBot mentioned this pull request Jul 2, 2024

Several randomness improvements #29625

Merged

hodlinator force-pushed the 2024-07_uint256S_consteval branch from d2e375d to a2c3d73 Compare July 2, 2024 19:10

DrahtBot added the Needs rebase label Jul 4, 2024

hodlinator marked this pull request as draft July 5, 2024 22:00

ryanofsky mentioned this pull request Jul 8, 2024

kernel: Streamline util library #29015

Merged

hodlinator force-pushed the 2024-07_uint256S_consteval branch from a2c3d73 to 67b1735 Compare July 12, 2024 09:03

hodlinator mentioned this pull request Jul 12, 2024

fix: Make TxidFromString() respect string_view length #30436

Merged

hodlinator force-pushed the 2024-07_uint256S_consteval branch from 67b1735 to 52f9666 Compare July 12, 2024 09:16

DrahtBot removed the Needs rebase label Jul 12, 2024

hodlinator force-pushed the 2024-07_uint256S_consteval branch from 52f9666 to afdd377 Compare July 12, 2024 10:10

hodlinator changed the title ~~refactor: Make uint256S(const char*) consteval~~ refactor: Make uint256S(const char*) and ParseHex(const char*) consteval Jul 12, 2024

DrahtBot mentioned this pull request Jul 12, 2024

test: Add a few more corner cases to the base58 test suite #30035

Closed

hodlinator force-pushed the 2024-07_uint256S_consteval branch from afdd377 to fb9bc91 Compare July 12, 2024 21:53

DrahtBot mentioned this pull request Jul 13, 2024

rest: Reject negative outpoint index early in getutxos parsing #30444

Merged

hodlinator force-pushed the 2024-07_uint256S_consteval branch 4 times, most recently from afbf8f9 to 92b0fb0 Compare July 16, 2024 21:51

DrahtBot added CI failed and removed CI failed labels Jul 16, 2024

maflcko reviewed Jul 17, 2024

View reviewed changes

Comment thread src/uint256.h Outdated

Comment thread src/uint256.h Outdated

Comment thread src/uint256.h Outdated

Comment thread src/uint256.h Outdated

Comment thread src/uint256.h Outdated

maflcko reviewed Jul 17, 2024

View reviewed changes

Comment thread src/util/strencodings.h Outdated

l0rinc reviewed Aug 24, 2024

View reviewed changes

Comment thread src/util/strencodings.h Outdated

ryanofsky approved these changes Aug 26, 2024

View reviewed changes

stickies-v approved these changes Aug 28, 2024

View reviewed changes

Comment thread src/util/strencodings.h Outdated

Comment thread src/util/strencodings.h Outdated

Comment thread src/util/strencodings.h Outdated

hodlinator and others added 10 commits August 28, 2024 19:09

refactor: Enforce lowercase hex digits for consteval uint256

7e1d9a8

Also changes compile-time asserts with comments into throws.

refactor: Improve CCrypter related lines

d99c816

Lines will be touched in next 2 commits.

refactor: de-Hungarianize CCrypter

bd0830b

Beyond renaming it also adjusts whitespace and adds braces to conform to current doc/developer-notes.md. TestEncrypt: Change iterator type to auto in ahead of vector -> span conversion. Only touches functions that will be modified in next commit.

refactor: vector -> span in CCrypter

403d86f

TestEncryptSingle: Remove no longer needed plaintext2-variable that existed because vectors had different allocators.

refactor: Make XOnlyPubKey tolerate constexpr std::arrays

2b5e6ef

Length was already asserted inside of base_blob-ctor.

ryanofsky approved these changes Aug 29, 2024

View reviewed changes

DrahtBot mentioned this pull request Aug 29, 2024

test: support std::optional in BOOST_CHECK_* and increase FromUserHex fuzz feature coverage #30618

Merged

l0rinc mentioned this pull request Aug 30, 2024

refactor: Allow CScript's operator<< to accept spans, not just vectors #30765

Merged

DrahtBot mentioned this pull request Aug 30, 2024

Remove the legacy wallet and BDB dependency #28710

Merged

stickies-v mentioned this pull request Sep 2, 2024

Remove unsafe uint256S() and test-only uint160S() #30773

Merged

ryanofsky mentioned this pull request Sep 4, 2024

contrib: fix check-deps.sh to check for weak symbols #30415

Merged

ryanofsky mentioned this pull request Dec 6, 2024

refactor: Add util::Result failure types and ability to merge result values #25665

Open

hebasto mentioned this pull request Dec 17, 2024

Compile error with GCC 14.2 and -D_GLIBCXX_DEBUG #31515

Closed

l0rinc mentioned this pull request Mar 9, 2025

RFC: Add operator""_uint256 compile-time user-defined literal #31991

Closed

hodlinator mentioned this pull request Apr 8, 2025

[IBD] batch block reads/writes during AutoFile serialization #31551

Merged

Conversation

hodlinator commented Jul 2, 2024 • edited Loading Uh oh! There was an error while loading. Please reload this page.

Uh oh!

Uh oh!

DrahtBot commented Jul 2, 2024 • edited Loading Uh oh! There was an error while loading. Please reload this page.

Uh oh!

Code Coverage

Reviews

Conflicts

Uh oh!

maflcko commented Jul 4, 2024

Uh oh!

hodlinator commented Jul 5, 2024

Uh oh!

maflcko commented Jul 8, 2024

Uh oh!

ryanofsky commented Jul 8, 2024

Uh oh!

hodlinator commented Jul 12, 2024

Uh oh!

DrahtBot commented Jul 16, 2024

Uh oh!

Uh oh!

Uh oh!

Uh oh!

Uh oh!

Uh oh!

Uh oh!

Uh oh!

l0rinc commented Aug 24, 2024 • edited Loading Uh oh! There was an error while loading. Please reload this page.

Uh oh!

Uh oh!

ryanofsky left a comment

Choose a reason for hiding this comment

Uh oh!

hodlinator commented Aug 28, 2024 • edited Loading Uh oh! There was an error while loading. Please reload this page.

Uh oh!

Uh oh!

stickies-v left a comment

Choose a reason for hiding this comment

Uh oh!

Uh oh!

Uh oh!

Uh oh!

stickies-v commented Aug 28, 2024

Uh oh!

l0rinc commented Aug 28, 2024

Uh oh!

l0rinc commented Aug 28, 2024

Uh oh!

ryanofsky left a comment

Choose a reason for hiding this comment

Uh oh!

stickies-v commented Aug 29, 2024

Uh oh!

maflcko commented Sep 4, 2024 • edited Loading Uh oh! There was an error while loading. Please reload this page.

Uh oh!

Uh oh!

Reviewers

Assignees

Labels

Projects

Milestone

Development

Uh oh!

9 participants

hodlinator commented Jul 2, 2024 •

edited

Loading

DrahtBot commented Jul 2, 2024 •

edited

Loading

l0rinc commented Aug 24, 2024 •

edited

Loading

hodlinator commented Aug 28, 2024 •

edited

Loading

maflcko commented Sep 4, 2024 •

edited

Loading