Add 'subtractFeeFromAmount' option to 'fundrawtransaction'. #9222

Merged
merged 1 commit into from Jan 12, 2017

Projects

None yet
@dooglus
Contributor
dooglus commented Nov 25, 2016

I notice it is now possible to have the fee subtracted from the output amounts by specifying a subtractFeeFromAmount parameter in both sendtoaddress and sendmany, but not in fundrawtransaction.

This commit adds the option to fundrawtransaction.

@gmaxwell
Member

Concept ACK.

@jonasschnelli
Member

Nice. Concept ACK.
Needs test.

@dooglus
Contributor
dooglus commented Nov 26, 2016

@jonasschnelli I tried finding the fundrawtransaction tests but couldn't. Where are they?

src/test/rpc_tests.cpp seems like the natural place for them, but I see no 'fund' in there at all.

@jonasschnelli
Member

@dooglus
There is one at ./qa/rpc-tests/fundrawtransaction.py.
The tests should make sure that the subtractFeeFromAmount option work in conjunction with the custom feerate option (haven't look at your code so far).

@dooglus
Contributor
dooglus commented Nov 27, 2016

@jonasschnelli Thanks for pointing me at the qa/ directory. I hadn't noticed it before.

I have added tests for subtractFeeFromAmount, including checking that it works in combination with custom feerate.

@sipa
Member
sipa commented Nov 28, 2016

Concept ACK

src/wallet/rpcwallet.cpp
+ if (options.exists("subtractFeeFromAmount")) {
+ subtractFeeFromAmount = options["subtractFeeFromAmount"].get_array();
+ for (unsigned int idx = 0; idx < subtractFeeFromAmount.size(); idx++) {
+ string strAddress(subtractFeeFromAmount[idx].get_str());
@jonasschnelli
jonasschnelli Nov 28, 2016 Member

This will thrown an exception if one of the elements in the array is numeric. But I think this is okay.

@jonasschnelli
Member

Code Review ACK a979010.
Squash required.

@dooglus
Contributor
dooglus commented Nov 28, 2016

To 'squash' the commits do I just rewrite the same branch with a push --force? Or make a new branch and a new pull request?

@jonasschnelli
Member

@dooglus: Yes. I normally do a git rebase -I head~<amount-of-commits>, find the commit you'd like to squash to and mark all later commits with a s. Then git push --force.

@dooglus
Contributor
dooglus commented Nov 28, 2016

@jonasschnelli Thanks. The 'i' is lowercase and the 'HEAD' is uppercase but it was close enough.

I used git rebase -i HEAD~3 and it appears to have worked.

@morcos
Contributor
morcos commented Nov 28, 2016

utACK

qa/rpc-tests/fundrawtransaction.py
+ assert(result[0]['fee'] == result[1]['fee'])
+ assert(result[0]['fee'] == result[2]['fee'])
+ assert(result[0]['fee'] == result[3]['fee'])
+ assert(result[4]['fee'] == result[5]['fee'])
@mrbandrews
mrbandrews Nov 29, 2016 Contributor

nit: you can condense this to:
assert(result[0]['fee']==result[1]['fee']== result[2]['fee']==result[3]['fee'])

@dooglus
dooglus Nov 29, 2016 Contributor

Interesting. I didn't know Python did that. I will do as you suggest.

@dooglus
dooglus Nov 29, 2016 Contributor

Addressed in 6a41954.

qa/rpc-tests/fundrawtransaction.py
+
+ # change amounts in result 0, 1, and 2 are the same
+ assert(change[0] == change[1])
+ assert(change[0] == change[2])
@mrbandrews
mrbandrews Nov 29, 2016 Contributor

same

@dooglus
dooglus Nov 29, 2016 edited Contributor

Yes. Addressed in 6a41954.

src/wallet/rpcwallet.cpp
+ throw JSONRPCError(RPC_INVALID_ADDRESS_OR_KEY, string("Invalid Bitcoin address: ")+strAddress);
+ if (setSubtractFeeFromAmount.count(strAddress))
+ throw JSONRPCError(RPC_INVALID_PARAMETER, string("Invalid parameter, duplicated address: ")+strAddress);
+ setSubtractFeeFromAmount.insert(strAddress);
@mrbandrews
mrbandrews Nov 29, 2016 Contributor

Should it throw an error if the given address is valid but is not among the outputs? (would have to check for this below, after retrieving the transaction). It seems like in this case, the user is trying to pay the fee with one of the outputs but has made an error.

@dooglus
dooglus Nov 29, 2016 Contributor

I wanted it to behave the same as it does in sendmany, where it doesn't complain if you include an address that isn't a recipient at all.

The user could have a list of addresses which should pay fees when sent to, and use that same list as their subtractFeeFromAmount parameter whichever addresses they are sending to.

@ryanofsky
ryanofsky Nov 30, 2016 Contributor

One difference between this and sendmany is that sendmany requires transaction outputs to be base58 addresses, and takes amounts and subtractfeefromamount arguments in base58 form, while fundrawtransaction allows outputs to be arbitrary scripts. This means with the PR in its current form, there may be no way for the new subtractFeeFromAmount argument to refer to certain outputs.

Instead of adding a subtractFeeFromAmount argument, I might suggest adding a subtractFeeFromPositions argument that takes a list of integer output indices. This would give callers the flexibility to refer to all outputs, be more consistent with the existing changePosition argument (which is also an integer output index), and also eliminate the need for ExtractDestination and CBitcoinAddress::ToString invocations in CWallet::FundTransaction.

@instagibbs
instagibbs Dec 1, 2016 Contributor

@ryanofsky I like the idea but am a bit worried about the interaction of subtractFeeFromPositions and changePosition. It might not be clear to the user if the position marking is done before or after change output is added, or discount the wrong output by adding a changePosition argument.

@dooglus
dooglus Dec 1, 2016 Contributor

At the time of running fundrawtransaction there is no change output, and the user wouldn't know where the change will be inserted, so the position marking must be done before the change output is added.

I think since it is possible to add arbitrary hex output scripts which may not even have a corresponding address we need to be able to address the outputs by number rather than by address. It's also kind of ugly having to give the same address twice, once to createrawtransaction and then again to fundrawtransaction. I think using the output index (0 based) is cleaner.

@instagibbs
instagibbs Dec 1, 2016 edited Contributor

@dooglus the user will "know" where change is going if they attempt to set the change index they're setting in the option, which is my point. It's not plainly clear how this should interact, unless you spell it out.

@dooglus
dooglus Dec 1, 2016 Contributor

Oh, I see. So I should spell it out...

I think it makes sense to use the position indices before the change output is added.

@dooglus
Contributor
dooglus commented Nov 29, 2016

Addressed @mrbandrews' nits. Should I re-squash now, or leave the 'nit' commit separate for a while?

@morcos
Contributor
morcos commented Nov 29, 2016

re-utACK 6a41954

@dooglus good question, its not always clear. I personally think that if the prior code is not broken , then its ok not to squash.

@jonasschnelli
Member

utACK 6a41954
@dooglus IMO squashing is not required when the commits has a reason to be separated. If it's just a trivial change/overhaul of the previous commit(s) in the PR, it should probably be squashed.

src/wallet/rpcwallet.cpp
+ " \"includeWatching\" (boolean, optional, default false) Also select inputs which are watch only\n"
+ " \"lockUnspents\" (boolean, optional, default false) Lock selected unspent outputs\n"
+ " \"feeRate\" (numeric, optional, default not set: makes wallet determine the fee) Set a specific feerate (" + CURRENCY_UNIT + " per KB)\n"
+ " \"subtractFeeFromAmount\" (array, optional) A json array with addresses.\n"
@ryanofsky
ryanofsky Nov 30, 2016 Contributor

Maybe mention after This will not modify existing inputs, and will add one change output to the outputs above that no existing outputs will be modified either unless subtractFeeFromAmount is specified.

src/wallet/rpcwallet.cpp
+ " \"subtractFeeFromAmount\" (array, optional) A json array with addresses.\n"
+ " The fee will be equally deducted from the amount of each selected address.\n"
+ " Those recipients will receive less bitcoins than you enter in their corresponding amount field.\n"
+ " If no addresses are specified here, the sender pays the fee.\n"
@ryanofsky
ryanofsky Nov 30, 2016 Contributor

Maybe s/If no addresses are specified here/If no addresses specified here are outputs in the transaction

src/wallet/rpcwallet.cpp
+ throw JSONRPCError(RPC_INVALID_ADDRESS_OR_KEY, string("Invalid Bitcoin address: ")+strAddress);
+ if (setSubtractFeeFromAmount.count(strAddress))
+ throw JSONRPCError(RPC_INVALID_PARAMETER, string("Invalid parameter, duplicated address: ")+strAddress);
+ setSubtractFeeFromAmount.insert(strAddress);
@ryanofsky
ryanofsky Nov 30, 2016 Contributor

One difference between this and sendmany is that sendmany requires transaction outputs to be base58 addresses, and takes amounts and subtractfeefromamount arguments in base58 form, while fundrawtransaction allows outputs to be arbitrary scripts. This means with the PR in its current form, there may be no way for the new subtractFeeFromAmount argument to refer to certain outputs.

Instead of adding a subtractFeeFromAmount argument, I might suggest adding a subtractFeeFromPositions argument that takes a list of integer output indices. This would give callers the flexibility to refer to all outputs, be more consistent with the existing changePosition argument (which is also an integer output index), and also eliminate the need for ExtractDestination and CBitcoinAddress::ToString invocations in CWallet::FundTransaction.

src/wallet/wallet.cpp
@@ -2181,14 +2181,16 @@ bool CWallet::SelectCoins(const vector<COutput>& vAvailableCoins, const CAmount&
return res;
}
-bool CWallet::FundTransaction(CMutableTransaction& tx, CAmount& nFeeRet, bool overrideEstimatedFeeRate, const CFeeRate& specificFeeRate, int& nChangePosInOut, std::string& strFailReason, bool includeWatching, bool lockUnspents, const CTxDestination& destChange)
+bool CWallet::FundTransaction(CMutableTransaction& tx, CAmount& nFeeRet, bool overrideEstimatedFeeRate, const CFeeRate& specificFeeRate, int& nChangePosInOut, std::string& strFailReason, bool includeWatching, bool lockUnspents, std::set<std::string>& setSubtractFeeFromAmount, const CTxDestination& destChange)
@ryanofsky
ryanofsky Nov 30, 2016 Contributor

Would suggest changing the new set<string> argument to set<int> to be consistent with the existing nChangePosInOut argument which refers to an output by integer index instead of base58 address string. This would give callers more flexibility in referring to outputs and also simplify handling of the new argument below.

qa/rpc-tests/fundrawtransaction.py
+ self.nodes[3].fundrawtransaction(rawtx, {"feeRate": 2*min_relay_tx_fee}),
+ self.nodes[3].fundrawtransaction(rawtx, {"feeRate": 2*min_relay_tx_fee, "subtractFeeFromAmount": [addr1]})]
+
+ dec_tx = [self.nodes[3].decoderawtransaction(result[0]['hex']),
@ryanofsky
ryanofsky Nov 30, 2016 Contributor

Could use list comprehension:

dec_tx = [self.nodes[3].decoderawtransaction(tx['hex'] for tx in result]
qa/rpc-tests/fundrawtransaction.py
+ self.nodes[3].decoderawtransaction(result[4]['hex']),
+ self.nodes[3].decoderawtransaction(result[5]['hex'])]
+
+ output = [dec_tx[0]['vout'][1 - result[0]['changepos']]['value'],
@ryanofsky
ryanofsky Nov 30, 2016 Contributor

Could use list comprehension (and similarly below):

output = [d['vout'][1 - r['changepos']]['value'] for d, r in zip(dec_tx, result)]
qa/rpc-tests/fundrawtransaction.py
+ assert(output[0] == output[1] == output[2])
+
+ # 0's output should be equal to 3's (output plus fee)
+ assert(output[0] == output[3] + result[3]['fee'])
@ryanofsky
ryanofsky Nov 30, 2016 Contributor

Debug output will be a little better if you use assert_equal instead of assert here and below.

@dooglus
dooglus Nov 30, 2016 Contributor

It appears that assert_equal can only compare two things. For cases like assert(A == B == C == D) would you prefer 3 separate assert_equal() calls instead?

@MarcoFalke
MarcoFalke Nov 30, 2016 Member

Makes sense. In case something fails we have the verbose output.

@ryanofsky
ryanofsky Nov 30, 2016 Contributor

I actually only meant to suggest using assert_equal for binary comparisons like the one on line 698. But if you wanted to use it more broadly, you could extend the function (in util.py) to accept more arguments:

def assert_equal(thing1, thing2, *args):
    if thing1 != thing2 or any(thing1 != arg for arg in args):
        raise AssertionError("!(%s)" % " == ".join(str(arg) for arg in (thing1, thing2) + args))
@dooglus
dooglus Nov 30, 2016 Contributor

Would it be better to extend assert_equal() to take an arbitrary number of parameters and have it compare them pairwise? Something like this would work:

def assert_equal(thing1, thing2, *other_things, depth=0):
    if thing1 != thing2:
        if depth or other_things:
            raise AssertionError("%s != %s (positions %d and %d)"%(str(thing1),str(thing2), depth, depth+1))
        else:
            raise AssertionError("%s != %s"%(str(thing1),str(thing2)))
    if other_things:
        assert_equal(thing2, *other_things, depth = depth + 1)

>>> assert_equal(4, 4, 5)
AssertionError: 4 != 5 (positions 1 and 2)
@dooglus
dooglus Nov 30, 2016 Contributor

I missed your last comment. Your solution is obviously much more elegant.

Is it acceptable to include a change like that in this pull request or should it be separate?

@MarcoFalke
MarcoFalke Nov 30, 2016 Member

Fine to include it here.

qa/rpc-tests/fundrawtransaction.py
+ assert(change[4] + result[4]['fee'] == change[5])
+
+ inputs = []
+ addr0 = self.nodes[2].getnewaddress()
@ryanofsky
ryanofsky Nov 30, 2016 Contributor

Could use dictionary comprehension:

outputs = {self.nodes[2].getnewaddress(): value for value in (1.0, 1.1, 1.2, 1.3)}
@dooglus
dooglus Nov 30, 2016 Contributor

Good idea, thanks.

qa/rpc-tests/fundrawtransaction.py
+ dec_tx[1]['vout'][3]['value'],
+ dec_tx[1]['vout'][4]['value']]]
+ del output[0][result[0]['changepos']]
+ del output[1][result[1]['changepos']]
@ryanofsky
ryanofsky Nov 30, 2016 Contributor

Could use list comprehension:

output = [[out[value] for i, out in enumerate(d['vout']) if i != r['changepos']]
          for d, r in zip(dec_tx, result)]]
@dooglus
Contributor
dooglus commented Dec 6, 2016

I've addressed all the review comments, rebased, squashed, and pushed the resulting commit.

I'm wondering whether there's a potential issue with using integers to select which outputs to subtract the fee from, since the outputs are specified by a JSON dictionary, and dictionary keys are inherently unordered. Are we guaranteed when we createrawtransaction '[]' '{"a0":1,"a1":1,"a2":1}' that a<n> will be output <n>?

@instagibbs
Contributor

@dooglus good point, I don't think so. Recently ran into this writing extended rpc tests for something.

@dooglus
Contributor
dooglus commented Dec 6, 2016

@instagibbs me too:

$ python3
Python 3.4.2

>>> [k for k in {'a':1,'b':2,'c':3,'d':1,'e':1,'f':1}]
['c', 'd', 'f', 'b', 'e', 'a']

Since the input to fundrawtransaction is a raw transaction with its outputs already serialized this is less of an issue. But I tend to string my RPC calls together and expect the outputs to be serialized in the order I type them to createrawtransaction. They always do seem to be in the correct order except when using Python.

@sipa
Member
sipa commented Dec 6, 2016
@MarcoFalke
Member

@dooglus For python you'd have to import OrderedDict (see #7980) but I don't think there is an ordered dict for json, so we should not rely on the order.

@dooglus
Contributor
dooglus commented Dec 6, 2016

So we are saying that it's OK to use a list of integer indexes into the list of outputs because:

  1. by the time we're running fundrawtransaction the output list already has its order fixed (it's a raw transaction already, not a JSON object)
  2. we have no other way to refer to general outputs, since they can be arbitrary hex strings and may not even have a base58 address
  3. Bitcoin Core's use of univalue means that the JSON output list provided to createrawtransaction is always interpreted as an ordered dictionary anyway

