wallet: Reset chain notifications handler if AttachChain fails

[achow101]

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

[DrahtBot]

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.

[jamesob]

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

[murchandamus]

Any idea why this didn’t fail in the CI on https://github.com/bitcoin/bitcoin/pull/28838 before getting merged?

[achow101]

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.

[achow101]

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

[jamesob]

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

CI is happy.

[maflcko]

Is this for backport?

[achow101]

Is this for backport?

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

[hebasto]

I've rerun the "Win64 native" CI job just to assure that the "wallet_assumeutxo.py --descriptors" test passes again.