net_processing: lock clean up #21527
Conversation
|
The following sections might be updated with supplementary metadata relevant to reviewers and maintainers. ConflictsReviewers, this pull request conflicts with the following ones:
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. |
jnewbery
left a comment
There was a problem hiding this comment.
Concept ACK. Lots of minor review comments inline, but a bit of high-level design feedback:
- I'm not at all convinced by the
g_mutex_message_handler. It seems like you're trying to reinvent thread_local storage. That makes potential future changes difficult. It also adds unnecessary coupling between net and net_processing. - This PR changes orphan processing from being done for the peer that provided the parent to the peer that provided the orphan. I think that's fine, but perhaps we should split the PR into one that does refactoring only and one that has behaviour changes.
src/net_processing.cpp
Outdated
There was a problem hiding this comment.
What do you think about removing this call? The comment above is incorrect (we only process one tx at most, not recursively), and we'll process the orphans in subsequent calls to ProcessMessages(). It seems strange that in this one case we can process up to two transactions in ProcessMessage() (the one that we just received, and up to one orphan child of it).
There was a problem hiding this comment.
We could process 3 txs I think -- the last orphan already in the workset (in ProcessMessages), one from a just received TX message, and an additional orphan whose parent was the contents of that TX message. I don't think it's problematic that way, but 1-non-trivial-ATMP-call per ProcessMessages invocation could be reasonable.
There was a problem hiding this comment.
Very good point. I hadn't considered processing the orphan before processing the net message.
src/txorphanage.h
Outdated
There was a problem hiding this comment.
This is a very strange interface. How about returning a std::optional<std::pair<CTransactionRef, NodeId>>, and then adding another public method HasTxToReconsider(). It's a bit less efficient, but we'll only ever call it after processing a transaction, so it seems negligible.
8e3a1bf to
9cc2da2
Compare
|
Rebased, some of the suggestions picked up, reworked the "message handler" mutex. |
jnewbery
left a comment
There was a problem hiding this comment.
I don't understand the second commit log: SendMessages() is now protected internally by m_mutex_messages; so this additional lock is not useful. SendMessages() is not protected by the new mutex as far as I can see.
I prefer the approach in #21563, which locks the mutex whenever any NetEvents method is called.
|
I've re-reviewed the locking changes and they look reasonable. There are still a few review comments outstanding. I'm happy to review this PR again once those have been addressed. |
9cc2da2 to
b67e5c8
Compare
|
I think this is now cleaned up, and ready for review. The behaviour changes that were in the last commit are now deferred to https://github.com/ajtowns/bitcoin/commits/202104-whohandlesorphans . |
|
When you rebase this, there are a few files that no longer need to include txorphanage.h: Diffdiff --git a/src/init.cpp b/src/init.cpp
index bb5b144802..e7b5ed60e3 100644
--- a/src/init.cpp
+++ b/src/init.cpp
@@ -52,7 +52,6 @@
#include <torcontrol.h>
#include <txdb.h>
#include <txmempool.h>
-#include <txorphanage.h>
#include <util/asmap.h>
#include <util/check.h>
#include <util/moneystr.h>
diff --git a/src/test/fuzz/process_message.cpp b/src/test/fuzz/process_message.cpp
index 7b99193ad0..5a71b25768 100644
--- a/src/test/fuzz/process_message.cpp
+++ b/src/test/fuzz/process_message.cpp
@@ -18,7 +18,6 @@
#include <test/util/net.h>
#include <test/util/setup_common.h>
#include <test/util/validation.h>
-#include <txorphanage.h>
#include <validationinterface.h>
#include <version.h>
diff --git a/src/test/fuzz/process_messages.cpp b/src/test/fuzz/process_messages.cpp
index 11b236c9bd..f8b1c8fc90 100644
--- a/src/test/fuzz/process_messages.cpp
+++ b/src/test/fuzz/process_messages.cpp
@@ -13,7 +13,6 @@
#include <test/util/net.h>
#include <test/util/setup_common.h>
#include <test/util/validation.h>
-#include <txorphanage.h>
#include <validation.h>
#include <validationinterface.h>Those files were only including txorphange.h to use the g_cs_orphans mutex. |
b67e5c8 to
499855a
Compare
499855a to
584043a
Compare
|
Rebased (and likewise the whohandlesorphans followup) |
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".
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.
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...
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.
SendMessages() is now protected g_mutex_msgproc_thread; so this additional per-node mutex is redundant.
This allows CNode members to be marked as guarded by the g_mutex_msgproc_thread mutex.
…ly via the msgproc thread
…ed only via the msgproc thread
…proc 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.
28079a3 to
023909c
Compare
|
🐙 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". |
|
Closing this. If you'd like it resurrected please help #25174 make progress. |
cs_sendProcessingis replaced by a private mutex in net_processing, non-orphan-specific things are moved out fromg_cs_orphansandg_cs_orphansis replaced by a private mutex in txorphanage.