This is an alternative to #6816 and #7566, implementing BIP9 with minimal code changes, including mining logic, softfork warning logic, and some unit tests.
It adds support for a begin time before version bits are counted, which is not in BIP9 currently, though it is compatible.
The last commit demonstrates how to add a simple softfork.
getblockchaininfo RPC?
versionbits_test.cpp formatting/syntax issues (https://travis-ci.org/bitcoin/bitcoin/jobs/110833452#L1736)
2093+ if (softforkcheck[i]->IsSetFor(pindexPrev, params)) {
2094+ nVersion |= softforkcheck[i]->Mask(params);
2095+ }
2096+ }
2097+
2098+ if (nVersion == 0) {
Why not simply start using the versionbit prefix without voting for any concrete bit instead?
That would mean that versionbits on itself is a change, and lack of adoption of versionbits by miners would then cause the warning logic to trigger on new nodes.
Using this logc here, versionbits gets rolled out simultaneously with the first fork that uses it.
I mean, if we are going to wait to deploy the first SF to merge it, I think it makes even more sense that we never return Consensus::VERSIONBITS_LAST_OLD_BLOCK_VERSION here.
That would mean that versionbits on itself is a change, and lack of adoption of versionbits by miners would then cause the warning logic to trigger on new nodes.
Just like changing the version to 8 without doing anything else would be. But it doesn’t affect functionality. The new nodes would only get warnings if the miners, apart from adopting this change were adopting or implementing other change to vote on concrete bits (if they only set nVersion to Consensus::VERSIONBITS_TOP_BITS), no warnings should be raised (unless an unknown SF has been deployed using versionbits, in which case is good that they get the warnings).
Assume the start date for the first bip9 softfork is april 1st, but we release code with bip9 and that softfork on march 1st.
That means that it’s likely that during march no new-style nVersion will be mined. That also means that anyone who does run upgraded code will get warnings, because the nVersion they expect (0x20000000) differs from the one they see (4).
Ok, I get it, it’s the old nodes seeing the “empty versionbits” version which would get (unnecessarily?) be told to upgrade and it doesn’t matter whether we introduce a SF with BIP9 or not because the start date will be after the miners start using the new code to produce their new versions.
But I think I still disagree, though. If in march 15th 95% of the miners are ready to deploy a softfork scheduled for april 1st, I believe old nodes should actually receive the upgrade warning already, even if the message is not accurate.
EDIT: anyway, this disagreement is minor in the sense that it cannot greatly affect the implementation, the difference will be about 3-lines. So don’t hurry to answer this.
EDIT: nah, 95% of miner will use BIP9 later, never mind, question answered.
2497@@ -2451,20 +2498,14 @@ void static UpdateTip(CBlockIndex *pindexNew) {
2498 static bool fWarned = false;
2499 if (!IsInitialBlockDownload() && !fWarned)
2512- {
2513+ if (warningcheck.IsLockedIn(pindex, chainParams.GetConsensus())) {
2514 // strMiscWarning is read by GetWarnings(), called by Qt and the JSON-RPC code to warn the user:
2515- strMiscWarning = _("Warning: This version is obsolete; upgrade required!");
2516+ if (warningcheck.IsActiveFor(pindex, chainParams.GetConsensus())) {
2517+ strMiscWarning = _("Warning: unknown softfork has activated");
42+
43+private:
44+ // A map that gives the state for blocks whose height is a multiple of Period().
45+ // The map is indexed by the block's parent, however, so all keys in the map
46+ // will either be NULL or a block with (height + 1) % Period() == 0.
47+ std::map<const CBlockIndex*, State> cache;
This is the main difference between your implementation and mine. You have a separate Cache for each deployment while I have a single cache with containing the states of all the deployments. While mine should be more efficient (at least storage-wise), your code is more reusable (as shown in the way you implement the warnings checks). Another goal for caching the states together is to simplify things in case libconsensus exposes (C API) the cache in the future to support concurrent calls.
EDIT, the biggest difference is my CVersionBitsState::usedBitsMaskCache see the comment in the state machine.
29+ virtual int Period(const Params& params) =0;
30+ virtual int Threshold(const Params& params) =0;
31+
32+ enum State {
33+ UNDEFINED,
34+ DEFINED,
49+ switch (state) {
50+ case UNDEFINED: {
51+ if (pindexPrev->GetMedianTimePast() >= nTimeTimeout) {
52+ stateNext = FAILED;
53+ } else if (pindexPrev->GetMedianTimePast() >= nTimeDefined) {
54+ stateNext = DEFINED;
46+ pindexPrev = vToCompute.back();
47+ vToCompute.pop_back();
48+
49+ switch (state) {
50+ case UNDEFINED: {
51+ if (pindexPrev->GetMedianTimePast() >= nTimeTimeout) {
2080@@ -2080,6 +2081,47 @@ void PartitionCheck(bool (*initialDownloadCheck)(), CCriticalSection& cs, const
2081 }
2082 }
2083
2084+Consensus::VersionBitsConditionChecker checkCSV(Consensus::DEPLOYMENT_CSV);
These variables could be both static and be in consensus/versionbits.cpp instead of main.cpp by moving ComputeBlockVersion() to versionbits as well (of course, without cs_main).
WarningBitsConditionChecker could be encapsulated behind a function there too. In a very far future in which Bitcoin Core only uses libconsensus through its C API, that GetVersionBitsWarnings(const CBlockIndex* pindexPrev, const Consensus::Params&) function seems much closer from being “exposable” in libconsensus than class Consensus::AbstractThresholdConditionChecker.
I know that ComputeBlockVersion() and GetVersionBitsWarnings() are not strictly consensus code, but they heavily depend on consensus code, and I think it would be a wasted opportunity not to have this functionality as encapsulated as possible from birth.
I don’t think we want locks in libconsensus, not even post c++11. This reminds me of a previous conversation we had about concurrency and the cache. It seems a renounced to supporting concurrent calls without locks in libconsensus for the wrong reason. When you previously said " no need to have locks in the internal cache, the caller can get cs_main", it seems you meant something different than what I understood. I will go back to having a C API interface for the cache.
In this PR, can’t the caller of GetFlags (), ComputeBlockVersion() and GetVersionBitsWarnings() take the lock (assuming you move this code behind those 3 functions to encapsulate the static vars)?
2217@@ -2176,6 +2218,11 @@ bool ConnectBlock(const CBlock& block, CValidationState& state, CBlockIndex* pin
2218 flags |= SCRIPT_VERIFY_CHECKLOCKTIMEVERIFY;
2219 }
2220
2221+ // Start enforcing CHECKSEQUENCEVERIFY using versionbits logic.
please, please, please libconsensus refactor nit:
Could you replace these lines with
0flags |= Consensus::GetFlags(pindex, chainparams.GetConsensus());
?
(is it pindex->pprev or just pindex here?)
Then the function defined in consensus/versionbits.cpp can be extended for new deployments while avoiding new consensus code in main.
That implies, again, that checkCSV (and thus softforkcheck) would need to move to consensus/versionbits.cpp.
GetFlags() function, as I believe we will need to have a separate one of those to deploy BIP68 (which doesn’t use a script flags variable, but uses a locktimeflags variable instead)
Updated to correspond to latest BIP9 (see https://github.com/bitcoin/bips/pull/344, https://github.com/bitcoin/bips/pull/345, https://github.com/bitcoin/bips/pull/346), and fixed the tests.
I also have an alternate version that factors out the cache, see https://github.com/sipa/bitcoin/commit/218d2fdcbfb207ed75f4f5a6fc7a1b5de6d791b8).
Should be ready for review.
2522+ if (state == THRESHOLD_ACTIVE || THRESHOLD_LOCKED_IN) {
2523+ if (state == THRESHOLD_ACTIVE) {
2524+ strMiscWarning = _("Warning: unknown softfork has activated");
2525+ CAlert::Notify(strMiscWarning, true);
2526+ } else {
2527+ LogPrintf("Warning: An unknown softfork is about to activate");
\n
Fixed a few bugs:
42@@ -22,6 +43,14 @@ struct Params {
43 /** Block height and hash at which BIP34 becomes active */
44 int BIP34Height;
45 uint256 BIP34Hash;
46+ /**
47+ * Minimum blocks including miner confirmation of the total of 2016 blocks in a retargetting period,
48+ * (nPowTargetTimespan / nPowTargetSpacing) wich is also used for BIP9 deployments.
86+ }
87+
88+ VersionBitsTester& TestStarted() {
89+ for (int i = 0; i < CHECKERS; i++) {
90+ if ((insecure_rand() & ((1 << i) - 1)) == 0) {
91+ BOOST_CHECK_MESSAGE(checker[i].GetStateFor(vpblock.empty() ? NULL : vpblock.back()) == THRESHOLD_STARTED, strprintf("Test %i for DEFINED", num));
strprintf("Test %i for STARTED", num)
129+BOOST_FIXTURE_TEST_SUITE(versionbits_tests, TestingSetup)
130+
131+BOOST_AUTO_TEST_CASE(versionbits_test)
132+{
133+ for (int i = 0; i < 64; i++) {
134+ // UNDEFINED -> FAILED
Tested ACK a7f15a6
Part of my tests involved porting the softforks activation from #7561 and converting the RPC tests to check both enforcement and RPC bip9 status. Work done in https://github.com/bitcoin/bitcoin/compare/master...btcdrak:vb_68_112_113_1
2115+ int Period(const Consensus::Params& params) const { return params.nMinerConfirmationWindow; }
2116+ int Threshold(const Consensus::Params& params) const { return params.nRuleChangeActivationThreshold; }
2117+
2118+ bool Condition(const CBlockIndex* pindex, const Consensus::Params& params) const
2119+ {
2120+ return (pindex->nVersion & ~ComputeBlockVersion(pindex->pprev, params)) != 0;
Won’t this have potential false alerts, if let’s say half the blocks in a period are signaling some unknown bit i, while the other half are signaling some other unknown bit j?
Anyway it occurred to me that it might be nice to record which bits are being set that are unknown and write that to the debug log when we warn an unknown softfork is about to activate (as a useful debugging/investigation aid), but I can see that is a little tricky.
Oh it also occurs to me that we probably shouldn’t count a block towards the warning unless its top bits match the 001 versionbits prefix? Otherwise we could see warnings if we deploy versionbits+some soft fork, but miners are slow to pick it up and continue using old version numbers.
Anyway I like the idea of a warning tracker per bit…
2080@@ -2080,6 +2081,50 @@ void PartitionCheck(bool (*initialDownloadCheck)(), CCriticalSection& cs, const
2081 }
2082 }
2083
2084+// Protected by cs_main
2085+static VersionBitsCache versionbitscache;
2086+
2087+int32_t ComputeBlockVersion(const CBlockIndex* pindexPrev, const Consensus::Params& params)
I think it’d be nice if we had test coverage for this function. I believe that as it stands, if this code were busted, then I’m not sure any tests would necessarily fail?
So far the best idea I have is that when we deploy a new soft fork, we write a unit test that exercises this function and verifies the right bit will be set at the right time (using a fake mainnet chain like the one you built in the versionbits unit test).
I think we should also have regtest RPC tests which exercise proposed soft forks as well, but I can’t think of any way to write such tests to adequately test mainnet parameters…
@sipa I tried implementing a unit test for ComputeBlockVersion() using a dummy deployment; not sure adding a dummy softfork for testing is something we want to do, but if you’re interested the code for that is here:
https://github.com/sdaftuar/bitcoin/commit/b8ec634651fbc5a41afc47a768de4357983d3b4f
If you think this is worth including in some form, feel free to grab it, or I am happy to separately PR this unit test later.
2096+ }
2097+ }
2098+
2099+ if (nVersion == 0) {
2100+ // Pre-versionbits
2101+ return VERSIONBITS_LAST_OLD_BLOCK_VERSION;
| VERSIONBITS_TOP_BITS and forget about version 4 blocks.
@sipa see https://github.com/sipa/bitcoin/pull/58 for suggestions on how to rework the warning logic. It separates it into two different things.
178+ .Mine(3999, TestTime(30001), 0).TestLockedIn()
179+ .Mine(4000, TestTime(30002), 0).TestActive()
180+ .Mine(14333, TestTime(30003), 0).TestActive()
181+ .Mine(24000, TestTime(40000), 0).TestActive();
182+ }
183+}
I wrote up a couple additional tests to try sanity checking the mainnet deployments:
0diff --git a/src/test/versionbits_tests.cpp b/src/test/versionbits_tests.cpp
1index 9de8461..c384f92 100644
2--- a/src/test/versionbits_tests.cpp
3+++ b/src/test/versionbits_tests.cpp
4@@ -6,6 +6,7 @@
5 #include "random.h"
6 #include "versionbits.h"
7 #include "test/test_bitcoin.h"
8+#include "chainparams.h"
9
10 #include <boost/test/unit_test.hpp>
11
12@@ -180,6 +181,28 @@ BOOST_AUTO_TEST_CASE(versionbits_test)
13 .Mine(14333, TestTime(30003), 0).TestActive()
14 .Mine(24000, TestTime(40000), 0).TestActive();
15 }
16+
17+ // Sanity checks of version bit deployments
18+ const Consensus::Params &mainnetParams = Params(CBaseChainParams::MAIN).GetConsensus();
19+ for (int i=0; i<(int) Consensus::MAX_VERSION_BITS_DEPLOYMENTS; i++) {
20+ uint32_t bitmask = VersionBitsMask(mainnetParams, (Consensus::DeploymentPos)i);
21+ // Make sure that no deployment tries to set an invalid bit.
22+ BOOST_CHECK(bitmask < (uint32_t)VERSIONBITS_TOP_BITS);
23+
24+ // Verify that the deployment windows of different deployment using the
25+ // same bit are disjoint.
26+ // This test may need modification at such time as a new deployment
27+ // is proposed that reuses the bit of an activated soft fork, before the
28+ // end time of that soft fork. (Alternatively, the end time of that
29+ // activated soft fork could be later changed to be earlier to avoid
30+ // overlap.)
31+ for (int j=i+1; j<(int) Consensus::MAX_VERSION_BITS_DEPLOYMENTS; j++) {
32+ if (VersionBitsMask(mainnetParams, (Consensus::DeploymentPos)j) == bitmask) {
33+ BOOST_CHECK(mainnetParams.vDeployments[j].nStartTime > mainnetParams.vDeployments[i].nTimeout ||
34+ mainnetParams.vDeployments[i].nStartTime > mainnetParams.vDeployments[j].nTimeout);
35+ }
36+ }
37+ }
38 }
39
40 BOOST_AUTO_TEST_SUITE_END()
Perhaps worth including something like that here?
https://github.com/sdaftuar/bitcoin/commit/471d10583b197556ff7d9ae4c30a258a8f3061ff
Actually, I rebased this, with a minor change to use VERSIONBITS_TOP_MASK rather than VERSIONBITS_TOP_BITS when checking the bitmask:
https://github.com/sdaftuar/bitcoin/commit/6b7ec42549d75292719ca7f77a087c4f471de92f
Edit: Ok I think this is what I intend: https://github.com/sdaftuar/bitcoin/commit/0e492acd62fa792f23e5400e810c7b8b1b13d7b9
2080@@ -2080,6 +2081,50 @@ void PartitionCheck(bool (*initialDownloadCheck)(), CCriticalSection& cs, const
2081 }
2082 }
2083
2084+// Protected by cs_main
2085+static VersionBitsCache versionbitscache;
UnloadBlockIndex(). In particular, I think that if we don’t, then the miner_tests will pollute the versionbitscache with cached information that might be invalid in later tests (because the CBlockIndex pointers could be reused, potentially resulting in incorrect state calculations).
Inspired by former implementations by Eric Lombrozo and Rusty Russell, and
based on code by Jorge Timon.
2534+ LogPrintf("%s: %d of last 100 blocks have unexpected version\n", __func__, nUpgraded);
2535+ if (nUpgraded > 100/2 && !fWarned)
2536 {
2537 // strMiscWarning is read by GetWarnings(), called by Qt and the JSON-RPC code to warn the user:
2538- strMiscWarning = _("Warning: This version is obsolete; upgrade required!");
2539+ strMiscWarning = _("Warning: unknown block versions being mined on the network!");
"Warning: Unknown block versions being mined! It's possible unknown rules are in effect" or something to that effect?
2514+ if (state == THRESHOLD_ACTIVE) {
2515+ strMiscWarning = strprintf(_("Warning: unknown new rules activated (versionbit %i)"), bit);
2516+ CAlert::Notify(strMiscWarning, true);
2517+ fWarned = true;
2518+ } else {
2519+ LogPrintf("%s: unknown new rules are about to activate (versionbit %i)\n", bit);
__func__?
2518+ } else {
2519+ LogPrintf("%s: unknown new rules are about to activate (versionbit %i)\n", bit);
2520+ }
2521+ }
2522+ }
2523+ int32_t nExpectedVersion = ComputeBlockVersion(pindex->pprev, chainParams.GetConsensus());
(Sorry if this is a repost, I think github ate my first attempt to comment)
I think we should move this line into the for loop below; otherwise we’d generate a warning after a known soft-fork activates.
ACK
If anyone is interested I also tested the warning logic with an RPC test (https://github.com/sdaftuar/bitcoin/commit/4c2e8ba2282e0948aa8680ab5e8283e5f8ef12f5)
603@@ -604,6 +604,20 @@ static UniValue SoftForkDesc(const std::string &name, int version, CBlockIndex*
604 return rv;
605 }
606
607+static UniValue BIP9SoftForkDesc(const std::string& name, const Consensus::Params& consensusParams, Consensus::DeploymentPos id)
It’s called once for every deployment below, similar to how SoftForkMajorityDesc is. There just are no deployments just yet.
Making this function iterate through all deployments would require putting knowledge of the names of all deployments in the consensus params, and would make it inevitable that the test deployment is listed.
Mhmm, I can only find one instance of the string “BIP9SoftForkDesc” in the changes.
re itarate with knowledge of the names, what’s wrong with
0for (int i = 0; i < MAX_VERSION_BITS_DEPLOYMENTS; ++i)
?
You don’t know what string name to give to the Object entry inside “bip9_softforks”.
Yes you can’t find any calls to it yet, because there are no BIP9 softforks defined. Future deployments using BIP9 are supposed to be added below, just like ISM softforks were supposed to be added below to “softforks”.
Style, oh, never mind: https://github.com/bitcoin/bitcoin/blob/master/src/.clang-format#L27
I see, I didn’t thought about the name.
@sipa I realize you might still be changing this, but in the event that you’re not, I backported it to 0.11 along with the soft fork for BIP 68,112,113 here: https://github.com/bitcoin/bitcoin/compare/0.11...morcos:backportBIP9SoftFork?expand=1
The RPC tests are annoying to backport, so I left 2 of them out, but tested using the recent RPC test bip68-112-113.py against the 0.11 binary.
6@@ -7,8 +7,29 @@
7 #define BITCOIN_CONSENSUS_PARAMS_H
8
9 #include "uint256.h"
10+#include <map>
11+#include <string>
12
13 namespace Consensus {
14+
15+enum DeploymentPos
Why is this called “DeploymentPos”? It’s not a bit position; the actual bit positions have another layer of indirection in the BIP9Deployment structure: https://github.com/sipa/bitcoin/blob/8c74cedef53ab791ed333f25794f8b9d2e9f51aa/src/consensus/params.h#L26
We should change this to DeploymentId
