test: avoid sporadic MINIMALDATA failure in feature_taproot.py (fixes #27595) #27631

The functional test feature_taproot.py fails in some rare cases on the execution of the following "branched_codesep" spending script (can be reproduced via $ ./test/functional/feature_taproot.py --randomseed 9048710178866422833 on master / 137a98c5a22e058ed7a7997a0a4dbd75301de51e):

https://github.com/bitcoin/bitcoin/blob/9d85c03620bf660cfa7d13080f5c0b191579cbc3/test/functional/feature_taproot.py#L741

The problem occurs if the first data-push (having random content with a random length in the range [0, 510]) has a length of 1 and the single byte has value of [1...16] or [-1]; in this case, the data-push is not minimally encoded by test framework's CScript class (i.e. doesn't use the special op-codes OP_1...OP_16 or OP_1NEGATE) and the script interpreter throws an SCRIPT_ERR_MINIMALDATA error:

test_framework.authproxy.JSONRPCException: non-mandatory-script-verify-flag (Data push larger than necessary) (-26)

Background: the functional test framework's CScript class translates passed bytes/bytearrays always to data pushes using OP_PUSHx/OP_PUSHDATA{1,2,4} op-codes (see CScript.__coerce_instance(...)). E.g. the expression CScript(bytes([1])) yields bytes([OP_PUSH1, 1]) instead of the minimal-encoded bytes([OP_1]).

Fix this by adapting the random-size range to [2,...], i.e. never pass byte-arrays below length two to be pushed.

Closes #27595.

<!--e57a25ab6845829454e8d69fc972939a-->

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

<!--021abf342d371248e50ceaed478a90ca-->

Reviews

See the guideline for information on the review process.

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

Concept ACK

IIUC the function of that data push is just to "randomize" the sighash, so I think dropping size 1 from the range is reasonable, maybe leave a comment as such if my intuition is correct?

Alternatively you can fix __coerce_instance:

        elif isinstance(other, (bytes, bytearray)):
            if len(other) == 1 and 0 <= other[0] <= 16:
                other = bytes([CScriptOp.encode_op_n(other[0])])
            else:
                other = CScriptOp.encode_op_pushdata(other)

This passes your example, and doesn't seem to break any other test.

cc @MarcoFalke

This passes your example, and doesn't seem to break any other test.

Seems fragile to change a data structure with one-off code for a single test case. If the one-off special case is needed, it can be put into the test case itself.

Added a comment, as suggested by @instagibbs. Let me know what you think, suggestions very welcome. @Sjors: thought about that approach too, but it seems that the devil is in the details; with your proposed fix, passing a single zero-byte would now wrongly translate to a OP_0 operation, which pushes a zero-length array instead. Concept ACK on fixing the test framework's CScript class to always correctly emit minimal-encoded scripts, but that seems to be a bigger operation that ideally also includes proper unit tests etc. (if anyone wants to open a PR for that, I'm happy to review).

ACK https://github.com/bitcoin/bitcoin/pull/27631/commits/54877253c807dac7a3720b2c3d1d989c410259a7

Concept ACK

utACK 54877253c807dac7a3720b2c3d1d989c410259a7

ACK 54877253c807dac7a3720b2c3d1d989c410259a7