Deadlock in key generation due to CCryptoKeyStore #453

From the forum: https://bitcointalk.org/index.php?topic=34642.0

This is in the latest version in Git. This is not in the released client.

Creating a new key can freeze the client. It appears to be much more frequent when there are blocks and transactions to download.

I've been digging in the code to try and find the cause, and I think this is the reason:

CAddressBookDialog::OnButtonNew grabs the cs_vMasterKey, then calls pwalletMain->GetOrReuseKeyFromPool() GetOrReuseKeyFromPool() calls ReserveKeyFromKeyPool() which attempts to grab cs_main

Meanwhile..

ProcessMessages grabs cs_main and then calls ProcessMessage ProcessMessage() calls SyncWithWallets, either directly when receiving "tx" or indirectly when receiving a block (via ProcessBlock -> AcceptBlock -> AddToBlockIndex -> SetBestChain -> ConnectBlock -> SyncWithWallets) SyncWithWallets calls AddToWalletIfInvolvingMe AddToWalletIfInvolvingMe calls CWallet::IsMine, which calls the global ::IsMine IsMine(const CKeyStore &keystore, const CScript& scriptPubKey) then calls keystore.GetPubKey CCryptoKeyStore::GetPubKey then grabs cs_vMasterKey

Deadlock.

Most of the critical sections protect something small well-defined, though I'm not quite sure what cs_main is supposed to be protecting.

I would generate a pull request, except that

1. I have an ugly hack but not a good fix. I'm not sure what the final strategy is going to be on wallet encryption, and whether the public keys are going to be encrypted too.
2. I am new to git and I am not yet skilled at rebasing commits to include only the changes I want.

Ah ok this is clearly a better place to post issues. Thank you.

I think the entire locking system may need to be revised in general, it is very hard currently to find this kind of bugs. Your analysis looks correct to be though, but wouldn't locking cs_main before cs_vMasterKey in CAddressBookDialog be sufficient?

If you lock it in CAddressBookDialog, as long as the password dialog box is open, the entire cs_main is locked, which is quite a problem (as nothing else important will work during that time)...so I'd say not really.

I think the locking system is fine as long as each lock is logically tied to a relatively small subsystem with few dependencies. This is already true for the most part. I think the problem here is cs_main, because it is not at all obvious what it is trying to lock.

One thing that would help an awful lot is some documentation explaining the intended locking hierarchy. Presumably, there is some intended order in which locks are acquired. If we know what that hierarchy is supposed to be, we can easily add test code to confirm that the hierarchy is not violated. Also, when it is, it will be obvious which section of code is at fault and (usually) how to fix it. (By acquiring the last lock sooner in the code path, usually.)

I think the current locks can be divided into three components: 1) main/blockdb/mempool, 2) wallet/keystore, 3) network

Within each component the order of locks can be fixed, and easily checked. However, between the components there are also dependencies. As both wallet and network request and submit information from main, I'd say 2>1 and 3>1. Wallet code also depends on the network, so 2>3. That gives us a total ordering on the locks: first wallet, then network, then main. This implies that you can call net and main functions while having a lock on wallet, and that you can call main while having a lock on network.

However, this also means it is not allowed to call (possibly locking) code in wallet while having a lock on main. That's exactly what I caused by introducing the wallet-dispatch functions in main (IsFromMe, GetTransaction, EraseFromWallets, SyncWithWallets, ...). The best solution, i suppose, would be to modify these callbacks to only happen outside of main-locked code, and only pass identifiers (by value) to them, as passing references to blocks may mean exposing data that needs locking, and can change while being inspected.

I'll be commiting lock order inconsistency detection code tomorrow (I need to rebase some code and make it #ifdef DEBUG_LOCKORDER). RE: Joel's suggestion on documentation: Good Idea. It might be a good idea to give each CCriticalSection an integer 'depth', and make it a run-time error to lock out of 'in-depth' order.