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

meshcollider · 2019-10-21T10:58:32Z

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.

laanwj · 2019-10-21T11:49:36Z

code review ACK 9ddc07f68208fcf0217df11d72a7a61fd369c297

Empact · 2019-10-21T13:33:29Z

How about a test?

laanwj · 2019-10-22T11:12:46Z

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.

Empact · 2019-10-22T11:55:18Z

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.

laanwj · 2019-10-23T09:54:45Z

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

practicalswift · 2019-10-23T14:02:53Z

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

Empact · 2019-10-23T15:33:59Z

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

laanwj · 2019-10-25T10:06:54Z

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.

luke-jr · 2019-11-09T20:43:21Z

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

luke-jr · 2019-11-09T20:45:44Z

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

meshcollider · 2019-11-20T08:47:57Z

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

luke-jr · 2019-12-24T23:25:45Z

You rebased in the wrong direction...

git rebase HEAD^ --onto e4382fbef56

(or just git reset --hard c74d19fcae5)

meshcollider · 2020-04-20T02:58:56Z

Fixed Luke, sorry 👍

jonatack · 2020-07-03T10:27:13Z

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).

jonatack

ACK 45af54f

Co-authored-by: Samuel Dobson <[email protected]>

with a regression test for PR 13084 and PR 17204.

luke-jr

ACK dca2863

jonatack

ACK dca2863

fjahr · 2020-07-21T11:00:13Z

Code review ACK dca2863

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.

