Net processing: move ProcessMessage() to PeerLogicValidation

jnewbery commented at 11:38 am on August 12, 2020: member

Rather than ProcessMessage() being a static function in net_processing.cpp, make it a private member function of PeerLogicValidation. This is the start of moving static functions and global variables into PeerLogicValidation to make it better encapsulated.

fanquake added the label P2P on Aug 12, 2020

jnewbery commented at 11:42 am on August 12, 2020: member

This was requested by @MarcoFalke @sdaftuar and @theuni in #19607 (https://github.com/bitcoin/bitcoin/pull/19607#discussion_r462032894).

Changing CConnman* connman to CConnman& m_connman is the continuation of work in #19174. See #19542 (comment) and #19174 (comment) for further discussion. Prior to this PR, connman was a pointer in PeerLogicValidation, but a reference everywhere else in net_processing. Changing it to a reference in PeerLogicValidation is required before moving the rest of the functions into PeerLogicValidation.

EDIT: I’ve started by moving ProcessMessage() into PLV because in order to move the new Peer struct into PLV, Misbehaving() needs to be a member function of PLV, which means everything above it in this call tree https://doxygen.bitcoincore.org/net__processing_8cpp.html#ad2983e754c4d90bd68adf91b208664f5 needs to be a member (or hold a reference to peer_logic, which I don’t think makes sense)

Crypt-iQ commented at 12:23 pm on August 12, 2020: contributor

Fuzzers fail because ProcessMessage is used in the harnesses.

theStack commented at 12:57 pm on August 12, 2020: member

Concept ACK – this is definitely a more appropriate location for ProcessMessage than the whole net_processing module scope.

jnewbery force-pushed on Aug 12, 2020

jnewbery commented at 1:20 pm on August 12, 2020: member

I’ve fixed the fuzz build (I think). Let’s see what Travis thinks.

[net_processing] Change PeerLogicValidation to hold a connman reference

Hold a reference to connman rather than a pointer because:

- PeerLogicValidation can't run without a connman
- The pointer never gets reseated

The alternative is to always assert that the pointer is non-null before
dereferencing.

Change the name from connman to m_connman at the same time to conform
with current style guidelines.

c556770b5e

jnewbery commented at 1:30 pm on August 12, 2020: member

rebased

jnewbery force-pushed on Aug 12, 2020

jonatack commented at 8:09 pm on August 13, 2020: member

Light approach ACK, if there is a follow-up code branch to see where this is heading I’d look at it.

JeremyRubin commented at 8:39 pm on August 13, 2020: contributor

Concept Ack and lite-cr ack (doesn’t seem to change any logic, just refactor).

In terms of approach might be nice to encapsulate processmessage as a subclass that only has access to the relevant references out of the PeerLogicValidation state, but I don’t think that’s required (just thinking ahead that we might want all the individual message handlers in their own class).

edit: another reason for this is you can appropriately bind things in the interior class as const where relevant.

jnewbery commented at 9:44 am on August 14, 2020: member

Lots of discussion on this at http://www.erisian.com.au/meetbot/bitcoin-core-dev/2020/bitcoin-core-dev.2020-08-13-19.00.log.html#l-104

promag commented at 2:03 pm on August 14, 2020: member

Code review ACK cbcb80abc8b3bddaba81e8ba2b22c7d957f02f37.

Am I missing something or this should have refactoring label?

First 2 commits could be merged on its own.

jnewbery added the label Refactoring on Aug 14, 2020

DrahtBot commented at 8:37 pm on August 20, 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:

#19763 (net: don’t try to relay to the address’ originator by vasild)
#19753 (p2p: don’t add AlreadyHave transactions to recentRejects by troygiorshev)
#19724 ([net] Cleanup connection types- followups by amitiuttarwar)
#19697 (Improvements on ADDR caching by naumenkogs)
#19668 (Do not hide compile-time thread safety warnings by hebasto)
#19610 (p2p: refactor AlreadyHave(), CInv::type, INV/TX processing by jonatack)
#19498 (Tidy up ProcessOrphanTx by jnewbery)
#19339 (validation: re-delegate absurd fee checking from mempool to clients by gzhao408)
#19306 (refactor: Replace RecursiveMutex with Mutex in CTxMemPool by hebasto)
#19107 (p2p: Move all header verification into the network layer, extend logging by troygiorshev)
#19031 (Implement ADDRv2 support (part of BIP155) by vasild)
#17785 (p2p: Unify Send and Receive protocol versions by hebasto)
#17479 (Return BlockValidationState from ProcessNewBlock if CheckBlock/AcceptBlock fails by jnewbery)
#15606 ([experimental] UTXO snapshots by jamesob)

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.

in src/test/denialofservice_tests.cpp:155 in eb170099f3 outdated

149@@ -149,8 +150,9 @@ static void AddRandomOutboundPeer(std::vector<CNode *> &vNodes, PeerLogicValidat
150 
151 BOOST_AUTO_TEST_CASE(stale_tip_peer_management)
152 {
153+    const CChainParams& chainparams = Params();
154     auto connman = MakeUnique<CConnmanTest>(0x1337, 0x1337);
155-    auto peerLogic = MakeUnique<PeerLogicValidation>(*connman, nullptr, *m_node.scheduler, *m_node.chainman, *m_node.mempool);
156+    auto peerLogic = MakeUnique<PeerLogicValidation>(chainparams, *connman, nullptr, *m_node.scheduler, *m_node.chainman, *m_node.mempool);

MarcoFalke commented at 7:36 am on August 21, 2020:

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:

  0diff --git a/src/net_processing.cpp b/src/net_processing.cpp
  1index 71b235f56b..4a5c7acc46 100644
  2--- a/src/net_processing.cpp
  3+++ b/src/net_processing.cpp
  4@@ -9,6 +9,7 @@
  5 #include <banman.h>
  6 #include <blockencodings.h>
  7 #include <blockfilter.h>
  8+#include <chainparams.h>
  9 #include <consensus/validation.h>
 10 #include <hash.h>
 11 #include <index/blockfilterindex.h>
 12@@ -1219,13 +1220,12 @@ PeerLogicValidation::PeerLogicValidation(const CChainParams& chainparams, CConnm
 13     // same probability that we have in the reject filter).
 14     g_recent_confirmed_transactions.reset(new CRollingBloomFilter(48000, 0.000001));
 15 
 16-    const Consensus::Params& consensusParams = Params().GetConsensus();
 17     // Stale tip checking and peer eviction are on two different timers, but we
 18     // don't want them to get out of sync due to drift in the scheduler, so we
 19     // combine them in one function and schedule at the quicker (peer-eviction)
 20     // timer.
 21     static_assert(EXTRA_PEER_CHECK_INTERVAL < STALE_CHECK_INTERVAL, "peer eviction timer should be less than stale tip check timer");
 22-    scheduler.scheduleEvery([this, consensusParams] { this->CheckForStaleTipAndEvictPeers(consensusParams); }, std::chrono::seconds{EXTRA_PEER_CHECK_INTERVAL});
 23+    scheduler.scheduleEvery([this] { this->CheckForStaleTipAndEvictPeers(); }, std::chrono::seconds{EXTRA_PEER_CHECK_INTERVAL});
 24 
 25     // schedule next run for 10-15 minutes in the future
 26     const std::chrono::milliseconds delta = std::chrono::minutes{10} + GetRandMillis(std::chrono::minutes{5});
 27@@ -4011,7 +4011,7 @@ void PeerLogicValidation::EvictExtraOutboundPeers(int64_t time_in_seconds)
 28     }
 29 }
 30 
 31-void PeerLogicValidation::CheckForStaleTipAndEvictPeers(const Consensus::Params &consensusParams)
 32+void PeerLogicValidation::CheckForStaleTipAndEvictPeers()
 33 {
 34     LOCK(cs_main);
 35 
 36@@ -4022,7 +4022,7 @@ void PeerLogicValidation::CheckForStaleTipAndEvictPeers(const Consensus::Params
 37     if (time_in_seconds > m_stale_tip_check_time) {
 38         // Check whether our tip is stale, and if so, allow using an extra
 39         // outbound peer
 40-        if (!fImporting && !fReindex && m_connman.GetNetworkActive() && m_connman.GetUseAddrmanOutgoing() && TipMayBeStale(consensusParams)) {
 41+        if (!fImporting && !fReindex && m_connman.GetNetworkActive() && m_connman.GetUseAddrmanOutgoing() && TipMayBeStale(m_chainparams.GetConsensus())) {
 42             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);
 43             m_connman.SetTryNewOutboundPeer(true);
 44         } else if (m_connman.GetTryNewOutboundPeer()) {
 45diff --git a/src/net_processing.h b/src/net_processing.h
 46index b7a680f8ff..819c2c3d9f 100644
 47--- a/src/net_processing.h
 48+++ b/src/net_processing.h
 49@@ -6,12 +6,11 @@
 50 #ifndef BITCOIN_NET_PROCESSING_H
 51 #define BITCOIN_NET_PROCESSING_H
 52 
 53-#include <chainparams.h>
 54-#include <consensus/params.h>
 55 #include <net.h>
 56 #include <sync.h>
 57 #include <validationinterface.h>
 58 
 59+class CChainParams;
 60 class CTxMemPool;
 61 class ChainstateManager;
 62 
 63@@ -82,7 +81,7 @@ public:
 64     /** Consider evicting an outbound peer based on the amount of time they've been behind our tip */
 65     void ConsiderEviction(CNode& pto, int64_t time_in_seconds) EXCLUSIVE_LOCKS_REQUIRED(cs_main);
 66     /** Evict extra outbound peers. If we think our tip may be stale, connect to an extra outbound */
 67-    void CheckForStaleTipAndEvictPeers(const Consensus::Params &consensusParams);
 68+    void CheckForStaleTipAndEvictPeers();
 69     /** If we have extra outbound peers, try to disconnect the one with the oldest block announcement */
 70     void EvictExtraOutboundPeers(int64_t time_in_seconds) EXCLUSIVE_LOCKS_REQUIRED(cs_main);
 71     /** Retrieve unbroadcast transactions from the mempool and reattempt sending to peers */
 72diff --git a/src/test/denialofservice_tests.cpp b/src/test/denialofservice_tests.cpp
 73index c94d7c2d00..024345326d 100644
 74--- a/src/test/denialofservice_tests.cpp
 75+++ b/src/test/denialofservice_tests.cpp
 76@@ -169,7 +169,7 @@ BOOST_AUTO_TEST_CASE(stale_tip_peer_management)
 77         AddRandomOutboundPeer(vNodes, *peerLogic, connman.get());
 78     }
 79 
 80-    peerLogic->CheckForStaleTipAndEvictPeers(consensusParams);
 81+    peerLogic->CheckForStaleTipAndEvictPeers();
 82 
 83     // No nodes should be marked for disconnection while we have no extra peers
 84     for (const CNode *node : vNodes) {
 85@@ -180,7 +180,7 @@ BOOST_AUTO_TEST_CASE(stale_tip_peer_management)
 86 
 87     // Now tip should definitely be stale, and we should look for an extra
 88     // outbound peer
 89-    peerLogic->CheckForStaleTipAndEvictPeers(consensusParams);
 90+    peerLogic->CheckForStaleTipAndEvictPeers();
 91     BOOST_CHECK(connman->GetTryNewOutboundPeer());
 92 
 93     // Still no peers should be marked for disconnection
 94@@ -193,7 +193,7 @@ BOOST_AUTO_TEST_CASE(stale_tip_peer_management)
 95     // required time connected check should be satisfied).
 96     AddRandomOutboundPeer(vNodes, *peerLogic, connman.get());
 97 
 98-    peerLogic->CheckForStaleTipAndEvictPeers(consensusParams);
 99+    peerLogic->CheckForStaleTipAndEvictPeers();
100     for (int i=0; i<max_outbound_full_relay; ++i) {
101         BOOST_CHECK(vNodes[i]->fDisconnect == false);
102     }
103@@ -206,7 +206,7 @@ BOOST_AUTO_TEST_CASE(stale_tip_peer_management)
104     // peer, and check that the next newest node gets evicted.
105     UpdateLastBlockAnnounceTime(vNodes.back()->GetId(), GetTime());
106 
107-    peerLogic->CheckForStaleTipAndEvictPeers(consensusParams);
108+    peerLogic->CheckForStaleTipAndEvictPeers();
109     for (int i=0; i<max_outbound_full_relay-1; ++i) {
110         BOOST_CHECK(vNodes[i]->fDisconnect == false);
111     }

jnewbery commented at 10:06 am on August 21, 2020:

good idea. Done

in src/net_processing.h:9 in eb170099f3 outdated

5@@ -6,6 +6,7 @@
6 #ifndef BITCOIN_NET_PROCESSING_H
7 #define BITCOIN_NET_PROCESSING_H
8 
9+#include <chainparams.h>

MarcoFalke commented at 7:38 am on August 21, 2020:

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

jnewbery commented at 10:07 am on August 21, 2020:

yup. That’s better. Done.

MarcoFalke approved

MarcoFalke commented at 7:42 am on August 21, 2020: member

cr ACK cbcb80abc8b3bddaba81e8ba2b22c7d957f02f37 🎤

Signature:

 0-----BEGIN PGP SIGNED MESSAGE-----
 1Hash: SHA512
 2
 3cr ACK cbcb80abc8b3bddaba81e8ba2b22c7d957f02f37 🎤
 4-----BEGIN PGP SIGNATURE-----
 5
 6iQGzBAEBCgAdFiEE+rVPoUahrI9sLGYTzit1aX5ppUgFAlwqrYAACgkQzit1aX5p
 7pUgG4wwAjDXVdxnfaSOSedVWUkK0Xn7dJWYM57677OgNjxyljDUvsMvySG6Gg60O
 8ffe/wB6mwYqXkW2+O+B+Tw2VQbC0AnaXlw9vNnuBDv3S8QDU3f1HE/a+48DWYqPc
 9ipnSEOMZuDsWi31mQrtOOBHxD5hDulhF0Wx6I8SJ+iMhEk20Nd2T1ZQKZR6yJ2oS
10zOUeRjr/7n3/aeXdCJZ/AiTkVLCZskNSn9fbTSpFsEdefw/kVFzfm4YViCfRvLsn
11sb7Cs2Us07P64erk5JsJIzIh+C8QjWgWo57UW/sOb9zF05q7fCgpXjzf2iWxMlBe
12jctdKBoo9Y6SVKY5+Lc3XX+Y36/ae6tKtO5KobjNCDACwLLRnITaJJzltKLs+dTU
13AriR/ssfIsamz8cbhiMeJiDggdlNugaenPaW9U/EIDx6seOhkR10FkMl4aHJhTUx
14VBoBzUnGnCf0nW0WVKd9uujdC8qG33ub5Ppq1s21i1NC47I8uv6hS4XZ8tUxCPlD
1580mbsjhw
16=WycG
17-----END PGP SIGNATURE-----

Timestamp of file with hash c58fbfd8242739033e1f88dae62117af90263272c9149109ad99aba5f1597e88 -

jnewbery force-pushed on Aug 21, 2020

jnewbery commented at 10:07 am on August 21, 2020: member

Great suggestions @MarcoFalke . I’ve taken them all.

jonatack commented at 11:08 am on August 21, 2020: member

travis and cirrus:

0MemorySanitizer: use-of-uninitialized-value 
1src/net_processing.cpp:656:82 in (anonymous namespace)::TipMayBeStale(Consensus::Params const&)

https://travis-ci.org/github/bitcoin/bitcoin/jobs/719886303

https://github.com/bitcoin/bitcoin/pull/19704/checks?check_run_id=1011864356

in src/net_processing.h:90 in 4d6c4d2058 outdated

87     void EvictExtraOutboundPeers(int64_t time_in_seconds) EXCLUSIVE_LOCKS_REQUIRED(cs_main);
88     /** Retrieve unbroadcast transactions from the mempool and reattempt sending to peers */
89     void ReattemptInitialBroadcast(CScheduler& scheduler) const;
90 
91+    /** Process a single message from a peer. Public for fuzz testing */
92+    void ProcessMessage(CNode& pfrom, const std::string& msg_type, CDataStream& vRecv,

jonatack commented at 11:28 am on August 21, 2020:

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

jnewbery commented at 12:14 pm on August 21, 2020:

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

jonatack commented at 9:31 am on August 24, 2020:

Yes, nvm.

jonatack commented at 11:29 am on August 21, 2020: member

Re-reviewing, I like these changes. One question below, in addition to the msan use-of-uninitialized-value issue.

[net_processing] Move ProcessMessage to PeerLogicValidation daed542a12

jnewbery force-pushed on Aug 21, 2020

jnewbery commented at 12:13 pm on August 21, 2020: member

travis and cirrus:

MemorySanitizer: use-of-uninitialized-value src/net_processing.cpp:656:82 in (anonymous namespace)::TipMayBeStale(Consensus::Params const&)

It looks like that happens because SelectParams() is called twice in the TestChain100Setup constructor, including after the PeerLogicValidation object is constructed (see https://github.com/bitcoin/bitcoin/commit/666696b673c5d3b7ab467a890cafd13ac19be503).

Rather than fight against the unit test framework, I’ve removed that commit. It’s not essential for this PR.

theStack approved

theStack commented at 10:10 pm on August 23, 2020: member

Code Review ACK daed542a12e0a6a4692aca12a61b84cd55accc33 It’s nice to see that the interface of ProcessMessage() shrinks down from 10 to 6 parameters (or 7, if you also count the implicit this pointer).

MarcoFalke commented at 8:57 am on August 24, 2020: member

re-ACK daed542a12, only change is removing second commit 🎴

Signature:

 0-----BEGIN PGP SIGNED MESSAGE-----
 1Hash: SHA512
 2
 3re-ACK daed542a12, only change is removing second commit 🎴
 4-----BEGIN PGP SIGNATURE-----
 5
 6iQGzBAEBCgAdFiEE+rVPoUahrI9sLGYTzit1aX5ppUgFAlwqrYAACgkQzit1aX5p
 7pUi57gv/VAtt+n9kwA+RHgQQQsRU5ybObKW5pKc8cpR0kcgd6rwgJPvCdnYUZyIG
 8eDpCMi4O4oHW7usvpn9koFWtHr8S3NPGqXbmF2PiIDb0aK8qkaUafqLWNgb/vI5T
 9xcTs1XpOF1SW3IB6LnrVtwjk9j9iCeQ67Uu1JdEa9o6e8d84CnMbYBiealLA4f5V
10UunvG+QZ/vCXzPks4+Q2u9k9FBGj68ED723xJILw743+JLa3Czu4JXT+SPLsC33D
119THMkP38eaWpothKivyE8aNWzG9vSidi3R12apLHCisg54MuWRWNYygD3t5QEvpb
12Ca8ZBXZy665M7TOkfrfayNtgWi39zNI7TJabFgH7OAa/InPlGKGtdMepgd1a3+EF
13ZPhItdhgzwy221a80W2NtEyzxXnA3cWY0JBumvWA0P1HY6PIkAAe24rY5Zu25dyk
14Nj3dBfg9LV/6cSSEJqIdZyWJ0/dPSeMDzejzFWwRGgF/gX4bpyYAhotHXcf1CcBY
15CYtzu5vy
16=l6KP
17-----END PGP SIGNATURE-----

Timestamp of file with hash 08eeaa12e1f73cc25bec71f7f0ba83ae3ad3bc6225f47932f8a0d313daf8ffd8 -

jonatack commented at 9:30 am on August 24, 2020: member

ACK daed542a12e0a6a4692aca12a61b84cd55accc33 code review and debug tested

promag commented at 9:36 am on August 24, 2020: member

Code review ACK daed542a12e0a6a4692aca12a61b84cd55accc33.

fanquake merged this on Aug 24, 2020

fanquake closed this on Aug 24, 2020

jnewbery deleted the branch on Aug 24, 2020

sidhujag referenced this in commit 91657de432 on Aug 24, 2020

fanquake referenced this in commit 92735e45ba on Aug 26, 2020

fanquake referenced this in commit 6a2ba62685 on Aug 26, 2020

sidhujag referenced this in commit 914f824016 on Aug 26, 2020

sidhujag referenced this in commit 60a866c284 on Aug 26, 2020

MarcoFalke referenced this in commit 147d50d63e on Sep 7, 2020

Fabcien referenced this in commit fd797bb033 on Jan 6, 2021

Fabcien referenced this in commit a5e788409e on Jan 6, 2021

DrahtBot locked this on Feb 15, 2022

Net processing: move ProcessMessage() to PeerLogicValidation #19704

Conflicts