Transition to requiring block height in block coinbases #1526

gavinandresen commented at 11:47 PM on June 27, 2012: contributor

This builds on #1525 (the framework for smoothly rolling out new chain/transaction rules), defining "nVersion=2" blocks that include the block height as the first bytes of their coinbase.

Putting the height in the coinbase is desired for at least two reasons:

It guarantees that every subsequent block and transaction hash is unique.
It can be used to better reason about plausible difficulty for not-yet-connected blocks.

The format of the height is "serialized CScript" -- first byte is number of bytes in the number (will be 0x03 on main net for the next 300 or so years), following bytes are little-endian representation of the number.

Only blocks with nVersion=2 are expected to have the height as the first bytes, and the "must have the height in the coinbase" rule is only enforced if nVersion=2 blocks are a super-majority (75% of last 1,000 blocks on main network) of the block's immediate ancestors.

This pull also contains a rule to REJECT nVersion=1 blocks once 95% of hashing power is producing nVersion=2 blocks. That means the last 5% of hashing power who refuse to upgrade will get orphaned.

All of this won't affect users/merchants at all, they will happily accept nVersion=1 or nVersion=2 blocks.

I tested this with a testnet-in-a-box setup, creating a 100-block-long nVersion=2 chain and then making sure that:

nVersion=1 blocks were accepted
nVersion=2 blocks that included the wrong block height were rejected (I hacked a bitcoind to test that).

luke-jr commented at 11:52 PM on June 27, 2012: member

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

gavinandresen commented at 11:56 PM on June 27, 2012: contributor

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.

luke-jr commented at 1:59 AM on June 28, 2012: member

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

gavinandresen commented at 7:54 PM on June 28, 2012: contributor

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

gmaxwell commented at 1:24 PM on July 2, 2012: contributor

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.

jgarzik commented at 4:35 PM on July 2, 2012: contributor

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.

gmaxwell commented at 4:38 PM on July 2, 2012: contributor

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

luke-jr commented at 4:41 PM on July 2, 2012: member

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.

jgarzik commented at 5:00 PM on July 2, 2012: contributor

@gmaxwell highly unlikely, but no objection to doing it the current way @luke-jr re-read, you missed the phrase "publish a BIP"

luke-jr commented at 5:08 PM on July 2, 2012: member

@jgarzik Yes, I did. Sorry.

gavinandresen commented at 6:05 PM on July 2, 2012: contributor

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.

rebroad commented at 7:00 PM on July 2, 2012: contributor

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

gmaxwell commented at 7:09 PM on July 2, 2012: contributor

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

gavinandresen commented at 7:09 PM on July 2, 2012: contributor

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.

gmaxwell commented at 7:13 PM on July 2, 2012: contributor

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

rebroad commented at 7:21 PM on July 2, 2012: contributor

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

gavinandresen commented at 7:34 PM on July 2, 2012: contributor

@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

jgarzik commented at 7:45 PM on July 2, 2012: contributor

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.

jgarzik commented at 7:45 PM on July 2, 2012: contributor

ACK

luke-jr commented at 8:01 PM on July 2, 2012: member

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

gavinandresen commented at 9:11 PM on July 2, 2012: contributor

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

gavinandresen commented at 1:32 AM on July 6, 2012: contributor

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.

jgarzik commented at 2:05 AM on July 6, 2012: contributor

Code ACK

IMO this warrants a BIP

gavinandresen commented at 2:55 AM on July 6, 2012: contributor

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

jgarzik commented at 4:59 AM on July 6, 2012: contributor

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

jgarzik commented at 5:25 AM on July 6, 2012: contributor

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