e7448d6 wallet: Don't override signing errors (Fabian Jahr) Pull request description: While reviewing #17204 I noticed that the errors in `input_errors` from `::SignTransaction` where being overridden by `CWallet::SignTransaction`. For example, a Script related error led to incomplete signature data which led to `CWallet::SignTransaction` reporting that keys were missing, which was a less precise error than the original one. Additionally, the error `"Input not found or already spent"` is [duplicated in `sign.cpp`](https://github.com/bitcoin/bitcoin/blob/c7b4968552c704f1e2e9a046911e1207f5af5fe0/src/script/sign.cpp#L481), so the error here is redundant at the moment. So technically the whole error block could be removed, I think. However, this code is affected by the ongoing work on the wallet so there might be a reason why these errors are here. But even if there is a reason to keep them, I don't think existing, potentially more precise errors should be overridden here unless we want to hide them from the users. I am looking for feedback if this is a work in progress state where these errors could be more useful in the future or if they can be removed. On testing: even though [the errors in `CWallet` are covered](https://marcofalke.github.io/btc_cov/total.coverage/src/wallet/wallet.cpp.gcov.html), all tests still pass after removing them. I am not sure if there is a desire to cover these specific error messages, tests in `test/functional/rpc_signrawtransaction.py` seem to aim for a more generic approach. ACKs for top commit: achow101: ACK e7448d6 meshcollider: Code review ACK e7448d6 Tree-SHA512: 3e2bc11d05379d2aef87b093a383d1b044787efc70e35955b2f8ecd028b6acef02f386180566af6a1a63193635f5d685466e2f6141c96326c49ffc5c81ca3e23

e7448d6 wallet: Don't override signing errors (Fabian Jahr) Pull request description: While reviewing bitcoin#17204 I noticed that the errors in `input_errors` from `::SignTransaction` where being overridden by `CWallet::SignTransaction`. For example, a Script related error led to incomplete signature data which led to `CWallet::SignTransaction` reporting that keys were missing, which was a less precise error than the original one. Additionally, the error `"Input not found or already spent"` is [duplicated in `sign.cpp`](https://github.com/bitcoin/bitcoin/blob/c7b4968552c704f1e2e9a046911e1207f5af5fe0/src/script/sign.cpp#L481), so the error here is redundant at the moment. So technically the whole error block could be removed, I think. However, this code is affected by the ongoing work on the wallet so there might be a reason why these errors are here. But even if there is a reason to keep them, I don't think existing, potentially more precise errors should be overridden here unless we want to hide them from the users. I am looking for feedback if this is a work in progress state where these errors could be more useful in the future or if they can be removed. On testing: even though [the errors in `CWallet` are covered](https://marcofalke.github.io/btc_cov/total.coverage/src/wallet/wallet.cpp.gcov.html), all tests still pass after removing them. I am not sure if there is a desire to cover these specific error messages, tests in `test/functional/rpc_signrawtransaction.py` seem to aim for a more generic approach. ACKs for top commit: achow101: ACK e7448d6 meshcollider: Code review ACK e7448d6 Tree-SHA512: 3e2bc11d05379d2aef87b093a383d1b044787efc70e35955b2f8ecd028b6acef02f386180566af6a1a63193635f5d685466e2f6141c96326c49ffc5c81ca3e23

… 0x0181 in signing code (sipa) dca2863 test: ensure OP_1NEGATE satisfies BIP62 minimal push rule (Jon Atack) e629d07 Do not turn OP_1NEGATE in scriptSig into 0x0181 in signing code (Pieter Wuille) Pull request description: A rebase of bitcoin#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. ACKs for top commit: fjahr: Code review ACK dca2863 luke-jr: ACK dca2863 jonatack: ACK dca2863 Tree-SHA512: 706d9a2ef20c809dea923e477a873e2fd60db8d0ae64289e510b766a38005c1f31ab0b5883f16b9c7863ff0d3f705e8e413f6121320028ac196b79c3184a4113

with a regression test for PR 13084 and PR 17204. Github-Pull: bitcoin#17204 Rebased-From: dca2863

meshcollider mentioned this pull request Oct 21, 2019

Do not turn OP_1NEGATE in scriptSig into 0x0181 in signing code #13084

Closed

meshcollider added the Wallet label Oct 21, 2019

maflcko changed the title ~~Do not turn OP_1NEGATE in scriptSig into 0x0181 in signing code (sipa)~~ wallet: Do not turn OP_1NEGATE in scriptSig into 0x0181 in signing code (sipa) Oct 22, 2019

laanwj added the Bug label Nov 1, 2019

laanwj added the Waiting for author label Nov 10, 2019

meshcollider removed the Waiting for author label Apr 20, 2020

jonatack mentioned this pull request Jul 3, 2020

test: ensure OP_1NEGATE satisfies BIP62 #19436

Closed

jonatack reviewed Jul 12, 2020

View reviewed changes

sipa and others added 2 commits July 15, 2020 15:29

Do not turn OP_1NEGATE in scriptSig into 0x0181 in signing code

e629d07

Co-authored-by: Samuel Dobson <[email protected]>

test: ensure OP_1NEGATE satisfies BIP62 minimal push rule

dca2863

with a regression test for PR 13084 and PR 17204.

luke-jr approved these changes Jul 16, 2020

View reviewed changes

jonatack approved these changes Jul 16, 2020

View reviewed changes

fjahr mentioned this pull request Jul 22, 2020

Wallet should not override signing errors #19568

Merged

laanwj merged commit 4d4bd5e into bitcoin:master Aug 14, 2020

meshcollider deleted the 201910_1negate_rebase branch August 14, 2020 18:59

luke-jr mentioned this pull request Aug 16, 2020

signrawtransactionwithwallet fails to sign non-wallet inputs (0.20 only; before/after already fixed) #19737

Closed

luke-jr pushed a commit to bitcoinknots/bitcoin that referenced this pull request Aug 17, 2020

test: ensure OP_1NEGATE satisfies BIP62 minimal push rule

faeca43

with a regression test for PR 13084 and PR 17204. Github-Pull: bitcoin#17204 Rebased-From: dca2863

bitcoin locked as resolved and limited conversation to collaborators Feb 15, 2022

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

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

Uh oh!

Conversation

meshcollider commented Oct 21, 2019 • edited Loading Uh oh! There was an error while loading. Please reload this page.

Uh oh!

Uh oh!

laanwj commented Oct 21, 2019 • edited Loading Uh oh! There was an error while loading. Please reload this page.

Uh oh!

Uh oh!

Empact commented Oct 21, 2019

Uh oh!

laanwj commented Oct 22, 2019

Uh oh!

Empact commented Oct 22, 2019 • edited Loading Uh oh! There was an error while loading. Please reload this page.

Uh oh!

Uh oh!

laanwj commented Oct 23, 2019

Uh oh!

practicalswift commented Oct 23, 2019

Uh oh!

Empact commented Oct 23, 2019 • edited Loading Uh oh! There was an error while loading. Please reload this page.

Uh oh!

Uh oh!

laanwj commented Oct 25, 2019

Uh oh!

luke-jr commented Nov 9, 2019

Uh oh!

luke-jr commented Nov 9, 2019

Uh oh!

meshcollider commented Nov 20, 2019 • edited Loading Uh oh! There was an error while loading. Please reload this page.

Uh oh!

Uh oh!

luke-jr commented Dec 24, 2019

Uh oh!

meshcollider commented Apr 20, 2020

Uh oh!

jonatack commented Jul 3, 2020 • edited Loading Uh oh! There was an error while loading. Please reload this page.

Uh oh!

Uh oh!

jonatack left a comment

Choose a reason for hiding this comment

Uh oh!

luke-jr left a comment

Choose a reason for hiding this comment

Uh oh!

jonatack left a comment

Choose a reason for hiding this comment

Uh oh!

fjahr commented Jul 21, 2020

Uh oh!

Reviewers

Assignees

Labels

Projects

Milestone

Development

Uh oh!

8 participants

meshcollider commented Oct 21, 2019 •

edited

Loading

laanwj commented Oct 21, 2019 •

edited

Loading

Empact commented Oct 22, 2019 •

edited

Loading

Empact commented Oct 23, 2019 •

edited

Loading

meshcollider commented Nov 20, 2019 •

edited

Loading

jonatack commented Jul 3, 2020 •

edited

Loading