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:

$ git grep 'uint256S("' | grep -v 'src/test/' | wc -l 
54
$ git grep 'uint256S("' src/kernel/chainparams.cpp  | wc -l 
48

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?

My first stab at this was much more strict but when I added tests I went for full compatibility with the runtime overload. I'm certainly open to making it less permissive though.

More strict as of 10baae11ecc376f2250d4a51d2bbfeea56c0a31d.

Same

Same (https://github.com/bitcoin/bitcoin/pull/30377#discussion_r1680963050)

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?

There is no uint160S() outside of uint256_tests.cpp and only uint256 and uint160 use base_blob so it felt alright having it here. If we abandon the uint256S()-overload approach I agree to change this.

consteval parsing now done inside base_blob(std::string_view) as of 10baae11ecc376f2250d4a51d2bbfeea56c0a31d.

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).

Fixed as of 10baae11ecc376f2250d4a51d2bbfeea56c0a31d.

Same here. Overloading ParseHex seems fragile, because a compile time string_view will be evaluate at runtime, while a string literal will be evaluated at compile time.

Still kept it as an overload as of 10baae11ecc376f2250d4a51d2bbfeea56c0a31d. If you prefer different names for runtime/compile time, which do you suggest?

Please add {, } for multiline-if, according to the dev notes.

Fixed as of 10baae11ecc376f2250d4a51d2bbfeea56c0a31d.

Why add a consteval overload here? Generally I am not a fan of adding test-only code (code that is only used in tests) to the real program. Performance or exe-size shouldn't matter for tests, unless it is significant. If you still want to add it to the tests, that is fine, but then please move it to the test code. But please change the function name of the test-only function to something else. It seems odd that test-only code forces real code to be renamed.

Good call, will fix.

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?

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.

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.

Not sure about these. It should fail compilation instead if non-hex is detected.

I think it's more acceptable to diverge on behavior if we call the consteval function something different, see #30377 (review)

Maybe BytesFromHex?

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.

Having problems inferring the size of the underlying std::array from the char-array size using the constructor approach. Looks like a free function is where it's at for this one. (And maybe going back to raw std::array for the container if we fail compilation on whitespace/invalid input).

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:

struct ConstevalHexLiteral {
  const char* const hex;
  consteval ConstevalHexLiteral(const char* str) : hex{str} { assert(IsCHex(str)); }
  consteval ConstevalHexLiteral(std::nullptr_t) = delete;
};

auto 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()).

Yeah, RuntimeParse is probably just TryParseHex, which returns a vector already, so you can drop the vector-move-constructor from my suggestion.

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.

<details> <summary>

4 runtime places tentatively replaced with BytesFromHex

</summary>

net_processing.cpp, the big one:

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

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

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

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

</details>

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

    constexpr const int kStackBufferSize = 512;
    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:

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

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

You can also use std::is_constant_evaluated to make a function behave differently inside constexpr and outside of it.

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?

(Feel free to resolve the discussion for now, given that you dropped those changes here for now)

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

nit: snake_case for new code: write_it, str_it, according to the dev notes.

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.

Agree, it turned out it could be worked around by switching to BOOST_CHECK_EQUAL_COLLECTIONS in key_tests.cpp.

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:

 * [@warning](/bitcoin-bitcoin/contributor/warning/) Unlike VecFromHex which returns a vector, the returned array may use
 *          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).

<3

nit: now that the code is available here, the comment bacame redundant, since it doesn't provide any info that the code doesn't already explain

Taken in latest push.

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).

Responding to https://github.com/bitcoin/bitcoin/pull/30377/files#r1716610850: I think it's more rigorous/strict to not use ParseHex on both left & right sides of the comparison.

By testing these transitively, we wouldn't test everything everywhere. But the latter can have the advantage of making mistakes really obvious :)

internal whitespaces are weird enough - but do we still encourage mixed casing?

Responding to https://github.com/bitcoin/bitcoin/pull/30377/files#r1716616576: Enough casing changes for one PR. :)

Can me modernized/simplified:

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

(nit: weird ParseHex_expected naming)

Taken in latest push.

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

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

or

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

Taken in latest push.

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

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

and

std::array<Byte, Size / 2>

in the signature

Taken in latest push, should add co-authorship on next re-touch.

Added 4 of you in b2fbe40cdf08b56d98e38ce261bfeeaf23b5f795, hopefully you don't mind the company. :) @maflcko let me know if you prefer a different identifier to make GitHub rendering prettier.

style nit in 29090eca423327353b474615d02ed7c3190e4a50: Can drop the trailing \, since this is a normal function and not a macro.

My original comment was:

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

But this answers it, thanks

nit in 9dab917088e83f627786ed5caafec859a3481b78: If you change the behavior, it also needs to adjust the error string. Now it should say: throw "Hex string must only contain lowercase hex chars";.

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

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

Checked manually to make sure this still produces the same CScript as before 👍

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.

Yeah :))

Seems related to: #30377 (review)

Now explained in commit message 0a82c18457ec81e911b835b2ac76ad7475384983

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.

Attempted to minimize the diff in latest push, but went with VecFromHex for string literals and introduced a new ScriptFromHexStr for the 2 runtime validated call sites.

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

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

Responding to https://github.com/bitcoin/bitcoin/pull/30377/files#r1716685019: I'll meet you half-way with constexpr auto. :)

I'll take what I can get

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:

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

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

Responding to https://github.com/bitcoin/bitcoin/pull/30377/files#r1716693711: @maflcko: Might introduce VecFromHex for tests but I like that this constant is fully compile time. @paplorinc: I have been trying out exactly that approach quite a bit, but was warned by maflcko that there may be lying dragons among the different serializations of vector/array/span/Span. Also had some annoying MSVC compiler errors when just trying to add an overload for std::array. So undid my changes to that class.

To keep previous behavior fully, we could we add a CheckedParseHex style validation here as well (since ArrayFromHex("") is theoretically accepted, while ScriptFromHex failed for empty inputs).

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.

in 29090eca423327353b474615d02ed7c3190e4a50: When touching all of those lines, I wonder if it makes sense to switch them to std::byte, but that will probably be more involved.

Responding to https://github.com/bitcoin/bitcoin/pull/30377/files#r1716703571: Tried to start switching to std::byte after your comment, but it quickly propagated to untouched files.

Do we even need the vector conversion here?

    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

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

Taken in latest push.

Further changed to just:

constexpr 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:

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

git clean -xdf && git checkout 09458eadc9a4484ba37a70d1b378ed3f3c9e31d0 && ./autogen.sh && ./configure --without-gui && make -j 7

./uint256.h:131:19: error: call to consteval function 'ConstevalHexDigit' is not a constant expression
        auto lo = ConstevalHexDigit(*(str_it++));
                  ^
./uint256.h:173:60: note: in instantiation of member function 'base_blob<256>::base_blob' requested here
    consteval explicit uint256(std::string_view hex_str) : base_blob<256>(hex_str) {}
                                                           ^
/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
    reverse_iterator operator++(int) {reverse_iterator __tmp(*this); --current; return __tmp;}
                                                       ^
/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)'
./uint256.h:131:39: note: in call to '&str_it->operator++(0)'
        auto lo = ConstevalHexDigit(*(str_it++));
                                      ^
./uint256.h:129:10: note: declared here
    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:

 % clang --version
Homebrew clang version 18.1.8
Target: arm64-apple-darwin23.5.0
Thread model: posix
InstalledDir: /opt/homebrew/opt/llvm/bin

 % xcodebuild -version
Xcode 15.4
Build 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:

$ clang --version
clang version 17.0.6
Target: 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

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.

nit: vch var name prefixes are annoying now that they're not vectors anymore. It'll increase the diff but I still think it might be worth modernizing the names here to avoid that inconsistency? (here + a bunch of other instances)

Done, slightly, vchCiphertext -> chCiphertext (already some precedence for ch-prefix). Also changed vchSalt -> salt in some places.

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?

BOOST_AUTO_TEST_CASE(consteval_hex_digit)
{
    BOOST_CHECK_EQUAL(ConstevalHexDigit('0'), 0);
    BOOST_CHECK_EQUAL(ConstevalHexDigit('9'), 9);
    BOOST_CHECK_EQUAL(ConstevalHexDigit('a'), 0xa);
    BOOST_CHECK_EQUAL(ConstevalHexDigit('f'), 0xf);
}

Now part of b2fbe40cdf08b56d98e38ce261bfeeaf23b5f795

nit: calling this non-iterator i would be more clear

Done.

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

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

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

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

This hex parsing duplication is a bit painful, will try to come up with a deduplicated alternative tomorrow

Done.

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.

In the PR summary you mean?

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:

size_t size{hex_str[Size-1] == '\0' ? Size-1 : Size};
if (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:

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

when they could just do:

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

Do we really need to support both?

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.

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

Clang unfortunately gives me: candidate function template not viable: no known conversion from 'const char[65]' to 'ConstevalHexLiteral' for 1st argument

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?

<details> <summary>git diff on 734ac5a900</summary>

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

</details>

Thank you for being persistent! Must have had other local changes interacting badly with it when I tried it. Got it working with only your diff, so if we end up resurrecting ConstevalHexLiteral I'll go with your version.

leftover?

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

Now explained in commit message 0a82c18457ec81e911b835b2ac76ad7475384983 as suggested in #30377 (review).

Nit: "vec" vs "array" (abbreviation vs full) -> can we either name them "VecFromHex" and "ArrFromHex" or rather "VectorFromHex" and "ArrayFromHex" (or even shorter: "HexToVector" and "HexToArray"?

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 (feel free to ignore): we might want to do the content checking after the size check, it's cheaper

Done in latest.

super-nit:

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

I'm burned by old MSVC warnings (probably not current) "implicit conversion to bool" so I'd rather not.

In other cases we're adding and subtracting booleans, but I'm fine with both, see: https://github.com/bitcoin/bitcoin/pull/30535/files#diff-09e6cf871236bf03d32cca9405837d9b7927690b2296a2de17c9be6ea0e75959R74

That's some next level terseness. Okay, will change if I re-touch.

    // Size should be odd due to implicit null terminator.

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.

The intention was that the string_view length which is used at runtime would have a higher chance of being calculated at compile time.

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:

constexpr char hex[] = {'a', 'b'};
VecFromHex(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

constexpr char hex[] = {'a', 'b', '\0', '\0', '\0'};
VecFromHex(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.

Thanks for finding this! Remains from battling the compiler.

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)

I agree. Would either use the fully correct naming style, or not do the rename at all.

Good suggestion, seems to work out well.

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?

See https://github.com/bitcoin/bitcoin/pull/30377/files#r1716903532

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.

I got lost around SerializeMany :), but agree, serialize.h does the std::array -> Span conversion internally.

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

Still not sure about this. The benefit seems limited to offer two functions that effectively do the same in tests. It is just extra stuff for test writers (and readers) to keep in mind, when reviewing or writing new tests. Just having one function (the already existing one) seems easier.

The benefit of compile-time enforcement in tests seems limited, because running this test is way faster than compiling it.

nit: this is closely tied to ParseHex_expected, if you modify again, consider extracting all 5 usages next to it.

To be clear, you are suggesting extracting the repeated string literal "04678afdb0fe5548271967f1a67130b7105c..." into a constexpr variable?

This was quite simple before, now it might be faster by a few nanos, but it's less readable. Given that HexStr was already tested properly, could we maybe go the other way and convert detsig to hex string and compare against the hard coded value, i.e.

    BOOST_CHECK_EQUAL(HexStr(detsig), "304402205dbbddda71772d95ce91cd2d14b592cfbc1dd0aabd6a394b6c2d377bbe59d31d022014ddda21494a4e221f0824f0b8b924c43fa43c0ad57dccdaa11f81a6bd4582f6");

Thank you!

If we insist that this is an important check, it seems a bit jumpy to do the check at the beginning, after which we iterate over the elements and arrive exactly at that element - and just silently skip it. Alternatively, we could check for \0 when we get there, after we've processed the rest of the chars, i.e.

    assert(!hex_str[i]);
    return rv;

I see what you mean and tried it out, but ended up preferring input validation at the top of the function.

btw, this is in the preprocessing phase of the benchmark, the performance doesn't matter here at all - only inside the .run( part, so I'd go with the cleanest code here, too (if this is it, just resolve the comment).

Yes, this change isn't to improve benchmark performance.

IMO the changed version is cleaner - the number of bytes to be converted is explicitly enforced in the type and can be connected to the calculation below.

In commit "refactor: add util::HexLiteral and util::Vec using statements" (ced7cec1c6d79159f8212d3a92a8f7583ef11884)

It seems like there is a problem with this on windows, looks like because begin and end are returning std::_Array_const_iterator types, instead of character pointers. Following change might fix it:

-    return {array.begin(), array.end()};
+    return {array.data(), array.data() + array.size()};

Error is:

https://github.com/bitcoin/bitcoin/actions/runs/10411271977/job/28834888600?pr=30377#step:20:2031

D:\a\bitcoin\bitcoin\src\test\script_tests.cpp(1367,12): error C2440: 'return': cannot convert from 'initializer list' to 'CScript' [D:\a\bitcoin\bitcoin\build_msvc\test_bitcoin\test_bitcoin.vcxproj]
  D:\a\bitcoin\bitcoin\src\test\script_tests.cpp(1367,5): note: 'CScript::CScript': no overloaded function could convert all the argument types
  D:\a\bitcoin\bitcoin\src\script\script.h(436,5): note: could be 'CScript::CScript(const unsigned char *,const unsigned char *)'
  D:\a\bitcoin\bitcoin\src\test\script_tests.cpp(1367,5): note: 'CScript::CScript(const unsigned char *,const unsigned char *)': cannot convert argument 1 from 'std::_Array_const_iterator<_Ty,4>' to 'const unsigned char *'
          with
          [
              _Ty=uint8_t
          ]
  D:\a\bitcoin\bitcoin\src\test\script_tests.cpp(1367,24): note: No user-defined-conversion operator available that can perform this conversion, or the operator cannot be called
  D:\a\bitcoin\bitcoin\src\script\script.h(435,5): note: or       'CScript::CScript(std::_Vector_const_iterator<std::_Vector_val<std::_Simple_types<_Ty>>>,std::_Vector_const_iterator<std::_Vector_val<std::_Simple_types<_Ty>>>)'
          with
          [
              _Ty=unsigned char
          ]
  D:\a\bitcoin\bitcoin\src\test\script_tests.cpp(1367,5): note: 'CScript::CScript(std::_Vector_const_iterator<std::_Vector_val<std::_Simple_types<_Ty>>>,std::_Vector_const_iterator<std::_Vector_val<std::_Simple_types<_Ty>>>)': cannot convert argument 1 from 'std::_Array_const_iterator<_Ty,4>' to 'std::_Vector_const_iterator<std::_Vector_val<std::_Simple_types<_Ty>>>'
          with
          [
              _Ty=unsigned char
          ]
          and
          [
              _Ty=uint8_t
          ]
          and
          [
              _Ty=unsigned char
          ]
  D:\a\bitcoin\bitcoin\src\test\script_tests.cpp(1367,24): note: No user-defined-conversion operator available that can perform this conversion, or the operator cannot be called
  D:\a\bitcoin\bitcoin\src\script\script.h(434,5): note: or       'CScript::CScript(prevector<28,unsigned char,uint32_t,int32_t>::const_iterator,prevector<28,unsigned char,uint32_t,int32_t>::const_iterator)'
  D:\a\bitcoin\bitcoin\src\test\script_tests.cpp(1367,5): note: 'CScript::CScript(prevector<28,unsigned char,uint32_t,int32_t>::const_iterator,prevector<28,unsigned char,uint32_t,int32_t>::const_iterator)': cannot convert argument 1 from 'std::_Array_const_iterator<_Ty,4>' to 'prevector<28,unsigned char,uint32_t,int32_t>::const_iterator'
          with
          [
              _Ty=uint8_t
          ]
  D:\a\bitcoin\bitcoin\src\test\script_tests.cpp(1367,24): note: No user-defined-conversion operator available that can perform this conversion, or the operator cannot be called
  D:\a\bitcoin\bitcoin\src\test\script_tests.cpp(1367,5): note: while trying to match the argument list '(std::_Array_const_iterator<_Ty,4>, std::_Array_const_iterator<_Ty,4>)'
          with
          [
              _Ty=uint8_t
          ]
  D:\a\bitcoin\bitcoin\src\test\script_tests.cpp(1367,5): note: the template instantiation context (the oldest one first) is
  D:\a\bitcoin\bitcoin\src\test\script_tests.cpp(1395,9): note: see reference to function template instantiation 'CScript script_tests::Script<4>(const std::array<uint8_t,4> &)' being compiled
  serfloat_tests.cpp
D:\a\bitcoin\bitcoin\src\test\script_tests.cpp(1367,12): error C2440: 'return': cannot convert from 'initializer list' to 'CScript' [D:\a\bitcoin\bitcoin\build_msvc\test_bitcoin\test_bitcoin.vcxproj]

Is #29369 related or possibly a fix?

Yes, #29369 looks like a possible fix. Applied @ryanofsky's workaround for now but will have a look at that PR.

style nit in 84c830f27fd62db4a9cb93bf6d28a86f7751e504: Could use std::byte as default? (See below for reasoning)

(Reply to #30377 (review))

I think your link should say #30377 (review)

This is a newly introduced function, so I think changing it should not extend to untouched files. To clarify, I am not saying that you should switch callers to use std::byte, only the default.

What I meant is that (for now) in this pull request you make the default choice std::byte for Byte and adjust the scripted-diff to use HexLiteral<uint8_t>. Obviously the diff will be <strike>larger</strike> more verbose, but longer term it seems cleaner to converge to std::byte. This means that the future change converging to std::byte will be cleaning up the code by being able to drop the <uint8_t> and fall back on the default. Also, using the explicit <uint8_t> makes it easier to spot places that still use the "legacy" description of bytes, and it acts as a sort-of "TODO-comment".

Also, "modern" places that require (or allow) std::byte today already, can simply use the function without having to specify that a Byte should mean std::byte.

This is in symmetry with the recently introduced TryParseHex, which also defaults to std::byte.

Ah, hadn't seen your #25227 before. It seems easier to default to std::byte when introducing TryParseHex without any prior callers. But thanks to your elaboration on the effort already under way to use std::byte, I did some more grepping and decided it might be worth attempting. Changed defaults in latest push 01e18d94d9577f415748869376988e3f0f59ced0 and worked through the fallout. The scripted-diff ended up with a lot less work in that version.

(The push right before it in 3a96b9f4b56c2ef47f41e9baa9ce36a268aa9440 applies the other more minor fixes without changing the default to std::byte).

Good to see that you were already thinking about compile-time versions in #25227 (comment).

nit in 1dddb4fb0ff88162e58e50e56d5e889bb5727a81: Any reason to remove the comment? Seems important to keep, otherwise a dev may be surprised when the function crashes when it isn't exactly 32 bytes.

Was removed it in response to #30377 (review) but then I went one step further and deferred the assert to the base_blob Span-ctor, making the comment more relevant again. Brought back now.

nit in https://github.com/bitcoin/bitcoin/commit/1dddb4fb0ff88162e58e50e56d5e889bb5727a81: Use std::span to avoid having to touch this again in the future for that reason?

Started going down the route of std::span<const unsigned char, 32> to enforce the constraint previously, got burnt and backed out a bit too far, std::span with dynamic extent and comment will do for this PR. :+1:

nit in https://github.com/bitcoin/bitcoin/commit/1dddb4fb0ff88162e58e50e56d5e889bb5727a81: Use the safe UCharCast while touching this, instead of the possibly unsafe pointer cast?

nit in https://github.com/bitcoin/bitcoin/commit/1dddb4fb0ff88162e58e50e56d5e889bb5727a81: Wrong rename?

Leftover from half-way rename, fixed here and function below.

nit in 82c9080f235f6ffbe983b1360dc5741ce95e9a7d: Follow-up nit: I think it was me who suggested to avoid array/span serialization in script, but I think it could be considered when the vector serialization is mirrored. I understand that this would be different from the serialize.h serialization, but the two are separate anyway (and script inherits the whole prevector functions to insert raw bytes at any point anyway).

In any case, this doesn't remove the need for Vec and can be done in a follow-up.

super-nit: during parsing we're checking now that the inputs contain an even number of hexadecimal digits, if you edit again, consider applying that to the comments as well (similarly to other comments, like 0x02ff03)

    s = ToScript(HexLiteral<uint8_t>("0302ff030302ff03")); // PUSH 0x02ff03 PUSH 0x02ff03

Given that

assert(Vec(HexLiteral<uint8_t>("01")) == valtype{1});
assert(Vec(HexLiteral<uint8_t>("ffffffffffffffffffffffffffffffffffffffffffffffffffffffffffffffffffffffff")) == valtype(36, 0xff));

consider simplifying these to e.g.

    t.vout[0].scriptPubKey = CScript() << OP_RETURN << 0 << valtype{1} << 2 << valtype(36, 0xff);

and many more, I think it would simplify a few trivial usages, hexadecimal encoding only makes sense for bigger values.

re: #30377 (review)

and many more, I think it would simplify a few trivial usages, hexadecimal encoding only makes sense for bigger values.

This would change which cscript overload is called, which could be good, but I think it would be better to do in a PR dedicated to improving cscript. That way this change can be focused on replacing runtime hex parsing with compile time parsing, and reviewers don't need to consider ways it is impacting test coverage of different CScript methods.

Both of them produce a vector, right? Edit:

• typedef std::vector<unsigned char> valtype
• std::vector<T> Vec(const std::array<T, N>& array)

i.e. both would call CScript& operator<<(const std::vector<unsigned char>& b) LIFETIMEBOUND, right?

re: #30377 (review)

Yes, you're right , I misread the suggestion. Using valtype to construct vectors would not change which CScript method is called, so it should be not a problem to make that change in this PR, and maybe it would be better. I do think tests would be more readable and consistent if they used HexLiteral as much as possible to represent raw bytes, but that's a subjective opinion and your approach seems fine too.

Yeah, when it came to short/empty hex strings I've been tempted to use something like std::vector{0xFF} but felt inconsistent. valtype is a good find, but doesn't provide so much of a win once we have full std::array<std::byte> support in CSscript: valtype{0xFF} vs HexLiteral("FF") valtype{} vs HexLiteral("") (I think switching to base 10 is or using valtype(36, 0xff); is too inconsistent). So keeping as-is for now.

once we have full std::arraystd::byte support in CSscript:

Valid point.

What about repeated values, which are already using the vector constructor, i.e.

valtype(36, 0xff)

instead of

Vec(HexLiteral<uint8_t>("ffffffffffffffffffffffffffffffffffffffffffffffffffffffffffffffffffffffff"))

What about repeated values, which are already using the vector constructor, i.e.

I'd say that there is more value in a test being consistent (using the same code patterns within a single unit test), than to use the shortest possible code. However that is a style question up to the author. Personally, I'd leave this line in this pull request as-is.

more value in a test being consistent

Agree, that's why I'm suggesting this, transaction_tests has many non-hexadecimal pushes.

What about repeated values, which are already using the vector constructor, i.e.

See my message before yours @paplorinc:

(I think switching to base 10 is or using valtype(36, 0xff); is too inconsistent).

valtype is not the point here, rather that for repeated values we've used the vector constructor. But we can close this comment, maybe we'll do it in another PR.

Yes. valtype was not my only point, it was also the repeated values.

But aren't we already doing that throughout the test, e.g. https://github.com/bitcoin/bitcoin/blob/master/src/test/transaction_tests.cpp#L1031?

Yes, we are. But I'd rather not change that aspect of this region in this PR. Arguably the current "flattened out" version is clearer for the casual reader who isn't interested in the exact number of bytes being repeated.

Ah, so it's just out of scope - I can work with that. :)

Is Vec needed here because the existing ToByteVector has a long name? Or because of some funny visual studio exception? If none, maybe we could get rid of either.

re: #30377 (review)

Is Vec needed here because the existing ToByteVector has a long name? Or because of some funny visual studio exception? If none, maybe we could get rid of either.

CScript only supports << with std::vector, not std::array. It would be natural for it support std::array too, but I think that would be better be done in a followup improving CScript so this PR can be focused on replacing runtime hex parsing with compile time parsing and not make sensitive changes to the cscript implementation.

I mean both Vec(HexLiteral( and the existing ToByteVector(HexLiteral( would work here, right?

re: #30377 (review)

I mean both Vec(HexLiteral( and the existing ToByteVector(HexLiteral( would work here, right?

Right, ToByteVector could be used here instead of Vec, and Vec was just chosen because it is shorter. Sorry I misunderstood your original question.

I do think in general Vec has some benefits over ToByteVector:

• Vec only changes the type of the container, not the type of the elements inside the container, so it seems preferable to use in cases like this where the internal type is not changing.

• The name ToByteVector seems not great because it returns a vector of unsigned char rather than a vector of std::byte. If intention is to be switched to std::byte later though then I think it is a good name.

But nice thing is neither of these should be neccessary if cscript supports std::array

But nice thing is neither of these should be necessary if cscript supports std::array

Would it be too early for me to provide a separate PR for that?

I would be happy if you took the ball on that. But would appreciate if you worked on it in a personal branch (still pushing to GitHub for CI) and waited a few days more with making it into a PR.

~6 pushes ago on this PR I had commit 5be34598c4683b2b44f607b28592a9e68e089761 which added targeted std::array support to script.h. It is conservative in that it doesn't publicly expose the possibility of appending raw std::span. IIRC MSVC had some issue with it, but could have been a different push.

I agree with Ryan, that this should be a follow-up PR, and that the changes in this line of the diff should probably be kept as-is.

I haven't seen https://github.com/bitcoin/bitcoin/commit/5be34598c4683b2b44f607b28592a9e68e089761 It seems to me there's a simpler solution than that, since the method contains two separate concerns (inserting size + inserting elements) - which are using different parts of the vector/array.

diff --git a/src/script/script.h b/src/script/script.h
--- a/src/script/script.h	(revision d5eed066d33780b12c1f0813a2adf021eac0ca5d)
+++ b/src/script/script.h	(date 1724057506103)
@@ -412,6 +412,34 @@
 /** Serialized script, used inside transaction inputs and outputs */
 class CScript : public CScriptBase
 {
+private:
+    void InsertSize(const size_t size)
+    {
+        if (size < OP_PUSHDATA1)
+        {
+            insert(end(), static_cast<unsigned char>(size));
+        }
+        else if (size <= 0xff)
+        {
+            insert(end(), OP_PUSHDATA1);
+            insert(end(), static_cast<unsigned char>(size));
+        }
+        else if (size <= 0xffff)
+        {
+            insert(end(), OP_PUSHDATA2);
+            uint8_t _data[2];
+            WriteLE16(_data, size);
+            insert(end(), _data, _data + sizeof(_data));
+        }
+        else
+        {
+            insert(end(), OP_PUSHDATA4);
+            uint8_t _data[4];
+            WriteLE32(_data, size);
+            insert(end(), _data, _data + sizeof(_data));
+        }
+    }
+
 protected:
     CScript& push_int64(int64_t n)
     {
@@ -465,29 +493,15 @@
 
     CScript& operator<<(const std::vector<unsigned char>& b) LIFETIMEBOUND
     {
-        if (b.size() < OP_PUSHDATA1)
-        {
-            insert(end(), (unsigned char)b.size());
-        }
-        else if (b.size() <= 0xff)
-        {
-            insert(end(), OP_PUSHDATA1);
-            insert(end(), (unsigned char)b.size());
-        }
-        else if (b.size() <= 0xffff)
-        {
-            insert(end(), OP_PUSHDATA2);
-            uint8_t _data[2];
-            WriteLE16(_data, b.size());
-            insert(end(), _data, _data + sizeof(_data));
-        }
-        else
-        {
-            insert(end(), OP_PUSHDATA4);
-            uint8_t _data[4];
-            WriteLE32(_data, b.size());
-            insert(end(), _data, _data + sizeof(_data));
-        }
+        InsertSize(b.size());
+        insert(end(), b.begin(), b.end());
+        return *this;
+    }
+
+    template<size_t N>
+    CScript& operator<<(const std::array<unsigned char, N>& b) LIFETIMEBOUND
+    {
+        InsertSize(N);
         insert(end(), b.begin(), b.end());
         return *this;
     }

i.e. for vector:

InsertSize(b.size());
insert(end(), b.begin(), b.end());

and for array:

InsertSize(N);
insert(end(), b.begin(), b.end());

that this should be a follow-up PR

I'm fine with both, I'm just providing alternatives.

I like that my version just forwards everything in one statement, while your leaves 3, duplicated. But either one works for me.

In my suggestion there is no std::span involved and the size + data writing parts are clearly separated. But I can also imagine a method accepting b.size(), b.begin(), b.end() vs N, b.begin(), b.end(), if you think that's cleaner.