kernel, refactor: Replace goto with RAII in bitcoin-chainstate
#31362
Conversation
|
The following sections might be updated with supplementary metadata relevant to reviewers and maintainers. Code Coverage & BenchmarksFor details see: https://corecheck.dev/bitcoin/bitcoin/pulls/31362. ReviewsSee the guideline for information on the review process. ConflictsReviewers, 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. |
l0rinc
force-pushed
the
l0rinc/remove-goto
branch
from
fa3c3ee to
c4df9ee
Compare
|
🚧 At least one of the CI tasks failed. HintsTry to run the tests locally, according to the documentation. However, a CI failure may still
Leave a comment here, if you need help tracking down a confusing failure. |
…ility This commit splits the `bitcoin-chainstate` into a `.h` and `.cpp` pair to facilitate isolated testing. Additionally, inner classes were extracted to improve readability. This restructuring is a preparatory step for future improvements, such as replacing the goto statement with RAII for better error handling. Other minor adjustments include: * Marked `KernelNotifications` as `final`, aligning with `SubmitBlock_StateCatcher`. * Corrected whitespace for consistent formatting. * Standardized casing for consistency. * Updated included headers to reflect the refactored structure.
Added basic tests to verify that `bitcoin-chainstate`'s execution completes successfully in regtest and that cleanup code is invoked, even during failure scenarios. These tests confirm the correct functioning of the current goto-based cleanup mechanism - but full functional coverage was not within the scope of this commit. Key changes: * Replaced `std::cin`, `std::cout`, and `std::cerr` with configurable streams to enable testability. * Added a "Shutting down" message to the epilogue to validate that the cleanup path is executed. * Tests are only compiled and executed when `BUILD_UTIL_CHAINSTATE` is enabled (similarly to the `bitcoin-chainstate` script itself). * Introduced `util::Contains` to simplify and standardize substring checks, reducing verbosity in test assertions.
Replaced the use of a goto statement with RAII for cleanup in `bitcoin-chainstate`. The cleanup logic for `ValidationSignals` and `ChainstateManager` is now encapsulated in the Epilogue class destructor, ensuring that cleanup is always executed when the object goes out of scope. Key changes: * Introduced an Epilogue class to manage resource cleanup, eliminating the need for a goto statement. * Replaced every instance where errors are written to err to `return 1` immediately instead of relying on goto or break for flow control. For more context, see https://github.com/bitcoin/bitcoin/pull/24304/files#diff-04e685224f1ac5bfd91d47d8d7528a2e44f94fab5535d4b6b5af79b5a13aeb93R91
l0rinc
force-pushed
the
l0rinc/remove-goto
branch
from
c4df9ee to
5bafb60
Compare
|
There is some context on both the usage of In my kernel API PR I think I also agree with fanquake and maflcko in the original PR that tests are probably not useful in this context. Maybe a tiny functional test to ensure that it still runs correctly could be useful, but unit testing its functionality when it is still an experimental playground with a dead-simple interface seems counter productive. |
|
Closing in favor of #31382 |
3 participants
This PR replaces the
gotostatement inbitcoin-chainstateby encapsulating resources into a dedicatedEpilogueclass.I've bumped into it while reviewing #25722.
Key Changes:
ValidationSignalsandChainstateManagercleanup into an RAII Epilogue class to replace the goto-based cleanup logic.bitcoin-chainstatelogic into a header to enable isolated testing and clarity.How to test it manually:
cmake -B build -DCMAKE_BUILD_TYPE=Debug -DBUILD_UTIL_CHAINSTATE=OFF && cmake --build build -j$(nproc) && ctest --test-dir build -j$(nproc)- should not containbitcoin_chainstate_testscmake -B build -DCMAKE_BUILD_TYPE=Debug -DBUILD_UTIL_CHAINSTATE=ON && cmake --build build -j$(nproc) && ctest --test-dir build -j$(nproc)- should contain a passingbitcoin_chainstate_testscmake -B build -DCMAKE_BUILD_TYPE=Debug -DBUILD_UTIL_CHAINSTATE=ON && cmake --build build -j$(nproc) && ./build/src/bitcoin-chainstate your-bitcoin-datadir- should work the same as before (needs an extra newline at the end to exit)