Intro: Have user choose assumevalid #149
pull luke-jr wants to merge 2 commits into bitcoin-core:master from luke-jr:intro_assumevalid changing 6 files +122 −18-
luke-jr commented at 11:30 pm on December 10, 2020: member
-
luke-jr force-pushed on Dec 10, 2020
-
Sjors commented at 10:32 am on December 11, 2020: memberConcept 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. -
Sjors commented at 4:59 pm on December 11, 2020: memberI’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 onelinerreturn (ui->prune->checkState() == Qt::Checked) ? PruneGBtoMiB(m_prune_target_gb) : 0
would look simpler (or justif
/else
) instead of “full-blown”switch
..?
luke-jr commented at 7:12 pm on March 23, 2022:n/a to this PRin 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:donein 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 thinkreset(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/ain 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: noneConcept ACKhebasto added the label Feature on Mar 25, 2021Rspigler commented at 10:49 pm on April 30, 2021: contributorHm, 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, 2021Rspigler commented at 11:41 pm on December 21, 2021: contributorStill getting errors trying to compileDuhluke-jr force-pushed on Mar 23, 2022luke-jr commented at 8:42 pm on March 23, 2022: memberRebased, reviews addressedin 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, 2022luke-jr force-pushed on Mar 23, 2022DrahtBot added the label Needs rebase on Apr 26, 2022luke-jr force-pushed on Apr 30, 2022DrahtBot removed the label Needs rebase on Apr 30, 2022DrahtBot added the label Needs rebase on Jun 15, 2022Rspigler commented at 5:09 pm on June 19, 2022: contributortACK 75aff9e0ff7f98a5ec64d18641a9654ab5b09cdc Simple, I like the language, and I think it is an important setting the user should setlaanwj commented at 8:15 am on June 20, 2022: memberConcept ACK, this choice seems sufficiently important to me to expose in the GUI.GUI: Intro: Output a std::unique_ptr<Intro> from Intro::showIfNeeded f06cf10ed0GUI: Intro: Have user choose assumevalid cf940f0e5fluke-jr force-pushed on Nov 15, 2022luke-jr commented at 5:02 am on November 15, 2022: memberRebasedDrahtBot removed the label Needs rebase on Nov 15, 2022DrahtBot commented at 11:58 pm on November 15, 2022: contributorThe 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, 2023DrahtBot added the label Needs rebase on Jun 9, 2023DrahtBot 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: memberPlease rebase if this PR is still relevant.pablomartin4btc commented at 8:51 pm on October 5, 2023: contributortACK 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 theseif
statements into functions like the aboveUpdateFreeSpaceLabel()
, 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: contributorThere 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: contributorConcept ACKDrahtBot requested review from laanwj on Jan 3, 2024DrahtBot requested review from Sjors on Jan 3, 2024DrahtBot requested review from Talkless on Jan 3, 2024DrahtBot removed review request from Talkless on Feb 12, 2024DrahtBot requested review from Talkless on Feb 12, 2024DrahtBot removed review request from Talkless on Feb 12, 2024DrahtBot requested review from Talkless on Feb 12, 2024DrahtBot 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: memberRealistically 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
andassumevalid=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 closed this on Aug 5, 2024
github-metadata-mirror
This is a metadata mirror of the GitHub repository bitcoin-core/gui. This site is not affiliated with GitHub. Content is generated from a GitHub metadata backup.
generated: 2024-12-03 21:20 UTC
This site is hosted by @0xB10C
More mirrored repositories can be found on mirror.b10c.me