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

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.

same

Yes. Addressed in 6a41954.

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.

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.

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.

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

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.

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.

Could use list comprehension:

dec_tx = [self.nodes[3].decoderawtransaction(tx['hex'] for tx in result]

Could use dictionary comprehension:

outputs = {self.nodes[2].getnewaddress(): value for value in (1.0, 1.1, 1.2, 1.3)}

Good idea, thanks.

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)]]

Could use list comprehension (and similarly below):

output = [d['vout'][1 - r['changepos']]['value'] for d, r in zip(dec_tx, result)]

New argument looks like it could be const reference

Indeed.

Little better to use size_t here instead of unsigned int.

OK.

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

Absolutely.

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.

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

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

Will change ! to not.

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

OK.

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

OK.

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.