[refactor] Move some net_processing globals into PeerManagerImpl #20942

ajtowns · 2021-01-15T08:34:19Z

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 · 2021-01-15T08:35:33Z

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

DrahtBot · 2021-01-15T15:04:31Z

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

Conflicts

Reviewers, this pull request conflicts with the following ones:

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

jnewbery

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.

src/net_processing.cpp

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

Allows making recentRejects and g_recent_confirmed_transactions members rather than globals.

No need to pass mempool to PeerManagerImpl methods.

…to PeerManagerImpl Allows converting mapBlocksInFlight and g_last_tip_update from globals to member variables.

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

…erImpl Allows making mapRelay and vRelayExpiration members rather than globals.

No need to pass mempool or connman to PeerManagerImpl methods.

…anagerImpl Allows making lNodesAnnouncingHeaderAndIDs and nPeersWithValidatedDownloads member vars instead of globals.

No need to pass connman to PeerManagerImpl methods.

ajtowns · 2021-01-29T16:55:03Z

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

jnewbery

Code review ACK 6452190

One minor suggestion inline

jnewbery · 2021-02-04T12:21:20Z

src/net_processing.cpp


-    /** Stack of nodes which we have set to announce using compact blocks */
-    std::list<NodeId> lNodesAnnouncingHeaderAndIDs GUARDED_BY(cs_main);
+    /* Returns a bool indicating whether we requested this block.


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

Added a todo commit in #20758 to track this

ariard

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.

src/net_processing.cpp

ariard · 2021-02-05T13:22:39Z

src/net_processing.cpp

+     */
+    bool MarkBlockAsInFlight(CTxMemPool& mempool, NodeId nodeid, const uint256& hash, const CBlockIndex* pindex = nullptr, std::list<QueuedBlock>::iterator** pit = nullptr) EXCLUSIVE_LOCKS_REQUIRED(cs_main);
+
+    bool TipMayBeStale(const Consensus::Params &consensusParams) EXCLUSIVE_LOCKS_REQUIRED(cs_main);


a490f0a

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

Added some text to 4f9931d

vasild · 2021-02-09T17:06:16Z

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 6452190 looks good.

jnewbery · 2021-02-15T10:37:04Z

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.

maflcko

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

Show signature and timestamp

Signature:

Timestamp of file with hash 39fd4a1774698d6853a62e53b467bd6ee8d7c78c218cf8d662c407770c5d971b -

maflcko · 2021-02-15T10:54:03Z

src/net_processing.cpp



-bool PeerManagerImpl::AlreadyHaveTx(const GenTxid& gtxid, const CTxMemPool& mempool) EXCLUSIVE_LOCKS_REQUIRED(cs_main)
+bool PeerManagerImpl::AlreadyHaveTx(const GenTxid& gtxid) EXCLUSIVE_LOCKS_REQUIRED(cs_main)


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.

Same for all other commits.

… PeerMa…

amitiuttarwar · 2021-02-15T21:59:39Z

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

Sjors · 2021-04-01T09:29:06Z

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

