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

diff --git a/db.cpp b/db.cpp
index aaa997b..3cfcce5 100644 (file)
--- 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 bfc45af..6bd90a3 100644 (file)
--- 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 97710ff..a634fff 100644 (file)
--- 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<uint256, CWalletTx>::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<uint256, CWalletTx>::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 fafd389..f4c0c4d 100644 (file)
--- 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
         {