GetCoinsCacheSizeState switches from OK→LARGE→CRITICAL
#33021
After the changes in #25325 getcoinscachesizestate always ended the test early:
| File | Line Rate | Line Total | Line Hit | Branch Rate | Branch Total | Branch Hit |
|---|---|---|---|---|---|---|
| validation_flush_tests.cpp | 31.5 % | 54 | 17 | 22.3 % | 242 | 54 |
The test revival was extracted from a related PR where it was discovered.
The following sections might be updated with supplementary metadata relevant to reviewers and maintainers.
For details see: https://corecheck.dev/bitcoin/bitcoin/pulls/33021.
See the guideline for information on the review process.
| Type | Reviewers |
|---|---|
| ACK | LarryRuane, w0xlt, achow101 |
If your review is incorrectly listed, please react with 👎 to this comment and the bot will ignore it on the next update.
No conflicts as of last run.
🚧 At least one of the CI tasks failed.
Task previous releases, depends DEBUG: https://github.com/bitcoin/bitcoin/runs/46324900666
LLM reason (✨ experimental): The CI failure is caused by a compilation error due to an intentional narrowing conversion that is rejected as an error, breaking the build.
Try to run the tests locally, according to the documentation. However, a CI failure may still happen due to a number of reasons, for example:
Possibly due to a silent merge conflict (the changes in this pull request being incompatible with the current code in the target branch). If so, make sure to rebase on the latest commit of the target branch.
A sanitizer issue, which can only be found by compiling with the sanitizer and running the affected test.
An intermittent issue.
Leave a comment here, if you need help tracking down a confusing failure.
tACK b2d82668a57d4ab4eef310c25a2aebda18563677
I thoroughly reviewed and understand all code changes. This test perfectly tests GetCoinsCacheSizeState() and is much simpler and less fragile. (Also, I learned a few C++ tricks, very nice.)
I ran the modified test using the debugger (on Ubuntu), and it behaves as designed. The only very minor suggestion if you need to retouch is to consider adding a comment explaining why it’s not necessary to verify that the loops didn’t exceed MAX_ATTEMPTS (I had to think about that for a bit; the answer is that if we exceed the attempts, the subsequent state check will fail).
By the way, MAX_ATTEMPTS is defined well; the loop iterator never exceeds 35k.
Move-only commit, enabled reusing the large cache size calculation logic later. The only difference is the removal of the `static` keyword (since in a constexpr function it's a C++23 extension)
* parameter name uses underscores
* commit message typo fixed and compacted
* used `10_MiB` to avoid having to comment
* swapped order of operands in (9 * x / 10) to make it obvious that we're calculating 90%
* inlined return value
After the changes in https://github.com/bitcoin/bitcoin/pull/25325 `getcoinscachesizestate` always end the test early, see:
https://maflcko.github.io/b-c-cov/test_bitcoin.coverage/src/test/validation_flush_tests.cpp.gcov.html
The test revival was extracted from a related PR where it was discovered, see: https://github.com/bitcoin/bitcoin/pull/28531#discussion_r2109417797
Co-authored-by: Larry Ruane <larryruane@gmail.com>
504@@ -503,6 +505,14 @@ enum class CoinsCacheSizeState
505 OK = 0
506 };
507
508+constexpr int64_t LargeCoinsCacheThreshold(int64_t total_space) noexcept
509+{
510+ // No periodic flush needed if at least this much space is free
511+ constexpr int64_t MAX_BLOCK_COINSDB_USAGE_BYTES{int64_t(10_MiB)};
Brace initialization will convert size_t to int64_t
0 constexpr int64_t MAX_BLOCK_COINSDB_USAGE_BYTES{10_MiB};
ACK https://github.com/bitcoin/bitcoin/pull/33021/commits/554befd8738ea993b3b555e7366558a9c32c915c
src/test/validation_flush_tests.cpp also ends early in my setup.MAX_ATTEMPTS is good safeguard to keep CI from hanging due to initial map sizes (libc++/libstdc++/arch differences).