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

theStack commented at 4:10 PM on August 18, 2021: member

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

DrahtBot added the label Tests on Aug 18, 2021

DrahtBot commented at 8:33 AM on August 19, 2021: member

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

Conflicts

Reviewers, this pull request conflicts with the following ones:

#20892 (tests: Run both descriptor and legacy tests within a single test invocation by achow101)

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.

in test/functional/feature_nulldummy.py:30 in f9fa8e1dee outdated

  23 | @@ -24,7 +24,10 @@
  24 |  from test_framework.messages import CTransaction
  25 |  from test_framework.script import CScript
  26 |  from test_framework.test_framework import BitcoinTestFramework
  27 | -from test_framework.util import assert_equal, assert_raises_rpc_error
  28 | +from test_framework.util import (
  29 | +    assert_equal,
  30 | +    assert_raises_rpc_error,
  31 | +)

josibake commented at 3:10 PM on August 23, 2021:

nit: style only change, not a big deal since this is a very small pr

in test/functional/feature_nulldummy.py:108 in f5da29536c outdated

 105 |          test4tx = create_transaction(self.nodes[0], test2tx.hash, self.address, amount=46)
 106 |          test6txs = [CTransaction(test4tx)]
 107 |          trueDummy(test4tx)
 108 |          assert_raises_rpc_error(-26, NULLDUMMY_ERROR, self.nodes[0].sendrawtransaction, test4tx.serialize_with_witness().hex(), 0)
 109 | -        self.block_submit(self.nodes[0], [test4tx])
 110 | +        self.block_submit(self.nodes[0], [test4tx], accept=False)

josibake commented at 3:23 PM on August 23, 2021:

can you rename this commit to refactor: ...? i stared at this line for longer than id like to admit, trying to understand why you were adding a new argument and how it was changing the test behavior :sweat_smile:

theStack commented at 4:25 PM on August 23, 2021:

Good point, I forgot this in the first commit. Sorry for creating confusion! Done.

josibake commented at 3:27 PM on August 23, 2021: member

Concept ACK

all for improving test precision by using specific error messages. i was unable, however, to reproduce the issue. i re-compiled with the change suggested in the description (return 1) and ran the test with and without -par=1. it passed both times, but maybe im not doing something right?

id also suggest renaming the first commit as a refactor and updating the description to note you are fixing the test and doing a small refactor. without that context, its a little confusing as to why the arguments are changing

josibake commented at 3:35 PM on August 23, 2021: member

i was unable, however, to reproduce the issue

sorry, i misunderstood the instructions. i was able to reproduce by doing the following: re-compile with the suggested change AND use the old error message block-validation-failed. i then re-compiled normally, changed the error message to the new message and ran without -par=1 to confirm i also got a failure.

using the more precise error message + par=1 definitely makes sense :+1:

test: refactor: use named args for block_submit in feature_nulldummy.py 646b3885f7

test: fix failure in feature_nulldummy.py on single-core machines

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

The behaviour can be reproduced on a multi-core machine by simply changing the
function GetNumCores() (in src/util/system.cpp) to return 1:

int GetNumCores()
{
    return 1;
}

7720d4f650

theStack force-pushed on Aug 23, 2021

theStack commented at 4:30 PM on August 23, 2021: member

@josibake: Thanks a lot for your review! I agree that the description was a bit sloppy/confusing w.r.t. how to reproduce the issue and not mentioning the refactoring commit. I updated the PR description and force-pushed with the following changes:

rebased on master
renamed first commit subject to include "refactor:" (as suggested #22738#pullrequestreview-736244239)

josibake commented at 4:49 PM on August 23, 2021: member

ACK https://github.com/bitcoin/bitcoin/pull/22738/commits/7720d4f650015272dc7109238230520f71858c6c

compiled with the PR suggestion and ensured i can reproduce the issue. re-compiled normally and verified test passes. great work on using a more specific error and improving the readability!

Saviour1001 commented at 12:28 PM on August 24, 2021: none

Tested ACK <code>7720d4f</code>

MarcoFalke merged this on Aug 26, 2021

MarcoFalke closed this on Aug 26, 2021

sidhujag referenced this in commit 73c2853f14 on Aug 28, 2021

DrahtBot locked this on Aug 26, 2022