Fixed potential deadlocks in GUI code.

Also changed semantics of CWalletTx::GetTxTime(); now always returns the time the transaction was received by this node, not the average block time.
And added information about -DDEBUG_LOCKORDER to coding.txt.

doc/coding.txt

@@ -39,3 +39,47 @@ v       vector or similar list objects
 map     map or multimap
 set     set or multiset
 bn      CBigNum
+
+-------------------------
+Locking/mutex usage notes
+
+The code is multi-threaded, and uses mutexes and the CRITICAL_BLOCK/TRY_CRITICAL_BLOCK macros to protect data structures.
+
+Deadlocks due to inconsistent lock ordering (thread 1 locks cs_main and then cs_wallet, while thread 2 locks them in the opposite order: result, deadlock as each waits for the other to release its lock) are a problem. Compile with -DDEBUG_LOCKORDER to get lock order inconsistencies reported in the debug.log file.
+
+Re-architecting the core code so there are better-defined interfaces between the various components is a goal, with any necessary locking done by the components (e.g. see the self-contained CKeyStore class and its cs_KeyStore lock for example).
+
+-------
+Threads
+
+StartNode : Starts other threads.
+
+ThreadGetMyExternalIP : Determines outside-the-firewall IP address, sends addr message to connected peers when it determines it.
+
+ThreadIRCSeed : Joins IRC bootstrapping channel, watching for new peers and advertising this node's IP address.
+
+ThreadSocketHandler : Sends/Receives data from peers on port 8333.
+
+ThreadMessageHandler : Higher-level message handling (sending and receiving).
+
+ThreadOpenConnections : Initiates new connections to peers.
+
+ThreadTopUpKeyPool : replenishes the keystore's keypool.
+
+ThreadCleanWalletPassphrase : re-locks an encrypted wallet after user has unlocked it for a period of time.
+
+SendingDialogStartTransfer : used by pay-via-ip-address code (obsolete)
+
+ThreadDelayedRepaint : repaint the gui 
+
+ThreadFlushWalletDB : Close the wallet.dat file if it hasn't been used in 500ms.
+
+ThreadRPCServer : Remote procedure call handler, listens on port 8332 for connections and services them.
+
+ThreadBitcoinMiner : Generates bitcoins
+
+ThreadMapPort : Universal plug-and-play startup/shutdown
+
+Shutdown : Does an orderly shutdown of everything
+
+ExitTimeout : Windows-only, sleeps 5 seconds then exits application

src/ui.cpp

@@ -708,7 +708,7 @@ bool CMainFrame::InsertTransaction(const CWalletTx& wtx, bool fNew, int nIndex)
                     CBitcoinAddress address;
                     if (ExtractAddress(txout.scriptPubKey, pwalletMain, address))
                     {
-                        CRITICAL_BLOCK(pwalletMain->cs_mapAddressBook)
+                        CRITICAL_BLOCK(pwalletMain->cs_wallet)
                         {
                             //strDescription += _("Received payment to ");
                             //strDescription += _("Received with address ");
@@ -792,7 +792,7 @@ bool CMainFrame::InsertTransaction(const CWalletTx& wtx, bool fNew, int nIndex)
                 }
 
                 string strDescription = _("To: ");
-                CRITICAL_BLOCK(pwalletMain->cs_mapAddressBook)
+                CRITICAL_BLOCK(pwalletMain->cs_wallet)
                     if (pwalletMain->mapAddressBook.count(address) && !pwalletMain->mapAddressBook[address].empty())
                         strDescription += pwalletMain->mapAddressBook[address] + " ";
                 strDescription += strAddress;
@@ -1032,6 +1032,7 @@ void MainFrameRepaint()
         printf("MainFrameRepaint\n");
         wxPaintEvent event;
         pframeMain->fRefresh = true;
+        pframeMain->fRefreshListCtrl = true;
         pframeMain->GetEventHandler()->AddPendingEvent(event);
     }
 }
@@ -1247,8 +1248,6 @@ void CMainFrame::OnMenuOptionsChangeWalletPassphrase(wxCommandEvent& event)
     strOldWalletPass = wxGetPasswordFromUser(_("Enter the current passphrase to the wallet."),
                                              _("Passphrase")).ToStdString();
 
-    CRITICAL_BLOCK(pwalletMain->cs_vMasterKey)
-    {
     bool fWasLocked = pwalletMain->IsLocked();
     pwalletMain->Lock();
 
@@ -1324,7 +1323,6 @@ void CMainFrame::OnMenuOptionsChangeWalletPassphrase(wxCommandEvent& event)
     munlock(&strNewWalletPassTest[0], strNewWalletPassTest.capacity());
     wxMessageBox(_("Wallet Passphrase Changed."), "Bitcoin");
 }
