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

Sjors commented at 6:26 PM on August 24, 2019: member

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:

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:

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.

hebasto commented at 6:29 PM on August 24, 2019: member

Concept ACK

Sjors force-pushed on Aug 24, 2019

jb55 commented at 6:49 PM on August 24, 2019: member

Awesome! Concept ACK!

Sjors force-pushed on Aug 24, 2019

DrahtBot commented at 7:35 PM on August 24, 2019: member

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

Conflicts

No conflicts as of last run.

DrahtBot added the label GUI on Aug 24, 2019

[node] add forceSetArg to interface 1bccf6a52d

[gui] add explicit prune setter

This makes it possible to enable pruning after the OptionsModel has been initialized and reset.

1957103786

[gui] intro: inform caller if intro was shown 1bbc49d207

[gui] intro: add prune preference

Adds a checkbox to the introduction screen letting the user enable pruning from the start.
Disable checkbox when launched with -prune

c8de347a9d

[gui] intro: enable pruning by default unless disk is big

Color the text "GB needed for full chain" orange for nodes that barely have enough space.

9924bce317

Sjors force-pushed on Aug 24, 2019

Sjors commented at 8:51 PM on August 24, 2019: member

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 commented at 9:20 PM on August 24, 2019: contributor

Concept ACK

instagibbs commented at 10:21 PM on August 24, 2019: member

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

achow101 commented at 11:06 PM on August 24, 2019: member

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 commented at 12:06 AM on August 25, 2019: member

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 renamed this:
~~[gui] add prune to intro screen with smart default~~
gui: add prune to intro screen with smart default
on Aug 25, 2019

Sjors commented at 8:57 AM on August 25, 2019: member

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 commented at 10:05 AM on August 25, 2019: contributor

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

promag commented at 10:15 AM on August 25, 2019: member

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

Let me see if it's that big.

luke-jr commented at 7:12 PM on August 25, 2019: member

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 commented at 6:50 AM on August 26, 2019: contributor

Nice! Concept ACK.

Sjors commented at 10:18 AM on August 26, 2019: member

@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 commented at 3:59 PM on August 26, 2019: member

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

ryanofsky approved

ryanofsky commented at 4:31 PM on August 26, 2019: member

utACK 9924bce317b96ab0c57efb99330abd11b6f16b9a. 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:

0ab41dd770c1f13983df528b111cfc8a51fe016a 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 commented at 2:04 PM on August 29, 2019: member

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 commented at 10:22 AM on August 30, 2019: contributor

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 commented at 6:41 AM on September 12, 2019: member

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 added this to the milestone 0.19.0 on Sep 12, 2019

jonasschnelli commented at 12:57 PM on September 12, 2019: contributor

Tested ACK 9924bce317b96ab0c57efb99330abd11b6f16b9a 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 referenced this in commit 884f7cc81b on Sep 12, 2019

jonasschnelli merged this on Sep 12, 2019

jonasschnelli closed this on Sep 12, 2019

instagibbs commented at 1:20 PM on September 12, 2019: member

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 referenced this in commit 62328be630 on Sep 12, 2019

Sjors deleted the branch on Oct 14, 2019

jasonbcox referenced this in commit 508e069b78 on Oct 16, 2020

jasonbcox referenced this in commit fc149ad0c4 on Oct 16, 2020

deadalnix referenced this in commit b49197b499 on Oct 16, 2020

jasonbcox referenced this in commit 18cc9093aa on Oct 16, 2020

jasonbcox referenced this in commit 4f80bc7eb4 on Oct 16, 2020

MarcoFalke locked this on Dec 16, 2021