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

jnewbery commented at 5:03 pm on April 20, 2018: member

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~ Removed in #23123
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:

~externalhd (#9728)~ (PR closed without merging)
~disableprivatekeys (#9662)~ (PR changed to make disableprivatekeys a createwallet RPC argument)

Prior to V0.16:

~usehd (whether to make this an HD wallet)~ Removed in #14282

Wallet startup actions

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

~salvagewallet~ Removed in #18918
~upgradewallet~ Removed in #15761
~zapwallettxes~ Remoced in #19671

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

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.
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).
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.
Wallet module options should remain as command line args, except rescan, which should be removed as a command-line argument.

ryanofsky commented at 6:09 pm on April 20, 2018: member

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.

fanquake added the label Brainstorming on Apr 20, 2018

fanquake added the label Wallet on Apr 24, 2018

skeees commented at 3:59 pm on April 25, 2018: contributor

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 commented at 5:01 pm on April 25, 2018: member

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 commented at 11:17 am on December 15, 2018: member

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.

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).

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.

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 commented at 10:41 pm on February 6, 2019: member

Should maxTxFee be mentioned here as a per-wallet setting? #15288 (review) 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 commented at 11:07 pm on February 6, 2019: member

Thanks for noting -maxtxfee here @ryanofsky. My view (https://github.com/bitcoin/bitcoin/issues/15355#issuecomment-461218339) is that -maxtxfee should be a wallet option (perhaps renamed -maxwallettxfee?) Interested in hearing other people’s thoughts.

jonasschnelli commented at 6:22 am on February 7, 2019: contributor

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 commented at 10:23 pm on February 7, 2019: member

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

MarcoFalke commented at 4:39 pm on February 15, 2019: member

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 commented at 4:53 pm on February 15, 2019: member

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 commented at 8:45 am on February 16, 2019: member

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 referenced this in commit 656a15e539 on Mar 27, 2019

MarcoFalke commented at 5:47 pm on August 19, 2019: member

Wallet creation options Currently none

Can this be updated to include #15226 ? @jnewbery

jnewbery commented at 3:07 pm on August 20, 2019: member

Can this be updated to include #15226 ?

This issue is tracking command-line args. #15226 adds an argument to the createwallet RPC (which is where wallet creation options should be!) No new wallet creation command-line args have been added since this issue was opened.

I have updated the description to add maxtxfee and avoidpartialspends.

MarcoFalke commented at 3:11 pm on August 20, 2019: member

Then, why do you mention disableprivatekeys, which is also not a command-line option?

jnewbery commented at 5:21 pm on August 20, 2019: member

why do you mention disableprivatekeys

#9662 originally proposed -disableprivatekeys as a command-line argument. I’ll update the description again.

MarcoFalke commented at 12:40 pm on October 23, 2019: member

Wallet module options should remain as command line args.

Could you please update the OP to indicate that rescan is to be removed and not kept (as this sentence implies)?

jnewbery commented at 3:09 pm on October 23, 2019: member

@MarcoFalke

Could you please update the OP to indicate that rescan is to be removed and not kept (as this sentence implies)?

Done

MarcoFalke commented at 7:23 pm on April 24, 2020: member

-upgradewallet has been removed

jnewbery commented at 1:04 pm on May 27, 2020: member

-salvagewallet was removed in #18918

glozow commented at 9:49 am on May 17, 2021: member

#19671 removed -zapwallettxes :eyes:

MarcoFalke commented at 5:57 am on September 29, 2021: member

#23123 removes -rescan

vijaydasmp referenced this in commit d1e396828e on Oct 30, 2021

jnewbery closed this on Jun 20, 2022

laanwj commented at 11:20 am on June 20, 2022: member

I don’t think this is entirely completed yet?

MarcoFalke commented at 11:21 am on June 20, 2022: member

I think it is? If not, maybe open a new issue for the remaining stuff?

laanwj commented at 11:32 am on June 20, 2022: member

If it is, that’s great. I saw some non-striked-through items but didn’t check thoroughly.

jnewbery commented at 8:36 am on June 29, 2022: member

I don’t have the bandwidth to maintain this issue. There may be other options that could be changed - as @MarcoFalke says, new issues can be used for tracking those if necessary.