Intro: Have user choose assumevalid #149

luke-jr · 2020-12-10T23:30:21Z

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

Sjors · 2020-12-11T10:32:20Z

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 · 2020-12-11T15:42:43Z

@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 · 2020-12-11T16:59:23Z

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

Talkless

Concept ACK

src/qt/intro.cpp

src/qt/intro.h

src/qt/intro.cpp

Rspigler · 2021-04-30T22:49:58Z

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

Rspigler · 2021-12-21T23:41:52Z

~~Still getting errors trying to compile~~ Duh

luke-jr · 2022-03-23T20:42:33Z

Rebased, reviews addressed

src/qt/intro.cpp

Talkless · 2022-03-28T17:47:22Z

Sorry for a lot of "n/a's" @luke-jr , I guess I need to up my diff reading game...

Rspigler · 2022-06-19T17:09:58Z

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

laanwj · 2022-06-20T08:15:51Z

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

luke-jr · 2022-11-15T05:02:11Z

Rebased

DrahtBot · 2022-11-15T23:58:23Z

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 · 2023-06-09T20:48:47Z

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

hebasto · 2023-09-22T18:16:06Z

Please rebase if this PR is still relevant.

pablomartin4btc

tACK cf940f0

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:

pablomartin4btc · 2023-10-05T21:14:28Z

src/qt/intro.cpp

+    if (gArgs.IsArgSet("-assumevalid")) {
+        const auto user_assumevalid = gArgs.GetArg("-assumevalid", /* ignored default; determines return type */ "");
+        if (uint256S(user_assumevalid).IsNull()) {
+            // -assumevalid=0: default checkbox to off, and initialise with chainparams later
+            ui->assumevalid->setChecked(false);
+        } else {
+            // -assumevalid=blockhash: initialise with the user-specified value, enabled
+            ui->assumevalid->setChecked(true);
+            ui->assumevalidBlock->setText(QString::fromStdString(user_assumevalid));
+            have_user_assumevalid = true;
+        }
+    }


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 · 2024-01-03T00:41:54Z

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 · 2024-01-03T00:45:56Z

Concept ACK

hebasto

Approach ACK cf940f0.

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

DrahtBot · 2024-05-13T09:19:11Z

⌛ 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 · 2024-05-13T09:19:13Z

🤔 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 · 2024-05-13T12:58:13Z

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.

luke-jr force-pushed the intro_assumevalid branch from f7e95fe to 15f007c Compare December 10, 2020 23:33

Talkless reviewed Mar 13, 2021

View reviewed changes

Talkless mentioned this pull request Mar 16, 2021

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

Merged

hebasto added the Feature label Mar 25, 2021

DrahtBot added the Needs rebase label Jul 22, 2021

luke-jr force-pushed the intro_assumevalid branch from 15f007c to 253e500 Compare March 23, 2022 20:42

fanquake reviewed Mar 23, 2022

View reviewed changes

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

DrahtBot removed the Needs rebase label Mar 23, 2022

luke-jr force-pushed the intro_assumevalid branch from 253e500 to 88863cf Compare March 23, 2022 20:53

DrahtBot added the Needs rebase label Apr 26, 2022

luke-jr force-pushed the intro_assumevalid branch from 88863cf to 75aff9e Compare April 30, 2022 03:23

DrahtBot removed the Needs rebase label Apr 30, 2022

luke-jr added 2 commits November 15, 2022 04:26

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

f06cf10

GUI: Intro: Have user choose assumevalid

cf940f0

luke-jr force-pushed the intro_assumevalid branch from 75aff9e to cf940f0 Compare November 15, 2022 05:01

DrahtBot removed the Needs rebase label Nov 15, 2022

DrahtBot mentioned this pull request Nov 16, 2022

multiprocess: Add bitcoin-gui -ipcconnect option bitcoin/bitcoin#19461

Draft

DrahtBot mentioned this pull request Apr 13, 2023

Make GUI and CLI tools use the same datadir bitcoin/bitcoin#27409

Draft

DrahtBot added the CI failed label May 15, 2023

DrahtBot added the Needs rebase label Jun 9, 2023

DrahtBot mentioned this pull request Jul 31, 2023

kernel: Prune leveldb headers bitcoin/bitcoin#28186

Merged

pablomartin4btc reviewed Oct 5, 2023

View reviewed changes

DrahtBot requested review from laanwj, Sjors and Talkless January 3, 2024 00:46

hebasto reviewed Feb 12, 2024

View reviewed changes

DrahtBot requested review from Talkless and removed request for Talkless February 12, 2024 11:19

Provide feedback

Saved searches

Use saved searches to filter your results more quickly

Intro: Have user choose assumevalid #149

Intro: Have user choose assumevalid #149

luke-jr commented Dec 10, 2020 •

edited

Sjors commented Dec 11, 2020 •

edited

luke-jr commented Dec 11, 2020

Sjors commented Dec 11, 2020 •

edited

Talkless left a comment

Rspigler commented Apr 30, 2021

Rspigler commented Dec 21, 2021 •

edited

luke-jr commented Mar 23, 2022

Talkless commented Mar 28, 2022

Rspigler commented Jun 19, 2022

laanwj commented Jun 20, 2022

luke-jr commented Nov 15, 2022

DrahtBot commented Nov 15, 2022 •

edited

DrahtBot commented Jun 9, 2023

hebasto commented Sep 22, 2023

pablomartin4btc left a comment

pablomartin4btc Oct 5, 2023

DrahtBot commented Jan 3, 2024

kristapsk commented Jan 3, 2024

hebasto left a comment •

edited

DrahtBot commented May 13, 2024

DrahtBot commented May 13, 2024

Sjors commented May 13, 2024

Intro: Have user choose assumevalid #149

Are you sure you want to change the base?

Intro: Have user choose assumevalid #149

Conversation

luke-jr commented Dec 10, 2020 • edited

Sjors commented Dec 11, 2020 • edited

luke-jr commented Dec 11, 2020

Sjors commented Dec 11, 2020 • edited

Talkless left a comment

Choose a reason for hiding this comment

Rspigler commented Apr 30, 2021

Rspigler commented Dec 21, 2021 • edited

luke-jr commented Mar 23, 2022

Talkless commented Mar 28, 2022

Rspigler commented Jun 19, 2022

laanwj commented Jun 20, 2022

luke-jr commented Nov 15, 2022

DrahtBot commented Nov 15, 2022 • edited

Reviews

Conflicts

DrahtBot commented Jun 9, 2023

hebasto commented Sep 22, 2023

pablomartin4btc left a comment

Choose a reason for hiding this comment

pablomartin4btc Oct 5, 2023

Choose a reason for hiding this comment

DrahtBot commented Jan 3, 2024

kristapsk commented Jan 3, 2024

hebasto left a comment • edited

Choose a reason for hiding this comment

DrahtBot commented May 13, 2024

DrahtBot commented May 13, 2024

Sjors commented May 13, 2024

luke-jr commented Dec 10, 2020 •

edited

Sjors commented Dec 11, 2020 •

edited

Sjors commented Dec 11, 2020 •

edited

Rspigler commented Dec 21, 2021 •

edited

DrahtBot commented Nov 15, 2022 •

edited

hebasto left a comment •

edited