test: check for specific block reject reasons in p2p_segwit.py #22711

In the test p2p_segwit.py, there are many instances where we send a segwit block to a node with the expectation that it is rejected. For this purpose, the helper function test_witness_block exists which allows also to check for a specific reject reason that is asserted in the debug log: https://github.com/bitcoin/bitcoin/blob/502d22ceed1f90ed41336260f8eb428d3acaf514/test/functional/p2p_segwit.py#L119-L120

This PR aims to increase the precision of the tests by adding the expected reject reasons to all test_witness_block call instances (found via greping after test_witness_block(.*accepted=False). For some blocks that are rejected due to failed script verification, the exact reason is only shown in the debug log if parallel script verification is disabled. For this reason, the addition of the reasons is split up in two commits:

• first, all instances are tackled that are not subject to script verification, i.e. it doesn't matter whether parallel script verification is enabled or not (e.g. bad-blk-weight, bad-witness-merkle-match...)
• then, we explicitely set -par=1 to only use one script thread, and add the remaining reasons (non-mandatory-script-verify-flag with the more specific reason in parantheses)

ACK https://github.com/bitcoin/bitcoin/pull/22711/commits/d742b1c7d05cfc56994833fd90aab0d8852ffe4a

this is a really important change: without it, the test could be failing for an entirely different reason and you would assume it was failing for the reason you expected. i tested this by adding the following lines to test_v0_outputs_arent_spendable:

from copy import deepcopy
fake_block = deepcopy(block)
# use a hardcoded hash from a previous test run
fake_block.hashPrevBlock = 45514493863593995281992143828725175249324913083213145988561565159796949646003
self.update_witness_block_with_transactions(block, [tx])
# Verify that segwit isn't activated. A block serialized with witness
# should be rejected prior to activation.
test_witness_block(self.nodes[0], self.test_node, fake_block, accepted=False, with_witness=True)

without a reason specified, this test passed! :scream:

after adding reason='unexpected-witness', the test failed with the following error:

AssertionError: [node 0] Expected messages "['unexpected-witness']" does not partially match log:

 - 2021-08-17T11:29:45.259050Z [msghand] [net_processing.cpp:2478] [ProcessMessage] received: block (144 bytes) peer=1
 - 2021-08-17T11:29:45.259090Z [msghand] [net_processing.cpp:3719] [ProcessMessage] received block 5be173022096f0ddfa40c27d8b1c6b8da20d2c2ede788c69f49c1c174559db08 peer=1
 - 2021-08-17T11:29:45.259114Z [msghand] [validation.cpp:3298] [AcceptBlockHeader] ERROR: AcceptBlockHeader: prev block not found

based on this, we should have assert_debug_log fail if both expected_msgs and unexpected_msgs are empty arrays. ill open a follow up PR for this.

Interesting. Are these error cases already covered by ./src/test/test_bitcoin -t script_tests?

Interesting. Are these error cases already covered by ./src/test/test_bitcoin -t script_tests?

It seems like each one of the following script interpreter errors (see src/script/script_error.cpp):

• ERR_WITNESS_PROGRAM_WITNESS_EMPTY ("Witness program was passed an empty witness")
• ERR_WITNESS_UNEXPECTED ("Witness provided for non-witness script")
• ERR_CLEANSTACK ("Stack size must be exactly one after execution")
• ERR_WITNESS_MALLEATED ("Witness requires empty scriptSig")
• ERR_PUSH_SIZE ("Push value size limit exceeded")
• ERR_SCRIPT_SIZE ("Script is too big")
• ERR_INVALID_STACK_OPERATION ("Operation not valid with the current stack size")
• ERR_EVAL_FALSE ("Script evaluated without error but finished with a false/empty top stack element")

can at least be found once in src/test/data/script_tests.json (without the ERR_ prefix), which is in turn used as input for the script tests in src/test/script_tests.cpp. I'm thinking that maybe a good follow-up PR idea would be to als introduce these script error constants in the functional test framework too, to not have those strings duplicated in the tests; it would be also easier to find which script errors are covered in unit and functional tests with a single grep. The test feature_taproot.py already does this locally:

https://github.com/bitcoin/bitcoin/blob/fdd80b0a53b4af0b29cb6e03118e2456d053a757/test/functional/feature_taproot.py#L554-L575

<!--e57a25ab6845829454e8d69fc972939a-->

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

<!--174a7506f384e20aa4161008e828411d-->

Conflicts

No conflicts as of last run.

reACK: https://github.com/bitcoin/bitcoin/pull/22711/commits/00b93e5e990b04e8f8b3e1d6d0a274de89e83f8b

Rebased on master.

Concept and approach ACK

tested ACK 45827fd.

Checking the reject reasons in test_witness_block is a great addition to the test.