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.
Concept ACK.
Nice. Concept ACK. Needs test.
@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.
@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).
@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.
Concept ACK
2560 | @@ -2553,6 +2561,19 @@ UniValue fundrawtransaction(const JSONRPCRequest& request) 2561 | feeRate = CFeeRate(AmountFromValue(options["feeRate"])); 2562 | overrideEstimatedFeerate = true; 2563 | } 2564 | + 2565 | + if (options.exists("subtractFeeFromAmount")) { 2566 | + subtractFeeFromAmount = options["subtractFeeFromAmount"].get_array(); 2567 | + for (unsigned int idx = 0; idx < subtractFeeFromAmount.size(); idx++) { 2568 | + string strAddress(subtractFeeFromAmount[idx].get_str());
This will thrown an exception if one of the elements in the array is numeric. But I think this is okay.
Code Review ACK a979010c80d5875ab26c9cdd5401e2b2905dd572. Squash required.
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?
@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.
@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.
utACK
686 | + 687 | + # total fee isn't affected by the subtractFeeFromAmount option 688 | + assert(result[0]['fee'] == result[1]['fee']) 689 | + assert(result[0]['fee'] == result[2]['fee']) 690 | + assert(result[0]['fee'] == result[3]['fee']) 691 | + assert(result[4]['fee'] == result[5]['fee'])
nit: you can condense this to: assert(result[0]['fee']==result[1]['fee']== result[2]['fee']==result[3]['fee'])
Interesting. I didn't know Python did that. I will do as you suggest.
Addressed in 6a41954.
690 | + assert(result[0]['fee'] == result[3]['fee']) 691 | + assert(result[4]['fee'] == result[5]['fee']) 692 | + 693 | + # change amounts in result 0, 1, and 2 are the same 694 | + assert(change[0] == change[1]) 695 | + assert(change[0] == change[2])
same
Yes. Addressed in 6a41954.
2569 | + CBitcoinAddress address(strAddress); 2570 | + if (!address.IsValid()) 2571 | + throw JSONRPCError(RPC_INVALID_ADDRESS_OR_KEY, string("Invalid Bitcoin address: ")+strAddress); 2572 | + if (setSubtractFeeFromAmount.count(strAddress)) 2573 | + throw JSONRPCError(RPC_INVALID_PARAMETER, string("Invalid parameter, duplicated address: ")+strAddress); 2574 | + setSubtractFeeFromAmount.insert(strAddress);
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.
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.
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.
@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.
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.
@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.
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.
Addressed @mrbandrews' nits. Should I re-squash now, or leave the 'nit' commit separate for a while?
utACK 6a41954895460a033afc352af5c137418591cc6b @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.
2180 | @@ -2181,14 +2181,16 @@ bool CWallet::SelectCoins(const vector<COutput>& vAvailableCoins, const CAmount& 2181 | return res; 2182 | } 2183 | 2184 | -bool CWallet::FundTransaction(CMutableTransaction& tx, CAmount& nFeeRet, bool overrideEstimatedFeeRate, const CFeeRate& specificFeeRate, int& nChangePosInOut, std::string& strFailReason, bool includeWatching, bool lockUnspents, const CTxDestination& destChange) 2185 | +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)
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.
2487 | + " \"lockUnspents\" (boolean, optional, default false) Lock selected unspent outputs\n" 2488 | + " \"feeRate\" (numeric, optional, default not set: makes wallet determine the fee) Set a specific feerate (" + CURRENCY_UNIT + " per KB)\n" 2489 | + " \"subtractFeeFromAmount\" (array, optional) A json array with addresses.\n" 2490 | + " The fee will be equally deducted from the amount of each selected address.\n" 2491 | + " Those recipients will receive less bitcoins than you enter in their corresponding amount field.\n" 2492 | + " If no addresses are specified here, the sender pays the fee.\n"
Maybe s/If no addresses are specified here/If no addresses specified here are outputs in the transaction
2484 | + " \"changeAddress\" (string, optional, default pool address) The bitcoin address to receive the change\n" 2485 | + " \"changePosition\" (numeric, optional, default random) The index of the change output\n" 2486 | + " \"includeWatching\" (boolean, optional, default false) Also select inputs which are watch only\n" 2487 | + " \"lockUnspents\" (boolean, optional, default false) Lock selected unspent outputs\n" 2488 | + " \"feeRate\" (numeric, optional, default not set: makes wallet determine the fee) Set a specific feerate (" + CURRENCY_UNIT + " per KB)\n" 2489 | + " \"subtractFeeFromAmount\" (array, optional) A json array with addresses.\n"
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.
693 | + 694 | + # outputs in result 0, 1, and 2 are the same 695 | + assert(output[0] == output[1] == output[2]) 696 | + 697 | + # 0's output should be equal to 3's (output plus fee) 698 | + assert(output[0] == output[3] + result[3]['fee'])
Debug output will be a little better if you use assert_equal instead of assert here and below.
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?
Makes sense. In case something fails we have the verbose output.
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))
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)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?
Fine to include it here.
661 | + self.nodes[3].fundrawtransaction(rawtx, {"subtractFeeFromAmount": [addr2]}), # addr2 isn't an output, so no subtraction 662 | + self.nodes[3].fundrawtransaction(rawtx, {"subtractFeeFromAmount": [addr1]}), # uses min_relay_tx_fee (set by settxfee) 663 | + self.nodes[3].fundrawtransaction(rawtx, {"feeRate": 2*min_relay_tx_fee}), 664 | + self.nodes[3].fundrawtransaction(rawtx, {"feeRate": 2*min_relay_tx_fee, "subtractFeeFromAmount": [addr1]})] 665 | + 666 | + dec_tx = [self.nodes[3].decoderawtransaction(result[0]['hex']),
Could use list comprehension:
dec_tx = [self.nodes[3].decoderawtransaction(tx['hex'] for tx in result]
705 | + 706 | + # 4's (change plus fee) should be equal to 5's change 707 | + assert(change[4] + result[4]['fee'] == change[5]) 708 | + 709 | + inputs = [] 710 | + addr0 = self.nodes[2].getnewaddress()
Could use dictionary comprehension:
outputs = {self.nodes[2].getnewaddress(): value for value in (1.0, 1.1, 1.2, 1.3)}
Good idea, thanks.
731 | + dec_tx[1]['vout'][1]['value'], 732 | + dec_tx[1]['vout'][2]['value'], 733 | + dec_tx[1]['vout'][3]['value'], 734 | + dec_tx[1]['vout'][4]['value']]] 735 | + del output[0][result[0]['changepos']] 736 | + del output[1][result[1]['changepos']]
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)]]
668 | + self.nodes[3].decoderawtransaction(result[2]['hex']), 669 | + self.nodes[3].decoderawtransaction(result[3]['hex']), 670 | + self.nodes[3].decoderawtransaction(result[4]['hex']), 671 | + self.nodes[3].decoderawtransaction(result[5]['hex'])] 672 | + 673 | + output = [dec_tx[0]['vout'][1 - result[0]['changepos']]['value'],
Could use list comprehension (and similarly below):
output = [d['vout'][1 - r['changepos']]['value'] for d, r in zip(dec_tx, result)]
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>?
@dooglus good point, I don't think so. Recently ran into this writing extended rpc tests for something.
@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.
I don't think objects in JSON are assumed to have a meaningful ordering.
So we are saying that it's OK to use a list of integer indexes into the list of outputs because:
fundrawtransaction the output list already has its order fixed (it's a raw transaction already, not a JSON object)createrawtransaction is always interpreted as an ordered dictionary anywayRight?
(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 | sha1sumand found that the outputs appear in the raw transaction in the same order as listed in the input to createrawtransaction)
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).
@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.
+1 on address reuse being a sometimes valuable tool
What happens next? I addressed all the comments. Is there something else I need to do?
2180 | @@ -2181,14 +2181,15 @@ bool CWallet::SelectCoins(const vector<COutput>& vAvailableCoins, const CAmount& 2181 | return res; 2182 | } 2183 | 2184 | -bool CWallet::FundTransaction(CMutableTransaction& tx, CAmount& nFeeRet, bool overrideEstimatedFeeRate, const CFeeRate& specificFeeRate, int& nChangePosInOut, std::string& strFailReason, bool includeWatching, bool lockUnspents, const CTxDestination& destChange) 2185 | +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)
New argument looks like it could be const reference
Indeed.
2186 | {
2187 | vector<CRecipient> vecSend;
2188 |
2189 | // Turn the txout set into a CRecipient vector
2190 | - BOOST_FOREACH(const CTxOut& txOut, tx.vout)
2191 | + for (unsigned int idx = 0; idx < tx.vout.size(); idx++)
Little better to use size_t here instead of unsigned int.
OK.
2579 | @@ -2567,10 +2580,21 @@ UniValue fundrawtransaction(const JSONRPCRequest& request) 2580 | if (changePosition != -1 && (changePosition < 0 || (unsigned int)changePosition > tx.vout.size())) 2581 | throw JSONRPCError(RPC_INVALID_PARAMETER, "changePosition out of bounds"); 2582 | 2583 | + for (unsigned int idx = 0; idx < subtractFeeFromOutputs.size(); idx++) { 2584 | + int pos = subtractFeeFromOutputs[idx].get_int(); 2585 | + if (setSubtractFeeFromOutputs.count(pos)) 2586 | + throw JSONRPCError(RPC_INVALID_PARAMETER, strprintf("%s%d", "Invalid parameter, duplicated position: ", pos));
Little unusual to use %s for the main string instead of strprintf("Invalid parameter, duplicated position: %d", pos)
Absolutely.
2485 | + " \"changeAddress\" (string, optional, default pool address) The bitcoin address to receive the change\n" 2486 | + " \"changePosition\" (numeric, optional, default random) The index of the change output\n" 2487 | + " \"includeWatching\" (boolean, optional, default false) Also select inputs which are watch only\n" 2488 | + " \"lockUnspents\" (boolean, optional, default false) Lock selected unspent outputs\n" 2489 | + " \"feeRate\" (numeric, optional, default not set: makes wallet determine the fee) Set a specific feerate (" + CURRENCY_UNIT + " per KB)\n" 2490 | + " \"subtractFeeFromOutputs\" (array, optional) A json array of integers.\n"
Maybe just say an array instead of a json array, since the whole data structure is json.
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.
523 | @@ -524,14 +524,18 @@ def assert_fee_amount(fee, tx_size, fee_per_kB): 524 | if fee > (tx_size + 2) * fee_per_kB / 1000: 525 | raise AssertionError("Fee of %s BTC too high! (Should be %s BTC)"%(str(fee), str(target_fee))) 526 | 527 | -def assert_equal(thing1, thing2): 528 | - if thing1 != thing2: 529 | - raise AssertionError("%s != %s"%(str(thing1),str(thing2))) 530 | +def assert_equal(thing1, thing2, *args): 531 | + if thing1 != thing2 or any(thing1 != arg for arg in args): 532 | + raise AssertionError("!(%s)" % " == ".join(str(arg) for arg in (thing1, thing2) + args))
Since it is python not c, maybe replace ! with not
I can't imagine who might have written that! ;)
Will change ! to not.
695 | + # split the fee between outputs 0, 2, and 3, but not output 1 696 | + self.nodes[3].fundrawtransaction(rawtx, {"subtractFeeFromOutputs": [0, 2, 3]})] 697 | + 698 | + dec_tx = [self.nodes[3].decoderawtransaction(result[0]['hex']), 699 | + self.nodes[3].decoderawtransaction(result[1]['hex'])] 700 | +
Maybe add comment describing output, could be # Nested list of non-change output amounts for each transaction
OK.
699 | + self.nodes[3].decoderawtransaction(result[1]['hex'])] 700 | + 701 | + output = [[out['value'] for i, out in enumerate(d['vout']) if i != r['changepos']] 702 | + for d, r in zip(dec_tx, result)] 703 | + 704 | + share = [o0 - o1 for o0, o1 in zip(output[0], output[1])]
Maybe add comment like # List of difference in output amounts between normal and subtractFee transactions.
OK.
666 | + 667 | + # total fee isn't affected by the subtractFeeFromOutputs option 668 | + assert_equal(result[0]['fee'], result[1]['fee'], result[2]['fee']) 669 | + assert_equal(result[3]['fee'], result[4]['fee']) 670 | + 671 | + # change amounts in result 0 and 1 are the same
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.
Agree.
Change looks good as is, just left a few possible suggestions.
Lightly tested ACK 56ea97409349baaf064c6c2a9da0ba8bbe207f27.
Addressed @ryanofsky nits, rebased, squashed.
Lightly tested ACK 453bda63dd90986501ee61426e4d768a400bd371
Can this be merged now?
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).
#11872 was recently posted implementing this change.
Indeed. Though, note that the rpc currently rejects duplicate addresses, and that specific case is not changed in #11872.