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.

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

[qt] Move some WalletModel functions into CWallet #10295

Merged
merged 4 commits into from May 23, 2017

Conversation

Projects
None yet
4 participants
@ryanofsky
Copy link
Contributor

ryanofsky commented Apr 28, 2017

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).

@jonasschnelli

This comment has been minimized.

Copy link
Member

jonasschnelli commented Apr 28, 2017

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.

@TheBlueMatt
Copy link
Contributor

TheBlueMatt left a comment

Looks good, seems reasonable, though I hope we can delete half the code here (see #10337).

@@ -1983,6 +1990,19 @@ CAmount CWallet::GetLegacyBalance(const isminefilter& filter, int minDepth, cons
return balance;
}

CAmount CWallet::GetAvailableBalance(const CCoinControl* coinControl) const
{
CAmount balance = 0;

This comment has been minimized.

@TheBlueMatt

TheBlueMatt May 4, 2017

Contributor

nit: might be nice to LOCK here.

This comment has been minimized.

@jonasschnelli

jonasschnelli May 4, 2017

Member

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.

This comment has been minimized.

@TheBlueMatt

TheBlueMatt May 4, 2017

Contributor

We have pointers in the COutput to entires in mapWallet, which can (theoretically) be delted out from under us thanks to the zap rpc.

This comment has been minimized.

@ryanofsky

ryanofsky May 4, 2017

Author Contributor

Added in 14c1174

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 */);

This comment has been minimized.

@TheBlueMatt

TheBlueMatt May 4, 2017

Contributor

Why did you change to false for safe here...looks like the old code did true? Does that have any effect?

This comment has been minimized.

@ryanofsky

ryanofsky May 4, 2017

Author Contributor

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 7a7795f in #10244) and false seemed like a more correct value.

@@ -596,37 +589,11 @@ bool WalletModel::isSpent(const COutPoint& outpoint) const
void WalletModel::listCoins(std::map<QString, std::vector<COutput> >& mapCoins) const
{
std::vector<COutput> vCoins;

This comment has been minimized.

@TheBlueMatt

TheBlueMatt May 4, 2017

Contributor

We dont need vCoins anymore either, no?

This comment has been minimized.

@ryanofsky

ryanofsky May 4, 2017

Author Contributor

Removed in f8bfecf.

@ryanofsky ryanofsky force-pushed the ryanofsky:pr/ipc-move branch from 861be88 to 7e9ee91 May 4, 2017

@ryanofsky
Copy link
Contributor Author

ryanofsky left a comment

@@ -596,37 +589,11 @@ bool WalletModel::isSpent(const COutPoint& outpoint) const
void WalletModel::listCoins(std::map<QString, std::vector<COutput> >& mapCoins) const
{
std::vector<COutput> vCoins;

This comment has been minimized.

@ryanofsky

ryanofsky May 4, 2017

Author Contributor

Removed in f8bfecf.

@@ -1983,6 +1990,19 @@ CAmount CWallet::GetLegacyBalance(const isminefilter& filter, int minDepth, cons
return balance;
}

CAmount CWallet::GetAvailableBalance(const CCoinControl* coinControl) const
{
CAmount balance = 0;

This comment has been minimized.

@ryanofsky

ryanofsky May 4, 2017

Author Contributor

Added in 14c1174

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 */);

This comment has been minimized.

@ryanofsky

ryanofsky May 4, 2017

Author Contributor

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 7a7795f in #10244) and false seemed like a more correct value.

{
return;

This comment has been minimized.

@TheBlueMatt

TheBlueMatt May 5, 2017

Contributor

Were you planning on re-enabling this?

This comment has been minimized.

@ryanofsky

ryanofsky May 5, 2017

Author Contributor

Wow, good catch. Fixed in 01f847b. This was just an accident, I didn't mean to disable it at all.

@ryanofsky ryanofsky force-pushed the ryanofsky:pr/ipc-move branch 4 times, most recently from b6a9cc4 to 38d3864 May 5, 2017

@ryanofsky
Copy link
Contributor Author

ryanofsky left a comment

{
return;

This comment has been minimized.

@ryanofsky

ryanofsky May 5, 2017

Author Contributor

Wow, good catch. Fixed in 01f847b. This was just an accident, I didn't mean to disable it at all.

@TheBlueMatt
Copy link
Contributor

TheBlueMatt left a comment

utACK 38d3864

@@ -657,10 +623,7 @@ void WalletModel::listLockedCoins(std::vector<COutPoint>& vOutpts)
void WalletModel::loadReceiveRequests(std::vector<std::string>& vReceiveRequests)
{
LOCK(wallet->cs_wallet);

This comment has been minimized.

@TheBlueMatt

TheBlueMatt May 5, 2017

Contributor

nit: maybe push this lock down into GetDestValues?

This comment has been minimized.

@ryanofsky

ryanofsky May 9, 2017

Author Contributor

Done in e60a311

@@ -2060,6 +2082,59 @@ void CWallet::AvailableCoins(std::vector<COutput>& vCoins, bool fOnlySafe, const
}
}

std::map<CTxDestination, std::vector<COutput>> CWallet::ListCoins() const
{
std::map<CTxDestination, std::vector<COutput>> result;

This comment has been minimized.

@TheBlueMatt

TheBlueMatt May 5, 2017

Contributor

nit: might be nice to AssertLockHeld here (because you cant call this without cs_wallet due to the COutput returns).

This comment has been minimized.

@ryanofsky

ryanofsky May 9, 2017

Author Contributor

Not currently possible to just add the assert because Qt doesn't acquire the lock, but I added a todo in fe8c036

@ryanofsky ryanofsky force-pushed the ryanofsky:pr/ipc-move branch from 38d3864 to a226c97 May 9, 2017

@ryanofsky
Copy link
Contributor Author

ryanofsky left a comment

@@ -657,10 +623,7 @@ void WalletModel::listLockedCoins(std::vector<COutPoint>& vOutpts)
void WalletModel::loadReceiveRequests(std::vector<std::string>& vReceiveRequests)
{
LOCK(wallet->cs_wallet);

This comment has been minimized.

@ryanofsky

ryanofsky May 9, 2017

Author Contributor

Done in e60a311

@@ -2060,6 +2082,59 @@ void CWallet::AvailableCoins(std::vector<COutput>& vCoins, bool fOnlySafe, const
}
}

std::map<CTxDestination, std::vector<COutput>> CWallet::ListCoins() const
{
std::map<CTxDestination, std::vector<COutput>> result;

This comment has been minimized.

@ryanofsky

ryanofsky May 9, 2017

Author Contributor

Not currently possible to just add the assert because Qt doesn't acquire the lock, but I added a todo in fe8c036

ryanofsky added some commits Apr 28, 2017

[test] Add tests for some walletmodel functions
Add unit tests for some walletmodel functions that will be refactored & moved
in the next commit.
[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).
[test] Move some tests from qt -> wallet
After previous refactoring, the tests make more sense here.

@ryanofsky ryanofsky force-pushed the ryanofsky:pr/ipc-move branch 2 times, most recently to 108f04f May 17, 2017

@ryanofsky

This comment has been minimized.

Copy link
Contributor Author

ryanofsky commented May 17, 2017

Rebased a226c97 -> 108f04f (pr/ipc-move.7 -> pr/ipc-move.8) because of conflict with #8952.

@laanwj laanwj added this to Blockers in High-priority for review May 18, 2017

@laanwj

This comment has been minimized.

Copy link
Member

laanwj commented May 23, 2017

utACK 108f04f

@laanwj laanwj merged commit 108f04f into bitcoin:master May 23, 2017

1 check passed

continuous-integration/travis-ci/pr The Travis CI build passed
Details

laanwj added a commit that referenced this pull request May 23, 2017

Merge #10295: [qt] Move some WalletModel functions into CWallet
108f04f Add missing LOCK2 in CWallet::GetAvailableBalance (Russell Yanofsky)
429aa9e [test] Move some tests from qt -> wallet (Russell Yanofsky)
d944bd7 [qt] Move some WalletModel functions into CWallet (Russell Yanofsky)
ef8ca17 [test] Add tests for some walletmodel functions (Russell Yanofsky)

Tree-SHA512: f6384d9f2ff3f7fb173d414588c3e7dc8c311b8ed2ce2b0979fb824a0ed83a7302890ccd3d83197f07f6fdcb6b1ca151584d90ea1961d88dfe8956c87087cde8

@sipa sipa removed this from Blockers in High-priority for review May 23, 2017

MarcoFalke added a commit that referenced this pull request Aug 31, 2018

Merge #10605: Add AssertLockHeld assertions in CWallet::ListCoins
62b6f0f Add EXCLUSIVE_LOCKS_REQUIRED to CWallet::ListCoins (Russell Yanofsky)
545e85e Add AssertLockHeld assertions in CWallet::ListCoins (Russell Yanofsky)

Pull request description:

  Fixes TODO from #10295

Tree-SHA512: 2dd03a8217e5e1313aa2119cb530e0c0daf3ae3a751b6fdec611df57b8090201a90b52ff05f8f696e978a1344aaf21989d67a03beb5ef6ef79b77be38d04b451
Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment
You can’t perform that action at this time.