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
  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
  18. DrahtBot requested review from dergoegge on Jul 24, 2024
  19. theStack approved
  20. 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)
  21. [refactor] change ActiveTipChange to use CBlockIndex ref instead of ptr bce5f37c7b
  22. release m_tx_download_mutex before MakeAndPushMessage GETDATA e543c657da
  23. 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
  24. glozow force-pushed on Jul 25, 2024
  25. DrahtBot requested review from theStack on Jul 25, 2024
  26. theStack approved
  27. theStack commented at 1:02 pm on July 25, 2024: contributor
    re-ACK 7c29e556c573a63351096c34bc072ae0c60ffa29
  28. dergoegge approved
  29. dergoegge commented at 1:06 pm on July 25, 2024: member
    reACK 7c29e556c573a63351096c34bc072ae0c60ffa29
  30. fanquake merged this on Jul 25, 2024
  31. fanquake closed this on Jul 25, 2024

  32. hebasto commented at 3:29 pm on July 25, 2024: member
    Post-merge ACK 7c29e556c573a63351096c34bc072ae0c60ffa29.
  33. glozow deleted the branch on Jul 25, 2024
  34. Theghost256 approved

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-11-21 12:12 UTC

This site is hosted by @0xB10C
More mirrored repositories can be found on mirror.b10c.me