coins: make cursor iteration DB-only #35572

l0rinc commented at 6:41 AM on June 21, 2026: contributor

Problem: CCoinsView::Cursor() makes cursor iteration look like a generic coins view operation, but cursor iteration is only supported by the DB-backed coins view. The cache override only threw, and the coins_view fuzz target only asserted that deterministic unsupported throw path.

Fix: Make cursor iteration a CCoinsViewDB operation. Cursor users now take the DB-backed view directly, CCoinsView no longer exposes Cursor(), and the fuzz target keeps DB-backed cursor coverage while dropping the unsupported cache throw probe. The UTXO stats path is also tightened to pass the non-null DB view by reference, and stale null handling for DB cursors is removed.

This was extracted from review discussion in #35295 (review) and extended based on #35562 (comment).

DrahtBot added the label UTXO Db and Indexes on Jun 21, 2026

DrahtBot commented at 6:41 AM on June 21, 2026: contributor

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/35572.

Reviews

See the guideline and AI policy for information on the review process.

Type	Reviewers
ACK	sedited, w0xlt

If your review is incorrectly listed, please copy-paste <code></code> into the comment that the bot should ignore.

Conflicts

Reviewers, this pull request conflicts with the following ones:

#35474 (node: move index ownership to NodeContext by w0xlt)

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.

in src/validation.cpp:5896 in 5df2bbf543

5892 | @@ -5893,7 +5893,7 @@ util::Result<void> ChainstateManager::PopulateAndValidateSnapshot(
5893 |  
5894 |      try {
5895 |          maybe_stats = ComputeUTXOStats(
5896 | -            CoinStatsHashType::HASH_SERIALIZED, snapshot_coinsdb, m_blockman, [&interrupt = m_interrupt] { SnapshotUTXOHashBreakpoint(interrupt); });
5897 | +            CoinStatsHashType::HASH_SERIALIZED, *snapshot_coinsdb, m_blockman, [&interrupt = m_interrupt] { SnapshotUTXOHashBreakpoint(interrupt); });

sedited commented at 12:50 PM on June 21, 2026:

Nit: Maybe rather change the snapshot_coinsdb to a reference?

l0rinc commented at 2:27 PM on June 21, 2026:

Indeed, pushed (+ found one additional case), thanks!

sedited approved

sedited commented at 1:08 PM on June 21, 2026: contributor

ACK 34620b0490a1a6be681afbbdc89ba037d6143739

This might be a nice use case for the NotNull<>.

l0rinc force-pushed on Jun 21, 2026

in src/rpc/blockchain.cpp:1087 in 66a1420343 outdated

1089 | -        LOCK(::cs_main);
1090 | -        coins_view = &active_chainstate.CoinsDB();
1091 | -        blockman = &active_chainstate.m_blockman;
1092 | -    }
1093 | +    const CCoinsViewDB& coins_view{WITH_LOCK(::cs_main, return active_chainstate.CoinsDB())};
1094 | +    BlockManager& blockman{active_chainstate.m_blockman};

sedited commented at 3:52 PM on June 21, 2026:

Nit: I think the correctness of the lock scope change would be even clearer if the blockman is retrieved from the chainman.

l0rinc commented at 4:02 PM on June 21, 2026:

If you don't mind I'd like to do that in a follow-up - current changes are all trivial, that needs a bit more thought

w0xlt commented at 9:20 PM on July 8, 2026:

Maybe BlockManager& blockman{chainman.m_blockman}; ?

sedited approved

sedited commented at 4:00 PM on June 21, 2026: contributor

Re-ACK 66a14203439e85136de703f2f2ab282aae102549

DrahtBot added the label Needs rebase on Jun 24, 2026

l0rinc force-pushed on Jun 24, 2026

coins: pass DB view to cursor users

Cursor iteration is only supported by the coins database view.

Pass `CCoinsViewDB` pointers to UTXO stats code and use the concrete DB pointer in the coins_view fuzz target before removing the abstract hook.

Co-authored-by: sedited <seb.kung@gmail.com>

c6fbe2f66c

coins: drop cursor from base view

`CCoinsView` does not need to expose cursor iteration now that the few cursor users take `CCoinsViewDB` directly.
Keep `Cursor()` on `CCoinsViewDB`, where iteration is supported, and remove the empty, forwarding, and throwing base-view overrides.

This also removes the coins_view fuzz target probe that only asserted the unsupported `CCoinsViewCache::Cursor()` throw path.

Co-authored-by: sedited <seb.kung@gmail.com>

35aedb2823

coins: pass UTXO stats view by reference

The UTXO stats helpers require a non-null coins DB view after cursor users were narrowed to `CCoinsViewDB`.

Use references for the DB view in the UTXO stats helpers and for local DB-view/block-manager variables at call sites, so the path no longer spells a nullable contract.

Co-authored-by: sedited <seb.kung@gmail.com>

3d2f2d8de0

coins: drop stale cursor null checks

`CCoinsViewDB::Cursor()` now owns cursor iteration and constructs the cursor directly.

Drop the leftover handling for the old generic nullable cursor contract from DB cursor call sites.

Co-authored-by: sedited <seb.kung@gmail.com>

72db4accbf

l0rinc force-pushed on Jun 24, 2026

DrahtBot added the label CI failed on Jun 24, 2026

DrahtBot commented at 9:31 PM on June 24, 2026: contributor

🚧 At least one of the CI tasks failed. <sub>Task tidy: https://github.com/bitcoin/bitcoin/actions/runs/28130662519/job/83305756869</sub> <sub>LLM reason (✨ experimental): CI failed during compilation of the fuzz target (test_fuzz): coins_view.cpp references an undeclared backend_coins_db (and calls Cursor() on CCoinsView), causing clang errors.</sub>

<details><summary>Hints</summary>

Try to run the tests locally, according to the documentation. However, a CI failure may still happen due to a number of reasons, for example:

Possibly due to a silent merge conflict (the changes in this pull request being incompatible with the current code in the target branch). If so, make sure to rebase on the latest commit of the target branch.
A sanitizer issue, which can only be found by compiling with the sanitizer and running the affected test.
An intermittent issue.

Leave a comment here, if you need help tracking down a confusing failure.

</details>

DrahtBot removed the label CI failed on Jun 24, 2026

DrahtBot removed the label Needs rebase on Jun 24, 2026

sedited approved

sedited commented at 4:31 PM on June 29, 2026: contributor

Re-ACK 72db4accbf5bae74d061db1aeee9e0688c0da2f5

andrewtoth commented at 7:39 PM on July 4, 2026: contributor

Why not have CCoinsViewCache just delegate to base->Cursor() instead of throwing? That way anyone who has a view to the coins can iterate through all of them if they wish?

l0rinc commented at 6:10 AM on July 5, 2026: contributor

Why not have CCoinsViewCache just delegate to base->Cursor() instead of throwing? That way anyone who has a view to the coins can iterate through all of them if they wish?

DB-only iteration seems cleaner to me. A delegated CCoinsViewCache::Cursor() would only be correct after flushing; before that it would ignore dirty state and be inconsistent with GetCoin()/HaveCoin(). Adding a separate in-memory cursor seems more confusing than useful. What do you think?

andrewtoth commented at 3:31 PM on July 6, 2026: contributor

DB-only iteration seems cleaner to me. A delegated CCoinsViewCache::Cursor() would only be correct after flushing; before that it would ignore dirty state and be inconsistent with GetCoin()/HaveCoin(). Adding a separate in-memory cursor seems more confusing than useful. What do you think?

Good call on flushing, that makes sense to me. So, a CCoinsViewCache::Cursor() would call Flush() before returning base->Cursor(). I think that's a much smaller diff. We would need to implement Cursor() to return an empty iterator to CoinsViewEmpty I think, but otherwise why not?

sedited commented at 3:36 PM on July 6, 2026: contributor

Good call on flushing, that makes sense to me. So, a CCoinsViewCache::Cursor() would call Flush() before returning base->Cursor(). I think that's a much smaller diff. We would need to implement Cursor() to return an empty iterator to CoinsViewEmpty I think, but otherwise why not?

It is my preference to go the way of using as little virtualization as possible. Especially with a layered type like this one, I don't see what the benefits are. Looking at the diff here, the code seems easier to understand to me, for example it takes fewer jumps to arrive at the conclusion that the cursor provides a stable view.

l0rinc commented at 3:39 PM on July 6, 2026: contributor

It is my preference to go the way of using as little virtualization as possible

Yes, having a cursor on anything else than the db seems confusing to me, too.

andrewtoth commented at 3:46 PM on July 6, 2026: contributor

Hmm but what about the other way around - bypassing the CCoinsViewCache to go directly to the db will ignore all dirty entries in the iterator.

l0rinc commented at 8:32 PM on July 6, 2026: contributor

You mean keeping Cursor() on the general coins view interface, with CCoinsViewCache::Cursor() flushing first and then delegating to the base cursor? I'm not sure that's the direction I want this to go in. A method named Cursor() looks like a read-only iterator factory, so having it implicitly flush dirty cache state to disk would be counter-intuitive to me. I'd also prefer to keep cursor iteration on the LevelDB-backed view only, instead of exposing it on the in-memory cache view. The current call sites make that boundary explicit: flush first, then iterate a stable DB snapshot.

w0xlt commented at 9:21 PM on July 8, 2026: contributor

ACK 72db4accbf5bae74d061db1aeee9e0688c0da2f5 The DB-only cursor boundary looks clearer and removes the unsupported cache/base cursor API.