Add CMerkleTx::IsImmatureCoinBase method #13631

pull Empact wants to merge 1 commits into bitcoin:master from Empact:is-immature-coinbase changing 3 files +21 −11
  1. Empact commented at 5:20 AM on July 11, 2018: member

    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.

  3. fanquake added the label Refactoring on Jul 11, 2018
  4. fanquake added the label Wallet on Jul 11, 2018
  5. DrahtBot cross-referenced this on Jul 11, 2018 from issue Fix get balance by jnewbery
  6. in src/wallet/wallet.h:280 in e7a1d43644 outdated 
     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
    l2a5b1 commented at 2:59 PM on July 11, 2018:

    Nits: Maybe move the comment to the function body, because it's an implementation detail of IsImmatureCoinbase; and update the comment by replacing "is" with "returns".

  7. in src/wallet/wallet.cpp:1897 in e7a1d43644 outdated 
    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())
    l2a5b1 commented at 5:18 PM on July 11, 2018:

    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.

  8. in src/wallet/wallet.h:281 in e7a1d43644 outdated 
     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; }
    l2a5b1 commented at 5:33 PM on July 11, 2018:

    If we are going to rely on the result of GetBlocksToMaturity, and not defensively call IsCoinBase(), a unit test would be nice to verify the expected behaviour.

  9. l2a5b1 commented at 5:35 PM on July 11, 2018: contributor

    Nice refactor, utACK.

  10. practicalswift commented at 2:28 PM on July 12, 2018: contributor

    Concept ACK

  11. promag commented at 3:30 PM on July 12, 2018: member

    Concept ACK. Makes the intention clear. Agree with @251Labs points.

  12. Empact force-pushed on Jul 12, 2018
  13. Empact force-pushed on Jul 12, 2018
  14. Empact commented at 6:13 PM on July 12, 2018: member

    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

  15. Empact force-pushed on Jul 13, 2018
  16. Empact commented at 4:48 PM on July 13, 2018: member

    Moved the GetBlocksToMaturity assert out into #13657

  17. Empact force-pushed on Jul 13, 2018
  18. Empact force-pushed on Jul 13, 2018
  19. DrahtBot cross-referenced this on Jul 13, 2018 from issue Drop unused pindexRet arg to CMerkleTx::GetDepthInMainChain by Empact
  20. Empact force-pushed on Jul 14, 2018
  21. Empact commented at 3:49 AM on July 14, 2018: member

    Rebased for #13630, #13072

  22. l2a5b1 commented at 10:09 PM on July 19, 2018: contributor

    Nice, utACK 860e214

  23. promag commented at 10:24 PM on July 19, 2018: member

    utACK 860e214.

  24. MarcoFalke commented at 11:36 PM on July 19, 2018: member

    utACK 860e214f7ba899efae397205891181056adf3fc2

  25. DrahtBot cross-referenced this on Jul 22, 2018 from issue PSBT key path cleanups by sipa
  26. DrahtBot added the label Needs rebase on Jul 24, 2018
  27. Empact force-pushed on Jul 25, 2018
  28. Empact commented at 2:57 PM on July 25, 2018: member

    Rebased for #12257

  29. DrahtBot removed the label Needs rebase on Jul 25, 2018
  30. MarcoFalke commented at 3:55 PM on July 27, 2018: member

    re-utACK bb653872be8d251c21ee32c2948100b7febbd477

  31. promag commented at 6:26 PM on July 27, 2018: member

    re-utACK bb65387.

  32. Add CMerkleTx::IsImmatureCoinBase method
    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.
    23f4343781
  33. Empact force-pushed on Jul 29, 2018
  34. Empact commented at 11:51 PM on July 29, 2018: member

    Noticed the whitespace was off. Diff:

    diff --git a/src/wallet/wallet.cpp b/src/wallet/wallet.cpp
