#36184 test: fix TaprootSignatureMsg default codeseparator_pos
https://github.com/bitcoin/bitcoin/pull/36184 · · +1/-1 in 1 files, 1 commits · labels: Tests
Goal
- Stop a taproot test helper from raising an exception when callers omit optional arguments
- Prevents errors for developers writing new functional tests that rely on helper defaults
Updates the default `codeseparator_pos` parameter in `TaprootSignatureMsg` in `test/functional/test_framework/script.py` from `-1` to `0xFFFFFFFF`. Because the function encodes `codeseparator_pos` as an unsigned 4-byte integer, the existing `-1` default raises an `OverflowError` whenever `scriptpath=True` is used without explicitly providing a position.
Problem: The `TaprootSignatureMsg` helper fails with an exception if a script-path caller omits `codeseparator_pos`. No in-tree functional tests currently hit this failure because existing test cases pass the argument explicitly, but it hinders developers writing new tests that rely on default arguments.
Category: Test infrastructure (#43 of 45)
P4 · bug fix
- P4 because it fixes a broken default argument in a test helper that has no in-tree callers
- Only affects future test authors writing new script-path test cases
P4 because it fixes a broken default argument in a test helper where no existing in-tree callers are affected. As the author notes, 'The only in-tree scriptpath caller passes codeseparator_pos explicitly, so CI does not hit it', making the impact limited to potential future test authors.
Membership: Changes a helper function in test/functional/test_framework/script.py used for generating Taproot signature messages.
Factors: security/stability 0, bug 1, performance 0, user value 0, leverage 0
Reviewability: Ready
- Ready to review
The change is a 1-line test framework fix with green CI and no blocking dependencies.
Author status: active
Open concerns:
- maflcko questioned the motivation for changing the default to 0xFFFFFFFF, suggesting that throwing an exception with -1 or None may be preferable to enforce explicit test parameters, and suggested either using None or closing the PR
Resolved concerns:
- fametrano explained that 0xFFFFFFFF is the BIP 342 value for no codeseparator executed rather than a sentinel, avoiding an extra branch
Agreement: Mild
- Questioned whether enforcing explicit parameters with an exception or None is preferable (maflcko)
- Suggests using None or closing the PR (maflcko)
- Author has not yet responded to the latest objection
maflcko questions the value of the PR and recommends closing it or using None; author has not replied to latest comment
maflcko noted that requiring explicit parameters via an exception might be preferable to defaulting to 0xFFFFFFFF, recommending that the PR be closed or changed to use None. The author has not yet responded to that latest comment.
- maflcko asked whether codeseparator_pos could default to None like leaf_script
- fametrano argued 0xFFFFFFFF is the BIP342 value for 'no codeseparator executed' and avoids an unnecessary branch
- maflcko responded that test code should be explicit, noted that -1 or None achieve this by raising an exception, and recommended closing the PR or using None
Review verdicts (DrahtBot): 0
Files
2 lines under test/bench/ci.
- test/functional/test_framework/script.py +1/-1
Card
This PR updates the default codeseparator_pos in TaprootSignatureMsg from -1 to 0xFFFFFFFF in the functional test framework. The -1 default triggers an OverflowError when serializing unsigned 4-byte integers if a caller enables scriptpath without setting the parameter. However, all in-tree tests currently specify the parameter explicitly, so no CI tests currently fail. maflcko questioned the change, suggesting that raising an exception might be desirable to enforce explicit test code and recommending either closing the PR or defaulting to None.