Add CMerkleTx::IsImmatureCoinBase method by Empact · Pull Request #13631 · bitcoin/bitcoin

Empact · 2018-07-11T05:20:43Z

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.

Empact · 2018-07-11T05:23:17Z

For reference:

bitcoin/src/wallet/wallet.cpp

Lines 4435 to 4438 in 062738c

    
           int CMerkleTx::GetBlocksToMaturity() const 
        
           { 
        
               if (!IsCoinBase()) 
        
                   return 0;

l2a5b1

Nice refactor, utACK.

l2a5b1 · 2018-07-11T14:59:55Z

src/wallet/wallet.h

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

l2a5b1 · 2018-07-11T17:18:02Z

src/wallet/wallet.cpp

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.

l2a5b1 · 2018-07-11T17:33:48Z

src/wallet/wallet.h

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.

practicalswift · 2018-07-12T14:28:19Z

Concept ACK

promag · 2018-07-12T15:30:19Z

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

Empact · 2018-07-12T18:13:43Z

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

Empact · 2018-07-13T16:48:11Z

Moved the GetBlocksToMaturity assert out into #13657

Empact · 2018-07-14T03:49:24Z

Rebased for #13630, #13072

l2a5b1 · 2018-07-19T22:09:02Z

Nice, utACK 860e214

promag · 2018-07-19T22:24:03Z

utACK 860e214.

maflcko · 2018-07-19T23:36:06Z

utACK 860e214f7ba899efae397205891181056adf3fc2

Empact · 2018-07-25T14:57:24Z

Rebased for #12257

maflcko · 2018-07-27T15:55:36Z

re-utACK bb653872be8d251c21ee32c2948100b7febbd477

promag · 2018-07-27T18:26:18Z

re-utACK bb65387.

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.

Empact · 2018-07-29T23:51:06Z

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)

DrahtBot · 2018-08-01T00:27:54Z

Note to reviewers: This pull request conflicts with the following ones:

Remove accounts rpcs #14023 (Remove accounts rpcs by jnewbery)
Add compile time checking for cs_main runtime locking assertions #13083 (Add compile time checking for cs_main runtime locking assertions by practicalswift)
Refactor: separate wallet from node #10973 (Refactor: separate wallet from node by ryanofsky)
Multiprocess bitcoin #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.

laanwj · 2018-08-25T14:53:02Z

utACK 23f4343

23f4343 Add CMerkleTx::IsImmatureCoinBase method (Ben Woosley) Pull request description: 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. Tree-SHA512: 4470d07404a0707144f9827b9a94c5c4905f23ee6f9248edc5df599a59d28e21ea0201d8abe5d5d73b39cb05b60c861ea8e04767eef04433e2ee95dcfed653ee

23f4343 Add CMerkleTx::IsImmatureCoinBase method (Ben Woosley) Pull request description: 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. Tree-SHA512: 4470d07404a0707144f9827b9a94c5c4905f23ee6f9248edc5df599a59d28e21ea0201d8abe5d5d73b39cb05b60c861ea8e04767eef04433e2ee95dcfed653ee # Conflicts: # src/wallet/wallet.h

fanquake added Refactoring Wallet labels Jul 11, 2018

DrahtBot mentioned this pull request Jul 11, 2018

Fix get balance #13566

Merged

l2a5b1 reviewed Jul 11, 2018

View reviewed changes

Empact force-pushed the is-immature-coinbase branch 2 times, most recently from d0fe31e to 1503a9a Compare July 12, 2018 18:10

Empact force-pushed the is-immature-coinbase branch from 1503a9a to 192075f Compare July 13, 2018 16:44

Empact force-pushed the is-immature-coinbase branch 2 times, most recently from 86a1caf to 328d830 Compare July 13, 2018 16:50

DrahtBot mentioned this pull request Jul 13, 2018

Drop unused pindexRet arg to CMerkleTx::GetDepthInMainChain #13630

Merged

Empact force-pushed the is-immature-coinbase branch from 328d830 to 860e214 Compare July 14, 2018 03:44

DrahtBot mentioned this pull request Jul 22, 2018

PSBT key path cleanups #13723

Merged

DrahtBot added the Needs rebase label Jul 24, 2018

Empact force-pushed the is-immature-coinbase branch from 860e214 to bb65387 Compare July 25, 2018 14:57

DrahtBot removed the Needs rebase label Jul 25, 2018

Empact force-pushed the is-immature-coinbase branch from bb65387 to 23f4343 Compare July 29, 2018 23:50

DrahtBot mentioned this pull request Jul 31, 2018

[wallet] Kill accounts #13825

Closed

DrahtBot mentioned this pull request Aug 22, 2018

Remove accounts rpcs #14023

Merged

laanwj merged commit 23f4343 into bitcoin:master Aug 25, 2018

Bushstar mentioned this pull request Sep 4, 2018

commits from bitcoin/master FeatherCoin/Feathercoin#359

Merged

ryanofsky mentioned this pull request Sep 5, 2018

Refactor: separate wallet from node #10973

Merged

bitcoin locked as resolved and limited conversation to collaborators Sep 8, 2021

Conversation

Empact commented Jul 11, 2018 • edited Loading Uh oh! There was an error while loading. Please reload this page.

Uh oh!

Uh oh!

Empact commented Jul 11, 2018

Uh oh!

l2a5b1 left a comment

Choose a reason for hiding this comment

Uh oh!

l2a5b1 Jul 11, 2018

Choose a reason for hiding this comment

Uh oh!

l2a5b1 Jul 11, 2018

Choose a reason for hiding this comment

Uh oh!

l2a5b1 Jul 11, 2018 • edited Loading Uh oh! There was an error while loading. Please reload this page.

Uh oh!

Choose a reason for hiding this comment

Uh oh!

practicalswift commented Jul 12, 2018

Uh oh!

promag commented Jul 12, 2018

Uh oh!

Empact commented Jul 12, 2018

Uh oh!

Empact commented Jul 13, 2018

Uh oh!

Empact commented Jul 14, 2018

Uh oh!

l2a5b1 commented Jul 19, 2018

Uh oh!

promag commented Jul 19, 2018

Uh oh!

maflcko commented Jul 19, 2018

Uh oh!

Empact commented Jul 25, 2018

Uh oh!

maflcko commented Jul 27, 2018

Uh oh!

promag commented Jul 27, 2018

Uh oh!

Empact commented Jul 29, 2018

Uh oh!

DrahtBot commented Aug 1, 2018 • edited Loading Uh oh! There was an error while loading. Please reload this page.

Uh oh!

Uh oh!

laanwj commented Aug 25, 2018

Uh oh!

Reviewers

Assignees

Labels

Projects

Milestone

Development

Uh oh!

8 participants

Empact commented Jul 11, 2018 •

edited

Loading

l2a5b1 Jul 11, 2018 •

edited

Loading

DrahtBot commented Aug 1, 2018 •

edited

Loading