Right?

(I tested point 3 as follows:

addr1=$(for x in {1..32}; do bitcoin-cli --testnet getnewaddress; done)
raw=$(bitcoin-cli --testnet createrawtransaction '[]' '{"'$(echo $addr1 | sed 's/ /":1,"/g')'":1}')
addr2=$(bitcoin-cli --testnet decoderawtransaction $raw | grep -E '^ {10}"' | cut -d'"' -f2)
echo $addr1 | sha1sum
echo $addr2 | sha1sum

and found that the outputs appear in the raw transaction in the same order as listed in the input to createrawtransaction)

@sipa
Member
sipa commented Dec 6, 2016

I think reason (1) is enough to make position based indexing ok, and (2) strengthens it.

I don't think (3) is a good reason or something we should ever rely on. The only reason this is brought up is because createrawtransaction accepts an object to list the outputs. If strict ordering is expected there, perhaps we should change that argument from {"addr1":val1, "addr2":val2} to [["addr1",val1],["addr2",val2]] instead (in another PR).

@dooglus
Contributor
dooglus commented Dec 7, 2016

@sipa I'll look into making such a change in a separate PR. Using an object for the outputs not only means we cannot guarantee the order of the outputs but also having the addresses as dictionary keys means we can't have multiple outputs with the same address, which I sometimes like to do to (example).

