Does it stop being enforced if ver=2 falls below 75% majority later?

Does it stop being enforced: yes, whether it is enforced or not depends on the previous 1,000 blocks. If more than 250 of the past 1,000 (starting at the block before the block being considered) is nVersion=0 or 1, then the rule isn't enforced.

Is that intentional? Would there be any harm to having a point of no return?

RE: point of no return: I don't see a good reason to write code to do that, and it would be hard to test (would have to notice when we hit the point of no return and record that in the block database, I suppose, then read and pay attention to that database setting).

If there isn't a point of no return the transition for this particular feature can never be removed. If there is, then once its hit after the next checkpoint has been set the code for counting the quorum for this could just be removed and replaced with a simple height comparison.

As I read the code of commit 93fdc48, it does not do what I expect.

A version>1 block with invalid height is simply an invalid block, to be unconditionally rejected. No need for supermajority check.

Nobody generates version>1 blocks right now (right?), so it should be fine to simply publish a BIP and note the new rejection policy.

The logic just described could be deployed immediately.

@jgarzik Then I, Greifer Mc. Greifer, mine a single invalid v2 block. The super majority of nodes will happily extend it and continue the chain, the minority of upgraded nodes will reject it forever and ignore that chain. Nice split you've got there.

And perhaps half as importantly, that would be an abuse of the centralization in a single client to force a blockchain rule through like that. Besides, @sipa already was working on a proper "block/transaction version rules" BIP.

@gmaxwell highly unlikely, but no objection to doing it the current way

@luke-jr re-read, you missed the phrase "publish a BIP"

@jgarzik Yes, I did. Sorry.

RE: locking issues setting strMiscWarnings : before this pull, strMiscWarnings is set from exception handlers (a "should never happen" case) and AddTimeData(), which is called from ProcessMessage when a new node connects.

The new code is in ProcessBlock(), which is also called from ProcessMessage but is also called from -loadblock.

I'm tempted to ignore locking in this case, because I think the risks of doing something like protecting strMiscWarnings with a new critical section outweigh the benefits, and worst-case scenario is somebody running an obsolete version of bitcoin with an incorrectly set system clock has a small chance of getting a crash or garbled message.

What would be needed to create a split with the code as it currently is?

@rebroad it can't make a split as it currently is, but because the v2 blocks are not enforced as it is a malicious party who wants to create trouble by mining duplicate coinbases could do so by just choosing to mine v1 blocks. Basically the patch as is only protects against mistaken duplication by incorrectly modified mining code.

As the pull request stands, the only way to get an orphan chain is:

• wait until 75% of the blocks created are nVersion=2
• some idiot creates/broadcasts a nVersion=2 block that does NOT have the height in the coinbase

25% of the network would build on idiot's block, the other 75% would reject that chain.

@gavinandresen but as soon as the 75% produces two blocks (or whatever is require to get ahead again) the 25% moves back. (thus the distinction between a split and a orphan-stub: a split never heals)

@gavinandresen thanks, you answered the question I had intended to ask.

