BIP9: versionbits

CodeShark commented at 12:17 pm on October 13, 2015: contributor

This is the reference implementation for BIP9.

CodeShark force-pushed on Oct 13, 2015

in src/consensus/versionbits.cpp: in 220bd24fcf outdated

0@@ -0,0 +1,394 @@
1+// Copyright (c) 2009-2015 Eric Lombrozo, The Bitcoin Core developers
2+// Distributed under the MIT software license, see the accompanying
3+// file COPYING or http://www.opensource.org/licenses/mit-license.php.
4+/*
5+                     _             _     _ _                         
6+ __   _____ _ __ ___(_) ___  _ __ | |__ (_) |_ ___   ___ _ __  _ __

jtimon commented at 8:18 pm on October 13, 2015:

Beauty, but nack this thing.

MarcoFalke commented at 8:20 pm on October 13, 2015:

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.

dexX7 commented at 1:10 pm on October 15, 2015:

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

btcdrak commented at 1:44 pm on October 15, 2015:

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

CodeShark force-pushed on Oct 14, 2015

jtimon commented at 1:05 pm on October 14, 2015: contributor

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 commented at 1:06 pm on October 14, 2015: member

Agree with @jtimon’s comments.

in src/test/versionbits_tests.cpp: in 8459a6f3ff outdated

128+void StateChanged(const CBlockIndex* pblockIndex, const SoftFork* psoftFork, State prevState, State newState, int bitCount)
129+{
130+    int bit = psoftFork->GetBit();
131+    bool isBitSet = (pblockIndex->pprev->nVersion >> bit) & 0x1;
132+
133+    cout << "STATE CHANGED - height: " << pblockIndex->nHeight << " median time: " << pblockIndex->GetMedianTimePast()

dexX7 commented at 1:34 pm on October 15, 2015:

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

CodeShark force-pushed on Oct 15, 2015

CodeShark force-pushed on Oct 16, 2015

CodeShark commented at 7:28 am on October 16, 2015: contributor

@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 commented at 7:39 am on October 16, 2015: contributor

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?

CodeShark force-pushed on Oct 16, 2015

sipa commented at 12:16 pm on October 16, 2015: member

@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 commented at 12:35 pm on October 16, 2015: contributor

@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 commented at 12:39 pm on October 16, 2015: member

Oh, sure, that’s perfect!

CodeShark force-pushed on Oct 17, 2015

in src/consensus/params.h: in 77c3fada7c outdated

19@@ -19,6 +20,8 @@ struct Params {
20     int nMajorityEnforceBlockUpgrade;
21     int nMajorityRejectBlockOutdated;
22     int nMajorityWindow;
23+    /** Used for versionbits */
24+    VersionBits::SoftForkDeployments softForkDeployments;

jtimon commented at 9:25 am on October 19, 2015:

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.

in src/consensus/softforks.h: in 77c3fada7c outdated

19+    BIP34,
20+    BIP42,
21+    BIP62,
22+    BIP65,
23+    BIP66,
24+    BIP68,

jtimon commented at 10:26 am on October 19, 2015:

Again, there’s not need to include future rule changes (their own PRs can do it if they get merged after this, maybe with the exception of BIP65 for which most of the code has been already merged). You don’t want people to start asking you to add other “future rule changes” like BIP100, BIP101 and BIP102, right? Also, (and also again) why use Softfork instead of something more generic that also serves for Hardfork deployments? Maybe “Deployments” or “RuleChanges” would be better.

CodeShark force-pushed on Oct 19, 2015

in src/miner.cpp: in 08083e3848 outdated

143@@ -148,6 +144,13 @@ CBlockTemplate* CreateNewBlock(const CScript& scriptPubKeyIn)
144         CBlockIndex* pindexPrev = chainActive.Tip();
145         const int nHeight = pindexPrev->nHeight + 1;
146         pblock->nTime = GetAdjustedTime();
147+
148+        // -regtest only: allow overriding block.nVersion with
149+        // -blockversion=N to test forking scenarios
150+        if (Params().MineBlocksOnDemand())

jtimon commented at 3:29 pm on October 19, 2015:

Please s/Params()/chainparams/

in src/consensus/blockruleindex.cpp: in 08083e3848 outdated

295+    m_ruleStateMap.clear();
296+}
297+
298+
299+static BlockRuleIndex g_blockRuleIndex;
300+BlockRuleIndex& Consensus::VersionBits::GetBlockRuleIndex() { return g_blockRuleIndex; }

jtimon commented at 3:33 pm on October 19, 2015:

Why the getter for the new global? If you are introducing a new global that consensus code will rely on, why not use the global as explicitly a possible? Just declare g_blockRuleIndex as extern in the .h file, s/Consensus::VersionBits::GetBlockRuleIndex()/g_blockRuleIndex and simply get rid of Consensus::VersionBits::GetBlockRuleIndex().

CodeShark force-pushed on Oct 19, 2015

in src/consensus/blockruleindex.h: in 08083e3848 outdated

15+class CBlockIndex;
16+
17+namespace Consensus
18+{
19+
20+namespace VersionBits

jtimon commented at 4:29 pm on October 19, 2015:

Is it really worth it to have so many namespaces (specially when you are avoiding to use them explicitly in so many cases with using namespace)? Isn’t the Consensus namespace enough?

jtimon commented at 4:47 pm on October 19, 2015: contributor

Here’s a modified version of the branch the doesn’t use library-unfriendly (and bad in general) global g_blockRuleIndex within the new consensus code. The global is maintained in main.cpp (where most globals currently are) but at least it is out of the consensus directory.

https://github.com/jtimon/bitcoin/commits/versionbits

sipa commented at 4:50 pm on October 19, 2015: member

Agree with maintaining the global itself in main. That also makes it clearer that it’s protected by the same locks as the other consensus-related state variables there.

CodeShark force-pushed on Oct 19, 2015

CodeShark commented at 6:21 pm on October 19, 2015: contributor

@jtimon I incorporated the ideas of https://github.com/jtimon/bitcoin/commits/versionbits regarding getting rid of GetBlockRuleIndex() Thanks!

in src/miner.cpp: in d15dcbe49f outdated

144@@ -148,6 +145,13 @@ CBlockTemplate* CreateNewBlock(const CScript& scriptPubKeyIn)
145         CBlockIndex* pindexPrev = chainActive.Tip();
146         const int nHeight = pindexPrev->nHeight + 1;
147         pblock->nTime = GetAdjustedTime();
148+        pblock->nVersion = blockRuleIndex.CreateBlockVersion(pblock->nTime, chainActive.Tip()->pprev);
149+
150+        // -regtest only: allow overriding block.nVersion with
151+        // -blockversion=N to test forking scenarios
152+        if (Params().MineBlocksOnDemand())

jtimon commented at 6:28 pm on October 19, 2015:

s/Params()/chainparams for free (diff-wise), please.

in src/main.cpp: in d15dcbe49f outdated

3114@@ -3109,6 +3115,10 @@ bool static LoadBlockIndexDB()
3115             pindex->BuildSkip();
3116         if (pindex->IsValid(BLOCK_VALID_TREE) && (pindexBestHeader == NULL || CBlockIndexWorkComparator()(pindexBestHeader, pindex)))
3117             pindexBestHeader = pindex;
3118+
3119+        // Insert into versionbits block rule index and recompute soft fork deployment states for chain
3120+        Consensus::VersionBits::BlockRuleIndex& blockRuleIndex = g_blockRuleIndex;

jtimon commented at 6:30 pm on October 19, 2015:

Why not on top of the function (like CChainParams& chainparams, like it was in my code) ready to become a parameter for the function? Why did you spend any time in moving it down?

CodeShark force-pushed on Oct 19, 2015

CodeShark force-pushed on Oct 20, 2015

in src/consensus/blockruleindex.h: in dea99a4d8f outdated

62+        m_ruleStateMap[pblockIndex] = ruleStates;
63+    }
64+#endif
65+
66+private:
67+    int m_activationInterval;

jtimon commented at 2:38 pm on October 20, 2015:

This should be moved to Consensus::Params. As always, “this could be done later”, but if we do it now (before it affects anyone building on top of master or stable releases) it will be less painful for everyone.

jtimon commented at 2:40 pm on October 20, 2015: contributor

Updated https://github.com/jtimon/bitcoin/commits/versionbits with one more commit that solves my last nit. EDIT: @CodeShark If you want me to do the squashing, I’m happy to do it.

CodeShark force-pushed on Oct 20, 2015

CodeShark force-pushed on Oct 21, 2015

CodeShark commented at 11:53 pm on October 21, 2015: contributor

@jtimon Please let’s hold off on any more code movement nits until after 0.12. Let’s focus the rest of this conversation on this PR’s behavior.

dcousens commented at 0:14 am on October 22, 2015: contributor

@CodeShark I think you meant @jtimon

CodeShark commented at 0:16 am on October 22, 2015: contributor

@dcousens lol, yeah. thx.

jtimon commented at 8:31 am on October 22, 2015: contributor

