Merge bitcoin/bitcoin#21280: test: bug fix in transaction_tests

b109bde46a8730afbc09c107b802a2c321293f4b [test] check that mapFlagNames is up to date (glozow)
5d3ced72f9b5f36db1a76bd8bc918d11b87dfd72 [test] remove unnecessary OP_1s from invalid tests (glozow)
5aee73d1759bcc0d1e951776942e616843934af1 [test] minor improvements / followups (glozow)
8a365df5586b36d1772c78069f9d93c56a81df6f [test] fix bug in ExcludeIndividualFlags (glozow)
8cac2923f57ac33848ff41b74c3be520b75936df [test] remove invalid test from tx_valid.json (glozow)

Pull request description:

  This is a followup to #19698.

  - There was a bug in the `ExcludeIndividualFlags` function which is fixed here.
  - Fixing this bug also showed that there is a test that's supposed to fail (already existing in tx_invalid.json) in tx_valid.json, so I removed it. Other than that, the tests should all pass.
  - Also implements a few suggestions I received offline: removing the `OP_1`s from the invalid tests (similar to https://github.com/bitcoin/bitcoin/commit/19db590d044efe7d474a16720e5b56e7b55db54c), comments, and style.
  - A few other small fixes, like adding asserts, putting all the flags in `mapFlagNames`, better error messages

ACKs for top commit:
  jnewbery:
    utACK b109bde46a8730afbc09c107b802a2c321293f4b

Tree-SHA512: 7233a8c0f1ae1172fac8000ea6e05384ecf79074c39948d118464868505c7f02f17e96503c81bd05c07adb2087648a5d93d9899e16fdefa6b7efcb51319444a9

3 files changed, 66 insertions, 47 deletions

diff --git a/src/test/data/tx_invalid.json b/src/test/data/tx_invalid.json
index c394356798..41bebab2a0 100644
--- a/src/test/data/tx_invalid.json
+++ b/src/test/data/tx_invalid.json
@@ -124,15 +124,15 @@
 ["CHECKLOCKTIMEVERIFY tests"],
 
 ["By-height locks, with argument just beyond tx nLockTime"],
-[[["0000000000000000000000000000000000000000000000000000000000000100", 0, "1 CHECKLOCKTIMEVERIFY 1"]],
+[[["0000000000000000000000000000000000000000000000000000000000000100", 0, "1 CHECKLOCKTIMEVERIFY"]],
 "010000000100010000000000000000000000000000000000000000000000000000000000000000000000000000000100000000000000000000000000", "CHECKLOCKTIMEVERIFY"],
-[[["0000000000000000000000000000000000000000000000000000000000000100", 0, "499999999 CHECKLOCKTIMEVERIFY 1"]],
+[[["0000000000000000000000000000000000000000000000000000000000000100", 0, "499999999 CHECKLOCKTIMEVERIFY"]],
 "0100000001000100000000000000000000000000000000000000000000000000000000000000000000000000000001000000000000000000fe64cd1d", "CHECKLOCKTIMEVERIFY"],
 
 ["By-time locks, with argument just beyond tx nLockTime (but within numerical boundaries)"],
-[[["0000000000000000000000000000000000000000000000000000000000000100", 0, "500000001 CHECKLOCKTIMEVERIFY 1"]],
+[[["0000000000000000000000000000000000000000000000000000000000000100", 0, "500000001 CHECKLOCKTIMEVERIFY"]],
 "01000000010001000000000000000000000000000000000000000000000000000000000000000000000000000000010000000000000000000065cd1d", "CHECKLOCKTIMEVERIFY"],
-[[["0000000000000000000000000000000000000000000000000000000000000100", 0, "4294967295 CHECKLOCKTIMEVERIFY 1"]],
+[[["0000000000000000000000000000000000000000000000000000000000000100", 0, "4294967295 CHECKLOCKTIMEVERIFY"]],
 "0100000001000100000000000000000000000000000000000000000000000000000000000000000000000000000001000000000000000000feffffff", "CHECKLOCKTIMEVERIFY"],
 
 ["Argument missing"],
@@ -142,11 +142,11 @@
 "010000000100010000000000000000000000000000000000000000000000000000000000000000000001b1010000000100000000000000000000000000", "CHECKLOCKTIMEVERIFY"],
 
 ["Argument negative with by-blockheight nLockTime=0"],
-[[["0000000000000000000000000000000000000000000000000000000000000100", 0, "-1 CHECKLOCKTIMEVERIFY 1"]],
+[[["0000000000000000000000000000000000000000000000000000000000000100", 0, "-1 CHECKLOCKTIMEVERIFY"]],
 "010000000100010000000000000000000000000000000000000000000000000000000000000000000000000000000100000000000000000000000000", "CHECKLOCKTIMEVERIFY"],
 
 ["Argument negative with by-blocktime nLockTime=500,000,000"],
-[[["0000000000000000000000000000000000000000000000000000000000000100", 0, "-1 CHECKLOCKTIMEVERIFY 1"]],
+[[["0000000000000000000000000000000000000000000000000000000000000100", 0, "-1 CHECKLOCKTIMEVERIFY"]],
 "01000000010001000000000000000000000000000000000000000000000000000000000000000000000000000000010000000000000000000065cd1d", "CHECKLOCKTIMEVERIFY"],
 [[["0000000000000000000000000000000000000000000000000000000000000100", 0, "1"]],
 "010000000100010000000000000000000000000000000000000000000000000000000000000000000004005194b1010000000100000000000000000002000000", "CHECKLOCKTIMEVERIFY"],
@@ -167,19 +167,19 @@
 "01000000010001000000000000000000000000000000000000000000000000000000000000000000000000000000010000000000000000000065cd1d", "CHECKLOCKTIMEVERIFY"],
 [[["0000000000000000000000000000000000000000000000000000000000000100", 0, "0"]],
 "01000000010001000000000000000000000000000000000000000000000000000000000000000000000251b100000000010000000000000000000065cd1d", "NONE"],
-[[["0000000000000000000000000000000000000000000000000000000000000100", 0, "499999999 CHECKLOCKTIMEVERIFY 1"]],
+[[["0000000000000000000000000000000000000000000000000000000000000100", 0, "499999999 CHECKLOCKTIMEVERIFY"]],
 "01000000010001000000000000000000000000000000000000000000000000000000000000000000000000000000010000000000000000000065cd1d", "CHECKLOCKTIMEVERIFY"],
-[[["0000000000000000000000000000000000000000000000000000000000000100", 0, "500000000 CHECKLOCKTIMEVERIFY 1"]],
+[[["0000000000000000000000000000000000000000000000000000000000000100", 0, "500000000 CHECKLOCKTIMEVERIFY"]],
 "010000000100010000000000000000000000000000000000000000000000000000000000000000000000000000000100000000000000000000000000", "CHECKLOCKTIMEVERIFY"],
-[[["0000000000000000000000000000000000000000000000000000000000000100", 0, "500000000 CHECKLOCKTIMEVERIFY 1"]],
+[[["0000000000000000000000000000000000000000000000000000000000000100", 0, "500000000 CHECKLOCKTIMEVERIFY"]],
 "0100000001000100000000000000000000000000000000000000000000000000000000000000000000000000000001000000000000000000ff64cd1d", "CHECKLOCKTIMEVERIFY"],
 
 ["Argument 2^32 with nLockTime=2^32-1"],
-[[["0000000000000000000000000000000000000000000000000000000000000100", 0, "0x050000000001 CHECKLOCKTIMEVERIFY 1"]],
+[[["0000000000000000000000000000000000000000000000000000000000000100", 0, "0x050000000001 CHECKLOCKTIMEVERIFY"]],
 "0100000001000100000000000000000000000000000000000000000000000000000000000000000000000000000001000000000000000000ffffffff", "CHECKLOCKTIMEVERIFY"],
 
 ["Same, but with nLockTime=2^31-1"],
-[[["0000000000000000000000000000000000000000000000000000000000000100", 0, "2147483648 CHECKLOCKTIMEVERIFY 1"]],
+[[["0000000000000000000000000000000000000000000000000000000000000100", 0, "2147483648 CHECKLOCKTIMEVERIFY"]],
 "0100000001000100000000000000000000000000000000000000000000000000000000000000000000000000000001000000000000000000ffffff7f", "CHECKLOCKTIMEVERIFY"],
 
 ["6 byte non-minimally-encoded arguments are invalid even if their contents are valid"],
@@ -201,15 +201,15 @@
 ["CHECKSEQUENCEVERIFY tests"],
 
 ["By-height locks, with argument just beyond txin.nSequence"],
-[[["0000000000000000000000000000000000000000000000000000000000000100", 0, "1 CHECKSEQUENCEVERIFY 1"]],
+[[["0000000000000000000000000000000000000000000000000000000000000100", 0, "1 CHECKSEQUENCEVERIFY"]],
 "020000000100010000000000000000000000000000000000000000000000000000000000000000000000000000000100000000000000000000000000", "CHECKSEQUENCEVERIFY"],
-[[["0000000000000000000000000000000000000000000000000000000000000100", 0, "4259839 CHECKSEQUENCEVERIFY 1"]],
+[[["0000000000000000000000000000000000000000000000000000000000000100", 0, "4259839 CHECKSEQUENCEVERIFY"]],
 "020000000100010000000000000000000000000000000000000000000000000000000000000000000000feff40000100000000000000000000000000", "CHECKSEQUENCEVERIFY"],
 
 ["By-time locks, with argument just beyond txin.nSequence (but within numerical boundaries)"],
-[[["0000000000000000000000000000000000000000000000000000000000000100", 0, "4194305 CHECKSEQUENCEVERIFY 1"]],
+[[["0000000000000000000000000000000000000000000000000000000000000100", 0, "4194305 CHECKSEQUENCEVERIFY"]],
 "020000000100010000000000000000000000000000000000000000000000000000000000000000000000000040000100000000000000000000000000", "CHECKSEQUENCEVERIFY"],
-[[["0000000000000000000000000000000000000000000000000000000000000100", 0, "4259839 CHECKSEQUENCEVERIFY 1"]],
+[[["0000000000000000000000000000000000000000000000000000000000000100", 0, "4259839 CHECKSEQUENCEVERIFY"]],
 "020000000100010000000000000000000000000000000000000000000000000000000000000000000000feff40000100000000000000000000000000", "CHECKSEQUENCEVERIFY"],
 
 ["Argument missing"],
@@ -217,21 +217,21 @@
 "020000000100010000000000000000000000000000000000000000000000000000000000000000000000000000000100000000000000000000000000", "CHECKSEQUENCEVERIFY"],
 
 ["Argument negative with by-blockheight txin.nSequence=0"],
-[[["0000000000000000000000000000000000000000000000000000000000000100", 0, "-1 CHECKSEQUENCEVERIFY 1"]],
+[[["0000000000000000000000000000000000000000000000000000000000000100", 0, "-1 CHECKSEQUENCEVERIFY"]],
 "020000000100010000000000000000000000000000000000000000000000000000000000000000000000000000000100000000000000000000000000", "CHECKSEQUENCEVERIFY"],
 
 ["Argument negative with by-blocktime txin.nSequence=CTxIn::SEQUENCE_LOCKTIME_TYPE_FLAG"],
-[[["0000000000000000000000000000000000000000000000000000000000000100", 0, "-1 CHECKSEQUENCEVERIFY 1"]],
+[[["0000000000000000000000000000000000000000000000000000000000000100", 0, "-1 CHECKSEQUENCEVERIFY"]],
 "020000000100010000000000000000000000000000000000000000000000000000000000000000000000000040000100000000000000000000000000", "CHECKSEQUENCEVERIFY"],
 
 ["Argument/tx height/time mismatch, both versions"],
 [[["0000000000000000000000000000000000000000000000000000000000000100", 0, "0 CHECKSEQUENCEVERIFY 1"]],
 "020000000100010000000000000000000000000000000000000000000000000000000000000000000000000040000100000000000000000000000000", "CHECKSEQUENCEVERIFY"],
-[[["0000000000000000000000000000000000000000000000000000000000000100", 0, "65535 CHECKSEQUENCEVERIFY 1"]],
+[[["0000000000000000000000000000000000000000000000000000000000000100", 0, "65535 CHECKSEQUENCEVERIFY"]],
 "020000000100010000000000000000000000000000000000000000000000000000000000000000000000000040000100000000000000000000000000", "CHECKSEQUENCEVERIFY"],
-[[["0000000000000000000000000000000000000000000000000000000000000100", 0, "4194304 CHECKSEQUENCEVERIFY 1"]],
+[[["0000000000000000000000000000000000000000000000000000000000000100", 0, "4194304 CHECKSEQUENCEVERIFY"]],
 "020000000100010000000000000000000000000000000000000000000000000000000000000000000000000000000100000000000000000000000000", "CHECKSEQUENCEVERIFY"],
-[[["0000000000000000000000000000000000000000000000000000000000000100", 0, "4259839 CHECKSEQUENCEVERIFY 1"]],
+[[["0000000000000000000000000000000000000000000000000000000000000100", 0, "4259839 CHECKSEQUENCEVERIFY"]],
 "020000000100010000000000000000000000000000000000000000000000000000000000000000000000000000000100000000000000000000000000", "CHECKSEQUENCEVERIFY"],
 
 ["6 byte non-minimally-encoded arguments are invalid even if their contents are valid"],
@@ -249,7 +249,7 @@
 ["Failure due to insufficient tx.nVersion (<2)"],
 [[["0000000000000000000000000000000000000000000000000000000000000100", 0, "0 CHECKSEQUENCEVERIFY 1"]],
 "010000000100010000000000000000000000000000000000000000000000000000000000000000000000000000000100000000000000000000000000", "CHECKSEQUENCEVERIFY"],
-[[["0000000000000000000000000000000000000000000000000000000000000100", 0, "4194304 CHECKSEQUENCEVERIFY 1"]],
+[[["0000000000000000000000000000000000000000000000000000000000000100", 0, "4194304 CHECKSEQUENCEVERIFY"]],
 "010000000100010000000000000000000000000000000000000000000000000000000000000000000000000040000100000000000000000000000000", "CHECKSEQUENCEVERIFY"],
 
 ["Unknown witness program version (with DISCOURAGE_UPGRADABLE_WITNESS_PROGRAM)"],
diff --git a/src/test/data/tx_valid.json b/src/test/data/tx_valid.json
index 2727af5abd..b874f6f26c 100644
--- a/src/test/data/tx_valid.json
+++ b/src/test/data/tx_valid.json
@@ -62,10 +62,6 @@
   ["c76168ef1a272a4f176e55e73157ecfce040cfad16a5272f6296eb7089dca846", 1, "DUP HASH160 0x14 0x34fea2c5a75414fd945273ae2d029ce1f28dafcf EQUALVERIFY CHECKSIG"]],
 "010000000390d31c6107013d754529d8818eff285fe40a3e7635f6930fec5d12eb02107a43010000006b483045022100f40815ae3c81a0dd851cc8d376d6fd226c88416671346a9033468cca2cdcc6c202204f764623903e6c4bed1b734b75d82c40f1725e4471a55ad4f51218f86130ac038321033d710ab45bb54ac99618ad23b3c1da661631aa25f23bfe9d22b41876f1d46e4effffffff3ff04a68e22bdd52e7c8cb848156d2d158bd5515b3c50adabc87d0ca2cd3482d010000006a4730440220598d263c107004008e9e26baa1e770be30fd31ee55ded1898f7c00da05a75977022045536bead322ca246779698b9c3df3003377090f41afeca7fb2ce9e328ec4af2832102b738b531def73020bd637f32935924cc88549c8206976226d968edd3a42fc2d7ffffffff46a8dc8970eb96622f27a516adcf40e0fcec5731e7556e174f2a271aef6861c7010000006b483045022100c5b90a777a9fdc90c208dbef7290d1fc1be651f47151ee4ccff646872a454cf90220640cfbc4550446968fbbe9d12528f3adf7d87b31541569c59e790db8a220482583210391332546e22bbe8fe3af54addfad6f8b83d05fa4f5e047593d4c07ae938795beffffffff028036be26000000001976a914ddfb29efad43a667465ac59ff14dc6442a1adfca88ac3d5cba01000000001976a914b64dde7a505a13ca986c40e86e984a8dc81368b688ac00000000", "NONE"],
 
-["An invalid P2SH Transaction"],
-[[["0000000000000000000000000000000000000000000000000000000000000100", 0, "HASH160 0x14 0x7a052c840ba73af26755de42cf01cc9e0a49fef0 EQUAL"]],
-"010000000100010000000000000000000000000000000000000000000000000000000000000000000009085768617420697320ffffffff010000000000000000015100000000", "P2SH,CLEANSTACK,WITNESS"],
-
 ["A valid P2SH Transaction using the standard transaction type put forth in BIP 16"],
 [[["0000000000000000000000000000000000000000000000000000000000000100", 0, "HASH160 0x14 0x8febbed40483661de6958d957412f82deed8e2f7 EQUAL"]],
 "01000000010001000000000000000000000000000000000000000000000000000000000000000000006e493046022100c66c9cdf4c43609586d15424c54707156e316d88b0a1534c9e6b0d4f311406310221009c0fe51dbc9c4ab7cc25d3fdbeccf6679fe6827f08edf2b4a9f16ee3eb0e438a0123210338e8034509af564c62644c07691942e0c056752008a173c89f60ab2a88ac2ebfacffffffff010000000000000000015100000000", "LOW_S"],
diff --git a/src/test/transaction_tests.cpp b/src/test/transaction_tests.cpp
index aaa6caa4f1..9bbf9567f5 100644
--- a/src/test/transaction_tests.cpp
+++ b/src/test/transaction_tests.cpp
@@ -20,6 +20,7 @@
 #include <script/signingprovider.h>
 #include <script/standard.h>
 #include <streams.h>
+#include <test/util/script.h>
 #include <test/util/transaction_utils.h>
 #include <util/strencodings.h>
 #include <util/string.h>
@@ -59,6 +60,9 @@ static std::map<std::string, unsigned int> mapFlagNames = {
     {std::string("WITNESS_PUBKEYTYPE"), (unsigned int)SCRIPT_VERIFY_WITNESS_PUBKEYTYPE},
     {std::string("CONST_SCRIPTCODE"), (unsigned int)SCRIPT_VERIFY_CONST_SCRIPTCODE},
     {std::string("TAPROOT"), (unsigned int)SCRIPT_VERIFY_TAPROOT},
+    {std::string("DISCOURAGE_UPGRADABLE_PUBKEYTYPE"), (unsigned int)SCRIPT_VERIFY_DISCOURAGE_UPGRADABLE_PUBKEYTYPE},
+    {std::string("DISCOURAGE_OP_SUCCESS"), (unsigned int)SCRIPT_VERIFY_DISCOURAGE_OP_SUCCESS},
+    {std::string("DISCOURAGE_UPGRADABLE_TAPROOT_VERSION"), (unsigned int)SCRIPT_VERIFY_DISCOURAGE_UPGRADABLE_TAPROOT_VERSION},
 };
 
 unsigned int ParseScriptFlags(std::string strFlags)
@@ -78,6 +82,16 @@ unsigned int ParseScriptFlags(std::string strFlags)
     return flags;
 }
 
+// Check that all flags in STANDARD_SCRIPT_VERIFY_FLAGS are present in mapFlagNames.
+bool CheckMapFlagNames()
+{
+    unsigned int standard_flags_missing{STANDARD_SCRIPT_VERIFY_FLAGS};
+    for (const auto& pair : mapFlagNames) {
+        standard_flags_missing &= ~(pair.second);
+    }
+    return standard_flags_missing == 0;
+}
+
 std::string FormatScriptFlags(unsigned int flags)
 {
     if (flags == 0) {
@@ -139,6 +153,7 @@ unsigned int TrimFlags(unsigned int flags)
 
     // CLEANSTACK requires WITNESS (and transitively CLEANSTACK requires P2SH)
     if (!(flags & SCRIPT_VERIFY_WITNESS)) flags &= ~(unsigned int)SCRIPT_VERIFY_CLEANSTACK;
+    Assert(IsValidFlagCombination(flags));
     return flags;
 }
 
@@ -149,17 +164,21 @@ unsigned int FillFlags(unsigned int flags)
 
     // WITNESS implies P2SH (and transitively CLEANSTACK implies P2SH)
     if (flags & SCRIPT_VERIFY_WITNESS) flags |= SCRIPT_VERIFY_P2SH;
+    Assert(IsValidFlagCombination(flags));
     return flags;
 }
 
-// Return valid flags that are all except one flag for each flag
-std::vector<unsigned int> ExcludeIndividualFlags(unsigned int flags)
+// Exclude each possible script verify flag from flags. Returns a set of these flag combinations
+// that are valid and without duplicates. For example: if flags=1111 and the 4 possible flags are
+// 0001, 0010, 0100, and 1000, this should return the set {0111, 1011, 1101, 1110}.
+// Assumes that mapFlagNames contains all script verify flags.
+std::set<unsigned int> ExcludeIndividualFlags(unsigned int flags)
 {
-    std::vector<unsigned int> flags_combos;
-    for (unsigned int i = 0; i < mapFlagNames.size(); ++i) {
-        const unsigned int flags_excluding_i = TrimFlags(flags & ~(1U << i));
-        if (flags != flags_excluding_i && std::find(flags_combos.begin(), flags_combos.end(), flags_excluding_i) != flags_combos.end()) {
-            flags_combos.push_back(flags_excluding_i);
+    std::set<unsigned int> flags_combos;
+    for (const auto& pair : mapFlagNames) {
+        const unsigned int flags_excluding_one = TrimFlags(flags & ~(pair.second));
+        if (flags != flags_excluding_one) {
+            flags_combos.insert(flags_excluding_one);
         }
     }
     return flags_combos;
@@ -169,6 +188,7 @@ BOOST_FIXTURE_TEST_SUITE(transaction_tests, BasicTestingSetup)
 
 BOOST_AUTO_TEST_CASE(tx_valid)
 {
+    BOOST_CHECK_MESSAGE(CheckMapFlagNames(), "mapFlagNames is missing a script verification flag");
     // Read tests from test/data/tx_valid.json
     UniValue tests = read_json(std::string(json_tests::tx_valid, json_tests::tx_valid + sizeof(json_tests::tx_valid)));
 
@@ -228,16 +248,15 @@ BOOST_AUTO_TEST_CASE(tx_valid)
                 BOOST_ERROR("Bad test flags: " << strTest);
             }
 
-            if (!CheckTxScripts(tx, mapprevOutScriptPubKeys, mapprevOutValues, ~verify_flags, txdata, strTest, /* expect_valid */ true)) {
-                BOOST_ERROR("Tx unexpectedly failed: " << strTest);
-            }
+            BOOST_CHECK_MESSAGE(CheckTxScripts(tx, mapprevOutScriptPubKeys, mapprevOutValues, ~verify_flags, txdata, strTest, /* expect_valid */ true),
+                                "Tx unexpectedly failed: " << strTest);
 
             // Backwards compatibility of script verification flags: Removing any flag(s) should not invalidate a valid transaction
-            for (size_t i = 0; i < mapFlagNames.size(); ++i) {
+            for (const auto& [name, flag] : mapFlagNames) {
                 // Removing individual flags
-                unsigned int flags = TrimFlags(~(verify_flags | (1U << i)));
+                unsigned int flags = TrimFlags(~(verify_flags | flag));
                 if (!CheckTxScripts(tx, mapprevOutScriptPubKeys, mapprevOutValues, flags, txdata, strTest, /* expect_valid */ true)) {
-                    BOOST_ERROR("Tx unexpectedly failed with flag " << ToString(i) << " unset: " << strTest);
+                    BOOST_ERROR("Tx unexpectedly failed with flag " << name << " unset: " << strTest);
                 }
                 // Removing random combinations of flags
                 flags = TrimFlags(~(verify_flags | (unsigned int)InsecureRandBits(mapFlagNames.size())));
@@ -247,7 +266,7 @@ BOOST_AUTO_TEST_CASE(tx_valid)
             }
 
             // Check that flags are maximal: transaction should fail if any unset flags are set.
-            for (auto flags_excluding_one: ExcludeIndividualFlags(verify_flags)) {
+            for (auto flags_excluding_one : ExcludeIndividualFlags(verify_flags)) {
                 if (!CheckTxScripts(tx, mapprevOutScriptPubKeys, mapprevOutValues, ~flags_excluding_one, txdata, strTest, /* expect_valid */ false)) {
                     BOOST_ERROR("Too many flags unset: " << strTest);
                 }
@@ -314,27 +333,31 @@ BOOST_AUTO_TEST_CASE(tx_invalid)
             PrecomputedTransactionData txdata(tx);
             unsigned int verify_flags = ParseScriptFlags(test[2].get_str());
 
-            // Not using FillFlags() in the main test, in order to detect invalid verifyFlags combination
-            if (!CheckTxScripts(tx, mapprevOutScriptPubKeys, mapprevOutValues, verify_flags, txdata, strTest, /* expect_valid */ false)) {
-                BOOST_ERROR("Tx unexpectedly passed: " << strTest);
+            // Check that the test gives a valid combination of flags (otherwise VerifyScript will throw). Don't edit the flags.
+            if (verify_flags != FillFlags(verify_flags)) {
+                BOOST_ERROR("Bad test flags: " << strTest);
             }
 
+            // Not using FillFlags() in the main test, in order to detect invalid verifyFlags combination
+            BOOST_CHECK_MESSAGE(CheckTxScripts(tx, mapprevOutScriptPubKeys, mapprevOutValues, verify_flags, txdata, strTest, /* expect_valid */ false),
+                                "Tx unexpectedly passed: " << strTest);
+
             // Backwards compatibility of script verification flags: Adding any flag(s) should not validate an invalid transaction
-            for (size_t i = 0; i < mapFlagNames.size(); i++) {
-                unsigned int flags = FillFlags(verify_flags | (1U << i));
+            for (const auto& [name, flag] : mapFlagNames) {
+                unsigned int flags = FillFlags(verify_flags | flag);
                 // Adding individual flags
                 if (!CheckTxScripts(tx, mapprevOutScriptPubKeys, mapprevOutValues, flags, txdata, strTest, /* expect_valid */ false)) {
-                    BOOST_ERROR("Tx unexpectedly passed with flag " << ToString(i) << " set: " << strTest);
+                    BOOST_ERROR("Tx unexpectedly passed with flag " << name << " set: " << strTest);
                 }
                 // Adding random combinations of flags
                 flags = FillFlags(verify_flags | (unsigned int)InsecureRandBits(mapFlagNames.size()));
                 if (!CheckTxScripts(tx, mapprevOutScriptPubKeys, mapprevOutValues, flags, txdata, strTest, /* expect_valid */ false)) {
-                    BOOST_ERROR("Tx unexpectedly passed with random flags " << ToString(flags) << ": " << strTest);
+                    BOOST_ERROR("Tx unexpectedly passed with random flags " << name << ": " << strTest);
                 }
             }
 
             // Check that flags are minimal: transaction should succeed if any set flags are unset.
-            for (auto flags_excluding_one: ExcludeIndividualFlags(verify_flags)) {
+            for (auto flags_excluding_one : ExcludeIndividualFlags(verify_flags)) {
                 if (!CheckTxScripts(tx, mapprevOutScriptPubKeys, mapprevOutValues, flags_excluding_one, txdata, strTest, /* expect_valid */ true)) {
                     BOOST_ERROR("Too many flags set: " << strTest);
                 }