refactor: move net_processing implementation details out of header #20811

ajtowns commented at 6:50 AM on December 31, 2020: member

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

ajtowns commented at 6:51 AM on December 31, 2020: member

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.

ajtowns force-pushed on Dec 31, 2020

DrahtBot added the label P2P on Dec 31, 2020

DrahtBot added the label Refactoring on Dec 31, 2020

DrahtBot commented at 8:17 AM on December 31, 2020: 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:

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

ajtowns force-pushed on Dec 31, 2020

jnewbery commented at 1:00 AM on January 1, 2021: member

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 https://github.com/google/sanitizers/issues/1259?

in src/net_processing.cpp:1482 in 13a7fec39d outdated

1478 | @@ -1295,7 +1479,7 @@ void PeerManager::NewPoWValidBlock(const CBlockIndex *pindex, const std::shared_
1479 |   * Update our best height and announce any block hashes which weren't previously
1480 |   * in ::ChainActive() to our peers.
1481 |   */
1482 | -void PeerManager::UpdatedBlockTip(const CBlockIndex *pindexNew, const CBlockIndex *pindexFork, bool fInitialDownload) {
1483 | +void PeerManagerImpl::UpdatedBlockTip(const CBlockIndex *pindexNew, const CBlockIndex *pindexFork, bool fInitialDownload) {

MarcoFalke commented at 11:15 AM on January 1, 2021:

When renaming this, you'll probably also have to rename mutex:PeerManager::UpdatedBlockTip in ./test/sanitizer_suppressions/tsan

in src/net_processing.h:66 in 13a7fec39d outdated

 252 | -     * their own locks.
 253 | -     */
 254 | -    std::map<NodeId, PeerRef> m_peer_map GUARDED_BY(m_peer_mutex);
 255 | +    /** Process a single message from a peer. Public for fuzz testing */
 256 | +    virtual void ProcessMessage(CNode& pfrom, const std::string& msg_type, CDataStream& vRecv,
 257 | +                        const std::chrono::microseconds time_received, const std::atomic<bool>& interruptMsgProc) = 0;

jnewbery commented at 11:18 AM on January 1, 2021:

Perhaps align with opening parens?

in src/net_processing.cpp:1221 in 13a7fec39d outdated

1210 | @@ -1034,7 +1211,7 @@ void PeerManager::Misbehaving(const NodeId pnode, const int howmuch, const std::
1211 |      }
1212 |  }
1213 |  
1214 | -bool PeerManager::MaybePunishNodeForBlock(NodeId nodeid, const BlockValidationState& state,
1215 | +bool PeerManagerImpl::MaybePunishNodeForBlock(NodeId nodeid, const BlockValidationState& state,
1216 |                                            bool via_compact_block, const std::string& message)

jnewbery commented at 11:24 AM on January 1, 2021:

Keep alignment with parens

jnewbery commented at 10:36 AM on January 3, 2021:

not resolved

ajtowns commented at 1:29 PM on January 3, 2021:

dammit

in src/net_processing.cpp:1316 in 13a7fec39d outdated

1306 | @@ -1130,9 +1307,16 @@ static bool BlockRequestAllowed(const CBlockIndex* pindex, const Consensus::Para
1307 |          (GetBlockProofEquivalentTime(*pindexBestHeader, *pindex, *pindexBestHeader, consensusParams) < STALE_RELAY_AGE_LIMIT);
1308 |  }
1309 |  
1310 | -PeerManager::PeerManager(const CChainParams& chainparams, CConnman& connman, BanMan* banman,
1311 | -                         CScheduler& scheduler, ChainstateManager& chainman, CTxMemPool& pool,
1312 | -                         bool ignore_incoming_txs)
1313 | +std::unique_ptr<PeerManager> PeerManager::make(const CChainParams& chainparams, CConnman& connman, BanMan* banman,

jnewbery commented at 11:33 AM on January 1, 2021:

Adding a new parameter to the ctor now requires updating five locations (PeerManager::make declaration, PeerManager::make definition signature, PeerManager::make function body PeerManagerImplctor declaration,PeerManagerImpl` ctor definition signature.

I wonder if it makes sense to create a PeerManagerArgs struct that's passed into the make() method and forwarded to the PeerManagerImpl ctor so that only one place needs to be changed?

ajtowns commented at 2:12 AM on January 3, 2021:

Also requires touching everywhere that calls the ctor. Not against it, but doesn't seem like a big win.

jnewbery commented at 10:24 AM on January 3, 2021:

Not necessarily if we're passing in arguments with defaults. Anyway, not important here. Can be considered separately.

ajtowns commented at 3:09 AM on January 4, 2021:

BTW, as part of turning (almost) all of net_processing.cpp into PeerManagerImpl methods, we could plausibly remove all the PeerManagerImpl declarations by just having net_processing.h start with namespace { class PeerManagerImpl : public PeerManager { and end with the closing brackets and the PeerManager::make definition -- there's no need to declare class methods before use. That'd remove one of the redundancies above, as well as a bunch of others.

jnewbery commented at 10:01 AM on January 4, 2021:

Seems reasonable, although like you say earlier, it's not a big win. Perhaps something for a follow-up once more of #20758 has landed.

in src/net_processing.cpp:2455 in 13a7fec39d outdated

2450 | @@ -2267,7 +2451,7 @@ static void ProcessGetCFCheckPt(CNode& peer, CDataStream& vRecv, const CChainPar
2451 |      connman.PushMessage(&peer, std::move(msg));
2452 |  }
2453 |  
2454 | -void PeerManager::ProcessMessage(CNode& pfrom, const std::string& msg_type, CDataStream& vRecv,
2455 | +void PeerManagerImpl::ProcessMessage(CNode& pfrom, const std::string& msg_type, CDataStream& vRecv,
2456 |                                           const std::chrono::microseconds time_received,

jnewbery commented at 11:33 AM on January 1, 2021:

Feel free to fix alignment while you're touching this function :)

in src/net_processing.h:18 in 13a7fec39d outdated

  19 |  class CTxMemPool;
  20 |  class ChainstateManager;
  21 | -class TxValidationState;
  22 |  
  23 |  extern RecursiveMutex cs_main;
  24 |  extern RecursiveMutex g_cs_orphans;

jnewbery commented at 11:58 AM on January 1, 2021:

This only needs to be exposed because of the weird lock ordering requirements documented here: https://github.com/bitcoin/bitcoin/blob/e75f91eae3936269b40b4bfdfe540d5526270936/src/init.cpp#L202-L208. Once tx data is moved into Peer and RelayTransactions() no longer needs to call ForEachNode() and therefore take cs_vNodes while holding g_cs_orphans, this mutex can be private in net_processing.

ajtowns commented at 2:11 AM on January 3, 2021:

I don't think that's relevant for this PR though?

jnewbery commented at 10:24 AM on January 3, 2021:

Correct, this was just an observation about why is required here now.

jnewbery commented at 11:58 AM on January 1, 2021: member

utACK 13a7fec39d7e30becd7bf6eb57709f471d8ba315 assuming the tsan suppression is fixed.

ajtowns force-pushed on Jan 1, 2021

jnewbery commented at 9:49 AM on January 2, 2021: member

utACK fa6b22736656b207251fabb068d9a3ddd91d1266

DrahtBot added the label Needs rebase on Jan 2, 2021

ajtowns force-pushed on Jan 3, 2021

DrahtBot removed the label Needs rebase on Jan 3, 2021

jnewbery commented at 10:37 AM on January 3, 2021: member

utACK 7bd4517e5830a09a173d82fe3184acf0bcea06a9

Sjors commented at 12:44 PM on January 3, 2021: member

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

ajtowns force-pushed on Jan 3, 2021

jnewbery commented at 1:30 PM on January 3, 2021: member

utACK abeecaa66ed136b3f0b7ff500ae5b592256072a5

Thanks for being so responsive to review comments!

ajtowns commented at 1:31 PM on January 3, 2021: member

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.

DrahtBot added the label Needs rebase on Jan 5, 2021

ajtowns commented at 1:48 AM on January 6, 2021: member

Rebased for conflict with #20842

ajtowns force-pushed on Jan 6, 2021

DrahtBot removed the label Needs rebase on Jan 6, 2021

jnewbery commented at 9:42 AM on January 6, 2021: member

utACK 916cfe29656a11592f9eff31219353ff1ac9d564

in src/net_processing.h:126 in d7c43797cc outdated

 123 | +    /** Implement NetEventsInterface */
 124 |      void InitializeNode(CNode* pnode) override;
 125 | -    /** Handle removal of a peer by updating various state and removing it from mapNodeState */
 126 |      void FinalizeNode(const CNode& node, bool& fUpdateConnectionTime) override;
 127 | -    /**
 128 | -    * Process protocol messages received from a given node

Sjors commented at 12:13 PM on January 6, 2021:

Are you sure you want to lose all this documentation?

ajtowns commented at 2:54 AM on January 7, 2021:

Added a commit to move them into NetEventsInterface; that makes more of a conflict with #20864 so probably a good idea to get that reviewed and merged first.

ajtowns force-pushed on Jan 7, 2021

in src/net.h:620 in 02f67760d9 outdated

 616 | @@ -617,11 +617,27 @@ uint16_t GetListenPort();
 617 |  class NetEventsInterface
 618 |  {
 619 |  public:
 620 | -    virtual bool ProcessMessages(CNode* pnode, std::atomic<bool>& interrupt) = 0;
 621 | -    virtual bool SendMessages(CNode* pnode) = 0;
 622 | +    /** Initialize a peer by adding it to mapNodeState and pushing a message requesting its version */

jnewbery commented at 9:24 AM on January 7, 2021:

This comment about the implementation (mapNodeState) doesn't make sense in the interface class. Same for FinalizeNode() below.

ajtowns commented at 6:51 AM on January 9, 2021:

Grr, edited twice, but rebased three times and lost them, I guess. Rebased and edited again.

jnewbery commented at 9:35 AM on January 7, 2021: member

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

utACK 02f67760d9014b4c963c3c8a57ed5d56de98205d.

DrahtBot added the label Needs rebase on Jan 7, 2021

richardsonmark316 approved

ajtowns force-pushed on Jan 9, 2021

DrahtBot removed the label Needs rebase on Jan 9, 2021

in src/net.h:772 in 3bcde8b847 outdated

 768 | @@ -769,11 +769,28 @@ 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) */

jnewbery commented at 12:39 PM on January 9, 2021:

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

ajtowns commented at 1:29 PM on January 9, 2021:

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

jnewbery commented at 12:39 PM on January 9, 2021: member

utACK 3bcde8b84722be58c68309b7076aba238cf4c305

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

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

net_processing: make more of PeerManager private 0df3d3fd6b

net_processing: split PeerManager into interface and implementation classes a568b82feb

net_processing: move PeerManagerImpl into cpp file e0f2e6d2df

net_processing: move Peer definition to .cpp c97f70c861

ajtowns force-pushed on Jan 9, 2021

jnewbery commented at 1:51 PM on January 9, 2021: member

ACK c97f70c861ac6959b8116a9bca3031edeb2b2aaa

Sjors commented at 11:50 AM on January 11, 2021: member

ACK c97f70c861ac6959b8116a9bca3031edeb2b2aaa

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.

in src/net_processing.h:36 in c97f70c861

  87 | -};
  88 | -
  89 | -using PeerRef = std::shared_ptr<Peer>;
  90 | -
  91 | -class PeerManager final : public CValidationInterface, public NetEventsInterface {
  92 | +class PeerManager : public CValidationInterface, public NetEventsInterface

vasild commented at 8:59 AM on January 12, 2021:

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

ajtowns commented at 11:54 AM on January 12, 2021:

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.

in src/net_processing.cpp:1321 in c97f70c861

1319 | +std::unique_ptr<PeerManager> PeerManager::make(const CChainParams& chainparams, CConnman& connman, BanMan* banman,
1320 | +                                               CScheduler& scheduler, ChainstateManager& chainman, CTxMemPool& pool,
1321 | +                                               bool ignore_incoming_txs)
1322 | +{
1323 | +    return std::make_unique<PeerManagerImpl>(chainparams, connman, banman, scheduler, chainman, pool, ignore_incoming_txs);
1324 | +}

vasild commented at 9:06 AM on January 12, 2021:

What is the rationale behind this method? It is not like callers can choose which implementation to use. Why not use std::make_unique<PeerManagerImpl>(...) in the callers and drop this method?

jnewbery commented at 10:02 AM on January 12, 2021:

PeerManagerImpl is only declared in the cpp file, so is not available to the callers.

vasild commented at 10:28 AM on January 12, 2021:

Right.

in src/net_processing.cpp:351 in c97f70c861

 346 | +     */
 347 | +    std::map<NodeId, PeerRef> m_peer_map GUARDED_BY(m_peer_mutex);
 348 | +};
 349 | +} // namespace
 350 | +
 351 |  namespace {

vasild commented at 9:11 AM on January 12, 2021:

nit:

jnewbery commented at 10:17 AM on January 12, 2021:

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.

ajtowns commented at 11:57 AM on January 12, 2021:

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.

vasild approved

vasild commented at 9:30 AM on January 12, 2021: member

ACK c97f70c861ac6959b8116a9bca3031edeb2b2aaa

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

ariard commented at 2:47 AM on January 13, 2021: member

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

laanwj commented at 8:47 AM on January 13, 2021: member

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

Code review ACK c97f70c861ac6959b8116a9bca3031edeb2b2aaa

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 😄

laanwj merged this on Jan 13, 2021

laanwj closed this on Jan 13, 2021

sidhujag referenced this in commit 375df7582c on Jan 13, 2021

in src/net_processing.h:152 in a568b82feb outdated

 169 | +    void InitializeNode(CNode* pnode) override;
 170 | +    void FinalizeNode(const CNode& node, bool& fUpdateConnectionTime) override;
 171 | +    bool ProcessMessages(CNode* pfrom, std::atomic<bool>& interrupt) override;
 172 | +    bool SendMessages(CNode* pto) override EXCLUSIVE_LOCKS_REQUIRED(pto->cs_sendProcessing);
 173 | +
 174 | +    /** Implement PeerManager */

ariard commented at 11:13 PM on January 14, 2021:

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

in src/net_processing.cpp:222 in e0f2e6d2df outdated

 217 | +    PeerRef RemovePeer(NodeId id);
 218 | +
 219 | +    /**
 220 | +     * Potentially mark a node discouraged based on the contents of a BlockValidationState object
 221 | +     *
 222 | +     * @param[in] via_compact_block this bool is passed in because net_processing should

ariard commented at 11:24 PM on January 14, 2021:

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.

in src/net_processing.cpp:255 in e0f2e6d2df outdated

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

ariard commented at 11:34 PM on January 14, 2021:

Should say PeerManagerImpl now ?

in src/net_processing.h:134 in 0df3d3fd6b outdated

 134 | -
 135 | -    /** Set the best height */
 136 | -    void SetBestHeight(int height) { m_best_height = height; };
 137 | +    /**
 138 | +     * Evict extra outbound peers. If we think our tip may be stale, connect to an extra outbound.
 139 | +     * Public for unit testing.

ariard commented at 12:15 AM on January 15, 2021:

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" :) ?

ariard commented at 12:27 AM on January 15, 2021: member

Code Review ACK c97f70c

Good code restructuring direction, only minors.

Fabcien referenced this in commit a5142b422d on Jan 25, 2022

Fabcien referenced this in commit 6002783def on Jan 25, 2022

Fabcien referenced this in commit bb213e6dee on Jan 25, 2022

Fabcien referenced this in commit 418a358112 on Jan 25, 2022

Fabcien referenced this in commit c85369a06e on Jan 25, 2022

DrahtBot locked this on Aug 16, 2022