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

dergoegge · 2022-09-20T13:56:43Z

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.

ajtowns · 2022-09-20T20:06:48Z

Concept ACK

DrahtBot · 2022-09-21T04:12:09Z

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 · 2022-09-21T16:42:45Z

Concept ACK.

src/net_processing.cpp

dergoegge · 2022-10-28T09:35:46Z

Rebased to avoid the p2p_sendtxrcncl.py failure.

src/net_processing.cpp

hernanmarino

Approach ACK

pablomartin4btc

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.

src/net_processing.cpp

glozow

light code review ACK de97ab2

…sgproc_mutex

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

dergoegge · 2023-03-30T12:58:40Z

Rebased

glozow

code review ACK 3a060ae, 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.

hebasto

ACK 3a060ae

hebasto · 2023-03-30T15:26:04Z

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.

jnewbery · 2023-03-30T16:19:38Z

src/net_processing.cpp

@@ -2248,7 +2258,7 @@ void PeerManagerImpl::ProcessGetBlockData(CNode& pfrom, Peer& peer, const CInv&
    }
 }

-CTransactionRef PeerManagerImpl::FindTxForGetData(const CNode& peer, const GenTxid& gtxid, const std::chrono::seconds mempool_req, const std::chrono::seconds now)
+CTransactionRef PeerManagerImpl::FindTxForGetData(const Peer& peer, const GenTxid& gtxid, const std::chrono::seconds mempool_req, const std::chrono::seconds now)


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

jnewbery · 2023-03-30T16:20:38Z

src/net_processing.cpp

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


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

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

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

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!

…sgproc mutex to Peer

3fa4c54 [net processing] Pass TxRelay to FindTxForGetData instead of Peer (dergoegge) c85ee76 [net processin] Don't take cs_main in FindTxForGetData (dergoegge) Pull request description: Addresses left over feedback from #26140. * bitcoin/bitcoin#26140 (comment) * bitcoin/bitcoin#26140 (comment) `mapRelay` is only accessed from the message processing thread and does not need to be kept in sync with anything validation specific, it is therfore perfectly fine to have it guarded by `g_msgproc_mutex`. ACKs for top commit: jnewbery: utACK 3fa4c54 hebasto: ACK 3fa4c54, I have reviewed the code and it looks OK, I agree it can be merged. Tree-SHA512: 3ef84bfe4abfa8d991a7e65d9184221294d80e0df0bbb47f0270ab6ca1593266c98abf83c610f9f86b4d16c7a4b62bcf83f8856c68d3c2e10894bff6ed3e88cd

…cs_main Summary: This is a partial backport of [[bitcoin/bitcoin#26140 | core#26140]] bitcoin/bitcoin@1d87137 Depends on D15287 Test Plan: With Debug: `ninja all check-all` Reviewers: #bitcoin_abc, Fabien Reviewed By: #bitcoin_abc, Fabien Differential Revision: https://reviews.bitcoinabc.org/D15288

…c_mutex and move to Peer Summary: > 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 is a partial backport of [[bitcoin/bitcoin#26140 | core#26140]] bitcoin/bitcoin@5f80d8d bitcoin/bitcoin@d8c0d1c Depends on D15288 Test Plan: With Debug: `ninja all check-all` Reviewers: #bitcoin_abc, Fabien Reviewed By: #bitcoin_abc, Fabien Differential Revision: https://reviews.bitcoinabc.org/D15289

…roc_mutex and move to Peer Summary: > 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 is a partial backport of [[bitcoin/bitcoin#26140 | core#26140]] bitcoin/bitcoin@4b84e50 bitcoin/bitcoin@4b84e50 Depends on D15289 Test Plan: With Debug: `ninja all check-all` Reviewers: #bitcoin_abc, Fabien Reviewed By: #bitcoin_abc, Fabien Differential Revision: https://reviews.bitcoinabc.org/D15290

…x and move to Peer Summary: > 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 is a partial backport of [[bitcoin/bitcoin#26140 | core#26140]] bitcoin/bitcoin@3605011 bitcoin/bitcoin@8a2cb1f Depends on D15290 Test Plan: With Debug: `ninja all check-all` Reviewers: #bitcoin_abc, Fabien Reviewed By: #bitcoin_abc, Fabien Differential Revision: https://reviews.bitcoinabc.org/D15291

…ed by g_msgproc_mutex and move to Peer Summary: This is a partial backport of [[bitcoin/bitcoin#26140 | core#26140]] bitcoin/bitcoin@938a8e2 bitcoin/bitcoin@279c53d Depends on D15291 Test Plan: With Debug `ninja all check-all` Reviewers: #bitcoin_abc, Fabien Reviewed By: #bitcoin_abc, Fabien Subscribers: Fabien Differential Revision: https://reviews.bitcoinabc.org/D15292

Summary: ``` -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 ``` This concludes backport of [[bitcoin/bitcoin#26140 | core#26140]] https://github.com/bitcoin/bitcoin/pull/26140/commits/3a060ae7b67cc28fc60cf28cbc518fa1df24f545- Depends on D15292 Test Plan: `ninja all check-all` Reviewers: #bitcoin_abc, Fabien Reviewed By: #bitcoin_abc, Fabien Differential Revision: https://reviews.bitcoinabc.org/D15293

fanquake added the P2P label Sep 20, 2022

dergoegge mentioned this pull request Sep 20, 2022

net: add NetEventsInterface::g_msgproc_mutex #26036

Merged

DrahtBot mentioned this pull request Sep 21, 2022

p2p: remove adjusted time #25908

Closed

dergoegge mentioned this pull request Sep 22, 2022

refactor: Guard TxRequestTracker by its own lock instead of cs_main #26151

Closed

DrahtBot mentioned this pull request Sep 22, 2022

p2p: Replace RecursiveMutex m_tx_inventory_mutex with Mutex and rename it #24125

Closed

DrahtBot mentioned this pull request Oct 8, 2022

p2p: Fill reconciliation sets and request reconciliation (Erlay) #26283

Closed

DrahtBot mentioned this pull request Oct 24, 2022

refactor: Move m_num_unconnecting_headers_msgs out of cs_main guard #25555

Closed

maflcko reviewed Oct 24, 2022

View reviewed changes

src/net_processing.cpp Outdated Show resolved Hide resolved

dergoegge force-pushed the 2022-09-move-cnodestate-msgproc-members branch from 8c22a6e to ca898b4 Compare October 24, 2022 14:27

DrahtBot mentioned this pull request Oct 24, 2022

refactor: Pass reference to last header, not pointer #26378

Merged

dergoegge force-pushed the 2022-09-move-cnodestate-msgproc-members branch from ca898b4 to cafaa98 Compare October 28, 2022 09:35

Riahiamirreza reviewed Oct 28, 2022

View reviewed changes

src/net_processing.cpp Show resolved Hide resolved

hernanmarino reviewed Nov 1, 2022

View reviewed changes

pablomartin4btc reviewed Nov 1, 2022

View reviewed changes

stickies-v reviewed Nov 2, 2022

View reviewed changes

stickies-v mentioned this pull request Nov 2, 2022

Notes and questions for #26140 bitcoin-core-review-club/website#578

Merged

DrahtBot mentioned this pull request Nov 11, 2022

p2p: Batch send NOTFOUND messages #26486

Closed

dergoegge force-pushed the 2022-09-move-cnodestate-msgproc-members branch from cafaa98 to de97ab2 Compare November 14, 2022 11:25

DrahtBot mentioned this pull request Nov 16, 2022

Erlay: bandwidth-efficient transaction relay protocol #21515

Closed

dergoegge requested a review from stickies-v November 28, 2022 19:08

glozow reviewed Dec 5, 2022

View reviewed changes

bitcoin deleted a comment from FullMoonBro Dec 5, 2022

DrahtBot mentioned this pull request Dec 6, 2022

refactor: Continue moving application data from CNode to Peer #26621

Closed

DrahtBot added the Needs rebase label Dec 8, 2022

dergoegge force-pushed the 2022-09-move-cnodestate-msgproc-members branch from de97ab2 to 7aa79e7 Compare December 19, 2022 11:35

DrahtBot removed the Needs rebase label Dec 19, 2022

dergoegge added 4 commits March 30, 2023 14:56

[net processing] Move fPreferHeaders from CNodeState to Peer

8a2cb1f

[net processing] Annotate m_recently_announced_invs as guarded by g_m…

938a8e2

…sgproc_mutex

[net processing] Move m_recently_announced_invs from CNodeState to Peer

279c53d

dergoegge force-pushed the 2022-09-move-cnodestate-msgproc-members branch from 37bb332 to 3a060ae Compare March 30, 2023 12:58

glozow reviewed Mar 30, 2023

View reviewed changes

DrahtBot requested a review from hernanmarino March 30, 2023 13:05

glozow requested review from hebasto and removed request for hernanmarino March 30, 2023 13:05

hebasto approved these changes Mar 30, 2023

View reviewed changes

DrahtBot requested a review from hernanmarino March 30, 2023 14:05

DrahtBot requested review from hernanmarino and removed request for hernanmarino March 30, 2023 15:26

glozow merged commit d4833e9 into bitcoin:master Mar 30, 2023

glozow removed their assignment Mar 30, 2023

jnewbery reviewed Mar 30, 2023

View reviewed changes

dergoegge mentioned this pull request Mar 31, 2023

net processing: #26140 follow-ups #27379

Merged

sidhujag pushed a commit to syscoin/syscoin that referenced this pull request Apr 1, 2023

Merge bitcoin#26140: refactor: Move CNodeState members guarded by g_m…

db2a0a1

…sgproc mutex to Peer

sidhujag pushed a commit to syscoin/syscoin that referenced this pull request Apr 2, 2023

Merge bitcoin#27379: net processing: bitcoin#26140 follow-ups

ba4a379

dergoegge mentioned this pull request Apr 20, 2023

meta: Isolated fuzzing of net processing #27502

Open

11 tasks

bitcoin locked and limited conversation to collaborators Mar 30, 2024

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

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

Uh oh!

Conversation

dergoegge commented Sep 20, 2022

Uh oh!

ajtowns commented Sep 20, 2022

Uh oh!

DrahtBot commented Sep 21, 2022 • edited Loading Uh oh! There was an error while loading. Please reload this page.

Uh oh!

Reviews

Conflicts

Uh oh!

hebasto commented Sep 21, 2022

Uh oh!

Uh oh!

dergoegge commented Oct 28, 2022

Uh oh!

Uh oh!

hernanmarino left a comment

Choose a reason for hiding this comment

Uh oh!

pablomartin4btc left a comment

Choose a reason for hiding this comment

Uh oh!

Uh oh!

Uh oh!

Uh oh!

Uh oh!

Uh oh!

Uh oh!

glozow left a comment

Choose a reason for hiding this comment

Uh oh!

dergoegge commented Mar 30, 2023

Uh oh!

glozow left a comment

Choose a reason for hiding this comment

Uh oh!

hebasto left a comment

Choose a reason for hiding this comment

Uh oh!

hebasto commented Mar 30, 2023

Uh oh!

jnewbery Mar 30, 2023

Choose a reason for hiding this comment

Uh oh!

jnewbery Mar 30, 2023

Choose a reason for hiding this comment

Uh oh!

dergoegge Mar 30, 2023

Choose a reason for hiding this comment

Uh oh!

dergoegge Mar 31, 2023

Choose a reason for hiding this comment

Uh oh!

dergoegge Mar 31, 2023

Choose a reason for hiding this comment

Uh oh!

Uh oh!

DrahtBot commented Sep 21, 2022 •

edited

Loading