This is part of the assumeutxo project:
Parent PR: #15606 Issue: #15605 Specification: https://github.com/jamesob/assumeutxo-docs/tree/master/proposal
This pulls out the routine for detection of how full the coins cache is from FlushStateToDisk. We use this logic independently when deciding when to flush the coins cache during UTXO snapshot activation (see here).
The following sections might be updated with supplementary metadata relevant to reviewers and maintainers.
Reviewers, this pull request conflicts with the following ones:
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.
2205@@ -2206,13 +2206,30 @@ bool CChainState::ConnectBlock(const CBlock& block, CValidationState& state, CBl
2206 return true;
2207 }
2208
2209+CoinsCacheSizeState CChainState::GetCoinsCacheSizeState()
2210+{
2211+ static int64_t nMempoolSizeMax = gArgs.GetArg("-maxmempool", DEFAULT_MAX_MEMPOOL_SIZE) * 1000000;
static here since there was no static previously, and in general this isn’t a pattern we use with other GetArg calls, and and it makes this code harder to test with different sizes, or allow updating the size without restarting.
2205@@ -2206,13 +2206,30 @@ bool CChainState::ConnectBlock(const CBlock& block, CValidationState& state, CBl
2206 return true;
2207 }
2208
2209+CoinsCacheSizeState CChainState::GetCoinsCacheSizeState()
2210+{
2211+ static int64_t nMempoolSizeMax = gArgs.GetArg("-maxmempool", DEFAULT_MAX_MEMPOOL_SIZE) * 1000000;
2212+ int64_t nMempoolUsage = mempool.DynamicMemoryUsage();
tx_pool argument instead of using the global in this function?
50+ BOOST_CHECK_EQUAL(
51+ chainstate.GetCoinsCacheSizeState(tx_pool, MAX_COINS_CACHE_BYTES, /*max_mempool_size_bytes*/ 0),
52+ CoinsCacheSizeState::OK);
53+
54+ print_view_mem_usage(view);
55+ BOOST_CHECK_EQUAL(view.DynamicMemoryUsage(), 32);
Looks like this fails:
0Running tests: validation_flush_tests from test/validation_flush_tests.cpp
1
2Running 1 test case...
3
4Test cases order is shuffled using seed: 1589090245
5
6Entering test module "Bitcoin Core Test Suite"
7
8test/validation_flush_tests.cpp(12): Entering test suite "validation_flush_tests"
9
10test/validation_flush_tests.cpp(19): Entering test case "getcoinscachesizestate"
11
12CCoinsViewCache memory usage: 16
13
14test/validation_flush_tests.cpp(55): error: in "validation_flush_tests/getcoinscachesizestate": check view.DynamicMemoryUsage() == 32 has failed [16 != 32]
101+ COutPoint res = add_coin(view);
102+ print_view_mem_usage(view);
103+
104+ BOOST_CHECK_EQUAL(
105+ chainstate.GetCoinsCacheSizeState(tx_pool, MAX_COINS_CACHE_BYTES, /*max_mempool_size_bytes*/ 0),
106+ CoinsCacheSizeState::CRITICAL);
0test/validation_flush_tests.cpp(106): error: in "validation_flush_tests/getcoinscachesizestate": check chainstate.GetCoinsCacheSizeState(tx_pool, MAX_COINS_CACHE_BYTES, 0) == CoinsCacheSizeState::CRITICAL has failed [1 != 2]
54+ // Without any coins in the cache, we shouldn't need to flush.
55+ BOOST_CHECK_EQUAL(
56+ chainstate.GetCoinsCacheSizeState(tx_pool, MAX_COINS_CACHE_BYTES, /*max_mempool_size_bytes*/ 0),
57+ CoinsCacheSizeState::OK);
58+
59+ // If the initial memory allocations of cacheCoins don't match these common
DynamicMemoryUsage() is just using sizeof internally, the test couldn’t take this into account using the same sizeof expressions. Would at least extend comment to explain the issue.
DynamicMemoryUsage() calculations explicitly because they’re not totally trivial and it seemed like something that’d be bad to copy/paste. To be honest I don’t understand why some 64-bit Travis platforms didn’t slot into these initial allocations and so I can’t think of what else to say here.
58+
59+ // If the initial memory allocations of cacheCoins don't match these common
60+ // cases, we can't really continue to make assertions about memory usage.
61+ // End the test early.
62+ if (view.DynamicMemoryUsage() != 32 && view.DynamicMemoryUsage() != 16) {
63+ // Add a bunch of coins to see that we at least flip over to CRITICAL.
It should probably report when these tests are skipped, which seems to be the case on native 32 bit machines.
Why not inspect the value of view.DynamicMemoryUsage() to infer how many coins you need to add to reach CRITICAL? Alternatively, why not just keep adding coins until you do reach CRITICAL (with some upper bound to prevent an infinite loop if there’s a serious bug)?
It should probably report when these tests are skipped
What are you thinking, BOOST_TEST_MESSAGE?
Why not inspect the value of view.DynamicMemoryUsage() to infer how many coins you need to add to reach CRITICAL?
I don’t think it’s that simple - different platforms/std::unordered_map implementations allocate differently, and so the current memory usage doesn’t tell us enough about how memory usage will change as individual coins are added.
Alternatively, why not just keep adding coins until you do reach CRITICAL (with some upper bound to prevent an infinite loop if there’s a serious bug)?
That’s basically what I do below: https://github.com/bitcoin/bitcoin/pull/16945/files#diff-1c02ab3ed49056a4df577f3db9b53d30R64-R71
2205@@ -2206,13 +2206,42 @@ bool CChainState::ConnectBlock(const CBlock& block, CValidationState& state, CBl
2206 return true;
2207 }
2208
2209+CoinsCacheSizeState CChainState::GetCoinsCacheSizeState(
2210+ const CTxMemPool& tx_pool,
2211+ size_t max_coins_cache_size_bytes,
2212+ int64_t max_mempool_size_bytes)
2213+{
2214+ if (max_coins_cache_size_bytes == -1) {
These magic -1 values do not seem like the best thing. They aren’t explicitly documented, assign a negative number to an unsigned type, add a runtime condition for something that is known at compile time, and cause the function to be linked against global variables that it doesn’t always need.
Would suggest using an overload instead of default values:
0CoinsCacheSizeState GetCoinsCacheSizeState(const CTxMemPool& tx_pool, size_t max_coins_cache_size_bytes, int64_t max_mempool_size_bytes);
1CoinsCacheSizeState GetCoinsCacheSizeState(const CTxMemPool& tx_pool) {
2 return GetCoinsCacheSizeState(tx_pool, ::nCoinCacheUsage, gArgs.GetArg("-maxmempool", DEFAULT_MAX_MEMPOOL_SIZE) * 1000000);
3}
728@@ -720,6 +729,17 @@ class CChainState {
729 /** Update the chain tip based on database information, i.e. CoinsTip()'s best block. */
730 bool LoadChainTip(const CChainParams& chainparams) EXCLUSIVE_LOCKS_REQUIRED(cs_main);
731
732+ //! Dictates whether we need to flush the cache to disk or not.
733+ //!
734+ //! @return the state of the size of the coins cache.
735+ CoinsCacheSizeState GetCoinsCacheSizeState(const CTxMemPool& tx_pool)
Even if mempool is passed as a const reference can’t we go further and pass only mempool memory usage ?
Also if wrapper is for test-only or convenience for its further usage inside PopulateAndValidateSnapshot add reason in function comment
2224+ int64_t nTotalSpace =
2225+ max_coins_cache_size_bytes + std::max<int64_t>(max_mempool_size_bytes - nMempoolUsage, 0);
2226+
2227+ //! No need to periodic flush if at least this much space still available.
2228+ static constexpr int64_t MAX_BLOCK_COINSDB_USAGE_BYTES = 10 * 1024 * 1024; // 10MB
2229+ int64_t large_threshold =
nLargeThreshold (naming conv shouldn’t be same with nTotalSpace?)
nTotalSpace?
total_space, cache_size and mempool_usage would make sense.
736+ EXCLUSIVE_LOCKS_REQUIRED(::cs_main);
737+
738+ CoinsCacheSizeState GetCoinsCacheSizeState(
739+ const CTxMemPool& tx_pool,
740+ size_t max_coins_cache_size_bytes,
741+ int64_t max_mempool_size_bytes) EXCLUSIVE_LOCKS_REQUIRED(::cs_main);
size_t instead of int64_t? DynamicUsage is already returning a size_t?
130+ // but not yet critical.
131+ add_coin(view);
132+ print_view_mem_usage(view);
133+
134+ // Only perform these checks on 64 bit hosts; I haven't done the math for 32.
135+ if (is_64_bit) {
Hmmm I spent some time trying to do the math for 32 but without a 32-bit platform it’s quite hard so I’ve tried to observe some allocation patterns thanks to print_view_mem_usage on 64-bit, but given there are allocation threshold effects on how std::unordered_map is behaving (common practices for allocators to multiply some magic numbers by 2 after being full) we can’t do hard assumptions…
Anyway I think this test is really dependent on how the standard library is implemented to get the right max_coins_cache_bytes/max_mempool_size_bytes.
Have you tried to run this part of the test as it is on 32-bit platform ?
CoinsCacheSizeState::LARGE is not hit.
113+ chainstate.GetCoinsCacheSizeState(tx_pool, MAX_COINS_CACHE_BYTES, /*max_mempool_size_bytes*/ 0),
114+ CoinsCacheSizeState::CRITICAL);
115+
116+ // Passing non-zero max mempool usage should allow us more headroom.
117+ BOOST_CHECK_EQUAL(
118+ chainstate.GetCoinsCacheSizeState(tx_pool, MAX_COINS_CACHE_BYTES, /*max_mempool_size_bytes*/ 1 << 10),
Code review ACK 028778a, tested validation_flush_tests on a 64-bit Linux platform.
Not sure about robustness of new test on all platforms…
I’ve revised the new unittest so that it passes on Travis. I don’t really follow the AppVeyor failure, so would appreciate any hints there.
Appveyor failure is a known issue and should be fixed by #17384 (comment)
int64_t with size_t argument (not actually sure why, since it’s called with an int64_t in the commit and only ever passed to std::max<int64_t>). Also: tweaking the
!is_64_bit test case to make it pass on travis. I lost track of the size calculations in the test a while ago, but they seem reasonable, and we can always scale back the test later if they turn out to be not stable.
Code review ACK 513981f.
Changes since last review: replacing int64_t with size_t argument (not actually sure why, since it’s called with an int64_t in the commit and only ever passed to std::max<int64_t>).
I think more variable types need to be updated in GetCoinsCacheSizeState to be size_t to make sense. IIRC, it’s better to use size_t as it let more flexibility to the compiler to optimize for the platform addresses size.
size_t change if reviewers want, but also not eager to invalidate ACKs.
Happy to make (or at least investigate) the
size_tchange if reviewers want, but also not eager to invalidate ACKs.
I’m not at all concerned about the size_t thing, so feel to just push for merge or more review
2223+ int64_t cacheSize = CoinsTip().DynamicMemoryUsage();
2224+ int64_t nTotalSpace =
2225+ max_coins_cache_size_bytes + std::max<int64_t>(max_mempool_size_bytes - nMempoolUsage, 0);
2226+
2227+ //! No need to periodic flush if at least this much space still available.
2228+ static constexpr int64_t MAX_BLOCK_COINSDB_USAGE_BYTES = 10 * 1024 * 1024; // 10MB
Happy to make (or at least investigate) the size_t change if reviewers want, but also not eager to invalidate ACKs.
Don’t bother for the size_t, not worth invalidating ACKs.
40+ coins_view.AddCoin(outp, std::move(newcoin), false);
41+
42+ return outp;
43+ };
44+
45+ constexpr int COIN_SIZE = is_64_bit ? 80 : 64;
528@@ -529,6 +529,15 @@ class CoinsViews {
529 void InitCache() EXCLUSIVE_LOCKS_REQUIRED(::cs_main);
530 };
531
532+enum class CoinsCacheSizeState
533+{
534+ //! The coins cache is in immediate need of a flush.
535+ CRITICAL = 2,
This separates out some logic for detecting how full the coins cache is from
FlushStateToDisk. We'll want to reuse this logic when deciding when to flush
the coins cache during UTXO snapshot activation.
Travis bloodbath :-(
Rebased to avoid carnage by fixing a silent merge conflict.
Saw this in IRC:
it’s discouraging to think that #16945 probably just would’ve sailed through review if I hadn’t bothered to add a unittest
I understand the sentiment, but I also do think the unit test is hard to understand, and I would not like to have to debug it in the future. I’m comfortable acking it now with the hope that maybe it is the type of thing that’s a little complicated to put in place, but once there will be reliable and useful. But if it started breaking in the future and the fix wasn’t obvious I could see wanting to drop it instead of update it.
The basic problem is that is that instead of being a test of the just
GetCoinsCacheSizeState function, it is a test of that function plus all the DynamicMemoryUsage functions which are very platform specific. I think a more ideal unit test would either mock or wrap the the DynamicMemoryUsage functions to be independent of the implementations of those functions.
The basic problem is that is that instead of being a test of the just GetCoinsCacheSizeState function, it is a test of that function plus all the DynamicMemoryUsage functions which are very platform specific. I think a more ideal unit test would either mock or wrap the the DynamicMemoryUsage functions to be independent of the implementations of those functions.
Personally I think it’s more desirable to have wider/incidental coverage (at the expense of lower specificity in terms of what failed) because the coins cache is a critically important part of the system. We should be alerted if e.g. the size of the coins cache entries changes, since that has pretty drastic effects on performance. Our unittest coverage is paltry as it stands, and when that’s the case I tend more towards adding integration- rather than strict unit-tests to get as much coverage out of what few tests we do have.
I’d much prefer to be hyper-aware of what’s going on with the coins cache and have to update a few tests once in a while than risk being unaware of even a small change to such an important part of the system. If there are ways I can make the test clearer or better, I’m all ears, but I am very hesitant to look to mocks - not only because that will probably require substantive changes to code purely for the sake of testing, but because for a system like Bitcoin it seems preferable have overzealous test coverage than the illusion of it thanks to a constellation of mocks.
There is an argument to be made that a test about when to flush the cache isn’t an appropriate place to make assertions about the size of its contents, but it was convenient and this stuff wasn’t tested so I figured better have it somewhere than not at all.
Code review ACK 02b9511.
Changes since 513981f, documenting better new test and printing an exit message in case of exotic archs.