Fix ComputeTimeSmart test failure with -DDEBUG_LOCKORDER #12681

pull ryanofsky wants to merge 1 commits into bitcoin:master from ryanofsky:pr/locksmart changing 2 files +5 −7
  1. ryanofsky commented at 10:44 pm on March 13, 2018: member

    Failure looks like:

    0Entering test case "ComputeTimeSmart"
    1test_bitcoin: sync.cpp:100: void potential_deadlock_detected(const std::pair<void*, void*>&, const LockStack&, const LockStack&): Assertion `false' failed.
    2unknown location(0): fatal error in "ComputeTimeSmart": signal: SIGABRT (application abort requested)
    3wallet/test/wallet_tests.cpp(566): last checkpoint
    

    Reproducible with:

    0./configure --enable-debug
    1make -C src test/test_bitcoin && src/test/test_bitcoin --log_level=test_suite --run_test=wallet_tests/ComputeTimeSmart
    

    Seems to be caused by acquiring cs_main inside CWallet::ComputeTimeSmart in #11041.

    I think this may be causing timeouts on travis like: https://travis-ci.org/bitcoin/bitcoin/jobs/353005676#L2692

  2. MarcoFalke added the label Tests on Mar 13, 2018
  3. in src/wallet/wallet.cpp:902 in 84ae51a95c outdated
    898@@ -899,6 +899,7 @@ bool CWallet::MarkReplaced(const uint256& originalHash, const uint256& newHash)
    899 
    900 bool CWallet::AddToWallet(const CWalletTx& wtxIn, bool fFlushOnClose)
    901 {
    902+    AssertLockHeld(cs_main); // Needed for ComputeTimeSmart
    


    MarcoFalke commented at 11:06 pm on March 13, 2018:

    Wouldn’t it make sense to instead move this down to ComputeTimeSmart?

     0--- a/src/wallet/wallet.cpp
     1+++ b/src/wallet/wallet.cpp
     2@@ -899,7 +899,6 @@ bool CWallet::MarkReplaced(const uint256& originalHash, const uint256& newHash)
     3 
     4 bool CWallet::AddToWallet(const CWalletTx& wtxIn, bool fFlushOnClose)
     5 {
     6-    AssertLockHeld(cs_main); // Needed for ComputeTimeSmart
     7     LOCK(cs_wallet);
     8 
     9     CWalletDB walletdb(*dbw, "r+", fFlushOnClose);
    10@@ -3815,10 +3814,8 @@ unsigned int CWallet::ComputeTimeSmart(const CWalletTx& wtx) const
    11     unsigned int nTimeSmart = wtx.nTimeReceived;
    12     if (!wtx.hashUnset()) {
    13         const CBlockIndex* pindex = nullptr;
    14-        {
    15-            LOCK(cs_main);
    16-            pindex = LookupBlockIndex(wtx.hashBlock);
    17-        }
    18+        AssertLockHeld(cs_main);
    19+        pindex = LookupBlockIndex(wtx.hashBlock);
    20         if (pindex) {
    21             int64_t latestNow = wtx.nTimeReceived;
    22             int64_t latestEntry = 0;
    
  4. promag commented at 11:16 pm on March 13, 2018: member
    Actually I think it’s enough to fix the test and remove the lock. The lock assertion is already in LookupBlockIndex.
  5. Fix ComputeTimeSmart test failure with -DDEBUG_LOCKORDER
    Failure looks like:
    
        Entering test case "ComputeTimeSmart"
        test_bitcoin: sync.cpp:100: void potential_deadlock_detected(const std::pair<void*, void*>&, const LockStack&, const LockStack&): Assertion `false' failed.
        unknown location(0): fatal error in "ComputeTimeSmart": signal: SIGABRT (application abort requested)
        wallet/test/wallet_tests.cpp(566): last checkpoint
    
    Reproducible with:
    
        ./configure --enable-debug
        make -C src test/test_bitcoin && src/test/test_bitcoin --log_level=test_suite --run_test=wallet_tests/ComputeTimeSmart
    
    Happens due to "92fabcd443 Add LookupBlockIndex function" which acquires
    cs_main from inside CWallet::ComputeTimeSmart.
    33eb9071b9
  6. ryanofsky force-pushed on Mar 13, 2018
  7. ryanofsky commented at 11:45 pm on March 13, 2018: member

    Actually I think it’s enough to fix the test and remove the lock. The lock assertion is already in LookupBlockIndex.

    Sounds good. Removed the lock and new assert.

    Updated 84ae51a95c40e1e11520f41c1f8783504de694b3 -> 33eb9071b98e63fd66c1211254e420535320eebd (pr/locksmart.1 -> pr/locksmart.2)

  8. promag commented at 0:07 am on March 14, 2018: member

    utACK 33eb9071.

    Sorry for the bug, thanks @ryanofsky for the quick fix.

  9. MarcoFalke commented at 0:11 am on March 14, 2018: member
    utACK 33eb9071b98e63fd66c1211254e420535320eebd
  10. MarcoFalke merged this on Mar 14, 2018
  11. MarcoFalke closed this on Mar 14, 2018

  12. MarcoFalke referenced this in commit 18462960c0 on Mar 14, 2018
  13. UdjinM6 referenced this in commit 8c9b446ffd on Jul 29, 2020
  14. PastaPastaPasta referenced this in commit e659cdc8ee on Dec 12, 2020
  15. PastaPastaPasta referenced this in commit 93b5a2018d on Dec 12, 2020
  16. PastaPastaPasta referenced this in commit daa7b95794 on Dec 15, 2020
  17. PastaPastaPasta referenced this in commit c8476ea708 on Dec 15, 2020
  18. PastaPastaPasta referenced this in commit e723edb4e6 on Dec 15, 2020
  19. PastaPastaPasta referenced this in commit dad966e9fa on Dec 18, 2020
  20. MarcoFalke locked this on Sep 8, 2021

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-18 12:12 UTC

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