test: cover invalid codesep positions for signature in taproot #32301
Conversation
|
The following sections might be updated with supplementary metadata relevant to reviewers and maintainers. Code Coverage & BenchmarksFor details see: https://corecheck.dev/bitcoin/bitcoin/pulls/32301. ReviewsSee 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. ConflictsNo conflicts as of last run. |
|
ACK 1757409 |
9922eee to
89646cb
Compare
|
rebased due to merge conflict |
| @@ -849,7 +849,7 @@ def TaprootSignatureMsg(txTo, spent_utxos, hash_type, input_index=0, *, scriptpa | |||
| if scriptpath: | |||
| ss += TaggedHash("TapLeaf", bytes([leaf_ver]) + ser_string(leaf_script)) | |||
| ss += bytes([0]) | |||
| ss += codeseparator_pos.to_bytes(4, "little", signed=True) | |||
| ss += codeseparator_pos.to_bytes(4, "little", signed=False) | |||
There was a problem hiding this comment.
Why this change? Having a code separator at position 2**31 or more doesn't seem very plausible given the block weight limit is lower than 2**22, and writing 0xffff_ffff as -1 and 0xffff_fffe as -2 seems fine?
There was a problem hiding this comment.
Having a code separator at position 231 or more doesn't seem very plausible given the block weight limit is lower than 222
This might be an argument for more coverage, but not against adding coverage. Obviously the case will never make it into the "success" column without a a hardfork.
writing 0xffff_ffff as -1 and 0xffff_fffe as -2 seems fine?
I think writing test code to be obvious is better, all else equal.
89646cb to
3657988
Compare
|
rebased |
|
CI failure: |
3657988 to
81e5c83
Compare
There is some basic coverage, but I felt like adding some boundary conditions where the only issue is the codesep value would be nice.