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

gui: add prune to intro screen with smart default #16714

Merged
merged 5 commits into from Sep 12, 2019

Conversation

@Sjors
Copy link
Member

Sjors commented Aug 24, 2019

This adds a checkbox to the intro screen to enable pruning from the get go.

If the user has plenty of space, it's unchecked by default:

big

If the user has insufficient space it's checked by default:
low

When the user has barely enough space and is likely to need pruning in the near future, this is shown in yellow and we also check the prune box:

medium

The cut-off for this 10 GB above m_assumed_blockchain_size (=240 in chainparams.cpp).

If the user launches the first time with -prune=... then we disable the check box and display the correct size (rounded to GB):
Schermafbeelding 2019-08-24 om 20 23 14

The 2 GB default matches the settings default. The user can't change it in the intro screen, but can change it later. I'm tempted to increase that default to 10 GB, and then have the intro screen reduce it if space is really tight.

Tips for testing:

  • move your existing data dir elsewhere
  • wipe data dir at every restart (behavior is different if it exists)
  • launch with bitcoin-qt -resetguisettings -lang=en (there's some space issues in different languages)
  • fake your free space by changing intro.cpp line 90: freeBytesAvailable = 5000000000; // 5 GB
  • try both testnet and mainnet, because settings are seperate. In particular note how step 7 in GuiMain switches where QTSettings settings points to; this had me thoroughly confused on testnet, because I was setting them too early.
@hebasto

This comment has been minimized.

Copy link
Member

hebasto commented Aug 24, 2019

Concept ACK

@Sjors Sjors force-pushed the Sjors:2019/08/wizard-prune branch from f247184 to dae3a1f Aug 24, 2019
@jb55

This comment has been minimized.

Copy link
Contributor

jb55 commented Aug 24, 2019

Awesome! Concept ACK!

@Sjors Sjors force-pushed the Sjors:2019/08/wizard-prune branch from dae3a1f to 4ccdc8d Aug 24, 2019
@DrahtBot

This comment has been minimized.

Copy link
Contributor

DrahtBot commented Aug 24, 2019

The following sections might be updated with supplementary metadata relevant to reviewers and maintainers.

Conflicts

No conflicts as of last run.

@DrahtBot DrahtBot added the GUI label Aug 24, 2019
Sjors added 5 commits Aug 24, 2019
This makes it possible to enable pruning after the OptionsModel has been initialized and reset.
Adds a checkbox to the introduction screen letting the user enable pruning from the start.
Disable checkbox when launched with -prune
Color the text "GB needed for full chain" orange for nodes that barely have enough space.
@Sjors Sjors force-pushed the Sjors:2019/08/wizard-prune branch from 4ccdc8d to 9924bce Aug 24, 2019
@Sjors

This comment has been minimized.

Copy link
Member Author

Sjors commented Aug 24, 2019

It turns out that applying the prune setting on first load was more involved than I thought. As part of the first launch sequence (or if you use -resetguisettings) it resets the QTSettings object. So you can't just do settings.setValue("bPrune", true) before the OptionsModel is instantiated. You can't change QTSettings later either, because although the settings dialog will look correct and pruning is enabled at next launch, it didn't actually tell the node to prune.

I ended up adding an explicit setter for prune to OptionsModel which uses node->forceSetArg() internally.

In addition it's necessary to know if the dialog was actually shown to the user, because otherwise the prune settings gets overridden with the default (false) at the next launch. So now there's 5 commits :-)

@practicalswift

This comment has been minimized.

Copy link
Member

practicalswift commented Aug 24, 2019

Concept ACK

@instagibbs

This comment has been minimized.

Copy link
Member

instagibbs commented Aug 24, 2019

concept ACK! It's the biggest hurdle to users today if social media is any indicator.

@achow101

This comment has been minimized.

Copy link
Member

achow101 commented Aug 24, 2019

It turns out that applying the prune setting on first load was more involved than I thought. As part of the first launch sequence (or if you use -resetguisettings) it resets the QTSettings object. So you can't just do settings.setValue("bPrune", true) before the OptionsModel is instantiated. You can't change QTSettings later either, because although the settings dialog will look correct and pruning is enabled at next launch, it didn't actually tell the node to prune.
...

In addition it's necessary to know if the dialog was actually shown to the user, because otherwise the prune settings gets overridden with the default (false) at the next launch. So now there's 5 commits :-)

A long time ago, there was a similar (or same) issue with strDataDir in QSettings. Couldn't you just do something similar to how the datadir is handled instead of having all this extra prune specific things?

I ended up adding an explicit setter for prune to OptionsModel which uses node->forceSetArg() internally.

Why does forceSetArg need to be exposed too? Could you use SoftSetArg?

@promag

This comment has been minimized.

Copy link
Member

promag commented Aug 25, 2019

Concept ACK.

I wonder if this should be the trigger to refactor the intro into a wizard? Maybe like this it's a bit overwhelming?

Had a quick look commit by commit and I think the approach could also be improved, still nice work!

@fanquake fanquake changed the title [gui] add prune to intro screen with smart default gui: add prune to intro screen with smart default Aug 25, 2019
@Sjors

This comment has been minimized.

Copy link
Member Author

Sjors commented Aug 25, 2019

I'm open to rebasing onto a bigger refactor of the intro wizard, but that might reduce the chance of making it into 0.19.

Why does forceSetArg need to be exposed too? Could you use SoftSetArg?

SoftSetArg is called twice; once during the reset where prune is set to its default 0, and then again when we set it to N. It fails the second time and instead the settings dialog will show -prune=0 as an override.

A long time ago, there was a similar (or same) issue with strDataDir in QSettings.

The reset code contains a workaround for strDataDir. It stores it in a temp variable, resets the settings and then sets it again. I could do the same thing for prune, but keep in mind that the Intro dialog isn't always shown. So if the default data dir exists and you reset your settings, then prune would still be enabled. I'd rather turn it around and add an explicit setter for the datadir.

@emilengler

This comment has been minimized.

Copy link
Contributor

emilengler commented Aug 25, 2019

Concept ACK
Could you add an option to change the pruning size?
IIRC Bitcoin Knots has such a feature @luke-jr.

@promag

This comment has been minimized.

Copy link
Member

promag commented Aug 25, 2019

I'm open to rebasing onto a bigger refactor of the intro wizard

Let me see if it's that big.

@luke-jr

This comment has been minimized.

Copy link
Member

luke-jr commented Aug 25, 2019

Yes, this is part of https://github.com/luke-jr/bitcoin/commits/rwconf_gui-0.18%2Bknots

Next step to get there is PR #11082

@jonasschnelli

This comment has been minimized.

Copy link
Member

jonasschnelli commented Aug 26, 2019

Nice!
Concept ACK.

@Sjors

This comment has been minimized.

Copy link
Member Author

Sjors commented Aug 26, 2019

@emilengler I'd like to keep the intro screen as simple as possible, not overwhelm users with choices. It's easy to adjust the prune size in settings and the user has plenty of time for that during IBD.

Also note the diminishing returns for a larger prune setting. 10 GB protects you against a 1 month reorg if blocks are 4 MB. Pruned nodes only share the most recent 288 blocks (BIP-159). Maybe after #15606 we can add another network flag to indicate down to which UTXO snapshot we have blocks. At that point it makes sense to suggest prune sizes worth N years of blocks. Again, rather than explain all those trade-offs on the intro screen, we can just pick a good default and leave the rest to the settings screen.

@luke-jr I think promag is talking about refactoring the intro screen so that it's better suited to handle more settings. In addition to that we could revamp the entire settings system, e.g. with your rw_config PR. I don't think this PR makes those efforts more difficult, or vice versa, so we can merge in any order.

@luke-jr

This comment has been minimized.

Copy link
Member

luke-jr commented Aug 26, 2019

@Sjors rwconf_gui DOES refactor the intro screen: it has options for pruning, tells you how old your backups can be, etc.

Copy link
Contributor

ryanofsky left a comment

utACK 9924bce. The changes are very logical, and implement the feature in a clean that way that doesn't add a lot of complication and shouldn't interfere with future improvements. I looked at Luke's branch too, and I think there's also a lot of great stuff there that seems fully compatible with this change.

Not a big concern, but this is also compatible with some changes I'm working on:

  • 0ab41dd from #15936 stops using QSettings for pruning and other settings shared between bitcoind and bitcoin-qt and instead stores them in the datadir.
  • 79d6760 from my ipc branch, gets rid of minute parseParameters, readConfigFiles, softSetArg, etc interfactions between node and gui code and just updates settings in batches.
