Implement BIP 8 based Speedy Trial activation #21392

pull achow101 wants to merge 8 commits into bitcoin:master from achow101:bip8-speedy-trial changing 17 files +503 −300
  1. achow101 commented at 10:31 pm on March 8, 2021: member

    Implements the block height (BIP 8) based Speedy Trial activation proposal that was discussed on the bitcoin-dev mailing list.

    In order to do so, this PR first changes the versionbits (BIP 9) implementation to use heights rather than MTP. Then the minimum activation height parameter is introduced. This also adds the minimum activation height as a third parameter of -vbparams. Additionally, the threshold is made a parameter of each deployment so that it can be customized for each deployment. Lastly several unit and functional tests are added.

    In order to make this easier to backport, the names of functions and classes are largely unchanged (except where necessary for the height based things). This means that some things are misnamed as they refer to BIP 9 even though this is really BIP 8.

    This PR borrows 2 commits from #19573 to implement height based versionbits, one commit from #21377 for tests, and 1 commit formerly part of #21380 for the threshold parameterization.

  2. in src/validation.h:226 in b16a755726 outdated
    225@@ -226,7 +226,6 @@ struct MempoolAcceptResult {
    226 MempoolAcceptResult AcceptToMemoryPool(CChainState& active_chainstate, CTxMemPool& pool, const CTransactionRef& tx,
    227                                        bool bypass_limits, bool test_accept=false) EXCLUSIVE_LOCKS_REQUIRED(cs_main);
    228 
    229-
    


    benthecarman commented at 10:39 pm on March 8, 2021:
    nit: unneeded

    achow101 commented at 10:56 pm on March 8, 2021:
    Removed
  3. in src/chainparamsbase.cpp:26 in b16a755726 outdated
    22@@ -23,7 +23,7 @@ void SetupChainParamsBaseOptions(ArgsManager& argsman)
    23                  "This is intended for regression testing tools and app development. Equivalent to -chain=regtest.", ArgsManager::ALLOW_ANY | ArgsManager::DEBUG_ONLY, OptionsCategory::CHAINPARAMS);
    24     argsman.AddArg("-segwitheight=<n>", "Set the activation height of segwit. -1 to disable. (regtest-only)", ArgsManager::ALLOW_ANY | ArgsManager::DEBUG_ONLY, OptionsCategory::DEBUG_TEST);
    25     argsman.AddArg("-testnet", "Use the test chain. Equivalent to -chain=test.", ArgsManager::ALLOW_ANY, OptionsCategory::CHAINPARAMS);
    26-    argsman.AddArg("-vbparams=deployment:start:end", "Use given start/end times for specified version bits deployment (regtest-only)", ArgsManager::ALLOW_ANY | ArgsManager::DEBUG_ONLY, OptionsCategory::CHAINPARAMS);
    27+    argsman.AddArg("-vbparams=deployment:@startheight:@timeoutheight", "Use given start/timeout heights for specified version bits deployment (regtest-only)", ArgsManager::ALLOW_ANY | ArgsManager::DEBUG_ONLY, OptionsCategory::CHAINPARAMS);
    


    benthecarman commented at 10:44 pm on March 8, 2021:
    I think it’d be worth adding the ability to set the min_activation_height for testing as well

    ajtowns commented at 10:52 pm on March 8, 2021:
    As it stands, -vbparams is only used to disable taproot activation to check soft-fork status, so this would only be useful if it came with an actual test.

    Sjors commented at 12:56 pm on March 9, 2021:
    7e47c280b7b5597e4a259f4d4b50d661fccd061a : I would be in favor of such a test, but I suggest introducing the @min_activation_height param in a later commit, rather than in this refactor.

    luke-jr commented at 4:18 am on March 14, 2021:
    -vbparams has been extended to accept min activation height, but the doc here isn’t updated with it.

    achow101 commented at 5:53 pm on March 14, 2021:
    Done
  4. ajtowns commented at 10:56 pm on March 8, 2021: member
    Maybe consider rebasing on #21380 ; it adds fuzz testing and removes the need for the MinActivationHeight() virtual functions, and should make the “per-deployment-threshold” part a bit simpler.
  5. achow101 force-pushed on Mar 8, 2021
  6. DrahtBot added the label Consensus on Mar 9, 2021
  7. DrahtBot added the label Docs on Mar 9, 2021
  8. DrahtBot added the label Mining on Mar 9, 2021
  9. DrahtBot added the label RPC/REST/ZMQ on Mar 9, 2021
  10. DrahtBot added the label Validation on Mar 9, 2021
  11. achow101 force-pushed on Mar 9, 2021
  12. achow101 commented at 1:47 am on March 9, 2021: member

    Maybe consider rebasing on #21380 ; it adds fuzz testing and removes the need for the MinActivationHeight() virtual functions, and should make the “per-deployment-threshold” part a bit simpler.

    Done. It did indeed make this simpler. The fuzzer also caught one minor issue.

  13. in src/consensus/params.h:29 in 3d6fa014f3 outdated
    30-    /** Start MedianTime for version bits miner confirmation. Can be a date in the past */
    31-    int64_t nStartTime;
    32-    /** Timeout/expiry MedianTime for the deployment attempt. */
    33-    int64_t nTimeout;
    34+    /** Start block height for version bits miner confirmation. Must be a retarget block, can be in the past. */
    35+    int64_t startheight;
    


    JeremyRubin commented at 1:54 am on March 9, 2021:
    Should this be set to NEVER_ACTIVE?

    achow101 commented at 2:21 am on March 9, 2021:
    The potential for uninitialized members here is concerning to me, so I am working on a followup to deal with those. That may be rolled into this PR.
  14. achow101 force-pushed on Mar 9, 2021
  15. in src/versionbits.cpp:42 in c25fe127af outdated
    35@@ -30,8 +36,8 @@ ThresholdState AbstractThresholdConditionChecker::GetStateFor(const CBlockIndex*
    36             cache[pindexPrev] = ThresholdState::DEFINED;
    37             break;
    38         }
    39-        if (pindexPrev->GetMedianTimePast() < nTimeStart) {
    40-            // Optimization: don't recompute down further, as we know every earlier block will be before the start time
    41+        if (pindexPrev->nHeight + 1 < height_start) {
    42+            // Optimization: don't recompute down further, as we know every earlier block will be before the start height
    


    JeremyRubin commented at 2:07 am on March 9, 2021:
    there’s not really a great way to rewrite this, but can we comment why it is +1

    JeremyRubin commented at 2:08 am on March 9, 2021:
    how it is doc’d below is great

    achow101 commented at 2:22 am on March 9, 2021:
    Done
  16. in src/versionbits.cpp:113 in c25fe127af outdated
    113-    stats.threshold = Threshold(params);
    114+    stats.period = m_period;
    115+    stats.threshold = m_dep.m_threshold;
    116 
    117-    if (pindex == nullptr)
    118+    if (pindexPrev == nullptr || (pindexPrev->nHeight + 1) < stats.period) {
    


    JeremyRubin commented at 2:09 am on March 9, 2021:
    ibid – a temporary makes this clearer

    achow101 commented at 2:22 am on March 9, 2021:
    Done
  17. JeremyRubin commented at 2:11 am on March 9, 2021: contributor
    generally looks ok to me couple minor comments; can do a more detailed review later
  18. achow101 force-pushed on Mar 9, 2021
  19. achow101 force-pushed on Mar 9, 2021
  20. achow101 force-pushed on Mar 9, 2021
  21. DrahtBot commented at 4:23 am on March 9, 2021: 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:

    • #21401 (Refactor versionbits deployments to avoid potential uninitialized variables by achow101)
    • #21391 ([Bundle 5/n] Prune g_chainman usage in RPC modules by dongcarl)
    • #21378 (Convert taproot to flag day activation by ajtowns)
    • #21377 (Speedy trial support for versionbits by ajtowns)
    • #20354 (test: Add feature_taproot.py –previous_release by MarcoFalke)
    • #19438 (Introduce deploymentstatus by ajtowns)

    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.

  22. achow101 force-pushed on Mar 9, 2021
  23. achow101 force-pushed on Mar 9, 2021
  24. in doc/bips.md:3 in 3395165dfe outdated
    0@@ -1,6 +1,7 @@
    1 BIPs that are implemented by Bitcoin Core (up-to-date up to **v0.21.0**):
    2 
    3-* [`BIP 9`](https://github.com/bitcoin/bips/blob/master/bip-0009.mediawiki): The changes allowing multiple soft-forks to be deployed in parallel have been implemented since **v0.12.1**  ([PR #7575](https://github.com/bitcoin/bitcoin/pull/7575))
    4+* [`BIP 8`](https://github.com/bitcoin/bips/blob/master/bip-0008.mediawiki): The changes for parallel, rapid deployment based on block height miner activation have been implemented since **v0.21.1** ([PR #21392](https://github.com/bitcoin/bitcoin/pull/21392)). The  with UASF failover is not yet implemented.
    


    Sjors commented at 12:37 pm on March 9, 2021:
    “The with UASF” -> “The UASF fallback with forced signalling (LOT=true) has not been implemented. The current implementation is the equivalent of LOT=false”

    achow101 commented at 5:47 pm on March 9, 2021:
    Done
  25. in src/rpc/blockchain.cpp:1300 in 3395165dfe outdated
    1294@@ -1297,24 +1295,24 @@ RPCHelpMan getblockchaininfo()
    1295                         {
    1296                             {RPCResult::Type::OBJ, "xxxx", "name of the softfork",
    1297                             {
    1298-                                {RPCResult::Type::STR, "type", "one of \"buried\", \"bip9\""},
    1299-                                {RPCResult::Type::OBJ, "bip9", "status of bip9 softforks (only for \"bip9\" type)",
    1300+                                {RPCResult::Type::STR, "type", "one of \"buried\", \"bip8\""},
    1301+                                {RPCResult::Type::OBJ, "bip8", "status of BIP 8 softforks (only for \"bip8\" type)",
    


    Sjors commented at 12:49 pm on March 9, 2021:
    This is a breaking RPC change, so needs a release note.

    achow101 commented at 5:47 pm on March 9, 2021:
    Done
  26. in src/rpc/blockchain.cpp:1230 in 7e47c280b7 outdated
    1228@@ -1229,7 +1229,7 @@ static void BIP9SoftForkDescPushBack(UniValue& softforks, const std::string &nam
    1229     // Deployments (e.g. testdummy) with timeout value before Jan 1, 2009 are hidden.
    1230     // A timeout value of 0 guarantees a softfork will never be activated.
    1231     // This is used when merging logic to implement a proposed softfork without a specified deployment schedule.
    1232-    if (consensusParams.vDeployments[id].nTimeout <= 1230768000) return;
    1233+    if (consensusParams.vDeployments[id].timeoutheight == Consensus::VBitsDeployment::NEVER_ACTIVE) return;
    


    Sjors commented at 12:59 pm on March 9, 2021:
    Comment needs update.

    achow101 commented at 5:47 pm on March 9, 2021:
    Done
  27. in test/functional/feature_taproot.py:1204 in 7e47c280b7 outdated
    1200@@ -1201,7 +1201,7 @@ def set_test_params(self):
    1201         self.num_nodes = 2
    1202         self.setup_clean_chain = True
    1203         # Node 0 has Taproot inactive, Node 1 active.
    1204-        self.extra_args = [["-par=1", "-vbparams=taproot:1:1"], ["-par=1"]]
    1205+        self.extra_args = [["-par=1", "-vbparams=taproot:@1:@1"], ["-par=1"]]
    


    Sjors commented at 1:03 pm on March 9, 2021:
    While you’re at it, the comment about which node is active is the wrong way around.

    achow101 commented at 5:37 pm on March 9, 2021:
    No? Node 0 has taproot inactive and setting the vbparams in this way (startheight=1, timeoutheight=1) ensures that it cannot activate on regtest.

    Sjors commented at 8:14 pm on March 9, 2021:

    No yes, taproot is always active on regtest: https://github.com/bitcoin/bitcoin/blob/968bc1e07b4a8a359876f5b8035fb4c369d04bad/src/chainparams.cpp#L418

    This had me massively confused too, but you can tell from testmempoolaccept if you manually craft a (broken) taproot transaction: https://github.com/BitMEXResearch/forkmonitor/blob/master/spec/models/chaintip_spec.rb#L377 (mempool_broken_tap is created by node A, which has no extra_args: https://github.com/BitMEXResearch/forkmonitor/blob/master/spec/models/chaintip_spec.rb#L22)


    Sjors commented at 8:18 pm on March 9, 2021:
    Ah crap, was reading the brackets wrong. Nvm. This is why I like to indent stuff :-)
  28. Sjors commented at 1:07 pm on March 9, 2021: member

    Concept ACK. Did some light review of 9e50401c8a593c569f586786d84554c027fd8e8d.

    Would love to see a functional test that shows the speedy trial in action, by looking at softforks in getblockchaininfo (I might write one myself)

  29. Sjors commented at 3:53 pm on March 9, 2021: member
    Here you go, a BIP 8 delayed activation functional test for your cherry-picking: https://github.com/Sjors/bitcoin/commits/2021/03/bip8-speedy-trial-test
  30. achow101 force-pushed on Mar 9, 2021
  31. achow101 commented at 5:48 pm on March 9, 2021: member

    Here you go, a BIP 8 delayed activation functional test for your cherry-picking: https://github.com/Sjors/bitcoin/commits/2021/03/bip8-speedy-trial-test

    Cherry picked.

  32. in doc/release-notes-21392.md:7 in 968bc1e07b outdated
    0@@ -0,0 +1,7 @@
    1+Low-level changes
    2+=================
    3+
    4+RPC
    5+---
    6+
    7+* BIP 9 has been replaced with a partial implementation of BIP 8. This change is reflected in `getblockchaininfo` where references to BIP 9 have been replaced with references to BIP 8.
    


    luke-jr commented at 7:54 pm on March 9, 2021:
    I don’t think this affects anything other than the docs?

    achow101 commented at 1:22 am on March 14, 2021:
    getblockchaininfo has a bip9 field that is changed to bip8. This only effects signet and regtest though since those are the only ones that have taproot show up in getblockchaininfo.

    Sjors commented at 1:34 pm on March 20, 2021:
    It also impacts mainnet for anyone automatically parsing getblockchaininfo for current softfork deployments.
  33. Sjors commented at 8:09 pm on March 9, 2021: member
    Linter complains 'time' imported but unused; my bad, I needed that import during an intermediate struggle (when generating coins on node 1, node 2 refused to sync).
  34. achow101 commented at 8:30 pm on March 9, 2021: member
    Fixed the linter
  35. achow101 force-pushed on Mar 9, 2021
  36. achow101 force-pushed on Mar 9, 2021
  37. in src/test/versionbits_tests.cpp:160 in d4c40a52cd outdated
    177@@ -178,65 +178,40 @@ BOOST_FIXTURE_TEST_SUITE(versionbits_tests, TestingSetup)
    178 BOOST_AUTO_TEST_CASE(versionbits_test)
    179 {
    180     for (int i = 0; i < 64; i++) {
    181-        // DEFINED -> FAILED
    


    Sjors commented at 1:47 pm on March 10, 2021:
    Why are you dropping all tests for DEFINED -> FAILED and DEFINED -> STARTED -> FAILED?

    achow101 commented at 5:38 pm on March 10, 2021:

    This change is from #19573. I believe it is dropped because BIP 8 specifies that the timeoutheight falls on a retarget block greater than the startheight, so this scenario is supposed to be impossible to occur. With BIP 9, it was possible because there was no guarantee of the number of blocks between the start time and timeout time, but with height, the number of blocks is guaranteed.

    DEFINE -> STARTED -> FAILED is still there though. I suppose you mean DEFINED -> STARTED -> Failed while threshold reached? In that case, it’s because the timeout height is supposed to fall on a retarget block so if the threshold is reached, it will always move to the locked in state.

  38. in src/test/versionbits_tests.cpp:282 in d4c40a52cd outdated
    258@@ -284,100 +259,67 @@ BOOST_AUTO_TEST_CASE(versionbits_params)
    259 BOOST_AUTO_TEST_CASE(versionbits_computeblockversion)
    260 {
    261     // Check that ComputeBlockVersion will set the appropriate bit correctly
    262-    // on mainnet.
    263-    const auto chainParams = CreateChainParams(*m_node.args, CBaseChainParams::MAIN);
    264+    const auto period = CreateChainParams(*m_node.args, CBaseChainParams::REGTEST)->GetConsensus().nMinerConfirmationWindow;
    265+    gArgs.ForceSetArg("-vbparams", strprintf("testdummy:@%s:@%s", period, period * 2));
    266+    const auto chainParams = CreateChainParams(*m_node.args, CBaseChainParams::REGTEST);
    


    Sjors commented at 1:52 pm on March 10, 2021:
    Why the switch from mainnet to regtest here?

    achow101 commented at 5:43 pm on March 10, 2021:
    I believe this change was done so that custom activation parameters could be used. Specifically, since mainnet start and timeout heights are in the hundreds of thousands of blocks, the test would just be wasting time building up a chain to reach that those heights. With regtest, it can use a way lower height so the test runs in a timely fashion.

    Sjors commented at 10:13 am on March 11, 2021:
    I guess the better question is: why did it use mainnet in the past?

    achow101 commented at 6:10 pm on March 11, 2021:
    I guess it was convenient and doesn’t really matter when the time can be faked?

    ajtowns commented at 2:46 am on March 12, 2021:
    I figured the theory was “if you’re going to test anything, test what’s going to happen on mainnet”. Since there’s no proof of work in the versionbits unit test, it might be reasonable to test with real heights – it’s just building a skiplist with half a million entries to get to the start height, and going from there?

    ajtowns commented at 8:07 pm on March 18, 2021:
    Oh, no – the reason was that it is possible to test “TESTDUMMY” activation on mainnet with MTP because you can fake the block time to be prior to the the genesis block’s timestamp; but you can’t do that with height because you obviously can’t have a block height prior to NEVER_ACTIVE ie -2.

    MarcoFalke commented at 8:42 am on March 26, 2021:
    ForceSetArg isn’t possible on mainnet? (This discussion thread can be closed?)
  39. Sjors commented at 2:04 pm on March 10, 2021: member
    The code changes for 7ff01fb91960b029f67fde68de6a9ea0178c8ab3 look good to me, but I’m a bit confused by the changes in and removal of tests in d4c40a5.
  40. kristapsk commented at 10:06 am on March 12, 2021: contributor

    There are some compiler warnings for me (GCC 9.3.0).

    0test/fuzz/versionbits.cpp: In function ‘void versionbits_fuzz_target(FuzzBufferType)’:
    1test/fuzz/versionbits.cpp:150:37: warning: ‘*((void*)& last_stats +16)’ may be used uninitialized in this function [-Wmaybe-uninitialized]
    2  150 |                     if (last_stats->possible) {
    3      |                         ~~~~~~~~~~~~^~~~~~~~
    4test/fuzz/versionbits.cpp:149:97: warning: ‘last_stats.VBitsStats::count’ may be used uninitialized in this function [-Wmaybe-uninitialized]
    5  149 |                     assert(stats.count == last_stats->count || stats.count == last_stats->count + 1);
    6      |                                                                                                 ^
    7test/fuzz/versionbits.cpp:127:35: warning: ‘last_stats.VBitsStats::elapsed’ may be used uninitialized in this function [-Wmaybe-uninitialized]
    8  127 |                 assert(last_stats && last_stats->elapsed == period - 1);
    9      |                                   ^~
    
  41. achow101 commented at 5:55 pm on March 12, 2021: member

    There are some compiler warnings for me (GCC 9.3.0).

    I believe these are caused by the changes from #21380.

  42. ajtowns commented at 4:54 am on March 13, 2021: member
    Hmm, I’m not seeing those warnings, and I think all those statements are protected by an if (last_stats) which should ensure it’s initialised.
  43. achow101 commented at 5:49 am on March 13, 2021: member
    I see this warnings with gcc 10.2.0
  44. ajtowns commented at 0:59 am on March 14, 2021: member
    Apparently I wasn’t seeing those warnings because my fuzz things were being compiled with -O0… Seems to be a gcc bug not handling optional very well to me (80635 maybe?) but it’s easy enough to not use an optional here, so I’ve added a patch to do that.
  45. in src/chainparams.cpp:196 in 7ff01fb919 outdated
    192@@ -189,22 +193,26 @@ class CTestNetParams : public CChainParams {
    193         consensus.BIP66Height = 330776; // 000000002104c8c45e99a8853285a3b592602a3ccde2b832481da85e9e4ba182
    194         consensus.CSVHeight = 770112; // 00000000025e930139bac5c6c31a403776da130831ab85be56578f3fa75369bb
    195         consensus.SegwitHeight = 834624; // 00000000002b980fcd729daaa248fd9316a5200e9b367f4ff2c42453e84201ca
    196-        consensus.MinBIP9WarningHeight = 836640; // segwit activation height + miner confirmation window
    197+        consensus.MinVBitsWarningHeight = 836640; // segwit activation height + miner confirmation window
    


    luke-jr commented at 1:07 am on March 14, 2021:
    We had discussed delaying the member/variable renames until after the actual logic changes for better backport/merge/review, and I thought that was a good idea. Are you planning to take that out of the PR? Been waiting to review pending on that…

    achow101 commented at 1:23 am on March 14, 2021:
    Oh yeah, forgot to do that. I’ve removed the rename commit that I added in this PR, but #21380 also has a rename commit and it’s up to @ajtowns to remove that if desired.
  46. achow101 force-pushed on Mar 14, 2021
  47. achow101 force-pushed on Mar 14, 2021
  48. achow101 force-pushed on Mar 14, 2021
  49. in src/chainparams.cpp:530 in 9f98cf7dfb outdated
    534             if (vDeploymentParams[0] == VersionBitsDeploymentInfo[j].name) {
    535-                UpdateVersionBitsParameters(Consensus::DeploymentPos(j), nStartTime, nTimeout);
    536+                UpdateVersionBitsParameters(Consensus::DeploymentPos(j), start_height, timeout_height, min_activation_height);
    537                 found = true;
    538-                LogPrintf("Setting version bits activation parameters for %s to start=%ld, timeout=%ld\n", vDeploymentParams[0], nStartTime, nTimeout);
    539+                LogPrintf("Setting version bits activation parameters for %s to start_height=%ld, timeout_height=%ld\n", vDeploymentParams[0], start_height, timeout_height);
    


    luke-jr commented at 4:08 am on March 14, 2021:
    Missing the new min_activation_height

    achow101 commented at 5:53 pm on March 14, 2021:
    Done
  50. in src/chainparams.cpp:513 in 9f98cf7dfb outdated
    508@@ -492,22 +509,25 @@ void CRegTestParams::UpdateActivationParametersFromArgs(const ArgsManager& args)
    509     for (const std::string& strDeployment : args.GetArgs("-vbparams")) {
    510         std::vector<std::string> vDeploymentParams;
    511         boost::split(vDeploymentParams, strDeployment, boost::is_any_of(":"));
    512-        if (vDeploymentParams.size() != 3) {
    513-            throw std::runtime_error("Version bits parameters malformed, expecting deployment:start:end");
    514+        if (vDeploymentParams.size() < 3 || vDeploymentParams.size() > 4) {
    515+            throw std::runtime_error("Version bits parameters malformed, expecting deployment:@start_height:@timeout_height[:@min_activation_height]");
    


    luke-jr commented at 4:08 am on March 14, 2021:
    I’m not sure it’s a good idea to use underscores here when everything else uses startheight

    achow101 commented at 5:53 pm on March 14, 2021:
    Done
  51. in src/consensus/params.h:37 in 9f98cf7dfb outdated
    37+    /** Threshold for activation */
    38+    int threshold;
    39+    /**
    40+     * If lock in occurs, delay activation until at least this block height. Activations only occur on retargets.
    41+     */
    42+    int64_t m_min_activation_height;
    


    luke-jr commented at 4:09 am on March 14, 2021:
    {0} here can avoid needing to initialise it everywhere it isn’t used (and reduce the risk of ever forgetting to do so)

    achow101 commented at 5:53 pm on March 14, 2021:
    Done
  52. in src/versionbits.cpp:14 in 9f98cf7dfb outdated
    14-    int64_t nTimeTimeout = EndTime(params);
    15+    int nPeriod = m_period;
    16+    int nThreshold = m_dep.threshold;
    17+    int64_t height_start = m_dep.startheight;
    18+    int64_t height_timeout = m_dep.timeoutheight;
    19+    int64_t min_activation_height = m_dep.m_min_activation_height;
    


    luke-jr commented at 4:10 am on March 14, 2021:
    height_active_min might be more consistent with the other variables.

    achow101 commented at 5:53 pm on March 14, 2021:
    Done
  53. in src/test/versionbits_tests.cpp:41 in 9f98cf7dfb outdated
    47-    ThresholdState GetStateFor(const CBlockIndex* pindexPrev) const { return AbstractThresholdConditionChecker::GetStateFor(pindexPrev, paramsDummy, cache); }
    48-    int GetStateSinceHeightFor(const CBlockIndex* pindexPrev) const { return AbstractThresholdConditionChecker::GetStateSinceHeightFor(pindexPrev, paramsDummy, cache); }
    49+    TestConditionChecker() : ThresholdConditionChecker(m_dep_storage, 10)
    50+    {
    51+        m_dep_storage.bit = 8;
    52+        m_dep_storage.startheight= 100;
    


    luke-jr commented at 4:11 am on March 14, 2021:
    nit: space

    achow101 commented at 5:53 pm on March 14, 2021:
    Done
  54. in src/versionbits.cpp:17 in 9f98cf7dfb outdated
    17+    int64_t height_start = m_dep.startheight;
    18+    int64_t height_timeout = m_dep.timeoutheight;
    19+    int64_t min_activation_height = m_dep.m_min_activation_height;
    20+
    21+    // Check if this deployment is never active.
    22+    if (height_start == Consensus::VBitsDeployment::NEVER_ACTIVE && height_timeout == Consensus::VBitsDeployment::NEVER_ACTIVE ) {
    


    luke-jr commented at 4:21 am on March 14, 2021:
    nit: extra space

    achow101 commented at 5:53 pm on March 14, 2021:
    Fixed
  55. luke-jr referenced this in commit d5594e6d99 on Mar 14, 2021
  56. luke-jr referenced this in commit f21d054aaf on Mar 14, 2021
  57. luke-jr referenced this in commit 12cd6b6b8a on Mar 14, 2021
  58. luke-jr referenced this in commit 3a56d56fa6 on Mar 14, 2021
  59. luke-jr referenced this in commit 798d7788e3 on Mar 14, 2021
  60. luke-jr referenced this in commit a050123661 on Mar 14, 2021
  61. in test/functional/feature_bip8.py:35 in 9f98cf7dfb outdated
    30+        # BIP 8 state transitions from "defined" to "started" or "failed" after
    31+        # the last block of the retargeting period has been mined. This means
    32+        # any new rules apply to transactions currently in the mempool, which
    33+        # might be mined in the next block.
    34+        #
    35+        # The next retarteting period starts at block 144, so nothing should
    


    michaelfolkson commented at 11:47 am on March 14, 2021:
    nit: s/retarteting/retargeting

    achow101 commented at 5:53 pm on March 14, 2021:
    Fixed
  62. in test/functional/feature_bip8.py:19 in 9f98cf7dfb outdated
    14+        # Node 1 has a regular activation window
    15+        # Node 2 uses speedy trial.
    16+        self.extra_args = [
    17+            [f'-vbparams=testdummy:@1:@1'],
    18+            [f'-vbparams=testdummy:@144:@{144 * 3}'],
    19+            [f'-vbparams=testdummy:@144:@{144 * 2}:@{144 * 4}'],
    


    luke-jr commented at 4:01 pm on March 14, 2021:
    (and below) f’‘should be avoided to make backport to 0.21 nicer

    achow101 commented at 5:54 pm on March 14, 2021:
    Done
  63. achow101 force-pushed on Mar 14, 2021
  64. DrahtBot added the label Needs rebase on Mar 15, 2021
  65. michaelfolkson commented at 12:25 pm on March 15, 2021: contributor

    Concept ACK, Approach ACK.

    There appears to be considerable community support for the Speedy Trial proposal and I think this PR is best positioned today to be merged in the near future given that it cherry picks commits from other authors (Luke, AJ, Sjors).

    This bitcoin-dev mailing list post had a startheight of May 1st (and that is the startheight Andrew is going with I think). That doesn’t leave much time if this is going to be released well in advance of that startheight with communication to users (including miners). The startheight can be pushed back if needed though ideally it wouldn’t need to be. The merge decision is obviously ultimately up to the maintainers. If this PR (or any PR) isn’t ready to be merged I wouldn’t expect it to be merged regardless of any upcoming planned startheight.

    edit: To be clear I don’t think this PR is ready to be merged yet. But hopefully it will be after at the very least a couple of weeks of sustained review.

  66. achow101 force-pushed on Mar 15, 2021
  67. DrahtBot removed the label Needs rebase on Mar 15, 2021
  68. DrahtBot commented at 4:48 pm on March 15, 2021: member

    🕵️ @harding @sipa have been requested to review this pull request as specified in the REVIEWERS file.

  69. DrahtBot added the label Needs rebase on Mar 15, 2021
  70. achow101 force-pushed on Mar 15, 2021
  71. DrahtBot removed the label Needs rebase on Mar 15, 2021
  72. amitiuttarwar commented at 8:24 pm on March 16, 2021: contributor

    Now that #21380 has been revamped to only add a fuzz test (no refactors), probably makes sense to update this PR to match?

    Also, can you update the OP description to match the commits? OP refers to 5 commits but the branch has 14 😛

  73. achow101 force-pushed on Mar 16, 2021
  74. achow101 commented at 9:10 pm on March 16, 2021: member

    Now that #21380 has been revamped to only add a fuzz test (no refactors), probably makes sense to update this PR to match?

    I have kept the refactoring commits and dropped the fuzzer commits, as the refactors were the reason this PR was based on #21380.

    Also, can you update the OP description to match the commits? OP refers to 5 commits but the branch has 14 stuck_out_tongue

    Updated the description.

  75. luke-jr referenced this in commit 36d7ef09a7 on Mar 17, 2021
  76. luke-jr referenced this in commit dc368e4957 on Mar 17, 2021
  77. Sjors commented at 4:21 pm on March 17, 2021: member
    It’s probably a good idea to extract 524b41ffdb67ebaf1538a39b88344e7d4118b4b8 to 414640fbaf8cf00e8e37451764a77cdec3fdd730 into a seperate PR, and perhaps also incorporate @luke-jr’s #21399. That covers all the refactoring and generic-making. This PR, the BIP 9 variant #21377, a BIP 8 (lot=false) and a BIP 9 activation can all build on that.
  78. achow101 force-pushed on Mar 17, 2021
  79. achow101 commented at 6:52 pm on March 17, 2021: member
    I’ve dropped the refactoring and will include it in another PR.
  80. laanwj added this to the "Blockers" column in a project

  81. achow101 force-pushed on Mar 18, 2021
  82. in src/test/versionbits_tests.cpp:211 in 2961b34e08 outdated
    213-                           .Mine(3999, TestTime(30001), 0).TestLockedIn().TestStateSinceHeight(3000)
    214-                           .Mine(4000, TestTime(30002), 0).TestActive().TestStateSinceHeight(4000)
    215-                           .Mine(14333, TestTime(30003), 0).TestActive().TestStateSinceHeight(4000)
    216-                           .Mine(24000, TestTime(40000), 0).TestActive().TestStateSinceHeight(4000)
    217+                           .Mine(99, TestTime(10000) - 1, 0x101).TestDefined().TestStateSinceHeight(0) // One second more and it would be started
    218+                           .Mine(100, TestTime(10000), 0x101).TestStarted().TestStateSinceHeight(100) // So that's what happens the next period
    


    ajtowns commented at 8:13 pm on March 18, 2021:
    Comment is wrong

    achow101 commented at 8:16 pm on March 18, 2021:
    Fixed.
  83. achow101 force-pushed on Mar 18, 2021
  84. in src/chainparamsbase.cpp:25 in 06b3b4de75 outdated
    21@@ -22,7 +22,7 @@ void SetupChainParamsBaseOptions(ArgsManager& argsman)
    22                  "This is intended for regression testing tools and app development. Equivalent to -chain=regtest.", ArgsManager::ALLOW_ANY | ArgsManager::DEBUG_ONLY, OptionsCategory::CHAINPARAMS);
    23     argsman.AddArg("-segwitheight=<n>", "Set the activation height of segwit. -1 to disable. (regtest-only)", ArgsManager::ALLOW_ANY | ArgsManager::DEBUG_ONLY, OptionsCategory::DEBUG_TEST);
    24     argsman.AddArg("-testnet", "Use the test chain. Equivalent to -chain=test.", ArgsManager::ALLOW_ANY, OptionsCategory::CHAINPARAMS);
    25-    argsman.AddArg("-vbparams=deployment:@startheight:@timeoutheight", "Use given start/timeout heights for specified version bits deployment (regtest-only)", ArgsManager::ALLOW_ANY | ArgsManager::DEBUG_ONLY, OptionsCategory::CHAINPARAMS);
    26+    argsman.AddArg("-vbparams=deployment:@startheight:@timeoutheight@min_activation_height", "Use given start, timeout, and minimum activation heights for specified version bits deployment (regtest-only)", ArgsManager::ALLOW_ANY | ArgsManager::DEBUG_ONLY, OptionsCategory::CHAINPARAMS);
    


    ajtowns commented at 8:18 pm on March 18, 2021:
    Missing [:] around min_activation_height

    achow101 commented at 11:34 pm on March 18, 2021:
    Done
  85. in test/functional/feature_bip8.py:28 in c3093dbc11 outdated
    23+        self.log.info("Test status at genesis...")
    24+        for i, node in enumerate(self.nodes):
    25+            self.log.debug('Node #{}...'.format(i))
    26+            info = node.getblockchaininfo()
    27+            assert_equal(info['blocks'], 0)
    28+            assert_equal(info["softforks"]["testdummy"]["bip8"]["status"], "defined")
    


    ajtowns commented at 8:47 pm on March 18, 2021:

    This loop structure is repeated a bunch; might be clearer as:

     0    def test_height(self, height, *status, mine=None):
     1        if height > self.height:
     2            assert mine is not None
     3            self.log.info(f"Test status at height {height}...")
     4            self.nodes[mine].generate(height - self.height)
     5            self.sync_blocks()
     6        elif height < self.height:
     7            assert mine is None
     8            self.log.info(f"Roll back to {height}...")
     9            old_block = self.nodes[0].getblockhash(height + 1)
    10            for node in self.nodes:
    11                node.invalidateblock(old_block)
    12        else:
    13            assert height == 0 and self.height == 0
    14            self.log.info(f"Test status at genesis...")
    15
    16        self.height = height
    17
    18        for (i, node), st in zip(enumerate(self.nodes), status):
    19            self.log.debug('Node #{}...'.format(i))
    20            info = node.getblockchaininfo()
    21            assert_equal(info['blocks'], height)
    22            assert_equal(info["softforks"]["testdummy"]["bip8"]["status"], st)
    23
    24    def run_test(self):
    25        self.test_height(0, "defined", "defined", "defined")
    26
    27        # BIP 8 state transitions from "defined" to "started" or "failed" after
    28        # the last block of the retargeting period has been mined. This means
    29        # any new rules apply to transactions currently in the mempool, which
    30        # might be mined in the next block.
    31        #
    32        # The next retargeting period starts at block 144, so nothing should
    33        # happen at 142 and the state should change at 143.
    34
    35        self.test_height(144-2, "defined", "defined", "defined", mine=0)
    36        self.test_height(144-1, "failed", "started", "started", mine=0)
    37
    38        self.log.info("Test status when not signalling...")
    39        self.test_height(144*2-1, "failed", "started", "failed", mine=0)
    40        self.test_height(144*3-1, "failed", "failed", "failed", mine=0)
    41
    42
    43        self.log.info("Test status when signalling...")
    44        # The new branch has unique block hashes, because of the signalling and
    45        # because generate uses a deterministic address that depends on the node
    46        # index.
    47
    48        self.test_height(144-1, "failed", "started", "started")
    49        self.test_height(144*2-1, "failed", "locked_in", "locked_in", mine=2)
    50        self.test_height(144*3-1, "failed", "active", "locked_in", mine=2)
    51        self.test_height(144*4-1, "failed", "active", "active", mine=2)
    

    achow101 commented at 11:34 pm on March 18, 2021:
    Done
  86. in test/functional/feature_bip8.py:17 in c3093dbc11 outdated
    12+        self.setup_clean_chain = True
    13+        # Node 0 has TestDummy inactive
    14+        # Node 1 has a regular activation window
    15+        # Node 2 uses speedy trial.
    16+        self.extra_args = [
    17+            ['-vbparams=testdummy:@1:@1'],
    


    ajtowns commented at 8:50 pm on March 18, 2021:
    Note @1:@1 gets interpreted equivalently to @144:@144 and transitions from DEFINED to FAILED for the block at height 144.

    achow101 commented at 11:34 pm on March 18, 2021:
    Since this is for a node that has testdummy inactive, I’ve just changed this to @-2:@-2.

    ajtowns commented at 0:02 am on March 19, 2021:
    I don’t think that works? “never active” means the fork won’t show up in getblockchaininfo (to avoid testdummy appearing on mainnet).

    achow101 commented at 0:08 am on March 19, 2021:
    I work around that by allowing a status of None to indicate that it shouldn’t show up in getblockchaininfo. The purpose of this node is to generate non-signaling blocks anyways, so the status of testdummy for it doesn’t matter that much.
  87. in src/versionbits.cpp:65 in 28f82b25ac outdated
    61+
    62         switch (state) {
    63             case ThresholdState::DEFINED: {
    64-                if (pindexPrev->GetMedianTimePast() >= nTimeTimeout) {
    65+                if (height >= height_timeout) {
    66                     stateNext = ThresholdState::FAILED;
    


    ajtowns commented at 8:51 pm on March 18, 2021:
    This doesn’t match the BIP 8 spec which specifies that DEFINED always transitions to STARTED and never skips directly to FAILED.

    achow101 commented at 9:37 pm on March 18, 2021:

    This condition was actually caught by the fuzzer (back when this PR included it).

    The scenario is that the timeoutheight is less than one period after the startheight. Without this, such a deployment would transition to the STARTED state, and then potentially to LOCKED_IN. But all of that after the timeoutheight has been passed. That doesn’t make any sense and doesn’t fit within what we are expecting in any tests.

    I think we need to revisit this case in the BIP.


    achow101 commented at 11:35 pm on March 18, 2021:
    After discussing this with @luke-jr, I’ve removed this transition. Since there should never be a deployment that has the timeoutheight less than one period after the startheight, I’ve also added a sanity check to the unit tests for this case.

    ajtowns commented at 0:23 am on March 19, 2021:

    Yes, the “always reach STARTED” issue was why there’s a separate NEVER_ACTIVE setting, I think.

    BIP8 does specify “timeoutheight must be at least 4096 blocks (2 retarget intervals) after startheight”, so you could treat timeoutheight being reached from DEFINED as an extension to the spec and define it any way you like, similar to the always/never active cases. (The old bip8 code in #19573 didn’t do that in order to match the spec in the bip as closely as possible to catch spec/implementation mismatches)

    As far as the fuzz tester goes, following the bip8 spec exactly should mean something like:

     0switch (state) {
     1case STARTED:
     2    assert(startheight <= height);
     3    if (exp_state == STARTED) {
     4        assert(height <= timeoutheight);
     5        assert(blocks_sig < threshold);
     6    } else {
     7        assert(exp_state == DEFINED);
     8    }
     9    break;
    10case FAILED:
    11    assert(exp_state == STARTED || exp_state == FAILED);
    12    assert(never_active_test || height >= timeoutheight);
    13} 
    

    or alternatively extending it so you can skip STARTED would probably not change as much but might warrant adding a check along the lines of:

    0static int next_boundary(int n, int m) { return (n + m - 1) - ((n - 1) % m); }
    1if (next_boundary(timeoutheight, period) >= next_boundary(startheight, period)) {
    2    assert(state == DEFINED || state == FAILED);
    3    assert(exp_state == DEFINED || exp_state == FAILED);
    4}
    
  88. in src/chainparams.cpp:557 in 28f82b25ac outdated
    512+            throw std::runtime_error(strprintf("Invalid startheight (%s)", vDeploymentParams[1]));
    513         }
    514-        if (!ParseInt64(vDeploymentParams[2], &nTimeout)) {
    515-            throw std::runtime_error(strprintf("Invalid nTimeout (%s)", vDeploymentParams[2]));
    516+        if (vDeploymentParams[2].empty() || vDeploymentParams[2].front() != '@' || !ParseInt64(vDeploymentParams[2].substr(1), &timeoutheight)) {
    517+            throw std::runtime_error(strprintf("Invalid timeoutheight (%s)", vDeploymentParams[2]));
    


    ajtowns commented at 8:56 pm on March 18, 2021:
    Could consider checking that startheight and timeoutheight (and min_activation_height) are either on retarget boundaries or match -1, max int or -2, -2. (Values not on retarget boundaries will be rounded up implicitly by the versionbits logic)

    achow101 commented at 11:35 pm on March 18, 2021:
    Perhaps for a followup.

    achow101 commented at 6:35 pm on March 19, 2021:
    I’ve decided to add checking for these parameters.
  89. achow101 force-pushed on Mar 18, 2021
  90. achow101 force-pushed on Mar 19, 2021
  91. achow101 force-pushed on Mar 19, 2021
  92. achow101 force-pushed on Mar 19, 2021
  93. achow101 commented at 8:27 pm on March 19, 2021: member
    Rebased for hidden merge conflict with the fuzzer. As such, the versionbits fuzzer is updated for heights and min activation height.
  94. in src/test/fuzz/versionbits.cpp:277 in 501e1371a7 outdated
    276         assert(since == 0);
    277         assert(exp_state == ThresholdState::DEFINED);
    278-        assert(current_block->GetMedianTimePast() < checker.m_begin);
    279-        assert(current_block->GetMedianTimePast() < checker.m_end);
    280+        assert(never_active_test || height < checker.m_begin);
    281+        assert(never_active_test || height < checker.m_end);
    


    ajtowns commented at 4:40 am on March 20, 2021:
    If height < checker.m_begin and m_begin + period <= m_end then height < checker.m_end is guaranteed. Suggest dropping the m_end test here, which then allows removing the assert(m_begin + period <= m_end) above, which then allows extending the range for both startheight and timeoutheight to [0, period*(max_periods-2)]. (Really the limit for timeouteight is period*(max_periods-1))

    achow101 commented at 5:32 am on March 20, 2021:
    I don’t quite follow as to why the range for both startheight and timeoutheight should be [0, period*(max_periods-2)]. I had set the range currently used in order to avoid the issue discussed in #21392 (review).

    ajtowns commented at 5:55 am on March 20, 2021:

    For bip9/mtp versionbits the cache lookup loop will say “oh, we haven’t reach start time yet, must still be DEFINED” which is inconsistent with the “DEFINED” check afterwards that says “if we reached timeout and start time at the same time, we go to FAILED instead of STARTED”. If the fuzzer tracked the mtp for the previous period, you could capture that logic too, but that seemed a bit messy.

    Because bip8 versionbits always goes DEFINED to STARTED it doesn’t have that inconsistency, so checking a broader range of parameters is easy.


    achow101 commented at 5:35 pm on March 20, 2021:
    Done
  95. in src/test/fuzz/versionbits.cpp:297 in 501e1371a7 outdated
    301+            assert(exp_state == ThresholdState::STARTED);
    302+            assert(blocks_sig >= threshold);
    303+        }
    304         break;
    305     case ThresholdState::ACTIVE:
    306+        assert(height >= checker.m_min_activation);
    


    ajtowns commented at 4:42 am on March 20, 2021:
    Changing this assertion to assert(always_active_test || height >= checker.m_min_activation) allows having m_min_activation be fuzzed unconditionally (ie for always/never active tests as well).

    achow101 commented at 5:38 am on March 20, 2021:
    Done
  96. in src/test/fuzz/versionbits.cpp:330 in 501e1371a7 outdated
    328-        if (exp_since == 0) {
    329-            assert(exp_state == ThresholdState::DEFINED);
    330-        } else {
    331-            assert(exp_state == ThresholdState::FAILED);
    332-        }
    333+        assert(state == ThresholdState::DEFINED);
    


    ajtowns commented at 4:43 am on March 20, 2021:
    I’m a bit surprised NEVER_ACTIVE is always DEFINED instead of always FAILED (it was this way in #19573 as well). I don’t think it makes any observable difference though.

    achow101 commented at 5:32 am on March 20, 2021:
    This was surprising to me as well. Since this commit was pulled from #19573, I just left it as it came from there.
  97. achow101 force-pushed on Mar 20, 2021
  98. in src/test/fuzz/versionbits.cpp:302 in 80fc57990b outdated
    307         assert(exp_state == ThresholdState::ACTIVE || exp_state == ThresholdState::LOCKED_IN);
    308         break;
    309     case ThresholdState::FAILED:
    310-        assert(current_block->GetMedianTimePast() >= checker.m_end);
    311+        assert(height >= checker.m_end);
    312         assert(exp_state != ThresholdState::LOCKED_IN && exp_state != ThresholdState::ACTIVE);
    


    ajtowns commented at 6:24 am on March 20, 2021:

    I think you should be able to make this:

    0    case ThresholdState::FAILED:
    1        assert(height >= checker.m_begin);
    2        assert(height >= checker.m_end);
    3        assert(exp_state == ThresholdState::FAILED || exp_state == ThresholdState::STARTED);
    

    achow101 commented at 5:35 pm on March 20, 2021:
    Done
  99. in src/versionbits.cpp:17 in 59833ff711 outdated
    13-    int64_t nTimeTimeout = EndTime(params);
    14+    int height_start = StartHeight(params);
    15+    int height_timeout = TimeoutHeight(params);
    16+
    17+    // Check if this deployment is never active.
    18+    if (height_start == Consensus::BIP9Deployment::NEVER_ACTIVE && height_timeout == Consensus::BIP9Deployment::NEVER_ACTIVE) {
    


    Sjors commented at 1:21 pm on March 20, 2021:
    59833ff7118510de7bd2e89d55e074dc23cef8ef : the second condition seems unnecessary, maybe just assert it?

    achow101 commented at 5:35 pm on March 20, 2021:
    I think it is fine to leave as is.

    Sjors commented at 6:42 pm on March 20, 2021:
    I could imagine some confusion if some sets height_start to NEVER_ACTIVE but then sets height_timeout to some real date. But hopefully this code gets enough review nobody would actually do that.

    achow101 commented at 7:08 pm on March 20, 2021:
    I’ve added an additional check to the sanity check unit test that the NEVER_ACTIVE and ALWAYS_ACTIVE parameters are set correctly.
  100. in src/chainparamsbase.cpp:25 in 59833ff711 outdated
    21@@ -22,7 +22,7 @@ void SetupChainParamsBaseOptions(ArgsManager& argsman)
    22                  "This is intended for regression testing tools and app development. Equivalent to -chain=regtest.", ArgsManager::ALLOW_ANY | ArgsManager::DEBUG_ONLY, OptionsCategory::CHAINPARAMS);
    23     argsman.AddArg("-segwitheight=<n>", "Set the activation height of segwit. -1 to disable. (regtest-only)", ArgsManager::ALLOW_ANY | ArgsManager::DEBUG_ONLY, OptionsCategory::DEBUG_TEST);
    24     argsman.AddArg("-testnet", "Use the test chain. Equivalent to -chain=test.", ArgsManager::ALLOW_ANY, OptionsCategory::CHAINPARAMS);
    25-    argsman.AddArg("-vbparams=deployment:start:end", "Use given start/end times for specified version bits deployment (regtest-only)", ArgsManager::ALLOW_ANY | ArgsManager::DEBUG_ONLY, OptionsCategory::CHAINPARAMS);
    26+    argsman.AddArg("-vbparams=deployment:@startheight:@timeoutheight", "Use given start/timeout heights for specified version bits deployment (regtest-only)", ArgsManager::ALLOW_ANY | ArgsManager::DEBUG_ONLY, OptionsCategory::CHAINPARAMS);
    


    Sjors commented at 1:25 pm on March 20, 2021:
    59833ff7118510de7bd2e89d55e074dc23cef8ef: doc should explain the special values -1 and -2

    achow101 commented at 5:35 pm on March 20, 2021:
    Added documentation for the always active and never active cases.

    luke-jr commented at 6:31 pm on March 20, 2021:
    Do we really need that level of detail in debug-only options?

    Sjors commented at 6:43 pm on March 20, 2021:
    It’s confused me more than zero times.
  101. in test/functional/feature_taproot.py:1204 in 59833ff711 outdated
    1200@@ -1201,7 +1201,7 @@ def set_test_params(self):
    1201         self.num_nodes = 2
    1202         self.setup_clean_chain = True
    1203         # Node 0 has Taproot inactive, Node 1 active.
    1204-        self.extra_args = [["-par=1", "-vbparams=taproot:1:1"], ["-par=1"]]
    1205+        self.extra_args = [["-par=1", "-vbparams=taproot:@-2:@-2"], ["-par=1"]]
    


    Sjors commented at 1:30 pm on March 20, 2021:

    59833ff7118510de7bd2e89d55e074dc23cef8ef: maybe use the exact terminology. I also find indentation less confusing:

    0self.extra_args = [
    1    ["-par=1", "-vbparams=taproot:@-2:@-2"], # Node 0 has Taproot NEVER_ACTIVE
    2    ["-par=1"] # Node 1 has Taproot ALWAYS_ACTIVE
    3]
    

    achow101 commented at 5:35 pm on March 20, 2021:
    Done
  102. Sjors approved
  103. Sjors commented at 2:31 pm on March 20, 2021: member

    ACK 80fc57990b4682bad350e1e7aff0ea4db73fec31

    Maybe wait with merging until BIP 8 is updated with min_activation_height support (see https://github.com/bitcoin/bips/pull/1080, https://github.com/bitcoin/bips/pull/1081)

  104. achow101 force-pushed on Mar 20, 2021
  105. achow101 force-pushed on Mar 20, 2021
  106. in src/chainparams.cpp:500 in 52990c1093 outdated
    498+        || (startheight != Consensus::BIP9Deployment::NEVER_ACTIVE && timeoutheight == Consensus::BIP9Deployment::NEVER_ACTIVE)) {
    499+        error = strprintf("When one of startheight or timeoutheight is %d, both must be %d", Consensus::BIP9Deployment::NEVER_ACTIVE, Consensus::BIP9Deployment::NEVER_ACTIVE);
    500+        return false;
    501+    }
    502+    if (startheight == Consensus::BIP9Deployment::ALWAYS_ACTIVE && timeoutheight != Consensus::BIP9Deployment::NO_TIMEOUT) {
    503+        error = strprintf("When startheight is %d, timeoutheight must be %d", Consensus::BIP9Deployment::ALWAYS_ACTIVE, Consensus::BIP9Deployment::NO_TIMEOUT);
    


    luke-jr commented at 6:44 pm on March 22, 2021:
    This isn’t true, and specifying NO_TIMEOUT is kinda annoying…?

    achow101 commented at 11:54 pm on March 22, 2021:
    Removed it.
  107. achow101 force-pushed on Mar 22, 2021
  108. achow101 force-pushed on Mar 23, 2021
  109. in src/chainparams.cpp:519 in 9a8334212b outdated
    517+    if (min_activation_height % consensus.nMinerConfirmationWindow != 0) {
    518+        error = strprintf("Invalid minimum activation height (%d), must be a multiple of %d", min_activation_height, consensus.nMinerConfirmationWindow);
    519+        return false;
    520+    }
    521+    if (timeoutheight <= startheight) {
    522+        error = strprintf("Invalid timeoutheight (%d), must be greater than the startheight (%d)", timeoutheight, timeoutheight);
    


    luke-jr commented at 10:30 pm on March 23, 2021:
    Second var is wrong

    achow101 commented at 0:13 am on March 24, 2021:
    Fixed. Also added tests for -vbparams.
  110. luke-jr changes_requested
  111. luke-jr commented at 11:44 pm on March 23, 2021: member

    utACK 9a8334212bfa557b06d300f49962b3a772b63837

    One last bug could be fixed, but it doesn’t really matter

  112. achow101 force-pushed on Mar 24, 2021
  113. achow101 force-pushed on Mar 24, 2021
  114. in src/chainparams.cpp:498 in bf4bddaac7 outdated
    496+    if ((startheight == Consensus::BIP9Deployment::NEVER_ACTIVE && timeoutheight != Consensus::BIP9Deployment::NEVER_ACTIVE)
    497+        || (startheight != Consensus::BIP9Deployment::NEVER_ACTIVE && timeoutheight == Consensus::BIP9Deployment::NEVER_ACTIVE)) {
    498+        error = strprintf("When one of startheight or timeoutheight is %d, both must be %d", Consensus::BIP9Deployment::NEVER_ACTIVE, Consensus::BIP9Deployment::NEVER_ACTIVE);
    499+        return false;
    500+    }
    501+    if (startheight == Consensus::BIP9Deployment::ALWAYS_ACTIVE) return true;
    


    sipa commented at 1:31 am on March 24, 2021:

    In commit “Migrate versionbits to use height instead of MTP”

    You already tested the case start==ALWAYS_ACTIVE && timeout==NO_TIMEOUT above. Would it make sense to enforce timeout==NO_TIMEOUT whenever start==ALWAYS_ACTIVE instead?


    luke-jr commented at 2:54 am on March 24, 2021:
    That was originally required, but unnecessary and since NO_TIMEOUT is a maxint, seems annoying to require for no reason.

    achow101 commented at 3:25 am on March 24, 2021:
    Oops. The requirement was intentionally removed, but I didn’t notice I had that earlier check. I’ve removed this line and dropped NO_TIMEOUT from the earlier check.
  115. in src/test/fuzz/versionbits.cpp:138 in bf4bddaac7 outdated
    144-        if (fuzzed_data_provider.ConsumeBool()) timeout += interval / 2;
    145-
    146-        // this may make timeout too early; if so, don't run the test
    147-        if (start_time > timeout) return;
    148+        startheight = fuzzed_data_provider.ConsumeIntegralInRange<int>(0, period * (max_periods - 2));
    149+        timeoutheight = fuzzed_data_provider.ConsumeIntegralInRange<int>(0, period * (max_periods - 2));
    


    sipa commented at 1:54 am on March 24, 2021:

    In commit “Migrate versionbits to use height instead of MTP”

    timeoutheight isn’t chosen to be >= startheight?


    achow101 commented at 2:49 am on March 24, 2021:
    afaict, the fuzzer is flexible enough to deal with that case. See also: #21392 (review)
  116. luke-jr approved
  117. luke-jr commented at 1:56 am on March 24, 2021: member
    utACK aa41d035137c0686ae3488e2f9e774c4edbf5bd7
  118. in src/chainparams.cpp:486 in bf4bddaac7 outdated
    484     }
    485+    bool CheckVBitsHeights(std::string& error, int64_t startheight, int64_t timeoutheight);
    486     void UpdateActivationParametersFromArgs(const ArgsManager& args);
    487 };
    488 
    489+bool CRegTestParams::CheckVBitsHeights(std::string& error, int64_t startheight, int64_t timeoutheight)
    


    sipa commented at 2:21 am on March 24, 2021:

    In commit “Migrate versionbits to use height instead of MTP”, but more general perhaps.

    There seems to be some overlap here with sanity_check_params. Would it make sense to have them merged, and perhaps make it always performed (also for mainnet/testnet, at startup)?


    luke-jr commented at 2:55 am on March 24, 2021:
    Seems best left for a refactor later IMO.

    achow101 commented at 3:26 am on March 24, 2021:

    I think it is reasonable to have the sanity check always performed at startup, but that can be left for a later PR.

    However I have removed the overlap with sanity_check_params and have that instead call CheckVBitsHeights (now part of CChainParams) in the test for the height sanity checks.

  119. achow101 force-pushed on Mar 24, 2021
  120. in src/consensus/params.h:37 in 2cb41863ab outdated
    36+    /** Threshold for activation */
    37+    int threshold;
    38+    /**
    39+     * If lock in occurs, delay activation until at least this block height. Activations only occur on retargets.
    40+     */
    41+    int64_t m_min_activation_height{0};
    


    benthecarman commented at 4:53 am on March 24, 2021:
    Why have startheight and timeoutheight as ints but as a int64_t for m_min_activation_height. For the vbparams you had them all as int64_t

    achow101 commented at 5:53 am on March 24, 2021:

    It should be int here, fixed.

    For vbparams, the string to int parser returns an int64_t, so that’s what I use there.

  121. in src/rpc/blockchain.cpp:1245 in 2cb41863ab outdated
    1241@@ -1243,8 +1242,8 @@ static void BIP9SoftForkDescPushBack(UniValue& softforks, const std::string &nam
    1242     {
    1243         bip9.pushKV("bit", consensusParams.vDeployments[id].bit);
    1244     }
    1245-    bip9.pushKV("start_time", consensusParams.vDeployments[id].nStartTime);
    1246-    bip9.pushKV("timeout", consensusParams.vDeployments[id].nTimeout);
    1247+    bip9.pushKV("startheight", consensusParams.vDeployments[id].startheight);
    


    benthecarman commented at 4:54 am on March 24, 2021:
    not a huge deal but this variable should be renamed eventually

    sipa commented at 1:22 am on March 27, 2021:
    @benthecarman To what?

    benthecarman commented at 2:16 am on March 27, 2021:
    bip8 or vbits since this is no longer a BIP 9 implementation

    sipa commented at 2:18 am on March 27, 2021:
    Oh, sure, I thought you were talking about “startheight”. This commit only touches observable names, not internal ones.

    achow101 commented at 3:35 am on March 27, 2021:
    The mass rename is planned for a followup so that this PR is easier to backport.
  122. benthecarman commented at 5:44 am on March 24, 2021: contributor

    ACK 2cb41863abe59717a199a66030185e2d23226a23

    Just a couple nits

  123. achow101 force-pushed on Mar 24, 2021
  124. chainparams: make versionbits threshold per-deployment 2be730c31d
  125. in src/versionbits.cpp:72 in 4a37423700 outdated
    74-                if (pindexPrev->GetMedianTimePast() >= nTimeTimeout) {
    75-                    stateNext = ThresholdState::FAILED;
    76-                    break;
    77-                }
    78                 // We need to count
    79                 const CBlockIndex* pindexCount = pindexPrev;
    


    benthecarman commented at 5:58 am on March 24, 2021:

    By starting here, are we then ignoring if the latest block is signaling?

    context:

    0// We track state by previous-block, so the height we should be comparing is +1
    1const int64_t height = pindexPrev->nHeight + 1;
    

    benthecarman commented at 5:59 am on March 24, 2021:
    Also for my sanity, is the reason the height check is moved from the beginning to the end of this case because we are using pindexPrev->nHeight + 1

    achow101 commented at 6:05 am on March 24, 2021:
    Yes. Determining whether the threshold is reached is done at the beginning of each signaling period, and determines the state for the entire signaling period. This is checking for whether the threshold is reached in the previous signaling period to determine whether the current signaling period (starting at block height) is locked_in. The current block (at height) is not part of the previous signaling period, so it is not counted.

    ajtowns commented at 6:07 am on March 24, 2021:

    Consider a block B just after a retarget period boundary, ie B->nHeight % 2016 == 0, and pindexPrev = B->pprev. pindexPrev was in state STARTED. What state should B be in? It depends on the signalling of the prior 2016 blocks – so we test pindexPrev->nVersion, pindexPrev->pprev->nVersion, etc. If enough signal, we switch to LOCKED_IN. If they don’t, we check B->nHeight (which is B->prev->nHeight + 1 aka pindexPrev->nHeight + 1) to see if it’s reached the timeout, and if so switch to FAILED.

    The reason the height check is moved to the end is so that every retarget period that begins in STARTED is allowed to transition to LOCKED_IN – previously, if the final period passed the timeout MTP midway through, it would be too late and would unconditionally transition to FAILED.


    achow101 commented at 6:10 am on March 24, 2021:

    Also for my sanity, is the reason the height check is moved from the beginning to the end of this case because we are using pindexPrev->nHeight + 1

    No. The height check is moved because BIP 8 specifies that the threshold being reached takes precedence over the timeoutheight being reached. This is because determining whether the threshold is reached would be done at timeoutheight. So if the height check came first, the last signaling period would always fail. Instead, we want to succeed if the threshold is reached in the last signaling period.

  126. benthecarman approved
  127. benthecarman commented at 6:30 am on March 24, 2021: contributor
    reACK 4a37423700f3432b976ada6adbc19e1ce75c7d9a
  128. achow101 commented at 6:32 am on March 24, 2021: member
    Had to rebase due to fuzzer changes
  129. achow101 force-pushed on Mar 24, 2021
  130. benthecarman commented at 6:38 am on March 24, 2021: contributor
    reACK 2f457ca73ce33557ecff0d45c37c33957b1b0764
  131. MarcoFalke removed the label Docs on Mar 24, 2021
  132. MarcoFalke removed the label Mining on Mar 24, 2021
  133. MarcoFalke removed the label RPC/REST/ZMQ on Mar 24, 2021
  134. in src/chainparams.cpp:490 in ce207a8f16 outdated
    487 
    488+bool CChainParams::CheckVBitsHeights(std::string& error, int64_t startheight, int64_t timeoutheight) const
    489+{
    490+    // Special always or never active cases
    491+    if ((startheight == Consensus::BIP9Deployment::NEVER_ACTIVE && timeoutheight == Consensus::BIP9Deployment::NEVER_ACTIVE)
    492+        || startheight == Consensus::BIP9Deployment::ALWAYS_ACTIVE) {
    


    MarcoFalke commented at 12:35 pm on March 24, 2021:
    Any reason to not enforce NO_TIMEOUT here?

    luke-jr commented at 3:34 pm on March 24, 2021:
    Not well-defined, not very friendly (it’s a max int value), and ignored anyway. (third time addressing this)

    MarcoFalke commented at 8:51 am on March 26, 2021:
    Then it could be ignored for NEVER as well or what is the reason to treat ALWAYS different from NEVER?

    achow101 commented at 4:32 pm on March 26, 2021:
    In GetStateFor, we are checking that both startheight and timeoutheight are NEVER_ACTIVE in order to return the correct state for never active. It only checks startheight is ALWAYS_ACTIVE when determining always active. This behavior came from #19573 and I didn’t change it. Perhaps @luke-jr or @ajtowns could explain why that is.

    luke-jr commented at 5:11 pm on March 26, 2021:

    It’s simply a non-change from the original BIP9 code.

    I don’t see a reason to change it here.


    MarcoFalke commented at 5:49 pm on March 26, 2021:
    ALWAYS and NEVER are not specified in any BIP, they are special magic values in our code only (introduced in this pull request). Treating them in the same fashion will be less code, and less code complexity.

    luke-jr commented at 8:25 pm on March 26, 2021:
    NO_TIMEOUT is not introduced in this PR. Feel free to refactor in a followup… or achow can do it here if you care so strongly. I don’t see that it matters either way.

    achow101 commented at 8:27 pm on March 26, 2021:
    I think we can leave this for a followup.

    sipa commented at 1:21 am on March 27, 2021:
    @MarcoFalke I think that the rationale is that NO_TIMEOUT isn’t a special value (unlike ALWAYS_ACTIVE and NEVER_ACTIVE), it’s just a timestamp very far in the future.
  135. in src/chainparams.cpp:480 in ce207a8f16 outdated
    478+    void UpdateVersionBitsParameters(Consensus::DeploymentPos d, int64_t startheight, int64_t timeoutheight)
    479     {
    480-        consensus.vDeployments[d].nStartTime = nStartTime;
    481-        consensus.vDeployments[d].nTimeout = nTimeout;
    482+        consensus.vDeployments[d].startheight = startheight;
    483+        consensus.vDeployments[d].timeoutheight = timeoutheight;
    


    MarcoFalke commented at 12:41 pm on March 24, 2021:
    I know this is just regtest, but an integral type of size 64-bit will be truncated if assigned to an integral type of size 32-bit

    luke-jr commented at 3:35 pm on March 24, 2021:
    Maybe have CheckVBitsHeights check this

    achow101 commented at 4:37 pm on March 24, 2021:
    Change these to int and changed the parsing to use ParseInt32 instead. Also added a test for out of range values.
  136. in src/test/fuzz/versionbits.cpp:32 in ce207a8f16 outdated
    32-    const int m_bit;
    33+    const int m_begin = 0;
    34+    const int m_end = 0;
    35+    const int m_period = 0;
    36+    const int m_threshold = 0;
    37+    const int m_bit = 0;
    


    MarcoFalke commented at 12:43 pm on March 24, 2021:
    Why is this changed?

    achow101 commented at 4:37 pm on March 24, 2021:
    Bad rebase conflict resolution, reverted.
  137. MarcoFalke commented at 12:49 pm on March 24, 2021: member
    Looked at the first commit, I’ll review the remaining commits later
  138. devrandom commented at 3:13 pm on March 24, 2021: none

    Tested as follows:

    • made unit tests fail by manually breaking the unit test versionbits_test in various ways
    • made unit tests fail by introducing off-by-one errors in all the relevant spots in versionbits.cpp
    • ensure no failure if cache is disabled by removing cache store statements in versionbits.cpp
    • manually tested with a single local regtest node
    • manually tested a reorg with two local regtest nodes going from STARTED to LOCKED_IN and back to STARTED when the reorg removes a single signalling block that brought the deployment just over the threshold
  139. achow101 force-pushed on Mar 24, 2021
  140. in src/chainparams.h:121 in dd363edd65 outdated
    115@@ -116,6 +116,9 @@ class CChainParams
    116     const MapAssumeutxo& Assumeutxo() const { return m_assumeutxo_data; }
    117 
    118     const ChainTxData& TxData() const { return chainTxData; }
    119+
    120+    /** Check that the given heights make sense */
    121+    bool CheckVBitsHeights(std::string& error, int startheight, int timeoutheight, int min_actvation_height) const;
    


    ariard commented at 10:35 pm on March 24, 2021:
    nit: min_activation_height

    achow101 commented at 1:32 am on March 25, 2021:
    Fixed
  141. in src/consensus/params.h:32 in dd363edd65 outdated
    31-    int64_t nTimeout;
    32+    /** Start block height for version bits miner confirmation. Must be a retarget block, can be in the past. */
    33+    int startheight;
    34+    /** Timeout/expiry block height for the deployment attempt. Must be a retarget block. */
    35+    int timeoutheight;
    36+    /** Threshold for activation */
    


    ariard commented at 10:41 pm on March 24, 2021:
    “Threshold for lockin. Must be a minimum of block per retarget period”

    achow101 commented at 1:32 am on March 25, 2021:
    Expanded this comment.
  142. in src/chainparams.h:121 in e43c8add34 outdated
    115@@ -116,6 +116,9 @@ class CChainParams
    116     const MapAssumeutxo& Assumeutxo() const { return m_assumeutxo_data; }
    117 
    118     const ChainTxData& TxData() const { return chainTxData; }
    119+
    120+    /** Check that the given heights make sense */
    121+    bool CheckVBitsHeights(std::string& error, int startheight, int timeoutheight) const;
    


    sipa commented at 11:05 pm on March 24, 2021:

    In commit “Migrate versionbits to use height instead of MTP”

    Nit: feels a bit strange to have this in CChainParams. Maybe put it in versionbits.{h,cpp}?


    achow101 commented at 1:36 am on March 25, 2021:
    Moving it to versionbits.{h,cpp} requires adding versionbits.cpp to libbitcoin_common and this introduces a bunch of other build system issues that I don’t want to figure out. I’ve moved the function out of CChainParams though so that it could be moved elsewhere in the future as a move-only.
  143. in src/versionbits.cpp:83 in dd363edd65 outdated
    78@@ -74,12 +79,16 @@ ThresholdState AbstractThresholdConditionChecker::GetStateFor(const CBlockIndex*
    79                 }
    80                 if (count >= nThreshold) {
    81                     stateNext = ThresholdState::LOCKED_IN;
    82+                } else if (height >= height_timeout) {
    83+                    stateNext = ThresholdState::FAILED;
    


    ariard commented at 0:06 am on March 25, 2021:

    Do you have test coverage for threshold taking precedence over reaching timeout ? This diff doesn’t break feature_bip8.py

     0@@ -77,10 +78,11 @@ ThresholdState AbstractThresholdConditionChecker::GetStateFor(const CBlockIndex*
     1                     }
     2                     pindexCount = pindexCount->pprev;
     3                 }
     4+                if (height >= height_timeout) {
     5+                    stateNext = ThresholdState::FAILED;
     6+                }
     7                 if (count >= nThreshold) {
     8                     stateNext = ThresholdState::LOCKED_IN;
     9-                } else if (height >= height_timeout) {
    10-                    stateNext = ThresholdState::FAILED;
    11                 }
    12                 break;
    13             }
    

    achow101 commented at 1:34 am on March 25, 2021:

    A unit test fails, although not because it explicitly tests this behavior. I’ve added such a test to feature_bip8.py.

    However this particular diff wouldn’t fail because it still moves to LOCKED_IN when the threshold is reached. You need to use else if in order for the height >= height_timeout check to take precedence.

  144. in src/versionbits.h:29 in dd363edd65 outdated
    23@@ -24,10 +24,10 @@ static const int32_t VERSIONBITS_NUM_BITS = 29;
    24  */
    25 enum class ThresholdState {
    26     DEFINED,   // First state that each softfork starts out as. The genesis block is by definition in this state for each deployment.
    27-    STARTED,   // For blocks past the starttime.
    28+    STARTED,   // For blocks past the startheight.
    29     LOCKED_IN, // For one retarget period after the first retarget period with STARTED blocks of which at least threshold have the associated bit set in nVersion.
    30     ACTIVE,    // For all blocks after the LOCKED_IN retarget period (final state)
    


    ariard commented at 0:13 am on March 25, 2021:
    I think you should document “For all blocks after the LOCKED_IN retarget period, if min activation height has been reached” ?

    achow101 commented at 1:35 am on March 25, 2021:
    Done
  145. in src/rpc/blockchain.cpp:1305 in dd363edd65 outdated
    1303                                     {RPCResult::Type::STR, "status", "one of \"defined\", \"started\", \"locked_in\", \"active\", \"failed\""},
    1304                                     {RPCResult::Type::NUM, "bit", "the bit (0-28) in the block version field used to signal this softfork (only for \"started\" status)"},
    1305-                                    {RPCResult::Type::NUM_TIME, "start_time", "the minimum median time past of a block at which the bit gains its meaning"},
    1306-                                    {RPCResult::Type::NUM_TIME, "timeout", "the median time past of a block at which the deployment is considered failed if not yet locked in"},
    1307+                                    {RPCResult::Type::NUM, "startheight", "the minimum height of a block at which the bit gains its meaning"},
    1308+                                    {RPCResult::Type::NUM, "timeoutheight", "the height of a block at which the deployment is considered failed if not yet locked in"},
    


    ariard commented at 0:20 am on March 25, 2021:
    Maybe display “min_activation_height” if relevant (!= 0) ?

    achow101 commented at 1:35 am on March 25, 2021:
    Changed to output it always.
  146. in src/test/fuzz/versionbits.cpp:282 in e43c8add34 outdated
    279+        assert(never_active_test || height < checker.m_begin);
    280         break;
    281     case ThresholdState::STARTED:
    282-        assert(current_block->GetMedianTimePast() >= checker.m_begin);
    283-        assert(current_block->GetMedianTimePast() < checker.m_end);
    284+        assert(height >= checker.m_begin);
    


    sipa commented at 0:22 am on March 25, 2021:

    In commit “Migrate versionbits to use height instead of MTP”:

    You could incorporate the later if (never_active_test) assert(state == ThresholdState::DEFINED); here as assert(!never_active_test);.

    (I found this by drawing the finite state machine based on these tests, and noticed this line was missing before noticing it’s done explicitly later)


    achow101 commented at 1:37 am on March 25, 2021:
    Added the new assert, but I did not remove the later assert as it seems like it is still useful to have that there.
  147. in src/chainparams.cpp:525 in dd363edd65 outdated
    523+    if (min_activation_height % consensus.nMinerConfirmationWindow != 0) {
    524+        error = strprintf("Invalid minimum activation height (%d), must be a multiple of %d", min_activation_height, consensus.nMinerConfirmationWindow);
    525+        return false;
    526+    }
    527+    if (timeoutheight < startheight + (int)consensus.nMinerConfirmationWindow) {
    528+        error = strprintf("Invalid timeoutheight (%d), must be at least one period greater than the startheight (%d)", timeoutheight, startheight);
    


    ariard commented at 0:24 am on March 25, 2021:
    According to BIP 8, “timeoutheight must be at least 4096 blocks (2 retarget intervals) after startheight” ?

    achow101 commented at 1:35 am on March 25, 2021:
    Done.
  148. in src/test/fuzz/versionbits.cpp:296 in e43c8add34 outdated
    300     case ThresholdState::FAILED:
    301-        assert(current_block->GetMedianTimePast() >= checker.m_end);
    302-        assert(exp_state != ThresholdState::LOCKED_IN && exp_state != ThresholdState::ACTIVE);
    303+        assert(height >= checker.m_begin);
    304+        assert(height >= checker.m_end);
    305+        assert(exp_state == ThresholdState::FAILED || exp_state == ThresholdState::STARTED);
    


    sipa commented at 0:25 am on March 25, 2021:

    In commit “Migrate versionbits to use height instead of MTP”:

    I believe you should do assert(exp_state == ThresholdState::FAILED || (exp_state == ThresholdState::STARTED && blocks_sig < threshold)); here instead. As stated, nothing prevents the FSM implementation from going from STARTED->FAILED when the threshold is reached in the last period.


    achow101 commented at 1:37 am on March 25, 2021:
    Done
  149. in src/chainparams.cpp:527 in dd363edd65 outdated
    525+        return false;
    526+    }
    527+    if (timeoutheight < startheight + (int)consensus.nMinerConfirmationWindow) {
    528+        error = strprintf("Invalid timeoutheight (%d), must be at least one period greater than the startheight (%d)", timeoutheight, startheight);
    529+        return false;
    530+    }
    


    ariard commented at 0:26 am on March 25, 2021:
    I think BIP 8 implies that if min_activation_height is set, it must be superior to start_height. Though not explicit “minimum_activation_height should be set to several retarget periods in the future if the startheight is to be very soon after software with parameters is expected to be released.”

    achow101 commented at 0:43 am on March 25, 2021:
    The minimum activation height can be any value, it does not need to be greater than the start_height. If it is less than or equal to the start height, it just means that the locked_in period does not become extended. We use 0 to mean “no minimum activation height”, but any height less than or equal to the start_height achieves the same effect, and in fact, all are handled the same way.
  150. ariard commented at 0:28 am on March 25, 2021: member
    Had a first parse on it
  151. Migrate versionbits to use height instead of MTP
    Co-authored-by: Anthony Towns <aj@erisian.com.au>
    c95287e378
  152. Rename user facing mentions of BIP 9 to versionbits and/or BIP 8
    Co-authored-by: Anthony Towns <aj@erisian.com.au>
    8f55573ed3
  153. Add minimum activation height to BIP9Deployments 1dac84b5de
  154. tests: test versionbits delayed activation 73d2756244
  155. Clarify and reduce nRuleChangeActivationThreshold
    As thresholds are now parameterized, nRuleChangeActivationThreshold is
    no longer the threshold used for activating new rule changes. Instead it
    is now only used to warn if there is an unkonwn versionbits deployment.
    To make this clear, rename to m_vbits_min_threshold and update the
    comment describing it.
    
    Additionally, because this is just a minimum used for a warning, reduce
    the threshold to 75% so that future soft forks which may have thresholds
    lower than 95% will still have warnings.
    50eb7f0b28
  156. test: add min_activation_height to -vbparams 91b2e4b4f9
  157. test: BIP 8 delayed activation functional test 2e55bcedb8
  158. achow101 force-pushed on Mar 25, 2021
  159. sipa commented at 2:30 am on March 25, 2021: member
    Code review ACK for all but the unit and functional tests (which I’ve only casually looked over). I inferred the finite state machine diagram based on the fuzz test code (making a few suggestions), and compared that with the implemented logic.
  160. Sjors commented at 2:12 pm on March 25, 2021: member
    re-ACK 2e55bcedb8d73e49620a5731196bf7e23bb53ccc
  161. in src/chainparams.cpp:525 in 2e55bcedb8
    523+    if (min_activation_height % consensus.nMinerConfirmationWindow != 0) {
    524+        error = strprintf("Invalid minimum activation height (%d), must be a multiple of %d", min_activation_height, consensus.nMinerConfirmationWindow);
    525+        return false;
    526+    }
    527+    if (timeoutheight < startheight + (2 * (int)consensus.nMinerConfirmationWindow)) {
    528+        error = strprintf("Invalid timeoutheight (%d), must be at least two periods greater than the startheight (%d)", timeoutheight, startheight);
    


    luke-jr commented at 7:37 pm on March 25, 2021:
    Why? Just one period should be enough…

    achow101 commented at 10:50 pm on March 25, 2021:

    Enforces this sentence in BIP 8:

    timeoutheight must be at least 4096 blocks (2 retarget intervals) after startheight.


    ajtowns commented at 0:00 am on March 26, 2021:
    That’s needed for lot=true/MUST_SIGNAL to work consistently with lot=false. Could also be solved by adding a transition directly from DEFINED to MUST_SIGNAL (skipping STARTED) I think.

    luke-jr commented at 3:05 am on March 26, 2021:
    Nah, probably not worth changing. One period is probably crazy anyway. And if we ever do need it, we can add the extra logic for it at that time.
  162. michaelfolkson commented at 8:46 pm on March 25, 2021: contributor

    ACK 2e55bcedb8d73e49620a5731196bf7e23bb53ccc

    Ideally a choice on a Speedy Trial PR would have been made by now so I could focus review and testing on one PR and one approach. Spreading review over two competing PRs is doing none of us any favors. I would have liked to have spent more time testing this PR before ACKing it but I think the biggest danger at this point is spreading review too thinly across two PRs.

  163. in src/consensus/params.h:32 in 2e55bcedb8
    31-    int64_t nTimeout;
    32+    /** Start block height for version bits miner confirmation. Must be a retarget block, can be in the past. */
    33+    int startheight;
    34+    /** Timeout/expiry block height for the deployment attempt. Must be a retarget block. */
    35+    int timeoutheight;
    36+    /** Threshold for lockin. Must be at least nRuleChangeActivationThreshold for that network. */
    


    JeremyRubin commented at 9:16 pm on March 25, 2021:
    I think this variable name changed.

    achow101 commented at 9:26 pm on March 25, 2021:
    Will fix if I retouch.
  164. in src/rpc/blockchain.cpp:1247 in 2e55bcedb8
    1241@@ -1243,8 +1242,9 @@ static void BIP9SoftForkDescPushBack(UniValue& softforks, const std::string &nam
    1242     {
    1243         bip9.pushKV("bit", consensusParams.vDeployments[id].bit);
    1244     }
    1245-    bip9.pushKV("start_time", consensusParams.vDeployments[id].nStartTime);
    1246-    bip9.pushKV("timeout", consensusParams.vDeployments[id].nTimeout);
    1247+    bip9.pushKV("startheight", consensusParams.vDeployments[id].startheight);
    1248+    bip9.pushKV("timeoutheight", consensusParams.vDeployments[id].timeoutheight);
    1249+    bip9.pushKV("minimum_activation_height", consensusParams.vDeployments[id].m_min_activation_height);
    


    luke-jr commented at 10:35 pm on March 25, 2021:
    Probably should either hide this when 0, or use starttime+2*period

    achow101 commented at 10:50 pm on March 25, 2021:
    I think it’s fine to always display.
  165. luke-jr changes_requested
  166. harding commented at 3:16 am on March 26, 2021: contributor

    As of commit 2e55bcedb8d73e49620a5731196bf7e23bb53ccc, lightly tested the likely outcomes of ST and confirmed that getblockchainfo returned the expected values whether activation was achieved or not, before and after the minimum_activation_period. I did not check close enough to detect off-by-one errors or other small details.

    Tested creating a regtest chain with this branch and -vbparams=taproot:@144:@576:@1008, producing enough blocks to lockin and activate taproot, synced them to a taproot-oblivious 0.20.1 node, then upgraded that node to this branch with the same vbparams and confirmed that the upgraded node’s getblockchaininfo results indicated it would be enforcing taproot.

    I skimmed the code and didn’t see any problems, but I don’t feel confident enough in my reviewing abilities to say it definitely is fine.

  167. in src/test/versionbits_tests.cpp:246 in c95287e378 outdated
    240@@ -263,105 +241,72 @@ BOOST_AUTO_TEST_CASE(versionbits_params)
    241 BOOST_AUTO_TEST_CASE(versionbits_computeblockversion)
    242 {
    243     // Check that ComputeBlockVersion will set the appropriate bit correctly
    244-    // on mainnet.
    245-    const auto chainParams = CreateChainParams(*m_node.args, CBaseChainParams::MAIN);
    246-    const Consensus::Params &mainnetParams = chainParams->GetConsensus();
    247+    const auto period = CreateChainParams(*m_node.args, CBaseChainParams::REGTEST)->GetConsensus().nMinerConfirmationWindow;
    248+    gArgs.ForceSetArg("-vbparams", strprintf("testdummy:@%s:@%s", period, period * 3));
    249+    const auto chainParams = CreateChainParams(*m_node.args, CBaseChainParams::REGTEST);
    


    MarcoFalke commented at 8:41 am on March 26, 2021:
    Any reason to use gArgs in the previous line, then m_node.args here? Might be better to just use a local symbol to avoid potentially polluting other tests

    achow101 commented at 4:33 pm on March 26, 2021:
    If I need to retouch
  168. in src/chainparams.cpp:499 in c95287e378 outdated
    497+        error = strprintf("When one of startheight or timeoutheight is %d, both must be %d", Consensus::BIP9Deployment::NEVER_ACTIVE, Consensus::BIP9Deployment::NEVER_ACTIVE);
    498+        return false;
    499+    }
    500+
    501+    // Actual params must be on retarget block
    502+    if (startheight < 0) {
    


    MarcoFalke commented at 8:44 am on March 26, 2021:
    Any reason to not enforce startheight >= nMinerConfirmationWindow? Otherwise it is possible to violate the “2 period rule” by setting @0:@288.

    achow101 commented at 4:33 pm on March 26, 2021:
    If I need to retouch
  169. in src/versionbits.cpp:55 in c95287e378 outdated
    60+        const int64_t height = pindexPrev->nHeight + 1;
    61+
    62         switch (state) {
    63             case ThresholdState::DEFINED: {
    64-                if (pindexPrev->GetMedianTimePast() >= nTimeTimeout) {
    65-                    stateNext = ThresholdState::FAILED;
    


    MarcoFalke commented at 9:01 am on March 26, 2021:
    Any reason to not assert this transition can’t happen?

    achow101 commented at 4:33 pm on March 26, 2021:
    I think that’s being checked by the test cases.

    sipa commented at 2:15 am on March 27, 2021:
    The fuzz test has all transitions of the state machine implemented (in a “come from” form rather than a “go to” form). I don’t think it’s possible to make incorrect transitions that it won’t detect.
  170. MarcoFalke commented at 10:43 am on March 26, 2021: member

    I looked at 2e55bcedb8d73e49620a5731196bf7e23bb53ccc 🚴

    Signature:

     0-----BEGIN PGP SIGNED MESSAGE-----
     1Hash: SHA512
     2
     3I looked at 2e55bcedb8d73e49620a5731196bf7e23bb53ccc 🚴
     4-----BEGIN PGP SIGNATURE-----
     5
     6iQGzBAEBCgAdFiEE+rVPoUahrI9sLGYTzit1aX5ppUgFAlwqrYAACgkQzit1aX5p
     7pUg82Av/XqMgcTbyFlMastLZ9AJdjHOHQjwpOkce6XC8ktfvqwbWpRioWiSHf0+C
     87bN6kFEJMxXPUAOZ4DeHLoBrsCMqTwPJZ21nNV9p8ZhByBBp5KPEA2PJ3s/RwBCp
     9YeRpKlLj856117mUv/5oFnABj4xuUMmZ3OkwuvU3RN7TrfKBUkR8ZkmzoSaOkv7s
    10O/bnZC4qudRl/dGFoa+CouYMWpi49mT+kB59Rs8UZha86KH5Td2siTAbbJBiMAi5
    118bFUwGSEsvxJGAr+52PHAcUUymGMZp29neY4Q5f7HIJ28uX5ged/EzSiFeeu4GZn
    12tH9B7mSqOvjnFIdOCvy8HYy8TzShKGvkiD4PcNvcVWfkQi2WbXtns1AjaZ5Tjeu9
    13ry2OHAzyWSvqbT1DIPD36VTb2c46u/vd9d05rkCjuccHcnXPkV49f68JblA2p9Fj
    14LUpoCswXIJPZtPxs2ZSwb0LxP54YW1RnguVOy6jg7PcMnecDeu/LFD4MsJtVJE9W
    15bqp2Q82X
    16=zjni
    17-----END PGP SIGNATURE-----
    

    Timestamp of file with hash 9fd4467b454c576b05f9c797bd20cd8620ad58c2b63ca8dd4e36f27ec7059a14 -

  171. devrandom commented at 2:28 pm on March 26, 2021: none

    ACK https://github.com/bitcoin/bitcoin/pull/21392/commits/2e55bcedb8d73e49620a5731196bf7e23bb53ccc

    In addition to the testing in #21392 (comment) I also checked that the fuzzer detects various off-by-one errors I manually introduced into the implementation.

  172. luke-jr approved
  173. luke-jr commented at 3:47 pm on March 26, 2021: member
    utACK 2e55bcedb8d73e49620a5731196bf7e23bb53ccc
  174. benthecarman commented at 3:55 pm on March 26, 2021: contributor
    reACK 2e55bcedb8d73e49620a5731196bf7e23bb53ccc
  175. sipa commented at 2:13 am on March 27, 2021: member
    Code review ACK 2e55bcedb8d73e49620a5731196bf7e23bb53ccc
  176. ajtowns commented at 4:56 am on March 28, 2021: member

    I think I’m an approach NACK, or at least approach -1, on using heights for activation at this point: they’re incompatible with activating on testnet due to hash rate inconsistency, and they’re incompatible with activation on signet due to custom signets being able to have completely random heights, since anyone can start a new one at any time. They’re not especially convenient for activating on mainnet either: we’re choosing timeframes and then converting that to expected block heights, but that can have significant errors – if we want to give people “3 months” to review an update before deploying it, and choose block heights to reflect that, we risk both increased hashpower reducing that time substantially, or reduced hashpower introducing unwanted delays.

    The main arguments I’ve seen for the height based approach is to avoid the signalling phase being skipped entirely, and to prevent a mandatory signalling phase (UASF/bip148/bip91/etc) from being skipped entirely, even in the event of a substantial loss of hashrate. But those goals can be achieved with the MTP approach as well – see the top commit on https://github.com/ajtowns/bitcoin/commits/202103-bip9-uasf .

    There’s the additional benefit that it’s simpler to explain “signalling begins at height X and ends at height Y” than “signalling begins at the first retarget period after time X, and ends in the last retarget period before time Y”, but that seems like a much lower priority than being able to test activations on testnet and signet prior to activation on mainnet…

  177. michaelfolkson commented at 11:55 am on March 28, 2021: contributor

    For the sake of respecting other reviewers’ time, Taproot is already active on the default Signet for testing and experimentation with Taproot transactions. Activating Taproot on testnet seems like a very low priority, certainly a lower priority than ensuring the optimal code is merged for mainnet activation.

    If you want a summary of the arguments for a consistent use of block height versus using a mix of block height and MTP there is this SE question with two answers, one from harding and one from me summarizing the arguments expressed in the two Speedy Trial PRs.

    This appears to be the major differentiator between the two PRs. If you would prefer a consistent use of block height I would recommend reviewing this PR. If you would prefer a mix of block height and MTP I would recommend reviewing AJ’s alternative Speedy Trial PR.

    As I’ve communicated on AJ’s PR, spreading review over two competing PRs is doing none of us any favors at this point.

  178. michaelfolkson commented at 12:35 pm on March 28, 2021: contributor

    Also this shows preferences for a consistent use of block height.

    I don’t have a strong view but it appears that @achow101 in #21392 and just by reading the comments in this PR @luke-jr @benthecarman @JeremyRubin @Sjors @roconnor-blockstream @harding all have a preference for a fully height based approach over any use of MTP.

    If that isn’t the case please correct me. And if anyone else has a view on entirely block height versus some limited use of MTP can you post it on IRC (##taproot-activation)?

    The fuzzing PR looks very cool. On that PR @MarcoFalke also expresses an expectation that activation will be based on block heights.

    edit: Some additional comments from today’s Core dev meeting

    “it comes down to mtp vs block height” @achow101

    “the main difference between these two PRs are using block height vs MTP " @amitiuttarwar

    “with mtp, we run the risk of losing 2 signaling periods. with already few signaling periods, this has the possibility of failing to activate due to bad luck” @achow101

    “the property that achow101’s PR improves is a fixed (number) of signals” @JeremyRubin

    In addition @JeremyRubin stated here “I have a preference for fully height based design, correct.”

    This is becoming a farce, pure and simple.

  179. benthecarman commented at 12:48 pm on March 28, 2021: contributor

    I think I’m an approach NACK, or at least approach -1, on using heights for activation at this point: they’re incompatible with activating on testnet due to hash rate inconsistency, and they’re incompatible with activation on signet due to custom signets being able to have completely random heights, since anyone can start a new one at any time

    I don’t think test networks should be relevant to mainnet’s activation mechanisms / consensus rules

  180. in src/versionbits.cpp:61 in 2e55bcedb8
    56@@ -49,20 +57,17 @@ ThresholdState AbstractThresholdConditionChecker::GetStateFor(const CBlockIndex*
    57         pindexPrev = vToCompute.back();
    58         vToCompute.pop_back();
    59 
    60+        // We track state by previous-block, so the height we should be comparing is +1
    61+        const int64_t height = pindexPrev->nHeight + 1;
    


    jnewbery commented at 7:46 pm on March 28, 2021:
    Why are you widening height from an int (32 bit) to a int64_t, and then comparing with ints below?

    achow101 commented at 9:30 pm on March 28, 2021:
    Will fix if I need to retouch.
  181. in test/functional/feature_bip8.py:5 in 2e55bcedb8
    0@@ -0,0 +1,124 @@
    1+#!/usr/bin/env python3
    2+# Copyright (c) 2021 The Bitcoin Core developers
    3+# Distributed under the MIT software license, see the accompanying
    4+# file COPYING or http://www.opensource.org/licenses/mit-license.php.
    5+# Test BIP 8 softforks
    


    jnewbery commented at 8:00 pm on March 28, 2021:
    Use docstrings for test description to separate from copyright notice

    achow101 commented at 9:30 pm on March 28, 2021:
    If I need to retouch
  182. in test/functional/feature_bip8.py:7 in 2e55bcedb8
    0@@ -0,0 +1,124 @@
    1+#!/usr/bin/env python3
    2+# Copyright (c) 2021 The Bitcoin Core developers
    3+# Distributed under the MIT software license, see the accompanying
    4+# file COPYING or http://www.opensource.org/licenses/mit-license.php.
    5+# Test BIP 8 softforks
    6+from collections import namedtuple
    7+from test_framework.test_framework import BitcoinTestFramework
    


    jnewbery commented at 8:00 pm on March 28, 2021:
    Separate std library imports from local imports

    achow101 commented at 9:30 pm on March 28, 2021:
    If I need to retouch
  183. in test/functional/feature_bip8.py:18 in 2e55bcedb8
    13+    def set_test_params(self):
    14+        self.num_nodes = 4
    15+        self.setup_clean_chain = True
    16+        self.extra_args = [
    17+            ['-vbparams=testdummy:@-2:@-2'], # Node 0 has TestDummy inactive
    18+            ['-vbparams=testdummy:@144:@{}'.format(144 * 3)], # Node 1 has regular activation window
    


    jnewbery commented at 8:00 pm on March 28, 2021:

    f-strings are generally preferred in new tests:

    0            [f'-vbparams=testdummy:@144:@{144 * 3}'], # Node 1 has regular activation window
    

    achow101 commented at 9:28 pm on March 28, 2021:
    These use this format so that it is easier to backport. 0.21 is using a version python that doesn’t support f-strings.
  184. in test/functional/feature_bip8.py:80 in 2e55bcedb8
    75+                assert_equal(info["softforks"]["testdummy"]["bip8"]["status"], status[restart.node])
    76+
    77+    def run_test(self):
    78+        self.log.info("Checking -vbparams")
    79+        self.stop_node(3)
    80+        self.nodes[3].assert_start_raises_init_error(extra_args=["-vbparams=testdummy:@-2:@1"], expected_msg="Error: When one of startheight or timeoutheight is -2, both must be -2")
    


    jnewbery commented at 8:02 pm on March 28, 2021:
    There’s a lot of repetition here. You could factor this out into a function.

    achow101 commented at 9:32 pm on March 28, 2021:
    It’s about as repetitive if this were a function IMO.
  185. in test/functional/feature_bip8.py:64 in 2e55bcedb8
    59+            # Restart this node and check that the status is what we expect
    60+            self.restart_node(restart.node, restart.extra_args)
    61+            info = self.nodes[restart.node].getblockchaininfo()
    62+            assert_equal(info["blocks"], height)
    63+            if restart.status is None:
    64+                assert "testdummy" not in info["softforks"]
    


    jnewbery commented at 8:13 pm on March 28, 2021:
    unused

    achow101 commented at 9:32 pm on March 28, 2021:
    If I need to retouch
  186. in test/functional/feature_bip8.py:75 in 2e55bcedb8
    70+            info = self.nodes[restart.node].getblockchaininfo()
    71+            assert_equal(info["blocks"], height)
    72+            if status[restart.node] is None:
    73+                assert "testdummy" not in info["softforks"]
    74+            else:
    75+                assert_equal(info["softforks"]["testdummy"]["bip8"]["status"], status[restart.node])
    


    jnewbery commented at 8:14 pm on March 28, 2021:
    unused

    achow101 commented at 9:32 pm on March 28, 2021:
    If I need to retouch
  187. in src/validation.cpp:1843 in 2e55bcedb8
    1841-    int64_t EndTime(const Consensus::Params& params) const override { return std::numeric_limits<int64_t>::max(); }
    1842+    int StartHeight(const Consensus::Params& params) const override { return 0; }
    1843+    int TimeoutHeight(const Consensus::Params& params) const override { return std::numeric_limits<int>::max(); }
    1844     int Period(const Consensus::Params& params) const override { return params.nMinerConfirmationWindow; }
    1845-    int Threshold(const Consensus::Params& params) const override { return params.nRuleChangeActivationThreshold; }
    1846+    int Threshold(const Consensus::Params& params) const override { return params.m_vbits_min_threshold; }
    


    jnewbery commented at 8:40 pm on March 28, 2021:

    The warning message emitted when this threshold is reached is “Warning: unknown new rules activated”, whereas now the threshold being reached really means “Warning: unknown new rules may be activated soon” (since 75% doesn’t actually indicate that any vbits deployment will activate).

    I think it might make sense to remove the m_vbits_min_threshold parameter from chain params and set the threshold to 75% of nMinerConfirmationWindow here.


    achow101 commented at 9:32 pm on March 28, 2021:
    Perhaps for a followup, or if I retouch.
  188. sipa commented at 2:07 am on March 30, 2021: member

    I believe this is the intended state machine:

    states

    To generate:

     0$ dot -Tpng >states.png
     1digraph versionbits {
     2    defined [shape=box,label="DEFINED"];
     3    started [shape=box,label="STARTED"];
     4    failed [shape=box,label="FAILED"];
     5    locked [shape=box,label="LOCKED_IN"];
     6    active [shape=box,label="ACTIVE"];
     7
     8    defined -> defined [label="height < begin"];
     9    defined -> started [label="height >= begin"];
    10    started -> started [label="sig < thresh\nheight < end"];
    11    started -> failed [label="sig < thresh\nheight >= end"];
    12    started -> locked [label="sig >= thresh\n"];
    13    locked -> locked [label="height < min_active"];
    14    locked -> active [label="height >= min_active"];
    15    active -> active [label="always"];
    16    failed -> failed [label="always"];
    17}
    
  189. Rspigler commented at 7:56 pm on April 2, 2021: contributor
    Should STARTED -> LOCKED_IN be defined as “sig >= thresh & height <= end” ?
  190. achow101 commented at 11:47 pm on April 6, 2021: member
    Closing this for now as #21377 (with some changes) is agreeable to most people.
  191. achow101 closed this on Apr 6, 2021

  192. laanwj removed this from the "Blockers" column in a project

  193. BitcoinMechanic referenced this in commit b8f6d10758 on Apr 9, 2021
  194. BitcoinMechanic referenced this in commit d16be281e9 on Apr 9, 2021
  195. BitcoinMechanic referenced this in commit d599c8f1bc on Apr 9, 2021
  196. BitcoinMechanic referenced this in commit b7e5779273 on Apr 9, 2021
  197. BitcoinMechanic referenced this in commit 49de1d8a8a on Apr 9, 2021
  198. BitcoinMechanic referenced this in commit 6d5454caf8 on Apr 9, 2021
  199. BitcoinMechanic referenced this in commit 718f289e65 on Apr 9, 2021
  200. BitcoinMechanic referenced this in commit bdafb8d435 on Apr 9, 2021
  201. DrahtBot locked this on Aug 18, 2022

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-12-21 15:12 UTC

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