The following sections might be updated with supplementary metadata relevant to reviewers and maintainers.

Code Coverage & Benchmarks

For details see: https://corecheck.dev/bitcoin/bitcoin/pulls/34320.

Reviews

See the guideline for information on the review process.

If your review is incorrectly listed, please copy-paste <!--meta-tag:bot-skip--> into the comment that the bot should ignore.

Conflicts

Reviewers, this pull request conflicts with the following ones:

• #34132 (coins: drop error catcher, centralize fatal read handling by l0rinc)
• #34124 (validation: make CCoinsView a pure virtual interface by l0rinc)
• #32427 ((RFC) kernel: Replace leveldb-based BlockTreeDB with flat-file based store by sedited)
• #32317 (kernel: Separate UTXO set access from validation functions by sedited)

If you consider this pull request important, please also help to review the conflicting pull requests. Ideally, start with the one that should be merged first.

One tries to deserialize, the other just returns whether any value is associated with the key.
But we usually need the full value we probed for, and HaveCoin cannot cache the returned value, so it can trigger an extra lookup if HaveCoin uses Exists and the caller later needs the actual value.

Are there any flamegraphs or benchmarks, to see how this affects performance?

Are there any flamegraphs or benchmarks, to see how this affects performance?

I'm not sure what to measure exactly because the current code was already delegating to GetCoin in most places.
The pruned IBD and -reindex-chainstate and AssumeUTXO loading benchmarks I started haven't finished yet - but I don't expect any change there.
If you can describe a scenario that you would find helpful, please let me know.

Are there any flamegraphs or benchmarks, to see how this affects performance?

Added assumeutxo load, reindex-chainstate and pruned IBD measurements to the PR description

I'm not sure what to measure exactly because the current code was already delegating to GetCoin in most places.

I'm not sure about this. CCoinsViewCache::HaveCoin seems to be the implementation that is called in all non-test places outside of coins.cpp, and only delegates to GetCoin if there's a cache miss. In CCoinsViewCache::HaveCoin we avoid copying Coin:

bool CCoinsViewCache::HaveCoin(const COutPoint &outpoint) const {
    CCoinsMap::const_iterator it = FetchCoin(outpoint);
    return (it != cacheCoins.end() && !it->second.coin.IsSpent());
}

vs CCoinsViewCache::GetCoin where we return a copy of Coin:

std::optional<Coin> CCoinsViewCache::GetCoin(const COutPoint& outpoint) const
{
    if (auto it{FetchCoin(outpoint)}; it != cacheCoins.end() && !it->second.coin.IsSpent()) return it->second.coin;
    return std::nullopt;
}

we usually need the full value we probed for, and HaveCoin cannot cache the returned value, so it can trigger an extra lookup if HaveCoin uses Exists and the caller later needs the actual value.

I see 4 uses of HaveCoin

1. In ConnectBlock, we ensure we do not already have outputs of a block for BIP30 checks.
2. In ApplyTxInUndo, we ensure we do not already have the coin we are about to roll back to unspent.
3. In MemPoolAccept::PreChecks, where we check that we have all inputs of a tx we are accepting to the mempool.
4. In TxVerify, where we call HaveInputs.

For 1 and 2, we are checking for the non-existence of the coin so we won't need the full value. In this case not caching the value makes sense.
For 3, it is explicitly relying on the fact that the value is cached. The backend is thrown away after all input prevouts are added to the top level cache.
For 4, it is a peformance improvement that the value is cached.

I think 1 and 2 would benefit from CCoinsViewCache::HaveCoin delegating to the base's HaveCoin and not going through FetchCoin.
For 3 this seems like an abuse of the CCoinsView API. But, calling GetCoin instead there would result in unnecessary copying of the Coin. I'm not sure what the best solution is here.
For 4 the current behavior makes sense.

only delegates to GetCoin if there's a cache miss. In CCoinsViewCache::HaveCoin we avoid copying Coin

Disassembly with separate translation units indicates GetCoin() does a new + memcpy of the Coin and the caller immediately deletes it, even when only checking .has_value(). HaveCoin() does indeed avoid it, but the extra copy only happens on the unexpectedly-present coin path, since if it's expected, we usually want to cache it. If we want to avoid the extra copy, we can use AccessCoin, but we would duplicate the spentness check.

In this case not caching the value makes sense.

But there's nothing to cache since these will return an empty optional. For BIP30 and ApplyTxInUndo the missing-coin path dominates, so GetCoin() short-circuits on Read() failure without deserialization, so I/O is the same as Exists(), no coin-copy happens (and when it does we crash or reorg anyway). But given that validation until bip30-end takes less than 3 minutes on my benchmarking server (see #33817), I'd say it's irrelevant either way.