in src/main.h:None in 8088e5751f outdated

 817 | @@ -817,6 +818,7 @@ class CBlock
 818 |  {
 819 |  public:
 820 |      // header
 821 | +    static const int CURRENT_VERSION=2;

genjix commented at 10:12 AM on July 6, 2012:

It's confusing to have 2 variables called CURRENT_VERSION. Better would be: CURRENT_TX_VERSION and CURRENT_BLOCK_VERSION.

gavinandresen commented at 1:51 PM on July 6, 2012:

Their full names are CBlock::CURRENT_VERSION and CTransaction::CURRENT_VERSION; I'll modify the code to use their full names.

genjix commented at 10:24 AM on July 6, 2012: none

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.

in src/main.cpp:None in 8088e5751f outdated

1807 | +            CScript expect = CScript() << nHeight;
1808 | +            if (!std::equal(expect.begin(), expect.end(), vtx[0].vin[0].scriptSig.begin()))
1809 | +                return DoS(100, error("AcceptBlock() : block height mismatch in coinbase"));
1810 | +        }
1811 | +    }
1812 | +

genjix commented at 10:27 AM on July 6, 2012:

This looks like we can get sequences of version 1 blocks following version 2 blocks. Doesn't seem good.

How were those magical numbers picked? Comments or constant names would be nice for illustrating them.

gavinandresen commented at 1:55 PM on July 6, 2012:

The magical numbers are arbitrary, but 75% network support before enforcing a "If you produce a v=2 block, then it must have heigh in the coinbase" and 95% support before enforcing "no more v1 blocks" feel reasonable.

You can get a sequence of version 1 block following version 2 blocks, there is no way to avoid that unless we think we can tell everybody to upgrade and start producing version=2 blocks at exactly the same time.

jgarzik commented at 3:04 PM on July 6, 2012: contributor

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

in src/main.cpp:None in 8088e5751f outdated

1638 | +            printf("SetBestChain: %d of last 100 blocks above version %d\n", nUpgraded, CBlock::CURRENT_VERSION);
1639 | +        if (nUpgraded > 100/2)
1640 | +            // strMiscWarning is read by GetWarnings(), called by Qt and the JSON-RPC code to warn the user:
1641 | +            strMiscWarning = _("Warning: this version is obsolete, upgrade required");
1642 | +    }
1643 | +

sipa commented at 3:16 PM on July 6, 2012:

I don't really like transaction code to look at (potentially unrelated) blockchain data, but it's probably by far easier this way.

EDIT: nevermind, it looked like this code was part of IsStandard()

luke-jr commented at 7:26 PM on August 1, 2012: member

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?

BitcoinPullTester commented at 7:13 PM on August 9, 2012: none

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:

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

luke-jr commented at 7:46 PM on August 12, 2012: member

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

luke-jr commented at 4:10 PM on August 13, 2012: member

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.

jgarzik commented at 5:35 PM on August 13, 2012: contributor

@luke-jr forrestv does not agree

Set block.nVersion to fix miner unit test 3fcec0d4a0

Block height in coinbase as a new block rule

"Version 2" blocks are blocks that have nVersion=2 and
have the block height as the first item in their coinbase.
Block-height-in-the-coinbase is strictly enforced when
version=2 blocks are a supermajority in the block chain
(750 of the last 1,000 blocks on main net, 51 of 100 for
testnet). This does not affect old clients/miners at all,
which will continue producing nVersion=1 blocks, and
which will continue to be valid.

de237cbfa4

Reject block.nVersion<=1 blocks if network has upgraded to version=2

If 950 of the last 1,000 blocks are nVersion=2, reject nVersion=1
(or zero, but no bitcoin release has created block.nVersion=0) blocks
-- 75 of last 100 on testnet3.

This rule is being put in place now so that we don't have to go
through another "express support" process to get what we really
want, which is for every single new block to include the block height
in the coinbase.

d18f2fd9d6

BitcoinPullTester commented at 7:25 PM on August 20, 2012: none

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

jgarzik referenced this in commit af3b5ea569 on Aug 20, 2012

jgarzik merged this on Aug 20, 2012

jgarzik closed this on Aug 20, 2012

suprnurd referenced this in commit 1b1d52ac3e on Dec 5, 2017

lateminer referenced this in commit 8b25a1eaf5 on May 6, 2020

DrahtBot locked this on Feb 15, 2022