LBRYCommunity/lbrycrd
1
0
Fork 0
Code Issues 29 Pull requests 1 Projects Releases 32 Packages Wiki Activity

Merge #12333: Make CWallet::ListCoins atomic

Browse source 
2f960b5 [wallet] Indent only change of CWallet::AvailableCoins (João Barbosa)
1beea7a [wallet] Make CWallet::ListCoins atomic (João Barbosa)

Pull request description:

  Fix a potencial race in `CWallet::ListCoins`.

  Replaces `cs_main` and `cs_wallet` locks by assertions in `CWallet::AvailableCoins`.

Tree-SHA512: 09109f44a08b4b53f7605d950ab506d3f748490ab9aed474aa200e93f7b0b9f96f9bf60abe1c5f658240fd13d9e3267c0dd43fd3c1695d82384198ce1da8109f
This commit is contained in:
Wladimir J. van der Laan 2018-02-08 19:44:52 +01:00
commit d405beea26
No known key found for this signature in database
GPG key ID: 1E4AED62986CD25D
2 changed files with 93 additions and 89 deletions
Show stats Download patch file Download diff file Expand all files Collapse all files

18
src/wallet/test/wallet_tests.cpp
View file

@ -676,18 +676,24 @@ BOOST_FIXTURE_TEST_CASE(ListCoins, ListCoinsTestingSetup)
BOOST_CHECK_EQUAL(list.begin()->second.size(), 2);
// Lock both coins. Confirm number of available coins drops to 0.
std::vector<COutput> available;
wallet->AvailableCoins(available);
BOOST_CHECK_EQUAL(available.size(), 2);
{
LOCK2(cs_main, wallet->cs_wallet);
std::vector<COutput> available;
wallet->AvailableCoins(available);
BOOST_CHECK_EQUAL(available.size(), 2);
}
for (const auto& group : list) {
for (const auto& coin : group.second) {
LOCK(wallet->cs_wallet);
wallet->LockCoin(COutPoint(coin.tx->GetHash(), coin.i));
}
}
wallet->AvailableCoins(available);
BOOST_CHECK_EQUAL(available.size(), 0);
{
LOCK2(cs_main, wallet->cs_wallet);
std::vector<COutput> available;
wallet->AvailableCoins(available);
BOOST_CHECK_EQUAL(available.size(), 0);
}
// Confirm ListCoins still returns same result as before, despite coins
// being locked.
list = wallet->ListCoins();

164
src/wallet/wallet.cpp
View file

