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] add startup option to reset Qt settings #7006

Merged
merged 2 commits into from Nov 25, 2015

Conversation

Projects
None yet
4 participants
@jonasschnelli
Member

jonasschnelli commented Nov 13, 2015

Fixes #6749. can cure issues like #6749.

Setting a invalid Proxy (-proxy) or TorProxy (-onion) over the GUI settings panel will result in an InitError() (terminate the app) during the next app startup. Qt stores its settings in the windows registry and similar infrastructures and are therefore non-trivial to flush.

This PR adds a startup argument -resetguisettings which resets all GUI settings and reenable a smooth startup.
This does not affect the bitcoin.conf file (different settings layer).

I'm not entirely happy with this solution because some (most?) non expert users have problems starting bitcoin-qt with a command line argument because it involves opening a shell (cmd.exe, etc.).

@paveljanik

This comment has been minimized.

Show comment
Hide comment
@paveljanik

paveljanik Nov 13, 2015

Contributor

What kind of invalid settings is it? Do we save invalid values or values become invalid by time or by other reasons? Do you have an example?
If we can print "invalid proxy or something", we can also open the relevant dialog setting the proxy and allow the user to fix it. No?

Contributor

paveljanik commented Nov 13, 2015

What kind of invalid settings is it? Do we save invalid values or values become invalid by time or by other reasons? Do you have an example?
If we can print "invalid proxy or something", we can also open the relevant dialog setting the proxy and allow the user to fix it. No?

@paveljanik

This comment has been minimized.

Show comment
Hide comment
@paveljanik

paveljanik Nov 13, 2015

Contributor

Otherwise we have to document this for users so they can find it. But I think it is cleaner to prevent it to happen.

Contributor

paveljanik commented Nov 13, 2015

Otherwise we have to document this for users so they can find it. But I think it is cleaner to prevent it to happen.

@jonasschnelli jonasschnelli added the GUI label Nov 13, 2015

@jonasschnelli

This comment has been minimized.

Show comment
Hide comment
@jonasschnelli

jonasschnelli Nov 13, 2015

Member

I think having a way of resetting the qt settings is nice, although i agree with @paveljanik that it should not be possible to "save" a invalid proxy over the GUI settings panel.

You can test it yourself by setting a proxy in the GUI settings panel with the IP "0.0.0.0" (port doesn't matter).

Open the settings window and allow the user to change it, would be a invasive change to the startup process. I have though about a "reset setting" button in a error dialog during startup,... but that also feels wrong.

Member

jonasschnelli commented Nov 13, 2015

I think having a way of resetting the qt settings is nice, although i agree with @paveljanik that it should not be possible to "save" a invalid proxy over the GUI settings panel.

You can test it yourself by setting a proxy in the GUI settings panel with the IP "0.0.0.0" (port doesn't matter).

Open the settings window and allow the user to change it, would be a invasive change to the startup process. I have though about a "reset setting" button in a error dialog during startup,... but that also feels wrong.

@laanwj

This comment has been minimized.

Show comment
Hide comment
@laanwj

laanwj Nov 13, 2015

Member

What kind of invalid settings is it? Do we save invalid values or values become invalid by time or by other reasons? Do you have an example?

See #6749.
It's not supposed to happen, but it can happen that the settings in the QSettings are - somehow - unparseable or corrupted. Passing a command line option may be somewhat easier than directing the user to find the settings in regedit.

Member

laanwj commented Nov 13, 2015

What kind of invalid settings is it? Do we save invalid values or values become invalid by time or by other reasons? Do you have an example?

See #6749.
It's not supposed to happen, but it can happen that the settings in the QSettings are - somehow - unparseable or corrupted. Passing a command line option may be somewhat easier than directing the user to find the settings in regedit.

@paveljanik

This comment has been minimized.

Show comment
Hide comment
@paveljanik

paveljanik Nov 13, 2015

Contributor

I do not want the user to be redirected to regedit ;-) I think he should end up in our dialog for setting the "corrupted" proxy setting in this case.

But anyway, having this option is good anyway, lets document it :-)

Contributor

paveljanik commented Nov 13, 2015

I do not want the user to be redirected to regedit ;-) I think he should end up in our dialog for setting the "corrupted" proxy setting in this case.

But anyway, having this option is good anyway, lets document it :-)

@laanwj

This comment has been minimized.

Show comment
Hide comment
@laanwj

laanwj Nov 14, 2015

Member

Well yes, a corrupted proxy setting should result in a warning, not a fatal error.

But I suspect that's only one of the things that can go wrong. Having a 'reset to factory settings' can never hurt.

Member

laanwj commented Nov 14, 2015

Well yes, a corrupted proxy setting should result in a warning, not a fatal error.

But I suspect that's only one of the things that can go wrong. Having a 'reset to factory settings' can never hurt.

@laanwj

This comment has been minimized.

Show comment
Hide comment
@laanwj

laanwj Nov 14, 2015

Member

Well yes, a corrupted proxy setting should result in a warning, not a fatal error.

Thinking about this a bit I'm not sure. It makes sense to have this fatal. Or at least have it disable all networking.
Say you set up a proxy to hide your identify.
It happens to be detected as corrupted (for some reason).
Then the last thing you want is to disable it, one such leak can ruin everything.

Member

laanwj commented Nov 14, 2015

Well yes, a corrupted proxy setting should result in a warning, not a fatal error.

Thinking about this a bit I'm not sure. It makes sense to have this fatal. Or at least have it disable all networking.
Say you set up a proxy to hide your identify.
It happens to be detected as corrupted (for some reason).
Then the last thing you want is to disable it, one such leak can ruin everything.

@paveljanik

This comment has been minimized.

Show comment
Hide comment
@paveljanik

paveljanik Nov 14, 2015

Contributor

Yes, proxy setting is very sensible (this is why I asked for other examples, because I have heard - and read in #6749 - only about proxy setting so far).

Contributor

paveljanik commented Nov 14, 2015

Yes, proxy setting is very sensible (this is why I asked for other examples, because I have heard - and read in #6749 - only about proxy setting so far).

@jonasschnelli

This comment has been minimized.

Show comment
Hide comment
@jonasschnelli

jonasschnelli Nov 14, 2015

Member

I agree, the proxy setting is very sensible. A better approach would be to check the proxy when entering an IP in the QT settings panel. We shouldn't allow a invalid proxy so it would never lead to a startup error.

Member

jonasschnelli commented Nov 14, 2015

I agree, the proxy setting is very sensible. A better approach would be to check the proxy when entering an IP in the QT settings panel. We shouldn't allow a invalid proxy so it would never lead to a startup error.

@paveljanik

This comment has been minimized.

Show comment
Hide comment
@paveljanik

paveljanik Nov 14, 2015

Contributor

Jonas: the setting can be invalidated by time, network issues etc...

Contributor

paveljanik commented Nov 14, 2015

Jonas: the setting can be invalidated by time, network issues etc...

@paveljanik

This comment has been minimized.

Show comment
Hide comment
@paveljanik

paveljanik Nov 14, 2015

Contributor

Or it can even be an attack.

Contributor

paveljanik commented Nov 14, 2015

Or it can even be an attack.

@MarcoFalke

This comment has been minimized.

Show comment
Hide comment
@MarcoFalke

MarcoFalke Nov 14, 2015

Member

It makes sense to have this fatal.

There is more than just fatal and warning. Couldn't we do a prompt with something like

Problem with $[invalid qt setting]
Button("Stop Bitcoin Core")     Button("Reset $[invalid qt setting]")

?

Member

MarcoFalke commented Nov 14, 2015

It makes sense to have this fatal.

There is more than just fatal and warning. Couldn't we do a prompt with something like

Problem with $[invalid qt setting]
Button("Stop Bitcoin Core")     Button("Reset $[invalid qt setting]")

?

@jonasschnelli

This comment has been minimized.

Show comment
Hide comment
@jonasschnelli

jonasschnelli Nov 14, 2015

Member

Hmm.. the only check where a invalid proxy can lead to a app startup error & termination is CAddress.isValid() (https://github.com/bitcoin/bitcoin/blob/master/src/init.cpp#L1117). I think changes in the local network or somehow something that influents the local time can't raise a startup issue like #6749. A invalid proxy address can be detected and rejected shortly before storing to the QSettings persistent store.

An attack would be possible, although not much different to an attack to bitcoin.conf. Out of scope for this PR.

However, I think this PR is useful in case of invalid or corrupt Qt settings.

Member

jonasschnelli commented Nov 14, 2015

Hmm.. the only check where a invalid proxy can lead to a app startup error & termination is CAddress.isValid() (https://github.com/bitcoin/bitcoin/blob/master/src/init.cpp#L1117). I think changes in the local network or somehow something that influents the local time can't raise a startup issue like #6749. A invalid proxy address can be detected and rejected shortly before storing to the QSettings persistent store.

An attack would be possible, although not much different to an attack to bitcoin.conf. Out of scope for this PR.

However, I think this PR is useful in case of invalid or corrupt Qt settings.

@MarcoFalke

This comment has been minimized.

Show comment
Hide comment
@MarcoFalke

MarcoFalke Nov 14, 2015

Member

Yes, this PR is useful, but you shouldn't call it "Fixes #6749."

Member

MarcoFalke commented Nov 14, 2015

Yes, this PR is useful, but you shouldn't call it "Fixes #6749."

@jonasschnelli

This comment has been minimized.

Show comment
Hide comment
@jonasschnelli

jonasschnelli Nov 16, 2015

Member

Added a HelpMessageOpt for -resetguisettings.

Member

jonasschnelli commented Nov 16, 2015

Added a HelpMessageOpt for -resetguisettings.

@jonasschnelli

This comment has been minimized.

Show comment
Hide comment
@jonasschnelli

jonasschnelli Nov 16, 2015

Member

@MarcoFalke: right. It doesn't fix #6749, i'll just changes the PR description. Will work on a additional fix.

Member

jonasschnelli commented Nov 16, 2015

@MarcoFalke: right. It doesn't fix #6749, i'll just changes the PR description. Will work on a additional fix.

@paveljanik

This comment has been minimized.

Show comment
Hide comment
@paveljanik

paveljanik Nov 18, 2015

Contributor

-help looks like this now:

  -min
       Start minimized
...
  -resetguisettings
       Reset all settings changes made over the GUI (default: 0)

What about removing (default: 0) completely?

Contributor

paveljanik commented Nov 18, 2015

-help looks like this now:

  -min
       Start minimized
...
  -resetguisettings
       Reset all settings changes made over the GUI (default: 0)

What about removing (default: 0) completely?

@jonasschnelli

This comment has been minimized.

Show comment
Hide comment
@jonasschnelli

jonasschnelli Nov 18, 2015

Member

Yes. The default: 0 is somehow useless. Just removed it over a amend force push.

Member

jonasschnelli commented Nov 18, 2015

Yes. The default: 0 is somehow useless. Just removed it over a amend force push.

@paveljanik

This comment has been minimized.

Show comment
Hide comment
@paveljanik

paveljanik Nov 18, 2015

Contributor

ACK

Thanks!

Contributor

paveljanik commented Nov 18, 2015

ACK

Thanks!

@MarcoFalke

This comment has been minimized.

Show comment
Hide comment
@MarcoFalke

MarcoFalke Nov 19, 2015

Member

Concept ACK

Member

MarcoFalke commented Nov 19, 2015

Concept ACK

@MarcoFalke MarcoFalke referenced this pull request Nov 24, 2015

Closed

gui does not open #7077

@jonasschnelli jonasschnelli merged commit f71bfef into bitcoin:master Nov 25, 2015

1 check passed

continuous-integration/travis-ci/pr The Travis CI build passed
Details

jonasschnelli added a commit that referenced this pull request Nov 25, 2015

Merge pull request #7006
f71bfef add UI help for -resetguisettings (Jonas Schnelli)
ae98388 [Qt] add startup option to reset Qt settings (Jonas Schnelli)
Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment