It seems modern compilers don’t realize that all invocations of operator""_mst can be evaluated at compile time, despite the constexpr keyword.
Since C++20, we can force them to evaluate at compile time using consteval, turning all the miniscript type constants into actual compile-time constants.
This should give a nice but not very important speedup for miniscript logic, but it’s also a way to start testing C++20 features.
The following sections might be updated with supplementary metadata relevant to reviewers and maintainers.
For detailed information about the code coverage, see the test coverage report.
See the guideline for information on the review process.
If your review is incorrectly listed, please react with 👎 to this comment and the bot will ignore it on the next update.
25@@ -26,6 +26,12 @@
26 #include <util/string.h>
27 #include <util/vector.h>
28
29+#if __cplusplus >= 201811L
and also Windows (see the apparent CI failure here).
For reference:
0 D:\a\bitcoin\bitcoin\src\script\miniscript.h(160,13): error C2248: 'miniscript::Type::Type': cannot access private member declared in class 'miniscript::Type' [D:\a\bitcoin\bitcoin\build_msvc\libbitcoin_common\libbitcoin_common.vcxproj]
1D:\a\bitcoin\bitcoin\src\script\miniscript.h(133,5): message : see declaration of 'miniscript::Type::Type' [D:\a\bitcoin\bitcoin\build_msvc\libbitcoin_common\libbitcoin_common.vcxproj]
2D:\a\bitcoin\bitcoin\src\script\miniscript.h(128,7): message : see declaration of 'miniscript::Type' [D:\a\bitcoin\bitcoin\build_msvc\libbitcoin_common\libbitcoin_common.vcxproj]
3D:\a\bitcoin\bitcoin\src\script\miniscript.cpp(17,27): message : while evaluating consteval function 'miniscript::operator ""_mst' [D:\a\bitcoin\bitcoin\build_msvc\libbitcoin_common\libbitcoin_common.vcxproj]
4D:\a\bitcoin\bitcoin\src\script\miniscript.cpp(234,33): error C7595: 'miniscript::operator ""_mst': call to immediate function is not a constant expression [D:\a\bitcoin\bitcoin\build_msvc\libbitcoin_common\libbitcoin_common.vcxproj]
5D:\a\bitcoin\bitcoin\src\script\miniscript.cpp(234,33): fatal error C1003: error count exceeds 100; stopping compilation [D:\a\bitcoin\bitcoin\build_msvc\libbitcoin_common\libbitcoin_common.vcxproj]
Now it is
0 miniscript.cpp
1D:\a\bitcoin\bitcoin\src\script\miniscript.cpp(234,33): error C7595: 'miniscript::operator ""_mst': call to immediate function is not a constant expression [D:\a\bitcoin\bitcoin\build_msvc\libbitcoin_common\libbitcoin_common.vcxproj]
2D:\a\bitcoin\bitcoin\src\script\miniscript.cpp(234,45): error C7595: 'miniscript::operator ""_mst': call to immediate function is not a constant expression [D:\a\bitcoin\bitcoin\build_msvc\libbitcoin_common\libbitcoin_common.vcxproj]
Go home MSVC, you’re drunk.
Somehow it thinks that calls to ""_mst are immediate, unless they occur inside ternary operators!?
Yeah, not sure if we are using the latest msvc, and if this is a bug that is fixed in a newer release. cc @hebasto @sipsorcery
0MSBuild version 17.7.2+d6990bcfa for .NET Framework
It is the latest one.
( ? : ) ternaries. I’ve pushed another attempt to avoid it by moving the subexpressions out of the ternaries.
conteval directly. Put this on 27.x.
CI is failing with
0/usr/bin/ld: /usr/bin/ld: DWARF error: invalid or unhandled FORM value: 0x23
1libbitcoin_common.a(libbitcoin_common_a-descriptor.o): in function `(anonymous namespace)::ParseScript(unsigned int&, Span<char const>&, (anonymous namespace)::ParseScriptContext, FlatSigningProvider&, std::__cxx11::basic_string<char, std::char_traits<char>, std::allocator<char> >&)':
2descriptor.cpp:(.text+0x390f): undefined reference to `miniscript::operator"" _mst(char const*, unsigned int)'
3/usr/bin/ld: descriptor.cpp:(.text+0x396c): undefined reference to `miniscript::operator"" _mst(char const*, unsigned int)'
4/usr/bin/ld: descriptor.cpp:(.text+0x39ed): undefined reference to `miniscript::operator"" _mst(char const*, unsigned int)'
5/usr/bin/ld: libbitcoin_common.a(libbitcoin_common_a-descriptor.o): in function `std::shared_ptr<miniscript::Node<unsigned int> const> miniscript::internal::Parse<unsigned int, (anonymous namespace)::KeyParser>(Span<char const>, (anonymous namespace)::KeyParser const&)':
6descriptor.cpp:(.text+0x1df07): undefined reference to `miniscript::operator"" _mst(char const*, unsigned int)'
7/usr/bin/ld: libbitcoin_common.a(libbitcoin_common_a-descriptor.o): in function `(anonymous namespace)::MiniscriptDescriptor::MaxSatisfactionElems() const':
8descriptor.cpp:(.text+0x2d316): undefined reference to `miniscript::operator"" _mst(char const*, unsigned int)'
9/usr/bin/ld: libbitcoin_common.a(libbitcoin_common_a-descriptor.o):descriptor.cpp:(.text+0x2d333): more undefined references to `miniscript::operator"" _mst(char const*, unsigned int)' follow
10clang: error: linker command failed with exit code 1 (use -v to see invocation)
clang++-15.
Could rebase, as CI on master should now be passing with clang-15 in.
As we now build the fuzz.exe on MSVC, this PR seems require a little adjustment while rebasing:
0--- a/src/test/fuzz/miniscript.cpp
1+++ b/src/test/fuzz/miniscript.cpp
2@@ -391,6 +391,7 @@ std::optional<NodeInfo> ConsumeNodeStable(MsCtx script_ctx, FuzzedDataProvider&
3 bool allow_K = (type_needed == ""_mst) || (type_needed << "K"_mst);
4 bool allow_V = (type_needed == ""_mst) || (type_needed << "V"_mst);
5 bool allow_W = (type_needed == ""_mst) || (type_needed << "W"_mst);
6+ static constexpr auto B{"B"_mst}, K{"K"_mst}, V{"V"_mst}, W{"W"_mst};
7
8 switch (provider.ConsumeIntegral<uint8_t>()) {
9 case 0:
10@@ -440,22 +441,22 @@ std::optional<NodeInfo> ConsumeNodeStable(MsCtx script_ctx, FuzzedDataProvider&
11 }
12 case 11:
13 if (!(allow_B || allow_K || allow_V)) return {};
14- return {{{"B"_mst, type_needed, type_needed}, Fragment::ANDOR}};
15+ return {{{B, type_needed, type_needed}, Fragment::ANDOR}};
16 case 12:
17 if (!(allow_B || allow_K || allow_V)) return {};
18- return {{{"V"_mst, type_needed}, Fragment::AND_V}};
19+ return {{{V, type_needed}, Fragment::AND_V}};
20 case 13:
21 if (!allow_B) return {};
22- return {{{"B"_mst, "W"_mst}, Fragment::AND_B}};
23+ return {{{B, W}, Fragment::AND_B}};
24 case 15:
25 if (!allow_B) return {};
26- return {{{"B"_mst, "W"_mst}, Fragment::OR_B}};
27+ return {{{B, W}, Fragment::OR_B}};
28 case 16:
29 if (!allow_V) return {};
30- return {{{"B"_mst, "V"_mst}, Fragment::OR_C}};
31+ return {{{B, V}, Fragment::OR_C}};
32 case 17:
33 if (!allow_B) return {};
34- return {{{"B"_mst, "B"_mst}, Fragment::OR_D}};
35+ return {{{B, B}, Fragment::OR_D}};
36 case 18:
37 if (!(allow_B || allow_K || allow_V)) return {};
38 return {{{type_needed, type_needed}, Fragment::OR_I}};
39@@ -472,25 +473,25 @@ std::optional<NodeInfo> ConsumeNodeStable(MsCtx script_ctx, FuzzedDataProvider&
40 }
41 case 20:
42 if (!allow_W) return {};
43- return {{{"B"_mst}, Fragment::WRAP_A}};
44+ return {{{B}, Fragment::WRAP_A}};
45 case 21:
46 if (!allow_W) return {};
47- return {{{"B"_mst}, Fragment::WRAP_S}};
48+ return {{{B}, Fragment::WRAP_S}};
49 case 22:
50 if (!allow_B) return {};
51- return {{{"K"_mst}, Fragment::WRAP_C}};
52+ return {{{K}, Fragment::WRAP_C}};
53 case 23:
54 if (!allow_B) return {};
55- return {{{"V"_mst}, Fragment::WRAP_D}};
56+ return {{{V}, Fragment::WRAP_D}};
57 case 24:
58 if (!allow_V) return {};
59- return {{{"B"_mst}, Fragment::WRAP_V}};
60+ return {{{B}, Fragment::WRAP_V}};
61 case 25:
62 if (!allow_B) return {};
63- return {{{"B"_mst}, Fragment::WRAP_J}};
64+ return {{{B}, Fragment::WRAP_J}};
65 case 26:
66 if (!allow_B) return {};
67- return {{{"B"_mst}, Fragment::WRAP_N}};
68+ return {{{B}, Fragment::WRAP_N}};
69 case 27: {
70 if (!allow_B || !IsTapscript(script_ctx)) return {};
71 const auto k = provider.ConsumeIntegral<uint16_t>();
ACK 73619b819c37e98bd9a100bea14a3366aebfabb8.
I apologise for messing up.
The #30031, which is built on top of this PR, succeeds. It includes additional code changes in src/test/fuzz/miniscript.cpp (with some inconsistency in local variable naming).
@sipa feel free to pull changes from #30031 into this PR.
It seems modern compilers don't realize that all invocations of operator""_mst
can be evaluated at compile time, despite the constexpr keyword.
Since C++20, we can force them to evaluate at compile time, turning all the
miniscript type constants into actual compile-time constants.
It appears that MSVC does not support consteval operator"" when used inside
certain expressions. For the few places where this happens, define a
constant outside the operator call.
Co-Authored-By: Hennadii Stepanov <32963518+hebasto@users.noreply.github.com>