add CWalletTx::GetImmatureCredit() and use it in CWallet::GetImmatureBalance() #1479

Merged
merged 1 commit into from Nov 13, 2012

Projects

None yet

6 participants

Diapolo commented Jun 18, 2012

This harmonizes CWallet::GetImmatureBalance() with CWallet::GetBalance() and CWallet::GetUnconfirmedBalance().

Contributor

How should this be tested?

Owner
sipa commented Jun 18, 2012

I suppose you could temporarily add an immature_amount to the gettransaction() RPC, and immature_balance to getinfo(), and mine a few blocks on testnet.

Diapolo commented Jun 20, 2012

I "only" verified this patch with Bitcoin-Qt on testnet, I'm not sure how to test this with bitcoind, as I can't compile it. Help would be appreciated, as this testing is blocking a merge.

Automatic sanity-testing: PASSED, see http://jenkins.bluematt.me/pull-tester/ed1afd67c1d3f485a9ba3850ba2751e50d897f53 for binaries and test log.

Member
luke-jr commented Aug 12, 2012

@Diapolo Those builds from @BitcoinPullTester should help you out

Diapolo commented Aug 13, 2012

@luke-jr How does this help? You mean using the Qt version from jenkins and using it on testnet?

Diapolo commented Aug 25, 2012

Guys, I setup bitcoind and can use it to pass RPC commands, but I simply don't know how you want me to test the function there, as GetImmatureBalance() seems to be only used by Qt?

I could for sure add obj.push_back(Pair("walletversion", pwalletMain->GetImmatureBalance())); to RPC getinfo, but as I can't compile bitcoind I would rely on @BitcoinPullTester to create a bitcoind.exe for me and need to revert that change before this would be considered mergeable.

Owner
sipa commented Aug 25, 2012

GetImmatureBalance looks correct from reading it, and it's only used within the Qt code. I'm sure we can use it in RPC code as well, but let's do that later.

Diapolo commented Aug 26, 2012

@sipa I didn't want to push or create a usage scenario for RPC, I just wanted to say I dunno how to further test this code :). As I said in Qt it does the job and asked if it will get accepted that way or what I can do now.

Owner
sipa commented Aug 26, 2012

ACK on the changes to core; I didn't test the UI changes.

Automatic sanity-testing: PASSED, see http://jenkins.bluematt.me/pull-tester/dfdd4dac8291b55fd96feea3b4545a0a5389e86b for binaries and test log.

Diapolo commented Nov 13, 2012

Rebased!

@laanwj laanwj commented on the diff Nov 13, 2012
src/wallet.cpp
@@ -926,9 +926,8 @@ int64 CWallet::GetImmatureBalance() const
LOCK(cs_wallet);
for (map<uint256, CWalletTx>::const_iterator it = mapWallet.begin(); it != mapWallet.end(); ++it)
{
- const CWalletTx& pcoin = (*it).second;
- if (pcoin.IsCoinBase() && pcoin.GetBlocksToMaturity() > 0 && pcoin.IsInMainChain())
- nTotal += GetCredit(pcoin);
+ const CWalletTx* pcoin = &(*it).second;
laanwj
laanwj Nov 13, 2012 Owner

Why change this to a pointer instead of a reference (&)? IMO &(*it).second; is slightly more ugly code, and in C++ it's generally advised to use references instead of pointers where possible (for type safety etc).

Diapolo
Diapolo Nov 13, 2012

Same as below, I wanted this to be similar to the other functions, perhaps it would be a good idea to update all these to be more C++ style then? But I'm not sure if core devs would ACK such a change on that code-part?

laanwj
laanwj Nov 13, 2012 Owner

Satoshi is a very bad person to learn C++ style from 🐩 If you want an example of well-structured, readable C++ I can recommend reading source from LLVM.
And I'm not sure either, it doesn't warrant changing all the functions I guess... maybe just leave it like this then.

@laanwj laanwj commented on the diff Nov 13, 2012
src/wallet.h
@@ -563,6 +567,20 @@ class CWalletTx : public CMerkleTx
return nCreditCached;
}
+ int64 GetImmatureCredit(bool fUseCache=true) const
+ {
laanwj
laanwj Nov 13, 2012 Owner

ACK
(I wonder why this implementation is in the .h, not the .cpp, but I suppose you simply followed the example of GetAvailableCredit)

Diapolo
Diapolo Nov 13, 2012

Indeed, I just used what was there. Sometimes I have not that wide view/knowlegde you have in the area of coding :).

@laanwj laanwj merged commit f2b1280 into bitcoin:master Nov 13, 2012
Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment