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

pull hebasto wants to merge 4 commits into bitcoin:master from hebasto:20191208-prune-settings-override changing 6 files +27 −10
  1. 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:

  2. util: Replace magics with DEFAULT_PRUNE_TARGET_GB
    This commit does not change behavior.
    a82bd8fa57
  3. DrahtBot added the label GUI on Dec 8, 2019
  4. 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.

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

  6. jonasschnelli commented at 11:46 pm on December 22, 2019: contributor
    utACK 6928c50fa3104ce1b77c5d12928d49584fa536d5
  7. Sjors commented at 9:57 am on January 2, 2020: member
    @promag can you do a review?
  8. hebasto commented at 3:49 pm on January 2, 2020: member
    @fanquake @MarcoFalke Would you mind reviewing this PR?
  9. 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?

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

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

  12. 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?
  13. 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.

  14. 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.
  15. ryanofsky approved
  16. 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.

  17. hebasto commented at 8:23 pm on January 7, 2020: member
    Implemented @ryanofsky’s suggestions. @Sjors @jonasschnelli @promag @ryanofsky Would you mind re-reviewing?
  18. 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.
  19. 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.
  20. ryanofsky approved
  21. 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.
  22. 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
  23. refactor: Drop `bool force' parameter b0bfbe5028
  24. qt: Rename SetPrune() to InitializePruneSetting()
    This function now resets the prune size and is only called after the
    intro sequence.
    af112ab628
  25. hebasto force-pushed on Jan 7, 2020
  26. hebasto commented at 10:19 pm on January 7, 2020: member
    @ryanofsky Thank you for your review. All your comments have been addressed.
  27. ryanofsky approved
  28. ryanofsky commented at 11:07 pm on January 7, 2020: member
    Code review ACK af112ab62895b145660f4cd7ff842e9cfea2a530. Just suggested changes since last review (thanks!)
  29. 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.

  30. Sjors commented at 6:32 am on January 8, 2020: member
    Code review re-ACK af112ab62895b145660f4cd7ff842e9cfea2a530
  31. fanquake referenced this in commit 7f3675b3ce on Jan 8, 2020
  32. fanquake merged this on Jan 8, 2020
  33. fanquake closed this on Jan 8, 2020

  34. hebasto deleted the branch on Jan 8, 2020
  35. sidhujag referenced this in commit 03ea43539f on Jan 8, 2020
  36. MarkLTZ referenced this in commit 136a27c84d on Feb 3, 2020
  37. sidhujag referenced this in commit a51fff8c3d on Nov 10, 2020
  38. Fabcien referenced this in commit 6a7934983f on Jan 14, 2021
  39. DrahtBot locked this on Feb 15, 2022

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-12-22 15:12 UTC

This site is hosted by @0xB10C
More mirrored repositories can be found on mirror.b10c.me