doc: Coin::IsSpent() can also mean never existed #18030

pull Sjors wants to merge 1 commits into bitcoin:master from Sjors:2020/01/doc_is_spent changing 1 files +3 −0

Sjors commented at 6:31 PM on January 30, 2020: member

This can be especially confusing where AccessCoin() is used with logic like this:

    while (iter.n < MAX_OUTPUTS_PER_BLOCK) {
        const Coin& alternate = view.AccessCoin(iter);
        if (!alternate.IsSpent()) return alternate;

Sjors requested review from jamesob on Jan 30, 2020
Sjors requested review from jnewbery on Jan 30, 2020
Sjors commented at 6:32 PM on January 30, 2020: member

Alternatively we could remove the use of static const Coin coinEmpty and disable the empty Coin constructor.
DrahtBot added the label Docs on Jan 30, 2020
DrahtBot added the label UTXO Db and Indexes on Jan 30, 2020
jnewbery commented at 3:47 PM on January 31, 2020: member

I think I prefer the second option (remove the use of static const Coin coinEmpty). I think the only time that we can get a never-existing coin from the cache is with AccessCoin(). We could change that to return a const pointer instead of a const reference and return nullptr if the coin doesn't exist. That's what AccessCoins() did before we changed to per-txout coinsdb.

Perhaps @sipa had a reason for introducing the coinEmpty that I don't understand though?
promag commented at 12:41 AM on February 3, 2020: member

How about rename to IsNull()?
Sjors force-pushed on Jun 10, 2020
fjahr commented at 2:13 PM on June 10, 2020: member

How about rename to IsNull()?

I think this is a more accurate name and just renaming is the smaller change, so I think this could be a good middle-ground solution. The downside of this name specifically is that this makes it harder to grep for this function. Maybe IsSpentOrEmpty() would be even a little bit better.
flack commented at 2:16 PM on June 10, 2020: contributor

or maybe turn it around and call the function isSpendable or something? Because it's kind of intuitive that a coin that doesn't exist can't be spent
[doc] Coin: explain that IsSpent() can also mean never existed 1404c57403

in src/coins.h:76 in c388b57a96 outdated

  72 | @@ -73,6 +73,8 @@ class Coin
  73 |          ::Unserialize(s, Using<TxOutCompression>(out));
  74 |      }
  75 |  
  76 | +    // Either this coin never existed (see e.g. coinEmpty in coins.cpp), or it

laanwj commented at 2:57 PM on June 10, 2020:

Please use doxygen function description syntax: /** ... */

Sjors commented at 3:47 PM on June 10, 2020:

Done

Sjors force-pushed on Jun 10, 2020
practicalswift commented at 9:59 PM on March 20, 2021: contributor

ACK 1404c574037da331c233317618b9b90d3329ed33
MarcoFalke commented at 7:18 AM on March 21, 2021: member

I think I prefer the second option (remove the use of static const Coin coinEmpty). I think the only time that we can get a never-existing coin from the cache is with AccessCoin(). We could change that to return a const pointer instead of a const reference and return nullptr if the coin doesn't exist. That's what AccessCoins() did before we changed to per-txout coinsdb.

Perhaps @sipa had a reason for introducing the coinEmpty that I don't understand though?

Wouldn't std::optional make more sense? See also #20833 (review)
MarcoFalke commented at 7:18 AM on March 21, 2021: member

ACK 1404c574037da331c233317618b9b90d3329ed33
jnewbery commented at 7:55 PM on March 21, 2021: member

utACK 1404c574037da331c233317618b9b90d3329ed33

It's a much bigger change, but how about replacing IsSpent() with the bool operator, and using that everywhere? If we're using a Coin in the null state to indicate "no UTXO", then it seems natural to define truthiness on the Coin object where false indicates no UTXO and true indicates UTXO.

Even after this PR, there are still places where this is confusing, eg:

https://github.com/bitcoin/bitcoin/blob/d2a78ee9288e4d3bace9125bcfae6b7747f85982/src/node/transaction.cpp#L47-L50

Better to update the interface to be intuitive than keep the outdated name and add comments about why it's confusing.
JeremyRubin commented at 3:14 AM on March 22, 2021: contributor

I think an optional API sounds nice, not a huge fan of making coin itself a bool operator since I don't think there's a straightforward "truthiness" that a coin would have in all contexts.
sipa commented at 3:33 AM on March 22, 2021: member

If you're going to consider an std::optional based interface, please benchmark it very well - this code is very performance critical.

I don't think there is a need; Coin has a well-defined "empty" state on its own (the current IsSpent; a name that's indeed outdated and confusing), so I don't think there is a need for a way to signal failure separately.

since I don't think there's a straightforward "truthiness" that a coin would have in all contexts.

What other thruthiness do you imagine besides the "exists and is isn't spent" there is now?
MarcoFalke merged this on Mar 23, 2021
MarcoFalke closed this on Mar 23, 2021
sidhujag referenced this in commit afe7a39067 on Mar 23, 2021
Sjors deleted the branch on Mar 24, 2021
PastaPastaPasta referenced this in commit 4a1a48377b on Jun 27, 2021
PastaPastaPasta referenced this in commit 04aa697c42 on Jun 28, 2021
PastaPastaPasta referenced this in commit 06a95fbb34 on Jun 29, 2021
PastaPastaPasta referenced this in commit c7190429df on Jul 1, 2021
PastaPastaPasta referenced this in commit 75d5e8246a on Jul 1, 2021
PastaPastaPasta referenced this in commit 8afb2293e8 on Jul 15, 2021
PastaPastaPasta referenced this in commit d82fdbfafe on Jul 15, 2021
PastaPastaPasta referenced this in commit cfd9a89298 on Jul 16, 2021
gabriel-bjg referenced this in commit 0f458dd60d on Jul 16, 2021
DrahtBot locked this on Aug 16, 2022

Contributors

Review Requested
jamesob jnewbery

Labels

Docs UTXO Db and Indexes