Versionbits: GBT support #7935
Versionbits: GBT support #7935
Conversation
|
@sipa Added a commit to try to address the locking stuff cleaner. |
| 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) { |
There was a problem hiding this comment.
Tangential nit (just so I don't forget): DeploymentPos should become a strongly typed enum
edit: ThresholdState too.
There was a problem hiding this comment.
Should I be iterating over them some other way?
There was a problem hiding this comment.
See below. I think returning a map would make this more clear.
|
ut ACK, other than the nits. |
|
Needs some tests, though. |
| { | ||
| private: | ||
| CCriticalSection cs; |
There was a problem hiding this comment.
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.
|
@luke-jr Can you revert the last commit? The design for having the data structure not do its own locking is very intentional. |
|
Reverted... but it's what I thought you wanted. |
|
Updated for '!' prefix |
| @@ -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(); | |||
There was a problem hiding this comment.
Bike-shedding: Can you call this var consensusParams for consistency with other parts of the code?
|
Fast-review ACK |
|
Tested with libblkmaker 0.5.3 (ie, using version/force) and bitcoin/libblkmaker#5 on testnet-in-a-box. |
|
utACK 46e8e06a5b62892024bf0da0e6104cc30a2a50cb. Squash some? |
|
Squashing is a bad practice, so I prefer not to, if that's okay... |
|
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. |
|
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.
|
|
The merged tree is forever. We should favor post-merge reviewability. |
|
Squashed into 4 logical commits, and added a few comment lines explaining the logic around omitting a rule check for version/force. |
| // 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)); |
There was a problem hiding this comment.
more than a small formatting nit here :)
There was a problem hiding this comment.
LOL, how did that happen? >_<
| @@ -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 | |||
|
ut ACK, other than the nits above. I'll do thorough testing of segwit's implementation here. |
- 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
…ting softforks vs not
|
Nits fixed. |
|
utACK 12c708a |
|
Also needs backport to 0.12. |
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)
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).