miniscript: make operator""_mst consteval

[sipa]

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.

[DrahtBot]

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	hebasto, theuni
Concept ACK	fanquake

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

[sipa]

Ping @maflcko.

[fanquake]

Concept ACK. It's annoying that we have to refactor this code just to accomodate MSVC. I agree that we should just wait until branch-off, and use conteval directly. Put this on 27.x.

[sipa]

Rebased.

[darosior]

CI is failing with

/usr/bin/ld: /usr/bin/ld: DWARF error: invalid or unhandled FORM value: 0x23
libbitcoin_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> >&)':
descriptor.cpp:(.text+0x390f): undefined reference to `miniscript::operator"" _mst(char const*, unsigned int)'
/usr/bin/ld: descriptor.cpp:(.text+0x396c): undefined reference to `miniscript::operator"" _mst(char const*, unsigned int)'
/usr/bin/ld: descriptor.cpp:(.text+0x39ed): undefined reference to `miniscript::operator"" _mst(char const*, unsigned int)'
/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&)':
descriptor.cpp:(.text+0x1df07): undefined reference to `miniscript::operator"" _mst(char const*, unsigned int)'
/usr/bin/ld: libbitcoin_common.a(libbitcoin_common_a-descriptor.o): in function `(anonymous namespace)::MiniscriptDescriptor::MaxSatisfactionElems() const':
descriptor.cpp:(.text+0x2d316): undefined reference to `miniscript::operator"" _mst(char const*, unsigned int)'
/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
clang: error: linker command failed with exit code 1 (use -v to see invocation)

[hebasto]

Concept ACK.

[hebasto]

Could the CI issues be due to Clang partially supporting consteval before clang-17?

[maflcko]

Locally, it works, starting with clang++-15.

[maflcko]

Confirmed that this cuts a third of the runtime off of the fuzz input from #28832 (comment)

[fanquake]

Maybe we should also retry the straight constexpr -> consteval change, without having to do all the refactoring for MSVC? Microsoft might have fixed their compiler.

[sipa]

@fanquake See #29167

[maflcko]

Maybe mark as draft while this is waiting on #29165? (29165 won't happen unless someone bumps google's oss-fuzz's clang)

[maflcko]

Could rebase, as CI on master should now be passing with clang-15 in.

[maflcko]

@sipa

[hebasto]

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:

--- a/src/test/fuzz/miniscript.cpp
+++ b/src/test/fuzz/miniscript.cpp
@@ -391,6 +391,7 @@ std::optional<NodeInfo> ConsumeNodeStable(MsCtx script_ctx, FuzzedDataProvider&
     bool allow_K = (type_needed == ""_mst) || (type_needed << "K"_mst);
     bool allow_V = (type_needed == ""_mst) || (type_needed << "V"_mst);
     bool allow_W = (type_needed == ""_mst) || (type_needed << "W"_mst);
+    static constexpr auto B{"B"_mst}, K{"K"_mst}, V{"V"_mst}, W{"W"_mst};
 
     switch (provider.ConsumeIntegral<uint8_t>()) {
         case 0:
@@ -440,22 +441,22 @@ std::optional<NodeInfo> ConsumeNodeStable(MsCtx script_ctx, FuzzedDataProvider&
         }
         case 11:
             if (!(allow_B || allow_K || allow_V)) return {};
-            return {{{"B"_mst, type_needed, type_needed}, Fragment::ANDOR}};
+            return {{{B, type_needed, type_needed}, Fragment::ANDOR}};
         case 12:
             if (!(allow_B || allow_K || allow_V)) return {};
-            return {{{"V"_mst, type_needed}, Fragment::AND_V}};
+            return {{{V, type_needed}, Fragment::AND_V}};
         case 13:
             if (!allow_B) return {};
-            return {{{"B"_mst, "W"_mst}, Fragment::AND_B}};
+            return {{{B, W}, Fragment::AND_B}};
         case 15:
             if (!allow_B) return {};
-            return {{{"B"_mst, "W"_mst}, Fragment::OR_B}};
+            return {{{B, W}, Fragment::OR_B}};
         case 16:
             if (!allow_V) return {};
-            return {{{"B"_mst, "V"_mst}, Fragment::OR_C}};
+            return {{{B, V}, Fragment::OR_C}};
         case 17:
             if (!allow_B) return {};
-            return {{{"B"_mst, "B"_mst}, Fragment::OR_D}};
+            return {{{B, B}, Fragment::OR_D}};
         case 18:
             if (!(allow_B || allow_K || allow_V)) return {};
             return {{{type_needed, type_needed}, Fragment::OR_I}};
@@ -472,25 +473,25 @@ std::optional<NodeInfo> ConsumeNodeStable(MsCtx script_ctx, FuzzedDataProvider&
         }
         case 20:
             if (!allow_W) return {};
-            return {{{"B"_mst}, Fragment::WRAP_A}};
+            return {{{B}, Fragment::WRAP_A}};
         case 21:
             if (!allow_W) return {};
-            return {{{"B"_mst}, Fragment::WRAP_S}};
+            return {{{B}, Fragment::WRAP_S}};
         case 22:
             if (!allow_B) return {};
-            return {{{"K"_mst}, Fragment::WRAP_C}};
+            return {{{K}, Fragment::WRAP_C}};
         case 23:
             if (!allow_B) return {};
-            return {{{"V"_mst}, Fragment::WRAP_D}};
+            return {{{V}, Fragment::WRAP_D}};
         case 24:
             if (!allow_V) return {};
-            return {{{"B"_mst}, Fragment::WRAP_V}};
+            return {{{B}, Fragment::WRAP_V}};
         case 25:
             if (!allow_B) return {};
-            return {{{"B"_mst}, Fragment::WRAP_J}};
+            return {{{B}, Fragment::WRAP_J}};
         case 26:
             if (!allow_B) return {};
-            return {{{"B"_mst}, Fragment::WRAP_N}};
+            return {{{B}, Fragment::WRAP_N}};
         case 27: {
             if (!allow_B || !IsTapscript(script_ctx)) return {};
             const auto k = provider.ConsumeIntegral<uint16_t>();

[sipa]

@hebasto Thanks, rebased and applied.

[hebasto]

@hebasto Thanks, rebased and applied.

Sorry. It doesn't work -- #30031.

UPD. More adjustment needed.

[hebasto]

ACK 73619b8.

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.

[theuni]

utACK with or without @hebasto's additional changes.

[sipa]

Incorporated the changes from #30031.

[hebasto]

re-ACK 6331710.

[theuni]

utACK 6331710