I assume it would be best to allow the current object format in addition to the new array format for backwards compatibility.

@morcos
Contributor
morcos commented Dec 7, 2016

+1 on address reuse being a sometimes valuable tool

@dooglus
Contributor
dooglus commented Dec 13, 2016

What happens next? I addressed all the comments. Is there something else I need to do?

@ryanofsky

Change looks good as is, just left a few possible suggestions.

Lightly tested ACK 56ea974.

src/wallet/wallet.cpp
@@ -2181,14 +2181,15 @@ bool CWallet::SelectCoins(const vector<COutput>& vAvailableCoins, const CAmount&
return res;
}
-bool CWallet::FundTransaction(CMutableTransaction& tx, CAmount& nFeeRet, bool overrideEstimatedFeeRate, const CFeeRate& specificFeeRate, int& nChangePosInOut, std::string& strFailReason, bool includeWatching, bool lockUnspents, const CTxDestination& destChange)
+bool CWallet::FundTransaction(CMutableTransaction& tx, CAmount& nFeeRet, bool overrideEstimatedFeeRate, const CFeeRate& specificFeeRate, int& nChangePosInOut, std::string& strFailReason, bool includeWatching, bool lockUnspents, std::set<int>& setSubtractFeeFromOutputs, const CTxDestination& destChange)
@ryanofsky
ryanofsky Dec 13, 2016 Contributor

New argument looks like it could be const reference

@dooglus
dooglus Dec 13, 2016 Contributor

Indeed.

src/wallet/wallet.cpp
{
vector<CRecipient> vecSend;
// Turn the txout set into a CRecipient vector
- BOOST_FOREACH(const CTxOut& txOut, tx.vout)
+ for (unsigned int idx = 0; idx < tx.vout.size(); idx++)
@ryanofsky
ryanofsky Dec 13, 2016 Contributor

Little better to use size_t here instead of unsigned int.

@dooglus
dooglus Dec 13, 2016 Contributor

OK.

+ " \"includeWatching\" (boolean, optional, default false) Also select inputs which are watch only\n"
+ " \"lockUnspents\" (boolean, optional, default false) Lock selected unspent outputs\n"
+ " \"feeRate\" (numeric, optional, default not set: makes wallet determine the fee) Set a specific feerate (" + CURRENCY_UNIT + " per KB)\n"
+ " \"subtractFeeFromOutputs\" (array, optional) A json array of integers.\n"
@ryanofsky
ryanofsky Dec 13, 2016 Contributor

Maybe just say an array instead of a json array, since the whole data structure is json.

@dooglus
dooglus Dec 13, 2016 Contributor

Agree, but it appears that everywhere else we refer to 'json array' (see sendmany, addmultisigaddress, lockunspent, listunspent...). Nowhere (in rpcwallet.cpp at least) do we simply say 'an array'.

Will leave as 'json array' for the sake of consistency.

src/wallet/rpcwallet.cpp
+ for (unsigned int idx = 0; idx < subtractFeeFromOutputs.size(); idx++) {
+ int pos = subtractFeeFromOutputs[idx].get_int();
+ if (setSubtractFeeFromOutputs.count(pos))
+ throw JSONRPCError(RPC_INVALID_PARAMETER, strprintf("%s%d", "Invalid parameter, duplicated position: ", pos));
@ryanofsky
ryanofsky Dec 13, 2016 Contributor

Little unusual to use %s for the main string instead of strprintf("Invalid parameter, duplicated position: %d", pos)

@dooglus
dooglus Dec 13, 2016 Contributor

Absolutely.

qa/rpc-tests/test_framework/util.py
- raise AssertionError("%s != %s"%(str(thing1),str(thing2)))
+def assert_equal(thing1, thing2, *args):
+ if thing1 != thing2 or any(thing1 != arg for arg in args):
+ raise AssertionError("!(%s)" % " == ".join(str(arg) for arg in (thing1, thing2) + args))
@ryanofsky
ryanofsky Dec 13, 2016 Contributor

Since it is python not c, maybe replace ! with not

@dooglus
dooglus Dec 13, 2016 Contributor

I can't imagine who might have written that! ;)

Will change ! to not.

qa/rpc-tests/fundrawtransaction.py
+ assert_equal(result[0]['fee'], result[1]['fee'], result[2]['fee'])
+ assert_equal(result[3]['fee'], result[4]['fee'])
+
+ # change amounts in result 0 and 1 are the same
@ryanofsky
ryanofsky Dec 13, 2016 Contributor

This and the 5 following comments are basically just describing the asserts without adding any information. Could maybe remove the comments and condense the asserts.

@dooglus
dooglus Dec 13, 2016 Contributor

Agree.

+
+ dec_tx = [self.nodes[3].decoderawtransaction(result[0]['hex']),
+ self.nodes[3].decoderawtransaction(result[1]['hex'])]
+
@ryanofsky
ryanofsky Dec 13, 2016 Contributor

Maybe add comment describing output, could be # Nested list of non-change output amounts for each transaction

@dooglus
dooglus Dec 13, 2016 Contributor

OK.

qa/rpc-tests/fundrawtransaction.py
+ output = [[out['value'] for i, out in enumerate(d['vout']) if i != r['changepos']]
+ for d, r in zip(dec_tx, result)]
+
+ share = [o0 - o1 for o0, o1 in zip(output[0], output[1])]
@ryanofsky
ryanofsky Dec 13, 2016 Contributor

Maybe add comment like # List of difference in output amounts between normal and subtractFee transactions.

@dooglus
dooglus Dec 13, 2016 Contributor

OK.

@dooglus
Contributor
dooglus commented Dec 13, 2016

Addressed @ryanofsky nits, rebased, squashed.

@ryanofsky
Contributor

Lightly tested ACK 453bda6

@luke-jr luke-jr added a commit to bitcoinknots/bitcoin that referenced this pull request Dec 21, 2016
@dooglus @luke-jr dooglus + luke-jr Add 'subtractFeeFromOutputs' option to 'fundrawtransaction'.
Github-Pull: #9222
Rebased-From: 453bda6
274975f
@dooglus
Contributor
dooglus commented Jan 2, 2017

Can this be merged now?

@laanwj laanwj merged commit 453bda6 into bitcoin:master Jan 12, 2017

1 check passed

continuous-integration/travis-ci/pr The Travis CI build passed
Details
@laanwj laanwj added a commit that referenced this pull request Jan 12, 2017
@laanwj laanwj Merge #9222: Add 'subtractFeeFromAmount' option to 'fundrawtransaction'.
453bda6 Add 'subtractFeeFromOutputs' option to 'fundrawtransaction'. (Chris Moore)
7cb024e
Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment