test: bug fix in transaction

glozow commented at 3:51 PM on February 23, 2021: member

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_1s 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

glozow commented at 3:55 PM on February 23, 2021: member

@MarcoFalke thanks for catching the bug - this should fix it

in src/test/data/tx_valid.json:67 in 8a8fe6af93 outdated

  61 | @@ -62,10 +62,6 @@
  62 |    ["c76168ef1a272a4f176e55e73157ecfce040cfad16a5272f6296eb7089dca846", 1, "DUP HASH160 0x14 0x34fea2c5a75414fd945273ae2d029ce1f28dafcf EQUALVERIFY CHECKSIG"]],
  63 |  "010000000390d31c6107013d754529d8818eff285fe40a3e7635f6930fec5d12eb02107a43010000006b483045022100f40815ae3c81a0dd851cc8d376d6fd226c88416671346a9033468cca2cdcc6c202204f764623903e6c4bed1b734b75d82c40f1725e4471a55ad4f51218f86130ac038321033d710ab45bb54ac99618ad23b3c1da661631aa25f23bfe9d22b41876f1d46e4effffffff3ff04a68e22bdd52e7c8cb848156d2d158bd5515b3c50adabc87d0ca2cd3482d010000006a4730440220598d263c107004008e9e26baa1e770be30fd31ee55ded1898f7c00da05a75977022045536bead322ca246779698b9c3df3003377090f41afeca7fb2ce9e328ec4af2832102b738b531def73020bd637f32935924cc88549c8206976226d968edd3a42fc2d7ffffffff46a8dc8970eb96622f27a516adcf40e0fcec5731e7556e174f2a271aef6861c7010000006b483045022100c5b90a777a9fdc90c208dbef7290d1fc1be651f47151ee4ccff646872a454cf90220640cfbc4550446968fbbe9d12528f3adf7d87b31541569c59e790db8a220482583210391332546e22bbe8fe3af54addfad6f8b83d05fa4f5e047593d4c07ae938795beffffffff028036be26000000001976a914ddfb29efad43a667465ac59ff14dc6442a1adfca88ac3d5cba01000000001976a914b64dde7a505a13ca986c40e86e984a8dc81368b688ac00000000", "NONE"],
  64 |  
  65 | -["An invalid P2SH Transaction"],
  66 | -[[["0000000000000000000000000000000000000000000000000000000000000100", 0, "HASH160 0x14 0x7a052c840ba73af26755de42cf01cc9e0a49fef0 EQUAL"]],
  67 | -"010000000100010000000000000000000000000000000000000000000000000000000000000000000009085768617420697320ffffffff010000000000000000015100000000", "P2SH,CLEANSTACK,WITNESS"],

MarcoFalke commented at 4:11 PM on February 23, 2021:

I think the test is testing that an invalid p2sh tx is valid before p2sh activation?

glozow commented at 4:41 PM on February 23, 2021:

With #19698, transaction_tests will check that excluding the P2SH flag will cause this test to pass, so it would still be a duplicate test.

glozow commented at 4:54 PM on February 23, 2021:

Added this comment to the commit log^

