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

A rebase of #13084 which additionally modifies the test code (unaddressed in the original, assuming sipa is too busy to deal with this at the moment).

Relatively simple bugfix so it'd be good to have merged soon.

Turning OP_1NEGATE into 0x0181 results in a larger-than-necessary data push instead of just actually using the OP_1NEGATE opcode (0x4f). This fails the minimal push rule of BIP 62 and makes the result non-standard.

code review ACK 9ddc07f68208fcf0217df11d72a7a61fd369c297

How about a test?

How about a test?

The test is updated. If you want a new, separate test for this, I think that doesn't need to be in this PR.

It’s good practice to include a regression test with any bugfix, both to confirm the new behavior and to ensure the bug isn’t reintroduced.

The change in src/test/transaction_tests.cpp is supposed to do this…

Update OP to make it explicit what the consequences are of turning OP_1NEGATE in scriptSig into 0x0181 in signing code? :)

The change in src/test/transaction_tests.cpp is supposed to do this…

You can confirm that it doesn't exercise the change by compiling without the sign.cpp changes, running the tests and seeing they don't fail:

$ git co 201910_1negate_rebase
# revert sign.cpp changes
$ git diff
diff --git a/src/script/sign.cpp b/src/script/sign.cpp
index 7ff06e526..0ed92e8d5 100644
--- a/src/script/sign.cpp
+++ b/src/script/sign.cpp
@@ -181,8 +181,6 @@ static CScript PushAll(const std::vector<valtype>& values)
             result << OP_0;
         } else if (v.size() == 1 && v[0] >= 1 && v[0] <= 16) {
             result << CScript::EncodeOP_N(v[0]);
-        } else if (v.size() == 1 && v[0] == 0x81) {
-            result << OP_1NEGATE;
         } else {
             result << v;
         }
$ make
$ src/test/test_bitcoin
Running 373 test cases...

*** No errors detected

You can confirm that it doesn't exercise the change by compiling without the sign.cpp changes, running the tests and seeing they don't fail:

Ok, wasn't aware of that, thanks.

If this is rebased on top of e4382fbef56a0e04b0ed834e8b3a3a16f81db149, it is a clean merge to 0.13, 0.14, 0.15, 0.16, 0.17, 0.18, 0.19, and master ;)

eg https://github.com/luke-jr/bitcoin/tree/bugfix_keepnegone-0.13

Rebased as requested, I'll try and add a test for this but this is such a small and obviously correct change that it should just be merged as-is IMO

You rebased in the wrong direction...

git rebase HEAD^ --onto e4382fbef56

(or just git reset --hard c74d19fcae5)

Fixed Luke, sorry 👍

Wrote a regression test while reviewing this PR and it looks like the issue has been resolved. Proposed the test in #19436.

EDIT: perhaps not resolved, see #19436 (comment).

ACK 45af54fbdc83699fab0a41284a3e8ae6195b0a07

ACK dca28634d77

ACK dca28634d779c775678cba402a85fe5bb9b3a5a9

Code review ACK dca28634d779c775678cba402a85fe5bb9b3a5a9

I spent some time trying to find out what the test was actually testing but gave up for now. It seems the raw transaction parser used to change the 0x4F in the redeem script to 0x0181 in the script and that was fixed some time ago. It's really hard to figure out because errors from script evaluation seem to have changed as well. I might go back to this but the change is good.