Summary: nSyncStarted, mapBlockSource, g_outbound_peers_with_protect_from_disconnect were all only used by PeerManagerImpl methods already. Changes related to `g_wtxid_relay_peers` are not present because these are for segwit. This is a backport of [[bitcoin/bitcoin#20942 | core#20942]] [1/9] bitcoin/bitcoin@9781c08 Test Plan: `ninja all check-all` Reviewers: #bitcoin_abc, sdulfari Reviewed By: #bitcoin_abc, sdulfari Subscribers: sdulfari Differential Revision: https://reviews.bitcoinabc.org/D11433

Summary: Allows making recentRejects and g_recent_confirmed_transactions members rather than globals, and remove need to pass mempool as argument. This is a backport of [[bitcoin/bitcoin#20942 | core#20942]] [2&3/9] bitcoin/bitcoin@eeac506 bitcoin/bitcoin@052d9bc Depends on D11433 Test Plan: `ninja all check-all` Reviewers: #bitcoin_abc, Fabien Reviewed By: #bitcoin_abc, Fabien Subscribers: Fabien Differential Revision: https://reviews.bitcoinabc.org/D11434

…to PeerManagerImpl Summary: Allows converting mapBlocksInFlight and g_last_tip_update from globals to member variables, and remove need to pass mempool or consensus params to methods. This is a backport of [[bitcoin/bitcoin#20942 | core#20942]] [4&5/9] bitcoin/bitcoin@a490f0a bitcoin/bitcoin@d440848 Note: `consensusParams` is not needed in our version of `FindNextBlocksToDownload`. Core uses it to test `IsWitnessEneabled`. Depends on D11434 Test Plan: `ninja all check-all` Reviewers: #bitcoin_abc, Fabien Reviewed By: #bitcoin_abc, Fabien Subscribers: Fabien Differential Revision: https://reviews.bitcoinabc.org/D11435

…erImpl Summary: Allows making mapRelay and vRelayExpiration members rather than globals. Remove need to pass mempool of conman to methods? This is a backport of [[bitcoin/bitcoin#20942 | core#20942]] [6&7/9] bitcoin/bitcoin@34207b9 bitcoin/bitcoin@7b7117e Depends on D11435 Notes : - also convert a uint256 to TxId in `mapRelay` type - rename `g_relay_expiration` to vRelayExpiration now that it is no longer a global (undoes a change in D10914). The `m_` prefix is not very commonly used in this class, so I feel like it is best to stick with the source material. Test Plan: `ninja all check-all` Reviewers: #bitcoin_abc, Fabien Reviewed By: #bitcoin_abc, Fabien Subscribers: Fabien Differential Revision: https://reviews.bitcoinabc.org/D11436

…anagerImpl Summary: Allows making lNodesAnnouncingHeaderAndIDs and nPeersWithValidatedDownloads member vars instead of globals. Removes the need to pass conman as a parameter to the method. This concludes a backport of [[bitcoin/bitcoin#20942 | core#20942]] [8&9/9] bitcoin/bitcoin@39c2a69 bitcoin/bitcoin@6452190 Depends on D11436 Test Plan: `ninja all check-all` Reviewers: #bitcoin_abc, Fabien Reviewed By: #bitcoin_abc, Fabien Differential Revision: https://reviews.bitcoinabc.org/D11437

ajtowns added Refactoring P2P labels Jan 15, 2021

ajtowns mentioned this pull request Jan 15, 2021

net-processing refactoring -- lose globals, move implementation details from .h to .cpp #20758

Closed

jnewbery reviewed Jan 18, 2021

View reviewed changes

DrahtBot mentioned this pull request Jan 28, 2021

tree-wide: De-globalize ChainstateManager #20158

Closed

12 tasks

DrahtBot added the Needs rebase label Jan 29, 2021

ajtowns added 9 commits January 30, 2021 01:26

net_processing: move some globals into PeerManagerImpl

9781c08

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

net_processing: move AlreadyHaveTx into PeerManageImpl

eeac506

Allows making recentRejects and g_recent_confirmed_transactions members rather than globals.

net_processing: simplify AlreadyHaveTx args

052d9bc

No need to pass mempool to PeerManagerImpl methods.

net_processing: move MarkBlockAs*, TipMayBeStale, FindNextBlocksToDL …

a490f0a

…to PeerManagerImpl Allows converting mapBlocksInFlight and g_last_tip_update from globals to member variables.

net_processing: simplify PeerManageImpl method args

d440848

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

net_processing: move FindTxForGetData and ProcessGetData to PeerManag…

34207b9

…erImpl Allows making mapRelay and vRelayExpiration members rather than globals.

net_processing: simplify ProcessGetData and FindTxForGetData args

7b7117e

No need to pass mempool or connman to PeerManagerImpl methods.

net_processing: move MaybeSetPeerAsAnnouncingHeadersAndIDs into PeerM…

39c2a69

…anagerImpl Allows making lNodesAnnouncingHeaderAndIDs and nPeersWithValidatedDownloads member vars instead of globals.

net_processing: simplify MaybeSetPeerAsAnnouncingHeaderAndIDs args

6452190

No need to pass connman to PeerManagerImpl methods.

ajtowns force-pushed the 202101-netproc-anti-globalisation-pt1 branch from a3ef949 to 6452190 Compare January 29, 2021 16:52

DrahtBot removed the Needs rebase label Jan 29, 2021

jnewbery approved these changes Feb 4, 2021

View reviewed changes

ariard reviewed Feb 5, 2021

View reviewed changes

ajtowns mentioned this pull request Feb 11, 2021

Split orphan handling from net_processing into txorphanage #21148

Merged

This was referenced Feb 12, 2021

Net Processing: Move RelayTransaction() into PeerManager #21162

Merged

net/net processing: Move tx inventory into net_processing #21160

Merged

maflcko reviewed Feb 15, 2021

View reviewed changes

maflcko merged commit 45ea103 into bitcoin:master Feb 15, 2021

maflcko mentioned this pull request Feb 15, 2021

scripted-diff: Remove redundant lock annotations in net processing #21188

Merged

sidhujag pushed a commit to syscoin/syscoin that referenced this pull request Feb 15, 2021

Merge bitcoin#20942: [refactor] Move some net_processing globals into…

026a911

… PeerMa…

bitcoin locked as resolved and limited conversation to collaborators Aug 16, 2022



		bool PeerManagerImpl::AlreadyHaveTx(const GenTxid& gtxid, const CTxMemPool& mempool) EXCLUSIVE_LOCKS_REQUIRED(cs_main)
		bool PeerManagerImpl::AlreadyHaveTx(const GenTxid& gtxid) EXCLUSIVE_LOCKS_REQUIRED(cs_main)

[refactor] Move some net_processing globals into PeerManagerImpl #20942

[refactor] Move some net_processing globals into PeerManagerImpl #20942

Uh oh!

Conversation

ajtowns commented Jan 15, 2021

Uh oh!

ajtowns commented Jan 15, 2021

Uh oh!

DrahtBot commented Jan 15, 2021 • edited Loading Uh oh! There was an error while loading. Please reload this page.

Uh oh!

Conflicts

Uh oh!

jnewbery left a comment

Choose a reason for hiding this comment

Uh oh!

Uh oh!

Uh oh!

Uh oh!

Uh oh!

Uh oh!

Uh oh!

ajtowns commented Jan 29, 2021

Uh oh!

jnewbery left a comment

Choose a reason for hiding this comment

Uh oh!

jnewbery Feb 4, 2021

Choose a reason for hiding this comment

Uh oh!

ajtowns Feb 15, 2021

Choose a reason for hiding this comment

Uh oh!

ariard left a comment • edited Loading Uh oh! There was an error while loading. Please reload this page.

Uh oh!

Choose a reason for hiding this comment

Uh oh!

Uh oh!

ariard Feb 5, 2021

Choose a reason for hiding this comment

Uh oh!

ajtowns Feb 11, 2021

Choose a reason for hiding this comment

Uh oh!

vasild commented Feb 9, 2021

Uh oh!

jnewbery commented Feb 15, 2021

Uh oh!

maflcko left a comment

Choose a reason for hiding this comment

Uh oh!

maflcko Feb 15, 2021

Choose a reason for hiding this comment

Uh oh!

maflcko Feb 15, 2021

Choose a reason for hiding this comment

Uh oh!

amitiuttarwar commented Feb 15, 2021

Uh oh!

Sjors commented Apr 1, 2021

Uh oh!

Reviewers

Assignees

Labels

Projects

Milestone

Development

Uh oh!

8 participants

DrahtBot commented Jan 15, 2021 •

edited

Loading

ariard left a comment •

edited

Loading