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] Add dbcache migration path #8407

Merged
merged 1 commit into from Jul 28, 2016

Conversation

Projects
None yet
4 participants
@jonasschnelli
Member

jonasschnelli commented Jul 26, 2016

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

@jonasschnelli jonasschnelli added the GUI label Jul 26, 2016

@jonasschnelli jonasschnelli added this to the 0.13.0 milestone Jul 26, 2016

@jonasschnelli

This comment has been minimized.

Show comment
Hide comment
@jonasschnelli
Member

jonasschnelli commented Jul 26, 2016

Fixes ##8406

@laanwj

This comment has been minimized.

Show comment
Hide comment
@laanwj

laanwj Jul 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.

Member

laanwj commented Jul 26, 2016

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.

Show outdated Hide outdated src/qt/optionsmodel.cpp
@jonasschnelli

This comment has been minimized.

Show comment
Hide comment
@jonasschnelli

jonasschnelli Jul 26, 2016

Member

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

Member

jonasschnelli commented Jul 26, 2016

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

@jonasschnelli

This comment has been minimized.

Show comment
Hide comment
@jonasschnelli

jonasschnelli Jul 26, 2016

Member

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

Member

jonasschnelli commented Jul 26, 2016

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

Show outdated Hide outdated src/qt/optionsmodel.cpp
@laanwj

This comment has been minimized.

Show comment
Hide comment
@laanwj

laanwj Jul 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.

Member

laanwj commented Jul 26, 2016

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.

@jonasschnelli

This comment has been minimized.

Show comment
Hide comment
@jonasschnelli

jonasschnelli Jul 27, 2016

Member

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

Member

jonasschnelli commented Jul 27, 2016

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

@laanwj

This comment has been minimized.

Show comment
Hide comment
@laanwj

laanwj Jul 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
  • 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
  • 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
  • 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
Member

laanwj commented Jul 28, 2016

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
  • 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
  • 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
  • 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

@laanwj laanwj merged commit 893f379 into bitcoin:master Jul 28, 2016

1 check was pending

continuous-integration/travis-ci/pr The Travis CI build is in progress
Details

laanwj added a commit that referenced this pull request Jul 28, 2016

Merge #8407: [Qt] Add dbcache migration path
893f379 [Qt] Add dbcache migration path (Jonas Schnelli)

laanwj added a commit that referenced this pull request Jul 28, 2016

[Qt] Add dbcache migration path
Github-Pull: #8407
Rebased-From: 893f379
@MarcoFalke

This comment has been minimized.

Show comment
Hide comment
@MarcoFalke

MarcoFalke Jul 28, 2016

Member
Member

MarcoFalke commented Jul 28, 2016

@dagurval dagurval referenced this pull request Nov 10, 2016

Merged

Bump default dbcache #168

codablock added a commit to codablock/dash that referenced this pull request Sep 19, 2017

Merge #8407: [Qt] Add dbcache migration path
893f379 [Qt] Add dbcache migration path (Jonas Schnelli)

codablock added a commit to codablock/dash that referenced this pull request Dec 29, 2017

Merge #8407: [Qt] Add dbcache migration path
893f379 [Qt] Add dbcache migration path (Jonas Schnelli)

codablock added a commit to codablock/dash that referenced this pull request Jan 8, 2018

Merge #8407: [Qt] Add dbcache migration path
893f379 [Qt] Add dbcache migration path (Jonas Schnelli)
Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment