Merge bitcoin/bitcoin#22738: test: fix failure in feature_nulldummy.py on single-core machines

7720d4f650015272dc7109238230520f71858c6c test: fix failure in feature_nulldummy.py on single-core machines (Sebastian Falbesoner)
646b3885f795c640a2ac979362c509c4a8ee592a test: refactor: use named args for block_submit in feature_nulldummy.py (Sebastian Falbesoner)

Pull request description:

  On single-core machines, executing the test `feature_nulldummy.py` results in the following assertion error:

  ```
      ...
      2021-08-18T15:37:58.805000Z TestFramework (INFO): Test 4: Non-NULLDUMMY base multisig transaction is invalid after activation
      2021-08-18T15:37:58.814000Z TestFramework (ERROR): Assertion failed
      Traceback (most recent call last):
        File "[...]/test/functional/test_framework/test_framework.py", line 131, in main
          self.run_test()
        File "[...]/test/functional/feature_nulldummy.py", line 107, in run_test
          self.block_submit(self.nodes[0], [test4tx], accept=False)
        File "[...]/test/functional/feature_nulldummy.py", line 134, in block_submit
          assert_equal(None if accept else 'block-validation-failed', node.submitblock(block.serialize().hex()))
        File "[...]/test/functional/test_framework/util.py", line 49, in assert_equal
          raise AssertionError("not(%s)" % " == ".join(str(arg) for arg in (thing1, thing2) + args))
      AssertionError: not(block-validation-failed == non-mandatory-script-verify-flag (Dummy CHECKMULTISIG argument must be zero))
      2021-08-18T15:37:58.866000Z TestFramework (INFO): Stopping nodes
      ...
  ```

  There are hardly any single-core machines around anymore, but the behaviour can be reproduced on a multi-core machine by patching the function `GetNumCores()` to return 1 on the master branch and running `feature_nulldummy.py`:

  ```diff
  diff --git a/src/util/system.cpp b/src/util/system.cpp
  index 30d410381..149b512fc 100644
  --- a/src/util/system.cpp
  +++ b/src/util/system.cpp
  @@ -1338,7 +1338,7 @@ bool SetupNetworking()

   int GetNumCores()
   {
  -    return std::thread::hardware_concurrency();
  +    return 1;
   }
  ```
  As solution, parallel script verification is disabled (`-par=1`) and the exact reject reason is checked, which also increases the precision of the test (the possibility that the block is rejected because of another unintended reason is ruled out). See also related PR #22711 which applies the same approach for the p2p segwit test. The PR also includes a refactoring commit which changes the calls to `self.block_submit()` to use named arguments and removes the default value for parameter `accept` (i.e. explicitely passing `accept=...` is mandatory), with the aim to increase the test readability.

ACKs for top commit:
  josibake:
    ACK https://github.com/bitcoin/bitcoin/pull/22738/commits/7720d4f650015272dc7109238230520f71858c6c
  Saviour1001:
    Tested ACK <code>[7720d4f](https://github.com/bitcoin/bitcoin/commit/7720d4f650015272dc7109238230520f71858c6c)</code>

Tree-SHA512: 8a31ebab3e2ab38e555d7a23139b3324a134a0dedc5b879a2419348ae858323882dbbfcbbf88b68e4f8d7eea8cfe43ee19da1d0d2a36c93ae7878c4980cac31d

1 files changed, 15 insertions, 10 deletions

diff --git a/test/functional/feature_nulldummy.py b/test/functional/feature_nulldummy.py
index f467626801..678e103e5f 100755
--- a/test/functional/feature_nulldummy.py
+++ b/test/functional/feature_nulldummy.py
@@ -24,7 +24,10 @@ from test_framework.blocktools import (
 from test_framework.messages import CTransaction
 from test_framework.script import CScript
 from test_framework.test_framework import BitcoinTestFramework
-from test_framework.util import assert_equal, assert_raises_rpc_error
+from test_framework.util import (
+    assert_equal,
+    assert_raises_rpc_error,
+)
 
 NULLDUMMY_ERROR = "non-mandatory-script-verify-flag (Dummy CHECKMULTISIG argument must be zero)"
 
@@ -51,6 +54,7 @@ class NULLDUMMYTest(BitcoinTestFramework):
         self.extra_args = [[
             f'-segwitheight={COINBASE_MATURITY + 5}',
             '-addresstype=legacy',
+            '-par=1',  # Use only one script thread to get the exact reject reason for testing
         ]]
 
     def skip_test_if_missing_module(self):
@@ -86,7 +90,7 @@ class NULLDUMMYTest(BitcoinTestFramework):
         txid2 = self.nodes[0].sendrawtransaction(test1txs[1].serialize_with_witness().hex(), 0)
         test1txs.append(create_transaction(self.nodes[0], coinbase_txid[1], self.wit_ms_address, amount=49))
         txid3 = self.nodes[0].sendrawtransaction(test1txs[2].serialize_with_witness().hex(), 0)
-        self.block_submit(self.nodes[0], test1txs, False, True)
+        self.block_submit(self.nodes[0], test1txs, accept=True)
 
         self.log.info("Test 2: Non-NULLDUMMY base multisig transaction should not be accepted to mempool before activation")
         test2tx = create_transaction(self.nodes[0], txid2, self.ms_address, amount=47)
@@ -94,28 +98,28 @@ class NULLDUMMYTest(BitcoinTestFramework):
         assert_raises_rpc_error(-26, NULLDUMMY_ERROR, self.nodes[0].sendrawtransaction, test2tx.serialize_with_witness().hex(), 0)
 
         self.log.info(f"Test 3: Non-NULLDUMMY base transactions should be accepted in a block before activation [{COINBASE_MATURITY + 4}]")
-        self.block_submit(self.nodes[0], [test2tx], False, True)
+        self.block_submit(self.nodes[0], [test2tx], accept=True)
 
         self.log.info("Test 4: Non-NULLDUMMY base multisig transaction is invalid after activation")
         test4tx = create_transaction(self.nodes[0], test2tx.hash, self.address, amount=46)
         test6txs = [CTransaction(test4tx)]
         trueDummy(test4tx)
         assert_raises_rpc_error(-26, NULLDUMMY_ERROR, self.nodes[0].sendrawtransaction, test4tx.serialize_with_witness().hex(), 0)
-        self.block_submit(self.nodes[0], [test4tx])
+        self.block_submit(self.nodes[0], [test4tx], accept=False)
 
         self.log.info("Test 5: Non-NULLDUMMY P2WSH multisig transaction invalid after activation")
         test5tx = create_transaction(self.nodes[0], txid3, self.wit_address, amount=48)
         test6txs.append(CTransaction(test5tx))
         test5tx.wit.vtxinwit[0].scriptWitness.stack[0] = b'\x01'
         assert_raises_rpc_error(-26, NULLDUMMY_ERROR, self.nodes[0].sendrawtransaction, test5tx.serialize_with_witness().hex(), 0)
-        self.block_submit(self.nodes[0], [test5tx], True)
+        self.block_submit(self.nodes[0], [test5tx], with_witness=True, accept=False)
 
         self.log.info(f"Test 6: NULLDUMMY compliant base/witness transactions should be accepted to mempool and in block after activation [{COINBASE_MATURITY + 5}]")
         for i in test6txs:
             self.nodes[0].sendrawtransaction(i.serialize_with_witness().hex(), 0)
-        self.block_submit(self.nodes[0], test6txs, True, True)
+        self.block_submit(self.nodes[0], test6txs, with_witness=True, accept=True)
 
-    def block_submit(self, node, txs, witness=False, accept=False):
+    def block_submit(self, node, txs, *, with_witness=False, accept):
         tmpl = node.getblocktemplate(NORMAL_GBT_REQUEST_PARAMS)
         assert_equal(tmpl['previousblockhash'], self.lastblockhash)
         assert_equal(tmpl['height'], self.lastblockheight + 1)
@@ -124,11 +128,12 @@ class NULLDUMMYTest(BitcoinTestFramework):
             tx.rehash()
             block.vtx.append(tx)
         block.hashMerkleRoot = block.calc_merkle_root()
-        witness and add_witness_commitment(block)
+        if with_witness:
+            add_witness_commitment(block)
         block.rehash()
         block.solve()
-        assert_equal(None if accept else 'block-validation-failed', node.submitblock(block.serialize().hex()))
-        if (accept):
+        assert_equal(None if accept else NULLDUMMY_ERROR, node.submitblock(block.serialize().hex()))
+        if accept:
             assert_equal(node.getbestblockhash(), block.hash)
             self.lastblockhash = block.hash
             self.lastblocktime += 1