Make signrawtransaction* give an error when amount is needed but missing #13547

pull ajtowns wants to merge 2 commits into bitcoin:master from ajtowns:signrawamount changing 2 files +61 −1

ajtowns commented at 4:16 PM on June 27, 2018: member

Signatures using segregated witness commit to the amount being spent, so that value must be passed into signrawtransactionwithkey and signrawtransactionwithwallet. This ensures an error is issued if that doesn't happen, rather than just assuming the value is 0 and producing a signature that is almost certainly invalid.

Based on Ben Woosley's #12458, Fixes: #12429.

Error on missing amount in signrawtransaction*

Signatures using segregated witness commit to the amount being spent,
so that value must be passed into signrawtransactionwithkey and
signrawtransactionwithwallet. This ensures an error is issued if that
doesn't happen, rather than just assuming the value is 0 and producing
a signature that is almost certainly invalid.

a3b065b51f

[tests] Check signrawtransaction* errors on missing prevtx info 685d1d8115
fanquake added the label RPC/REST/ZMQ on Jun 27, 2018
ajtowns commented at 4:18 PM on June 27, 2018: member

Note that this does the signature, and then checks if it didn't make sense. It might be cleaner to work out if the scriptPubKey implies segwit is used and not do the signature at all, but that seemed a bit more invasive.

It's currently missing tests to check signrawtransactionwithkey also fails correctly.
DrahtBot commented at 9:29 AM on June 28, 2018: member
Note to reviewers: This pull request conflicts with the following ones:
- #12458 (Enforce that amount is provided for signrawtransaction prevtxs by Empact)
If you consider this pull request important, please also help to review the conflicting pull requests. Ideally, start with the one that should be merged first.
laanwj added this to the "Blockers" column in a project

sipa commented at 3:34 AM on July 6, 2018: member

This looks nearly identical to #12458. Can @ajtowns and @Empact figure out which one to move forward?

In any case, utACK 685d1d8115f61b15115d80523dd8273f0a816534. Also tested that it still works after merge with #13425.
Empact commented at 3:50 AM on July 6, 2018: member

Either works. Remaining differences are a few incidental invalid call tests and returning without updating the input on mine. @ajtowns?
in test/functional/rpc_rawtransaction.py:160 in 685d1d8115
```
 155 | +            if type == "legacy":
 156 | +                del prevtx["amount"]
 157 | +                succ = self.nodes[0].signrawtransactionwithwallet(rawtx, [prevtx])
 158 | +                assert succ["complete"]
 159 | +
 160 | +            if type != "legacy":
```
MarcoFalke commented at 7:52 AM on July 7, 2018:

Looks like this condition is false when the above is true, so a plain else seems more appropriate. Also, could move the del prevtx["amount"] out of the scopes of the if-else and drop the line below this one?

MarcoFalke commented at 7:58 AM on July 7, 2018:

Alternatively remove the prevtx variable from the patch and always pass a fresh dict to signrawtransactionwithwallet: prevtxs=dict(txid=txid, amount missing...)
MarcoFalke commented at 7:58 AM on July 7, 2018: member

utACK 685d1d8115f61b15115d80523dd8273f0a816534 tests only (haven't looked at the code changes, feel free to ignore my style nits)
in src/rpc/rawtransaction.cpp:814 in 685d1d8115
```
 810 | @@ -811,7 +811,7 @@ UniValue SignTransaction(CMutableTransaction& mtx, const UniValue& prevTxsUnival
 811 |                  }
 812 |                  Coin newcoin;
 813 |                  newcoin.out.scriptPubKey = scriptPubKey;
 814 | -                newcoin.out.nValue = 0;
 815 | +                newcoin.out.nValue = MAX_MONEY;
```
laanwj commented at 3:34 PM on July 9, 2018:

isn't MAX_MONEY theoretically a valid amount to send?

l2a5b1 commented at 6:56 PM on July 9, 2018:

I left a similar comment in #12458#pullrequestreview-135231078, which has been addressed by @Empact in a163673.

achow101 commented at 9:21 PM on July 9, 2018:

No, MAX_MONEY is defined as 21000000 BTC, but the actual maximum is 20999999.97690000 BTC so you couldn't actually have MAX_MONEY.

l2a5b1 commented at 8:12 AM on July 10, 2018:

I was wondering about the use of MAX_MONEY understanding that it doesn't represent the maximum money supply (Note that this constant is *not* the total money supply, which in Bitcoin currently happens to be less than 21,000,000 BTC), but still felt the need to comment on it, because it seemed a valid transaction amount to me on the basis of at least its documentation (No amount larger than this (in satoshi) is valid.), the function MoneyRange (which defines a range that includes MAX_MONEY) and unit test vectors below.

https://github.com/bitcoin/bitcoin/blob/b641f60425674d737d77abd8c49929d953ea4154/src/amount.h#L17-L27

https://github.com/bitcoin/bitcoin/blob/b641f60425674d737d77abd8c49929d953ea4154/src/test/data/tx_valid.json#L78-L80

https://github.com/bitcoin/bitcoin/blob/b641f60425674d737d77abd8c49929d953ea4154/src/test/data/tx_invalid.json#L45-L47

To me the amount MAX_MONEY is as valid as the value 0 that it replaces, but admittedly I don't fully comprehend all the details.

l2a5b1 commented at 10:17 AM on July 10, 2018:

Please ignore the explanation why the use of MAX_MONEY is confusing to me. I needed @sipa's confirmation #12458 (comment) to connect the dots.

laanwj commented at 2:51 PM on July 10, 2018:

@achow101 True, thanks. Ok with me then.
achow101 commented at 9:26 PM on July 9, 2018: member

utACK 685d1d8115f61b15115d80523dd8273f0a816534
laanwj commented at 3:04 PM on July 10, 2018: member

utACK 685d1d8115f61b15115d80523dd8273f0a816534
laanwj merged this on Jul 10, 2018
laanwj closed this on Jul 10, 2018
laanwj referenced this in commit fad42e8c4a on Jul 10, 2018
laanwj removed this from the "Blockers" column in a project