From 471426fb3b2c2fa37640c03819c4f7be69ba8301 Mon Sep 17 00:00:00 2001 From: Gavin Andresen Date: Wed, 31 Aug 2011 12:27:19 -0400 Subject: [PATCH] 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 | 44 ++++++ src/ui.cpp | 358 ++++++++++++++++++++++++------------------------- src/wallet.cpp | 16 --- 3 files changed, 223 insertions(+), 195 deletions(-) diff --git a/doc/coding.txt b/doc/coding.txt index 47074766..ec31ccde 100644 --- a/doc/coding.txt +++ b/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 diff --git a/src/ui.cpp b/src/ui.cpp index 867c9c0a..5ca66619 100644 --- a/src/ui.cpp +++ b/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,83 +1248,80 @@ 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(); + bool fWasLocked = pwalletMain->IsLocked(); + pwalletMain->Lock(); - if (!strOldWalletPass.size() || !pwalletMain->Unlock(strOldWalletPass)) - { - fill(strOldWalletPass.begin(), strOldWalletPass.end(), '\0'); - munlock(&strOldWalletPass[0], strOldWalletPass.capacity()); - wxMessageBox(_("The passphrase entered for the wallet decryption was incorrect."), "Bitcoin", wxOK | wxICON_ERROR); - return; - } + if (!strOldWalletPass.size() || !pwalletMain->Unlock(strOldWalletPass)) + { + fill(strOldWalletPass.begin(), strOldWalletPass.end(), '\0'); + munlock(&strOldWalletPass[0], strOldWalletPass.capacity()); + wxMessageBox(_("The passphrase entered for the wallet decryption was incorrect."), "Bitcoin", wxOK | wxICON_ERROR); + return; + } - if (fWasLocked) - pwalletMain->Lock(); + if (fWasLocked) + pwalletMain->Lock(); - string strNewWalletPass; - strNewWalletPass.reserve(100); - mlock(&strNewWalletPass[0], strNewWalletPass.capacity()); + string strNewWalletPass; + strNewWalletPass.reserve(100); + mlock(&strNewWalletPass[0], strNewWalletPass.capacity()); - // obtain new wallet encrypt/decrypt key, from passphrase - // Note that the passphrase is not mlock()d during this entry and could potentially - // be obtained from disk long after bitcoin has run. - strNewWalletPass = wxGetPasswordFromUser(_("Enter the new passphrase for the wallet."), - _("Passphrase")).ToStdString(); + // obtain new wallet encrypt/decrypt key, from passphrase + // Note that the passphrase is not mlock()d during this entry and could potentially + // be obtained from disk long after bitcoin has run. + strNewWalletPass = wxGetPasswordFromUser(_("Enter the new passphrase for the wallet."), + _("Passphrase")).ToStdString(); - if (!strNewWalletPass.size()) - { - fill(strOldWalletPass.begin(), strOldWalletPass.end(), '\0'); - fill(strNewWalletPass.begin(), strNewWalletPass.end(), '\0'); - munlock(&strOldWalletPass[0], strOldWalletPass.capacity()); - munlock(&strNewWalletPass[0], strNewWalletPass.capacity()); - wxMessageBox(_("Error: The supplied passphrase was too short."), "Bitcoin", wxOK | wxICON_ERROR); - return; - } + if (!strNewWalletPass.size()) + { + fill(strOldWalletPass.begin(), strOldWalletPass.end(), '\0'); + fill(strNewWalletPass.begin(), strNewWalletPass.end(), '\0'); + munlock(&strOldWalletPass[0], strOldWalletPass.capacity()); + munlock(&strNewWalletPass[0], strNewWalletPass.capacity()); + wxMessageBox(_("Error: The supplied passphrase was too short."), "Bitcoin", wxOK | wxICON_ERROR); + return; + } - string strNewWalletPassTest; - strNewWalletPassTest.reserve(100); - mlock(&strNewWalletPassTest[0], strNewWalletPassTest.capacity()); + string strNewWalletPassTest; + strNewWalletPassTest.reserve(100); + mlock(&strNewWalletPassTest[0], strNewWalletPassTest.capacity()); - // obtain new wallet encrypt/decrypt key, from passphrase - // Note that the passphrase is not mlock()d during this entry and could potentially - // be obtained from disk long after bitcoin has run. - strNewWalletPassTest = wxGetPasswordFromUser(_("Re-enter the new passphrase for the wallet."), - _("Passphrase")).ToStdString(); + // obtain new wallet encrypt/decrypt key, from passphrase + // Note that the passphrase is not mlock()d during this entry and could potentially + // be obtained from disk long after bitcoin has run. + strNewWalletPassTest = wxGetPasswordFromUser(_("Re-enter the new passphrase for the wallet."), + _("Passphrase")).ToStdString(); - if (strNewWalletPassTest != strNewWalletPass) - { - fill(strOldWalletPass.begin(), strOldWalletPass.end(), '\0'); - fill(strNewWalletPass.begin(), strNewWalletPass.end(), '\0'); - fill(strNewWalletPassTest.begin(), strNewWalletPassTest.end(), '\0'); - munlock(&strOldWalletPass[0], strOldWalletPass.capacity()); - munlock(&strNewWalletPass[0], strNewWalletPass.capacity()); - munlock(&strNewWalletPassTest[0], strNewWalletPassTest.capacity()); - wxMessageBox(_("Error: the supplied passphrases didn't match."), "Bitcoin", wxOK | wxICON_ERROR); - return; - } + if (strNewWalletPassTest != strNewWalletPass) + { + fill(strOldWalletPass.begin(), strOldWalletPass.end(), '\0'); + fill(strNewWalletPass.begin(), strNewWalletPass.end(), '\0'); + fill(strNewWalletPassTest.begin(), strNewWalletPassTest.end(), '\0'); + munlock(&strOldWalletPass[0], strOldWalletPass.capacity()); + munlock(&strNewWalletPass[0], strNewWalletPass.capacity()); + munlock(&strNewWalletPassTest[0], strNewWalletPassTest.capacity()); + wxMessageBox(_("Error: the supplied passphrases didn't match."), "Bitcoin", wxOK | wxICON_ERROR); + return; + } - if (!pwalletMain->ChangeWalletPassphrase(strOldWalletPass, strNewWalletPass)) - { - fill(strOldWalletPass.begin(), strOldWalletPass.end(), '\0'); - fill(strNewWalletPass.begin(), strNewWalletPass.end(), '\0'); - fill(strNewWalletPassTest.begin(), strNewWalletPassTest.end(), '\0'); - munlock(&strOldWalletPass[0], strOldWalletPass.capacity()); - munlock(&strNewWalletPass[0], strNewWalletPass.capacity()); - munlock(&strNewWalletPassTest[0], strNewWalletPassTest.capacity()); - wxMessageBox(_("The passphrase entered for the wallet decryption was incorrect."), "Bitcoin", wxOK | wxICON_ERROR); - return; - } + if (!pwalletMain->ChangeWalletPassphrase(strOldWalletPass, strNewWalletPass)) + { fill(strOldWalletPass.begin(), strOldWalletPass.end(), '\0'); fill(strNewWalletPass.begin(), strNewWalletPass.end(), '\0'); fill(strNewWalletPassTest.begin(), strNewWalletPassTest.end(), '\0'); munlock(&strOldWalletPass[0], strOldWalletPass.capacity()); munlock(&strNewWalletPass[0], strNewWalletPass.capacity()); munlock(&strNewWalletPassTest[0], strNewWalletPassTest.capacity()); - wxMessageBox(_("Wallet Passphrase Changed."), "Bitcoin"); + wxMessageBox(_("The passphrase entered for the wallet decryption was incorrect."), "Bitcoin", wxOK | wxICON_ERROR); + return; } + fill(strOldWalletPass.begin(), strOldWalletPass.end(), '\0'); + fill(strNewWalletPass.begin(), strNewWalletPass.end(), '\0'); + fill(strNewWalletPassTest.begin(), strNewWalletPassTest.end(), '\0'); + munlock(&strOldWalletPass[0], strOldWalletPass.capacity()); + munlock(&strNewWalletPass[0], strNewWalletPass.capacity()); + munlock(&strNewWalletPassTest[0], strNewWalletPassTest.capacity()); + wxMessageBox(_("Wallet Passphrase Changed."), "Bitcoin"); } void CMainFrame::OnMenuOptionsOptions(wxCommandEvent& event) @@ -1387,21 +1385,19 @@ void CMainFrame::OnButtonNew(wxCommandEvent& event) string strName = dialog.GetValue(); string strAddress; - CRITICAL_BLOCK(pwalletMain->cs_vMasterKey) - { - bool fWasLocked = pwalletMain->IsLocked(); - if (!GetWalletPassphrase()) - return; - // Generate new key - strAddress = CBitcoinAddress(pwalletMain->GetOrReuseKeyFromPool()).ToString(); + bool fWasLocked = pwalletMain->IsLocked(); + if (!GetWalletPassphrase()) + return; - if (fWasLocked) - pwalletMain->Lock(); - } + // Generate new key + strAddress = CBitcoinAddress(pwalletMain->GetOrReuseKeyFromPool()).ToString(); + + 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); @@ -2160,38 +2156,39 @@ void CSendDialog::OnButtonSend(wxCommandEvent& event) if (fBitcoinAddress) { + bool fWasLocked = pwalletMain->IsLocked(); + if (!GetWalletPassphrase()) + return; + + string strError; CRITICAL_BLOCK(cs_main) - CRITICAL_BLOCK(pwalletMain->cs_vMasterKey) + CRITICAL_BLOCK(pwalletMain->cs_wallet) { - bool fWasLocked = pwalletMain->IsLocked(); - if (!GetWalletPassphrase()) - return; - // Send to bitcoin address CScript scriptPubKey; scriptPubKey.SetBitcoinAddress(address); - string strError = pwalletMain->SendMoney(scriptPubKey, nValue, wtx, true); - if (strError == "") - wxMessageBox(_("Payment sent "), _("Sending...")); - else if (strError == "ABORTED") - { - if (fWasLocked) - pwalletMain->Lock(); - return; // leave send dialog open - } - else - { - wxMessageBox(strError + " ", _("Sending...")); - EndModal(false); - if (fWasLocked) - pwalletMain->Lock(); - return; - } - + strError = pwalletMain->SendMoney(scriptPubKey, nValue, wtx, true); + } + if (strError == "") + wxMessageBox(_("Payment sent "), _("Sending...")); + else if (strError == "ABORTED") + { if (fWasLocked) pwalletMain->Lock(); - } + return; // leave send dialog open + } + else + { + wxMessageBox(strError + " ", _("Sending...")); + EndModal(false); + if (fWasLocked) + pwalletMain->Lock(); + return; + } + + if (fWasLocked) + pwalletMain->Lock(); } else { @@ -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,83 +2461,89 @@ void CSendingDialog::OnReply2(CDataStream& vRecv) return; } - CRITICAL_BLOCK(cs_main) + // Pay + if (!Status(_("Creating transaction..."))) + return; + if (nPrice + nTransactionFee > pwalletMain->GetBalance()) { - // Pay - if (!Status(_("Creating transaction..."))) - return; - if (nPrice + nTransactionFee > pwalletMain->GetBalance()) - { - Error(_("Insufficient funds")); - return; - } + Error(_("Insufficient funds")); + return; + } - CReserveKey reservekey(pwalletMain); - int64 nFeeRequired; - CRITICAL_BLOCK(pwalletMain->cs_vMasterKey) - { - bool fWasLocked = pwalletMain->IsLocked(); - if (!GetWalletPassphrase()) - return; + CReserveKey reservekey(pwalletMain); + int64 nFeeRequired; + bool fWasLocked = pwalletMain->IsLocked(); + if (!GetWalletPassphrase()) + return; - if (!pwalletMain->CreateTransaction(scriptPubKey, nPrice, wtx, reservekey, nFeeRequired)) - { - 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())); - else - Error(_("Transaction creation failed")); - return; - } + 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())); + else + Error(_("Transaction creation failed")); + return; + } - if (fWasLocked) - pwalletMain->Lock(); - } + if (fWasLocked) + pwalletMain->Lock(); - // Transaction fee - if (!ThreadSafeAskFee(nFeeRequired, _("Sending..."), this)) - { - Error(_("Transaction aborted")); - return; - } + // Transaction fee + if (!ThreadSafeAskFee(nFeeRequired, _("Sending..."), this)) + { + Error(_("Transaction aborted")); + return; + } - // Make sure we're still connected - CNode* pnode = ConnectNode(addr, 2 * 60 * 60); - if (!pnode) - { - Error(_("Lost connection, transaction cancelled")); - return; - } + // Make sure we're still connected + CNode* pnode = ConnectNode(addr, 2 * 60 * 60); + if (!pnode) + { + Error(_("Lost connection, transaction cancelled")); + return; + } - // Last chance to cancel - Sleep(50); + // Last chance to cancel + Sleep(50); + if (!Status()) + return; + fCanCancel = false; + if (fAbort) + { + fCanCancel = true; if (!Status()) return; fCanCancel = false; - if (fAbort) - { - fCanCancel = true; - if (!Status()) - return; - fCanCancel = false; - } - if (!Status(_("Sending payment..."))) - return; + } + if (!Status(_("Sending payment..."))) + return; - // Commit - if (!pwalletMain->CommitTransaction(wtx, reservekey)) - { - 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; - } + // Commit + 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; + } - // Send payment tx to seller, with response going to OnReply3 via event handler - CWalletTx wtxSend = wtx; - wtxSend.fFromMe = false; - pnode->PushRequest("submitorder", wtxSend, SendingDialogOnReply3, this); + // Send payment tx to seller, with response going to OnReply3 via event handler + CWalletTx wtxSend = wtx; + wtxSend.fFromMe = false; + pnode->PushRequest("submitorder", wtxSend, SendingDialogOnReply3, this); - Status(_("Waiting for confirmation...")); - MainFrameRepaint(); - } + 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,22 +2821,19 @@ void CAddressBookDialog::OnButtonNew(wxCommandEvent& event) return; strName = dialog.GetValue(); - CRITICAL_BLOCK(pwalletMain->cs_vMasterKey) - { - bool fWasLocked = pwalletMain->IsLocked(); - if (!GetWalletPassphrase()) - return; + bool fWasLocked = pwalletMain->IsLocked(); + if (!GetWalletPassphrase()) + return; - // Generate new key - strAddress = CBitcoinAddress(pwalletMain->GetOrReuseKeyFromPool()).ToString(); + // Generate new key + strAddress = CBitcoinAddress(pwalletMain->GetOrReuseKeyFromPool()).ToString(); - if (fWasLocked) - pwalletMain->Lock(); - } + 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); diff --git a/src/wallet.cpp b/src/wallet.cpp index 745fbefd..1daec98d 100644 --- a/src/wallet.cpp +++ b/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::iterator mi = mapBlockIndex.find(hashBlock); - if (mi != mapBlockIndex.end()) - { - CBlockIndex* pindex = (*mi).second; - if (pindex) - return pindex->GetMedianTime(); - } - } - } return nTimeReceived; }