test: fix accurate multisig sigop count (BIP16), add unit test #29615

pull theStack wants to merge 1 commits into bitcoin:master from theStack:202403-test-fix_GetSigOpCount_accurate_counting_bip16 changing 1 files +16 −2

theStack commented at 7:31 PM on March 10, 2024: contributor

In the course of reviewing #29589 I noticed the following buggy call-site of CScriptOp.decode_op_n in the CScript's GetSigOpCount method: https://github.com/bitcoin/bitcoin/blob/4cc99df44aec4d104590aee46cf18318e22a8568/test/functional/test_framework/script.py#L591-L593 This should be lastOpcode rather than opcode. The latter is either OP_CHECKMULTISIG or OP_CHECKMULTISIGVERIFY at this point, so decode_op_n would result in an error. Also, in CScript.raw_iter, we have to return the op as CScriptOp type instead of a bare integer, otherwise we can't call the decode method on it. To prevent this in the future, add some simple unit tests for GetSigOpCount.

Note that this was unnoticed, as the code part was never hit so far in the test framework.
test: fix accurate multisig sigop count (BIP16), add unit test 3e9c736a26
Christewart approved
Christewart commented at 3:15 PM on March 11, 2024: contributor

ACK 3e9c736a26724ffe3b70b387995fbf48c06300e2

Please review #29589 and (if you are feeling very generous) #29371 :-)
DrahtBot commented at 3:16 PM on March 11, 2024: contributor



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



Code Coverage

For detailed information about the code coverage, see the test coverage report.



Reviews

See the guideline for information on the review process.

Type Reviewers

ACK Christewart, rkrux, hernanmarino, achow101

If your review is incorrectly listed, please react with 👎 to this comment and the bot will ignore it on the next update.
maflcko added the label DrahtBot Guix build requested on Mar 11, 2024
maflcko removed the label DrahtBot Guix build requested on Mar 11, 2024
DrahtBot added the label Tests on Mar 11, 2024
in test/functional/test_framework/script.py:592 in 3e9c736a26
```
 589 | @@ -590,7 +590,7 @@ def GetSigOpCount(self, fAccurate):
 590 |                  n += 1
 591 |              elif opcode in (OP_CHECKMULTISIG, OP_CHECKMULTISIGVERIFY):
 592 |                  if fAccurate and (OP_1 <= lastOpcode <= OP_16):
```
rkrux commented at 9:54 AM on April 8, 2024:

Would you know why the function accepts a fAccurate boolean? The only occurrence of fAccurate = false is in feature_taproot.py, rest of them pass true by default.

rkrux commented at 10:03 AM on April 8, 2024:

Note that this was unnoticed, as the code part was never hit so far in the test framework.

The other occurrences where true is passed end up being ultimately called by feature_block.py test via get_legacy_sigopcount_block and get_legacy_sigopcount_tx functions. Am I missing something here?
rkrux approved
rkrux commented at 10:03 AM on April 8, 2024: contributor
tACK 3e9c736
1. Make successful
2. Functional tests successful
Asked couple questions for my clarity.
hernanmarino approved
hernanmarino commented at 10:08 PM on April 12, 2024: contributor

tACK 3e9c736a26724ffe3b70b387995fbf48c06300e2
achow101 commented at 5:43 PM on April 25, 2024: member

ACK 3e9c736a26724ffe3b70b387995fbf48c06300e2
achow101 merged this on Apr 25, 2024
achow101 closed this on Apr 25, 2024
theStack deleted the branch on Apr 25, 2024
bitcoin locked this on Apr 25, 2025