Skip to content
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

WIP: Add loadwallet and createwallet load_on_startup options #15937

Draft
wants to merge 7 commits into
base: master
from

Conversation

@ryanofsky
Copy link
Contributor

ryanofsky commented May 1, 2019

This is based on #15934 + #15935. The non-base commits are:


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 #15006, but it's reasonable to expose this feature by RPC as well.


This is marked work-in-progress only because it depends on another work-in-progress PR.

@DrahtBot

This comment has been minimized.

Copy link
Contributor

DrahtBot commented May 2, 2019

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

Conflicts

Reviewers, this pull request conflicts with the following ones:

  • #16215 (gui: Refactor wallet controller activities by promag)
  • #16097 (WIP: Prevent meaningless negating of arguments by hebasto)
  • #16069 (test: move-only: Split large tests into smaller compile units by MarcoFalke)
  • #15934 (Separate settings merging from parsing by ryanofsky)
  • #15450 ([GUI] Create wallet menu option by achow101)
  • #14045 (refactor: Fix the chainparamsbase -> util circular dependency by Empact)
  • #13756 (wallet: "avoid_reuse" wallet flag for improved privacy by kallewoof)
  • #8994 (Testchains: Introduce custom chain whose constructor... by jtimon)

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.

ryanofsky added a commit to ryanofsky/bitcoin that referenced this pull request May 3, 2019
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.
@ryanofsky ryanofsky force-pushed the ryanofsky:pr/walset branch from cd80a09 to 17d54f5 May 3, 2019
ryanofsky added 5 commits Mar 5, 2019
Implement merging of settings from different sources (command line and config
file) separately from parsing code in system.cpp, so it is easier to add new
sources.

Document current inconsistent merging behavior without changing it.

This commit only adds new settings code without using it. The next commit calls
the new code to replace existing code in system.cpp.
Get rid of settings merging code mixed in settings parsing code in
util/system.cpp repeated 5 places, inconsistently:

- ArgsManagerHelper::GetArg
- ArgsManagerHelper::GetNetBoolArg
- ArgsManager::GetArgs
- ArgsManager::IsArgNegated
- ArgsManager::GetUnsuitableSectionOnlyArgs

Having settings merging code separated from parsing simplifies parsing somewhat
(for example negated values can simply be represented as false values instead
of partially cleared or emply placeholder lists).

Having settings merge happen one place instead of 5 makes it easier to add new
settings sources and harder to introduce new inconsistencies in the way
settings are merged.

This commit does not change behavior in any way.
Persistent settings are used in followup PRs #15936 to unify gui settings
between bitcoin-qt and bitcoind, and #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.
ryanofsky added a commit to ryanofsky/bitcoin that referenced this pull request May 7, 2019
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.
@ryanofsky ryanofsky force-pushed the ryanofsky:pr/walset branch from 17d54f5 to 5990d3e May 7, 2019
@ryanofsky ryanofsky force-pushed the ryanofsky:pr/walset branch from 5990d3e to 04c80c4 May 8, 2019
@jonasschnelli

This comment has been minimized.

Copy link
Member

jonasschnelli commented May 18, 2019

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

ryanofsky added 2 commits May 1, 2019
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 #15006, but it's reasonable to expose this feature by RPC as well.
@ryanofsky ryanofsky force-pushed the ryanofsky:pr/walset branch from 04c80c4 to 9d5c780 Jun 8, 2019
@DrahtBot DrahtBot removed the Needs rebase label Jun 8, 2019
Copy link
Member

promag left a comment

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.

@@ -2688,6 +2701,11 @@ static UniValue createwallet(const JSONRPCRequest& request)
flags |= WALLET_FLAG_BLANK_WALLET;
}

bool load_on_startup = false;
if (!request.params[4].isNull() && request.params[4].isBool()) {

This comment has been minimized.

Copy link
@promag

promag Jun 25, 2019

Member

Nit, remove && request.params[4].isBool()?

std::string error, warning;
std::shared_ptr<CWallet> const wallet = LoadWallet(*g_rpc_interfaces->chain, location, error, warning);
if (!wallet) throw JSONRPCError(RPC_WALLET_ERROR, error);

if (load_on_startup && !AddWalletSetting(wallet->chain(), location.GetName())) {
throw JSONRPCError(RPC_MISC_ERROR, "Wallet loaded, but load-on-startup list could not be written, so wallet "

This comment has been minimized.

Copy link
@promag

promag Jun 25, 2019

Member

An error is thrown but wallet remains loaded. Either:

  • make this a warning in the response;
  • move AddWalletSetting() before loading - which must be reverted if the actual load fails;
  • unload the wallet before throwing.
laanwj added a commit that referenced this pull request Nov 8, 2019
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
sidhujag added a commit to syscoin/syscoin that referenced this pull request Nov 9, 2019
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
Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment
Projects
None yet
4 participants
You can’t perform that action at this time.