BIP9: versionbits #6816

CodeShark · 2015-10-13T12:17:12Z

This is the reference implementation for BIP9.

jtimon · 2015-10-13T20:18:41Z

src/consensus/versionbits.cpp

Beauty, but nack this thing.

Agree with @jtimon. Also it may be better to split the copyright (c) line into two. One for the initial author and initial year and a second one for the bitcoin core devs.

Doxygen supports documentation on a file level, which seems more useful than ascii arts.

One for the initial author and initial year and a second one for the bitcoin core devs.

Is this really necessary? There is already the much more precise git history, and to my understanding, all contributors qualify as "Bitcoin Core developers".

Sorry to be a spoilsport, but the ASCII art has to go.

jtimon · 2015-10-14T13:05:54Z

The deployment times of concrete softforks depend on the chain (ie they are expected to be different in main and testnet3). Here's one solution:

The Softfork class moves to consensus/params.h (and becomes a method-less struct, since that file is expected to be exposed to a complete libconsensus).
Consensus::Params gets a new field softforks, which is an array of these new Softfork structs, and whose static size is determined by MAX_SOFTFORKS, the last element of an enumeration of all currently supported softforks.
Every time a new softfork is supported, it must be included in that enum and the static size of the array will increase.

This way we maintain the ability to independently deploy different softforks in different testchains while respecting Consensus::Params as the only interface to communicate differences between chains to the consensus code (a problem we had already solved, in a way that is compatible with a C API).

sipa · 2015-10-14T13:06:37Z

Agree with @jtimon's comments.

dexX7 · 2015-10-15T13:34:59Z

src/test/versionbits_tests.cpp

I like the extra information, but you may look into BOOST_TEST_MESSAGE().

CodeShark · 2015-10-16T07:28:24Z

@jtimon @sipa After thinking a little bit more about how to make the soft fork deployments chain-specific, I think CChainParams is the place to initialize that, not Consensus::Params. So I added a SoftForkDeployments member to CChainParams and created a dummy BIP9999 on regtest to demonstrate the deployment mechanism in action.

jtimon · 2015-10-16T07:39:43Z

Consensus::Params it's the place to put consensus critical chain params, not CChainParams. If libconsensus was complete you would have just broke its build or moved consensus functionality out of libconsensus by putting it in the NON CONSENSUS COMPATIBLE CChainParams. Can you please follow the C API compatible suggestion?
How do you plan to expose CChainParams in libconsensus' API if not by passing Consensus::Params to the exposed functions that need it?

sipa · 2015-10-16T12:16:40Z

@CodeShark The idea is that there are three layers of chain-specific parameters...:

basechainparams, which contain things shared by both nodes and RPC clients
consensus/params, which contains all consensus-critical parametersl
chainparams, which contains node operation parameters, including an encapsulated consensus/params

The reason is to avoid having the consensus code depend on knowing where DNS seeds are, or to avoid having the RPC code depend on CBlock because of the genesis definition.

CodeShark · 2015-10-16T12:35:49Z

@sipa OK, I guess what I meant to say is that CChainParams (or its subclasses, rather) is where the Consensus::Params structure gets initialized. So I went ahead and moved the SoftForkDeployments instance to Consensus::Params and am initializing immediately after the other Consensus::Params fields are initialized.

sipa · 2015-10-16T12:39:36Z

Oh, sure, that's perfect!

jtimon · 2015-10-19T09:25:15Z

src/consensus/params.h

