From ef8ca179ef007eba5bed497be18f5bc90a4194a6 Mon Sep 17 00:00:00 2001 From: Russell Yanofsky Date: Fri, 28 Apr 2017 13:59:25 -0400 Subject: [PATCH 1/4] [test] Add tests for some walletmodel functions Add unit tests for some walletmodel functions that will be refactored & moved in the next commit. --- src/Makefile.qttest.include | 3 +- src/qt/test/wallettests.cpp | 127 +++++++++++++++++++++++++++++++++++- 2 files changed, 127 insertions(+), 3 deletions(-) diff --git a/src/Makefile.qttest.include b/src/Makefile.qttest.include index 948e13a9e1..391b9ebdf6 100644 --- a/src/Makefile.qttest.include +++ b/src/Makefile.qttest.include @@ -46,7 +46,8 @@ qt_test_test_bitcoin_qt_SOURCES = \ if ENABLE_WALLET qt_test_test_bitcoin_qt_SOURCES += \ qt/test/paymentservertests.cpp \ - qt/test/wallettests.cpp + qt/test/wallettests.cpp \ + wallet/test/wallet_test_fixture.cpp endif nodist_qt_test_test_bitcoin_qt_SOURCES = $(TEST_QT_MOC_CPP) diff --git a/src/qt/test/wallettests.cpp b/src/qt/test/wallettests.cpp index a0dce3d997..867711309e 100644 --- a/src/qt/test/wallettests.cpp +++ b/src/qt/test/wallettests.cpp @@ -1,5 +1,6 @@ #include "wallettests.h" +#include "consensus/validation.h" #include "qt/bitcoinamountfield.h" #include "qt/callback.h" #include "qt/optionsmodel.h" @@ -11,6 +12,8 @@ #include "qt/walletmodel.h" #include "test/test_bitcoin.h" #include "validation.h" +#include "wallet/test/wallet_test_fixture.h" +#include "wallet/coincontrol.h" #include "wallet/wallet.h" #include @@ -20,6 +23,118 @@ namespace { + +void TestLoadReceiveRequests() +{ + WalletTestingSetup test; + OptionsModel optionsModel; + WalletModel walletModel(nullptr, pwalletMain, &optionsModel); + + CTxDestination dest = CKeyID(); + pwalletMain->AddDestData(dest, "misc", "val_misc"); + pwalletMain->AddDestData(dest, "rr0", "val_rr0"); + pwalletMain->AddDestData(dest, "rr1", "val_rr1"); + + std::vector values; + walletModel.loadReceiveRequests(values); + QCOMPARE((int)values.size(), 2); + QCOMPARE(QString::fromStdString(values[0]), QString("val_rr0")); + QCOMPARE(QString::fromStdString(values[1]), QString("val_rr1")); +} + +class ListCoinsTestingSetup : public TestChain100Setup +{ +public: + ListCoinsTestingSetup() + { + CreateAndProcessBlock({}, GetScriptForRawPubKey(coinbaseKey.GetPubKey())); + ::bitdb.MakeMock(); + wallet.reset(new CWallet(std::unique_ptr(new CWalletDBWrapper(&bitdb, "wallet_test.dat")))); + bool firstRun; + wallet->LoadWallet(firstRun); + LOCK(wallet->cs_wallet); + wallet->AddKeyPubKey(coinbaseKey, coinbaseKey.GetPubKey()); + wallet->ScanForWalletTransactions(chainActive.Genesis()); + } + + ~ListCoinsTestingSetup() + { + ::bitdb.Flush(true); + ::bitdb.Reset(); + } + + CWalletTx& AddTx(CRecipient recipient) + { + CWalletTx wtx; + CReserveKey reservekey(wallet.get()); + CAmount fee; + int changePos = -1; + std::string error; + wallet->CreateTransaction({recipient}, wtx, reservekey, fee, changePos, error); + CValidationState state; + wallet->CommitTransaction(wtx, reservekey, nullptr, state); + auto it = wallet->mapWallet.find(wtx.GetHash()); + CreateAndProcessBlock({CMutableTransaction(*it->second.tx)}, GetScriptForRawPubKey(coinbaseKey.GetPubKey())); + it->second.SetMerkleBranch(chainActive.Tip(), 1); + return it->second; + } + + std::unique_ptr wallet; +}; + +void TestListCoins() +{ + ListCoinsTestingSetup test; + OptionsModel optionsModel; + WalletModel walletModel(nullptr, test.wallet.get(), &optionsModel); + QString coinbaseAddress = QString::fromStdString(CBitcoinAddress(test.coinbaseKey.GetPubKey().GetID()).ToString()); + + LOCK(test.wallet->cs_wallet); + + // Confirm ListCoins initially returns 1 coin grouped under coinbaseKey + // address. + std::map> list; + walletModel.listCoins(list); + QCOMPARE((int)list.size(), 1); + QCOMPARE(list.begin()->first, coinbaseAddress); + QCOMPARE((int)list.begin()->second.size(), 1); + + // Check initial balance from one mature coinbase transaction. + CCoinControl coinControl; + QCOMPARE(50 * COIN, walletModel.getBalance(&coinControl)); + + // Add a transaction creating a change address, and confirm ListCoins still + // returns the coin associated with the change address underneath the + // coinbaseKey pubkey, even though the change address has a different + // pubkey. + test.AddTx(CRecipient{GetScriptForRawPubKey({}), 1 * COIN, false /* subtract fee */}); + list.clear(); + walletModel.listCoins(list); + QCOMPARE((int)list.size(), 1); + QCOMPARE(list.begin()->first, coinbaseAddress); + QCOMPARE((int)list.begin()->second.size(), 2); + + // Lock both coins. Confirm number of available coins drops to 0. + std::vector available; + test.wallet->AvailableCoins(available); + QCOMPARE((int)available.size(), 2); + for (const auto& group : list) { + for (const auto& coin : group.second) { + test.wallet->LockCoin(COutPoint(coin.tx->GetHash(), coin.i)); + } + } + test.wallet->AvailableCoins(available); + QCOMPARE((int)available.size(), 0); + + // Confirm ListCoins still returns same result as before, despite coins + // being locked. + list.clear(); + walletModel.listCoins(list); + QCOMPARE((int)list.size(), 1); + QCOMPARE(list.begin()->first, coinbaseAddress); + QCOMPARE((int)list.begin()->second.size(), 2); +} + //! Press "Yes" button in modal send confirmation dialog. void ConfirmSend() { @@ -65,7 +180,6 @@ QModelIndex FindTx(const QAbstractItemModel& model, const uint256& txid) } return {}; } -} //! Simple qt wallet tests. // @@ -80,7 +194,7 @@ QModelIndex FindTx(const QAbstractItemModel& model, const uint256& txid) // src/qt/test/test_bitcoin-qt -platform xcb # Linux // src/qt/test/test_bitcoin-qt -platform windows # Windows // src/qt/test/test_bitcoin-qt -platform cocoa # macOS -void WalletTests::walletTests() +void TestSendCoins() { // Set up wallet and chain with 101 blocks (1 mature block for spending). TestChain100Setup test; @@ -117,3 +231,12 @@ void WalletTests::walletTests() bitdb.Flush(true); bitdb.Reset(); } + +} + +void WalletTests::walletTests() +{ + TestLoadReceiveRequests(); + TestListCoins(); + TestSendCoins(); +} From d944bd7a27abe0fa74d40ba5e90f345f51bb5141 Mon Sep 17 00:00:00 2001 From: Russell Yanofsky Date: Fri, 28 Apr 2017 14:10:21 -0400 Subject: [PATCH 2/4] [qt] Move some WalletModel functions into CWallet Motivation for moving these is to make supporting IPC simpler (#10102), so these lookups can be one-shot IPC requests, instead of back-and-forth interactions over the IPC channel. Also these functions are potentially useful outside of the bitcoin GUI (e.g. for RPCs). --- src/qt/walletmodel.cpp | 56 +++--------------------- src/wallet/wallet.cpp | 99 +++++++++++++++++++++++++++++++++++++++++- src/wallet/wallet.h | 18 +++++++- 3 files changed, 122 insertions(+), 51 deletions(-) diff --git a/src/qt/walletmodel.cpp b/src/qt/walletmodel.cpp index a2a9271904..719089d05a 100644 --- a/src/qt/walletmodel.cpp +++ b/src/qt/walletmodel.cpp @@ -63,14 +63,7 @@ CAmount WalletModel::getBalance(const CCoinControl *coinControl) const { if (coinControl) { - CAmount nBalance = 0; - std::vector vCoins; - wallet->AvailableCoins(vCoins, true, coinControl); - BOOST_FOREACH(const COutput& out, vCoins) - if(out.fSpendable) - nBalance += out.tx->tx->vout[out.i].nValue; - - return nBalance; + return wallet->GetAvailableBalance(coinControl); } return wallet->GetBalance(); @@ -595,38 +588,11 @@ bool WalletModel::isSpent(const COutPoint& outpoint) const // AvailableCoins + LockedCoins grouped by wallet address (put change in one group with wallet address) void WalletModel::listCoins(std::map >& mapCoins) const { - std::vector vCoins; - wallet->AvailableCoins(vCoins); - - LOCK2(cs_main, wallet->cs_wallet); // ListLockedCoins, mapWallet - std::vector vLockedCoins; - wallet->ListLockedCoins(vLockedCoins); - - // add locked coins - BOOST_FOREACH(const COutPoint& outpoint, vLockedCoins) - { - if (!wallet->mapWallet.count(outpoint.hash)) continue; - int nDepth = wallet->mapWallet[outpoint.hash].GetDepthInMainChain(); - if (nDepth < 0) continue; - COutput out(&wallet->mapWallet[outpoint.hash], outpoint.n, nDepth, true /* spendable */, true /* solvable */, true /* safe */); - if (outpoint.n < out.tx->tx->vout.size() && wallet->IsMine(out.tx->tx->vout[outpoint.n]) == ISMINE_SPENDABLE) - vCoins.push_back(out); - } - - BOOST_FOREACH(const COutput& out, vCoins) - { - COutput cout = out; - - while (wallet->IsChange(cout.tx->tx->vout[cout.i]) && cout.tx->tx->vin.size() > 0 && wallet->IsMine(cout.tx->tx->vin[0])) - { - if (!wallet->mapWallet.count(cout.tx->tx->vin[0].prevout.hash)) break; - cout = COutput(&wallet->mapWallet[cout.tx->tx->vin[0].prevout.hash], cout.tx->tx->vin[0].prevout.n, 0 /* depth */, true /* spendable */, true /* solvable */, true /* safe */); + for (auto& group : wallet->ListCoins()) { + auto& resultGroup = mapCoins[QString::fromStdString(CBitcoinAddress(group.first).ToString())]; + for (auto& coin : group.second) { + resultGroup.emplace_back(std::move(coin)); } - - CTxDestination address; - if(!out.fSpendable || !ExtractDestination(cout.tx->tx->vout[cout.i].scriptPubKey, address)) - continue; - mapCoins[QString::fromStdString(CBitcoinAddress(address).ToString())].push_back(out); } } @@ -656,11 +622,7 @@ void WalletModel::listLockedCoins(std::vector& vOutpts) void WalletModel::loadReceiveRequests(std::vector& vReceiveRequests) { - LOCK(wallet->cs_wallet); - BOOST_FOREACH(const PAIRTYPE(CTxDestination, CAddressBookData)& item, wallet->mapAddressBook) - BOOST_FOREACH(const PAIRTYPE(std::string, std::string)& item2, item.second.destdata) - if (item2.first.size() > 2 && item2.first.substr(0,2) == "rr") // receive request - vReceiveRequests.push_back(item2.second); + vReceiveRequests = wallet->GetDestValues("rr"); // receive request } bool WalletModel::saveReceiveRequest(const std::string &sAddress, const int64_t nId, const std::string &sRequest) @@ -680,11 +642,7 @@ bool WalletModel::saveReceiveRequest(const std::string &sAddress, const int64_t bool WalletModel::transactionCanBeAbandoned(uint256 hash) const { - LOCK2(cs_main, wallet->cs_wallet); - const CWalletTx *wtx = wallet->GetWalletTx(hash); - if (!wtx || wtx->isAbandoned() || wtx->GetDepthInMainChain() > 0 || wtx->InMempool()) - return false; - return true; + return wallet->TransactionCanBeAbandoned(hash); } bool WalletModel::abandonTransaction(uint256 hash) const diff --git a/src/wallet/wallet.cpp b/src/wallet/wallet.cpp index dcb452a009..e62b24cdbc 100644 --- a/src/wallet/wallet.cpp +++ b/src/wallet/wallet.cpp @@ -982,6 +982,13 @@ bool CWallet::AddToWalletIfInvolvingMe(const CTransactionRef& ptx, const CBlockI return false; } +bool CWallet::TransactionCanBeAbandoned(const uint256& hashTx) const +{ + LOCK2(cs_main, cs_wallet); + const CWalletTx* wtx = GetWalletTx(hashTx); + return wtx && !wtx->isAbandoned() && wtx->GetDepthInMainChain() <= 0 && !wtx->InMempool(); +} + bool CWallet::AbandonTransaction(const uint256& hashTx) { LOCK2(cs_main, cs_wallet); @@ -1977,6 +1984,19 @@ CAmount CWallet::GetLegacyBalance(const isminefilter& filter, int minDepth, cons return balance; } +CAmount CWallet::GetAvailableBalance(const CCoinControl* coinControl) const +{ + CAmount balance = 0; + std::vector vCoins; + AvailableCoins(vCoins, true, coinControl); + for (const COutput& out : vCoins) { + if (out.fSpendable) { + balance += out.tx->tx->vout[out.i].nValue; + } + } + return balance; +} + void CWallet::AvailableCoins(std::vector &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 { vCoins.clear(); @@ -2088,6 +2108,69 @@ void CWallet::AvailableCoins(std::vector &vCoins, bool fOnlySafe, const } } +std::map> CWallet::ListCoins() const +{ + // TODO: Add AssertLockHeld(cs_wallet) here. + // + // Because the return value from this function contains pointers to + // CWalletTx objects, callers to this function really should acquire the + // cs_wallet lock before calling it. However, the current caller doesn't + // acquire this lock yet. There was an attempt to add the missing lock in + // https://github.com/bitcoin/bitcoin/pull/10340, but that change has been + // postponed until after https://github.com/bitcoin/bitcoin/pull/10244 to + // avoid adding some extra complexity to the Qt code. + + std::map> result; + + std::vector availableCoins; + AvailableCoins(availableCoins); + + LOCK2(cs_main, cs_wallet); + for (auto& coin : availableCoins) { + CTxDestination address; + if (coin.fSpendable && + ExtractDestination(FindNonChangeParentOutput(*coin.tx->tx, coin.i).scriptPubKey, address)) { + result[address].emplace_back(std::move(coin)); + } + } + + std::vector lockedCoins; + ListLockedCoins(lockedCoins); + for (const auto& output : lockedCoins) { + auto it = mapWallet.find(output.hash); + if (it != mapWallet.end()) { + int depth = it->second.GetDepthInMainChain(); + if (depth >= 0 && output.n < it->second.tx->vout.size() && + IsMine(it->second.tx->vout[output.n]) == ISMINE_SPENDABLE) { + CTxDestination address; + if (ExtractDestination(FindNonChangeParentOutput(*it->second.tx, output.n).scriptPubKey, address)) { + result[address].emplace_back( + &it->second, output.n, depth, true /* spendable */, true /* solvable */, false /* safe */); + } + } + } + } + + return result; +} + +const CTxOut& CWallet::FindNonChangeParentOutput(const CTransaction& tx, int output) const +{ + const CTransaction* ptx = &tx; + int n = output; + while (IsChange(ptx->vout[n]) && ptx->vin.size() > 0) { + const COutPoint& prevout = ptx->vin[0].prevout; + auto it = mapWallet.find(prevout.hash); + if (it == mapWallet.end() || it->second.tx->vout.size() <= prevout.n || + !IsMine(it->second.tx->vout[prevout.n])) { + break; + } + ptx = it->second.tx.get(); + n = prevout.n; + } + return ptx->vout[n]; +} + static void ApproximateBestSubset(const std::vector& vValue, const CAmount& nTotalLower, const CAmount& nTargetValue, std::vector& vfBest, CAmount& nBest, int iterations = 1000) { @@ -3408,7 +3491,7 @@ bool CWallet::IsLockedCoin(uint256 hash, unsigned int n) const return (setLockedCoins.count(outpt) > 0); } -void CWallet::ListLockedCoins(std::vector& vOutpts) +void CWallet::ListLockedCoins(std::vector& vOutpts) const { AssertLockHeld(cs_wallet); // setLockedCoins for (std::set::iterator it = setLockedCoins.begin(); @@ -3609,6 +3692,20 @@ bool CWallet::GetDestData(const CTxDestination &dest, const std::string &key, st return false; } +std::vector CWallet::GetDestValues(const std::string& prefix) const +{ + LOCK(cs_wallet); + std::vector values; + for (const auto& address : mapAddressBook) { + for (const auto& data : address.second.destdata) { + if (!data.first.compare(0, prefix.size(), prefix)) { + values.emplace_back(data.second); + } + } + } + return values; +} + std::string CWallet::GetWalletHelpString(bool showDebug) { std::string strUsage = HelpMessageGroup(_("Wallet options:")); diff --git a/src/wallet/wallet.h b/src/wallet/wallet.h index 69f51b3f64..11b2f7a663 100644 --- a/src/wallet/wallet.h +++ b/src/wallet/wallet.h @@ -820,6 +820,16 @@ public: */ void AvailableCoins(std::vector& vCoins, bool fOnlySafe=true, const CCoinControl *coinControl = NULL, const CAmount& nMinimumAmount = 1, const CAmount& nMaximumAmount = MAX_MONEY, const CAmount& nMinimumSumAmount = MAX_MONEY, const uint64_t& nMaximumCount = 0, const int& nMinDepth = 0, const int& nMaxDepth = 9999999) const; + /** + * Return list of available coins and locked coins grouped by non-change output address. + */ + std::map> ListCoins() const; + + /** + * Find non-change parent output. + */ + const CTxOut& FindNonChangeParentOutput(const CTransaction& tx, int output) const; + /** * Shuffle and select coins until nTargetValue is reached while avoiding * small change; This method is stochastic for some inputs and upon @@ -834,7 +844,7 @@ public: void LockCoin(const COutPoint& output); void UnlockCoin(const COutPoint& output); void UnlockAllCoins(); - void ListLockedCoins(std::vector& vOutpts); + void ListLockedCoins(std::vector& vOutpts) const; /* * Rescan abort properties @@ -873,6 +883,8 @@ public: bool LoadDestData(const CTxDestination &dest, const std::string &key, const std::string &value); //! Look up a destination data tuple in the store, return true if found false otherwise bool GetDestData(const CTxDestination &dest, const std::string &key, std::string *value) const; + //! Get all destination values matching a prefix. + std::vector GetDestValues(const std::string& prefix) const; //! Adds a watch-only address to the store, and saves it to disk. bool AddWatchOnly(const CScript& dest, int64_t nCreateTime); @@ -917,6 +929,7 @@ public: CAmount GetUnconfirmedWatchOnlyBalance() const; CAmount GetImmatureWatchOnlyBalance() const; CAmount GetLegacyBalance(const isminefilter& filter, int minDepth, const std::string* account) const; + CAmount GetAvailableBalance(const CCoinControl* coinControl = nullptr) const; /** * Insert additional inputs into the transaction by @@ -1066,6 +1079,9 @@ public: /** Set whether this wallet broadcasts transactions. */ void SetBroadcastTransactions(bool broadcast) { fBroadcastTransactions = broadcast; } + /** Return whether transaction can be abandoned */ + bool TransactionCanBeAbandoned(const uint256& hashTx) const; + /* Mark a transaction (and it in-wallet descendants) as abandoned so its inputs may be respent. */ bool AbandonTransaction(const uint256& hashTx); From 429aa9eb519b72a79f8bb41d0c4d7d47bcd9dd0d Mon Sep 17 00:00:00 2001 From: Russell Yanofsky Date: Fri, 28 Apr 2017 14:18:14 -0400 Subject: [PATCH 3/4] [test] Move some tests from qt -> wallet After previous refactoring, the tests make more sense here. --- src/qt/test/wallettests.cpp | 117 ------------------------------- src/wallet/test/wallet_tests.cpp | 101 ++++++++++++++++++++++++++ 2 files changed, 101 insertions(+), 117 deletions(-) diff --git a/src/qt/test/wallettests.cpp b/src/qt/test/wallettests.cpp index 867711309e..798d333d63 100644 --- a/src/qt/test/wallettests.cpp +++ b/src/qt/test/wallettests.cpp @@ -1,6 +1,5 @@ #include "wallettests.h" -#include "consensus/validation.h" #include "qt/bitcoinamountfield.h" #include "qt/callback.h" #include "qt/optionsmodel.h" @@ -12,8 +11,6 @@ #include "qt/walletmodel.h" #include "test/test_bitcoin.h" #include "validation.h" -#include "wallet/test/wallet_test_fixture.h" -#include "wallet/coincontrol.h" #include "wallet/wallet.h" #include @@ -23,118 +20,6 @@ namespace { - -void TestLoadReceiveRequests() -{ - WalletTestingSetup test; - OptionsModel optionsModel; - WalletModel walletModel(nullptr, pwalletMain, &optionsModel); - - CTxDestination dest = CKeyID(); - pwalletMain->AddDestData(dest, "misc", "val_misc"); - pwalletMain->AddDestData(dest, "rr0", "val_rr0"); - pwalletMain->AddDestData(dest, "rr1", "val_rr1"); - - std::vector values; - walletModel.loadReceiveRequests(values); - QCOMPARE((int)values.size(), 2); - QCOMPARE(QString::fromStdString(values[0]), QString("val_rr0")); - QCOMPARE(QString::fromStdString(values[1]), QString("val_rr1")); -} - -class ListCoinsTestingSetup : public TestChain100Setup -{ -public: - ListCoinsTestingSetup() - { - CreateAndProcessBlock({}, GetScriptForRawPubKey(coinbaseKey.GetPubKey())); - ::bitdb.MakeMock(); - wallet.reset(new CWallet(std::unique_ptr(new CWalletDBWrapper(&bitdb, "wallet_test.dat")))); - bool firstRun; - wallet->LoadWallet(firstRun); - LOCK(wallet->cs_wallet); - wallet->AddKeyPubKey(coinbaseKey, coinbaseKey.GetPubKey()); - wallet->ScanForWalletTransactions(chainActive.Genesis()); - } - - ~ListCoinsTestingSetup() - { - ::bitdb.Flush(true); - ::bitdb.Reset(); - } - - CWalletTx& AddTx(CRecipient recipient) - { - CWalletTx wtx; - CReserveKey reservekey(wallet.get()); - CAmount fee; - int changePos = -1; - std::string error; - wallet->CreateTransaction({recipient}, wtx, reservekey, fee, changePos, error); - CValidationState state; - wallet->CommitTransaction(wtx, reservekey, nullptr, state); - auto it = wallet->mapWallet.find(wtx.GetHash()); - CreateAndProcessBlock({CMutableTransaction(*it->second.tx)}, GetScriptForRawPubKey(coinbaseKey.GetPubKey())); - it->second.SetMerkleBranch(chainActive.Tip(), 1); - return it->second; - } - - std::unique_ptr wallet; -}; - -void TestListCoins() -{ - ListCoinsTestingSetup test; - OptionsModel optionsModel; - WalletModel walletModel(nullptr, test.wallet.get(), &optionsModel); - QString coinbaseAddress = QString::fromStdString(CBitcoinAddress(test.coinbaseKey.GetPubKey().GetID()).ToString()); - - LOCK(test.wallet->cs_wallet); - - // Confirm ListCoins initially returns 1 coin grouped under coinbaseKey - // address. - std::map> list; - walletModel.listCoins(list); - QCOMPARE((int)list.size(), 1); - QCOMPARE(list.begin()->first, coinbaseAddress); - QCOMPARE((int)list.begin()->second.size(), 1); - - // Check initial balance from one mature coinbase transaction. - CCoinControl coinControl; - QCOMPARE(50 * COIN, walletModel.getBalance(&coinControl)); - - // Add a transaction creating a change address, and confirm ListCoins still - // returns the coin associated with the change address underneath the - // coinbaseKey pubkey, even though the change address has a different - // pubkey. - test.AddTx(CRecipient{GetScriptForRawPubKey({}), 1 * COIN, false /* subtract fee */}); - list.clear(); - walletModel.listCoins(list); - QCOMPARE((int)list.size(), 1); - QCOMPARE(list.begin()->first, coinbaseAddress); - QCOMPARE((int)list.begin()->second.size(), 2); - - // Lock both coins. Confirm number of available coins drops to 0. - std::vector available; - test.wallet->AvailableCoins(available); - QCOMPARE((int)available.size(), 2); - for (const auto& group : list) { - for (const auto& coin : group.second) { - test.wallet->LockCoin(COutPoint(coin.tx->GetHash(), coin.i)); - } - } - test.wallet->AvailableCoins(available); - QCOMPARE((int)available.size(), 0); - - // Confirm ListCoins still returns same result as before, despite coins - // being locked. - list.clear(); - walletModel.listCoins(list); - QCOMPARE((int)list.size(), 1); - QCOMPARE(list.begin()->first, coinbaseAddress); - QCOMPARE((int)list.begin()->second.size(), 2); -} - //! Press "Yes" button in modal send confirmation dialog. void ConfirmSend() { @@ -236,7 +121,5 @@ void TestSendCoins() void WalletTests::walletTests() { - TestLoadReceiveRequests(); - TestListCoins(); TestSendCoins(); } diff --git a/src/wallet/test/wallet_tests.cpp b/src/wallet/test/wallet_tests.cpp index 8eeba72a06..8e56f1efeb 100644 --- a/src/wallet/test/wallet_tests.cpp +++ b/src/wallet/test/wallet_tests.cpp @@ -9,6 +9,7 @@ #include #include +#include "consensus/validation.h" #include "rpc/server.h" #include "test/test_bitcoin.h" #include "validation.h" @@ -515,4 +516,104 @@ BOOST_AUTO_TEST_CASE(ComputeTimeSmart) SetMockTime(0); } +BOOST_AUTO_TEST_CASE(LoadReceiveRequests) +{ + CTxDestination dest = CKeyID(); + pwalletMain->AddDestData(dest, "misc", "val_misc"); + pwalletMain->AddDestData(dest, "rr0", "val_rr0"); + pwalletMain->AddDestData(dest, "rr1", "val_rr1"); + + auto values = pwalletMain->GetDestValues("rr"); + BOOST_CHECK_EQUAL(values.size(), 2); + BOOST_CHECK_EQUAL(values[0], "val_rr0"); + BOOST_CHECK_EQUAL(values[1], "val_rr1"); +} + +class ListCoinsTestingSetup : public TestChain100Setup +{ +public: + ListCoinsTestingSetup() + { + CreateAndProcessBlock({}, GetScriptForRawPubKey(coinbaseKey.GetPubKey())); + ::bitdb.MakeMock(); + wallet.reset(new CWallet(std::unique_ptr(new CWalletDBWrapper(&bitdb, "wallet_test.dat")))); + bool firstRun; + wallet->LoadWallet(firstRun); + LOCK(wallet->cs_wallet); + wallet->AddKeyPubKey(coinbaseKey, coinbaseKey.GetPubKey()); + wallet->ScanForWalletTransactions(chainActive.Genesis()); + } + + ~ListCoinsTestingSetup() + { + wallet.reset(); + ::bitdb.Flush(true); + ::bitdb.Reset(); + } + + CWalletTx& AddTx(CRecipient recipient) + { + CWalletTx wtx; + CReserveKey reservekey(wallet.get()); + CAmount fee; + int changePos = -1; + std::string error; + BOOST_CHECK(wallet->CreateTransaction({recipient}, wtx, reservekey, fee, changePos, error)); + CValidationState state; + BOOST_CHECK(wallet->CommitTransaction(wtx, reservekey, nullptr, state)); + auto it = wallet->mapWallet.find(wtx.GetHash()); + BOOST_CHECK(it != wallet->mapWallet.end()); + CreateAndProcessBlock({CMutableTransaction(*it->second.tx)}, GetScriptForRawPubKey(coinbaseKey.GetPubKey())); + it->second.SetMerkleBranch(chainActive.Tip(), 1); + return it->second; + } + + std::unique_ptr wallet; +}; + +BOOST_FIXTURE_TEST_CASE(ListCoins, ListCoinsTestingSetup) +{ + std::string coinbaseAddress = coinbaseKey.GetPubKey().GetID().ToString(); + LOCK(wallet->cs_wallet); + + // Confirm ListCoins initially returns 1 coin grouped under coinbaseKey + // address. + auto list = wallet->ListCoins(); + BOOST_CHECK_EQUAL(list.size(), 1); + BOOST_CHECK_EQUAL(boost::get(list.begin()->first).ToString(), coinbaseAddress); + BOOST_CHECK_EQUAL(list.begin()->second.size(), 1); + + // Check initial balance from one mature coinbase transaction. + BOOST_CHECK_EQUAL(50 * COIN, wallet->GetAvailableBalance()); + + // Add a transaction creating a change address, and confirm ListCoins still + // returns the coin associated with the change address underneath the + // coinbaseKey pubkey, even though the change address has a different + // pubkey. + AddTx(CRecipient{GetScriptForRawPubKey({}), 1 * COIN, false /* subtract fee */}); + list = wallet->ListCoins(); + BOOST_CHECK_EQUAL(list.size(), 1); + BOOST_CHECK_EQUAL(boost::get(list.begin()->first).ToString(), coinbaseAddress); + BOOST_CHECK_EQUAL(list.begin()->second.size(), 2); + + // Lock both coins. Confirm number of available coins drops to 0. + std::vector available; + wallet->AvailableCoins(available); + BOOST_CHECK_EQUAL(available.size(), 2); + for (const auto& group : list) { + for (const auto& coin : group.second) { + wallet->LockCoin(COutPoint(coin.tx->GetHash(), coin.i)); + } + } + wallet->AvailableCoins(available); + BOOST_CHECK_EQUAL(available.size(), 0); + + // Confirm ListCoins still returns same result as before, despite coins + // being locked. + list = wallet->ListCoins(); + BOOST_CHECK_EQUAL(list.size(), 1); + BOOST_CHECK_EQUAL(boost::get(list.begin()->first).ToString(), coinbaseAddress); + BOOST_CHECK_EQUAL(list.begin()->second.size(), 2); +} + BOOST_AUTO_TEST_SUITE_END() From 108f04f2d973adac5313c7e4e17a59766a3cc1b6 Mon Sep 17 00:00:00 2001 From: Russell Yanofsky Date: Thu, 4 May 2017 17:02:12 -0400 Subject: [PATCH 4/4] Add missing LOCK2 in CWallet::GetAvailableBalance --- src/wallet/wallet.cpp | 2 ++ 1 file changed, 2 insertions(+) diff --git a/src/wallet/wallet.cpp b/src/wallet/wallet.cpp index e62b24cdbc..ca620b5af1 100644 --- a/src/wallet/wallet.cpp +++ b/src/wallet/wallet.cpp @@ -1986,6 +1986,8 @@ CAmount CWallet::GetLegacyBalance(const isminefilter& filter, int minDepth, cons CAmount CWallet::GetAvailableBalance(const CCoinControl* coinControl) const { + LOCK2(cs_main, cs_wallet); + CAmount balance = 0; std::vector vCoins; AvailableCoins(vCoins, true, coinControl);