@gmaxwell : I think of orphan-stubs as temporary splits, and use the term "hard fork" for permanent splits (but don't really care about vocabulary as long as we all understand each other).

Ok: One issue remains with this: should this pull include rules for eventually rejecting nVersion=1 blocks ?

If that is not done now, then we'll be bumping block.nVersion=3 in a year and writing code that says "when X% of the network is v3 and less than Y% is v1 then reject nVersion=1 blocks as too old to support any more."

Suggestion from @gmaxwell in IRC is: stop accepting v1 when v1 blocks are 5% or less of the last 1,000 blocks

Agree with what someone (@gmaxwell?) said on IRC, about stopping v1 acceptance: don't make it a software rule that can "flap" (rule switches on and off and on and off). Just pick a height, once you hit 5% or whatever threshold.

ACK

I think we can add the "reject v1 blocks" rule later without bumping to version=3; since all the "this generation" nodes will enforce it so long as the majority remains over 75%, the only risk is someone making version=1 blocks having more than "future v1-rejecting version" plus this version combined...

@jgarzik: once past 95% v2 blocks (assuming there's consensus on "orphan the last 5% of miners who refuse to get with the program and upgrade") there will be no flapping, because 95% of the network will reject v1 blocks past that point. The release after that happens a checkpoint can be added and the code can be simplified to "require valid v2+ blocks."

@luke-jr: so we release code that creates v2 blocks, but always accepts v1 blocks. Then a while from now we release code that creates v2 blocks but rejects v1 blocks if some threshold has been reached. If I'm a miner, why would I risk running that code; I need to SEE network support for the "reject v1 block" rule before I start doing that.

Added a commit with a "reject nVersion=1 blocks when 95% of the last 1,000 blocks are nVersion=2" -- the point of no return.

Code ACK

IMO this warrants a BIP

I ran my 'coinbase_integers.py' tool on the chain, and somebody is producing block.nVersion=1 blocks with blockheight-1 in their coinbases; see blocks 172036 or 174854 for example (coinbases start with PUSH 172035/174853). Interesting, but not a problem.

There are also some accidental "looks like a block height" blocks (e.g. block 164384 starts its coinbase with PUSH 1983702).

That opens a tiny window for whoever created block 164384 to try to create a duplicate coinbase in the year 2046 when block 1,983,702 is mined. The chances of them mining that particular block will probably be pretty small, although maybe there would be some incentive for them to sell the private key to a big mining consortium who would... do something evil maybe.

We could close that remote possibility by giving the coinbase transactions nVersion=2; the transaction's version is part of the transaction id hash, so a nVersion=1 transaction will never hash to the same id as a nVersion=2 transaction.

We do have the problem that between the time we announce what we're going to do and the time when there is 75% main network support rogue miners could create block.nVersion=2 / coinbase.nVersion=2 coinbases that contain "height = sometime in the future".

The most likely error scenario is probably a buggy miner creating a version=2, height=$ValidHeight+1 or height=$ValidHeight-1 block.

First draft of BIP text: https://gist.github.com/3058253

This is a good rule change, and I only wish it was in all blocks already. It's such a pain to have different transactions with the same hash.

Does this mean sipa's rule change will be removed? This change seems to obselete that once network hashing majority is achieved.

About blocks that have a coinbase which looks like a block number and could cause problems: the only way around that is to add a fixed workaround (if nHeight == foo). It's so far in the future though, that it's not worth worrying about.

New URL for spec, with assigned BIP number: https://en.bitcoin.it/wiki/BIP_0034

Can this be amended to only pay attention to the last octet, just in case we end up using the first 3 for something else?

Automatic sanity-testing: FAILED BUILD/TEST, see http://jenkins.bluematt.me/pull-tester/fa174be9b58de7377b46416d3a040c979d27d7e2 for binaries and test log.

This could happen for one of several reasons:

1. It chanages paths in makefile.linux-mingw or otherwise changes build scripts in a way that made them incompatible with the automated testing scripts
2. It does not build on either Linux i386 or Win32 (via MinGW cross compile)
3. The test suite fails on either Linux i386 or Win32

This should fix the test:

diff --git a/src/test/miner_tests.cpp b/src/test/miner_tests.cpp
index c9981fb..8ffc9b4 100644
--- a/src/test/miner_tests.cpp
+++ b/src/test/miner_tests.cpp
@@ -62,6 +62,7 @@ BOOST_AUTO_TEST_CASE(CreateNewBlock_validity)
     std::vector<CTransaction*>txFirst;
     for (unsigned int i = 0; i < sizeof(blockinfo)/sizeof(*blockinfo); ++i)
     {
+        pblock->nVersion = 1;
         pblock->nTime = pindexBest->GetMedianTimePast()+1;
         pblock->vtx[0].vin[0].scriptSig = CScript();
         pblock->vtx[0].vin[0].scriptSig.push_back(blockinfo[i].extranonce);

Note that this should be merged only after #936: there is software (p2pool?) which will create version==2 blocks without the height right now. #936 will workaround this problem by breaking compatibility with said application, and BIP22 explicitly requires clients understand and implement height-in-coinbase for version 2.

@luke-jr forrestv does not agree

Automatic sanity-testing: PASSED, see http://jenkins.bluematt.me/pull-tester/d18f2fd9d6927b1a132c5e0094479cf44fc54aeb for binaries and test log.