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

ajtowns · 2018-06-27T16:16:35Z

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.

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.

ajtowns · 2018-06-27T16:18:36Z

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 · 2018-06-28T09:29:54Z

Note to reviewers: This pull request conflicts with the following ones:

Enforce that amount is provided for signrawtransaction prevtxs #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.

sipa · 2018-07-06T03:34:55Z

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

In any case, utACK 685d1d8. Also tested that it still works after merge with #13425.

Empact · 2018-07-06T03:50:22Z

Either works. Remaining differences are a few incidental invalid call tests and returning without updating the input on mine. @ajtowns?

maflcko · 2018-07-07T07:52:13Z

test/functional/rpc_rawtransaction.py

+                succ = self.nodes[0].signrawtransactionwithwallet(rawtx, [prevtx])
+                assert succ["complete"]
+
+            if type != "legacy":


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?

Alternatively remove the prevtx variable from the patch and always pass a fresh dict to signrawtransactionwithwallet: prevtxs=dict(txid=txid, amount missing...)

maflcko · 2018-07-07T07:58:52Z

utACK 685d1d8 tests only (haven't looked at the code changes, feel free to ignore my style nits)

laanwj · 2018-07-09T15:34:32Z

src/rpc/rawtransaction.cpp

@@ -811,7 +811,7 @@ UniValue SignTransaction(CMutableTransaction& mtx, const UniValue& prevTxsUnival
                }
                Coin newcoin;
                newcoin.out.scriptPubKey = scriptPubKey;
-                newcoin.out.nValue = 0;
+                newcoin.out.nValue = MAX_MONEY;


isn't MAX_MONEY theoretically a valid amount to send?

I left a similar comment in #12458 (review), which has been addressed by @Empact in a163673.

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

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.

bitcoin/src/amount.h

Lines 17 to 27 in b641f60

/** No amount larger than this (in satoshi) is valid.

*

* Note that this constant is *not* the total money supply, which in Bitcoin

* currently happens to be less than 21,000,000 BTC for various reasons, but

* rather a sanity check. As this sanity check is used by consensus-critical

* validation code, the exact value of the MAX_MONEY constant is consensus

* critical; in unusual circumstances like a(nother) overflow bug that allowed

* for the creation of coins out of thin air modification could lead to a fork.

* */

static const CAmount MAX_MONEY = 21000000 * COIN;

inline bool MoneyRange(const CAmount& nValue) { return (nValue >= 0 && nValue <= MAX_MONEY); }

bitcoin/src/test/data/tx_valid.json

Lines 78 to 80 in b641f60

["MAX_MONEY output + 0 output"],

[[["0000000000000000000000000000000000000000000000000000000000000100", 0, "HASH160 0x14 0xb558cbf4930954aa6a344363a15668d7477ae716 EQUAL"]],

"01000000010001000000000000000000000000000000000000000000000000000000000000000000006d483045022027deccc14aa6668e78a8c9da3484fbcd4f9dcc9bb7d1b85146314b21b9ae4d86022100d0b43dece8cfb07348de0ca8bc5b86276fa88f7f2138381128b7c36ab2e42264012321029bb13463ddd5d2cc05da6e84e37536cb9525703cfd8f43afdb414988987a92f6acffffffff020040075af075070001510000000000000000015100000000", "P2SH"],

bitcoin/src/test/data/tx_invalid.json

Lines 45 to 47 in b641f60

["MAX_MONEY output + 1 output"],

[[["0000000000000000000000000000000000000000000000000000000000000100", 0, "HASH160 0x14 0xb558cbf4930954aa6a344363a15668d7477ae716 EQUAL"]],

"01000000010001000000000000000000000000000000000000000000000000000000000000000000006d483045022027deccc14aa6668e78a8c9da3484fbcd4f9dcc9bb7d1b85146314b21b9ae4d86022100d0b43dece8cfb07348de0ca8bc5b86276fa88f7f2138381128b7c36ab2e42264012321029bb13463ddd5d2cc05da6e84e37536cb9525703cfd8f43afdb414988987a92f6acffffffff020040075af075070001510001000000000000015100000000", "P2SH"],

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.

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.

@achow101 True, thanks. Ok with me then.

achow101 · 2018-07-09T21:26:08Z

utACK 685d1d8

laanwj · 2018-07-10T15:04:38Z

utACK 685d1d8

…eeded but missing 685d1d8 [tests] Check signrawtransaction* errors on missing prevtx info (Anthony Towns) a3b065b Error on missing amount in signrawtransaction* (Anthony Towns) Pull request description: 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. Tree-SHA512: 8e2ff89d5bcf79548e569210af0d850028bc98d86c149b92207c9300ab1d63664a7e2b222c1be403a15941aa5cf36ccc3c0d570ee1c1466f3496b4fe06c17e11

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. Github-Pull: bitcoin#13547 Rebased-From: a3b065b

Github-Pull: bitcoin#13547 Rebased-From: 685d1d8

…t is needed but missing 212ef1f [tests] Check signrawtransaction* errors on missing prevtx info (Anthony Towns) 1825e37 Error on missing amount in signrawtransaction* (Anthony Towns) Pull request description: Backport of #13547 to 0.16 Tree-SHA512: 7a660023b6948632a1f949443c18fa45add75ec8c36df1ebbaccd181dd1560c1bef460f061f8dab36b6a5df295eb4967effaa2cf55ea06b41d8f7562842a39ec

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. Github-Pull: bitcoin#13547 Rebased-From: a3b065b

Github-Pull: bitcoin#13547 Rebased-From: 685d1d8

…nt is needed but missing 685d1d8 [tests] Check signrawtransaction* errors on missing prevtx info (Anthony Towns) a3b065b Error on missing amount in signrawtransaction* (Anthony Towns) Pull request description: 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 bitcoin#12458, Fixes: bitcoin#12429. Tree-SHA512: 8e2ff89d5bcf79548e569210af0d850028bc98d86c149b92207c9300ab1d63664a7e2b222c1be403a15941aa5cf36ccc3c0d570ee1c1466f3496b4fe06c17e11

Summary: * 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. * [tests] Check signrawtransaction* errors on missing prevtx info Backport of Core [[bitcoin/bitcoin#13547 | PR13547]] Test Plan: ninja all check-all Reviewers: #bitcoin_abc, Fabien Reviewed By: #bitcoin_abc, Fabien Differential Revision: https://reviews.bitcoinabc.org/D8082

…nt is needed but missing 685d1d8 [tests] Check signrawtransaction* errors on missing prevtx info (Anthony Towns) a3b065b Error on missing amount in signrawtransaction* (Anthony Towns) Pull request description: 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 bitcoin#12458, Fixes: bitcoin#12429. Tree-SHA512: 8e2ff89d5bcf79548e569210af0d850028bc98d86c149b92207c9300ab1d63664a7e2b222c1be403a15941aa5cf36ccc3c0d570ee1c1466f3496b4fe06c17e11

ajtowns added 2 commits June 28, 2018 02:06

[tests] Check signrawtransaction* errors on missing prevtx info

685d1d8

fanquake added the RPC/REST/ZMQ label Jun 27, 2018

ajtowns mentioned this pull request Jun 27, 2018

Enforce that amount is provided for signrawtransaction prevtxs #12458

Closed

Empact mentioned this pull request Jul 7, 2018

WIP: bitcoin-tx: Require that input amount is provided for witness transactions #13608

Closed

maflcko reviewed Jul 7, 2018

View reviewed changes

laanwj reviewed Jul 9, 2018

View reviewed changes

laanwj merged commit 685d1d8 into bitcoin:master Jul 10, 2018

Bushstar mentioned this pull request Jul 13, 2018

commits from bitcoin/master FeatherCoin/Feathercoin#354

Merged

luke-jr pushed a commit to luke-jr/bitcoin that referenced this pull request Jul 29, 2018

[tests] Check signrawtransaction* errors on missing prevtx info

212ef1f

Github-Pull: bitcoin#13547 Rebased-From: 685d1d8

luke-jr mentioned this pull request Jul 29, 2018

[0.16] Make signrawtransaction give an error when amount is needed but missing #13796

Merged

sipa added the Needs release note label Aug 14, 2018

HashUnlimited pushed a commit to chaincoin/chaincoin that referenced this pull request Jan 11, 2019

[tests] Check signrawtransaction* errors on missing prevtx info

d84b3c7

Github-Pull: bitcoin#13547 Rebased-From: 685d1d8

fanquake removed the Needs release note label Mar 20, 2019

theStack mentioned this pull request Dec 15, 2021

bitcoin-tx: Require that input amount is provided for witness transactions #23784

Merged

bitcoin locked as resolved and limited conversation to collaborators Dec 16, 2021

Provide feedback

Saved searches

Use saved searches to filter your results more quickly

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

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

ajtowns commented Jun 27, 2018 •

edited

Loading

ajtowns commented Jun 27, 2018

DrahtBot commented Jun 28, 2018 •

edited

Loading

sipa commented Jul 6, 2018

Empact commented Jul 6, 2018

maflcko Jul 7, 2018

maflcko Jul 7, 2018 •

edited

Loading

maflcko commented Jul 7, 2018 •

edited

Loading

laanwj Jul 9, 2018

l2a5b1 Jul 9, 2018

achow101 Jul 9, 2018

l2a5b1 Jul 10, 2018

l2a5b1 Jul 10, 2018

laanwj Jul 10, 2018

achow101 commented Jul 9, 2018

laanwj commented Jul 10, 2018

	/** No amount larger than this (in satoshi) is valid.
	*
	* Note that this constant is not the total money supply, which in Bitcoin
	* currently happens to be less than 21,000,000 BTC for various reasons, but
	* rather a sanity check. As this sanity check is used by consensus-critical
	* validation code, the exact value of the MAX_MONEY constant is consensus
	* critical; in unusual circumstances like a(nother) overflow bug that allowed
	* for the creation of coins out of thin air modification could lead to a fork.
	* */
	static const CAmount MAX_MONEY = 21000000 * COIN;
	inline bool MoneyRange(const CAmount& nValue) { return (nValue >= 0 && nValue <= MAX_MONEY); }

	["MAX_MONEY output + 0 output"],
	[[["0000000000000000000000000000000000000000000000000000000000000100", 0, "HASH160 0x14 0xb558cbf4930954aa6a344363a15668d7477ae716 EQUAL"]],
	"01000000010001000000000000000000000000000000000000000000000000000000000000000000006d483045022027deccc14aa6668e78a8c9da3484fbcd4f9dcc9bb7d1b85146314b21b9ae4d86022100d0b43dece8cfb07348de0ca8bc5b86276fa88f7f2138381128b7c36ab2e42264012321029bb13463ddd5d2cc05da6e84e37536cb9525703cfd8f43afdb414988987a92f6acffffffff020040075af075070001510000000000000000015100000000", "P2SH"],

	["MAX_MONEY output + 1 output"],
	[[["0000000000000000000000000000000000000000000000000000000000000100", 0, "HASH160 0x14 0xb558cbf4930954aa6a344363a15668d7477ae716 EQUAL"]],
	"01000000010001000000000000000000000000000000000000000000000000000000000000000000006d483045022027deccc14aa6668e78a8c9da3484fbcd4f9dcc9bb7d1b85146314b21b9ae4d86022100d0b43dece8cfb07348de0ca8bc5b86276fa88f7f2138381128b7c36ab2e42264012321029bb13463ddd5d2cc05da6e84e37536cb9525703cfd8f43afdb414988987a92f6acffffffff020040075af075070001510001000000000000015100000000", "P2SH"],

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

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

Conversation

ajtowns commented Jun 27, 2018 • edited Loading

ajtowns commented Jun 27, 2018

DrahtBot commented Jun 28, 2018 • edited Loading

sipa commented Jul 6, 2018

Empact commented Jul 6, 2018

maflcko Jul 7, 2018

Choose a reason for hiding this comment

maflcko Jul 7, 2018 • edited Loading

Choose a reason for hiding this comment

maflcko commented Jul 7, 2018 • edited Loading

laanwj Jul 9, 2018

Choose a reason for hiding this comment

l2a5b1 Jul 9, 2018

Choose a reason for hiding this comment

achow101 Jul 9, 2018

Choose a reason for hiding this comment

l2a5b1 Jul 10, 2018

Choose a reason for hiding this comment

l2a5b1 Jul 10, 2018

Choose a reason for hiding this comment

laanwj Jul 10, 2018

Choose a reason for hiding this comment

achow101 commented Jul 9, 2018

laanwj commented Jul 10, 2018

ajtowns commented Jun 27, 2018 •

edited

Loading

DrahtBot commented Jun 28, 2018 •

edited

Loading

maflcko Jul 7, 2018 •

edited

Loading

maflcko commented Jul 7, 2018 •

edited

Loading