If requested, actually treat uncomfirmed change as being uncomfirmed #3657

This commit strengthens 1bbca249b202c4802cc2c4d4de4a26e6392b4d92 by updating the CWalletTx::IsConfirmed() function.

If (bSpendZeroConfChange==false), then IsConfirmed() should actually treat unconfirmed change as being unconfirmed.

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

As per the discussion here, there is the question as to how calculating balances should be handled; this commit alters how balances are calculated.

Sipa pointed out:

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!!!".

and:

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.

to which laanwj responded:

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.

Regardless of how altered transactions should be handled, I think it's important never to try to hide reality.

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

Right - RPC has a 'getunconfirmedbalance' call since 0.9, and the GUI has shown the unconfirmed balance for a while.

This change would make unconfirmed change part of the unconfirmed balance (when -nospendzeroconfchange). It is a bit strange to people that are used to the old way ("why is such a large part of my wallet suddenly unconfirmed").

But you have a point that this at least makes getbalance show how much can really be sent, which is good for consistency (and has always been the definition of getbalance).

I didn't like this when I first looked at it, but I'm warming up to it. I think what the regular balance display is showing is, indeed, what you can spend.

Was there a reason we didn't promote the unconfirmed balance to the getinfo output?

@gmaxwell Because Gavin was afraid of the things people would do with unconfirmed balances. See #3369. Also adding another (expensive) balance computation in there would have performance drawbacks. It's already a grabbag of random info.

But I agree after this change it makes sense to show unconfirmed balance there.

So, I replicated the mallability problem in regtest:

Balance looks like this without this change (and -nospendzeroconfchange):

Balance looks like this with this change (and -nospendzeroconfchange):

If you compute the balance manually and ignore mutated duplicate it should be: 4799.80 mBTC

So it's a bit saner, but still no dice I suppose.

Edit: after another 123 mBTC send [which is still unconfirmed]

Without this change (and with -nospendzeroconfchange): With this change (and with -nospendzeroconfchange):

This makes sense. There is indeed nothing to be spent after this, as the one input has been used, until another block has been mined.

Your test case actually shows that this patch is working as intended: Confirmed transactions are accounted for properly, but unconfirmed transactions are a total crapshoot; indeed, this outcome is all that Bitcoin has ever guaranteed.

Fixing the computation of the sum `Total' can—and should—be left to another patch.

In particular, I think it's important to distinguish between "confirmed" and "spendable". Hitherto, the reference client has conflated these two notions; this _hiding of reality_ is now coming back to bite us hard.

Regardless of whether a user wishes to treat unconfirmed change as spendable, it should never be treated as confirmed because… well… it's not confirmed.

@b6393ce9-d324-4fe1-996b-acf82dbc3d53 I strongly disagree with your complaints about "hiding reality", but until now I've refrained from responding because correcting your philosophy is not critical to improving things. But since you keep repeating it... Part of the entire purpose of the software's front end is to make the software usable to humans by taking care of and reducing the visibility of complicated details which would over load the user. It is essential that the software abstract away all sorts of details in order to make it comprehensible at all. A totally non-hidden interface would be something like listunspent and wouldn't even present a balance— a balance is a fiction, coins are not really fungible—. In any case, you run into problems when you've chosen the wrong abstractions or provided an incomplete set of them.

What the balance is showing is coins the software believes it can safely spend. It is "intuitively obvious" that your own change is safe to spent, except the intuition is made incorrect by the fact that malleability can invalidate the children of a mutated transaction. The software does, however, only spend them as a last resort when the only other option would be to reject the transaction.

Abstraction is not about hiding reality (I should say "ignoring reality"); abstraction is about constraining one's view of reality in order to avoid distracting details—the result of abstraction is still consistent with reality.

What we have here is not an abstraction; the results are not consistent with reality, as proved by this entire, extremely costly debacle.

@b6393ce9-d324-4fe1-996b-acf82dbc3d53 Abstraction can be completely at odds with reality for the sake of usability and simplicity. For this reason the client pretends that change addresses do not exist, where if they were following the internal workings of the client they’d be presented on the receiving list as any other. This behavior in no way follows reality for the sake of users not needing to know every internal facet of transactions before using the client.

Unfortunately this particular wallet abstraction frequently breaks down when users compare their wallet display to that of a block explorer like blockchain.info, compounded by their recent decision to occasionally hide the output they believe is "change".

• An abstraction is never at odds with reality.

  An attempt that is at odds with reality is merely a mistake.

• I am not disputing the example you are trying to describe; you are fabricating a straw man.

WALLET is a lousy abstraction in this instance. There's no bitcoin in it. It is a KEYRING. This hindered my comprehension of the entire system for longer than I care to admit.

@dgenr8 Wallet makes a lot more sense in the confines of the Bitcoin software being used as a means of storage and spending of monetary value. If you don't use terms that a user finds familiar, there's nothing to base the interaction metaphor on. To that end we use "wallet" as it's a concept the user finds familiar, you put coins in your wallet and that makes a reasonable amount of sense without explanation. The term "keyring" makes sense with something like GPG/PGP where the user is actively managing private and public keys themselves, but Bitcoin operates a layer above this where we attempt to completely hide the fact that public/private keys even exist to the non-technical user.

The fact that we have people messing with importing and exporting keypairs shows that we just haven't enforced the wallet abstraction enough. People using blockchain.info as a source of information is extremely unfortunate in this regard, I would personally have preferred much of this to have stayed hidden as much as possible from the eyes of the general user. Blockchain.info unfortunately enforces the notion that "address" == "wallet", which is incorrect and undesirable in many ways.

This is of course completely off topic as far as this PR goes.

Also at a purely technical level, it is a misunderstanding that a Bitcoin wallet is just a keyring. The wallet also contains the knowledge of transactions that provide the outputs it can spend. Without that knowledge you cannot send money.

A wallet contains both the coins, and the keys to prove that you own them. Despite that, the network will still prevent the same coin from being spent twice.

@sipa I think that's somewhat due to the way people develop "wallets", in that they don't consider discovering unspent outputs a part of the process. The vast majority seem to offload the task to blockchain.info's API rather than managing themselves. The rest is just users being given simplistic explanations that they spread without the knowledge that they are limited.

KEY as an abstraction would make some things easier to understand.

People understand that keys can be copied without duplicating the contents of that which they unlock. This would explain why wallets can be backed up.

People understand that a key exists in a different location than that which it unlocks. This would explain why the network must be consulted to determine what is spendable, as per this PR.

Wallet = KEYRING Privkey = VAULT BOX KEY Address / pubkey = VAULT BOX Collection of all addresses = VAULT Bitcoin network = BANK

Bitcoin = MONEY

@dgenr8 Only it doesn't work like that. People expect funds to remain at an address after they have been sent there, which is where your analogy begins to break down. Why when a user makes a transaction does all or some of the funds in a vault disappear and move to one that's completely unrelated? If we are to avoid explaining inputs and change addresses to people, we've got to be higher leveled than that. Presently we have a "wallet" which stores funds, which has an unlimited number of "receiving addresses" (note, never "sending address", they don't exist), and that's pretty much it.

With Bitcoin don't need to have levels of abstraction for the whole network's infinitesimal details. Private keys, keypairs, change addresses; it's just information overload for all of these to be visible when a typical user has absolutely no need to know that they exist. We certainly shouldn't be encouraging users to ever come in contact with private keys, so it's best to just avoid them in the interface layer entirely. I believe @sipa (or somebody else) made a comment late last year that the importprivkey command should be entirely removed with a similar reasoning.

It's more difficult to explain blocks and confirmations without dipping into the technical side of things, but we must be able to in order to have semi-coherent, semi-usable software outside of those who have developed with it or who want to have a detailed understanding.

Channeling @b6393ce9-d324-4fe1-996b-acf82dbc3d53 here. Tell them the truth. Tell them that the bank's transaction slips don't have a spot to fill in for how much to take out when spending a vault box's contents. You have to empty the whole thing, so any change goes to another box. Your wallet software actually makes a change box for you, amazingly, without any cooperation from the bank, until the bank processes the transaction. The vault boxes also have deposit slots, so anyone can add to their balance without the key.

I just think maybe this isn't complex enough to justify telling users something else. Something false.

This is getting out of hand... Let's not try to write a philosophy book here.

I do think we should merge this, as it makes getbalance with -nospendzeroconf say how much can really be spent now. To have it otherwise is confusing.

As -nospendzeroconf is not enabled by default people know what they're getting into when they enable it.

After the fact ACK.