Versionbits: GBT support

luke-jr commented at 5:09 am on April 25, 2016: member

Versionbits was released in 0.12.1, but only included updates for the consensus layer. Due to the low-level design of GBT, it is currently not possible to use in practice, since the client has no way to identify the meaning of the block version anymore. Miners and pools can as a hack override/ignore the block version entirely, but this is not really a solution.

This change adds the necessary versionbits information to GBT so that miners can implement it safely.

It also detects when the client is outdated, but can still safely use the template as-is, and uses the GBT version/force and/or rules/force mutability flags to indicate that. This enables older miners that only support at least BIP 34 (block v2) to work with the newer bitcoind. Obviously this is a very short-term benefit in practice, since segwit necessarily will break such miners, but will become useful again as soon as the next simple softfork is deployed (in which case clients need only support segwit).

Corresponding BIP changes: bitcoin/bips#365
Corresponding libblkmaker changes: bitcoin/libblkmaker#3 (0.4.x) and bitcoin/libblkmaker#4 (0.5.x)
Corresponding 0.12 backport: TODO following review of this PR

jonasschnelli added the label Mining on Apr 25, 2016

luke-jr commented at 5:09 am on April 27, 2016: member

@sipa Added a commit to try to address the locking stuff cleaner.

luke-jr force-pushed on Apr 27, 2016

in src/rpc/mining.cpp: in 672796e329 outdated

