Rename IsConfirmed to IsTrusted to better match the intended behavior. #3662

gmaxwell commented at 12:28 AM on February 13, 2014: contributor

This doesn't change the functionality at all.

I don't really care about this, pull in case of unproductive navel gazing disrupting progress on other things.

Rename IsConfirmed to IsTrusted to better match the intended behavior.

This doesn't change the functionality at all.

0542619d93

BitcoinPullTester commented at 12:48 AM on February 13, 2014: none

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

in src/wallet.cpp:None in 0542619d93

 979 | @@ -980,7 +980,7 @@ int64_t CWallet::GetUnconfirmedBalance() const
 980 |          for (map<uint256, CWalletTx>::const_iterator it = mapWallet.begin(); it != mapWallet.end(); ++it)
 981 |          {
 982 |              const CWalletTx* pcoin = &(*it).second;
 983 | -            if (!IsFinalTx(*pcoin) || !pcoin->IsConfirmed())
 984 | +            if (!IsFinalTx(*pcoin) || !pcoin->IsTrusted())

laanwj commented at 8:52 AM on February 13, 2014:

IsTrusted already checks !IsFinalTx(*pcoin)! :p (sorry)

ACK, I agree with the rename, trusted is an improvement in naming here although it is very general.

jgarzik commented at 2:29 PM on February 13, 2014: contributor

ACK

b6393ce9-d324-4fe1-996b-acf82dbc3d53 commented at 5:34 PM on February 13, 2014: none

NACK

It's becoming clear that the Bitcoin world is actually long overdue for resuming some serious "navel gazing".

Again (updated for this proposal):

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 "trusted" 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 think your CWalletTx::IsTrusted() is a lie, and it certainly doesn't take care of the fact that the confirmed balance will still be including unconfirmed change. These issues need to be handled altogether differently.

and:

sipa: 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.

laanwj commented at 5:55 PM on February 13, 2014: member

@ weird uuid person: How is that a NACK on this rename? I get it that you don't agree with trusting your own outputs in the first place, and want to get rid of this function, but that's what -nospendzeroconfchange is for now. This is just a rename and Trusted makes a lot more sense than Confirmed in context what the function does.

sneak commented at 6:27 PM on February 13, 2014: none

Perhaps "LocallyTrusted" may satisfy b6393ce9's semantic issue with the bare term "Trusted"? I agree with him that it's somewhat misleading, but it needs to be called something and the string isn't user-facing - it would only affect those reading the code.

laanwj commented at 7:12 PM on February 13, 2014: member

You know what, I'm just going to merge this, there are way more important things to bikeshed over.

laanwj referenced this in commit 6056c87d25 on Feb 13, 2014

laanwj merged this on Feb 13, 2014

laanwj closed this on Feb 13, 2014

ghost commented at 7:13 PM on February 13, 2014: none

@laanwj ACK

b6393ce9-d324-4fe1-996b-acf82dbc3d53 commented at 9:08 PM on February 13, 2014: none

You're not understanding me.

YOU CANNOT EVER TRUST A TRANSACTION THAT'S NOT IN THE BLOCKCHAIN.

The function itself—not just the name—is wrongheaded.

gmaxwell commented at 9:12 PM on February 13, 2014: contributor

Of course you can, even with malleability in mind you might "just" have to reissue the transaction to spend the mutant instead. It's unlike an unconfirmed third party payment in which the funds may never be yours.

We already only spend unconfirmed coins as a last resort when the transaction would otherwise fail (and only if that hasn't been disabled).

b6393ce9-d324-4fe1-996b-acf82dbc3d53 commented at 9:17 PM on February 13, 2014: none

In the face of current transaction mutability, spending unconfirmed transaction outputs should probably be considered an advanced usage not immediately available to casual users (e.g., GUI users); for other users, it should be labeled caveat emptor.

sidhujag referenced this in commit 3dcc650717 on Sep 21, 2020

DrahtBot locked this on Sep 8, 2021