/bitcoin

Have a question about this project? Sign up for a free GitHub account to open an issue and contact its maintainers and the community.

Already on GitHub? Sign in to your account

Versionbits: GBT support #7935

Merged
merged 4 commits into from Jun 8, 2016

Conversation

Reviewers
No reviews
Assignees
No one assigned
Labels
Mining Needs backport
Projects
None yet
Milestone
  0.12.branch
9 participants
@luke-jr @theuni @sipa @jtimon @MarcoFalke @gmaxwell @sdaftuar @jonasschnelli @laanwj
@luke-jr
Member

luke-jr commented Apr 25, 2016 •
Edited 1 time

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

@jonasschnelli jonasschnelli added the Mining label Apr 25, 2016

@luke-jr
Member

luke-jr commented Apr 27, 2016

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

@theuni theuni and 1 other commented on an outdated diff May 3, 2016

src/rpc/mining.cpp
UniValue result(UniValue::VOBJ);
result.push_back(Pair("capabilities", aCaps));
+
+ UniValue aRules(UniValue::VARR);
+ UniValue vbavailable(UniValue::VOBJ);
+ const std::vector<ThresholdState> vbstates = versionbitscache.GetAllThresholdStates(pindexPrev, cparams);
+ for (int i = 0; i < (int)Consensus::MAX_VERSION_BITS_DEPLOYMENTS; ++i) {
@theuni

theuni May 3, 2016 •
Edited 1 time

Member

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

@luke-jr

luke-jr May 3, 2016

Member

Should I be iterating over them some other way?

@theuni

theuni May 3, 2016

Member

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

@theuni theuni and 1 other commented on an outdated diff May 3, 2016

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

theuni May 3, 2016

Member

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

@luke-jr

luke-jr May 3, 2016

Member

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.

@theuni theuni commented on an outdated diff May 3, 2016

src/versionbits.cpp
for (unsigned int d = 0; d < Consensus::MAX_VERSION_BITS_DEPLOYMENTS; d++) {
caches[d].clear();
}
}
+
+ThresholdState VersionBitsCache::VersionBitsState(const CBlockIndex* pindexPrev, const Consensus::Params& params, Consensus::DeploymentPos pos)
+{
+ LOCK(cs);
+ return VersionBitsConditionChecker(pos).CachedGetStateFor(pindexPrev, params, caches[pos]);
+}
+
+std::vector<ThresholdState> VersionBitsCache::GetAllThresholdStates(const CBlockIndex* pindexPrev, const Consensus::Params& params)
+{
+ std::vector<ThresholdState> ret;
+ ret.reserve((int)Consensus::MAX_VERSION_BITS_DEPLOYMENTS);
+ LOCK(cs);
+ for (int i = 0; i < (int)Consensus::MAX_VERSION_BITS_DEPLOYMENTS; ++i) {
@theuni

theuni May 3, 2016 •
Edited 1 time

Member

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.

@theuni theuni commented on an outdated diff May 3, 2016

src/rpc/mining.cpp
@@ -369,6 +380,17 @@ UniValue getblocktemplate(const UniValue& params, bool fHelp)
TestBlockValidity(state, Params(), block, pindexPrev, false, true);
return BIP22ValidationResult(state);
}
+
+ const UniValue& aClientRules = find_value(oparam, "rules");
+ if (aClientRules.isArray()) {
+ for (unsigned int i = 0; i < aClientRules.size(); ++i) {
+ const UniValue& v = aClientRules[i];
+ setClientRules.insert(v.get_str());
+ }
+ } else {
+ const UniValue& uvMaxVersion = find_value(oparam, "maxversion");
@theuni

theuni May 3, 2016

Member

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

@theuni theuni commented on the diff May 4, 2016

View full changes
src/versionbits.cpp
@@ -4,7 +4,20 @@
#include "versionbits.h"
-ThresholdState AbstractThresholdConditionChecker::GetStateFor(const CBlockIndex* pindexPrev, const Consensus::Params& params, ThresholdConditionCache& cache) const
+#include "consensus/params.h"
+
+const struct BIP9DeploymentInfo VersionBitsDeploymentInfo[Consensus::MAX_VERSION_BITS_DEPLOYMENTS] = {
+ {
+ /*.name =*/ "testdummy",
+ /*.gbt_force =*/ true,
+ },
+ {
+ /*.name =*/ "csv",
+ /*.gbt_force =*/ true,
@theuni

theuni May 4, 2016

Member

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

luke-jr May 4, 2016

Member

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

theuni May 4, 2016

Member

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
Member

theuni commented May 5, 2016

ut ACK, other than the nits.
@theuni
Member

theuni commented May 5, 2016

Needs some tests, though.

@sipa sipa commented on an outdated diff May 9, 2016

src/versionbits.h
{
+private:
+ CCriticalSection cs;
@sipa

sipa May 9, 2016 •
Edited 1 time

Owner

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
Owner

sipa commented May 9, 2016

@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
Member

luke-jr commented May 9, 2016

Reverted... but it's what I thought you wanted.
@luke-jr
Member

luke-jr commented May 13, 2016

Updated for '!' prefix

@luke-jr luke-jr referenced this pull request May 18, 2016

Closed

[0.11] Backport BIP9 and softfork for BIP's 68,112,113 #7716

@jtimon jtimon commented on an outdated diff May 20, 2016

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

jtimon May 20, 2016 •
Edited 1 time

Member

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

@sdaftuar sdaftuar and 1 other commented on an outdated diff May 21, 2016

src/rpc/mining.cpp
+ UniValue vbavailable(UniValue::VOBJ);
+ for (int i = 0; i < (int)Consensus::MAX_VERSION_BITS_DEPLOYMENTS; ++i) {
+ Consensus::DeploymentPos pos = Consensus::DeploymentPos(i);
+ ThresholdState state = VersionBitsState(pindexPrev, consensusParams, pos, versionbitscache);
+ switch (state) {
+ case THRESHOLD_DEFINED:
+ case THRESHOLD_FAILED:
+ // Not exposed to GBT at all
+ break;
+ case THRESHOLD_LOCKED_IN:
+ // Ensure bit is set in block version
+ pblock->nVersion |= VersionBitsMask(consensusParams, pos);
+ // FALL THROUGH to get vbavailable set...
+ case THRESHOLD_STARTED:
+ // Add to vbavailable (and it's presumably in version already)
+ vbavailable.push_back(Pair(gbt_vb_name(pos), consensusParams.vDeployments[pos].bit));
@sdaftuar

sdaftuar May 21, 2016

Member

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

luke-jr May 21, 2016

Member

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

@jtimon jtimon commented on the diff May 21, 2016

View full changes
src/versionbits.h
@@ -30,6 +30,15 @@ enum ThresholdState {
// will either be NULL or a block with (height + 1) % Period() == 0.
typedef std::map<const CBlockIndex*, ThresholdState> ThresholdConditionCache;
+struct BIP9DeploymentInfo {
@jtimon

jtimon May 21, 2016

Member

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

@luke-jr

luke-jr May 21, 2016

Member

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

@jtimon

jtimon May 22, 2016 •
Edited 1 time

Member

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
Member

jtimon commented May 21, 2016

Fast-review ACK

@mrCertified mrCertified referenced this pull request May 25, 2016

Closed

Option for disabling the system tray #7160

@luke-jr
Member

luke-jr commented May 29, 2016

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

sipa commented May 31, 2016

utACK 46e8e06a5b62892024bf0da0e6104cc30a2a50cb. Squash some?
@luke-jr
Member

luke-jr commented Jun 1, 2016

Squashing is a bad practice, so I prefer not to, if that's okay...
@MarcoFalke
Member

MarcoFalke commented Jun 1, 2016

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
Owner

sipa commented Jun 1, 2016

@gmaxwell
Member

gmaxwell commented Jun 1, 2016

The merged tree is forever. We should favor post-merge reviewability.
@luke-jr
Member

luke-jr commented Jun 1, 2016

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

@theuni theuni and 1 other commented on an outdated diff Jun 4, 2016

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

theuni Jun 4, 2016

Member

more than a small formatting nit here :)

@luke-jr

luke-jr Jun 4, 2016

Member

LOL, how did that happen? >_<

@theuni theuni commented on an outdated diff Jun 6, 2016

src/consensus/params.h
@@ -16,6 +16,7 @@ enum DeploymentPos
{
DEPLOYMENT_TESTDUMMY,
DEPLOYMENT_CSV, // Deployment of BIP68, BIP112, and BIP113.
+ // NOTE: Also add new deployments to VersionBitsDeploymentNames in versionbits.cpp
@theuni

theuni Jun 6, 2016

Member

VersionBitsDeploymentInfo
@theuni
Member

theuni commented Jun 6, 2016

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

luke-jr added some commits Apr 23, 2016

@luke-jr
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
d3df40e
@luke-jr
qa/rpc-tests: bip9-softforks: Add tests for getblocktemplate versionb… 
…its updates
72cd6b2
@luke-jr
getblocktemplate: Explicitly handle the distinction between GBT-affec… 
…ting softforks vs not
9879060
@luke-jr
getblocktemplate: Use version/force mutation to support pre-BIP9 clients
12c708a
@luke-jr
Member

luke-jr commented Jun 6, 2016

Nits fixed.
@sipa
Owner

sipa commented Jun 6, 2016

utACK 12c708a
@sipa
Owner

sipa commented Jun 8, 2016

Also needs backport to 0.12.

@sipa sipa merged commit 12c708a into bitcoin:master Jun 8, 2016
1 check passed

1 check passed

continuous-integration/travis-ci/pr The Travis CI build passed
Details

sipa added a commit that referenced this pull request Jun 8, 2016

@sipa
Merge #7935: Versionbits: GBT support 
12c708a getblocktemplate: Use version/force mutation to support pre-BIP9 clients (Luke Dashjr)
9879060 getblocktemplate: Explicitly handle the distinction between GBT-affecting softforks vs not (Luke Dashjr)
72cd6b2 qa/rpc-tests: bip9-softforks: Add tests for getblocktemplate versionbits updates (Luke Dashjr)
d3df40e Implement BIP 9 GBT changes (Luke Dashjr)
This user has not uploaded their public key yet.
GPG key ID: DBA1A67379A1A931 Learn about signing commits
66ed450

@MarcoFalke MarcoFalke added the Needs backport label Jun 8, 2016

This was referenced Jun 8, 2016

Merged
Closed

luke-jr added a commit to bitcoinknots/bitcoin that referenced this pull request Jun 27, 2016

@luke-jr
Merge #7935 gbt_bip9-0.12.x
e79e131

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

Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment