Skip to content
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

Bugfix: GUI: Options: Initialise prune setting range before loading current value, and remove upper bound limit #15801

Merged
merged 2 commits into from Apr 18, 2019

Conversation

@luke-jr
Copy link
Member

@luke-jr luke-jr commented Apr 11, 2019

This fixes two bugs:

  1. The prune setting range was set after loading the current value. If users had a prune of (eg) 200, it would get limited to 99 before the range was raised. This is fixed by setting the range first.
  2. The prune setting was limited to <= the chainparams' "assumed blockchain size". There's no reason for this limit (the UX is the same either way), and there are use cases it breaks (eg, setting a prune size such that it begins pruning at some future point). Therefore, I raised it to the max value.

This is a daggy fix, so should cleanly merge to both master and 0.18 branches.

luke-jr added 2 commits Apr 11, 2019
Without this, an out-of-default-range value gets limited to the range
Hypothetically, someone may wish to begin pruning at a future blockchain size, and there's no reason to limit it lower
@fanquake fanquake added the GUI label Apr 11, 2019
@@ -148,6 +148,10 @@ void OptionsDialog::setModel(OptionsModel *_model)
if (_model->isRestartRequired())
showRestartWarning(true);

// Prune values are in GB to be consistent with intro.cpp
static constexpr uint64_t nMinDiskSpace = (MIN_DISK_SPACE_FOR_BLOCK_FILES / GB_BYTES) + (MIN_DISK_SPACE_FOR_BLOCK_FILES % GB_BYTES) ? 1 : 0;
ui->pruneSize->setRange(nMinDiskSpace, std::numeric_limits<int>::max());
Copy link
Member

@MarcoFalke MarcoFalke Apr 12, 2019

Why was this changed in the first place?

Couldn't you restore the ui->pruneSize->setMinimum(nMinDiskSpace); like it was before?

Copy link
Member Author

@luke-jr luke-jr Apr 12, 2019

The default max is 99.

Copy link
Member

@promag promag Apr 14, 2019

@MarcoFalke
Copy link
Member

@MarcoFalke MarcoFalke commented Apr 12, 2019

utACK 8a33f4d

@luke-jr
Copy link
Member Author

@luke-jr luke-jr commented Apr 12, 2019

(should probably get tagged for 0.18 since we're doing another rc)

Copy link
Member

@promag promag left a comment

utACK 8a33f4d.

@@ -148,6 +148,10 @@ void OptionsDialog::setModel(OptionsModel *_model)
if (_model->isRestartRequired())
showRestartWarning(true);

// Prune values are in GB to be consistent with intro.cpp
static constexpr uint64_t nMinDiskSpace = (MIN_DISK_SPACE_FOR_BLOCK_FILES / GB_BYTES) + (MIN_DISK_SPACE_FOR_BLOCK_FILES % GB_BYTES) ? 1 : 0;
ui->pruneSize->setRange(nMinDiskSpace, std::numeric_limits<int>::max());
Copy link
Member

@promag promag Apr 14, 2019

@laanwj
Copy link
Member

@laanwj laanwj commented Apr 18, 2019

utACK 8a33f4d

laanwj added a commit that referenced this issue Apr 18, 2019
Without this, an out-of-default-range value gets limited to the range

Github-Pull: #15801
Rebased-From: 4ddeb2f
Tree-SHA512: 64c18c8be6756fc0130d3fe934edb1ea7758877d4049c5729fa2adb05abb3af4e4bbe1d5f910224e16070aada5470637a35ed14b12391efd48cf035e8a22a949
laanwj added a commit that referenced this issue Apr 18, 2019
Hypothetically, someone may wish to begin pruning at a future blockchain size, and there's no reason to limit it lower

Github-Pull: #15801
Rebased-From: 8a33f4d
Tree-SHA512: 814dc5f004c3418216a12f8e0ef1a8db2c586d98b515d48b31a8ccd7f4e0deb12b9f2b6110bf702576a32802ca1d30e518df5ad7c28046deb4d0e9be46a6e7b8
@laanwj laanwj merged commit 8a33f4d into bitcoin:master Apr 18, 2019
2 checks passed
laanwj added a commit that referenced this issue Apr 18, 2019
…fore loading current value, and remove upper bound limit

8a33f4d GUI: Options: Remove the upper-bound limit from pruning size setting (Luke Dashjr)
4ddeb2f GUI: Options: Set the range of pruning size before loading its value (Luke Dashjr)

Pull request description:

  This fixes two bugs:

  1. The prune setting range was set *after* loading the current value. If users had a prune of (eg) 200, it would get limited to 99 before the range was raised. This is fixed by setting the range first.
  2. The prune setting was limited to <= the chainparams' "assumed blockchain size". There's no reason for this limit (the UX is the same either way), and there are use cases it breaks (eg, setting a prune size such that it begins pruning at some future point). Therefore, I raised it to the max value.

  This is a daggy fix, so should cleanly merge to both master and 0.18 branches.

ACKs for commit 8a33f4:
  MarcoFalke:
    utACK 8a33f4d
  laanwj:
    utACK 8a33f4d
  promag:
    utACK 8a33f4d.

Tree-SHA512: 480570fa243ab5cc76af76fded18cb8cb2d3194b9f050fec5e03ca551edeeda72ee8b06312e200a9e49404ec1cdffa62f7150cf9982ec1b282f17d90879ce438
HashUnlimited pushed a commit to HashUnlimited/chaincoin that referenced this issue Apr 19, 2019
Without this, an out-of-default-range value gets limited to the range

Github-Pull: bitcoin#15801
Rebased-From: 4ddeb2f
Tree-SHA512: 64c18c8be6756fc0130d3fe934edb1ea7758877d4049c5729fa2adb05abb3af4e4bbe1d5f910224e16070aada5470637a35ed14b12391efd48cf035e8a22a949
HashUnlimited pushed a commit to HashUnlimited/chaincoin that referenced this issue Apr 19, 2019
Hypothetically, someone may wish to begin pruning at a future blockchain size, and there's no reason to limit it lower

Github-Pull: bitcoin#15801
Rebased-From: 8a33f4d
Tree-SHA512: 814dc5f004c3418216a12f8e0ef1a8db2c586d98b515d48b31a8ccd7f4e0deb12b9f2b6110bf702576a32802ca1d30e518df5ad7c28046deb4d0e9be46a6e7b8
@goomario
Copy link

@goomario goomario commented Jul 8, 2020

Look like QSpinBox::setRange() not effect on Qt5.9.8 macOS Catalina.

jasonbcox pushed a commit to Bitcoin-ABC/bitcoin-abc that referenced this issue Oct 13, 2020
…s value

Summary:
PR description:
> This fixes two bugs:
> - The prune setting range was set after loading the current value. If users had a prune of (eg) 200, it would get limited to 99 before the range was raised. This is fixed by setting the range first.
> - The prune setting was limited to <= the chainparams' "assumed blockchain size". There's no reason for this limit (the UX is the same either way), and there are use cases it breaks (eg, setting a prune size such that it begins pruning at some future point). Therefore, I raised it to the max value.

Backport of Core [[bitcoin/bitcoin#15801 | PR15801]]

Test Plan:
`ninja && ninja check`

Build and install bitcoin-qt, check that this setting is not limited to current blockchain size.
Menu `Settings -> Options -> Main -> Prune block storage to ___ Gb`

Reviewers: O1 Bitcoin ABC, #bitcoin_abc, deadalnix

Reviewed By: O1 Bitcoin ABC, #bitcoin_abc, deadalnix

Differential Revision: https://reviews.bitcoinabc.org/D7900
PastaPastaPasta added a commit to PastaPastaPasta/dash that referenced this issue Sep 11, 2021
…ange before loading current value, and remove upper bound limit

8a33f4d GUI: Options: Remove the upper-bound limit from pruning size setting (Luke Dashjr)
4ddeb2f GUI: Options: Set the range of pruning size before loading its value (Luke Dashjr)

Pull request description:

  This fixes two bugs:

  1. The prune setting range was set *after* loading the current value. If users had a prune of (eg) 200, it would get limited to 99 before the range was raised. This is fixed by setting the range first.
  2. The prune setting was limited to <= the chainparams' "assumed blockchain size". There's no reason for this limit (the UX is the same either way), and there are use cases it breaks (eg, setting a prune size such that it begins pruning at some future point). Therefore, I raised it to the max value.

  This is a daggy fix, so should cleanly merge to both master and 0.18 branches.

ACKs for commit 8a33f4:
  MarcoFalke:
    utACK 8a33f4d
  laanwj:
    utACK 8a33f4d
  promag:
    utACK 8a33f4d.

Tree-SHA512: 480570fa243ab5cc76af76fded18cb8cb2d3194b9f050fec5e03ca551edeeda72ee8b06312e200a9e49404ec1cdffa62f7150cf9982ec1b282f17d90879ce438
PastaPastaPasta added a commit to PastaPastaPasta/dash that referenced this issue Sep 11, 2021
…ange before loading current value, and remove upper bound limit

8a33f4d GUI: Options: Remove the upper-bound limit from pruning size setting (Luke Dashjr)
4ddeb2f GUI: Options: Set the range of pruning size before loading its value (Luke Dashjr)

Pull request description:

  This fixes two bugs:

  1. The prune setting range was set *after* loading the current value. If users had a prune of (eg) 200, it would get limited to 99 before the range was raised. This is fixed by setting the range first.
  2. The prune setting was limited to <= the chainparams' "assumed blockchain size". There's no reason for this limit (the UX is the same either way), and there are use cases it breaks (eg, setting a prune size such that it begins pruning at some future point). Therefore, I raised it to the max value.

  This is a daggy fix, so should cleanly merge to both master and 0.18 branches.

ACKs for commit 8a33f4:
  MarcoFalke:
    utACK 8a33f4d
  laanwj:
    utACK 8a33f4d
  promag:
    utACK 8a33f4d.

Tree-SHA512: 480570fa243ab5cc76af76fded18cb8cb2d3194b9f050fec5e03ca551edeeda72ee8b06312e200a9e49404ec1cdffa62f7150cf9982ec1b282f17d90879ce438
PastaPastaPasta added a commit to PastaPastaPasta/dash that referenced this issue Sep 12, 2021
…ange before loading current value, and remove upper bound limit

8a33f4d GUI: Options: Remove the upper-bound limit from pruning size setting (Luke Dashjr)
4ddeb2f GUI: Options: Set the range of pruning size before loading its value (Luke Dashjr)

Pull request description:

  This fixes two bugs:

  1. The prune setting range was set *after* loading the current value. If users had a prune of (eg) 200, it would get limited to 99 before the range was raised. This is fixed by setting the range first.
  2. The prune setting was limited to <= the chainparams' "assumed blockchain size". There's no reason for this limit (the UX is the same either way), and there are use cases it breaks (eg, setting a prune size such that it begins pruning at some future point). Therefore, I raised it to the max value.

  This is a daggy fix, so should cleanly merge to both master and 0.18 branches.

ACKs for commit 8a33f4:
  MarcoFalke:
    utACK 8a33f4d
  laanwj:
    utACK 8a33f4d
  promag:
    utACK 8a33f4d.

Tree-SHA512: 480570fa243ab5cc76af76fded18cb8cb2d3194b9f050fec5e03ca551edeeda72ee8b06312e200a9e49404ec1cdffa62f7150cf9982ec1b282f17d90879ce438
Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment
Labels
Projects
None yet
Linked issues

Successfully merging this pull request may close these issues.

None yet

6 participants