These aren’t code movements, these are (I think very legitimate) nits to NEW code. This is the best time to make modifications to the new code, once it is merged, we won’t have the chance to change it without affecting many many people again. If you are not going to incorporate more suggestions that’s fine too: I will create an alternative PR then. Little by little I want to get to what I suggested in #6816 (comment) . I won’t change functionality (at least on purpose) and it will also help me review the implemented functionality. All I want to avoid is unwalking what we’ve already walked, because that’s never necessary to deploy new functionality. This is NOT bike-shedding even if you may perceive it as such. I’m sorry but there will be more nits (although you are free not to solve them, and reviewers can chose the PR they prefer).

in src/consensus/softforks.cpp: in 1b8acb8f91 outdated

89+        
90+    case BIP42:
91+        return false;
92+
93+    case BIP62:
94+        return false;

jtimon commented at 7:55 pm on October 22, 2015:

Why have BIP62 only to return false? Can’t we add it later if needed? BIP42 seems completely unnecessary.

in src/consensus/versionbits.h: in 1b8acb8f91 outdated

46+    uint32_t    GetExpireTime() const { return m_expireTime; }
47+
48+private:
49+    int m_bit;
50+    int m_rule;
51+    int m_threshold;

jtimon commented at 8:24 pm on October 22, 2015:

According to BIP9, the threshold is not per-deployment, it’s per-chain, it says “Threshold If bit B is set in 1916 (1512 on testnet) or more of the 2016 blocks within a retarget period”. This should move to Consensus::Params as a single field (just like BlockRuleIndex::m_activationInterval).

sipa commented at 8:25 pm on October 22, 2015:

BIP9 is just advisory, and individual softforks can deviate from it arbitrarily. So I think it should be both per chain and per fork :)

jtimon commented at 10:35 pm on October 22, 2015:

Well, if it is per fork it’s also per chain as each fork can be deployed differently per chain. I think this is an unnecessary complication (I think all of them are going to be 95% for the main chain). But if we go for it, we should update BIP9.

rustyrussell commented at 4:37 am on October 25, 2015:

Yeah, if someone wants to modify their special BIP to deviate from this, that’s their job to introduce a new per-chain parameter.

CodeShark commented at 6:09 am on October 25, 2015:

@rustyrussell We can just use the same threshold for each soft fork in a given chain. In the future if we ever decide to change the threshold, it will be straightforward to do so without requiring any changes to the implementation while allowing us to keep the old thresholds for the earlier forks. The BIP document mentions the possibility of modifying the threshold at some point in the future. So I don’t see any downside in supplying this parameter for each fork.

Adding the parameter to Consensus::Params means that if we ever decide to support a different threshold we’ll have to hack around it, which seems completely unnecessary when the current implementation can already cleanly support it.

luke-jr commented at 8:33 am on October 25, 2015:

Maybe it would be useful to plan for 10 bits 95%, 10 bits 85%, 5 bits 75%, and 5 bits 65%? Does anyone expect to have more than 10 ongoing softforks at a time anyway?

jtimon commented at 9:31 am on October 25, 2015:

To be clear, whether the threshold is per softfork or per chain, it is in Consensus::Params (just once or repeated per softfork). If we only use one per chain (the same for all softforks), we are wasting space, although it is true that not much (even after hundreds of deployments).

jtimon commented at 2:07 pm on October 26, 2015:

Anyway, I will change my modification of this implementation back to a threshold per softfork like in this implementation so that they don’t differ too much. In my case it will be trivial to change later.

jtimon commented at 6:42 pm on October 23, 2015: contributor

Updated https://github.com/bitcoin/bitcoin/compare/master...jtimon:versionbits with the changes described in #6816 (comment) .

CodeShark force-pushed on Oct 25, 2015

laanwj added the label Consensus on Oct 26, 2015

in src/consensus/blockruleindex.h: in ca60edb609 outdated

60+    }
61+#endif
62+
63+private:
64+    RuleStateMap m_ruleStateMap;
65+    int m_activationInterval;

jtimon commented at 12:31 pm on October 26, 2015:

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?

in src/Makefile.am: in ca60edb609 outdated

 98@@ -99,7 +99,10 @@ BITCOIN_CORE_H = \
 99   compressor.h \
100   consensus/consensus.h \
101   consensus/params.h \
102+  consensus/blockruleindex.h \

jtimon commented at 12:36 pm on October 26, 2015:

We’re avoiding state-full structures like this on consensus folder. Even if the folder has to depend on state-full structures like CBlockIndex, CCoinsViewCache and now also BlockRuleIndex temporarily (in this case it could be avoided directly), the consensus code will have to be decoupled from them to complete libconsensus, so chain.o, coins.o and blockruleindex.o don’t belong in this folder.

jtimon commented at 0:03 am on November 4, 2015:

Is this just an in-memory cache that stores nothing in disk? If so, my previous criticisms don’t apply.

in src/consensus/softforks.h: in ca60edb609 outdated

