refactor: replace CNode pointers by references within net

theStack commented at 4:16 pm on May 22, 2020: member

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
…

MarcoFalke commented at 4:27 pm on May 22, 2020: member

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.

instagibbs commented at 4:46 pm on May 22, 2020: member

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

DrahtBot added the label P2P on May 22, 2020

DrahtBot added the label Refactoring on May 22, 2020

jnewbery commented at 6:07 pm on May 22, 2020: member

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)

theStack force-pushed on May 22, 2020

practicalswift commented at 9:51 pm on May 22, 2020: contributor

Strong concept ACK

DrahtBot commented at 10:36 pm on May 22, 2020: 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:

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

MarcoFalke commented at 10:40 pm on May 22, 2020: member

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

practicalswift commented at 6:51 am on May 23, 2020: contributor

@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:

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

A subset of functions with raw pointer arguments:

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

DrahtBot added the label Needs rebase on May 23, 2020

MarcoFalke commented at 12:21 pm on May 23, 2020: member

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

practicalswift commented at 3:09 pm on May 23, 2020: contributor

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

theStack commented at 10:48 am on May 24, 2020: member

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.

theStack force-pushed on May 26, 2020

DrahtBot removed the label Needs rebase on May 26, 2020

theStack commented at 4:59 pm on May 26, 2020: member

Rebased.

MarcoFalke commented at 4:32 pm on May 30, 2020: member

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

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

jnewbery commented at 11:51 pm on May 30, 2020: member

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.

MarcoFalke commented at 11:54 pm on May 30, 2020: member

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

sipa commented at 0:12 am on May 31, 2020: member

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

MarcoFalke commented at 10:24 pm on May 31, 2020: member

Will review after rebase

DrahtBot added the label Needs rebase on Jun 1, 2020

refactor: replace CNode pointers by references within net_processing.{h,cpp} 8b3136bd30

theStack force-pushed on Jun 2, 2020

theStack commented at 11:20 am on June 2, 2020: member

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.

DrahtBot removed the label Needs rebase on Jun 2, 2020

MarcoFalke commented at 12:12 pm on June 2, 2020: member

ACK 8b3136bd307123a255b9166aa42a497a44bcce70 🔻

Signature:

 0-----BEGIN PGP SIGNED MESSAGE-----
 1Hash: SHA512
 2
 3ACK 8b3136bd307123a255b9166aa42a497a44bcce70 🔻
 4-----BEGIN PGP SIGNATURE-----
 5
 6iQGzBAEBCgAdFiEE+rVPoUahrI9sLGYTzit1aX5ppUgFAlwqrYAACgkQzit1aX5p
 7pUgYLwv/c9bS0cnJ+GOBDdUcgtKm/Ppzag390mFQ/y1/buwrd+uPlTUMxrJsOcPq
 8NDfhbH0yE3D3+ZpXJ7F3YKuoClsFx8Hsc/kfm06C2BbJ2adpROQnpgJDQ89rvHsf
 9n1hdiWSltXJJP2Otgo4ebR9QUMpsgeHj3ZpQODwNhAZ6p4i4dnTde+FEBI+aM6bz
100C/05YhduIce1sKTxuGU3VJYegjhqQ4oTxe8Ej4YlExCyEBrZ1cSlfhWjbxFVdW1
117Ax6MO6LIvzMx32c/2OSUA4J+3VdX8kdY9DSI8yhCLyaj4glNmrDpJ27YBWybzP+
12/29WsCtrV08yKMQrx05jY93wpd+jGW75jJKao+cilSxEtaZZpydGBtF25PdI2a1w
13F6uptyxYDec/G8QOMzCTa+7+zI7OB5eJkKJJ0Q6J1UjE4fQSk0oM2+sG1nHs637Y
14K1xzRP1CQhT4uybFbR1yzIFjC99C5icvARVxagZIlDFKkP6f4ErE7p+VaBONsbU/
15l0f3mGjA
16=2B9E
17-----END PGP SIGNATURE-----

Timestamp of file with hash 1a1913878ea3e329bad84656db36b61c6f84e595e6167b335174ea5beeea883c -

MarcoFalke commented at 12:35 pm on June 2, 2020: member

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.

practicalswift commented at 12:37 pm on June 2, 2020: contributor

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:

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

MarcoFalke merged this on Jun 4, 2020

MarcoFalke closed this on Jun 4, 2020

sidhujag referenced this in commit 04b1107aac on Jun 4, 2020

MarcoFalke referenced this in commit a79bca2f1f on Jun 8, 2020

sidhujag referenced this in commit b8f1025f30 on Jun 8, 2020

MarcoFalke referenced this in commit affed844ba on Jul 16, 2020

sidhujag referenced this in commit 647a40216e on Jul 18, 2020

jasonbcox referenced this in commit a33c930370 on Nov 19, 2020

theStack deleted the branch on Dec 1, 2020

DrahtBot locked this on Feb 15, 2022

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

Conflicts