p2p: Clarify disconnect log message in ProcessGetBlockData, remove send bool #21235

1. 
   MarcoFalke commented at 10:26 am on February 19, 2021: member

   • Clarify that “ignoring” really means “disconnect” in the log
   • Revive a refactor I took from #13670
2. MarcoFalke added the label P2P on Feb 19, 2021
3. MarcoFalke added the label Refactoring on Feb 19, 2021
4. MarcoFalke added the label Utils/log/libs on Feb 19, 2021
5. theStack approved
6. 
   theStack commented at 2:16 pm on February 20, 2021: member

   ACK c56f1401b43d8235713745a35594e7e7c1b9f5e0 ♻️ Quite surprising that the PR where the second commit is taken from was never merged, at least IMHO getting rid of this unnecessary state variable is a definitive improvement w.r.t. readability.
7. 
   DrahtBot commented at 1:22 am on February 23, 2021: 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:

   • #21160 (Net/Net processing: Move tx inventory into net_processing by jnewbery)

   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.

8. in src/net_processing.cpp:1570 in c56f1401b4 outdated

   1771-        if (!send) {
   1772-            LogPrint(BCLog::NET, "%s: ignoring request from peer=%i for old block that isn't in the main chain\n", __func__, pfrom.GetId());
   1773-        }
   1774+    if (!pindex) {
   1775+        return;
   1776+    }
   

   ---

   ---

   

   jnewbery commented at 9:59 am on February 23, 2021:

   nit: consider joining these three lines

   ---

   

   MarcoFalke commented at 10:20 am on February 23, 2021:

   I like the “bloated” version better for several reasons:

   • It is easier to set a breakpoint in that line in gdb
   • It is easier to see line coverage for that line with lcov
   • It is easier to add a (debug) log statement (or any other statement), even if just for local playing
   • The other return statements in this function are also in a {} scope

   ---

   

   jnewbery commented at 10:34 am on February 23, 2021:

   sure, just a suggestion!
9. in src/net_processing.cpp:1797 in c56f1401b4 outdated

   1807+        return;
   1808     }
   1809     // Pruned nodes may have deleted the block, so check whether
   1810     // it's available before trying to send.
   1811-    if (send && (pindex->nStatus & BLOCK_HAVE_DATA))
   1812+    if (pindex->nStatus & BLOCK_HAVE_DATA)
   

   ---

   ---

   

   jnewbery commented at 10:02 am on February 23, 2021:

   nit: this can also be converted to an early exit to avoid nesting the rest of the function:

   0    if (!pindex->nStatus & BLOCK_HAVE_DATA) return;
   

   ---

   

   MarcoFalke commented at 10:21 am on February 23, 2021:

   Thanks, done
10. 
    jnewbery commented at 10:03 am on February 23, 2021: member

    utACK c56f1401b43d8235713745a35594e7e7c1b9f5e0

    Thanks for doing this. I’ve also had a branch floating around to remove this variable.

    Couple of small style suggestions inline.

11. MarcoFalke force-pushed on Feb 23, 2021
12. MarcoFalke force-pushed on Feb 23, 2021
13. 
    jnewbery commented at 10:38 am on February 23, 2021: member

    utACK fa55ab88dd4b15dc1a4b59ae6d8157edbd9b91de

    Very nice to remove the indenting in a separate commit :100:

14. 
    practicalswift commented at 5:28 pm on February 24, 2021: contributor

    Concept ACK

    Returning early here makes the code more robust and easier to reason about. Glad to see send go :)

15. DrahtBot added the label Needs rebase on Mar 11, 2021
16. MarcoFalke force-pushed on Mar 11, 2021
17. 
    MarcoFalke commented at 3:46 pm on March 11, 2021: member

    Rebased. Should be trivial to re-ACK
18. DrahtBot removed the label Needs rebase on Mar 11, 2021
19. 
    jnewbery commented at 5:18 pm on March 11, 2021: member

    utACK 7ed521036cfd9795c63aaab8a59dadc34999e79c

    Checked the git range-diff. Only difference is things like using m_chainman.ActiveChain() instead of ::ChainActive() due to #21270.

20. 
    DrahtBot commented at 4:49 pm on March 15, 2021: member

    🕵️ @sipa has been requested to review this pull request as specified in the REVIEWERS file.

21. log: Clarify that block request below NODE_NETWORK_LIMITED_MIN_BLOCKS disconnects fae7c0429f
22. net: Simplify ProcessGetBlockData execution by removing send flag.

    Setting the send flag to false can be replaced by simply returning.

    fae77b9e6d
23. style-only: Remove whitespace

    Can be reviewed with --ignore-all-space

    fa81773243
24. MarcoFalke force-pushed on Mar 18, 2021
25. 
    MarcoFalke commented at 8:19 am on March 18, 2021: member

    Rebased after conflict
26. sipa approved
27. 
    sipa commented at 2:00 am on March 19, 2021: member

    utACK fa8177324392c923c6ce39056cfd870af55ab673
28. 
    jnewbery commented at 10:12 am on March 19, 2021: member

    utACK fa8177324392c923c6ce39056cfd870af55ab673

    the range-diff was a bit messy so I just reviewed again from scratch.

29. MarcoFalke merged this on Mar 19, 2021
30. MarcoFalke closed this on Mar 19, 2021

    ---

31. MarcoFalke deleted the branch on Mar 19, 2021
32. DrahtBot locked this on Aug 18, 2022

Contributors
MarcoFalke theStack DrahtBot jnewbery practicalswift sipa

Labels
Refactoring P2P Utils/log/libs