Merge #1775: [Core] Remove BIP30 check

c894e8f [Core] Remove BIP30 check (random-zebra)

Pull request description:

  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](https://github.com/bitcoin/bips/blob/master/bip-0030.mediawiki)]

  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](https://github.com/bitcoin/bips/blob/master/bip-0034.mediawiki)]

  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`
  https://github.com/PIVX-Project/PIVX/blob/af3dd41f86684a5e4a18cfb85d471a199cc980da/src/main.cpp#L3605-L3612

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

ACKs for top commit:
  furszy:
    Great find! ACK c894e8f.
  Fuzzbawls:
    ACK c894e8f

Tree-SHA512: bc0dec1ef68c05db35ee0a132d0817c851667ce47ba5ec7eae31d29f1a44266f987db4462dc16d2320684bf76a6088c3fdbe77e08a9312ab6a1b7a58b6632661

src/chain.h

@@ -139,7 +139,7 @@ enum BlockStatus {
      */
     BLOCK_VALID_TRANSACTIONS = 3,
 
-    //! Outputs do not overspend inputs, no double spends, coinbase output ok, immature coinbase spends, BIP30.
+    //! Outputs do not overspend inputs, no double spends, coinbase output ok, immature coinbase spends.
     //! Implies all parents are also at least CHAIN.
     BLOCK_VALID_CHAIN = 4,
 

src/main.cpp

@@ -2241,20 +2241,6 @@ bool ConnectBlock(const CBlock& block, CValidationState& state, CBlockIndex* pin
     for (unsigned int i = 0; i < block.vtx.size(); i++) {
         const CTransaction& tx = block.vtx[i];
 
-        // First check for BIP30.
-        // Do not allow blocks that contain transactions which 'overwrite' older transactions,
-        // unless those are already completely spent.
-        // If such overwrites are allowed, coinbases and transactions depending upon those
-        // can be duplicated to remove the ability to spend the first instance -- even after
-        // being sent to another address.
-        // See BIP30 and http://r6.ca/blog/20120206T005236Z.html for more information.
-        // This logic is not necessary for memory pool transactions, as AcceptToMemoryPool
-        // already refuses previously-known transaction ids entirely.
-        const CCoins* coins = view.AccessCoins(tx.GetHash());
-        if (coins && !coins->IsPruned())
-            return state.DoS(100, error("ConnectBlock() : tried to overwrite transaction"),
-                             REJECT_INVALID, "bad-txns-BIP30");
-
         nInputs += tx.vin.size();
         nSigOps += GetLegacySigOpCount(tx);
         if (nSigOps > nMaxBlockSigOps)

test/functional/feature_block.py

@@ -829,23 +829,6 @@ def update_block(block_number, new_transactions):
         yield accepted()
         save_spendable_output()
 
-        # Test BIP30
-        #
-        # -> b39 (11) -> b42 (12) -> b43 (13) -> b53 (14) -> b55 (15) -> b57 (16) -> b60 (17)
-        #                                                                                    \-> b61 (18)
-        #
-        # Blocks are not allowed to contain a transaction whose id matches that of an earlier,
-        # not-fully-spent transaction in the same chain. To test, make identical coinbases;
-        # the second one should be rejected.
-        #
-        tip(60)
-        b61 = block(61, spend=out[18])
-        b61.vtx[0].vin[0].scriptSig = b60.vtx[0].vin[0].scriptSig  #equalize the coinbases
-        b61.vtx[0].rehash()
-        b61 = update_block(61, [])
-        assert_equal(b60.vtx[0].serialize(), b61.vtx[0].serialize())
-        yield rejected(RejectResult(16, b'bad-txns-BIP30'))
-
 
         # Test tx.isFinal is properly rejected (not an exhaustive tx.isFinal test, that should be in data-driven transaction tests)
         #