m_tx_download_mutex followups

glozow commented at 12:36 pm on July 23, 2024: member

Followup to #30111. Includes suggestions:

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: member

Concept 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:

done

dergoegge approved

dergoegge commented at 8:24 am on July 24, 2024: member

ACK 1a0212c8e5c25fce2e0a2cab3113b25f2fbc38b7

DrahtBot requested review from hebasto on Jul 24, 2024

in 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.

6f49548670

[doc] TxOrphanage is no longer thread-safe 7cc5ac5a67

in 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 if ACQUIRED_BEFORE helps here at compile time

glozow commented at 11:51 am on July 24, 2024:

Hm, added a ACQUIRED_BEFORE, but compiler didn’t complain when I added a LOCK2(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(…) and ACQUIRED_AFTER(…) are currently being developed under the -Wthread-safety-beta flag.

Hm, added a ACQUIRED_BEFORE, but compiler didn’t complain when I added a LOCK2(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, 2024

glozow marked this as ready for review on Jul 24, 2024

in 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 fails

instagibbs approved

instagibbs commented at 2:16 pm on July 24, 2024: member

ACK https://github.com/bitcoin/bitcoin/pull/30507/commits/1b732dbc3bd9c59303a774cf2095c2d6639dbe1d

DrahtBot requested review from dergoegge on Jul 24, 2024

theStack approved

theStack commented at 3:59 pm on July 24, 2024: contributor

lgtm ACK 1b732dbc3bd9c59303a774cf2095c2d6639dbe1d (happy to re-ACK if the Assert suggestion for pindexNewTip is taken)

[refactor] change ActiveTipChange to use CBlockIndex ref instead of ptr bce5f37c7b

release m_tx_download_mutex before MakeAndPushMessage GETDATA e543c657da

m_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

7c29e556c5

glozow force-pushed on Jul 25, 2024

instagibbs commented at 12:05 pm on July 25, 2024: member

reACK https://github.com/bitcoin/bitcoin/pull/30507/commits/7c29e556c573a63351096c34bc072ae0c60ffa29

DrahtBot requested review from theStack on Jul 25, 2024

theStack approved

theStack commented at 1:02 pm on July 25, 2024: contributor

re-ACK 7c29e556c573a63351096c34bc072ae0c60ffa29

dergoegge approved

dergoegge commented at 1:06 pm on July 25, 2024: member

reACK 7c29e556c573a63351096c34bc072ae0c60ffa29

fanquake merged this on Jul 25, 2024

fanquake closed this on Jul 25, 2024

hebasto commented at 3:29 pm on July 25, 2024: member

Post-merge ACK 7c29e556c573a63351096c34bc072ae0c60ffa29.

glozow deleted the branch on Jul 25, 2024

m_tx_download_mutex followups #30507

Code Coverage

Reviews

Conflicts