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
  1. luke-jr commented at 11:30 pm on December 10, 2020: member
  2. luke-jr force-pushed on Dec 10, 2020
  3. 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.
  4. 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?

  5. 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).
  6. 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
  7. 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
  8. 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.
  9. 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.
  10. 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.
  11. 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
  12. 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").

  13. Talkless commented at 3:30 pm on March 13, 2021: none
    Concept ACK
  14. hebasto added the label Feature on Mar 25, 2021
  15. 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

  16. DrahtBot added the label Needs rebase on Jul 22, 2021
  17. Rspigler commented at 11:41 pm on December 21, 2021: contributor
    Still getting errors trying to compile Duh
  18. luke-jr force-pushed on Mar 23, 2022
  19. luke-jr commented at 8:42 pm on March 23, 2022: member
    Rebased, reviews addressed
  20. 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.
  21. DrahtBot removed the label Needs rebase on Mar 23, 2022
  22. luke-jr force-pushed on Mar 23, 2022
  23. 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…
  24. DrahtBot added the label Needs rebase on Apr 26, 2022
  25. luke-jr force-pushed on Apr 30, 2022
  26. DrahtBot removed the label Needs rebase on Apr 30, 2022
  27. DrahtBot added the label Needs rebase on Jun 15, 2022
  28. 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
  29. 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.
  30. GUI: Intro: Output a std::unique_ptr<Intro> from Intro::showIfNeeded f06cf10ed0
  31. GUI: Intro: Have user choose assumevalid cf940f0e5f
  32. luke-jr force-pushed on Nov 15, 2022
  33. luke-jr commented at 5:02 am on November 15, 2022: member
    Rebased
  34. DrahtBot removed the label Needs rebase on Nov 15, 2022
  35. 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:

    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.

  36. DrahtBot added the label CI failed on May 15, 2023
  37. DrahtBot added the label Needs rebase on Jun 9, 2023
  38. DrahtBot commented at 8:48 pm on June 9, 2023: contributor

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

  39. hebasto commented at 6:16 pm on September 22, 2023: member
    Please rebase if this PR is still relevant.
  40. pablomartin4btc commented at 8:51 pm on October 5, 2023: contributor

    tACK cf940f0e5f5ffb14ec7b6f751879bd70b3c350be

    Without -assumevalid its checkbox is ticked:

    image

    Witth -assumevalid its checkbox is un-ticked:

    image

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

    On -regtest the layoutAssumeValid is not visible:

    image

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

    image

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

    image

  41. 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.
  42. 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.
  43. kristapsk commented at 0:45 am on January 3, 2024: contributor
    Concept ACK
  44. DrahtBot requested review from laanwj on Jan 3, 2024
  45. DrahtBot requested review from Sjors on Jan 3, 2024
  46. DrahtBot requested review from Talkless on Jan 3, 2024
  47. 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.

  48. DrahtBot removed review request from Talkless on Feb 12, 2024
  49. DrahtBot requested review from Talkless on Feb 12, 2024
  50. DrahtBot removed review request from Talkless on Feb 12, 2024
  51. DrahtBot requested review from Talkless on Feb 12, 2024
  52. 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.
  53. 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.

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

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

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