[qt] OptionsDialog: add prune setting #13043

Sjors commented at 1:22 PM on April 20, 2018: member

The default suggested value is 2 GB. Minimum is 1 GB (550 MB rounded up).

When the user toggles this setting, a strong warning appears that undoing requires re-downloading the chain:

Tooltip points out that actual disk usage can be higher. It's a bit vague on the "advanced features", because I'm assuming anyone who needs to use -rescan and -txindex will read the documentation, and a more detailed text would needlessly confuse everyone else.

The UI uses gigabytes for readability and easy of use. There is also no manual pruning UI (prune=1). The user will have to use bitcoin.conf for those things.

Fixes #6461. When combined with #13029 the user, after pruning their node, can safely reset settings and/or use bitcoind without having to edit bitcoin.conf. However I don't think that's an essential prerequisite.

fanquake added the label GUI on Apr 20, 2018

randolf approved

randolf commented at 1:52 PM on April 20, 2018: contributor

Adding this feature to the GUI is a great idea.

laanwj commented at 10:58 AM on April 23, 2018: member

Concept ACK, will test and review. THanks a lot for adding this.

jonasschnelli commented at 11:56 AM on April 23, 2018: contributor

Nice! Concept ACK. Thanks for adding this One thought: why not adding this (additionally) to the intro screen (including showing the approximate disk usage reduction)?

laanwj commented at 1:27 PM on April 23, 2018: member

The default suggested value is 20 GB; ~1 month of worst case data. Minimum is 1 GB (550 MB rounded up).

What would be the motivation to not to default to the minimum?

Tested OK:

Enabled the checkbox (without changing the default of 20GB), restarted the client, next run it prunes:

2018-04-23T13:27:38Z init message: Pruning blockstore...
2018-04-23T13:28:20Z Prune: UnlinkPrunedFiles deleted blk/rev (00000)
...
2018-04-23T13:28:20Z Prune: UnlinkPrunedFiles deleted blk/rev (00783)

Re-enabling the checkbox and restarting causes the client to re-download the block chain, as expected, after the following dialog and clicking OK:

Sjors commented at 10:48 AM on April 25, 2018: member

why not adding this (additionally) to the intro screen (including showing the approximate disk usage reduction)?

That's what I had in mind as well, but I figured it can go in a separate PR.

What would be the motivation to not to default to the minimum?

To be on the safe side in case of a very large reorg, e.g. if some contentious user activated soft fork gets majority cumulative difficulty after a few weeks. If people think it's overkill, I'm happy to set it to 1 GB. If I remember correctly the limit of 550 MB was decided before SegWit, so maybe 2 GB would be a better default?

laanwj commented at 2:08 PM on April 25, 2018: member

To be on the safe side in case of a very large reorg, e.g. if some contentious user activated soft fork gets majority cumulative difficulty after a few weeks. If people think it's overkill, I'm happy to set it to 1 GB. If I remember correctly the limit of 550 MB was decided before SegWit, so maybe 2 GB would be a better default?

Oh, right, good point. From what I remember, the minimum was meant to be 550 blocks, reserving 1MiB per block, so 550MiB. This is no longer a good assumption with segwit blocks so having a higher default makes sense.

Also: NODE_NETWORK_LIMITED (BIP159) requires 288 blocks.

laanwj commented at 2:10 PM on April 25, 2018: member

@jonasschnelli can you trigger a build for this PR? Would be useful to have so people can test, I think.

jonasschnelli commented at 6:43 AM on April 26, 2018: contributor

Gitian build: https://bitcoin.jonasschnelli.ch/build/584

luke-jr commented at 7:03 AM on April 26, 2018: member

This adds a new QSettings prune option, distinct from the conf file. But the option cannot be set separately for GUI vs bitcoind, so this makes no sense...

jonasschnelli commented at 7:22 AM on April 26, 2018: contributor

Oh, right, good point. From what I remember, the minimum was meant to be 550 blocks, reserving 1MiB per block, so 550MiB. This is no longer a good assumption with segwit blocks so having a higher default makes sense.

Also: NODE_NETWORK_LIMITED (BIP159) requires 288 blocks.

The -prune=<value> is a "target" and there is a pruning protection of block files that contain blocks higher in the chain then the MIN_BLOCKS_TO_KEEP constant (288 blocks). AFAIK it is impossible with the current code to prune blocks height then the 288 limit.

But, I agree that that the prune min target of 550 is misleading because it seems like that 550 can no longer be reached.

I also think it should be labeled as "target" (something we want to reach but not sure we can).

harding commented at 3:23 PM on April 26, 2018: contributor

Tested it a little bit. No problems found but here are two things I found unintuitive:

There doesn't seem to be any consistency on this screen for where to hover to see the tooltip. I've draw highlight boxes on a screenshot approximately where hovering shows me a tooltip. This seems like a somewhat pre-existing inconsistency only made slightly worse by this PR. (Debian 9, QT5 used)
The message that a restart is needed before the settings takes effect disappears after 10-15 seconds, which seems weird (but is probably pre-existing behavior).

Everything else seemed to work as expected on this previously-pruned node after restart.

harding commented at 4:07 PM on April 26, 2018: contributor

An additional suggestion: if the tooltip isn't already overfull, it might be worth mentioning that the software remains a full node even after limiting block storage, as I find it's a common misconception for people to think that pruned nodes aren't fully validating nodes. Perhaps saying something like:

Disables some advanced features but all blocks will still be fully validated. Reverting this setting requires re-downloading the entire blockchain. Actual disk usage may be somewhat higher.

(5 extra words; 31 extra characters)

mess110 commented at 5:13 PM on April 26, 2018: contributor

