Fix ComputeTimeSmart test failure with -DDEBUG_LOCKORDER #12681

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

Contributors
ryanofsky MarcoFalke promag

Labels
Tests