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).
Yes. They belong to CWallet rather then WalletModel. The coincontrol's ListCoins function can potentially be useful for non GUI features. My only nit is if we should rename ListCoins to something more meaningful.
Concept ACK.
1989 | @@ -1983,6 +1990,19 @@ CAmount CWallet::GetLegacyBalance(const isminefilter& filter, int minDepth, cons 1990 | return balance; 1991 | } 1992 | 1993 | +CAmount CWallet::GetAvailableBalance(const CCoinControl* coinControl) const 1994 | +{ 1995 | + CAmount balance = 0;
nit: might be nice to LOCK here.
Not sure if a lock is required at this point. Do you want to protect const CCoinControl* coinControl?
AvailableCoins() does protect via cs_main/cs_wallet.
We have pointers in the COutput to entires in mapWallet, which can (theoretically) be delted out from under us thanks to the zap rpc.
Added in 14c1174c9aadb446c9a9e16626611f476610f4ba
2105 | + if (depth >= 0 && output.n < it->second.tx->vout.size() && 2106 | + IsMine(it->second.tx->vout[output.n]) == ISMINE_SPENDABLE) { 2107 | + CTxDestination address; 2108 | + if (ExtractDestination(FindNonChangeParentOutput(*it->second.tx, output.n).scriptPubKey, address)) { 2109 | + result[address].emplace_back( 2110 | + &it->second, output.n, depth, true /* spendable */, true /* solvable */, false /* safe */);
Why did you change to false for safe here...looks like the old code did true? Does that have any effect?
Why did you change to false for safe here...looks like the old code did true? Does that have any effect?
It has no effect (I'm not even exposing it to Qt any more after 7a7795fdd5fe86b9918b8055666b8365e4342eb6 in #10244) and false seemed like a more correct value.
588 | @@ -596,37 +589,11 @@ bool WalletModel::isSpent(const COutPoint& outpoint) const
589 | void WalletModel::listCoins(std::map<QString, std::vector<COutput> >& mapCoins) const
590 | {
591 | std::vector<COutput> vCoins;
We dont need vCoins anymore either, no?
Removed in f8bfecf09037f3c49594ddc1ccadb9f98a354a84.
Looks good, seems reasonable, though I hope we can delete half the code here (see #10337).
Squashed 14c1174c9aadb446c9a9e16626611f476610f4ba -> b6a9cc48fb950d1f0daa6c3db1df2bbc2b591ab0 (pr/ipc-move.2 -> pr/ipc-move.3)
78 | @@ -80,8 +79,9 @@ QModelIndex FindTx(const QAbstractItemModel& model, const uint256& txid) 79 | // src/qt/test/test_bitcoin-qt -platform xcb # Linux 80 | // src/qt/test/test_bitcoin-qt -platform windows # Windows 81 | // src/qt/test/test_bitcoin-qt -platform cocoa # macOS 82 | -void WalletTests::walletTests() 83 | +void TestSendCoins() 84 | { 85 | + return;
Were you planning on re-enabling this?
Wow, good catch. Fixed in 01f847bf2b0ea395f8533e4d52c9422807af0566. This was just an accident, I didn't mean to disable it at all.
Squashed 01f847bf2b0ea395f8533e4d52c9422807af0566 -> 38d3864eccd6abbbcf22619c8109a1b6e015f901 (pr/ipc-move.4 -> pr/ipc-move.5)
622 | @@ -657,10 +623,7 @@ void WalletModel::listLockedCoins(std::vector<COutPoint>& vOutpts)
623 | void WalletModel::loadReceiveRequests(std::vector<std::string>& vReceiveRequests)
624 | {
625 | LOCK(wallet->cs_wallet);
nit: maybe push this lock down into GetDestValues?
Done in e60a3110c12a47fdcf7fb488b17ae145058c020e
2081 | @@ -2060,6 +2082,59 @@ void CWallet::AvailableCoins(std::vector<COutput>& vCoins, bool fOnlySafe, const 2082 | } 2083 | } 2084 | 2085 | +std::map<CTxDestination, std::vector<COutput>> CWallet::ListCoins() const 2086 | +{ 2087 | + std::map<CTxDestination, std::vector<COutput>> result;
nit: might be nice to AssertLockHeld here (because you cant call this without cs_wallet due to the COutput returns).
Not currently possible to just add the assert because Qt doesn't acquire the lock, but I added a todo in fe8c036cb87c62bee653fe581544c714ce8808d2
utACK 38d3864eccd6abbbcf22619c8109a1b6e015f901
Squashed fe8c036cb87c62bee653fe581544c714ce8808d2 -> a226c97c336c44b4b8320328debea69e576f0fa8 (pr/ipc-move.6 -> pr/ipc-move.7)
Add unit tests for some walletmodel functions that will be refactored & moved
in the next commit.
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).
After previous refactoring, the tests make more sense here.
Rebased a226c97c336c44b4b8320328debea69e576f0fa8 -> 108f04f2d973adac5313c7e4e17a59766a3cc1b6 (pr/ipc-move.7 -> pr/ipc-move.8) because of conflict with #8952.
utACK 108f04f