[Qt] Add dbcache migration path #8407

1. 
   jonasschnelli commented at 12:07 PM on July 26, 2016: contributor

   During the first start, the GUI writes down default values of -dbcache, -par, -upnp, -listen into its internal GUI only settings container.

   Changing the default value will have no effect on the GUI level because the old default value – even if it was untouched by the user – was persisted.

   This adds a simple migration path for -dbcache because we have bumped it up to 300MB in 0.13 (see #8273).

   I also had a solution where we don't store the default value at all. But, because we show the value as "set" in the GUI settings, we should also persist it locally. This would only make sense if there would be a switch between custom and default ( [ ] custom value ____ | [ ] default value ).

2. jonasschnelli added the label GUI on Jul 26, 2016
3. jonasschnelli added this to the milestone 0.13.0 on Jul 26, 2016
4. jonasschnelli force-pushed on Jul 26, 2016
5. 
   jonasschnelli commented at 12:08 PM on July 26, 2016: contributor

   Fixes #https://github.com/bitcoin/bitcoin/issues/8406

6. 
   laanwj commented at 12:31 PM on July 26, 2016: member

   I like this solution; can you please move the upgrader to a separate function? I'm sure this won't be the last time we need this.

7. in src/qt/optionsmodel.cpp:None in 2ca37bf1e1 outdated

     92 | +    if (!settings.contains(strSettingsVersionKey) || settings.value(strSettingsVersionKey) < CLIENT_VERSION)
     93 | +    {
     94 | +        // -dbcache was bumped from 100 to 300 in 0.13
     95 | +        // see https://github.com/bitcoin/bitcoin/pull/8273
     96 | +        // force people to upgrade to the new value if they are using 100MB
     97 | +        if (settings.contains("nDatabaseCache") && settings.value("nDatabaseCache") == 100)
   

   ---

   ---

   

   laanwj commented at 12:36 PM on July 26, 2016:

   To reduce the scope for spurious settings changes, I think it makes sense to compare the version to a specific version (where the default was changed) for each upgrader case. E.g.

   int settingsVersion = settings.value(strSettingsVersionKey);
   if (settingsVersion < 130000 && settings.contains("nDatabaseCache") && settings.value("nDatabaseCache") == 100)
   

   ---

   

   sipa commented at 12:44 PM on July 26, 2016:

   Would it be possible to treat a value of 0 (on disk) as "default", and whenever a value equal to the current software default is to be written, write 0 instead?

   That would simplify future changes...

8. jonasschnelli force-pushed on Jul 26, 2016
9. 
   jonasschnelli commented at 1:51 PM on July 26, 2016: contributor

   Factored out, fixed @laanwj finding (explicit < 130000 check).

10. jonasschnelli added the label Needs backport on Jul 26, 2016
11. 
    jonasschnelli commented at 2:01 PM on July 26, 2016: contributor

    Added another commit that will ensure that the current default value will never be written to the disk. This would simplify future changes.

12. in src/qt/optionsmodel.cpp:None in 8fe0aba2cc outdated

     434 | +{
     435 | +    // Migration of default values
     436 | +    // Check if the QSettings container was already loaded with this client version
     437 | +    QSettings settings;
     438 | +    static const char strSettingsVersionKey[] = "nSettingsVersion";
     439 | +    if (!settings.contains(strSettingsVersionKey) || settings.value(strSettingsVersionKey) < CLIENT_VERSION)
    

    ---

    ---

    

    laanwj commented at 2:14 PM on July 26, 2016:

    Just thought of this: we need to handle the case !settings.contains(strSettingsVersionKey) inside the if() as well.

    int settingsVersion = settings.contains(strSettingsVersionKey) ? settings.value(strSettingsVersionKey) : 0;
    if (settingsVersion < CLIENT_VERSION) {
        if (settingsVersion < 130000 && settings.contains("nDatabaseCache") && settings.value("nDatabaseCache") == 100)
        ....
    }
    

    Makes the code slightly cleaner as well

13. 
    laanwj commented at 2:16 PM on July 26, 2016: member

    Not sure about the second commit. If we're going to do something like that, we need a structured solution that will never write the default value for any setting to disk - as it's likely that next time we want to change the default for another option, and then singling out dbcache won't help at all. I think the upgrade approach is fine really.

14. jonasschnelli force-pushed on Jul 27, 2016
15. 
    jonasschnelli commented at 11:16 AM on July 27, 2016: contributor

    Removed the second commit after discussion with @laanwj on IRC.

16. jonasschnelli force-pushed on Jul 28, 2016
17. jonasschnelli force-pushed on Jul 28, 2016
18. jonasschnelli force-pushed on Jul 28, 2016
19. jonasschnelli force-pushed on Jul 28, 2016
20. [Qt] Add dbcache migration path 893f379ba0
21. jonasschnelli force-pushed on Jul 28, 2016
22. 
    laanwj commented at 9:28 AM on July 28, 2016: member

    Tested ACK 893f379:

    • Testing upgrader: In ~/.config/Bitcoin/Bitcoin-Qt-testnet.conf I set nDatabaseCache=100, and remove any previous nSettingsVersion line. After starting and quitting bitcoin-qt -testnet, nDatabaseCache=300 and nSettingsVersion=139900 :white_check_mark:
    • User changed it from the default before upgrading: In ~/.config/Bitcoin/Bitcoin-Qt-testnet.conf I set nDatabaseCache=25, and remove nSettingsVersion lines. After starting and quitting bitcoin-qt -testnet, values are unchanged :white_check_mark:
    • User changed it to 100 after upgrading: In ~/.config/Bitcoin/Bitcoin-Qt-testnet.conf I set nDatabaseCache=100, and nSettingsVersion=130000 lines. After starting and quitting bitcoin-qt -testnet, values are unchanged :white_check_mark:
    • Reset to correct default after empty configuration: In ~/.config/Bitcoin/Bitcoin-Qt-testnet.conf I remove nDatabaseCache, and set nSettingsVersion=130000 lines. After starting and quitting bitcoin-qt -testnet, nDatabaseCache=300 and nSettingsVersion=139900 :white_check_mark:
23. laanwj merged this on Jul 28, 2016
24. laanwj closed this on Jul 28, 2016

    ---

25. laanwj referenced this in commit 30a87c0747 on Jul 28, 2016
26. laanwj referenced this in commit 45eba4b1e0 on Jul 28, 2016
27. 
    MarcoFalke commented at 1:44 PM on July 28, 2016: member

    Fine for 0.13, but I think for 0.14 we should aim to properly support default values in qt. I will try to look at this after catching up with the IRC log...

28. MarcoFalke removed the label Needs backport on Jul 31, 2016
29. codablock referenced this in commit acd98341c6 on Sep 19, 2017
30. codablock referenced this in commit 67ca101628 on Dec 29, 2017
31. codablock referenced this in commit e7da254c5e on Jan 8, 2018
32. andvgal referenced this in commit 808ceff413 on Jan 6, 2019
33. DrahtBot locked this on Sep 8, 2021

Contributors

jonasschnellilaanwjsipaMarcoFalke

Milestone
0.13.0