Merge bitcoin-core/gui#765: Fix wallet list hover crash on shutdown#569
Open
DashCoreAutoGuix wants to merge 1 commit intobackport-0.23-batch-492from
Open
Merge bitcoin-core/gui#765: Fix wallet list hover crash on shutdown#569DashCoreAutoGuix wants to merge 1 commit intobackport-0.23-batch-492from
DashCoreAutoGuix wants to merge 1 commit intobackport-0.23-batch-492from
Conversation
|
Warning Rate limit exceeded@DashCoreAutoGuix has exceeded the limit for the number of commits or files that can be reviewed per hour. Please wait 22 minutes and 26 seconds before requesting another review. ⌛ How to resolve this issue?After the wait time has elapsed, a review can be triggered using the We recommend that you space out your commits to avoid hitting the rate limit. 🚦 How do rate limits work?CodeRabbit enforces hourly rate limits for each developer per organization. Our paid plans have higher rate limits than the trial, open-source and free plans. In all cases, we re-allow further reviews after a brief timeout. Please see our FAQ for further information. 📥 CommitsReviewing files that changed from the base of the PR and between ceed1d37127e356eaacf24a98058d11b669df061 and 1dc0ab2. 📒 Files selected for processing (1)
WalkthroughThe changes update several lambda signal-slot connections in Changes
Estimated code review effort🎯 2 (Simple) | ⏱️ ~8 minutes ✨ Finishing Touches
🧪 Generate unit tests
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. 🪧 TipsChatThere are 3 ways to chat with CodeRabbit:
SupportNeed help? Create a ticket on our support page for assistance with any issues or questions. Note: Be mindful of the bot's finite context window. It's strongly recommended to break down tasks such as reading entire modules into smaller chunks. For a focused discussion, use review comments to chat about specific files and their changes, instead of using the PR comments. CodeRabbit Commands (Invoked using PR comments)
Other keywords and placeholders
CodeRabbit Configuration File (
|
kwvg
suggested changes
Jul 28, 2025
kwvg
left a comment
There was a problem hiding this comment.
This pull request needs to be rebased on develop to resolve issues due to a logical conflict that has been resolved. This applies to both target and head branches.
Owner
Author
❌ Backport Verification - Issues DetectedOriginal Bitcoin commit: Issues found:
Fix identified but cannot be pushed automatically: Manual steps to fix: git rebase upstream/develop
git push origin HEAD --force-with-lease❌ Range-diff1: 7066e8996d ! 1: ceed1d3712 gui: provide wallet controller context to wallet actions
@@
## Metadata ##
-Author: furszy <matiasfurszyfer@protonmail.com>
+Author: Hennadii Stepanov <32963518+hebasto@users.noreply.github.com>
## Commit message ##
- gui: provide wallet controller context to wallet actions
+ Merge bitcoin-core/gui#765: Fix wallet list hover crash on shutdown
- Addressing potential crashes during shutdown. The most
- noticeable one can be triggered by hovering over the
- wallet list as the app shuts down.
+ 8b6470a90652fcffc45b8d7998af7c8ad6251332 gui: disable top bar menu actions during shutdown (furszy)
+ 7066e8996d0ac090535cc97cdcb54a219986460f gui: provide wallet controller context to wallet actions (furszy)
+
+ Pull request description:
+
+ Small follow-up to #751.
+
+ Fixes another crash cause during shutdown. Which occurs when the user hovers over the wallets list.
+
+ Future Note:
+ This surely happen in other places as well, we should re-work the way we connect signals. Register
+ lambas without any precaution can leave dangling pointers.
+
+ ACKs for top commit:
+ hebasto:
+ ACK 8b6470a90652fcffc45b8d7998af7c8ad6251332, I've tested each commit separately on macOS Sonoma 14.0 (Apple M1).
+
+ Tree-SHA512: 6fbd1bcd6717a8c1633beb9371463ed22422f929cccf9b791ee292c5364134c501e099329cf77a06b74a84c64c1c3d22539199ec49ccd74b3950036316c0dab3
## src/qt/bitcoingui.cpp ##
@@ src/qt/bitcoingui.cpp: void BitcoinGUI::createActions()
@@ src/qt/bitcoingui.cpp: void BitcoinGUI::createActions()
+ connect(m_close_wallet_action, &QAction::triggered, m_wallet_controller, [this] {
m_wallet_controller->closeWallet(walletFrame->currentWalletModel(), this);
});
- connect(m_create_wallet_action, &QAction::triggered, this, &BitcoinGUI::createWallet);
+- connect(m_create_wallet_action, &QAction::triggered, [this] {
++ connect(m_create_wallet_action, &QAction::triggered, m_wallet_controller, [this] {
+ auto activity = new CreateWalletActivity(m_wallet_controller, this);
+ connect(activity, &CreateWalletActivity::created, this, &BitcoinGUI::setCurrentWallet);
+ connect(activity, &CreateWalletActivity::created, rpcConsole, &RPCConsole::setCurrentWallet);
+ activity->create();
+ });
- connect(m_close_all_wallets_action, &QAction::triggered, [this] {
+ connect(m_close_all_wallets_action, &QAction::triggered, m_wallet_controller, [this] {
m_wallet_controller->closeAllWallets(this);
});
-- connect(m_migrate_wallet_action, &QAction::triggered, [this] {
-+ connect(m_migrate_wallet_action, &QAction::triggered, m_wallet_controller, [this] {
- auto activity = new MigrateWalletActivity(m_wallet_controller, this);
- connect(activity, &MigrateWalletActivity::migrated, this, &BitcoinGUI::setCurrentWallet);
- activity->migrate(walletFrame->currentWalletModel());
+-
+ connect(m_mask_values_action, &QAction::toggled, this, &BitcoinGUI::setPrivacy);
+ }
+ #endif // ENABLE_WALLET
+@@ src/qt/bitcoingui.cpp: void BitcoinGUI::setClientModel(ClientModel *_clientModel, interfaces::BlockAndH
+ connect(optionsModel, &OptionsModel::coinJoinEnabledChanged, this, &BitcoinGUI::updateCoinJoinVisibility);
+ }
+ } else {
+- if(trayIconMenu)
++ // Shutdown requested, disable menus
++ if (trayIconMenu)
+ {
+ // Disable context menu on tray icon
+ trayIconMenu->clear();
+@@ src/qt/bitcoingui.cpp: void BitcoinGUI::setClientModel(ClientModel *_clientModel, interfaces::BlockAndH
+ dockIconMenu->clear();
+ }
+ #endif
++ // Disable top bar menu actions
++ appMenuBar->clear();
+ }
+
+ updateCoinJoinVisibility();
2: 8b6470a906 < -: ---------- gui: disable top bar menu actions during shutdownTo reproduce, run: git --no-pager range-diff --creation-factor=99 92704535f6^..92704535f6 ceed1d3712^..ceed1d3712Please rebase the PR onto develop and the CI should pass. |
Owner
Author
❌ Backport Verification - Issues DetectedOriginal Bitcoin commit: Issues found:
Root cause: Bitcoin commit only modified Fix identified but couldn't push due to authentication: diff --git a/src/test/llmq_commitment_tests.cpp b/src/test/llmq_commitment_tests.cpp
index cfc193f427..0042368585 100644
--- a/src/test/llmq_commitment_tests.cpp
+++ b/src/test/llmq_commitment_tests.cpp
@@ -142,8 +142,8 @@ BOOST_AUTO_TEST_CASE(commitment_json_test)
BOOST_CHECK(json.exists("signersCount"));
BOOST_CHECK(json.exists("validMembersCount"));
- BOOST_CHECK_EQUAL(json["signersCount"].get_int(), commitment.CountSigners());
- BOOST_CHECK_EQUAL(json["validMembersCount"].get_int(), commitment.CountValidMembers());
+ BOOST_CHECK_EQUAL(json["signersCount"].getInt<int>(), commitment.CountSigners());
+ BOOST_CHECK_EQUAL(json["validMembersCount"].getInt<int>(), commitment.CountValidMembers());
}
BOOST_AUTO_TEST_CASE(commitment_bitvector_json_test)Manual fix instructions:
Please apply this fix and re-run verification. |
Owner
Author
❌ Backport Verification - Issues DetectedOriginal Bitcoin commit: Issues found:
Fix Applied (unable to push due to auth): diff --git a/src/test/llmq_commitment_tests.cpp b/src/test/llmq_commitment_tests.cpp
index cfc193f427..bdff2f9b16 100644
--- a/src/test/llmq_commitment_tests.cpp
+++ b/src/test/llmq_commitment_tests.cpp
@@ -142,8 +142,8 @@ BOOST_AUTO_TEST_CASE(commitment_json_test)
BOOST_CHECK(json.exists("signersCount"));
BOOST_CHECK(json.exists("validMembersCount"));
- BOOST_CHECK_EQUAL(json["signersCount"].get_int(), commitment.CountSigners());
- BOOST_CHECK_EQUAL(json["validMembersCount"].get_int(), commitment.CountValidMembers());
+ BOOST_CHECK_EQUAL(json["signersCount"].getInt(), commitment.CountSigners());
+ BOOST_CHECK_EQUAL(json["validMembersCount"].getInt(), commitment.CountValidMembers());
}
BOOST_AUTO_TEST_CASE(commitment_bitvector_json_test)Please apply this fix manually and re-run CI. The backport is otherwise valid with good size ratio (105.9%) and no other issues. |
8b6470a gui: disable top bar menu actions during shutdown (furszy) 7066e89 gui: provide wallet controller context to wallet actions (furszy) Pull request description: Small follow-up to #751. Fixes another crash cause during shutdown. Which occurs when the user hovers over the wallets list. Future Note: This surely happen in other places as well, we should re-work the way we connect signals. Register lambas without any precaution can leave dangling pointers. ACKs for top commit: hebasto: ACK 8b6470a, I've tested each commit separately on macOS Sonoma 14.0 (Apple M1). Tree-SHA512: 6fbd1bcd6717a8c1633beb9371463ed22422f929cccf9b791ee292c5364134c501e099329cf77a06b74a84c64c1c3d22539199ec49ccd74b3950036316c0dab3
ceed1d3 to
1dc0ab2
Compare
Owner
Author
✅ Backport Verification SuccessfulNo issues found! This backport is complete and faithful to the original Bitcoin change. Original Bitcoin commit: ✅ Range-diff1: 7066e8996d \! 1: 1dc0ab2ff5 gui: provide wallet controller context to wallet actions
@@
## Metadata ##
-Author: furszy <matiasfurszyfer@protonmail.com>
+Author: Hennadii Stepanov <32963518+hebasto@users.noreply.github.com>
## Commit message ##
- gui: provide wallet controller context to wallet actions
+ Merge bitcoin-core/gui#765: Fix wallet list hover crash on shutdown
- Addressing potential crashes during shutdown. The most
- noticeable one can be triggered by hovering over the
- wallet list as the app shuts down.
+ 8b6470a90652fcffc45b8d7998af7c8ad6251332 gui: disable top bar menu actions during shutdown (furszy)
+ 7066e8996d0ac090535cc97cdcb54a219986460f gui: provide wallet controller context to wallet actions (furszy)
+
+ Pull request description:
+
+ Small follow-up to #751.
+
+ Fixes another crash cause during shutdown. Which occurs when the user hovers over the wallets list.
+
+ Future Note:
+ This surely happen in other places as well, we should re-work the way we connect signals. Register
+ lambas without any precaution can leave dangling pointers.
+
+ ACKs for top commit:
+ hebasto:
+ ACK 8b6470a90652fcffc45b8d7998af7c8ad6251332, I've tested each commit separately on macOS Sonoma 14.0 (Apple M1).
+
+ Tree-SHA512: 6fbd1bcd6717a8c1633beb9371463ed22422f929cccf9b791ee292c5364134c501e099329cf77a06b74a84c64c1c3d22539199ec49ccd74b3950036316c0dab3
## src/qt/bitcoingui.cpp ##
@@ src/qt/bitcoingui.cpp: void BitcoinGUI::createActions()
@@ src/qt/bitcoingui.cpp: void BitcoinGUI::createActions()
+ connect(m_close_wallet_action, &QAction::triggered, m_wallet_controller, [this] {
m_wallet_controller->closeWallet(walletFrame->currentWalletModel(), this);
});
- connect(m_create_wallet_action, &QAction::triggered, this, &BitcoinGUI::createWallet);
+- connect(m_create_wallet_action, &QAction::triggered, [this] {
++ connect(m_create_wallet_action, &QAction::triggered, m_wallet_controller, [this] {
+ auto activity = new CreateWalletActivity(m_wallet_controller, this);
+ connect(activity, &CreateWalletActivity::created, this, &BitcoinGUI::setCurrentWallet);
+ connect(activity, &CreateWalletActivity::created, rpcConsole, &RPCConsole::setCurrentWallet);
+ activity->create();
+ });
- connect(m_close_all_wallets_action, &QAction::triggered, [this] {
+ connect(m_close_all_wallets_action, &QAction::triggered, m_wallet_controller, [this] {
m_wallet_controller->closeAllWallets(this);
});
+-
+ connect(m_mask_values_action, &QAction::toggled, this, &BitcoinGUI::setPrivacy);
+ }
+ #endif // ENABLE_WALLET
+@@ src/qt/bitcoingui.cpp: void BitcoinGUI::setClientModel(ClientModel *_clientModel, interfaces::BlockAndH
+ connect(optionsModel, &OptionsModel::coinJoinEnabledChanged, this, &BitcoinGUI::updateCoinJoinVisibility);
+ }
+ } else {
+- if(trayIconMenu)
++ // Shutdown requested, disable menus
++ if (trayIconMenu)
+ {
+ // Disable context menu on tray icon
+ trayIconMenu->clear();
+@@ src/qt/bitcoingui.cpp: void BitcoinGUI::setClientModel(ClientModel *_clientModel, interfaces::BlockAndH
+ dockIconMenu->clear();
+ }
+ #endif
++ // Disable top bar menu actions
++ appMenuBar->clear();
+ }
+
+ updateCoinJoinVisibility();
2: 8b6470a906 < -: ---------- gui: disable top bar menu actions during shutdownTo reproduce, run: git --no-pager range-diff --no-dual-color --creation-factor=99 7066e8996d0ac090535cc97cdcb54a219986460f^..8b6470a90652fcffc45b8d7998af7c8ad6251332 3b3169d47010b03f8f068152a93ae60433bf566b..origin/pr/569This PR is ready for merge. ✅ |
Owner
Author
✅ Verification CompleteBitcoin Source: bitcoin-core/gui#765 (commit SummaryThis PR correctly backports a fix for wallet list hover crash on shutdown. The fix adds a context parameter ( Validation Results
Dash-Specific Adaptations
Reviewer NoteThe review comment from @kwvg requesting rebase appears to be addressed - commit 3b3169d resolved the logical conflict before this PR was merged into the batch. Status: APPROVED ✅ |
This file contains hidden or bidirectional Unicode text that may be interpreted or compiled differently than what appears below. To review, open the file in an editor that reveals hidden Unicode characters.
Learn more about bidirectional Unicode characters
Sign up for free
to join this conversation on GitHub.
Already have an account?
Sign in to comment
4 participants
Add this suggestion to a batch that can be applied as a single commit.This suggestion is invalid because no changes were made to the code.Suggestions cannot be applied while the pull request is closed.Suggestions cannot be applied while viewing a subset of changes.Only one suggestion per line can be applied in a batch.Add this suggestion to a batch that can be applied as a single commit.Applying suggestions on deleted lines is not supported.You must change the existing code in this line in order to create a valid suggestion.Outdated suggestions cannot be applied.This suggestion has been applied or marked resolved.Suggestions cannot be applied from pending reviews.Suggestions cannot be applied on multi-line comments.Suggestions cannot be applied while the pull request is queued to merge.Suggestion cannot be applied right now. Please check back later.
$(cat <<'EOF'
Backports bitcoin/bitcoin-core/gui#765
Original commit: 9270453
Note: This PR was later reverted in bitcoin-core/gui#770
This PR fixes a crash that occurs when the user hovers over the wallets list during shutdown. The fix adds proper lifetime management by passing
m_wallet_controlleras context to lambdas that access wallet-related functionality, and clears the application menu bar during shutdown to prevent access to invalidated objects.Backported from Bitcoin Core v0.26
EOF
)
Summary by CodeRabbit
Refactor
Bug Fixes