refactor: Remove confusing BlockIndex global #19413

1. 
   MarcoFalke commented at 2:32 pm on June 29, 2020: member

   The global ::BlockIndex() is problematic for several reasons:

   • It returns a mutable reference to the block tree, without the appropriate lock annotation (m_block_index is guarded by cs_main). The current code is fine, but in the future this might lead to accidental races and data corruption.
   • The rpc server shouldn’t rely on node globals, but rather a context that is passed in to the RPC method.
   • Tests might want to spin up their own block tree, and thus should also not rely on a single global.

   Fix all issues by removing the global

2. MarcoFalke added the label Refactoring on Jun 29, 2020
3. 
   MarcoFalke commented at 2:34 pm on June 29, 2020: member

   The --word-diff-regex=. git option might come in handy during review
4. 
   laanwj commented at 3:21 pm on June 29, 2020: member

   Nice change, good to get rid of an valdiation global, and it’s surprisingly low impact.

   ACK fa82d8494e6db431e324bf4bec980aa044761f96

5. in src/rpc/blockchain.cpp:1336 in fa82d8494e outdated

   1333+     * Idea: The set of chain tips is the active chain tip, plus orphan blocks which do not have another orphan building off of them.
   1334      * Algorithm:
   1335      *  - Make one pass through BlockIndex(), picking out the orphan blocks, and also storing a set of the orphan block's pprev pointers.
   1336      *  - Iterate through the orphan blocks. If the block isn't pointed to by another orphan, it is a chain tip.
   1337-     *  - add ::ChainActive().Tip()
   1338+     *  - add the active chain tip
   

   ---

   ---

   

   jonatack commented at 3:31 pm on June 29, 2020:

   nit s/add/Add/ if you retouch

   ---

   

   MarcoFalke commented at 0:29 am on June 30, 2020:

   thx, fixed
6. 
   jonatack commented at 3:33 pm on June 29, 2020: member

   Light code review ACK fa82d8494e6d I agree that that this is a nice change
7. MarcoFalke closed this on Jun 29, 2020

   ---

8. MarcoFalke reopened this on Jun 29, 2020

   ---

9. 
   DrahtBot commented at 7:04 pm on June 29, 2020: member

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

   Conflicts

   Reviewers, this pull request conflicts with the following ones:

   • #19011 (Reduce cs_main lock accumulation during GUI startup by jonasschnelli)
   • #18354 (Use shared pointers only in validation interface by bvbfan)

   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.

10. 
    practicalswift commented at 8:02 pm on June 29, 2020: contributor

    Concept ACK
11. refactor: Remove confusing BlockIndex global fa0dfdf447
12. MarcoFalke force-pushed on Jun 30, 2020
13. 
    promag commented at 8:33 am on July 3, 2020: member

    Code review ACK fa0dfdf447d5b84a1849dc823d8508463600136a.
14. 
    jonatack commented at 10:49 am on July 3, 2020: member

    re-ACK fa0dfdf
15. MarcoFalke merged this on Jul 3, 2020
16. MarcoFalke closed this on Jul 3, 2020

    ---

17. MarcoFalke deleted the branch on Jul 3, 2020
18. domob1812 referenced this in commit 1cea402022 on Jul 6, 2020
19. domob1812 referenced this in commit 55f4edbdcc on Jul 6, 2020
20. domob1812 referenced this in commit 9e7394ab62 on Jul 6, 2020
21. Fabcien referenced this in commit 721672a587 on May 26, 2021
22. DrahtBot locked this on Feb 15, 2022

Contributors
MarcoFalke laanwj jonatack DrahtBot practicalswift promag

Labels
Refactoring