Enable changing the autoprune block space size in intro dialog

luke-jr commented at 4:15 pm on October 29, 2020: member

Screenshot_20200911_095102

Moved from https://github.com/bitcoin/bitcoin/pull/18728

jonasschnelli commented at 5:54 pm on October 29, 2020: contributor

Concept ACK

in src/qt/bitcoin.cpp:318 in 273e57bd4c outdated

314@@ -315,11 +315,9 @@ void BitcoinApplication::parameterSetup()
315     InitParameterInteraction(gArgs);
316 }
317 
318-void BitcoinApplication::InitializePruneSetting(bool prune)
319+void BitcoinApplication::InitPruneSetting(int64_t prune_opt)

promag commented at 3:31 pm on November 1, 2020:

273e57bd4cd91e8399f3567c79241bbc416ea690

Why the rename? To ensure calls are refactored?

luke-jr commented at 4:53 pm on November 1, 2020:

Yes, otherwise a bool might get silently treated as a 1

in src/qt/intro.cpp:158 in 77f3f2dda6 outdated

153 
154     connect(ui->prune, &QCheckBox::toggled, [this](bool prune_checked) {
155         UpdatePruneLabels(prune_checked);
156         UpdateFreeSpaceLabel();
157     });
158+    connect(ui->pruneGB, static_cast<void (QSpinBox::*)(int)>(&QSpinBox::valueChanged), [this](int prune_GB) {

promag commented at 3:47 pm on November 1, 2020:

77f3f2dda6918b0986f785b3061cfb8c97ed2b5c

just noting that this cast is only required because of valueChanged signal is overloaded:

0#if QT_DEPRECATED_SINCE(5, 14)
1    QT_DEPRECATED_X("Use textChanged(QString) instead")
2    void valueChanged(const QString &);
3#endif

hebasto commented at 10:05 am on November 16, 2020:

77f3f2dda6918b0986f785b3061cfb8c97ed2b5c

0    connect(ui->pruneGB, QOverload<int>::of(&QSpinBox::valueChanged), [this](int prune_GB) {

looks more neat.

in src/qt/intro.cpp:20 in 77f3f2dda6 outdated

16@@ -17,6 +17,7 @@
17 
18 #include <interfaces/node.h>
19 #include <util/system.h>
20+#include <validation.h>

promag commented at 3:49 pm on November 1, 2020:

77f3f2dda6918b0986f785b3061cfb8c97ed2b5c

It’s unfortunate to include this just because of MIN_DISK_SPACE_FOR_BLOCK_FILES.

in src/qt/intro.cpp:387 in 7d40a48052 outdated

381@@ -381,6 +382,9 @@ void Intro::UpdatePruneLabels(bool prune_checked)
382     }
383     ui->lblExplanation3->setVisible(prune_checked);
384     ui->pruneGB->setEnabled(prune_checked);
385+    static const uint64_t nPowTargetSpacing = 10 * 60;  // from chainparams, which we don't have at this stage
386+    static const uint32_t expected_block_data_size = 2250000;  // includes undo data
387+    ui->lblPruneSuffix->setText(tr("(sufficient to restore backups %n day(s) old)", "block chain pruning", m_prune_target_gb * 1e9 / (uint64_t(expected_block_data_size) * 86400 / nPowTargetSpacing)));

promag commented at 3:52 pm on November 1, 2020:

7d40a4805229618a364dc53a84cd5af2ac273b7d

nit, suggest to declare a local variable.

in src/qt/intro.cpp:144 in 77f3f2dda6 outdated

139@@ -139,17 +140,25 @@ Intro::Intro(QWidget *parent, int64_t blockchain_size_gb, int64_t chain_state_si
140     );
141     ui->lblExplanation2->setText(ui->lblExplanation2->text().arg(PACKAGE_NAME));
142 
143+    const int min_prune_target = std::ceil(MIN_DISK_SPACE_FOR_BLOCK_FILES / 1e9);
144+    ui->pruneGB->setRange(min_prune_target, std::numeric_limits<int>::max());

promag commented at 3:55 pm on November 1, 2020:

77f3f2dda6918b0986f785b3061cfb8c97ed2b5c

Just setMinimum?

nit, min_prune_target_gb?

luke-jr commented at 4:54 pm on November 1, 2020:

Need to set the maximum too… Default is IIRC 100

promag commented at 3:56 pm on November 1, 2020: contributor

Concept ACK.

Bosch-0 commented at 7:03 am on November 9, 2020: none

Testing ACK 7e37329 on Windows 10.0.18363 Build 18363. Articulating how exactly the pruned storage works is a great UX move. Many users seem to think pruning only stores data relevant to them, not that its only storing a recent history of blocks. Can be quite a shock importing an old wallet that you believe has coins only to get a zero balance displayed.

Screenshot 2020-11-09 144538

Just some minor nits that could be fixed along with this PR:

“The wallet will also be stored in this directory” should be changed to “Wallets will also be stored in this directory” in the top dialogue. Users can / may have multiple wallets.

Blockchain size is around ~350GB now, may want to update the 320GB text in the second last paragraph.

ryanofsky approved

ryanofsky commented at 8:36 pm on November 11, 2020: contributor

Code review ACK 7d40a4805229618a364dc53a84cd5af2ac273b7d

MarcoFalke renamed this:
~~GUI: Enable changing the autoprune block space size in intro dialog~~
Enable changing the autoprune block space size in intro dialog
on Nov 12, 2020

promag commented at 10:01 am on November 12, 2020: contributor

Tested ACK 7d40a4805229618a364dc53a84cd5af2ac273b7d.

Could fix the following when setting an invalid prune target (big number or empty string):

Sjors commented at 12:45 pm on November 12, 2020: member

Concept ACK

sipa commented at 0:56 am on November 14, 2020: contributor

@Bosch-0 Does rescanning a wallet with a pruned node that misses relevant blocks result in an incorrect balance? That sounds like a bug - it should just abort with missing blocks or so.

hebasto commented at 8:13 am on November 16, 2020: member

@Bosch-0

Just some minor nits that could be fixed along with this PR:

“The wallet will also be stored in this directory” should be changed to “Wallets will also be stored in this directory” in the top dialogue. Users can / may have multiple wallets.

Usually, we are trying to keep PRs focused :) A small pull with a label correction could be easy to review.

Blockchain size is around ~350GB now, may want to update the 320GB text in the second last paragraph.

This number update is a part of the release process, e.g., https://github.com/bitcoin/bitcoin/pull/20263.

in src/qt/bitcoin.cpp:509 in 273e57bd4c outdated

505@@ -508,7 +506,7 @@ int GuiMain(int argc, char* argv[])
506     /// 5. Now that settings and translations are available, ask user for data directory
507     // User language is set up: pick a data directory
508     bool did_show_intro = false;
509-    bool prune = false; // Intro dialog prune check box
510+    int64_t prune = 0;  // Intro dialog prune configuration

hebasto commented at 9:34 am on November 16, 2020:

273e57bd4cd91e8399f3567c79241bbc416ea690

nit: As through the codebase the prune numbers have different units, I’d suggest to add unit mention to the variable name.

in src/qt/intro.h:39 in dd50e8a312 outdated

35@@ -36,6 +36,7 @@ class Intro : public QDialog
36 
37     QString getDataDirectory();
38     void setDataDirectory(const QString &dataDir);
39+    int64_t getPrune() const;

hebasto commented at 9:36 am on November 16, 2020:

dd50e8a312baba4340bf8854d4e0c96bfadf1763

nit: s/getPrune/getPruneMiB/ ?

in src/qt/intro.cpp:202 in dd50e8a312 outdated

187+    switch (ui->prune->checkState()) {
188+    case Qt::Checked:
189+         return PruneGBtoMiB(m_prune_target_gb);
190+    case Qt::Unchecked: default:
191+        return 0;
192+    }

hebasto commented at 9:43 am on November 16, 2020:

dd50e8a312baba4340bf8854d4e0c96bfadf1763

Using the switch statement seems overkill here. Why not simple if (ui->prune->isChecked())?

luke-jr commented at 3:18 am on November 19, 2020:

It becomes tri-state in a followup PR.

hebasto commented at 5:50 am on November 19, 2020:

Have you a branch to look into upcoming changes?

jarolrod commented at 8:35 am on November 30, 2020:

@luke-jr is this still going to become tri-state? otherwise hebasto’s suggestion would be preferred

in src/qt/forms/intro.ui:241 in 77f3f2dda6 outdated

206+      <widget class="QCheckBox" name="prune">
207+       <property name="text">
208+        <string>Limit block chain storage to</string>
209+       </property>
210+       <property name="toolTip">
211+        <string>Reverting this setting requires re-downloading the entire blockchain. It is faster to download the full chain first and prune it later. Disables some advanced features.</string>

hebasto commented at 9:47 am on November 16, 2020:

77f3f2dda6918b0986f785b3061cfb8c97ed2b5c

s/blockchain/block chain/

luke-jr commented at 3:25 am on November 19, 2020:

“blockchain” and “block chain” are tied in existing strings. I think the single word is better.

hebasto commented at 5:56 am on November 19, 2020:

Screenshot from 2020-11-19 07-54-29

https://github.com/bitcoinops/bitcoinops.github.io/blob/master/STYLE.md

in src/qt/forms/intro.ui:256 in 7d40a48052 outdated

218@@ -219,6 +219,13 @@
219        </property>
220       </widget>
221      </item>
222+     <item>
223+      <widget class="QLabel" name="lblPruneSuffix">
224+       <property name="buddy">
225+        <cstring>pruneGB</cstring>
226+       </property>

hebasto commented at 10:08 am on November 16, 2020:

7d40a4805229618a364dc53a84cd5af2ac273b7d Where is this used?

luke-jr commented at 3:30 am on November 19, 2020:

It’s here for correctness. I don’t know if there’s a platform that acts on it.

hebasto commented at 5:52 am on November 19, 2020:

It’s here for correctness. I don’t know if there’s a platform that acts on it.

I cannot parse this message. Correctness of what?

hebasto commented at 2:50 am on March 24, 2021:

@luke-jr

From QLabel::setBuddy:

The buddy mechanism is only available for QLabels that contain text in which one character is prefixed with an ampersand, ‘&’. This character is set as the shortcut key.

You do not set a shortcut key, therefore linking lblPruneSuffix to pruneGB buddy is noop, and should be dropped.

in src/qt/intro.cpp:386 in 7d40a48052 outdated

381@@ -381,6 +382,9 @@ void Intro::UpdatePruneLabels(bool prune_checked)
382     }
383     ui->lblExplanation3->setVisible(prune_checked);
384     ui->pruneGB->setEnabled(prune_checked);
385+    static const uint64_t nPowTargetSpacing = 10 * 60;  // from chainparams, which we don't have at this stage
386+    static const uint32_t expected_block_data_size = 2250000;  // includes undo data

hebasto commented at 10:09 am on November 16, 2020:

7d40a4805229618a364dc53a84cd5af2ac273b7d, nit:

0    static constexpr uint64_t nPowTargetSpacing = 10 * 60;  // from chainparams, which we don't have at this stage
1    static constexpr uint32_t expected_block_data_size = 2250000;  // includes undo data

hebasto changes_requested

hebasto commented at 10:12 am on November 16, 2020: member

Concept ACK 7d40a4805229618a364dc53a84cd5af2ac273b7d.

Master (fb7726e56d0f0617076c89cbc279547cc9d07341): DeepinScreenshot_select-area_20201116112204

This PR: DeepinScreenshot_select-area_20201116112251

The visual distance between two closely related items has been increased (see the screeshots above). This change makes harder for novices to grasp the pruning details. I’d suggest the opposite: to place those items side by side.

GUI/Intro: Return actual prune setting from showIfNeeded 62932cc686

GUI/Intro: Abstract GUI-to-option into Intro::getPrune f2e5a6b54f

GUI/Intro: Rework UI flow to let the user set prune size in GBs e2dcd957fa

luke-jr commented at 3:32 am on November 19, 2020: member

Addressed @hebasto’s comments.

Ping @promag @ryanofsky @Bosch-0 for re-ACKing

Bosch-0 commented at 3:37 am on November 19, 2020: none

@Bosch-0 Does rescanning a wallet with a pruned node that misses relevant blocks result in an incorrect balance? That sounds like a bug - it should just abort with missing blocks or so.

I haven’t actually tested this but I don’t think it does - will test this today. The only time an incorrect balance may be shown is when importing a wallet post-prune that has tx history older than the block history stored as far as I’m concerned.

luke-jr requested review from promag on Nov 19, 2020

luke-jr requested review from ryanofsky on Nov 19, 2020

luke-jr requested review from hebasto on Nov 19, 2020

Bosch-0 commented at 4:03 am on November 19, 2020: none

ACK ea7926527ce36c05cacb99602b2b59579ff646e8, looks good.

hebasto commented at 5:43 am on November 19, 2020: member

ACK ea79265, looks good.

Which commit did you mean?

hebasto commented at 5:53 am on November 19, 2020: member

@luke-jr

Addressed @hebasto’s comments.

Sorry, but I don’t think just that marking a comment as resolved means that it is addressed. If the current state of code is preferred that could be stated explicitly, right?

Bosch-0 commented at 7:13 am on November 19, 2020: none

Which commit did you mean?

I just re-tested the changes made since my first ACK, I did not review the code

hebasto commented at 9:39 am on November 19, 2020: member

Which commit did you mean?

I just re-tested the changes made since my first ACK, I did not review the code

I mean that ACK should be followed by the top commit of the pr branch, i.e. 7d40a4805229618a364dc53a84cd5af2ac273b7d for now, not the master branch (that ea7926527ce36c05cacb99602b2b59579ff646e8 is).

ryanofsky commented at 12:17 pm on November 19, 2020: contributor

Is an update supposed to be pushed? My review was requested but there haven’t been any changes since my last review #125#pullrequestreview-528515367

luke-jr force-pushed on Nov 19, 2020

luke-jr commented at 3:59 pm on November 19, 2020: member

Ah, that’s the problem: Forgot we have to push to a different repo to get PRs in /gui to update :/

Should be ready now

in src/qt/intro.cpp:387 in 28ec1b239f outdated

380@@ -361,6 +381,11 @@ void Intro::UpdatePruneLabels(bool prune_checked)
381         storageRequiresMsg = tr("Approximately %1 GB of data will be stored in this directory.");
382     }
383     ui->lblExplanation3->setVisible(prune_checked);
384+    ui->pruneGB->setEnabled(prune_checked);
385+    static constexpr uint64_t nPowTargetSpacing = 10 * 60;  // from chainparams, which we don't have at this stage
386+    static constexpr uint32_t expected_block_data_size = 2250000;  // includes undo data
387+    static const uint64_t expected_backup_days = m_prune_target_gb * 1e9 / (uint64_t(expected_block_data_size) * 86400 / nPowTargetSpacing);

jarolrod commented at 5:03 pm on November 28, 2020:

setting expected_backup_days as a static const means that the number of days the prune target is good for will never update as the prune target is changed.

0    uint64_t expected_backup_days = m_prune_target_gb * 1e9 / (uint64_t(expected_block_data_size) * 86400 / nPowTargetSpacing);

luke-jr commented at 2:46 am on November 30, 2020:

Fixed (const can remain)

jarolrod changes_requested

GUI/Intro: Estimate max age of backups that can be restored with pruning 2a84c6bcf6

GUI/Intro: Move prune setting below explanation 415fb2e1ab

luke-jr force-pushed on Nov 30, 2020

ryanofsky approved

ryanofsky commented at 4:11 pm on December 1, 2020: contributor

Code review ACK 415fb2e1abac189fcbe8eccf2ea065724d17460f. Changes since last review: mb/gib suffixes, constexpr QOverload expected_backup_days tweaks, new moveonly layout commit

jarolrod approved

jarolrod commented at 3:13 am on December 2, 2020: member

Tested ACK 415fb2e. Stress-tested on Ubuntu 20.10 and Arch Linux. Synced a node on each machine with no issues. Ubuntu machine set to 6gb prune target, Arch machine set to 9gb prune target.

luke-jr commented at 2:00 am on February 1, 2021: member

QOverload doesn’t work in Qt 5.5 … reverted that change

luke-jr force-pushed on Feb 1, 2021

luke-jr force-pushed on Mar 2, 2021

luke-jr commented at 3:36 am on March 2, 2021: member

master is bumped to Qt 5.9, so went back to QOverload…

Talkless commented at 6:32 pm on March 16, 2021: none

@luke-jr feeling dejavu - I see Intro::getPruneMiB() added here and int #149 :)

Talkless approved

Talkless commented at 7:03 pm on March 16, 2021: none

tACK 415fb2e1abac189fcbe8eccf2ea065724d17460f, tested on Debian Sid with Qt 5.15.2.

in src/qt/bitcoin.cpp:509 in 415fb2e1ab

505@@ -508,9 +506,9 @@ int GuiMain(int argc, char* argv[])
506     /// 5. Now that settings and translations are available, ask user for data directory
507     // User language is set up: pick a data directory
508     bool did_show_intro = false;
509-    bool prune = false; // Intro dialog prune check box
510+    int64_t prune_MiB = 0;  // Intro dialog prune configuration

hebasto commented at 2:25 am on March 24, 2021:

style piconit: clang-format-diff.py suggests

0    int64_t prune_MiB = 0; // Intro dialog prune configuration

in src/qt/forms/intro.ui:242 in 415fb2e1ab

237+       <property name="text">
238+        <string>Limit block chain storage to</string>
239+       </property>
240+       <property name="toolTip">
241+        <string>Reverting this setting requires re-downloading the entire blockchain. It is faster to download the full chain first and prune it later. Disables some advanced features.</string>
242+       </property>

hebasto commented at 2:29 am on March 24, 2021:

Qt Designer insists on changing properties order:

0       <property name="toolTip">
1        <string>Reverting this setting requires re-downloading the entire blockchain. It is faster to download the full chain first and prune it later. Disables some advanced features.</string>
2       </property>
3       <property name="text">
4        <string>Limit block chain storage to</string>
5       </property>

in src/qt/intro.cpp:143 in 415fb2e1ab

139@@ -139,17 +140,26 @@ Intro::Intro(QWidget *parent, int64_t blockchain_size_gb, int64_t chain_state_si
140     );
141     ui->lblExplanation2->setText(ui->lblExplanation2->text().arg(PACKAGE_NAME));
142 
143+    const int min_prune_target_GB = std::ceil(MIN_DISK_SPACE_FOR_BLOCK_FILES / 1e9);

hebasto commented at 2:34 am on March 24, 2021:

nit: why here and after not use https://github.com/bitcoin-core/gui/blob/fa2a5b8f3aeb978b64189203cbbe95a00ac92a6a/src/qt/guiconstants.h#L52-L53 ?

in src/qt/intro.cpp:158 in 415fb2e1ab

154 
155     connect(ui->prune, &QCheckBox::toggled, [this](bool prune_checked) {
156         UpdatePruneLabels(prune_checked);
157         UpdateFreeSpaceLabel();
158     });
159+    connect(ui->pruneGB, QOverload<int>::of(&QSpinBox::valueChanged), [this](int prune_GB) {

hebasto commented at 2:36 am on March 24, 2021:

The QOverload helper class is required for C++11 code. Having C++17, the qOverload should be used.

Talkless commented at 6:21 pm on March 31, 2021:

IIRC qOverload function template does not work with Visual Studio 2017 due to implementation limitations, and I see that compiler mentioned in https://github.com/bitcoin-core/gui/blob/master/build_msvc/README.md .

hebasto commented at 7:56 pm on March 31, 2021:

IIRC qOverload function template does not work with Visual Studio 2017 due to implementation limitations

Oh, I was not aware of it. I’d like to know more about this pitfall. Any proof/link/test is appreciated :)

Talkless commented at 5:40 pm on May 3, 2021:

@hebasto see https://bugreports.qt.io/browse/QTBUG-61667?focusedCommentId=462231&page=com.atlassian.jira.plugin.system.issuetabpanels%3Acomment-tabpanel#comment-462231

hebasto commented at 6:10 pm on May 3, 2021:

@Talkless

Thanks! Since https://github.com/bitcoin/bitcoin/pull/21811 is merged, it isn’t a problem, is it?

Btw, could you look at #257?

Talkless commented at 5:36 pm on May 5, 2021:

It seem so, yes. Maybe I should try building on some older GCC to make sure other compiler does not have similar limitations (though I doubt it).

I will take a look at 257 at some time.

in src/qt/intro.cpp:201 in 415fb2e1ab

197+{
198+    switch (ui->prune->checkState()) {
199+    case Qt::Checked:
200+        return PruneGBtoMiB(m_prune_target_gb);
201+    case Qt::Unchecked: default:
202+        return 0;

hebasto commented at 2:39 am on March 24, 2021:

0    case Qt::Unchecked:
1    [[fallthrough]];
2    default:
3        return 0;

in src/qt/intro.cpp:386 in 415fb2e1ab

380@@ -361,6 +381,11 @@ void Intro::UpdatePruneLabels(bool prune_checked)
381         storageRequiresMsg = tr("Approximately %1 GB of data will be stored in this directory.");
382     }
383     ui->lblExplanation3->setVisible(prune_checked);
384+    ui->pruneGB->setEnabled(prune_checked);
385+    static constexpr uint64_t nPowTargetSpacing = 10 * 60;  // from chainparams, which we don't have at this stage
386+    static constexpr uint32_t expected_block_data_size = 2250000;  // includes undo data

hebasto commented at 2:41 am on March 24, 2021:

style piconit: clang-format-diff.py suggests

0    static constexpr uint64_t nPowTargetSpacing = 10 * 60;        // from chainparams, which we don't have at this stage
1    static constexpr uint32_t expected_block_data_size = 2250000; // includes undo data

in src/qt/intro.h:39 in 415fb2e1ab

35@@ -36,6 +36,7 @@ class Intro : public QDialog
36 
37     QString getDataDirectory();
38     void setDataDirectory(const QString &dataDir);
39+    int64_t getPruneMiB() const;

hebasto commented at 2:45 am on March 24, 2021:

Could be private?

hebasto commented at 2:51 am on March 24, 2021: member

Tested 415fb2e1abac189fcbe8eccf2ea065724d17460f on Linux Mint 20.1 (Qt 5.12.8). Works great!

I believe the commit “GUI/Intro: Move prune setting below explanation” (415fb2e1abac189fcbe8eccf2ea065724d17460f) should be reordered and squashed into the “GUI/Intro: Rework UI flow to let the user set prune size in GBs” (e2dcd957fa206a28404ff824fb764b8889705fb2).

hebasto added the label Feature on Mar 25, 2021

hebasto approved

hebasto commented at 8:30 pm on April 29, 2021: member

ACK 415fb2e1abac189fcbe8eccf2ea065724d17460f, my unresolved comments are not blockers, and they could be resolved in follow ups.

hebasto merged this on Apr 29, 2021

hebasto closed this on Apr 29, 2021

sidhujag referenced this in commit c908b2a13e on Apr 30, 2021

Rspigler commented at 9:41 pm on April 30, 2021: contributor

Post-merge tACK. Great improvement!

hebasto referenced this in commit b789914f17 on May 27, 2021

hebasto added the label Needs release notes on Jun 28, 2021

hebasto commented at 4:24 am on June 28, 2021: member

@luke-jr

~Could you add release notes into https://github.com/bitcoin-core/bitcoin-devwiki/wiki/22.0-Release-Notes-draft#gui-changes?~

See IRC discussion:

04:22:48 <hebasto> also I think that #125 by luke-jr should be included as well 04:27:06 <luke-jr> hebasto: not sure release notes apply to firstrun things? XD

hebasto added this to the milestone 22.0 on Jun 28, 2021

hebasto removed the label Needs release notes on Jun 28, 2021

gwillen referenced this in commit 1e75f936fb on Jun 1, 2022

bitcoin-core locked this on Aug 16, 2022

Enable changing the autoprune block space size in intro dialog #125