From bddcadee82daf3ed1441820a0ffc4c5ef78f64f1 Mon Sep 17 00:00:00 2001 From: Anthony Towns Date: Sat, 26 Jul 2025 18:51:24 +1000 Subject: [PATCH] script/verify_flags: make script_verify_flags type safe `using script_verify_flags = uint32_t` allows implicit conversion to and from int, so replace it with a class to have the compiler ensure we use the correct type. Provide from_int and as_int to allow for explicit conversions when desired. Introduces the type `script_verify_flag_name` for the individual flag name enumeration. --- src/script/interpreter.cpp | 4 +- src/script/interpreter.h | 15 ++++- src/script/verify_flags.h | 60 ++++++++++++++++++- src/test/fuzz/coins_view.cpp | 2 +- src/test/fuzz/eval_script.cpp | 2 +- src/test/fuzz/script.cpp | 4 +- .../fuzz/script_assets_test_minimizer.cpp | 2 +- src/test/fuzz/script_flags.cpp | 8 +++ src/test/fuzz/signature_checker.cpp | 2 +- src/test/script_tests.cpp | 16 ++--- src/test/transaction_tests.cpp | 8 +-- src/test/txvalidationcache_tests.cpp | 2 +- 12 files changed, 100 insertions(+), 25 deletions(-) diff --git a/src/script/interpreter.cpp b/src/script/interpreter.cpp index f6aeb5e6024..11d313928d9 100644 --- a/src/script/interpreter.cpp +++ b/src/script/interpreter.cpp @@ -2164,7 +2164,7 @@ size_t CountWitnessSigOps(const CScript& scriptSig, const CScript& scriptPubKey, } #define FLAG_NAME(flag) {std::string(#flag), SCRIPT_VERIFY_##flag} -const std::map g_verify_flag_names{ +const std::map g_verify_flag_names{ FLAG_NAME(P2SH), FLAG_NAME(STRICTENC), FLAG_NAME(DERSIG), @@ -2203,7 +2203,7 @@ std::vector GetScriptFlagNames(script_verify_flags flags) } } if (leftover != 0) { - res.push_back(strprintf("0x%08x", leftover)); + res.push_back(strprintf("0x%08x", leftover.as_int())); } return res; } diff --git a/src/script/interpreter.h b/src/script/interpreter.h index 0d1a44b1f9e..9ae29e0f1f4 100644 --- a/src/script/interpreter.h +++ b/src/script/interpreter.h @@ -14,6 +14,7 @@ #include #include +#include #include #include #include @@ -43,9 +44,10 @@ enum * All flags are intended to be soft forks: the set of acceptable scripts under * flags (A | B) is a subset of the acceptable scripts under flag (A). */ -enum : uint32_t { - SCRIPT_VERIFY_NONE = 0, +static constexpr script_verify_flags SCRIPT_VERIFY_NONE{0}; + +enum class script_verify_flag_name : uint32_t { // Evaluate P2SH subscripts (BIP16). SCRIPT_VERIFY_P2SH = (1U << 0), @@ -148,6 +150,13 @@ enum : uint32_t { // SCRIPT_VERIFY_END_MARKER }; +using enum script_verify_flag_name; + +// assert there is still a spare bit +static_assert(static_cast(SCRIPT_VERIFY_END_MARKER) < (1u << 31)); + +static constexpr script_verify_flags::value_type MAX_SCRIPT_VERIFY_FLAGS = ((static_cast(SCRIPT_VERIFY_END_MARKER) - 1) << 1) - 1; +static constexpr int MAX_SCRIPT_VERIFY_FLAGS_BITS = std::bit_width(MAX_SCRIPT_VERIFY_FLAGS); bool CheckSignatureEncoding(const std::vector &vchSig, script_verify_flags flags, ScriptError* serror); @@ -372,7 +381,7 @@ size_t CountWitnessSigOps(const CScript& scriptSig, const CScript& scriptPubKey, int FindAndDelete(CScript& script, const CScript& b); -extern const std::map g_verify_flag_names; +extern const std::map g_verify_flag_names; std::vector GetScriptFlagNames(script_verify_flags flags); diff --git a/src/script/verify_flags.h b/src/script/verify_flags.h index 360ce2b8f27..ba75f0894be 100644 --- a/src/script/verify_flags.h +++ b/src/script/verify_flags.h @@ -6,8 +6,66 @@ #ifndef BITCOIN_SCRIPT_VERIFY_FLAGS_H #define BITCOIN_SCRIPT_VERIFY_FLAGS_H +#include #include -using script_verify_flags = uint32_t; +enum class script_verify_flag_name : uint32_t; + +class script_verify_flags +{ +public: + using value_type = uint32_t; + + consteval script_verify_flags() = default; + + // also allow construction with hard-coded 0 (but not other integers) + consteval explicit(false) script_verify_flags(value_type f) : m_value{f} { if (f != 0) throw 0; } + + // implicit construction from a hard-coded SCRIPT_VERIFY_* constant is also okay + constexpr explicit(false) script_verify_flags(script_verify_flag_name f) : m_value{static_cast(f)} { } + + // rule of 5 + constexpr script_verify_flags(const script_verify_flags&) = default; + constexpr script_verify_flags(script_verify_flags&&) = default; + constexpr script_verify_flags& operator=(const script_verify_flags&) = default; + constexpr script_verify_flags& operator=(script_verify_flags&&) = default; + constexpr ~script_verify_flags() = default; + + // integer conversion needs to be very explicit + static constexpr script_verify_flags from_int(value_type f) { script_verify_flags r; r.m_value = f; return r; } + constexpr value_type as_int() const { return m_value; } + + // bitwise operations + constexpr script_verify_flags operator~() const { return from_int(~m_value); } + friend constexpr script_verify_flags operator|(script_verify_flags a, script_verify_flags b) { return from_int(a.m_value | b.m_value); } + friend constexpr script_verify_flags operator&(script_verify_flags a, script_verify_flags b) { return from_int(a.m_value & b.m_value); } + + // in-place bitwise operations + constexpr script_verify_flags& operator|=(script_verify_flags vf) { m_value |= vf.m_value; return *this; } + constexpr script_verify_flags& operator&=(script_verify_flags vf) { m_value &= vf.m_value; return *this; } + + // 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 + { + return a.m_value <=> b.m_value; + } + +private: + value_type m_value{0}; // default value is SCRIPT_VERIFY_NONE +}; + +inline constexpr script_verify_flags operator~(script_verify_flag_name f) +{ + return ~script_verify_flags{f}; +} + +inline constexpr script_verify_flags operator|(script_verify_flag_name f1, script_verify_flag_name f2) +{ + return script_verify_flags{f1} | f2; +} #endif // BITCOIN_SCRIPT_VERIFY_FLAGS_H diff --git a/src/test/fuzz/coins_view.cpp b/src/test/fuzz/coins_view.cpp index 2b9b81e0306..dacef9a70ad 100644 --- a/src/test/fuzz/coins_view.cpp +++ b/src/test/fuzz/coins_view.cpp @@ -288,7 +288,7 @@ void TestCoinsView(FuzzedDataProvider& fuzzed_data_provider, CCoinsView& backend // consensus/tx_verify.cpp:130: unsigned int GetP2SHSigOpCount(const CTransaction &, const CCoinsViewCache &): Assertion `!coin.IsSpent()' failed. return; } - const auto flags{fuzzed_data_provider.ConsumeIntegral()}; + const auto flags = script_verify_flags::from_int(fuzzed_data_provider.ConsumeIntegral()); if (!transaction.vin.empty() && (flags & SCRIPT_VERIFY_WITNESS) != 0 && (flags & SCRIPT_VERIFY_P2SH) == 0) { // Avoid: // script/interpreter.cpp:1705: size_t CountWitnessSigOps(const CScript &, const CScript &, const CScriptWitness *, unsigned int): Assertion `(flags & SCRIPT_VERIFY_P2SH) != 0' failed. diff --git a/src/test/fuzz/eval_script.cpp b/src/test/fuzz/eval_script.cpp index 205e4f526d3..43e3546627f 100644 --- a/src/test/fuzz/eval_script.cpp +++ b/src/test/fuzz/eval_script.cpp @@ -12,7 +12,7 @@ FUZZ_TARGET(eval_script) { FuzzedDataProvider fuzzed_data_provider(buffer.data(), buffer.size()); - const script_verify_flags flags = fuzzed_data_provider.ConsumeIntegral(); + const auto flags = script_verify_flags::from_int(fuzzed_data_provider.ConsumeIntegral()); const std::vector script_bytes = [&] { if (fuzzed_data_provider.remaining_bytes() != 0) { return fuzzed_data_provider.ConsumeRemainingBytes(); diff --git a/src/test/fuzz/script.cpp b/src/test/fuzz/script.cpp index 811f0141fd3..dfad0b71848 100644 --- a/src/test/fuzz/script.cpp +++ b/src/test/fuzz/script.cpp @@ -118,8 +118,8 @@ FUZZ_TARGET(script, .init = initialize_script) (void)FindAndDelete(script_mut, *other_script); } const std::vector random_string_vector = ConsumeRandomLengthStringVector(fuzzed_data_provider); - const uint32_t u32{fuzzed_data_provider.ConsumeIntegral()}; - const script_verify_flags flags{u32 | SCRIPT_VERIFY_P2SH}; + const auto flags_rand{fuzzed_data_provider.ConsumeIntegral()}; + const auto flags = script_verify_flags::from_int(flags_rand) | SCRIPT_VERIFY_P2SH; { CScriptWitness wit; for (const auto& s : random_string_vector) { diff --git a/src/test/fuzz/script_assets_test_minimizer.cpp b/src/test/fuzz/script_assets_test_minimizer.cpp index ba6d8239245..b26aa84e6eb 100644 --- a/src/test/fuzz/script_assets_test_minimizer.cpp +++ b/src/test/fuzz/script_assets_test_minimizer.cpp @@ -90,7 +90,7 @@ CScriptWitness ScriptWitnessFromJSON(const UniValue& univalue) return scriptwitness; } -const std::map FLAG_NAMES = { +const std::map FLAG_NAMES = { {std::string("P2SH"), SCRIPT_VERIFY_P2SH}, {std::string("DERSIG"), SCRIPT_VERIFY_DERSIG}, {std::string("NULLDUMMY"), SCRIPT_VERIFY_NULLDUMMY}, diff --git a/src/test/fuzz/script_flags.cpp b/src/test/fuzz/script_flags.cpp index 7e28b52fdc8..0920f7a408e 100644 --- a/src/test/fuzz/script_flags.cpp +++ b/src/test/fuzz/script_flags.cpp @@ -15,6 +15,14 @@ #include #include +static DataStream& operator>>(DataStream& ds, script_verify_flags& f) +{ + script_verify_flags::value_type n{0}; + ds >> n; + f = script_verify_flags::from_int(n); + return ds; +} + FUZZ_TARGET(script_flags) { if (buffer.size() > 100'000) return; diff --git a/src/test/fuzz/signature_checker.cpp b/src/test/fuzz/signature_checker.cpp index d1591464f2a..d38dbf2ae08 100644 --- a/src/test/fuzz/signature_checker.cpp +++ b/src/test/fuzz/signature_checker.cpp @@ -51,7 +51,7 @@ public: FUZZ_TARGET(signature_checker) { FuzzedDataProvider fuzzed_data_provider(buffer.data(), buffer.size()); - const script_verify_flags flags = fuzzed_data_provider.ConsumeIntegral(); + const auto flags = script_verify_flags::from_int(fuzzed_data_provider.ConsumeIntegral()); const SigVersion sig_version = fuzzed_data_provider.PickValueInArray({SigVersion::BASE, SigVersion::WITNESS_V0}); const auto script_1{ConsumeScript(fuzzed_data_provider)}; const auto script_2{ConsumeScript(fuzzed_data_provider)}; diff --git a/src/test/script_tests.cpp b/src/test/script_tests.cpp index 561660464a9..c1a9947395a 100644 --- a/src/test/script_tests.cpp +++ b/src/test/script_tests.cpp @@ -96,7 +96,7 @@ static ScriptErrorDesc script_errors[]={ {SCRIPT_ERR_SIG_FINDANDDELETE, "SIG_FINDANDDELETE"}, }; -static std::string FormatScriptFlags(uint32_t flags) +static std::string FormatScriptFlags(script_verify_flags flags) { return util::Join(GetScriptFlagNames(flags), ","); } @@ -134,13 +134,13 @@ void DoTest(const CScript& scriptPubKey, const CScript& scriptSig, const CScript BOOST_CHECK_MESSAGE(err == scriptError, FormatScriptError(err) + " where " + FormatScriptError((ScriptError_t)scriptError) + " expected: " + message); // Verify that removing flags from a passing test or adding flags to a failing test does not change the result. - for (int i = 0; i < 16; ++i) { - uint32_t extra_flags(m_rng.randbits(16)); - uint32_t combined_flags{expect ? (flags & ~extra_flags) : (flags | extra_flags)}; + for (int i = 0; i < 256; ++i) { + script_verify_flags extra_flags = script_verify_flags::from_int(m_rng.randbits(MAX_SCRIPT_VERIFY_FLAGS_BITS)); + script_verify_flags combined_flags{expect ? (flags & ~extra_flags) : (flags | extra_flags)}; // Weed out some invalid flag combinations. if (combined_flags & SCRIPT_VERIFY_CLEANSTACK && ~combined_flags & (SCRIPT_VERIFY_P2SH | SCRIPT_VERIFY_WITNESS)) continue; if (combined_flags & SCRIPT_VERIFY_WITNESS && ~combined_flags & SCRIPT_VERIFY_P2SH) continue; - BOOST_CHECK_MESSAGE(VerifyScript(scriptSig, scriptPubKey, &scriptWitness, combined_flags, MutableTransactionSignatureChecker(&tx, 0, txCredit.vout[0].nValue, MissingDataBehavior::ASSERT_FAIL), &err) == expect, message + strprintf(" (with flags %x)", combined_flags)); + BOOST_CHECK_MESSAGE(VerifyScript(scriptSig, scriptPubKey, &scriptWitness, combined_flags, MutableTransactionSignatureChecker(&tx, 0, txCredit.vout[0].nValue, MissingDataBehavior::ASSERT_FAIL), &err) == expect, message + strprintf(" (with flags %x)", combined_flags.as_int())); } } }; // struct ScriptTest @@ -1716,9 +1716,9 @@ BOOST_AUTO_TEST_CASE(formatscriptflags) { // quick check that FormatScriptFlags reports any unknown/unexpected bits BOOST_CHECK_EQUAL(FormatScriptFlags(SCRIPT_VERIFY_P2SH), "P2SH"); - BOOST_CHECK_EQUAL(FormatScriptFlags(SCRIPT_VERIFY_P2SH | (1u<<31)), "P2SH,0x80000000"); - BOOST_CHECK_EQUAL(FormatScriptFlags(SCRIPT_VERIFY_TAPROOT | (1u<<27)), "TAPROOT,0x08000000"); - BOOST_CHECK_EQUAL(FormatScriptFlags(1u<<26), "0x04000000"); + BOOST_CHECK_EQUAL(FormatScriptFlags(SCRIPT_VERIFY_P2SH | script_verify_flags::from_int(1u<<31)), "P2SH,0x80000000"); + BOOST_CHECK_EQUAL(FormatScriptFlags(SCRIPT_VERIFY_TAPROOT | script_verify_flags::from_int(1u<<27)), "TAPROOT,0x08000000"); + BOOST_CHECK_EQUAL(FormatScriptFlags(script_verify_flags::from_int(1u<<26)), "0x04000000"); } BOOST_AUTO_TEST_SUITE_END() diff --git a/src/test/transaction_tests.cpp b/src/test/transaction_tests.cpp index c2f64cc8217..b2a70057d5b 100644 --- a/src/test/transaction_tests.cpp +++ b/src/test/transaction_tests.cpp @@ -50,7 +50,7 @@ typedef std::vector valtype; static CFeeRate g_dust{DUST_RELAY_TX_FEE}; static bool g_bare_multi{DEFAULT_PERMIT_BAREMULTISIG}; -static const std::map& mapFlagNames = g_verify_flag_names; +static const std::map& mapFlagNames = g_verify_flag_names; script_verify_flags ParseScriptFlags(std::string strFlags) { @@ -230,9 +230,9 @@ BOOST_AUTO_TEST_CASE(tx_valid) BOOST_ERROR("Tx unexpectedly failed with flag " << name << " unset: " << strTest); } // Removing random combinations of flags - flags = TrimFlags(~(verify_flags | (unsigned int)m_rng.randbits(mapFlagNames.size()))); + flags = TrimFlags(~(verify_flags | script_verify_flags::from_int(m_rng.randbits(MAX_SCRIPT_VERIFY_FLAGS_BITS)))); if (!CheckTxScripts(tx, mapprevOutScriptPubKeys, mapprevOutValues, flags, txdata, strTest, /*expect_valid=*/true)) { - BOOST_ERROR("Tx unexpectedly failed with random flags " << ToString(flags) << ": " << strTest); + BOOST_ERROR("Tx unexpectedly failed with random flags " << ToString(flags.as_int()) << ": " << strTest); } } @@ -321,7 +321,7 @@ BOOST_AUTO_TEST_CASE(tx_invalid) BOOST_ERROR("Tx unexpectedly passed with flag " << name << " set: " << strTest); } // Adding random combinations of flags - flags = FillFlags(verify_flags | (unsigned int)m_rng.randbits(mapFlagNames.size())); + flags = FillFlags(verify_flags | script_verify_flags::from_int(m_rng.randbits(MAX_SCRIPT_VERIFY_FLAGS_BITS))); if (!CheckTxScripts(tx, mapprevOutScriptPubKeys, mapprevOutValues, flags, txdata, strTest, /*expect_valid=*/false)) { BOOST_ERROR("Tx unexpectedly passed with random flags " << name << ": " << strTest); } diff --git a/src/test/txvalidationcache_tests.cpp b/src/test/txvalidationcache_tests.cpp index 8c14dd65a14..46cb84824d7 100644 --- a/src/test/txvalidationcache_tests.cpp +++ b/src/test/txvalidationcache_tests.cpp @@ -130,7 +130,7 @@ static void ValidateCheckInputsForAllFlags(const CTransaction &tx, script_verify TxValidationState state; // Randomly selects flag combinations - script_verify_flags test_flags = (uint32_t) insecure_rand.randrange((SCRIPT_VERIFY_END_MARKER - 1) << 1); + script_verify_flags test_flags = script_verify_flags::from_int(insecure_rand.randrange(MAX_SCRIPT_VERIFY_FLAGS)); // Filter out incompatible flag choices if ((test_flags & SCRIPT_VERIFY_CLEANSTACK)) {