14+struct Params;
15+namespace VersionBits { class BlockRuleIndex; }
16+
17+namespace SoftForks {
18+
19+enum Rule

jtimon commented at 2:00 pm on October 26, 2015:

Can you move this enum to consensus/params.h ? It will be eventually necessary to expose the versionbits functionality to libconsensus.

in src/consensus/versionbits.h: in ca60edb609 outdated

27+
28+const char* GetRuleStateText(int ruleState, bool bUseCaps = false);
29+
30+bool UsesVersionBits(int nVersion);
31+
32+class SoftFork

jtimon commented at 2:03 pm on October 26, 2015:

Can you move this class (ideally as a struct with public attributes instead of getters) to consensus/params.h ? It will be eventually necessary to expose the versionbits functionality in libconsensus.

in src/bitcoind.cpp: in ca60edb609 outdated

115@@ -114,6 +116,12 @@ bool AppInit(int argc, char* argv[])
116             return false;
117         }
118 
119+        // Initialize block rule index for versionbits support

jtimon commented at 2:11 pm on October 26, 2015:

Wouldn’t it be better to do this in init::AppInit2() ?

jtimon commented at 3:05 pm on October 26, 2015: contributor

Here’s a version rebased on top of master:

https://github.com/bitcoin/bitcoin/compare/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: https://github.com/jtimon/bitcoin/compare/CodeShark_versionbits_rebased...jtimon:versionbits-nits

CodeShark force-pushed on Oct 26, 2015

in src/consensus/softforks.cpp: in 06992eb9eb outdated

29+
30+VersionStatus Consensus::SoftForks::CheckVersion(const CBlockIndex& blockIndex, const Consensus::VersionBits::BlockRuleIndex& blockRuleIndex, const Consensus::Params& consensusParams, CBlockIndex* pindexPrev)
31+{
32+    using namespace Consensus::VersionBits;
33+
34+    if (!pindexPrev)

jtimon commented at 1:34 pm on October 28, 2015:

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

in src/consensus/softforks.cpp: in 06992eb9eb outdated

25+        pstart = pstart->pprev;
26+    }
27+    return (nFound >= nRequired);
28+}
29+
30+VersionStatus Consensus::SoftForks::CheckVersion(const CBlockIndex& blockIndex, const Consensus::VersionBits::BlockRuleIndex& blockRuleIndex, const Consensus::Params& consensusParams, CBlockIndex* pindexPrev)

jtimon commented at 1:43 pm on October 28, 2015:

Can’t this return a boolean and use CValidationState& state like main::ContextualCheckBlockHeader() (the functions where some of Consensus::SoftForks::CheckVersion() comes from) is currently doing?

in src/consensus/versionbits.cpp: in 06992eb9eb outdated

79+        // Do softFork times overlap?
80+        const SoftFork* softFork = it->second;
81+        if (((deployTime >= softFork->GetDeployTime()) && (deployTime <  softFork->GetExpireTime())) ||
82+            ((expireTime >  softFork->GetDeployTime()) && (expireTime <= softFork->GetExpireTime())) ||
83+            ((deployTime <= softFork->GetDeployTime()) && (expireTime >= softFork->GetExpireTime())))
84+                return false;

morcos commented at 5:30 pm on December 10, 2015:

This seems to me that we can’t reuse a bit for a new soft fork until the expire time, regardless of whether the previous soft fork at that bit has already activated. I thought the intention was to allow reuse immediately after activation.

dcousens commented at 1:19 am on December 11, 2015: contributor

concept ACK, needs rebase

CodeShark force-pushed on Jan 14, 2016

Added versionbits and blockruleindex units. a528b64311

Added CreateBlockVersion to BlockRuleIndex. 40328449cc

Added unit tests for versionbits and blockruleindex. 5be7fa0662

Added versionbits support to Consensus::Params, added global BlockRuleIndex instance to main.cpp, initializing the instance in AppInit2(). 4f606b4efb

Added softforks unit. 46cdc1cb28

Added BIP9999 to regtest chainparams as example softfork deployment. 01b0bf4cad

Inserting into BlockRuleIndex when first loading the block index and from AddToBlockIndex acc091d4d2

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

Added miner support for versionbits. 21f2f1c9f6

Added versionbits status info to getblockchaininfo RPC. 009bf08a65

CodeShark force-pushed on Jan 14, 2016

jtimon commented at 6:09 pm on March 16, 2016: contributor

close in favor of #7575 ?

btcdrak commented at 6:11 pm on March 16, 2016: contributor

@jtimon Yes.

laanwj closed this on Mar 17, 2016

DrahtBot locked this on Sep 8, 2021

BIP9: versionbits #6816