test: fix failure in feature_nulldummy.py on single-core machines #22738
Conversation
|
The following sections might be updated with supplementary metadata relevant to reviewers and maintainers. ConflictsReviewers, this pull request conflicts with the following ones:
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. |
josibake
left a comment
There was a problem hiding this comment.
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
test/functional/feature_nulldummy.py
Outdated
There was a problem hiding this comment.
nit: style only change, not a big deal since this is a very small pr
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 using the more precise error message + par=1 definitely makes sense 👍 |
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;
}
f9fa8e1 to
7720d4f
Compare
|
@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:
|
|
ACK 7720d4f 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! |
|
Tested ACK |
On single-core machines, executing the test
feature_nulldummy.pyresults in the following assertion error: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 runningfeature_nulldummy.py: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 toself.block_submit()to use named arguments and removes the default value for parameteraccept(i.e. explicitely passingaccept=...is mandatory), with the aim to increase the test readability.