Replace unused BIP 9 logic with draft BIP 8

luke-jr commented at 9:01 pm on July 23, 2020: member

Since all BIP 9 deployments are buried, we can simply replace it with BIP 8 cleanly.

luke-jr force-pushed on Jul 23, 2020

DrahtBot added the label Consensus on Jul 23, 2020

DrahtBot added the label Docs on Jul 23, 2020

DrahtBot added the label Mining on Jul 23, 2020

DrahtBot added the label RPC/REST/ZMQ on Jul 23, 2020

DrahtBot added the label Tests on Jul 23, 2020

DrahtBot added the label Validation on Jul 23, 2020

DrahtBot commented at 3:31 am on July 24, 2020: 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)
#21377 (Speedy trial support for versionbits by ajtowns)
#20354 (test: Add feature_taproot.py –previous_release by MarcoFalke)
#19461 (multiprocess: Add bitcoin-gui -ipcconnect option by ryanofsky)
#19460 (multiprocess: Add bitcoin-wallet -ipcconnect option by ryanofsky)
#17783 (util: Fix -norpcwhitelist, -norpcallowip, and similar corner case behavior by ryanofsky)
#17581 (refactor: Remove settings merge reverse precedence code by ryanofsky)
#17580 (refactor: Add ALLOW_LIST flags and enforce usage in CheckArgFlags by ryanofsky)
#17493 (util: Forbid ambiguous multiple assignments in config file by ryanofsky)
#10102 ([experimental] Multiprocess bitcoin by ryanofsky)

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.

MarcoFalke removed the label Tests on Aug 23, 2020

luke-jr force-pushed on Oct 15, 2020

luke-jr renamed this:
~~[WIP] Replace unused BIP 9 logic with BIP 8~~
Replace unused BIP 9 logic with draft BIP 8
on Oct 15, 2020

luke-jr commented at 6:30 pm on October 15, 2020: member

Rebased and updated with some of @ajtowns ’s changes to BIP 8.

