Fix getbalance handling of spent, unconfirmed change #3661

First: txnmall.sh is the start of an integration test for wallet handling of mutated (malleable) transaction handling.

Second is a one-line change to IsConfirmed that adds a condition when considering change outputs : only consider them confirmed if they are in the memory pool (inspired by a comment sipa made yesterday).

This makes getbalance, getbalance '*', and listunspent all agree when run on the txnmall.sh test wallets. Before this fix, those three would return three different results.

Before: listunspent: one 49BTC output getbalance: 96 BTC (change counted twice) getbalance '*': 46 BTC (spends counted twice)

After: all agree, 49 BTC available to spend.

Can you fix all of the bash nits in qa/rpc-tests/wallet.sh, test with directories with spaces/etc. and submit a pull request for that? I based the txnmall.sh code on wallet.sh, and, frankly, making it work if you've got a wacky directory hierarchy is not high on my TODO list.

Sorry, I didn't realize line comments would become so obtrusive; I'll remove them presently.

That being said, there is a real danger here, particularly with your EXIT trap.

Agree with @gavinandresen, perfecting the hacky test script is not the first priority. Feel free to do this in a separate pull though!

The focus of testing and review should be the change in wallet.h here.

Edit: @b6393ce9-d324-4fe1-996b-acf82dbc3d53 the rm -rf in the EXIT trap can indeed lead to problems if you have a datadir with a space in it, it's could be dangerous if it results in a recursive remove of the wrong directory. Putting "$D" there would be wise precaution :)

I'm uncomfortable with considering as "confirmed" what we are all already acknowledging as "unconfirmed" transactions.

What the Bitcoin world has come to understand recently is that even a transaction created by a trusted party—namely by yourself—is not necessarily ever going to get into the blockchain; thus, it's fairly risky to base any calculation on any transaction that is not already buried in the blockchain.

More to the point of this commit, though: Your "confirmed" balance is meaningless unless you can spend that value, and yet that value cannot be spent unless the Bitcoin network—yes, the third-party payment processor—says you can spend it; your personal, individual client really has very little say in the matter, a point that is compounded by mutable (malleable) transactions.

So, I don't think either CWalletTx::IsConfirmed() or the balance of confirmed value is the proper place to be accounting for unconfirmed change. These issues need to be handled altogether differently.

That's a confusion that afaik as existed forever in the source code: IsConfirmed should be called IsTrusted or something instead (it means confirmed OR from ourselves).

But the larger point is that a transaction from oneself cannot actually be trusted; due to transaction mutability, any source other than the blockchain itself is irrelevant (for various degrees of risk aversion). So, this is not just a question of naming; it's a question of real risk.

When you think about it, you cannot be terribly miffed about this larger point, because that's all Bitcoin ever proffered, anyway: "Data buried in the blockchain is what can be trusted." It's better to work with that reality rather than against it.

Automatic sanity-testing: PASSED, see http://jenkins.bluematt.me/pull-tester/efd8eb3f371369914a84d489ec86873cbc327285 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.

Replaced with a deeper level fix (make GetDepthInMainChain() return -1 for transactions that are unconfirmed and no longer in the memory pool).