Even if there's a reorg below consensus.BIP34Height, after BIP90, BIP34 is still going to be activated at consensus.BIP34Height. Therefore there's no need to make sure we're on the chain that originally activated BIP34: there's no need to validate bip30 after consensus.BIP34Height regardless of such a reorg.
Besides, if there's such a reorg we're screwed anyway.
1711 | @@ -1712,10 +1712,7 @@ static bool ConnectBlock(const CBlock& block, CValidationState& state, CBlockInd 1712 | // before the first had been spent. Since those coinbases are sufficiently buried its no longer possible to create further 1713 | // duplicate transactions descending from the known pairs either. 1714 | // If we're on the known chain at height greater than where BIP34 activated, we can save the db accesses needed for the BIP30 check. 1715 | - assert(pindex->pprev); 1716 | - CBlockIndex *pindexBIP34height = pindex->pprev->GetAncestor(chainparams.GetConsensus().BIP34Height); 1717 | - //Only continue to enforce if we're below BIP34 activation height or the block hash at that height doesn't correspond. 1718 | - fEnforceBIP30 = fEnforceBIP30 && (!pindexBIP34height || !(pindexBIP34height->GetBlockHash() == chainparams.GetConsensus().BIP34Hash)); 1719 | + fEnforceBIP30 = fEnforceBIP30 && pindex->nHeight > chainparams.GetConsensus().BIP34Height;
should be pindex->nHeight < chainparams.GetConsensus().BIP34Height; ?
oops, yeah it's bip34 that we need to enforce it after BIP34Height but not before, not bip30. bip30b is the opposite. Thanks!
1711 | @@ -1712,10 +1712,7 @@ static bool ConnectBlock(const CBlock& block, CValidationState& state, CBlockInd 1712 | // before the first had been spent. Since those coinbases are sufficiently buried its no longer possible to create further 1713 | // duplicate transactions descending from the known pairs either. 1714 | // If we're on the known chain at height greater than where BIP34 activated, we can save the db accesses needed for the BIP30 check. 1715 | - assert(pindex->pprev); 1716 | - CBlockIndex *pindexBIP34height = pindex->pprev->GetAncestor(chainparams.GetConsensus().BIP34Height); 1717 | - //Only continue to enforce if we're below BIP34 activation height or the block hash at that height doesn't correspond. 1718 | - fEnforceBIP30 = fEnforceBIP30 && (!pindexBIP34height || !(pindexBIP34height->GetBlockHash() == chainparams.GetConsensus().BIP34Hash)); 1719 | + fEnforceBIP30 = fEnforceBIP30 && pindex->nHeight < chainparams.GetConsensus().BIP34Height;
I think we could make this an if condition before any BIP30 code, and skip all BIP30 codes after BIP34Height. Therefore, we don't need to compare the block hash of every blocks with the BIP30 special cases
This is incorrect -- on a random chain, we can't disable BIP30 merely because BIP34 has activated. It's possible to have a chain where there are multiple coinbases with the same txid (mined pre-bip34), where the first bip30 violation would be after the bip34 activation height, because the coinbases didn't overwrite each other. This in turn could prevent reorging back to the honest chain. NACK.
I don't follow. In a "random chain", let's say testnet where bip34 is buried in the past, the situation is similar to bitcoin main. On a newly created "random chain", bip34 (or a hf equivalent) can be validated from block 1.
On 1 Oct 2017 8:31 pm, "Suhas Daftuar" notifications@github.com wrote:
This is incorrect -- on a random chain, we can't disable BIP30 merely because BIP34 has activated. It's possible to have a chain where there are multiple coinbases with the same txid (mined pre-bip34), where the first bip30 violation would be after the bip34 activation height, because the coinbases didn't overwrite each other. This in turn could prevent reorging back to the honest chain. NACK.
— You are receiving this because you authored the thread. Reply to this email directly, view it on GitHub https://github.com/bitcoin/bitcoin/pull/11427#issuecomment-333396501, or mute the thread https://github.com/notifications/unsubscribe-auth/AA9jSkF5wksLWayT_gw9awHBpyTdZX8fks5sn9qLgaJpZM4PpjbI .
@jtimon , say I start from genesis block. Since BIP34 is not checked before BIP34Height, I could make a pre-BIP34 coinbase transaction that includes a post-BIP34Height value in scriptSig, and is perfectly valid. After BIP34Height, with this PR, BIP30 is disabled, so I could replay the same pre-BIP34 coinbase tx. @sdaftuar thanks for pointing out. I missed it.
say I start from genesis block
The genesis block never needs to be checked.
After BIP34Height, with this PR, BIP30 is disabled, so I could replay the same pre-BIP34 coinbase tx.
Not after this PR, after BIP34Height bip30 is always disabled (assuming no huge reorg, an assumption buried deployments is doing for bip34 already anyway).
Since BIP34 is not checked before BIP34Height, I could make a pre-BIP34 coinbase transaction that includes a post-BIP34Height value in scriptSig, and is perfectly valid.
This is assuming a huge reorg. Otherwise, how can you do this if all pre-bip34 blocks were mined long ago?
@jtimon : I mean I start from block 1. Say I make a new block 1 with a post-BIP34 height written in the coinbase. It is valid. Now with this PR this tx could be replayed after BIP34 height. Of course I need a quantum computer to rewrite the chain. But the point is this PR is redefining the consensus and could create a situation where BIP30 is bypassed.
Just like bip90 redefines consensus. So we agree: the only problem would be with a big reorg. But we disagree on that being a problem or not. Unless I'm missing the point about the "random chain".
On 2 Oct 2017 12:54 pm, "Johnson Lau" notifications@github.com wrote:
@jtimon https://github.com/jtimon : I mean I start from block 1. Say I make a new block 1 with a post-BIP34 height written in the coinbase. It is valid. Now with this PR this tx could be replayed after BIP34 height. Of course I need a quantum computer to rewrite the chain. But the point is this PR is redefining the consensus and could create a situation where BIP30 is bypassed.
— You are receiving this because you were mentioned. Reply to this email directly, view it on GitHub https://github.com/bitcoin/bitcoin/pull/11427#issuecomment-333501712, or mute the thread https://github.com/notifications/unsubscribe-auth/AA9jSs2S8BWKAuCnxQTheMQF2a_n33qvks5soMDTgaJpZM4PpjbI .
@jtimon someone can feed you a chain during IBD which will result in you being unable to reorg back to the main chain with this change, ie your node will be permanently stuck and you will have to reindex-chainstate (but there will be no error printed to that effect).
Thank you for trying to explain, but, I'm sorry, I still don't get it. Let's assume the same attack with this change and without it during IBD.
In case A (without this change) you're fed a chain that diverges pre-bip34Height and thus continues to validate bip30 after bip34 is being valdiated too. But that chain has less work than the most-work valid chain, so when you see a most-work chain you change.
In case B (with this change) you're fed a chain that diverges pre-bip34Height but after this change bip30 will stop being validated after bip34Height because one is still validating bip34 anyway. But that chain has less work than the most-work valid chain, so when you see a most-work chain you change.
I'm sorry but I don't see how this changes anything for the IBD attack case either. What am I missing?
@jtimon (someone correct me if im wrong but this is what I think is the issue): if before BIP34 activation, you mine a block with a post-BIP34 blockheight in coinbase, and then replay it after BIP30 stops being validated, that replay is valid on that chain (like @jl2012 said) and the replay overwrites the original transaction. I believe it's this overwriting which makes the reorg impossible, because you've replaced the original transaction?
Or, just generally, after this change, I can feed an IBD node a two-block chain: start with the real block 1, then a block 2 which duplicates the block 1 coinbase. Now the node will (eventually) try to reorg to the real chain and find itself with a UTXO set which is missing the block 1 coinbase output!
@TheBlueMatt what wouldn't be possible below BIP34Height though because BIP30 is still enforced right? You'd have to feed the bad block in after BIP34Height on the bad chain wouldn't you?
@MeshCollider Oh, indeed, yes, your original description is accurate, not mine.
Alright, thanks again for the explanations. I believe I may actually being explained this before but forgot the part about not needing a huge reorg. Sorry for the noise, closing.