in src/test/transaction_tests.cpp:161 in 834f22e64a outdated

 157 | @@ -158,7 +158,7 @@ std::vector<unsigned int> ExcludeIndividualFlags(unsigned int flags)
 158 |      std::vector<unsigned int> flags_combos;
 159 |      for (unsigned int i = 0; i < mapFlagNames.size(); ++i) {
 160 |          const unsigned int flags_excluding_i = TrimFlags(flags & ~(1U << i));
 161 | -        if (flags != flags_excluding_i && std::find(flags_combos.begin(), flags_combos.end(), flags_excluding_i) != flags_combos.end()) {
 162 | +        if (flags != flags_excluding_i && std::find(flags_combos.begin(), flags_combos.end(), flags_excluding_i) == flags_combos.end()) {

MarcoFalke commented at 4:11 PM on February 23, 2021:

Any reason to re-invent the wheel and avoid std::set?

glozow commented at 4:26 PM on February 23, 2021:

Nope, I could change it to set.

glozow commented at 4:54 PM on February 23, 2021:

I've changed it to use a std::set

glozow force-pushed on Feb 23, 2021

DrahtBot added the label Tests on Feb 23, 2021

glozow commented at 7:33 PM on February 23, 2021: member

Cirrus failure is in feature_blockfilterindex_prune.py, which I'm pretty sure is unrelated, given that this is only touching the unit tests

in src/test/transaction_tests.cpp:167 in b1ea5c5bf7 outdated

 159 | +// this should return the vector [0111, 1011, 1101, 1110].
 160 | +std::set<unsigned int> ExcludeIndividualFlags(unsigned int flags)
 161 |  {
 162 | -    std::vector<unsigned int> flags_combos;
 163 | +    std::set<unsigned int> flags_combos;
 164 |      for (unsigned int i = 0; i < mapFlagNames.size(); ++i) {

MarcoFalke commented at 10:46 AM on March 30, 2021:

how is the map supposed to be kept up to date? Looks like it is missing SCRIPT_VERIFY_DISCOURAGE_UPGRADABLE_PUBKEYTYPE?

glozow commented at 3:19 PM on March 30, 2021:

Hm, is it possible to iterate through all the script verify flags? Could add an assert that they're all present in mapFlagNames

glozow commented at 3:19 PM on March 30, 2021:

Looks like it is missing SCRIPT_VERIFY_DISCOURAGE_UPGRADABLE_PUBKEYTYPE?

Oop, will add

MarcoFalke commented at 5:46 PM on March 30, 2021:

Oh, my comment was just an example. There are more missing than just the one, IIRC

glozow commented at 11:56 PM on March 30, 2021:

should all be there now

in src/test/transaction_tests.cpp:168 in 0f6d0ffc8a outdated

 151 | @@ -152,14 +152,16 @@ unsigned int FillFlags(unsigned int flags)
 152 |      return flags;

MarcoFalke commented at 10:48 AM on March 30, 2021:

unrelated nit: Could Assert(IsValidFlagCombination(flags));? (Needs #21553)

glozow commented at 3:08 PM on March 30, 2021:

~There are places where we need to fill the flags as well as check that they're a valid combination~

glozow commented at 1:33 PM on March 31, 2021:

I completely misread this for some reason 🤦 sorry about that Just rebased on top of #21553 and added the asserts

glozow force-pushed on Mar 30, 2021

[test] remove invalid test from tx_valid.json

This exact test is also already in tx_invalid.json#L29
It will also test with no-P2SH flags, so duplicating with
different flags is not necessary.

8cac2923f5

glozow force-pushed on Mar 31, 2021

in src/test/transaction_tests.cpp:161 in a944a21be4 outdated

 158 |      return flags;
 159 |  }
 160 |  
 161 | -// Return valid flags that are all except one flag for each flag
 162 | -std::vector<unsigned int> ExcludeIndividualFlags(unsigned int flags)
 163 | +// Exclude each possible script verify flag from flags. Returns a list of these flag combinations

jnewbery commented at 2:12 PM on April 1, 2021:

This returns a set, not a list.

You could copy the set into a vector to avoid changing the return type if you really felt like it.

diff --git a/src/test/transaction_tests.cpp b/src/test/transaction_tests.cpp
index 3d6ad20b85..235862ec7a 100644
--- a/src/test/transaction_tests.cpp
+++ b/src/test/transaction_tests.cpp
@@ -161,7 +161,7 @@ unsigned int FillFlags(unsigned int flags)
 // Exclude each possible script verify flag from flags. Returns a list of these flag combinations
 // that are valid and without duplicates. For example: if flags=1111 and there are 4 possible flags,
 // this should return the vector [0111, 1011, 1101, 1110].
-std::set<unsigned int> ExcludeIndividualFlags(unsigned int flags)
+std::vector<unsigned int> ExcludeIndividualFlags(unsigned int flags)
 {
     std::set<unsigned int> flags_combos;
     for (unsigned int i = 0; i < mapFlagNames.size(); ++i) {
@@ -170,7 +170,7 @@ std::set<unsigned int> ExcludeIndividualFlags(unsigned int flags)
             flags_combos.insert(flags_excluding_i);
         }
     }
-    return flags_combos;
+    return {flags_combos.begin(), flags_combos.end()};
 }
 
 BOOST_FIXTURE_TEST_SUITE(transaction_tests, BasicTestingSetup)

glozow commented at 3:46 PM on April 1, 2021:

True, although hopefully it's obvious - will update if I need to retouch. Thanks!

glozow commented at 4:46 PM on April 5, 2021:

Fixed

jnewbery commented at 2:33 PM on April 1, 2021: member

utACK a944a21be41d1e5469822b1858c304c983010880

in src/test/transaction_tests.cpp:168 in a944a21be4 outdated

 166 | +std::set<unsigned int> ExcludeIndividualFlags(unsigned int flags)
 167 |  {
 168 | -    std::vector<unsigned int> flags_combos;
 169 | +    std::set<unsigned int> flags_combos;
 170 |      for (unsigned int i = 0; i < mapFlagNames.size(); ++i) {
 171 |          const unsigned int flags_excluding_i = TrimFlags(flags & ~(1U << i));

MarcoFalke commented at 5:20 PM on April 1, 2021:

I still don't think this is right. A std::map doesn't order its elements based on the order they were inserted.

Even if it did, the values aren't guaranteed to never miss a power of two.

glozow commented at 4:54 PM on April 2, 2021:

Sorry, what do you mean? The map mapFlagNames is just to identify the flags from strings in the json files. I guess if there wasn't a script verify flag for 1 << i for some i, it would get ignored by the interpreter if we added it. So as long as we have the leftmost flag we should be okay?

MarcoFalke commented at 4:59 PM on April 2, 2021:

Let's assume mapFlagNames is only a single entry {"EXAMPLE", 1U << 4} (because for some reason other flags are no longer needed and have been removed).

The loop will only cover 1U << 1, a non-existent flag.

glozow commented at 10:00 PM on April 2, 2021:

Ah true. Is there a way of checking whether an unsigned int corresponds to a script verify flag?

MarcoFalke commented at 6:09 AM on April 3, 2021:

Just iterate over the map and use the value of the key-value pair?

in src/test/transaction_tests.cpp:347 in a944a21be4 outdated

 333 | +            BOOST_CHECK_MESSAGE(CheckTxScripts(tx, mapprevOutScriptPubKeys, mapprevOutValues, verify_flags, txdata, strTest, /* expect_valid */ false),
 334 | +                                "Tx unexpectedly passed: " << strTest);
 335 | +
 336 |              // Backwards compatibility of script verification flags: Adding any flag(s) should not validate an invalid transaction
 337 |              for (size_t i = 0; i < mapFlagNames.size(); i++) {
 338 |                  unsigned int flags = FillFlags(verify_flags | (1U << i));

MarcoFalke commented at 5:20 PM on April 1, 2021:

same (and everywhere else)

glozow force-pushed on Apr 5, 2021

glozow commented at 5:00 PM on April 5, 2021: member

Updates: Iterate through the values in mapFlagNames instead of all bits from 1 to size(). Check that all flags in STANDARD_SCRIPT_VERIFY_FLAGS are present in mapFlagNames. This would cover all relevant flags used for consensus and policy checks.

in src/test/transaction_tests.cpp:89 in 24f2951e11 outdated

  81 | @@ -78,6 +82,16 @@ unsigned int ParseScriptFlags(std::string strFlags)
  82 |      return flags;
  83 |  }
  84 |  
  85 | +// Check that all flags in STANDARD_SCRIPT_VERIFY_FLAGS are present in mapFlagNames.
  86 | +bool CheckMapFlagNames()
  87 | +{
  88 | +    unsigned int standard_flags_missing{STANDARD_SCRIPT_VERIFY_FLAGS};
  89 | +    for (const auto & [_, flag] : mapFlagNames) {

jnewbery commented at 8:53 AM on April 7, 2021:

    for (const auto& [_, flag] : mapFlagNames) {

in src/test/transaction_tests.cpp:178 in 24f2951e11 outdated

 180 | -    for (unsigned int i = 0; i < mapFlagNames.size(); ++i) {
 181 | -        const unsigned int flags_excluding_i = TrimFlags(flags & ~(1U << i));
 182 | -        if (flags != flags_excluding_i && std::find(flags_combos.begin(), flags_combos.end(), flags_excluding_i) != flags_combos.end()) {
 183 | -            flags_combos.push_back(flags_excluding_i);
 184 | +    std::set<unsigned int> flags_combos;
 185 | +    for (const auto & [_, flag] : mapFlagNames) {

jnewbery commented at 8:54 AM on April 7, 2021:

    for (const auto& [_, flag] : mapFlagNames) {

in src/test/transaction_tests.cpp:259 in 24f2951e11 outdated

 260 |                  // Removing individual flags
 261 | -                unsigned int flags = TrimFlags(~(verify_flags | (1U << i)));
 262 | +                unsigned int flags = TrimFlags(~(verify_flags | flag));
 263 |                  if (!CheckTxScripts(tx, mapprevOutScriptPubKeys, mapprevOutValues, flags, txdata, strTest, /* expect_valid */ true)) {
 264 | -                    BOOST_ERROR("Tx unexpectedly failed with flag " << ToString(i) << " unset: " << strTest);
 265 | +                    BOOST_ERROR("Tx unexpectedly failed with flag " << ToString(flag) << " unset: " << strTest);

jnewbery commented at 10:24 AM on April 7, 2021:

This doesn't need ToString():

                    BOOST_ERROR("Tx unexpectedly failed with flag " << flag << " unset: " << strTest);

or alternatively, print the name of the flag:

                    BOOST_ERROR("Tx unexpectedly failed with flag " << _ << " unset: " << strTest);

(but probably change _ to flag_name or similar)

glozow commented at 2:03 AM on April 8, 2021:

printing name of flag! :D

in src/test/transaction_tests.cpp:346 in 24f2951e11 outdated

 346 | +                                "Tx unexpectedly passed: " << strTest);
 347 | +
 348 |              // Backwards compatibility of script verification flags: Adding any flag(s) should not validate an invalid transaction
 349 | -            for (size_t i = 0; i < mapFlagNames.size(); i++) {
 350 | -                unsigned int flags = FillFlags(verify_flags | (1U << i));
 351 | +            for (const auto & [_, flag] : mapFlagNames) {

jnewbery commented at 10:25 AM on April 7, 2021:

            for (const auto& [_, flag] : mapFlagNames) {

in src/test/transaction_tests.cpp:350 in 24f2951e11 outdated

 351 | +            for (const auto & [_, flag] : mapFlagNames) {
 352 | +                unsigned int flags = FillFlags(verify_flags | flag);
 353 |                  // Adding individual flags
 354 |                  if (!CheckTxScripts(tx, mapprevOutScriptPubKeys, mapprevOutValues, flags, txdata, strTest, /* expect_valid */ false)) {
 355 | -                    BOOST_ERROR("Tx unexpectedly passed with flag " << ToString(i) << " set: " << strTest);
 356 | +                    BOOST_ERROR("Tx unexpectedly passed with flag " << ToString(flag) << " set: " << strTest);

jnewbery commented at 10:25 AM on April 7, 2021:

Again, ToString() is not necessary.

jnewbery commented at 10:28 AM on April 7, 2021: member

ACK 24f2951e11d0aa88e3b45271ec457abaadc6fde0

A few small style suggestions inline if you retouch this.

[test] fix bug in ExcludeIndividualFlags

PR #19168 introduced this function but it always returns an empty vector.

8a365df558

[test] minor improvements / followups

Add missing script verify flags to mapFlagNames.
iterate through mapFlagNames values instead of bits.

BOOST_CHECK_MESSAGE better reports which test failed exactly, whereas
BOOST_ERROR was just incrementing the error counter.

5aee73d175

[test] remove unnecessary OP_1s from invalid tests

Similar to 19db590d044efe7d474a16720e5b56e7b55db54c, which removed these
for the valid tests. Not removing ones that cause a false/empty stack
error because these tests should fail due to being invalid with CSV/CLTV

5d3ced72f9

[test] check that mapFlagNames is up to date

There is no way to iterate through all script verification flags, and
it's not guaranteed that every power of 2 is used. Just make sure that
all flags in STANDARD_SCRIPT_VERIFY_FLAGS are present in mapFlagNames;
this covers all consensus and policy flags. If mapFlagNames has more
flags than STANDARD_SCRIPT_VERIFY_FLAGS, that's okay. Nonexistent flags
will be caught by the compiler.

b109bde46a

glozow force-pushed on Apr 8, 2021

glozow commented at 2:04 AM on April 8, 2021: member

Addressed the compiler warning about unused variable and @jnewbery's comments - thanks!

jnewbery commented at 8:05 AM on April 8, 2021: member

utACK b109bde46a8730afbc09c107b802a2c321293f4b

MarcoFalke renamed this:
~~test / bug fix in transaction_tests~~
test: bug fix in transaction_tests
on Apr 19, 2021

in src/test/data/tx_invalid.json:135 in 5d3ced72f9 outdated

 134 |  ["By-time locks, with argument just beyond tx nLockTime (but within numerical boundaries)"],
 135 | -[[["0000000000000000000000000000000000000000000000000000000000000100", 0, "500000001 CHECKLOCKTIMEVERIFY 1"]],
 136 | +[[["0000000000000000000000000000000000000000000000000000000000000100", 0, "500000001 CHECKLOCKTIMEVERIFY"]],
 137 |  "01000000010001000000000000000000000000000000000000000000000000000000000000000000000000000000010000000000000000000065cd1d", "CHECKLOCKTIMEVERIFY"],
 138 | -[[["0000000000000000000000000000000000000000000000000000000000000100", 0, "4294967295 CHECKLOCKTIMEVERIFY 1"]],
 139 | +[[["0000000000000000000000000000000000000000000000000000000000000100", 0, "4294967295 CHECKLOCKTIMEVERIFY"]],

MarcoFalke commented at 9:27 AM on April 19, 2021:

It might be nice to check the exact script reject reason instead of just failure

MarcoFalke merged this on Apr 19, 2021

MarcoFalke closed this on Apr 19, 2021

sidhujag referenced this in commit cc55c0c312 on Apr 19, 2021

JeremyRubin referenced this in commit b94b7acd06 on Apr 23, 2021

JeremyRubin referenced this in commit 2cbbbc6156 on Apr 23, 2021

JeremyRubin referenced this in commit b6e9cc0bae on Apr 23, 2021

glozow deleted the branch on Jan 16, 2022

gwillen referenced this in commit 3853f8f7d4 on Jun 1, 2022

DrahtBot locked this on Jan 16, 2023

test: bug fix in transaction_tests #21280