coins: allow cache resize after init

jamesob commented at 6:54 pm on April 14, 2020: member

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 added the label Build system on Apr 14, 2020

DrahtBot added the label Tests on Apr 14, 2020

DrahtBot added the label UTXO Db and Indexes on Apr 14, 2020

DrahtBot added the label Validation on Apr 14, 2020

jamesob force-pushed on Apr 14, 2020

DrahtBot commented at 9:10 pm on April 14, 2020: member

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

Conflicts

Reviewers, this pull request conflicts with the following ones:

#19604 (Pass mempool pointer to UnloadBlockIndex/GetCoinsCacheSizeState by MarcoFalke)
#19556 (Remove mempool global by MarcoFalke)
#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.

in src/test/validation_chainstate_tests.cpp:73 in 03e5fe6ee3 outdated

68+        // The view cache should be empty since we had to destruct to downsize.
69+        BOOST_CHECK(!c1.CoinsTip().HaveCoinInCache(outpoint));
70+    }
71+
72+    // Avoid triggering the address sanitizer.
73+    WITH_LOCK(::cs_main, manager.Unload());

jamesob commented at 3:57 pm on April 15, 2020:

This will need to be modified with the change that @MarcoFalke has made in #18615.

MarcoFalke commented at 12:28 pm on May 23, 2020:

I don’t think this needs a SyncWithValidationInterfaceQueue before the unload because in this test not blocks are mined and no txs are added to the mempool.

in src/txdb.cpp:65 in 565a4b5c65 outdated

61+    db.reset(new CDBWrapper(ldb_path, nCacheSize, fMemory, fWipe, true));
62+}
63+
64+// XXX this should never be called without holding cs_main. Maybe figure out
65+// a better way of articulating this (lock annotations can't be used due to
66+// circular dep. with validation).

ryanofsky commented at 4:36 pm on April 16, 2020:

In commit “coins: add Sync() method to allow flush without cacheCoins drop” (b2abb396c64e96585a0c38a269372b35cced08bd)

AssertLockHeld would be one option

jamesob commented at 3:29 pm on April 22, 2020:

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.

in src/txdb.cpp:47 in 565a4b5c65 outdated

52@@ -53,35 +53,50 @@ struct CoinEntry {
53 
54 }
55 
56-CCoinsViewDB::CCoinsViewDB(fs::path ldb_path, size_t nCacheSize, bool fMemory, bool fWipe) : db(ldb_path, nCacheSize, fMemory, fWipe, true)
57+CCoinsViewDB::CCoinsViewDB(fs::path ldb_path, size_t nCacheSize, bool fMemory, bool fWipe) :
58+    m_ldb_path(ldb_path),

ryanofsky commented at 4:39 pm on April 16, 2020:

In commit “coins: add Sync() method to allow flush without cacheCoins drop” (b2abb396c64e96585a0c38a269372b35cced08bd)

Would be nice to stick to member initialization and keep empty constructor body db(MakeUnique<CDBWrapper>(...)))

in src/txdb.cpp:71 in 565a4b5c65 outdated

67+void CCoinsViewDB::ResizeCache(size_t new_cache_size)
68+{
69+    // Have to do a reset first to get the original `db` state to release its
70+    // filesystem lock.
71+    db.reset();
72+    db.reset(new CDBWrapper(

ryanofsky commented at 4:42 pm on April 16, 2020:

In commit “coins: add Sync() method to allow flush without cacheCoins drop” (b2abb396c64e96585a0c38a269372b35cced08bd)

Doesn’t really matter but general better to avoid new with MakeUnique https://isocpp.org/blog/2019/06/quick-q-differences-between-stdmake-unique-and-stdunique-ptr-with-new

jamesob commented at 3:28 pm on April 22, 2020:

Ah thanks, didn’t know that.

in src/txdb.h:46 in 565a4b5c65 outdated

42@@ -43,7 +43,9 @@ static const int64_t nMaxCoinsDBCache = 8;
43 class CCoinsViewDB final : public CCoinsView
44 {
45 protected:
46-    CDBWrapper db;
47+    std::unique_ptr<CDBWrapper> db;

ryanofsky commented at 4:45 pm on April 16, 2020:

In commit “coins: add Sync() method to allow flush without cacheCoins drop” (b2abb396c64e96585a0c38a269372b35cced08bd)

Since this commit is already changing the type of db and updating every single line where it’s referenced, it’d be nice to rename it to m_db at the same time

in src/test/validation_chainstate_tests.cpp:20 in 03e5fe6ee3 outdated

15+
16+BOOST_FIXTURE_TEST_SUITE(validation_chainstate_tests, TestingSetup)
17+
18+//! Test resizing coins-related CChainState caches during runtime.
19+//!
20+BOOST_AUTO_TEST_CASE(validation_chainstate_resize_caches)

ryanofsky commented at 4:51 pm on April 16, 2020:

In commit “test: add test for CChainState::ResizeCoinsCaches()” (03e5fe6ee3cc44f8a2b27de31e3fcd9612f907b5)

Nice test!

in src/validation.cpp:4949 in df0f720cfd outdated

4944+    BlockValidationState state;
4945+    const CChainParams& chainparams = Params();
4946+
4947+    if (coinstip_size > old_coinstip_size) {
4948+        // Likely no need to flush if cache sizes have grown.
4949+        FlushStateToDisk(chainparams, state, FlushStateMode::IF_NEEDED);

ryanofsky commented at 4:59 pm on April 16, 2020:

In commit “Add CChainState::ResizeCoinsCaches” (df0f720cfdcc1ad70d5ef2656039f008b3bc4bba)

Would be good to pass along false return value to caller in case this fails

in src/validation.cpp:138 in df0f720cfd outdated

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

ryanofsky commented at 5:07 pm on April 16, 2020:

In commit “Add CChainState::ResizeCoinsCaches” (df0f720cfdcc1ad70d5ef2656039f008b3bc4bba)

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.

in src/validation.cpp:4978 in df0f720cfd outdated

4932+        return;
4933+    }
4934+    int old_coinstip_size = m_coinstip_cache_size_bytes;
4935+    m_coinstip_cache_size_bytes = coinstip_size;
4936+    m_coinsdb_cache_size_bytes = coinsdb_size;
4937+    CoinsDB().ResizeCache(coinsdb_size);

ryanofsky commented at 5:15 pm on April 16, 2020:

In commit “Add CChainState::ResizeCoinsCaches” (df0f720cfdcc1ad70d5ef2656039f008b3bc4bba)

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

in src/validation.cpp:5301 in 69514bd9fc outdated

5236@@ -5237,3 +5237,33 @@ void ChainstateManager::Reset()
5237     m_active_chainstate = nullptr;
5238     m_snapshot_validated = false;
5239 }
5240+
5241+void ChainstateManager::MaybeRebalanceCaches()

ryanofsky commented at 5:22 pm on April 16, 2020:

In commit “add ChainstateManager::MaybeRebalanceCaches()” (69514bd9fce0f867256bd08ec9f1b616a3ece573)

Could probably write a short unit test exercising this (checking m_coins{tip,db}_cache_size_bytes)

jamesob commented at 3:28 pm on April 22, 2020:

Good idea, done.

ryanofsky approved

ryanofsky commented at 5:29 pm on April 16, 2020: member

Code review ACK 03e5fe6ee3cc44f8a2b27de31e3fcd9612f907b5. 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

jamesob force-pushed on Apr 22, 2020

jamesob commented at 3:28 pm on April 22, 2020: member

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

jamesob force-pushed on May 4, 2020

in src/test/validation_chainstatemanager_tests.cpp:120 in db5e36a033 outdated

113+
114+    // Create a legacy (IBD) chainstate.
115+    //
116+    ENTER_CRITICAL_SECTION(cs_main);
117+    CChainState& c1 = manager.InitializeChainstate();
118+    LEAVE_CRITICAL_SECTION(cs_main);

ryanofsky commented at 7:31 pm on May 19, 2020:

In commit “add ChainstateManager::MaybeRebalanceCaches()” (db5e36a033e642599db808f59d16d03a40860f17)

This is interesting. Once we update to c++14 or 17 we should be able to use WITH_LOCK like

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

I think it changing it wouldn’t work now without decltype(auto)

sipa commented at 5:37 pm on May 22, 2020:

What is different between this potential WITH_LOCK use, and the existing ones that return a value?

MarcoFalke commented at 6:00 pm on May 22, 2020:

I don’t know, but maybe C++11 lambda can not return references? If yes, this can be solved with

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

ryanofsky commented at 6:44 pm on May 22, 2020:

Yes, just what marco said, auto return type won’t deduce a reference, and I think the pointer trick would work here, so it might nice to incorporate if the PR is gets updated again

ryanofsky approved

ryanofsky commented at 7:32 pm on May 19, 2020: member

Code review ACK 4cc7b612d1413451d500c5fd6eefebea34216460. 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

ryanofsky commented at 5:32 pm on May 22, 2020: member

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

DrahtBot added the label Needs rebase on May 23, 2020

in src/init.cpp:1536 in 4cc7b612d1 outdated

1531@@ -1532,6 +1532,9 @@ bool AppInitMain(NodeContext& node)
1532             try {
1533                 LOCK(cs_main);
1534                 g_chainman.InitializeChainstate();
1535+                g_chainman.m_total_coinstip_cache = nCoinCacheUsage;
1536+                g_chainman.m_total_coinsdb_cache = nCoinDBCache;

MarcoFalke commented at 12:26 pm on May 23, 2020:

0                chainman.m_total_coinsdb_cache = nCoinDBCache;

Needs rebase to remove the g_ prefix in this line

jamesob commented at 5:37 pm on May 28, 2020:

Pushed, thanks!

jamesob force-pushed on May 28, 2020

DrahtBot removed the label Needs rebase on May 28, 2020

laanwj added this to the "Blockers" column in a project

MarcoFalke commented at 12:59 pm on May 31, 2020: member

This doesn’t compile

0validation.cpp: In member function ‘bool CChainState::ResizeCoinsCaches(size_t, size_t)’:
1
2validation.cpp:4990:23: error: comparison of integer expressions of different signedness: ‘size_t’ {aka ‘unsigned int’} and ‘int’ [-Werror=sign-compare]
3
4     if (coinstip_size > old_coinstip_size) {
5
6         ~~~~~~~~~~~~~~^~~~~~~~~~~~~~~~~~~
7
8cc1plus: some warnings being treated as errors

MarcoFalke commented at 1:00 pm on May 31, 2020: member

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

in src/test/validation_chainstatemanager_tests.cpp:120 in 1e6e0b9af6 outdated

116+
117+    // Create a legacy (IBD) chainstate.
118+    //
119+    ENTER_CRITICAL_SECTION(cs_main);
120+    CChainState& c1 = manager.InitializeChainstate();
121+    LEAVE_CRITICAL_SECTION(cs_main);

MarcoFalke commented at 1:02 pm on May 31, 2020:

style-nit in commit 1e6e0b9af6c5470dbf1e61:

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

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

in src/test/validation_chainstatemanager_tests.cpp:139 in 1e6e0b9af6 outdated

135+
136+    // Create a snapshot-based chainstate.
137+    //
138+    ENTER_CRITICAL_SECTION(cs_main);
139+    CChainState& c2 = manager.InitializeChainstate(GetRandHash());
140+    LEAVE_CRITICAL_SECTION(cs_main);

MarcoFalke commented at 1:03 pm on May 31, 2020:

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

Same

jamesob force-pushed on May 31, 2020

MarcoFalke removed the label Build system on Jun 11, 2020

MarcoFalke removed the label Tests on Jun 11, 2020

in src/txdb.cpp:47 in 0fd9039ab3 outdated

41@@ -41,35 +42,45 @@ struct CoinEntry {
42 
43 }
44 
45-CCoinsViewDB::CCoinsViewDB(fs::path ldb_path, size_t nCacheSize, bool fMemory, bool fWipe) : db(ldb_path, nCacheSize, fMemory, fWipe, true)
46+CCoinsViewDB::CCoinsViewDB(fs::path ldb_path, size_t nCacheSize, bool fMemory, bool fWipe) :
47+    m_ldb_path(ldb_path),
48+    m_is_memory(fMemory),

fjahr commented at 8:01 pm on June 15, 2020:

0fd9039ab3de00a0ac03b8fe0463f0c62a92dd43

This currently introduces a new compiler warning. The initializer list should be in the same order as they members are declared.

0txdb.cpp:47:5: warning: field 'm_is_memory' will be initialized after field 'm_db' [-Wreorder]
1    m_is_memory(fMemory),

jamesob commented at 6:20 pm on June 30, 2020:

FWIW I can’t reproduce this locally with either clang or gcc:

 0CC            = /usr/bin/ccache /usr/bin/clang
 1CFLAGS        = -g -O2
 2CPPFLAGS      =  -DDEBUG -DDEBUG_LOCKORDER  -I/home/james/src/bitcoin/db4/include/ -DHAVE_BUILD_INFO -D__STDC_FORMAT_MACROS
 3CXX           = /usr/bin/ccache /usr/bin/clang++ -std=c++11
 4CXXFLAGS      =  -O0 -g3 -ftrapv  -Wstack-protector -fstack-protector-all     -std=c++11 -Wthread-safety-analysis -Werror
 5
 6% clang++ --version
 7clang version 7.0.1-8 (tags/RELEASE_701/final)
 8Target: x86_64-pc-linux-gnu
 9Thread model: posix
10InstalledDir: /usr/bin
11
12% gcc --version
13gcc (Debian 8.3.0-6) 8.3.0
14Copyright (C) 2018 Free Software Foundation, Inc.
15This is free software; see the source for copying conditions.  There is NO
16warranty; not even for MERCHANTABILITY or FITNESS FOR A PARTICULAR PURPOSE.

ajtowns commented at 3:52 pm on July 1, 2020:

You just need to move m_db to be the first thing initialized, just as it’s the first thing declared in txdb.h. Warning appears for me with clang 9.0 fwiw. cf https://en.cppreference.com/w/cpp/language/constructor#Initialization_order

jamesob commented at 6:47 pm on July 1, 2020:

Fixed, thanks

in src/test/util/setup_common.cpp:142 in 1bf9a83671 outdated

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

fjahr commented at 8:02 pm on June 15, 2020:

1bf9a836713b44d873dd6f2b99ce8ab343808ffe

This looks like it could be a constant?

in src/test/validation_chainstatemanager_tests.cpp:127 in 6df742f093 outdated

122+    c1.InitCoinsDB(
123+        /* cache_size_bytes */ 1 << 23, /* in_memory */ true, /* should_wipe */ false);
124+
125+    {
126+        LOCK(::cs_main);
127+        c1.InitCoinsCache(1 << 23);

fjahr commented at 8:05 pm on June 15, 2020:

6df742f093de1c1360e3f37d011fbdf0a3888017

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

fjahr approved

fjahr commented at 8:09 pm on June 15, 2020: member

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

ryanofsky approved

ryanofsky commented at 5:19 pm on June 30, 2020: member

Code review ACK bb6ba5ff781e54eb83dc271c335031c7e9488f86. 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 commented at 5:20 pm on June 30, 2020: member

[“if anyone wants to see forward motion on assumeutxo, #18637 is the one to review”](http://www.erisian.com.au/bitcoin-core-dev/log-2020-06-11.html#l-442)

txdb: add CCoinsViewDB::ChangeCacheSize

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.

b223111da2

Add CChainState::ResizeCoinsCaches

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.

f36aaa6392

add ChainstateManager::MaybeRebalanceCaches()

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

8ac3ef4699

test: add test for CChainState::ResizeCoinsCaches() f19fdd47a6

in src/txdb.h:70 in 0fd9039ab3 outdated

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

ajtowns commented at 4:26 pm on July 1, 2020:

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.

jamesob force-pushed on Jul 1, 2020

ajtowns commented at 6:47 pm on July 1, 2020: member

weak utACK f19fdd47a6371dcbe0760ef6f3c3c5adb31b1bb4 – didn’t find any major problems, but not super confident that I didn’t miss anything

EDIT: (update commit fixes init order)

in src/validation.cpp:5314 in 8ac3ef4699 outdated

5309+        LogPrintf("[snapshot] allocating all cache to the snapshot chainstate\n");
5310+        // Allocate everything to the snapshot chainstate.
5311+        m_snapshot_chainstate->ResizeCoinsCaches(m_total_coinstip_cache, m_total_coinsdb_cache);
5312+    }
5313+    else if (m_ibd_chainstate && m_snapshot_chainstate) {
5314+        // If both chainstates exist, determine who needs more cache based on IBD status.

fjahr commented at 9:59 pm on July 3, 2020:

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.

fjahr approved

fjahr commented at 10:00 pm on July 3, 2020: member

Code review ACK f19fdd4

ryanofsky approved

ryanofsky commented at 6:06 pm on July 8, 2020: member

Code review ACK f19fdd47a6371dcbe0760ef6f3c3c5adb31b1bb4. 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 commented at 10:15 am on July 24, 2020: member

This looks ready to merge

MarcoFalke merged this on Jul 29, 2020

MarcoFalke closed this on Jul 29, 2020

in src/test/validation_chainstatemanager_tests.cpp:59 in f19fdd47a6

54@@ -57,12 +55,13 @@ BOOST_AUTO_TEST_CASE(chainstatemanager)
55     // Create a snapshot-based chainstate.
56     //
57     ENTER_CRITICAL_SECTION(cs_main);
58-    CChainState& c2 = manager.InitializeChainstate(GetRandHash());
59+    CChainState& c2 = *WITH_LOCK(::cs_main,
60+        return &manager.InitializeChainstate(GetRandHash()));

darosior commented at 12:45 pm on July 29, 2020:

Doesn’t this lock cs_main twice ?

MarcoFalke commented at 12:48 pm on July 29, 2020:

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

darosior commented at 12:55 pm on July 29, 2020:

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

laanwj removed this from the "Blockers" column in a project

sidhujag referenced this in commit 191c48d32d on Jul 31, 2020

fanquake added this to the "Done" column in a project

deadalnix referenced this in commit 8af6d9df95 on Feb 23, 2021

EyeOfPython referenced this in commit 7a7219e18b on Apr 10, 2021

EyeOfPython referenced this in commit cd94d41e6d on Apr 10, 2021

EyeOfPython referenced this in commit 3b4b7cde43 on Apr 10, 2021

DrahtBot locked this on Feb 15, 2022

coins: allow cache resize after init #18637

Conflicts