net processing: Move remaining globals into PeerManagerImpl

dergoegge commented at 4:55 pm on March 12, 2022: member

This PR moves the remaining net processing globals into PeerManagerImpl. This will make testing the peer manager in isolation easier and also acts as a code clean up.

fanquake added the label P2P on Mar 12, 2022

dergoegge force-pushed on Mar 12, 2022

DrahtBot commented at 9:08 pm on March 12, 2022: 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:

#24970 (net processing: Move cleanSubVer, fPreferredDownload and nLocalHostNonce to Peer by jnewbery)
#24931 (Strengthen thread safety assertions by ajtowns)
#24474 (Additional thread safety annotations for CNode/Peer by ajtowns)
#24062 (refactor: replace RecursiveMutex cs_most_recent_block with Mutex (and rename) by theStack)
#24008 (assumeutxo: net_processing changes by jamesob)
#20799 (net processing: Only support version 2 compact blocks 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.

dergoegge force-pushed on Mar 14, 2022

w0xlt commented at 3:50 am on March 16, 2022: contributor

Concept ACK.

in src/net_processing.cpp:334 in 7cba3e8f39 outdated

331@@ -330,6 +332,8 @@ class PeerManagerImpl final : public PeerManager
332     void ProcessMessage(CNode& pfrom, const std::string& msg_type, CDataStream& vRecv,
333                         const std::chrono::microseconds time_received, const std::atomic<bool>& interruptMsgProc) override;
334

jnewbery commented at 4:38 pm on March 17, 2022:

Remove this blank line above UpdateLastBlockAnnounceTime() (since the declaration should be grouped with the other “Implement PeerManager” method declarations)

in src/net_processing.cpp:1206 in 7cba3e8f39 outdated

1203@@ -1178,7 +1204,7 @@ void PeerManagerImpl::AddTxAnnouncement(const CNode& node, const GenTxid& gtxid,
1204 
1205 // This function is used for testing the stale tip eviction logic, see
1206 // denialofservice_tests.cpp

jnewbery commented at 4:40 pm on March 17, 2022:

Move this comment to be a doxygen comment in the header file (function-level comments should be with the function declaration).

in src/net_processing.cpp:300 in 7cba3e8f39 outdated

296@@ -297,6 +297,8 @@ struct Peer {
297 
298 using PeerRef = std::shared_ptr<Peer>;
299 
300+struct CNodeState;

jnewbery commented at 4:50 pm on March 17, 2022:

What do you think about an extra commit that moves the declaration of CNodeState up here, rather than having a forward declaration?

in src/net_processing.cpp:704 in 7cba3e8f39 outdated

702@@ -678,11 +703,6 @@ class PeerManagerImpl final : public PeerManager
703 };
704 } // namespace

jnewbery commented at 4:54 pm on March 17, 2022:

What do you think about also removing:

0} // namespace
1
2namespace {

?

in src/net_processing.cpp:855 in 7cba3e8f39 outdated

851@@ -826,14 +852,14 @@ static void PushAddress(Peer& peer, const CAddress& addr, FastRandomContext& ins
852     }
853 }
854 
855-static void UpdatePreferredDownload(const CNode& node, CNodeState* state) EXCLUSIVE_LOCKS_REQUIRED(cs_main)
856+void PeerManagerImpl::UpdatePreferredDownload(const CNode& node, CNodeState* state) EXCLUSIVE_LOCKS_REQUIRED(cs_main)

jnewbery commented at 4:56 pm on March 17, 2022:

This function is only called in one place and the logic could easily be inlined directly into the version message processing. I don’t think having this extra function adds any value. What do you think?

in src/net_processing.cpp:557 in 7cba3e8f39 outdated

551@@ -535,6 +552,14 @@ class PeerManagerImpl final : public PeerManager
552     std::chrono::microseconds NextInvToInbounds(std::chrono::microseconds now,
553                                                 std::chrono::seconds average_interval);
554 
555+
556+    // All of the following cache a recent block, and are protected by cs_most_recent_block
557+    RecursiveMutex cs_most_recent_block;

jnewbery commented at 4:57 pm on March 17, 2022:

Perhaps rename to m_most_recent_block_mutex in the scripted diff

in src/net_processing.cpp:1754 in 7cba3e8f39 outdated

