Bump SCRIPT_VERIFY flags to 64 bit

[ajtowns]

We currently use 21 of 32 possible bits for SCRIPT_VERIFY_* flags, with open PRs that may use 8 more (#29247, #31989, #32247, #32453). The mutinynet fork that has included many experimental soft fork features is already reusing bits here. Therefore, bump this to 64 bits.

In order to make it easier to update this logic in future, this PR also introduces a dedicated type for the script flags, and disables implicit conversion between that type and the underlying integer type. To make verifying that this change doesn't cause flags to disappear, this PR also resurrects the changes from #28806 so that the script flags that are consensus enforced on each block can be queried via getdeploymentinfo.

[DrahtBot]

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

Code Coverage & Benchmarks

For details see: https://corecheck.dev/bitcoin/bitcoin/pulls/32998.

Reviews

See the guideline for information on the review process.

Type	Reviewers
ACK	darosior, instagibbs, theStack, achow101
Stale ACK	sipa

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:

• #32575 (refactor: Split multithreaded case out of CheckInputScripts by fjahr)
• #32453 ([Policy] Discourage Unsigned Annexes by JeremyRubin)
• #32317 (kernel: Separate UTXO set access from validation functions by TheCharlatan)
• #31989 (BIP-119 (OP_CHECKTEMPLATEVERIFY) (regtest only) by jamesob)
• #29843 (policy: Allow non-standard scripts with -acceptnonstdtxn=1 (test nets only) by ajtowns)
• #29247 (CAT in Tapscript (BIP-347) by arminsabouri)

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.

[ajtowns]

The "introduce script_verify_flags typename" commit is probably best reviewed with --word-diff fwiw.

cc @instagibbs @darosior @benthecarman

[DrahtBot]

🚧 At least one of the CI tasks failed.
_{Task lint: https://github.com/bitcoin/bitcoin/runs/46136681380
LLM reason (✨ experimental): Lint failure caused by missing include guard in src/script/verify_flags.h.}

Hints

Try to run the tests locally, according to the documentation. However, a CI failure may still
happen due to a number of reasons, for example:

• Possibly due to a silent merge conflict (the changes in this pull request being
  incompatible with the current code in the target branch). If so, make sure to rebase on the latest
  commit of the target branch.

• A sanitizer issue, which can only be found by compiling with the sanitizer and running the
  affected test.

• An intermittent issue.

Leave a comment here, if you need help tracking down a confusing failure.

[ajtowns]

CI failure seems to be due to a bug in qt6 6.4:

Also for people coming here from Google: I had the same error message, but for me the problem was that I was using Qt 6.4.2 which apparently thinks that #if 202002L < 201103L is true, which causes c++config.h to not be included (and no #error is generated because moc doesn't support the directive) . This has been fixed somewhere before Qt 6.7.3.

[20:04:17.396] Get:287 http://archive.ubuntu.com/ubuntu noble/universe amd64 qt6-base-dev-tools amd64 6.4.2+dfsg-21.1build5 [970 kB]
[20:04:17.414] Get:288 http://archive.ubuntu.com/ubuntu noble/universe amd64 qt6-qpa-plugins amd64 6.4.2+dfsg-21.1build5 [89.3 kB]
[20:04:17.417] Get:289 http://archive.ubuntu.com/ubuntu noble/universe amd64 qt6-base-dev amd64 6.4.2+dfsg-21.1build5 [1458 kB]
...
[20:12:02.187] Command
[20:12:02.187] -------
[20:12:02.187] /usr/lib/qt6/libexec/moc -DBOOST_MULTI_INDEX_DISABLE_SERIALIZATION -DBOOST_NO_CXX98_FUNCTION_BASE -DQT_CORE_LIB -DQT_DBUS_LIB -DQT_GUI_LIB -DQT_NETWORK_LIB -DQT_NO_DEBUG -DQT_NO_KEYWORDS -DQT_USE_QSTRINGBUILDER -DQT_WIDGETS_LIB -I/ci_container_base/ci/scratch/build-x86_64-pc-linux-gnu/src -I/ci_container_base/src -I/ci_container_base/src/univalue/include -I/ci_container_base/src/leveldb/include -I/usr/include/x86_64-linux-gnu/qt6/QtWidgets -I/usr/include/x86_64-linux-gnu/qt6 -I/usr/include/x86_64-linux-gnu/qt6/QtCore -I/usr/lib/x86_64-linux-gnu/qt6/mkspecs/linux-g++ -I/usr/include/x86_64-linux-gnu/qt6/QtGui -I/usr/include/x86_64-linux-gnu/qt6/QtNetwork -I/usr/include/x86_64-linux-gnu/qt6/QtDBus -I/usr/include -I/usr/include/c++/13 -I/usr/include/x86_64-linux-gnu/c++/13 -I/usr/include/c++/13/backward -I/usr/lib/llvm-20/lib/clang/20/include -I/usr/local/include -I/usr/include/x86_64-linux-gnu --include /ci_container_base/ci/scratch/build-x86_64-pc-linux-gnu/src/qt/bitcoinqt_autogen/moc_predefs.h -p/ci_container_base/src/qt --output-dep-file -o /ci_container_base/ci/scratch/build-x86_64-pc-linux-gnu/src/qt/bitcoinqt_autogen/include/qt/intro.moc /ci_container_base/src/qt/intro.cpp
[20:12:02.187] 
[20:12:02.187] Output
[20:12:02.187] ------
[20:12:02.187] usr/include/c++/13/concept:46:1: error: Parse error at "std"

The error output also seems to be cutting off the last letter of the filename ("concept" vs "concepts" here, and "stl_relops." vs "stl_relops.h" in the original bug report linked above).

[hebasto]

CI failure seems to be due to a bug in qt6 6.4:

Although both failed CI job use Clang 20.1.7, the error can also be reproduced with GCC 13.3.

[instagibbs]

agreed, build issue also on my end with gcc 13.3.0

[fanquake]

Minified this is:

# podman run -it ubuntu:noble

apt update && apt upgrade -y
apt install g++ qt6-base-dev qt6-tools-dev git

git clone https://github.com/bitcoin/bitcoin
cd bitcoin
git fetch origin pull/32998/head:32998
git checkout 32998

/usr/lib/qt6/libexec/moc -I/usr/include/c++/13 --output-dep-file -o test.moc /bitcoin/src/qt/intro.cpp
usr/include/c++/13/bits/cpp_type_traits.:69:1: error: Parse error at "std"

[hebasto]

CI failure seems to be due to a bug in qt6 6.4:

Although both failed CI job use Clang 20.1.7, the error can also be reproduced with GCC 13.3.

Refactoring helps.

[ajtowns]

Minified this is:

I don't think you need the git fetch; git checkout here -- you get the same error with that command (dropping the /bitcoin) on master afaics?

[fanquake]

@hebasto how can we move past the GUI build failures here? https://github.com/bitcoin/bitcoin/actions/runs/16538448423/job/46776353144?pr=32998#step:6:3769.

[hebasto]

@hebasto how can we move past the GUI build failures here? https://github.com/bitcoin/bitcoin/actions/runs/16538448423/job/46776353144?pr=32998#step:6:3769.

Sure!

CI failure seems to be due to a bug in qt6 6.4:

Although both failed CI job use Clang 20.1.7, the error can also be reproduced with GCC 13.3.

Refactoring helps.

Implemented in bitcoin-core/gui#881.

[ajtowns]

Rebased past header conflict with #33116

[sipa]

utACK 417437e

[instagibbs]

ACK 417437e

non-blocking suggestions for coverage

[darosior]

ACK 652424a

I think it would also make sense, in a follow-up, to move the rest of the Script verification flags stuff to the new verify_flags.h.

[darosior]

I was curious why this constructor from int with a runtime check was necessary. It's because of all the if ((flags & SCRIPT_VERIFY_X) != 0) in the interpreter. This permits to implicitly construct a script_verify_flags from the 0 there instead of either having an implicit conversion to integer (the very thing this aims to avoid), or touching every single of those checks to make the conversion from the 0 integer explicit.

[ajtowns]

Note that it's consteval, so the throw is a compile-time check, not a runtime one.

[darosior]

How is the rule of 5 relevant for this class? It does not need a user-defined destructor.

From cppreference's Rule of 5 section:

Because the presence of a user-defined (include = default or = delete declared) destructor, copy-constructor, or copy-assignment operator prevents implicit definition of the move constructor and the move assignment operator, any class for which move semantics are desirable, has to declare all five special member functions:

[ajtowns]

The rule of 5 just says if you define one, define/delete them all; I do it because that ensures I get a compile time error if there's some problem that makes one of them not possible. Move semantics probably aren't very interesting for a wrapper around an int though.

The C++ Core Guidelines version of the "rule of 0" says not to define any of them when possible, however.

[instagibbs]

reACK 652424a

[theStack]

Code-review ACK 652424a 🎏

[theStack]

nit: while touching, could switch to the more modern .contains

Suggested change

	if (!mapFlagNames.count(word)) {
	if (!mapFlagNames.contains(word)) {

[theStack]

nit: operator< would be sufficient (currently only needed in the transaction tests, where a std::set of script flag combinations is used), I doubt that the other ones would ever have a use-case

diff --git a/src/script/verify_flags.h b/src/script/verify_flags.h
index 95a55d2c79..e14a329ace 100644
--- a/src/script/verify_flags.h
+++ b/src/script/verify_flags.h
@@ -47,11 +47,9 @@ public:
     // tests
     constexpr explicit operator bool() const { return m_value != 0; }
     constexpr bool operator==(script_verify_flags other) const { return m_value == other.m_value; }
-
-    /** Compare two script_verify_flags. <, >, <=, and >= are auto-generated from this. */
-    friend constexpr std::strong_ordering operator<=>(const script_verify_flags& a, const script_verify_flags& b) noexcept
+    friend constexpr bool operator<(const script_verify_flags& a, const script_verify_flags& b) noexcept
     {
-        return a.m_value <=> b.m_value;
+        return a.m_value < b.m_value;
     }

 private:

[achow101]

ACK 652424a

[fanquake]

Note that this caused the MSAN Fuzz job to start failing, so is being fixed / changed again in #33600.