test: ensure `OP_1NEGATE` satisfies BIP62 #19436

pull jonatack wants to merge 1 commits into bitcoin:master from jonatack:test-OP_1NEGATE-and-use-OP_RIGHT changing 1 files +20 −0

jonatack commented at 10:24 AM on July 3, 2020: member

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_1NEGATE satisfies 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.

in test/functional/rpc_signrawtransaction.py:208 in 4122a7b069 outdated

 203 | @@ -198,10 +204,32 @@ def verify_txn_with_witness_script(self, tx_type):
 204 |          assert_equal(spending_tx_signed['complete'], True)
 205 |          self.nodes[0].sendrawtransaction(spending_tx_signed['hex'])
 206 |  
 207 | +    def OP_1NEGATE_test(self):
 208 | +        self.log.info("Test OP_1NEGATE satisfies BIP62 minimal push standardness rule.")

jonatack commented at 10:30 AM on July 3, 2020:

self-nit: remove full stop at end of this log message if retouch

jonatack force-pushed on Jul 3, 2020
jonatack force-pushed on Jul 3, 2020
jonatack commented at 11:43 AM on July 3, 2020: member

Minimised the changeset.
practicalswift commented at 12:12 PM on July 3, 2020: contributor

Concept ACK

Thanks for filling this testing gap!
DrahtBot added the label Consensus on Jul 3, 2020
DrahtBot added the label Tests on Jul 3, 2020
meshcollider commented at 9:05 AM on July 11, 2020: contributor

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 🤔
jonatack commented at 9:10 AM on July 11, 2020: member

I vaguely recall it was fixed shortly after #13084 in another commit; I should have specified it.
jonatack commented at 9:17 AM on July 11, 2020: member

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

Oh right, in BIP62 it says "Pushing the byte 0x81 must use OP_1NEGATE." I'll remove that commit.
test: ensure OP_1NEGATE satisfies BIP62 minimal push rule
with a regression test for PR 13084 and PR 17204.
3913278c08
jonatack renamed this:
~~consensus, test: use `OP_RIGHT`, ensure `OP_1NEGATE` satisfies BIP62~~
test: ensure `OP_1NEGATE` satisfies BIP62
on Jul 11, 2020
jonatack force-pushed on Jul 11, 2020
meshcollider commented at 9:37 AM on July 11, 2020: contributor

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.
jonatack commented at 11:29 AM on July 11, 2020: member

Ok, removed "Closes #17204" from this PR description.
meshcollider commented at 11:35 AM on July 11, 2020: contributor

Code review ACK 3913278c0827f40478fa325feb6f89ee171fdd3b
MarcoFalke removed the label Consensus on Jul 11, 2020
MarcoFalke commented at 1:08 PM on July 11, 2020: member

The test fails at the commit immediately before each of these PRs

Can you explain which prs you are referring to?
jonatack commented at 1:41 PM on July 11, 2020: member

The test fails at the commit immediately before each of these PRs

Can you explain which prs you are referring to?

The PRs in the first sentence -- updated to make it more clear.
MarcoFalke commented at 2:44 PM on July 11, 2020: member

How can the test pass on master but fail before those pull requests? That sounds like a merge conflict
jonatack commented at 3:18 PM on July 11, 2020: member
Maybe it was fixed since those PRs in other work that touched the code.
- The test fails at a0079d4, and passes at 5af7625 and at 45af54f. I just re-ran these.
- The test passed on master when I opened the PR and passes on master at 160800ac now.
jonatack commented at 3:24 PM on July 11, 2020: member

Just built at 476cb35 and it fails as well.
meshcollider commented at 1:18 AM on July 12, 2020: contributor

How can the test pass on master but fail before those pull requests? That sounds like a merge conflict

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.
MarcoFalke commented at 7:08 AM on July 12, 2020: member

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 ed94c8b556dbbfb62452eaefd9ee7841df09777a

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.
jonatack commented at 7:40 AM on July 12, 2020: member

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.
jonatack closed this on Jul 12, 2020
MarcoFalke commented at 8:09 AM on July 12, 2020: member

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.
meshcollider commented at 11:21 PM on July 12, 2020: contributor

I'll cherry-pick this into #17204 if that's ok with you @jonatack
jonatack commented at 4:56 AM on July 13, 2020: member

@MarcoFalke thanks, @meshcollider ACK
DrahtBot locked this on Feb 15, 2022

Contributors

Labels

Linked (view graph)

#13084 Do not turn OP_1NEGATE in scriptSig into 0x0181 in signing code #17204 wallet: Do not turn OP_1NEGATE in scriptSig into 0x0181 in signing code (sipa)