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

[qt] OptionsDialog: add prune setting #13043

Merged
merged 1 commit into from Jun 11, 2018

Conversation

Projects
None yet
10 participants
@Sjors
Copy link
Member

Sjors commented Apr 20, 2018

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:

schermafbeelding 2018-05-15 om 12 35 24

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.

schermafbeelding 2018-05-15 om 12 33 51

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 fanquake added the GUI label Apr 20, 2018

@randolf
Copy link
Contributor

randolf left a comment

Adding this feature to the GUI is a great idea.

@laanwj

This comment has been minimized.

Copy link
Member

laanwj commented Apr 23, 2018

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

@jonasschnelli

This comment has been minimized.

Copy link
Member

jonasschnelli commented Apr 23, 2018

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

This comment has been minimized.

Copy link
Member

laanwj commented Apr 23, 2018

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:
    untitled
@Sjors

This comment has been minimized.

Copy link
Member Author

Sjors commented Apr 25, 2018

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

This comment has been minimized.

Copy link
Member

laanwj commented Apr 25, 2018

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

This comment has been minimized.

Copy link
Member

laanwj commented Apr 25, 2018

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

@jonasschnelli

This comment has been minimized.

Copy link
Member

jonasschnelli commented Apr 26, 2018

@luke-jr

This comment has been minimized.

Copy link
Member

luke-jr commented Apr 26, 2018

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

This comment has been minimized.

Copy link
Member

jonasschnelli commented Apr 26, 2018

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

This comment has been minimized.

Copy link
Member

harding commented Apr 26, 2018

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.

@harding

This comment has been minimized.

Copy link
Member

harding commented Apr 26, 2018

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

This comment has been minimized.

Copy link
Contributor

mess110 commented Apr 26, 2018

Tested ACK a475493

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

src/qt/optionsdialog.cpp Outdated
@@ -36,8 +36,17 @@ OptionsDialog::OptionsDialog(QWidget *parent, bool enableWallet) :
/* Main elements init */
ui->databaseCache->setMinimum(nMinDbCache);
ui->databaseCache->setMaximum(nMaxDbCache);
static const uint64_t GB = 1024 * 1024 * 1024;

This comment has been minimized.

@MarcoFalke

MarcoFalke Apr 30, 2018

Member

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

This comment has been minimized.

@luke-jr

luke-jr Apr 30, 2018

Member

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

This comment has been minimized.

@Sjors

Sjors May 15, 2018

Author Member

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

@luke-jr
Copy link
Member

luke-jr left a comment

Needs to not add a QSettings prune option

@Sjors

This comment has been minimized.

Copy link
Member Author

Sjors commented Apr 30, 2018

@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

@Sjors Sjors force-pushed the Sjors:2018/04/qt-prune branch to cbede7d May 15, 2018

@Sjors

This comment has been minimized.

Copy link
Member Author

Sjors commented May 15, 2018

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

This comment has been minimized.

Copy link
Member

harding commented May 16, 2018

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

@mess110

This comment has been minimized.

Copy link
Contributor

mess110 commented May 16, 2018

Tested ACK cbede7d

@@ -266,6 +280,11 @@ void OptionsDialog::on_hideTrayIcon_stateChanged(int fState)
}
}

void OptionsDialog::togglePruneWarning(bool enabled)
{
ui->pruneWarning->setVisible(!ui->pruneWarning->isVisible());

This comment has been minimized.

@ken2812221

ken2812221 Jun 7, 2018

Member

enabled is unused

This comment has been minimized.

@MarcoFalke

MarcoFalke Jun 11, 2018

Member

^ This is up for grabs

@ken2812221

This comment has been minimized.

Copy link
Member

ken2812221 commented Jun 7, 2018

Concept ACK

@laanwj

This comment has been minimized.

Copy link
Member

laanwj commented Jun 11, 2018

utACK cbede7d

@laanwj laanwj merged commit cbede7d into bitcoin:master Jun 11, 2018

1 check passed

continuous-integration/travis-ci/pr The Travis CI build passed
Details

laanwj added a commit that referenced this pull request Jun 11, 2018

Merge #13043: [qt] OptionsDialog: add prune setting
cbede7d [qt] OptionsDialog: add prune setting (Sjors Provoost)

Pull request description:

  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:

  <img width="478" alt="schermafbeelding 2018-05-15 om 12 35 24" src="https://user-images.githubusercontent.com/10217/40051858-7939cc20-583c-11e8-9120-327a75376732.png">

  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.

  <img width="450" alt="schermafbeelding 2018-05-15 om 12 33 51" src="https://user-images.githubusercontent.com/10217/40051791-49d6156a-583c-11e8-97b9-7de6dfd8c481.png">

  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.

Tree-SHA512: e17aff276d7235fbd40796adb6431d430620788a753ee13bc064abd35d2edc4280a3d3cddc18e42b4e00edff13ed18fd4f2a966c6f0b43b689afd13673e0c4bf

@Sjors Sjors deleted the Sjors:2018/04/qt-prune branch Jun 11, 2018

@luke-jr

This comment has been minimized.

Copy link
Member

luke-jr commented Jun 12, 2018

Why was this merged with QSettings?

@Sjors

This comment has been minimized.

Copy link
Member Author

Sjors commented Jun 13, 2018

#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

This comment has been minimized.

Copy link
Member

jonasschnelli commented Jun 13, 2018

Post merge tested ACK

Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment
You can’t perform that action at this time.