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

   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	maflcko, l0rinc, stickies-v

   If your review is incorrectly listed, please copy-paste <!–meta-tag:bot-skip–> into the comment that the bot should ignore.

   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.

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 🎐

   Signature:

   0untrusted 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}"
   1RUTRmVTMeKV5npGrKx1nqXCw5zeVHdtdYURB/KlyA/LMFgpNCs+SkW9a8N95d+U4AP1RJMi+krxU1A3Yux4bpwZNLvVBKy0wLgM=
   2trusted comment: review ACK 1f8f7d477ae0d33bd96f7936889c17bd40805fb9 🎐
   3XAbdEgvzeaut4OjiEtc7By3sU6e6R0WhXzYJrriCAiHnOBPQo+yWHscCVaz18PJOWb9tz90BCrZjFBQDivXXCQ==
   

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
optout21 DrahtBot maflcko l0rinc sedited stickies-v

Review Requested
sedited