Skip to content
New issue

Have a question about this project? Sign up for a free GitHub account to open an issue and contact its maintainers and the community.

Sign up for GitHub

By clicking “Sign up for GitHub”, you agree to our terms of service and privacy statement. We’ll occasionally send you account related emails.

Already on GitHub? Sign in to your account

Fix deadlocks in getaccountaddress, setaccount, sendfrom RPC calls #143

Merged
merged 2 commits into from
Apr 5, 2011
Merged

Fix deadlocks in getaccountaddress, setaccount, sendfrom RPC calls #143

Show file tree
Hide file tree
Changes from 1 commit
Commits
Show all changes
2 commits
Select commit Hold shift + click to select a range
f5f1878
Fix deadlocks in setaccount, sendfrom RPC calls
Apr 5, 2011
6f074b7
RPC sendmany must acquire cs_main lock
Apr 5, 2011
File filter

Filter by extension

Filter by extension
Conversations
Failed to load comments.
Loading
Jump to
Jump to file
Failed to load files.
Loading
Diff view
Diff view
Next Next commit
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
  • Loading branch information
@jgarzik
Jeff Garzik authored and jgarzik committed Apr 5, 2011
commit f5f1878ba104a5d6a32eeb20b341326dca6086a5
2 changes: 1 addition & 1 deletion db.cpp
View file
Open in desktop
Original file line number Diff line number Diff line change
Expand Up @@ -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();
Expand Down
36 changes: 18 additions & 18 deletions main.cpp
View file
Open in desktop
Original file line number Diff line number Diff line change
Expand Up @@ -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
Expand Down
82 changes: 48 additions & 34 deletions rpc.cpp
View file
Open in desktop
Original file line number Diff line number Diff line change
Expand Up @@ -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;
}

Expand All @@ -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;
}


Expand All @@ -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))
Expand Down Expand Up @@ -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();
}

Expand Down Expand Up @@ -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
Expand Down
31 changes: 17 additions & 14 deletions ui.cpp
View file
Open in desktop
Original file line number Diff line number Diff line change
Expand Up @@ -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
{
Expand Down