refactor: replace CNode pointers by references within net_processing.{h,cpp} #19053

This PR is inspired by a recent code review comment on a PR that introduced new functions to the net_processing module. The point of the discussion was basically that whenever we pass something not by value (in the concrete example it was about CNode* and CConnman*) we should either use

• a pointer (CType*) with null pointer check or
• a reference (CType&)

To keep things simple, this PR for a first approach

• only tackles CNode* pointers
• only within the net_processing module, i.e. no changes that would need adaption in other modules
• keeps the names of the variables as they are

I'm aware that PRs like this are kind of a PITA to review, but I think the code quality would increase if we get rid of pointers without nullptr check -- bloating up the code by adding all the missing checks would be the worse alternative, in my opinion.

Possible follow-up PRs, in case this is received well:

• replace CNode pointers by references for net module
• replace CConnman pointers by references for net_processing module
• ...

Concept ACK for the reasons you mention.

Review should be easy, but this might conflict with some other work in net processing which is going on right now. Let's wait for DrahtBot to list the conflicts to get a better idea when a change like this is least disruptive.

concept ACK(boo pointers) and agree this should be fairly simple to review.

Very mild concept ACK. Pass-by-reference makes sense for CNode since we can't call ProcessMessages() with a null CNode and we never rebind the node reference when we're in net processing. Passing by reference should generally be the preferred way to pass in-out params (https://isocpp.github.io/CppCoreGuidelines/CppCoreGuidelines#Rf-inout)

Strong concept ACK

<!--e57a25ab6845829454e8d69fc972939a-->

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

<!--174a7506f384e20aa4161008e828411d-->

Conflicts

Reviewers, this pull request conflicts with the following ones:

• #19109 (Only allow getdata of recently announced invs by sipa)
• #19107 (p2p: Refactor, move all header verification into the network layer, without changing behavior by troygiorshev)
• #18991 (Cache responses to GETADDR to prevent topology leaks by naumenkogs)
• #18968 (doc: noban precludes maxuploadtarget disconnects by MarcoFalke)
• #18819 (net: Replace cs_feeFilter with simple std::atomic by MarcoFalke)
• #18638 (net: Use mockable time for ping/pong, add tests by MarcoFalke)
• #18238 (net_processing: Retry notfounds with more urgency by ajtowns)
• #17785 (p2p: Unify Send and Receive protocol versions by hebasto)
• #17428 (p2p: Try to preserve outbound block-relay-only connections during restart by hebasto)
• #15206 (Immediately disconnect on invalid net message checksum by jonasschnelli)
• #14033 (p2p: Drop CADDR_TIME_VERSION checks now that MIN_PEER_PROTO_VERSION is greater by Empact)

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.

Hm. Let's wait until a bunch of those conflicts are in first.

@theStack Thanks for doing this! If you want to investigate other cases where we are currently using raw pointer arguments without any reason doing so these commands might be helpful:

Top list of types passed as raw pointer arguments:

$ git grep -E '^[a-zA-Z].* [a-zA-Z:]+\([^()]*\*[^()]*\)' -- "src/**.cpp" "src/**.h" 
      ":(exclude)src/bench/" ":(exclude)src/compat/" ":(exclude)src/crc32c/" \
      ":(exclude)src/leveldb/" ":(exclude)src/qt/" ":(exclude)src/secp256k1/" \
      ":(exclude)src/test/" ":(exclude)src/tinyformat.h" ":(exclude)src/univalue/" \
      ":(exclude)src/zmq/" ":(exclude)src/wallet/" | grep -vE "char *\*" | \
      cut -f2 -d'(' | cut -f1 -d')' | tr "," "\n" | grep "\*" | sed 's/^ *const  *//g' | \
      sed 's/^ *//g' | sed 's/ \*/* /g' | sed 's/\*.*$/*/g' | grep -E '[a-zA-Z]' | \
      sort | uniq -c | sort -rn
    118 CBlockIndex*
     24 CNode*
     11 ScriptError*
     10 void*
      9 FILE*
      9 bool*
      8 CConnman*
      7 std::string*
      6 RPCTimerInterface*
      6 CNetAddr*
      4 std::vector<int>*
      4 SigningProvider*
      4 LockPoints*
      4 HTTPRequest*
      4 FlatFilePos*
      4 CValidationInterface*
      4 CScriptWitness*
      4 CBlockHeader*
      3 uint32_t*
      3 struct sockaddr*
…

A subset of functions with raw pointer arguments:

$ git grep -E '^[a-zA-Z].* [a-zA-Z:]+\([^()]*\*[^()]*\)' -- "src/**.cpp" "src/**.h" \
      ":(exclude)src/bench/" ":(exclude)src/compat/" ":(exclude)src/crc32c/" \
      ":(exclude)src/leveldb/" ":(exclude)src/qt/" ":(exclude)src/secp256k1/" \
      ":(exclude)src/test/" ":(exclude)src/tinyformat.h" ":(exclude)src/univalue/" \
      ":(exclude)src/zmq/" ":(exclude)src/wallet/" | grep -vE "char *\*"
src/addrman.cpp:CAddrInfo* CAddrMan::Find(const CNetAddr& addr, int* pnId)
src/addrman.cpp:CAddrInfo* CAddrMan::Create(const CAddress& addr, const CNetAddr& addrSource, int* pnId)
src/bitcoin-cli.cpp:static UniValue CallRPC(BaseRequestHandler *rh, const std::string& strMethod, const std::vector<std::string>& args)
src/blockfilter.cpp:bool GCSFilter::MatchInternal(const uint64_t* element_hashes, size_t size) const
src/chain.cpp:void CChain::SetTip(CBlockIndex *pindex) {
src/chain.cpp:CBlockLocator CChain::GetLocator(const CBlockIndex *pindex) const {
src/chain.cpp:const CBlockIndex* LastCommonAncestor(const CBlockIndex* pa, const CBlockIndex* pb) {
src/chain.h:const CBlockIndex* LastCommonAncestor(const CBlockIndex* pa, const CBlockIndex* pb);
src/coins.cpp:bool CCoinsViewCache::SpendCoin(const COutPoint &outpoint, Coin* moveout) {
src/consensus/merkle.cpp:uint256 ComputeMerkleRoot(std::vector<uint256> hashes, bool* mutated) {
src/consensus/merkle.cpp:uint256 BlockMerkleRoot(const CBlock& block, bool* mutated)
src/consensus/merkle.cpp:uint256 BlockWitnessMerkleRoot(const CBlock& block, bool* mutated)
src/consensus/merkle.h:uint256 ComputeMerkleRoot(std::vector<uint256> hashes, bool* mutated = nullptr);
src/consensus/merkle.h:uint256 BlockMerkleRoot(const CBlock& block, bool* mutated = nullptr);
src/consensus/merkle.h:uint256 BlockWitnessMerkleRoot(const CBlock& block, bool* mutated = nullptr);
src/consensus/tx_verify.cpp:std::pair<int, int64_t> CalculateSequenceLocks(const CTransaction &tx, int flags, std::vector<int>* prevHeights, const CBlockIndex& block)
src/consensus/tx_verify.cpp:bool SequenceLocks(const CTransaction &tx, int flags, std::vector<int>* prevHeights, const CBlockIndex& block)
src/consensus/tx_verify.h:std::pair<int, int64_t> CalculateSequenceLocks(const CTransaction &tx, int flags, std::vector<int>* prevHeights, const CBlockIndex& block);
src/consensus/tx_verify.h:bool SequenceLocks(const CTransaction &tx, int flags, std::vector<int>* prevHeights, const CBlockIndex& block);
src/crypto/ripemd160.cpp:void inline Initialize(uint32_t* s)
…

@practicalswift This seems off-topic here. They need a case-by-case evaluation. I suggest you create a brainstorming issue to discuss this.

@MarcoFalke Yes, that these need case-by-case evaluation goes without saying: the lists are showing potential candidates only -- sorry if that was unclear :)

Thanks to all for the quick conceptual reviews! Will rebase every once in a while to get an updated list of conflicts by Drahtbot. @practicalswift: Nice, that's for sure helpful commands for finding more replace-pointers-by-references candidates (always impressed by your mega-grep-sed-etc..-chains ;-)). I agree with MarcoFalke though that opening an issue would make sense here, also to potentially attract more people discussing/working on this.

Rebased.

• #19044 (net processing: Add support for getcfilters by jnewbery)

has three acks already, so let's get that in before this one.

Review comment from #19044 (https://github.com/bitcoin/bitcoin/pull/19044#discussion_r432204149): since the parameters are no longer pointers, they shouldn't be called pnode, pfrom and pto. I think the variable name peer makes sense in all cases, but there's plenty of opportunity to bikeshed that.

Doesn't p in pfrom already mean peer? :thinking:

No, see https://github.com/bitcoin/bitcoin/blob/v0.3.17/coding.txt#L35.

Will review after rebase

Rebased.

Review comment from #19044 (#19044 (comment)): since the parameters are no longer pointers, they shouldn't be called pnode, pfrom and pto. I think the variable name peer makes sense in all cases, but there's plenty of opportunity to bikeshed that.

Thanks, good point! Before I start renaming: are there any other opinions on the naming suggestions? I also think peer (or node) for all those cases would be fine, though one could argue that the information on the message direction is lost in the name.

ACK 8b3136bd307123a255b9166aa42a497a44bcce70 🔻

<details><summary>Show signature and timestamp</summary>

Signature:

-----BEGIN PGP SIGNED MESSAGE-----
Hash: SHA512

ACK 8b3136bd307123a255b9166aa42a497a44bcce70 🔻
-----BEGIN PGP SIGNATURE-----

iQGzBAEBCgAdFiEE+rVPoUahrI9sLGYTzit1aX5ppUgFAlwqrYAACgkQzit1aX5p
pUgYLwv/c9bS0cnJ+GOBDdUcgtKm/Ppzag390mFQ/y1/buwrd+uPlTUMxrJsOcPq
NDfhbH0yE3D3+ZpXJ7F3YKuoClsFx8Hsc/kfm06C2BbJ2adpROQnpgJDQ89rvHsf
n1hdiWSltXJJP2Otgo4ebR9QUMpsgeHj3ZpQODwNhAZ6p4i4dnTde+FEBI+aM6bz
0C/05YhduIce1sKTxuGU3VJYegjhqQ4oTxe8Ej4YlExCyEBrZ1cSlfhWjbxFVdW1
7Ax6MO6LIvzMx32c/2OSUA4J+3VdX8kdY9DSI8yhCLyaj4glNmrDpJ27YBWybzP+
/29WsCtrV08yKMQrx05jY93wpd+jGW75jJKao+cilSxEtaZZpydGBtF25PdI2a1w
F6uptyxYDec/G8QOMzCTa+7+zI7OB5eJkKJJ0Q6J1UjE4fQSk0oM2+sG1nHs637Y
K1xzRP1CQhT4uybFbR1yzIFjC99C5icvARVxagZIlDFKkP6f4ErE7p+VaBONsbU/
l0f3mGjA
=2B9E
-----END PGP SIGNATURE-----

Timestamp of file with hash 1a1913878ea3e329bad84656db36b61c6f84e595e6167b335174ea5beeea883c -

</details>

I like that the only changes are foo-> to foo., which wouldn't compile if foo was a raw pointer. I also like that it keeps all symbol names as they were before. If they were changed, that would make review harder because generic names such as peer could be shadowed by similarly named symbols from outer or global scopes. And while p used to mean pointer, I don't see why it can be changed to mean peer instead (without having to change code at all).

So my preference would be to keep the names as they were, but no strong opinion. Though, if the names are changed, it should be done in a new commit, so that scripts can be used to help review.

ACK 8b3136bd307123a255b9166aa42a497a44bcce70

Checked that set of inserted/deleted characters appear to be limited to *&>.- (as expected when changing from -> to ., and from * to &) and the string "const" (which was added for GetFetchFlags and UpdatePreferredDownload) by doing the following quick sanity check:

$ curl -s https://patch-diff.githubusercontent.com/raw/bitcoin/bitcoin/pull/19053.patch | \
      grep -E '^[+-][^+-]' | cut -b2- | tr -d "*&>.-" | sort | uniq -c | grep -E '^ *1 '
      1 static uint32_t GetFetchFlags(CNode pfrom) EXCLUSIVE_LOCKS_REQUIRED(cs_main) {
      1 static uint32_t GetFetchFlags(const CNode pfrom) EXCLUSIVE_LOCKS_REQUIRED(cs_main) {
      1 static void UpdatePreferredDownload(CNode node, CNodeState state) EXCLUSIVE_LOCKS_REQUIRED(cs_main)
      1 static void UpdatePreferredDownload(const CNode node, CNodeState state) EXCLUSIVE_LOCKS_REQUIRED(cs_main)
$ curl -s https://patch-diff.githubusercontent.com/raw/bitcoin/bitcoin/pull/19053.patch | \
      grep -E '^.static.*(GetFetchFlags|UpdatePreferredDownload)'
-static void UpdatePreferredDownload(CNode* node, CNodeState* state) EXCLUSIVE_LOCKS_REQUIRED(cs_main)
+static void UpdatePreferredDownload(const CNode& node, CNodeState* state) EXCLUSIVE_LOCKS_REQUIRED(cs_main)
-static uint32_t GetFetchFlags(CNode* pfrom) EXCLUSIVE_LOCKS_REQUIRED(cs_main) {
+static uint32_t GetFetchFlags(const CNode& pfrom) EXCLUSIVE_LOCKS_REQUIRED(cs_main) {