kernel, refactor: Replace `goto` with RAII in `bitcoin-chainstate`

l0rinc commented at 3:42 pm on November 24, 2024: contributor

This PR replaces the goto statement in bitcoin-chainstate by encapsulating resources into a dedicated Epilogue class. I’ve bumped into it while reviewing #25722.

Key Changes:

Encapsulated ValidationSignals and ChainstateManager cleanup into an RAII Epilogue class to replace the goto-based cleanup logic.
Split bitcoin-chainstate logic into a header to enable isolated testing and clarity.
Introduced basic tests to ensure normal execution completes successfully in regtest, and cleanup code is called in both success and failure scenarios.

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 contain bitcoin_chainstate_tests
cmake -B build -DCMAKE_BUILD_TYPE=Debug -DBUILD_UTIL_CHAINSTATE=ON && cmake --build build -j$(nproc) && ctest --test-dir build -j$(nproc) - should contain a passing bitcoin_chainstate_tests
cmake -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)

DrahtBot commented at 3:42 pm on November 24, 2024: contributor

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

Code Coverage & Benchmarks

For details see: https://corecheck.dev/bitcoin/bitcoin/pulls/31362.

Reviews

See the guideline for information on the review process. A summary of reviews will appear here.

Conflicts

Reviewers, this pull request conflicts with the following ones:

#31382 (kernel: Flush in ChainstateManager destructor by TheCharlatan)
#31175 (rpc: Remove submitblock pre-checks by TheCharlatan)
#30965 (kernel: Move block tree db open to block manager by TheCharlatan)
#30901 (cmake: Revamp handling of data files for {test,bench}_bitcoin targets by hebasto)
#30610 (validation: do not wipe utxo cache for stats/scans/snapshots by sipa)
#28792 (Embed default ASMap as binary dump header file by fjahr)
#26022 (Add util::ResultPtr class by ryanofsky)
#25722 (refactor: Use util::Result class for wallet loading by ryanofsky)
#25665 (refactor: Add util::Result failure values, multiple error and warning messages by ryanofsky)

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 on Nov 24, 2024

DrahtBot added the label CI failed on Nov 24, 2024

DrahtBot commented at 3:51 pm on November 24, 2024: contributor

🚧 At least one of the CI tasks failed. Debug: https://github.com/bitcoin/bitcoin/runs/33443127689

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.

kernel, refactor: Split bitcoin-chainstate logic to header for testability

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.

d80006d68a

kernel, test: Add basic tests for bitcoin_chainstate

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.

e8f35f2565

kernel, refactor: Replace goto with RAII

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

5bafb60099

l0rinc force-pushed on Nov 24, 2024

DrahtBot removed the label CI failed on Nov 24, 2024

TheCharlatan commented at 12:07 pm on November 26, 2024: contributor

There is some context on both the usage of goto and tests in the original PR #24304 (review), and #24304 (comment) and #24304 (comment) respectively. If you look at aj’s original suggested solution for using RAII instead of goto you can see the progress that was made to strip down the amount of tear down code required for the kernel. I agree with the others in that original PR that for this experimental binary, a separate RAII wrapper is probably a bit much and gets in the way of having a dead-simple user of the kernel. Also having this ugly bit there instead of hiding it behind a wrapper could be motivational to come up with actual solutions.

In my kernel API PR bitcoin-chainstate is also superseded by a new binary using just the kernel API. The C++ wrapper header for the C header already implements all the required RAII wrappers, so there is no need to further abstract it: https://github.com/bitcoin/bitcoin/pull/30595/files#diff-bff8e101d19543c74d1d63f19e9b80c9e21f1f66e48c297356f39d9292cc18ef.

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.

l0rinc commented at 2:12 pm on November 27, 2024: contributor

Closing in favor of https://github.com/bitcoin/bitcoin/pull/31382

l0rinc closed this on Nov 27, 2024

l0rinc deleted the branch on Nov 27, 2024

kernel, refactor: Replace goto with RAII in bitcoin-chainstate #31362

Code Coverage & Benchmarks

Reviews

Conflicts

kernel, refactor: Replace `goto` with RAII in `bitcoin-chainstate` #31362