Enable changing the autoprune block space size in intro dialog by luke-jr · Pull Request #125 · bitcoin-core/gui

luke-jr · 2020-10-29T16:15:45Z

Moved from bitcoin/bitcoin#18728

jonasschnelli · 2020-10-29T17:54:52Z

Concept ACK

promag

Concept ACK.

src/qt/bitcoin.cpp

src/qt/intro.cpp

promag · 2020-11-01T15:49:35Z

src/qt/intro.cpp


 #include <interfaces/node.h>
 #include <util/system.h>
+#include <validation.h>


77f3f2d

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

src/qt/intro.cpp

Bosch-0 · 2020-11-09T07:03:13Z

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.

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

Code review ACK 7d40a48

promag

Tested ACK 7d40a48.

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

Sjors · 2020-11-12T12:45:29Z

Concept ACK

sipa · 2020-11-14T00:56:39Z

@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 · 2020-11-16T08:13:33Z

@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., bitcoin/bitcoin#20263.

hebasto

Concept ACK 7d40a48.

Master (fb7726e):

This PR:

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.

src/qt/bitcoin.cpp

src/qt/intro.h

src/qt/intro.cpp

src/qt/forms/intro.ui

src/qt/intro.cpp

src/qt/forms/intro.ui

src/qt/intro.cpp

luke-jr · 2020-11-19T03:32:17Z

Addressed @hebasto's comments.

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

Bosch-0 · 2020-11-19T03:37:02Z

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

Bosch-0 · 2020-11-19T04:03:21Z

ACK ea79265, looks good.

hebasto · 2020-11-19T05:43:54Z

ACK ea79265, looks good.

Which commit did you mean?

hebasto · 2020-11-19T05:53:11Z

@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 · 2020-11-19T07:13:17Z

Which commit did you mean?

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

hebasto · 2020-11-19T09:39:46Z

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. 7d40a48 for now, not the master branch (that ea79265 is).

ryanofsky · 2020-11-19T12:17:08Z

Is an update supposed to be pushed? My review was requested but there haven't been any changes since my last review #125 (review)

luke-jr · 2020-11-19T15:59:21Z

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

src/qt/intro.cpp

ryanofsky

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

jarolrod

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 · 2021-02-01T02:00:02Z

QOverload doesn't work in Qt 5.5 ... reverted that change

luke-jr · 2021-03-02T03:36:39Z

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

Talkless · 2021-03-16T18:32:27Z

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

Talkless

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

hebasto

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

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

hebasto · 2021-03-24T02:25:03Z

src/qt/bitcoin.cpp

    // User language is set up: pick a data directory
    bool did_show_intro = false;
-    bool prune = false; // Intro dialog prune check box
+    int64_t prune_MiB = 0;  // Intro dialog prune configuration


style piconit: clang-format-diff.py suggests

Suggested change

int64_t prune_MiB = 0; // Intro dialog prune configuration

int64_t prune_MiB = 0; // Intro dialog prune configuration

hebasto · 2021-03-24T02:29:45Z

src/qt/forms/intro.ui

+       <property name="text">
+        <string>Limit block chain storage to</string>
+       </property>
+       <property name="toolTip">
+        <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>
+       </property>


Qt Designer insists on changing properties order:

Suggested change

<property name="text">

<string>Limit block chain storage to</string>

</property>

<property name="toolTip">

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

</property>

<property name="toolTip">

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

</property>

<property name="text">

<string>Limit block chain storage to</string>

</property>

hebasto · 2021-03-24T02:34:08Z

src/qt/intro.cpp

    );
    ui->lblExplanation2->setText(ui->lblExplanation2->text().arg(PACKAGE_NAME));

+    const int min_prune_target_GB = std::ceil(MIN_DISK_SPACE_FOR_BLOCK_FILES / 1e9);


nit: why here and after not use

gui/src/qt/guiconstants.h

Lines 52 to 53 in fa2a5b8

/* One gigabyte (GB) in bytes */

static constexpr uint64_t GB_BYTES{1000000000};

?

hebasto · 2021-03-24T02:36:58Z

src/qt/intro.cpp

        UpdatePruneLabels(prune_checked);
        UpdateFreeSpaceLabel();
    });
+    connect(ui->pruneGB, QOverload<int>::of(&QSpinBox::valueChanged), [this](int prune_GB) {


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

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 .

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

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

@Talkless

Thanks! Since bitcoin/bitcoin#21811 is merged, it isn't a problem, is it?

Btw, could you look at #257?

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.

hebasto · 2021-03-24T02:39:07Z

src/qt/intro.cpp

+    case Qt::Unchecked: default:
+        return 0;


Suggested change

case Qt::Unchecked: default:

return 0;

case Qt::Unchecked:

[[fallthrough]];

default:

return 0;

hebasto · 2021-03-24T02:41:23Z

src/qt/intro.cpp

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


style piconit: clang-format-diff.py suggests

Suggested change

static constexpr uint64_t nPowTargetSpacing = 10 * 60; // from chainparams, which we don't have at this stage

static constexpr uint32_t expected_block_data_size = 2250000; // includes undo data

static constexpr uint64_t nPowTargetSpacing = 10 * 60; // from chainparams, which we don't have at this stage

static constexpr uint32_t expected_block_data_size = 2250000; // includes undo data

hebasto · 2021-03-24T02:45:41Z

src/qt/intro.h


    QString getDataDirectory();
    void setDataDirectory(const QString &dataDir);
+    int64_t getPruneMiB() const;


Could be private?

src/qt/forms/intro.ui

hebasto

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

…… … size in intro dialog

Rspigler · 2021-04-30T21:41:40Z

Post-merge tACK. Great improvement!

8b77133 qt: Replace disambiguation strings with translator comments (Hennadii Stepanov) Pull request description: Since bitcoin/bitcoin#21694 is merged, translator comments is the right way to pass context to translators. This PR fixes changes were made: - in #220 before bitcoin/bitcoin#21694 - in bitcoin/bitcoin#21694 on testing purpose - in #125 Closes #288. ACKs for top commit: jarolrod: ACK 8b77133 Tree-SHA512: 466ade35f4969a41fbf3196780b1ae9fa810bab5d2f09077f8631604636cc63b24a901c719f6b5797366d2aa307993d0aa419ce35200c8d0a741a3d81cad3e6b

hebasto · 2021-06-28T04:24:53Z

@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

promag reviewed Nov 1, 2020

View reviewed changes

ryanofsky approved these changes Nov 11, 2020

View reviewed changes

maflcko changed the title ~~GUI: Enable changing the autoprune block space size in intro dialog~~ Enable changing the autoprune block space size in intro dialog Nov 12, 2020

promag reviewed Nov 12, 2020

View reviewed changes

luke-jr mentioned this pull request Nov 13, 2020

Avoid pruning blocks with transactions in wallets (even after wallets sync) bitcoin/bitcoin#20384

Open

hebasto requested changes Nov 16, 2020

View reviewed changes

luke-jr added 3 commits November 19, 2020 03:15

GUI/Intro: Return actual prune setting from showIfNeeded

62932cc

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

f2e5a6b

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

e2dcd95

luke-jr requested review from hebasto, promag and ryanofsky November 19, 2020 03:44

luke-jr force-pushed the intro_prune_size branch from 7d40a48 to 28ec1b2 Compare November 19, 2020 15:58

jarolrod suggested changes Nov 28, 2020

View reviewed changes

src/qt/intro.cpp Outdated Show resolved Hide resolved

luke-jr added 2 commits November 30, 2020 02:46

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

2a84c6b

GUI/Intro: Move prune setting below explanation

415fb2e

luke-jr force-pushed the intro_prune_size branch from 28ec1b2 to 415fb2e Compare November 30, 2020 02:46

ryanofsky approved these changes Dec 1, 2020

View reviewed changes

jarolrod approved these changes Dec 1, 2020

View reviewed changes

jarolrod approved these changes Dec 2, 2020

View reviewed changes

luke-jr mentioned this pull request Dec 10, 2020

Intro: Have user choose assumevalid #149

Closed

luke-jr force-pushed the intro_prune_size branch from 415fb2e to a479be9 Compare February 1, 2021 02:00

luke-jr force-pushed the intro_prune_size branch from a479be9 to 415fb2e Compare March 2, 2021 03:36

Talkless approved these changes Mar 16, 2021

View reviewed changes

hebasto reviewed Mar 24, 2021

View reviewed changes

hebasto added the Feature label Mar 25, 2021

hebasto mentioned this pull request Apr 29, 2021

refactor: Use template function qOverload in signal-slot connections #257

Merged

hebasto approved these changes Apr 29, 2021

View reviewed changes

hebasto merged commit e1e1e70 into bitcoin-core:master Apr 29, 2021

sidhujag pushed a commit to syscoin/syscoin that referenced this pull request Apr 30, 2021

Merge bitcoin-core/gui#125: Enable changing the autoprune block space…

c908b2a

…… … size in intro dialog

hebasto mentioned this pull request May 15, 2021

Replace disambiguation strings with translator comments #332

Merged

hebasto added the Needs release notes label Jun 28, 2021

hebasto added this to the 22.0 milestone Jun 28, 2021

hebasto removed the Needs release notes label Jun 28, 2021

gwillen pushed a commit to ElementsProject/elements that referenced this pull request Jun 1, 2022

Merge e1e1e70 into merged_master (Bitcoin PR bitcoin-core/gui#125)

1e75f93

bitcoin-core locked as resolved and limited conversation to collaborators Aug 16, 2022

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

	/* One gigabyte (GB) in bytes */
	static constexpr uint64_t GB_BYTES{1000000000};

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

Conversation

luke-jr commented Oct 29, 2020

Uh oh!

jonasschnelli commented Oct 29, 2020

Uh oh!

promag left a comment

Choose a reason for hiding this comment

Uh oh!

Uh oh!

Uh oh!

Choose a reason for hiding this comment

Uh oh!

Uh oh!

Uh oh!

Bosch-0 commented Nov 9, 2020

Uh oh!

ryanofsky left a comment

Choose a reason for hiding this comment

Uh oh!

promag left a comment

Choose a reason for hiding this comment

Uh oh!

Sjors commented Nov 12, 2020

Uh oh!

sipa commented Nov 14, 2020

Uh oh!

hebasto commented Nov 16, 2020

Uh oh!

hebasto left a comment

Choose a reason for hiding this comment

Uh oh!

Uh oh!

Uh oh!

Uh oh!

Uh oh!

Uh oh!

Uh oh!

Uh oh!

luke-jr commented Nov 19, 2020

Uh oh!

Bosch-0 commented Nov 19, 2020

Uh oh!

Bosch-0 commented Nov 19, 2020

Uh oh!

hebasto commented Nov 19, 2020

Uh oh!

hebasto commented Nov 19, 2020

Uh oh!

Bosch-0 commented Nov 19, 2020

Uh oh!

hebasto commented Nov 19, 2020

Uh oh!

ryanofsky commented Nov 19, 2020

Uh oh!

luke-jr commented Nov 19, 2020 • edited Loading Uh oh! There was an error while loading. Please reload this page.

Uh oh!

Uh oh!

Uh oh!

ryanofsky left a comment

Choose a reason for hiding this comment

Uh oh!

jarolrod left a comment

Choose a reason for hiding this comment

Uh oh!

luke-jr commented Feb 1, 2021

Uh oh!

luke-jr commented Mar 2, 2021

Uh oh!

Talkless commented Mar 16, 2021 • edited Loading Uh oh! There was an error while loading. Please reload this page.

Uh oh!

Uh oh!

Talkless left a comment

Choose a reason for hiding this comment

Uh oh!

hebasto left a comment

Choose a reason for hiding this comment

Uh oh!

Choose a reason for hiding this comment

Uh oh!

Choose a reason for hiding this comment

luke-jr commented Nov 19, 2020 •

edited

Loading

Talkless commented Mar 16, 2021 •

edited

Loading

hebasto commented Jun 28, 2021 •

edited

Loading