[refactor] Move some net_processing globals into PeerManagerImpl

ajtowns commented at 8:34 am on January 15, 2021: member

Turns some globals into member variables, and simplifies the parameter list for some of net_processing’s internal functions. Mostly just serves as a code cleanup at this point.

ajtowns added the label Refactoring on Jan 15, 2021

ajtowns added the label P2P on Jan 15, 2021

ajtowns commented at 8:35 am on January 15, 2021: member

Next set of patches from #20758 – should be straightforward to review hopefully.

DrahtBot commented at 3:04 pm on January 15, 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:

#21162 (Net Processing: Move RelayTransaction() into PeerManager by jnewbery)
#21160 (Net/Net processing: Move tx inventory into net_processing by jnewbery)
#20925 (RFC: Move Peer and PeerManagerImpl declarations to separate header by jnewbery)
#20799 (net processing: Only support version 2 compact blocks by jnewbery)
#20729 (p2p: standardize outbound full/block relay connection type naming by jonatack)

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:358 in a3ef94957b outdated

352@@ -345,10 +353,7 @@ class PeerManagerImpl final : public PeerManager
353      * their own locks.
354      */
355     std::map<NodeId, PeerRef> m_peer_map GUARDED_BY(m_peer_mutex);
356-};
357-} // namespace
358 
359-namespace {
360     /** Number of nodes with fSyncStarted. */
361     int nSyncStarted GUARDED_BY(cs_main) = 0;

jnewbery commented at 11:21 am on January 18, 2021:

You’re renaming the other data members as you move them into PeerManagerImpl. Perhaps rename this to m_sync_started?

ajtowns commented at 3:56 pm on January 29, 2021:

I’m only renaming the ones that are currently marked g_ since that would be wrong once they’re no longer global. I’m avoiding renaming the others because that’s unnecessary churn.

I think all of them are GUARDED_BY(cs_main) so renaming them at the same time as switching them to a new mutex would make sense – that’s the point at which you need to be reviewing every place the variable is touched to ensure the locking makes sense, so having every place the variable is touched show up in the diff because it’s getting renamed would make sense.

jnewbery commented at 12:29 pm on February 4, 2021:

ok!

in src/net_processing.cpp:366 in a3ef94957b outdated

364@@ -360,6 +365,14 @@ namespace {
365      */
366     std::map<uint256, std::pair<NodeId, bool>> mapBlockSource GUARDED_BY(cs_main);

jnewbery commented at 11:22 am on January 18, 2021:

Perhaps rename this to m_block_sources?

in src/net_processing.cpp:418 in a3ef94957b outdated

433-    /** Number of preferable block download peers. */
434-    int nPreferredDownload GUARDED_BY(cs_main) = 0;
435-
436-    /** Number of peers from which we're downloading blocks. */
437-    int nPeersWithValidatedDownloads GUARDED_BY(cs_main) = 0;
438+    Mutex m_cs_recent_confirmed_transactions;

jnewbery commented at 11:29 am on January 18, 2021:

Since you’re already renaming this, consider dropping the outdated cs naming. Perhaps m_recent_confirmed_transactions_mutex?

ajtowns commented at 3:57 pm on January 29, 2021:

Sure, makes sense.

in src/net_processing.cpp:437 in a3ef94957b outdated

441-    /** Number of peers with wtxid relay. */
442-    int g_wtxid_relay_peers GUARDED_BY(cs_main) = 0;
443+    bool MarkBlockAsReceived(const uint256& hash) EXCLUSIVE_LOCKS_REQUIRED(cs_main);
444+    bool MarkBlockAsInFlight(NodeId nodeid, const uint256& hash, const CBlockIndex* pindex = nullptr, std::list<QueuedBlock>::iterator** pit = nullptr) EXCLUSIVE_LOCKS_REQUIRED(cs_main);
445+    bool TipMayBeStale() EXCLUSIVE_LOCKS_REQUIRED(cs_main);
446+    void FindNextBlocksToDownload(NodeId nodeid, unsigned int count, std::vector<const CBlockIndex*>& vBlocks, NodeId& nodeStaller) EXCLUSIVE_LOCKS_REQUIRED(cs_main);

jnewbery commented at 11:36 am on January 18, 2021:

Move the function comments to the declarations now that these are declared separately from the function definition.

in src/net_processing.cpp:786 in a3ef94957b outdated

771     AssertLockHeld(cs_main);
772-    if (g_last_tip_update == 0) {
773-        g_last_tip_update = GetTime();
774+    const Consensus::Params& consensusParams = m_chainparams.GetConsensus();
775+    if (m_last_tip_update == 0) {
776+        m_last_tip_update = GetTime();

jnewbery commented at 11:40 am on January 18, 2021:

Now that m_last_tip_update isn’t a global and can be initialized at the time that PeerManagerImpl is constructed, consider initializing it to GetTime() in the initializer list and removing this special casing.

ajtowns commented at 4:27 pm on January 29, 2021:

That seems like a good idea – I think it’d let you mark TipMayBeStale as a const method as well; but I think it goes beyond the mechanical code shuffling that the rest of this PR does.

jnewbery commented at 12:30 pm on February 4, 2021:

Yep, fine to leave this. There’s plenty more to be cleaned up after this PR.

in src/net_processing.cpp:445 in a3ef94957b outdated

451 
452     /** When our tip was last updated. */
453-    std::atomic<int64_t> g_last_tip_update(0);
454+    std::atomic<int64_t> m_last_tip_update{0};
455+
456+    CTransactionRef FindTxForGetData(const CNode& peer, const GenTxid& gtxid, const std::chrono::seconds mempool_req, const std::chrono::seconds now) LOCKS_EXCLUDED(cs_main);

jnewbery commented at 11:42 am on January 18, 2021:

Move function comment to the declaration.

jnewbery commented at 11:57 am on January 18, 2021: member

Code review ACK a3ef94957be28d1b94ee0a34e015b39e60895e0d

This seems fine, but since this is mostly a code cleanup PR, it’d be nice to tidy this up a bit further while you’re touching all these lines:

you rename some of the variables you’re moving to the standard m_ convention, but leave others unchanged. Perhaps move all of them without changing the naming, and then finish this branch with a single scripted diff which changes everything to use standard naming?
The PeerManagerImpl declaration mixes up the data members and member functions. I think it’s much easier to read if the data members are grouped first and then the member functions. Perhaps add a final move-only commit that groups data members and member functions?
Some of the member function comments have been left attached to the definitions rather than moved to the declaration.

DrahtBot added the label Needs rebase on Jan 29, 2021

net_processing: move some globals into PeerManagerImpl

nSyncStarted, mapBlockSource, g_wtxid_relay_peers,
g_outbound_peers_with_protect_from_disconnect were all only used by
PeerManagerImpl methods already.

9781c08a33

net_processing: move AlreadyHaveTx into PeerManageImpl

Allows making recentRejects and g_recent_confirmed_transactions members
rather than globals.

eeac506250

net_processing: simplify AlreadyHaveTx args

No need to pass mempool to PeerManagerImpl methods.

052d9bc7e5

net_processing: move MarkBlockAs*, TipMayBeStale, FindNextBlocksToDL to PeerManagerImpl

Allows converting mapBlocksInFlight and g_last_tip_update from globals
to member variables.

a490f0a056

net_processing: simplify PeerManageImpl method args

No need to pass mempool to MarkBlockAsInFlight, or consensusParams to
TipMayBeStale or FindNextBlocksToDownload.

d44084883a

net_processing: move FindTxForGetData and ProcessGetData to PeerManagerImpl

Allows making mapRelay and vRelayExpiration members rather than globals.

34207b9004

net_processing: simplify ProcessGetData and FindTxForGetData args

No need to pass mempool or connman to PeerManagerImpl methods.

7b7117efd0

net_processing: move MaybeSetPeerAsAnnouncingHeadersAndIDs into PeerManagerImpl

Allows making lNodesAnnouncingHeaderAndIDs and
nPeersWithValidatedDownloads member vars instead of globals.

39c2a69bc2

net_processing: simplify MaybeSetPeerAsAnnouncingHeaderAndIDs args

No need to pass connman to PeerManagerImpl methods.

6452190841

ajtowns force-pushed on Jan 29, 2021

ajtowns commented at 4:55 pm on January 29, 2021: member

Rebased, addressed some of @jnewbery’s comments. There’s still more stuff coming from #20758 so will leave reorganising PeerManagerImpl declaration until later.

DrahtBot removed the label Needs rebase on Jan 29, 2021

in src/net_processing.cpp:421 in 6452190841

429+    Mutex m_recent_confirmed_transactions_mutex;
430+    std::unique_ptr<CRollingBloomFilter> m_recent_confirmed_transactions GUARDED_BY(m_recent_confirmed_transactions_mutex);
431 
432-    /** Stack of nodes which we have set to announce using compact blocks */
433-    std::list<NodeId> lNodesAnnouncingHeaderAndIDs GUARDED_BY(cs_main);
434+    /* Returns a bool indicating whether we requested this block.

jnewbery commented at 12:21 pm on February 4, 2021:

This (and the comment for MarkBlockAsInFlight()) won’t get picked up by doxygen without the /** comment marker (https://www.doxygen.nl/manual/docblocks.html)

ajtowns commented at 2:30 am on February 15, 2021:

Added a todo commit in #20758 to track this

jnewbery approved

jnewbery commented at 12:27 pm on February 4, 2021: member

Code review ACK 6452190841f8da1cdaf899d064974136ab37659f

One minor suggestion inline

in src/net_processing.cpp:366 in eeac506250 outdated

361@@ -362,10 +362,9 @@ class PeerManagerImpl final : public PeerManager
362 
363     /** Number of outbound peers with m_chain_sync.m_protect. */
364     int m_outbound_peers_with_protect_from_disconnect GUARDED_BY(cs_main) = 0;
365-};
366-} // namespace
367 
368-namespace {
369+    bool AlreadyHaveTx(const GenTxid& gtxid, const CTxMemPool& mempool) EXCLUSIVE_LOCKS_REQUIRED(cs_main);

ariard commented at 1:09 pm on February 5, 2021:

eeac506

I don’t know if we have code guidelines on this but maybe move this method at the bottom of the other ones instead of among variables members.

Also maybe add a small comment “Check if we have already seen this transaction by looking up its identifier among reject filter, orphan pool, recent confirmations and mempool” ?

jnewbery commented at 2:05 pm on February 5, 2021:

See discussion here: #20942 (review)

in src/net_processing.cpp:432 in a490f0a056 outdated

427+     * Returns false, still setting pit, if the block was already in flight from the same peer
428+     * pit will only be valid as long as the same cs_main lock is being held
429+     */
430+    bool MarkBlockAsInFlight(CTxMemPool& mempool, NodeId nodeid, const uint256& hash, const CBlockIndex* pindex = nullptr, std::list<QueuedBlock>::iterator** pit = nullptr) EXCLUSIVE_LOCKS_REQUIRED(cs_main);
431+
432+    bool TipMayBeStale(const Consensus::Params &consensusParams) EXCLUSIVE_LOCKS_REQUIRED(cs_main);

ariard commented at 1:22 pm on February 5, 2021:

a490f0a

“Check if our chain is stalling by measuring if a delta of pow targets has been reached since last time of tip update” ?

ajtowns commented at 1:58 am on February 11, 2021:

Added some text to https://github.com/bitcoin/bitcoin/pull/20758/commits/4f9931df03ac5ea223733abe3909db8c61aa559e

ariard commented at 1:47 pm on February 5, 2021: member

Code Review ACK 6452190, changes are pretty straightforward.

See this stylistic comment, overall we can order better PeerManagerImpl members instead of intertwining methods and vars ? Up to you.

vasild commented at 5:06 pm on February 9, 2021: member

It would be really nice to get these right before merge:

Order of member variables and methods, private and public. I think the Google C++ Style Guide makes perfect sense.
Member variables naming.
Proper doxygen comments.

Otherwise 6452190841f8da1cdaf899d064974136ab37659f looks good.

jnewbery commented at 10:37 am on February 15, 2021: member

I think this is probably ready for merge. It’s a simple refactor and has three ACKs. @ajtowns - All three reviewers have asked for sorting members, doxygen comments and renaming data members. I have a small branch that does those things here: https://github.com/jnewbery/bitcoin/tree/pr20942.1. Feel free to take those commits in a follow-up PR.

in src/net_processing.cpp:1568 in 052d9bc7e5 outdated

1564@@ -1565,7 +1565,7 @@ void PeerManagerImpl::BlockChecked(const CBlock& block, const BlockValidationSta
1565 //
1566 
1567 
1568-bool PeerManagerImpl::AlreadyHaveTx(const GenTxid& gtxid, const CTxMemPool& mempool) EXCLUSIVE_LOCKS_REQUIRED(cs_main)
1569+bool PeerManagerImpl::AlreadyHaveTx(const GenTxid& gtxid) EXCLUSIVE_LOCKS_REQUIRED(cs_main)

MarcoFalke commented at 10:54 am on February 15, 2021:

Would be good to not shadow the lock annotation from the “header”. Otherwise it won’t result in a compile failure if a new lock requirement is added to the “header”, but not here, and the new lock isn’t taken.

MarcoFalke commented at 10:54 am on February 15, 2021:

Same for all other commits.

MarcoFalke commented at 11:01 am on February 15, 2021: member

Concept ACK 6452190841 I have not reviewed this, but I left a comment 🐡

Signature:

 0-----BEGIN PGP SIGNED MESSAGE-----
 1Hash: SHA512
 2
 3Concept ACK 6452190841 I have not reviewed this, but I left a comment 🐡
 4-----BEGIN PGP SIGNATURE-----
 5
 6iQGzBAEBCgAdFiEE+rVPoUahrI9sLGYTzit1aX5ppUgFAlwqrYAACgkQzit1aX5p
 7pUiXRwv+Ot8x+JJaOzZ1kM34x9ikAMnoPkV6rbc1xn4csg3qX+vlwOoAsQU9S1VX
 8UmBGAg9aiY5q5IiNlmBL+17Oay0iQOUYnqkhnPqtoIFHLbTW5QnoC31bi7csjuGj
 9XASAOnwk8IcXHi19lUdq0LuHld3vdT4bYD+vViO8ANLU8NT7xKgn4/hfXiyL01Qw
10rqGGda/3oIzJXZC9/BIBOCfS5xIPfbilQtT3XllyxJBhI8+kblGhbqPQ+GfYZ+to
11PoG4mpFzQVrUEL7aYnUapwQ3x0+Rh4+y7UMcz2KC76PjYfDOk1pH8Nnswr8fUBNs
12KV7ofcN6cdg6QAdse8GpOQozZdMc706bBLtp1XyZT3MyaRibm/Kw8KYCoQcrqeF2
13ZPlzqI55FKybz9YFfAZ0g9xE2G2E/m0k/S2ulbepKboelqvRAQ0j0PMUIV4VQPOW
145QZ0C1ka/jobPQt7XYYxBMhlUpofXzHPe+A0iN6cURPJ9b2SQ+br7BFKAt3aq1HZ
150UwAzlpn
16=PF32
17-----END PGP SIGNATURE-----

Timestamp of file with hash 39fd4a1774698d6853a62e53b467bd6ee8d7c78c218cf8d662c407770c5d971b -

MarcoFalke merged this on Feb 15, 2021

MarcoFalke closed this on Feb 15, 2021

sidhujag referenced this in commit 026a911437 on Feb 15, 2021

amitiuttarwar commented at 9:59 pm on February 15, 2021: contributor

fwiw, I had reviewed until a490f0a056456d683dd8ef6f89a5af1a13792118, so post-merge-ACK the first few commits =P

Sjors commented at 9:29 am on April 1, 2021: member

Thanks, d44084883adcf00f50d3d5a9e0c88e3a0b276817 allows me to simplify #20295 a bit.

Fabcien referenced this in commit 2e470f8845 on May 10, 2022

Fabcien referenced this in commit 77d90e8d8e on May 10, 2022

Fabcien referenced this in commit 3e0b98aeda on May 10, 2022

Fabcien referenced this in commit bc00297c37 on May 10, 2022

Fabcien referenced this in commit 5538fc4f30 on May 10, 2022

DrahtBot locked this on Aug 16, 2022

[refactor] Move some net_processing globals into PeerManagerImpl #20942

Conflicts