refactor: Replace ParseHex with consteval “"_hex literals #30377

not sure about overloading this. Otherwise, one may get a runtime evaluation when switching from a raw string literal pointer to a consteval string_view, no?

Seems clearer to make it a constructor taking a string_view? I guess the only confusion could be that the Span<const unsigned char> and string_view (aka Span<const char>) do different things (one is hex decoding and the other is not), but that should be fine, because both are distinct types.

Thanks for taking a closer look!

I want to enforce consteval for existing string literals. Ideally the string_view overload should be constexpr but from my research on MSVC assembly output in the PR summary it seems to handle lookup tables poorly. One possibility would be to remove the lookup table and use if (c >= '0' && c <= '9'), sacrificing some performance, but we don’t often parse ASCII hex strings in runtime anyway, right?

The main purpose of overloading uint256S() with a consteval version was to avoid changing all the call-sites. If we are okay with changing call-sites, it might be better to introduce a uint256(uint64_t,uint64_t,uint64_t,uint64_t) constructor. That way it would be the compiler parsing hexadecimal integer literals directly. See my raw array (54e0213c9c7a7a942dd320264c1f9224c494b777) and span (bfc2fb049f2a12e9936b938fb073738babac0cbb) explorations.

uint256(uint64_t,uint64_t,uint64_t,uint64_t) constructor. That way it would be the compiler parsing hexadecimal integer literals directly.

Not sure. That’d make it impossible to grep for a (let’s say) block hash. Also, it would be harder to copy-paste one, if the developer has to manually split it into 4 parts and add 0x prefixes. Finally, truncation checks can’t be done, see #30377 (review)

was to avoid changing all the call-sites.

How many are there? Should be trivial to write a scripted-diff to replace uint256S(" with something else, no?

Edit: There are around 50 instances. Most of them in a single file:

0$ git grep 'uint256S("' | grep -v 'src/test/' | wc -l 
154
2$ git grep 'uint256S("' src/kernel/chainparams.cpp  | wc -l 
348

I want to enforce consteval for existing string literals. Ideally the string_view overload should be constexpr but from my research on MSVC assembly output

I think string_view can be used in a consteval context, no?

I want to enforce consteval for existing string literals. Ideally the string_view overload should be constexpr but from my research on MSVC assembly output

I think string_view can be used in a consteval context, no?

Yes, it’s the rest of that paragraph that poses some concern.

Ideally the string_view overload should be constexpr but from my research on MSVC assembly output

Please don’t be overly concerned about MSVC. We’ve had issues with it in the past, where it’s failed to optimize code properly, or we’ve had to write workarounds for it, when code was otherwise fine in Clang or GCC, i.e (https://github.com/bitcoin/bitcoin/pull/28657#discussion_r1360780446). We don’t use it as a release compiler (and never will), so if it’s failing to do X, that isn’t necessarily a blocker to making changes (if X is working correctly in Clang & GCC). MSVC can always be improved later, and shouldn’t prevent us from writing better code now.

As of 10baae11ecc376f2250d4a51d2bbfeea56c0a31d I have stopped overloading uint256S():

Changed from consteval uint256 uint256S(const char *str) -> consteval explicit uint256(const char (&str)[65]) and base_blob(std::string_view str) This means the string width is enforced at compile time. (The new parsing code is much more strict and also asserts on the length).

Applied uint256S -> uint256 conversion where applicable, removing “0x”-prefixes. TODO: if there is agreement on the current approach - create scripted diff commit.

Added TxidFromStringS() to mirror uint256S() for runtime use.

Clarified reason for introduction of FixedVec in commit message 10baae11ecc376f2250d4a51d2bbfeea56c0a31d.

What is the point of this? Seems odd to add space before compilation and then remove it during compilation.

Seems easier to not add the space in the first place and fail compilation when there is one?

Not sure about hardcoding this for every base_blob.

Seems easier to just implement it once for base_blob and then have it available for all?

Not sure about fuzzy decoding here. Accidentally truncating an uint256 and thus parsing it as something else seems dangerous.

The convenience of being able to truncate leading zeros of a hex-encoded base_blob are never used, are they? The two constants ZERO and ONE are constructed without hex-decoding, so this isn’t needed there, and I fail to see another use case right now.

It seems easier to just assert WIDTH.

Generally agree #30377 (review)

(Some call-sites do use “0x”-prefix now that I think of it, while others do not, but aligning that would probably be an easy diff to accept).

A more flexible alternative would be to just accept a string_view and rely on the length assert. However, I guess this prints a more confusing compile error.

Seems fine to change later to string_view, if this is needed.

However, I wonder if you can replace 65 by WIDTH+1?

Seems fine to change later to string_view, if this is needed.

However, I wonder if you can replace 65 by WIDTH+1?

Done now in 79921003ffc858ca4b47e0fb187ed83c1667bc27 along with added comment and more descriptive parameter name.

Also, I wonder if the three duplicate redirects can be replaced by a single uinsg base_blob::base_blob; (or similar) to import all constructors.

I guess that would only be possible if switching the uint256-ctor to string_view, which I’d rather hold off on for now.

Yes, that would be along the lines of uint256::FromHex() that you have in the works in #30482.

One thing that comes to mind now though is that uint256::FromHex() can return a failure state. Maybe it would be better to have a constructor for this consteval thing - rename FixedVec into ByteVec with an added consteval constructor.

I am a bit worried that putting the bytes directly onto the stack (or easily allowing the dev to do so) may lead to high stack usage in some code paths and stack overflow on some platforms that have tighter limits.

Maybe a better overall approach is to validate the hex string at compile time, but then parse into a (heap) vector at runtime?

I don’t think there are any performance concerns where hex parsing is used right now, so doing the parsing at runtime or compile time shouldn’t matter. The important thing is compile-time checking, to catch bugs before the code compiles.

Good point.. the compile-time parsed bytes would go into a data section in the binary instead of the hex string literal, which should :tm: make the binary smaller. But when the std::array containers are initialized during runtime they will take up stack space, instead of the former std::vector taking up heap-space.

Heap is usually slower than the stack, but if the vector was just allocated in the local function, the data should be “hot” anyway.

Not sure how to elegantly achieve compile time validation + runtime parsing.

Will ruminate on this, cheers!

Heap is usually slower than the stack, but if the vector was just allocated in the local function, the data should be “hot” anyway.

Right, and I think there is no performance critical path.

Not sure how to elegantly achieve compile time validation + runtime parsing.

Should be easy:

0
1struct ConstevalHexLiteral {
2  const char* const hex;
3  consteval ConstevalHexLiteral(const char* str) : hex{str} { assert(IsCHex(str)); }
4  consteval ConstevalHexLiteral(std::nullptr_t) = delete;
5};
6
7auto BytesFromHex(ConstevalHexLiteral hex) { return std::vector{*Assert(RuntimeParse(hex))}; }

Thanks! That is a piece of art. :)

(Think I would put any Assert()s and vector initialization inside of RuntimeParse()).

I am a bit worried that putting the bytes directly onto the stack (or easily allowing the dev to do so) may lead to high stack usage in some code paths and stack overflow on some platforms that have tighter limits.

Outside of tests & benchmarks there would currently only be 4 places where ParseHex() is replaced with BytesFromHex. One has static storage file scope, so wouldn’t affect the stack. The longest one is 336 chars, which becomes 168 bytes on the stack.

4 runtime places tentatively replaced with BytesFromHex

net_processing.cpp, the big one:

0// If the peer is old enough to have the old alert system, send it the final alert.
1if (greatest_common_version <= 70012) {
2    constexpr auto finalAlert{BytesFromHex("60010000000000000000000000ffffff7f00000000ffffff7ffeffff7f01ffffff7f00000000ffffff7f00ffffff7f002f555247454e543a20416c657274206b657920636f6d70726f6d697365642c2075706772616465207265717569726564004630440220653febd6410f470f6bae11cad19c48413becb1ac2c17f908fd0fd53bdc3abd5202206d0e9c96fe88d4a0f01ed9dedae2b6f9e00da94cad0fecaae66ecf689bf71b50")};
3    MakeAndPushMessage(pfrom, "alert", Span{finalAlert});
4}

0const CScript genesisOutputScript = CScript() << BytesFromHex("04678afdb0fe5548271967f1a67130b7105cd6a828e03909a67962e0ea1f61deb649f6bc3f4cef38c4f35504e51ec112de5c384df7ba0b8d578a4c702b6bf11d5f") << OP_CHECKSIG;

0bin = ToByteVector(BytesFromHex("512103ad5e0edad18cb1f0fc0d28a3d4f1f3e445640337489abb10404f2d1e086be430210359ef5021964fe22d6f8e05b2463c9540ce96883fe3b278760f048f5189f2e6c452ae"));

0constexpr XOnlyPubKey XOnlyPubKey::NUMS_H{BytesFromHex("50929b74c1a04954b78b4b6035e97a5e078a5a0f28ec96d547bfee9ace803ac0")};

For reference, src/leveldb/util/posix_logger.h has:

0    constexpr const int kStackBufferSize = 512;
1    char stack_buffer[kStackBufferSize];

(But it may be dead code, and would also just be part of a leaf in the call graph).

On my workstation I have an 8MB stack (50000x 160 bytes) according to:

0    struct rlimit rl;
1    int result = getrlimit(RLIMIT_STACK, &rl);
2    if (result == 0) {
3        LogInfo("Stack size: %ld", rl.rlim_cur);
4    } else {
5        LogInfo("getrlimit failed: %d", result);
6    }

This article gives 135168 bytes of stack size (~800x 168 bytes) for unnamed Raspberry Pi device in 2020: https://www.embeddedrelated.com/showarticle/1330.php

Yes, I was experimenting with std::is_constant_evaluated early on (don’t think it was discussed). If I remember correctly, the reason I abandoned it in favor of consteval overloads taking string literals was that I was never getting compile-time evaluation where I expected it. There’s also the divergence in behavior that runtime-functions may need to return optional or heap-allocated types, while compile time-functions can keep all error handling internal and need to return stack-allocated types.

If we were to go with compile time validation when possible, but always heap-allocation for ParseHex, I can see how it may be useful.

Outside of tests & benchmarks there would currently only be 4 places where ParseHex() is replaced with BytesFromHex.

Thanks for enumerating. Looking at them:

• finalAlert -> Shouldn’t matter much, either way (Span-serialization doesn’t care about the underlying type)
• genesisOutputScript -> This must be a vector and can not be an array, because array-serialization is different from vector-serialization
• ToByteVector -> Yeah, vector as well
• XOnlyPubKey -> Shouldn’t matter?

Again, up to you what you see is a better fit.

If you want to provide a function for arrays and another for vectors, it seems fine as well.

genesisOutputScript -> This must be a vector and can not be an array, because array-serialization is different from vector-serialization

Thanks for pointing that difference out, already ran into some other issues with local changes in test code regarding this. Seems a bit brittle that vectors serialize with the size prefixed while Spans don’t, as vectors can implicitly convert to Spans.

This difference in behavior affects DataStream and possibly other types using serialize.h, but as far as I see CScript only has CScript& operator<<(const std::vector<unsigned char>& b).

I am considering proposing the CScript method take std::span instead. Either way I think genesisOutputScript should still be okay.

Seems a bit brittle that vectors serialize with the size prefixed while Spans don’t, as vectors can implicitly convert to Spans.

As for serialize code, an implicit conversion should not happen. If it does anywhere, then that is a bug and should be fixed.

I am considering proposing the CScript method take std::span instead. Either way I think genesisOutputScript should still be okay.

I am not sure if it is beneficial to have cscript-serialization differ from streams-serialization here. In any case, it shouldn’t be required for the changes here, or otherwise touch the same lines of code as the changes here?

Went with ArrayFromBytes for all in the latest tip ~d6d7a0b5221935518d2797aec7abc5c9632cbf68~ 240bc10e3f5f90fcc386b1a72ee2067a156abff3.

~Avoided adding a public CScript::operator<<(std::span) and went for a private method + public std::array overload.~ (Latest doesn’t touch script.h-header).

Have a variant with consteval validation in VecFromHex-branch, but didn’t feel it was urgent to use in any of the cases, so currently dropped from this PR: https://github.com/hodlinator/bitcoin/blob/2024-07/uint256S_consteval_VecFromHex/src/util/strencodings.h#L95-L110

In commit “refactor: Add consteval ArrayFromHex()” (00dc16bc29124dfbaf9d47c5411e4ebcf95e30b5)

Is this necessary? I’d be concerned adding such a generic top-level operator might cause this to get called in places we are not expecting, and maybe lead to hard to explain behavior or bugs. Would be good to at least have a comment explaining what this is intended to be used for.

In commit “refactor: Add consteval ArrayFromHex()” (00dc16bc29124dfbaf9d47c5411e4ebcf95e30b5)

This warning seems unnecessarily scary and maybe not true. I don’t think the function itself would use any stack space with NRVO, and callers could be calling this to initialize an array on the heap not the stack. Would suggest deleting this comment, because it seems like a warning about std::array in general, and I don’t think there is anything that really distinguishes this function from other functions using std::array.

This is in response to #30377 (review)

To summarize - the concern is not stack memory being used inside ArrayFromBytes itself, but rather that the std::array returned contains an inner C-array (https://github.com/gcc-mirror/gcc/blob/61715e9340ab8941d40d62158fe437e9dbe3e068/libstdc%2B%2B-v3/include/std/array) that is not allocated from the heap and so will bump the stack pointer in the calling function by a fair bit for long hex strings.

Should clarify the comment a bit on next re-touch.

re: #30377 (review)

The comment seems not true and not helpful.

• If the concern is not stack space used inside the function, and this warning is only being added because the return type of the function is a variable-sized std::array, then I’m not sure why every function that returns a variable-sized std::array shouldn’t have the same warning. This actually seems less deserving of a warning than other functions returning a std::array object, because the size of the returned array is not determined by a number passed by the caller, but by a literal string passed by the caller, and is half the size of that string, so pretty much guaranteed to be small.

• It is also inaccurate to say std::array is a stack based container or that calling this function will use stack space to store the array. This function can be called to initialize not just stack objects, but also global and heap objects due to NRVO, without being allocated on or copied from the stack.

New version of the comment in latest push:

0 * [@warning](/bitcoin-bitcoin/contributor/warning/) Unlike VecFromHex which returns a vector, the returned array may use
1 *          significant stack space if called inside a function.

re: #30377 (review)

New version of the comment in latest push:

Thank you! That seems accurate now.

In commit “refactor: Make code more tolerant of constexpr std::arrays” (05c16ae8649ee3b6bbcdbdf0541a65ccf4477537)

What is this referring to?

I was running into an issue with CScript (inheriting prevector) and std::arrays, similar to here: https://gitlab.freedesktop.org/pipewire/media-session/-/issues/4

Reverted this change in the latest push as I’m no longer modifying CScript. (Was running into MSVC CI-failures stemming from inability to infer N-parameter in constructor taking std::array<unsigned char, N>::const_iterator).

Nice!

Alternatively, since we claim to be testing whitespace support - and we’ve already tested that non-whitespace values are parsed correctly -, we might as well compare ParseHex("12 34 56 78") to ParseHex("12345678") instead - otherwise all tests would fail all the time when an error is introduced, this way only the appropriate tests would fail (+ testing is simpler).

Can me modernized/simplified:

0    std::vector<unsigned char> expected(std::begin(ParseHex_expected), std::end(ParseHex_expected));

(nit: weird ParseHex_expected naming)

• x - 1 is odd means x is even
• digits is plural, are required seems better

0    if (Size % 2 == 0) throw "2 hex digits per byte are required";

or

0    if (Size % 2 == 0) throw "2 hex digits required per byte";

we’ve checked already that Size is always odd, it seems to me that we can safely truncate:

0    std::array<Byte, Size  / 2> rv{};

and

0std::array<Byte, Size / 2>

in the signature

My original comment was:

Q: what the purpose of the trailing backslash, is it an automatic formatter trick?

But this answers it, thanks

Updated with slightly different string in latest push and removed comment about consistency above as well.

0throw "Only lowercase hex digits are allowed, for consistency";

Responding to https://github.com/bitcoin/bitcoin/pull/30377/files#r1716678159:

Thanks, better get this part right. :)

In commit “refactor: Hand-replace some ParseHex -> ArrayFromHex + VecFromHex” (8ad7e7d3fdfed4b126859dd83ab81cf8a8ae82a9)

It’s not obvious why an extra ToByteVector conversion is necessary here and why ArrayFromHex is not sufficient. Does CScript not accept std::array, or does it not accept uint8_t? I think this change is ok but it would be good to say in commit message what is happening here and how we could simplify this in the future, assuming it is something we should be able to clean up later.

style nit in 0a82c18457ec81e911b835b2ac76ad7475384983:

I know this has been mentioned before, but performance doesn’t matter here, so using just VecFromHex seems shorter and clearer?

It’s because part of my goal with this PR was to etch these bytes into the runtime binary. Things that can be done at compile time without too much hassle should be done at compile time IMO.

If we go the Vec(HexLiteral route at least it becomes shorter.

It’s because part of my goal with this PR was to etch these bytes into the runtime binary. Things that can be done at compile time without too much hassle should be done at compile time IMO.

If we go the Vec(HexLiteral route at least it becomes shorter.

Makes sense, thanks for looking into it!

style nit in 6a256b96af8c94804538eb9e78964557c032b74f: I think in tests it is more important that they are short, easy to read, and write. Compile time enforcements are neat at best, but shouldn’t be a goal. Generally, the unit tests are single threaded and fully deterministic, so if the test ran once (and passed), it should always pass.

So my preference would be to just call data = *Assert(TryParseHex(str)) inside ScriptFromHex.

This makes the diff smaller and also ensures that the same validation is done for ScriptFromHex that remain untouched in this pull.

Could we simplify some of these declarations (at least in tests) to e.g.

0    auto expected{ArrayFromHex("971a55")};

ToByteVector seems to be called in a few places, though mostly in tests.

I wonder if it makes sense to just go with https://github.com/hodlinator/bitcoin/blob/2024-07/uint256S_consteval_VecFromHex/src/util/strencodings.h#L95-L110

Or maybe we could change the CScript& operator<< to accept std::span instead:

 0diff --git a/src/script/script.h b/src/script/script.h
 1--- a/src/script/script.h	(revision 8509f5ade3ad7e1f1a727fe027483faaf2f2d4fe)
 2+++ b/src/script/script.h	(date 1723634083423)
 3@@ -463,7 +463,7 @@
 4         return *this;
 5     }
 6 
 7-    CScript& operator<<(const std::vector<unsigned char>& b) LIFETIMEBOUND
 8+    CScript& operator<<(const std::span<const unsigned char>& b) LIFETIMEBOUND
 9     {
10         if (b.size() < OP_PUSHDATA1)
11         {

Edit: these would work for CScript() << OP_RETURN << ArrayFromHex("") only (eagerly merged my comment with @maflcko’s, but it wasn’t a perfect match)

ScriptFromHex failed for empty inputs.

How come? ParseHex("") returns 0 as verified by util_tests.cpp. Besides that, I think it’s not really what we are testing in this file.

Do we even need the vector conversion here?

0    return CScript(data.begin(), data.end());

Clang and GCC don’t, but MSVC does. :(

Removed this function in latest push.

Checked that ScriptFromHex is only used for non-static values now 👍

Though I’m not sure ToScript(ArrayFromHex("0302ff03")) is more readable (or performant) than ScriptFromHex("0302ff03".

nit: while touching, might be worth updating to

0        assert(bytes.size() == m_keydata.size());

Further changed to just:

0constexpr explicit XOnlyPubKey(Span<const unsigned char> bytes) : m_keydata{bytes} {}

Since base_blob(Span<const unsigned char>) does the exact length assert internally.

I’m unable to compile 09458eadc9a4484ba37a70d1b378ed3f3c9e31d0 using:

0% clang --version
1Homebrew clang version 17.0.6
2Target: arm64-apple-darwin23.3.0
3Thread model: posix
4InstalledDir: /opt/homebrew/opt/llvm/bin

 0git clean -xdf && git checkout 09458eadc9a4484ba37a70d1b378ed3f3c9e31d0 && ./autogen.sh && ./configure --without-gui && make -j 7
 1
 2./uint256.h:131:19: error: call to consteval function 'ConstevalHexDigit' is not a constant expression
 3        auto lo = ConstevalHexDigit(*(str_it++));
 4                  ^
 5./uint256.h:173:60: note: in instantiation of member function 'base_blob<256>::base_blob' requested here
 6    consteval explicit uint256(std::string_view hex_str) : base_blob<256>(hex_str) {}
 7                                                           ^
 8/Library/Developer/CommandLineTools/SDKs/MacOSX14.0.sdk/usr/include/c++/v1/__iterator/reverse_iterator.h:168:56: note: read of non-constexpr variable 'str_it' is not allowed in a constant expression
 9    reverse_iterator operator++(int) {reverse_iterator __tmp(*this); --current; return __tmp;}
10                                                       ^
11/Library/Developer/CommandLineTools/SDKs/MacOSX14.0.sdk/usr/include/c++/v1/__iterator/reverse_iterator.h:168:56: note: in call to 'reverse_iterator(str_it)'
12./uint256.h:131:39: note: in call to '&str_it->operator++(0)'
13        auto lo = ConstevalHexDigit(*(str_it++));
14                                      ^
15./uint256.h:129:10: note: declared here
16    auto str_it = hex_str.rbegin();

What is your macOS and XCode version? IIRC Xcode 14 is no longer supported, see also #29934, but I think this isn’t documented, nor tested, so probably up for debate.

The CI passes, because it is using Xcode 15, see d742be3d3f5d5063d7160f72422bce2fec953f38

It was working for me with:

0 % clang --version
1Homebrew clang version 18.1.8
2Target: arm64-apple-darwin23.5.0
3Thread model: posix
4InstalledDir: /opt/homebrew/opt/llvm/bin
5
6 % xcodebuild -version
7Xcode 15.4
8Build version 15F31d

Responding to https://github.com/bitcoin/bitcoin/pull/30377/files#r1716752262:

Weird, actually works for me on same clang version as @stickies-v, but under Linux:

0$ clang --version
1clang version 17.0.6
2Target: x86_64-unknown-linux-gnu

Maybe it’s the standard library that is somehow different (missing some constexpr)?

What is your macOS and XCode version? IIRC Xcode 14 is no longer supported, see also #29934, but I think this isn’t documented, nor tested, so probably up for debate.

Thanks, I’m on macOS 14.3.1 and just bumped XCode from 14.3.1.0.1.1683849156 to 15.3.0.0.1.1708646388 which resolves the issue 👍

So I guess this commit drops support for XCode on macOS Ventura 13. Not sure if doc/build-osx.md needs to be adjusted, similar to https://github.com/bitcoin/bitcoin/pull/29934/files

According to my earlier (pending and therefore out of order) message, #30377 (review), the clang version is not the issue. So simply changing to llvm@18 in the docs might not be correct.

What is your clang version after the XCode upgrade @stickies-v?

Correct, the issue is not the clang version, but the stdlib version. MacOSX14.0.sdk does seem to have an issue in the iterators implementation. The minimum clang (and libc++) version is documented in doc/dependencies.md to be clang-16.

However, Apple somehow ships a completely separately versioned clang and stdlib with Xcode. Apples Xcode 14 stdlib seems to be no longer supported after this commit.

Given that Xcode 15 dropped support for macOS Ventura 13, made me leave the previous comment #30377 (review) about possibly extending the commit to mention that.

Again, I don’t use Apple, so I don’t care, but I wanted to mention it.

Testing consteval functions is quite constrained, but I think it still might be useful to add some happy path test cases to util_tests.cpp?

0BOOST_AUTO_TEST_CASE(consteval_hex_digit)
1{
2    BOOST_CHECK_EQUAL(ConstevalHexDigit('0'), 0);
3    BOOST_CHECK_EQUAL(ConstevalHexDigit('9'), 9);
4    BOOST_CHECK_EQUAL(ConstevalHexDigit('a'), 0xa);
5    BOOST_CHECK_EQUAL(ConstevalHexDigit('f'), 0xf);
6}

nit: The size requirement could also be expressed as a requires clause:

0consteval std::array<Byte, (Size - 1) / 2> ArrayFromHex(const char (&hex_str)[Size])
1    requires (Size % 2 == 1)

It provides more helpful information on why the constraints aren’t met, e.g.:

0net_processing.cpp:3934:39: error: no matching function for call to 'ArrayFromHex'
1            constexpr auto finalAlert{ArrayFromHex("60010000000000000000000000ffffff7f00000000ffffff7ffeffff7f01ffffff7f00000000ffffff7f00ffffff7f002f555247454e543a20416c657274206b657920636f6d70726f6d697365642c2075706772616465207265717569726564004630440220653febd6410f470f6bae11cad19c48413becb1ac2c17f908fd0fd53bdc3abd5202206d0e9c96fe88d4a0f01ed9dedae2b6f9e00da94cad0fecaae66ecf689bf71b50")};
2                                      ^~~~~~~~~~~~
3./util/strencodings.h:88:44: note: candidate template ignored: constraints not satisfied [with Byte = unsigned char, Size = 337]
4consteval std::array<Byte, (Size - 1) / 2> ArrayFromHex(const char (&hex_str)[Size])    
5                                           ^
6./util/strencodings.h:89:15: note: because '337UL % 2 == 0' (1 == 0) evaluated to false
7    requires (Size % 2 == 0)

In commit “test refactor: util_tests - Use BOOST_CHECK_EQUAL_COLLECTIONS()” (c6e1d5bff44b7d382a5a91b6722743b5fe07a95d)

Might be worth mentioning this change in commit message or saying it has other test cleanups.

re: #30377 (review)

In the PR summary you mean?

I do actually mean the commit message. The current commit message ddd06a0ec0a3b2c1d128fe5986a1363e7cf8e365 makes it sounds like it only switching to BOOST_CHECK_EQUAL_COLLECTIONS, but in reality is doing that and also making a seemingly unrelated test cleanup. Would suggest mentioning the other cleanup in the commit message, so for example, if someone is debugging something and wants to see if this commit is relevant, they can judge that based on the commit message.

In commit “refactor: Add consteval ArrayFromHex and VecFromHex” (b997d266e549ad6745d01a696cd9fbf1d3525944)

It seems like this is assuming the last character in the string is \0 and then ignoring it? I guess that is good because it makes this convenient to call, but in theory it means somebody could call this with a character array that is does not end in \0 and then the last character of the string would be ignored.

Would suggest changing this to:

0size_t size{hex_str[Size-1] == '\0' ? Size-1 : Size};
1if (size % 2 != 0) throw "2 hex digits required per byte";

to work correctly in either case and actually verify that the last character is \0 if an odd-size array is passed.

Changing it to a requires-clause as per https://github.com/bitcoin/bitcoin/pull/30377/files#r1717324499 with additional check for null terminator in the function body. Don’t think someone would do:

0constexpr char my_hex[] = {'f', 'f'};
1constexpr std::array<uint8_t, 1> arr = ArrayFromHex(my_hex);

when they could just do:

0constexpr std::array<uint8_t, 1> arr = {0xff};

re: #30377 (review)

Do we really need to support both?

It seems easier to support both than to add an error message explaining that the last byte has to be null, when there is no particular reason it needs to be null. But mainly I didn’t think the last byte should be silently ignored, so at least that problem is fixed now.

Continued in #30377 (review)

Resolving this one for now.

Since it’s a consteval ctor, I think taking a const char* parameter should behave the same but avoid having to instantiate for each differently sized hex_str input? But I’m still easily confused by how consteval behaves.

0   consteval ConstevalHexLiteral(const char* hex_str) : inner{hex_str} { Validate(inner); }

That’s strange, I don’t see how a string literal would need a conversion when there’s a const char* ctor? It seems to work fine for ConstevalStringLiteral too, so pardon my insistence but I think perhaps this compiler error might be because of another issue? Would you mind trying this diff on 734ac5a9002493013c0f8afe763f751ac99f89c8?

 0diff --git a/src/util/strencodings.h b/src/util/strencodings.h
 1index fbd116c77c..63f46396d3 100644
 2--- a/src/util/strencodings.h
 3+++ b/src/util/strencodings.h
 4@@ -80,8 +80,7 @@ consteval uint8_t ConstevalHexDigit(const char c)
 5 
 6 struct ConstevalHexLiteral {
 7     const std::string_view inner;
 8-    template <size_t N>
 9-    consteval ConstevalHexLiteral(const char (&hex_str)[N]) : ConstevalHexLiteral{std::string_view{hex_str}} {}
10+    consteval ConstevalHexLiteral(const char* hex_str) : ConstevalHexLiteral{std::string_view{hex_str}} {}
11     consteval ConstevalHexLiteral(const std::string_view hex_str) : inner{hex_str} { Validate(inner); }
12     consteval ConstevalHexLiteral(std::nullptr_t) = delete;
13 

leftover?

0        const CScript testnet4_genesis_script = CScript() << VecFromHex("000000000000000000000000000000000000000000000000000000000000000000") << OP_CHECKSIG;

I think *FromHex jives well with uint256::FromHex().. only thing is the latter stores the bytes in reverse order. :face_in_clouds:

Could go for VectorFromHex, but there is already some weak precedence in the type VecDeque.

super-nit:

0        if (hex_str.size() % 2) throw "2 hex digits required per byte";

re: #30377 (review)

On second thought, while this warning message is more accurate, I don’t think it is clear, because it is implying that return value will use stack space, when we should not expect that to be the case normally. Would suggest:

• It may be preferable to call VecFromHex instead of this function to save stack space, or to declare the returned constexpr to avoid using stack space, if returned array is large and being used to initialize a local variable.

In commit “refactor: Add consteval ArrayFromHex and VecFromHex” (b2fbe40cdf08b56d98e38ce261bfeeaf23b5f795)

I can’t see a benefit to defining a string_view constructor here and depending on string_view when this class is only supposed to accept null terminated string literals. This class could be simplified by dropping this constructor, dropping the string_view member and just declaring a const char* member.

In commit “refactor: Add consteval ArrayFromHex and VecFromHex” (b2fbe40cdf08b56d98e38ce261bfeeaf23b5f795)

This will do a weird thing when passed a non-null terminated string and give an error about an out of bounds array access (“error: array subscript value ‘2’ is outside the bounds of array ‘hex’ of type ‘const char [2]’”) for:

0constexpr char hex[] = {'a', 'b'};
1VecFromHex(hex);

This could be simplified and made less confusing by not requiring character arrays to be null terminated.

As mentioned in #30377 (review), who would do that?

Someone could theoretically do

0constexpr char hex[] = {'a', 'b', '\0', '\0', '\0'};
1VecFromHex(hex);

and have your version fail to compile.

In commit “refactor: Add consteval ArrayFromHex and VecFromHex” (b2fbe40cdf08b56d98e38ce261bfeeaf23b5f795)

I don’t think it makes sense to have a class that check a hex string at compile time, store the literal hex string in the binary, and then parse the hex string again. It makes the implementation unnecessarily complicated because code loops over the hex string ConstevalHexLiteral 3 different places instead of 1, duplicates checks like throw "2 hex digits required per byte" twice, and defines an extra class to implement an implicit conversion all for the benefit of storing hex instead of binary data in the compiled code and doing unnecessary parsing at runtime. It don’t think there is a justification for this and would recommend going for the alternate approach suggested above using util::HexLiteral and util::Vec functions.

In commit “refactor: Add consteval ArrayFromHex and VecFromHex” (b2fbe40cdf08b56d98e38ce261bfeeaf23b5f795)

I think it would be safer and more explicit to use static_cast<Byte>(...) instead of functional cast Byte(...). Not actually sure about this particular situation, but in general there are unsafe conversions that C casts allow which static_cast does not allow.

style nit in 9c4aad3fe4cb74bb1d9afeece78e2dd6ae1ad08e: When renaming this, I’d suggest to use the “fully” correct naming style ciphertext (snake_case without type prefix), according to the style guide.

(An alternative would be to leave it completely untouched)

style nit in https://github.com/bitcoin/bitcoin/commit/b2fbe40cdf08b56d98e38ce261bfeeaf23b5f795:

I wonder if it makes sense to pick the default as Byte = std::byte now. Obviously the diff will be larger, but longer term it seems cleaner to converge to std::byte, so having that as default will make the future easier, no?

style nit in https://github.com/bitcoin/bitcoin/commit/0a82c18457ec81e911b835b2ac76ad7475384983:

Span and array serialization is the same, so in theory you could drop this call to Span.