Currently there is this bug when enabling and disabling pruning: The intro screen isn’t updated when something is changing.
This fixes the storage message in the top. Here is a video of this fix Unfortunately it doesn’t fix the required storage amount, I will be working on this next. Done! This needs to get into 0.19 as well
355+ QObject::tr("The wallet will also be stored in this directory.")
356+ );
357+ // Reset GUI width and update storage warning if neccesary
358+ this->adjustSize();
359+}
360+void Intro::on_prune_stateChanged(int arg1)
IMO we should move away from these dynamic connections. My suggestion is to move to the constructor:
0connect(ui->prune, &QCheckBox::stateChanged, [this](int state) {
1 setSizeWarningLabel(/* isPruning = */ state == 2, m_blockchain_size, m_chain_state_size);
2});
331@@ -349,3 +332,39 @@ QString Intro::getPathToCheck()
332 mutex.unlock();
333 return retval;
334 }
335+
336+void Intro::setSizeWarningLabel(bool isPruning, uint64_t blockchain_size, uint64_t chain_state_size)
Intro: https://github.com/bitcoin/bitcoin/blob/7b701fef58f627956d597817a1f9422edd890cdc/src/qt/intro.h#L70-L71
334@@ -349,3 +335,28 @@ QString Intro::getPathToCheck()
335 mutex.unlock();
336 return retval;
337 }
338+
339+void Intro::setSizeWarningLabel(bool isPruning)
304@@ -322,6 +305,9 @@ void Intro::startThread()
305
306 connect(executor, &FreespaceChecker::reply, this, &Intro::setStatus);
307 connect(this, &Intro::requestCheck, executor, &FreespaceChecker::check);
308+ connect(ui->prune, &QCheckBox::stateChanged, [this](int state) {
309+ setSizeWarningLabel(state == 2);
310+ });
This does not work if “-prune” is set:
0src/qt/bitcoin-qt -prune=3000
Unfortunately it doesn’t fix the required storage amount, I will be working on this next.
I tend to Approach NACK. The full problem solution, with the required storage amount is fixed, is welcome.
304@@ -322,6 +305,9 @@ void Intro::startThread()
305
306 connect(executor, &FreespaceChecker::reply, this, &Intro::setStatus);
307 connect(this, &Intro::requestCheck, executor, &FreespaceChecker::check);
308+ connect(ui->prune, &QCheckBox::stateChanged, [this](int state) {
309+ setSizeWarningLabel(state == 2);
Could get rid of the magic number 2?
See:
129@@ -130,30 +130,16 @@ Intro::Intro(QWidget *parent, uint64_t blockchain_size, uint64_t chain_state_siz
130 );
131 ui->lblExplanation2->setText(ui->lblExplanation2->text().arg(PACKAGE_NAME));
132
133+ // This is used to default set the prune box if not enough storage is available
134+ pruneWasNotSet = true;
Developer Notes - C++ data structures:
Initialize all non-static class members where they are defined.
68@@ -69,10 +69,12 @@ private Q_SLOTS:
69 QString pathToCheck;
70 uint64_t m_blockchain_size;
71 uint64_t m_chain_state_size;
72+ bool pruneWasNotSet;
Developer Notes - Coding Style (C++):
- Variable (including function arguments) and namespace names are all lowercase and may use
_to separate words (snake_case).
- Class member variables have a
m_prefix.
Also a negated name is confusing. Could it be positive?
ui->prune->isChecked()?
It seems commits need to be squashed in two ones:
pruneWasNotSet member68@@ -69,10 +69,12 @@ private Q_SLOTS:
69 QString pathToCheck;
70 uint64_t m_blockchain_size;
71 uint64_t m_chain_state_size;
72+ bool pruneWasNotSet;
73
74 void startThread();
75 void checkPath(const QString &dataDir);
76 QString getPathToCheck();
77+ void setSizeWarningLabel(bool is_pruned);
ui->prune->isChecked() is accessible here. bool is_pruned parameter seems redundant.
68@@ -69,10 +69,12 @@ private Q_SLOTS:
69 QString pathToCheck;
70 uint64_t m_blockchain_size;
71 uint64_t m_chain_state_size;
72+ bool pruneSet = false;
0 bool m_prune_set{false};
Approach ACK 727f86f3a75051daaeb81cb3984574c06eb62449, tested on Linux Mint 19.2.
Nit: #17035 (review)
Commit 727f86f3a75051daaeb81cb3984574c06eb62449 message “… It also removes a useless boolean added before” is a hint ;) Why to introduce a variable in a commit to remove it in the next one?
It does not work if disk space is insufficient and -prune=0 or -noprune is passed as an command-line option (it could be leave for the following PRs, IMO).
EDIT: The “qt: Update the required space message when the prune box is checked” commit (727f86f3a75051daaeb81cb3984574c06eb62449) should deal with both ui->prune->setChecked() and pruneTarget. The latter is ignored now ;)
EDIT: The “qt: Update the required space message when the prune box is checked” commit (727f86f) should deal with both ui->prune->setChecked() and pruneTarget. The latter is ignored now ;)
I removed this message completely because a variable rename wasn’t the real point of this commit
As I wrote:
… should deal with both
ui->prune->setChecked()andpruneTarget. The latter is ignored now ;)
More details are in the screenshot:
If pruneTarget is set to 2 GB, amount 3 GB of stored data is obviously incorrect.
Tested a bit, something seems incorrect. If I start with -prune=2000 I’ll get a text “Approx 7GB” while when I start without -prune, I’ll get 4GB as size warning.
I know this is not the place to discuss that, but why have we chosen 2GB as prune target instead of the minimum of 550MB?
If I start with -prune=2000 I’ll get a text “Approx 7GB” while when I start without -prune, I’ll get 4GB as size warning.
I confirm this behavior, setting -prune on command line and setting in the UI should give the same approximation?
See Line 361:
0requiredSpace += m_chain_state_size;
Tested this PR on top of the current master (dcc640811c8c1a0678a3c3726cb7df14f0a60250).
0src/qt/bitcoin-qt -chain=test -prune=2000
That is correct, as 5 GB is a sum of pruned blockchain size (2 GB) and chainstate size (3 GB).
0src/qt/bitcoin-qt -chain=test
That is wrong, as 2 GB is the default pruned blockchain size, and chainstate size is skipped. @promag
I confirm this behavior, setting
-pruneon command line and setting in the UI should give the same approximation?
Yes, they should.
Tested 9ab3876b43d45e8d742adcad996537b2dcbacff3 on Linux Mint 19.2:
0$ git log -1
1commit 7fabf14f9556c6d56219f9c835d78a458d862ad5 (HEAD -> pr17035-20191017)
2Merge: 46d6930f8 9ab3876b4
3Author: Emil Engler <me@emilengler.com>
4Date: Wed Oct 16 21:36:43 2019 +0000
5
6 Merge 9ab3876b43d45e8d742adcad996537b2dcbacff3 into 46d6930f8c7ba7cbcd7d86dd5d0117642fcbc819
7
8$ src/qt/bitcoin-qt -prune=42000
154- tr("%1 will download and store a copy of the Bitcoin block chain.").arg(PACKAGE_NAME) + " " +
155- storageRequiresMsg.arg(requiredSpace) + " " +
156- tr("The wallet will also be stored in this directory.")
157- );
158+ setSizeWarningLabel();
159+ pruneTarget = pruneTarget ? pruneTarget / 1000 : 2;
pruneTarget variable holds semantically different values at different points: (1) pruned target in mebibytes (MiB), and (2) pruned target in gigabytes (10^6 bytes). IMO, such semantic overloading should be avoided.
Please note, that the binary coefficient is used in master branch: https://github.com/bitcoin/bitcoin/blob/46d6930f8c7ba7cbcd7d86dd5d0117642fcbc819/src/qt/intro.cpp#L142
-prune option also uses a binary prefix for its value – MiB.
68@@ -69,10 +69,13 @@ private Q_SLOTS:
69 QString pathToCheck;
70 uint64_t m_blockchain_size;
71 uint64_t m_chain_state_size;
72+ bool m_prune_set{false};
73+ uint64_t pruneTarget;
pruneTarget/m_prune_target/ and initialized to 0 by default?
348@@ -349,7 +349,7 @@ void Intro::setSizeWarningLabel()
349 requiredSpace = m_blockchain_size;
350 QString storageRequiresMsg = QObject::tr("At least %1 GB of data will be stored in this directory, and it will grow over time.");
351 if (ui->prune->isChecked()) {
352- uint64_t prunedGBs = std::ceil(pruneTarget * 1024 * 1024.0 / GB_BYTES);
353+ uint64_t prunedGBs = std::ceil(m_prune_target * 1000 * 1000 / GB_BYTES);
uint64_t GB_BYTES{1000000000} to uint64_t GiB_BYTES{1073741824}. But that impacts the settings screen. Probably better to leave this change for another PR.
Instead of doing ad-hoc calculations everywhere, I’d strongly encourage using the following well-defined PruneGBtoMiB and PruneMiBtoGB functions (from #15936) for this:
This would increase readability and prevent bugs and let us be sure when we migrate this setting out of qsettings that round trips between units work will correctly (e.g. 3GB -> 2861 MiB -> 3GB not 2GB)
MiB is indeed consistent with other places, but you should also change
uint64_t GB_BYTES{1000000000}touint64_t GiB_BYTES{1073741824}. But that impacts the settings screen. Probably better to leave this change for another PR.
I’m not sure what the goal of this change would be, and would definitely think it should be done as separately from this PR. ~The existing conversions in this PR as of f3108a01a03a937fc3d31f007caa69e1f95f182f look correct to me~, and I’d expect normal users to be more familiar with GB than GiB. Also if a user sees a GiB value and mistakenly interprets it as a GB value their node will use more storage than expected and maybe lead to problems.
EDIT: f3108a01a03a937fc3d31f007caa69e1f95f182f does actually have some buggy conversions I missed the first pass, see #17035#pullrequestreview-305331256, but this tangential to the point about wanting to stick to GB instead of switching to GiB.
356+ }
357+ Intro::ui->lblExplanation3->setVisible(true);
358+ } else {
359+ Intro::ui->lblExplanation3->setVisible(false);
360+ }
361+ requiredSpace += m_chain_state_size;
storageRequiresMsg will have the wrong amount.
m_assumed_chain_state_size).
@ryanofsky These functions are not existent in my branch. Should I copy them?
Yes, that’s why I linked to my branch.
Concept ACK. I don’t think the original behavior is a bug, but this PR does make things more clear. The intro dialog explained how much space is needed without pruning, and then offers to reduce that if you do prune. It’s nice that it now updates the amounts and colors.
“GB needed for full chain” needs to be " GB needed for pruned chain" when prune is checked.
I suggest quashing these commits. Perhaps start with a commit that introduces setSizeWarningLabel and adds member variables, but doesn’t change behavior. That way it’s more clear what behavior you’re changing.
This fixes the bug of the required amount of storage in the intro screen
155- tr("%1 will download and store a copy of the Bitcoin block chain.").arg(PACKAGE_NAME) + " " +
156- storageRequiresMsg.arg(requiredSpace) + " " +
157- tr("The wallet will also be stored in this directory.")
158- );
159+ setSizeWarningLabel();
160+ m_prune_target = m_prune_target ? m_prune_target / 1024 : 2;
This is doing a conversion from MiB to GiB not GB. Also it is rounding down instead of up, which means round trips between two units are not stable. (Maybe not a problem for this PR since it is a one-time conversion, but definitely a problem for #12833 and #15936 which need consistent repeatable conversions.)
Would suggest:
m_prune_target to m_prune_target_gb to be clear about units it is supposed to contain.int64_t prune_target_mib variable above to hold the -prune value.m_prune_target_gb = prune_target_mib ? PruneMiBtoGB(prune_target_mib) : 2 using the PruneGBtoMiB function below (orignally from #15936)348+{
349+ requiredSpace = m_blockchain_size;
350+ QString storageRequiresMsg = QObject::tr("At least %1 GB of data will be stored in this directory, and it will grow over time.");
351+ if (ui->prune->isChecked()) {
352+ uint64_t prunedGBs = std::ceil(m_prune_target * 1024 * 1024 / GB_BYTES);
353+ if (prunedGBs <= requiredSpace) {
prunedGBs + m_chain_state_size <= requiredSpace (assuming you add m_chain_state_size to requiredSpace earlier).
347+void Intro::setSizeWarningLabel()
348+{
349+ requiredSpace = m_blockchain_size;
350+ QString storageRequiresMsg = QObject::tr("At least %1 GB of data will be stored in this directory, and it will grow over time.");
351+ if (ui->prune->isChecked()) {
352+ uint64_t prunedGBs = std::ceil(m_prune_target * 1024 * 1024 / GB_BYTES);
m_prune_target from mib to gb when m_prune_target would already contain gib after running the code in the constructor.
So this can finally get merged
Most of the reviews on the high-priority list are older than this, for some perspective: https://github.com/bitcoin/bitcoin/projects/8. More reviewers -> faster merges, of course.
