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. A summary of reviews will appear here.
Reviewers, this pull request conflicts with the following ones:
P2WSH/P2TR/P2PK scripts inline by l0rinc)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.
🚧 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.
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
Hi! I’m a grad student working on a research project about using large language models to automate code review. Based on your commit 966bbabbd69039a2c7a03429c783f7d6e6a7c2a7 and the changes in src/test/validation_flush_tests.cpp, my tool generated this comment:
DynamicMemoryUsage() aligns with the assumptions made in this test.view.DynamicMemoryUsage() are essential to ensure that memory usage does not exceed expected limits. Consider adding assertions or logging to capture any unexpected memory usage patterns during the tests.view.Flush() operation. Ensure that there are checks after critical operations to handle potential failures gracefully.AddTestCoin function properly manages memory to avoid unbounded memory growth, especially since it is called in loops.constexpr with 8_MiB: Ensure that 8_MiB is defined correctly as a compile-time constant. If it is not defined, this will lead to a syntax error. Consider replacing it with a numeric literal, e.g., constexpr size_t MAX_COINS_BYTES{8 * 1024 * 1024};.view.DynamicMemoryUsage() in a variable to avoid redundant calculations.BOOST_TEST_MESSAGE calls marked with // TODO remove are removed or replaced with proper logging before finalizing the code.// PoolResource defaults to 256 KiB that will be allocated, so we'll take that and make it a bit larger. to explain why this is necessary.// TODO remove comments before finalizing the code to maintain a clean codebase.256, 1024, 3, 4, 1000) with named constants to improve readability and maintainability.MAX_COINS_CACHE_BYTES is replaced with MAX_COINS_BYTES. If MAX_COINS_CACHE_BYTES was the established naming convention, it would be better to retain it for clarity and consistency.MAX_COINS_BYTES, MAX_MEMPOOL_BYTES, and MAX_ATTEMPTS use an uppercase naming style with underscores, which is appropriate for constants. The identifier full_cap should be renamed to FULL_CAP for consistency.max_mempool_size_bytes could be shortened to max_mempool_bytes for better readability.i, such as attempt or index, especially in the context of for (size_t i{0}; i < MAX_ATTEMPTS && int64_t(view.DynamicMemoryUsage()) < large_cap; ++i).BOOST_TEST_MESSAGE: Reduce the frequency of BOOST_TEST_MESSAGE logging or use a more efficient logging mechanism that can be toggled based on a debug flag.MAX_COINS_BYTES and MAX_MEMPOOL_BYTES configurable or dynamically calculated based on the system’s available memory.As part of my research, I’m trying to understand how useful these comments are in real-world development. If you have a moment, I’d be super grateful if you could quickly reply to these two yes/no questions:
Does this comment provide suggestions from a dimension you hadn’t considered?
Do you find this comment helpful?
Thanks a lot for your time and feedback! And sorry again if this message is a bother.
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>
Labels
Tests