qt: Force set nPruneSize in QSettings after the intro dialog

hebasto commented at 10:45 am on December 8, 2019: member

On master (5622d8f3156a293e61d0964c33d4b21d8c9fd5e0), having QSettings set already

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

enabling prune option in the intro dialog

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

DeepinScreenshot_select-area_20191208120425

has no effect:

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

With this PR:

0$ grep Prune ~/.bitcoin/testnet3/debug.log 
12019-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:

util: Replace magics with DEFAULT_PRUNE_TARGET_GB

This commit does not change behavior.

a82bd8fa57

DrahtBot added the label GUI on Dec 8, 2019

DrahtBot commented at 1:47 pm on December 8, 2019: member

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 commented at 1:25 pm on December 10, 2019: member

Tested ACK 6928c50fa3104ce1b77c5d12928d49584fa536d5

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

jonasschnelli commented at 11:46 pm on December 22, 2019: contributor

utACK 6928c50fa3104ce1b77c5d12928d49584fa536d5

Sjors commented at 9:57 am on January 2, 2020: member

@promag can you do a review?

hebasto commented at 3:49 pm on January 2, 2020: member

@fanquake @MarcoFalke Would you mind reviewing this PR?

promag commented at 4:32 pm on January 2, 2020: member

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 commented at 6:03 am on January 3, 2020: member

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 commented at 10:10 am on January 3, 2020: member

the plan seems to be to store settings in the datadir

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

hebasto commented at 10:18 pm on January 3, 2020: member

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

Sjors commented at 5:40 am on January 4, 2020: member

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.

in src/qt/bitcoin.cpp:289 in 6928c50fa3 outdated

281@@ -282,7 +282,7 @@ void BitcoinApplication::parameterSetup()
282 }
283 
284 void BitcoinApplication::SetPrune(bool prune, bool force) {
285-     optionsModel->SetPrune(prune, force);
286+    optionsModel->SetPruneTargetGB(prune ? DEFAULT_PRUNE_TARGET_GB : 0, force);
287 }

ryanofsky commented at 9:42 pm on January 6, 2020:

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”

hebasto commented at 10:29 am on January 7, 2020:

@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?

ryanofsky commented at 11:49 am on January 7, 2020:

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.

hebasto commented at 8:20 pm on January 7, 2020:

Done in the latest push.

ryanofsky approved

ryanofsky commented at 10:10 pm on January 6, 2020: member

Code review ACK 6928c50fa3104ce1b77c5d12928d49584fa536d5. 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.

hebasto commented at 8:23 pm on January 7, 2020: member

Implemented @ryanofsky’s suggestions. @Sjors @jonasschnelli @promag @ryanofsky Would you mind re-reviewing?

in src/qt/bitcoin.h:72 in 192151ff19 outdated

66@@ -67,8 +67,10 @@ class BitcoinApplication: public QApplication
67     void parameterSetup();
68     /// Create options model
69     void createOptionsModel(bool resetSettings);
70-    /// Update prune value
71-    void SetPrune(bool prune);
72+    /// Initialize prune setting. If prune is set, intentionally override
73+    /// existing prune size with the default size since this is called when
74+    /// choosing a new datadir.

ryanofsky commented at 9:23 pm on January 7, 2020:

In commit “qt: Rename SetPrune() to InitializePruneSetting()” (192151ff193ceff8ad7177b07f0b98bf0ebcfc0d)

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” (6928c50fa3104ce1b77c5d12928d49584fa536d5) instead of here, because that is the commit which introduces the behavior this comment is describing.

hebasto commented at 10:20 pm on January 7, 2020:

Done.

in src/qt/optionsmodel.cpp:255 in 0849b51291 outdated

251@@ -252,14 +252,14 @@ void OptionsModel::SetPruneEnabled(bool prune, bool force)
252     }
253 }
254 
255-void OptionsModel::SetPruneTargetGB(int prune_target_gb, bool force)
256+void OptionsModel::SetPruneTargetGB(int prune_target_gb)

ryanofsky commented at 9:43 pm on January 7, 2020:

In commit “refactor: Drop `bool force’ parameter” (0849b512916be3411db9d1efa447f2c12552ee0d)

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.

hebasto commented at 10:21 pm on January 7, 2020:

Done.

ryanofsky approved

ryanofsky commented at 9:46 pm on January 7, 2020: member

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

qt: Force set nPruneSize in QSettings after intro

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

68c9bbe9bc

refactor: Drop `bool force' parameter b0bfbe5028

qt: Rename SetPrune() to InitializePruneSetting()

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

af112ab628

hebasto force-pushed on Jan 7, 2020

hebasto commented at 10:19 pm on January 7, 2020: member

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

ryanofsky approved

ryanofsky commented at 11:07 pm on January 7, 2020: member

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

promag commented at 1:02 am on January 8, 2020: member

Tested ACK af112ab62895b145660f4cd7ff842e9cfea2a530. 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 commented at 6:32 am on January 8, 2020: member

Code review re-ACK af112ab62895b145660f4cd7ff842e9cfea2a530

fanquake referenced this in commit 7f3675b3ce on Jan 8, 2020

fanquake merged this on Jan 8, 2020

fanquake closed this on Jan 8, 2020

hebasto deleted the branch on Jan 8, 2020

sidhujag referenced this in commit 03ea43539f on Jan 8, 2020

MarkLTZ referenced this in commit 136a27c84d on Feb 3, 2020

sidhujag referenced this in commit a51fff8c3d on Nov 10, 2020

Fabcien referenced this in commit 6a7934983f on Jan 14, 2021

DrahtBot locked this on Feb 15, 2022

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

Conflicts