refactor: remove circular dependencies through net_processing (2/N) #5792
Conversation
|
This pull request has conflicts, please rebase. |
knst
force-pushed
the
fix-circular-net-processing-p2
branch
from
97053d5 to
5d3d3a8
Compare
UdjinM6
changed the title
knst
force-pushed
the
fix-circular-net-processing-p2
branch
2 times, most recently
from
d23f495 to
95972ed
Compare
|
This pull request has conflicts, please rebase. |
knst
force-pushed
the
fix-circular-net-processing-p2
branch
from
95972ed to
b42f0e3
Compare
… calls Asynchronous processing of IS by batches still depends on net_processing
They are static. Preparation for further refactorings
knst
force-pushed
the
fix-circular-net-processing-p2
branch
from
b42f0e3 to
afe08c0
Compare
| if (m_peerman == nullptr) { | ||
| m_peerman = peerman; | ||
| } | ||
| // we should never use one CInstantSendManager with different PeerManager | ||
| assert(m_peerman == peerman); |
There was a problem hiding this comment.
this scares me; why can this not be initialized in the CInstantSendManager constructor?
This feels like it introduces a data race
There was a problem hiding this comment.
There's is already race but in constructor.
Due to that LLMQContext accept an unique_ptr as an argument because PeerManager would be initialized after initializing of these classes (but required by them):
- CDKGSessionManager (eliminated by this PR)
- CSigningManager (eliminated by this PR)
- CSigSharesManager (still here)
- CInstantSendManager (eliminated by this PR)
When ProcessMessage is called first time, PeerManager is initialized; before any call of Process Message -> there's no any messages -> m_peerman is not needed because we can't ban anyone if no one send messages yet.
There was a problem hiding this comment.
Well; there's definitely still a race here if two threads call this ProcessMessage function while m_peerman is null (same in CDKGPendingMessages::PushPendingMessage)
There was a problem hiding this comment.
there's no actually race because network messages are accepted in one thread.
But it may change in future. Here's a fix for it: 7286643
That race is not happens currently because net_processing processes incomming messages in one thread at the moment, but that can be an issue some day.
|
Thoughts @UdjinM6 ? |
Issue being fixed or feature implemented
The architecture of bitcoin assumes that there's no any external class that processes network messages and knows anything about PeerManager from net_processing; no any external call for PeerManager::Misbehaving in bitcoin. All logic related to processing messages are located in net_processing.
Dash has many many extra types of network messages and many of them processed by external components such as llmq/signing or coinjoin/client. Current architecture creates multiple circular dependency.
What was done?
That's part II of refactorings.
This PR removes PeerManager from several constructor and let LLMQContext to forget about PeerManager.
Prior work in this PR: #5782
What else to do?
Some network messages are processed asynchronously in external components such as llmq/signing, llmq/instantsend, llmq/dkgsessionhandler. It doesn't let to refactor them easily, because they can't just simple return status of processing; status of processing would be available sometime later and there's need callback or other way to pass result code without spreading PeerManager over codebase.
How Has This Been Tested?
Breaking Changes
N/A
Checklist: