m_tx_download_mutex followups#30507
Conversation
|
The following sections might be updated with supplementary metadata relevant to reviewers and maintainers. Code CoverageFor detailed information about the code coverage, see the test coverage report. ReviewsSee the guideline for information on the review process.
If your review is incorrectly listed, please react with 👎 to this comment and the bot will ignore it on the next update. 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. |
|
Concept ACK. |
dergoegge
left a comment
dergoegge
left a comment
There was a problem hiding this comment.
ACK 1a0212c8e5c25fce2e0a2cab3113b25f2fbc38b7
src/net_processing.cpp
Outdated
There was a problem hiding this comment.
note to myself: See if ACQUIRED_BEFORE helps here at compile time
There was a problem hiding this comment.
Hm, added a ACQUIRED_BEFORE, but compiler didn't complain when I added a LOCK2(m_mempool.cs, m_tx_download_mutex)
There was a problem hiding this comment.
From https://clang.llvm.org/docs/ThreadSafetyAnalysis.html:
ACQUIRED_BEFORE(…)andACQUIRED_AFTER(…)are currently being developed under the-Wthread-safety-betaflag.
Hm, added a
ACQUIRED_BEFORE, but compiler didn't complain when I added aLOCK2(m_mempool.cs, m_tx_download_mutex)
With CXXFLAGS=-Wthread-safety-beta, clang emits a warning:
net_processing.cpp:2078:5: warning: mutex 'm_tx_download_mutex' must be acquired before 'cs' [-Wthread-safety-analysis]
LOCK2(m_mempool.cs, m_tx_download_mutex);
^
./sync.h:260:16: note: expanded from macro 'LOCK2'
UniqueLock criticalblock2(MaybeCheckNotHeld(cs2), #cs2, __FILE__, __LINE__)
^
1 warning generated.
Now that m_txrequest and m_recent_confirmed_transactions are guarded by the same mutex, there is no benefit to processing them separately. Instead, just loop through pblock->vtx once.
glozow
force-pushed
the
2024-07-30111-followups
branch
from
1a0212c to
1b732db
Compare
theStack
left a comment
theStack
left a comment
There was a problem hiding this comment.
lgtm ACK 1b732dbc3bd9c59303a774cf2095c2d6639dbe1d
(happy to re-ACK if the Assert suggestion for pindexNewTip is taken)
- add AssertLockNotHeld(m_tx_download_mutex) in net_processing - move doc about m_tx_download_mutex and mempool mutex to ActiveTipChange
glozow
force-pushed
the
2024-07-30111-followups
branch
from
1b732db to
7c29e55
Compare
|
reACK 7c29e55 |
8 participants
Followup to #30111. Includes suggestions: