wallet: move lock at the top of ReleaseWallet #29155

1. 
   dimitaracev commented at 9:22 pm on December 29, 2023: contributor

   Fixes https://github.com/bitcoin/bitcoin/issues/29073
2. wallet: move lock at the top of ReleaseWallet c3d4463efe
3. 
   DrahtBot commented at 9:22 pm on December 29, 2023: 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. A summary of reviews will appear here.

   Conflicts

   Reviewers, this pull request conflicts with the following ones:

   • #25390 (sync: introduce a thread-safe generic container and use it to remove a bunch of “GlobalMutex"es by vasild)

   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.

4. DrahtBot added the label Wallet on Dec 29, 2023
5. in src/wallet/wallet.cpp:246 in c3d4463efe

   247+    bool wallet_already_marked_for_unloading = false;
   248     const std::string name = wallet->GetName();
   249     {
   250         LOCK(g_wallet_release_mutex);
   251         auto it = g_unloading_wallet_set.insert(name);
   252-        assert(it.second);
   

   ---

   ---

   

   furszy commented at 1:00 pm on December 30, 2023:

   If this would be part of the issue presented in #29073, the test error logs would be showing a node crash. It wouldn’t reach the got_loading_error check.

   ---

   

   dimitaracev commented at 6:53 pm on December 30, 2023:

   Correct, this was not the issue as initially thought. The removal of this is only so that we return an appropriate message if it ever comes to this instead of crashing the node. I can change it back if it matters. Not sure if it makes sense this to lead to a node crash to be honest.

   ---

   

   maflcko commented at 9:01 pm on December 30, 2023:

   I think it would be good to explain the changes, also, as mentioned previously, steps to reproduce the bug and the fix would be helpful.

   ---

   

   furszy commented at 1:16 pm on December 31, 2023:

   The removal of this is only so that we return an appropriate message if it ever comes to this instead of crashing the node. I can change it back if it matters. Not sure if it makes sense this to lead to a node crash to be honest.

   The current structure of the code disallows it from happening; prior to every UnloadWallet() call, we call RemoveWallet(). Which ensures that we never crash at this point (https://github.com/bitcoin/bitcoin/pull/29143#issuecomment-1869817009). So, if it crashes, the assertion failure makes sense because it means we are pushing more than one pointer of the same wallet into the WalletContext wallets vector. We could document this behavior but, as is a different topic, it should be treated separately. Either on a standalone commit or a separate PR.

   Also, agree with Marko that further explanations about the failure cause would be desirable. e.g. why moving the lock up to the top of ReleaseWallet fixes the problematic?

6. DrahtBot added the label CI failed on Jan 15, 2024
7. 
   maflcko commented at 6:07 pm on February 22, 2024: member

   Closing for now due to inactivity, and unclear status (why is this the correct fix?). However, a fix for this wallet crash is still needed and very much welcome.
8. maflcko closed this on Feb 22, 2024

   ---

Contributors
dimitaracev DrahtBot furszy maflcko

Labels
Wallet CI failed