For 3 this seems like an abuse of the CCoinsView API. But, calling GetCoin instead there would result in unnecessary copying of the Coin. I'm not sure what the best solution is here.

This is a good point, the disassembly indicates there's no devirtualization and no inlining in CCoinsViewCache::HaveInputs and MemPoolAccept::PreChecks, so the copy is real - though a lot faster than disk access, so it's not actually measurable.
We can use AccessCoin here and with explicit IsSpent call instead which would avoid the call (according to the assembly):

if (AccessCoin(tx.vin[i].prevout).IsSpent()) {

and

if (m_view.AccessCoin(txin.prevout).IsSpent()) {

I have added a new benchmark as a first commit to check the performance of on-disk HaveInputs to make sure the performance is retained.

Benchmark baseline:

Benchmark with !GetCoin() instead of HaveCoin:

Replace remaining HaveCoin calls with GetCoin()/AccessCoin and remove the former

(ran it 5 times for each commit and copied the fastest (with smallest noise). The small difference is reproducible, but not very important as long as it's not measurably slower)

for commit in 3544b98927050956f68cb183bf2686f5c65e5ebb 8c568500442144fbcd457eccda5a1956aa98ca81; do \                                        
    git fetch origin $commit >/dev/null 2>&1 && git checkout $commit >/dev/null 2>&1 && echo "" && git log -1 --pretty='%h %s' && \
    rm -rf build >/dev/null 2>&1 && cmake -B build -DBUILD_BENCH=ON -DCMAKE_BUILD_TYPE=Release >/dev/null 2>&1 && \
    cmake --build build -j$(nproc) >/dev/null 2>&1 && \
    for _ in $(seq 5); do \
      sleep 5; \
      ./build/bin/bench_bitcoin -filter='HaveInputsOnDisk' -min-time=1000; \
    done; \
done

If we want to avoid the extra copy, we can use AccessCoin, but we would duplicate the spentness check.

This tells us that HaveCoin is the right level of abstraction then, and is a useful method, no?

In this case not caching the value makes sense.

But there's nothing to cache since these will return an empty optional

Same as above.

We can use AccessCoin here and with explicit IsSpent call instead which would avoid the call (according to the assembly)

I think everywhere we use AccessCoin we assert the unspentness. This would now break that pattern and use AccessCoin to check existence via spentness. I think HaveCoin conveys intent better here.

Can we instead just remove HaveCoin on the db then, and the default implementation would just forward to !!GetCoin for it?

Thanks @andrewtoth, I didn't like the suggestion at first since we keep polluting the interface, but after applying your suggestion I'm fine with the results, it's simpler and better separated from the other similar PRs.

🚧 At least one of the CI tasks failed.
_{Task lint: https://github.com/bitcoin/bitcoin/actions/runs/21641577498/job/62382439018
LLM reason (✨ experimental): Lint check failed: direct use of std::filesystem was flagged by the std_filesystem lint rule.}

Concept ACK

I'm a bit confused by this patch set. If the CCoinsViewDB's HaveCoin is only used in the coins tests to verify invariants (which can be checked by replacing its implementation with a throw), why is this adding a benchmark? I'm also not sure why this is also changing the implementation of Exists if it is not used by production code in the coins db path. Can't that just be left alone? Why not scope this PR to just remove the HaveCoin member function and add the unit test from the second commit?

why is this adding a benchmark?

I touched on that briefly in the commit message:

CCoinsViewCache::HaveInputs() is used by Consensus::CheckTxInputs() and currently exercises the HaveCoin() path in the UTXO cache.
Performance could be affected due to Coin copying and different cache-warming behavior.

The motivation was @andrewtoth's concern about a coin copy regression. See #34320 (comment)

It also checks the cached behavior, not just the DB query.

I'm also not sure why this is also changing the implementation of Exists

I tried to cover this in the commit message as well. Please let me know which part needs more detail:

ExistsImpl was removed since it duplicates CDBWrapper::ReadImpl.

Basically, we can remove it now since the underlying premise, trying to avoid deserialization cost, isn't warranted anymore. I could inline the Exists method now (I did so in a previous version), but this results in a smaller diff.

The motivation was @andrewtoth's concern about a coin copy regression. See #34320 (comment)

Why do these concerns apply to its removal if the coins db's HaveCoin is never called in production?

Basically, we can remove it now since the underlying premise, trying to avoid deserialization cost, isn't warranted anymore. I could inline the Exists method now (I did so in a previous version), but this results in a smaller diff.

Sure, but it would be good to mention why this cost isn't relevant to its current use in the indexes.