This hardcodes CSV and segwit activation heights, similar to the BIP 90 buried deployments for BIPs 34, 65 and 66.
CSV and segwit have been active for over 18 months. Hardcoding the activation height is a code simplification, makes it easier to understand segwit activation status, and reduces technical debt.
This was originally attempted by jl2012 in #11398 and again by me in #12360.
The following sections might be updated with supplementary metadata relevant to reviewers and maintainers.
Reviewers, this pull request conflicts with the following ones:
If you consider this pull request important, please also help to review the conflicting pull requests. Ideally, start with the one that should be merged first.
Force-pushed to fix some minor issues from rebasing.
Wouldn’t it help to split out the first commit, which changes how ISM deployments are reported?
All three commits change the output of getblockchaininfo and how soft forks are reported, so it makes sense to merge them in the same PR.
349+ if (gArgs.IsArgSet("-segwitheight")) {
350+ int64_t height = gArgs.GetArg("-segwitheight", consensus.SegwitHeight);
351+ if (height < -1 || height >= std::numeric_limits<int>::max()) {
352+ throw std::runtime_error(strprintf("Activation height %ld for segwit is out of valid range. Use -1 to disable segwit.", height));
353+ }
354+ else if (height == -1) {
} else if (height == -1) {
1287+ rv.pushKV("type", "bip9");
1288+ rv.pushKV("bip9", bip9);
1289+ if (ThresholdState::LOCKED_IN == thresholdState) {
1290+ rv.pushKV("height", since_height + consensusParams.nMinerConfirmationWindow);
1291+ }
1292+ else if (ThresholdState::ACTIVE == thresholdState) {
68@@ -69,6 +69,8 @@ class CMainParams : public CChainParams {
69 consensus.BIP34Hash = uint256S("0x000000000000024b89b42a942fe0d9fea3bb44ab7bd1b19115dd6a759c0808b8");
70 consensus.BIP65Height = 388381; // 000000000000000004c2b624ed5d7756c508d90fd0da2c7c679febfa6c4735f0
71 consensus.BIP66Height = 363725; // 00000000000000000379eaa19dce8c9b722d46ae6a57c2f1a988119488b50931
72+ consensus.CSVHeight = 419328; // 000000000000000004a1b34462cb8aeebd5799177f7a29cf28f2d1961716b5b5
73+ consensus.SegwitHeight = 481824; // 0000000000000000001c8018d9cb3b742ef25114f27563e3fc4a1902167f9893
Checked blockhashes against bock chain, and checked the activation heights against getblockchaininfo that they match since
0 "bip9_softforks": {
1 "csv": {
2 "status": "active",
3 "startTime": 1462060800,
4 "timeout": 1493596800,
5 "since": 419328
6 },
7 "segwit": {
8 "status": "active",
9 "startTime": 1479168000,
10 "timeout": 1510704000,
11 "since": 481824
12 }
13 },
1317- " \"status\": xx, (boolean) true if threshold reached\n"
1318- " },\n"
1319- " }, ...\n"
1320- " ],\n"
1321- " \"bip9_softforks\": { (object) status of BIP9 softforks in progress\n"
1322+ " \"softforks\": { (object) status of softforks\n"
I like making this a object indexed by name, makes it easier to look one up in languages that treat them as dictionaries.
For the benefit of other reviewers - that’s what this PR does already.
this is a breaking change, seems unnecessary
I doubt that there are many clients programmatically querying this, since the value never changes once it’s locked in. After this PR, there don’t need to be further breaking changes, since the new softfork object is able to hold both bip9 and buried fork details (or any other potential activation method types).
3163+ return (height >= params.SegwitHeight);
3164 }
3165
3166 bool IsNullDummyEnabled(const CBlockIndex* pindexPrev, const Consensus::Params& params)
3167 {
3168 LOCK(cs_main);
GetBlockScriptFlags(), which holds cs_main. After this PR, this is now a simple one-line check, so I’ve removed the function and placed the conditional directly into GetBlockScriptFlags().
344 };
345
346-void CRegTestParams::UpdateVersionBitsParametersFromArgs(const ArgsManager& args)
347+void CRegTestParams::UpdateActivationParametersFromArgs(const ArgsManager& args)
348 {
349+ if (gArgs.IsArgSet("-segwitheight")) {
-con_xxx naming scheme is better. For now, I’d like to keep this as it is, rather than optimizing for a PR that hasn’t been merged yet.
Why are there competing proposals? Is there a comparison sometimes of the pros/cons of either approach?
The propsals take different approaches: this one (#16060) changes segwit/csv from bip9 deployments to be hardcoded fixed height deployments; the other one (#16229) generalises bip9 deployments to also support fixed height deployments, and converts strictder and cltv into this generalised deployment method and updates segwit/csv to be fixed height rather than bip9 based.
I think the benefits of hardcoded fixed height stuff is that it’s a bit more efficient (the height’s hardcoded and every codepath knows the height is hardcoded), while the cost is that anytime you change a bip9 deployment to a fixed height deployment you have to change the code in a few places. Doing it this way also means you can’t have tests that want to control how the forks activated, which results in substantive changes to the test suite when burying a deployment.
The cost of the generalised approach is that you can’t assume that the deployments are just activated at a particular height, which slows mainnet handling of the deployments down for (at most) a benefit in regtest. I think this slowdown is pretty trivial, but haven’t benchmarked it. The benefits (I think) are that it means we can bury BIP9 deployments in future just by changing the declaration in chainparams.cpp, with tests generally continuing to work unchanged, and no code changes needed for getblockchaininfo or in checking when the soft-fork is enabled. This approach can also be done with minimal changes to getblockchaininfo output if we thought that was a benefit.
The propsals take different approaches….
Thanks @ajtowns - that seems like a good summary to me. I left a comment in your PR about why I prefer the approach here, which I’ll copy below:
I prefer the approach in #16060. The changes in this PR seem more complex for achieving a similar goal (+110 lines here vs -70 lines in 16060)
I think this should make future burials much simpler (it becomes a one-line change)
Softfork activation/burials are sufficiently rare that I don’t think we have to optimize to minimize the code changes when they do happen.
it doesn’t change RPC or tests much, and I think it will make it easier to make future changes to activation methods should we wish to do so.
I like the new RPC return object softfork in 16060. It seems simple, non-breaking when a deployment goes from bip9 to buried (the type field determines which other fields or sub-objects appear in the softfork object), and allows for other deployment methods (just add a new type).
We’ve been trying to bury these deployments for almost two years now (see #11398), so I just want to see it done. If people prefer this approach, I’m happy to close 16060 in favour of this.
Rebased
This PR (or earlier versions) have concept ACKs from:
I’d really appreciate some code review to get this unblocked.
1203- }
1204- rv.pushKV("status", activated);
1205- return rv;
1206-}
1207+ // For buried deployments.
1208+ // Buried deployments with activation height value of std::numeric_limits<int>::max() are hidden.
1228 {
1229- UniValue rv(UniValue::VOBJ);
1230+ // For BIP9 deployments.
1231+ // Deployments (e.g. testdummy) with timeout value before Jan 1, 2009 are hidden.
1232+ // A timeout value of 0 guarantees a softfork will never be activated.
1233+ // This is used when softfork codes are merged without specifying the deployment schedule.
1344+ BuriedForkDescPushBack(softforks, "bip34", consensusParams.BIP34Height);
1345+ BuriedForkDescPushBack(softforks, "bip66", consensusParams.BIP66Height);
1346+ BuriedForkDescPushBack(softforks, "bip65", consensusParams.BIP65Height);
1347+ BIP9SoftForkDescPushBack(softforks, "csv", consensusParams, Consensus::DEPLOYMENT_CSV);
1348+ BIP9SoftForkDescPushBack(softforks, "segwit", consensusParams, Consensus::DEPLOYMENT_SEGWIT);
1349+ BIP9SoftForkDescPushBack(softforks, "testdummy", consensusParams, Consensus::DEPLOYMENT_TESTDUMMY);
1291- " \"threshold\": xx, (numeric) the number of blocks with the version bit set required to activate the feature \n"
1292- " \"elapsed\": xx, (numeric) the number of blocks elapsed since the beginning of the current period \n"
1293- " \"count\": xx, (numeric) the number of blocks with the version bit set in the current period \n"
1294- " \"possible\": xx (boolean) returns false if there are not enough blocks left in this period to pass activation threshold \n"
1295- " }\n"
1296+ " \"type\": \"xxxx\", (string) one of \"buried\", \"bip9\"\n"
Is the goal just to be able to indicate which entries will have a bip9 field and which ones won’t?
I think it’s useful for users to know how the softfork logic is implemented. We could remove this and it be implicit that the activation method is bip9 if there’s a bip9 object, but I think having the field makes the getblockchaininfo return object extensible if we have a different activation method for a future softfork.
We leak the details on how we implement the fork deployment anyway – at worst you could invalidateblock back to before the fork activated and query the fork state to see if it’s buried or bip9. I think fork activation is important/risky enough that it’s worthwhile explicitly revealing how it’s being determined.
If we were to do a bip-16-like deployment or a fixed-height-after-bip9-expiry UASF deployment as suggested in the Great Consensus Cleanup bip proposal, I think using the type field to differentiate between old, buried, fixed height deployments and future ones would make sense.
1229- UniValue rv(UniValue::VOBJ);
1230+ // For BIP9 deployments.
1231+ // Deployments (e.g. testdummy) with timeout value before Jan 1, 2009 are hidden.
1232+ // A timeout value of 0 guarantees a softfork will never be activated.
1233+ // This is used when softfork codes are merged without specifying the deployment schedule.
1234+ if (consensusParams.vDeployments[id].nTimeout <= 1230768000) return;
If we get rid of this filtering and the needless invocation of this function on “testdummy” I think things would be simpler.
Actually, it looks like you do this so that you can test the RPC in regtest even after everything else is buried? I guess code coverage of the RPC is a good reason to do it this way, if that’s what you have in mind.
1306+ " \"elapsed\": xx, (numeric) the number of blocks elapsed since the beginning of the current period \n"
1307+ " \"count\": xx, (numeric) the number of blocks with the version bit set in the current period \n"
1308+ " \"possible\": xx (boolean) returns false if there are not enough blocks left in this period to pass activation threshold \n"
1309+ " }\n"
1310+ " },\n"
1311+ " \"height\": \"xxxxxx\", (numeric) height of the first block which the rules are enforced (only for \"buried\" type, or \"bip9\" type with \"locked_in\" or \"active\" status)\n"
1307+ " \"count\": xx, (numeric) the number of blocks with the version bit set in the current period \n"
1308+ " \"possible\": xx (boolean) returns false if there are not enough blocks left in this period to pass activation threshold \n"
1309+ " }\n"
1310+ " },\n"
1311+ " \"height\": \"xxxxxx\", (numeric) height of the first block which the rules are enforced (only for \"buried\" type, or \"bip9\" type with \"locked_in\" or \"active\" status)\n"
1312+ " \"active\": xx, (boolean) true if the rules are enforced\n"
57@@ -59,6 +58,8 @@ struct Params {
58 int BIP66Height;
59 /** Block height at which CSV (BIP68, BIP112 and BIP113) becomes active */
60 int CSVHeight;
61+ /** Block height at which Segwit (BIP141, BIP143 and BIP147) becomes active */
1247- bip9_softforks.pushKV(VersionBitsDeploymentInfo[id].name, BIP9SoftForkDesc(consensusParams, id));
1248+ UniValue rv(UniValue::VOBJ);
1249+ rv.pushKV("type", "bip9");
1250+ rv.pushKV("bip9", bip9);
1251+ if (ThresholdState::LOCKED_IN == thresholdState) {
1252+ rv.pushKV("height", since_height + consensusParams.nMinerConfirmationWindow);
VersionBitsActivationHeight() function to validation.cpp
Concept ACK, left a bunch of comments but I need to go through the code more carefully still.
I went back and forth on the first commit, because I don’t like breaking interfaces, but overall I think it’s a big enough improvement that it’s worth doing the overhaul this PR proposes (though I did nitpick a few details! which is partly why I hope we don’t revisit this RPC output anytime soon after this, as there’s infinite room for bikeshedding).
1277- " \"status\": xx, (boolean) true if threshold reached\n"
1278- " },\n"
1279- " }, ...\n"
1280- " ],\n"
1281- " \"bip9_softforks\": { (object) status of BIP9 softforks in progress\n"
1282+ " \"softforks\": { (object) status of softforks\n"
I think some users may be interested in which height the node started enforcing rules. Even though that’s leaking some implementation detail (see #16060 (review)), I think it’s better to show more to the user than less.
As for adding a new RPC, that’s even more of a breaking change for clients. For me, it makes most sense to return all blockchain info to the client in a single object from getblockchaininfo.
As Suhas points out (https://github.com/bitcoin/bitcoin/pull/16060#pullrequestreview-264351017), there’s infinite room for bikeshedding here. Unless there’s a really compelling reason to add a separate RPC for softfork deployments, I’d prefer to keep this as it is.
1255- rv.pushKV("startTime", consensusParams.vDeployments[id].nStartTime);
1256- rv.pushKV("timeout", consensusParams.vDeployments[id].nTimeout);
1257- rv.pushKV("since", VersionBitsTipStateSinceHeight(consensusParams, id));
1258+ bip9.pushKV("startTime", consensusParams.vDeployments[id].nStartTime);
1259+ bip9.pushKV("timeout", consensusParams.vDeployments[id].nTimeout);
1260+ int64_t since_height = VersionBitsTipStateSinceHeight(consensusParams, id);
You’d have to hold cs_main from VersionBitsTipState to this line to protect against a race where a block comes in during this function.
Though, I’d prefer to hold cs_main until the whole json softforks object is constructed.
cs_main is held for the duration of getblockchaininfo(). That’s not changed by this PR.
Indeed, you are right.
style-nit: You could add EXCLUSIVE_LOCKS_REQUIRED to BIP9SoftForkDescPushBack and it should still compile.
EXCLUSIVE_LOCKS_REQUIRED(cs_main) to BuriedForkDescPushBack() and BIP9SoftForkDescPushBack().
116@@ -117,9 +117,7 @@ TestingSetup::~TestingSetup()
117
118 TestChain100Setup::TestChain100Setup() : TestingSetup(CBaseChainParams::REGTEST)
119 {
120- // CreateAndProcessBlock() does not support building SegWit blocks, so don't activate in these tests.
121- // TODO: fix the code to support SegWit blocks.
1336- UniValue bip9_softforks(UniValue::VOBJ);
1337- softforks.push_back(SoftForkDesc("bip34", 2, tip, consensusParams));
1338- softforks.push_back(SoftForkDesc("bip66", 3, tip, consensusParams));
1339- softforks.push_back(SoftForkDesc("bip65", 4, tip, consensusParams));
1340- for (int pos = Consensus::DEPLOYMENT_CSV; pos != Consensus::MAX_VERSION_BITS_DEPLOYMENTS; ++pos) {
1341- BIP9SoftForkDescPushBack(bip9_softforks, consensusParams, static_cast<Consensus::DeploymentPos>(pos));
Thanks for digging that out. I really don’t understand why that PR was merged. The author’s justification (https://github.com/bitcoin/bitcoin/pull/10874#issuecomment-321609838) was “I had only made the change since I needed to add stats about BIP 91 to one of my branches and the fact that the forks needed to be hard coded in to be displayed with getblockchaininfo bothered me.” It doesn’t make sense to me that lines of code were added to bitcoin core to support someone’s fork of the project.
When there are only three soft fork deployments, listing them out explicitly is less lines of code and seems clearer to me.
Just some style questions. Feel free to ignore/hide the comments.
I am +-0 on the overall change. So consider my “concept nack” in the previous pull withdrawn, since I don’t want to block this pull. I like that it is cleaning up technical debt a bit, but I also think it doesn’t improve user experience significantly, which is why I am +-0 on it.
347+void CRegTestParams::UpdateActivationParametersFromArgs(const ArgsManager& args)
348 {
349+ if (gArgs.IsArgSet("-segwitheight")) {
350+ int64_t height = gArgs.GetArg("-segwitheight", consensus.SegwitHeight);
351+ if (height < -1 || height >= std::numeric_limits<int>::max()) {
352+ throw std::runtime_error(strprintf("Activation height %ld for segwit is out of valid range. Use -1 to disable segwit.", height));
Correct, this isn’t called during validation, only during initialization:
0→ bitcoind -segwitheight=99999999999999999999999999
1Error: Activation height 9223372036854775807 for segwit is out of valid range. Use -1 to disable segwit.
std::logic_error when calling VersionBits(Tip)ActivationHeight() on a deployment that isn’t locked in or active
std::logic_error
4625@@ -4626,6 +4626,22 @@ int VersionBitsTipStateSinceHeight(const Consensus::Params& params, Consensus::D
4626 return VersionBitsStateSinceHeight(::ChainActive().Tip(), params, pos, versionbitscache);
4627 }
4628
4629+int64_t VersionBitsActivationHeight(const Consensus::Params& params, Consensus::DeploymentPos pos)
VersionBitsTipActivationHeight to match the other functions in validation.cpp (if there’s orphaned blocks that aren’t part of the most-work chain that has different signalling, those blocks can have a different activation height; that can occur with a single block reorg with sufficiently bad luck). Really the logic should be in versionbits.cpp as well and should use Period(params) instead of params.nMinerconfirmationWindow directly.
urgh. None of the BIP9 code will be used after this PR. I’ve now moved logic from rpc/blockchain.cpp into validation.cpp after @sdaftuar’s comment here: #16060 (review), then pulled it into versionbits.cpp because of your comment here #16060 (review).
I can add even more machinery to versionbits.cpp to avoid making Period() public, but I don’t understand where we’re going with this. Once again, this is code that is not expected to be run. This PR was supposed to be reducing the code complexity, but successive comments have asked for more and more code to be added to cover unexercised code paths.
4635+ case ThresholdState::LOCKED_IN:
4636+ return since_height + params.nMinerConfirmationWindow;
4637+ case ThresholdState::ACTIVE:
4638+ return since_height;
4639+ default:
4640+ throw std::logic_error("VersionBitsActivationHeight called for non-ACTIVE non-LOCKED-IN deployment");
-1 as “unknown activation height” rather than having a new failure path.
1631@@ -1632,7 +1632,8 @@ static unsigned int GetBlockScriptFlags(const CBlockIndex* pindex, const Consens
1632 flags |= SCRIPT_VERIFY_CHECKSEQUENCEVERIFY;
1633 }
1634
1635- if (IsNullDummyEnabled(pindex->pprev, consensusparams)) {
1636+ // Start enforcing BIP147 NULLDUMMY (activated simultaneously with segwit)
1637+ if (pindex->nHeight >= consensusparams.SegwitHeight) {
IsNullDummyenabled() with a hardcoded test, rather than IsWitnessEnabled().
481@@ -482,9 +482,8 @@ static UniValue getblocktemplate(const JSONRPCRequest& request)
482 // TODO: Maybe recheck connections/IBD and (if something wrong) send an expires-immediately template to stop miners?
483 }
484
485- const struct VBDeploymentInfo& segwit_info = VersionBitsDeploymentInfo[Consensus::DEPLOYMENT_SEGWIT];
486 // GBT must be called with 'segwit' set in the rules
487- if (setClientRules.count(segwit_info.name) != 1) {
488+ if (setClientRules.count("segwit") != 1) {
aRules based on versionbits state so whether or not “segwit” is passed as an arg, it ends up outputting an empty array {"rules": []} for the template. I think that conflicts with BIP145: “Transactions with witness data may only be included if the template’s “rules” list (see BIP 9) includes “segwit”.”
I think we’re not compliant with this part of BIP 145 anyway, specifically:
The ‘!’ rule prefix MUST be enabled on the “segwit” rule for templates including transactions with witness data.
There’s no code to insert the ‘!’ in templates with blocks containing segwit transactions. I’ve also tested on master that getblocktemplate doesn’t include !segwit as a rule for templates with witness data.
The fact that this has always been broken suggests to me that no-one is looking at the rules field on the return object.
I can add some code to update the rules based on whether csv and segwit are activated, but it doesn’t seem worth it.
I don’t think it makes much sense to require segwit in the params if we’re not going to bother reporting it ourselves. I think the additional code would just be:
0 if (pindex->nHeight+1 >= consensusparams.CSVHeight) aRules.push_back("csv");
1 if (!fPreSegWit) aRules.push_back("segwit");
(also, could have said fPreSegWit = !IsWitnessEnabled(pindexPrev, consensusParams);)
Looks like including the ! got broken prior to activation in March 2017 in #9955 with segwit’s gbt_force value changing from false to true, so bitcoind hasn’t been compliant with bip145 since 0.15, though 0.14 should have been.
I think the logic here isn’t right and we should have two settings in VBDeploymentInfo, gbt_simplified which just adds the ! when false, and gbt_optional which, when the client doesn’t specify the rule, avoids setting the versionbit while signalling if gbt_optional is true, and errors out when the rule is activated and gbt_optional is false. segwit should initially have had gbt_simplified=false and gbt_optional=true, and just changed gbt_optional=false in #14811.
There’s no provision in bip9 or bip145 for ever removing forks from the rules list, which might be worth improving, as an alternative. @luke-jr – thoughts? You added the gbt changes for bip9 and acked 9955 :)
ah, I see: https://github.com/bitcoin/bitcoin/pull/9955/files#diff-d6954440f0346f8e42dcd669dd4aeafaL20
‘force’ is defined as “Whether GBT clients can safely ignore this rule in simplified usage” which seems backwards to me.
I guess gbt_force is too unclear a parameter name…
We should fix this. Maybe in a separate PR.
I think I’m about -0.3 on this PR (say -0.8 on the approach, +0.5 on the implementation given the approach). So not an ACK, but not a NACK either. (Well, I think there’s a bug in the getblocktemplate changes as above below, but presumably that’ll be fixed if it is a bug)
The rationale for it is essentially a refactoring: “Hardcoding the activation height is a code simplification, makes it easier to understand segwit activation status, and reduces technical debt.” but most of this PR isn’t just refactoring:
It moves from a somewhat generic interface for soft-fork changes to per-fork code, making it harder to change the way soft-forks are checked in future, which seems like it increased technical debt to me – doing a generic interface for reporting soft-fork activation will require reverting all the explicit Height member variables and checks.
I think it also turns out to be a more invasive change than necessary, as evidenced by the number of rebases this PR has required, compared to the lack of rebases the alternative PR #16229 has needed. Honestly, I find that a bit surprising, unless the rebasing has been mostly due to the test case changes somehow?
If we want to change the way getblockchaininfo reports softforks (and I think we do), we should split that out as a separate PR (like #16328 proposed) and review those changes independently, rather than forcing them in as part of tidying up the codebase.
I don’t think the auxiliary changes for segwit are great: adding a special case -segwitheight arg to replace the generic -vbparams and dropping get_bip9_status entirely both seem like backwards steps. If we’re going to stop testing bip9 behaviour via csv or segwit activation in the functional tests, probably should add explicit tests for it using testdummy, which would then use get_bip9_status. Likewise, it seems like it would make more sense to have all soft forks always active in regtest, with exceptions for feature specific tests to ensure pre-activation behaviour is correct, rather than having that be segwit specific.
I think we should be doing what sipa suggested in the first PR: “I would prefer it if a buried deployment wouldn’t require all code paths that check the BIP9 status to require changing (and instead plug in the burying into the BIP9 status logic, so that it just always returns DEFINED before the bury height, and always ACTIVE after?).”
Thanks for the review @ajtowns
If we want to change the way getblockchaininfo reports softforks (and I think we do), we should split that out as a separate PR (like #16328 proposed) and review those changes independently, rather than forcing them in as part of tidying up the codebase.
This PR changes the way that softforks are reported, so that buried softforks are reported as such. As you point out, this is a breaking change to the getblockchaininfo API, but one that I think makes the reporting clearer (why does softforks[0]['reject']['status'] = true currently mean that bip34 is active?!) The new format is also extensible and allows alternative activation methods in future, if those are required.
That API change is good by itself, but doesn’t make much sense outside the context of burying the bip9 softforks. The later commits in this PR change the ‘activation method’ of those softforks from bip9 to buried, so is another change to how the softforks are reported in getblockchaininfo. I think it makes sense to look at the changes as a whole. That’s why these commits are contained in the same PR.
#16328 was originally opened as just the first commit from PR, but Marco then made significant changes to the code, meaning that this, #16328 and #16229 were three competing and incompatible PRs to implement overlapping changes. I think it makes sense to focus review on just one PR rather than spread it across many PRs (which probably results in none of them getting merged).
It includes substantial user-visible changes, “breaking” the getblockchaininfo API
Both #16328 and #16229 also break getblockchaininfo API. As explained above, I think it makes sense to break it once to clean up that interface and make it extensible for future soft forks.
It drops chunks of functional test coverage, in particular, after this PR, I don’t think we have functional tests checking BIP9 activation works anymore
Yes! That’s part of the point. We have test code that is entirely redundant and useless now that all bip9 forks are activated. If schnorr/taproot or future softforks use bip9, we can easily add tests for them.
… evidenced by the number of rebases this PR has required, compared to the lack of rebases the alternative PR #16229 has needed.
This PR has required many rebases because it (and its predecessors) have been open for almost two years. I haven’t checked how frequently #16229 has needed rebase, but a lot of the touched code seems comparable (RPC, validation). This PR contains quite a few changes to the functional tests which has contributed to the number of rebases.
I think we should be doing what sipa suggested in the first PR: “I would prefer it if a buried deployment wouldn’t require all code paths that check the BIP9 status to require changing (and instead plug in the burying into the BIP9 status logic, so that it just always returns DEFINED before the bury height, and always ACTIVE after?).”
Defining (and burying) soft forks should be a very infrequent activity. After this PR, the code changes required to bury a softfork should be minimal.
I’ll take a look at the inline code comments tomorrow.
height return value entirely from BIP9 forks which are locked_in. That avoids having to add a bunch of BIP9 machinery for very little benefit.
515@@ -516,6 +516,7 @@ void SetupServerArgs()
516 gArgs.AddArg("-maxtipage=<n>", strprintf("Maximum tip age in seconds to consider node in initial block download (default: %u)", DEFAULT_MAX_TIP_AGE), true, OptionsCategory::DEBUG_TEST);
517 gArgs.AddArg("-printpriority", strprintf("Log transaction fee per kB when mining blocks (default: %u)", DEFAULT_PRINTPRIORITY), true, OptionsCategory::DEBUG_TEST);
518 gArgs.AddArg("-printtoconsole", "Send trace/debug info to console (default: 1 when no -daemon. To disable logging to file, set -nodebuglogfile)", false, OptionsCategory::DEBUG_TEST);
519+ gArgs.AddArg("-segwitheight=<n>", "Set the activation height of segwit. -1 to disable. (regtest-only)", false, OptionsCategory::DEBUG_TEST);
Squashed the fixup commits and addressed @ajtowns comment here: #16060 (review)
Pre-squashed branch is here: https://github.com/jnewbery/bitcoin/tree/pr16060.1
This is ready for re-review
1677- // Note that setting NODE_WITNESS is never required: the only downside from not
1678- // doing so is that after activation, no upgraded nodes will fetch from you.
1679+ if (chainparams.GetConsensus().SegwitHeight != std::numeric_limits<int>::max()) {
1680+ // Advertise witness capabilities.
1681+ // The option to not set NODE_WITNESS is only used in the tests and should be removed.
1682 nLocalServices = ServiceFlags(nLocalServices | NODE_WITNESS);
Instead of adding a TODO in the code, why not replace the check in the tests assert(nServices&NODE_WITNESS) with assert_debug_log(['Segwit disabled for testing']) or something?
That way you could remove this whole hunk
I don’t think this works – p2p_segwit launches some nodes with -segwitheight=120 and one with segwit disabled, and expects them to follow the same blockchain. But if you set NODE_WITNESS then when the first node thinks segwit is enabled at block 120, it’ll send the witness data to the other node, which will then give an error message, “AcceptBlock FAILED (unexpected-witness, …” and fail the test.
The test case is essentially testing that upgrades work, though, which would be more useful if the non-segwit node was actually running old code, rather than just simulating it… I think the only changes needed to p2p_segwit for this are:
test_upgrade_after_activation()test_getblocktemplate_before_lockin() set node=self.nodes[0], drop the for loop, and drop the dead code for node == self.nodes[2].I don’t think this needs to be done in this PR though.
1205- return rv;
1206-}
1207+ // For buried deployments.
1208+ // A buried deployment is one where the height of the activation has been hardcoded into
1209+ // the client implementation long after the consensus change has activated. See BIP 90.
1210+ // Buried deployments with activation height value of std::numeric_limits<int>::max() are hidden.
Instead of explaining what the code does, you could explain why the code does?
0 // Buried deployments with activation height value of std::numeric_limits<int>::max() are disabled and thus hidden.
235@@ -267,7 +236,7 @@ def run_test(self):
236 self.send_blocks(test_blocks)
237
238 self.log.info("Not yet advanced to ACTIVE, height = 574 (will activate for block 576, not 575)")
You are setting the activation height to
0+ consensus.CSVHeight = 432; // CSV activated on regtest (Used in rpc activation tests)
but fail to update this comment.
Can you use the same constant in the params and this comment, please?
partial ACK aece4b20841f30afa1d4a988034d07fd660b19df
Signature:
0-----BEGIN PGP SIGNED MESSAGE-----
1Hash: SHA512
2
3ACK aece4b20841f30afa1d4a988034d07fd660b19df
4-----BEGIN PGP SIGNATURE-----
5
6iQGzBAEBCgAdFiEE+rVPoUahrI9sLGYTzit1aX5ppUgFAlwqrYAACgkQzit1aX5p
7pUj3XAwAutgRJEbg4jm5hZzCo6MigswK3XZbtbOlEdZWCMTDKtcopN+VU6fshU8j
8lWEzufNDXu/LYQyFfp2ljZPKPkLeDg+/WmLvyGsoaygMSxb8JHYbJUv9nwiLNkBG
9J1kRtgfMUombAL/TyZEeeDyD8i7CRFAy7ebi7k3HRntISqJ4gF/2qcUT3PHGjV9f
1089srx9ju9d/f6CAh7V8WHBJWqhEHWNzxsHg9PrG/kmj5h6JHWjpgJFArdBZYiznF
11XJAnR/MPePt9O8JTTcaxlkG8uuDuNyOYiQj+SQjc11e1ZRiBatBugZ0v36Xrk7qy
12wQQHFP0uWz7RbJFao1TPiRkJC2PUiDVVj1xJHkZTBBxS840AAFZ8QjiY2Zb06yIb
132GQK25KGiF4RVkSxIaXS/Knaq7iVZQev3wwYwcCEKkD4/9r4JQYsmQBTHN5epmsW
14ov2cF37OAsCswjorn2bWJfjmZ5DzBeSi52RVIfbl3EQogJvEfFUXb/J7Z+X9olkU
15qA5NhpYa
16=qhoe
17-----END PGP SIGNATURE-----
Timestamp of file with hash 158b950bfd063584b8e84c3c60eb8c5aedcfdf52005b2c7784a8cb0cef5d418b -
384@@ -385,9 +385,6 @@ bool TestBlockValidity(CValidationState& state, const CChainParams& chainparams,
385 /** Check whether witness commitments are required for block. */
386 bool IsWitnessEnabled(const CBlockIndex* pindexPrev, const Consensus::Params& params);
387
388-/** Check whether NULLDUMMY (BIP 147) has activated. */
sigh Probably should update the comment for IsWitnessEnabled above…
Might be good to note that segwit is enforced for all transactions, and this function is only used for nulldummy and block-level things (ie, working out if you’ve upgraded and need to reprocess segwit blocks that you didn’t download witness data for, if you should generate witness data for getblocktemplate, and so you can accept old blocks from non-witness peers)
1249- if (consensusParams.vDeployments[id].nTimeout > 0)
1250- bip9_softforks.pushKV(VersionBitsDeploymentInfo[id].name, BIP9SoftForkDesc(consensusParams, id));
1251+ UniValue rv(UniValue::VOBJ);
1252+ rv.pushKV("type", "bip9");
1253+ rv.pushKV("bip9", bip9);
1254+ if (ThresholdState::ACTIVE == thresholdState) {
68@@ -69,14 +69,11 @@ def skip_test_if_missing_module(self):
69 self.skip_if_no_wallet()
70
71 def test_cltv_info(self, *, is_active):
72- assert_equal(
73- next(s for s in self.nodes[0].getblockchaininfo()['softforks'] if s['id'] == 'bip65'),
74+ assert_equal(self.nodes[0].getblockchaininfo()['softforks']['bip66'],
Good catch; good example of why not to use ‘bipXXX’ as identifiers imo. Also the first check should be something like:
0- self.test_cltv_info(is_active=True)
1+ self.test_cltv_info(is_active=False) # Not active as of current tip and next block does not need to obey rules
Great catch. This was careless on my part (copy-pasting from feature_dersig.py to feature_cltv.py).
I’ve fixed this and added the comment as suggested by AJ.
3186@@ -3187,7 +3187,7 @@ static bool ContextualCheckBlock(const CBlock& block, CValidationState& state, c
3187
3188 // Start enforcing BIP113 (Median Time Past) using versionbits logic.
340@@ -341,7 +341,7 @@ def test_nonzero_locks(orig_tx, node, relayfee, use_height_lock):
341 # being run, then it's possible the test has activated the soft fork, and
342 # this test should be moved to run earlier, or deleted.
343 def test_bip68_not_consensus(self):
187@@ -187,45 +188,14 @@ def run_test(self):
188 self.tip = int(self.nodes[0].getbestblockhash(), 16)
189 self.nodeaddress = self.nodes[0].getnewaddress()
190
191- self.log.info("Test that the csv softfork is DEFINED")
192- assert_equal(get_bip9_status(self.nodes[0], 'csv')['status'], 'defined')
193- test_blocks = self.generate_blocks(61, 4)
194+ # Activation height is hardcoded
51@@ -52,14 +52,11 @@ def skip_test_if_missing_module(self):
52 self.skip_if_no_wallet()
53
54 def test_dersig_info(self, *, is_active):
55- assert_equal(
56- next(s for s in self.nodes[0].getblockchaininfo()['softforks'] if s['id'] == 'bip66'),
57+ assert_equal(self.nodes[0].getblockchaininfo()['softforks']['bip66'],
266@@ -275,6 +267,7 @@ class CRegTestParams : public CChainParams {
267 consensus.BIP34Hash = uint256();
268 consensus.BIP65Height = 1351; // BIP65 activated on regtest (Used in functional tests)
269 consensus.BIP66Height = 1251; // BIP66 activated on regtest (Used in functional tests)
270+ consensus.CSVHeight = 432; // CSV activated on regtest (Used in rpc activation tests)
3052-
3053-bool IsNullDummyEnabled(const CBlockIndex* pindexPrev, const Consensus::Params& params)
3054-{
3055- LOCK(cs_main);
3056- return (VersionBitsState(pindexPrev, params, Consensus::DEPLOYMENT_SEGWIT, versionbitscache) == ThresholdState::ACTIVE);
3057+ int height = pindexPrev == nullptr ? 0 : pindexPrev->nHeight + 1;
pindexPrev for the underlying GetStateFor, we may just pass pindex, assert it and return equality, a quick skim it may simplify callers, add a TODO?
UpdateUncommittedBlockStructures or BlockAssembler::CreateNewBlock, so you’d still need this function to be available.
pindexPrev->nHeight + 1 to IsWitnessEnabled, should work but need to refactor also callers ?
Right, I think this could also be refactored to take a height parameter and just return whether segwit should be enforced for a block at that height.
We can leave that for a follow-up PR if people think it’s worthwhile.
Reviewed c64c9b5
Mostly grep’ed versionbits refs, which should be updated IMO.
Tested getblockchaininfo:
With this PR, bip 9 logic isn’t tested anymore, is there an easy way to write a dummy test for it without polluting code paths ? Don’t think so..
(also add comments for versionbits.h in https://github.com/ariard/bitcoin/commit/fca79982227978c4b5ecbb7489754e54fb179696, obviously no one wants to weigh this already wide-scoped PR but it may be relevant on its own)
ACK 20e60006de5015e057121b74b623698e96c0bfae ; code review, compiles, tests pass, reports same activation heights as master for segwit/csv on testnet/mainnet. I’ve not thoroughly reviewed the changes in the python tests.
Nits:
0x7fffffffffffffff(or even 2**63-1)I think release notes are warranted for the change to getblockchaininfo and probably getblocktemplate. I’m not sure the change to getblocktemplate (no longer returning “segwit” in the “rules” field of the result) are okay, but Luke didn’t complain, so hopefully it’s fine. Both those seem fine to worry about later.
I tried drafting up a bip9 functional test making use of testdummy on regtest, but I’m not convinced it makes sense to do that directly rather than improving our bip9 implementation or improving bip9 first. Either way, that can be left to a later PR.
This combines reporting of buried (formally ISM) softfork deployments
and BIP9 versionbits softfork deployments into one JSON object in the
getblockchaininfo return object.
Thanks @ajtowns . I’ve force-pushed a fix to those nits and also release notes.
I also intend to send a post to the mailing list if this gets merged (I don’t think a full BIP is necessary).
I tried drafting up a bip9 functional test making use of testdummy on regtest, but I’m not convinced it makes sense to do that directly rather than improving our bip9 implementation or improving bip9 first. Either way, that can be left to a later PR.
Very happy to review any tests or changes to BIP 9 you PR.
(I don’t think a full BIP is necessary)
I agree; IMO that the heights are already included in https://github.com/bitcoin/bips/blob/master/bip-0009/assignments.mediawiki means the BIP side of things is already covered.
1643@@ -1644,8 +1644,8 @@ static unsigned int GetBlockScriptFlags(const CBlockIndex* pindex, const Consens
1644 flags |= SCRIPT_VERIFY_CHECKLOCKTIMEVERIFY;
1645 }
1646
1647- // Start enforcing BIP68 (sequence locks) and BIP112 (CHECKSEQUENCEVERIFY) using versionbits logic.
1648- if (VersionBitsState(pindex->pprev, consensusparams, Consensus::DEPLOYMENT_CSV, versionbitscache) == ThresholdState::ACTIVE) {
1649+ // Start enforcing BIP68 (sequence locks) and BIP112 (CHECKSEQUENCEVERIFY)
LOCKTIME_VERIFY_SEQUENCE beneath (even if these 2 BIPs are strongly tied, their enforcement is done by different components)
303@@ -340,10 +304,10 @@ def run_test(self):
304 self.send_blocks([self.create_test_block(success_txs)])
305 self.nodes[0].invalidateblock(self.nodes[0].getbestblockhash())
306
307- # 1 more version 4 block to get us to height 575 so the fork should now be active for the next block
308- test_blocks = self.generate_blocks(1, 4)
309+ # 1 more version 4 block to get us to height 432 so the fork should now be active for the next block
Hard code CSV deployment height to 419328 for mainnet.
Hardcode segwit deployment height to 481824 for mainnet.
ACK e78aaf41f43d0e2ad78fa6d8dad61032c8ef73d0 (still didn’t check if the mainnet block heights are correct, but the code looks good now)
Signature:
0-----BEGIN PGP SIGNED MESSAGE-----
1Hash: SHA512
2
3ACK e78aaf41f43d0e2ad78fa6d8dad61032c8ef73d0 (still didn't check if the mainnet block heights are correct, but the code looks good now)
4-----BEGIN PGP SIGNATURE-----
5
6iQGyBAEBCgAdFiEE+rVPoUahrI9sLGYTzit1aX5ppUgFAlwqrYAACgkQzit1aX5p
7pUhaqwv42/MfP5paFChypsDXr5M3MZtfZyD36YtC9JCjdtdBff2zKLKONh1PvrYj
8d7MuAJ6fgsaS7qE4GIbCo77EDzJziqTsgoruoWckqzLrHgr2Bt4gaxEpciQhUrVD
9sR4GjnWuVPkawnIPkr2+hJkScU2/fFGVnVzF1Hg0A7mIzkr9xdt1ofJvb9GQ4O2Y
10fteE78HpUAgwvmoudddIsRDKGui/0tTClzhx8D+1cVz7MtSU7+2bmpcvdhn++vHl
11q6apUyGTzOFZtC4cJeV/1cv7Jka3M59IC/U05zZU6CD/ocDM3xrXY3z+tZCJEUIJ
12MdwWcqBuaK01ytvEr9ZmQ4cutLZFIkVPkz7FgayVU7XZ1X9666TGA1vnY/k1oh7e
13vl5+QZl+2bgleQRLot1vGRM5WXzHyksQUGT/XFTmumYmBY36zu4+pt4cUtLabnhW
14G3S2joUyMM9JonKCgYGcyPx3XUvLan5E5pZp092RXGoRB2f2T3ZwwDqwhIfmDFhj
151HTWBpo=
16=HDUJ
17-----END PGP SIGNATURE-----
Timestamp of file with hash f62d1be3493fe779f9feb2f59b46db1f25e6e089a7d58db2e083156c727bb44a -