wallet, contract: Implement contract change option

[jamescowens]

This PR introduces a new setting, contractchangetoinputaddress, that defaults to false, and which controls whether the change from a contract transaction goes back to an input address rather than a new address being created.

Setting this new setting to true will cause the wallet to return the change on an input address for the contract transaction, and avoid the creation of a new change address. This reduces anonymity, which is why the default setting is false to retain the original behavior, but addresses #2326, and any other contract type when set to true.

Note that the CreateTransaction function in wallet.h/cpp has been extended to have a default boolean change_back_to_input_address to support this, and also allow this same feature to be done in a general sendtoaddress RPC (not yet implemented).

A checkbox has also been implemented to be able to set the value of contractchangetoinputaddress from the gui. This uses the dynamic settings file to enable this to be shared between the core and the GUI and allow the setting to be immediately active and the state maintained over a wallet restart.

Closes #2326

[jamescowens]

Tested with voting on testnet node and works.

[jamescowens]

Tested with master key requirement by putting GPUGrid on the greylist. Worked correctly.

[jamescowens]

Just adding an explanatory comment on why we need a boolean change_back_to_input_address, when it would seem to duplicate the functionality of CCoinControl.

The key to understanding why we need this boolean rests in the fact that the destination address for change in CCoinControl must be set explicitly using SetDestination(address). If you use setCoins_in along with CCoinControl, then you don't need the change_back_to_input_address, and in fact it is ignored in that situation. However if you intend on having CreateTransaction CHOOSE the coins automatically by leaving setCoins_in empty, then without the boolean change_back_to_address, you would need to figure out the address you need to send the change to to specify in a CCoinControl object instead - catch-22. This is not optimal, since the case we are dealing with here is to pick an address of one of the inputs to return the change. In the instance where the inputs are actually being decided by SelectCoins in the CreateTransaction, it makes sense for CreateTransaction to determine the change address for one of the inputs after SelectCoins determines the inputs to be used, hence the need for this boolean. (Remember SelectCoins is a private member of CWallet.)

A pretty good argument can be made to put the change_back_to_input_address in the CCoinControl class instead, which would avoid the extra parameter, but that is window dressing in my opinion, and other than avoiding an optional (defaulted) parameter on CreateTransaction, it doesn't really help anything. You still have to teach anything that consumes CCoinControl objects what to do with the change_back_to_input_address boolean...

[div72]

CreateTransaction already has special logic for contracts around line 1842. What about handling the logic there instead of adding a new parameter to the function or adding a variable to CCoinControl?

One possible argument against this is (as mentioned in the PR body) doing it like this way would also allow RPC like sendtoaddress to use it, which frankly I'm against on two grounds:

1. While we haven't done the best job with keeping up with Bitcoin's RPC interface, I am against adding a Gridcoin specific argument for such a generic RPC command. Adding an global option instead of an argument is also an option, however that won't be visible to most RPC users(as people mostly aren't prone to reading documentation) and GUI users can already do this with relative ease.
2. I prefer having usages of non-standard change addresses to be cumbersome. It's already guarded with an option labeled as advanced and I don't like RPC users having an easy way to do it. This is okay for contracts as generally contract transactions are obvious in the way that their output is the address of the owner of the contract/input. Changing this would only affect a small (if any people who do this exist) percentage who uses transactions with contracts to hide the fact they're sending coins. On the other hand having the chance to easily use the same address could affect privacy of both the sender and the receiver.

I also think that this option should be enabled by default as this option does not affect the user's privacy.

[jamescowens]

Yep. That is another good reason why I like this approach. It is very narrowly scoped with a defaulted parameter on the end which is not used anywhere (for right now) except this special case. The reason I did not extend the sendtoaddress RPC is that would add extra functionality not compatible with Bitcoin. You are probably right and we never should add it.

On the comment around the logic around line 1842, that is exactly what we are doing, except you can't decide the input address until after the SelectCoins is done. Doing it in the change determination block, downstream, where you have setCoins_out is the right place. The only question is whether it should be conditioned on a boolean (which effectively comes from the GUI setting).

I am not sure I agree with you on no effect on the user's privacy. In most contract sends, you have a burn address and a change address. Simply having the contract is a reduction of privacy, but I think most people that are privacy conscious would want the obfuscation of a newly generated change address, where people for convenience and backup ease would want the change returned to one of the input addresses.

I think the GUI checkbox (and boolean) should remain to give users a choice. I am on the fence on which way to default it. Perhaps @cyrossignol or @barton2526 can weigh in.

[div72]

Simply having the contract is a reduction of privacy

Random change addresses in non-message contracts are basically no-ops for privacy as assuming the single return to be sender's change is a fair assumption to make in my opinion.

...except you can't decide the input address until after the SelectCoins is done. Doing it in the change determination block, downstream, where you have setCoins_out is the right place.

You're right, what I've meant was that with minor refactoring(change is common enough to justify moving the scriptChange to the top and setting it in a if (contract && gArgs.GetBoolArg("-contractchangetoinputaddress", false)) block after SelectCoins), the parameter could be made redundant. Since the parameter is essentially only used by CreateContractTx, it's existence felt like a misuse of the DRY principle but then again my feelings towards the parameter isn't strong enough to justify typing more.

[jamescowens]

Haha exactly. Your way is probably better but the change is not significant enough for me to do the work to make the change. It is good enough as is…

[jamescowens]

@div72, it sounds like we have agreed this PR is ok in its existing state, and that we will NOT be extending rpc calls to provide this functionality in RPC in a more generalized way. If so, please approve the PR.

[jamescowens]

@div72 has found something very weird in testing... from our slack conversation....

Sorry for the delay on approving the PR, I was testing it. There seems to be a weird edge case where if a contract is sent with the option on, then another contract is sent with the option off and a normal transaction is sent; the normal transaction in the end seems to return the change back to itself. See https://testnet.gridcoinstats.eu/tx/24d1369f55094ea3ef468f366733f72adac963d387b07339862e679b6e87cd37 and https://testnet.gridcoinstats.eu/tx/5d4f7ee1d6d7cf788ffe6430efbd68dc23d4a224911c62e3749bd862e0429f81.
testnet.gridcoinstats.eutestnet.gridcoinstats.eu
Gridcoin TX 24d1369f55094ea3ef468f366733f72adac963d387b07339862e679b6e87cd37, | Gridcoinstats - Your independent block explorer
View details about Gridcoin Network, balances, BOINC users rewards much more.
testnet.gridcoinstats.eutestnet.gridcoinstats.eu
Gridcoin TX 5d4f7ee1d6d7cf788ffe6430efbd68dc23d4a224911c62e3749bd862e0429f81, | Gridcoinstats - Your independent block explorer
View details about Gridcoin Network, balances, BOINC users rewards much more.

Jim Owens 2:49 PM
Hmm... that is weird.
2:49
Are you using sendtoaddress rpc for the normal transaction? (edited)

div72 2:51 PM
GUI.

Jim Owens 2:53 PM
Will you try it with sendtoaddress and see if the same behavior occurs?

div72 2:53 PM
Will do.

Jim Owens 2:54 PM
I am not sure how that would happen unless there is something weird going on with the CCoinControl object used by the GUI.
2:54
Was the custom change address filled out in the GUI send screen?

div72 2:56 PM
I don't think so, I will check again in any case.

Jim Owens 2:56 PM
We definitely will have to track this down. I am puzzled by the coupling.

div72 3:11 PM
Custom change address was empty on the send screen, the behavior is the same with sendtoaddress.

Jim Owens 3:11 PM
If you issue a second normal transaction does it create a new change address?

div72 3:13 PM
Yep, regardless of the status of the setting.

Jim Owens 3:21 PM
what kind of contract did you send?

div72 3:23 PM
Beacon advertisement.

Jim Owens 6:22 PM
I will try to troubleshoot this over the next day or two.
6:23
I just reviewed the code paths and I just don't see how this should be happening.

[div72]

ACK 9b0e959, I was able to reproduce the issue on the tip of development. Since I used beacon contracts, I assume there's a problem with reserving a key as a beacon address. I'll investigate further.

The functionality provided by this PR seems to be working without an issue otherwise.

[jamescowens]

@div72, can you create an issue to track the above problem since it is now confirmed to be independent of this PR? Thanks.

[div72]

can you create an issue to track the above problem since it is now confirmed to be independent of this PR? Thanks.

#2395.