This is probably still not the final form of BIP 8 (see https://github.com/bitcoin/bips/pull/950), but merging this sooner means we have fewer changes to backport together with the activation parameters later on.

(It also means the RPC interfaces are updated now instead of later)

luke-jr marked this as ready for review on Oct 15, 2020

luke-jr force-pushed on Oct 15, 2020

in src/consensus/params.h:25 in 22f81eaebe outdated

19@@ -20,24 +20,29 @@ enum DeploymentPos
20 };
21 
22 /**
23- * Struct for each individual consensus rule change using BIP9.
24+ * Struct for each individual consensus rule change using version bits (BIP 8).
25  */
26-struct BIP9Deployment {
27+struct VBitsDeployment {

ajtowns commented at 3:44 am on October 19, 2020:

Maybe DeploymentParams ? Might be good to do as many of the renames as possible via scripted-diff.

in src/consensus/params.h:42 in 22f81eaebe outdated

46+    /** Special value for startheight indicating that the deployment is always active.
47      *  This is useful for testing, as it means tests don't need to deal with the activation
48-     *  process (which takes at least 3 BIP9 intervals). Only tests that specifically test the
49+     *  process (which takes at least 3 intervals). Only tests that specifically test the
50      *  behaviour during activation cannot use this. */
51     static constexpr int64_t ALWAYS_ACTIVE = -1;

ajtowns commented at 4:15 am on October 19, 2020:

Maybe both startheight and timeoutheight should be set to ALWAYS_ACTIVE in the same way they are for NEVER_ACTIVE.

in src/test/versionbits_tests.cpp:255 in 22f81eaebe outdated

260@@ -249,100 +261,67 @@ BOOST_AUTO_TEST_CASE(versionbits_test)
261 BOOST_AUTO_TEST_CASE(versionbits_computeblockversion)
262 {
263     // Check that ComputeBlockVersion will set the appropriate bit correctly
264-    // on mainnet.
265-    const auto chainParams = CreateChainParams(*m_node.args, CBaseChainParams::MAIN);
266+    const auto period = CreateChainParams(*m_node.args, CBaseChainParams::REGTEST)->GetConsensus().nMinerConfirmationWindow;
267+    gArgs.ForceSetArg("-vbparams", strprintf("testdummy:@%s:@%s", period, period * 2));
268+    const auto chainParams = CreateChainParams(*m_node.args, CBaseChainParams::REGTEST);
269     const Consensus::Params &mainnetParams = chainParams->GetConsensus();

ajtowns commented at 9:35 am on October 19, 2020:

This is no longer mainnet params…

in src/chainparams.cpp:485 in 22f81eaebe outdated

484+            throw std::runtime_error("Version bits parameters malformed, expecting deployment:@startheight:@timeoutheight");
485         }
486-        int64_t nStartTime, nTimeout;
487-        if (!ParseInt64(vDeploymentParams[1], &nStartTime)) {
488-            throw std::runtime_error(strprintf("Invalid nStartTime (%s)", vDeploymentParams[1]));
489+        int64_t startheight, timeoutheight;

ajtowns commented at 7:09 am on October 21, 2020:

Not sure why it’s a new error, but clang insists that these need to be initialised, and is causing travis to fail.

in src/versionbits.cpp:17 in 22f81eaebe outdated

15+    int64_t height_timeout = TimeoutHeight(params);
16+    const bool lockinontimeout = LockinOnTimeout(params);
17 
18     // Check if this deployment is always active.
19-    if (nTimeStart == Consensus::BIP9Deployment::ALWAYS_ACTIVE) {
20+    if (height_start == Consensus::VBitsDeployment::ALWAYS_ACTIVE) {

ajtowns commented at 7:10 am on October 21, 2020:

Need to also special case NEVER_ACTIVE here.

in src/chainparams.cpp:396 in 22f81eaebe outdated

392@@ -388,11 +393,11 @@ class CRegTestParams : public CChainParams {
393         consensus.nRuleChangeActivationThreshold = 108; // 75% for testchains
394         consensus.nMinerConfirmationWindow = 144; // Faster than normal for regtest (144 instead of 2016)
395         consensus.vDeployments[Consensus::DEPLOYMENT_TESTDUMMY].bit = 28;
396-        consensus.vDeployments[Consensus::DEPLOYMENT_TESTDUMMY].nStartTime = 0;
397-        consensus.vDeployments[Consensus::DEPLOYMENT_TESTDUMMY].nTimeout = Consensus::BIP9Deployment::NO_TIMEOUT;
398+        consensus.vDeployments[Consensus::DEPLOYMENT_TESTDUMMY].startheight = 0;

ajtowns commented at 12:07 pm on October 21, 2020:

startheight = 144 would be more accurate, it can’t actually start at height 0.

(This caused a weird bug in validation_blocks_tests when I tried implementing bitcoin/bips#1023 and it was able to go to STARTED in the first block; didn’t track down exactly what the issue was though)

ajtowns commented at 12:20 pm on October 21, 2020: member

This has some bugs, see below.

I’ve done a branch with my suggestions at https://github.com/ajtowns/bitcoin/tree/202010-bip8-suggestions – it does the following:

makes the output of the versionbits unit test more helpful when it finds bugs
updates the unit tests to check the “never active” case (which catches a bug in this PR)
adapts your commits for the changes above, since there were merge conflicts
has some explicit “fixups” for your commits fixing bugs or updating the changes above for your patches
has a few refactors for versionbits and bip8 support that I think makes things a bit easier to follow
has draft implementations for the open PRs against BIP8

DrahtBot added the label Needs rebase on Nov 9, 2020

michaelfolkson commented at 10:14 am on February 1, 2021: contributor

Concept ACK.

Since revised BIP 8 is a strict improvement on BIP 9 it doesn’t hurt to replace BIP 9 logic with revised BIP 8 logic. AJ has highlighted some bugs, improvements that haven’t been addressed yet. I also don’t know if you are waiting for revised BIP 8 to be finalized at this point before a rebased version of this is ready to be merged.

luke-jr force-pushed on Feb 2, 2021

DrahtBot removed the label Needs rebase on Feb 2, 2021

luke-jr force-pushed on Feb 4, 2021

luke-jr force-pushed on Feb 7, 2021

luke-jr commented at 4:45 am on February 8, 2021: member

Rebased, merged in @ajtowns’s fixes and other changes, and got CI passing.

in src/rpc/blockchain.cpp:1256 in 6f27bdeba7 outdated

1261-        BIP9Stats statsStruct = VersionBitsTipStatistics(consensusParams, id);
1262+        VBitsStats statsStruct = VersionBitsTipStatistics(consensusParams, id);
1263         statsUV.pushKV("period", statsStruct.period);
1264-        statsUV.pushKV("threshold", statsStruct.threshold);
1265+        if (ThresholdState::STARTED == thresholdState) {
1266+            statsUV.pushKV("threshold", statsStruct.threshold);

ajtowns commented at 4:05 am on February 9, 2021:

Test should be thresholdState != ThresholdState::LOCKED_IN – now that https://github.com/bitcoin/bips/pull/1021/ is merged, threshold is still relevant during MUST_SIGNAL phase.

ajtowns commented at 4:17 am on February 9, 2021: member

Sample functional test at https://github.com/ajtowns/bitcoin/commits/202102-bip8-functest – it only picked up a minor issue with getblockchaininfo

in doc/bips.md:3 in 6f27bdeba7 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 with UASF failover have been implemented since **v0.21.0**  ([PR #19573](https://github.com/bitcoin/bitcoin/pull/19573))

benthecarman commented at 0:47 am on February 10, 2021:

shouldn’t this be v0.21.1?

in src/chainparams.cpp:493 in 6f27bdeba7 outdated

497             if (vDeploymentParams[0] == VersionBitsDeploymentInfo[j].name) {
498-                UpdateVersionBitsParameters(Consensus::DeploymentPos(j), nStartTime, nTimeout);
499+                UpdateVersionBitsParameters(Consensus::DeploymentPos(j), startheight, timeoutheight, lockinontimeout);
500                 found = true;
501-                LogPrintf("Setting version bits activation parameters for %s to start=%ld, timeout=%ld\n", vDeploymentParams[0], nStartTime, nTimeout);
502+                LogPrintf("Setting version bits activation parameters for %s to startheight=%ld, timeoutheight=%ld\n", vDeploymentParams[0], startheight, timeoutheight);

benthecarman commented at 1:04 am on February 10, 2021:

this log should probably include the timeoutheight

luke-jr commented at 4:54 pm on February 19, 2021:

You mean lockinontimeout, I assume?

benthecarman commented at 5:24 pm on February 19, 2021:

Yes, sorry

in src/consensus/params.h:31 in 6f27bdeba7 outdated

32-    /** Timeout/expiry MedianTime for the deployment attempt. */
33-    int64_t nTimeout;
34+    /** Start block height for version bits miner confirmation. Should be a retarget block, can be in the past. */
35+    int64_t startheight;
36+    /** Timeout/expiry block height for the deployment attempt. Should be a retarget block. */
37+    int64_t timeoutheight;

benthecarman commented at 1:06 am on February 10, 2021:

Should be a retarget block

In BIP 8, it defines these as a must

startheight and timeoutheight must be an exact multiple of 2016 (ie, at a retarget boundary), and timeoutheight must be at least 4096 blocks (2 retarget intervals) after startheight.

in src/rpc/mining.cpp:830 in 6f27bdeba7 outdated

823@@ -823,6 +824,10 @@ static RPCHelpMan getblocktemplate()
824             case ThresholdState::FAILED:
825                 // Not exposed to GBT at all
826                 break;
827+            case ThresholdState::MUST_SIGNAL:
828+                // Bit must be set in block version
829+                vbrequired |= VersionBitsMask(consensusParams, pos);
830+                // FALL THROUGH to sed nVersion and get vbavailable set...

benthecarman commented at 1:08 am on February 10, 2021:

sed

set

in src/test/versionbits_tests.cpp:23 in 6f27bdeba7 outdated

19+    return (state == ThresholdState::DEFINED ?     "DEFINED" :
20+            state == ThresholdState::STARTED ?     "STARTED" :
21+            state == ThresholdState::LOCKED_IN ?   "LOCKED_IN" :
22+            state == ThresholdState::ACTIVE ?      "ACTIVE" :
23+            state == ThresholdState::FAILED ?      "FAILED" :
24+            "");

benthecarman commented at 1:10 am on February 10, 2021:

Couldn’t this use a switch statement?

luke-jr commented at 5:01 pm on February 19, 2021:

This PR is intended to merge cleanly to 0.21, which only requires C++11. (switch inside constexpr functions requires C++14)

luke-jr force-pushed on Feb 19, 2021

luke-jr commented at 5:04 pm on February 19, 2021: member

Addressed review comments & added @ajtowns ’s new test.

Intentionally left threshold at 95% for now - that is best left for a future PR to change (when defining a softfork to use BIP8)

luke-jr force-pushed on Feb 19, 2021

benthecarman approved

benthecarman commented at 5:27 pm on February 19, 2021: contributor

ACK bd8517135fc839c3332fea4d9c8373b94c8c9de8

TheBlueMatt commented at 7:03 am on February 22, 2021: member

I’m a little dubious that forced-signaling logic shouldn’t come with code to handle the network forking (ie more aggressive peer-searching via protocol magic or quicker new connections when the chain has stalled). It seems like forced-signaling (which happily forks itself off the network in many of the ways people are talking about using the code today) probably isn’t something that should be merged unless the code for it is complete in the sense that it handles likely outcomes gracefully.

In general, there seems to be little reason to include support for the forced-signaling variant of BIP 9 in this PR - I don’t believe there is immediate desire to use it in Bitcoin Core, at least if I’m understanding the proposals correctly, and it adds a lot of complexity.

darosior commented at 1:27 pm on February 22, 2021: member

Concept ACK – considerations on the parameters aside, BIP8 is a strict improvement over BIP9. Will review soon.

luke-jr force-pushed on Feb 22, 2021

in src/rpc/blockchain.cpp:1306 in 52fcdb6b00 outdated

1289@@ -1290,8 +1290,8 @@ RPCHelpMan getblockchaininfo()
1290                                 {
1291                                     {RPCResult::Type::STR, "status", "one of \"defined\", \"started\", \"locked_in\", \"active\", \"failed\""},
1292                                     {RPCResult::Type::NUM, "bit", "the bit (0-28) in the block version field used to signal this softfork (only for \"started\" status)"},
1293-                                    {RPCResult::Type::NUM_TIME, "start_time", "the minimum median time past of a block at which the bit gains its meaning"},
1294-                                    {RPCResult::Type::NUM_TIME, "timeout", "the median time past of a block at which the deployment is considered failed if not yet locked in"},
1295+                                    {RPCResult::Type::NUM, "startheight", "the minimum height of a block at which the bit gains its meaning"},

unknown commented at 7:37 pm on February 23, 2021:

nit: “the minimum height of a block at which the bit gains its meaning” can be changed to one of the below

“the minimum height of a block at which the ‘bit’ gains its meaning” “the minimum height of a block at which the ‘bit’ becomes relevant” “the minimum height of a block at which the ‘bit’ used for signaling gains its meaning”

lucasmoten commented at 8:32 pm on February 23, 2021: none

utACK

ghost commented at 8:50 pm on February 23, 2021: none

Concept ACK. Need to do more research and test everything soon.

michaelfolkson commented at 9:22 pm on February 23, 2021: contributor

Based on today’s code review session on IRC I think I can ACK the non-test commits.

I’ve got an assertion error on the feature_bip8.py functional test when run locally which I will look into tomorrow.

AssertionError: [node 3] Expected messages "['bad-vbit-unset-testdummy']" does not partially match log:

I’m a little dubious that forced-signaling logic shouldn’t come with code to handle the network forking (ie more aggressive peer-searching via protocol magic or quicker new connections when the chain has stalled).

I’m not sure there is much we can do in the (worst case) scenario of a chain split due to a proportion of the network running lot=true and a proportion of the network running lot=false during the MUST_SIGNAL phase. I can see there is a desire to do something as it is a worry but I wonder if these suggested P2P changes would potentially do as much harm as good. For example a lot=x node would want to find out if they were on the wrong side of network consensus (lot=y) rather than just pairing with lot=x nodes.

However, I think that even if revised BIP 8 didn’t have the lot=true option there would still be demand for a UASF solution in certain scenarios and hence that is why I originally ACKed the changes to BIP 8 to include a lot=true option. Better to have it within a flexible BIP (with parameters) rather than a UASF effort just grabbing a random BIP off the shelf.

Sjors commented at 3:01 pm on February 24, 2021: member

This or a followup might be a good opportunity to test p2p behavior against older versions, see feature_backwards_compatibility.py for inspiration. You can also use this framework to test the interaction between lockinontimeout true/false nodes. Though I agree with @TheBlueMatt that this PR would be simpler is we only support false for now.

Some commits like bae9a452191a7a83478f7d508a54f4a04d385505 and bd715ff894344408df2345d9bd75a08bd7c9c6f6 could be standalone pull requests. Similarly - though I’m not sure how practical - it might be better to make 1 pull request that strips everything from BIP 9 we don’t need, and then focus this on adding BIP 8.

benthecarman commented at 7:17 pm on February 24, 2021: contributor

ACK 977280c5f04c70c2610ef5fcfde57e5256171f65

ghost commented at 4:29 am on February 25, 2021: none

ACK 977280c5f04c70c2610ef5fcfde57e5256171f65 based on the discussions in yesterday’s IRC chat and some additional code review & running the tests locally.

michaelfolkson commented at 1:48 pm on February 27, 2021: contributor

In light of a project set up to implement a default of lot=true I think Core should implement a default of lot=false with no user configuration option. I also think there should be signposting (comments etc) of where lot=true logic would go in the Core codebase (and referring to the lot parameter in BIP 8) but I don’t think it makes sense to have any lot=true logic in the Core codebase that will remain unused for Taproot activation.

The alternative is Core doesn’t release any activation code and entirely leaves it to non-Core releases. Personally I don’t think this is optimal. I think some users will want to run a Core release that can activate Taproot with miner signaling without being forced to run a non-Core release to get Taproot activated. A significant proportion (unclear whether majority) of the community seems perfectly happy with lot=false as a default and have no desire to change it.

In the worst case scenario of miners failing to activate within say the first 6 months we will need to take the chain split risk at the end the year (2 weeks before timeoutheight) more seriously (and consider providing tooling and communications to Core users well in advance). I think the highly likely scenario in Taproot’s case is miners activate in the first 6 months and the lot parameter ends up being entirely irrelevant.

luke-jr commented at 3:19 pm on February 27, 2021: member

No, the alternative is Core release LOT=true and stay consensus-compatible instead of intentionally harming Core’s users. The latter would be highly inappropriate.

JeremyRubin commented at 4:06 pm on February 27, 2021: contributor

I think that it is a really bad idea to release LOT=false without the ability to trivially set LOT=true. Should LOT=true become widespread, doing things with less time creates risk of a ‘forced error’ on activation. It’s entirely possible that miners will have proprietary mining software that they would need to rebase onto a recompiled client, and that might be a bottleneck. I suppose that LOT=false -> LOT=true patches would be small, but it seems to me much safer to release a client that could maintain consensus with a network that is LOT=true, rather than having to take the hit on the software upgade cycles necessitated by a re-release.

My suggestion (which perhaps belongs on the mailing list!) is that if in aggregate the sentiment of core contributors is that it is a bad idea to release with LOT=true available via configuration, we could make it such that LOT=true being set requires LOTHASH = block hash at height startheight + 6 months of blocks. This way clients provably cannot be configured with LOT=true unless they restart their node with the config parameter determined in the future. This reduces the client “supply chain risk” engendered by making users recompile / switch binaries. edit: while still giving plenty of optimistic time for it to activate ahead of the 6 month mark.

luke-jr commented at 4:44 pm on February 27, 2021: member

Also, as a reminder, this PR is not about setting activation parameters, only merging BIP 8 code.

michaelfolkson commented at 4:48 pm on February 27, 2021: contributor

Also, as a reminder, this PR is not about setting activation parameters, only merging BIP 8 code.

That’s true but we do have Matt and Sjors both commenting that this PR should perhaps only include lot=false code hence the reason for my comment. I wouldn’t seek to add noise for no reason.

luke-jr commented at 5:04 pm on February 27, 2021: member

There is no reason to include LOT=False code and not LOT=True code also. In any case, this PR has sufficient ACKs and is ready to merge today.

TheBlueMatt commented at 7:54 pm on February 27, 2021: member

Let’s keep this discussion to code-level - broad debates about what Core should or shouldn’t include and philosophical debates probably belong on the ML. My comment was that it doesn’t make sense to include highly feature-incomplete code for LOT=true (which is the case in this PR). It may make sense to just merge a LOT=false PR, or to delay this until there is code to properly handle the network splitting case implied by a LOT=true option, but absent at least a strong commitment to fix the issues (which I don’t see happening otherwise), I don’t think it makes sense to merge this as-is.

JeremyRubin commented at 10:27 pm on February 27, 2021: contributor

@luke-jr / @TheBlueMatt do you have any insight on how urgent improving peer selection is in any case? Are the usual mechanisms for dealing with nodes which mine an invalid tip (from the pov of a lot=true) or nodes which send stale headers (pov of lot=false) insufficient?

It’s not immediately obvious to me that the network as-is wouldn’t be tolerably resilient to this sort of issue.

luke-jr commented at 1:17 am on February 28, 2021: member

LOT=True does not differ from any other softfork in respect to handling network divergence. Perhaps the latter can be improved, but it is out of scope to this PR.

benthecarman commented at 1:31 am on February 28, 2021: contributor

LOT=True does not differ from any other softfork in respect to handling network divergence. Perhaps the latter can be improved, but it is out of scope to this PR.

Agree with luke-jr here, if you are running a lot=true client then you deem the old chain invalid

TheBlueMatt commented at 2:44 am on February 28, 2021: member

An option for LOT=true and an expectation that nodes on the network will be running different consensus rules implies something very, very different from any previous fork, even if the code, naively, does not. Indeed, during the Segwit2x/etc debacle some emergency changes were rushed in to hopefully make the P2P network slightly more robust against such things, but they’re fare from an ideal scenario, and if we’re going into such a case with eyes open, there’s no question they should be improved significantly.

luke-jr commented at 3:02 am on February 28, 2021: member

This PR does not add such a (user-accessible) option, nor does it set any expectation for divergent rules.

TheBlueMatt commented at 3:03 am on February 28, 2021: member

I wasn’t leaning on the PR’s current code for my expectation of divergent rules :).

michaelfolkson commented at 10:07 am on March 1, 2021: contributor

ACK 977280c5f04c70c2610ef5fcfde57e5256171f65

I’m getting a little concerned that some individuals will continuously shift the goalposts in search of a philosopher’s stone for an activation mechanism that has 100 percent consensus or a philosopher’s stone to magic away the (unlikely but still possible) risks of a chain split after a year of miners failing to activate.

Anyone who doesn’t like this PR is free to open an alternative PR.

Are the usual mechanisms for dealing with nodes which mine an invalid tip (from the pov of a lot=true) or nodes which send stale headers (pov of lot=false) insufficient? It’s not immediately obvious to me that the network as-is wouldn’t be tolerably resilient to this sort of issue.

This question needs to be answered with more specifics than just “they should be improved significantly.” And then those suggestions should ideally be implemented in an independent PR so there can be discussion on whether they are indeed P2P improvements in a chain split scenario or just additional complexity. That would certainly be more productive than creating a logjam on any activation PRs that are opened.

in src/validation.cpp:3565 in 40fc1c5ff3 outdated

3560+                BIP9Stats stats = VersionBitsStatistics(pindexPrev, consensusParams, deployment_pos);
3561+                if (stats.elapsed == stats.period) {
3562+                    // first block in new period
3563+                    stats.count = stats.elapsed = 0;
3564+                }
3565+                ++stats.elapsed; ++stats.count;

ajtowns commented at 2:11 pm on March 1, 2021:

I don’t think the ++stats.count there is correct – if the final block in the MUST_SIGNAL period does not signal, and the total number of blocks signalling in the period is threshold-1, this will cause lockinontimeout=true nodes to transition from MUST_SIGNAL to LOCKED_IN (and later to ACTIVE) while lockinontimeout=false nodes transition from STARTED to FAILED.

Setting N=107 instead of a random value and removing the random.shuffle(bits) in the test case should duplicate the failure.

TheBlueMatt commented at 3:09 pm on March 1, 2021: member

This question needs to be answered with more specifics than just “they should be improved significantly.”

I did, read above :) More broadly, it seems the discussion on the mailing list and elsewhere is moving beyond BIP 8 as the only option, so probably good to put this on ice until there’s more consensus around what is worth reviewing/merging so there isn’t unused consensus code in Bitcoin Core.

michaelfolkson commented at 4:04 pm on March 1, 2021: contributor

More broadly, it seems the discussion on the mailing list and elsewhere is moving beyond BIP 8

I guess if you don’t engage in the discussion for 6 months as the alternatives are outlined, discussed and narrowed down it might seem that way. From what I see a UASF LOT=true release effort is beginning. But you are free to start the 6 month long activation discussion from scratch in search for your philosopher’s stone if you feel that is a good use of your time.

Sjors commented at 10:31 am on March 2, 2021: member

I’m not opposed to LOT=True code, but I’m (currently) not interested in developing, reviewing or testing it. I might be, if there was a PR specifically dedicated to adding such functionality. But this PR holds back LOT=False support, by making it a package deal.

I’m also opposed to merging this without said review. The ACKs so far are not from people with sufficient expertise to judge soft-fork activation code (I don’t consider myself qualified either).

I’m happy to review LOT=False code. I may also extract a few bits from this PR into separate PR’s so the review burden decreases for everyone.

Jeremy Rubin wrote:

I think that it is a really bad idea to release LOT=false without the ability to trivially set LOT=true.

This is certainly not ideal, but perhaps that suggest a problem with the LOT parameter in BIP 8 introducing too much complexity.

MarcoFalke removed the label Docs on Mar 2, 2021

michaelfolkson commented at 10:45 am on March 2, 2021: contributor

I’m also opposed to merging this without said review. The ACKs so far are not from people with sufficient expertise to judge soft-fork activation code (I don’t consider myself qualified either).

Totally agree. This (or an alternative PR) desperately needs long term contributors reviewing the code. I suspect the reason why they are delaying reviewing the code (or opening an alternative PR) is an unwillingness to get involved in the high level discussion (which has been done to death imo). With the effective NACKs by Core contributors I have seen (we can list them if this is helpful) on releasing a default LOT=true I can’t see any route to Core releasing a default of LOT=true. It is a default of LOT=false or releasing nothing (or another 6-8 months of searching for the philosopher’s stone of course).

I’m happy to review LOT=False code. I may also extract a few bits from this PR into separate PR’s so the review burden decreases for everyone.

This would be great.

in src/test/versionbits_tests.cpp:115 in bae9a45219 outdated

113             if (InsecureRandBits(i) == 0) {
114-                BOOST_CHECK_MESSAGE(checker[i].GetStateFor(vpblock.empty() ? nullptr : vpblock.back()) == ThresholdState::DEFINED, strprintf("Test %i for DEFINED", num));
115-                BOOST_CHECK_MESSAGE(checker_always[i].GetStateFor(vpblock.empty() ? nullptr : vpblock.back()) == ThresholdState::ACTIVE, strprintf("Test %i for ACTIVE (always active)", num));
116+                ThresholdState got = checker[i].GetStateFor(vpblock.empty() ? nullptr : vpblock.back());
117+                ThresholdState got_always = checker_always[i].GetStateFor(vpblock.empty() ? nullptr : vpblock.back());
118+                int height = vpblock.empty() ? 0 : (1 + vpblock.back()->nHeight);

Sjors commented at 11:30 am on March 2, 2021:

bae9a452191a7a83478f7d508a54f4a04d385505 and bd715ff894344408df2345d9bd75a08bd7c9c6f6 : why the + 1? The log will show 0 for the genesis block, but 2 the block at height 1.

ajtowns commented at 11:05 am on March 3, 2021:

height here is the height of the next block – if vpblock is empty, the next (ie first) block should be the genesis block with nHeight == 0. Note that when populating vpblock in VersionBitsTester::Mine() we set nHeight=vblock.size() so the first entry will have nHeight == 0 and act as the genesis block; at which point 1+vpblock.back().nHeight will evaluate to 1+0 giving the height of the next block to be added. When vpblock is empty, it’ll evaluate to 0, still giving the height of the next block to be added.

ajtowns commented at 11:06 am on March 3, 2021:

Note that GetStateFor(block) is telling you what the state should be for the next block to be mined, not the last block that was mined.

Sjors commented at 1:06 pm on March 3, 2021:

I see, I’ll add a comment to that effect in my PR.

Sjors commented at 1:39 pm on March 2, 2021: member

Some additional notes: @TheBlueMatt wrote on the mailinglist:

Bitcoin Core does not have infrastructure to handle switching consensus rules with the same datadir - after running with uasf=true for some time, valid blocks will be marked as invalid, and additional development would need to occur to enable switching back to uasf=false. This is complex, critical code to get right, and the review and testing cycles needed seem to be not worth it.

This scenario can be tested by stopping the -vbparams=testdummy:@{start}:@{stop}:1 node in feature_bip8.py and then restarting it with -vbparams=testdummy:@{start}:@{stop}:0 (and vice versa for someone opting in later.

A more practical solution might be to require a user who changes their mind to resync the chain. But you’d have to store the choice somewhere. We do something similar with regards to tracking if pruning ever happened.

TheBlueMatt commented at 5:55 pm on March 2, 2021: member

I guess if you don’t engage in the discussion for 6 months as the alternatives are outlined, discussed and narrowed down it might seem that way.

Lets keep the debates to the ML. I’m not gonna spam this PR with responses to personal attacks.

This scenario can be tested by stopping the -vbparams=testdummy:@{start}:@{stop}:1 node in feature_bip8.py and then restarting it with -vbparams=testdummy:@{start}:@{stop}:0 (and vice versa for someone opting in later.

Right, I believe AJ mentioned in that thread that it may (mostly) work already (though that certainly needs careful thought!), however there’s not a good way to test the P2P aspects of a network split, and that’s certainly also important to handle for a BIP8-based fork with strong community views.

luke-jr force-pushed on Mar 2, 2021

luke-jr commented at 5:58 pm on March 2, 2021: member

Fixed the bugs @ajtowns and @Sjors found

luke-jr force-pushed on Mar 2, 2021

sdaftuar commented at 9:32 pm on March 2, 2021: member

I’m a Concept NACK on BIP 8 as drafted. I think the MUST_SIGNAL period is needlessly risky for maintaining consensus (as I wrote back in 2017 on the mailing list, responding to a similar proposal at the time); if that part of BIP 8 were dropped I’d be fine with an implementation of the rest, though I haven’t reviewed the code here.

michaelfolkson commented at 9:44 pm on March 2, 2021: contributor

@sdaftuar: I’m sure you already know this but just to ensure clarity. BIP 8 (LOT = false) does not have the MUST_SIGNAL period and hence an implementation of BIP 8 (LOT = false) does not have the MUST_SIGNAL period.

If you want MUST_SIGNAL or the LOT parameter stripped from the actual BIP you’ll need to open a BIP PR. I don’t personally see what that would achieve unless you think LOT=true and MUST_SIGNAL should not be used in any circumstance whatsoever by either Core or an alternative implementation.

Sjors commented at 1:02 pm on March 3, 2021: member

@TheBlueMatt wrote:

there’s not a good way to test the P2P aspects of a network split

We can now test using binaries of earlier releases, so this might be more feasible. But I’d rather see that in a PR that’s specifically focussed on LOT. (my other objections aside)

michaelfolkson commented at 1:03 pm on March 3, 2021: contributor

It appears this PR doesn’t have much (any?) chance of being merged in its current form (ie whilst it has any LOT=true code). Hence I (and others I have discussed it with) think it is best if this PR is closed.

An approach like Sjors’ recent PR #21334 stripping out particular commits seem the best way forward.

I’ll reiterate my view (and I know Luke is strongly opposed) that if Core ships any activation code it will be BIP 8(1 year, false).

I also highly doubt (though people are welcome to try by opening a BIP PR) that the LOT parameter will be stripped out of BIP 8 given the work and the consensus has built around it over the past weeks. But a LOT=false implementation in Core does not need any LOT code.

DrahtBot added the label Needs rebase on Mar 4, 2021

luke-jr force-pushed on Mar 5, 2021

ProofOfKeags commented at 10:58 pm on March 5, 2021: none

@michaelfolkson Are you saying that the LOT=true code can’t even exist, or just that it can’t be enabled for any SF proposals (specifically Taproot)?

Seems like a bizarre requirement considering it is actually in the BIP, but perhaps theres a reason.

michaelfolkson commented at 1:21 pm on March 6, 2021: contributor

@ProofOfKeags: I think it is just semantics. It appears some people don’t want LOT=true code in the Core codebase. Obviously it would be dead code if Core implemented the equivalent of LOT=false so it really doesn’t matter. The Core codebase and the BIP (BIP 8) are separate objects. I can’t see LOT=true code being merged into Core but I also can’t see LOT or MUST_SIGNAL getting taken out of the BIP.

Having said that I didn’t think there would be an alternative proposal (“Speedy trial”) gather momentum at this late stage so please take any/all of my predictions with a large pinch of salt :)

achow101 commented at 9:23 pm on March 6, 2021: member

If 31eac50c721dd3b0bd2347e76196bf16913e9be9 (from #21055) is backported to 0.21, then this PR rebased on master will also backport to 0.21 cleanly.

in src/versionbits.cpp:206 in cf53e6e65c outdated

202@@ -203,9 +203,9 @@ ThresholdState VersionBitsState(const CBlockIndex* pindexPrev, const Consensus::
203     return VersionBitsConditionChecker(pos).GetStateFor(pindexPrev, params, cache.caches[pos]);
204 }
205 
206-VBitsStats VersionBitsStatistics(const CBlockIndex* pindexPrev, const Consensus::Params& params, Consensus::DeploymentPos pos)
207+VBitsStats VersionBitsStatistics(const CBlockIndex* pindex, const Consensus::Params& params, Consensus::DeploymentPos pos)

ajtowns commented at 9:45 am on March 7, 2021:

Sorry, I think this change is wrong and it should be the other way around – AbstractThresholdConditionChecker::GetStateStatisticsFor should be expecting pindexPrev not pindex. I believe that gives the right behaviour for MUST_SIGNAL. I’d suggest just dropping this commit.

MarcoFalke referenced this in commit 1020b04c39 on Mar 7, 2021

sidhujag referenced this in commit 17f4c4d8cc on Mar 7, 2021

jtimon commented at 3:47 pm on March 8, 2021: contributor

If I’m understanding the code correctly at a glance, this also gets rid of the LOT=false option and instead provides an option to never activate the change regardless of signaling, which in my opinion makes more sense as it gives users a more meaningful option than “do you want to allow miners to have veto powers over the change and potentially fork off from other users who like you also want this change to be applied?”. Giving users the option of giving miners veto power doesn’t empower users at all. if you’re super worried about “perception”, then give users the option to be against the change directly.

Concept ACK.

But I would actually go further and for the users who select that option, not only they should not activate the change, but also consider the block that activates the change invalid, so that they can cleanly fork off to their own chain without the change they oppose.

luke-jr commented at 4:26 am on March 14, 2021: member

This is being split up and merged in pieces (some modified), so closing it.

luke-jr closed this on Mar 14, 2021

chainparams: make versionbits threshold per-deployment 2be730c31d

Migrate versionbits to use height instead of MTP

Co-authored-by: Anthony Towns <aj@erisian.com.au>

c95287e378

Rename user facing mentions of BIP 9 to versionbits and/or BIP 8

Co-authored-by: Anthony Towns <aj@erisian.com.au>

8f55573ed3

Add minimum activation height to BIP9Deployments 1dac84b5de

tests: test versionbits delayed activation 73d2756244

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

test: add min_activation_height to -vbparams 91b2e4b4f9

test: BIP 8 delayed activation functional test 2e55bcedb8

luke-jr reopened this on Apr 7, 2021

luke-jr force-pushed on Apr 7, 2021

Add BIP 8 lockinontimeout flag and MUST_SIGNAL phase for versionbits 045fff1b49

Enforce mandatory signalling during BIP 8 MUST_SIGNAL phase

Co-authored-by: Anthony Towns <aj@erisian.com.au>

1b6a36a9b0

vbparams: require lockinontimeout val 57199cbd00

tests: add functional test for bip8 activation

Co-authored-by: Luke Dashjr <luke-jr+git@utopios.org>

0a8b9745ec

luke-jr force-pushed on Apr 7, 2021

DrahtBot removed the label Needs rebase on Apr 7, 2021

luke-jr commented at 10:13 pm on April 7, 2021: member

Rebased (CI failure appears unrelated)

laanwj added this to the "Blockers" column in a project

DrahtBot commented at 4:00 am on April 15, 2021: member

🐙 This pull request conflicts with the target branch and needs rebase.

Want to unsubscribe from rebase notifications on this pull request? Just convert this pull request to a “draft”.

DrahtBot added the label Needs rebase on Apr 15, 2021

MarcoFalke closed this on Apr 15, 2021

MarcoFalke removed this from the "Blockers" column in a project

achow101 commented at 5:38 am on April 15, 2021: member

Note that we may still want to use this in the future. Merging #21377 now does not preclude the use of BIP 8 in a future deployment.

MarcoFalke commented at 6:03 am on April 15, 2021: member

Ok, let me know when to reopen, but I don’t think this needs to stay in “High-priority for review”

Sjors commented at 7:12 am on April 15, 2021: member

Agreed, it’s probably a good idea to keep working on BIP 8 between now and the next soft fork attempt (especially the LOT=false part). That seems better than to develop these mechanisms only just in time.

But in order to keep backporting easy, I suggest we don’t merge a BIP 8 implementation until Speedy Trial expires or locks in.

MarcoFalke reopened this on Apr 15, 2021

luke-jr closed this on Apr 15, 2021

luke-jr commented at 3:22 pm on April 15, 2021: member

Thinking about it, it doesn’t make sense to continue this PR here. This has LOT=True support and such, and what is needed right now is simply heights.

When/if the folks impeding progress of BIP 8 change their mind, I can do that. But not sure there’s any point rebasing until then.

DrahtBot locked this on Aug 16, 2022

Replace unused BIP 9 logic with draft BIP 8 #19573

Conflicts