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
  1. glozow commented at 12:36 pm on July 23, 2024: member

    Followup to #30111. Includes suggestions:

  2. glozow added the label Refactoring on Jul 23, 2024
  3. glozow added the label Docs on Jul 23, 2024
  4. 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.

  5. hebasto commented at 7:15 am on July 24, 2024: member
    Concept ACK.
  6. 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
  7. dergoegge approved
  8. dergoegge commented at 8:24 am on July 24, 2024: member
    ACK 1a0212c8e5c25fce2e0a2cab3113b25f2fbc38b7
  9. DrahtBot requested review from hebasto on Jul 24, 2024
  10. 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
  11. [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
  12. [doc] TxOrphanage is no longer thread-safe 7cc5ac5a67
  13. 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.
  14. glozow force-pushed on Jul 24, 2024
  15. glozow marked this as ready for review on Jul 24, 2024
  16. 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
  17. instagibbs approved
  19. DrahtBot requested review from dergoegge on Jul 24, 2024
  20. theStack approved
  21. 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)
  22. [refactor] change ActiveTipChange to use CBlockIndex ref instead of ptr bce5f37c7b
  23. release m_tx_download_mutex before MakeAndPushMessage GETDATA e543c657da
  24. 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
  25. glozow force-pushed on Jul 25, 2024
  27. DrahtBot requested review from theStack on Jul 25, 2024
  28. theStack approved
  29. theStack commented at 1:02 pm on July 25, 2024: contributor
    re-ACK 7c29e556c573a63351096c34bc072ae0c60ffa29
  30. dergoegge approved
  31. dergoegge commented at 1:06 pm on July 25, 2024: member
    reACK 7c29e556c573a63351096c34bc072ae0c60ffa29
  32. fanquake merged this on Jul 25, 2024
  33. fanquake closed this on Jul 25, 2024
  34. hebasto commented at 3:29 pm on July 25, 2024: member
    Post-merge ACK 7c29e556c573a63351096c34bc072ae0c60ffa29.
  35. glozow deleted the branch on Jul 25, 2024
  36. Theghost256 approved


glozow DrahtBot hebasto dergoegge maflcko instagibbs theStack Theghost256

Labels
Refactoring Docs

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-12-22 00:12 UTC
This site is hosted by @0xB10C
More mirrored repositories can be found on mirror.b10c.me