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

[RFC] Long term plan for wallet command-line args #13044

Open
jnewbery opened this Issue Apr 20, 2018 · 11 comments

Comments

Projects
None yet
7 participants
@jnewbery
Copy link
Member

jnewbery commented Apr 20, 2018

The wallet command line arguments are a mixture of start-up actions, wallet creation options, updatable wallet parameters and wallet module modes. This is a proposal on how to rationalize the different types of arguments.

Current wallet arguments

  • -addresstype (What type of addresses to use)
  • -changetype (What type of change to use)
  • -disablewallet (Do not load the wallet and disable wallet RPC calls)
  • -discardfee=<amt> (The fee rate that indicates your tolerance for discarding change by adding it to the fee)
  • -fallbackfee=<amt> (A fee rate that will be used when fee estimation has insufficient data)
  • -keypool=<n> (Key pool size)
  • -mintxfee=<amt> (Fees smaller than this are considered zero fee for transaction creation)
  • -paytxfee=<amt> (Fee to add to transactions you send)
  • -rescan (Rescan the block chain for missing wallet transactions on startup));
  • -salvagewallet (Attempt to recover private keys from a corrupt wallet on startup));
  • -spendzeroconfchange (Spend unconfirmed change when sending transactions)
  • -txconfirmtarget=<n> (If paytxfee is not set, include enough fee so transactions begin confirmation on average within n blocks)
  • -upgradewallet (Upgrade wallet to latest format on startup));
  • -wallet=<path> (Specify wallet database path. Can be specified multiple times to load multiple wallets.)
  • -walletbroadcast (Make the wallet broadcast transactions))
  • -walletdir=<dir> (Specify directory to hold wallets)
  • -walletnotify=<cmd> (Execute command when a wallet transaction changes)
  • -walletrbf (Send transactions with full-RBF opt-in enabled)
  • -zapwallettxes=<mode> (Delete all wallet transactions and only recover those parts of the blockchain through -rescan on startup)
  • -dblogsize=<n> (DEBUG) (Flush wallet database activity from memory to disk log every megabytes)
  • -flushwallet (DEBUG) (Run a thread to flush wallet periodically)
  • -privdb (DEBUG) (Sets the DB_PRIVATE flag in the wallet db environment)
  • -walletrejectlongchains (DEBUG) (Wallet will not create transactions that violate mempool chain limits)

These can be categorised as follows:

Wallet module options

These act on the wallet software module, not on individual wallets:

  • disablewallet
  • rescan
  • wallet (included here because it tells the wallet module which individual wallets to load)
  • walletdir
  • walletnotify
  • dblogsize (DEBUG)
  • flushwallet (DEBUG)
  • privdb (DEBUG)
  • walletrejectlongchains (DEBUG)

Wallet creation options

Currently none, but these options have been proposed in PRs:

Prior to V0.16:

  • usehd (whether to make this an HD wallet)

Wallet startup actions

These act on an individual wallet at node startup (cannot currently be used in multiwallet mode)

  • salvagewallet
  • upgradewallet
  • zapwallettxes

Wallet config options

