refactor: Move CNodeState members guarded by g_msgproc

dergoegge commented at 1:56 PM on September 20, 2022: member

nUnconnectingHeaders, m_headers_sync_timeout, fPreferHeaders and m_recently_announced_headers are currently all CNodeState members even though they are only ever accessed from the message processing thread (therefore sufficiently guarded exclusively by g_msgproc_mutex). CNodeState exists purely to hold validation-specific state guarded by cs_main that is accessed by multiple threads.

This PR adds thread-safety annotations for the above mentioned CNodeState members and moves them to Peer.

fanquake added the label P2P on Sep 20, 2022

ajtowns commented at 8:06 PM on September 20, 2022: contributor

Concept ACK

DrahtBot commented at 4:12 AM on September 21, 2022: contributor

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

Reviews

See the guideline for information on the review process.

Type	Reviewers
ACK	glozow, hebasto
Concept ACK	ajtowns
Approach ACK	pablomartin4btc
Stale ACK	hernanmarino

If your review is incorrectly listed, please react with 👎 to this comment and the bot will ignore it on the next update.

Conflicts

Reviewers, this pull request conflicts with the following ones:

#26621 (refactor: Continue moving application data from CNode to Peer by dergoegge)
#26283 (p2p: Fill reconciliation sets and request reconciliation (Erlay) by naumenkogs)
#25908 (p2p: remove adjusted time by fanquake)

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.

hebasto commented at 4:42 PM on September 21, 2022: member

Concept ACK.

in src/net_processing.cpp:395 in 8c22a6e0bf outdated

 390 | @@ -391,6 +391,18 @@ struct Peer {
 391 |      /** Whether we've sent our peer a sendheaders message. **/
 392 |      std::atomic<bool> m_sent_sendheaders{false};
 393 |  
 394 | +    /** Length of current-streak of unconnecting headers announcements */
 395 | +    int m_unconnecting_headers GUARDED_BY(NetEventsInterface::g_msgproc_mutex){0};

maflcko commented at 1:19 PM on October 24, 2022:

If this is touched, why not pick a better name, like m_num_unconnecting_headers_msgs (https://github.com/bitcoin/bitcoin/pull/25555/files)

dergoegge commented at 2:28 PM on October 24, 2022:

Right, makes sense. Also renamed MAX_UNCONNECTING_HEADERS.

stickies-v commented at 4:30 PM on November 2, 2022:

nit: I think MarcoFalke's suggestion m_num_unconnecting_headers_msgs (with num) is better than m_unconnecting_headers_msgs, quickly shows that this is a count.

dergoegge commented at 11:31 AM on November 14, 2022:

I meant to name it exactly like Marco did in #25555 🤦 Has the correct name now.

dergoegge force-pushed on Oct 24, 2022

dergoegge force-pushed on Oct 28, 2022

dergoegge commented at 9:35 AM on October 28, 2022: member

Rebased to avoid the p2p_sendtxrcncl.py failure.

in src/net_processing.cpp:138 in cafaa98104 outdated

 133 | @@ -134,7 +134,7 @@ static constexpr double BLOCK_DOWNLOAD_TIMEOUT_PER_PEER = 0.5;
 134 |  /** Maximum number of headers to announce when relaying blocks with headers message.*/
 135 |  static const unsigned int MAX_BLOCKS_TO_ANNOUNCE = 8;
 136 |  /** Maximum number of unconnecting headers announcements before DoS score */
 137 | -static const int MAX_UNCONNECTING_HEADERS = 10;

Riahiamirreza commented at 5:32 PM on October 28, 2022:

Would you explain what are unconnecting headers? My understanding is that these headers are for blocks which are received via an inv message but do not have any parent and belong to no chain. Is this correct?

dergoegge commented at 11:49 AM on November 14, 2022:

nUnconnectingHeaders tracks the number of headers messages (with fewer than MAX_BLOCKS_TO_ANNOUNCE headers) in which the first header did not connect to any known chain.

hernanmarino commented at 8:19 PM on November 1, 2022: contributor

Approach ACK

pablomartin4btc commented at 8:34 PM on November 1, 2022: member

Approach ACK. I agree with the thread-safety annotation and moving CNodeState members into Peer which concurs with @jnewbery's original refactoring intentions on P2P -> #19398.

in src/net_processing.cpp:665 in cafaa98104 outdated

 661 | @@ -659,7 +662,8 @@ class PeerManagerImpl final : public PeerManager
 662 |      /** Potentially fetch blocks from this peer upon receipt of a new headers tip */
 663 |      void HeadersDirectFetchBlocks(CNode& pfrom, const Peer& peer, const CBlockIndex* pindexLast);
 664 |      /** Update peer state based on received headers message */
 665 | -    void UpdatePeerStateForReceivedHeaders(CNode& pfrom, const CBlockIndex *pindexLast, bool received_new_header, bool may_have_more_headers);
 666 | +    void UpdatePeerStateForReceivedHeaders(CNode& pfrom, Peer& peer, const CBlockIndex* pindexLast, bool received_new_header, bool may_have_more_headers)

stickies-v commented at 4:22 PM on November 2, 2022:

Do we really need to pass Peer& peer here? I know it's a pattern that's used in other functions too, but I don't understand why we wouldn't just call GetPeerRef(pfrom.GetId()) instead. Unless I'm misunderstanding, pfrom and peer should always refer to the same node, so this seems unnecessarily confusing and bloating the function signature?

dergoegge commented at 11:43 AM on November 14, 2022:

We do this in a lot of places in net_processing, afaict we pass in a Peer when possible (because it is already available at the call site) otherwise we use GetPeerRef.

in src/net_processing.cpp:2698 in cafaa98104 outdated

2694 |          const CBlockIndex *pindexLast, bool received_new_header, bool may_have_more_headers)
2695 |  {
2696 | +    if (peer.m_unconnecting_headers_msgs > 0) {
2697 | +        LogPrint(BCLog::NET, "peer=%d: resetting m_unconnecting_headers_msgs (%d -> 0)\n", pfrom.GetId(), peer.m_unconnecting_headers_msgs);
2698 | +    }
2699 | +    peer.m_unconnecting_headers_msgs = 0;

stickies-v commented at 4:23 PM on November 2, 2022:

I know this is a move operation, but seems more logical to put it inside the if statement imo

    if (peer.m_unconnecting_headers_msgs > 0) {
        LogPrint(BCLog::NET, "peer=%d: resetting m_unconnecting_headers_msgs (%d -> 0)\n", pfrom.GetId(), peer.m_unconnecting_headers_msgs);
        peer.m_unconnecting_headers_msgs = 0;
    }

dergoegge commented at 11:45 AM on November 14, 2022:

Don't see the benefit of this and it would only make this harder to review imo.

in src/net_processing.cpp:2425 in cafaa98104 outdated

2427 |      if (MaybeSendGetHeaders(pfrom, GetLocator(m_chainman.m_best_header), peer)) {
2428 | -        LogPrint(BCLog::NET, "received header %s: missing prev block %s, sending getheaders (%d) to end (peer=%d, nUnconnectingHeaders=%d)\n",
2429 | +        LogPrint(BCLog::NET, "received header %s: missing prev block %s, sending getheaders (%d) to end (peer=%d, m_unconnecting_headers_msgs=%d)\n",
2430 |              headers[0].GetHash().ToString(),
2431 |              headers[0].hashPrevBlock.ToString(),
2432 |              m_chainman.m_best_header->nHeight,

stickies-v commented at 4:26 PM on November 2, 2022:

Since this is no longer locked with cs_main, I'm wondering if the m_chainman.m_best_header passed to MaybeSendGetHeaders and the m_chainman.m_best_header->nHeight passed to LogPrint could be different? I know it's just logging and not super critical, but could be annoying for debugging if so. Probably better to make that more robust?

dergoegge commented at 11:32 AM on November 14, 2022:

Added a commit to annotate m_best_header as guarded by cs_main and fixed this up to log the same height that is being passed to MaybeSendGetHeaders.

in src/net_processing.cpp:2430 in cafaa98104 outdated

2433 | -            pfrom.GetId(), nodestate->nUnconnectingHeaders);
2434 | +            pfrom.GetId(), peer.m_unconnecting_headers_msgs);
2435 | +    }
2436 | +
2437 | +    {
2438 | +        LOCK(cs_main);

stickies-v commented at 4:27 PM on November 2, 2022:

nit: Could use a WITH_LOCK here

in src/net_processing.cpp:901 in cafaa98104 outdated

 898 | +    CTransactionRef FindTxForGetData(const Peer& peer, const GenTxid& gtxid, const std::chrono::seconds mempool_req, const std::chrono::seconds now)
 899 | +        LOCKS_EXCLUDED(cs_main) EXCLUSIVE_LOCKS_REQUIRED(NetEventsInterface::g_msgproc_mutex);
 900 |  
 901 |      void ProcessGetData(CNode& pfrom, Peer& peer, const std::atomic<bool>& interruptMsgProc)
 902 | -        EXCLUSIVE_LOCKS_REQUIRED(!m_most_recent_block_mutex, peer.m_getdata_requests_mutex) LOCKS_EXCLUDED(::cs_main);
 903 | +        EXCLUSIVE_LOCKS_REQUIRED(!m_most_recent_block_mutex, peer.m_getdata_requests_mutex, NetEventsInterface::g_msgproc_mutex) LOCKS_EXCLUDED(::cs_main);

stickies-v commented at 4:28 PM on November 2, 2022:

nit: line length

        EXCLUSIVE_LOCKS_REQUIRED(!m_most_recent_block_mutex, peer.m_getdata_requests_mutex, NetEventsInterface::g_msgproc_mutex)
        LOCKS_EXCLUDED(::cs_main);

dergoegge force-pushed on Nov 14, 2022

dergoegge requested review from stickies-v on Nov 28, 2022

glozow commented at 12:02 PM on December 5, 2022: member

light code review ACK de97ab234f8479d388b0925dad75e83cf4148413

bitcoin deleted a comment on Dec 5, 2022

DrahtBot added the label Needs rebase on Dec 8, 2022

dergoegge force-pushed on Dec 19, 2022

DrahtBot removed the label Needs rebase on Dec 19, 2022

hernanmarino approved

hernanmarino commented at 12:34 AM on January 28, 2023: contributor

Re ACK 7aa79e77de222126b2f079ce26bf2792d5f66f1c

dergoegge removed review request from stickies-v on Mar 27, 2023

dergoegge requested review from glozow on Mar 27, 2023

in src/net_processing.cpp:2442 in 7aa79e77de outdated

2430 |  void PeerManagerImpl::HandleFewUnconnectingHeaders(CNode& pfrom, Peer& peer,
2431 |          const std::vector<CBlockHeader>& headers)
2432 |  {
2433 |      const CNetMsgMaker msgMaker(pfrom.GetCommonVersion());
2434 |  
2435 | -    LOCK(cs_main);

hebasto commented at 3:11 PM on March 29, 2023:

Before this change the value of m_chainman.m_best_header was synced with the UpdateBlockAvailability() call. Now they are not. Is it safe?

dergoegge commented at 4:01 PM on March 29, 2023:

Yes that is fine because m_best_header is not used by UpdateBlockAvailability afaict.

in src/net_processing.cpp:2723 in 7aa79e77de outdated

2701 | +void PeerManagerImpl::UpdatePeerStateForReceivedHeaders(CNode& pfrom, Peer& peer,
2702 |          const CBlockIndex& last_header, bool received_new_header, bool may_have_more_headers)
2703 |  {
2704 | +    if (peer.m_num_unconnecting_headers_msgs > 0) {
2705 | +        LogPrint(BCLog::NET, "peer=%d: resetting m_num_unconnecting_headers_msgs (%d -> 0)\n", pfrom.GetId(), peer.m_num_unconnecting_headers_msgs);
2706 | +    }

hebasto commented at 3:27 PM on March 29, 2023:

That's unfortunate that we call a logging function while holding a lock. Maybe

--- a/src/net_processing.cpp
+++ b/src/net_processing.cpp
@@ -2697,10 +2696,15 @@ void PeerManagerImpl::HeadersDirectFetchBlocks(CNode& pfrom, const Peer& peer, c
 void PeerManagerImpl::UpdatePeerStateForReceivedHeaders(CNode& pfrom, Peer& peer,
         const CBlockIndex& last_header, bool received_new_header, bool may_have_more_headers)
 {
-    if (peer.nUnconnectingHeaders > 0) {
-        LogPrint(BCLog::NET, "peer=%d: resetting nUnconnectingHeaders (%d -> 0)\n", pfrom.GetId(), peer.nUnconnectingHeaders);
+    int num_unconnecting_headers_msgs;
+    {
+        LOCK(NetEventsInterface::g_msgproc_mutex);
+        num_unconnecting_headers_msgs = peer.nUnconnectingHeaders;
+        peer.nUnconnectingHeaders = 0;
+    }
+    if (num_unconnecting_headers_msgs > 0) {
+        LogPrint(BCLog::NET, "peer=%d: resetting nUnconnectingHeaders (%d -> 0)\n", pfrom.GetId(), num_unconnecting_headers_msgs);
     }
-    peer.nUnconnectingHeaders = 0;
 
     LOCK(cs_main);
     CNodeState *nodestate = State(pfrom.GetId());

and drop EXCLUSIVE_LOCKS_REQUIRED(g_msgproc_mutex) annotation?

hebasto commented at 3:51 PM on March 29, 2023:

nm, it will cause double lock.

hebasto commented at 4:00 PM on March 29, 2023: member

Approach ACK 7aa79e77de222126b2f079ce26bf2792d5f66f1c

glozow assigned glozow on Mar 30, 2023

in src/net_processing.cpp:398 in c5799bde08 outdated

 393 | @@ -394,6 +394,9 @@ struct Peer {
 394 |      /** Whether this peer wants invs or headers (when possible) for block announcements */
 395 |      bool fPreferHeaders GUARDED_BY(NetEventsInterface::g_msgproc_mutex){false};
 396 |  
 397 | +    /** A rolling bloom filter of all announced tx CInvs to this peer */
 398 | +    CRollingBloomFilter m_recently_announced_invs GUARDED_BY(NetEventsInterface::g_msgproc_mutex){INVENTORY_MAX_RECENT_RELAY, 0.000001};

glozow commented at 11:28 AM on March 30, 2023:

Question: what is the reason to put m_recently_announced_invs in Peer as opposed to TxRelay within Peer?

dergoegge commented at 12:45 PM on March 30, 2023:

No reason, it should be in TxRelay, which also results in a small resource optimization because that filter now only exists for tx relay peers. Thanks!

DrahtBot requested review from glozow on Mar 30, 2023

dergoegge force-pushed on Mar 30, 2023

[validation] Annotate ChainstateManager::m_best_header as guarded by cs_main 1d87137227

[net processing] Annotate nUnconnectingHeaders as guarded by g_msgproc_mutex 5f80d8d1ee

[net processing] Move nUnconnectingHeaders from CNodeState to Peer d8c0d1c345

[net processing] Annotate m_headers_sync_timeout as guarded by g_msgproc_mutex 689b747fc3

[net processing] Move m_headers_sync_timeout from CNodeState to Peer 4b84e502f5

[net processing] Annotate fPreferHeaders as guarded by g_msgproc_mutex 3605011e79

[net processing] Move fPreferHeaders from CNodeState to Peer 8a2cb1f749

[net processing] Annotate m_recently_announced_invs as guarded by g_msgproc_mutex 938a8e2566

[net processing] Move m_recently_announced_invs from CNodeState to Peer 279c53d7e4

scripted-diff: Rename nUnconnectingHeaders and fPreferHeaders

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

ren nUnconnectingHeaders     m_num_unconnecting_headers_msgs
ren fPreferHeaders           m_prefers_headers
ren MAX_UNCONNECTING_HEADERS MAX_NUM_UNCONNECTING_HEADERS_MSGS

-END VERIFY SCRIPT-

3a060ae7b6

dergoegge force-pushed on Mar 30, 2023

dergoegge commented at 12:58 PM on March 30, 2023: member

Rebased

glozow commented at 1:05 PM on March 30, 2023: member

code review ACK 3a060ae7b67cc28fc60cf28cbc518fa1df24f545, as in I am convinced these members shouldn't be guarded by cs_main and belong in Peer/TxRelay. clang checked the annotations for me.

DrahtBot requested review from hernanmarino on Mar 30, 2023

glozow removed review request from hernanmarino on Mar 30, 2023

glozow requested review from hebasto on Mar 30, 2023

hebasto approved

hebasto commented at 2:05 PM on March 30, 2023: member

ACK 3a060ae7b67cc28fc60cf28cbc518fa1df24f545

DrahtBot requested review from hernanmarino on Mar 30, 2023

hebasto commented at 3:26 PM on March 30, 2023: member

FWIW, I have no issues with TSan-instrumented builds locally using both clang-14 and clang-15. Also the "TSan, depends, gui" CI task passed for me locally.

DrahtBot removed review request from hernanmarino on Mar 30, 2023

DrahtBot requested review from hernanmarino on Mar 30, 2023

glozow merged this on Mar 30, 2023

glozow closed this on Mar 30, 2023

glozow unassigned glozow on Mar 30, 2023

in src/net_processing.cpp:2261 in 3a060ae7b6

2257 | @@ -2248,7 +2258,7 @@ void PeerManagerImpl::ProcessGetBlockData(CNode& pfrom, Peer& peer, const CInv&
2258 |      }
2259 |  }
2260 |  
2261 | -CTransactionRef PeerManagerImpl::FindTxForGetData(const CNode& peer, const GenTxid& gtxid, const std::chrono::seconds mempool_req, const std::chrono::seconds now)
2262 | +CTransactionRef PeerManagerImpl::FindTxForGetData(const Peer& peer, const GenTxid& gtxid, const std::chrono::seconds mempool_req, const std::chrono::seconds now)

jnewbery commented at 4:19 PM on March 30, 2023:

Would it make sense to pass in a const TxRelay& here instead of a const Peer&?

in src/net_processing.cpp:2276 in 3a060ae7b6

2272 | @@ -2263,7 +2273,7 @@ CTransactionRef PeerManagerImpl::FindTxForGetData(const CNode& peer, const GenTx
2273 |      {
2274 |          LOCK(cs_main);
2275 |          // Otherwise, the transaction must have been announced recently.
2276 | -        if (State(peer.GetId())->m_recently_announced_invs.contains(gtxid.GetHash())) {
2277 | +        if (Assume(peer.GetTxRelay())->m_recently_announced_invs.contains(gtxid.GetHash())) {

jnewbery commented at 4:20 PM on March 30, 2023:

Can you remove the LOCK(cs_main) now (and the LOCKS_EXCLUDED(cs_main) annotation from the function declaration)?

dergoegge commented at 6:00 PM on March 30, 2023:

Yes, seems like it. Will do this in a follow up (including your other suggestion), thanks!

dergoegge commented at 11:04 AM on March 31, 2023:

Actually this doesn't work because mapRelay is guraded by cs_main😭😭😭

dergoegge commented at 11:09 AM on March 31, 2023:

But... mapRelay is actually only accessed from the msg proc thread, so we can simply mark it as guarded by g_msgproc_mutex and still do this!

sidhujag referenced this in commit db2a0a15ce on Apr 1, 2023

fanquake referenced this in commit 8e9e2b4cb3 on Apr 2, 2023

sidhujag referenced this in commit ba4a379d50 on Apr 2, 2023

Fabcien referenced this in commit 744681778e on Jan 26, 2024

Fabcien referenced this in commit 858e7997f2 on Jan 26, 2024

Fabcien referenced this in commit a2bb882b90 on Jan 26, 2024

Fabcien referenced this in commit cb3c8f71f0 on Jan 26, 2024

Fabcien referenced this in commit 763d666f73 on Jan 26, 2024

Fabcien referenced this in commit be8ada7181 on Jan 26, 2024

bitcoin locked this on Mar 30, 2024

refactor: Move CNodeState members guarded by g_msgproc_mutex to Peer #26140

Reviews

Conflicts