Remove unnecessary call to CTransaction::IsCoinBase() #16103

Replaces CTransaction::IsCoinbase() with the same cheap check as: https://github.com/bitcoin/bitcoin/blob/76e2cded477bc483ec610212bdadf21fe35292d4/src/validation.cpp#L2048

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

-0 IMO better to rely on the definition in IsCoinBase. I think the line you cite may be for coherence with https://github.com/bitcoin/bitcoin/blob/76e2cded477bc483ec610212bdadf21fe35292d4/src/validation.cpp#L1973

/cc @sipa #4835

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

@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? :-)

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

PR timeout.