wallet: Reset chain notifications handler if AttachChain fails #29243

1. 
   achow101 commented at 1:13 am on January 13, 2024: member

   AttachChain will create the chain notifications handler which contains a reference to the wallet’s shared_ptr. If AttachChain fails, the wallet needs to be unloaded, and this is expected to happen with its custom deleter ReleaseWallet. However, if the chain notifications handler is still set, then the shared_ptr is still referenced by something, so the wallet is never actually released.

   This behavior can also be verified by looking at the debug.log file. When the wallet is released, the line “Releasing wallet” should appear in the debug.log file. However the failing test does not contain that line, indicating that the problem is that the CWallet object is not being destroyed. After this PR, that log line now appears, and the test also passes.

   Fixes #29234

2. wallet: Reset chain notifications handler if AttachChain fails

   AttachChain will create the chain notifications handler which contains a
   reference to the wallet's shared_ptr. If AttachChain fails, the wallet
   needs to be unloaded, and this is expected to happen with its custom
   deleter ReleaseWallet. However, if the chain notifications handler is
   still set, then the shared_ptr is still referenced by something, so the
   wallet is never actually released.

   ea2551e55d
3. 
   DrahtBot commented at 1:13 am on January 13, 2024: 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	jamesob, murchandamus, TheCharlatan, furszy, BrandonOdiwuor

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

4. DrahtBot added the label Wallet on Jan 13, 2024
5. 
   jamesob commented at 1:51 am on January 13, 2024: member

   Huh, any clue as to why this only becomes an issue on Windows?
6. 
   murchandamus commented at 1:53 am on January 13, 2024: contributor

   Any idea why this didn’t fail in the CI on #28838 before getting merged?
7. 
   achow101 commented at 1:54 am on January 13, 2024: member

   Huh, any clue as to why this only becomes an issue on Windows?

   Presumably only Windows is checking whether other things have the file open before attempting to remove them. Since the other shared_ptr reference was still hanging around in memory, bitcoind still had the database files open when RestoreWallet tries to delete the files for its cleanup on failure.

8. 
   achow101 commented at 1:59 am on January 13, 2024: member

   Any idea why this didn’t fail in the CI on #28838 before getting merged?

   Apparently we weren’t running functional tests in the Windows CI until recently: #29059

9. DrahtBot added the label CI failed on Jan 13, 2024
10. DrahtBot removed the label CI failed on Jan 13, 2024
11. 
    jamesob commented at 3:41 am on January 13, 2024: member

    ACK https://github.com/bitcoin/bitcoin/pull/29243/commits/ea2551e55d260854a5cca8aa95034970d4adca1c

    CI is happy.

12. 
    maflcko commented at 9:17 am on January 13, 2024: member

    Is this for backport?
13. 
    achow101 commented at 5:02 pm on January 13, 2024: member

    Is this for backport?

    This is a problem that goes back several major versions (it is possible to hit without assumeutxo), so probably not.

14. 
    murchandamus commented at 7:27 pm on January 13, 2024: contributor

    ACK ea2551e55d260854a5cca8aa95034970d4adca1c
15. TheCharlatan approved
16. 
    TheCharlatan commented at 9:28 pm on January 13, 2024: contributor

    ACK ea2551e55d260854a5cca8aa95034970d4adca1c
17. furszy approved
18. 
    furszy commented at 10:48 pm on January 13, 2024: member

    Code review ACK ea2551e5

    Plus, made a quick test replicating the behavior https://github.com/furszy/bitcoin-core/commit/b6aefde6bdb9bf52e43c54d98921d8da2f0a6b5f (it is not meant to be merged, just to trigger the failure locally). It fails without ea2551e5, and passes with it.

19. 
    hebasto commented at 8:06 pm on January 14, 2024: member

    I’ve rerun the “Win64 native” CI job just to assure that the “wallet_assumeutxo.py –descriptors” test passes again.
20. BrandonOdiwuor approved
21. 
    BrandonOdiwuor commented at 8:16 am on January 15, 2024: contributor

    Code Review ACK ea2551e55d260854a5cca8aa95034970d4adca1c
22. fanquake merged this on Jan 15, 2024
23. fanquake closed this on Jan 15, 2024

    ---

Contributors
achow101 DrahtBot jamesob murchandamus maflcko TheCharlatan furszy hebasto BrandonOdiwuor

Labels
Wallet