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

pull MarcoFalke wants to merge 3 commits into bitcoin:master from MarcoFalke:2102-logDisconnect changing 1 files +86 −90

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
MarcoFalke added the label P2P on Feb 19, 2021
MarcoFalke added the label Refactoring on Feb 19, 2021
MarcoFalke added the label Utils/log/libs on Feb 19, 2021
theStack approved
theStack commented at 2:16 PM on February 20, 2021: contributor

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.
DrahtBot cross-referenced this on Feb 23, 2021 from issue [Bundle 4/n] Prune g_chainman usage in validation-adjacent modules by dongcarl
DrahtBot commented at 1:22 AM on February 23, 2021: contributor


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.
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!

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:

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

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

Thanks, done

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.
MarcoFalke force-pushed on Feb 23, 2021
MarcoFalke force-pushed on Feb 23, 2021
jnewbery commented at 10:38 AM on February 23, 2021: member

utACK fa55ab88dd4b15dc1a4b59ae6d8157edbd9b91de

Very nice to remove the indenting in a separate commit :100:
DrahtBot cross-referenced this on Feb 23, 2021 from issue net/net processing: Move tx inventory into net_processing by jnewbery
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 :)
DrahtBot cross-referenced this on Mar 9, 2021 from issue [Bundle 5/n] Prune g_chainman usage in RPC modules by dongcarl
DrahtBot added the label Needs rebase on Mar 11, 2021
MarcoFalke force-pushed on Mar 11, 2021
MarcoFalke commented at 3:46 PM on March 11, 2021: member

Rebased. Should be trivial to re-ACK
DrahtBot removed the label Needs rebase on Mar 11, 2021
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.
DrahtBot cross-referenced this on Mar 12, 2021 from issue refactor: Pass PeerManagerImpl members only once by MarcoFalke
DrahtBot commented at 4:49 PM on March 15, 2021: contributor



🕵️ @sipa has been requested to review this pull request as specified in the REVIEWERS file.
log: Clarify that block request below NODE_NETWORK_LIMITED_MIN_BLOCKS disconnects fae7c0429f
net: Simplify ProcessGetBlockData execution by removing send flag.
Setting the send flag to false can be replaced by simply returning.
fae77b9e6d
style-only: Remove whitespace
Can be reviewed with --ignore-all-space
fa81773243
MarcoFalke force-pushed on Mar 18, 2021
MarcoFalke commented at 8:19 AM on March 18, 2021: member

Rebased after conflict
sipa approved
sipa commented at 2:00 AM on March 19, 2021: member

utACK fa8177324392c923c6ce39056cfd870af55ab673
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.
MarcoFalke merged this on Mar 19, 2021
MarcoFalke closed this on Mar 19, 2021
MarcoFalke deleted the branch on Mar 19, 2021
adamjonas cross-referenced this on Aug 4, 2022 from issue noban permission probably shouldn't give additional permissions implcitly by luke-jr
bitcoin locked this on Aug 18, 2022

Contributors

Labels

Refactoring P2P Utils/log/libs

Linked (view graph)

#13670 Simplify ProcessGetBlockData execution by removing send flag #19886 noban permission probably shouldn't give additional permissions implcitly

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

Conflicts