Add coverage for the error branch of OP_NUMEQUALVERIFY: https://github.com/bitcoin/bitcoin/blob/d861c3820528fad2e17a45549bec70b6ba434dcc/src/script/interpreter.cpp#L997
Note the code coverage miss: https://maflcko.github.io/b-c-cov/total.coverage/src/script/interpreter.cpp.gcov.html (around line 997)
I ran: cmake -B build -DENABLE_WALLET=OFF && cmake --build build -j 8 && ctest --test-dir build -j 8
<!--e57a25ab6845829454e8d69fc972939a-->
The following sections might be updated with supplementary metadata relevant to reviewers and maintainers.
<!--006a51241073e994b41acfe9ec718e94-->
For details see: https://corecheck.dev/bitcoin/bitcoin/pulls/34145.
<!--021abf342d371248e50ceaed478a90ca-->
See the guideline for information on the review process.
| Type | Reviewers |
|---|---|
| ACK | yashbhutwala, darosior, sedited |
| Stale ACK | rkrux, bensig |
If your review is incorrectly listed, please copy-paste <code><!--meta-tag:bot-skip--></code> into the comment that the bot should ignore.
<!--5faf32d7da4f0f540f40219e4f7537a3-->
lgtm crACK 8dd3db347fc199cc819cf9729688faaddbfc441f
Can mention line 1200 in the PR description that didn't have coverage until this PR instead of adding the other test case that was referred: https://github.com/bitcoin/bitcoin/blob/d861c3820528fad2e17a45549bec70b6ba434dcc/src/script/interpreter.cpp#L1200
ACK 8dd3db3
Built and ran script_tests on macOS - all pass. Both test cases work as expected.
Side note: I have another PR out for script_tests: #34099. Thought I'd mention it since it's within the same context (improving interpreter coverage).
Concept ACK, ACK b7625387569af059adb359af4cff18dd7850f213. For the second commit, i think it would be nice if you could correct the existing test case that intends to cover this check, or possibly remove it in favour of the one you introduce after understanding why it does not currently cover the intended line: https://github.com/bitcoin/bitcoin/blob/62557c95298d88be8920c1b3b952d07d0aedb1af/src/test/data/tx_invalid.json#L83-L86
@darosior thank you, I did not notice that test case in tx_invalid.json. I've removed the second commit (https://github.com/bitcoin/bitcoin/commit/8dd3db347fc199cc819cf9729688faaddbfc441f) from this PR.
To be clear, the test case for OP_CHECKMULTISIG that I wanted to add in https://github.com/bitcoin/bitcoin/commit/8dd3db347fc199cc819cf9729688faaddbfc441f did not improve code coverage.
cc @glozow
ACK b7625387569af059adb359af4cff18dd7850f213
Built and tested on macOS (Apple Silicon). Ran the full test suite with:
cmake -B build -DENABLE_WALLET=OFF -DENABLE_IPC=OFF
cmake --build build -j 8
ctest --test-dir build -j 8
All 132 tests pass. Also ran test_bitcoin --run_test=script_tests separately to confirm the new test case exercises the SCRIPT_ERR_NUMEQUALVERIFY error path at src/script/interpreter.cpp:997.
Nice little coverage improvement - straightforward and correct.
To be clear, the test case for OP_CHECKMULTISIG that I wanted to add in 8dd3db3 did not improve code coverage.
How comes? I think you were correct in OP that the line that check for a missing dummy element is currently not covered.
ACK b7625387569af059adb359af4cff18dd7850f213
It's an improvement so i'm fine if it's merged but i think the other commit is still worth pursuing as per my comment above.
ACK b7625387569af059adb359af4cff18dd7850f213
How comes? I think you were correct in OP that the line that check for a missing dummy element is currently not covered.
It's impossible to cover that line - it's dead code. Have a look at #33977.
The test case that I added in 8dd3db3 is just as good as the one you cited in bitcoin/src/test/data/tx_invalid.json. It wouldn't hurt to merge 8dd3db3, but it wouldn't improve code coverage.