refactor: rpc: Pass CBlockIndex by reference instead of pointer #29021

1. 
   maflcko commented at 11:08 am on December 7, 2023: member

   Follow-up to #29003 (comment)
2. refactor: Use reference instead of pointer in IsBlockPruned

   This makes it harder to pass nullptr and cause issues such as
   https://github.com/bitcoin/bitcoin/commit/dde7ac5c704688c8a9af29bd07e5ae8114824ce7

   fa604eb6cf
3. refactor: rpc: Pass CBlockIndex by reference instead of pointer

   All functions assume that the pointer is never null, so pass by
   reference, to avoid accidental segfaults at runtime, or at least make
   them more obvious.
   
   Also, remove unused c-style casts in touched lines.
   
   Also, add CHECK_NONFATAL checks, to turn segfault crashes into an
   recoverable runtime error with debug information.

   fa5989d514
4. 
   DrahtBot commented at 11:08 am on December 7, 2023: contributor

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

   Code Coverage

   For detailed information about the code coverage, see the test coverage report.

   Reviews

   See the guideline for information on the review process.

   Type	Reviewers
   ACK	TheCharlatan, pablomartin4btc, dergoegge

   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:

   • #26415 (rpc,rest,zmq: faster getblock, NotifyBlock and rest_block by reading raw block by andrewtoth)

   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.

5. DrahtBot renamed this:
   refactor: rpc: Pass CBlockIndex by reference instead of pointer
   refactor: rpc: Pass CBlockIndex by reference instead of pointer
   on Dec 7, 2023
6. DrahtBot added the label Refactoring on Dec 7, 2023
7. in src/rpc/blockchain.cpp:419 in fa5989d514

   415@@ -418,7 +416,7 @@ static RPCHelpMan getdifficulty()
   416 {
   417     ChainstateManager& chainman = EnsureAnyChainman(request.context);
   418     LOCK(cs_main);
   419-    return GetDifficulty(chainman.ActiveChain().Tip());
   420+    return GetDifficulty(*CHECK_NONFATAL(chainman.ActiveChain().Tip()));
   

   ---

   ---

   

   TheCharlatan commented at 4:11 pm on December 7, 2023:

   The chainman.ActiveChain().Tip() is checked here, but there are a few other places where the tip pointer is not checked (e.g. here, here, and here). Should these be checked too?

   ---

   

   maflcko commented at 4:51 pm on December 7, 2023:

   The weren’t previously checked either (I presume because if the tip is nullptr, then pblockindex should be nullptr as well, which is checked).

   However, I am happy to review a pull request adding a check, or push any changes that compile to this pull request. (If someone writes them).

   ---

   

   TheCharlatan commented at 4:57 pm on December 7, 2023:

   Right, you only added checks to the lines you already were touching, so I guess that makes sense.
8. TheCharlatan approved
9. 
   TheCharlatan commented at 4:58 pm on December 7, 2023: contributor

   ACK fa5989d514d246e56977c528b2dd2abe6dc8efcc
10. pablomartin4btc approved
11. 
    pablomartin4btc commented at 5:44 pm on December 7, 2023: member

    tACK fa5989d514d246e56977c528b2dd2abe6dc8efcc

    I’ve re-tested #29003 & getblockchaininfo on mainnet.

12. fanquake requested review from dergoegge on Dec 8, 2023
13. dergoegge approved
14. 
    dergoegge commented at 4:52 pm on December 11, 2023: member

    Code review ACK fa5989d514d246e56977c528b2dd2abe6dc8efcc
15. fanquake merged this on Dec 12, 2023
16. fanquake closed this on Dec 12, 2023

    ---

17. maflcko deleted the branch on Dec 12, 2023

Contributors
maflcko DrahtBot TheCharlatan pablomartin4btc dergoegge

Labels
Refactoring