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

ajtowns · 2020-12-23T21:25:17Z

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 · 2020-12-23T21:37:16Z

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 · 2020-12-23T21:37:50Z

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

Conflicts

Reviewers, this pull request conflicts with the following ones:

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

jnewbery · 2020-12-23T22:49:28Z

src/net_processing.cpp

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


You're the second person who's been confused by this comment (#19829 (comment)), 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.

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 · 2020-12-23T22:54:45Z

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.

jnewbery · 2020-12-23T23:43:08Z

src/net_processing.cpp

+};
+
+namespace {
+class PeerManagerImpl final : public PeerManager {


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.

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

jnewbery

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

→ git --no-pager d
diff --git a/src/net_processing.cpp b/src/net_processing.cpp
index ca016ec52b..1c7638a774 100644
--- a/src/net_processing.cpp
+++ b/src/net_processing.cpp
@@ -26,6 +26,7 @@
 #include <streams.h>
 #include <tinyformat.h>
 #include <txmempool.h>
+#include <txrequest.h>
 #include <util/check.h> // For NDEBUG compile time check
 #include <util/strencodings.h>
 #include <util/system.h>
@@ -290,6 +291,20 @@ struct CNodeState {
 };
 } // namespace
 
+/**
+ * Data structure for an individual peer. This struct is not protected by
+ * cs_main since it does not contain validation-critical data.
+ *
+ * Memory is owned by shared pointers and this object is destructed when
+ * the refcount drops to zero.
+ *
+ * Mutexes inside this struct must not be held when locking m_peer_mutex.
+ *
+ * Details are all local to net_processing.cpp
+ *
+ * TODO: move most members from CNodeState to this structure.
+ * TODO: move remaining application-layer data members from CNode to this structure.
+ */
 struct Peer {
     /** Same id as the CNode object for this peer */
     const NodeId m_id{0};
@@ -334,6 +349,8 @@ struct Peer {
     explicit Peer(NodeId id, CAddress addr, bool is_inbound) : m_id(id),  nodestate(addr, is_inbound) {}
 };
 
+using PeerRef = std::shared_ptr<Peer>;
+
 namespace {
 class PeerManagerImpl final : public PeerManager {
 public:
diff --git a/src/net_processing.h b/src/net_processing.h
index ee34a38178..74eabd6376 100644
--- a/src/net_processing.h
+++ b/src/net_processing.h
@@ -6,19 +6,12 @@
 #ifndef BITCOIN_NET_PROCESSING_H
 #define BITCOIN_NET_PROCESSING_H
 
-#include <consensus/params.h>
 #include <net.h>
-#include <sync.h>
-#include <txrequest.h>
 #include <validationinterface.h>
 
-class BlockTransactionsRequest;
-class BlockValidationState;
-class CBlockHeader;
 class CChainParams;
 class CTxMemPool;
 class ChainstateManager;
-class TxValidationState;
 
 extern RecursiveMutex cs_main;
 extern RecursiveMutex g_cs_orphans;
@@ -40,24 +33,6 @@ struct CNodeStateStats {
     std::vector<int> vHeightInFlight;
 };
 
-/**
- * Data structure for an individual peer. This struct is not protected by
- * cs_main since it does not contain validation-critical data.
- *
- * Memory is owned by shared pointers and this object is destructed when
- * the refcount drops to zero.
- *
- * Mutexes inside this struct must not be held when locking m_peer_mutex.
- *
- * Details are all local to net_processing.cpp
- *
- * TODO: move most members from CNodeState to this structure.
- * TODO: move remaining application-layer data members from CNode to this structure.
- */
-struct Peer; // body is defined in net_processing.cpp
-
-using PeerRef = std::shared_ptr<Peer>;
-
 class PeerManager : public CValidationInterface, public NetEventsInterface {
 public:
     /** Evict extra outbound peers. If we think our tip may be stale, connect to an extra outbound */

src/net_processing.h

jnewbery · 2020-12-24T11:39:50Z

src/net_processing.h

+    struct COrphanTx {
+        CTransactionRef tx;
+        NodeId fromPeer;
+        int64_t nTimeExpire;
+        size_t list_pos;
+    };
+
+    virtual bool AddOrphanTx(const CTransactionRef& tx, NodeId peer) EXCLUSIVE_LOCKS_REQUIRED(g_cs_orphans) = 0;
+    virtual void EraseOrphansFor(NodeId peer) EXCLUSIVE_LOCKS_REQUIRED(g_cs_orphans) = 0;
+    virtual unsigned int LimitOrphanTxSize(unsigned int nMaxOrphans) = 0;
+    virtual void UpdateLastBlockAnnounceTime(NodeId node, int64_t time_in_seconds) = 0;
+
+    /** Map from txid to orphan transaction record. Limited by
+     *  -maxorphantx/DEFAULT_MAX_ORPHAN_TRANSACTIONS */
+    std::map<uint256, COrphanTx> mapOrphanTransactions GUARDED_BY(g_cs_orphans);


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

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 · 2020-12-26T14:01:01Z

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 · 2020-12-28T00:33:44Z

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 · 2020-12-29T16:13:20Z

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 · 2020-12-30T04:54:58Z

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

DrahtBot · 2021-12-13T23:18:56Z

🐙 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 · 2022-03-21T13:06:56Z

There hasn't been much activity lately and the patch still needs rebase. What is the status here?

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.

Allows nPreferredDownload to become a member var instead of global.

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.

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

Also obsoletes mapNodeState.

Fixes lock order violations introduced by merging CNodeState into Peer

Fixes lock order violations caused by merging CNodeState into Peer

…ld be k-razy

ajtowns · 2022-03-21T14:21:01Z

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 · 2022-08-29T06:55:12Z

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

jonatack · 2022-08-29T11:10:46Z

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

DrahtBot added the P2P label Dec 23, 2020

DrahtBot mentioned this pull request Dec 23, 2020

p2p: standardize outbound full/block relay connection type naming #20729

Closed

This was referenced Dec 23, 2020

Net: Move ping data to net_processing #20721

Merged

Move remaining application layer data to net processing #19398

Closed

This was referenced Dec 23, 2020

p2p: improve logging in EvictExtraOutboundPeers() #20723

Closed

refactor: Remove nMyStartingHeight from CNode/Connman #20649

Merged

jnewbery reviewed Dec 23, 2020

View reviewed changes

DrahtBot mentioned this pull request Dec 23, 2020

rpc: getblockfrompeer #20295

Merged

DrahtBot mentioned this pull request Dec 23, 2020

addrman: Make addrman a top-level component #20228

Merged

jnewbery reviewed Dec 23, 2020

View reviewed changes

DrahtBot mentioned this pull request Dec 23, 2020

Introduce deploymentstatus #19438

Merged

Limpisey168 approved these changes Dec 24, 2020

View reviewed changes

jnewbery reviewed Dec 24, 2020

View reviewed changes

DrahtBot mentioned this pull request Dec 25, 2020

Erlay: bandwidth-efficient transaction relay protocol #18261

Closed

DrahtBot mentioned this pull request Dec 27, 2020

doc: Fix some typos #20762

Closed

DrahtBot mentioned this pull request Dec 29, 2020

net processing: Only support version 2 compact blocks #20799

Merged

ajtowns mentioned this pull request Dec 31, 2020

refactor: move net_processing implementation details out of header #20811

Merged

ajtowns force-pushed the 202012-netproc-refactor branch 6 times, most recently from fc83bf0 to 2e57317 Compare January 4, 2021 08:33

ajtowns force-pushed the 202012-netproc-refactor branch from 4f9931d to 3f90a18 Compare February 16, 2021 02:42

This was referenced Feb 16, 2021

Default to NODE_WITNESS in nLocalServices #21090

Merged

[p2p] Introduce node rebroadcast module #21061

Closed

ajtowns mentioned this pull request Feb 17, 2021

Split orphan handling from net_processing into txorphanage #21148

Merged

jnewbery mentioned this pull request Mar 4, 2021

net processing: Extract addr send functionality into MaybeSendAddr() #21236

Merged

jnewbery mentioned this pull request Apr 10, 2021

net/net processing: Move addr data into net_processing #21186

Merged

DrahtBot added the Needs rebase label Dec 13, 2021

ajtowns added 15 commits March 21, 2022 23:52

net_processing: move CNodeState definition earlier

b3978c4

net_processing: Move UpdatePreferredDownload to PeerManagerImpl

2aa24ef

Allows nPreferredDownload to become a member var instead of global.

net_processing: RelayTransaction no longer needs connman passed in

1e00db1

net_processing: move cs_most_recent_block globals into PeerManagerImpl

8a34a51

net_processing: annotate local static variables

2bdb8d5

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

net_processing: move static local variables to PeerManagerImpl members

63f89de

net_processing: Provide State(CNode&) shortcut

918a8fc

net_processing: move CNodeState into Peer

7cb410c

Also obsoletes mapNodeState.

Make ProcessBlockAvailability take a CNodeState& instead of a NodeId

58b7755

Fixes lock order violations introduced by merging CNodeState into Peer

Use Peer::nodestate directly instead of State()

5658c7a

Fixes lock order violations caused by merging CNodeState into Peer

Make NetEventsInterface take a reference to a peer, since nullptr wou…

8d938ed

…ld be k-razy

wip update PeerManagerImpl doxygen comments

89da0ce

net_processing: don't use gArgs directly

d0825ab

threadsafety fixup for old code

915da49

ajtowns force-pushed the 202012-netproc-refactor branch from 3f90a18 to 915da49 Compare March 21, 2022 14:18

ajtowns closed this Aug 29, 2022

bitcoin locked and limited conversation to collaborators Aug 29, 2023

Provide feedback

Saved searches

Use saved searches to filter your results more quickly

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

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

ajtowns commented Dec 23, 2020 •

edited

Loading

ajtowns commented Dec 23, 2020

DrahtBot commented Dec 23, 2020 •

edited

Loading

jnewbery Dec 23, 2020

ajtowns Dec 30, 2020

jnewbery commented Dec 23, 2020

jnewbery Dec 23, 2020

ajtowns Dec 24, 2020

jnewbery left a comment

jnewbery Dec 24, 2020

ajtowns Dec 30, 2020

jnewbery commented Dec 26, 2020

ajtowns commented Dec 28, 2020

jnewbery commented Dec 29, 2020

ajtowns commented Dec 30, 2020

DrahtBot commented Dec 13, 2021

DrahtBot commented Mar 21, 2022

ajtowns commented Mar 21, 2022

ajtowns commented Aug 29, 2022

jonatack commented Aug 29, 2022

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

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

Conversation

ajtowns commented Dec 23, 2020 • edited Loading

ajtowns commented Dec 23, 2020

DrahtBot commented Dec 23, 2020 • edited Loading

Conflicts

jnewbery Dec 23, 2020

Choose a reason for hiding this comment

ajtowns Dec 30, 2020

Choose a reason for hiding this comment

jnewbery commented Dec 23, 2020

jnewbery Dec 23, 2020

Choose a reason for hiding this comment

ajtowns Dec 24, 2020

Choose a reason for hiding this comment

jnewbery left a comment

Choose a reason for hiding this comment

jnewbery Dec 24, 2020

Choose a reason for hiding this comment

ajtowns Dec 30, 2020

Choose a reason for hiding this comment

jnewbery commented Dec 26, 2020

ajtowns commented Dec 28, 2020

jnewbery commented Dec 29, 2020

ajtowns commented Dec 30, 2020

DrahtBot commented Dec 13, 2021

DrahtBot commented Mar 21, 2022

ajtowns commented Mar 21, 2022

ajtowns commented Aug 29, 2022

jonatack commented Aug 29, 2022

ajtowns commented Dec 23, 2020 •

edited

Loading

DrahtBot commented Dec 23, 2020 •

edited

Loading