All but one call to GetBlocksToMaturity is testing it relative to 0
for the purposes of determining whether the coinbase tx is immature.
In such case, the value greater than 0 implies that the tx is coinbase,
so there is no need to separately test that status.
This names the concept for easy singular use.
276@@ -277,6 +277,8 @@ class CMerkleTx
277
278 const uint256& GetHash() const { return tx->GetHash(); }
279 bool IsCoinBase() const { return tx->IsCoinBase(); }
280+ // note GetBlocksToMaturity is 0 for non-coinbase tx
IsImmatureCoinbase; and update the comment by replacing “is” with “returns”.
1923@@ -1924,7 +1924,7 @@ CAmount CWalletTx::GetDebit(const isminefilter& filter) const
1924 CAmount CWalletTx::GetCredit(const isminefilter& filter) const
1925 {
1926 // Must wait until coinbase is safely deep enough in the chain before valuing it
1927- if (IsCoinBase() && GetBlocksToMaturity() > 0)
1928+ if (IsImmatureCoinBase())
It seems safe to optimize this boolean expression and remove the IsCoinBase() condition in IsImmatureCoinBase, because GetBlocksToMaturity already calls IsCoinBase, which effectively makes this condition redundant.
I do however appreciate the defensiveness of having the redundantIsCoinBase() condition; it is not specified that GetBlocksToMaturity must return 0 if it is not a coinbase transaction.
Therefore, if we optimize the boolean expression in IsImmatureCoinBase and fully rely on the result of GetBlocksToMaturity, I would like to suggest to add a comment to the interface documentation of the CMerkleTx::GetBlocksToMaturity method that explicitly states the postcondition that GetBlocksToMaturity must return 0 if it is not a coinbase transaction.
276@@ -277,6 +277,8 @@ class CMerkleTx
277
278 const uint256& GetHash() const { return tx->GetHash(); }
279 bool IsCoinBase() const { return tx->IsCoinBase(); }
280+ // note GetBlocksToMaturity is 0 for non-coinbase tx
281+ bool IsImmatureCoinBase() const { return GetBlocksToMaturity() > 0; }
GetBlocksToMaturity, and not defensively call IsCoinBase(), a unit test would be nice to verify the expected behaviour.
Added docs, moved the implementations together.
I also noticed that the result of GetBlocksToMaturity would be inaccurate if the TX was marked as conflicted - my impression is that coinbase transactions can’t be conflicting, so I added an assert to make that expectation explicit:
https://github.com/bitcoin/bitcoin/pull/13631/files#diff-b2bb174788c7409b671c46ccc86034bdR4440
All but one call to GetBlocksToMaturity is testing it relative to 0
for the purposes of determining whether the coinbase tx is immature.
In such case, the value greater than 0 implies that the tx is coinbase,
so there is no need to separately test that status.
This names the concept for easy singular use.
Noticed the whitespace was off. Diff:
0diff --git a/src/wallet/wallet.cpp b/src/wallet/wallet.cpp
1index fc6f03a16..540a7b0fc 100644
2--- a/src/wallet/wallet.cpp
3+++ b/src/wallet/wallet.cpp
4@@ -1926,8 +1926,7 @@ CAmount CWalletTx::GetCredit(const isminefilter& filter) const
5
6 CAmount CWalletTx::GetImmatureCredit(bool fUseCache) const
7 {
8- if (IsImmatureCoinBase() && IsInMainChain())
9- {
10+ if (IsImmatureCoinBase() && IsInMainChain()) {
11 if (fUseCache && fImmatureCreditCached)
12 return nImmatureCreditCached;
13 nImmatureCreditCached = pwallet->GetCredit(*tx, ISMINE_SPENDABLE);
14@@ -1985,8 +1984,7 @@ CAmount CWalletTx::GetAvailableCredit(bool fUseCache, const isminefilter& filter
15
16 CAmount CWalletTx::GetImmatureWatchOnlyCredit(const bool fUseCache) const
17 {
18- if (IsImmatureCoinBase() && IsInMainChain())
19- {
20+ if (IsImmatureCoinBase() && IsInMainChain()) {
21 if (fUseCache && fImmatureWatchCreditCached)
22 return nImmatureWatchCreditCached;
23 nImmatureWatchCreditCached = pwallet->GetCredit(*tx, ISMINE_WATCH_ONLY);
24@@ -4399,8 +4397,8 @@ int CMerkleTx::GetBlocksToMaturity() const
25
26 bool CMerkleTx::IsImmatureCoinBase() const
27 {
28- // note GetBlocksToMaturity is 0 for non-coinbase tx
29- return GetBlocksToMaturity() > 0;
30+ // note GetBlocksToMaturity is 0 for non-coinbase tx
31+ return GetBlocksToMaturity() > 0;
32 }
33
34 bool CWalletTx::AcceptToMemoryPool(const CAmount& nAbsurdFee, CValidationState& state)
If you consider this pull request important, please also help to review the conflicting pull requests. Ideally, start with the one that should be merged first.
