Add loadwallet and createwallet load_on_startup options by ryanofsky · Pull Request #15937 · bitcoin/bitcoin

ryanofsky · 2019-05-01T21:50:12Z

This maintains a persistent list of wallets stored in settings that will automatically be loaded on startup. Being able to load a wallet automatically on startup will be more useful in the GUI, but it's reasonable to expose this feature by RPC as well.

DrahtBot · 2019-05-02T00:41:53Z

The following sections might be updated with supplementary metadata relevant to reviewers and maintainers.

Conflicts

Reviewers, this pull request conflicts with the following ones:

refactor: Move wallet methods out of chain.h and node.h #19099 (refactor: Move wallet methods out of chain.h and node.h by ryanofsky)
rpc: separate bumpfee's psbt creation function into psbtbumpfee #18654 (rpc: separate bumpfee's psbt creation function into psbtbumpfee by achow101)
Log RPC parameters (arguments) if -debug=rpcparams #16365 (Log RPC parameters (arguments) if -debug=rpcparams by LarryRuane)
Wallet passive startup #15719 (Wallet passive startup by ryanofsky)

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.

Persistent settings are used in followup PRs bitcoin#15936 to unify gui settings between bitcoin-qt and bitcoind, and bitcoin#15937 to add a load_on_startup flag to the loadwallet RPC and maintain a dynamic list of wallets that should be loaded on startup that also can be shared between bitcoind and bitcoin-qt.

jonasschnelli · 2019-05-18T10:23:02Z

Strong Concept ACK
"Loosing" the wallets especially in conjunction with pruning is a concept hard to understand.

promag

but it's reasonable to expose this feature by RPC as well

For GUI this makes perfect sense, for RPC I'm not sure if there's a use case that pays off.

Quickly reviewed just 9d5c780.

src/wallet/rpcwallet.cpp

