refactor: Add consteval uint256 constructor

hodlinator commented at 1:26 pm on July 31, 2024: contributor

Motivation:

Validates and converts the hex string at compile time instead of at runtime into the resulting bytes.
Makes it possible to derive other compile time constants from uint256.
Potentially eliminates runtime dependencies (SetHexDeprecated() is called in less places).
Has stricter requirements than the deprecated uint256S() (requiring 64 chars exactly, disallows garbage at the end) and replaces it in a bunch of places.
Makes the binary smaller (tested Guix-built x86_64-linux-gnu bitcoind binary).
Minor: should shave off a few cycles of start-up time.

Extracted from #30377 which diverged into exploring consteval ParseHex() solutions.

DrahtBot commented at 1:26 pm on July 31, 2024: contributor

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

Code Coverage

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

Reviews

See the guideline for information on the review process.

Type	Reviewers
ACK	paplorinc, maflcko, stickies-v
Stale ACK	TheCharlatan

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

Conflicts

Reviewers, this pull request conflicts with the following ones:

#30526 (doc: Correct uint256 hex string endianness by hodlinator)
#30377 (refactor: Add consteval uint256{“str”} by hodlinator)
#29656 (chainparams: Change nChainTx type to uint64_t by fjahr)
#29553 (assumeutxo: Add dumptxoutset height param, remove shell scripts by fjahr)
#27433 (getblocktemplate improvements for segwit and sigops by Sjors)
#27427 (validation: Replace MinBIP9WarningHeight with MinBIP9WarningStartTime by dimitaracev)
#26966 (index: blockfilter initial sync speedup, parallelize process by furszy)
#26201 (Remove Taproot activation height by Sjors)
#24230 (indexes: Stop using node internal types and locking cs_main, improve sync logic by ryanofsky)

If you consider this pull request important, please also help to review the conflicting pull requests. Ideally, start with the one that should be merged first.

DrahtBot added the label Refactoring on Jul 31, 2024

maflcko commented at 1:44 pm on July 31, 2024: member

Eliminates runtime dependencies.

Is this true? IIRC it was true for ParseHex, because that returns a std::vector<std::byte> (and the module that was used to create the bytes could be dropped before linking. However, the uint256 type lives in the same module as the parsing code, so I don’t think this is true?

Has stricter requirements than the deprecated uint256S() and replaces it in a bunch of places.

It would be good to mention them: “Requires exactly 64 hex characters” (or so). Also, it could recap the risks of uint256S: “Previously, any invalid string was accepted silently.”?

in src/test/arith_uint256_tests.cpp:27 in 18011e7556 outdated

21@@ -22,7 +22,7 @@ static inline arith_uint256 arith_uint256V(const std::vector<unsigned char>& vch
22 {
23     return UintToArith256(uint256(vch));
24 }
25-static inline arith_uint256 arith_uint256S(const std::string& str) { return UintToArith256(uint256S(str)); }
26+static inline arith_uint256 arith_uint256S(std::string_view str) { return UintToArith256(uint256S(str)); }

maflcko commented at 1:50 pm on July 31, 2024:

style nit in 18011e7556af94e9fe3a8f0a1b0f58bbc1dd46e7: Not sure about this test-only change in a line that will be touched in the future anyway. uint256S doesn’t have any error feedback on invalid input, which will be done in a follow-up, or separate pull request.

hodlinator commented at 8:28 pm on July 31, 2024:

Keeping as is unless anyone feels stronger about this. Not big enough to separate out to own PR IMO.

maflcko commented at 6:53 am on August 1, 2024:

Ah, I didn’t say that the commit here should be split up into its own pull. I wanted to say that it can be dropped, because:

the first changed line will be changed in the future. (The remaining uint256S’s removal is a large-ish change overall, which is why it will be done in a follow-up)
the second changed line in this commit will either be removed, or changed in the same pull request that removes the remaining uint256S’s)

Generally, I find that if a line will soon be touched or removed anyway, applying style fixes to it seems useless churn.

But just a style nit, because it is test code anyway.

hodlinator commented at 1:03 pm on August 1, 2024:

Dropped in latest force-push. #30526 is the true follow-up change for #30436 and already contains a better version of it.

in src/test/uint256_tests.cpp:320 in 18011e7556 outdated

306@@ -307,7 +307,7 @@ BOOST_AUTO_TEST_CASE(parse)
307     {
308         std::string s_12{"0000000000000000000000000000000000000000000000000000000000000012"};
309         BOOST_CHECK_EQUAL(uint256S("12\0").GetHex(), s_12);
310-        BOOST_CHECK_EQUAL(uint256S(std::string{"12\0", 3}).GetHex(), s_12);
311+        BOOST_CHECK_EQUAL(uint256S(std::string_view{"12\0", 3}).GetHex(), s_12);

maflcko commented at 1:50 pm on July 31, 2024:

Same

hodlinator commented at 8:28 pm on July 31, 2024:

Same #30560 (review)

in src/test/uint256_tests.cpp:343 in df16ac11c1 outdated

335@@ -336,4 +336,13 @@ BOOST_AUTO_TEST_CASE( check_ONE )
336     BOOST_CHECK_EQUAL(one, uint256::ONE);
337 }
338 
339+BOOST_AUTO_TEST_CASE(check_uint256S_vs_uint256)
340+{
341+    constexpr uint256 consteval_uint{
342+           "4a5e1e4baab89f3a32518a88c31bc87f618F76673E2CC77AB2127B7AFDEDA33B"};
343+    const uint256 runtime_uint = uint256S(std::string_view{

maflcko commented at 1:51 pm on July 31, 2024:

nit in df16ac11c1fcd9535bd56d71465c6dbea715a567: Seems odd to add a call to a deprecated function. Why not use uint256::FromHex?

hodlinator commented at 8:29 pm on July 31, 2024:

Thanks, shows that this change predates FromHex. :) Fixed!

in src/uint256.h:128 in df16ac11c1 outdated

92@@ -91,6 +93,27 @@ class base_blob
93     }
94 };
95 
96+template<unsigned int BITS>
97+consteval base_blob<BITS>::base_blob(std::string_view hex_str)
98+{
99+    // Non-lookup table version of HexDigit().

maflcko commented at 1:56 pm on July 31, 2024:

nit in df16ac11c1fcd9535bd56d71465c6dbea715a567: Why not make this a consteval std::byte hex_digit(char c) { ... } inline function in the same header? Otherwise, it will have to be re-implemented for ParseHexConseval, or other code that wants to use it?

hodlinator commented at 8:30 pm on July 31, 2024:

I’d rather hold off on breaking it out until this one is merged, not sure the ParseHexConsteval header will be including this one.

hodlinator commented at 1:57 pm on July 31, 2024: contributor

Eliminates runtime dependencies.

Is this true? IIRC it was true for ParseHex, because that returns a std::vector<std::byte> (and the module that was used to create the bytes could be dropped before linking. However, the uint256 type lives in the same module as the parsing code, so I don’t think this is true?

Your memory serves you right: #30377 (comment) However, this change eliminates some calls to the .cpp SetHexDeprecated() which has potential to decrease runtime dependencies in the future, even if it were not to happen right now. Adjusted summary.

Has stricter requirements than the deprecated uint256S() and replaces it in a bunch of places.

It would be good to mention them: “Requires exactly 64 hex characters” (or so). Also, it could recap the risks of uint256S: “Previously, any invalid string was accepted silently.”?

Cheers, adjusted summary somewhat.

in src/uint256.h:153 in df16ac11c1 outdated

149@@ -127,6 +150,7 @@ class uint160 : public base_blob<160> {
150 class uint256 : public base_blob<256> {
151 public:
152     static std::optional<uint256> FromHex(std::string_view str) { return detail::FromHex<uint256>(str); }
153+    consteval explicit uint256(std::string_view hex_str) : base_blob<256>(hex_str) {}

maflcko commented at 1:59 pm on July 31, 2024:

nit in https://github.com/bitcoin/bitcoin/commit/df16ac11c1fcd9535bd56d71465c6dbea715a567: Could add the same line for uint160

hodlinator commented at 8:32 pm on July 31, 2024:

Attempted doing:

0using base_blob<256>::base_blob;

and same for uint160.

But ran into GCC issues with inherited consteval constructors (didn’t try other compilers): https://gcc.gnu.org/bugzilla/show_bug.cgi?id=106084

Tried adding

0consteval explicit uint160(std::string_view hex_str)

And replacing

0    BOOST_CHECK_EQUAL(HexStr(cc1.out.scriptPubKey), HexStr(GetScriptForDestination(PKHash(uint160(ParseHex("816115944e077fe7c803cfa57f29b36bf87c1d35"))))));

with

0    BOOST_CHECK_EQUAL(HexStr(cc1.out.scriptPubKey), HexStr(GetScriptForDestination(PKHash(uint160{"816115944e077fe7c803cfa57f29b36bf87c1d35"})))));

But test failed because the bytes are reversed. So holding off on touching uint160.

maflcko commented at 7:38 am on August 1, 2024:

I just meant the following diff, which compiles and passes:

 0diff --git a/src/test/uint256_tests.cpp b/src/test/uint256_tests.cpp
 1index e1a7d7c5a9..dd354343d3 100644
 2--- a/src/test/uint256_tests.cpp
 3+++ b/src/test/uint256_tests.cpp
 4@@ -105,6 +105,7 @@ BOOST_AUTO_TEST_CASE( basics ) // constructors, equality, inequality
 5     BOOST_CHECK_EQUAL(uint256(OneL), OneL);
 6 
 7     BOOST_CHECK_EQUAL(uint160S("0x"+R1S.ToString()), R1S);
 8+    BOOST_CHECK_EQUAL(uint160{"7D1DE5EAF9B156D53208F033B5AA8122D2d2355d"}, R1S);
 9     BOOST_CHECK_EQUAL(uint160S("0x"+R2S.ToString()), R2S);
10     BOOST_CHECK_EQUAL(uint160S("0x"+ZeroS.ToString()), ZeroS);
11     BOOST_CHECK_EQUAL(uint160S("0x"+OneS.ToString()), OneS);
12diff --git a/src/uint256.h b/src/uint256.h
13index 83f008fa55..5399c42117 100644
14--- a/src/uint256.h
15+++ b/src/uint256.h
16@@ -139,6 +139,7 @@ class uint160 : public base_blob<160> {
17 public:
18     static std::optional<uint160> FromHex(std::string_view str) { return detail::FromHex<uint160>(str); }
19     constexpr uint160() = default;
20+    consteval explicit uint160(std::string_view hex_str) : base_blob<160>(hex_str) {}
21     constexpr explicit uint160(Span<const unsigned char> vch) : base_blob<160>(vch) {}
22 };
23

However, leaving it for a follow-up makes sense.

Given the confusion you ran into, it could make sense to replace it with just using blob_160 = std::array<std::byte, 20> and possibly a standalone (consteval) helper function to construct it from a span or hex can be added, if needed.

in src/test/uint256_tests.cpp:157 in 7f6ef14dfd outdated

152@@ -153,8 +153,8 @@ BOOST_AUTO_TEST_CASE( comparison ) // <= >= < >
153     BOOST_CHECK_LT(R2S, MaxS);
154 
155     // Verify hex strings are little-endian
156-    BOOST_CHECK_LT(uint256S("2000000000000000000000000000000000000000000000000000000000000001"),
157-                   uint256S("1000000000000000000000000000000000000000000000000000000000000002"));
158+    BOOST_CHECK_LT(uint256{"2000000000000000000000000000000000000000000000000000000000000001"},
159+                   uint256{"1000000000000000000000000000000000000000000000000000000000000002"});

maflcko commented at 2:00 pm on July 31, 2024:

q in 7f6ef14dfd7fcb2381603730bba2abcc2fca4c43: Is it required to replace them by hand, or can they replaced by the scripted-diff?

hodlinator commented at 8:34 pm on July 31, 2024:

Good point, fixed by changing regexp:

sed -i --regexp-extended -e 's/uint256S$\"(0x)?([^\"]*)\"$/uint256{\"\2\"}/g' $(git grep -l 'uint256S' -- ':src' ':(exclude)src/test/uint256_tests.cpp' ':(exclude)src/test/arith_uint256_tests.cpp') -> sed -i --regexp-extended -e 's/uint256S$\"(0x)?([^\"]{64})\"$/uint256{\"\2\"}/g' $(git grep -l 'uint256S' -- ':src')

maflcko commented at 8:06 am on August 1, 2024:

sed -i --regexp-extended -e 's/uint256S$\"(0x)?([^\"]{64})\"$/uint256{\"\2\"}/g' $(git grep -l 'uint256S' -- ':src')

nit in the scripted-diff: Is the -- ':src' required? Seems fine to just drop it for brevity? Alternatively, drop the :, because it seems a bit odd to use it but no magic signature or parenthesis. (https://git-scm.com/docs/gitglossary#Documentation/gitglossary.txt-aiddefpathspecapathspec)

maflcko commented at 8:11 am on August 1, 2024:

Also, escaping the \" isn’t needed, is it?

hodlinator commented at 12:31 pm on August 1, 2024:

Thanks, that helped narrow it down a bit: sed -i --regexp-extended -e 's/uint256S$\"(0x)?([^\"]{64})\"$/uint256{\"\2\"}/g' $(git grep -l 'uint256S' -- ':src') -> sed -i --regexp-extended -e 's/uint256S$"(0x)?([^"]{64})"$/uint256{"\2"}/g' $(git grep -l uint256S)

maflcko approved

maflcko commented at 2:03 pm on July 31, 2024: member

lgtm. Left some nits, feel free to ignore them.

Mazzika1 commented at 2:29 pm on July 31, 2024: none

Mazzika

Mazzika1 changes_requested

Mazzika1 commented at 2:29 pm on July 31, 2024: none

Mazzika

hodlinator force-pushed on Jul 31, 2024

in src/test/uint256_tests.cpp:342 in 63a91ce42c outdated

335@@ -336,4 +336,11 @@ BOOST_AUTO_TEST_CASE( check_ONE )
336     BOOST_CHECK_EQUAL(one, uint256::ONE);
337 }
338 
339+BOOST_AUTO_TEST_CASE(FromHex_vs_uint256)
340+{
341+    const uint256 runtime_uint = uint256::FromHex("4A5E1E4BAAB89F3A32518A88C31BC87F618f76673e2cc77ab2127b7afdeda33b").value();
342+    constexpr uint256 consteval_uint{             "4a5e1e4baab89f3a32518a88c31bc87f618F76673E2CC77AB2127B7AFDEDA33B"};

maflcko commented at 7:40 am on August 1, 2024:

style-nit in 63a91ce42c4ce070f4ea68f7adc964146e4a45c5:

Would be nice to use clang-format on new code. Suggested diff for this commit

 0diff --git a/src/test/uint256_tests.cpp b/src/test/uint256_tests.cpp
 1index dd354343d3..4ddd8c28de 100644
 2--- a/src/test/uint256_tests.cpp
 3+++ b/src/test/uint256_tests.cpp
 4@@ -339,8 +339,10 @@ BOOST_AUTO_TEST_CASE( check_ONE )
 5 
 6 BOOST_AUTO_TEST_CASE(FromHex_vs_uint256)
 7 {
 8-    const uint256 runtime_uint = uint256::FromHex("4A5E1E4BAAB89F3A32518A88C31BC87F618f76673e2cc77ab2127b7afdeda33b").value();
 9-    constexpr uint256 consteval_uint{             "4a5e1e4baab89f3a32518a88c31bc87f618F76673E2CC77AB2127B7AFDEDA33B"};
10+    constexpr auto hex_a{"4A5E1E4BAAB89F3A32518A88C31BC87F618f76673e2cc77ab2127b7afdeda33b"};
11+    constexpr auto hex_b{"4a5e1e4baab89f3a32518a88c31bc87f618F76673E2CC77AB2127B7AFDEDA33B"};
12+    const uint256 runtime_uint{uint256::FromHex(hex_a).value()};
13+    constexpr uint256 consteval_uint{hex_b};
14     BOOST_CHECK_EQUAL(consteval_uint, runtime_uint);
15 }
16 
17diff --git a/src/uint256.h b/src/uint256.h
18index 5399c42117..13c78200d9 100644
19--- a/src/uint256.h
20+++ b/src/uint256.h
21@@ -93,7 +93,7 @@ public:
22     }
23 };
24 
25-template<unsigned int BITS>
26+template <unsigned int BITS>
27 consteval base_blob<BITS>::base_blob(std::string_view hex_str)
28 {
29     // Non-lookup table version of HexDigit().

hodlinator commented at 12:30 pm on August 1, 2024:

Yeah, should probably try to incorporate clang-format more into my changes.

Made the indentation in FromHex_vs_uint256 a bit less insane by hand without introducing new variables.

Fixed missing space after template.

in src/uint256.h:103 in 63a91ce42c outdated

 98+{
 99+    // Non-lookup table version of HexDigit().
100+    auto from_hex = [](const char c) -> int8_t {
101+        if (c >= '0' && c <= '9') return c - '0';
102+        if (c >= 'a' && c <= 'f') return c - ('a' - 0xA);
103+        if (c >= 'A' && c <= 'F') return c - ('A' - 0xA);

maflcko commented at 7:51 am on August 1, 2024:

style nit in 63a91ce42c4ce070f4ea68f7adc964146e4a45c5:

Seems better to avoid double negation. All code I’ve seen just uses c - 'a' + 0xA, see for example hatoui in src/univalue/lib/univalue_read.cpp

hodlinator commented at 12:30 pm on August 1, 2024:

Fixed.

maflcko approved

maflcko commented at 8:11 am on August 1, 2024: member

Left some more nits.

ACK 9315c1f34abaa4cd78185818ed8d2df5f35eb3ce 🕵

Signature:

0untrusted comment: signature from minisign secret key on empty file; verify via: minisign -Vm "${path_to_any_empty_file}" -P RWTRmVTMeKV5noAMqVlsMugDDCyyTSbA3Re5AkUrhvLVln0tSaFWglOw -x "${path_to_this_whole_four_line_signature_blob}"
1RUTRmVTMeKV5npGrKx1nqXCw5zeVHdtdYURB/KlyA/LMFgpNCs+SkW9a8N95d+U4AP1RJMi+krxU1A3Yux4bpwZNLvVBKy0wLgM=
2trusted comment: ACK 9315c1f34abaa4cd78185818ed8d2df5f35eb3ce 🕵
3GZQfOWL8G0spNKvjqmkS65RYUYIQBpZjUi0j+fcqLs9A5d+ffS+M/zKWMvJBpjmBad5mU4mrr/6AY/sNvV6DBg==

in src/uint256.h:109 in 9315c1f34a outdated

104+
105+        assert(false);
106+    };
107+
108+    // 2 chars per byte.
109+    assert(hex_str.length() == m_data.size() * 2);

stickies-v commented at 11:09 am on August 1, 2024:

nit: can just use size() here

0    assert(hex_str.length() == size() * 2);

hodlinator commented at 9:25 pm on August 1, 2024:

Seeing as m_data is used two lines below, I prefer mentioning it here.

I will however move the comment down to this line like you suggested for the other assert.

hodlinator force-pushed on Aug 1, 2024

TheCharlatan approved

TheCharlatan commented at 12:45 pm on August 1, 2024: contributor

ACK ed9823f7175a451a0170490e4eb96ceb2632b7a0

DrahtBot requested review from maflcko on Aug 1, 2024

hodlinator force-pushed on Aug 1, 2024

TheCharlatan approved

TheCharlatan commented at 1:05 pm on August 1, 2024: contributor

Re-ACK c0b259b62a16e5aa657b817d99167e4295342ca5

Only changes are dropping the commit changing some strings to string_view.

hodlinator commented at 1:08 pm on August 1, 2024: contributor

@TheCharlatan thanks for the speedy re-ACK! My latest force-push actually dropped a tangential commit: #30560 (review)

in src/uint256.h:105 in c0b259b62a outdated

100+    auto from_hex = [](const char c) -> int8_t {
101+        if (c >= '0' && c <= '9') return c - '0';
102+        if (c >= 'a' && c <= 'f') return c - 'a' + 0xA;
103+        if (c >= 'A' && c <= 'F') return c - 'A' + 0xA;
104+
105+        assert(false);

stickies-v commented at 1:54 pm on August 1, 2024:

nit: to make the compiler error message a bit more developer friendly, could add this comment?

0        assert(false); // reached if ctor is called with an invalid hex character

produces:

0validation.cpp:2285:81: error: call to consteval function 'uint256::uint256' is not a constant expression
1    bool fEnforceBIP30 = !((pindex->nHeight==91722 && pindex->GetBlockHash() == uint256{"X0000000000271f2dc26e7667f8419f2e15416dc6955e5a6c6cdf3f2574dd08e"}) ||
2                                                                                ^
3./uint256.h:105:9: note: non-constexpr function '__assert_rtn' cannot be used in a constant expression
4        assert(false); // reached if ctor is called with an invalid hex character

stickies-v approved

stickies-v commented at 2:45 pm on August 1, 2024: contributor

ACK c0b259b62a16e5aa657b817d99167e4295342ca5

Very nice PR, more compile-time checks and easy to review. Makes my branch removing uint256S a bunch more compact too. Non-blocking nits.

in src/kernel/chainparams.cpp:210 in c0b259b62a outdated

206@@ -197,15 +207,15 @@ class CTestNetParams : public CChainParams {
207         consensus.signet_challenge.clear();
208         consensus.nSubsidyHalvingInterval = 210000;
209         consensus.script_flag_exceptions.emplace( // BIP16 exception
210-            uint256S("0x00000000dd30457c001f4095d208cc1296b0eed002427aa599874af7a432b105"), SCRIPT_VERIFY_NONE);
211+            uint256{"00000000dd30457c001f4095d208cc1296b0eed002427aa599874af7a432b105"}, SCRIPT_VERIFY_NONE);

paplorinc commented at 8:38 pm on August 1, 2024:

Did not expect to see so many hex constants in prod. Is the compilation time difference measurable?

hodlinator commented at 10:04 pm on August 1, 2024:

#30560 (review)

in src/uint256.h:128 in c0b259b62a outdated

92@@ -91,6 +93,27 @@ class base_blob
93     }
94 };
95 
96+template <unsigned int BITS>
97+consteval base_blob<BITS>::base_blob(std::string_view hex_str)
98+{
99+    // Non-lookup table version of HexDigit().

paplorinc commented at 8:42 pm on August 1, 2024:

what would be wrong with a lookup, why is this an important note here?

hodlinator commented at 9:48 pm on August 1, 2024:

Was having issues with inefficient MSVC code-generation and also for a time was trying to make it constexpr instead of consteval and missing static constexpr from C++23. It’s doable under consteval but the diff ain’t too pretty:

 0diff --git a/src/uint256.h b/src/uint256.h
 1index a52b0a1f71..be27be93de 100644
 2--- a/src/uint256.h
 3+++ b/src/uint256.h
 4@@ -98,18 +98,35 @@ consteval base_blob<BITS>::base_blob(std::string_view hex_str)
 5 {
 6     // Non-lookup table version of HexDigit().
 7     auto from_hex = [](const char c) -> int8_t {
 8-        if (c >= '0' && c <= '9') return c - '0';
 9-        if (c >= 'a' && c <= 'f') return c - 'a' + 0xA;
10-        if (c >= 'A' && c <= 'F') return c - 'A' + 0xA;
11-
12-        assert(false); // Reached if ctor is called with an invalid hex digit.
13+        constexpr signed char hexdigit[256] =
14+        { -1,-1,-1,-1,-1,-1,-1,-1,-1,-1,-1,-1,-1,-1,-1,-1,
15+          -1,-1,-1,-1,-1,-1,-1,-1,-1,-1,-1,-1,-1,-1,-1,-1,
16+          -1,-1,-1,-1,-1,-1,-1,-1,-1,-1,-1,-1,-1,-1,-1,-1,
17+          0,1,2,3,4,5,6,7,8,9,-1,-1,-1,-1,-1,-1,
18+          -1,0xa,0xb,0xc,0xd,0xe,0xf,-1,-1,-1,-1,-1,-1,-1,-1,-1,
19+          -1,-1,-1,-1,-1,-1,-1,-1,-1,-1,-1,-1,-1,-1,-1,-1,
20+          -1,0xa,0xb,0xc,0xd,0xe,0xf,-1,-1,-1,-1,-1,-1,-1,-1,-1,
21+          -1,-1,-1,-1,-1,-1,-1,-1,-1,-1,-1,-1,-1,-1,-1,-1,
22+          -1,-1,-1,-1,-1,-1,-1,-1,-1,-1,-1,-1,-1,-1,-1,-1,
23+          -1,-1,-1,-1,-1,-1,-1,-1,-1,-1,-1,-1,-1,-1,-1,-1,
24+          -1,-1,-1,-1,-1,-1,-1,-1,-1,-1,-1,-1,-1,-1,-1,-1,
25+          -1,-1,-1,-1,-1,-1,-1,-1,-1,-1,-1,-1,-1,-1,-1,-1,
26+          -1,-1,-1,-1,-1,-1,-1,-1,-1,-1,-1,-1,-1,-1,-1,-1,
27+          -1,-1,-1,-1,-1,-1,-1,-1,-1,-1,-1,-1,-1,-1,-1,-1,
28+          -1,-1,-1,-1,-1,-1,-1,-1,-1,-1,-1,-1,-1,-1,-1,-1,
29+          -1,-1,-1,-1,-1,-1,-1,-1,-1,-1,-1,-1,-1,-1,-1,-1, };
30+
31+        return hexdigit[(uint8_t)c];
32     };
33 
34     assert(hex_str.length() == m_data.size() * 2); // 2 hex digits per byte.
35     auto str_it = hex_str.rbegin();
36     for (auto& elem : m_data) {
37         auto lo = from_hex(*(str_it++));
38-        elem = (from_hex(*(str_it++)) << 4) | lo;
39+        assert(lo != -1);
40+        auto hi = from_hex(*(str_it++));
41+        assert(hi != -1);
42+        elem = (hi << 4) | lo;
43     }
44 }

Depending on compile time measurement results, I’ll reconsider this.

paplorinc commented at 5:31 am on August 2, 2024:

useful context, thanks!

in src/uint256.h:142 in c0b259b62a outdated

109+    assert(hex_str.length() == m_data.size() * 2);
110+    auto str_it = hex_str.rbegin();
111+    for (auto& elem : m_data) {
112+        auto lo = from_hex(*(str_it++));
113+        elem = (from_hex(*(str_it++)) << 4) | lo;
114+    }

paplorinc commented at 8:43 pm on August 1, 2024:

nit, the inline ++ makes these a bit dangerous, that’s why we can’t just inline lo, right? So what do you think of doing the opposite (while making lo/hi slightly more symmetric) and extract hi as well:

0    for (auto& elem : m_data) {
1        auto lo = from_hex(*(str_it++));
2        auto hi = from_hex(*(str_it++));
3        elem = (hi << 4) | lo;
4    }

hodlinator commented at 9:57 pm on August 1, 2024:

Yes, that is why I inlined lo. I don’t see the danger now that str_it is only used once per statement. You used --it in your suggestion where you only broke out one of the parts: #30436 (review)

paplorinc commented at 5:37 am on August 2, 2024:

It’s a nit, I’m just providing an alternative, will of course let you decide

in src/uint256.h:131 in c0b259b62a outdated

 97+consteval base_blob<BITS>::base_blob(std::string_view hex_str)
 98+{
 99+    // Non-lookup table version of HexDigit().
100+    auto from_hex = [](const char c) -> int8_t {
101+        if (c >= '0' && c <= '9') return c - '0';
102+        if (c >= 'a' && c <= 'f') return c - 'a' + 0xA;

paplorinc commented at 8:47 pm on August 1, 2024:

nit: in hexadecimal the a-f range is preceeded by 0-9 , so it might make slightly more sense to signal this by prefixing with the offset in the return as well, i.e.:

0    auto from_hex = [](char c) -> int8_t {
1        if (c >= '0' && c <= '9') return c - '0';
2        c = std::tolower(c);
3        if (c >= 'a' && c <= 'f') return 10 + (c - 'a');
4        throw std::invalid_argument("Invalid hex character");
5    };

(node 0, normalized to lowercase, most of our code seems to use that anyway) (note 1, I used a throw instead of assert) (note 2, I used 10 instead of 0xA, makes more sense to me to signal that 10 actually refers to the digits)

hodlinator commented at 10:00 pm on August 1, 2024:

Feel this has already been bikeshedded enough today. #30560 (review)

paplorinc commented at 5:32 am on August 2, 2024:

Maybe, but was my first bikeshedding attempt at it :) I’m not sure there’s value in having both casing in the code, i.e. 4a5e1e4baab89f3a32518a88c31bc87f618F76673E2CC77AB2127B7AFDEDA33B. Otherwise we could leave out the uppercase and it would still compile:

0    auto from_hex = [](char c) -> int8_t {
1        if (c >= '0' && c <= '9') return c - '0';
2        else if (c >= 'a' && c <= 'f') return 10 + (c - 'a');
3        else throw std::invalid_argument("Invalid hex character");
4    };

in src/kernel/chainparams.cpp:89 in c0b259b62a outdated

85@@ -76,17 +86,17 @@ class CMainParams : public CChainParams {
86         consensus.signet_challenge.clear();
87         consensus.nSubsidyHalvingInterval = 210000;
88         consensus.script_flag_exceptions.emplace( // BIP16 exception
89-            uint256S("0x00000000000002dc756eebf4f49723ed8d30cc28a5f108eb94b1ba88ac4f9c22"), SCRIPT_VERIFY_NONE);
90+            uint256{"00000000000002dc756eebf4f49723ed8d30cc28a5f108eb94b1ba88ac4f9c22"}, SCRIPT_VERIFY_NONE);

paplorinc commented at 8:57 pm on August 1, 2024:

In theory I love the idea of migrating these calculations to compile time, but in practice compilation is already way too slow - are we sure we want to make it even heavier?

hodlinator commented at 9:49 pm on August 1, 2024:

Good point. It depends on how much heavier I guess. Rediscovered you offered to measure in #30560#pullrequestreview-2213841923, that would be mighty kind. Not sure when I will get around to it.

sipa commented at 2:20 am on August 2, 2024:

Compilation is certainly slow, but I don’t think some consteval expressions inside .cpp files are going to meaningfully contribute to that. If it were a bunch of templated .h code that gets instantiated everywhere the situation may be different, but even then…

paplorinc commented at 5:35 am on August 2, 2024:

I also don’t think this will be significant, will rent a cheap hetzner server and run something like:

0hyperfine \
1  --export-markdown make_bench.md \
2  --warmup 1 --runs 10 \
3  --parameter-list COMMIT 30cef5,be750d \
4  --prepare 'git checkout {COMMIT} && git clean -fxd && git reset --hard && ./autogen.sh && ./configure --disable-ccache' \
5  'echo {COMMIT} && make -j1'

i.e. 10 single-threaded, uncached compilations for 30cef5 vs be750d - would that be representative?

maflcko commented at 7:36 am on August 2, 2024:

I checked src/kernel/chainparams.cpp locally and I got that it takes 1MB more memory (from 217MB) and runtime (both +1%), so I think this is fine, if it isn’t just random noise anyway.

Generally, if you want to optimize the compile-time of anything that includes serialize.h, you have already lost ;)

Also, let’s recall that the goal here isn’t that the compile-time is faster, but that more checks are done at compile-time.

Would still be fun to check the overall compile time, but I’d be surprised if it differed from my check on a single module.

fanquake commented at 10:06 am on August 2, 2024:

uncached compilations for 30cef5 vs be750d - would that be representative?

Maybe, but I don’t think this is that useful. Anyone doing development, should be using ccache, only compiling the code they need, etc, and in that case, I don’t think compilation is “way too slow”. Anyone who isn’t doing development, is likely only compiling once, where +/- N seconds/minutes in compilation time, doesn’t really matter.

paplorinc commented at 5:42 am on August 3, 2024:

0Summary
1  'echo 30cef5 && make -j1 && ./src/bitcoind --version | head -n 1' ran
2    1.00 ± 0.00 times faster than 'echo be750d && make -j1 && ./src/bitcoind --version | head -n 1'

both of them take 45 minutes on a i7-7700, without cache: no measurable slowdown!

hodlinator commented at 6:48 pm on August 3, 2024:

Thanks @paplorinc for helping put that worry to rest!

in src/uint256.h:108 in c0b259b62a outdated

103+        if (c >= 'A' && c <= 'F') return c - 'A' + 0xA;
104+
105+        assert(false);
106+    };
107+
108+    // 2 chars per byte.

paplorinc commented at 9:04 pm on August 1, 2024:

The purpose of code should be to express context which we couldn’t convey using code, right? Here the code already states this clearly, the comment just seems like a distraction.

hodlinator commented at 10:02 pm on August 1, 2024:

Slightly more okay if moved to the same line? #30560 (review)

paplorinc commented at 5:36 am on August 2, 2024:

it’s just a nit from my part, but I just see noise here, I’m not a big fan of dead comments, especially when they repeat the code

hodlinator commented at 3:42 pm on August 2, 2024:

Agree it’s better to make the code self-documenting in general. My threshold for what is a useless comment seems slightly different than yours in this case.

paplorinc commented at 9:25 pm on August 1, 2024: contributor

I’ve reviewed a part of it, left some questions. I will have to measure the before/after compilation speeds and the before/after artefact size to be able to approve - will do that in the following days.

hodlinator force-pushed on Aug 1, 2024

in src/test/uint256_tests.cpp:404 in be750dccb0 outdated

337     BOOST_CHECK_EQUAL(one, uint256::ONE);
338 }
339 
340+BOOST_AUTO_TEST_CASE(FromHex_vs_uint256)
341+{
342+    auto runtime_uint{uint256::FromHex("4A5E1E4BAAB89F3A32518A88C31BC87F618f76673e2cc77ab2127b7afdeda33b").value()};

paplorinc commented at 6:03 am on August 2, 2024:

are we sure we want to mandate that mixed-case hex strings can be parsed? I consider that an error instead

hodlinator commented at 9:11 pm on August 2, 2024:

At some point, retouching ACKed PRs becomes an attack on Bitcoin. But this is a good enough question to possibly trigger a re-touch.

Would be happy to hear opinions from others on this. But I understand if they prefer spending time on more significant things.

Supporting both cases could be considered a hangover from SetHexDeprecated, but on the other hand, mixed cases are still supported by FromHex/IsHex/HexDigit.

Checked for occurrences of uppercase hex after “uint256”.

0$ git grep -E 'uint256.*"[a-fA-F0-9]{64}"' |grep -E 'uint256.{0,32}".{0,63}[A-F]+.{0,63}"'
1src/kernel/chainparams.cpp:            uint256{"0000A000000002dc756eebf4f49723ed8d30cc28a5f108eb94b1ba88ac4f9c22"}, SCRIPT_VERIFY_NONE);
2src/test/pow_tests.cpp:        arith_uint256 targ_max{UintToArith256(uint256{"FFFFFFFFFFFFFFFFFFFFFFFFFFFFFFFFFFFFFFFFFFFFFFFFFFFFFFFFFFFFFFFF"})};
3src/test/uint256_tests.cpp:    auto runtime_uint{uint256::FromHex("4A5E1E4BAAB89F3A32518A88C31BC87F618f76673e2cc77ab2127b7afdeda33b").value()};
4src/test/uint256_tests.cpp:    constexpr uint256 consteval_uint{  "4a5e1e4baab89f3a32518a88c31bc87f618F76673E2CC77AB2127B7AFDEDA33B"};

src/kernel/chainparams.cpp was my modified local file. So it’s only pow_tests.cpp and the current test use uppercase.

Implemented a version of this PR with lowercase enforcement (+ including assert => throw from #30560 (review)) in 40bd3c1aa766572bf5eb8e14bbc9f9606a69299b. It uses \L in the scripted-diff regexp to lowercase the hex number (might not be supported on Mac sed I think, but Mac is not officially supported anyways).

 0$ git range-diff master be750dccb08050bcca48c4236ca6b902dd911c7b 40bd3c1aa766572bf5eb8e14bbc9f9606a69299b
 11:  9f4ca1f293 ! 1:  49ff7af46c refactor: Add consteval uint256(hex_str)
 2    @@ src/test/uint256_tests.cpp: BOOST_AUTO_TEST_CASE( check_ONE )
 3     +BOOST_AUTO_TEST_CASE(FromHex_vs_uint256)
 4     +{
 5     +    auto runtime_uint{uint256::FromHex("4A5E1E4BAAB89F3A32518A88C31BC87F618f76673e2cc77ab2127b7afdeda33b").value()};
 6    -+    constexpr uint256 consteval_uint{  "4a5e1e4baab89f3a32518a88c31bc87f618F76673E2CC77AB2127B7AFDEDA33B"};
 7    ++    constexpr uint256 consteval_uint{  "4a5e1e4baab89f3a32518a88c31bc87f618f76673e2cc77ab2127b7afdeda33b"};
 8     +    BOOST_CHECK_EQUAL(consteval_uint, runtime_uint);
 9     +}
10     +
11    @@ src/uint256.h: public:
12     +    // Non-lookup table version of HexDigit().
13     +    auto from_hex = [](const char c) -> int8_t {
14     +        if (c >= '0' && c <= '9') return c - '0';
15    ++        // Only lowercase letters are supported, for consistency.
16     +        if (c >= 'a' && c <= 'f') return c - 'a' + 0xA;
17    -+        if (c >= 'A' && c <= 'F') return c - 'A' + 0xA;
18     +
19    -+        assert(false); // Reached if ctor is called with an invalid hex digit.
20    ++        throw "Called ctor with an invalid hex digit";
21     +    };
22     +
23    -+    assert(hex_str.length() == m_data.size() * 2); // 2 hex digits per byte.
24    ++    if (hex_str.length() != m_data.size() * 2) throw "Hex string must fit exactly";
25     +    auto str_it = hex_str.rbegin();
26     +    for (auto& elem : m_data) {
27     +        auto lo = from_hex(*(str_it++));
282:  d5e5d7eacc = 2:  5994e659c0 refactor: Hand-replace some uint256S -> uint256
293:  be750dccb0 ! 3:  40bd3c1aa7 scripted-diff: Replace uint256S("str") -> uint256{"str"}
30    @@ Commit message
31         scripted-diff: Replace uint256S("str") -> uint256{"str"}
32     
33         -BEGIN VERIFY SCRIPT-
34    -    sed -i --regexp-extended -e 's/uint256S\("(0x)?([^"]{64})"\)/uint256{"\2"}/g' $(git grep -l uint256S)
35    +    sed -i --regexp-extended -e 's/uint256S\("(0x)?([^"]{64})"\)/uint256{"\L\2"}/g' $(git grep -l uint256S)
36         -END VERIFY SCRIPT-
37     
38      ## src/kernel/chainparams.cpp ##
39    @@ src/test/pow_tests.cpp: void sanity_check_chainparams(const ArgsManager& args, C
40          // check max target * 4*nPowTargetTimespan doesn't overflow -- see pow.cpp:CalculateNextWorkRequired()
41          if (!consensus.fPowNoRetargeting) {
42     -        arith_uint256 targ_max{UintToArith256(uint256S("0xFFFFFFFFFFFFFFFFFFFFFFFFFFFFFFFFFFFFFFFFFFFFFFFFFFFFFFFFFFFFFFFF"))};
43    -+        arith_uint256 targ_max{UintToArith256(uint256{"FFFFFFFFFFFFFFFFFFFFFFFFFFFFFFFFFFFFFFFFFFFFFFFFFFFFFFFFFFFFFFFF"})};
44    ++        arith_uint256 targ_max{UintToArith256(uint256{"ffffffffffffffffffffffffffffffffffffffffffffffffffffffffffffffff"})};
45              targ_max /= consensus.nPowTargetTimespan*4;
46              BOOST_CHECK(UintToArith256(consensus.powLimit) < targ_max);
47          }

paplorinc commented at 5:59 am on August 3, 2024:

Thanks for considering, I’m personally fine with doing it in a different PR as well

stickies-v commented at 7:32 am on August 3, 2024:

I don’t have a strong view on this but I would prefer getting this PR over the line as is.

hodlinator commented at 8:10 pm on August 3, 2024:

Trying to not spawn yet another PR from this work but will post that as a separate PR then if this one is merged in the current state.

maflcko commented at 6:36 am on August 2, 2024: member

re-ACK be750dccb08050bcca48c4236ca6b902dd911c7b 📩

Signature:

0untrusted comment: signature from minisign secret key on empty file; verify via: minisign -Vm "${path_to_any_empty_file}" -P RWTRmVTMeKV5noAMqVlsMugDDCyyTSbA3Re5AkUrhvLVln0tSaFWglOw -x "${path_to_this_whole_four_line_signature_blob}"
1RUTRmVTMeKV5npGrKx1nqXCw5zeVHdtdYURB/KlyA/LMFgpNCs+SkW9a8N95d+U4AP1RJMi+krxU1A3Yux4bpwZNLvVBKy0wLgM=
2trusted comment: re-ACK be750dccb08050bcca48c4236ca6b902dd911c7b 📩
3XKgaX77QrcUqp/QwF4ALbBeRlRY0IPgLGnco+g/GineLXZ8qsKhe6zjomAMVRK99+J/zCiYZfKgLifTLg4SKBA==

DrahtBot requested review from stickies-v on Aug 2, 2024

DrahtBot requested review from TheCharlatan on Aug 2, 2024

in src/uint256.h:134 in be750dccb0 outdated

100+    auto from_hex = [](const char c) -> int8_t {
101+        if (c >= '0' && c <= '9') return c - '0';
102+        if (c >= 'a' && c <= 'f') return c - 'a' + 0xA;
103+        if (c >= 'A' && c <= 'F') return c - 'A' + 0xA;
104+
105+        assert(false); // Reached if ctor is called with an invalid hex digit.

maflcko commented at 7:07 am on August 2, 2024:

0        throw "Invalid hex digit encountered";

style-nit (Only if you re-touch for other reasons):

Can avoid the cassert include in a consteval context by using throw (anything). Doesn’t matter in this file, because assert() is already used, so it is just a style nit.

edit: Below would become: if (hex_str.length() != m_data.size() * 2) throw "Hex string must fit exactly";

TheCharlatan commented at 8:20 am on August 2, 2024:

Might be a good idea to add a small suggestion in the developer-notes for using throw for these checks, since we already do so in https://github.com/bitcoin/bitcoin/blob/master/src/script/miniscript.h#L177 . I initially assumed this was not supported, but seems like that was fixed in recent compiler versions.

maflcko commented at 8:25 am on August 2, 2024:

Hmm, throw std::_{"bla"}; is no different than assert(false); // bla, because both need an include. No objection about mentioning throw "bla"; in the dev notes, but it is probably just a minor style issue.

hodlinator commented at 3:48 pm on August 2, 2024:

Iff I retouch, I might experiment with including the offending digit in the throw.

maflcko commented at 9:06 am on August 5, 2024:

Iff I retouch, I might experiment with including the offending digit in the throw.

That’d probably be overkill. So far the only errors I’ve seen were truncated input or overly large (possibly with trailing trash, see #30329 (review)), but those are already covered by the length check (and the hex check wouldn’t run on them due to an early return).

A non-hex should be rare, as the input is otherwise copy-pasted from somewhere.

hodlinator commented at 7:52 pm on August 5, 2024:

Yeah, I experimented with using std::format but at least GCC would not print the actual thrown value anyway.

maflcko added the label DrahtBot Guix build requested on Aug 2, 2024

stickies-v commented at 9:47 am on August 2, 2024: contributor

re-ACK be750dccb08050bcca48c4236ca6b902dd911c7b - doc-only changes to address review nits

TheCharlatan approved

TheCharlatan commented at 9:53 am on August 2, 2024: contributor

Re-ACK be750dccb08050bcca48c4236ca6b902dd911c7b

maflcko removed the label DrahtBot Guix build requested on Aug 2, 2024

DrahtBot commented at 3:19 pm on August 2, 2024: contributor

Guix builds (on x86_64) [untrusted test-only build, possibly unsafe, not for production use]

File	commit 9774a958b501a6d439a734e18b29e04f59f973f6(master)	commit fc222d05fbb3b109372024a9086585962a3bff10(master and this pull)
SHA256SUMS.part	`57b529722ff8170d...`	`9cc32e96ee19c7ce...`
*-aarch64-linux-gnu-debug.tar.gz	`9e133d3761048d87...`	`d731065f25a2067f...`
*-aarch64-linux-gnu.tar.gz	`b51473e78eeb01d9...`	`4bc1a640bbdce4e6...`
*-arm-linux-gnueabihf-debug.tar.gz	`7b5b1955f6863a67...`	`ec8dc8be9bd051b0...`
*-arm-linux-gnueabihf.tar.gz	`bafac190444cf1c9...`	`b3aca56136bd925e...`
*-arm64-apple-darwin-unsigned.tar.gz	`49cd7a29e7b8870d...`	`d944d0ed24bf738b...`
*-arm64-apple-darwin-unsigned.zip	`3e59d7fdbd306ba9...`	`559ea1aeb9a6bd40...`
*-arm64-apple-darwin.tar.gz	`1baa8000875e847a...`	`57bc5fe398d3ff8f...`
*-powerpc64-linux-gnu-debug.tar.gz	`9d9523154952b3e4...`	`cd06d4a9bde80644...`
*-powerpc64-linux-gnu.tar.gz	`0a463da65598d2de...`	`a47fd84db9f6fe0c...`
*-riscv64-linux-gnu-debug.tar.gz	`ac4f691dabc2379b...`	`93e983f0ff97c2e7...`
*-riscv64-linux-gnu.tar.gz	`62b45e48084b1122...`	`22d3d90b92261af1...`
*-x86_64-apple-darwin-unsigned.tar.gz	`754416c2368c0bc2...`	`5c156fce57a8f679...`
*-x86_64-apple-darwin-unsigned.zip	`1fc341265dc614fb...`	`35e716173c704538...`
*-x86_64-apple-darwin.tar.gz	`4570c83abed42575...`	`d1b73111d170624a...`
*-x86_64-linux-gnu-debug.tar.gz	`a3749460e54f3799...`	`4358edf1c01d6d59...`
*-x86_64-linux-gnu.tar.gz	`1ed77103dbec7f67...`	`9c2c3a637b594eb5...`
*.tar.gz	`229aab5f94e60c45...`	`f2d306ae36933f32...`
guix_build.log	`3ad35213427a9854...`	`8803670ef192f8ac...`
guix_build.log.diff		`750efac38926403e...`

paplorinc commented at 5:57 am on August 3, 2024: contributor

ACK be750dccb08050bcca48c4236ca6b902dd911c7b

DrahtBot added the label Needs rebase on Aug 5, 2024

hodlinator force-pushed on Aug 5, 2024

hodlinator commented at 7:36 am on August 5, 2024: contributor

Rebased with minimal changes to fix merge-issues.

sed -i --regexp-extended -e 's/uint256S$"(0x)?([^"]{64})"$/uint256{"\2"}/g' $(git grep -l uint256S) -> sed -i --regexp-extended -e 's/\buint256S$"(0x)?([^"]{64})"$/uint256{"\2"}/g' $(git grep -l uint256S) to make it avoid matching new instances of arith_uint256S being passed 64 chars for the first time in arith_uint256_tests.cpp.

Edit: I realized too late that this might have been an okay time to incorporate #30560 (review) but it is probably just as well as it could lead to further discussion and dragging out this PR.

DrahtBot removed the label Needs rebase on Aug 5, 2024

maflcko commented at 8:20 am on August 5, 2024: member

rebase re-ACK a34ba0eed401f2b48e168b857d05eab065d9ab42 🔺

Signature:

0untrusted comment: signature from minisign secret key on empty file; verify via: minisign -Vm "${path_to_any_empty_file}" -P RWTRmVTMeKV5noAMqVlsMugDDCyyTSbA3Re5AkUrhvLVln0tSaFWglOw -x "${path_to_this_whole_four_line_signature_blob}"
1RUTRmVTMeKV5npGrKx1nqXCw5zeVHdtdYURB/KlyA/LMFgpNCs+SkW9a8N95d+U4AP1RJMi+krxU1A3Yux4bpwZNLvVBKy0wLgM=
2trusted comment: rebase re-ACK a34ba0eed401f2b48e168b857d05eab065d9ab42 🔺
3iFR6SWJkjcVURezwm7wcmHE5vJpN52mrZZQxoZ7seS8rcmQLasm5/VYX2GSWvh9sBxcklTxuhbSz+NQ29GbiDw==

DrahtBot requested review from paplorinc on Aug 5, 2024

DrahtBot requested review from TheCharlatan on Aug 5, 2024

paplorinc commented at 8:47 am on August 5, 2024: contributor

ACK a34ba0eed401f2b48e168b857d05eab065d9ab42

maflcko commented at 9:08 am on August 5, 2024: member

(needs another rebase)

maflcko added the label Needs rebase on Aug 5, 2024

refactor: Add consteval uint256(hex_str)

Complements uint256::FromHex() nicely in that it naturally does all error checking at compile time and so doesn't need to return an std::optional.

Will be used in the following 2 commits to replace many calls to uint256S(). uint256S() calls taking C-string literals are littered throughout the codebase and executed at runtime to perform parsing unless a given optimizer was surprisingly efficient. While this may not be a hot spot, it's better hygiene in C++20 to store the parsed data blob directly in the binary, without any parsing at runtime.

b74d8d58fa

refactor: Hand-replace some uint256S -> uint256

chainparams.cpp - workaround for MSVC bug triggering C7595 - Calling consteval constructors in initializer lists fails, but works on GCC (13.2.0) & Clang (17.0.6).

c06f2368e2

scripted-diff: Replace uint256S("str") -> uint256{"str"}

-BEGIN VERIFY SCRIPT-
sed -i --regexp-extended -e 's/\buint256S\("(0x)?([^"]{64})"\)/uint256{"\2"}/g' $(git grep -l uint256S)
-END VERIFY SCRIPT-

2d9d752e4f

hodlinator force-pushed on Aug 5, 2024

paplorinc approved

paplorinc commented at 1:24 pm on August 5, 2024: contributor

ACK 2d9d752e4fc30aabf2ddd36ca7a63432d26d4fea

DrahtBot requested review from maflcko on Aug 5, 2024

maflcko commented at 1:31 pm on August 5, 2024: member

Checked with git range-diff bitcoin-core/master a34ba0eed401f2b48e168b857d05eab065d9ab42 2d9d752e4fc30aabf2ddd36ca7a63432d26d4fea --word-diff -U1

rebase re-cr-ACK 2d9d752e4fc30aabf2ddd36ca7a63432d26d4fea 🎐

Signature:

0untrusted comment: signature from minisign secret key on empty file; verify via: minisign -Vm "${path_to_any_empty_file}" -P RWTRmVTMeKV5noAMqVlsMugDDCyyTSbA3Re5AkUrhvLVln0tSaFWglOw -x "${path_to_this_whole_four_line_signature_blob}"
1RUTRmVTMeKV5npGrKx1nqXCw5zeVHdtdYURB/KlyA/LMFgpNCs+SkW9a8N95d+U4AP1RJMi+krxU1A3Yux4bpwZNLvVBKy0wLgM=
2trusted comment: rebase re-cr-ACK 2d9d752e4fc30aabf2ddd36ca7a63432d26d4fea 🎐
3bVzdyt2phN87YtMtJ9BYOknRRBRRiX84Qv5+49gXFTHu5Il0aHR0r7rJUIcOTH6e4oMVe06eFh7+BrYY91yqCg==

stickies-v commented at 1:56 pm on August 5, 2024: contributor

re-ACK 2d9d752e4fc30aabf2ddd36ca7a63432d26d4fea

DrahtBot removed the label Needs rebase on Aug 5, 2024

ryanofsky merged this on Aug 5, 2024

ryanofsky closed this on Aug 5, 2024

hodlinator deleted the branch on Aug 5, 2024

refactor: Add consteval uint256 constructor #30560

Code Coverage

Reviews

Conflicts

Guix builds (on x86_64) [untrusted test-only build, possibly unsafe, not for production use]