@ -2198,111 +2198,109 @@ CAmount CWallet::GetAvailableBalance(const CCoinControl* coinControl) const
void CWallet::AvailableCoins(std::vector<COutput> &vCoins, bool fOnlySafe, const CCoinControl *coinControl, const CAmount &nMinimumAmount, const CAmount &nMaximumAmount, const CAmount &nMinimumSumAmount, const uint64_t nMaximumCount, const int nMinDepth, const int nMaxDepth) const
{
AssertLockHeld(cs_main);
AssertLockHeld(cs_wallet);
vCoins.clear();
CAmount nTotal = 0;
for (const auto& entry : mapWallet)
{
LOCK2(cs_main, cs_wallet);
const uint256& wtxid = entry.first;
const CWalletTx* pcoin = &entry.second;
CAmount nTotal = 0;
if (!CheckFinalTx(*pcoin->tx))
continue;
for (const auto& entry : mapWallet)
{
const uint256& wtxid = entry.first;
const CWalletTx* pcoin = &entry.second;
if (pcoin->IsCoinBase() && pcoin->GetBlocksToMaturity() > 0)
continue;
if (!CheckFinalTx(*pcoin->tx))
int nDepth = pcoin->GetDepthInMainChain();
if (nDepth < 0)
continue;
// We should not consider coins which aren't at least in our mempool
// It's possible for these to be conflicted via ancestors which we may never be able to detect
if (nDepth == 0 && !pcoin->InMempool())
continue;
bool safeTx = pcoin->IsTrusted();
// We should not consider coins from transactions that are replacing
// other transactions.
//
// Example: There is a transaction A which is replaced by bumpfee
// transaction B. In this case, we want to prevent creation of
// a transaction B' which spends an output of B.
//
// Reason: If transaction A were initially confirmed, transactions B
// and B' would no longer be valid, so the user would have to create
// a new transaction C to replace B'. However, in the case of a
// one-block reorg, transactions B' and C might BOTH be accepted,
// when the user only wanted one of them. Specifically, there could
// be a 1-block reorg away from the chain where transactions A and C
// were accepted to another chain where B, B', and C were all
// accepted.
if (nDepth == 0 && pcoin->mapValue.count("replaces_txid")) {
safeTx = false;
}
// Similarly, we should not consider coins from transactions that
// have been replaced. In the example above, we would want to prevent
// creation of a transaction A' spending an output of A, because if
// transaction B were initially confirmed, conflicting with A and
// A', we wouldn't want to the user to create a transaction D
// intending to replace A', but potentially resulting in a scenario
// where A, A', and D could all be accepted (instead of just B and
// D, or just A and A' like the user would want).
if (nDepth == 0 && pcoin->mapValue.count("replaced_by_txid")) {
safeTx = false;
}
if (fOnlySafe && !safeTx) {
continue;
}
if (nDepth < nMinDepth || nDepth > nMaxDepth)
continue;
for (unsigned int i = 0; i < pcoin->tx->vout.size(); i++) {
if (pcoin->tx->vout[i].nValue < nMinimumAmount || pcoin->tx->vout[i].nValue > nMaximumAmount)
continue;
if (pcoin->IsCoinBase() && pcoin->GetBlocksToMaturity() > 0)
if (coinControl && coinControl->HasSelected() && !coinControl->fAllowOtherInputs && !coinControl->IsSelected(COutPoint(entry.first, i)))
continue;
int nDepth = pcoin->GetDepthInMainChain();
if (nDepth < 0)
if (IsLockedCoin(entry.first, i))
continue;
// We should not consider coins which aren't at least in our mempool
// It's possible for these to be conflicted via ancestors which we may never be able to detect
if (nDepth == 0 && !pcoin->InMempool())
if (IsSpent(wtxid, i))
continue;
bool safeTx = pcoin->IsTrusted();
isminetype mine = IsMine(pcoin->tx->vout[i]);
// We should not consider coins from transactions that are replacing
// other transactions.
//
// Example: There is a transaction A which is replaced by bumpfee
// transaction B. In this case, we want to prevent creation of
// a transaction B' which spends an output of B.
//
// Reason: If transaction A were initially confirmed, transactions B
// and B' would no longer be valid, so the user would have to create
// a new transaction C to replace B'. However, in the case of a
// one-block reorg, transactions B' and C might BOTH be accepted,
// when the user only wanted one of them. Specifically, there could
// be a 1-block reorg away from the chain where transactions A and C
// were accepted to another chain where B, B', and C were all
// accepted.
if (nDepth == 0 && pcoin->mapValue.count("replaces_txid")) {
safeTx = false;
}
// Similarly, we should not consider coins from transactions that
// have been replaced. In the example above, we would want to prevent
// creation of a transaction A' spending an output of A, because if
// transaction B were initially confirmed, conflicting with A and
// A', we wouldn't want to the user to create a transaction D
// intending to replace A', but potentially resulting in a scenario
// where A, A', and D could all be accepted (instead of just B and
// D, or just A and A' like the user would want).
if (nDepth == 0 && pcoin->mapValue.count("replaced_by_txid")) {
safeTx = false;
}
if (fOnlySafe && !safeTx) {
if (mine == ISMINE_NO) {
continue;
}
if (nDepth < nMinDepth || nDepth > nMaxDepth)
continue;
bool fSpendableIn = ((mine & ISMINE_SPENDABLE) != ISMINE_NO) || (coinControl && coinControl->fAllowWatchOnly && (mine & ISMINE_WATCH_SOLVABLE) != ISMINE_NO);
bool fSolvableIn = (mine & (ISMINE_SPENDABLE | ISMINE_WATCH_SOLVABLE)) != ISMINE_NO;
for (unsigned int i = 0; i < pcoin->tx->vout.size(); i++) {
if (pcoin->tx->vout[i].nValue < nMinimumAmount || pcoin->tx->vout[i].nValue > nMaximumAmount)
continue;
vCoins.push_back(COutput(pcoin, i, nDepth, fSpendableIn, fSolvableIn, safeTx));
if (coinControl && coinControl->HasSelected() && !coinControl->fAllowOtherInputs && !coinControl->IsSelected(COutPoint(entry.first, i)))
continue;
// Checks the sum amount of all UTXO's.
if (nMinimumSumAmount != MAX_MONEY) {
nTotal += pcoin->tx->vout[i].nValue;
if (IsLockedCoin(entry.first, i))
continue;
if (IsSpent(wtxid, i))
continue;
isminetype mine = IsMine(pcoin->tx->vout[i]);
if (mine == ISMINE_NO) {
continue;
}
bool fSpendableIn = ((mine & ISMINE_SPENDABLE) != ISMINE_NO) || (coinControl && coinControl->fAllowWatchOnly && (mine & ISMINE_WATCH_SOLVABLE) != ISMINE_NO);
bool fSolvableIn = (mine & (ISMINE_SPENDABLE | ISMINE_WATCH_SOLVABLE)) != ISMINE_NO;
vCoins.push_back(COutput(pcoin, i, nDepth, fSpendableIn, fSolvableIn, safeTx));
// Checks the sum amount of all UTXO's.
if (nMinimumSumAmount != MAX_MONEY) {
nTotal += pcoin->tx->vout[i].nValue;
if (nTotal >= nMinimumSumAmount) {
return;
}
}
// Checks the maximum number of UTXO's.
if (nMaximumCount > 0 && vCoins.size() >= nMaximumCount) {
if (nTotal >= nMinimumSumAmount) {
return;
}
}
// Checks the maximum number of UTXO's.
if (nMaximumCount > 0 && vCoins.size() >= nMaximumCount) {
return;
}
}
}
}
@ -2320,11 +2318,11 @@ std::map<CTxDestination, std::vector<COutput>> CWallet::ListCoins() const
// avoid adding some extra complexity to the Qt code.
std::map<CTxDestination, std::vector<COutput>> result;
std::vector<COutput> availableCoins;
AvailableCoins(availableCoins);
LOCK2(cs_main, cs_wallet);
AvailableCoins(availableCoins);
for (auto& coin : availableCoins) {
CTxDestination address;
if (coin.fSpendable &&