Followup to #30111. Includes suggestions:
m_tx_download_mutex followups #30507
pull glozow wants to merge 5 commits into bitcoin:master from glozow:2024-07-30111-followups changing 5 files +39 −37-
glozow commented at 12:36 pm on July 23, 2024: member
-
glozow added the label Refactoring on Jul 23, 2024
-
glozow added the label Docs on Jul 23, 2024
-
DrahtBot commented at 12:36 pm on July 23, 2024: contributor
The following sections might be updated with supplementary metadata relevant to reviewers and maintainers.
Code Coverage
For detailed information about the code coverage, see the test coverage report.
Reviews
See the guideline for information on the review process.
Type Reviewers ACK instagibbs, theStack, dergoegge Concept ACK hebasto 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:
- #30110 (refactor: TxDownloadManager by glozow)
- #29641 (scripted-diff: Use LogInfo/LogDebug over LogPrintf/LogPrint by maflcko)
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 commented at 7:15 am on July 24, 2024: memberConcept ACK.
-
in src/net_processing.cpp:6300 in 137b968502 outdated
6294@@ -6295,6 +6295,7 @@ bool PeerManagerImpl::SendMessages(CNode* pto) 6295 // 6296 // Message: getdata (transactions) 6297 // 6298+ {
dergoegge commented at 8:23 am on July 24, 2024:style nit: Indenting the body of the new scope would be nice
glozow commented at 11:51 am on July 24, 2024:donedergoegge approveddergoegge commented at 8:24 am on July 24, 2024: memberACK 1a0212c8e5c25fce2e0a2cab3113b25f2fbc38b7DrahtBot requested review from hebasto on Jul 24, 2024in src/validationinterface.cpp:188 in 1a0212c8e5 outdated
182@@ -183,6 +183,12 @@ void ValidationSignals::UpdatedBlockTip(const CBlockIndex *pindexNew, const CBlo 183 fInitialDownload); 184 } 185 186+void ValidationSignals::ActiveTipChange(const CBlockIndex *new_tip, bool is_ibd) 187+{ 188+ LOG_EVENT("%s: new block hash=%s block height=%d", __func__, new_tip->GetBlockHash().ToString(), new_tip->nHeight);
maflcko commented at 9:09 am on July 24, 2024:style-nit: Generally, when a pointer is unconditionally dereferenced, it is better to use a reference, because it can not be null. This means that any possible undefined behavior, or crash, is visible early on in the call stack and will more likely be caught early during review, instead of only at runtime (when debug logging is active), or not at all.
0void ValidationSignals::ActiveTipChange(const CBlockIndex& new_tip, bool is_ibd) 1{ 2 LOG_EVENT("%s: new block hash=%s block height=%d", __func__, new_tip.GetBlockHash().ToString(), new_tip.nHeight);
glozow commented at 11:52 am on July 24, 2024:done[refactor] combine block vtx loops in BlockConnected
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.
[doc] TxOrphanage is no longer thread-safe 7cc5ac5a67in src/net_processing.cpp:2074 in 1a0212c8e5 outdated
2069@@ -2072,6 +2070,8 @@ void PeerManagerImpl::StartScheduledTasks(CScheduler& scheduler) 2070 2071 void PeerManagerImpl::ActiveTipChange(const CBlockIndex* new_tip, bool is_ibd) 2072 { 2073+ // Ensure mempool mutex was released, otherwise deadlock may occur if another thread holding 2074+ // m_tx_download_mutex waits on the mempool mutex.
maflcko commented at 9:13 am on July 24, 2024:note to myself: See ifACQUIRED_BEFORE
helps here at compile time
glozow commented at 11:51 am on July 24, 2024:Hm, added aACQUIRED_BEFORE
, but compiler didn’t complain when I added aLOCK2(m_mempool.cs, m_tx_download_mutex)
hebasto commented at 3:24 pm on July 25, 2024:From https://clang.llvm.org/docs/ThreadSafetyAnalysis.html:
ACQUIRED_BEFORE(…)
andACQUIRED_AFTER(…)
are currently being developed under the-Wthread-safety-beta
flag.
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:0net_processing.cpp:2078:5: warning: mutex 'm_tx_download_mutex' must be acquired before 'cs' [-Wthread-safety-analysis] 1 LOCK2(m_mempool.cs, m_tx_download_mutex); 2 ^ 3./sync.h:260:16: note: expanded from macro 'LOCK2' 4 UniqueLock criticalblock2(MaybeCheckNotHeld(cs2), #cs2, __FILE__, __LINE__) 5 ^ 61 warning generated.
glozow force-pushed on Jul 24, 2024glozow marked this as ready for review on Jul 24, 2024in src/validation.cpp:3556 in 3b724e6163 outdated
3552@@ -3553,7 +3553,7 @@ bool Chainstate::ActivateBestChain(BlockValidationState& state, std::shared_ptr< 3553 } // release MempoolMutex 3554 // Notify external listeners about the new tip, even if pindexFork == pindexNewTip. 3555 if (m_chainman.m_options.signals && this == &m_chainman.ActiveChainstate()) { 3556- m_chainman.m_options.signals->ActiveTipChange(pindexNewTip, m_chainman.IsInitialBlockDownload()); 3557+ m_chainman.m_options.signals->ActiveTipChange(*pindexNewTip, m_chainman.IsInitialBlockDownload());
instagibbs commented at 2:02 pm on July 24, 2024:aside: we could throw an ~Assume()
~Assert()
on the pointer before deref, since it’s not set in a non-trivial way above
dergoegge commented at 2:21 pm on July 24, 2024:(If you’re gonna do this)Assert
would make more sense, since we’d never want to hit the deref if it failsinstagibbs approvedinstagibbs commented at 2:16 pm on July 24, 2024: memberDrahtBot requested review from dergoegge on Jul 24, 2024theStack approvedtheStack commented at 3:59 pm on July 24, 2024: contributorlgtm ACK 1b732dbc3bd9c59303a774cf2095c2d6639dbe1d (happy to re-ACK if theAssert
suggestion forpindexNewTip
is taken)[refactor] change ActiveTipChange to use CBlockIndex ref instead of ptr bce5f37c7brelease m_tx_download_mutex before MakeAndPushMessage GETDATA e543c657dam_tx_download_mutex followups
- add AssertLockNotHeld(m_tx_download_mutex) in net_processing - move doc about m_tx_download_mutex and mempool mutex to ActiveTipChange
glozow force-pushed on Jul 25, 2024instagibbs commented at 12:05 pm on July 25, 2024: memberDrahtBot requested review from theStack on Jul 25, 2024theStack approvedtheStack commented at 1:02 pm on July 25, 2024: contributorre-ACK 7c29e556c573a63351096c34bc072ae0c60ffa29dergoegge approveddergoegge commented at 1:06 pm on July 25, 2024: memberreACK 7c29e556c573a63351096c34bc072ae0c60ffa29fanquake merged this on Jul 25, 2024fanquake closed this on Jul 25, 2024
hebasto commented at 3:29 pm on July 25, 2024: memberPost-merge ACK 7c29e556c573a63351096c34bc072ae0c60ffa29.glozow deleted the branch on Jul 25, 2024
github-metadata-mirror
This is a metadata mirror of the GitHub repository bitcoin/bitcoin. This site is not affiliated with GitHub. Content is generated from a GitHub metadata backup.
generated: 2024-09-08 01:12 UTC
This site is hosted by @0xB10C
More mirrored repositories can be found on mirror.b10c.me