qt: Force set nPruneSize in QSettings after the intro dialog #17696

hebasto · 2019-12-08T10:45:19Z

On master (5622d8f), having QSettings set already

$ grep nPruneSize ~/.config/Bitcoin/Bitcoin-Qt-testnet.conf
nPruneSize=6

enabling prune option in the intro dialog

$ ./src/qt/bitcoin-qt -choosedatadir -testnet

has no effect:

$ grep Prune ~/.bitcoin/testnet3/debug.log
2019-12-08T10:04:41Z Prune configured to target 5722 MiB on disk for block and undo files.

With this PR:

$ grep Prune ~/.bitcoin/testnet3/debug.log 
2019-12-08T10:20:35Z Prune configured to target 1907 MiB on disk for block and undo files.

This PR has been split of #17453 (the first two commits) as it fixes an orthogonal bug.

Refs:

This commit does not change behavior.

DrahtBot · 2019-12-08T13:47:53Z

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

Conflicts

Reviewers, this pull request conflicts with the following ones:

#17453 (gui: Fix intro dialog labels when the prune button is toggled by hebasto)

If you consider this pull request important, please also help to review the conflicting pull requests. Ideally, start with the one that should be merged first.

Sjors · 2019-12-10T13:25:04Z

Tested ACK 6928c50

In macOS the settings are in ~/Library/Preferences/org.bitcoin.Bitcoin-Qt-regtest.plist which can be viewed with Xcode.

jonasschnelli · 2019-12-22T23:46:53Z

utACK 6928c50

Sjors · 2020-01-02T09:57:08Z

@promag can you do a review?

hebasto · 2020-01-02T15:49:24Z

@fanquake @MarcoFalke
Would you mind reviewing this PR?

promag · 2020-01-02T16:32:53Z

I have tested that this change resets to 2GB regardless of what's in the settings.

However I'm not sure this is correct, actually I think current behavior is correct - only problem is that the intro dialog doesn't show what's in the settings. In your case the intro should have the checkbox checked and it should say "6 GB (prune)".

I don't think running with -choosedatadir should make it ignore existing settings - for that we have -resetguisettings?

Sjors · 2020-01-03T06:03:43Z

I don't think running with -choosedatadir should make it ignore existing settings - for that we have -resetguisettings?

I don't mind either way; the plan seems to be to store settings in the datadir, e.g. using rw_config #11082.

promag · 2020-01-03T10:10:33Z

the plan seems to be to store settings in the datadir

In that case the current prune setting would still be available.

hebasto · 2020-01-03T22:18:41Z

@ryanofsky Would you mind reviewing this PR w.r.t. future of settings handling?

Sjors · 2020-01-04T05:40:10Z

the plan seems to be to store settings in the datadir

In that case the current prune setting would still be available.

It doesn't make sense to me to keep the same prune setting for a different data directory; it's probably on a different disk.

ryanofsky

Code review ACK 6928c50. I think the change makes sense and does fix a surprising behavior. Thanks for splitting this from the other PR and describing the problem so clearly here!

I think the alternate approach suggested by promag #17696 (comment) to just fix the displayed size and not change the behavior would also address the bug, and maybe be less surprising for someone not expecting -choosedatadir to affect the prune size. But as Sjors points out there are multiple reasons to expect that changing the datadir would affect the size, so I think that as long as the displayed size is correct, using either the current value or default value seems ok.

ryanofsky · 2020-01-06T21:42:18Z

src/qt/bitcoin.cpp

 void BitcoinApplication::SetPrune(bool prune, bool force) {
-     optionsModel->SetPrune(prune, force);
+    optionsModel->SetPruneTargetGB(prune ? DEFAULT_PRUNE_TARGET_GB : 0, force);
 }


I think this BitcoinApplication::SetPrune function is confusing now that it is no longer just forwarding to optionsModel. Would suggest:

Renaming SetPrune to something like InitializePruneSetting or ResetPrune since it now resets the pruning size and is only called after the intro sequence.

Dropping the bool force parameter since it is only called one place with force=true

Maybe adding a comment to explain the behavior like, "Intentionally override existing prune size and use default size since this is called when choosing a new datadir"

@ryanofsky

Agree with all your suggestions.

As this PR has 3 ACKs already, is it acceptable to implement your suggestions in a following PR, e.g., #17453?

As this PR has 3 ACKs already, is it acceptable to implement your suggestions in a following PR, e.g., #17453?

Of course yes, my suggestions are always just suggestions, and I wouldn't ACK a PR that I didn't think was ready to merge. I'd slightly prefer cleanup here or in a standalone PR instead of #17453 to make #17453 more focused and understandable, but it doesn't matter too much.

Done in the latest push.

hebasto · 2020-01-07T20:23:20Z

Implemented @ryanofsky's suggestions.

@Sjors @jonasschnelli @promag @ryanofsky
Would you mind re-reviewing?

ryanofsky

Code review ACK 192151f. Just suggested changes since the last review and some minor related changes.

ryanofsky · 2020-01-07T21:23:03Z

src/qt/bitcoin.h

+    /// Initialize prune setting. If prune is set, intentionally override
+    /// existing prune size with the default size since this is called when
+    /// choosing a new datadir.


In commit "qt: Rename SetPrune() to InitializePruneSetting()" (192151f)

I think this comment would be a better placed inside the InitializePruneSetting method body because it's describing and justifying the implementation of the function, which (I think) is pretty confusing without a nearby explanation. I also think it would be better to add this comment in the earlier commit "qt: Force set nPruneSize in QSettings after intro" (6928c50) instead of here, because that is the commit which introduces the behavior this comment is describing.

ryanofsky · 2020-01-07T21:43:13Z

src/qt/optionsmodel.cpp

@@ -252,14 +252,14 @@ void OptionsModel::SetPruneEnabled(bool prune, bool force)
    }
 }

-void OptionsModel::SetPruneTargetGB(int prune_target_gb, bool force)
+void OptionsModel::SetPruneTargetGB(int prune_target_gb)


In commit "refactor: Drop `bool force' parameter" (0849b51)

Not a big deal, but I don't think the force argument should be dropped from this function. The call site in InitializePruneSetting is clearer if it has to explicitly set force to true, and it seems inconsistent and unsafe for one optionsmodel SetPrune method to soft-set by default while the other one force sets. The need to soft or force set depends on the context of the SetPrune call, and isn't something that seems right to hardcode here.

If QSettings is set already, it is required to force set nPruneSize after the intro dialog.

This function now resets the prune size and is only called after the intro sequence.

hebasto · 2020-01-07T22:19:15Z

@ryanofsky
Thank you for your review. All your comments have been addressed.

ryanofsky

Code review ACK af112ab. Just suggested changes since last review (thanks!)

promag · 2020-01-08T01:02:31Z

Tested ACK af112ab. Latest suggestions and changes look good to me.

Once settings get stored in datadir I think the intro dialog could show prune target of the currently selected directory, if any (so it isn't overridden). Or disable/hide the prune option in the intro dialog if the currently selected directory exists and is valid - it's already initialized after all.

Sjors · 2020-01-08T06:32:07Z

Code review re-ACK af112ab

…ialog af112ab qt: Rename SetPrune() to InitializePruneSetting() (Hennadii Stepanov) b0bfbe5 refactor: Drop `bool force' parameter (Hennadii Stepanov) 68c9bbe qt: Force set nPruneSize in QSettings after intro (Hennadii Stepanov) a82bd8f util: Replace magics with DEFAULT_PRUNE_TARGET_GB (Hennadii Stepanov) Pull request description: On master (5622d8f), having `QSettings` set already ``` $ grep nPruneSize ~/.config/Bitcoin/Bitcoin-Qt-testnet.conf nPruneSize=6 ``` enabling prune option in the intro dialog ``` $ ./src/qt/bitcoin-qt -choosedatadir -testnet ``` ![DeepinScreenshot_select-area_20191208120425](https://user-images.githubusercontent.com/32963518/70388183-eed68580-19b6-11ea-9aa1-f9ad9aaa68a6.png) has no effect: ``` $ grep Prune ~/.bitcoin/testnet3/debug.log 2019-12-08T10:04:41Z Prune configured to target 5722 MiB on disk for block and undo files. ``` --- With this PR: ``` $ grep Prune ~/.bitcoin/testnet3/debug.log 2019-12-08T10:20:35Z Prune configured to target 1907 MiB on disk for block and undo files. ``` This PR has been split of #17453 (the first two commits) as it fixes an orthogonal bug. Refs: - #17453 (comment) - #17453 (comment) ACKs for top commit: Sjors: Code review re-ACK af112ab ryanofsky: Code review ACK af112ab. Just suggested changes since last review (thanks!) promag: Tested ACK af112ab. Latest suggestions and changes look good to me. Tree-SHA512: 8ddad34b30dcc2cdcad6678ba8a0b36fa176e4e3465862ef6eee9be0f98d8146705138c9c7995dd8c0990af41078ca743fef1a90ed9240081f052f32ddec72b9

…intro dialog af112ab qt: Rename SetPrune() to InitializePruneSetting() (Hennadii Stepanov) b0bfbe5 refactor: Drop `bool force' parameter (Hennadii Stepanov) 68c9bbe qt: Force set nPruneSize in QSettings after intro (Hennadii Stepanov) a82bd8f util: Replace magics with DEFAULT_PRUNE_TARGET_GB (Hennadii Stepanov) Pull request description: On master (5622d8f), having `QSettings` set already ``` $ grep nPruneSize ~/.config/Bitcoin/Bitcoin-Qt-testnet.conf nPruneSize=6 ``` enabling prune option in the intro dialog ``` $ ./src/qt/bitcoin-qt -choosedatadir -testnet ``` ![DeepinScreenshot_select-area_20191208120425](https://user-images.githubusercontent.com/32963518/70388183-eed68580-19b6-11ea-9aa1-f9ad9aaa68a6.png) has no effect: ``` $ grep Prune ~/.bitcoin/testnet3/debug.log 2019-12-08T10:04:41Z Prune configured to target 5722 MiB on disk for block and undo files. ``` --- With this PR: ``` $ grep Prune ~/.bitcoin/testnet3/debug.log 2019-12-08T10:20:35Z Prune configured to target 1907 MiB on disk for block and undo files. ``` This PR has been split of bitcoin#17453 (the first two commits) as it fixes an orthogonal bug. Refs: - bitcoin#17453 (comment) - bitcoin#17453 (comment) ACKs for top commit: Sjors: Code review re-ACK af112ab ryanofsky: Code review ACK af112ab. Just suggested changes since last review (thanks!) promag: Tested ACK af112ab. Latest suggestions and changes look good to me. Tree-SHA512: 8ddad34b30dcc2cdcad6678ba8a0b36fa176e4e3465862ef6eee9be0f98d8146705138c9c7995dd8c0990af41078ca743fef1a90ed9240081f052f32ddec72b9

Fix intro dialog labels when the prune button is toggled (bitcoin#17453) Rename debug window (bitcoin#17096) Add close window shortcut (bitcoin#15768) Add privacy to the Overview page (bitcoin#16432) Remove WalletView and BitcoinGUI circular dependency (bitcoin#17937) Force set nPruneSize in QSettings after the intro dialog (bitcoin#17696)

…intro dialog af112ab qt: Rename SetPrune() to InitializePruneSetting() (Hennadii Stepanov) b0bfbe5 refactor: Drop `bool force' parameter (Hennadii Stepanov) 68c9bbe qt: Force set nPruneSize in QSettings after intro (Hennadii Stepanov) a82bd8f util: Replace magics with DEFAULT_PRUNE_TARGET_GB (Hennadii Stepanov) Pull request description: On master (5622d8f), having `QSettings` set already ``` $ grep nPruneSize ~/.config/Bitcoin/Bitcoin-Qt-testnet.conf nPruneSize=6 ``` enabling prune option in the intro dialog ``` $ ./src/qt/bitcoin-qt -choosedatadir -testnet ``` ![DeepinScreenshot_select-area_20191208120425](https://user-images.githubusercontent.com/32963518/70388183-eed68580-19b6-11ea-9aa1-f9ad9aaa68a6.png) has no effect: ``` $ grep Prune ~/.bitcoin/testnet3/debug.log 2019-12-08T10:04:41Z Prune configured to target 5722 MiB on disk for block and undo files. ``` --- With this PR: ``` $ grep Prune ~/.bitcoin/testnet3/debug.log 2019-12-08T10:20:35Z Prune configured to target 1907 MiB on disk for block and undo files. ``` This PR has been split of bitcoin#17453 (the first two commits) as it fixes an orthogonal bug. Refs: - bitcoin#17453 (comment) - bitcoin#17453 (comment) ACKs for top commit: Sjors: Code review re-ACK af112ab ryanofsky: Code review ACK af112ab. Just suggested changes since last review (thanks!) promag: Tested ACK af112ab. Latest suggestions and changes look good to me. Tree-SHA512: 8ddad34b30dcc2cdcad6678ba8a0b36fa176e4e3465862ef6eee9be0f98d8146705138c9c7995dd8c0990af41078ca743fef1a90ed9240081f052f32ddec72b9

…o dialog Summary: af112ab62895b145660f4cd7ff842e9cfea2a530 qt: Rename SetPrune() to InitializePruneSetting() (Hennadii Stepanov) b0bfbe50282877a1eee87118902901a280d6656d refactor: Drop `bool force' parameter (Hennadii Stepanov) 68c9bbe9bc91f882404556998666b1b5acea60e4 qt: Force set nPruneSize in QSettings after intro (Hennadii Stepanov) a82bd8fa5708c16d1db3edc4e82d70788eb5af19 util: Replace magics with DEFAULT_PRUNE_TARGET_GB (Hennadii Stepanov) Pull request description: On master (5622d8f3156a293e61d0964c33d4b21d8c9fd5e0), having `QSettings` set already ``` $ grep nPruneSize ~/.config/Bitcoin/Bitcoin-Qt-testnet.conf nPruneSize=6 ``` enabling prune option in the intro dialog ``` $ ./src/qt/bitcoin-qt -choosedatadir -testnet ``` ![DeepinScreenshot_select-area_20191208120425](https://user-images.githubusercontent.com/32963518/70388183-eed68580-19b6-11ea-9aa1-f9ad9aaa68a6.png) has no effect: ``` $ grep Prune ~/.bitcoin/testnet3/debug.log 2019-12-08T10:04:41Z Prune configured to target 5722 MiB on disk for block and undo files. ``` --- With this PR: ``` $ grep Prune ~/.bitcoin/testnet3/debug.log 2019-12-08T10:20:35Z Prune configured to target 1907 MiB on disk for block and undo files. ``` This PR has been split of #17453 (the first two commits) as it fixes an orthogonal bug. Refs: - bitcoin/bitcoin#17453 (comment) - bitcoin/bitcoin#17453 (comment) --- Backport of Core [[bitcoin/bitcoin#17696 | PR17696]] Test Plan: ninja all check check-functional ./src/qt/bitcoin-qt -choosedatadir -testnet start a new datadir with prune option selected, make sure the debug.log returns the correct value (~2GB) Reviewers: #bitcoin_abc, PiRK Reviewed By: #bitcoin_abc, PiRK Differential Revision: https://reviews.bitcoinabc.org/D8913

util: Replace magics with DEFAULT_PRUNE_TARGET_GB

a82bd8f

This commit does not change behavior.

DrahtBot added the GUI label Dec 8, 2019

hebasto mentioned this pull request Dec 8, 2019

gui: Fix intro dialog labels when the prune button is toggled #17453

Merged

ryanofsky approved these changes Jan 6, 2020

View reviewed changes

ryanofsky approved these changes Jan 7, 2020

View reviewed changes

hebasto added 3 commits January 8, 2020 00:12

qt: Force set nPruneSize in QSettings after intro

68c9bbe

If QSettings is set already, it is required to force set nPruneSize after the intro dialog.

refactor: Drop `bool force' parameter

b0bfbe5

qt: Rename SetPrune() to InitializePruneSetting()

af112ab

This function now resets the prune size and is only called after the intro sequence.

hebasto force-pushed the 20191208-prune-settings-override branch from 192151f to af112ab Compare January 7, 2020 22:17

ryanofsky approved these changes Jan 7, 2020

View reviewed changes

fanquake merged commit af112ab into bitcoin:master Jan 8, 2020

hebasto deleted the 20191208-prune-settings-override branch January 8, 2020 14:51

MarkLTZ mentioned this pull request Apr 4, 2020

Bitcoin PR tracking litecoinz-core/litecoinz#14

Closed

ryanofsky mentioned this pull request Jun 5, 2020

interfaces: Expose settings.json methods to GUI #15936

Merged

bitcoin locked as resolved and limited conversation to collaborators Feb 15, 2022

qt: Force set nPruneSize in QSettings after the intro dialog #17696

qt: Force set nPruneSize in QSettings after the intro dialog #17696

Uh oh!

Conversation

hebasto commented Dec 8, 2019

Uh oh!

DrahtBot commented Dec 8, 2019 • edited Loading Uh oh! There was an error while loading. Please reload this page.

Uh oh!

Conflicts

Uh oh!

Sjors commented Dec 10, 2019

Uh oh!

jonasschnelli commented Dec 22, 2019

Uh oh!

Sjors commented Jan 2, 2020

Uh oh!

hebasto commented Jan 2, 2020

Uh oh!

promag commented Jan 2, 2020

Uh oh!

Sjors commented Jan 3, 2020

Uh oh!

promag commented Jan 3, 2020

Uh oh!

hebasto commented Jan 3, 2020

Uh oh!

Sjors commented Jan 4, 2020

Uh oh!

ryanofsky left a comment

Choose a reason for hiding this comment

Uh oh!

Choose a reason for hiding this comment

Uh oh!

Choose a reason for hiding this comment

Uh oh!

Choose a reason for hiding this comment

Uh oh!

Choose a reason for hiding this comment

Uh oh!

hebasto commented Jan 7, 2020

Uh oh!

ryanofsky left a comment

Choose a reason for hiding this comment

Uh oh!

Choose a reason for hiding this comment

Uh oh!

Choose a reason for hiding this comment

Uh oh!

Choose a reason for hiding this comment

Uh oh!

Choose a reason for hiding this comment

Uh oh!

hebasto commented Jan 7, 2020

Uh oh!

ryanofsky left a comment

Choose a reason for hiding this comment

Uh oh!

promag commented Jan 8, 2020 • edited Loading Uh oh! There was an error while loading. Please reload this page.

Uh oh!

Uh oh!

Sjors commented Jan 8, 2020

Uh oh!

Uh oh!

DrahtBot commented Dec 8, 2019 •

edited

Loading

promag commented Jan 8, 2020 •

edited

Loading