Add option to avoid spending unconfirmed change #3651

Adds the option -spendzeroconfchange=0 to make the wallet avoid spending zero conf change outputs.

Rough workaround to the problems with '-4. Error: The transaction was rejected!' and other problems with chains of unconfirmed transactions and malleability.

The drawback of this is that sending will run out of inputs sooner (resulting in unintuitive "insufficient balance" errors), so if you have a lot of sends you'll need to queue up sends and use sendmany.

Automatic sanity-testing: PASSED, see http://jenkins.bluematt.me/pull-tester/1bbca249b202c4802cc2c4d4de4a26e6392b4d92 for binaries and test log. This test script verifies pulls every time they are updated. It, however, dies sometimes and fails to test properly. If you are waiting on a test, please check timestamps to verify that the test.log is moving at http://jenkins.bluematt.me/pull-tester/current/ Contact BlueMatt on freenode if something looks broken.

Might make sense to make it -minconfstospend so you can ask for more than one confirmation for those who can wait. (reorgs)

ACK code changes.

Note that getbalance will still be counting unconfirmed change after this patch. GetBalance doesn't use AvailableCoins.

Another option that would affect the balance computation as well as coin selection may be to put the condition in CWalletTx::IsConfirmed instead (here https://github.com/bitcoin/bitcoin/blob/master/src/wallet.h#L707).

This would change the functionality from "don't spend unconfirmed change" to "don't count unconfirmed change at all". Not sure if that's desirable, but it's a possibility...

Setting minconf=1 on send* RPCs should have the same effect.

The impression I get is the getbalance issue is less important than ensuring unconfirmed tx's don't get spent; I'd get this patch done and do something more complex with getbalance later. @sipa Good point!

This patch is useful for all users too after all.

An alternative to this is just checking whether unconfirmed coins are in the mempool, before considering them available. That will be more transparent, should always be right, and less restrictive.

@sipa That doesn't fix the issue; they could be mutated in a mempool elsewhere.

@sipa I thought minconf wouldn't have effect on your own unconfirmed change, but that would be great, in that case no patch is needed at all.

Re: checking mempool as I asked through mail would that solve all the issues? What if another transaction variant gets mined than is in the (local) mempool?

I mean, specifically the problem is when I tell bitcoind to make a payment, I want that payment to go through. If I spend confirmed inputs the transaction making the payment might get mutated, but it will get mined eventually and the payment will happen without any further intervention. Spending unconfirmed in a mempool doesn't have that guarantee, and similarly there's the edge case of a reorganization with, say, single confirmation coins.

@laanwj When searching for coins to spend, we always consider just the confirmed ones first (and the >=6 confirmed ones before that), so preventing payments in case not enough balance exists in confirmed coins should be enough to prevent unconfirmed change from being used. @petertodd @laanwj You're right, ignore my suggestion.

@sipa But minconf already defaults to 1 on sendfrom/sendmany. And it still spends unconfirmed change.

@laanwj In that case something is going on that I'm missing.

@laanwj All minconf does on those rpcs is changes what it checks for deciding the account has enough balance, it has no influence on coin-selection.

ACK this change, it improves a minor privacy leak too (when you spend unconfirmed change it was very very likely change, and not a third party payment). It's too disruptive to turn on by default, alas.

@gmaxwell Right now the alternative is more disruptive; turn it on by default. Other wallets already work that way with user acceptance, and users can easily turn it off again once malleability becomes less of a issue.

Bleh, nobody will ever use this, defaulted off.

ACK, merge it, then let's create a new PR immediately, to discuss changing the default! :)

See #3654

@sipa > In that case something is going on that I'm missing.

I guess what's happening is that we have enough confirmed coins to make up the amount being sent, but not enough to include the required fee. So we end up using unconfirmed change.

That would be one hell of a coincidence, and there many people reporting failed transactions being created.

Maybe most of those failed transactions are created using sendtoaddress, which doesn't have a minconf argument.

Oh, then I misread @laanwj's comment. Yeah, that would explain it...