Tested ACK https://github.com/bitcoin/bitcoin/pull/13043/commits/a47549348afd96aa902babfdacae0160afcc015e

Should the word prune appear in the label? Maybe Prune block storage to

nit: if I click the Limit block storage to text, nothing happens, but if I click the other texts in the options dialog the corresponding checkbox is ticked.

nit: it is also the only option in the screen missing a alt+random-key shortcut. I can press alt+s to tick the first checkbox. However, this is the only element in this screen which has 2 things which would need a shortcut: the text and the input box. To solve this, you could do something similar to Connect through SOCKS5 proxy from Network tab.

tested on ubuntu qt5

in src/qt/optionsdialog.cpp:39 in a47549348a outdated

  35 | @@ -36,8 +36,17 @@ OptionsDialog::OptionsDialog(QWidget *parent, bool enableWallet) :
  36 |      /* Main elements init */
  37 |      ui->databaseCache->setMinimum(nMinDbCache);
  38 |      ui->databaseCache->setMaximum(nMaxDbCache);
  39 | +    static const uint64_t GB = 1024 * 1024 * 1024;

MarcoFalke commented at 2:17 AM on April 30, 2018:

note that GB is 1000^3 and 1024^3 is GiB

luke-jr commented at 2:21 AM on April 30, 2018:

GB=1024^3 came first, although admittedly we have such precedent already.

Sjors commented at 10:14 AM on May 15, 2018:

I'll rename the variable to GiB, to be consistent with prune which also uses MiB.

luke-jr changes_requested

luke-jr commented at 2:22 AM on April 30, 2018: member

Needs to not add a QSettings prune option

Sjors commented at 11:13 AM on April 30, 2018: member

@luke-jr wrote:

This adds a new QSettings prune option, distinct from the conf file. But the option cannot be set separately for GUI vs bitcoind, so this makes no sense...

That is what I was referring to in the description:

When combined with #13029 the user, after pruning their node, can safely reset settings and/or use bitcoind without having to edit bitcoin.conf.

It's why I created #12833 (let QT write to config file, based on your #11082) and alternative #13029 (interpret absence of prune= setting in a way that using QSettings for this isn't a big deal). @harding @mess110 I'll look into the tooltip range and click area. It should probably be broader. Will also look at your suggested text improvement. @jonasschnelli thanks for the build

[qt] OptionsDialog: add prune setting cbede7dbfd

Sjors force-pushed on May 15, 2018

Sjors commented at 10:47 AM on May 15, 2018: member

Rebased, and:

reduced default to 2 GiB
fixed tooltip range and click area
included the suggested tooltip text improvement
replaced the word "limit" with "prune"
added alt+b keyboard shortcut, though I can't test that on macOS. Once checked, tab can be used to reached all thee input fields on that screen.

I don't see any way to address "Needs to not add a QSettings prune option" other than #12833 or #13029, so unless there is a better idea out there this PR will likely be stuck for a while. @jonasschnelli:

I also think it should be labeled as "target" (something we want to reach but not sure we can).

Bit of a cop-out, but I think the tooltip "Actual disk usage may be somewhat higher" encapsulates that.

harding commented at 5:41 PM on May 16, 2018: contributor

Lightly tested cbede7dbfde83d53ef38d257e9940af5f163b03c (but did not review code). Updated tooltip and tooltip range LGTM, thanks!

mess110 commented at 10:43 PM on May 16, 2018: contributor

Tested ACK cbede7d

in src/qt/optionsdialog.cpp:285 in cbede7dbfd

 279 | @@ -266,6 +280,11 @@ void OptionsDialog::on_hideTrayIcon_stateChanged(int fState)
 280 |      }
 281 |  }
 282 |  
 283 | +void OptionsDialog::togglePruneWarning(bool enabled)
 284 | +{
 285 | +    ui->pruneWarning->setVisible(!ui->pruneWarning->isVisible());

ken2812221 commented at 6:44 PM on June 7, 2018:

enabled is unused

MarcoFalke commented at 12:26 PM on June 11, 2018:

^ This is up for grabs

ken2812221 commented at 6:49 PM on June 7, 2018: contributor

Concept ACK

laanwj commented at 12:21 PM on June 11, 2018: member

utACK cbede7dbfde83d53ef38d257e9940af5f163b03c

laanwj merged this on Jun 11, 2018

laanwj closed this on Jun 11, 2018

laanwj referenced this in commit 6e249e4678 on Jun 11, 2018

Sjors deleted the branch on Jun 11, 2018

luke-jr commented at 8:17 PM on June 12, 2018: member

Why was this merged with QSettings?

Sjors commented at 9:39 AM on June 13, 2018: member

#12833 gets rid of QSettings almost completely. Adding this one setting to the migration there is trivial

If #11082 makes it into 0.17 then I can remove the QSettings stuff in #12833 as if it was never there.

If it takes longer, then the only gotcha is that users need to put prune=1 in bitcoin.conf if you want to use bitcoind.

jonasschnelli commented at 9:49 AM on June 13, 2018: contributor

Post merge tested ACK

jasonbcox referenced this in commit 606edea786 on Oct 25, 2019

PastaPastaPasta referenced this in commit fdfbd01954 on May 11, 2020

jonspock referenced this in commit d48e51eed6 on Oct 1, 2020

jonspock referenced this in commit 31ee871433 on Oct 1, 2020

jonspock referenced this in commit fcb4409aac on Oct 5, 2020

jonspock referenced this in commit fdb7aa1801 on Oct 10, 2020

PastaPastaPasta referenced this in commit 1541e44d85 on Apr 13, 2021

xdustinface referenced this in commit e5915e4764 on Apr 13, 2021

MarcoFalke locked this on Sep 8, 2021