@laanwj

This comment has been minimized.

Copy link
Member

laanwj commented Aug 29, 2019

Concept and light code review ACK. Thanks for working on this. Haven't tested yet.

My only remark is that this makes the already monstrously complex and fickle GUI initialization sequence even more complex, by adding more possible paths, and punching holes for one-time settings. This is mostly concerning because there are no tests and also not really the possibility for them.

@GChuf

This comment has been minimized.

Copy link
Contributor

GChuf commented Aug 30, 2019

Concept ACK, should make running a node much easier for users.

While looking around I also noticed this - would it be smart to decrease the block download window to 128 for prune mode?

bitcoin/src/validation.h

/** Size of the "block download window": how far ahead of our current height do we fetch?
 *  Larger windows tolerate larger download speed differences between peer, but increase the potential
 *  degree of disordering of blocks on disk (which make reindexing and pruning harder). We'll probably
 *  want to make this a per-peer adaptive value at some point. */
static const unsigned int BLOCK_DOWNLOAD_WINDOW = 1024;
@laanwj

This comment has been minimized.

Copy link
Member

laanwj commented Sep 12, 2019

Would be nice to get this in before the feature freeze.

@GChuf maybe, though it would negatively affect performance for initial sync on pruned nodes, please open a separate issue for this discussion, it's out of scope here

@fanquake fanquake added this to the 0.19.0 milestone Sep 12, 2019
@jonasschnelli

This comment has been minimized.

Copy link
Member

jonasschnelli commented Sep 12, 2019

Tested ACK 9924bce
This is good enough.
There is one little issue which can be fixed later:
if one starts with -prune=1 and won't check the new into screen checkbox, it will ignore the -prune=1 argument.

jonasschnelli added a commit that referenced this pull request Sep 12, 2019
9924bce [gui] intro: enable pruning by default unless disk is big (Sjors Provoost)
c8de347 [gui] intro: add prune preference (Sjors Provoost)
1bbc49d [gui] intro: inform caller if intro was shown (Sjors Provoost)
1957103 [gui] add explicit prune setter (Sjors Provoost)
1bccf6a [node] add forceSetArg to interface (Sjors Provoost)

