Net processing: move ProcessMessage() to PeerLogicValidation #19704

in commit eb170099f3ea91555b56fdaaae89a64fe04b93be: It seems a bit confusing to pass in chainparams to be stored as a reference in a member variable, but then pass in the consensus params via a parameter of a function call, even though they can be "derived" from the chain params.

I'd suggest the following patch:

diff --git a/src/net_processing.cpp b/src/net_processing.cpp
index 71b235f56b..4a5c7acc46 100644
--- a/src/net_processing.cpp
+++ b/src/net_processing.cpp
@@ -9,6 +9,7 @@
 #include <banman.h>
 #include <blockencodings.h>
 #include <blockfilter.h>
+#include <chainparams.h>
 #include <consensus/validation.h>
 #include <hash.h>
 #include <index/blockfilterindex.h>
@@ -1219,13 +1220,12 @@ PeerLogicValidation::PeerLogicValidation(const CChainParams& chainparams, CConnm
     // same probability that we have in the reject filter).
     g_recent_confirmed_transactions.reset(new CRollingBloomFilter(48000, 0.000001));
 
-    const Consensus::Params& consensusParams = Params().GetConsensus();
     // Stale tip checking and peer eviction are on two different timers, but we
     // don't want them to get out of sync due to drift in the scheduler, so we
     // combine them in one function and schedule at the quicker (peer-eviction)
     // timer.
     static_assert(EXTRA_PEER_CHECK_INTERVAL < STALE_CHECK_INTERVAL, "peer eviction timer should be less than stale tip check timer");
-    scheduler.scheduleEvery([this, consensusParams] { this->CheckForStaleTipAndEvictPeers(consensusParams); }, std::chrono::seconds{EXTRA_PEER_CHECK_INTERVAL});
+    scheduler.scheduleEvery([this] { this->CheckForStaleTipAndEvictPeers(); }, std::chrono::seconds{EXTRA_PEER_CHECK_INTERVAL});
 
     // schedule next run for 10-15 minutes in the future
     const std::chrono::milliseconds delta = std::chrono::minutes{10} + GetRandMillis(std::chrono::minutes{5});
@@ -4011,7 +4011,7 @@ void PeerLogicValidation::EvictExtraOutboundPeers(int64_t time_in_seconds)
     }
 }
 
-void PeerLogicValidation::CheckForStaleTipAndEvictPeers(const Consensus::Params &consensusParams)
+void PeerLogicValidation::CheckForStaleTipAndEvictPeers()
 {
     LOCK(cs_main);
 
@@ -4022,7 +4022,7 @@ void PeerLogicValidation::CheckForStaleTipAndEvictPeers(const Consensus::Params
     if (time_in_seconds > m_stale_tip_check_time) {
         // Check whether our tip is stale, and if so, allow using an extra
         // outbound peer
-        if (!fImporting && !fReindex && m_connman.GetNetworkActive() && m_connman.GetUseAddrmanOutgoing() && TipMayBeStale(consensusParams)) {
+        if (!fImporting && !fReindex && m_connman.GetNetworkActive() && m_connman.GetUseAddrmanOutgoing() && TipMayBeStale(m_chainparams.GetConsensus())) {
             LogPrintf("Potential stale tip detected, will try using extra outbound peer (last tip update: %d seconds ago)\n", time_in_seconds - g_last_tip_update);
             m_connman.SetTryNewOutboundPeer(true);
         } else if (m_connman.GetTryNewOutboundPeer()) {
diff --git a/src/net_processing.h b/src/net_processing.h
index b7a680f8ff..819c2c3d9f 100644
--- a/src/net_processing.h
+++ b/src/net_processing.h
@@ -6,12 +6,11 @@
 #ifndef BITCOIN_NET_PROCESSING_H
 #define BITCOIN_NET_PROCESSING_H
 
-#include <chainparams.h>
-#include <consensus/params.h>
 #include <net.h>
 #include <sync.h>
 #include <validationinterface.h>
 
+class CChainParams;
 class CTxMemPool;
 class ChainstateManager;
 
@@ -82,7 +81,7 @@ public:
     /** Consider evicting an outbound peer based on the amount of time they've been behind our tip */
     void ConsiderEviction(CNode& pto, int64_t time_in_seconds) EXCLUSIVE_LOCKS_REQUIRED(cs_main);
     /** Evict extra outbound peers. If we think our tip may be stale, connect to an extra outbound */
-    void CheckForStaleTipAndEvictPeers(const Consensus::Params &consensusParams);
+    void CheckForStaleTipAndEvictPeers();
     /** If we have extra outbound peers, try to disconnect the one with the oldest block announcement */
     void EvictExtraOutboundPeers(int64_t time_in_seconds) EXCLUSIVE_LOCKS_REQUIRED(cs_main);
     /** Retrieve unbroadcast transactions from the mempool and reattempt sending to peers */
diff --git a/src/test/denialofservice_tests.cpp b/src/test/denialofservice_tests.cpp
index c94d7c2d00..024345326d 100644
--- a/src/test/denialofservice_tests.cpp
+++ b/src/test/denialofservice_tests.cpp
@@ -169,7 +169,7 @@ BOOST_AUTO_TEST_CASE(stale_tip_peer_management)
         AddRandomOutboundPeer(vNodes, *peerLogic, connman.get());
     }
 
-    peerLogic->CheckForStaleTipAndEvictPeers(consensusParams);
+    peerLogic->CheckForStaleTipAndEvictPeers();
 
     // No nodes should be marked for disconnection while we have no extra peers
     for (const CNode *node : vNodes) {
@@ -180,7 +180,7 @@ BOOST_AUTO_TEST_CASE(stale_tip_peer_management)
 
     // Now tip should definitely be stale, and we should look for an extra
     // outbound peer
-    peerLogic->CheckForStaleTipAndEvictPeers(consensusParams);
+    peerLogic->CheckForStaleTipAndEvictPeers();
     BOOST_CHECK(connman->GetTryNewOutboundPeer());
 
     // Still no peers should be marked for disconnection
@@ -193,7 +193,7 @@ BOOST_AUTO_TEST_CASE(stale_tip_peer_management)
     // required time connected check should be satisfied).
     AddRandomOutboundPeer(vNodes, *peerLogic, connman.get());
 
-    peerLogic->CheckForStaleTipAndEvictPeers(consensusParams);
+    peerLogic->CheckForStaleTipAndEvictPeers();
     for (int i=0; i<max_outbound_full_relay; ++i) {
         BOOST_CHECK(vNodes[i]->fDisconnect == false);
     }
@@ -206,7 +206,7 @@ BOOST_AUTO_TEST_CASE(stale_tip_peer_management)
     // peer, and check that the next newest node gets evicted.
     UpdateLastBlockAnnounceTime(vNodes.back()->GetId(), GetTime());
 
-    peerLogic->CheckForStaleTipAndEvictPeers(consensusParams);
+    peerLogic->CheckForStaleTipAndEvictPeers();
     for (int i=0; i<max_outbound_full_relay-1; ++i) {
         BOOST_CHECK(vNodes[i]->fDisconnect == false);
     }

good idea. Done

in commit eb170099f3ea91555b56fdaaae89a64fe04b93be: Seems a bit odd to require the whole header when only the name is needed (the compiler doesn't need to know the struct memory layout to keep a pointer or reference as a member variable)

Note that txmemool.h isn't included either.

I suggest the patch from the other comment

yup. That's better. Done.

Does this need to be moved from the fuzz test into the codebase?

I don't understand the question. The method is public so it can be called by the fuzz test.

Yes, nvm.