coins: allow cache resize after init #18637

jamesob · 2020-04-14T18:54:22Z

Parent PR: #15606
Issue: #15605
Specification: https://github.com/jamesob/assumeutxo-docs/tree/master/proposal

In the assumeutxo implementation draft (#15056), once a UTXO snapshot is loaded, a new chainstate object is created after initialization. This means that we have to reclaim some of the cache that we've allocated to the original chainstate (per dbcache=) to repurpose for the snapshot chainstate.

Furthermore, it makes sense to have different cache allocations depending on which chainstate is more active. While the snapshot chainstate is working to get to the network tip (and the background validation chainstate is idle), it makes sense that the snapshot chainstate should have the majority of cache allocation. And contrariwise once the snapshot has reached network tip, most of the cache should be given to the background validation chainstate.

This set of changes (detailed in the commit messages) allows us to dynamically resize the various coins caches. None of the functionality introduced here is used at the moment, but will be in the next AU PR (which introduces ActivateSnapshot).

ChainstateManager::MaybeRebalanceCaches() defines the (somewhat normative) cache allocations between the snapshot and background validation chainstates. I'd be interested in feedback if anyone has thoughts on the proportions I've set there.

DrahtBot · 2020-04-14T21:10:15Z

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

Conflicts

Reviewers, this pull request conflicts with the following ones:

Pass mempool pointer to UnloadBlockIndex/GetCoinsCacheSizeState #19604 (Pass mempool pointer to UnloadBlockIndex/GetCoinsCacheSizeState by MarcoFalke)
Remove mempool global #19556 (Remove mempool global by MarcoFalke)
coins: allow write to disk without cache drop #17487 (coins: allow write to disk without cache drop by jamesob)

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.

src/test/validation_chainstate_tests.cpp

ryanofsky

Code review ACK 03e5fe6. Very clean, straightforward changes. Feel free to ignore suggestions below, but one I'd consider more is passing up FlushStateToDisk return false status, to be able to log more context information if it fails for some reason

ryanofsky · 2020-04-16T16:36:13Z

src/txdb.cpp

+// XXX this should never be called without holding cs_main. Maybe figure out
+// a better way of articulating this (lock annotations can't be used due to
+// circular dep. with validation).


In commit "coins: add Sync() method to allow flush without cacheCoins drop" (b2abb39)

AssertLockHeld would be one option

I just ended up using an extern declaration for the lock and adding an annotation. Not sure why I didn't do that in the first place.

src/txdb.cpp

src/txdb.h

ryanofsky · 2020-04-16T16:51:46Z

src/test/validation_chainstate_tests.cpp

+
+//! Test resizing coins-related CChainState caches during runtime.
+//!
+BOOST_AUTO_TEST_CASE(validation_chainstate_resize_caches)


In commit "test: add test for CChainState::ResizeCoinsCaches()" (03e5fe6)

Nice test!

src/validation.cpp

ryanofsky · 2020-04-16T17:07:19Z

src/validation.cpp

@@ -116,7 +116,6 @@ bool fPruneMode = false;
 bool fRequireStandard = true;
 bool fCheckBlockIndex = false;
 bool fCheckpointsEnabled = DEFAULT_CHECKPOINTS_ENABLED;
-size_t nCoinCacheUsage = 5000 * 300;


In commit "Add CChainState::ResizeCoinsCaches" (df0f720)

Just confirming but looks like this 1.5MB default cache size was only used by test cases previously, which are now using 8MB or 1KB caches depending on the case.

ryanofsky · 2020-04-16T17:15:24Z

src/validation.cpp

+    int old_coinstip_size = m_coinstip_cache_size_bytes;
+    m_coinstip_cache_size_bytes = coinstip_size;
+    m_coinsdb_cache_size_bytes = coinsdb_size;
+    CoinsDB().ResizeCache(coinsdb_size);


In commit "Add CChainState::ResizeCoinsCaches" (df0f720)

Just curious, but would it make sense to call FlushStateToDisk before resetting CoinsDB instead of after? Thought would be that maybe live CDBWrapper object has cached state that would make the flush faster, or maybe it could be good to start with a new CDBWrapper after the flush

src/validation.cpp

jamesob · 2020-04-22T15:28:09Z

Thanks for the review and good suggestions, @ryanofsky. I've incorporated your feedback in the latest revision.

ryanofsky

Code review ACK 4cc7b61. Changes since previous review were suggested things: using MakeUnique instead of new, renaming db to m_db while all lines are changing anyway, returning failure from FlushStateToDisk, and adding rebalance test

src/test/validation_chainstatemanager_tests.cpp

ryanofsky · 2020-05-22T17:32:30Z

Should this be added to in progress column https://github.com/bitcoin/bitcoin/projects/11?

Also maybe if there's a main assumeutxo pr you'd prioritize for review now it could go on high priority reviews https://github.com/bitcoin/bitcoin/projects/8

src/init.cpp

maflcko · 2020-05-31T12:59:37Z

This doesn't compile

validation.cpp: In member function ‘bool CChainState::ResizeCoinsCaches(size_t, size_t)’:

validation.cpp:4990:23: error: comparison of integer expressions of different signedness: ‘size_t’ {aka ‘unsigned int’} and ‘int’ [-Werror=sign-compare]

     if (coinstip_size > old_coinstip_size) {

         ~~~~~~~~~~~~~~^~~~~~~~~~~~~~~~~~~

cc1plus: some warnings being treated as errors

maflcko · 2020-05-31T13:00:07Z

It does compile locally, so I am not sure how to reproduce this failure :(

maflcko · 2020-05-31T13:02:50Z

src/test/validation_chainstatemanager_tests.cpp

+    ENTER_CRITICAL_SECTION(cs_main);
+    CChainState& c1 = manager.InitializeChainstate();
+    LEAVE_CRITICAL_SECTION(cs_main);


style-nit in commit 1e6e0b9:

Now that you have to force push anyway, might as well remove the clumsy ENTER_CRITICAL_SECTION?

Suggested change

6DAF

ENTER_CRITICAL_SECTION(cs_main);

CChainState& c1 = manager.InitializeChainstate();

LEAVE_CRITICAL_SECTION(cs_main);

CChainState& c1 = *WITH_LOCK(cs_main, return &manager.InitializeChainstate());

maflcko · 2020-05-31T13:03:51Z

src/test/validation_chainstatemanager_tests.cpp

+    ENTER_CRITICAL_SECTION(cs_main);
+    CChainState& c2 = manager.InitializeChainstate(GetRandHash());
+    LEAVE_CRITICAL_SECTION(cs_main);


Suggested change

ENTER_CRITICAL_SECTION(cs_main);

CChainState& c2 = manager.InitializeChainstate(GetRandHash());

LEAVE_CRITICAL_SECTION(cs_main);

CChainState& c2 = *WITH_LOCK(cs_main, return &manager.InitializeChainstate(GetRandHash()));

Same

fjahr

Code looks great but the warning should be fixed. Will do another pass then.

src/txdb.cpp

fjahr · 2020-06-15T20:02:47Z

src/test/util/setup_common.cpp

@@ -139,7 +139,7 @@ TestingSetup::TestingSetup(const std::string& chainName, const std::vector<const
    ::ChainstateActive().InitCoinsDB(
        /* cache_size_bytes */ 1 << 23, /* in_memory */ true, /* should_wipe */ false);
    assert(!::ChainstateActive().CanFlushToDisk());
-    ::ChainstateActive().InitCoinsCache();
+    ::ChainstateActive().InitCoinsCache(1 << 23);


1bf9a83

This looks like it could be a constant?

fjahr · 2020-06-15T20:05:53Z

src/test/validation_chainstatemanager_tests.cpp

+              


            
+    {
+        LOCK(::cs_main);
+        c1.InitCoinsCache(1 << 23);


6df742f

In the test I think 1 << 23 could also be a variable to help a little bit with readability.

ryanofsky

Code review ACK bb6ba5f. Only changes since last review are rebase, replacing int with size_t, replacing ENTER_CRITICAL_SECTION with WITH_LOCK.

There are some open review suggestions above but they seem pretty minor and fine to save for a followup

ryanofsky · 2020-06-30T17:20:44Z

"if anyone wants to see forward motion on assumeutxo, #18637 is the one to review"

We'll need this to dynamically update the cache size of the existing CCoinsViewDB instance when we create a new one during snapshot activation. This requires us to keep the CDBWrapper instance as a pointer instead of a reference so that we're able to destruct it and create a new instance when the cache size changes. Also renames `db` to `m_db` since we're already modifying each usage. Includes feedback from Russ Yanofsky.

Also adds CCoinsViewCache::ReallocateCache() to attempt to free memory that the cacheCoins's allocator may be hanging onto when downsizing the cache. Adds `CChainState::m_coins{tip,db}_cache_size_bytes` data members so that we can reference cache size on a per-chainstate basis for flushing.

Aside from in unittests, this method is unused at the moment. It will be used in upcoming commits that enable utxo snapshot activation.

ajtowns

weak utACK f19fdd4 -- didn't find any major problems, but not super confident that I didn't miss anything

EDIT: (update commit fixes init order)

ajtowns · 2020-07-01T16:26:23Z

src/txdb.h

@@ -60,6 +65,9 @@ class CCoinsViewDB final : public CCoinsView
    //! Attempt to update from an older database format. Returns whether an error occurred.
    bool Upgrade();
    size_t EstimateSize() const override;
+
+    //! Dynamically alter the underlying leveldb cache size.
+    void ResizeCache(size_t new_cache_size) EXCLUSIVE_LOCKS_REQUIRED(cs_main);


This seems a bit of an anti-pattern -- it's not ResizeCache() that needs cs_main, it's the fact that when it's called it ends up operating on m_coins_views->m_dbview which does need cs_main, but that's not enforced because the constraint is thrown away by CoinsDB() returning a reference to the guarded object.

I think it might be better to replace CoinsDB() by a GetCoinsViews() that returns a reference to m_coins_views, and then have auto& x = GetCoinsViews(); LOCK(cs_main); x.m_dbview.ResizeCache(y); ? No need to address in this PR though, I think.

fjahr

Code review ACK f19fdd4

fjahr · 2020-07-03T21:59:03Z

src/validation.cpp

+        m_snapshot_chainstate->ResizeCoinsCaches(m_total_coinstip_cache, m_total_coinsdb_cache);
+    }
+    else if (m_ibd_chainstate && m_snapshot_chainstate) {
+        // If both chainstates exist, determine who needs more cache based on IBD status.


super-nit: A bit more info on the rationale behind 0.05/0.95 would be nice. Also, the values could be constants. But that can be considered for a follow-up.

ryanofsky

Code review ACK f19fdd4. Only change since last review is constructor cleanup (no change in behavior). I think the suggestions here from ajtowns and others are good, but shouldn't delay merging the PR (and hold up assumeutxo)

ryanofsky · 2020-07-24T10:15:42Z

This looks ready to merge

darosior · 2020-07-29T12:45:58Z

src/test/validation_chainstatemanager_tests.cpp

    ENTER_CRITICAL_SECTION(cs_main);
-    CChainState& c2 = manager.InitializeChainstate(GetRandHash());
+    CChainState& c2 = *WITH_LOCK(::cs_main,
+        return &manager.InitializeChainstate(GetRandHash()));


Doesn't this lock cs_main twice ?

The test nits are fixed in the first commit of #19604

Yep. Brainfart. Just noticed while rebasing #19556 and commented without thinking.

f19fdd4 test: add test for CChainState::ResizeCoinsCaches() (James O'Beirne) 8ac3ef4 add ChainstateManager::MaybeRebalanceCaches() (James O'Beirne) f36aaa6 Add CChainState::ResizeCoinsCaches (James O'Beirne) b223111 txdb: add CCoinsViewDB::ChangeCacheSize (James O'Beirne) Pull request description: This is part of the [assumeutxo project](https://github.com/bitcoin/bitcoin/projects/11): Parent PR: bitcoin#15606 Issue: bitcoin#15605 Specification: https://github.com/jamesob/assumeutxo-docs/tree/master/proposal --- In the assumeutxo implementation draft (bitcoin#15056), once a UTXO snapshot is loaded, a new chainstate object is created after initialization. This means that we have to reclaim some of the cache that we've allocated to the original chainstate (per `dbcache=`) to repurpose for the snapshot chainstate. Furthermore, it makes sense to have different cache allocations depending on which chainstate is more active. While the snapshot chainstate is working to get to the network tip (and the background validation chainstate is idle), it makes sense that the snapshot chainstate should have the majority of cache allocation. And contrariwise once the snapshot has reached network tip, most of the cache should be given to the background validation chainstate. This set of changes (detailed in the commit messages) allows us to dynamically resize the various coins caches. None of the functionality introduced here is used at the moment, but will be in the next AU PR (which introduces `ActivateSnapshot`). `ChainstateManager::MaybeRebalanceCaches()` defines the (somewhat normative) cache allocations between the snapshot and background validation chainstates. I'd be interested in feedback if anyone has thoughts on the proportions I've set there. ACKs for top commit: ajtowns: weak utACK f19fdd4 -- didn't find any major problems, but not super confident that I didn't miss anything fjahr: Code review ACK f19fdd4 ryanofsky: Code review ACK f19fdd4. Only change since last review is constructor cleanup (no change in behavior). I think the suggestions here from ajtowns and others are good, but shouldn't delay merging the PR (and hold up assumeutxo) Tree-SHA512: fffb7847fb6993dd4a1a41cf11179b211b0b20b7eb5f7cf6266442136bfe9d43b830bbefcafd475bfd4af273f5573500594aa41fff03e0ed5c2a1e8562ff9269

Summary: f19fdd47a6371dcbe0760ef6f3c3c5adb31b1bb4 test: add test for CChainState::ResizeCoinsCaches() (James O'Beirne) 8ac3ef46999ed676ca3775f7b2f461d92f09a542 add ChainstateManager::MaybeRebalanceCaches() (James O'Beirne) f36aaa6392fdbdac6891d92202d3efeff98754f4 Add CChainState::ResizeCoinsCaches (James O'Beirne) b223111da2e0e9ceccef75df8a20252b0094b7bc txdb: add CCoinsViewDB::ChangeCacheSize (James O'Beirne) Pull request description: This is part of the [assumeutxo project](https://github.com/bitcoin/bitcoin/projects/11): Parent PR: #15606 Issue: #15605 Specification: https://github.com/jamesob/assumeutxo-docs/tree/master/proposal --- In the assumeutxo implementation draft (#15056), once a UTXO snapshot is loaded, a new chainstate object is created after initialization. This means that we have to reclaim some of the cache that we've allocated to the original chainstate (per `dbcache=`) to repurpose for the snapshot chainstate. Furthermore, it makes sense to have different cache allocations depending on which chainstate is more active. While the snapshot chainstate is working to get to the network tip (and the background validation chainstate is idle), it makes sense that the snapshot chainstate should have the majority of cache allocation. And contrariwise once the snapshot has reached network tip, most of the cache should be given to the background validation chainstate. This set of changes (detailed in the commit messages) allows us to dynamically resize the various coins caches. None of the functionality introduced here is used at the moment, but will be in the next AU PR (which introduces `ActivateSnapshot`). `ChainstateManager::MaybeRebalanceCaches()` defines the (somewhat normative) cache allocations between the snapshot and background validation chainstates. I'd be interested in feedback if anyone has thoughts on the proportions I've set there. --- Backport of [[bitcoin/bitcoin#18637 | core#18637]] Test Plan: ninja all check check-functional Reviewers: #bitcoin_abc, PiRK Reviewed By: #bitcoin_abc, PiRK Subscribers: PiRK Differential Revision: https://reviews.bitcoinabc.org/D9259

DrahtBot added Build system Tests UTXO Db and Indexes Validation labels Apr 14, 2020

jamesob force-pushed the 2020-04-au.cache-resize branch from a363733 to 03e5fe6 Compare April 14, 2020 21:05

This was referenced Apr 14, 2020

coins: allow write to disk without cache drop #17487

Merged

assumeutxo #15606

Closed

jamesob commented Apr 15, 2020

View reviewed changes

src/test/validation_chainstate_tests.cpp Show resolved Hide resolved

ryanofsky approved these changes Apr 16, 2020

View reviewed changes

This was referenced Apr 18, 2020

Make g_chainman internal to validation #18698

Merged

Add local thread pool to CCheckQueue #18710

Merged

refactor: Make CCheckQueue RAII-styled #18731

Closed

jamesob force-pushed the 2020-04-au.cache-resize branch from 03e5fe6 to bd350f7 Compare April 22, 2020 15:26

jamesob force-pushed the 2020-04-au.cache-resize branch from bd350f7 to 4cc7b61 Compare May 4, 2020 19:14

This was referenced May 10, 2020

validation: Persist coins cache to disk and load on startup #18936

Closed

validation: Persist coins cache to disk and load on startup #18941

Closed

ryanofsky approved these changes May 19, 2020

View reviewed changes

src/test/validation_chainstatemanager_tests.cpp Show resolved Hide resolved

DrahtBot added the Needs rebase label May 23, 2020

maflcko reviewed May 23, 2020

View reviewed changes

src/init.cpp Outdated Show resolved Hide resolved

jamesob force-pushed the 2020-04-au.cache-resize branch 2 times, most recently from ee91979 to 4f8e999 Compare May 28, 2020 17:39

DrahtBot removed the Needs rebase label May 28, 2020

maflcko reviewed May 31, 2020

View reviewed changes

maflcko mentioned this pull request May 31, 2020

How to reproduce -Werror=sign-compare failures locally? #19123

Closed

jamesob force-pushed the 2020-04-au.cache-resize branch from 4f8e999 to d055659 Compare May 31, 2020 15:17

jamesob force-pushed the 2020-04-au.cache-resize branch from d055659 to bb6ba5f Compare May 31, 2020 17:09

maflcko removed Build system Tests labels Jun 11, 2020

fjahr approved these changes Jun 15, 2020

View reviewed changes

DrahtBot mentioned this pull request Jun 17, 2020

refactor: Replace RecursiveMutex with Mutex in CTxMemPool #19306

Closed

ryanofsky approved these changes Jun 30, 2020

View reviewed changes

jamesob and others added 4 commits July 1, 2020 14:44

add ChainstateManager::MaybeRebalanceCaches()

8ac3ef4

Aside from in unittests, this method is unused at the moment. It will be used in upcoming commits that enable utxo snapshot activation.

test: add test for CChainState::ResizeCoinsCaches()

f19fdd4

jamesob force-pushed the 2020-04-au.cache-resize branch from bb6ba5f to f19fdd4 Compare July 1, 2020 18:46

ajtowns reviewed Jul 1, 2020

View reviewed changes

fjahr approved these changes Jul 3, 2020

View reviewed changes

ryanofsky approved these changes Jul 8, 2020

View reviewed changes

DrahtBot mentioned this pull request Jul 20, 2020

Remove mempool global #19556

Merged

DrahtBot mentioned this pull request Jul 28, 2020

Pass mempool pointer to UnloadBlockIndex/GetCoinsCacheSizeState #19604

Merged

maflcko merged commit 2f71a1e into bitcoin:master Jul 29, 2020

darosior reviewed Jul 29, 2020

View reviewed changes

maflcko mentioned this pull request Feb 18, 2021

test: Avoid comparision of integers with different signs #21228

Merged

kwvg mentioned this pull request Oct 9, 2021

merge bitcoin#15948, #15976, #16194...: assumeutxo project backports (part 1) dashpay/dash#4498

Merged

bitcoin locked as resolved and limited conversation to collaborators Feb 15, 2022

coins: allow cache resize after init #18637

coins: allow cache resize after init #18637

Uh oh!

Conversation

Uh oh!

Uh oh!

Conflicts

Uh oh!

Uh oh!

Choose a reason for hiding this comment

Uh oh!

Choose a reason for hiding this comment

Uh oh!

Choose a reason for hiding this comment

Uh oh!

Uh oh!

Uh oh!

Uh oh!

Choose a reason for hiding this comment

Uh oh!

Uh oh!

Choose a reason for hiding this comment

Uh oh!

Choose a reason for hiding this comment

Uh oh!

Uh oh!

Uh oh!

Choose a reason for hiding this comment

Uh oh!

Uh oh!

Uh oh!

Uh oh!

Uh oh!

Uh oh!

Choose a reason for hiding this comment

Uh oh!

Choose a reason for hiding this comment

Uh oh!

Choose a reason for hiding this comment

Uh oh!

Uh oh!

Choose a reason for hiding this comment

Uh oh!

Choose a reason for hiding this comment

Uh oh!

Choose a reason for hiding this comment

Uh oh!

Uh oh!

Uh oh!

Choose a reason for hiding this comment

Uh oh!

Choose a reason for hiding this comment

Uh oh!

Choose a reason for hiding this comment

Uh oh!

Choose a reason for hiding this comment

Uh oh!

Choose a reason for hiding this comment

Uh oh!

Uh oh!

Choose a reason for hiding this comment

Uh oh!

Choose a reason for hiding this comment

Uh oh!

Choose a reason for hiding this comment

Uh oh!

Uh oh!