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

[Qt] support for persisted rpc console history #5891

Closed
wants to merge 2 commits into
base: master
from

Conversation

Projects
None yet
7 participants
@jonasschnelli
Member

jonasschnelli commented Mar 13, 2015

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

This comment has been minimized.

Show comment
Hide comment
@gmaxwell

gmaxwell Mar 13, 2015

Member

Cool. Can you blacklist walletpassphrase?

Member

gmaxwell commented Mar 13, 2015

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)
@jonasschnelli

This comment has been minimized.

Show comment
Hide comment
@jonasschnelli

jonasschnelli Mar 13, 2015

Member

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

Member

jonasschnelli commented Mar 13, 2015

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

@jonasschnelli

This comment has been minimized.

Show comment
Hide comment
@jonasschnelli

jonasschnelli Mar 13, 2015

Member

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

Member

jonasschnelli commented Mar 13, 2015

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

@paveljanik

This comment has been minimized.

Show comment
Hide comment
@paveljanik

paveljanik Mar 13, 2015

Contributor

@jonasschnelli thanks. This is ultra-cool feature 👍

Contributor

paveljanik commented Mar 13, 2015

@jonasschnelli thanks. This is ultra-cool feature 👍

@luke-jr

This comment has been minimized.

Show comment
Hide comment
@luke-jr

luke-jr Mar 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))

Member

luke-jr commented Mar 13, 2015

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

This comment has been minimized.

Show comment
Hide comment
@gmaxwell

gmaxwell Mar 13, 2015

Member

Masking out specific parameters seems unduly complex.

Member

gmaxwell commented Mar 13, 2015

Masking out specific parameters seems unduly complex.

@jonasschnelli

This comment has been minimized.

Show comment
Hide comment
@jonasschnelli

jonasschnelli Mar 13, 2015

Member

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

Member

jonasschnelli commented Mar 13, 2015

@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

This comment has been minimized.

Show comment
Hide comment
@luke-jr

luke-jr Mar 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

Member

luke-jr commented Mar 13, 2015

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

This comment has been minimized.

Show comment
Hide comment
@jonasschnelli

jonasschnelli Mar 13, 2015

Member

@luke-jr s/historyBlacklist/historyFilter/?

Member

jonasschnelli commented Mar 13, 2015

@luke-jr s/historyBlacklist/historyFilter/?

@luke-jr

This comment has been minimized.

Show comment
Hide comment
@luke-jr

luke-jr Mar 13, 2015

Member

Sure

Member

luke-jr commented Mar 13, 2015

Sure

@paveljanik

This comment has been minimized.

Show comment
Hide comment
@paveljanik

paveljanik Mar 13, 2015

Contributor

ACK

Contributor

paveljanik commented Mar 13, 2015

ACK

@sipa sipa added the GUI label Mar 16, 2015

@Diapolo

This comment has been minimized.

Show comment
Hide comment
@Diapolo

Diapolo Mar 16, 2015

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

Diapolo commented Mar 16, 2015

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

@jonasschnelli

This comment has been minimized.

Show comment
Hide comment
@jonasschnelli

jonasschnelli Mar 16, 2015

Member

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

Member

jonasschnelli commented Mar 16, 2015

@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

This comment has been minimized.

Show comment
Hide comment
@Diapolo

Diapolo Mar 16, 2015

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

Diapolo commented Mar 16, 2015

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

This comment has been minimized.

Show comment
Hide comment
@jonasschnelli

jonasschnelli Mar 16, 2015

Member

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

Member

jonasschnelli commented Mar 16, 2015

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

@laanwj

This comment has been minimized.

Show comment
Hide comment
@laanwj

laanwj Mar 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.

Member

laanwj commented Mar 16, 2015

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

This comment has been minimized.

Show comment
Hide comment
@paveljanik

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

Contributor

paveljanik commented Mar 22, 2015

@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

This comment has been minimized.

Show comment
Hide comment
@luke-jr

luke-jr Mar 22, 2015

Member

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

Member

luke-jr commented Mar 22, 2015

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

@paveljanik

This comment has been minimized.

Show comment
Hide comment
@paveljanik

paveljanik Mar 22, 2015

Contributor

This logs arguments.

Contributor

paveljanik commented Mar 22, 2015

This logs arguments.

@gmaxwell

This comment has been minimized.

Show comment
Hide comment
@gmaxwell

gmaxwell Mar 22, 2015

Member

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

Member

gmaxwell commented Mar 22, 2015

@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

This comment has been minimized.

Show comment
Hide comment
@jonasschnelli

jonasschnelli Mar 22, 2015

Member

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.

Member

jonasschnelli commented Mar 22, 2015

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

This comment has been minimized.

Show comment
Hide comment
@gmaxwell

gmaxwell Mar 22, 2015

Member

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

Member

gmaxwell commented Mar 22, 2015

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

This comment has been minimized.

Show comment
Hide comment
@laanwj

laanwj Mar 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?

Member

laanwj commented Mar 24, 2015

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

This comment has been minimized.

Show comment
Hide comment
@jonasschnelli

jonasschnelli Apr 16, 2015

Member

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.

Member

jonasschnelli commented Apr 16, 2015

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

This comment has been minimized.

Show comment
Hide comment
@jonasschnelli

jonasschnelli Jun 19, 2015

Member

Closing because this is controversial.

Member

jonasschnelli commented Jun 19, 2015

Closing because this is controversial.

Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment