Intro: Have user choose assumevalid

luke-jr commented at 11:30 pm on December 10, 2020: member

Context: https://twitter.com/notgrubles/status/1297671475059728390

luke-jr force-pushed on Dec 10, 2020

Sjors commented at 10:32 am on December 11, 2020: member

Concept ACK on allowing the GUI user to opt-out of -assumevalid. The text needs some work, e.g. it should explain what is and isn’t validated.

luke-jr commented at 3:42 pm on December 11, 2020: member

@Sjors Any suggestions for the text?

Note that validation is largely a black-and-white thing: unless you validate everything, you might as well be validating nothing. I think the only exception is the PoW?

Sjors commented at 4:59 pm on December 11, 2020: member

I’m pretty sure it also checks that coins aren’t created out of nothing (i.e. it checks a coin spends outpoints and tracks which new ones it creates, and that the amounts add up).

in src/qt/intro.cpp:228 in 15f007c09b outdated

222@@ -182,9 +223,27 @@ void Intro::setDataDirectory(const QString &dataDir)
223     }
224 }
225 
226-bool Intro::showIfNeeded(bool& did_show_intro, bool& prune)
227+int64_t Intro::getPruneMiB() const
228 {
229-    did_show_intro = false;
230+    switch (ui->prune->checkState()) {

Talkless commented at 3:03 pm on March 13, 2021:

It feels like oneliner return (ui->prune->checkState() == Qt::Checked) ? PruneGBtoMiB(m_prune_target_gb) : 0 would look simpler (or just if/else) instead of “full-blown” switch..?

luke-jr commented at 7:12 pm on March 23, 2022:

n/a to this PR

in src/qt/intro.cpp:239 in 15f007c09b outdated

236+}
237+
238+QString Intro::getAssumeValid() const
239+{
240+    if (!ui->assumevalid->isChecked()) {
241+        return "0";

Talkless commented at 3:03 pm on March 13, 2021:

QStringLiteral(“0”);

luke-jr commented at 8:41 pm on March 23, 2022:

done

in src/qt/intro.cpp:270 in 15f007c09b outdated

269-        intro.setDataDirectory(dataDir);
270-        intro.setWindowIcon(QIcon(":icons/bitcoin"));
271-        did_show_intro = true;
272+        intro.reset(new Intro(0, Params().AssumedBlockchainSize(), Params().AssumedChainStateSize()));
273+        intro->setDataDirectory(dataDir);
274+        intro->setWindowIcon(QIcon(":icons/bitcoin"));

Talkless commented at 3:07 pm on March 13, 2021:

nit: QStringLiteral(":icons/bitcoin") ?

luke-jr commented at 8:41 pm on March 23, 2022:

Not new code here, but ok.

in src/qt/intro.cpp:268 in 15f007c09b outdated

264@@ -206,19 +265,18 @@ bool Intro::showIfNeeded(bool& did_show_intro, bool& prune)
265         }
266 
267         /* If current default data directory does not exist, let the user choose one */
268-        Intro intro(0, Params().AssumedBlockchainSize(), Params().AssumedChainStateSize());
269-        intro.setDataDirectory(dataDir);
270-        intro.setWindowIcon(QIcon(":icons/bitcoin"));
271-        did_show_intro = true;
272+        intro.reset(new Intro(0, Params().AssumedBlockchainSize(), Params().AssumedChainStateSize()));

Talkless commented at 3:15 pm on March 13, 2021:

nanonit:

0intro = std::make_unique<Intro>(0, Params().AssumedBlockchainSize(), Params().AssumedChainStateSize());

for more idiomatic (“avoid new/delete”) C++ :)

luke-jr commented at 8:41 pm on March 23, 2022:

idk, I think reset(new was better/more readable, but changed anyway.

in src/qt/intro.cpp:421 in 15f007c09b outdated

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

Talkless commented at 3:18 pm on March 13, 2021:

Where does this constant comes from?

luke-jr commented at 7:24 pm on March 23, 2022:

Real-world block size limit + undo data. But not part of this PR anyway.

in src/qt/intro.h:79 in 15f007c09b outdated

75@@ -72,7 +76,7 @@ private Q_SLOTS:
76     //! Total required space (in GB) depending on user choice (prune or not prune).
77     int64_t m_required_space_gb{0};
78     uint64_t m_bytes_available{0};
79-    const int64_t m_prune_target_gb;
80+    int64_t m_prune_target_gb;

Talkless commented at 3:22 pm on March 13, 2021:

maybe still worth having {0} in case some wild new constructor overload appears?

luke-jr commented at 7:25 pm on March 23, 2022:

n/a

in src/qt/intro.cpp:191 in 15f007c09b outdated

187+            ui->assumevalidBlock->setReadOnly(true);
188+        }
189+    }
190+    {
191+        // TODO: Ideally, we would include actual margins here (instead of extra digits), but this seems non-trivial
192+        const int text_width = ui->assumevalidBlock->fontMetrics().width("4") * (64 + 4);

Talkless commented at 3:29 pm on March 13, 2021:

0../../src/qt/intro.cpp:191:77: warning: ‘int QFontMetrics::width(const QString&, int) const’ is deprecated: Use QFontMetrics::horizontalAdvance [-Wdeprecated-declarations]
1  191 |         const int text_width = ui->assumevalidBlock->fontMetrics().width("4") * (64 + 4);

Also nit: QStringLiteral("4").

Talkless commented at 3:30 pm on March 13, 2021: none

Concept ACK

hebasto added the label Feature on Mar 25, 2021

Rspigler commented at 10:49 pm on April 30, 2021: contributor

Hm, I keep getting errors when trying to compile

make:

qt/intro.cpp: In constructor ‘Intro::Intro(QWidget*, int64_t, int64_t)’: qt/intro.cpp:178:52: error: invalid initialization of reference of type ‘const string&’ {aka ‘const std::__cxx11::basic_string&’} from expression of type ‘ArgsManager’ const auto chainparams = CreateChainParams(gArgs, gArgs.GetChainName()); ^~~~~ In file included from qt/intro.cpp:9: ./chainparams.h:117:37: note: in passing argument 1 of ‘std::unique_ptr CreateChainParams(const string&)’ std::unique_ptr CreateChainParams(const std::string& chain); ^~~~~~~~~~~~~~~~~ make[2]: *** [Makefile:13191: qt/libbitcoinqt_a-intro.o] Error 1 make[2]: Leaving directory ‘/home/user/gui/src’ make[1]: *** [Makefile:18989: all-recursive] Error 1 make[1]: Leaving directory ‘/home/user/gui/src’ make: *** [Makefile:798: all-recursive] Error 1

DrahtBot added the label Needs rebase on Jul 22, 2021

Rspigler commented at 11:41 pm on December 21, 2021: contributor

~~Still getting errors trying to compile~~ Duh

luke-jr force-pushed on Mar 23, 2022

luke-jr commented at 8:42 pm on March 23, 2022: member

Rebased, reviews addressed

in src/qt/intro.cpp:191 in 253e500c9a outdated

186+            ui->assumevalidBlock->setReadOnly(true);
187+        }
188+    }
189+    {
190+        // TODO: Ideally, we would include actual margins here (instead of extra digits), but this seems non-trivial
191+#if (QT_VERSION >= QT_VERSION_CHECK(5, 11, 0))

fanquake commented at 8:46 pm on March 23, 2022:

Our minimum required Qt is already 5.11

luke-jr commented at 8:53 pm on March 23, 2022:

Oh, nice. Dropped compat code.

DrahtBot removed the label Needs rebase on Mar 23, 2022

luke-jr force-pushed on Mar 23, 2022

Talkless commented at 5:47 pm on March 28, 2022: none

Sorry for a lot of “n/a’s” @luke-jr , I guess I need to up my diff reading game…

DrahtBot added the label Needs rebase on Apr 26, 2022

luke-jr force-pushed on Apr 30, 2022

DrahtBot removed the label Needs rebase on Apr 30, 2022

DrahtBot added the label Needs rebase on Jun 15, 2022

Rspigler commented at 5:09 pm on June 19, 2022: contributor

tACK 75aff9e0ff7f98a5ec64d18641a9654ab5b09cdc Simple, I like the language, and I think it is an important setting the user should set

laanwj commented at 8:15 am on June 20, 2022: member

Concept ACK, this choice seems sufficiently important to me to expose in the GUI.

GUI: Intro: Output a std::unique_ptr<Intro> from Intro::showIfNeeded f06cf10ed0

GUI: Intro: Have user choose assumevalid cf940f0e5f

luke-jr force-pushed on Nov 15, 2022

luke-jr commented at 5:02 am on November 15, 2022: member

Rebased

DrahtBot removed the label Needs rebase on Nov 15, 2022

DrahtBot commented at 11:58 pm on November 15, 2022: contributor

The following sections might be updated with supplementary metadata relevant to reviewers and maintainers.

Reviews

See the guideline for information on the review process.

Type	Reviewers
ACK	pablomartin4btc
Concept ACK	Sjors, Talkless, laanwj, kristapsk
Approach ACK	hebasto
Stale ACK	Rspigler

If your review is incorrectly listed, please react with 👎 to this comment and the bot will ignore it on the next update.

Conflicts

Reviewers, this pull request conflicts with the following ones:

#bitcoin/bitcoin/28186 (kernel: Prune leveldb headers by TheCharlatan)

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.

DrahtBot added the label CI failed on May 15, 2023

DrahtBot added the label Needs rebase on Jun 9, 2023

DrahtBot commented at 8:48 pm on June 9, 2023: contributor

🐙 This pull request conflicts with the target branch and needs rebase.

hebasto commented at 6:16 pm on September 22, 2023: member

Please rebase if this PR is still relevant.

pablomartin4btc commented at 8:51 pm on October 5, 2023: contributor

tACK cf940f0e5f5ffb14ec7b6f751879bd70b3c350be

Without -assumevalid its checkbox is ticked:

Witth -assumevalid its checkbox is un-ticked:

On -signet & -tesnet behave similarly, block hash updates correctly ofc.

On -regtest the layoutAssumeValid is not visible:

Shouldn’t we reduce the empty space/ windows size a bit when the layoutAssumeValid is not visible? As currently without this PR:

I think this shouldn’t happen -assumevalid=23aaa:

in src/qt/intro.cpp:176 in cf940f0e5f

171+            // -assumevalid=blockhash: initialise with the user-specified value, enabled
172+            ui->assumevalid->setChecked(true);
173+            ui->assumevalidBlock->setText(QString::fromStdString(user_assumevalid));
174+            have_user_assumevalid = true;
175+        }
176+    }

pablomartin4btc commented at 9:14 pm on October 5, 2023:

nit: while not mandatory nor blocker, maybe you could consider refactoring these if statements into functions like the above UpdateFreeSpaceLabel(), as it could improve code readability and maintainability, especially as we keep adding more logic.

DrahtBot commented at 0:41 am on January 3, 2024: contributor

There hasn’t been much activity lately and the patch still needs rebase. What is the status here?

Is it still relevant? ➡️ Please solve the conflicts to make it ready for review and to ensure the CI passes.
Is it no longer relevant? ➡️ Please close.
Did the author lose interest or time to work on this? ➡️ Please close it and mark it ‘Up for grabs’ with the label, so that it can be picked up in the future.

kristapsk commented at 0:45 am on January 3, 2024: contributor

Concept ACK

DrahtBot requested review from laanwj on Jan 3, 2024

DrahtBot requested review from Sjors on Jan 3, 2024

DrahtBot requested review from Talkless on Jan 3, 2024

hebasto commented at 11:19 am on February 12, 2024: member

Approach ACK cf940f0e5f5ffb14ec7b6f751879bd70b3c350be.

This branch has to be rebased due to the conflicts with the master branch.

DrahtBot removed review request from Talkless on Feb 12, 2024

DrahtBot requested review from Talkless on Feb 12, 2024

DrahtBot removed review request from Talkless on Feb 12, 2024

DrahtBot requested review from Talkless on Feb 12, 2024

DrahtBot commented at 9:19 am on May 13, 2024: contributor

⌛ There hasn’t been much activity lately and the patch still needs rebase. What is the status here?

Is it still relevant? ➡️ Please solve the conflicts to make it ready for review and to ensure the CI passes.
Is it no longer relevant? ➡️ Please close.
Did the author lose interest or time to work on this? ➡️ Please close it and mark it ‘Up for grabs’ with the label, so that it can be picked up in the future.

DrahtBot commented at 9:19 am on May 13, 2024: contributor

🤔 There hasn’t been much activity lately and the CI seems to be failing.

If no one reviewed the current pull request by commit hash, a rebase can be considered. While the CI failure may be a false positive, the CI hasn’t been running for some time, so there may be a real issue hiding as well. A rebase triggers the latest CI and makes sure that no silent merge conflicts have snuck in.

Sjors commented at 12:58 pm on May 13, 2024: member

Realistically anyone who wants to use this probably won’t do so the first time they install the node. So they’ll have to carefully delete the blocks and chainstate from their data dir, wipe the GUI settings and trigger this dialog again.

It seems both easier and safer to encourage such a user to add reindex-chainstate=1 and assumevalid=0 to their config file instead.

I don’t think we should be cluttering the Intro screen for this, especially when looking ahead to Assume UTXO.

hebasto commented at 3:53 pm on August 5, 2024: member

Closing due to author’s inactivity (the branch has been required to be rebased for months).

Feel free to re-open.

hebasto closed this on Aug 5, 2024

Intro: Have user choose assumevalid #149

Reviews

Conflicts