-
Notifications
You must be signed in to change notification settings - Fork 322
qt: add createwallet, createwalletdescriptor, and migratewallet to history filter #901
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
base: master
Are you sure you want to change the base?
qt: add createwallet, createwalletdescriptor, and migratewallet to history filter #901
Conversation
|
The following sections might be updated with supplementary metadata relevant to reviewers and maintainers. ReviewsSee the guideline for information on the review process.
If your review is incorrectly listed, please react with 👎 to this comment and the bot will ignore it on the next update. |
881a7a0 to
4e352ef
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.
utACK 4e352ef
(historyFilter was originally added in bitcoin/bitcoin#8877)
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.
lgtm, seems fine to add. Left a question
| // don't add private key handling cmd's to the history | ||
| const QStringList historyFilter = QStringList() | ||
| << "createwallet" | ||
| << "createwalletdescriptor" |
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.
why add this, but not importdescriptors?
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.
When I was testing this patch, I ran into some exceptions in importdescriptors related to
the internal label and the range label (see bitcoin repo #31514, #32376, #33614, #33655)
and the command line history was helpful for manually editing to get it to pass succesfully;
At that point, I used #882 to manually clear the command history.
I wasn't sure about the commands that require a "desc" (descriptor) and allowed private ones,
so I held off until I could do some manual testing.
Potential list:
-
Have
passphrase:
addedcreatewallet "wallet_name" ( disable_private_keys blank "passphrase" avoid_reuse descriptors load_on_startup external_signer )
addedencryptwallet "passphrase"
addedmigratewallet ( "wallet_name" "passphrase" )
addedwalletpassphrase "passphrase" timeout
addedwalletpassphrasechange "oldpassphrase" "newpassphrase -
Have
hdkey:
addedcreatewalletdescriptor "type" ( {"internal":bool,"hdkey":"str",...} ) -
Have
privatekeyorprivkey
addedsignrawtransactionwithkey "hexstring" ["privatekey",...] ( [{"txid":"hex","vout":n,"scriptPubKey":"hex","redeemScript":"hex","witnessScript":"hex","amount":amount},...] "sighashtype" )
addedsignmessagewithprivkey "privkey" "message"
To check which support or allow private descriptors:
- Have
descordescriptor:
deriveaddresses "descriptor" ( range )[accepts private descriptor]
getdescriptorinfo "descriptor"[accepts private descriptor]
descriptorprocesspsbt "psbt" ["",{"desc":"str","range":n or [n,n]},...] ( "sighashtype" bip32derivs finalize )[accepts private descriptor]
utxoupdatepsbt "psbt" ( ["",{"desc":"str","range":n or [n,n]},...] )[accepts private descriptor]
importdescriptors requestsdescriptor in requests [accepts private descriptor]
I also noticed that commands with more complex parameters or arguments (#907) can behave unexpectedly when repeated from the command line, at which point manually clearing the command history (#882) can be more practical for the user.
I'd be happy to add importdescriptors and other suggested privacy related commands.
Should all the commands accepting descriptor be added?
Added
createwallet,createwalletdescriptorandmigratewalletRPC commands to the Qt console history filter since they may include passphrases or other sensitive data that should not be stored in command history.