gui: Fix intro dialog labels when the prune button is toggled

hebasto commented at 3:25 pm on November 12, 2019: member

On master (a6f6333ba253cda83221ee529810cacf930e413f) and on 0.19.0.1 the intro dialog with prune enabled (checkbox “Discard blocks…” is checked) provides a user with wrong info about the required disk space:

DeepinScreenshot_bitcoin-qt_20191208112228

Also the paragraph “If you have chosen to limit…” is missed.

With this PR when prune checkbox is toggled, the related text labels and the amount of required space shown are updated (previously they were only updated when the data directory was updated): Screenshot from 2019-12-08 11-34-53

This PR is an alternative to #17035.

ryanofsky’s suggestion also has been implemented.

fanquake added the label GUI on Nov 12, 2019

hebasto commented at 3:27 pm on November 12, 2019: member

ping @ryanofsky @Sjors @promag

in src/qt/optionsmodel.h:21 in 4824eeb91e outdated

16@@ -16,6 +17,9 @@ class Node;
17 extern const char *DEFAULT_GUI_PROXY_HOST;
18 static constexpr unsigned short DEFAULT_GUI_PROXY_PORT = 9050;
19 
20+static inline uint64_t PruneGBtoMiB(unsigned int gb) { return (gb * GB_BYTES) >> 20; }
21+static inline unsigned int PruneMiBtoGB(uint64_t mib) { return ((mib << 20) + GB_BYTES / 2) / GB_BYTES; }

ryanofsky commented at 4:39 pm on November 12, 2019:

In commit “util: Add PruneMiBtoGB() function” (eb498bd0980850f1b2de61cf6a4d5afc728a60d6)

Should change GB_BYTES / 2 to GB_BYTES - 1 as previously suggested #17035 (review). The point of the suggestion was to always round up to the nearest number of GB, not sometimes round up and sometimes round down. This was for two reasons: 1) avoid misleading users and not show 2GB of disk usage when actually configured for 2.49GB, etc. 2) Always rounding down in PruneGBtoMiB (display -> settings) and always rounding up in PruneMiBtoGB (settings -> display) would be symmetric avoid roundtrip conversion bugs in #15936

Also this isn’t following the suggestion to use signed types. PR is also changing existing code to use unsigned instead of signed types. This isn’t great because ArgsManager, SettingsValue, etc use signed types so these changes can trigger spurious conversion warnings. Also just in general signed types work better for numbers and quantities (don’t have crazy wrapping behavior at 0) and unsigned types work better for bit data.

hebasto commented at 8:43 pm on November 12, 2019:

Agree with rounding and types. Going to fix.

Do you think the rounding rationale should be commented? If so, what could you suggest?

ryanofsky commented at 9:43 pm on November 12, 2019:

Do you think the rounding rationale should be commented? If so, what could you suggest?

Maybe something like “Round up when converting from configured MiB to displayed GB to avoid underestimating max disk usage. Round down in the other direction so roundtrip GB -> MiB -> GB conversion is stable.”

hebasto commented at 8:52 pm on November 13, 2019:

Done.

in src/qt/optionsmodel.h:77 in 5fd852990c outdated

73@@ -74,6 +74,7 @@ class OptionsModel : public QAbstractListModel
74 
75     /* Explicit setters */
76     void SetPrune(bool prune, bool force = false);
77+    void SetPrune(unsigned int prune_size_gb, bool force = false);

ryanofsky commented at 5:07 pm on November 12, 2019:

In commit “gui: Fix prune settings after intro” (5fd852990cd0e5c94e384fcf6c5fb22891534f4f)

It seems dangerous to give two functions that change different settings (“bPrune” vs “nPruneSize”) the exact same name distinguished only by a bool int overload. Should give the functions different names. Might suggest SetPruneSizeGB and SetPruneEnabled.

hebasto commented at 8:53 pm on November 13, 2019:

Done.

in src/qt/bitcoin.cpp:285 in 5fd852990c 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->SetPrune(prune ? DEFAULT_PRUNE_SIZE_GB : 0, force);

ryanofsky commented at 5:13 pm on November 12, 2019:

In commit “gui: Fix prune settings after intro” (5fd852990cd0e5c94e384fcf6c5fb22891534f4f)

I can’t figure out what this commit is doing. What is being fixed? What was the behavior before this commit and what is the behavior after? Description just says “If QSettings are set already, it is required to reset prune size option,” which doesn’t make sense to me, but I must be missing context that would make it understandable.

hebasto commented at 8:20 pm on November 12, 2019:

@ryanofsky

I can’t figure out what this commit is doing. What is being fixed?

Steps to reproduce this bug:

f25c99e735a2abc930752249687d780923e941c8 (before 5fd852990cd0e5c94e384fcf6c5fb22891534f4f)

run bitcoin-qt, set GUI option as pictured: and quit
run bitcoin-qt -choosedatadir, in intro check prune:

Expected prune target 2 GB = 1907 MiB. Actually, logged:

0Prune configured to target 5722 MiB on disk for block and undo files.

It seems, I should split first two commits into separated PR, right?

hebasto commented at 10:52 am on December 8, 2019:

See #17696.

in src/qt/intro.cpp:136 in 4824eeb91e outdated

132@@ -133,9 +133,9 @@ Intro::Intro(QWidget *parent, uint64_t blockchain_size, uint64_t chain_state_siz
133         ui->prune->setChecked(true);
134         ui->prune->setEnabled(false);
135     }
136-    const unsigned int prune_target_gb = PruneMiBtoGB(prune_target_mib);
137-    ui->prune->setText(tr("Discard blocks after verification, except most recent %1 GB (prune)").arg(prune_target_gb ? prune_target_gb : DEFAULT_PRUNE_SIZE_GB));
138-    UpdatePruneLabels(prune_target_gb);
139+    m_prune_target_gb = prune_target_mib ? PruneMiBtoGB(prune_target_mib) : DEFAULT_PRUNE_SIZE_GB;

ryanofsky commented at 5:21 pm on November 12, 2019:

In commit “gui: Fix intro dialog when prune is toogled” (4824eeb91e8ee7ed2e46519f56d860635e7c4818)

I can’t figure out what this commit is doing. Could use description of what behavior was before and how it is changed after. Just looking at code I see it is moving away from convention of using 0 pruning size to represent no pruning, but I don’t actually see how behavior is changing.

EDIT: Just noticed the new QCheckBox::toggled handler below. Maybe pull refactoring changes out of this commit to make each set of changes more obvious? Also s/toogled/toggled/

hebasto commented at 2:58 pm on November 13, 2019:

I can’t figure out what this commit is doing. Could use description of what behavior was before and how it is changed after.

The bug and fixed behavior are described in #17035 OP.

hebasto commented at 10:53 am on December 8, 2019:

This PR description has been updated.

in src/qt/intro.cpp:325 in 4824eeb91e outdated

301@@ -302,7 +302,10 @@ void Intro::startThread()
302 
303     connect(executor, &FreespaceChecker::reply, this, &Intro::setStatus);
304     connect(this, &Intro::requestCheck, executor, &FreespaceChecker::check);
305-    /*  make sure executor object is deleted in its own thread */

ryanofsky commented at 5:23 pm on November 12, 2019:

In commit “gui: Fix intro dialog when prune is toogled” (4824eeb91e8ee7ed2e46519f56d860635e7c4818)

Suggest not deleting this comment line assuming it is still applicable.

hebasto commented at 8:38 pm on November 12, 2019:

This line

0connect(thread, &QThread::finished, executor, &QObject::deleteLater);

seems to be a standard pattern in Qt framework. Yes, the comment is still applicable, but it does not provide any additional knowledge, IMHO.

in src/qt/intro.cpp:337 in 27f716ebba outdated

