Remove unnecessary call to CTransaction::IsCoinBase()

[promag]

Replaces CTransaction::IsCoinbase() with the same cheap check as:

bitcoin/src/validation.cpp

Line 2048 in 76e2cde

if (i > 0) {

[promag]

Am I missing something? Is there a case where this change is wrong?

[Empact]

-0 IMO better to rely on the definition in IsCoinBase. I think the line you cite may be for coherence with

bitcoin/src/validation.cpp

Line 1973 in 76e2cde

blockundo.vtxundo.reserve(block.vtx.size() - 1);

/cc @sipa #4835

[promag]

@Empact that commit also replaces IsCoinBase() with i > 0. I just think it could be consistent.

[practicalswift]

@promag I think this change makes the code less clear for new contributors who might not be aware of the fact that the first transaction must be a coinbase transaction. If the rationale is performance: the call to IsCoinBase() should be essentially free, no? :-)

[mzumsande]

For me as a new contributor, the fact that there is currently a mix (lines 2048 and 2051 use i) is more confusing. If we don't want to rely on i>0 in spite of having called CheckBlock() at the beginning of ConnectBlock(), why not use IsCoinBase() consistently (maybe call only once per tx and store in local bool for performance).

[promag]

PR timeout.