index fc6f03a16..540a7b0fc 100644
--- a/src/wallet/wallet.cpp
+++ b/src/wallet/wallet.cpp
@@ -1926,8 +1926,7 @@ CAmount CWalletTx::GetCredit(const isminefilter& filter) const
 
 CAmount CWalletTx::GetImmatureCredit(bool fUseCache) const
 {
-    if (IsImmatureCoinBase() && IsInMainChain())
-    {
+    if (IsImmatureCoinBase() && IsInMainChain()) {
         if (fUseCache && fImmatureCreditCached)
             return nImmatureCreditCached;
         nImmatureCreditCached = pwallet->GetCredit(*tx, ISMINE_SPENDABLE);
@@ -1985,8 +1984,7 @@ CAmount CWalletTx::GetAvailableCredit(bool fUseCache, const isminefilter& filter
 
 CAmount CWalletTx::GetImmatureWatchOnlyCredit(const bool fUseCache) const
 {
-    if (IsImmatureCoinBase() && IsInMainChain())
-    {
+    if (IsImmatureCoinBase() && IsInMainChain()) {
         if (fUseCache && fImmatureWatchCreditCached)
             return nImmatureWatchCreditCached;
         nImmatureWatchCreditCached = pwallet->GetCredit(*tx, ISMINE_WATCH_ONLY);
@@ -4399,8 +4397,8 @@ int CMerkleTx::GetBlocksToMaturity() const
 
 bool CMerkleTx::IsImmatureCoinBase() const
 {
-   // note GetBlocksToMaturity is 0 for non-coinbase tx
-   return GetBlocksToMaturity() > 0;
+    // note GetBlocksToMaturity is 0 for non-coinbase tx
+    return GetBlocksToMaturity() > 0;
 }
 
 bool CWalletTx::AcceptToMemoryPool(const CAmount& nAbsurdFee, CValidationState& state)
  35. DrahtBot cross-referenced this on Jul 31, 2018 from issue [wallet] Kill accounts by jnewbery
  36. DrahtBot commented at 12:27 AM on August 1, 2018: contributor

    <!--e57a25ab6845829454e8d69fc972939a-->Note to reviewers: This pull request conflicts with the following ones:

    • #14023 (Remove accounts rpcs by jnewbery)
    • #13083 (Add compile time checking for cs_main runtime locking assertions by practicalswift)
    • #10973 (Refactor: separate wallet from node by ryanofsky)
    • #10102 ([experimental] Multiprocess bitcoin by ryanofsky)

    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.

  37. DrahtBot cross-referenced this on Aug 22, 2018 from issue Remove accounts rpcs by jnewbery
  38. laanwj commented at 2:53 PM on August 25, 2018: member

    utACK 23f434378153cf764230066662f3ec3ad614ff30

  39. laanwj merged this on Aug 25, 2018
  40. laanwj closed this on Aug 25, 2018
  41. laanwj referenced this in commit 776fa60c4b on Aug 25, 2018
  42. Bushstar cross-referenced this on Sep 4, 2018 from issue commits from bitcoin/master by Bushstar
  43. ryanofsky cross-referenced this on Sep 5, 2018 from issue Refactor: separate wallet from node by ryanofsky
  44. Munkybooty referenced this in commit 119f6b42b7 on Jun 30, 2021
  45. Munkybooty referenced this in commit 0ee85e49ba on Jun 30, 2021
  46. Munkybooty referenced this in commit e8a5281918 on Jul 1, 2021
  47. Munkybooty referenced this in commit 527007bf01 on Jul 1, 2021
  48. Munkybooty referenced this in commit d48320aa4d on Jul 2, 2021
  49. Munkybooty referenced this in commit 97470e86c2 on Jul 2, 2021
  50. Munkybooty referenced this in commit 19b9e74026 on Jul 2, 2021
  51. bitcoin locked this on Sep 8, 2021
Contributors
Empactl2a5b1practicalswiftpromagMarcoFalkeDrahtBotlaanwj
Labels
RefactoringWallet
Linked (view graph)
#10973 Refactor: separate wallet from node#13072 Update createmultisig RPC to support segwit#13657 wallet: assert to ensure accuracy of CMerkleTx::GetBlocksToMaturity
github-metadata-mirror

This is a metadata mirror of the GitHub repository bitcoin/bitcoin. This site is not affiliated with GitHub. Content is generated from a GitHub metadata backup.
generated: 2026-05-19 06:54 UTC