revert: Revert "merge bitcoin-core/gui#835: Fix crash when closing wallet"

[UdjinM6]

Issue being fixed or feature implemented

This reverts commit 20763f1 from #6829.

The issue that was meant to be fixed by that backport does not exist in our codebase yet, see bitcoin-core/gui#835 (review). Even worse, backporting it too early created an issue where wallets would not be removed from gui while being removed in background.

What was done?

revert it

How Has This Been Tested?

open multiple wallets, try closing one of them.

develop: no visual changes, feels like "Close wallet" menu is not working but wallet is actually removed which can be confirmed by restarting qt
this PR/previous behaviour: wallet is actually gone from ui asap

Breaking Changes

n/a

Checklist:

• I have performed a self-review of my own code
• I have commented my code, particularly in hard-to-understand areas
• I have added or updated relevant unit/integration/functional/e2e tests
• I have made corresponding changes to the documentation
• I have assigned this pull request to a milestone (for repository code-owners and collaborators only)

[github-actions]

✅ No Merge Conflicts Detected

This PR currently has no conflicts with other open PRs.

[coderabbitai]

Walkthrough

The change removes the WalletController::removeWallet helper from both the header and implementation. closeWallet now directly calls wallet_model->wallet().remove() before invoking removeAndDeleteWallet(wallet_model). closeAllWallets iterates wallets, calls wallet_model->wallet().remove(), emits walletRemoved(wallet_model), deletes each wallet_model, and then clears m_wallets. All previous usages of removeWallet are replaced with the inlined removal and cleanup logic.

Estimated code review effort

🎯 2 (Simple) | ⏱️ ~10 minutes

Pre-merge checks and finishing touches

❌ Failed checks (1 warning)

Check name	Status	Explanation	Resolution
Docstring Coverage	⚠️ Warning	Docstring coverage is 0.00% which is insufficient. The required threshold is 80.00%.	You can run @coderabbitai generate docstrings to improve docstring coverage.

✅ Passed checks (2 passed)

Check name	Status	Explanation
Title Check	✅ Passed	The title clearly indicates that this pull request reverts the previous merge of bitcoin-core/gui#835 and directly summarizes the primary change being made in this branch.
Description Check	✅ Passed	The description directly explains that the commit from PR #6829 is being reverted, outlines why the backported fix was problematic, and details how the change was tested, making it fully relevant to the changeset.

✨ Finishing touches

• 📝 Generate docstrings

🧪 Generate unit tests (beta)

• Create PR with unit tests
• Post copyable unit tests in a comment

---

📜 Recent review details

Configuration used: CodeRabbit UI

Review profile: CHILL

Plan: Pro

📥 Commits

Reviewing files that changed from the base of the PR and between fb01c5a and 826221b.

📒 Files selected for processing (2)

• src/qt/walletcontroller.cpp (2 hunks)
• src/qt/walletcontroller.h (0 hunks)

💤 Files with no reviewable changes (1)

• src/qt/walletcontroller.h

🧰 Additional context used

📓 Path-based instructions (1)

src/**/*.{cpp,h,cc,cxx,hpp}

📄 CodeRabbit inference engine (CLAUDE.md)

src/**/*.{cpp,h,cc,cxx,hpp}: Dash Core C++ codebase must be written in C++20 and require at least Clang 16 or GCC 11.1
Dash uses unordered_lru_cache for efficient caching with LRU eviction

Files:

• src/qt/walletcontroller.cpp

🔇 Additional comments (2)

src/qt/walletcontroller.cpp (2)

91-94: LGTM! Correct removal order restored.

The inline removal of wallet_model->wallet().remove() before calling removeAndDeleteWallet ensures the wallet is removed from the node first, then the GUI model is cleaned up. This restores the immediate GUI update behavior that was broken by the upstream backport.

---

106-111: LGTM! Thread-safe batch removal implemented correctly.

The inlined removal logic correctly:

• Holds the mutex for the entire operation ensuring thread safety
• Removes each wallet from the node via wallet_model->wallet().remove()
• Emits walletRemoved signal before deletion (consistent with removeAndDeleteWallet)
• Safely iterates the vector (deleting pointed-to objects without modifying the container until clear())

The mutex-protected scope ensures consistency, though it does hold the lock during model destruction. This is acceptable for correctness and matches the revert's goal of immediate GUI updates.

Warning

Review ran into problems

🔥 Problems

Git: Failed to clone repository. Please run the @coderabbitai full review command to re-trigger a full review. If the issue persists, set path_filters to include or exclude specific files.

---

Thanks for using CodeRabbit! It's free for OSS, and your support helps us grow. If you like it, consider giving us a shout-out.

❤️ Share

• X
• Mastodon
• Reddit
• LinkedIn

_{Comment @coderabbitai help to get the list of available commands and usage tips.}

[knst]

LGTM 826221b

Backport gui-835 is marked as reverted in spreadsheet

[kwvg]

utACK 826221b