refactor: introduce CChainState::GetCoinsCacheSizeState

[jamesob]

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).

[DrahtBot]

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

Conflicts

Reviewers, this pull request conflicts with the following ones:

• #17737 (Add ChainstateManager, remove BlockManager global by jamesob)
• #15606 ([experimental] UTXO snapshots 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.

[ryanofsky]

utACK e3ac4eb. Verified refactoring seems equivalent to current code

[jamesob]

I'll see about writing some tests as well.

[jamesob]

Thanks for the looks so far @ryanofsky @MarcoFalke. I've taken all proposed advice, as well as going further to parameterize the maximum coins cache and mempool target sizes. I've also added a unittest exercising this.

[jamesob]

Okay, after some fiddling the unittests are passing.

[ryanofsky]

utACK d780c6a. Changes since last review: adding test, getting rid of cached static size limit value. I left some suggestions, but think this is fine in it's current form.

[ryanofsky]

It's not clear from the comment when this is expected to happen, like whether it happens on a certain type of plaform. Also it's not clear how if 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.

[jamesob]

I wanted to avoid duplicating the 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.

[ryanofsky]

It might be clearer to move these fallback checks into a basic test and have the checks below be an extended test, running the basic test on all platforms and the extended test only on supported platforms. This would make the basic test more robust and easier to debug since it would run everywhere, and improve the extended test by making it shorter.

[jamesob]

I tried breaking the extended tests out but the two end up sharing so much data that it's a pain to pass it all into the extended function. If you really think this is worthwhile I can think harder about how to break it up cleanly but in the meantime I don't think it's too egregious to leave as-is.

[Sjors]

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)?

[jamesob]

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

[ryanofsky]

Code review ACK 028778a. Changes since last review were getting rid of special -1 defaults values, tweaking the 64-bit test check.

[ariard]

Code review ACK 028778a, tested validation_flush_tests on a 64-bit Linux platform.

Not sure about robustness of new test on all platforms...

[ariard]

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

[ariard]

nit: nLargeThreshold (naming conv shouldn't be same with nTotalSpace?)

[jamesob]

Snake case is preferred in new code per the styleguide (https://github.com/bitcoin/bitcoin/blob/master/doc/developer-notes.md#coding-style-c) -- I don't think naming new variables to match the naming convention of old ones is something we've asked for previously.

[ariard]

Ah thanks and so snaking case nTotalSpace?

[Sjors]

total_space, cache_size and mempool_usage would make sense.

[ariard]

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 ?

[mzumsande]

Just noting that the test fails on ARM (travis-log) because CoinsCacheSizeState::LARGE is not hit.

[jamesob]

Yeah I'm beginning to think that this test is a fool's errand. I might early-exit if the first map expansion doesn't match some pattern, but in general everything feels a little flaky here.

[ariard]

nit: could use MAX_MEMPOOL_SIZE_BYTES = 1024, I like bitwise but maybe not everyone :p

[jamesob]

I think this is fine as-is for tests.

[jamesob]

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.

[ryanofsky]

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)

[ryanofsky]

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>). 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.

[jamesob]

Thanks for the review, Russ.

replacing int64_t with size_t argument

I did this to appease @ariard without thinking too hard about it but you're right, I think it doesn't make too much sense. Can revert if preferable.

[ariard]

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.

[ryanofsky]

Status of this PR? I guess it needs a little more review. Hopefully it shouldn't take much. The code change is simple even though the test is complicated.

[jamesob]

Happy to make (or at least investigate) the size_t change if reviewers want, but also not eager to invalidate ACKs.

[jamesob]

I've pushed changes addressing @Sjors' feedback.

So far the ACK tally for the previous HEAD (and current HEAD, less comment changes) is

@promag - code review ACK
@Sjors - code review ACK
@ryanofsky - code review ACK
@ariard - code review ACK

[Sjors]

Travis bloodbath :-(

[jamesob]

Travis bloodbath :-(

Rebased to avoid carnage by fixing a silent merge conflict.

[ryanofsky]

Code review ACK 02b9511. Just rebase, new COIN_SIZE comment, and new test message since last review

[ryanofsky]

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.

[jamesob]

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.

[ryanofsky]

It seems like this has had enough review, but some of the previous reviews are out of date. @promag, @Sjors, @ariard maybe you could reack?

[ariard]

Code review ACK 02b9511.

Changes since 513981f, documenting better new test and printing an exit message in case of exotic archs.