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

pull Diapolo wants to merge 1 commits into bitcoin:master from Diapolo:wallet_add_GetImmatureCredit changing 2 files +20 −3

Diapolo commented at 6:35 AM on June 18, 2012: none

This harmonizes CWallet::GetImmatureBalance() with CWallet::GetBalance() and CWallet::GetUnconfirmedBalance().
gavinandresen commented at 1:45 PM on June 18, 2012: contributor

How should this be tested?
sipa commented at 1:56 PM on June 18, 2012: member

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 at 1:19 PM on June 20, 2012: none

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.
BitcoinPullTester commented at 11:41 PM on August 9, 2012: none

Automatic sanity-testing: PASSED, see http://jenkins.bluematt.me/pull-tester/ed1afd67c1d3f485a9ba3850ba2751e50d897f53 for binaries and test log.
luke-jr commented at 6:56 PM on August 12, 2012: member

@Diapolo Those builds from @BitcoinPullTester should help you out
Diapolo commented at 8:44 PM on August 13, 2012: none

@luke-jr How does this help? You mean using the Qt version from jenkins and using it on testnet?
luke-jr commented at 10:34 PM on August 13, 2012: member

@Diapolo I was thinking http://jenkins.bluematt.me/pull-tester/ed1afd67c1d3f485a9ba3850ba2751e50d897f53/bitcoin/src/bitcoind.exe
Diapolo commented at 8:33 AM on August 25, 2012: none

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.
sipa commented at 11:38 AM on August 25, 2012: member

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 at 12:18 AM on August 26, 2012: none

@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.
sipa commented at 12:19 AM on August 26, 2012: member

ACK on the changes to core; I didn't test the UI changes.
BitcoinPullTester commented at 1:07 PM on August 26, 2012: none

Automatic sanity-testing: PASSED, see http://jenkins.bluematt.me/pull-tester/dfdd4dac8291b55fd96feea3b4545a0a5389e86b for binaries and test log.
add CWalletTx::GetImmatureCredit() and use it in CWallet::GetImmatureBalance() 966a0e8cc9
Diapolo commented at 6:58 AM on November 13, 2012: none

Rebased!
in src/wallet.cpp:None in 966a0e8cc9
```
 925 | @@ -926,9 +926,8 @@ int64 CWallet::GetImmatureBalance() const
 926 |          LOCK(cs_wallet);
 927 |          for (map<uint256, CWalletTx>::const_iterator it = mapWallet.begin(); it != mapWallet.end(); ++it)
 928 |          {
 929 | -            const CWalletTx& pcoin = (*it).second;
 930 | -            if (pcoin.IsCoinBase() && pcoin.GetBlocksToMaturity() > 0 && pcoin.IsInMainChain())
 931 | -                nTotal += GetCredit(pcoin);
 932 | +            const CWalletTx* pcoin = &(*it).second;
```
laanwj commented at 7:10 AM on November 13, 2012:

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 commented at 7:23 AM on November 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 commented at 7:31 AM on November 13, 2012:

Satoshi is a very bad person to learn C++ style from :poodle: 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.
in src/wallet.h:None in 966a0e8cc9
```
 566 | @@ -563,6 +567,20 @@ class CWalletTx : public CMerkleTx
 567 |          return nCreditCached;
 568 |      }
 569 |  
 570 | +    int64 GetImmatureCredit(bool fUseCache=true) const
 571 | +    {
```
laanwj commented at 7:17 AM on November 13, 2012:

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

Diapolo commented at 7:21 AM on November 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 referenced this in commit f2b12807d0 on Nov 13, 2012
laanwj merged this on Nov 13, 2012
laanwj closed this on Nov 13, 2012
laudney referenced this in commit 80dc8604fe on Mar 19, 2014
suprnurd referenced this in commit fc406f2d82 on Dec 5, 2017
lateminer referenced this in commit 3de9bd56af on Jan 22, 2019
lateminer referenced this in commit c9c618e365 on May 6, 2020
DrahtBot locked this on Sep 8, 2021

Contributors

Linked (view graph)

#25824 UndefinedBehaviorSanitizer: stack-overflow in miniscript (descriptor_parse)