From f5f1878ba104a5d6a32eeb20b341326dca6086a5 Mon Sep 17 00:00:00 2001 From: Jeff Garzik Date: Mon, 4 Apr 2011 22:24:35 -0400 Subject: [PATCH 1/2] Fix deadlocks in setaccount, sendfrom RPC calls SendMoney*() now requires caller to acquire cs_main. GetAccountAddress() now requires caller to acquire cs_main, cs_mapWallet. Ordering is intended to match these two callchains[1]: 1. CRITICAL_BLOCK(cs_main) ProcessMessage(pfrom, strCommand, vMsg) AddToWalletIfMine() AddToWallet(wtx) CRITICAL_BLOCK(cs_mapWallet) 2. CRITICAL_BLOCK(cs_main) ProcessMessage(pfrom, strCommand, vMsg) AddToWalletIfMine() AddToWallet(wtx) CRITICAL_BLOCK(cs_mapWallet) walletdb.WriteName(PubKeyToAddress(vchDefaultKey), "") CRITICAL_BLOCK(cs_mapAddressBook) Spotted by ArtForz. Additional deadlock fixes by Gavin. [1] http://www.bitcoin.org/smf/index.php?topic=4904.msg71897#msg71897 --- db.cpp | 2 +- main.cpp | 36 ++++++++++++------------- rpc.cpp | 82 +++++++++++++++++++++++++++++++++----------------------- ui.cpp | 31 +++++++++++---------- 4 files changed, 84 insertions(+), 67 deletions(-) diff --git a/db.cpp b/db.cpp index aaa997be..3cfcce56 100644 --- a/db.cpp +++ b/db.cpp @@ -668,8 +668,8 @@ bool CWalletDB::LoadWallet() #endif //// todo: shouldn't we catch exceptions and try to recover and continue? - CRITICAL_BLOCK(cs_mapKeys) CRITICAL_BLOCK(cs_mapWallet) + CRITICAL_BLOCK(cs_mapKeys) { // Get cursor Dbc* pcursor = GetCursor(); diff --git a/main.cpp b/main.cpp index bfc45af2..6bd90a32 100644 --- a/main.cpp +++ b/main.cpp @@ -4046,35 +4046,35 @@ bool CommitTransaction(CWalletTx& wtxNew, CReserveKey& reservekey) +// requires cs_main lock string SendMoney(CScript scriptPubKey, int64 nValue, CWalletTx& wtxNew, bool fAskFee) { - CRITICAL_BLOCK(cs_main) + CReserveKey reservekey; + int64 nFeeRequired; + if (!CreateTransaction(scriptPubKey, nValue, wtxNew, reservekey, nFeeRequired)) { - CReserveKey reservekey; - int64 nFeeRequired; - if (!CreateTransaction(scriptPubKey, nValue, wtxNew, reservekey, nFeeRequired)) - { - string strError; - if (nValue + nFeeRequired > GetBalance()) - strError = strprintf(_("Error: This is an oversized transaction that requires a transaction fee of %s "), FormatMoney(nFeeRequired).c_str()); - else - strError = _("Error: Transaction creation failed "); - printf("SendMoney() : %s", strError.c_str()); - return strError; - } + string strError; + if (nValue + nFeeRequired > GetBalance()) + strError = strprintf(_("Error: This is an oversized transaction that requires a transaction fee of %s "), FormatMoney(nFeeRequired).c_str()); + else + strError = _("Error: Transaction creation failed "); + printf("SendMoney() : %s", strError.c_str()); + return strError; + } - if (fAskFee && !ThreadSafeAskFee(nFeeRequired, _("Sending..."), NULL)) - return "ABORTED"; + if (fAskFee && !ThreadSafeAskFee(nFeeRequired, _("Sending..."), NULL)) + return "ABORTED"; + + if (!CommitTransaction(wtxNew, reservekey)) + return _("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."); - if (!CommitTransaction(wtxNew, reservekey)) - return _("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."); - } MainFrameRepaint(); return ""; } +// requires cs_main lock string SendMoneyToBitcoinAddress(string strAddress, int64 nValue, CWalletTx& wtxNew, bool fAskFee) { // Check amount diff --git a/rpc.cpp b/rpc.cpp index 97710ff6..a634fff6 100644 --- a/rpc.cpp +++ b/rpc.cpp @@ -315,46 +315,45 @@ Value getnewaddress(const Array& params, bool fHelp) } +// requires cs_main, cs_mapWallet locks string GetAccountAddress(string strAccount, bool bForceNew=false) { string strAddress; - CRITICAL_BLOCK(cs_mapWallet) - { - CWalletDB walletdb; - walletdb.TxnBegin(); + CWalletDB walletdb; + walletdb.TxnBegin(); - CAccount account; - walletdb.ReadAccount(strAccount, account); + CAccount account; + walletdb.ReadAccount(strAccount, account); - // Check if the current key has been used - if (!account.vchPubKey.empty()) - { - CScript scriptPubKey; - scriptPubKey.SetBitcoinAddress(account.vchPubKey); - for (map::iterator it = mapWallet.begin(); - it != mapWallet.end() && !account.vchPubKey.empty(); - ++it) - { - const CWalletTx& wtx = (*it).second; - foreach(const CTxOut& txout, wtx.vout) - if (txout.scriptPubKey == scriptPubKey) - account.vchPubKey.clear(); - } - } - - // Generate a new key - if (account.vchPubKey.empty() || bForceNew) + // Check if the current key has been used + if (!account.vchPubKey.empty()) + { + CScript scriptPubKey; + scriptPubKey.SetBitcoinAddress(account.vchPubKey); + for (map::iterator it = mapWallet.begin(); + it != mapWallet.end() && !account.vchPubKey.empty(); + ++it) { - account.vchPubKey = GetKeyFromKeyPool(); - string strAddress = PubKeyToAddress(account.vchPubKey); - SetAddressBookName(strAddress, strAccount); - walletdb.WriteAccount(strAccount, account); + const CWalletTx& wtx = (*it).second; + foreach(const CTxOut& txout, wtx.vout) + if (txout.scriptPubKey == scriptPubKey) + account.vchPubKey.clear(); } + } - walletdb.TxnCommit(); - strAddress = PubKeyToAddress(account.vchPubKey); + // Generate a new key + if (account.vchPubKey.empty() || bForceNew) + { + account.vchPubKey = GetKeyFromKeyPool(); + string strAddress = PubKeyToAddress(account.vchPubKey); + SetAddressBookName(strAddress, strAccount); + walletdb.WriteAccount(strAccount, account); } + + walletdb.TxnCommit(); + strAddress = PubKeyToAddress(account.vchPubKey); + return strAddress; } @@ -368,7 +367,15 @@ Value getaccountaddress(const Array& params, bool fHelp) // Parse the account first so we don't generate a key if there's an error string strAccount = AccountFromValue(params[0]); - return GetAccountAddress(strAccount); + Value ret; + + CRITICAL_BLOCK(cs_main) + CRITICAL_BLOCK(cs_mapWallet) + { + ret = GetAccountAddress(strAccount); + } + + return ret; } @@ -392,6 +399,8 @@ Value setaccount(const Array& params, bool fHelp) strAccount = AccountFromValue(params[1]); // Detect when changing the account of an address that is the 'unused current key' of another account: + CRITICAL_BLOCK(cs_main) + CRITICAL_BLOCK(cs_mapWallet) CRITICAL_BLOCK(cs_mapAddressBook) { if (mapAddressBook.count(strAddress)) @@ -475,9 +484,13 @@ Value sendtoaddress(const Array& params, bool fHelp) if (params.size() > 3 && params[3].type() != null_type && !params[3].get_str().empty()) wtx.mapValue["to"] = params[3].get_str(); - string strError = SendMoneyToBitcoinAddress(strAddress, nAmount, wtx); - if (strError != "") - throw JSONRPCError(-4, strError); + CRITICAL_BLOCK(cs_main) + { + string strError = SendMoneyToBitcoinAddress(strAddress, nAmount, wtx); + if (strError != "") + throw JSONRPCError(-4, strError); + } + return wtx.GetHash().GetHex(); } @@ -752,6 +765,7 @@ Value sendfrom(const Array& params, bool fHelp) if (params.size() > 5 && params[5].type() != null_type && !params[5].get_str().empty()) wtx.mapValue["to"] = params[5].get_str(); + CRITICAL_BLOCK(cs_main) CRITICAL_BLOCK(cs_mapWallet) { // Check funds diff --git a/ui.cpp b/ui.cpp index fafd3893..f4c0c4d7 100644 --- a/ui.cpp +++ b/ui.cpp @@ -1934,20 +1934,23 @@ void CSendDialog::OnButtonSend(wxCommandEvent& event) if (fBitcoinAddress) { - // Send to bitcoin address - CScript scriptPubKey; - scriptPubKey << OP_DUP << OP_HASH160 << hash160 << OP_EQUALVERIFY << OP_CHECKSIG; - - string strError = SendMoney(scriptPubKey, nValue, wtx, true); - if (strError == "") - wxMessageBox(_("Payment sent "), _("Sending...")); - else if (strError == "ABORTED") - return; // leave send dialog open - else - { - wxMessageBox(strError + " ", _("Sending...")); - EndModal(false); - } + CRITICAL_BLOCK(cs_main) + { + // Send to bitcoin address + CScript scriptPubKey; + scriptPubKey << OP_DUP << OP_HASH160 << hash160 << OP_EQUALVERIFY << OP_CHECKSIG; + + string strError = SendMoney(scriptPubKey, nValue, wtx, true); + if (strError == "") + wxMessageBox(_("Payment sent "), _("Sending...")); + else if (strError == "ABORTED") + return; // leave send dialog open + else + { + wxMessageBox(strError + " ", _("Sending...")); + EndModal(false); + } + } } else { From 6f074b71bb4c21e0a3ce059a64be1e9383538413 Mon Sep 17 00:00:00 2001 From: Jeff Garzik Date: Tue, 5 Apr 2011 15:15:20 -0400 Subject: [PATCH 2/2] RPC sendmany must acquire cs_main lock --- rpc.cpp | 1 + 1 file changed, 1 insertion(+) diff --git a/rpc.cpp b/rpc.cpp index a634fff6..6041de05 100644 --- a/rpc.cpp +++ b/rpc.cpp @@ -822,6 +822,7 @@ Value sendmany(const Array& params, bool fHelp) vecSend.push_back(make_pair(scriptPubKey, nAmount)); } + CRITICAL_BLOCK(cs_main) CRITICAL_BLOCK(cs_mapWallet) { // Check funds