[qt] OptionsDialog: add prune setting #13043

pull Sjors wants to merge 1 commits into bitcoin:master from Sjors:2018/04/qt-prune changing 6 files +118 −2
  1. 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.

  2. fanquake added the label GUI on Apr 20, 2018
  3. randolf approved
  4. randolf commented at 1:52 pm on April 20, 2018: contributor
    Adding this feature to the GUI is a great idea.
  5. laanwj commented at 10:58 am on April 23, 2018: member
    Concept ACK, will test and review. THanks a lot for adding this.
  6. 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)?
  7. 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:
    02018-04-23T13:27:38Z init message: Pruning blockstore...
12018-04-23T13:28:20Z Prune: UnlinkPrunedFiles deleted blk/rev (00000)
2...
32018-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: untitled
  8. 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?

  9. 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.

  10. 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.
  11. jonasschnelli commented at 6:43 am on April 26, 2018: contributor
    Gitian build: https://bitcoin.jonasschnelli.ch/build/584
  12. 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…
  13. 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).

  14. 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:

    1. 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)

      pr13043-dialog-highlight

    2. 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.

  15. 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)

  16. 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

  17. 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.
  18. luke-jr changes_requested
  19. luke-jr commented at 2:22 am on April 30, 2018: member
    Needs to not add a QSettings prune option
  20. 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

  21. [qt] OptionsDialog: add prune setting cbede7dbfd
  22. Sjors force-pushed on May 15, 2018
  23. 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.

  24. 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!
  25. mess110 commented at 10:43 pm on May 16, 2018: contributor
    Tested ACK cbede7d
  26. 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
  27. ken2812221 commented at 6:49 pm on June 7, 2018: contributor
    Concept ACK
  28. laanwj commented at 12:21 pm on June 11, 2018: member
    utACK cbede7dbfde83d53ef38d257e9940af5f163b03c
  29. laanwj merged this on Jun 11, 2018
  30. laanwj closed this on Jun 11, 2018
  31. laanwj referenced this in commit 6e249e4678 on Jun 11, 2018
  32. Sjors deleted the branch on Jun 11, 2018
  33. luke-jr commented at 8:17 pm on June 12, 2018: member
    Why was this merged with QSettings?
  34. 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.

  35. jonasschnelli commented at 9:49 am on June 13, 2018: contributor
    Post merge tested ACK
  36. jasonbcox referenced this in commit 606edea786 on Oct 25, 2019
  37. PastaPastaPasta referenced this in commit fdfbd01954 on May 11, 2020
  38. jonspock referenced this in commit d48e51eed6 on Oct 1, 2020
  39. jonspock referenced this in commit 31ee871433 on Oct 1, 2020
  40. jonspock referenced this in commit fcb4409aac on Oct 5, 2020
  41. jonspock referenced this in commit fdb7aa1801 on Oct 10, 2020
  42. PastaPastaPasta referenced this in commit 1541e44d85 on Apr 13, 2021
  43. xdustinface referenced this in commit e5915e4764 on Apr 13, 2021
  44. MarcoFalke locked this on Sep 8, 2021


Sjors randolf laanwj jonasschnelli luke-jr harding mess110 MarcoFalke ken2812221

Labels
GUI

github-metadata-mirror

This is a metadata mirror of the GitHub repository bitcoin/bitcoin. This site is not affiliated with GitHub. Content is generated from a GitHub metadata backup.
generated: 2024-10-04 19:12 UTC
This site is hosted by @0xB10C
More mirrored repositories can be found on mirror.b10c.me