coins: warn on shutdown for big UTXO set flushes

l0rinc commented at 6:44 pm on December 18, 2024: contributor

Setting a large -dbcache size postpones the index writes until the coins cache size exceeds the specified limit. This causes the final flush after manual termination to seemingly hang forever (e.g. tens of minutes for 20 GiB); Now that the dbcache upper cap has been lifted, this will become even more apparent, so a warning will be shown when large UTXO sets are flushed (currently >1 GiB), such as:

2024-12-18T18:25:03Z Flushed fee estimates to fee_estimates.dat. 2024-12-18T18:25:03Z [warning] Flushing large 1 GiB UTXO set to disk may take several minutes. 2024-12-18T18:25:09Z Shutdown: done

You can reproduce it by starting bitcoind with a large -dbcache:

mkdir demo && cmake -B build -DCMAKE_BUILD_TYPE=Release && cmake –build build -j$(nproc) && build/src/bitcoind -datadir=demo -dbcache=10000

Waiting until the used memory is over 1 GiB

2024-12-18T18:25:02Z UpdateTip: […] progress=0.069009 cache=1181.1MiB(8827981txo)

And cancelling the process from the terminal:

^C2024-12-18T18:25:03Z tor: Thread interrupt […] 2024-12-18T18:25:03Z [warning] Flushing large 1 GiB UTXO set to disk may take several minutes.

DrahtBot commented at 6:44 pm on December 18, 2024: 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/31534.

Reviews

See the guideline for information on the review process.

Type	Reviewers
Concept ACK	theuni, BrandonOdiwuor
Stale ACK	1440000bytes

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:

#30611 (validation: write chainstate to disk every hour by andrewtoth)

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.

DrahtBot added the label UTXO Db and Indexes on Dec 18, 2024

1440000bytes commented at 7:48 pm on December 18, 2024: none

Why is this a warning and not normal log?

l0rinc commented at 7:55 pm on December 18, 2024: contributor

Why is this a warning and not normal log?

Instead of logging every time that we’re flushing, we only show this when the process will hang for a (likely unexpectedly) long time - hence the warn.

1440000bytes approved

1440000bytes commented at 8:09 pm on December 18, 2024: none

utACK https://github.com/bitcoin/bitcoin/pull/31534/commits/2abfb8372dc06b0eeaccc3ef354173fecc2cedae

in src/validation.cpp:2932 in 2abfb8372d outdated

2928@@ -2929,6 +2929,7 @@ bool Chainstate::FlushStateToDisk(
2929         }
2930         // Flush best chain related state. This can only be done if the blocks / block index write was also done.
2931         if (fDoFullFlush && !CoinsTip().GetBestBlock().IsNull()) {
2932+            if (auto mem_gib{coins_mem_usage >> 30}) LogWarning("Flushing large %d GiB UTXO set to disk...", mem_gib);

theuni commented at 9:56 pm on December 18, 2024:

Could you please name a constant for this?

And while we’re at it… it’d be nice to express these sizes in terms of user-defined literals. See: https://github.com/theuni/bitcoin/commits/user-defined-size-literals/

So this could become:

0static constexpr auto WARN_FLUSH_COINS_SIZE = 1GiB;
1if(coins_mem_usage >= WARN_FLUSH_COINS_SIZE)
2...

l0rinc commented at 10:15 pm on December 18, 2024:

I don’t mind user defined literals (it’s what we did for hexadecimal conversion as well) - but to be fair, 32<<20 isn’t a lot worse than32_MiB (but the second may be more descriptive, indeed).

Extracted the constant, let me know how you’d like to apply the user-defined literals - my understanding is that they’re not merged yet, right?

Edit: how would we convert bytes to gigabytes that way? coins_mem_usage / 1_GiB instead of coins_mem_usage >> 30?

theuni commented at 10:25 pm on December 18, 2024:

Yeah, I find coins_mem_usage / 1_GiB to be much more obvious in meaning.

But sure, it’s out of scope for this PR. I’ll open another one for that.

theuni commented at 9:56 pm on December 18, 2024: member

Concept ACK. Good idea!

l0rinc force-pushed on Dec 18, 2024

in src/validation.cpp:2934 in 36a8133c7b outdated

2930@@ -2929,6 +2931,7 @@ bool Chainstate::FlushStateToDisk(
2931         }
2932         // Flush best chain related state. This can only be done if the blocks / block index write was also done.
2933         if (fDoFullFlush && !CoinsTip().GetBestBlock().IsNull()) {
2934+            if (coins_mem_usage >= WARN_FLUSH_COINS_SIZE) LogWarning("Flushing large %d GiB UTXO set to disk...", coins_mem_usage >> 30);

theuni commented at 10:30 pm on December 18, 2024:

Thanks for the constant.

This is now printing GiB, and the very next log line will be printing kB. Not exactly pretty, but I suppose it’s only when bench is on.

Also, to the average user seeing this, it may not be exactly clear what you’re trying to communicate. How about adding “This may take some time.” or so?

l0rinc commented at 10:45 pm on December 18, 2024:

This is now printing GiB, and the very next log line will be printing kB. Not exactly pretty, but I suppose it’s only when bench is on.

Yeah, I saw that, it’s dividing by 1000 - changed to KiB and >> 10.

theuni commented at 10:33 pm on December 18, 2024: member

utACK after addressing the string nit.

coins: warn on shutdown for big UTXO set flushes

Setting a large `-dbcache` size postpones the index writes until the coins cache size exceeds the specified limit.
This causes the final flush after manual termination to seemingly hang forever (e.g. tens of minutes for 20 GiB);
Now that the `dbcache` upper cap has been lifted, this will become even more apparent, so a warning will be shown when large UTXO sets are flushed (currently >1 GiB), such as:
> 2024-12-18T18:25:03Z Flushed fee estimates to fee_estimates.dat.
> 2024-12-18T18:25:03Z [warning] Flushing large 1 GiB UTXO set to disk may take several minutes.
> 2024-12-18T18:25:09Z Shutdown: done

Note that the related BCLog::BENCH units were also converted to `KiB` from `kB` to unify the bases.

Co-authored-by: Cory Fields <cory-nospam-@coryfields.com>

ec8fa17872

l0rinc force-pushed on Dec 18, 2024

BrandonOdiwuor commented at 10:39 am on December 19, 2024: contributor

Concept ACK

coins: warn on shutdown for big UTXO set flushes #31534

Code Coverage & Benchmarks

Reviews

Conflicts