-}
 
 void CMainFrame::OnMenuOptionsOptions(wxCommandEvent& event)
 {
@@ -1387,8 +1385,7 @@ void CMainFrame::OnButtonNew(wxCommandEvent& event)
     string strName = dialog.GetValue();
 
     string strAddress;
-    CRITICAL_BLOCK(pwalletMain->cs_vMasterKey)
+
-    {
     bool fWasLocked = pwalletMain->IsLocked();
     if (!GetWalletPassphrase())
         return;
@@ -1398,10 +1395,9 @@ void CMainFrame::OnButtonNew(wxCommandEvent& event)
 
     if (fWasLocked)
         pwalletMain->Lock();
-    }
 
     // Save
-    CRITICAL_BLOCK(pwalletMain->cs_mapAddressBook)
+    CRITICAL_BLOCK(pwalletMain->cs_wallet)
         pwalletMain->SetAddressBookName(strAddress, strName);
     SetDefaultReceivingAddress(strAddress);
 }
@@ -1451,7 +1447,7 @@ CTxDetailsDialog::CTxDetailsDialog(wxWindow* parent, CWalletTx wtx) : CTxDetails
 #ifdef __WXMSW__
     SetSize(nScaleX * GetSize().GetWidth(), nScaleY * GetSize().GetHeight());
 #endif
-    CRITICAL_BLOCK(pwalletMain->cs_mapAddressBook)
+    CRITICAL_BLOCK(pwalletMain->cs_wallet)
     {
         string strHTML;
         strHTML.reserve(4000);
@@ -2159,19 +2155,21 @@ void CSendDialog::OnButtonSend(wxCommandEvent& event)
         bool fBitcoinAddress = address.IsValid();
 
         if (fBitcoinAddress)
-        {
-	    CRITICAL_BLOCK(cs_main)
-            CRITICAL_BLOCK(pwalletMain->cs_vMasterKey)
         {
             bool fWasLocked = pwalletMain->IsLocked();
             if (!GetWalletPassphrase())
                 return;
 
+            string strError;
+	    CRITICAL_BLOCK(cs_main)
+            CRITICAL_BLOCK(pwalletMain->cs_wallet)
+	    {
                 // Send to bitcoin address
                 CScript scriptPubKey;
                 scriptPubKey.SetBitcoinAddress(address);
 
-                string strError = pwalletMain->SendMoney(scriptPubKey, nValue, wtx, true);
+                strError = pwalletMain->SendMoney(scriptPubKey, nValue, wtx, true);
+            }
             if (strError == "")
                 wxMessageBox(_("Payment sent  "), _("Sending..."));
             else if (strError == "ABORTED")
@@ -2192,7 +2190,6 @@ void CSendDialog::OnButtonSend(wxCommandEvent& event)
             if (fWasLocked)
                 pwalletMain->Lock();
         }
-        }
         else
         {
             // Parse IP address
@@ -2212,7 +2209,7 @@ void CSendDialog::OnButtonSend(wxCommandEvent& event)
                 return;
         }
 
-        CRITICAL_BLOCK(pwalletMain->cs_mapAddressBook)
+        CRITICAL_BLOCK(pwalletMain->cs_wallet)
             if (!pwalletMain->mapAddressBook.count(address))
                 pwalletMain->SetAddressBookName(strAddress, "");
 
@@ -2464,8 +2461,6 @@ void CSendingDialog::OnReply2(CDataStream& vRecv)
             return;
     }
 
-    CRITICAL_BLOCK(cs_main)
-    {
     // Pay
     if (!Status(_("Creating transaction...")))
         return;
@@ -2477,13 +2472,17 @@ void CSendingDialog::OnReply2(CDataStream& vRecv)
 
     CReserveKey reservekey(pwalletMain);
     int64 nFeeRequired;
-        CRITICAL_BLOCK(pwalletMain->cs_vMasterKey)
-        {
     bool fWasLocked = pwalletMain->IsLocked();
     if (!GetWalletPassphrase())
         return;
 
-            if (!pwalletMain->CreateTransaction(scriptPubKey, nPrice, wtx, reservekey, nFeeRequired))
+    bool fTxCreated = false;
+    CRITICAL_BLOCK(cs_main)
+    CRITICAL_BLOCK(pwalletMain->cs_wallet)
+    {
+        fTxCreated = pwalletMain->CreateTransaction(scriptPubKey, nPrice, wtx, reservekey, nFeeRequired);
+    }
+    if (!fTxCreated)
     {
         if (nPrice + nFeeRequired > pwalletMain->GetBalance())
             Error(strprintf(_("This transaction requires a transaction fee of at least %s because of its amount, complexity, or use of recently received funds"), FormatMoney(nFeeRequired).c_str()));
@@ -2494,7 +2493,6 @@ void CSendingDialog::OnReply2(CDataStream& vRecv)
 
     if (fWasLocked)
         pwalletMain->Lock();
-       }
 
     // Transaction fee
     if (!ThreadSafeAskFee(nFeeRequired, _("Sending..."), this))
@@ -2527,7 +2525,13 @@ void CSendingDialog::OnReply2(CDataStream& vRecv)
         return;
 
     // Commit
-        if (!pwalletMain->CommitTransaction(wtx, reservekey))
+    bool fTxCommitted = false;
+    CRITICAL_BLOCK(cs_main)
+    CRITICAL_BLOCK(pwalletMain->cs_wallet)
+    {
+        fTxCommitted = pwalletMain->CommitTransaction(wtx, reservekey);
+    }
+    if (!fTxCommitted)
     {
         Error(_("The transaction was rejected.  This might happen if some of the coins in your wallet were already spent, such as if you used a copy of wallet.dat and coins were spent in the copy but not marked as spent here."));
         return;
@@ -2541,7 +2545,6 @@ void CSendingDialog::OnReply2(CDataStream& vRecv)
     Status(_("Waiting for confirmation..."));
     MainFrameRepaint();
 }
-}
 
 void SendingDialogOnReply3(void* parg, CDataStream& vRecv)
 {
@@ -2621,7 +2624,7 @@ CAddressBookDialog::CAddressBookDialog(wxWindow* parent, const wxString& strInit
     m_listCtrlReceiving->SetFocus();
 
     // Fill listctrl with address book data
-    CRITICAL_BLOCK(pwalletMain->cs_mapAddressBook)
+    CRITICAL_BLOCK(pwalletMain->cs_wallet)
     {
         string strDefaultReceiving = (string)pframeMain->m_textCtrlAddress->GetValue();
         BOOST_FOREACH(const PAIRTYPE(CBitcoinAddress, string)& item, pwalletMain->mapAddressBook)
@@ -2682,7 +2685,7 @@ void CAddressBookDialog::OnListEndLabelEdit(wxListEvent& event)
     if (event.IsEditCancelled())
         return;
     string strAddress = (string)GetItemText(m_listCtrl, event.GetIndex(), 1);
-    CRITICAL_BLOCK(pwalletMain->cs_mapAddressBook)
+    CRITICAL_BLOCK(pwalletMain->cs_wallet)
         pwalletMain->SetAddressBookName(strAddress, string(event.GetText()));
     pframeMain->RefreshListCtrl();
 }
@@ -2718,7 +2721,7 @@ void CAddressBookDialog::OnButtonDelete(wxCommandEvent& event)
         if (m_listCtrl->GetItemState(nIndex, wxLIST_STATE_SELECTED))
         {
             string strAddress = (string)GetItemText(m_listCtrl, nIndex, 1);
-            CRITICAL_BLOCK(pwalletMain->cs_mapAddressBook)
+            CRITICAL_BLOCK(pwalletMain->cs_wallet)
                 pwalletMain->DelAddressBookName(strAddress);
             m_listCtrl->DeleteItem(nIndex);
         }
@@ -2778,7 +2781,7 @@ void CAddressBookDialog::OnButtonEdit(wxCommandEvent& event)
     }
 
     // Write back
-    CRITICAL_BLOCK(pwalletMain->cs_mapAddressBook)
+    CRITICAL_BLOCK(pwalletMain->cs_wallet)
     {
         if (strAddress != strAddressOrg)
             pwalletMain->DelAddressBookName(strAddressOrg);
@@ -2818,8 +2821,6 @@ void CAddressBookDialog::OnButtonNew(wxCommandEvent& event)
             return;
         strName = dialog.GetValue();
 
-        CRITICAL_BLOCK(pwalletMain->cs_vMasterKey)
-        {
         bool fWasLocked = pwalletMain->IsLocked();
         if (!GetWalletPassphrase())
             return;
@@ -2830,10 +2831,9 @@ void CAddressBookDialog::OnButtonNew(wxCommandEvent& event)
         if (fWasLocked)
             pwalletMain->Lock();
     }
-    }
 
     // Add to list and select it
-    CRITICAL_BLOCK(pwalletMain->cs_mapAddressBook)
+    CRITICAL_BLOCK(pwalletMain->cs_wallet)
         pwalletMain->SetAddressBookName(strAddress, strName);
     int nIndex = InsertLine(m_listCtrl, strName, strAddress);
     SetSelection(m_listCtrl, nIndex);

src/wallet.cpp

@@ -353,22 +353,6 @@ int64 CWallet::GetDebit(const CTxIn &txin) const
 
 int64 CWalletTx::GetTxTime() const
 {
-    CRITICAL_BLOCK(cs_main)
-    {
-        if (!fTimeReceivedIsTxTime && hashBlock != 0)
-        {
-            // If we did not receive the transaction directly, we rely on the block's
-            // time to figure out when it happened.  We use the median over a range
-            // of blocks to try to filter out inaccurate block times.
-            map<uint256, CBlockIndex*>::iterator mi = mapBlockIndex.find(hashBlock);
-            if (mi != mapBlockIndex.end())
-            {
-                CBlockIndex* pindex = (*mi).second;
-                if (pindex)
-                    return pindex->GetMedianTime();
-            }
-        }
-    }
     return nTimeReceived;
 }