328@@ -346,3 +329,25 @@ QString Intro::getPathToCheck()
329     mutex.unlock();
330     return retval;
331 }
332+
333+void Intro::UpdatePruneLabels(uint64_t prune_target_gb)
334+{
335+    m_required_space_gb = m_blockchain_size;
336+    QString storageRequiresMsg = tr("At least %1 GB of data will be stored in this directory, and it will grow over time.");
337+    if (ui->prune->isChecked()) {

ryanofsky commented at 5:27 pm on November 12, 2019:

In commit “refactor: Add Intro::UpdatePruneLabels()” (27f716ebbadfefee8eac7e43374f080ff3dbc68a)

Why did this line change from if (prune_target_gb) if this commit is supposed to be move-only?

hebasto commented at 3:02 pm on November 13, 2019:

Why did this line change from if (prune_target_gb) if this commit is supposed to be move-only?

You are right. It is a remnant of the previous branch. Will be fixed.

hebasto commented at 8:53 pm on November 13, 2019:

Fixed.

emilengler commented at 8:28 pm on November 12, 2019: contributor

Thanks, Concept ACK

hebasto force-pushed on Nov 13, 2019

hebasto commented at 8:57 pm on November 13, 2019: member

@ryanofsky

Thank you for your review. All yours comments have been addressed: implemented, fixed or replied to.

Sjors commented at 1:03 pm on November 14, 2019: member

When you select a volume with low disk space (e.g. a RAM disk), when the user unchecks the prune box, it automatically checks itself again.

hebasto commented at 1:54 pm on November 14, 2019: member

@Sjors

When you select a volume with low disk space (e.g. a RAM disk), when the user unchecks the prune box, it automatically checks itself again.

It seems reasonable (the same behavior was in #17035). How should it be in your opinion?

Sjors commented at 8:20 am on November 15, 2019: member

Either the user should be allowed to go ahead anyway (maybe they’ll delete some movies during IBD), or the checkbox should be disabled.

hebasto commented at 6:09 pm on November 15, 2019: member

@Sjors

Thank you for your review.

Either the user should be allowed to go ahead anyway (maybe they’ll delete some movies during IBD), or the checkbox should be disabled.

The former suggestion has been implemented in the latest push 9672ea841d7a66c765d734cfab57210baa4b074d.

MarkLTZ referenced this in commit ac8cef4feb on Nov 17, 2019

ryanofsky commented at 8:08 pm on November 26, 2019: member

PR description doesn’t describe PR. Can you update it to say what the problem with intro dialog labels is and how the problems is fixed? Also if behavior or code organization is changing in other ways because this seems to do more than just change labels…

in src/qt/bitcoin.cpp:285 in f1ae0412d8 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);

ryanofsky commented at 8:16 pm on November 26, 2019:

In commit “gui: Fix prune in QSettings after intro” (f1ae0412d80ac7f8b30ec412564aaec782b3df0b)

I can’t figure out what this commit is doing. Can the commit description be updated to say what the behavior is before and after this commit?

hebasto commented at 10:54 am on December 8, 2019:

See #17696.

in src/qt/intro.cpp:138 in 162dafaaa9 outdated

136+    int64_t prune_target_mib = std::max<int64_t>(0, gArgs.GetArg("-prune", 0));
137+    if (prune_target_mib > 1) { // -prune=1 means enabled, above that it's a size in MiB
138         ui->prune->setChecked(true);
139         ui->prune->setEnabled(false);
140     }
141-    ui->prune->setText(tr("Discard blocks after verification, except most recent %1 GB (prune)").arg(pruneTarget ? pruneTarget / 1000 : DEFAULT_PRUNE_TARGET_GB));

ryanofsky commented at 8:22 pm on November 26, 2019:

In commit “util: Add PruneMiBtoGB() function” (162dafaaa9d47e7d40db09040a265bccf975be20)

Maybe update commit description to note slight change in label text: now displays GB number instead of GiB number.

I think there are no other changes in behavior in this commit, or at least I don’t see any other changes.

hebasto commented at 9:48 pm on December 8, 2019:

Done.

I think there are no other changes in behavior in this commit, or at least I don’t see any other changes.

Correct.

in src/qt/intro.cpp:136 in f07dd7c69e outdated

132@@ -133,9 +133,9 @@ Intro::Intro(QWidget *parent, uint64_t blockchain_size, uint64_t chain_state_siz
133         ui->prune->setChecked(true);
134         ui->prune->setEnabled(false);
135     }
136-    const int prune_target_gb = PruneMiBtoGB(prune_target_mib);
137-    ui->prune->setText(tr("Discard blocks after verification, except most recent %1 GB (prune)").arg(prune_target_gb ? prune_target_gb : DEFAULT_PRUNE_TARGET_GB));
138-    UpdatePruneLabels(prune_target_gb);
139+    m_prune_target_gb = prune_target_mib ? PruneMiBtoGB(prune_target_mib) : DEFAULT_PRUNE_TARGET_GB;

ryanofsky commented at 8:28 pm on November 26, 2019:

In commit “gui: Fix intro dialog when prune is toggled” (f07dd7c69ed7a5245f79662100c859286557b73b)

I think this should say prune_target_mib > 1 ? ... instead of prune_target_mib ? ...

If the pruning value is 1, pruning is still effectively disabled and more than 1GB will be required unless manual pruning is done.

Could also combine this check into the if (prune_target_mib > 1) check above.

hebasto commented at 9:48 pm on December 8, 2019:

Fixed in the latest push.

in src/qt/intro.cpp:308 in f07dd7c69e outdated

301@@ -302,7 +302,10 @@ void Intro::startThread()
302 
303     connect(executor, &FreespaceChecker::reply, this, &Intro::setStatus);
304     connect(this, &Intro::requestCheck, executor, &FreespaceChecker::check);
305-    /*  make sure executor object is deleted in its own thread */
306+    connect(ui->prune, &QCheckBox::toggled, [this](bool prune_checked) {
307+        UpdatePruneLabels(prune_checked);
308+        Q_EMIT requestCheck();
309+    });

ryanofsky commented at 8:41 pm on November 26, 2019:

In commit “gui: Fix intro dialog when prune is toggled” (f07dd7c69ed7a5245f79662100c859286557b73b)

Why trigger a new disk space check here? Can you add explanatory comments? I think I understand updating the labels because the label text depends on the check state, but I don’t get the disk space thing.

It would also be good to update commit description to say what behavior was before and after this change, and how it works.

hebasto commented at 9:50 pm on December 8, 2019:

Why trigger a new disk space check here?

Reworked. No unneeded disk space check anymore.

in src/qt/intro.cpp:258 in 9672ea841d outdated

254@@ -255,11 +255,11 @@ void Intro::setStatus(int status, const QString &message, quint64 bytesAvailable
255         if (bytesAvailable < m_required_space_gb * GB_BYTES) {
256             freeString += " " + tr("(of %n GB needed)", "", m_required_space_gb);
257             ui->freeSpace->setStyleSheet("QLabel { color: #800000 }");
258-            ui->prune->setChecked(true);
259+            if (!keep_prune) ui->prune->setChecked(true);

ryanofsky commented at 8:46 pm on November 26, 2019:

In commit “gui: Let user to uncheck prune on low-space dirs” (9672ea841d7a66c765d734cfab57210baa4b074d)

Can you update the commit description to say behavior was before and after this change? It seems without this code the previous commit f07dd7c69ed7a5245f79662100c859286557b73b will be broken, because you added a new space check when the checkbox is toggled, so the box would automatically toggle itself back on after the check completed? All of this is very unclear to me.

hebasto commented at 9:51 pm on December 8, 2019:

It seems without this code the previous commit f07dd7c will be broken…

You are right. Fixed in the latest push.

ryanofsky approved

ryanofsky commented at 8:51 pm on November 26, 2019: member

Partial code review ACK 9672ea841d7a66c765d734cfab57210baa4b074d

I reviewed the settings changes I’m more familiar with. Some the GUI commits went over my head though. I think they’d make more sense to me if they described the effects they have.

f2afd0705de8f0906a2ce996fa49673633caeeb1 util: Replace magics with DEFAULT_PRUNE_TARGET_GB (1/8)
f1ae0412d80ac7f8b30ec412564aaec782b3df0b gui: Fix prune in QSettings after intro (2/8)
162dafaaa9d47e7d40db09040a265bccf975be20 util: Add PruneMiBtoGB() function (3/8)
0f8830b08bc19d62c995434ad47a01577eaa4b6c util: Add PruneGBtoMiB() function (4/8)
5d8dbfa34655a9fa89a69b971451cf05356ca434 refactor: Replace static variable with data member (5/8)
6dd682a01fd9788f4944ac4be84897ae31925f2c refactor: Add Intro::UpdatePruneLabels() (6/8)
f07dd7c69ed7a5245f79662100c859286557b73b gui: Fix intro dialog when prune is toggled (7/8)
9672ea841d7a66c765d734cfab57210baa4b074d gui: Let user to uncheck prune on low-space dirs (8/8)

DrahtBot commented at 1:37 am on December 4, 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:

#17877 (qt, refactor: Make enums in BitcoinUnits class scoped by hebasto)
#17822 (refactor: Use uint16_t instead of unsigned short by ahook)

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.

hebasto commented at 9:42 am on December 8, 2019: member

@ryanofsky

PR description doesn’t describe PR. Can you update it to say what the problem with intro dialog labels is and how the problems is fixed?

The OP has been updated.

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

@ryanofsky

In commit “gui: Fix prune in QSettings after intro” (f1ae041)

I can’t figure out what this commit is doing. Can the commit description be updated to say what the behavior is before and after this commit?

The first two commits has been split of into #17696, as they are dedicated to an orthogonal bug. The related conversations are marked as resolved here.

hebasto force-pushed on Dec 8, 2019

hebasto commented at 10:02 pm on December 8, 2019: member

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

Would you mind re-reviewing this PR?

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

DrahtBot added the label Needs rebase on Jan 8, 2020

util: Add PruneMiBtoGB() function

Now the text of prune QCheckBox shows the space in GB instead of
thousands MiB, which is consistent with other parts of the GUI.

e35e4b2ba0

util: Add PruneGBtoMiB() function

This commit does not change behavior.

2bede28cd9

hebasto force-pushed on Jan 8, 2020

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

Rebased after #17696 has been merged.

DrahtBot removed the label Needs rebase on Jan 8, 2020

in src/qt/intro.cpp:138 in a7454533bc outdated

134@@ -138,21 +135,21 @@ Intro::Intro(QWidget *parent, uint64_t blockchain_size, uint64_t chain_state_siz
135     }
136     const int prune_target_gb = PruneMiBtoGB(prune_target_mib);
137     ui->prune->setText(tr("Discard blocks after verification, except most recent %1 GB (prune)").arg(prune_target_gb ? prune_target_gb : DEFAULT_PRUNE_TARGET_GB));
138-    requiredSpace = m_blockchain_size;
139+    m_required_space_gb = m_blockchain_size;

ryanofsky commented at 4:27 pm on January 8, 2020:

In commit “refactor: Replace static variable with data member” (a7454533bc1fc0c78c4b7d8c3acbf1c23ddf4929)

It might be clearer to initialize this directly next to m_blockchain_size and m_chain_state_size above, instead of initializing this to 0 before setting it to m_blockchain_size.

hebasto commented at 11:38 pm on January 10, 2020:

The code is reorganized to make assigning values to m_required_space_gb variable more clear.

in src/qt/intro.cpp:282 in 2ee1e1c94b outdated

257-            ui->freeSpace->setStyleSheet("QLabel { color: #800000 }");
258-            ui->prune->setChecked(true);
259-        } else if (bytesAvailable / GB_BYTES - m_required_space_gb < 10) {
260-            freeString += " " + tr("(%n GB needed for full chain)", "", m_required_space_gb);
261-            ui->freeSpace->setStyleSheet("QLabel { color: #999900 }");
262-            ui->prune->setChecked(true);

ryanofsky commented at 4:52 pm on January 8, 2020:

In commit “gui: Add Intro::UpdateFreeSpaceLabel()” (2ee1e1c94b7344acfae572bcfb3618f35e0ab1b5)

Change in behavior: if new datadir has enough size, the prune checkbox unchecks.

Just for clarification, it seems like on initial setup both before and after this PR, the checkbox would automatically be checked if the default datadir drive couldn’t hold m_required_space_gb + 10, and unchecked otherwise.

The difference with this PR is just that the checkbox gets unchecked when a new datadir is selected, where previously it would never get unchecked?

hebasto commented at 12:43 pm on January 10, 2020:

The difference with this PR is just that the checkbox gets unchecked when a new datadir is selected, where previously it would never get unchecked?

Steps to reproduce:

the default datadir not having enough free space causes the prune checkbox gets checked (both on master and with this PR)
a user chooses a custom datadir with enough free space:

on master the prune checkbox remains checked
with this PR the prune checkbox gets unchecked

ryanofsky commented at 6:11 pm on January 13, 2020:

In commit “gui: Add Intro::UpdateFreeSpaceLabel()” (a208b68b6f30f932d82e0d625bb230f9c1d5ab72)

The difference with this PR is just that the checkbox gets unchecked when a new datadir is selected, where previously it would never get unchecked?

Steps to reproduce: […]

Thanks for the description. It sounds like the answer is yes.

in src/qt/intro.cpp:135 in 3f28e1025a outdated

132+        UpdatePruneLabels(prune_checked);
133+        UpdateFreeSpaceLabel();
134+    });
135+
136+    int64_t prune_target_mib = gArgs.GetArg("-prune", 0);
137+    if (prune_target_mib * 1024 * 1024 >= MIN_DISK_SPACE_FOR_BLOCK_FILES) {

ryanofsky commented at 6:31 pm on January 8, 2020:

In commit “gui: Make intro consistent with prune checkbox” (3f28e1025a97f29f5265d75cdca0cd9e4330a087)

Note: I found the change on this line confusing, and might suggest reverting it or moving it to a separate refactoring commit:

The previous version of this was:

0if (prune_target_mib > 1) { // -prune=1 means enabled, above that it's a size in MiB

now replaced by:

0if (prune_target_mib * 1024 * 1024 >= MIN_DISK_SPACE_FOR_BLOCK_FILES) {

The old version checked the prune box when the -prune setting was > 1, while the new version checks the box when the -prune setting is >= 550.

The two versions are equivalent because of other code forbidding values from 2 to 549). But the new version seems less clear with the loss of the comment and extra * 1024 * 1024 conversion and MIN_DISK_SPACE_FOR_BLOCK_FILES reference.

hebasto commented at 1:02 pm on January 10, 2020:

The two versions are equivalent because of other code forbidding values from 2 to 549).

That is not the case. The code you mentioned is called much later: https://github.com/bitcoin/bitcoin/blob/e7f84503571c171a7e6728cd2d77dd4103bd7a6f/src/qt/bitcoin.cpp#L581 after https://github.com/bitcoin/bitcoin/blob/e7f84503571c171a7e6728cd2d77dd4103bd7a6f/src/qt/bitcoin.cpp#L491

It implies that forbidding values from 2 to 549 are accepted and

0$ bitcoin-qt -choosedatadir -prune=42

works as prune=1 (on master).

This PR ignores prune forbidding values from 2 to 549.

Move it to a separate non-refactoring commit?

hebasto commented at 11:35 pm on January 10, 2020:

Note: I found the change on this line confusing, and might suggest reverting…

Reverted.

in src/qt/intro.cpp:145 in 3f28e1025a outdated

142+        m_prune_target_gb = DEFAULT_PRUNE_TARGET_GB;
143     }
144-    const int prune_target_gb = PruneMiBtoGB(prune_target_mib);
145-    ui->prune->setText(tr("Discard blocks after verification, except most recent %1 GB (prune)").arg(prune_target_gb ? prune_target_gb : DEFAULT_PRUNE_TARGET_GB));
146-    UpdatePruneLabels(prune_target_gb);
147+    ui->prune->setText(tr("Discard blocks after verification, except most recent %1 GB (prune)").arg(m_prune_target_gb));

ryanofsky commented at 7:00 pm on January 8, 2020:

In commit “gui: Make intro consistent with prune checkbox” (3f28e1025a97f29f5265d75cdca0cd9e4330a087)

Note: I think there’s no change in behavior on this line except in the -prune=1 case. Now the prune size text will show “2 GB” instead of “1 GB” there. (Before the earlier commit “util: Add PruneMiBtoGB() function” it showed “0 GB”).

“2 GB” seems like the correct value to show in this case, since after #17696 2GB is the size that will actually be used if automatic pruning is enabled later.

in src/qt/intro.cpp:132 in 3f28e1025a outdated

125@@ -128,14 +126,22 @@ Intro::Intro(QWidget *parent, uint64_t blockchain_size, uint64_t chain_state_siz
126     );
127     ui->lblExplanation2->setText(ui->lblExplanation2->text().arg(PACKAGE_NAME));
128 
129-    int64_t prune_target_mib = std::max<int64_t>(0, gArgs.GetArg("-prune", 0));
130-    if (prune_target_mib > 1) { // -prune=1 means enabled, above that it's a size in MiB
131+    connect(ui->prune, &QCheckBox::toggled, [this](bool prune_checked) {
132+        UpdatePruneLabels(prune_checked);
133+        UpdateFreeSpaceLabel();
134+    });

ryanofsky commented at 7:17 pm on January 8, 2020:

In commit “gui: Make intro consistent with prune checkbox” (3f28e1025a97f29f5265d75cdca0cd9e4330a087)

Current commit description is vague. It would be good to say what behavior is changing. As far as I can tell, there are two fixes in this commit and the other changes are refactoring:

Main fix: when prune checkbox in the intro dialog is toggled, the amount of required space shown is updated. (Previously it was only updated when the data directory was updated.)
Corner case fix: prune size shown in the case where pruning is disabled is now consistently shown as 2GB, instead of 1GB in the corner case where -prune=1 is set from an external config

hebasto commented at 11:34 pm on January 10, 2020:

Current commit description is vague.

Fixed.

in src/qt/intro.h:75 in 3f28e1025a outdated

71@@ -72,11 +72,12 @@ private Q_SLOTS:
72     //! Total required space (in GB) depending on user choice (prune or not prune).
73     int64_t m_required_space_gb{0};
74     uint64_t m_bytes_available{0};
75+    int64_t m_prune_target_gb{0};

ryanofsky commented at 7:38 pm on January 8, 2020:

In commit “gui: Make intro consistent with prune checkbox” (3f28e1025a97f29f5265d75cdca0cd9e4330a087)

I think it’d be clearer to drop this new member and instead just have a function:

0//! Return pruning size that will be used if automatic pruning is enabled.
1int GetPruneTargetGB()
2{
3    int64_t prune_target_mib = gArgs.GetArg("-prune", 0);
4    // >1 means automatic pruning is enabled by config, 1 means manual pruning, 0 means no pruning.
5    return prune_target_mib > 1 ? PruneMiBtoGB(prune_target_mib) : DEFAULT_PRUNE_TARGET_GB;
6}

If you’d prefer keeping the member, having this function would still simplify the Intro::Intro code and allow declaring the member const.

hebasto commented at 11:33 pm on January 10, 2020:

… having this function would still simplify the Intro::Intro code and allow declaring the member const.

Implemented.

ryanofsky approved

ryanofsky commented at 8:02 pm on January 8, 2020: member

Code review ACK 3f28e1025a97f29f5265d75cdca0cd9e4330a087. This looks good. Description of the PR is very clear, and the commits are well organized and mostly a breeze to review. There’s a lot going on in the final commit, and I left some comments there to help clarify it, but feel free to ignore my suggestions if they don’t make sense

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

hebasto force-pushed on Jan 10, 2020

hebasto commented at 11:52 pm on January 10, 2020: member

Updated 3f28e1025a97f29f5265d75cdca0cd9e4330a087 -> 91cc02dbeb026f5f029b7368c25739c754f19667 (pr17453.10 -> pr17453.11, compare). @ryanofsky Thank you for your review. Your comments have been addressed.

in src/qt/intro.cpp:141 in 02c6060ba6 outdated

134@@ -138,21 +135,20 @@ Intro::Intro(QWidget *parent, uint64_t blockchain_size, uint64_t chain_state_siz
135     }
136     const int prune_target_gb = PruneMiBtoGB(prune_target_mib);
137     ui->prune->setText(tr("Discard blocks after verification, except most recent %1 GB (prune)").arg(prune_target_gb ? prune_target_gb : DEFAULT_PRUNE_TARGET_GB));
138-    requiredSpace = m_blockchain_size;

ryanofsky commented at 6:03 pm on January 13, 2020:

In commit “refactor: Replace static variable with data member” (02c6060ba65c160dcd336b57752ce304ecd4ca9f)

This line doesn’t seem to be replaced by anything. It looks like m_required_space_gb will now be set to 0 instead of m_blockchain_size when prune_target_gb is greater than m_blockchain_size_gb.

It seems like this should set m_required_space_gb = m_blockchain_size_gb + m_chain_state_size_gb when prune_target_gb > m_blockchain_size_gb

hebasto commented at 11:38 am on January 14, 2020:

Yes, this is a bug, and it is easy to reproduce it. Going to fix it.

in src/qt/intro.cpp:337 in 9759ccbb40 outdated

328@@ -345,3 +329,24 @@ QString Intro::getPathToCheck()
329     mutex.unlock();
330     return retval;
331 }
332+
333+void Intro::UpdatePruneLabels(int64_t prune_target_gb)
334+{
335+    QString storageRequiresMsg = tr("At least %1 GB of data will be stored in this directory, and it will grow over time.");
336+    if (prune_target_gb) {
337+        if (prune_target_gb <= m_blockchain_size_gb) {

ryanofsky commented at 6:14 pm on January 13, 2020:

In commit “refactor: Add Intro::UpdatePruneLabels()” (9759ccbb40a1ce333d8ae3b6c3ed82520a948b76)

It seems like this should set m_required_space_gb = m_blockchain_size_gb + m_chain_state_size_gb in the case where prune_target_gb > m_blockchain_size_gb instead of not updating m_required_space_gb

ryanofsky approved

ryanofsky commented at 6:52 pm on January 13, 2020: member

Conditional code review ACK 91cc02dbeb026f5f029b7368c25739c754f19667 if questions below about m_required_space_gb being left 0 when prune size is greater than blockchain size are addressed.

Changes since last review:

More variables renamed, made signed and given _gb suffixes
Possible bug added in commit “refactor: Replace static variable with data member” could let m_required_space_gb be 0
Prune box unchecking code in commit “gui: Add Intro::UpdateFreeSpaceLabel()” is fixed to check enabled status and not rely on m_required_space_gb. Commit description is also updated
Label updating changes in commit “gui: Make Intro consistent with prune checkbox” have some refactoring and go back to treating values 2-549 as pruning to simplify checks and avoid changing behavior

refactor: Replace static variable with data member e4caa82a03

refactor: Add Intro::UpdatePruneLabels()

This is a move-only commit and it does not change behavior.

daa3f3fa90

gui: Add Intro::UpdateFreeSpaceLabel()

If a new custom datadir has enough free space, the prune checkbox gets
unchecked, unless -prune=NNN command-line option is provided.

4824a7d36c

hebasto force-pushed on Jan 14, 2020

gui: Make Intro consistent with prune checkbox

When prune checkbox is toggled, the related text labels and the amount
of required space shown are updated (previously they were only updated
when the data directory was updated).

4f7127d1e3

hebasto force-pushed on Jan 14, 2020

hebasto commented at 5:18 pm on January 14, 2020: member

Updated 91cc02dbeb026f5f029b7368c25739c754f19667 -> 4f7127d1e3a51f0f55d42a08439c516dcc8d1a26 (pr17453.11 -> pr17453.12, compare). @ryanofsky Thank you for your review. The bug you mentioned has been fixed.

ryanofsky approved

ryanofsky commented at 6:11 pm on January 14, 2020: member

Code review ACK 4f7127d1e3a51f0f55d42a08439c516dcc8d1a26. It seems like there are a few visible changes here:

Main change is amount of required disk space display now being updated when PR is checked and unchecked. This change is mentioned in the PR description and implemented in commit “gui: Make Intro consistent with prune checkbox”
Another change is required space for pruning now being converted correctly to GB instead of GiB in commit “util: Add PruneMiBtoGB() function”
Another change is that the pruning checkbox will automatically get unchecked if a newly selected datadir has enough free space to hold the chainstate and blocks in commit “gui: Add Intro::UpdateFreeSpaceLabel()”
Another change is that “Discard blocks after verification, except most recent %1 GB” label now shows 2GB instead of 0GB if bitcoin is started with -prune=1 and does the MiB to GB conversion more correctly in other cases where -prune= is set, using the right conversion factor and rounding up instead of down

in src/qt/intro.cpp:110 in 4f7127d1e3

106@@ -109,52 +107,49 @@ void FreespaceChecker::check()
107     Q_EMIT reply(replyStatus, replyMessage, freeBytesAvailable);
108 }
109 
110+namespace {

emilengler commented at 8:26 am on January 18, 2020:

Why do we need a namespace here?

hebasto commented at 9:11 am on January 18, 2020:

Use an unnamed (anonymous) namespace for all internal/nonexported entities

emilengler commented at 9:20 am on January 18, 2020:

Why has it no intends?

hebasto commented at 9:24 am on January 18, 2020:

Why has it no intends?

Sorry. “intends” or “indents”?

emilengler commented at 9:27 am on January 18, 2020:

indents, sorry

hebasto commented at 9:38 am on January 18, 2020:

It follows our Coding Style. Also see clang-format-diff.py.

emilengler commented at 2:17 pm on January 20, 2020: contributor

I tested it and it works and the code also looks OK to me. Please squash and I will ACK. This should get into 0.19.1 btw. Maybe it’s worth talking about it on the next meeting

hebasto commented at 2:55 pm on January 20, 2020: member

@emilengler

Please squash…

It seems it could make reviewing a bit harder for other reviewers, no?

emilengler commented at 3:16 pm on January 20, 2020: contributor

It seems it could make reviewing a bit harder for other reviewers, no?

If the commit message matters, put them into the longer description. But I don’t think it is worth looking at the diffs from commit to commit. It is probably the easiest to just look at the diff from HEAD to master

sipa commented at 3:50 pm on January 20, 2020: member

@emilengler Even if there are multiple commits, you’re free as a reviewer to look at the entire diff at once. The decision whether to break things up is mostly up to the author, with the caveat that each commit must make sense on its own, and ideally, the code compiles and passes tests for every individual step. I don’t see a reason here to request squashing - that’s usually only done when a PR contains “fixups” as a result of review that really should be integrated into the body.

emilengler commented at 7:23 pm on January 20, 2020: contributor

ACK 4f7127d1e3a51f0f55d42a08439c516dcc8d1a26

Sjors commented at 5:18 pm on January 22, 2020: member

tACK 4f7127d1e3a51f0f55d42a08439c516dcc8d1a26

hebasto requested review from jonasschnelli on Jan 23, 2020

jonasschnelli approved

jonasschnelli commented at 5:14 pm on January 27, 2020: contributor

utACK 4f7127d1e3a51f0f55d42a08439c516dcc8d1a26

jonasschnelli referenced this in commit b89f2d0599 on Jan 27, 2020

jonasschnelli merged this on Jan 27, 2020

jonasschnelli closed this on Jan 27, 2020

hebasto deleted the branch on Jan 27, 2020

sidhujag referenced this in commit c928853f90 on Feb 1, 2020

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

sidhujag referenced this in commit a51fff8c3d on Nov 10, 2020

sidhujag referenced this in commit 30f59ff225 on Nov 10, 2020

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

Fabcien referenced this in commit fbd564ae6a on Jan 14, 2021

DrahtBot locked this on Feb 15, 2022

gui: Fix intro dialog labels when the prune button is toggled #17453

Conflicts