-
Notifications
You must be signed in to change notification settings - Fork 36.3k
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
wallet, gui: Reload previously loaded wallets on startup #19754
Conversation
da97453
to
e8bfa09
Compare
The following sections might be updated with supplementary metadata relevant to reviewers and maintainers. ConflictsReviewers, this pull request conflicts with the following ones:
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. |
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
Code review ACK e8bfa09
I think the PR title and description here are hard to follow and it would be better to just describe the behavior change that this is implementing: reloading wallets previously loaded in the GUI on startup. A good title might be "gui: Load previously loaded wallets on startup." Release notes can be updated too with this. Suggest maybe:
--- a/doc/release-notes-15937.md
+++ b/doc/release-notes-15937.md
@@ -1,12 +1,15 @@
Configuration
-------------
-The `createwallet`, `loadwallet`, and `unloadwallet` RPCs now accept
-`load_on_startup` options that modify bitcoin's dynamic configuration in
-`\<datadir\>/settings.json`, and can add or remove a wallet from the list of
-wallets automatically loaded at startup. Unless these options are explicitly
-set to true or false, the load on startup wallet list is not modified, so this
-change is backwards compatible.
+Wallets created or loaded in the GUI will now be automatically loaded on
+startup, so they don't need to be manually reloaded next time bitcoin is
+started. The list of wallets to load on startup is stored in
+`\<datadir\>/settings.json` and augments any command line or `bitcoin.conf`
+`-wallet=` settings that specify more wallets to load. Wallets that are
+unloaded in the GUI get removed from the settings list so they won't load again
+automatically next startup.
-In the future, the GUI will start updating the same startup wallet list as the
-RPCs to automatically reopen wallets previously opened in the GUI.
+The `createwallet`, `loadwallet`, and `unloadwallet` RPCs accept new
+`load_on_startup` options to modify the settings list. Unless these options are
+explicitly set to true or false, the list is not modified, so the RPC methods
+remain backward compatible.
Historical note: Commit e8bfa09 is an expanded version of 546bdce from #15454. New commit and old commit do the same thing but the new commit does extra refactoring in wallet code.
e8bfa09
to
4cff046
Compare
Done as suggested. |
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
Code review ACK 4cff046. Only doc changes since last review.
I might still try to make the PR description easier to understand. Not referencing #15937 in the first sentence for example, because you don't need to know about #15937 to understand this PR. The main thing I think this PR is doing is automatically loading previously loaded wallets automatically on startup so you don't need to load them manually. Rest is refactoring.
Also could tweak title
wallet: Reload previously loaded wallets on GUI startup
to
gui, wallet: Reload previously loaded wallets on startup
The "on GUI startup" part seems a little inaccurate because the wallets are loaded on bitcoind startup, too, not just bitcoin-qt startup.
Updated PR description and title. |
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
doc nits
Enable the GUI to also use the load_on_startup feature. Wallets loaded in the GUI always have load_on_startup=true. When they are unloaded from the GUI, load_on_startup=false. To facilitate this change, UpdateWalletSetting is moved into the wallet module and called from within LoadWallet, RemoveWallet, and Createwallet. This change does not actually touch the GUI code but rather the wallet functions that are shared between the GUI and RPC.
2125f6d
to
f1ee373
Compare
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
Code review ACK 952bf2c. Just rebased and tweaked release notes since last review
Tested but found a corner case that I think it should be fixed. If I manually move away a wallet then it will be created again. I think that |
I think that's a bit orthogonal to this PR. It's part of the settings reading stuff I think. I would prefer that to be fixed in a followup as I believe it is a current bug in the loading implementation. |
This is similar to what happens with the default wallet currently. If you move it away and restart, it will be created again. I don't think this PR really changes the situation: all it is doing is expanding the list of default wallets. I do agree it would be better not to create settings wallets on startup if they don't exist. It would be natural to do this in #15454 as part of the change that avoids creating the default wallet. I also think we could go further and not create wallets at all on startup. There are 4 sources of wallet names that can be created on startup:
Simplest and safest thing would be to just load wallets with these names and not create them |
Right right, didn't mean to fix it here. |
Tested ACK 952bf2c. |
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
ACK f1ee373, I have tested the code.
Tested ACK f1ee373 - works as expected. Wallets loaded via bitcoin-cli (in |
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
ACK f1ee373
… startup f1ee373 wallet: Reload previously loaded wallets on GUI startup (Andrew Chow) Pull request description: Enable the GUI to also use the load_on_startup feature. Wallets loaded in the GUI always have load_on_startup=true. When they are unloaded from the GUI, load_on_startup=false. To facilitate this change, UpdateWalletSetting is moved into the wallet module and called from within LoadWallet, RemoveWallet, and Createwallet. This change does not actually touch the GUI code but rather the wallet functions that are shared between the GUI and RPC. ACKs for top commit: jonasschnelli: Tested ACK f1ee373 - works as expected. Wallets loaded via bitcoin-cli (in `-server` mode) or through the RPC console won't be loaded on startup but wallets loaded via the GUI menu will. kristapsk: ACK f1ee373, I have tested the code. Tree-SHA512: f5b44aa763cf761d919015c5fbc0600b72434aa71e3b57007fd7530a29c3da1a9a0c98c4f22cb6cdffba61150a31170056a7d4737625e7b76f6958f3d584da8c
…t wallet d26f064 Tell users how to load or create a wallet when no wallet is loaded (Andrew Chow) 1bee1e6 Do not create default wallet (Andrew Chow) Pull request description: Instead of automatically creating and loading a default wallet, users should instead explicitly create their wallet or load it on start. Builds on #19754 which provides the `load_on_startup` behavior for the GUI. ACKs for top commit: jnewbery: Manual test and very light code review ACK d26f064 ryanofsky: Code review ACK d26f064. Just suggested changes to first commit (reusing MakeWalletDatabase and adding release notes), no changes to second commit jonatack: ACK d26f064 light code review, debug build, ran tests, did manual testing with testnet, rebased on master, on linux debian. Tree-SHA512: 091d785aef64736f7df661c576e815a87f3d029cfa32f3a75ba86fc25795f10b022ab3ae15c5b61a10b8cee16f5650f15cd79cbd6127e5e3ccbef631966d3c30
… default wallet d26f064 Tell users how to load or create a wallet when no wallet is loaded (Andrew Chow) 1bee1e6 Do not create default wallet (Andrew Chow) Pull request description: Instead of automatically creating and loading a default wallet, users should instead explicitly create their wallet or load it on start. Builds on bitcoin#19754 which provides the `load_on_startup` behavior for the GUI. ACKs for top commit: jnewbery: Manual test and very light code review ACK d26f064 ryanofsky: Code review ACK d26f064. Just suggested changes to first commit (reusing MakeWalletDatabase and adding release notes), no changes to second commit jonatack: ACK d26f064 light code review, debug build, ran tests, did manual testing with testnet, rebased on master, on linux debian. Tree-SHA512: 091d785aef64736f7df661c576e815a87f3d029cfa32f3a75ba86fc25795f10b022ab3ae15c5b61a10b8cee16f5650f15cd79cbd6127e5e3ccbef631966d3c30
This changes -wallet setting to only load existing wallets, not create new ones. - Fixes settings.json corner cases reported by sjors & promag: bitcoin-core/gui#95, bitcoin#19754 (comment), bitcoin#19754 (comment) - Prevents accidental creation of wallets reported most recently by jb55 http://www.erisian.com.au/bitcoin-core-dev/log-2020-09-14.html#l-355 - Simplifies behavior after bitcoin#15454. bitcoin#15454 took the big step of disabling creation of the default wallet. This PR extends it to avoid creating other wallets as well. With this change, new wallets just aren't created on startup, instead of sometimes being created, sometimes not. bitcoin#15454 release notes are updated here and are simpler. This change should be targetted for 0.21.0. It's a bug fix and simplifies behavior of the bitcoin#15937 / bitcoin#19754 / bitcoin#15454 features added in 0.21.0.
This changes -wallet setting to only load existing wallets, not create new ones. - Fixes settings.json corner cases reported by sjors & promag: bitcoin-core/gui#95, bitcoin#19754 (comment), bitcoin#19754 (comment) - Prevents accidental creation of wallets reported most recently by jb55 http://www.erisian.com.au/bitcoin-core-dev/log-2020-09-14.html#l-355 - Simplifies behavior after bitcoin#15454. bitcoin#15454 took the big step of disabling creation of the default wallet. This PR extends it to avoid creating other wallets as well. With this change, new wallets just aren't created on startup, instead of sometimes being created, sometimes not. bitcoin#15454 release notes are updated here and are simpler. This change should be targetted for 0.21.0. It's a bug fix and simplifies behavior of the bitcoin#15937 / bitcoin#19754 / bitcoin#15454 features added in 0.21.0.
This changes -wallet setting to only load existing wallets, not create new ones. - Fixes settings.json corner cases reported by sjors & promag: bitcoin-core/gui#95, bitcoin#19754 (comment), bitcoin#19754 (comment) - Prevents accidental creation of wallets reported most recently by jb55 http://www.erisian.com.au/bitcoin-core-dev/log-2020-09-14.html#l-355 - Simplifies behavior after bitcoin#15454. bitcoin#15454 took the big step of disabling creation of the default wallet. This PR extends it to avoid creating other wallets as well. With this change, new wallets just aren't created on startup, instead of sometimes being created, sometimes not. bitcoin#15454 release notes are updated here and are simpler. This change should be targetted for 0.21.0. It's a bug fix and simplifies behavior of the bitcoin#15937 / bitcoin#19754 / bitcoin#15454 features added in 0.21.0.
This changes -wallet setting to only load existing wallets, not create new ones. - Fixes settings.json corner cases reported by sjors & promag: bitcoin-core/gui#95, bitcoin#19754 (comment), bitcoin#19754 (comment) - Prevents accidental creation of wallets reported most recently by jb55 http://www.erisian.com.au/bitcoin-core-dev/log-2020-09-14.html#l-355 - Simplifies behavior after bitcoin#15454. bitcoin#15454 took the big step of disabling creation of the default wallet. This PR extends it to avoid creating other wallets as well. With this change, new wallets just aren't created on startup, instead of sometimes being created, sometimes not. bitcoin#15454 release notes are updated here and are simpler. This change should be targetted for 0.21.0. It's a bug fix and simplifies behavior of the bitcoin#15937 / bitcoin#19754 / bitcoin#15454 features added in 0.21.0.
This changes -wallet setting to only load existing wallets, not create new ones. - Fixes settings.json corner cases reported by sjors & promag: bitcoin-core/gui#95, bitcoin#19754 (comment), bitcoin#19754 (comment) - Prevents accidental creation of wallets reported most recently by jb55 http://www.erisian.com.au/bitcoin-core-dev/log-2020-09-14.html#l-355 - Simplifies behavior after bitcoin#15454. bitcoin#15454 took the big step of disabling creation of the default wallet. This PR extends it to avoid creating other wallets as well. With this change, new wallets just aren't created on startup, instead of sometimes being created, sometimes not. bitcoin#15454 release notes are updated here and are simpler. This change should be targeted for 0.21.0. It's a bug fix and simplifies behavior of the bitcoin#15937 / bitcoin#19754 / bitcoin#15454 features added in 0.21.0.
01476a8 wallet: Make -wallet setting not create wallets (Russell Yanofsky) Pull request description: This changes `-wallet` setting to only load existing wallets, not create new ones. - Fixes settings.json corner cases reported by sjors & promag: #95, bitcoin/bitcoin#19754 (comment), bitcoin/bitcoin#19754 (comment) - Prevents accidental creation of wallets reported most recently by jb55 http://www.erisian.com.au/bitcoin-core-dev/log-2020-09-14.html#l-355 - Simplifies behavior after #15454. #15454 took the big step of disabling creation of the default wallet. This PR extends it to avoid creating other wallets as well. With this change, new wallets just aren't created on startup, instead of sometimes being created, sometimes not. #15454 release notes are updated here and are simpler. This change should be targeted for 0.21.0. It's a bug fix and simplifies behavior of the #15937 / #19754 / #15454 features added in 0.21.0. --- This PR is implementing the simplest, most basic alternative listed in #95 (comment). Other improvements mentioned there can build on top of this. ACKs for top commit: achow101: ACK 01476a8 hebasto: re-ACK 01476a8 MarcoFalke: review ACK 01476a8 🏂 Tree-SHA512: 0d50f4e5dfbd04a2efd9fd66c02085a0ed705807bdec1cf5770d0ae8cb6af07080fb81306349937bf66acdb713d03fb35636f6442b650d0820e66cbae09c2f87
01476a8 wallet: Make -wallet setting not create wallets (Russell Yanofsky) Pull request description: This changes `-wallet` setting to only load existing wallets, not create new ones. - Fixes settings.json corner cases reported by sjors & promag: bitcoin-core/gui#95, bitcoin#19754 (comment), bitcoin#19754 (comment) - Prevents accidental creation of wallets reported most recently by jb55 http://www.erisian.com.au/bitcoin-core-dev/log-2020-09-14.html#l-355 - Simplifies behavior after bitcoin#15454. bitcoin#15454 took the big step of disabling creation of the default wallet. This PR extends it to avoid creating other wallets as well. With this change, new wallets just aren't created on startup, instead of sometimes being created, sometimes not. bitcoin#15454 release notes are updated here and are simpler. This change should be targeted for 0.21.0. It's a bug fix and simplifies behavior of the bitcoin#15937 / bitcoin#19754 / bitcoin#15454 features added in 0.21.0. --- This PR is implementing the simplest, most basic alternative listed in bitcoin-core/gui#95 (comment). Other improvements mentioned there can build on top of this. ACKs for top commit: achow101: ACK 01476a8 hebasto: re-ACK 01476a8 MarcoFalke: review ACK 01476a8 🏂 Tree-SHA512: 0d50f4e5dfbd04a2efd9fd66c02085a0ed705807bdec1cf5770d0ae8cb6af07080fb81306349937bf66acdb713d03fb35636f6442b650d0820e66cbae09c2f87
This changes -wallet setting to only load existing wallets, not create new ones. - Fixes settings.json corner cases reported by sjors & promag: bitcoin-core/gui#95, bitcoin/bitcoin#19754 (comment), bitcoin/bitcoin#19754 (comment) - Prevents accidental creation of wallets reported most recently by jb55 http://www.erisian.com.au/bitcoin-core-dev/log-2020-09-14.html#l-355 - Simplifies behavior after #15454. #15454 took the big step of disabling creation of the default wallet. This PR extends it to avoid creating other wallets as well. With this change, new wallets just aren't created on startup, instead of sometimes being created, sometimes not. #15454 release notes are updated here and are simpler. This change should be targeted for 0.21.0. It's a bug fix and simplifies behavior of the #15937 / #19754 / #15454 features added in 0.21.0.
Summary: > Enable the GUI to also use the load_on_startup feature. > Wallets loaded in the GUI always have load_on_startup=true. > When they are unloaded from the GUI, load_on_startup=false. > > To facilitate this change, UpdateWalletSetting is moved into the wallet > module and called from within LoadWallet, RemoveWallet, and > Createwallet. This change does not actually touch the GUI code but > rather the wallet functions that are shared between the GUI and RPC. This is a backport of [[bitcoin/bitcoin#19754 | core#19754]] Depends on D10185 Note: `std::optional` works a bit differently than core's `Optional`, so I had to account for a couple of changes in `wallet.cpp::UpdateWalletSetting`. See : - bitcoin/bitcoin@ebc4ab7: use `.value()` instead of `.get()` - fanquake/bitcoin@fa4435e#diff-1f2db0e4d5c12d109c7f0962333c245b49b696cb39ff432da048e9d6c08944d8: don't check for equality with `std::nullopt` Test Plan: `ninja all check-all` Reviewers: #bitcoin_abc, Fabien Reviewed By: #bitcoin_abc, Fabien Subscribers: Fabien Differential Revision: https://reviews.bitcoinabc.org/D10189
Enable the GUI to also use the load_on_startup feature. Wallets loaded in the GUI always have load_on_startup=true. When they are unloaded from the GUI, load_on_startup=false.
To facilitate this change, UpdateWalletSetting is moved into the wallet module and called from within LoadWallet, RemoveWallet, and Createwallet. This change does not actually touch the GUI code but rather the wallet functions that are shared between the GUI and RPC.