-
Notifications
You must be signed in to change notification settings - Fork 36.2k
New issue
Have a question about this project? Sign up for a free GitHub account to open an issue and contact its maintainers and the community.
By clicking “Sign up for GitHub”, you agree to our terms of service and privacy statement. We’ll occasionally send you account related emails.
Already on GitHub? Sign in to your account
BIP9: versionbits #6816
BIP9: versionbits #6816
Conversation
3b58dd7
to
220bd24
Compare
// file COPYING or http://www.opensource.org/licenses/mit-license.php. | ||
/* | ||
_ _ _ _ | ||
__ _____ _ __ ___(_) ___ _ __ | |__ (_) |_ ___ ___ _ __ _ __ |
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
Beauty, but nack this thing.
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
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.
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
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".
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
Sorry to be a spoilsport, but the ASCII art has to go.
220bd24
to
8459a6f
Compare
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:
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). |
Agree with @jtimon's comments. |
int bit = psoftFork->GetBit(); | ||
bool isBitSet = (pblockIndex->pprev->nVersion >> bit) & 0x1; | ||
|
||
cout << "STATE CHANGED - height: " << pblockIndex->nHeight << " median time: " << pblockIndex->GetMedianTimePast() |
There was a problem hiding this comment.
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().
dd93db7
to
329ffc7
Compare
@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. |
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? |
b96ad40
to
f0cb9d3
Compare
@CodeShark The idea is that there are three layers of chain-specific parameters...:
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. |
@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. |
Oh, sure, that's perfect!
|
1e1dc7e
to
77c3fad
Compare
@@ -19,6 +20,8 @@ struct Params { | |||
int nMajorityEnforceBlockUpgrade; | |||
int nMajorityRejectBlockOutdated; | |||
int nMajorityWindow; | |||
/** Used for versionbits */ | |||
VersionBits::SoftForkDeployments softForkDeployments; |
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
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.
77c3fad
to
c2f8414
Compare
3b4dd4a
to
08083e3
Compare
|
||
// -regtest only: allow overriding block.nVersion with | ||
// -blockversion=N to test forking scenarios | ||
if (Params().MineBlocksOnDemand()) |
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
Please s/Params()/chainparams/
|
||
private: | ||
RuleStateMap m_ruleStateMap; | ||
int m_activationInterval; |
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
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?
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 |
c671a6e
to
06992eb
Compare
{ | ||
using namespace Consensus::VersionBits; | ||
|
||
if (!pindexPrev) |
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
In which cases does pindexPrev need be passed as something different from NULL?
Same question for Consensus::SoftForks::UseRule()
.
concept ACK, needs rebase |
6ea6a86
to
06992eb
Compare
…eIndex instance to main.cpp, initializing the instance in AppInit2().
…from AddToBlockIndex
06992eb
to
009bf08
Compare
close in favor of #7575 ? |
@jtimon Yes. |
This is the reference implementation for BIP9.