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

pull sipa wants to merge 1 commits into bitcoin:master from sipa:201804_keepnegone changing 1 files +2 −0

sipa commented at 11:50 PM on April 25, 2018: member

Reported by arubi on IRC.
Do not turn OP_1NEGATE in scriptSig into 0x0181 in signing code 5af7625079
laanwj commented at 6:12 AM on April 26, 2018: member

Needs a unit test, I think.
laanwj added the label Wallet on Apr 26, 2018
MarcoFalke commented at 12:03 PM on April 26, 2018: member

Yeah, a test makes sense for this function, since it is not particularly well covered: https://marcofalke.github.io/btc_cov/total.coverage/src/script/sign.cpp.gcov.html

fivepiece commented at 9:32 PM on April 26, 2018: contributor

ACK on the fix! Many thanks for looking into it :)

Before :

$ bitcoin-cli -regtest signrawtransactionwithwallet \
0200000001FFFFFFFFFFFFFFFFFFFFFFFFFFFFFFFFFFFFFFFFFFFFFFFFFFFFFFFFFFFFFFFF00000000044F024F9CFDFFFFFF01F0B9F505000000002321027777777777777777777777777777777777777777777777777777777777777777AC66030000 \
'[{"txid":"FFFFFFFFFFFFFFFFFFFFFFFFFFFFFFFFFFFFFFFFFFFFFFFFFFFFFFFFFFFFFFFF","vout":0,"scriptPubKey":"A914AE44AB6E9AA0B71F1CD2B453B69340E9BFBAEF6087","redeemScript":"4F9C","amount":1}]'

{                                                                                             VVVV
  "hex": "0200000001ffffffffffffffffffffffffffffffffffffffffffffffffffffffffffffffff00000000050181024f9cfdffffff01f0b9f505000000002321027777777777777777777777777777777777777777777777777777777777777777ac66030000",
  "complete": false,
  "errors": [
    {
      "txid": "ffffffffffffffffffffffffffffffffffffffffffffffffffffffffffffffff",
      "vout": 0,
      "witness": [
      ],
      "scriptSig": "0181024f9c",
      "sequence": 4294967293,
      "error": "Data push larger than necessary"
    }
  ]
}

After:

$ bitcoin-cli -regtest signrawtransactionwithwallet \
0200000001FFFFFFFFFFFFFFFFFFFFFFFFFFFFFFFFFFFFFFFFFFFFFFFFFFFFFFFFFFFFFFFF00000000044F024F9CFDFFFFFF01F0B9F505000000002321027777777777777777777777777777777777777777777777777777777777777777AC66030000 \
'[{"txid":"FFFFFFFFFFFFFFFFFFFFFFFFFFFFFFFFFFFFFFFFFFFFFFFFFFFFFFFFFFFFFFFF","vout":0,"scriptPubKey":"A914AE44AB6E9AA0B71F1CD2B453B69340E9BFBAEF6087","redeemScript":"4F9C","amount":1}]' \

{                                                                                             VV
  "hex": "0200000001ffffffffffffffffffffffffffffffffffffffffffffffffffffffffffffffff00000000044f024f9cfdffffff01f0b9f505000000002321027777777777777777777777777777777777777777777777777777777777777777ac66030000",
  "complete": true
}

laanwj commented at 8:23 AM on April 27, 2018: member

Thanks for testing @fivepiece
melG81 approved
laanwj commented at 12:56 PM on May 7, 2018: member

It looks like there is a copy of this function in src/test/transaction_tests.cpp: static CScript PushAll(const std::vector<valtype>& values) used in ReplaceRedeemScript. Does this need update as well?
fanquake commented at 7:48 AM on July 18, 2018: member

@sipa Would this be good to get into 0.17.0?
DrahtBot closed this on Jul 22, 2018
DrahtBot reopened this on Jul 22, 2018
meshcollider commented at 8:59 AM on November 9, 2018: contributor

utACK https://github.com/bitcoin/bitcoin/pull/13084/commits/5af76250792b3bc7cedfdc8cd036ba4dcdb844c6 but as @laanwj says, looks like transaction_tests.cpp also needs updating
laanwj commented at 3:24 PM on January 9, 2019: member

looks like transaction_tests.cpp also needs updating

Can you please at least comment on this @sipa? If the answer is no that's fine and this can be merged.
DrahtBot closed this on Apr 28, 2019
DrahtBot commented at 7:11 PM on April 28, 2019: member

The last travis run for this pull request was 279 days ago and is thus outdated. To trigger a fresh travis build, this pull request should be closed and re-opened.
DrahtBot reopened this on Apr 28, 2019
meshcollider commented at 10:59 AM on October 21, 2019: contributor

Re-opened in #17204
meshcollider closed this on Oct 21, 2019
laanwj referenced this in commit 4d4bd5ed74 on Aug 14, 2020
sidhujag referenced this in commit f593e66ad8 on Aug 16, 2020
DrahtBot locked this on Feb 15, 2022

Contributors

Labels

Linked (view graph)

#17204 wallet: Do not turn OP_1NEGATE in scriptSig into 0x0181 in signing code (sipa)#19436 test: ensure `OP_1NEGATE` satisfies BIP62