Bury bip9 deployments #16060

pull jnewbery wants to merge 5 commits into bitcoin:master from jnewbery:bury_bip9_deployments changing 21 files +242 −306
  1. jnewbery commented at 8:38 pm on May 20, 2019: member

    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.

  2. jnewbery commented at 8:40 pm on May 20, 2019: member
    @jl2012 originally attempted this in #11398, which had concept ACKs from @gmaxwell @dcousens and @sdaftuar . I then tried again in #12360, which got ACKs from @jamesob and @jtimon .
  3. practicalswift commented at 8:47 pm on May 20, 2019: contributor
    Concept ACK
  4. DrahtBot added the label Consensus on May 20, 2019
  5. DrahtBot added the label Mining on May 20, 2019
  6. DrahtBot added the label RPC/REST/ZMQ on May 20, 2019
  7. DrahtBot added the label Tests on May 20, 2019
  8. DrahtBot added the label Validation on May 20, 2019
  9. sipa commented at 9:35 pm on May 20, 2019: member
    Concept ACK
  10. DrahtBot commented at 1:21 am on May 21, 2019: member

    The following sections might be updated with supplementary metadata relevant to reviewers and maintainers.

    Conflicts

    Reviewers, this pull request conflicts with the following ones:

    • #8994 (Testchains: Introduce custom chain whose constructor… by jtimon)

    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.

  11. jnewbery force-pushed on May 22, 2019
  12. jnewbery commented at 1:28 pm on May 22, 2019: member

    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.

  13. laanwj commented at 10:18 am on May 29, 2019: member
    Concept ACK
  14. in src/chainparams.cpp:352 in 152af4db7f outdated
    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) {
    


    PastaPastaPasta commented at 3:12 am on May 31, 2019:
    based on https://github.com/bitcoin/bitcoin/blob/master/doc/developer-notes.md#coding-style-c it appears this line should be on the line above with the bracket. } else if (height == -1) {

    jnewbery commented at 9:49 am on June 5, 2019:
    fixed
  15. in src/rpc/blockchain.cpp:1282 in 152af4db7f outdated
    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) {
    


    PastaPastaPasta commented at 3:13 am on May 31, 2019:
    same as above based on https://github.com/bitcoin/bitcoin/blob/master/doc/developer-notes.md#coding-style-c it appears this line should be on the line above with the bracket.

    jnewbery commented at 9:49 am on June 5, 2019:
    fixed
  16. PastaPastaPasta changes_requested
  17. jnewbery force-pushed on Jun 5, 2019
  18. jnewbery commented at 9:50 am on June 5, 2019: member
    Thanks for the review @PastaPastaPasta . I’ve fixed your two comments.
  19. in src/chainparams.cpp:73 in f395b23aa0 outdated
    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
    


    laanwj commented at 11:41 am on June 5, 2019:

    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  },
    

    laanwj commented at 12:00 pm on June 5, 2019:
    It’s slightly inconsistent how all of these (BIP34, BIP65, BIP66) are BIPxx and the new ones are written out as name. I don’t personally mind, to be honest, I even think this is clearer than having numbers.

    jnewbery commented at 1:54 pm on July 2, 2019:
    Neither the CSV nor segwit softforks were specified by a single BIP. CSV was BIPs 68, 112, and 113 and segwit was BIPs 141, 143, and 147. We could go back and give BIP34, BIP65 and BIP66 ‘descriptive’ names, but I think that’s an unnecessary breaking change.
  20. in src/rpc/blockchain.cpp:1273 in f395b23aa0 outdated
    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"
    


    laanwj commented at 12:09 pm on June 5, 2019:
    I like making this a object indexed by name, makes it easier to look one up in languages that treat them as dictionaries.

    promag commented at 1:49 pm on June 30, 2019:
    Agree, but this is a breaking change, seems unnecessary.

    jnewbery commented at 2:21 pm on July 2, 2019:

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


    promag commented at 2:28 pm on July 2, 2019:
    We are on the same page then, but for the sake of complaints, just add a small release note? @MarcoFalke also asked it above.
  21. in src/validation.cpp:3166 in f395b23aa0 outdated
    3163+    return (height >= params.SegwitHeight);
    3164 }
    3165 
    3166 bool IsNullDummyEnabled(const CBlockIndex* pindexPrev, const Consensus::Params& params)
    3167 {
    3168     LOCK(cs_main);
    


    merehap commented at 5:19 pm on June 5, 2019:
    Can this LOCK(cs_main) be removed just like the one in IsWitnessEnabled was? If it shouldn’t be, the reason would be unintuitive, so could you leave a comment why it is still needed/desirable in that case?

    jnewbery commented at 2:18 pm on July 2, 2019:
    Yes, it can. In fact, this function is only called from within 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().
  22. in src/chainparams.cpp:338 in 1949ee1f93 outdated
    344 };
    345 
    346-void CRegTestParams::UpdateVersionBitsParametersFromArgs(const ArgsManager& args)
    347+void CRegTestParams::UpdateActivationParametersFromArgs(const ArgsManager& args)
    348 {
    349+    if (gArgs.IsArgSet("-segwitheight")) {
    


    jtimon commented at 10:47 am on June 13, 2019:
    It would be nicer to do something more generic that works for other deployments, not just segwit. For example, if anybody was relying on vparams to test csv activation, it won’t be possible after this PR. But perhaps this is out of the scope for this PR, With something like #8994 we could simply make the all the buried heights configurable for custom regtests.

    jtimon commented at 10:53 am on June 13, 2019:
    Perhaps we can call the argument “-con_segwitheight” like in #8994 ?

    jnewbery commented at 2:22 pm on July 2, 2019:
    This argument is just for testing, so it’s not disruptive to change this in future if we think the -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.
  23. jtimon commented at 10:54 am on June 13, 2019: contributor
    I didn’t check the actual activation heights, but beyond that and my small nit, utACK f395b23aa0d125aa9534ff05df8d7ccd2d576868
  24. laanwj added this to the "Blockers" column in a project

  25. meshcollider commented at 7:12 pm on June 13, 2019: contributor
    Concept ACK
  26. moneyball commented at 7:04 pm on June 14, 2019: contributor
    I’ve looked through the history of the 3 PRs trying to get this change merged, and there is widespread support for it, but we need to get it to the finish line! Now that it has been added to the high priority list, would one or two of the following folks be able to review this in the next week or so? @sdaftuar @TheBlueMatt @sipa @jl2012 🙏
  27. instagibbs commented at 1:57 pm on June 18, 2019: member
    just noting there is a competing proposal at #16229
  28. laanwj commented at 1:06 pm on July 2, 2019: member

    just noting there is a competing proposal at #16229

    Why are there competing proposals? Is there a comparison sometimes of the pros/cons of either approach?

  29. jnewbery force-pushed on Jul 2, 2019
  30. jnewbery added the label Needs release note on Jul 2, 2019
  31. jnewbery commented at 2:51 pm on July 2, 2019: member
    This needs release notes if it reaches a stage where it’s ready for merge.
  32. ajtowns commented at 3:08 pm on July 2, 2019: member

    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.

  33. jnewbery commented at 3:15 pm on July 2, 2019: member

    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.

  34. jonasschnelli commented at 3:52 pm on July 16, 2019: contributor
    Concept ACK
  35. DrahtBot added the label Needs rebase on Jul 17, 2019
  36. jnewbery force-pushed on Jul 19, 2019
  37. jnewbery commented at 2:29 pm on July 19, 2019: member

    Rebased

    This PR (or earlier versions) have concept ACKs from:

    I’d really appreciate some code review to get this unblocked.

  38. DrahtBot removed the label Needs rebase on Jul 19, 2019
  39. in src/rpc/blockchain.cpp:1191 in f6e1b6153f outdated
    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.
    


    sdaftuar commented at 5:54 pm on July 19, 2019:
    Should some explanation of buried deployments be given in the code, for future code readers? Not sure if we have docs we can point to in our code somewhere already, but BIP 90 has some information if not.

    jnewbery commented at 9:00 pm on July 23, 2019:
    Added a short description and a reference to BIP 90.
  40. in src/rpc/blockchain.cpp:1206 in f6e1b6153f outdated
    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.
    


    sdaftuar commented at 6:50 pm on July 19, 2019:
    “softfork codes” seems like an odd phrase. Perhaps something like “This is used when merging logic to implement a proposed softfork without a specified deployment schedule”?

    jnewbery commented at 8:36 pm on July 23, 2019:
    Thanks. Changed to use your wording.
  41. in src/rpc/blockchain.cpp:1342 in f6e1b6153f outdated
    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);
    


    sdaftuar commented at 7:06 pm on July 19, 2019:
    Shouldn’t we just remove this line? This seems unnecessary…

    jnewbery commented at 1:07 pm on July 24, 2019:
    I think it’s ok to leave this for testing BIP9 logic.
  42. in src/rpc/blockchain.cpp:1275 in f6e1b6153f outdated
    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"
    


    sdaftuar commented at 7:12 pm on July 19, 2019:
    I think this concept of “type” is slightly weird – we’re leaking an implementation detail that shouldn’t have any bearing on how this information is used. It seems strange to me that we’d distinguish between long-since-activated bip9 softforks, and buried softforks, when reporting to users. Is the goal just to be able to indicate which entries will have a bip9 field and which ones won’t?

    jnewbery commented at 8:53 pm on July 23, 2019:

    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.


    ajtowns commented at 11:53 pm on July 24, 2019:

    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.

  43. in src/rpc/blockchain.cpp:1210 in f6e1b6153f outdated
    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;
    


    sdaftuar commented at 7:26 pm on July 19, 2019:

    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.


    jnewbery commented at 1:07 pm on July 24, 2019:
    Leaving this in so we can continue to test BIP 9 logic.
  44. in src/rpc/blockchain.cpp:1289 in f6e1b6153f outdated
    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"
    


    sdaftuar commented at 7:32 pm on July 19, 2019:
    “are or will be enforced”? That seems like more accurate phrasing if you’re going to do this for LOCKED_IN rules.

    jnewbery commented at 1:05 pm on July 24, 2019:
    Fixed
  45. in src/rpc/blockchain.cpp:1290 in f6e1b6153f outdated
    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"
    


    sdaftuar commented at 7:33 pm on July 19, 2019:
    Maybe “true if the rules are enforced in the block that is the current chain tip” to be more explicit about how to think about this for buried deployments?

    jnewbery commented at 1:06 pm on July 24, 2019:
    fixed
  46. in src/consensus/params.h:61 in b1cab3acaf outdated
    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 */
    


    sdaftuar commented at 7:39 pm on July 19, 2019:
    I wonder if it’s worth adding a comment here to explain that segwit v0 script rules are enforced on all blocks, other than the BIP 16 exception?
  47. jnewbery force-pushed on Jul 19, 2019
  48. in src/rpc/blockchain.cpp:1242 in 87860d59eb outdated
    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);
    


    sdaftuar commented at 7:49 pm on July 19, 2019:
    I don’t love that we’re leaking bip9 implementation calculations into the rpc code. Though it is useful to see when a LOCKED_IN softfork will actually activate. Can we invoke a method in the bip9 machinery instead?

    jnewbery commented at 1:03 pm on July 24, 2019:
    Done. Added a VersionBitsActivationHeight() function to validation.cpp
  49. sdaftuar commented at 7:52 pm on July 19, 2019: member

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

  50. in src/rpc/blockchain.cpp:1273 in 01463a8055 outdated
    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"
    


    MarcoFalke commented at 8:14 pm on July 19, 2019:
    Since this interface change is a breaking change, I’d like to split this object up into a new rpc. When calling getblockchaininfo you are not really interested in the state of all softforks that ever happened, especially ones that have activated a long time ago.

    jnewbery commented at 9:00 pm on July 24, 2019:

    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.

  51. in src/rpc/blockchain.cpp:1227 in 01463a8055 outdated
    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);
    


    MarcoFalke commented at 8:16 pm on July 19, 2019:

    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.


    jnewbery commented at 8:35 pm on July 24, 2019:
    cs_main is held for the duration of getblockchaininfo(). That’s not changed by this PR.

    MarcoFalke commented at 9:25 pm on July 24, 2019:

    Indeed, you are right.

    style-nit: You could add EXCLUSIVE_LOCKS_REQUIRED to BIP9SoftForkDescPushBack and it should still compile.


    jnewbery commented at 9:56 pm on July 24, 2019:
    Done. I’ve added EXCLUSIVE_LOCKS_REQUIRED(cs_main) to BuriedForkDescPushBack() and BIP9SoftForkDescPushBack().
  52. in src/test/setup_common.cpp:124 in 01463a8055 outdated
    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.
    


    MarcoFalke commented at 8:18 pm on July 19, 2019:
    Why remove the TODO?

    jnewbery commented at 8:51 pm on July 24, 2019:
    oops. Bad rebase. I’ve readded the message.
  53. in src/rpc/blockchain.cpp:1351 in 87860d59eb outdated
    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));
    


    MarcoFalke commented at 9:53 pm on July 19, 2019:
    This has been changed in " [RPC] getblockchaininfo: Loop through the bip9 soft fork deployments instead of hard coding #10874 " and now you are changing it back again. I’d prefer to just keep one version and stick with it, especially if there is no reason to switch.

    jnewbery commented at 8:40 pm on July 24, 2019:

    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.


    jnewbery commented at 8:58 pm on July 24, 2019:
    (also, at the end of this PR, there’s only one BIP9 softfork left (TESTDUMMY), so having a for loop really doesn’t make sense)
  54. jnewbery force-pushed on Jul 24, 2019
  55. jnewbery commented at 1:27 pm on July 24, 2019: member
    I think I’ve addressed all of @sdaftuar’s comments.
  56. MarcoFalke commented at 3:48 pm on July 24, 2019: member

    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.

  57. jnewbery force-pushed on Jul 24, 2019
  58. jnewbery commented at 9:00 pm on July 24, 2019: member
    Thanks for the review @MarcoFalke . I believe I’ve addressed all your comments.
  59. jnewbery force-pushed on Jul 24, 2019
  60. in src/chainparams.cpp:341 in fb5cbf3b6a outdated
    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));
    


    TheBlueMatt commented at 5:53 pm on July 25, 2019:
    Oof, please do not add new exception throws, especially not in validation. Either assert false or return 0.

    laanwj commented at 10:04 am on July 30, 2019:
    As I understand this is not called during validation, but during argument parsing at start. This follows the example of the other parsing failures later in this function.

    jnewbery commented at 9:51 pm on July 30, 2019:

    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.
    

    ajtowns commented at 4:51 am on July 31, 2019:
    The throw that occurs via validation.cpp is the std::logic_error when calling VersionBits(Tip)ActivationHeight() on a deployment that isn’t locked in or active

    jnewbery commented at 3:18 pm on July 31, 2019:
    I’ve now removed that std::logic_error
  61. jnewbery force-pushed on Jul 25, 2019
  62. jnewbery commented at 7:07 pm on July 25, 2019: member
    Oops. I dropped a change to the help text (here: #16060 (review)). Force pushed to get those changes.
  63. in src/validation.cpp:4629 in 05fb7e4a31 outdated
    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)
    


    ajtowns commented at 1:06 am on July 29, 2019:
    This should be 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.

    jnewbery commented at 9:48 pm on July 30, 2019:
    done

    ajtowns commented at 4:50 am on July 31, 2019:
    Making Period() a public member is cheating :)

    jnewbery commented at 3:13 pm on July 31, 2019:

    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.


    ajtowns commented at 3:33 pm on July 31, 2019:
    Sorry, that was just meant as a tease, not a criticism or a suggestion to change it. The ThresholdConditionChecker API isn’t great.
  64. in src/validation.cpp:4640 in 05fb7e4a31 outdated
    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");
    


    ajtowns commented at 1:20 am on July 29, 2019:
    Seems simpler to me to return -1 as “unknown activation height” rather than having a new failure path.

    jnewbery commented at 9:48 pm on July 30, 2019:
    done

    ajtowns commented at 4:49 am on July 31, 2019:
    versionbits.cpp::VersionBitsActivationHeight() still has the throw rather than returning -1 as of 41fd9d327f544d0e2f1f419a9ca70282ef5d30d2 though there’s a test to see if it returns -1 in rpc/blockchain.cpp

    jnewbery commented at 3:18 pm on July 31, 2019:
    fixed
  65. in src/validation.cpp:1636 in fa5b4afc64 outdated
    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) {
    


    ajtowns commented at 1:46 am on July 29, 2019:
    Seems odd to replace IsNullDummyenabled() with a hardcoded test, rather than IsWitnessEnabled().

    jnewbery commented at 9:48 pm on July 30, 2019:
    done
  66. in src/rpc/mining.cpp:486 in fa5b4afc64 outdated
    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) {
    


    ajtowns commented at 3:19 am on July 29, 2019:
    I don’t think this is sufficient – getblocktemplate only populates 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”.”

    jnewbery commented at 9:38 pm on July 30, 2019:

    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.


    ajtowns commented at 4:38 am on July 31, 2019:

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


    jnewbery commented at 3:29 pm on July 31, 2019:
    I can’t see how #9955 changed including the !. Am I missing something?

    jnewbery commented at 4:00 pm on July 31, 2019:

    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.


    luke-jr commented at 4:44 pm on July 31, 2019:

    I guess gbt_force is too unclear a parameter name…

    We should fix this. Maybe in a separate PR.

  67. ajtowns commented at 3:42 am on July 29, 2019: member

    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 includes substantial user-visible changes, “breaking” the getblockchaininfo API
    • 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

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

  68. jnewbery commented at 10:04 pm on July 29, 2019: member

    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.

  69. TheBlueMatt commented at 10:30 pm on July 29, 2019: member
    Concept ACK, though I have zero opinion on this vs #16229. If we want something fancier, lets do it, otherwise this is fine.
  70. fanquake removed the label Mining on Jul 30, 2019
  71. fanquake removed the label Tests on Jul 30, 2019
  72. jnewbery commented at 9:52 pm on July 30, 2019: member
    Pushed a commit to address @ajtowns feedback. I’ll squash it into the appropriate commits if people are happy with it.
  73. jnewbery force-pushed on Jul 31, 2019
  74. jnewbery commented at 3:26 pm on July 31, 2019: member
    Added another commit that removes the 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.
  75. jnewbery commented at 7:48 pm on July 31, 2019: member
    I think all review comments are addressed. @sdaftuar @TheBlueMatt : can you re-review with the fixup commits? If you’re happy, I’ll squash.
  76. in src/init.cpp:519 in 88a0a15bf0 outdated
    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);
    


    ajtowns commented at 7:42 am on August 1, 2019:
    This probably should be in chainparamsbase.cpp? At least that’s where the help for -vbparams is.

    jnewbery commented at 7:07 pm on August 1, 2019:
    moved
  77. jnewbery force-pushed on Aug 1, 2019
  78. jnewbery commented at 7:08 pm on August 1, 2019: member

    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

  79. DrahtBot added the label Needs rebase on Aug 2, 2019
  80. jnewbery commented at 5:33 pm on August 2, 2019: member
    rebased
  81. jnewbery force-pushed on Aug 2, 2019
  82. DrahtBot removed the label Needs rebase on Aug 2, 2019
  83. in src/init.cpp:1676 in 9dc5ade384 outdated
    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);
    


    MarcoFalke commented at 6:43 pm on August 2, 2019:

    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


    ajtowns commented at 3:30 pm on August 5, 2019:

    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:

    • drop test_upgrade_after_activation()
    • in 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.

  84. in src/rpc/blockchain.cpp:1191 in 9dc5ade384 outdated
    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.
    


    MarcoFalke commented at 6:44 pm on August 2, 2019:

    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.
    

    jnewbery commented at 7:25 pm on August 6, 2019:
    I’ve taken your suggestion. Thanks
  85. in test/functional/feature_csv_activation.py:238 in 9dc5ade384 outdated
    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)")
    


    MarcoFalke commented at 6:59 pm on August 2, 2019:

    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?


    jnewbery commented at 7:25 pm on August 6, 2019:
    done, and fixed a bunch of other comments in the test.
  86. MarcoFalke commented at 7:02 pm on August 2, 2019: member

    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 -

  87. in src/validation.h:389 in 5aa9124b83 outdated
    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. */
    


    ajtowns commented at 3:37 pm on August 5, 2019:

    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)


    jnewbery commented at 7:25 pm on August 6, 2019:
    comment updated
  88. jnewbery force-pushed on Aug 6, 2019
  89. jnewbery commented at 7:25 pm on August 6, 2019: member
    Updated to address @MarcoFalke and @ajtowns comments. Thanks for the review!
  90. in src/rpc/blockchain.cpp:1244 in 3739be48f8 outdated
    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) {
    


    ariard commented at 10:18 pm on August 6, 2019:
    nit: LOCKED_IN could also display a “activate_at” value based on the height of the beginning of the period + size of period

    jnewbery commented at 6:49 pm on August 8, 2019:
    This was in the original version of the PR, but Suhas didn’t like that it leaked details of the BIP9 implementation into the RPC code (https://github.com/bitcoin/bitcoin/pull/16060#discussion_r305502345). I tried adding additional BIP9 code to expose just the activation height (https://github.com/bitcoin/bitcoin/pull/16060#issuecomment-516898719), but it didn’t seem worth it so I ended up removing that (https://github.com/bitcoin/bitcoin/pull/16060#discussion_r308028983).

    laanwj commented at 6:55 pm on August 8, 2019:
    imo: let’s try not to circle around too much here, and get the consensus burying part merged, the RPC part can be improved in later PRs
  91. in test/functional/feature_cltv.py:72 in 3739be48f8 outdated
    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'],
    


    ariard commented at 10:50 pm on August 6, 2019:
    Isn’t this a cltv test ? Buried height of BIP66 being inferior at BIP65, strict der-sig will always be activated before cltv, and height digits are almost the same, maybe use a CLTV_HEIGHT alias.

    ajtowns commented at 4:54 am on August 7, 2019:

    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
    

    jnewbery commented at 7:18 pm on August 8, 2019:

    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.

  92. in src/validation.cpp:3188 in 7e33aea89f outdated
    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.
    


    ariard commented at 11:14 pm on August 6, 2019:
    Comment should be updated to drop versionbits ref

    jnewbery commented at 7:18 pm on August 8, 2019:
    fixed
  93. in test/functional/feature_bip68_sequence.py:343 in 7e33aea89f outdated
    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):
    


    ariard commented at 11:29 pm on August 6, 2019:
    Maybe the 2 aforementioned versionbits references should be updated, while top test comment with something similar to cltv : “Test that CSV soft-fork activates at (regtest) block height 432”.

    jnewbery commented at 7:28 pm on August 8, 2019:
    fixed
  94. in test/functional/feature_csv_activation.py:184 in 7e33aea89f outdated
    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
    


    ariard commented at 11:35 pm on August 6, 2019:
    nit: top comment should be updated too?

    jnewbery commented at 7:49 pm on August 8, 2019:
    fixed
  95. in test/functional/feature_dersig.py:55 in 3739be48f8 outdated
    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'],
    


    ariard commented at 11:45 pm on August 6, 2019:
    nit : is softfork_active defined in next commit could be used here?

    jnewbery commented at 7:51 pm on August 8, 2019:
    this also tests the height and type, so I’ll leave it as it is.
  96. in src/chainparams.cpp:262 in 7e33aea89f outdated
    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)
    


    ariard commented at 11:50 pm on August 6, 2019:
    Wouldn’t be better to CSV regtest value to be superior to BIP65 and so respect the historical activation order? Is there anyways activating MTP before BIP65 or others interact badly ?

    jnewbery commented at 7:53 pm on August 8, 2019:
    hmmm, it’s an interesting point, but I don’t think there’s an issue here. Previously, feature_csv_activation.py activated CSV at height 576, so that was still before bip 65/66 activation.
  97. in src/validation.cpp:3049 in 50fe006d1d outdated
    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;
    


    ariard commented at 0:35 am on August 7, 2019:
    It’s out of PR scope, but now that we don’t need 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?

    ajtowns commented at 5:00 am on August 7, 2019:
    I don’t think that works for UpdateUncommittedBlockStructures or BlockAssembler::CreateNewBlock, so you’d still need this function to be available.

    ariard commented at 2:13 pm on August 7, 2019:
    Right these functions test for the next block to be generated/submitted based on the parent, for them you may pass pindexPrev->nHeight + 1 to IsWitnessEnabled, should work but need to refactor also callers ?

    jnewbery commented at 7:58 pm on August 8, 2019:

    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.


    ajtowns commented at 3:41 pm on August 13, 2019:
    If it was going to take a height parameter, might as well just have the test in place directly. I don’t think it’s worth fiddling with for just segwit.
  98. ariard commented at 2:41 am on August 7, 2019: member

    Reviewed c64c9b5

    Mostly grep’ed versionbits refs, which should be updated IMO.

    Tested getblockchaininfo:

    • tip on testnet
    • tip on testnet, with a segwit activation height far in the future
    • at 7e33aea, to see if bip 9 object display well
    • csv activation at height 431 on regtest

    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)

  99. jnewbery commented at 8:15 pm on August 8, 2019: member
    Thanks for the review @ariard . I think I’ve addressed all your comments.
  100. jnewbery force-pushed on Aug 8, 2019
  101. ajtowns commented at 4:07 pm on August 13, 2019: member

    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:

    • the first commit (“tidy up reporting…”) only touches validation.h and validation.cpp in order to add/remove blank lines
    • the constant 9223372036854775807 in the tests would probably be clearer as 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.

  102. [rpc] Tidy up reporting of buried and ongoing softforks
    This combines reporting of buried (formally ISM) softfork deployments
    and BIP9 versionbits softfork deployments into one JSON object in the
    getblockchaininfo return object.
    3862e473f0
  103. jnewbery force-pushed on Aug 13, 2019
  104. jnewbery commented at 8:16 pm on August 13, 2019: member

    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.

  105. ajtowns commented at 1:01 am on August 14, 2019: member

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

  106. in src/validation.cpp:1647 in d4043c5683 outdated
    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)
    


    ariard commented at 4:19 pm on August 14, 2019:
    nit: seems to me this check is just about enforcing BIP112, BIP 68 reference should be there ? Same with LOCKTIME_VERIFY_SEQUENCE beneath (even if these 2 BIPs are strongly tied, their enforcement is done by different components)

    jnewbery commented at 7:53 pm on August 14, 2019:
    good spot. Fixed!
  107. in test/functional/feature_csv_activation.py:307 in d4043c5683 outdated
    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
    


    ariard commented at 5:14 pm on August 14, 2019:
    nit : maybe you could add an assert not active before activation, just to be sure everything is clean after the send_blocks/invalidateblock sequence

    jnewbery commented at 7:53 pm on August 14, 2019:
    done
  108. ariard approved
  109. ariard commented at 5:43 pm on August 14, 2019: member
    ACK dbe75a0, reviewed code, tests ok. I’ve had a look on python tests, comments have been addressed since last time and generally they look good.
  110. [Consensus] Bury CSV deployment height
    Hard code CSV deployment height to 419328 for mainnet.
    1c93b9b31c
  111. [Consensus] Bury segwit deployment
    Hardcode segwit deployment height to 481824 for mainnet.
    0328dcdcfc
  112. [tests] Add coverage for the content of getblockchaininfo.softforks 8319e738f9
  113. [docs] Add release notes for burying bip 9 soft fork deployments e78aaf41f4
  114. jnewbery force-pushed on Aug 14, 2019
  115. jnewbery commented at 7:54 pm on August 14, 2019: member
    Thanks for the review @ariard - I’ve addressed your comments.
  116. ajtowns commented at 8:32 am on August 15, 2019: member
    ACK e78aaf41f43d0e2ad78fa6d8dad61032c8ef73d0 ; checked diff to previous acked commit, checked tests still work
  117. fanquake removed the label Needs release note on Aug 15, 2019
  118. ariard commented at 3:02 pm on August 15, 2019: member
    ACK e78aaf4, check diff, run the tests again and successfully activated csv/segwit heights on mainnet as expected.
  119. MarcoFalke commented at 8:00 pm on August 15, 2019: member

    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 -

  120. MarcoFalke merged this on Aug 15, 2019
  121. MarcoFalke closed this on Aug 15, 2019

  122. MarcoFalke referenced this in commit 1bf2ff2bf8 on Aug 15, 2019
  123. jnewbery deleted the branch on Aug 15, 2019
  124. fanquake removed this from the "Blockers" column in a project

  125. domob1812 referenced this in commit 1597df21e2 on Aug 19, 2019
  126. laanwj referenced this in commit a54e52b4ae on Oct 1, 2019
  127. sidhujag referenced this in commit c4a8725587 on Oct 2, 2019
  128. MarcoFalke referenced this in commit aa8d76806c on May 19, 2020
  129. sidhujag referenced this in commit 2b8f1168f5 on May 19, 2020
  130. Munkybooty referenced this in commit e136fbc6e3 on Dec 9, 2021
  131. DrahtBot locked this on Dec 16, 2021

github-metadata-mirror

This is a metadata mirror of the GitHub repository bitcoin/bitcoin. This site is not affiliated with GitHub. Content is generated from a GitHub metadata backup.
generated: 2024-10-04 13:12 UTC

This site is hosted by @0xB10C
More mirrored repositories can be found on mirror.b10c.me