Fix overly eager BIP30 bypass #12204

In #6931 we introduced a possible consensus breaking change by misunderstanding how completely BIP 34 obviated the need for BIP 30. Unfixed, this could break consensus after block height about 1.9M. Explained in code comment.

h/t @sdaftuar

concept ACK 9ab7fe0b3f50b2cc3514975b26eb7c63134bdd40.

I'm currently running my own analysis of pre-BIP34 blocks to make sure that it agrees with what you found. It would be good to have a gist showing those results with code so other people can easily validate for themselves.

In general, I'm not sure whether code comments are appropriate for such a long and nuanced description. It certainly breaks up the flow of the ConnectBlock(). However, more verbose is generally better than less, and I don't have a better suggestion of where to put this level of comment that would stay maintained with the code.

Concept ACK.

code-level utACK, modulo comment comments.

Concept ACK. Though, I have never checked if any of the heights or hashes make sense. Wouldn't it help review if #6931 or this pull actually included the code that was used to determine those magic numbers?

My analysis matches yours exactly. The pre-BIP34 blocks with coinbase height > block height are (in ascending order):

block_height | coinbase_height 209920 | 209921 176684 | 490897 164384 | 1983702 169895 | 3708179 <---- I expect to be dead before this height ....

so you're right that the only heights that we should care about are 209921, 490897 and 1983702.

If anyone wants to re-run the analysis, the gist with my code is here: https://gist.github.com/jnewbery/df0a98f3d2fea52e487001bf2b9ef1fd

(this is quite slow since it's hitting the RPC interface 3 times for each block. It'd be faster to deserialize block files directly, patch bitcoind to print this data itself, or use BlockSci or a similar block analysis tool)

code utACK, haven't actually read through the whole comment.

I agree with the above analysis that block 1983702 is the lowest block height that we need to worry about. We could soft fork in something prior to that block to guarantee that the txids will be different, e.g. block height in the nLockTime field.

Comment updated to hopefully be clearer. Code unchanged.

For reference here is how I double checked the possible violations. I patched bitcoind with this snippet and then ran with -reindex-chainstate.

@@ -1724,6 +1724,16 @@ static bool ConnectBlock(const CBlock& block, CValidationState& state, CBlockInd
                 }
             }
         }
+        if (block.vtx[0]->vin[0].scriptSig.size() >= 4 && block.vtx[0]->vin[0].scriptSig[0] == 3) {
+            std::vector<unsigned char> vch;
+            for (int i=1;i<4;i++)
+                vch.push_back( block.vtx[0]->vin[0].scriptSig[i]);
+            CScriptNum height(vch,false);
+            int height_indicated = height.getint();
+            if (height_indicated > pindex->nHeight) {
+                fprintf(stdout,"Potential future duplicate: block %d has indicated height %d\n",pindex->nHeight,height_indicated);
+            }
+        }
     }
 
     // Start enforcing BIP68 (sequence locks) and BIP112 (CHECKSEQUENCEVERIFY) using versionbits logic.

Tested ACK 3f3b991

Looks sensible to me. Although I think we should definitely make this a chain parameter, or at least define a constant, instead of hardcoding this magic number:

Optional TODO: Have BIP30 checking activate at a per chain block height.

I don't think we're in enough of a hurry here to skip that.

@laanwj I'm happy to define it as a constant locally, but it seems like adding it as a chain parameter just adds unnecessary cruft to the code. This is such an obscure (and irrelevant for decades) element of the consensus that it seems better to keep it entirely contained to one spot in the code base, so it doesn't occupy mental energy in other places. That said, will defer to your judgement...

EDIT: Or if we think testnet3 will get longer than 1.9 million blocks, then I suppose that's an argument for per chain so we don't do unnecessary calculations on testnet3.

Nono I'm OK with that, but if we actively argue against making it a chain parameter we should remove the TODO as well, otherwise it's inviting someone else to make that change. My only point is that there is no hurry to merge this, so no reason to leave unfinished TODOs in the code.

updated to a constant and removed optional TODO, I believe its better to keep this mess contained to one section of the code

utACK https://github.com/bitcoin/bitcoin/pull/12204/commits/5b8b387752e8c493a8e87795ae6ddb78b45b1a5d

utACK 5b8b387752e8c493a8e87795ae6ddb78b45b1a5d

Difference is that there is now a constexpr int BIP34_IMPLIES_BIP30_LIMIT and the comment has been updated.

Travis failed because of a timeout. I'll kick it.

ACK