541     result.push_back(Pair("capabilities", aCaps));
542+
543+    UniValue aRules(UniValue::VARR);
544+    UniValue vbavailable(UniValue::VOBJ);
545+    const std::vector<ThresholdState> vbstates = versionbitscache.GetAllThresholdStates(pindexPrev, cparams);
546+    for (int i = 0; i < (int)Consensus::MAX_VERSION_BITS_DEPLOYMENTS; ++i) {

theuni commented at 8:49 pm on May 3, 2016:

Tangential nit (just so I don’t forget): DeploymentPos should become a strongly typed enum edit: ThresholdState too.

luke-jr commented at 9:03 pm on May 3, 2016:

Should I be iterating over them some other way?

theuni commented at 10:55 pm on May 3, 2016:

See below. I think returning a map would make this more clear.

in src/versionbits.h: in f8419e0184 outdated

29@@ -30,6 +30,8 @@ enum ThresholdState {
30 // will either be NULL or a block with (height + 1) % Period() == 0.
31 typedef std::map<const CBlockIndex*, ThresholdState> ThresholdConditionCache;
32 
33+extern const char * const VersionBitsDeploymentNames[];

theuni commented at 9:32 pm on May 3, 2016:

std::array here would be much nicer. Hooray for c++11!

luke-jr commented at 11:07 pm on May 3, 2016:

Whatever we’re doing with C++11 in 0.13 doesn’t help here, since this needs to be backported to 0.11 and 0.12.

in src/versionbits.cpp: in 672796e329 outdated

155+std::vector<ThresholdState> VersionBitsCache::GetAllThresholdStates(const CBlockIndex* pindexPrev, const Consensus::Params& params)
156+{
157+    std::vector<ThresholdState> ret;
158+    ret.reserve((int)Consensus::MAX_VERSION_BITS_DEPLOYMENTS);
159+    LOCK(cs);
160+    for (int i = 0; i < (int)Consensus::MAX_VERSION_BITS_DEPLOYMENTS; ++i) {

theuni commented at 10:22 pm on May 3, 2016:

Aligning indices everywhere is a bit clunky, and will get worse with more deployments. Just return a std::map<DeploymentPos,ThresholdState> ? Then the caller can iterate cleanly.

in src/rpc/mining.cpp: in 672796e329 outdated

386+            for (unsigned int i = 0; i < aClientRules.size(); ++i) {
387+                const UniValue& v = aClientRules[i];
388+                setClientRules.insert(v.get_str());
389+            }
390+        } else {
391+            const UniValue& uvMaxVersion = find_value(oparam, "maxversion");

theuni commented at 11:27 pm on May 3, 2016:

need to make sure this was found before get_int64()?

in src/versionbits.cpp: in 672796e329 outdated

12+        /*.name =*/ "testdummy",
13+        /*.gbt_force =*/ true,
14+    },
15+    {
16+        /*.name =*/ "csv",
17+        /*.gbt_force =*/ true,

theuni commented at 0:13 am on May 4, 2016:

Is a binary forced/not-forced enough?

Taking segwit as an example, post-activation, what would gbt_force = true mean? should I expect “segwit” to be always forced and transactions filtered? Forced if an commitment isn’t required and an error otherwise? Isn’t this a per-block property?

As gbt rules aren’t necessarily static, I’m not sure that a simple flag makes sense. Or maybe I’m missing the bigger picture. Do you have that part coded up somewhere already, by chance?

luke-jr commented at 0:18 am on May 4, 2016:

This is just an internal detail to bitcoind, so we can change it in the future if needed. See how rpc/mining applies it in this PR…

theuni commented at 0:31 am on May 4, 2016:

Understood, I was just making the point that it’s not really defining much so it’s somewhat misleading. No problem making it more dynamic once that’s required, though.

theuni commented at 4:42 pm on May 5, 2016: member

ut ACK, other than the nits.

theuni commented at 4:44 pm on May 5, 2016: member

Needs some tests, though.

in src/versionbits.h: in 672796e329 outdated

60 
61-struct VersionBitsCache
62+class VersionBitsCache
63 {
64+private:
65+    CCriticalSection cs;

sipa commented at 7:47 pm on May 9, 2016:

NACK on this; this class will be needed inside abstracted consensus logic, which shouldn’t do its own locking.

VersionBitsCache is intended to be a black box data type locked by the user (main, in this case) but only accessed through functions in versionbits.cpp.

sipa commented at 7:49 pm on May 9, 2016: member

@luke-jr Can you revert the last commit? The design for having the data structure not do its own locking is very intentional.

luke-jr force-pushed on May 9, 2016

luke-jr commented at 8:17 pm on May 9, 2016: member

Reverted… but it’s what I thought you wanted.

luke-jr force-pushed on May 13, 2016

luke-jr commented at 10:34 pm on May 13, 2016: member

Updated for ‘!’ prefix

in src/rpc/mining.cpp: in f369434a64 outdated

489@@ -458,9 +490,10 @@ UniValue getblocktemplate(const UniValue& params, bool fHelp)
490         pindexPrev = pindexPrevNew;
491     }
492     CBlock* pblock = &pblocktemplate->block; // pointer for convenience
493+    const Consensus::Params& cparams = Params().GetConsensus();

jtimon commented at 11:57 am on May 20, 2016:

Bike-shedding: Can you call this var consensusParams for consistency with other parts of the code?

in src/rpc/mining.cpp: in c52f7cab0b outdated

567+                // Ensure bit is set in block version
568+                pblock->nVersion |= VersionBitsMask(consensusParams, pos);
569+                // FALL THROUGH to get vbavailable set...
570+            case THRESHOLD_STARTED:
571+                // Add to vbavailable (and it's presumably in version already)
572+                vbavailable.push_back(Pair(gbt_vb_name(pos), consensusParams.vDeployments[pos].bit));

sdaftuar commented at 11:42 am on May 21, 2016:

For deployments where gbt_force == False, we should clear the bit if the client hasn’t signaled support, to prevent activation of proposed soft forks that require software changes downstream.

luke-jr commented at 1:03 pm on May 21, 2016:

Redid this logic (finding and fixing a few more bugs in the process) - how’s it look now?

in src/versionbits.h: in c52f7cab0b outdated

29@@ -30,6 +30,15 @@ enum ThresholdState {
30 // will either be NULL or a block with (height + 1) % Period() == 0.
31 typedef std::map<const CBlockIndex*, ThresholdState> ThresholdConditionCache;
32 
33+struct BIP9DeploymentInfo {

jtimon commented at 1:25 pm on May 21, 2016:

Does this need to be with the consensus code? Couldn’t it be moved to miner.o or somewhere else?

luke-jr commented at 2:08 pm on May 21, 2016:

Possibly. @sdaftuar was thinking maybe we should move the bit assignments here, however…

jtimon commented at 11:59 am on May 22, 2016:

I mean, I would say the only reason for not having versionbits.o in the consensus package already is because it still depends on chain.o (which is storage-dependent).

jtimon commented at 1:27 pm on May 21, 2016: contributor

Fast-review ACK

luke-jr commented at 1:31 pm on May 29, 2016: member

Tested with libblkmaker 0.5.3 (ie, using version/force) and bitcoin/libblkmaker#5 on testnet-in-a-box.

sipa commented at 1:43 pm on May 31, 2016: member

utACK 46e8e06a5b62892024bf0da0e6104cc30a2a50cb. Squash some?

luke-jr commented at 2:39 pm on June 1, 2016: member

Squashing is a bad practice, so I prefer not to, if that’s okay…

MarcoFalke commented at 3:13 pm on June 1, 2016: member

There is 12 commits, so squashing would help to not bury the previous code under several layers of history. Not to mention that squashing would make it probably easier to read the commits of this pull.

sipa commented at 3:26 pm on June 1, 2016: member

Squashing is worse for retaining the history of the patch and downstream branches, but better for post-merge reviewability. I think that as a project, we favor having good transparency of the changes over the exact history that led to the development of those changes.

gmaxwell commented at 4:27 pm on June 1, 2016: contributor

The merged tree is forever. We should favor post-merge reviewability.

luke-jr force-pushed on Jun 1, 2016

luke-jr commented at 4:56 pm on June 1, 2016: member

Squashed into 4 logical commits, and added a few comment lines explaining the logic around omitting a rule check for version/force.

in src/rpc/mining.cpp: in e8caa6c11e outdated

554@@ -544,13 +555,29 @@ UniValue getblocktemplate(const UniValue& params, bool fHelp)
555                 pblock->nVersion |= VersionBitsMask(consensusParams, pos);
556                 // FALL THROUGH to get vbavailable set...
557             case THRESHOLD_STARTED:
558-                // Add to vbavailable (and it's presumably in version already)
559-                vbavailable.push_back(Pair(gbt_vb_name(pos), consensusParams.vDeployments[pos].bit));
560+            {
561+                const struct BIP9DeploymentInfo& vbinfo = VersionBitsDeploymentInfo[pos];                vbavailable.push_back(Pair(gbt_vb_name(pos), consensusParams.vDeployments[pos].bit));

theuni commented at 2:33 am on June 4, 2016:

more than a small formatting nit here :)

luke-jr commented at 3:10 am on June 4, 2016:

LOL, how did that happen? >_<

in src/consensus/params.h: in 2d11ce63b1 outdated

15@@ -16,6 +16,7 @@ enum DeploymentPos
16 {
17     DEPLOYMENT_TESTDUMMY,
18     DEPLOYMENT_CSV, // Deployment of BIP68, BIP112, and BIP113.
19+    // NOTE: Also add new deployments to VersionBitsDeploymentNames in versionbits.cpp

theuni commented at 2:24 pm on June 6, 2016:

VersionBitsDeploymentInfo

theuni commented at 2:46 pm on June 6, 2016: member

ut ACK, other than the nits above. I’ll do thorough testing of segwit’s implementation here.

Implement BIP 9 GBT changes

- BIP9DeploymentInfo struct for static deployment info
- VersionBitsDeploymentInfo: Avoid C++11ism by commenting parameter names
- getblocktemplate: Make sure to set deployments in the version if it is LOCKED_IN
- In this commit, all rules are considered required for clients to support

d3df40e51a

qa/rpc-tests: bip9-softforks: Add tests for getblocktemplate versionbits updates 72cd6b20ca

getblocktemplate: Explicitly handle the distinction between GBT-affecting softforks vs not 98790608a4

getblocktemplate: Use version/force mutation to support pre-BIP9 clients 12c708a4b3

luke-jr force-pushed on Jun 6, 2016

luke-jr commented at 5:11 pm on June 6, 2016: member

Nits fixed.

sipa commented at 10:16 pm on June 6, 2016: member

utACK 12c708a4b3a799478fbb3f93fda696706177a824

sipa commented at 1:04 pm on June 8, 2016: member

Also needs backport to 0.12.

sipa merged this on Jun 8, 2016

sipa closed this on Jun 8, 2016

sipa referenced this in commit 66ed450d77 on Jun 8, 2016

MarcoFalke added the label Needs backport on Jun 8, 2016

luke-jr referenced this in commit e79e131a15 on Jun 27, 2016

laanwj added this to the milestone 0.12.2 on Sep 26, 2016

fanquake removed the label Needs backport on Mar 7, 2018

MarcoFalke locked this on Sep 8, 2021

Versionbits: GBT support #7935