test: fix pushdata scripts #32270

pull instagibbs wants to merge 1 commits into bitcoin:master from instagibbs:2025-04-taproot_sample_fix changing 1 files +5 −5

instagibbs commented at 7:11 pm on April 14, 2025: member

The original scripts were done incorrectly, so they are changed to represent two different 2-byte pushes.

Fixes #32114 (review)
DrahtBot commented at 7:11 pm on April 14, 2025: contributor
The following sections might be updated with supplementary metadata relevant to reviewers and maintainers.

Code Coverage & Benchmarks

For details see: https://corecheck.dev/bitcoin/bitcoin/pulls/32270.

Reviews

See the guideline for information on the review process.

Type Reviewers

Concept ACK rkrux

If your review is incorrectly listed, please react with 👎 to this comment and the bot will ignore it on the next update.

Conflicts

Reviewers, this pull request conflicts with the following ones:
- #32415 (scripted-diff: adapt script error constant names in feature_taproot.py by theStack)
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.
DrahtBot added the label Tests on Apr 14, 2025
fanquake requested review from ajtowns on Apr 17, 2025
in test/functional/feature_taproot.py:1244 in c679958eaf outdated
1239@@ -1240,8 +1240,8 @@ def sample_spenders(): 1240 # Create a list of scripts which will be built into a taptree 1241 scripts = [ 1242 # leaf label, followed by CScript 1243- ("encodeable_pushdata1", CScript([OP_DROP, OP_PUSHDATA1, b'aa' * 75])), 1244- ("nonstd_encodeable_pushdata1", CScript([OP_PUSHDATA1, b'aa'])), 1245+ ("2byte_pushdata", CScript([OP_DROP, b'\xaa\xaa'])), 1246+ ("nonstd_2byte_pushdata", CScript.fromhex("4c02aaaa")),
rkrux commented at 7:08 pm on April 22, 2025:

Shouldn’t this one be called nonstd_2byte_pushdata1 because it uses the PUSHDATA1 opcode? That’s what is mentioned in the comments of the previous PR and in the spender comment down below.

rkrux commented at 7:09 pm on April 22, 2025:

I’m guessing it’s intentional to have two different ways of creating CScript here? For better illustration.

instagibbs commented at 8:20 pm on April 22, 2025:

The python framework won’t let you construct a non-standard serialization natively, AFAIK. So we’re doing it manually.

instagibbs commented at 8:21 pm on April 22, 2025:

I was focusing on two different ways of pushing 2 bytes, not naming the specific opcode. If it would help legibility I can re-introduce the exact pushdata opcode name in use.

rkrux commented at 10:53 am on April 23, 2025:

This is fine but seeing pushdata1 in tutorial/nonminpushdata1 later in the spender comment seems unnecessary then.

instagibbs commented at 1:36 pm on April 28, 2025:

changed to push everywhere

Type	Reviewers
Concept ACK	rkrux

rkrux commented at 7:20 pm on April 22, 2025: contributor

Concept ACK

My bad I didn’t look at the scripts thoroughly in the previous PR and focussed more on the design of the examples.

I debugged the scripts in the test and can indeed see the unintended script as described in this comment: #32114 (review)

0('encodeable_pushdata1', CScript([OP_DROP, x('96616161616161616161616161616161616161616161616161616161616161616161616161616161616161616161616161616161616161616161616161616161616161616161616161616161'), OP_NOP, OP_NOP, OP_NOP, OP_NOP, OP_NOP, OP_NOP, OP_NOP, OP_NOP, OP_NOP, OP_NOP, OP_NOP, OP_NOP, OP_NOP, OP_NOP, OP_NOP, OP_NOP, OP_NOP, OP_NOP, OP_NOP, OP_NOP, OP_NOP, OP_NOP, OP_NOP, OP_NOP, OP_NOP, OP_NOP, OP_NOP, OP_NOP, OP_NOP, OP_NOP, OP_NOP, OP_NOP, OP_NOP, OP_NOP, OP_NOP, OP_NOP, OP_NOP, OP_NOP, OP_NOP, OP_NOP, OP_NOP, OP_NOP, OP_NOP, OP_NOP, OP_NOP, OP_NOP, OP_NOP, OP_NOP, OP_NOP, OP_NOP, OP_NOP, OP_NOP, OP_NOP, OP_NOP, OP_NOP, OP_NOP, OP_NOP, OP_NOP, OP_NOP, OP_NOP, OP_NOP, OP_NOP, OP_NOP, OP_NOP, OP_NOP, OP_NOP, OP_NOP, OP_NOP, OP_NOP, OP_NOP, OP_NOP, OP_NOP, OP_NOP, OP_NOP, OP_NOP]))

in test/functional/feature_taproot.py:1264 in c679958eaf outdated
1262- add_spender(spenders, comment="tutorial/pushdata1redux", tap=tap, leaf="encodeable_pushdata1", inputs=[b'\x00'], failure={"leaf": "dummyleaf"}, **ERR_NO_SUCCESS) 1263+ add_spender(spenders, comment="tutorial/pushdataredux", tap=tap, leaf="2byte_pushdata", inputs=[b'\x00'], failure={"leaf": "dummyleaf"}, **ERR_NO_SUCCESS) 1264 1265 # Spender that is non-standard but otherwise valid, with extraneous signature data from inner key for optional failure condition 1266- add_spender(spenders, comment="tutorial/nonminpushdata1", tap=tap, leaf="nonstd_encodeable_pushdata1", key=secs[0], standard=False, failure={"inputs": [getter("sign")]}, **ERR_CLEANSTACK) 1267+ add_spender(spenders, comment="tutorial/nonminpushdata1", tap=tap, leaf="nonstd_2byte_pushdata", key=secs[0], standard=False, failure={"inputs": [getter("sign")]}, **ERR_CLEANSTACK)
rkrux commented at 11:35 am on April 23, 2025:

I don’t have a firm grasp on script execution yet (as I have not dealt with enough script execution) and there are few things going on inside the “test_spenders” as well that I have not gone through in its entirety. These seem like focussed test cases that I am trying to reason through.

Are following the ways these 3 tapscript scenarios executed?

During script execution, the empty string witness data is dropped because of “OP_DROP” and the 2 byte data is left on the stack. I’m assuming this 2 byte data counts as 1 stack item (because the stack is a vector of vector) and that’s why script execution doesn’t lead to an error in script execution? References based on a quick look:

https://github.com/bitcoin/bitcoin/blob/82d1e94838ee52b979146077a53fdaa244e9260a/src/script/script.h#L588

https://github.com/bitcoin/bitcoin/blob/82d1e94838ee52b979146077a53fdaa244e9260a/src/script/interpreter.cpp#L2061-L2062

The “2byte_pushdata” leaf is overridden with the “dummyleaf”, and because there is no “OP_DROP” now, thus after the script execution, there is the empty string witness data left that leads to the “Script evaluated without error but finished with a false/empty top stack element” error. This shows that the emptiness of the witness data is crucial for this spender case but not so much for the first spender case because in it any witness data gets dropped due to the subsequent “OP_DROP” - I tested with “\x11” and that test case passed.

The “nonstd_2byte_pushdata” leaf contains no “OP_DROP”, and thus after script execution, multiple items are left on the stack: a signature from the “secs[0]” key & the 2 byte data that leads to the “Stack size must be exactly one after execution” error.

in test/functional/feature_taproot.py:1243 in c679958eaf outdated

1239@@ -1240,8 +1240,8 @@ def sample_spenders():
1240     # Create a list of scripts which will be built into a taptree
1241     scripts = [
1242         # leaf label, followed by CScript
1243-        ("encodeable_pushdata1", CScript([OP_DROP, OP_PUSHDATA1, b'aa' * 75])),
1244-        ("nonstd_encodeable_pushdata1", CScript([OP_PUSHDATA1, b'aa'])),
1245+        ("2byte_pushdata", CScript([OP_DROP, b'\xaa\xaa'])),

TheCharlatan commented at 9:19 am on April 24, 2025:

Maybe call this just 2byte_push, since there is no pushdata going on here?

instagibbs commented at 1:36 pm on April 28, 2025:

done

test: fix pushdata scripts

The original scripts were done incorrectly,
so they are changed to represent two
different 2-byte pushes.

8425c5ccfb

instagibbs force-pushed on Apr 28, 2025
DrahtBot added the label CI failed on Apr 28, 2025
DrahtBot removed the label CI failed on May 2, 2025
DrahtBot added the label Needs rebase on May 6, 2025
DrahtBot commented at 11:17 pm on May 6, 2025: contributor

🐙 This pull request conflicts with the target branch and needs rebase.

Contributors
instagibbs DrahtBot rkrux TheCharlatan

Review Requested
ajtowns

Labels
Tests Needs rebase

test: fix pushdata scripts #32270

Code Coverage & Benchmarks

Reviews

Conflicts