Additional thread safety annotations for CNode/Peer members accessed via the message processing thread

ajtowns commented at 7:14 pm on March 4, 2022: contributor

Documents why the existing accesses of elements in net.h’s CNode and net_processing.cpp’s Peer objects is thread safe, generally by making it so that the compiler will complain if the non-safe accesses are added. Only leaves CNode’s m_permissionFlags and m_prefer_evict unenforced by the compiler.

Adds a global mutex NetEventsInterface::g_mutex_msgproc_thread which is used to document the contract between net_processing and net that ProcessMessages and SendMessages are only invoked from a single thread, and which replaces the per-peer cs_sendProcessing mutex.

DrahtBot added the label P2P on Mar 4, 2022

ajtowns commented at 7:39 pm on March 4, 2022: contributor

I’ve pulled the m_mutex_message_handling change out of #21527 since it seems to have been the sticking point and refined it so that it is used to document all the un-annotated net/net_processing CNode/Peer variables in regards to thread-safety. The idea is this PR should just be converting the implicit assumptions we’re already making to explicit ones.

DrahtBot commented at 8:35 pm on March 4, 2022: contributor

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

Conflicts

Reviewers, this pull request conflicts with the following ones:

#25168 (refactor: Avoid passing params where not needed by MarcoFalke)
#24970 (net processing: Move cleanSubVer, fPreferredDownload and nLocalHostNonce to Peer 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.

ajtowns force-pushed on Mar 7, 2022

ajtowns commented at 8:12 am on March 7, 2022: contributor

Rebased on top of #24427

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

Concept ACK.

jonatack commented at 1:20 pm on March 24, 2022: contributor

Will have a look soon.

ajtowns force-pushed on Mar 25, 2022

ajtowns commented at 4:52 pm on March 25, 2022: contributor

Rebased past #21160

DrahtBot added the label Needs rebase on Apr 30, 2022

ajtowns force-pushed on May 3, 2022

DrahtBot removed the label Needs rebase on May 3, 2022

DrahtBot added the label Needs rebase on May 16, 2022

ajtowns force-pushed on May 16, 2022

ajtowns commented at 2:28 pm on May 16, 2022: contributor

Rebased past #25109

DrahtBot removed the label Needs rebase on May 16, 2022

DrahtBot added the label Needs rebase on May 19, 2022

net/net_processing: add missing thread safety annotations 70a3324d84

net: mark TransportSerializer/m_serializer as const

The (V1)TransportSerializer instance CNode::m_serializer is used from
multiple threads via PushMessage without protection by a mutex. This
is only thread safe because the class does not have any mutable state,
so document that by marking the methods and the object as "const".

fca9fadd4b

net: mark CNode unique_ptr members as const

Dereferencing a unique_ptr is not necessarily thread safe. The reason
these are safe is because their values are set at construction and do
not change later; so mark them as const and set them via the initializer
list to guarantee that.

021a96d41b

net: note CNode members that are treated as const

m_permissionFlags and m_prefer_evict are treated as const -- they're
only set immediately after construction before any other thread has
access to the object, and not changed again afterwards. As such they
don't need to be marked atomic or guarded by a mutex; though it would
probably be better to actually mark them as const...

5df0bd1e8d

net: add NetEventsInterface::g_mutex_msgproc_thread mutex

There are many cases where we asssume message processing is
single-threaded in order for how we access node-related memory to be
safe. Add an explicit mutex that we can use to document this, which allows
the compiler to catch any cases where we try to access that memory from
other threads and break that assumption.

c8c4faac48

net: drop cs_sendProcessing

SendMessages() is now protected g_mutex_msgproc_thread; so this additional
per-node mutex is redundant.

b1af785346

[move-only] net: move NetEventsInterface before CNode

This allows CNode members to be marked as guarded by the
g_mutex_msgproc_thread mutex.

8e75e74e6a

net: add thread safety annotations for CNode/Peer members accessed only via the msgproc thread 49b72d235c

net_processing: add thread safety annotations for Peer members accessed only via the msgproc thread 3ca9617d45

net_processing: extra transactions data are accessed only via the msgproc thread

Previously vExtraTxnForCompact and vExtraTxnForCompactIt were protected
by g_cs_orphans; protect them by g_mutex_msgproc_thread instead, as they
are only used during message processing.

49c3b28ab4

ajtowns force-pushed on May 19, 2022

ajtowns commented at 8:32 pm on May 19, 2022: contributor

Rebased past #22778

DrahtBot removed the label Needs rebase on May 19, 2022

ajtowns commented at 3:00 am on May 20, 2022: contributor

See #25174 for the first few patches from this.

DrahtBot added the label Needs rebase on May 20, 2022

DrahtBot commented at 1:40 pm on May 20, 2022: contributor

🐙 This pull request conflicts with the target branch and needs rebase.

Want to unsubscribe from rebase notifications on this pull request? Just convert this pull request to a “draft”.

ajtowns renamed this:
~~Additional thread safety annotations for CNode/Peer~~
Additional thread safety annotations for CNode/Peer members accessed via the message processing thread
on May 20, 2022

ajtowns commented at 6:53 am on August 29, 2022: contributor

Closing this. If you’d like it resurrected please help #25174 make progress.

ajtowns closed this on Aug 29, 2022

ajtowns commented at 6:06 pm on September 7, 2022: contributor

Replaced by #26036

bitcoin locked this on Sep 7, 2023

Additional thread safety annotations for CNode/Peer members accessed via the message processing thread #24474

Conflicts