1765@@ -1747,7 +1766,7 @@ void PeerManagerImpl::RelayTransaction(const uint256& txid, const uint256& wtxid
1766 
1767 void PeerManagerImpl::_RelayTransaction(const uint256& txid, const uint256& wtxid)
1768 {
1769-    m_connman.ForEachNode([&txid, &wtxid](CNode* pnode) EXCLUSIVE_LOCKS_REQUIRED(::cs_main) {

jnewbery commented at 5:03 pm on March 17, 2022:

Note that this call to ForEachNode() is removed in #21160 (not suggesting you do anything differently here).

jnewbery commented at 5:03 pm on March 17, 2022: member

Concept and approach ACK. A few minor suggestions inline.

dergoegge force-pushed on Mar 17, 2022

dergoegge commented at 6:01 pm on March 17, 2022: member

Thanks @jnewbery for the review! I took all your suggestions.

in src/net_processing.cpp:2736 in 8c08e33f67 outdated

2685@@ -2685,8 +2686,12 @@ void PeerManagerImpl::ProcessMessage(CNode& pfrom, const std::string& msg_type,
2686 
2687         // Potentially mark this peer as a preferred download peer.
2688         {
2689-        LOCK(cs_main);
2690-        UpdatePreferredDownload(pfrom, State(pfrom.GetId()));
2691+            LOCK(cs_main);
2692+            CNodeState* state = State(pfrom.GetId());

jnewbery commented at 10:11 am on March 18, 2022:

It may be better to move this code inline in one commit, separate from changing nPreferredDownload from a global to a member variable, so you’re not mixing move-only changes and refactor changes in the same commit.

This code could also be cleaned up in a couple of ways now that it’s inline (but that doesn’t need to happen in this PR):

m_num_preferred_download_peers -= state->fPreferredDownload; can be removed, since version processing can only happen once for each peer, and fPreferredDownload is initialized to false, so this line is always a no-op.
// Whether this node should be marked as a preferred download node. is redundant with the // Potentially mark this peer as a preferred download peer. comment above and can be removed.

dergoegge commented at 11:00 pm on March 18, 2022:

Good idea, I split the inlining and clean ups into a separate commit.

in src/net_processing.cpp:803 in 8c08e33f67 outdated

908-
909-/** Map maintaining per-node state. */
910-static std::map<NodeId, CNodeState> mapNodeState GUARDED_BY(cs_main);
911+CNodeState* PeerManagerImpl::State(NodeId pnode) EXCLUSIVE_LOCKS_REQUIRED(cs_main)
912+{
913+    std::map<NodeId, CNodeState>::iterator it = m_node_states.find(pnode);

jnewbery commented at 10:23 am on March 18, 2022:

To avoid the code duplication here, you can define this in terms of the State() const function, and cast away constness:

 0diff --git a/src/net_processing.cpp b/src/net_processing.cpp
 1index 877bca4c0c..6cadef2a37 100644
 2--- a/src/net_processing.cpp
 3+++ b/src/net_processing.cpp
 4@@ -555,8 +555,10 @@ private:
 5     /** Map maintaining per-node state. */
 6     std::map<NodeId, CNodeState> m_node_states GUARDED_BY(cs_main);
 7 
 8-    CNodeState* State(NodeId pnode) EXCLUSIVE_LOCKS_REQUIRED(cs_main);
 9+    /** Get a pointer to a const CNodeState, used when not mutating the CNodeState object.*/
10     const CNodeState* State(NodeId pnode) const EXCLUSIVE_LOCKS_REQUIRED(cs_main);
11+    /** Get a pointer to a mutable CNodeState.*/
12+    CNodeState* State(NodeId pnode) EXCLUSIVE_LOCKS_REQUIRED(cs_main);
13 
14     uint32_t GetFetchFlags(const CNode& pfrom) const EXCLUSIVE_LOCKS_REQUIRED(cs_main);
15 
16@@ -798,20 +800,17 @@ private:
17     bool SetupAddressRelay(const CNode& node, Peer& peer);
18 };
19 
20-CNodeState* PeerManagerImpl::State(NodeId pnode) EXCLUSIVE_LOCKS_REQUIRED(cs_main)
21+const CNodeState* PeerManagerImpl::State(NodeId pnode) const EXCLUSIVE_LOCKS_REQUIRED(cs_main)
22 {
23-    std::map<NodeId, CNodeState>::iterator it = m_node_states.find(pnode);
24+    std::map<NodeId, CNodeState>::const_iterator it = m_node_states.find(pnode);
25     if (it == m_node_states.end())
26         return nullptr;
27     return &it->second;
28 }
29 
30-const CNodeState* PeerManagerImpl::State(NodeId pnode) const EXCLUSIVE_LOCKS_REQUIRED(cs_main)
31+CNodeState* PeerManagerImpl::State(NodeId pnode) EXCLUSIVE_LOCKS_REQUIRED(cs_main)
32 {
33-    std::map<NodeId, CNodeState>::const_iterator it = m_node_states.find(pnode);
34-    if (it == m_node_states.end())
35-        return nullptr;
36-    return &it->second;
37+    return const_cast<CNodeState*>(std::as_const(*this).State(pnode));
38 }

See https://stackoverflow.com/a/47369227/933705. Generally const_cast is discouraged, but this is one instance where it’s considered acceptable.

jnewbery commented at 10:25 am on March 18, 2022: member

Code review ACK 8c08e33f678bd10254fc4ca1ed6f47ad52b027a5.

A couple of small suggestions inline.

dergoegge force-pushed on Mar 18, 2022

in src/test/denialofservice_tests.cpp:198 in 822152c506 outdated

196@@ -197,7 +197,7 @@ BOOST_AUTO_TEST_CASE(stale_tip_peer_management)
197 
198     // Update the last announced block time for the last
199     // peer, and check that the next newest node gets evicted.
200-    UpdateLastBlockAnnounceTime(vNodes.back()->GetId(), GetTime());
201+    peerLogic->UpdateLastBlockAnnounceTime(vNodes.back()->GetId(), GetTime());

jnewbery commented at 9:35 am on March 21, 2022:

You can remove the UpdateLastBlockAnnounceTime() declaration on line 39 (it’s now no longer used).

dergoegge commented at 3:38 pm on March 21, 2022:

Done, good catch!

in src/net_processing.cpp:2736 in 822152c506 outdated

2684@@ -2685,8 +2685,10 @@ void PeerManagerImpl::ProcessMessage(CNode& pfrom, const std::string& msg_type,
2685 
2686         // Potentially mark this peer as a preferred download peer.
2687         {
2688-        LOCK(cs_main);
2689-        UpdatePreferredDownload(pfrom, State(pfrom.GetId()));
2690+            LOCK(cs_main);
2691+            CNodeState* state = State(pfrom.GetId());

jnewbery commented at 9:38 am on March 21, 2022:

I think the commit [net processing] Inline and simplify UpdatePreferredDownload could have slightly more explanation in the commit log (explaining that this logic can only be hit once per peer, so fPreferredDownload will always be false, and the m_num_preferred_download_peers -= state->fPreferredDownload; line is always a no-op).

jnewbery commented at 9:39 am on March 21, 2022: member

Code review ACK 822152c50654228a926ec56819878a5429d75c2e

dergoegge force-pushed on Mar 21, 2022

jnewbery commented at 4:13 pm on March 21, 2022: member

Code review ACK 41b08b114f7494aa452dd5c15c10242651a9d12d

Thank you for such quick responses to my review comments!

DrahtBot added the label Needs rebase on Mar 22, 2022

dergoegge force-pushed on Mar 22, 2022

dergoegge commented at 9:28 am on March 22, 2022: member

Rebased

jnewbery commented at 11:12 am on March 22, 2022: member

ACK 45f3b1dbebf799b386907de1a4eed20777e92073

Verified rebase by doing it myself and comparing.

DrahtBot removed the label Needs rebase on Mar 22, 2022

DrahtBot added the label Needs rebase on Mar 25, 2022

dergoegge force-pushed on Mar 25, 2022

dergoegge commented at 5:52 pm on March 25, 2022: member

Rebased

jnewbery commented at 6:03 pm on March 25, 2022: member

Code review ACK 79f6a554354fa923167979faf8cf8f3120fc9bfa

I noticed the following line in PeerManagerImpl::NewPOWValidBlock():

0    static int nHighestFastAnnounce = 0;

It’s perhaps out of scope for this PR, but perhaps that should be made a member variable of PeerManagerImpl alongside the cached recent block members.

DrahtBot removed the label Needs rebase on Mar 25, 2022

MarcoFalke added the label Refactoring on Mar 29, 2022

DrahtBot added the label Needs rebase on Apr 20, 2022

[net processing] Move CNodeState declaration above PeerManagerImpl 37ecaf3e7a

[net processing] Move mapNodeState into PeerManagerImpl a292df283a

[net processing] Move nPreferredDownload into PeerManagerImpl 490c08f96a

[net processing] Inline and simplify UpdatePreferredDownload

We inline `UpdatePreferredDownload` because it is only used in one
location during the version handshake. We simplify it by removing the
initial subtraction of `state->fPreferredDownload` from
`nPreferredDownload`. This is ok since the version handshake is only
called once per peer and `state->fPreferredDownload` will always be
false before the newly inlined code is called, making the subtraction a
noop.

a4c55a93ef

[net processing] Move block cache state into PeerManagerImpl 10b83e2aa3

dergoegge force-pushed on Apr 20, 2022

dergoegge commented at 2:11 pm on April 20, 2022: member

Rebased and added a commit (da9e51a1fe704d4a6a93de34e6837fab37655860) to move nHighestFastAnnounce into PeerManagerImpl.

DrahtBot removed the label Needs rebase on Apr 20, 2022

in src/net_processing.cpp:691 in 8fa7fa0cb0 outdated

686+    std::shared_ptr<const CBlockHeaderAndShortTxIDs> m_most_recent_compact_block GUARDED_BY(m_most_recent_block_mutex);
687+    uint256 m_most_recent_block_hash GUARDED_BY(m_most_recent_block_mutex);
688+    bool m_most_recent_compact_block_has_witnesses GUARDED_BY(m_most_recent_block_mutex){false};
689+
690+    /**
691+     * Height of the highest fast announced block.

jnewbery commented at 1:19 pm on April 25, 2022:

This comment could be more explicit in referring to “block announced using BIP 152 high-bandwidth mode” or similar. I don’t think “fast announced blocks” is commonly understood terminology.

dergoegge commented at 9:21 am on April 26, 2022:

Good point, adjusted the comment

jnewbery commented at 1:19 pm on April 25, 2022: member

Code review ACK 8fa7fa0cb04661c06fc1fd44667bfebcc6716a9d

[net processing] Move nHighestFastAnnounce into PeerManagerImpl 91c339243e

scripted-diff: Rename PeerManagerImpl members

-BEGIN VERIFY SCRIPT-
ren() { sed -i "s:\<$1\>:$2:g" $(git grep -l "\<$1\>" ./src ./test); }

ren mapNodeState                              m_node_states
ren cs_most_recent_block                      m_most_recent_block_mutex
ren most_recent_block                         m_most_recent_block
ren most_recent_compact_block                 m_most_recent_compact_block
ren most_recent_block_hash                    m_most_recent_block_hash
ren fWitnessesPresentInMostRecentCompactBlock m_most_recent_compact_block_has_witnesses
ren nPreferredDownload                        m_num_preferred_download_peers
ren nHighestFastAnnounce                       m_highest_fast_announce
-END VERIFY SCRIPT-

778343a379

dergoegge force-pushed on Apr 26, 2022

jnewbery commented at 9:58 am on April 26, 2022: member

Code review ACK 778343a379026ef233dffea67f5226565f6d5720

MarcoFalke approved

MarcoFalke commented at 9:51 am on April 30, 2022: member

ACK 778343a379026ef233dffea67f5226565f6d5720 🗒

Signature:

 0-----BEGIN PGP SIGNED MESSAGE-----
 1Hash: SHA512
 2
 3ACK 778343a379026ef233dffea67f5226565f6d5720 🗒
 4-----BEGIN PGP SIGNATURE-----
 5
 6iQGzBAEBCgAdFiEE+rVPoUahrI9sLGYTzit1aX5ppUgFAlwqrYAACgkQzit1aX5p
 7pUjBIwv+MjgYz0poUymP0rv825e0CNNMrgOXg5hJmGXAF7dDpzn6Kqje2icEX1Iy
 8vNbpPsRDachm/67cOdTJinNgw8TStVI+KxURKCSCl1A30Xz01groLbTyXjVn2gwy
 9Bk9HoIRpa0CLbvGenk2OW6jjxYFgbK81TKDTHqHEM6laayMlKHFaloO/AlB+SfYL
10TRNCBjxGnzoBzPfG3d5JMI5Rt15lBSvKRm6dCe5pnUdDLJyfJLVT5CIb606Tkxoz
110ASF4H0nvsdPCR5t3T+spy8fFDgmsUwtUNbpOAKKkm3Vmr1EGxHxyGCW3Uyq3eLQ
12YKefQrFjp21dcxeCeCcsZp30Ropssp1Dc1bOBgPIworvUzTlb8H/WU1aDr16Eis+
13/GITCLQ75eifYYaPPEPnJxLwW8QLE4k38zhmYKtR8EafRpB8Zh1NAt+TSWB8iSOl
14yjRfMQGD7TeIDVnXzOz+fEwzI7JVsexqhu1BD14Xfx3pUsXfclbnFWHH2/0XaTwp
1559A5jsEa
16=56fQ
17-----END PGP SIGNATURE-----

MarcoFalke merged this on Apr 30, 2022

MarcoFalke closed this on Apr 30, 2022

sidhujag referenced this in commit 064218a5c4 on Apr 30, 2022

MarcoFalke deleted a comment on May 1, 2022

DrahtBot locked this on May 1, 2023

net processing: Move remaining globals into PeerManagerImpl #24543

Conflicts