tests: reset fIsBareMultisigStd after bare-multisig tests #18018

pull fanquake wants to merge 1 commits into bitcoin:master from fanquake:fix_p2sh_tests_failure changing 1 files +1 −0

fanquake commented at 5:47 AM on January 29, 2020: member
Fixes: #18015

The bug this fixes is two-part.
1. The fIsBareMultisigStd global is being reused by other tests, such as script_p2sh_tests(set), after being set to false.
2. The order our tests run in doesn't always? seem to be random, which meant that the script_p2sh tests would only fail if they were run in an order where the transaction_tests ran first, mutating the fIsBareMultisigStd global.
This doesn't seem to happen when running make check, but if you run src/test/test_bitcoin and pass --random=99999, the failure in script_p2sh will occur (on most, but maybe not all systems):
```
src/test/test_bitcoin --random=99999
Running 389 test cases...
test/script_p2sh_tests.cpp:200: error: in "script_p2sh_tests/set": txTo[1].IsStandard
test/script_p2sh_tests.cpp:200: error: in "script_p2sh_tests/set": txTo[2].IsStandard
test/script_p2sh_tests.cpp:200: error: in "script_p2sh_tests/set": txTo[3].IsStandard

*** 3 failures are detected in the test module "Bitcoin Core Test Suite"
```
The new test for bare multisig was introduced in #17502.
fanquake added the label Tests on Jan 29, 2020
fanquake requested review from instagibbs on Jan 29, 2020
fanquake commented at 5:47 AM on January 29, 2020: member

cc @theStack
laanwj commented at 10:59 AM on January 29, 2020: member

Good catch! Concept ACK.

These kind of order-dependent stale state errors are kind of nasty, and hard to keep track of. Would it make sense to do this reset as part of the test fixture?
fanquake commented at 11:03 AM on January 29, 2020: member

These kind of order-dependent stale state errors are kind of nasty, and hard to keep track of. Would it make sense to do this reset as part of the test fixture?

Yes, I think so. Tests really shouldn't be sharing any sort of global, mutable state, and them passing should not be dependent on the order in which they are run.
MarcoFalke commented at 2:42 PM on January 29, 2020: member

This can be fixed by parsing the command line arguments when the TestingSetup is constructed. Calling AppInitParameterInteraction will reset them to the default.
practicalswift commented at 4:47 PM on January 29, 2020: contributor

Concept ACK, but would be better if solved globally in TestingSetup rather than locally in test_IsStandard.
MarcoFalke commented at 6:47 PM on January 29, 2020: member

This is a nice one-line fix in the same fashion as we used to handle globals in unit tests in the past: If a unit-test changes them, they are reset by the same test to the value they were before.

As a follow-up it could make sense to normalize argument parsing and have the TestingSetup re-parse the arguments to set them to the default. Though, I suspect that that would be a larger change.

in src/test/transaction_tests.cpp:834 in eb9e0aab56 outdated

 830 | @@ -831,6 +831,7 @@ BOOST_AUTO_TEST_CASE(test_IsStandard)
 831 |      reason.clear();
 832 |      BOOST_CHECK(!IsStandardTx(CTransaction(t), reason));
 833 |      BOOST_CHECK_EQUAL(reason, "bare-multisig");
 834 | +    fIsBareMultisigStd = true;

MarcoFalke commented at 6:49 PM on January 29, 2020:

    fIsBareMultisigStd = DEFAULT_PERMIT_BAREMULTISIG;

Empact commented at 2:34 AM on January 30, 2020:

Alternatively, store the value at 824 and reload the stored value at 834.

fanquake commented at 3:13 AM on January 30, 2020:

Agree that using the constant is clearer.

MarcoFalke approved

tests: reset fIsBareMultisigStd after bare-multisig tests

The bug this fixes is two-part.

1.The fIsBareMultisigStd global is being reused by other tests,
i.e script_p2sh_tests(set), after being set to false.

2. The order our tests run in doesn't always? seem to be random,
which meant that the script_p2sh tests would only fail if they
were run in an order where transaction_tests ran first, mutating
the fIsBareMultisigStd global.

This doesn't seem to happen when running make check, but if you
run src/test/test_bitcoin and pass --random=99999, the failure
in script_p2sh:

test/script_p2sh_tests.cpp:200: error: in "script_p2sh_tests/set": txTo[1].IsStandard

will occur (on most systems).

The new test was introduced in 1bb5d517aa616c1d5b5801d2ea36a2de5fb61eba.

1b96a3cd1e

fanquake force-pushed on Jan 30, 2020
fanquake commented at 3:16 AM on January 30, 2020: member

I've updated to use DEFAULT_PERMIT_BAREMULTISIG in place of true, and opened #18024 to follow up with the test setup suggestions.
theStack commented at 4:33 AM on January 30, 2020: member

ACK https://github.com/bitcoin/bitcoin/pull/18018/commits/1b96a3cd1ebe725896f59614903184289fe62c Sorry for introducing this bug.
Empact commented at 5:00 AM on January 30, 2020: member

Code Review ACK https://github.com/bitcoin/bitcoin/pull/18018/commits/1b96a3cd1ebe725896f59614903184289fe62cf8
MarcoFalke referenced this in commit 0130abbdb7 on Jan 30, 2020
MarcoFalke merged this on Jan 30, 2020
MarcoFalke closed this on Jan 30, 2020
fanquake deleted the branch on Feb 13, 2020
jasonbcox referenced this in commit 89268501a6 on Nov 9, 2020
DrahtBot locked this on Feb 15, 2022

Contributors

Review Requested
instagibbs

Labels

Tests

Linked (view graph)

#17502 test: add unit test for non-standard bare multisig txs #18015 Gitian-built unit tests fail on windows #18024 tests: reset default arguments during test setup