083c954 Add settings_tests (Russell Yanofsky) 7f40528 Deduplicate settings merge code (Russell Yanofsky) 9dcb952 Add util::Settings struct and helper functions. (Russell Yanofsky) e2e37cf Remove includeconf nested scope (Russell Yanofsky) 5a84aa8 Rename includeconf variables for clarity (Russell Yanofsky) dc8e1e7 Clarify emptyIncludeConf logic (Russell Yanofsky) Pull request description: This is a refactoring-only change that makes it easier to add a new settings source. This PR doesn't change behavior. The [`util_ArgsMerge`](https://github.com/bitcoin/bitcoin/blob/deb2327b435925c6a39ca654a79283b8eb6aeb86/src/test/util_tests.cpp#L626-L822) and [`util_ChainMerge`](https://github.com/bitcoin/bitcoin/blob/deb2327b435925c6a39ca654a79283b8eb6aeb86/src/test/util_tests.cpp#L843-L924) tests added in #15869 and #15988 were written specifically to confirm that ArgsManager settings are parsed, merged, and returned the same way before and after this change. This change: - Makes it easier to add new settings sources that can get merged with existing sources (see 70675c3 from #15935). - Separates parsing of settings from merging of settings, and deduplicates merging code so it doesn't happen five different places ([GetArg](https://github.com/bitcoin/bitcoin/blob/c459c5f70176928adcee4935813a2dbe7f4dbd51/src/util/system.cpp#L221-L244), [GetNetBoolArg](https://github.com/bitcoin/bitcoin/blob/c459c5f70176928adcee4935813a2dbe7f4dbd51/src/util/system.cpp#L255-L261), [GetArgs](https://github.com/bitcoin/bitcoin/blob/c459c5f70176928adcee4935813a2dbe7f4dbd51/src/util/system.cpp#L460-L467), [IsArgNegated](https://github.com/bitcoin/bitcoin/blob/c459c5f70176928adcee4935813a2dbe7f4dbd51/src/util/system.cpp#L482-L491), [GetUnsuitableSectionOnlyArgs](https://github.com/bitcoin/bitcoin/blob/c459c5f70176928adcee4935813a2dbe7f4dbd51/src/util/system.cpp#L343-L352)) in inconsistent ways. - Documents and tests current strange merging behaviors, so they be cleaned up in the future if resulting code simplifications and UX improvements warrant loss of backwards compatibility. The newly documented behaviors are: command line [ignored arguments](https://github.com/ryanofsky/bitcoin/blob/69d44f3cc75a68d404ca0e1ca2b4831fd2bac4bb/src/util/system.cpp#L323-L326) and [more ignored arguments](https://github.com/ryanofsky/bitcoin/blob/69d44f3cc75a68d404ca0e1ca2b4831fd2bac4bb/src/util/settings.cpp#L67-L72), and config file [reverse precedence](https://github.com/ryanofsky/bitcoin/blob/69d44f3cc75a68d404ca0e1ca2b4831fd2bac4bb/src/util/settings.cpp#L61-L65), [inconsistently applied top-level settings](https://github.com/ryanofsky/bitcoin/blob/69d44f3cc75a68d404ca0e1ca2b4831fd2bac4bb/src/util/settings.cpp#L55-L59), and [zombie values](https://github.com/ryanofsky/bitcoin/blob/69d44f3cc75a68d404ca0e1ca2b4831fd2bac4bb/src/util/settings.cpp#L101-L108). The original motivation for this change was to make it easy to add a new persistent setting source without introducing more bugs and inconsistencies. Two commits building on top of this to add a persistent `-wallet` setting are pretty straightforward and show how the new code can be extended: * 70675c3 from #15935 – _Add \<datadir>/settings.json persistent settings storage_ * 04c80c4 from #15937 – _Add loadwallet and createwallet RPC load_on_startup options_ ACKs for top commit: ariard: ACK 083c954 jnewbery: ACK 083c954 jamesob: ACK 083c954 Tree-SHA512: 5d106746a44d64d3963c4ef3f4a2fa668a4bedcc9018d3ea12c86beae2fda48a0b036241665837f68685712366f70f2e1faba84d193fa1f456013503097b7659

083c954 Add settings_tests (Russell Yanofsky) 7f40528 Deduplicate settings merge code (Russell Yanofsky) 9dcb952 Add util::Settings struct and helper functions. (Russell Yanofsky) e2e37cf Remove includeconf nested scope (Russell Yanofsky) 5a84aa8 Rename includeconf variables for clarity (Russell Yanofsky) dc8e1e7 Clarify emptyIncludeConf logic (Russell Yanofsky) Pull request description: This is a refactoring-only change that makes it easier to add a new settings source. This PR doesn't change behavior. The [`util_ArgsMerge`](https://github.com/bitcoin/bitcoin/blob/deb2327b435925c6a39ca654a79283b8eb6aeb86/src/test/util_tests.cpp#L626-L822) and [`util_ChainMerge`](https://github.com/bitcoin/bitcoin/blob/deb2327b435925c6a39ca654a79283b8eb6aeb86/src/test/util_tests.cpp#L843-L924) tests added in bitcoin#15869 and bitcoin#15988 were written specifically to confirm that ArgsManager settings are parsed, merged, and returned the same way before and after this change. This change: - Makes it easier to add new settings sources that can get merged with existing sources (see 70675c3 from bitcoin#15935). - Separates parsing of settings from merging of settings, and deduplicates merging code so it doesn't happen five different places ([GetArg](https://github.com/bitcoin/bitcoin/blob/c459c5f70176928adcee4935813a2dbe7f4dbd51/src/util/system.cpp#L221-L244), [GetNetBoolArg](https://github.com/bitcoin/bitcoin/blob/c459c5f70176928adcee4935813a2dbe7f4dbd51/src/util/system.cpp#L255-L261), [GetArgs](https://github.com/bitcoin/bitcoin/blob/c459c5f70176928adcee4935813a2dbe7f4dbd51/src/util/system.cpp#L460-L467), [IsArgNegated](https://github.com/bitcoin/bitcoin/blob/c459c5f70176928adcee4935813a2dbe7f4dbd51/src/util/system.cpp#L482-L491), [GetUnsuitableSectionOnlyArgs](https://github.com/bitcoin/bitcoin/blob/c459c5f70176928adcee4935813a2dbe7f4dbd51/src/util/system.cpp#L343-L352)) in inconsistent ways. - Documents and tests current strange merging behaviors, so they be cleaned up in the future if resulting code simplifications and UX improvements warrant loss of backwards compatibility. The newly documented behaviors are: command line [ignored arguments](https://github.com/ryanofsky/bitcoin/blob/69d44f3cc75a68d404ca0e1ca2b4831fd2bac4bb/src/util/system.cpp#L323-L326) and [more ignored arguments](https://github.com/ryanofsky/bitcoin/blob/69d44f3cc75a68d404ca0e1ca2b4831fd2bac4bb/src/util/settings.cpp#L67-L72), and config file [reverse precedence](https://github.com/ryanofsky/bitcoin/blob/69d44f3cc75a68d404ca0e1ca2b4831fd2bac4bb/src/util/settings.cpp#L61-L65), [inconsistently applied top-level settings](https://github.com/ryanofsky/bitcoin/blob/69d44f3cc75a68d404ca0e1ca2b4831fd2bac4bb/src/util/settings.cpp#L55-L59), and [zombie values](https://github.com/ryanofsky/bitcoin/blob/69d44f3cc75a68d404ca0e1ca2b4831fd2bac4bb/src/util/settings.cpp#L101-L108). The original motivation for this change was to make it easy to add a new persistent setting source without introducing more bugs and inconsistencies. Two commits building on top of this to add a persistent `-wallet` setting are pretty straightforward and show how the new code can be extended: * 70675c3 from bitcoin#15935 – _Add \<datadir>/settings.json persistent settings storage_ * 04c80c4 from bitcoin#15937 – _Add loadwallet and createwallet RPC load_on_startup options_ ACKs for top commit: ariard: ACK 083c954 jnewbery: ACK 083c954 jamesob: ACK 083c954 Tree-SHA512: 5d106746a44d64d3963c4ef3f4a2fa668a4bedcc9018d3ea12c86beae2fda48a0b036241665837f68685712366f70f2e1faba84d193fa1f456013503097b7659

Persistent settings are used in followup PRs bitcoin#15936 to unify gui settings between bitcoin-qt and bitcoind, and bitcoin#15937 to add a load_on_startup flag to the loadwallet RPC and maintain a dynamic list of wallets that should be loaded on startup that also can be shared between bitcoind and bitcoin-qt.

luke-jr · 2020-01-29T03:32:32Z

For GUI this makes perfect sense, for RPC I'm not sure if there's a use case that pays off.

bitcoind also autoloads wallets as configured. (And it only really makes sense in general to use bitcoin_rw.conf for this...)

Persistent settings are used in followup PRs bitcoin#15936 to unify gui settings between bitcoin-qt and bitcoind, and bitcoin#15937 to add a load_on_startup flag to the loadwallet RPC and maintain a dynamic list of wallets that should be loaded on startup that also can be shared between bitcoind and bitcoin-qt.

jonatack · 2020-07-24T11:45:57Z

Concept ACK.

I would definitely find this useful and use it.

…storage 9c69cfe Add <datadir>/settings.json persistent settings storage. (Russell Yanofsky) eb682c5 util: Add ReadSettings and WriteSettings functions (Russell Yanofsky) Pull request description: Persistent settings are used in followup PRs bitcoin#15936 to unify gui settings between bitcoin-qt and bitcoind, and bitcoin#15937 to add a load_on_startup flag to the loadwallet RPC and maintain a dynamic list of wallets that should be loaded on startup that also can be shared between bitcoind and bitcoin-qt. ACKs for top commit: MarcoFalke: Approach re-ACK 9c69cfe 🌾 jnewbery: utACK 9c69cfe Tree-SHA512: 39fcc6051717117c9141e934de1d0d3f739484be4685cdf97d54de967c8c816502b4fd0de12114433beaa5c5b7060c810fd8ae4e2b3ce7c371eb729ac01ba2e1

meshcollider · 2020-08-02T00:12:39Z

Concept ACK

achow101

ACK 3062303

test/functional/wallet_startup.py

promag

Tested ACK 3062303. Played around and works as expected.

How would you see this option in the GUI? A checkbox somewhere?

Could also test with load_on_startup omitted.

achow101 · 2020-08-06T00:12:48Z

How would you see this option in the GUI? A checkbox somewhere?

For #15454, the GUI implementation is to just make this the default.

promag · 2020-08-07T22:08:49Z

@achow101 need to review #15454 again. Do you mean load_on_startup=true for menus create and load? If so, I think menu unload should set load_on_startup=false.

achow101 · 2020-08-10T16:29:32Z

Do you mean load_on_startup=true for menus create and load?

load_on_startup=true for loading, and load_on_startup=false on unloading.

This maintains a persistent list of wallets stored in settings that will automatically be loaded on startup. Being able to load a wallet automatically on startup will be more useful in the GUI when the option to create wallets is added in bitcoin#15006, but it's reasonable to expose this feature by RPC as well.

ryanofsky · 2020-08-13T18:23:57Z

Rebased 3062303 -> 904bc01 (pr/walset.15 -> pr/walset.16, compare) due to conflict with #18654
Updated 904bc01 -> 61709e8 (pr/walset.16 -> pr/walset.17, compare) with test suggestion

achow101 · 2020-08-13T18:35:44Z

re-ACK 61709e8. Only changes since last was rebase and test suggestion.

test/functional/wallet_startup.py

meshcollider

Code review ACK 61709e8

ryanofsky

Updated 61709e8 -> 642ad31 (pr/walset.17 -> pr/walset.18, compare) with test suggestion

test/functional/wallet_startup.py

meshcollider

re-utACK 642ad31

Only change is modification to test as suggested

achow101 · 2020-08-13T22:52:06Z

re-ACK 642ad31 Only change is the test

promag · 2020-08-15T00:30:33Z

Do you mean load_on_startup=true for menus create and load?

load_on_startup=true for loading, and load_on_startup=false on unloading.

Not sure if you interpret create as loading, but IMO created wallets should have load_on_startup=true too.

jonatack · 2020-08-16T08:22:29Z

Posthumous strong concept ACK -- looking forward to using this feature.

ryanofsky mentioned this pull request May 1, 2019

Add <datadir>/settings.json persistent settings storage #15935

Merged

DrahtBot added Build system GUI RPC/REST/ZMQ Tests Utils/log/libs Wallet labels May 1, 2019

ryanofsky force-pushed the pr/walset branch from cd80a09 to 17d54f5 Compare May 3, 2019 21:18

ryanofsky force-pushed the pr/walset branch from 17d54f5 to 5990d3e Compare May 7, 2019 22:09

ryanofsky mentioned this pull request May 8, 2019

Merge settings one place instead of five places #15934

Merged

ryanofsky force-pushed the pr/walset branch from 5990d3e to 04c80c4 Compare May 8, 2019 16:15

DrahtBot added the Needs rebase label May 16, 2019

ryanofsky force-pushed the pr/walset branch from 04c80c4 to 9d5c780 Compare June 8, 2019 15:38

DrahtBot removed the Needs rebase label Jun 8, 2019

promag reviewed Jun 25, 2019

View reviewed changes

src/wallet/rpcwallet.cpp Outdated Show resolved Hide resolved

src/wallet/rpcwallet.cpp Outdated Show resolved Hide resolved

ryanofsky force-pushed the pr/walset branch from 9d5c780 to 5ccdb04 Compare December 3, 2019 23:58

ryanofsky mentioned this pull request May 21, 2020

Close wallet in prunign mode #19039

Closed

ryanofsky force-pushed the pr/walset branch from 5ccdb04 to 9d5c780 Compare June 5, 2020 01:22

maflcko added the Utils/log/libs label Jul 23, 2020

DrahtBot removed the Needs rebase label Jul 23, 2020

ryanofsky force-pushed the pr/walset branch 2 times, most recently from 0b146a4 to 3062303 Compare July 23, 2020 17:55

ryanofsky mentioned this pull request Jul 23, 2020

test feature_backwards_compatibility fails with "Transaction not eligible for abandonment" #18161

Closed

achow101 reviewed Aug 5, 2020

View reviewed changes

test/functional/wallet_startup.py Outdated Show resolved Hide resolved

promag reviewed Aug 5, 2020

View reviewed changes

DrahtBot added the Needs rebase label Aug 13, 2020

meshcollider reviewed Aug 13, 2020

View reviewed changes

test/functional/wallet_startup.py Outdated Show resolved Hide resolved

meshcollider reviewed Aug 13, 2020

View reviewed changes

ryanofsky commented Aug 13, 2020

View reviewed changes

test/functional/wallet_startup.py Outdated Show resolved Hide resolved

meshcollider reviewed Aug 13, 2020

View reviewed changes

achow101 mentioned this pull request Aug 17, 2020

wallet, gui: Reload previously loaded wallets on startup #19754

Merged

ghost mentioned this pull request Aug 27, 2020

Dashboard (Load Wallet replacement) zkSNACKs/WasabiDesign#10

Closed

jsarenik mentioned this pull request Oct 1, 2020

pyln: Add a fixture for createwallet ElementsProject/lightning#4088

Merged

ryanofsky mentioned this pull request Oct 19, 2020

wallet: Make -wallet setting not create wallets #20186

Merged

hebasto mentioned this pull request Jan 4, 2021

duplicate wallet warning after closing and reopening wallet #20840

Open

Conversation

ryanofsky commented May 1, 2019 • edited Loading Uh oh! There was an error while loading. Please reload this page.

Uh oh!

Uh oh!

DrahtBot commented May 2, 2019 • edited Loading Uh oh! There was an error while loading. Please reload this page.

Uh oh!

Conflicts

Uh oh!

jonasschnelli commented May 18, 2019

Uh oh!

promag left a comment

Choose a reason for hiding this comment

Uh oh!

Uh oh!

Uh oh!

luke-jr commented Jan 29, 2020

Uh oh!

jonatack commented Jul 24, 2020

Uh oh!

meshcollider commented Aug 2, 2020

Uh oh!

achow101 left a comment

Choose a reason for hiding this comment

Uh oh!

Uh oh!

promag left a comment

Choose a reason for hiding this comment

Uh oh!

achow101 commented Aug 6, 2020

Uh oh!

promag commented Aug 7, 2020 • edited Loading Uh oh! There was an error while loading. Please reload this page.

Uh oh!

Uh oh!

achow101 commented Aug 10, 2020

Uh oh!

ryanofsky commented Aug 13, 2020

Uh oh!

achow101 commented Aug 13, 2020

Uh oh!

Uh oh!

meshcollider left a comment

Choose a reason for hiding this comment

Uh oh!

ryanofsky left a comment

Choose a reason for hiding this comment

Uh oh!

Uh oh!

meshcollider left a comment

Choose a reason for hiding this comment

Uh oh!

achow101 commented Aug 13, 2020

Uh oh!

promag commented Aug 15, 2020

Uh oh!

jonatack commented Aug 16, 2020

Uh oh!

Reviewers

Assignees

Labels

Projects

Milestone

Development

Uh oh!

9 participants

ryanofsky commented May 1, 2019 •

edited

Loading

DrahtBot commented May 2, 2019 •

edited

Loading

promag commented Aug 7, 2020 •

edited

Loading