[Qt] support for persisted rpc console history

jonasschnelli commented at 8:53 pm on March 13, 2015: contributor

While developing i often use the rpc console for some quick checks. I always missed a persisted history.

This will add storing of the history.
importprivkey and signrawtransaction are blacklisted for the history (new: blacklisted also for the in-memory-history).
Last command will not get overwritten when browsing the history (shell like, but currently not possibility to store changed history commands till executing a new command)

gmaxwell commented at 9:05 pm on March 13, 2015: contributor

Cool. Can you blacklist walletpassphrase?

[Qt] support for persisted rpc console history

While developing i often use the rpc console for some quick checks. I always missed a persisted history.

* This will add storing of the history.
* "importprivkey" and "signrawtransaction" are blacklisted for the history (new: blacklisted also for the in-memory-history).
* Last command will not get overwritten when browsing the history (shell like, but currently not possibility to store changed history commands till executing a new command)

ead5585144

jonasschnelli force-pushed on Mar 13, 2015

jonasschnelli commented at 9:13 pm on March 13, 2015: contributor

@gmaxwell uh. How could i miss this! Just added sensitive wallet-crypto calls to the blacklist.

jonasschnelli commented at 9:15 pm on March 13, 2015: contributor

@paveljanik I did also add walletpassphrasechange and encryptwallet in the last update.

paveljanik commented at 9:18 pm on March 13, 2015: contributor

@jonasschnelli thanks. This is ultra-cool feature :+1:

luke-jr commented at 9:24 pm on March 13, 2015: member

Rather than hardcoding a list of filtered stuff, maybe it should go where the parameter conversion is defined? (ideally masking out only the sensitive parameter(s))

gmaxwell commented at 9:27 pm on March 13, 2015: contributor

Masking out specific parameters seems unduly complex.

jonasschnelli commented at 9:28 pm on March 13, 2015: contributor

@luke-jr yes. I also thought about this. Especially for signrawtransaction, there a history would def. make sense in case the user did not entre a priv-key as arg.

But i don’t want to hold this back until it ends up in the bin because it was getting to complicate. :) An additional PR could address more precise history storing.

luke-jr commented at 9:28 pm on March 13, 2015: member

Well, it could also be per-RPC and stored in the RPC info list rather than Qt code. OTOH, thinking on this further, it screws up modularity somewhat.. so maybe the list is for the better.

Nit: this isn’t a blacklist, so best to rename it to filter or something

jonasschnelli commented at 9:30 pm on March 13, 2015: contributor

@luke-jr s/historyBlacklist/historyFilter/?

luke-jr commented at 9:40 pm on March 13, 2015: member

Sure

paveljanik commented at 9:55 pm on March 13, 2015: contributor

ACK

[Qt] RPC console history: rename blacklist to filter d8a8f1b09d

sipa added the label GUI on Mar 16, 2015

Diapolo commented at 10:14 am on March 16, 2015: none

Not sure this is best implemented as QSetting, because on Windows this is written to the registry.

jonasschnelli commented at 10:17 am on March 16, 2015: contributor

@Diapolo IMO windows registry or OSX library/preferences is a good place for storing the history. As long as there is a filter for the security critical rpc calls (and this PR comes with a filter).

Diapolo commented at 10:19 am on March 16, 2015: none

Okay, but could we use a better naming for nRPCConsoleWindowHistory at least? IMHO the n was there for numbers, but that array doesn’t fit into n :).

jonasschnelli commented at 10:20 am on March 16, 2015: contributor

@Diapolo oh yes. This var-name is wrong. Will change it.

laanwj commented at 10:47 am on March 16, 2015: member

I tend towards NACK. Logging commands persistently is paramount to a privacy leak. At the very least this shouldn’t be enabled by default.

paveljanik commented at 5:36 pm on March 22, 2015: contributor

@laanwj Do you unset HISTFILE before or after running bitcoin-cli? But I’m not against adding an option to enable this (ie. disabled by default - like CoinControl).

luke-jr commented at 5:43 pm on March 22, 2015: member

We already log RPC commands in debug.log, I’m not sure this is that much different?

paveljanik commented at 5:59 pm on March 22, 2015: contributor

This logs arguments.

gmaxwell commented at 6:12 pm on March 22, 2015: contributor

@laanwj What if it were just per session only (e.g. not saving it to disk in any way)? I think that has 90% of the benefit and avoids 90% of the harm.

jonasschnelli commented at 7:45 pm on March 22, 2015: contributor

Current master keeps a history in memory (per session). This PR would extend the session/in-memory history to a persisted “kept-on-disk” history. Indeed, the place where the history is stored would be selected by Qt/OS and it would be outside of -datadir.

Maybe a conclusion could be to add a UI option to enable/disable this feature. I just think it (persisted history) is useful when working / developing / debugging so a per-default-disable option would be okay IMO.

gmaxwell commented at 9:16 pm on March 22, 2015: contributor

I don’t know that an option to disable it solves that much; the sequence of events that most likely results in harm is that the user has no idea that the data is being saved– so they wouldn’t think to go find it, or knows it is but doesn’t realize that later they’ll regret it (because their system is subsequently compromised).

laanwj commented at 8:22 am on March 24, 2015: member

There has been history per session ever since I added this console. The change here is to persist it between sessions. @paveljanik

Do you unset HISTFILE before or after running bitcoin-cli

Right. I don’t keep persistent shell history if an account is used for anything sensitive like managing a wallet.

Yes, the problem will be that people don’t expect what they enter to be saved to disk. I don’t think the slight extra convenience weighs up against this. We have good reason to be a bit paranoid.

We already log RPC commands in debug.log, I’m not sure this is that much different?

That’s a separate issue. Maybe we should be more prudent what is logged by default. ‘We’re already doing wrong thing!’ is not a valid reason to do more of it.

I would be somewhat more receptive to this if it as whitelist-based instead of blacklist-based. This would avoid sensitive new commands from being logged if someone forgets to update this file (which will happen). Then again, there is bound to be disagreement about what is safe and what is not. To me, it seems that this is not worth the extra complexity or worry. @jonasschnelli What kinds of commands would you want to repeat across sessions for “working / developing / debugging”? Is this use case not better solved with a script and JSON RPC?

jonasschnelli commented at 7:46 am on April 16, 2015: contributor

Yes. This feature is controversial and can harm your security (same as a bash/shell history).

I started writing this when i did some GUI tests where “createrawtransaction”, “signrawtransaction” was involved (and other chains of commands). So i had to write in again the tx-hex (copy&pase) every time i recompiled and restarted bitcoin-qt. There it was very handy to have a persistent history.

What about adding a settings flag as discussed (“persist rpc console history [yes|no]”)? Another thing which must be added here is a option to clear the persisted history. Maybe when a user presses the “clear” button ([X]) it should also clears the persisted history instead clearing only the sessions history.

jonasschnelli commented at 7:43 pm on June 19, 2015: contributor

Closing because this is controversial.

jonasschnelli closed this on Jun 19, 2015

luke-jr referenced this in commit 4dd907e9d6 on Jun 27, 2016

luke-jr referenced this in commit 08eb7da671 on Jun 27, 2016

MarcoFalke locked this on Sep 8, 2021

[Qt] support for persisted rpc console history #5891