These are all currently treated as global wallet module options, but logically should be per-wallet config:

  • addresstype
  • changetype
  • keypool
  • discardfee (Changed to per-wallet config in #12909)
  • fallbackfee (Changed to per-wallet config in #12909)
  • mintxfee (Changed to per-wallet config in #12909)
  • paytxfee (Changed to per-wallet config in #12909)
  • spendzeroconfchange
  • txconfirmtarget
  • walletrbf

This is treated as per-wallet config, but there is currently no way to specify different config for different wallets at startup:

  • walletbroadcast

Proposed plan

  1. Wallet startup actions should be removed as command line args and added to an external wallet tool (#8745). We shouldn't need to spin up a full node to carry out these actions.
  2. Wallet creation options should not be added as command line args. These should be added as arguments to a new createwallet RPC (and as options in a new 'Create Wallet' dialog in qt).
  3. Wallet config options should be moved to be class members in CWallet, saved to/loaded from BDB wallet.dat, and updated via a walletsettings RPC and 'Wallet Settings` dialog in qt. Once that's in place, these should be removed as command line args.
  4. Wallet module options should remain as command line args.
@ryanofsky

This comment has been minimized.

Copy link
Contributor

ryanofsky commented Apr 20, 2018

Seems like a good classification and plan. "Wallet config options" seem to have a lot of overlap with CoinControl options, so maybe there should be well-defined correspondence or shared implementation there.

@skeees

This comment has been minimized.

Copy link
Member

skeees commented Apr 25, 2018

wow - that's a lot
now that theres a dedicated rescanblockchain rpc - might want to eliminate that cmd line option too - afaik there's no use case where a rescan must be initiated on startup and can't be initiated later via rpc?

@jnewbery

This comment has been minimized.

Copy link
Member Author

jnewbery commented Apr 25, 2018

now that theres a dedicated rescanblockchain rpc - might want to eliminate that cmd line option too

Yes - good point. We should remove the -rescan argument.

There's interaction between -rescan and -zapwallettxes/-salvagewallet (both of those options imply -rescan. It'd be (marginally) easier to remove -rescan once those two options have been removed.

@Sjors

This comment has been minimized.

Copy link
Member

Sjors commented Dec 15, 2018

  1. Wallet startup actions

Agree with moving these to a separate tool #13926. I'm assuming here that nobody uses these commands in an automated environment, so we're not breaking anyones setup by just removing this from bitcoind.

  1. Wallet creation options

Also agree that createwallet RPC should be used to create additional wallets, as well as for anyone who wants their default wallet to be different than the default (yikes, that's a bad sentence). I propose an additional creation option in #14938 (empty wallet).

  1. Wallet config options

We could add [wallet] specific sections in bitcoin.conf. Those could either override bitcoind arguments, or we could disallow these bitcoind arguments if any [wallet] section exists.

Once #11082 is merged, it should be easy to update wallet-specific settings in config files, rather than in the payload (though I'm not fully convinced storing settings in the payload is bad).

I do wonder if spendzeroconfchange and txconfirmtarget are really wallet specific things, or rather more of a mempool weather dependent global preference.

In general I'd really like to avoid having a given setting both global and with wallet specific overrides.

  1. Wallet module options

If we add descriptor support to wallets, and assuming new wallets won't use combo() descriptors, then -addresstype and -changetype could safely remain global (wallet module) settings. Their meaning would then decide which descriptor to use by default when creating a new wallet, and if a wallet has multiple address type descriptors, it could pick the default for getnewaddress (which would fail if the descriptor type isn't present in the wallet).

The keypool might disappear altogether, so that could remain a global option for pre-descriptor wallets.

@ryanofsky

This comment has been minimized.

Copy link
Contributor

ryanofsky commented Feb 6, 2019

Should maxTxFee be mentioned here as a per-wallet setting? #15288 (comment) seems to suggest that it should, but #15355 (comment) seems to say that maxTxFee should be a global setting that a wallet only needs to be able to query.

@jnewbery

This comment has been minimized.

Copy link
Member Author

jnewbery commented Feb 6, 2019

Thanks for noting -maxtxfee here @ryanofsky. My view (#15355 (comment)) is that -maxtxfee should be a wallet option (perhaps renamed -maxwallettxfee?) Interested in hearing other people's thoughts.

@jonasschnelli

This comment has been minimized.

Copy link
Member

jonasschnelli commented Feb 7, 2019

Great writeup @jnewbery!
I agree with the plan and do also agree with @skeees to remove -rescan (not sure if it can be before moving -salvage and co to the wallet tool).

@jnewbery

This comment has been minimized.

Copy link
Member Author

jnewbery commented Feb 7, 2019

Moving salvage and zaptxs to wallet-tool is not a hard prerequisite for removing -rescan. It could easily be done in parallel.

@MarcoFalke

This comment has been minimized.

Copy link
Member

MarcoFalke commented Feb 15, 2019

keypool is both a "wallet startup action" and "wallet config option". You are saying to move those to the RPCs (such as createwallet and walletsettings). However, the node (as well as the wallet-tool) will create a fresh wallet on startup and would thus fall back to the default keypool if the command line setting was removed.

@jnewbery

This comment has been minimized.

Copy link
Member Author

jnewbery commented Feb 15, 2019

However, the node (as well as the wallet-tool) will create a fresh wallet on startup

My preference would be for bitcoind to never create a wallet on startup. This has been suggested a few times on IRC, for example, here: http://www.erisian.com.au/bitcoin-core-dev/log-2018-12-20.html#l-243.

The wallet tool can easily be extended to take arguments for settings like keypool.

@Sjors

This comment has been minimized.

Copy link
Member

Sjors commented Feb 16, 2019

I agree about not creating a wallet on startup. For RPC users, if they call any command such as getnewaddress there could be an error message that's a little more instructive "Load an existing wallet or call createwallet". The GUI should still create a new wallet automatically, but can potentially defer that until the user requests their first address.

MarcoFalke added a commit to MarcoFalke/bitcoin that referenced this issue Mar 27, 2019

Merge bitcoin#15620: rpc: Uncouple non-wallet rpcs from maxTxFee global
fa1ad20 doc: Add release notes for 15620 (MarcoFalke)
fa96d76 rpc: Uncouple rpcs from maxTxFee global (MarcoFalke)
fa965e0 rpc: Use IsValidNumArgs over hardcoded size checks (MarcoFalke)

Pull request description:

  This makes the rpcs a bit more stateless by falling back to their own default max fee instead of the global maxTxFee.

  A follow up pull request will move `-maxtxfee` to the wallet.

  See also related discussions:

  * `-maxtxfee` should not be used by both node and wallet bitcoin#15355
  *  [RFC] Long term plan for wallet command-line args bitcoin#13044

ACKs for commit fa1ad2:
  jnewbery:
    utACK fa1ad20
  Empact:
    utACK bitcoin@fa1ad20
  jnewbery:
    utACK fa1ad20
  promag:
    utACK fa1ad20.

Tree-SHA512: c9cf0b54cd30ff3ab0d090b072cc38fcbb2840bc6ad9a9711995333bc927d2500aece6b5a60e061666eca5ed72b70aa318d21e51eb15ee0106b41f5b6e4e1adf
Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment
You can’t perform that action at this time.