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

ajtowns commented at 9:25 pm on December 23, 2020: contributor

Done in #20811 :

Moves implementation details of PeerManager into PeerManagerImpl; moves PeerManagerImpl into .cpp
Moves struct Peer definition into .cpp

Done in #20942:

Moves net_processing globals into PeerManagerImpl.
Moves a lot of net_processing functions into PeerManagerImpl and simplifies some of their arguments as a result.

In #21148:

Split orphan management into its own module
More more globals/functions into PeerManagerImpl

Additional things in this PR:

Moves more globals/functions into PeerManagerImpl; simplify their arguments.
Makes test/denialofservice_tests use the PeerManager api instead of directly accessing net_processing implementation details
Introduces State(CNode&) and State(Peer&) alternatives to calling State(node.GetId()) all the time.
Moves CNodeState into Peer as a step towards consolidating state information
Make NetEventsInterface take a CNode& instead of a CNode*
Pass ArgsManager into the constructor and collect parameters once rather than calling GetArgs regularly at runtime

Not done yet:

Split parts of PeerManager into their own modules a la txrequest (block downloads?)

ajtowns commented at 9:37 pm on December 23, 2020: contributor

Marked as WIP; definitely needs some review/testing.

The move CNodeState into Peer commit might not be the greatest idea – the locking issues are a little subtle (assumign I got them right). Not sure the the locking guidelines for Peer make sense (see “annotate lock guideline violation?” commit) though (and generally not convinced we need the multiple per-peer state mutexes that Peer has).

I think it might make sense to have CNode have an opaque Peer pointer member that’s provided by net_processing when the connection is first made and passed back to net_processing via the PeerManager interface – that would obsolete a lot of the “state for this peer” lookups, and might simplify locking substantially. Bit complicated to think about, so I stopped at this point, but that was what I was originally looking to explore here.

DrahtBot commented at 9:37 pm on December 23, 2020: contributor

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

Conflicts

Reviewers, this pull request conflicts with the following ones:

#21090 (Default to NODE_WITNESS in nLocalServices by dhruv)
#21061 ([p2p] Introduce node rebroadcast module by amitiuttarwar)
#21015 (Make all of net_processing (and some of net) use std::chrono types by dhruv)
#20966 (banman: save the banlist in a JSON format on disk by vasild)
#20721 (Net: Move ping data to net_processing by jnewbery)
#20295 (rpc: getblockfrompeer by Sjors)
#20228 (addrman: Make addrman a top-level component by jnewbery)
#19438 (Introduce deploymentstatus by ajtowns)
#18261 (Erlay: bandwidth-efficient transaction relay protocol by naumenkogs)

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.

DrahtBot added the label P2P on Dec 23, 2020

in src/net_processing.cpp:1322 in 7f479a56b7 outdated

1318@@ -1319,7 +1319,7 @@ void PeerManager::UpdatedBlockTip(const CBlockIndex *pindexNew, const CBlockInde
1319         LOCK(m_peer_mutex);
1320         for (auto& it : m_peer_map) {
1321             Peer& peer = *it.second;
1322-            LOCK(peer.m_block_inv_mutex);
1323+            LOCK(peer.m_block_inv_mutex); // TODO: violates "Mutexes inside this struct must not be held when locking m_peer_mutex." rule??

jnewbery commented at 10:49 pm on December 23, 2020:

You’re the second person who’s been confused by this comment (https://github.com/bitcoin/bitcoin/pull/19829#discussion_r546338754), which tells me that I didn’t make it clear enough.

It’s fine to lock m_peer_mutex and then lock one of the inner mutexes, or even to lock m_peer_mutex, take a shared_ptr to the Peer, release m_peer_mutex and then lock an inner mutex. The only rule is that we don’t want to lock an inner mutex and then lock m_peer_mutex.

If you have better wording for the comment, I’m very happy to take it.

ajtowns commented at 5:09 am on December 30, 2020:

Ah, in that case I would describe it as a lock ordering constraint – “if you need to acquire internal locks from multiple peers, acquire m_peer_mutex first, then acquire the peers’ locks and do your work, then release the internal locks, then release m_peer_mutex. If you are already holding an internal lock, you must release it before acquiring m_peer_mutex (eg by calling GetPeerRef)”.

(It’s probably already clear enough on its own; it’s just that the above would go unwritten everywhere else in the codebase, so that it’s written down here make it sound like there’s some stricter requirement going on…)

jnewbery commented at 10:54 pm on December 23, 2020: member

Strong concept ACK. I really like the idea of compilation firewalls and not exposing the innards of net_processing through the header file.

I don’t know where this fits in the sequence of PRs for #19398. Doing it first makes some sense because it means we won’t be putting stuff into the header file only to remove it later, but I think this PR will need to be split up into more digestible pieces to be merged.

in src/net_processing.cpp:338 in 85d78a9112 outdated

333+
334+    explicit Peer(NodeId id, CAddress addr, bool is_inbound) : m_id(id),  nodestate(addr, is_inbound) {}
335+};
336+
337+namespace {
338+class PeerManagerImpl final : public PeerManager {

jnewbery commented at 11:43 pm on December 23, 2020:

This isn’t what I was expecting to see. I was assuming that you’d use a pimpl idiom where the PeerManager would hold a unique_ptr to the PeerManagerImpl rather than have PeerManagerImpl inherit from PeerManager.

I’m not saying there’s a problem with this way, but I’m curious what made you choose it.

ajtowns commented at 3:47 am on December 24, 2020:

PeerManager does multiple inheritance already so the virtual overhead’s already there, and this avoids two lots of unique pointers?

Limpisey168 approved

Limpisey168 commented at 0:14 am on December 24, 2020: none

in src/net_processing.h:59 in 85d78a9112 outdated

94-
95-    explicit Peer(NodeId id) : m_id(id) {}
96-};
97+struct Peer; // body is defined in net_processing.cpp
98 
99 using PeerRef = std::shared_ptr<Peer>;

jnewbery commented at 11:18 am on December 24, 2020:

This alias (and therefore the forward declaration of Peer above) don’t need to be in the header file. PeerRef is only used in net_processing.cpp. I’d also suggest moving the Peer comment to the cpp file.

ajtowns commented at 4:48 am on December 30, 2020:

I left PeerRef in the header in the hopes of having net.cpp manage the state (though in that case it would potentially become a unique_ptr)

ajtowns commented at 6:54 am on December 31, 2020:

Moved now. Trivial to move back if net.cpp ends up owning them.

in src/net_processing.h:69 in 85d78a9112 outdated

163-     */
164-    void Misbehaving(const NodeId pnode, const int howmuch, const std::string& message);
165+    /** Relay transaction to every node */
166+    virtual void RelayTransaction(const uint256& txid, const uint256& wtxid) EXCLUSIVE_LOCKS_REQUIRED(cs_main) = 0;
167 
168+public: // exposed as debugging info for RPC

jnewbery commented at 11:20 am on December 24, 2020:

I’m not sure these additional public access specifiers are useful. Just the comments is sufficient.

ajtowns commented at 6:54 am on December 31, 2020:

Removed

in src/net_processing.h:64 in 85d78a9112 outdated

152-    void ReattemptInitialBroadcast(CScheduler& scheduler) const;
153-
154-    /** Process a single message from a peer. Public for fuzz testing */
155-    void ProcessMessage(CNode& pfrom, const std::string& msg_type, CDataStream& vRecv,
156-                        const std::chrono::microseconds time_received, const std::atomic<bool>& interruptMsgProc);
157+    virtual void CheckForStaleTipAndEvictPeers() = 0;

jnewbery commented at 11:20 am on December 24, 2020:

This can be in the ’exposed for tests’ section.

in src/net_processing.h:108 in 85d78a9112 outdated

283 };
284 
285-/** Relay transaction to every node */
286-void RelayTransaction(const uint256& txid, const uint256& wtxid, const CConnman& connman) EXCLUSIVE_LOCKS_REQUIRED(cs_main);
287+std::unique_ptr<PeerManager> make_PeerManager(const CChainParams& chainparams, CConnman& connman, BanMan* banman,
288+                CScheduler& scheduler, ChainstateManager& chainman, CTxMemPool& pool,

jnewbery commented at 11:23 am on December 24, 2020:

Should this be aligned with the ( above instead of the <?

in src/net_processing.h:87 in 85d78a9112 outdated

275+    virtual unsigned int LimitOrphanTxSize(unsigned int nMaxOrphans) = 0;
276+    virtual void UpdateLastBlockAnnounceTime(NodeId node, int64_t time_in_seconds) = 0;
277+
278+    /** Map from txid to orphan transaction record. Limited by
279+     *  -maxorphantx/DEFAULT_MAX_ORPHAN_TRANSACTIONS */
280+    std::map<uint256, COrphanTx> mapOrphanTransactions GUARDED_BY(g_cs_orphans);

jnewbery commented at 11:39 am on December 24, 2020:

It’d be great to eventually move all of this fully inside PeerManagerImpl, or break it out into an OrphansManager class.

ajtowns commented at 4:56 am on December 30, 2020:

I did spend a few moments trying to break it out, but my first approach didn’t work immediately, so I’m leaving it for later

jnewbery commented at 11:40 am on December 24, 2020: member

There are a few more things that can be deleted from the header file or moved into the cpp file:

 0→ git --no-pager d
 1diff --git a/src/net_processing.cpp b/src/net_processing.cpp
 2index ca016ec52b..1c7638a774 100644
 3--- a/src/net_processing.cpp
 4+++ b/src/net_processing.cpp
 5@@ -26,6 +26,7 @@
 6 #include <streams.h>
 7 #include <tinyformat.h>
 8 #include <txmempool.h>
 9+#include <txrequest.h>
10 #include <util/check.h> // For NDEBUG compile time check
11 #include <util/strencodings.h>
12 #include <util/system.h>
13@@ -290,6 +291,20 @@ struct CNodeState {
14 };
15 } // namespace
16 
17+/**
18+ * Data structure for an individual peer. This struct is not protected by
19+ * cs_main since it does not contain validation-critical data.
20+ *
21+ * Memory is owned by shared pointers and this object is destructed when
22+ * the refcount drops to zero.
23+ *
24+ * Mutexes inside this struct must not be held when locking m_peer_mutex.
25+ *
26+ * Details are all local to net_processing.cpp
27+ *
28+ * TODO: move most members from CNodeState to this structure.
29+ * TODO: move remaining application-layer data members from CNode to this structure.
30+ */
31 struct Peer {
32     /** Same id as the CNode object for this peer */
33     const NodeId m_id{0};
34@@ -334,6 +349,8 @@ struct Peer {
35     explicit Peer(NodeId id, CAddress addr, bool is_inbound) : m_id(id),  nodestate(addr, is_inbound) {}
36 };
37 
38+using PeerRef = std::shared_ptr<Peer>;
39+
40 namespace {
41 class PeerManagerImpl final : public PeerManager {
42 public:
43diff --git a/src/net_processing.h b/src/net_processing.h
44index ee34a38178..74eabd6376 100644
45--- a/src/net_processing.h
46+++ b/src/net_processing.h
47@@ -6,19 +6,12 @@
48 #ifndef BITCOIN_NET_PROCESSING_H
49 #define BITCOIN_NET_PROCESSING_H
50 
51-#include <consensus/params.h>
52 #include <net.h>
53-#include <sync.h>
54-#include <txrequest.h>
55 #include <validationinterface.h>
56 
57-class BlockTransactionsRequest;
58-class BlockValidationState;
59-class CBlockHeader;
60 class CChainParams;
61 class CTxMemPool;
62 class ChainstateManager;
63-class TxValidationState;
64 
65 extern RecursiveMutex cs_main;
66 extern RecursiveMutex g_cs_orphans;
67@@ -40,24 +33,6 @@ struct CNodeStateStats {
68     std::vector<int> vHeightInFlight;
69 };
70 
71-/**
72- * Data structure for an individual peer. This struct is not protected by
73- * cs_main since it does not contain validation-critical data.
74- *
75- * Memory is owned by shared pointers and this object is destructed when
76- * the refcount drops to zero.
77- *
78- * Mutexes inside this struct must not be held when locking m_peer_mutex.
79- *
80- * Details are all local to net_processing.cpp
81- *
82- * TODO: move most members from CNodeState to this structure.
83- * TODO: move remaining application-layer data members from CNode to this structure.
84- */
85-struct Peer; // body is defined in net_processing.cpp
86-
87-using PeerRef = std::shared_ptr<Peer>;
88-
89 class PeerManager : public CValidationInterface, public NetEventsInterface {
90 public:
91     /** Evict extra outbound peers. If we think our tip may be stale, connect to an extra outbound */

jnewbery commented at 2:01 pm on December 26, 2020: member

I’ve made a branch at https://github.com/jnewbery/bitcoin/tree/pr20758.1 that implements something I’ve wanted to try for a while. It moves PeerManagerImpl into its own translation unit (net_processing_impl.cpp), with the declaration of PeerManagerImpl in net_processing_impl.h. That header file is only included by net_processing.cpp (so the std::unique_ptr can be constructed) and the test files. The tests are updated to use a PeerManagerImpl instead of a PeerManager.

The result is that PeerManager and net_processing.h don’t need to include any functions that are for test only. That header file and PeerManager’s public interface are about as minimal as possible (the only exposed methods are RelayTransaction, GetNodeStateStats and IgnoresIncomingTxs). Meanwhile, everything in PeerManagerImpl can be made public (since it’s behind a compilation firewall so effectively everything is hidden from other translation units), and the tests can access any method/data in the class.

What do you think? Is this something we should consider?

ajtowns commented at 0:33 am on December 28, 2020: contributor

It moves PeerManagerImpl into its own translation unit

I think a better approach would be to have module.h as the public interface, module_impl.h as the white-box testing interface (ie the class definition of PeerManagerImpl), and module.cpp as the implementation – no need to have two cpp files for what’s really only a single module, and it also avoids moving the code around.

I think maybe it’d be even better to focus on making the classes smaller so they can be tested purely via their public interfaces though, and not worry too much about exposing a few extra methods in the meantime?

jnewbery commented at 4:13 pm on December 29, 2020: member

I think a better approach would be to have module.h as the public interface, module_impl.h as the white-box testing interface (ie the class definition of PeerManagerImpl), and module.cpp as the implementation – no need to have two cpp files for what’s really only a single module, and it also avoids moving the code around.

ACK. This seems even better!

I think maybe it’d be even better to focus on making the classes smaller so they can be tested purely via their public interfaces though, and not worry too much about exposing a few extra methods in the meantime?

Also ACK making classes smaller. I think the tx_request module is a great example of how we can make the code clearer and better tested by breaking up the functionality. Perhaps block downloading, orphan handling, addr relay, etc could get similar treatment in future.

Aside: I think ’tested purely via their public interfaces’ is an OOP mantra that sounds good in theory, but white-box testing is too useful to throw out entirely, especially when dealing with legacy code that has existing large modules/classes.

This can all be left for future work though. I think just implementing the PeerManagerImpl is a great first step. I’d love to help get this reviewed and merged. Perhaps everything up to move PeerManagerImpl into cpp file could be split into a PR for review first?

ajtowns commented at 4:54 am on December 30, 2020: contributor

Aside: I think ’tested purely via their public interfaces’ is an OOP mantra that sounds good in theory, but white-box testing is too useful to throw out entirely, especially when dealing with legacy code that has existing large modules/classes.

I still think that’s white-box testing – it’s just that calling public functions is equivalent to entering at function boundaries, while calling private functions is more like a GOTO into the middle of a function. But I don’t mind having test-only sanity check functions (like in txrequest.h)…

ajtowns force-pushed on Dec 31, 2020

ajtowns force-pushed on Jan 1, 2021

ajtowns force-pushed on Jan 4, 2021

ajtowns force-pushed on Jan 9, 2021

ajtowns force-pushed on Jan 10, 2021

in src/net_processing.cpp:154 in 536a57b076 outdated

160@@ -161,6 +161,14 @@ void EraseOrphansFor(NodeId peer);
161 
162 // Internal stuff
163 namespace {
164+/** Blocks that are in flight, and that are in the queue to be downloaded. */
165+struct QueuedBlock {
166+    uint256 hash;
167+    const CBlockIndex* pindex;                               //!< Optional.
168+    bool fValidatedHeaders;                                  //!< Whether this block has validated headers at the time of request.

ariard commented at 1:13 am on January 13, 2021:

It was already there but it’s confusing to employ a verb for a data struct. Also blocks have only one header.

ajtowns commented at 4:34 am on January 13, 2021:

I think you’re misreading? Should be parsed as “Whether (this block) has (validated headers)” not “Whether (this block) (has validated) headers”. Unless I’ve messed up, this should just be moving in this PR, so would rather avoid editing it.

ariard commented at 0:29 am on January 15, 2021:

Honestly, I’m not native so don’t get the semantic difference you’re pointing to me :/

ajtowns commented at 3:40 am on January 15, 2021:

I think “Whether this block has (headers which have been validated)” is the right interpretation, versus “Whether this block (has itself taken the action of validating) headers” while a valid interpretation isn’t the intended one. “has validated” is ambiguous between being the past tense of the verb to validate, or the present tense of the verb “to have” and the adjective “validated”.

ariard commented at 11:31 pm on January 16, 2021:

Gotcha, I agree with the interpretation :)

in src/net_processing.cpp:1130 in 77aa3edcb0 outdated

1077@@ -1075,7 +1078,7 @@ PeerRef PeerManagerImpl::GetPeerRef(NodeId id) const
1078     return it != m_peer_map.end() ? it->second : nullptr;
1079 }
1080 
1081-PeerRef PeerManagerImpl::RemovePeer(NodeId id)
1082+PeerRef PeerManagerImpl::RemovePeer(NodeId id) EXCLUSIVE_LOCKS_REQUIRED(cs_main)

ariard commented at 1:46 am on January 13, 2021:

Superfluous lock annotation, already present on method declaration in net_processing.h?

ajtowns commented at 4:40 am on January 13, 2021:

The declaration is in net_processing.cpp. It is redundant, yeah; I’m not 100% convinced the locking changes for including CNodeState into Peer are adequate (in particular, that it’s reasonable to be confident that the interaction with m_peer_mutex is okay), so haven’t cleaned this up.

in src/net_processing.cpp:614 in 2c5ae7a923 outdated

603@@ -601,6 +604,9 @@ class PeerManagerImpl final : public PeerManager
604     /** Number of preferable block download peers. */
605     int nPreferredDownload GUARDED_BY(cs_main) = 0;
606 
607+    /** Height of highest fast announce block, updated by NewPoWValidBlock */
608+    int m_height_of_highest_fast_announce GUARDED_BY(cs_main) = 0;

ariard commented at 2:30 am on January 13, 2021:

Maybe m_height_highest_cb_fast_announced, inducing a unit is always better IMO.

in src/net.h:768 in 0d246a59b6 outdated

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

ariard commented at 2:31 am on January 13, 2021:

Maybe precise that’s network-related state, and that we queue ready-to-send messages, not received messages yet.

ariard commented at 2:45 am on January 13, 2021: member

Just did a first look to understand magnitude of change. I think it’s hard to Concept ACK the whole as it’s more a bundle of refactoring, maybe you can provide a rational for each subset of changes.

Also ACK making classes smaller. I think the tx_request module is a great example of how we can make the code clearer and better tested by breaking up the functionality. Perhaps block downloading, orphan handling, addr relay, etc could get similar treatment in future.

Yes I’m concept ACK on this overall direction!

ajtowns force-pushed on Jan 13, 2021

ajtowns commented at 0:53 am on January 14, 2021: contributor

Rebased now #20811 is merged. I think the next step is to split some of the commits moving globals into PeerManagerImpl members into a separate PR.

ajtowns force-pushed on Jan 15, 2021

ajtowns commented at 8:40 am on January 15, 2021: contributor

Pulled out some commits (and added one to drop connman from MaybeSetPeerAsAnnoun...) into #20942, so that’s the next set to review.

ajtowns force-pushed on Jan 31, 2021

ajtowns commented at 4:41 pm on January 31, 2021: contributor

Rebased on #20942 and included a set of 10 commits to introduce txorphanage.h and cpp that pulls out (and de-globalises) orphan tracking. The only remaining global there is g_cs_orphans. I think it would make sense to move the orphan work sets from net_processing.cpp:Peer into a new map in txorphanage.h:TxOrphanage – that would allow the lock to be moved into the class as well, and might also make it reasonable to do cleverer things like track multiple announcements of orphans (if we need to retry requesting their parents eg).

ajtowns force-pushed on Feb 16, 2021

DrahtBot added the label Needs rebase on Dec 13, 2021

DrahtBot commented at 11:18 pm on December 13, 2021: contributor

🐙 This pull request conflicts with the target branch and needs rebase.

Want to unsubscribe from rebase notifications on this pull request? Just convert this pull request to a “draft”.

DrahtBot commented at 1:06 pm on March 21, 2022: contributor

Is it still relevant? ➡️ Please solve the conflicts to make it ready for review and to ensure the CI passes.
Is it no longer relevant? ➡️ Please close.
Did the author lose interest or time to work on this? ➡️ Please close it and mark it ‘Up for grabs’ with the label, so that it can be picked up in the future.

net_processing: move CNodeState definition earlier b3978c41af

net_processing: Move UpdatePreferredDownload to PeerManagerImpl

Allows nPreferredDownload to become a member var instead of global.

2aa24ef943

net_processing: move State() into PeerManagerImpl

This allows making mapNodeState not a global. Requires also moving
ProcessBlockAvailability, UpdateBlockAvailability, RelayTransaction,
ProcessGetBlockData, GetFetchFlags, and UpdateLastBlockAnnounceTime
into PeerManagerImpl, as well as removing the const annotation from
ReattemptInitialBroadcast (as they invoke State()).

Also requires moving RelayTransaction and UpdateLastBlockAnnounceTime
into PeerManager.

ea240c7a8b

net_processing: RelayTransaction no longer needs connman passed in 1e00db1979

net_processing: move cs_most_recent_block globals into PeerManagerImpl 8a34a51306

net_processing: annotate local static variables

These function-local variables are effectively globals; annotate them
so it's easier to refactor them later.

2bdb8d53b4

net_processing: move static local variables to PeerManagerImpl members 63f89dee79

net_processing: Provide State(CNode&) shortcut 918a8fc8b1

net_processing: move CNodeState into Peer

Also obsoletes mapNodeState.

7cb410cfd9

Make ProcessBlockAvailability take a CNodeState& instead of a NodeId

Fixes lock order violations introduced by merging CNodeState into Peer

58b7755297

Use Peer::nodestate directly instead of State()

Fixes lock order violations caused by merging CNodeState into Peer

5658c7a0f4

Make NetEventsInterface take a reference to a peer, since nullptr would be k-razy 8d938eda6a

wip update PeerManagerImpl doxygen comments 89da0cee4e

net_processing: don't use gArgs directly d0825abaa7

threadsafety fixup for old code 915da49bfe

ajtowns force-pushed on Mar 21, 2022

ajtowns commented at 2:21 pm on March 21, 2022: contributor

Hmm, in my head, the next set of changes for this PR are in #24474 and #21527, though I guess they’re technically orthogonal. Rebased on top of #21148 merge so there aren’t redundant commits at least.

ajtowns commented at 6:55 am on August 29, 2022: contributor

Closing this. If you’d like it resurrected please help #25174 make progress.

ajtowns closed this on Aug 29, 2022

jonatack commented at 11:10 am on August 29, 2022: contributor

Don’t hesitate to ping me to review net and lock related changes like this.

bitcoin locked this on Aug 29, 2023

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

Conflicts