Remove reindex special case from the progress bar label #697

1. 
   maflcko commented at 3:44 pm on January 17, 2023: contributor

   The user knows which option they passed to the program, so it seems overly verbose to offer the user feedback whether or not they passed -reindex. Treat it as DISK, like all other cases that are treated as DISK:

   • -reindex-chainstate
   • -loadblock
2. 
   DrahtBot commented at 3:44 pm on January 17, 2023: 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	john-moffett, hebasto
   Stale ACK	LarryRuane

   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/25781 (Remove almost all blockstorage globals by MarcoFalke)

   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.

3. hebasto requested review from ryanofsky on Jan 17, 2023
4. 
   maflcko commented at 4:28 pm on January 17, 2023: contributor

   For reference:

   • -reindex-chainstate remains unchanged:

   • -loadblock remains unchanged:

   • -reindex stage 2 remains unchanged:

   • -reindex stage 1 (master):

   • -reindex stage 1 (this pull):

5. DrahtBot cross-referenced this on Jan 18, 2023 from issue Remove almost all blockstorage globals by maflcko
6. maflcko added the label UI on Jan 18, 2023
7. 
   maflcko commented at 11:05 am on January 28, 2023: contributor

   So the only user-visible change should be changing the word “Reindex” to “Index”
8. 
   hebasto commented at 12:14 pm on February 2, 2023: member

   Concept ACK.

   As this PR touches code outside the src/qt directory, i.e., src/interfaces/node.h and src/node/interfaces.cpp, should it be moved to the main repo for a broader reviewing?

9. 
   maflcko commented at 2:04 pm on February 2, 2023: contributor

   It is only changing an interface that the gui uses, so I think it should be fine
10. 
    hebasto commented at 2:08 pm on February 2, 2023: member

    cc @ryanofsky
11. in src/qt/clientmodel.cpp:152 in fa20044294 outdated

    155-        return BlockSource::DISK;
    156-    else if (getNumConnections() > 0)
    157-        return BlockSource::NETWORK;
    158-
    159+    if (m_node.isLoadingBlocks()) return BlockSource::DISK;
    160+    if (getNumConnections() > 0) return BlockSource::NETWORK;
    

    ---

    ---

    

    LarryRuane commented at 4:55 am on February 4, 2023:

    I like this, somehow else following return (or break, continue) doesn’t seem right.
12. LarryRuane approved
13. 
    LarryRuane commented at 5:01 am on February 4, 2023: contributor

    code-review, tested ACK fa200442946437f6de8aec95487ccbb541cc485b

    I ran src/qt/bitcoin-qt -noconnect -signet -reindex with and without the PR, and the status message (lower-left corner) for stage 1 was as expected. I didn’t try reindex-chainstate, loadblock, or reindex stage 2.

14. john-moffett approved
15. 
    john-moffett commented at 9:09 pm on February 6, 2023: contributor

    ACK fa200442946437f6de8aec95487ccbb541cc485b

    Nit: Maybe remove the one remaining unnecessary enum use for BlockSource?

    https://github.com/bitcoin-core/gui/blob/fa200442946437f6de8aec95487ccbb541cc485b/src/qt/bitcoingui.cpp#L1074

16. Remove reindex special case from the progress bar label faff2ba4f8
17. 
    maflcko commented at 10:03 am on February 7, 2023: contributor

    Thanks, removed and rebased. Should be trivial to re-ACK with git range-diff
18. 
    john-moffett commented at 1:42 pm on February 7, 2023: contributor

    Re-ACK faff2ba4f80fad5af63a31559fd4065e631a8166
19. hebasto approved
20. 
    hebasto commented at 3:41 pm on February 7, 2023: member

    ACK faff2ba4f80fad5af63a31559fd4065e631a8166, I have reviewed the code and it looks OK, I agree it can be merged.
21. maflcko merged this on Feb 7, 2023
22. maflcko closed this on Feb 7, 2023

    ---

23. sidhujag referenced this in commit 50203a8be1 on Feb 7, 2023
24. 
    LarryRuane commented at 1:25 am on February 8, 2023: contributor

    post-merge re-ACK
25. maflcko deleted the branch on Feb 8, 2023
26. ryanofsky cross-referenced this on Feb 13, 2023 from issue Multiprocess bitcoin by ryanofsky
27. bitcoin-core locked this on Feb 8, 2024

Contributors
maflcko DrahtBot hebasto LarryRuane john-moffett

Review Requested
ryanofsky

Labels
UI