Implement CCoinsViewErrorCatcher::HaveCoin and check disk space periodically #26331

1. 
   aureleoules commented at 9:29 am on October 18, 2022: member

   Attempt to fix #26112.

   As suggested by sipa in #26112 (comment):

   CCoinsViewErrorCatcher, the wrapper class used around CCoinsViewDB that’s supposed to detect these problems and forcefully exit the application, has an override for GetCoins. But in CheckTxInputs, HaveInputs is first invoked, which on its turn calls HaveCoin. HaveCoin is implemented in CCoinsViewDB, but not in CCoinsViewErrorCatcher, and thus the disk read exception escapes. A solution may be to just add an override for HaveCoin in CCoinsViewErrorCatcher.

   I implemented CCoinsViewErrorCatcher::HaveCoin and also added a periodic disk space check that shutdowns the node if there is not enough space left on disk, the minimum here is 50MB.

   For reviewers, it’s possible to saturate disk space to test the PR by creating large files with fallocate -l 50G test.bin

2. in src/coins.cpp:295 in 2c4d837242 outdated

   291@@ -292,11 +292,12 @@ const Coin& AccessByTxid(const CCoinsViewCache& view, const uint256& txid)
   292     return coinEmpty;
   293 }
   294 
   295-bool CCoinsViewErrorCatcher::GetCoin(const COutPoint &outpoint, Coin &coin) const {
   296+static bool ExecuteBackedWrapper(const std::function<bool()>& func, const std::vector<std::function<void()>>& err_callbacks)
   

   ---

   ---

   

   sipa commented at 2:09 pm on October 18, 2022:

   Would it be possible to use a template here?

   0template<typename Func>
   1static bool ExecuteBackedWrapper(Func func, const std::vector<std::function<void()>>& err_callbacks)
   2{
   

   That avoids a runtime function lookup indirection for calling func.

   ---

   

   aureleoules commented at 2:24 pm on October 18, 2022:

   Done, thanks!
3. aureleoules force-pushed on Oct 18, 2022
4. 
   DrahtBot commented at 2:30 pm on October 18, 2022: contributor

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

   Code Coverage

   For detailed information about the code coverage, see the test coverage report.

   Reviews

   See the guideline for information on the review process.

   Type	Reviewers
   ACK	w0xlt, sipa, achow101

   If your review is incorrectly listed, please react with 👎 to this comment and the bot will ignore it on the next update.

   Conflicts

   No conflicts as of last run.

5. Implement CCoinsViewErrorCatcher::HaveCoin 7fe537f7a4
6. Periodically check disk space to avoid corruption ed52e71176
7. in src/coins.cpp:319 in 1527570bf0 outdated

   314+    return ExecuteBackedWrapper([&]() { return CCoinsViewBacked::GetCoin(outpoint, coin); }, m_err_callbacks);
   315+}
   316+
   317+bool CCoinsViewErrorCatcher::HaveCoin(const COutPoint &outpoint) const {
   318+    return ExecuteBackedWrapper([&]() { return CCoinsViewBacked::HaveCoin(outpoint); }, m_err_callbacks);
   319+}
   

   ---

   ---

   

   sipa commented at 7:24 pm on October 18, 2022:

   Needs newline.

   ---

   

   aureleoules commented at 8:41 am on October 19, 2022:

   Done
8. aureleoules force-pushed on Oct 19, 2022
9. w0xlt approved
10. 
    w0xlt commented at 5:13 pm on October 31, 2022: contributor

    Code Review ACK https://github.com/bitcoin/bitcoin/pull/26331/commits/ed52e71176fc97c6ed01e3eebd85acdec54b4448
11. achow101 requested review from sipa on Apr 25, 2023
12. DrahtBot added the label CI failed on May 29, 2023
13. DrahtBot removed the label CI failed on May 31, 2023
14. achow101 requested review from TheCharlatan on Sep 20, 2023
15. achow101 requested review from josibake on Sep 20, 2023
16. achow101 requested review from furszy on Sep 20, 2023
17. DrahtBot removed review request from sipa on Sep 29, 2023
18. 
    sipa commented at 2:11 pm on September 29, 2023: member

    utACK ed52e71176fc97c6ed01e3eebd85acdec54b4448
19. fanquake added this to the milestone 26.0 on Sep 29, 2023
20. 
    fanquake commented at 4:09 pm on October 2, 2023: member

    @willcl-ark you might have some thoughts on writing some sort of test for this?
21. 
    achow101 commented at 6:36 pm on October 9, 2023: member

    ACK ed52e71176fc97c6ed01e3eebd85acdec54b4448
22. achow101 merged this on Oct 9, 2023
23. achow101 closed this on Oct 9, 2023

    ---

24. Frank-GER referenced this in commit 80f8443569 on Oct 13, 2023
25. 
    jonatack commented at 6:03 pm on November 14, 2023: contributor

    Post-merge concept ACK. Some kind of test coverage would be good.

Contributors
aureleoules sipa DrahtBot w0xlt fanquake achow101 jonatack

Review Requested
furszy josibake TheCharlatan

Milestone
26.0