init: Correct coins db cache size setting

TheCharlatan commented at 9:26 pm on October 9, 2024: contributor

The chainstate caches are currently re-balanced on startup even in the non-assumeutxo case, leading to the database being needlessly re-opened and its cache re-allocated.

Similar to InitCoinsCache and m_coinstip_cache_size_bytes, the m_coinsdb_cache_size_bytes should be set in InitCoinsDB.

Together with only conservatively setting the cache values when a assumeutxo chainstate is present, this allows for skipping the cache re-balance during initialization in the normal non-assumeutxo case.

Before:

 02024-10-09T21:22:17Z Checking all blk files are present...
 12024-10-09T21:22:17Z Initializing chainstate Chainstate [ibd] @ height -1 (null)
 22024-10-09T21:22:17Z Opening LevelDB in /home/drgrid/.bitcoin/signet/chainstate
 32024-10-09T21:22:17Z Opened LevelDB successfully
 42024-10-09T21:22:17Z Using obfuscation key for /home/drgrid/.bitcoin/signet/chainstate: b0a6f4e95fd05c92
 52024-10-09T21:22:17Z Loaded best chain: hashBestChain=0000000e119967d4937dad58456885ae43fb1761db686947e2f8e168c9a39a4f height=216852 date=2024-10-09T21:06:16Z progress=0.999989
 62024-10-09T21:22:17Z Opening LevelDB in /home/drgrid/.bitcoin/signet/chainstate
 72024-10-09T21:22:17Z Opened LevelDB successfully
 82024-10-09T21:22:17Z Using obfuscation key for /home/drgrid/.bitcoin/signet/chainstate: b0a6f4e95fd05c92
 92024-10-09T21:22:17Z [Chainstate [ibd] @ height 216852 (0000000e119967d4937dad58456885ae43fb1761db686947e2f8e168c9a39a4f)] resized coinsdb cache to 8.0 MiB
102024-10-09T21:22:17Z [Chainstate [ibd] @ height 216852 (0000000e119967d4937dad58456885ae43fb1761db686947e2f8e168c9a39a4f)] resized coinstip cache to 440.0 MiB
112024-10-09T21:22:17Z init message: Verifying blocks…

After:

02024-10-09T21:21:37Z Checking all blk files are present...
12024-10-09T21:21:37Z Initializing chainstate Chainstate [ibd] @ height -1 (null)
22024-10-09T21:21:37Z Opening LevelDB in /home/drgrid/.bitcoin/signet/chainstate
32024-10-09T21:21:37Z Opened LevelDB successfully
42024-10-09T21:21:37Z Using obfuscation key for /home/drgrid/.bitcoin/signet/chainstate: b0a6f4e95fd05c92
52024-10-09T21:21:37Z Loaded best chain: hashBestChain=0000012c12b48011a7d9150ce96ed6a44bbf32b09eeecaff4a667789dda2a566 height=216850 date=2024-10-09T20:37:05Z progress=0.999971
62024-10-09T21:21:37Z init message: Verifying blocks…

The change may also be verified by looking at the feature_assumeutxo.py functional test debug logs.

DrahtBot commented at 9:26 pm on October 9, 2024: contributor

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

Code Coverage

For detailed information about the code coverage, see the test coverage report.

Reviews

See the guideline for information on the review process.

Type	Reviewers
ACK	fjahr, laanwj, BrandonOdiwuor, achow101

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

in src/node/chainstate.cpp:102 in e24783efe5 outdated

 98@@ -99,7 +99,7 @@ static ChainstateLoadResult CompleteChainstateInitialization(
 99     // by a call to `chainman.MaybeRebalanceCaches()`. We just need to make sure
100     // that the sum of the two caches (40%) does not exceed the allowable amount
101     // during this temporary initialization state.
102-    double init_cache_fraction = 0.2;

fjahr commented at 8:48 am on October 10, 2024:

The comment above needs to be edited. At least “Conservative value which is arbitrarily chosen” should make clear that it’s referring to 0.2. And the reference to MaybeRebalanceCache should probably also be changed.

TheCharlatan commented at 8:52 am on October 10, 2024:

Seemed clear to me that the comment refers to the smaller factor, but reading it again, I think you are right. Will re-word.

fjahr commented at 8:49 am on October 10, 2024: contributor

Concept ACK

init: Correct coins db cache size setting

The chainstate caches are currently re-balanced on startup
even in the non-assumeutxo case, leading to the database being
needlessly re-opened and its cache re-allocated.

Similar to `InitCoinsCache` and `m_coinstip_cache_size_bytes` the
`m_coinsdb_cache_size_bytes` should be set in `InitCoinsDB`.

Together with only conservatively setting the cache values when a
assumeutxo chainstate is present, this allows for skipping the cache
re-balance during initialization in the normal non-assumeutxo case.

3a4a788ee0

TheCharlatan force-pushed on Oct 10, 2024

TheCharlatan commented at 9:32 am on October 10, 2024: contributor

Updated e24783efe5cc17c17349b8ed96ef852e73ff8309 -> 3a4a788ee0db83d20607f14801dbed2ee932943c (patchCoinsDBCacheSizeInit_0 -> patchCoinsDBCacheSizeInit_1, compare)

Addressed @fjahr’s comment, rewording the comment describing the init_cache_fraction.

in src/node/chainstate.cpp:103 in 3a4a788ee0

103+    // If running with multiple chainstates, limit the cache sizes with a
104+    // discount factor. If discounted the actual cache size will be
105+    // recalculated by `chainman.MaybeRebalanceCaches()`. The discount factor
106+    // is conservatively chosen such that the sum of the caches does not exceed
107+    // the allowable amount during this temporary initialization state.
108+    double init_cache_fraction = chainman.GetAll().size() > 1 ? 0.2 : 1.0;

l0rinc commented at 9:40 am on October 10, 2024:

Is this the same as

0    auto init_cache_fraction = chainman.IsSnapshotActive() ? 0.2 : 1.0;

?

TheCharlatan commented at 9:49 am on October 10, 2024:

I think in practice it is, since we call this after DetectSnapshotChainstate(), which in turn calls ActivateExistingSnapshot if there is a snapshot chainstate, but checking if there is more than one chainstate seems more defensive to me, since it relies on fewer assumptions.