Skip to content
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

RFC: Is it OK to reset unused prune and proxy settings to default values after restarting? #596

Closed
ryanofsky opened this issue Apr 29, 2022 · 7 comments
Labels

Comments

@ryanofsky
Copy link
Contributor

ryanofsky commented Apr 29, 2022

I'm opening this issue to get feedback and figure out how to resolve a disagreement that came up in bitcoin/bitcoin#15936 (comment).

Background

PR bitcoin/bitcoin#15936 unifies Bitcoin-Qt and Bitcoind settings so they are stored in <datadir>/settings.json instead of externally (in windows registry or external config directories via QSettings). It fixes current behavior where Bitcoind and Bitcoin-Qt can be started from the same datadir but may have inconsistent settings applied. It should make it easier to see what settings are in effect, because they will live in the datadir instead of being merged from outside sources. And it can enable new features like a bitcoin-cli config command that can update the unified settings dynamically.

The problem is the <datadir>/settings.json file does not currently store unused prune and proxy values when prune and proxy settings are disabled. So if you check the "Prune block storage" box and change the default "2GB" value to something else, then uncheck the box, then restart, then recheck the box, the previous value you typed will be lost, and the default "2GB" will be shown again. The same thing happens with the "Connect through SOCKS5 proxy" box. If you change the default tor proxy address "localhost:9050" to something else, and then disable the proxy, and restart, then recheck the box, the proxy address shown will be the default proxy address, not whatever proxy address you typed previously.

It is possible to change this behavior by storing unused proxy and prune settings somewhere. I don't personally think storing unused proxy or pruning settings values is a good thing or that it justifies extra complexity. But it would be possible to implement as a followup to bitcoin/bitcoin#15936. One approach I implemented in ryanofsky/bitcoin@4e86ab5 does this by adding prune-prev and proxy-prev settings that hold unused values when corresponding prune and proxy settings are disabled. Other approaches like storing unused prune and proxy settings in QSettings, or completely changing the representation of prune and proxy settings so they have "enabled" fields separate from the value fields would also be possible.

Questions

  1. What is ideal behavior when checking pruning/proxy boxes, typing pruning/proxy values, unchecking the boxes, restarting Bitcoin-Qt, checking the boxes again? Is it ideal to show default values (2GB and localhost:9050) when these boxes are checked? Or is ideal to show previous values? Or is some other behavior better?

  2. Is this issue important and should it block #15936? Would it be ok for #15936 to implement simplest possible behavior of not saving unused prune and proxy values to disk, and displaying default pruning and proxy values when the boxes are checked after a restart?

Screenshots

These are the settings in question for reference
Screenshot_20220429_100134
Screenshot_20220429_100104

@Sjors
Copy link
Member

Sjors commented Apr 29, 2022

It would be ideal to show the previously entered value. Especially for a (Tor) proxy it could be quite annoying if the IP and port are forgotten every time you turn them off.

However I don't know if turning proxies on and off is a common use case. It's not something I've encountered. So at this point I don't see it as a high priority or blocking issue.

Another point brought up in the thread:

I think it would be less surprising and more obvious what is going on if we clear the values after the user disables the option. Like, immediately after the click on the checkbox. Then the behavior will be consistent between "close & re-open the config dialog box" and "close & re-open the app"

IUC this will require modifying OptionsDialog as well as OptionsModel, and will make interactions between the classes more involved.

If we end up adding complexity, I'd rather just store the value.

@ryanofsky
Copy link
Contributor Author

I think it would be less surprising and more obvious what is going on if we clear the values after the user disables the option. Like, immediately after the click on the checkbox.

If we end up adding complexity, I'd rather just store the value.

Thanks, I didn't mention this alternative in the description, because I'm pretty sure nobody actually wants it. The question isn't really "should option values be immediately reset to default as soon as they are disabled?" but "is it worth preserving disabled option values across restarts, and if so, how should those disabled option values be stored on disk?"

@Rspigler
Copy link
Contributor

Rspigler commented May 2, 2022

I think the prune-prev and proxy-prev approach is best, especially because there is a reset button on that screen (so users will be able to return to default settings).

I do not believe this is blocking for #15936

@vasild
Copy link
Contributor

vasild commented May 3, 2022

Thanks for the input! Just a note - currently in master the values are preserved through restart when disabled.

@jarolrod
Copy link
Member

I'm for the prev values approach.

If a user chose a prune target in the gui, ran a pruned node, then for whatever reason disabled it; they still decided what prune target they would like, and we should display this value in the box for easy configuration if they would ever run in prune mode again. In terms of thinking about user-flows this situation should be rare, yet, it would be what the user expects. "Hey bitcoin gui, why are you making me choose my prune target every time, I told you I want it to prune to 10gb if I'm running in pruned mode."

Following the same logic, the proxy settings provide a stronger case for restoring these values. While prune targets would rarely change, I'd assume it's an even rarer occurrence to change your proxy settings. And there are situations where you would cycle it off and on, especially if you're running the gui on a laptop, traveling and using different networks.

@ryanofsky
Copy link
Contributor Author

ryanofsky commented Oct 24, 2022

I'm for the prev values approach.

All right, it sounds like most people prefer storing disabled values over resetting them, and PR #603 implements the change that's been asked for, so it can be reviewed and hopefully merged.

I'll close this issue, since the question it raises "RFC: Is it OK to reset unused prune and proxy settings to default values after restarting?" seems to have a clear answer now: "No". It can be reopened if there is need for more discussion.

@hebasto
Copy link
Member

hebasto commented Oct 26, 2022

While reviewing #603, I've made an opinion that

Other approaches like storing unused prune and proxy settings in QSettings

looks nicer, as all those settings are GUI-specific.

hebasto added a commit that referenced this issue Feb 15, 2023
…ings

9d3127b Add settings.json prune-prev, proxy-prev, onion-prev settings (Ryan Ofsky)

Pull request description:

  With #602, if proxy and pruning settings are disabled in the GUI and the GUI is restarted, proxy and prune values are not stored anywhere. So if these settings are enabled in the future, default values will be shown, not previous values.

  This PR stores previous values so they will preserved across restarts. I'm not sure I like this behavior because showing default values seems simpler and safer to me. Previous values may just have been set temporarily and may have never actually worked, and it adds some code complexity to store them.

  This PR is one way of resolving #596. Other solutions are possible and could be implemented as alternatives.

ACKs for top commit:
  hebasto:
    ACK 9d3127b, tested on Ubuntu 22.04.
  vasild:
    ACK 9d3127b
  jarolrod:
    tACK 9d3127b

Tree-SHA512: 1778d1819443490c880cfd5c1711d9c5ac75ea3ee8440e2f0ced81d293247163a78ae8aba6027215110aec6533bd7dc6472aeead6796bfbd51bf2354e28f24a9
@bitcoin-core bitcoin-core locked and limited conversation to collaborators Oct 26, 2023
Sign up for free to subscribe to this conversation on GitHub. Already have an account? Sign in.
Labels
Projects
None yet
Development

No branches or pull requests

6 participants