validation: do not wipe utxo cache for stats/scans/snapshots

l0rinc commented at 2:46 pm on October 22, 2025: contributor

Revival of #30610 (comment) with the remaining comments applied on top

Since #28280, the cost of a non-wiping sync of the UTXO cache is only proportional to the number of dirty entries, rather than proportional to the size of the entire cache. Because of that, there is no reason to perform a wiping flush in case the contents of the cache is still useful.

Split the FlushStateMode::ALWAYS mode into a FORCE_SYNC (non-wiping) and a FORCE_FLUSH (wiping), and then use the former in scantxoutset, gettxoutsetinfo, and in the currently-unused CreateUTXOSnapshot.

DrahtBot added the label Validation on Oct 22, 2025

DrahtBot commented at 2:46 pm on October 22, 2025: 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/33680.

Reviews

See the guideline for information on the review process.

Type	Reviewers
ACK	optout21, cedwies

If your review is incorrectly listed, please react with 👎 to this comment and the bot will ignore it on the next update.

Conflicts

Reviewers, this pull request conflicts with the following ones:

#33866 (refactor: Let CCoinsViewCache::BatchWrite return void by TheCharlatan)
#31382 (kernel: Flush in ChainstateManager destructor by TheCharlatan)
#30342 (kernel, logging: Pass Logger instances to kernel objects by ryanofsky)
#30214 (refactor: Improve assumeutxo state representation by ryanofsky)
#29700 (kernel, refactor: return error status on all fatal errors by ryanofsky)
#29641 (scripted-diff: Use LogInfo over LogPrintf [WIP, NOMERGE, DRAFT] by maflcko)

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.

l0rinc force-pushed on Oct 22, 2025

in src/test/fuzz/utxo_snapshot.cpp:60 in b92ee75510

56@@ -57,7 +57,7 @@ void sanity_check_snapshot()
57     // Connect the chain to the tmp chainman and sanity check the chainparams snapshot values.
58     LOCK(cs_main);
59     auto& cs{node.chainman->ActiveChainstate()};
60-    cs.ForceFlushStateToDisk();
61+    cs.ForceFlushStateToDisk(/*wipe_cache=*/true);

cedwies commented at 9:28 am on October 28, 2025:

Why are we wiping the cache here? We only compute stats from CoinsDB(), not the in-memory cache, so a non-wiping flush (wipe_cache=false) should be enough for consistency (?)

l0rinc commented at 9:25 pm on November 1, 2025:

Good point, changed to false, added you as coauthor, thanks.

in src/validation.cpp:2932 in b92ee75510 outdated

2913+void Chainstate::ForceFlushStateToDisk(bool wipe_cache)
2914 {
2915     BlockValidationState state;
2916-    if (!this->FlushStateToDisk(state, FlushStateMode::ALWAYS)) {
2917+    if (!this->FlushStateToDisk(state, wipe_cache ? FlushStateMode::FORCE_FLUSH : FlushStateMode::FORCE_SYNC)) {
2918         LogPrintf("%s: failed to flush state (%s)\n", __func__, state.ToString());

cedwies commented at 10:21 am on October 28, 2025:

Isn’t LogPrintf deprecated? Could we replace this with LogError or LogWarning? Maybe also including the flush mode can be helpful.

l0rinc commented at 9:25 pm on November 1, 2025:

We could, but I’m not touching that line, it’s complicated enough as it is.

cedwies commented at 11:03 am on October 28, 2025: none

Approach ACK

Do ForceFlushToDisk(...)/FlushToDisk(...) require cs_main to be held? Most call sites hold it during shutdown flushes, but the function itself doesn’t assert or document that (no AssertLockHeld(cs_main)). If this is correct, I think a short comment can help that a Lock is not strictly needed.

optout21 commented at 5:52 am on November 1, 2025: none

I suggest splitting into two commits:

Preparatory rename & new parameter(s) but without behavior change, to minimize diff in 2nd commit (rename ALWAYS to FORCE_FLUSH I suppose)
Change in behavior in the respective places (scantxoutset, gettxoutsetinfo, etc.)

l0rinc force-pushed on Nov 1, 2025

l0rinc commented at 9:26 pm on November 1, 2025: contributor

Thanks @optout21 & @cedwies, split the change to 2 commits, one just renames the old value and adjusts the docs and scripts, the second introduces the new value. I’m not sure about the locking, let’s investigate that as a follow-up.

in contrib/tracing/README.md:250 in a59aacefd1 outdated

245@@ -246,10 +246,10 @@ $ python3 contrib/tracing/log_utxocache_flush.py $(pidof bitcoind)
246 
247 ```
248 Logging utxocache flushes. Ctrl-C to end...
249-Duration (µs)   Mode       Coins Count     Memory Usage    Prune
250-730451          IF_NEEDED  22990           3323.54 kB      True
251-637657          ALWAYS     122320          17124.80 kB     False
252-81349           ALWAYS     0               1383.49 kB      False
253+Duration (µs)   Mode         Coins Count     Memory Usage    Flush for Prune
254+2556340         IF_NEEDED    2899141         394844.34 kB    False

optout21 commented at 10:50 am on November 3, 2025:

The ‘Flush for Prune’ has changed here to False, and now all lines have False value here. Can the False line be preserved, or is it OK in this sample output?

l0rinc commented at 10:56 am on November 3, 2025:

This is the actual output of running the script, looks like it wasn’t updated in a while

optout21 commented at 11:11 am on November 3, 2025:

I was wondering how tracing is affected by this change. The trace includes the mode, and it should display it fine, so the trace should be OK.

It would be nice to check how the trace looks for a FORCE_SYNC case as well; I guess that can be triggered by an RPC call to the running node.

l0rinc commented at 11:14 am on November 3, 2025:

I’ve never used tracing before, my understanding is that @0xB10C uses these for monitoring mostly.

optout21 commented at 11:36 am on November 3, 2025:

This is the actual output of running the script, looks like it wasn’t updated in a while

I think that the scripts only enables the trace and listens to it, but it does not trigger a particular use case. If trace outputs are displayed with these changes, that should be sufficient check (i.e., tracing a FORCE_SYNC is not crucial).

0xB10C commented at 4:58 pm on November 3, 2025:

I plan have a closer look.

I don’t use the utxocache tracepoints at the moment.

0xB10C commented at 4:07 pm on November 19, 2025:

This is the actual output of running the script, looks like it wasn’t updated in a while

Since the script logs UTXO cache events, running it against a different node will produce different output. Not sure if worth changing the example output of the script here. The example output is just as valid just as it was before.

If you retouch, feel free to revert, but I don’t think it matters much.

l0rinc commented at 5:16 pm on November 19, 2025:

Thank you for checking! The headers have changed and the column widths, so I had to regenerate the example to be accurate:

0-Duration (µs)   Mode       Coins Count     Memory Usage    Prune
1+Duration (µs)   Mode         Coins Count     Memory Usage    Flush for Prune

optout21 commented at 10:53 am on November 3, 2025: none

re utACK d4425982ca4aff9b856a26f1d83d1419252f10a2

(stale utACK a59aacefd1a0cdb448c8f847665a5809f9897605)

DrahtBot requested review from cedwies on Nov 3, 2025

DrahtBot added the label Needs rebase on Nov 4, 2025

refactor: rename `FlushStateMode::ALWAYS` to `FORCE_FLUSH`

This prepares the addition of `FORCE_SYNC`.

`empty_cache` in `FlushStateToDisk` was moved up to be reusable and `FlushStateMode::FORCE_FLUSH` was used as a placeholder before we properly split the two new states.
`log_utxocache_flush.py` was regenerated and the alignment adjusted for the wider `FlushStateMode` values.

Co-authored-by: Pieter Wuille <pieter@wuille.net>
Co-authored-by: optout <13562139+optout21@users.noreply.github.com>

e99134b094

validation: do not wipe utxo cache for stats/scans/snapshots

Co-authored-by: l0rinc <pap.lorinc@gmail.com>
Co-authored-by: cedwies <141683552+cedwies@users.noreply.github.com>

d4425982ca

l0rinc commented at 5:49 pm on November 4, 2025: contributor

rebased after #30595

l0rinc force-pushed on Nov 4, 2025

DrahtBot removed the label Needs rebase on Nov 4, 2025

cedwies commented at 9:27 am on November 17, 2025: none

utACK d442598

0xB10C commented at 4:12 pm on November 19, 2025: contributor

Looked at the flush tracing changes a bit (d4425982ca4aff9b856a26f1d83d1419252f10a2). They seem fine to me. I can’t really comment on the other changes, as I’m not too familiar with the utxo cache in general.

from #33680 (review):

I was wondering how tracing is affected by this change. The trace includes the mode, and it should display it fine, so the trace should be OK. It would be nice to check how the trace looks for a FORCE_SYNC case as well; I guess that can be triggered by an RPC call to the running node.

Tested this against a signet node. gettxoutsetinfo produces FORCE_SYNC (and the cache remains filled) and shutting down produces FORCE_FLUSH. As I would have expected.

0$ python3 contrib/tracing/log_utxocache_flush.py 1234
1Hooking into bitcoind with pid 1234
2Logging utxocache flushes. Ctrl-C to end...
3Duration (µs)   Mode         Coins Count     Memory Usage    Flush for Prune
43911            FORCE_SYNC   3               262.34 kB       False
512439           FORCE_SYNC   3               262.34 kB       False
64652            FORCE_SYNC   3               262.34 kB       False
71331            FORCE_FLUSH  16              262.46 kB       False
8939             FORCE_FLUSH  0               262.24 kB       False

validation: do not wipe utxo cache for stats/scans/snapshots #33680

Code Coverage & Benchmarks

Reviews

Conflicts