This field ( by being a C++ class with methods) impedes this file from being exposed in the libconsensus C API. What's wrong with moving the rules enum to this file and having a static size array as suggested?
Maybe you have an alternative to passing a Consensus::Params struct as a parameter of the exposed C libconsensus runctions that need it the communicate chain-dependent data?
Can you draft a C API for contextualcheckblockheader after this (don't worry about CBlockIndex which is that part to be solved, worry about the part that was solved but you are breaking here). Maybe taking a look at #5995 (where a C API compatible VerifyHeader is compiled within libconsensus) helps you understand what exactly you are breaking with this.

jtimon · 2015-10-19T15:29:44Z

src/miner.cpp

Please s/Params()/chainparams/

jtimon · 2015-10-26T12:31:16Z

src/consensus/blockruleindex.h

This field is unnecessarily redundant. If you don't want to create a new field in Consensus::Params, can you at least use consensusParams.DifficultyAdjustmentInterval() directly?

jtimon · 2015-10-26T15:05:40Z

Here's a version rebased on top of master:

bitcoin:master...jtimon:CodeShark_versionbits_rebased

Here are a couple of nits (don't BIP42 and BIP62, and don't BlockRuleIndex::m_activationInterval) implemented in code: jtimon/bitcoin@CodeShark_versionbits_rebased...jtimon:versionbits-nits

jtimon · 2015-10-28T13:34:36Z

src/consensus/softforks.cpp

In which cases does pindexPrev need be passed as something different from NULL?
Same question for Consensus::SoftForks::UseRule().

dcousens · 2015-12-11T01:19:32Z

concept ACK, needs rebase

…eIndex instance to main.cpp, initializing the instance in AppInit2().

…from AddToBlockIndex

jtimon · 2016-03-16T18:09:18Z

close in favor of #7575 ?

btcdrak · 2016-03-16T18:11:29Z

@jtimon Yes.

CodeShark force-pushed the versionbits branch 2 times, most recently from 3b58dd7 to 220bd24 Compare October 13, 2015 16:52

jtimon reviewed Oct 13, 2015
View reviewed changes

CodeShark force-pushed the versionbits branch from 220bd24 to 8459a6f Compare October 14, 2015 02:22

jtimon mentioned this pull request Oct 15, 2015

Softforks unit #6747

Closed

dexX7 reviewed Oct 15, 2015
View reviewed changes

src/test/versionbits_tests.cpp Outdated

Copy link

Contributor

dexX7 Oct 15, 2015

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

I like the extra information, but you may look into BOOST_TEST_MESSAGE().

CodeShark force-pushed the versionbits branch 5 times, most recently from dd93db7 to 329ffc7 Compare October 16, 2015 07:11

CodeShark force-pushed the versionbits branch 3 times, most recently from b96ad40 to f0cb9d3 Compare October 16, 2015 10:40

CodeShark force-pushed the versionbits branch 2 times, most recently from 1e1dc7e to 77c3fad Compare October 17, 2015 10:01

jtimon reviewed Oct 19, 2015
View reviewed changes

CodeShark force-pushed the versionbits branch from 77c3fad to c2f8414 Compare October 19, 2015 10:55

btcdrak mentioned this pull request Oct 19, 2015

Use 512-bit arithmetic for difficulty retarget calculation. #6162

Closed

CodeShark force-pushed the versionbits branch from 3b4dd4a to 08083e3 Compare October 19, 2015 12:32

CodeShark mentioned this pull request Oct 19, 2015

Added fPowNoRetargeting field to Consensus::Params #6853

Merged

jtimon reviewed Oct 19, 2015
View reviewed changes

src/miner.cpp Outdated

Copy link

Contributor

jtimon Oct 19, 2015

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

Please s/Params()/chainparams/

laanwj added the Consensus label Oct 26, 2015

jtimon reviewed Oct 26, 2015
View reviewed changes

CodeShark force-pushed the versionbits branch 2 times, most recently from c671a6e to 06992eb Compare October 26, 2015 18:42

jtimon reviewed Oct 28, 2015
View reviewed changes

CodeShark force-pushed the versionbits branch from 6ea6a86 to 06992eb Compare January 14, 2016 06:33

CodeShark added 10 commits January 13, 2016 22:36

Added versionbits and blockruleindex units.

a528b64

Added CreateBlockVersion to BlockRuleIndex.

4032844

Added unit tests for versionbits and blockruleindex.

5be7fa0

Added versionbits support to Consensus::Params, added global BlockRul…

4f606b4

…eIndex instance to main.cpp, initializing the instance in AppInit2().

Added softforks unit.

46cdc1c

Added BIP9999 to regtest chainparams as example softfork deployment.

01b0bf4

Inserting into BlockRuleIndex when first loading the block index and …

acc091d

…from AddToBlockIndex

Use Consensus::SoftForks::CheckVersion() for warning system.

4737fa2

Added miner support for versionbits.

21f2f1c

Added versionbits status info to getblockchaininfo RPC.

009bf08

CodeShark force-pushed the versionbits branch from 06992eb to 009bf08 Compare January 14, 2016 07:00

sipa mentioned this pull request Feb 21, 2016

Minimal BIP9 implementation #7575

Merged

laanwj closed this Mar 17, 2016

bitcoin locked as resolved and limited conversation to collaborators Sep 8, 2021

BIP9: versionbits #6816

BIP9: versionbits #6816

Uh oh!

Conversation

CodeShark commented Oct 13, 2015

Uh oh!

Choose a reason for hiding this comment

Uh oh!

Choose a reason for hiding this comment

Uh oh!

Choose a reason for hiding this comment

Uh oh!

Choose a reason for hiding this comment

Uh oh!

jtimon commented Oct 14, 2015

Uh oh!

sipa commented Oct 14, 2015

Uh oh!

Choose a reason for hiding this comment

Uh oh!

CodeShark commented Oct 16, 2015

Uh oh!

jtimon commented Oct 16, 2015

Uh oh!

sipa commented Oct 16, 2015

Uh oh!

CodeShark commented Oct 16, 2015

Uh oh!

sipa commented Oct 16, 2015 via email

Uh oh!

Choose a reason for hiding this comment

Uh oh!

Choose a reason for hiding this comment

Uh oh!

Choose a reason for hiding this comment

Uh oh!

jtimon commented Oct 26, 2015

Uh oh!

Choose a reason for hiding this comment

Uh oh!

dcousens commented Dec 11, 2015

Uh oh!

jtimon commented Mar 16, 2016

Uh oh!

btcdrak commented Mar 16, 2016

Uh oh!

Reviewers

Assignees

Labels

Projects

Milestone

Development

Uh oh!

11 participants