test: ensure OP_1NEGATE satisfies BIP62
#19436
Conversation
jonatack
force-pushed
the
test-OP_1NEGATE-and-use-OP_RIGHT
branch
2 times, most recently
from
f1b4654 to
0b26e1e
Compare
|
Minimised the changeset. |
|
Concept ACK Thanks for filling this testing gap! |
|
I don't think it makes sense to replace 0x81 with OP_RIGHT, 0x81 is not an op code, it has nothing to do with OP_RIGHT, it just represents the number -1 I'm just looking into how this was fixed in master, I don't immediately see where the change is that fixed it 🤔 |
|
I vaguely recall it was fixed shortly after #13084 in another commit; I should have specified it. |
Oh right, in BIP62 it says "Pushing the byte 0x81 must use OP_1NEGATE." I'll remove that commit. |
with a regression test for PR 13084 and PR 17204.
jonatack
changed the title
OP_RIGHT, ensure OP_1NEGATE satisfies BIP62OP_1NEGATE satisfies BIP62
jonatack
force-pushed
the
test-OP_1NEGATE-and-use-OP_RIGHT
branch
from
0b26e1e to
3913278
Compare
|
Somehow I think this is avoiding the issue, the test is good but PushAll will still push 0x81 as a byte instead of using the OP_1NEGATE opcode if #17204 doesn't go in as well. I think it would still be good to include it for future safety, even if this practical instance of the bug is fixed. |
|
Ok, removed "Closes #17204" from this PR description. |
meshcollider
left a comment
meshcollider
left a comment
There was a problem hiding this comment.
Code review ACK 3913278
Can you explain which prs you are referring to? |
The PRs in the first sentence -- updated to make it more clear. |
|
How can the test pass on master but fail before those pull requests? That sounds like a merge conflict |
|
Just built at 476cb35 and it fails as well. |
As I said above, the behavior in master avoids the previous issue, those PRs fixed the issue in a different way which would still be good to include. The symptom is gone but the original cause is still present in master so it isn't a conflict. |
|
It just seemed a bit weird to call this a regression test for a patch that is not yet merged. It should mention that the regression test can only be tested on the patch when rebased on commits prior to ed94c8b Otherwise, if the patch gets rebased on current master, the test will pass both before and after. Something that seems odd for a regression test. |
I think the PR description is fine. The test is to protect against regression to the state before the mentioned PRs. I thought it might be helpful, but I'd rather not spend time bikeshedding. |
|
Sorry, I didn't want to imply to close this. The test is fine and useful. I think the description can be improved to mention the interaction with commit ed94c8b, but that has nothing to do with the test code itself. |
|
@MarcoFalke thanks, @meshcollider ACK |
5 participants
Seen while reviewing/testing #13084 and #17204 that both needed a regression test.
This PR adds a regression test based on #13084 (comment) to ensure
OP_1NEGATEsatisfies the BIP62 minimal push standardness rule. This test fails at the commit immediately before each of these PRs (#13084 and #17204) and passes with their fixes. It also passes on current master.