Skip to content

Fix handling of load wallet in controller #447

New issue

Have a question about this project? Sign up for a free GitHub account to open an issue and contact its maintainers and the community.

By clicking “Sign up for GitHub”, you agree to our terms of service and privacy statement. We’ll occasionally send you account related emails.

Already on GitHub? Sign in to your account

Merged
merged 1 commit into from
Mar 11, 2025

Conversation

johnny9
Copy link
Collaborator

@johnny9 johnny9 commented Mar 10, 2025

Don't load wallet if we already have them loaded.

There are 3 changes to the original code. In setSelectedWallet there is a proper check to see if there's already an instance of the wallet loaded in our m_wallets list and sets selected_wallet to that if true, It only loads the wallet into the list once (handleLoadWallet gets called when doing loadWallet), and it fixes the initialize to not call handlLoadWallet and emit selectedWalletChanged a bunch of times.

Previous implementation was loading the wallet twice
@hebasto
Copy link
Member

hebasto commented Mar 10, 2025

Don't load wallet if we already have them loaded.

What scenarios are you referring to?

@johnny9
Copy link
Collaborator Author

johnny9 commented Mar 10, 2025

Don't load wallet if we already have them loaded.

What scenarios are you referring to?

This is the QList member of the walletqmlcontroller. Previously it wasn't properly checking if we had the wallet in the list already before calling loadWallet and storing the unique_ptr in this list. You'll see the check at the top of setSelectedWallet now. The other change is that we don't use the unique_ptr after loadWallet, instead the handleLoadWallet method is used to store the resulting wallet. handleLoadWallet gets called anytime a wallet is loaded either from calling that interface method, rpc load, or at startup.

@johnny9 johnny9 changed the title qml: Fix handling of load wallet in controller Fix handling of load wallet in controller Mar 10, 2025
Copy link
Collaborator Author

@johnny9 johnny9 left a comment

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

Tested on Ubuntu desktop.

  1. With multiple wallets created and all of them unloaded, selected each wallet to test that it doesn't crash.
  2. With the wallets previously loaded, start the application and select each wallet in the select to test that it doesn't crash
  3. Started the application with a datadir that contains no wallets to validate initialization handles the empty case properly

@johnny9
Copy link
Collaborator Author

johnny9 commented Mar 10, 2025

@jarolrod

Copy link
Member

@jarolrod jarolrod left a comment

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

ACK 6ba6907

@hebasto hebasto merged commit a38cf5b into bitcoin-core:main Mar 11, 2025
9 checks passed
Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment
Labels
None yet
Projects
None yet
Development

Successfully merging this pull request may close these issues.

3 participants