wallet: Assert holding cs_wallet lock in GetAvailableCredit/GetAvailableWatchOnlyCredit [wip] #11591

practicalswift commented at 1:26 PM on November 1, 2017: contributor

Note: Still investigating. WIP.

~~Add missing cs_wallet locks in GetAvailableCredit, GetAvailableWatchOnlyCredit and CreateWalletFromFile:~~

~~GetAvailableCredit and GetAvailableWatchOnlyCredit read the variables mapTxSpends and mapWallet via the call to IsSpent(...) without holding cs_wallet.~~
~~CreateWalletFromFile reads the variable nTimeFirstKey without holding cs_wallet.~~

fanquake added the label Wallet on Nov 1, 2017

practicalswift renamed this:
~~wallet: Add missing cs_wallet locks in GetAvailableCredit/GetAvailableWatchOnlyCredit/CreateWalletFromFile~~
wallet: Add missing cs_wallet locks in GetAvailable{,WatchOnly}Credit and CreateWalletFromFile
on Nov 1, 2017

MarcoFalke commented at 4:12 PM on November 1, 2017: member

This fails with

bitcoind: sync.cpp:98: void potential_deadlock_detected(const std::pair<void*, void*>&, const LockStack&, const LockStack&): Assertion `false' failed.

You might be able to compile with -DDEBUG_LOCKORDER locally and get a verbose output.

practicalswift commented at 4:43 PM on November 1, 2017: contributor

@MarcoFalke Thanks! I'll investigate!

practicalswift renamed this:
~~wallet: Add missing cs_wallet locks in GetAvailable{,WatchOnly}Credit and CreateWalletFromFile~~
wallet: Add missing cs_wallet locks in GetAvailable{,WatchOnly}Credit and CreateWalletFromFile [WIP]
on Nov 1, 2017

in src/wallet/wallet.cpp:3940 in a01b1e50fc outdated

3936 | @@ -3935,6 +3937,7 @@ CWallet* CWallet::CreateWalletFromFile(const std::string walletFile)
3937 |  
3938 |          // No need to read and scan block if block was created before
3939 |          // our wallet birthday (as adjusted for block time variability)
3940 | +        LOCK(walletInstance->cs_wallet);

practicalswift commented at 5:43 PM on November 2, 2017:

This lock causes problems. Removing temporarily. I'll investigate.

practicalswift force-pushed on Nov 2, 2017

in src/wallet/wallet.cpp:1803 in 3150a6ee0e outdated

1799 | @@ -1800,6 +1800,7 @@ CAmount CWalletTx::GetAvailableCredit(bool fUseCache) const
1800 |      uint256 hashTx = GetHash();
1801 |      for (unsigned int i = 0; i < tx->vout.size(); i++)
1802 |      {
1803 | +        LOCK(pwallet->cs_wallet);

promag commented at 2:23 PM on November 3, 2017:

Callers of GetAvailableCredit already have cs_wallet locked.

in src/wallet/wallet.cpp:1847 in 3150a6ee0e outdated

1843 | @@ -1843,6 +1844,7 @@ CAmount CWalletTx::GetAvailableWatchOnlyCredit(const bool& fUseCache) const
1844 |      CAmount nCredit = 0;
1845 |      for (unsigned int i = 0; i < tx->vout.size(); i++)
1846 |      {
1847 | +        LOCK(pwallet->cs_wallet);

promag commented at 2:24 PM on November 3, 2017:

Callers of GetAvailableWatchOnlyCredit already have cs_wallet locked.

promag commented at 2:24 PM on November 3, 2017: member

Maybe just assert the lock is held?

wallet: Assert holding cs_wallet locks in GetAvailableCredit/GetAvailableWatchOnlyCredit

GetAvailableCredit and GetAvailableWatchOnlyCredit read the variables
mapTxSpends and mapWallet via the call to IsSpent(...). These are
guarded by cs_wallet.

58df673831

practicalswift force-pushed on Nov 3, 2017

practicalswift renamed this:
~~wallet: Add missing cs_wallet locks in GetAvailable{,WatchOnly}Credit and CreateWalletFromFile [WIP]~~
wallet: Assert holding cs_wallet lock in GetAvailableCredit/GetAvailableWatchOnlyCredit
on Nov 3, 2017

practicalswift commented at 3:30 PM on November 3, 2017: contributor

@promag Oh, you're right that the callers are locking properly - my annotations fooled me here. Thanks for reviewing. I've now changed to AssertLockHeld(…).

Would you mind taking a look at the potential deadlock (reported when building with -DDEBUG_LOCKORDER) discussed in #11591 (review)? The potential deadlock is not triggered when doing make check but test/functional/test_runner.py triggers it.

practicalswift renamed this:
~~wallet: Assert holding cs_wallet lock in GetAvailableCredit/GetAvailableWatchOnlyCredit~~
wallet: Assert holding cs_wallet lock in GetAvailableCredit/GetAvailableWatchOnlyCredit [wip]
on Nov 3, 2017

practicalswift closed this on Nov 3, 2017

practicalswift deleted the branch on Apr 10, 2021

DrahtBot locked this on Aug 16, 2022