Change default for -spendzeroconfchange to 0

[laanwj]

Changing the default to 0 is disruptive, but on the other hand with the current flurry of malleability abuse it is the safer option.

Edit: I do wonder, if this is to be the default do we want to change the output of getbalance as well to make sure it matches what can actually be spent? Currently it still shows unconfirmed change either way. On the other hand it is unintuitive to have the entire input disappear from the balance if you send. I think we do need some getbalance changes but I'm not entirely sure what...

[petertodd]

Strong ACK. From the user's point of view the attack can make coins vanish; much better to just temporarily annoy people until a better fix is implemented.

If the getbalance change is simple, I'd say go for it too.

[sipa]

Not couning unconfirmed change in getbalance will result in users complaining "I had 15000 BTC in my wallet, I sent 0.001 to a friend, and now all of it is GONE!!!".

[ghost]

It's worth documenting it somewhere though.

[BitcoinPullTester]

Automatic sanity-testing: FAILED BUILD/TEST, see http://jenkins.bluematt.me/pull-tester/e7d1e1eb4e8b584adfdc44bac6f52938e134b8a4 for binaries and test log.

This could happen for one of several reasons:

1. It chanages paths in makefile.linux-mingw or otherwise changes build scripts in a way that made them incompatible with the automated testing scripts (please tweak those patches in qa/pull-tester)
2. It adds/modifies tests which test network rules (thanks for doing that), which conflicts with a patch applied at test time
3. It does not build on either Linux i386 or Win32 (via MinGW cross compile)
4. The test suite fails on either Linux i386 or Win32
5. The block test-cases failed (lookup the first bNN identifier which failed in https://github.com/TheBlueMatt/test-scripts/blob/master/FullBlockTestGenerator.java)

If you believe this to be in error, please ping BlueMatt on freenode or TheBlueMatt here.

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.

[petertodd]

@sipa Excellent point.

[sipa]

On the other hand, if you DO count unconfirmed change, I'm afraid balances may be off in the other direction (too high), if you get a mutated transactions in your wallet. I'm not sure about this though - balance is computed in multiple ways.

[laanwj]

@sipa Agreed. I think if the wallet is fixed to remove/hide duplicated (due to malleability) unconfirmed transactions, the balance will be fixed too, and we don't need to do anything else.

[b6393ce9-d324-4fe1-996b-acf82dbc3d53]

You should never try to hide reality.

You're over-engineering. Just show the user both the confirmed and unconfirmed balances, particularly when they differ (and particularly when unconfirmed change has been set to be treated as truly unconfirmed).

For starters, CWalletTx::IsConfirmed() needs to be updated to reflect reality; see this pull request: #3657

[jspilman]

@sipa if balances are too high when counting unconfirmed change, then balance is being calculated wrong. Two conflicting unconfirmed transactions cannot both contribute to getbalance. If there are multiple conflicting futures, it can't sum all of them together, it has to choose one. But this is out of scope anyway. If there are merely mutated same-spend transactions in the mempool, the worst case should be the mutated tx hits the blockchain, and it breaks a spend-chain which would then cause getbalance to jump UP.

[laanwj]

This is causing a pull tester failure because default wallet behavior changed ("unconfirmed balance").

[sipa]

@jspilman I'm very aware that something is wrong. Fixing it requires a lot more work though.

[laanwj]

I don't want this as default.

(Spending unconfirmed change gives a larger chance of never-confirming transactions, but it sure makes the wallet a lot more user friendly in the goodweather case. With dead transaction handling the duplicates are handled better and having this as default is not needed)