refactor: move net_processing implementation details out of header #20811

ajtowns · 2020-12-31T06:50:36Z

Moves the implementation details of PeerManager and all of struct Peer into net_processing.cpp.

ajtowns · 2020-12-31T06:51:51Z

This is the first step of the refactoring proposed in #20758 and just does encapsulation, it doesn't remove any of the globals.

EDIT: Rationale for moving things from the header into cpp files is to:

reduce compilation times (the code only gets compiled for a single cpp, rather than every cpp that includes the header)
reduce churn when updating the implementation (you only need to touch the cpp, not the header; and don't need to recompile other modules)
makes it easier to see what's interface and what's implementation details

More specifically, #20758 aims to reduce the number of net_processing globals by moving them into the PeerManager class, which will involve less churn if that class isn't exposed as a header file; and #19398 aims to move things from net to net_processing which also involves less churn if it doesn't need to go into the net_processing header file.

DrahtBot · 2020-12-31T08:17:51Z

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: (de)serialize CSubNet in addrv2 format #20904 (net: (de)serialize CSubNet in addrv2 format by vasild)
p2p: standardize outbound full/block relay connection type naming #20729 (p2p: standardize on outbound-{full, block}-relay connection type naming by jonatack)
rpc: getblockfrompeer #20295 (rpc: getblockfrompeer by Sjors)
addrman: Make addrman a top-level component #20228 (addrman: Make addrman a top-level component 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.

jnewbery · 2021-01-01T01:00:38Z

Concept ACK. I like this kind of encapsulation a lot.

I'm not sure why the TSAN build is failing with "WARNING: ThreadSanitizer: double lock of a mutex". It seems like a false alarm to me. I think it's choking on std::condition_variable condMsgProc/Mutex mutexMsgProc so maybe it's something similar to google/sanitizers#1259?

src/net_processing.cpp

jnewbery

utACK 13a7fec assuming the tsan suppression is fixed.

src/net_processing.h

src/net_processing.cpp

src/net_processing.h

jnewbery · 2021-01-02T09:49:38Z

utACK fa6b227

jnewbery · 2021-01-03T10:37:12Z

utACK 7bd4517

Sjors · 2021-01-03T12:44:42Z

Would it make sense to move PeerManager to its own file (given how much this PR is moving stuff around anyway)?

jnewbery · 2021-01-03T13:30:53Z

utACK abeecaa

Thanks for being so responsive to review comments!

ajtowns · 2021-01-03T13:31:47Z

Bumped to fix missed indentation.

@Sjors After the further commits in #20758 almost all of net_processing is moved into PeerManager, so its effectively a dedicated file anyway. Splitting out sub-components (like orphan handling) into their own modules would be a good next step afterwards, I think.

src/net.h

jnewbery · 2021-01-07T09:35:09Z

I'm not sure if adding the comments to NetEventsInterface adds much.

utACK 02f6776.

jnewbery

utACK 3bcde8b

Did a git range-diff to verify that only the comments in NetEventsInterface have changed.

jnewbery · 2021-01-09T12:39:03Z

src/net.h

@@ -769,11 +769,28 @@ class CNode
 class NetEventsInterface
 {
 public:
-    virtual bool ProcessMessages(CNode* pnode, std::atomic<bool>& interrupt) = 0;
-    virtual bool SendMessages(CNode* pnode) EXCLUSIVE_LOCKS_REQUIRED(pnode->cs_sendProcessing) = 0;
+    /** Initialize a peer (setup state, queue any initial messages) */


I don't think it's a big deal, but if you're going to add these comments to the interface class, I think there are some potential improvements:

"queue any initial messages" is only true for the version message for outbound peers. Since it's not universally true, it probably doesn't belong in the description of the interface.

SendMessages() return value is not used to indicate if more work is done. The function always returns true and that value is discarded by the caller. A future commit could change the function to return void.

ProcessMessages() does return a bool that indicates whether there's more work.

Eventually, these functions could all be updated to just take a single CNode& argument, which would be a very clean interface (#20228 removes the update_connection_time arg from FinalizeNode(), and I have another branch that eliminates interrupt).

SendMessages lost its new lock annotation in the rebase, so I've added the doc from ProcessMessages return value.

I think "any initial messages" covers nothing, just a version message, or anything else we might think up fine. The return value of SendMessages is "used" in the unit tests, so haven't removed that in this PR. Looking at the CNode& thing in #20758

…lasses

jnewbery · 2021-01-09T13:51:21Z

ACK c97f70c

Sjors · 2021-01-11T11:50:00Z

ACK c97f70c

After the further commits in #20758 almost all of net_processing is moved into PeerManager, so its effectively a dedicated file anyway. Splitting out sub-components (like orphan handling) into their own modules would be a good next step afterwards, I think.

That sounds good, because net_processing.cpp is almost 5000 lines now, with a whole bunch of classes and structs.

vasild

ACK c97f70c

Code review ok (some minor questions below), compiles locally with clang 12 without warnings, unit and functional tests pass.

vasild · 2021-01-12T08:59:18Z

src/net_processing.h

-using PeerRef = std::shared_ptr<Peer>;
-
-class PeerManager final : public CValidationInterface, public NetEventsInterface {
+class PeerManager : public CValidationInterface, public NetEventsInterface


nit: maybe consider renaming
PeerManager -> PeerManagerInterface
PeerManagerImpl -> PeerManager
for consistency with CValidationInterface and NetEventsInterface.

The way I look at it is that Interface is for defining a class where some other module is going to implement the methods that do the actual work; but the work to fulfill the PeerManager API really is done by net_processing; it's just an implementation detail that the methods are virtual and inheritance is used -- the unique_ptr<Impl> m_impl; approach used in txrequest could equally well have been used too. So I think PeerManager* is the right name for what is being exposed to other modules here.

src/net_processing.cpp

vasild · 2021-01-12T09:11:01Z

src/net_processing.cpp

+} // namespace
+
 namespace {


nit:

Suggested change

} // namespace

namespace {

Same for below L462-464. I don't think this needs to be fixed in this PR. When it is, everything in the second anonymous namespace (L351-462) should be unindented - our style guide says that namespace blocks shouldn't be indented.

See #20758 -- that has patches that move the entries from those namespaces into PeerManager (and thus skips over unindenting them). Certainly could remove the redundant close/open, but it seemed a useful indicator of where the work's up to to me, and it doesn't seem harmful.

ariard · 2021-01-13T02:47:09Z

Reviewing this really soon if you're looking for more eyes on it before merge.

laanwj · 2021-01-13T08:47:28Z

Nice, good thing to have less implementation details in (commonly included) header files.

Code review ACK c97f70c

Reviewing this really soon if you're looking for more eyes on it before merge.

Don't let it being merged keep you from reviewing it please 😄

…ails out

ariard

Code Review ACK c97f70c

Good code restructuring direction, only minors.

ariard · 2021-01-14T23:13:36Z

src/net_processing.h

+    bool ProcessMessages(CNode* pfrom, std::atomic<bool>& interrupt) override;
+    bool SendMessages(CNode* pto) override EXCLUSIVE_LOCKS_REQUIRED(pto->cs_sendProcessing);
+
+    /** Implement PeerManager */


I think comment here and above should have been updated as "Overriden from..." now we have a final class.

ariard · 2021-01-14T23:24:28Z

src/net_processing.cpp

+    /**
+     * Potentially mark a node discouraged based on the contents of a BlockValidationState object
+     *
+     * @param[in] via_compact_block this bool is passed in because net_processing should


nit: This param name could be more precise, according to the BIP, the waiver around invalid transactions is restrained to high-bandwidth only, so could be via_hb_compact_block.

ariard · 2021-01-14T23:34:42Z

src/net_processing.cpp

+    void SendBlockTransactions(CNode& pfrom, const CBlock& block, const BlockTransactionsRequest& req);
+
+    /** Register with TxRequestTracker that an INV has been received from a
+     *  peer. The announcement parameters are decided in PeerManager and then


Should say PeerManagerImpl now ?

ariard · 2021-01-15T00:15:35Z

src/net_processing.h

-    void SetBestHeight(int height) { m_best_height = height; };
+    /**
+     * Evict extra outbound peers. If we think our tip may be stale, connect to an extra outbound.
+     * Public for unit testing.


Could have been better to comment in commit message the re-ordering logic followed "Move method exposed for test-only at the end of public method space declaration" :) ?

Summary: Move only, comment only. Partial backport of [[bitcoin/bitcoin#20811 | core#20811]]: bitcoin/bitcoin@0d246a5 Ref T1696. Depends on D10881. Test Plan: Read the comments Reviewers: #bitcoin_abc, PiRK Reviewed By: #bitcoin_abc, PiRK Maniphest Tasks: T1696 Differential Revision: https://reviews.bitcoinabc.org/D10882

Summary: Partial backport of [[bitcoin/bitcoin#20811 | core#20811]]: bitcoin/bitcoin@0df3d3f Ref T1696. Test Plan: ninja all check-all ninja bitcoin-fuzzers Reviewers: #bitcoin_abc, PiRK Reviewed By: #bitcoin_abc, PiRK Maniphest Tasks: T1696 Differential Revision: https://reviews.bitcoinabc.org/D10886

…lasses Summary: Partial backport of [[bitcoin/bitcoin#20811 | core#20811]]: bitcoin/bitcoin@a568b82 Note that the cpp file deserves a good reordering, but that's out of scope for this diff. Depends on D10886. Ref T1696. Test Plan: ninja all check-all ninja bitcoin-fuzzers Reviewers: #bitcoin_abc, PiRK Reviewed By: #bitcoin_abc, PiRK Maniphest Tasks: T1696 Differential Revision: https://reviews.bitcoinabc.org/D10887

Summary: Partial backport of [[bitcoin/bitcoin#20811 | core#20811]]: bitcoin/bitcoin@e0f2e6d This is a move only change. The double namespace is kept to limit later merge conflicts, it's harmless. Depends on D10887. Ref T1696. Test Plan: ninja all check-all Reviewers: #bitcoin_abc, PiRK Reviewed By: #bitcoin_abc, PiRK Maniphest Tasks: T1696 Differential Revision: https://reviews.bitcoinabc.org/D10890

Summary: Completes backport of [[bitcoin/bitcoin#20811 | core#20811]]: bitcoin/bitcoin@c97f70c Depends on D10890. Ref T1696. Test Plan: ninja all check-all Reviewers: #bitcoin_abc, PiRK Reviewed By: #bitcoin_abc, PiRK Maniphest Tasks: T1696 Differential Revision: https://reviews.bitcoinabc.org/D10892

ajtowns force-pushed the 202012-netproc-impl-split branch from 62ac65d to 56423c5 Compare December 31, 2020 06:57

DrahtBot added P2P Refactoring labels Dec 31, 2020

ajtowns force-pushed the 202012-netproc-impl-split branch from 56423c5 to 13a7fec Compare December 31, 2020 13:17

maflcko reviewed Jan 1, 2021

View reviewed changes

src/net_processing.cpp Outdated Show resolved Hide resolved

jnewbery reviewed Jan 1, 2021

View reviewed changes

src/net_processing.h Outdated Show resolved Hide resolved

src/net_processing.cpp Outdated Show resolved Hide resolved

src/net_processing.cpp Show resolved Hide resolved

src/net_processing.cpp Outdated Show resolved Hide resolved

src/net_processing.h Show resolved Hide resolved

ajtowns force-pushed the 202012-netproc-impl-split branch from 13a7fec to fa6b227 Compare January 1, 2021 12:51

DrahtBot added the Needs rebase label Jan 2, 2021

ajtowns force-pushed the 202012-netproc-impl-split branch from fa6b227 to 7bd4517 Compare January 3, 2021 02:09

DrahtBot removed the Needs rebase label Jan 3, 2021

DrahtBot mentioned this pull request Jan 3, 2021

doc: Corrected Typos in Documentation #20836

Closed

ajtowns force-pushed the 202012-netproc-impl-split branch from 7bd4517 to abeecaa Compare January 3, 2021 13:28

ajtowns mentioned this pull request Jan 4, 2021

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

Closed

DrahtBot mentioned this pull request Jan 4, 2021

docs: consolidate typo & url fixing #20842

Merged

DrahtBot added the Needs rebase label Jan 5, 2021

jnewbery reviewed Jan 7, 2021

View reviewed changes

src/net.h Outdated Show resolved Hide resolved

DrahtBot added the Needs rebase label Jan 7, 2021

jnewbery mentioned this pull request Jan 7, 2021

Move remaining application layer data to net processing #19398

Closed

16 tasks

richardsonmark316 approved these changes Jan 7, 2021

View reviewed changes

ajtowns force-pushed the 202012-netproc-impl-split branch from 02f6776 to 3bcde8b Compare January 9, 2021 06:51

DrahtBot removed the Needs rebase label Jan 9, 2021

jnewbery reviewed Jan 9, 2021

View reviewed changes

ajtowns added 5 commits January 9, 2021 23:27

net, net_processing: move NetEventsInterface method docs to net.h

0d246a5

net_processing: make more of PeerManager private

0df3d3f

net_processing: split PeerManager into interface and implementation c…

a568b82

…lasses

net_processing: move PeerManagerImpl into cpp file

e0f2e6d

net_processing: move Peer definition to .cpp

c97f70c

ajtowns force-pushed the 202012-netproc-impl-split branch from 3bcde8b to c97f70c Compare January 9, 2021 13:29

DrahtBot mentioned this pull request Jan 11, 2021

net: (de)serialize CSubNet in addrv2 format #20904

Closed

vasild approved these changes Jan 12, 2021

View reviewed changes

laanwj merged commit 60427ee into bitcoin:master Jan 13, 2021

sidhujag pushed a commit to syscoin/syscoin that referenced this pull request Jan 13, 2021

Merge bitcoin#20811: refactor: move net_processing implementation det…

375df75

…ails out

ariard reviewed Jan 15, 2021

View reviewed changes

amitiuttarwar mentioned this pull request May 19, 2021

Dedup and RAII-fy the creation of a copy of CConnman::vNodes #21943

Closed

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

Provide feedback

Saved searches

Use saved searches to filter your results more quickly

refactor: move net_processing implementation details out of header #20811

refactor: move net_processing implementation details out of header #20811

ajtowns commented Dec 31, 2020

ajtowns commented Dec 31, 2020 •

edited

Loading

DrahtBot commented Dec 31, 2020 •

edited

Loading

jnewbery commented Jan 1, 2021

jnewbery left a comment

jnewbery commented Jan 2, 2021

jnewbery commented Jan 3, 2021

Sjors commented Jan 3, 2021

jnewbery commented Jan 3, 2021

ajtowns commented Jan 3, 2021

jnewbery commented Jan 7, 2021

jnewbery left a comment

jnewbery Jan 9, 2021 •

edited

Loading

ajtowns Jan 9, 2021

jnewbery commented Jan 9, 2021

Sjors commented Jan 11, 2021

vasild left a comment

vasild Jan 12, 2021

ajtowns Jan 12, 2021

vasild Jan 12, 2021

jnewbery Jan 12, 2021

ajtowns Jan 12, 2021

ariard commented Jan 13, 2021

laanwj commented Jan 13, 2021 •

edited

Loading

ariard left a comment

ariard Jan 14, 2021

ariard Jan 14, 2021

ariard Jan 14, 2021

ariard Jan 15, 2021

refactor: move net_processing implementation details out of header #20811

refactor: move net_processing implementation details out of header #20811

Conversation

ajtowns commented Dec 31, 2020

ajtowns commented Dec 31, 2020 • edited Loading

DrahtBot commented Dec 31, 2020 • edited Loading

Conflicts

jnewbery commented Jan 1, 2021

jnewbery left a comment

Choose a reason for hiding this comment

jnewbery commented Jan 2, 2021

jnewbery commented Jan 3, 2021

Sjors commented Jan 3, 2021

jnewbery commented Jan 3, 2021

ajtowns commented Jan 3, 2021

jnewbery commented Jan 7, 2021

jnewbery left a comment

Choose a reason for hiding this comment

jnewbery Jan 9, 2021 • edited Loading

Choose a reason for hiding this comment

Choose a reason for hiding this comment

jnewbery commented Jan 9, 2021

Sjors commented Jan 11, 2021

vasild left a comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

ariard commented Jan 13, 2021

laanwj commented Jan 13, 2021 • edited Loading

ariard left a comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

ajtowns commented Dec 31, 2020 •

edited

Loading

DrahtBot commented Dec 31, 2020 •

edited

Loading

jnewbery Jan 9, 2021 •

edited

Loading

laanwj commented Jan 13, 2021 •

edited

Loading