Change BlockRequestAllowed() to take ref (minor refactor) #34464

1. 
   optout21 commented at 4:57 PM on January 30, 2026: contributor

   As suggested here, a minor refactor of PeerManagerImpl::BlockRequestAllowed() to take reference parameter (instead of pointer). The motivation is to make the code safer, by minimizing the risk of null-dereference, and to be more consistent. The change is local to the PeerManagerImpl::BlockRequestAllowed() class. Related to #34440.

2. 
   DrahtBot commented at 4:57 PM on January 30, 2026: contributor

   <!--e57a25ab6845829454e8d69fc972939a-->

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

   <!--021abf342d371248e50ceaed478a90ca-->

   Reviews

   See the guideline for information on the review process.

   Type	Reviewers
   ACK	maflcko, l0rinc, stickies-v

   If your review is incorrectly listed, please copy-paste <code>&lt;!--meta-tag:bot-skip--&gt;</code> into the comment that the bot should ignore.

   <!--174a7506f384e20aa4161008e828411d-->

   Conflicts

   Reviewers, this pull request conflicts with the following ones:

   • #34440 (Refactor CChain methods to use references, tests by optout21)
   • #34416 (Add nullptr-check to CChain::Contains(), tests by optout21)

   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.

   <!--5faf32d7da4f0f540f40219e4f7537a3-->

3. optout21 renamed this:
   Change BlockRequestAllowed() to take ref
   Change BlockRequestAllowed() to take ref (minor refactor)
   on Jan 30, 2026
4. 
   maflcko commented at 5:15 PM on January 30, 2026: member

   Please squash your commits according to https://github.com/bitcoin/bitcoin/blob/master/CONTRIBUTING.md#squashing-commits

5. Change BlockRequestAllowed() to take ref

   The input parameter of `PeerManagerImpl::BlockRequestAllowed()` changed to
   reference from pointer. The change is local to the class.

   1f8f7d477a
6. optout21 force-pushed on Jan 30, 2026
7. optout21 marked this as ready for review on Jan 30, 2026
8. 
   optout21 commented at 10:37 PM on January 30, 2026: contributor

   Squashed (as suggested), marked ready for review.

9. 
   maflcko commented at 8:32 AM on January 31, 2026: member

   review ACK 1f8f7d477ae0d33bd96f7936889c17bd40805fb9 🎐

   <details><summary>Show signature</summary>

   Signature:

   untrusted comment: signature from minisign secret key on empty file; verify via: minisign -Vm "${path_to_any_empty_file}" -P RWTRmVTMeKV5noAMqVlsMugDDCyyTSbA3Re5AkUrhvLVln0tSaFWglOw -x "${path_to_this_whole_four_line_signature_blob}"
   RUTRmVTMeKV5npGrKx1nqXCw5zeVHdtdYURB/KlyA/LMFgpNCs+SkW9a8N95d+U4AP1RJMi+krxU1A3Yux4bpwZNLvVBKy0wLgM=
   trusted comment: review ACK 1f8f7d477ae0d33bd96f7936889c17bd40805fb9 🎐
   XAbdEgvzeaut4OjiEtc7By3sU6e6R0WhXzYJrriCAiHnOBPQo+yWHscCVaz18PJOWb9tz90BCrZjFBQDivXXCQ==
   

   </details>

10. 
    l0rinc commented at 8:44 AM on January 31, 2026: contributor

    tested ACK 1f8f7d477ae0d33bd96f7936889c17bd40805fb9

    Recreated the change locally (there was only a newline difference which is fine), rebased and ran all tests.

11. in src/net_processing.cpp:2343 in 1f8f7d477a

    2339 | @@ -2340,7 +2340,7 @@ void PeerManagerImpl::ProcessGetBlockData(CNode& pfrom, Peer& peer, const CInv&
    2340 |          if (!pindex) {
    2341 |              return;
    2342 |          }
    2343 | -        if (!BlockRequestAllowed(pindex)) {
    2344 | +        if (!BlockRequestAllowed(*pindex)) {
    

    ---

    ---

    

    sedited commented at 9:41 AM on February 2, 2026:

    Would it better if we Assert( in these cases too?

    ---

    

    optout21 commented at 9:46 AM on February 2, 2026:

    I considered adding an assert, but the if (!pindex) ... condition is on the immediately preceding line, so that's why I found it superfluous to add an assert as well.

    ---

    

    l0rinc commented at 11:32 AM on February 2, 2026:

    Agree, we're basically in the if (pindex != nullptr) branch here

12. optout21 requested review from sedited on Feb 3, 2026
13. stickies-v approved
14. 
    stickies-v commented at 2:28 PM on February 3, 2026: contributor

    ACK 1f8f7d477ae0d33bd96f7936889c17bd40805fb9

15. 
    optout21 commented at 9:07 AM on February 5, 2026: contributor

    Can this be considered for merge maybe or more review needed?

16. fanquake merged this on Feb 5, 2026
17. fanquake closed this on Feb 5, 2026

    ---

18. optout21 deleted the branch on Feb 9, 2026

Contributors

optout21DrahtBotmaflckol0rincseditedstickies-v

Review Requested
sedited