[Core] Remove BIP30 check

[random-zebra]

A bit of history

Two transactions can have the same txid if their parents are identical, since the txids of the parents are included in a transaction.
Coinbases have no parents, so it used to be possible for two of them to be identical.
Further, by building on duplicate coinbases, duplicate normal transactions were possible as well (http://r6.ca/blog/20120206T005236Z.html).

In order to remove the possibility of having duplicate transaction ids, Bitcoin introduced, with BIP30, the following consensus rule:

• Blocks are not allowed to contain a transaction whose identifier matches that of an earlier, not-fully-spent transaction in the same chain. [1]

This rule was enforced by verifying (in ConnectBlock) that none of the transactions in the block was overwriting an already existent non-pruned CCoins entry.

BIP34 was later added in Bitcoin to enforce better transaction uniqueness, with the update of block version to 2, which introduced the following consensus rule:

• the first serialized element in the scriptSig of coinbase transactions must be the height of the chain. [2]

After the complete activation of BIP34, there seemed to be no longer need for the check in ConnectBlock, added for BIP30, as duplicated coinbases were rendered impossible with BIP34 (bitcoin#6931).

This assumption was later revisited, when several blocks were found on Bitcoin's main chain (before BIP34 activation), having coinbase scripts with an height higher than the current chain height (and higher than the BIP34 activation height).
Thus, coinbases for blocks at these "future" heights would have given the opportunity for BIP30 to be violated even with BIP34 enforced (bitcoin#12204).

Motivation for this PR

PIVX has BIP30 and BIP34 consensus rules already implemented since the chain start.
The first block after the genesis (height=1) has version 3.
The "block.nVersion=2 rule" is enforced in ContextualCheckBlock

PIVX/src/main.cpp

Lines 3605 to 3612 in af3dd41

	// Enforce block.nVersion=2 rule that the coinbase starts with serialized block height
	if (pindexPrev) { // pindexPrev is only null on the first block which is a version 1 block.
	CScript expect = CScript() << nHeight;
	if (block.vtx[0].vin[0].scriptSig.size() < expect.size() ||
	!std::equal(expect.begin(), expect.end(), block.vtx[0].vin[0].scriptSig.begin())) {
	return state.DoS(100, false, REJECT_INVALID, "bad-cb-height", false, "block height mismatch in coinbase");
	}
	}

However the code still has the (somewhat expensive) BIP30 check in ConnectBlock, which wasn't needed at all, given the full enforcement of BIP34 since the start of the chain.

This PR removes it.

Side Note: Even without BIP34, with Proof-of-Stake, coinbase transactions have empty scriptPubKey (thus unspendable outputs), therefore there would have been no need for BIP30 checks in any case (at least after PoS activation height).

[furszy]

Great find! ACK c894e8f.

[Fuzzbawls]

Nice one!
ACK c894e8f