Pull request description:

  This adds a checkbox to the intro screen to enable pruning from the get go.

  If the user has plenty of space, it's unchecked by default:

  <img width="671" alt="big" src="https://user-images.githubusercontent.com/10217/63641289-10339000-c6ac-11e9-98d7-caf64dff0da6.png">

  If the user has insufficient space it's checked by default:
  <img width="897" alt="low" src="https://user-images.githubusercontent.com/10217/63641276-d4002f80-c6ab-11e9-9f5b-a53472f814ff.png">

  When the user has barely enough space and is likely to need pruning in the near future, this is shown in yellow and we also check the prune box:

  <img width="662" alt="medium" src="https://user-images.githubusercontent.com/10217/63641294-1c1f5200-c6ac-11e9-8ecb-6b69e42b1ece.png">

  The cut-off for this 10 GB above `m_assumed_blockchain_size` (`=240` in `chainparams.cpp`).

  If the user launches the first time with `-prune=...` then we disable the check box and display the correct size (rounded to GB):
  <img width="658" alt="Schermafbeelding 2019-08-24 om 20 23 14" src="https://user-images.githubusercontent.com/10217/63641351-09594d00-c6ad-11e9-94fe-fe5ed562e109.png">

  The 2 GB default matches the settings default. The user can't change it in the intro screen, but can change it later. I'm tempted to increase that default to 10 GB, and then have the intro screen reduce it if space is really tight.

  Tips for testing:
  * move your existing data dir elsewhere
  * wipe data dir at every restart (behavior is different if it exists)
  * launch with `bitcoin-qt -resetguisettings -lang=en` (there's some space issues in different languages)
  * fake your free space by changing `intro.cpp` line 90: `freeBytesAvailable = 5000000000; // 5 GB`
  * try both testnet and mainnet, because settings are seperate. In particular note how step 7 in `GuiMain` switches where `QTSettings settings` points to; this had me thoroughly confused on testnet, because I was setting them too early.

ACKs for top commit:
  jonasschnelli:
    Tested ACK 9924bce
  ryanofsky:
    utACK 9924bce. The changes are very logical, and implement the feature in a clean that way that doesn't add a lot of complication and shouldn't interfere with future improvements. I looked at Luke's branch too, and I think there's also a lot of great stuff there that seems fully compatible with this change.

Tree-SHA512: 9523961451c53aebd347716976bc3a4a398f989dc21e9bbbd357060bd11a8f46c435f068bd421bb31ccb08e55445ef67bc347d8d19a4fb8fde9d6d3f9a3bcbb0
@jonasschnelli jonasschnelli merged commit 9924bce into bitcoin:master Sep 12, 2019
2 checks passed
2 checks passed
continuous-integration/appveyor/pr AppVeyor build succeeded
Details
continuous-integration/travis-ci/pr The Travis CI build passed
Details
@instagibbs

This comment has been minimized.

Copy link
Member

instagibbs commented Sep 12, 2019

if one starts with -prune=1 and won't check the new into screen checkbox, it will ignore the -prune=1 argument

Given that the checkbox is set at 2GB, I'm not sure that's completely unreasonable choice for the GUI.

sidhujag added a commit to syscoin/syscoin that referenced this pull request Sep 12, 2019
9924bce [gui] intro: enable pruning by default unless disk is big (Sjors Provoost)
c8de347 [gui] intro: add prune preference (Sjors Provoost)
1bbc49d [gui] intro: inform caller if intro was shown (Sjors Provoost)
1957103 [gui] add explicit prune setter (Sjors Provoost)
1bccf6a [node] add forceSetArg to interface (Sjors Provoost)

Pull request description:

  This adds a checkbox to the intro screen to enable pruning from the get go.

  If the user has plenty of space, it's unchecked by default:

  <img width="671" alt="big" src="https://user-images.githubusercontent.com/10217/63641289-10339000-c6ac-11e9-98d7-caf64dff0da6.png">

  If the user has insufficient space it's checked by default:
  <img width="897" alt="low" src="https://user-images.githubusercontent.com/10217/63641276-d4002f80-c6ab-11e9-9f5b-a53472f814ff.png">

  When the user has barely enough space and is likely to need pruning in the near future, this is shown in yellow and we also check the prune box:

  <img width="662" alt="medium" src="https://user-images.githubusercontent.com/10217/63641294-1c1f5200-c6ac-11e9-8ecb-6b69e42b1ece.png">

  The cut-off for this 10 GB above `m_assumed_blockchain_size` (`=240` in `chainparams.cpp`).

  If the user launches the first time with `-prune=...` then we disable the check box and display the correct size (rounded to GB):
  <img width="658" alt="Schermafbeelding 2019-08-24 om 20 23 14" src="https://user-images.githubusercontent.com/10217/63641351-09594d00-c6ad-11e9-94fe-fe5ed562e109.png">

  The 2 GB default matches the settings default. The user can't change it in the intro screen, but can change it later. I'm tempted to increase that default to 10 GB, and then have the intro screen reduce it if space is really tight.

  Tips for testing:
  * move your existing data dir elsewhere
  * wipe data dir at every restart (behavior is different if it exists)
  * launch with `bitcoin-qt -resetguisettings -lang=en` (there's some space issues in different languages)
  * fake your free space by changing `intro.cpp` line 90: `freeBytesAvailable = 5000000000; // 5 GB`
  * try both testnet and mainnet, because settings are seperate. In particular note how step 7 in `GuiMain` switches where `QTSettings settings` points to; this had me thoroughly confused on testnet, because I was setting them too early.

ACKs for top commit:
  jonasschnelli:
    Tested ACK 9924bce
  ryanofsky:
    utACK 9924bce. The changes are very logical, and implement the feature in a clean that way that doesn't add a lot of complication and shouldn't interfere with future improvements. I looked at Luke's branch too, and I think there's also a lot of great stuff there that seems fully compatible with this change.

Tree-SHA512: 9523961451c53aebd347716976bc3a4a398f989dc21e9bbbd357060bd11a8f46c435f068bd421bb31ccb08e55445ef67bc347d8d19a4fb8fde9d6d3f9a3bcbb0
@Sjors Sjors deleted the Sjors:2019/08/wizard-prune branch Oct 14, 2019
Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment
Projects
None yet
Linked issues

Successfully merging this pull request may close these issues.

None yet

You can’t perform that action at this time.