Introduce deploymentstatus

ajtowns commented at 5:03 pm on July 3, 2020: member

Introduces helper functions to make it easy to bury future deployments, along the lines of the suggestion from 11398 “I would prefer it if a buried deployment wouldn’t require all code paths that check the BIP9 status to require changing”.

This provides three functions: DeploymentEnabled() which tests if a deployment can ever be active, DeploymentActiveAt() which checks if a deployment should be enforced in the given block, and DeploymentActiveAfter() which checks if a deployment should be enforced in the block following the given block, and overloads all three to work both with buried deployments and versionbits deployments.

This adds a dedicated lock for the versionbits cache, which is acquired internally by the versionbits functions, rather than relying on cs_main. It also moves moves versionbitscache into deploymentstatus to avoid a circular dependency with validation.

ajtowns added the label Refactoring on Jul 3, 2020

ajtowns added the label Consensus on Jul 3, 2020

DrahtBot commented at 7:35 pm on July 3, 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:

#22371 by MarcoFalke
#22242 by MarcoFalke
#21702 by JeremyRubin
#21430 by hebasto
#21090 by dhruv

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.

in src/consensus/params.h:14 in 5834cc8654 outdated

10@@ -11,6 +11,15 @@
11 
12 namespace Consensus {
13 
14+enum BuriedDeployment

JeremyRubin commented at 8:10 pm on July 3, 2020:

prefer enum class to prevent leaking names.

JeremyRubin commented at 8:10 pm on July 3, 2020:

Also helps with avoiding overlap with SignalledDeployment.

ajtowns commented at 9:14 pm on July 3, 2020:

Leaking the names is the point – otherwise you’d need to change every use from Consensus::DeploymentPos::SEGWIT to Consensus::BuriedDeployment::SEGWIT. With C++20, you will be able to specify enum class DeploymentPos { .. }; using enum DeploymentPos; but that’s a while away.

in src/consensus/params.h:21 in 5834cc8654 outdated

15+{
16+    // buried deployments get negative values to avoid overlap with SignalledDeployment
17+    DEPLOYMENT_CLTV = -255,
18+    DEPLOYMENT_DERSIG,
19+    DEPLOYMENT_CSV,
20+    DEPLOYMENT_SEGWIT,

JeremyRubin commented at 8:11 pm on July 3, 2020:

prefer explicitly numbering these.

-255 is an extremely odd number, can you clarify why you picked it? It doesn’t fit in a signed byte.

ajtowns commented at 10:16 pm on July 3, 2020:

Explicitly numbering them risks accidentally repeating a number. No particular objection to a different starting point. Could perhaps make it enum DeploymentPos : uint8_t and enum BuriedDeployment : int16_t { DEPLOYMENT_CLTV = 256, // larger than largest possible DeploymentPos value.

(I guess 2**n is an “extremely even” number, so calling +/- 2**n-1 “extremely odd” makes a lot of sense…)

in src/consensus/params.h:116 in 5834cc8654 outdated

88@@ -80,7 +89,19 @@ struct Params {
89     int64_t DifficultyAdjustmentInterval() const { return nPowTargetTimespan / nPowTargetSpacing; }
90     uint256 nMinimumChainWork;
91     uint256 defaultAssumeValid;
92+
93+    inline int DeploymentHeight(BuriedDeployment dep) const

JeremyRubin commented at 8:13 pm on July 3, 2020:

Maybe tighter to make this a templated function as we’re never calling DeploymentHeight with a variable, only literal.

Then we get not just a warning, but a compiler error.

ajtowns commented at 9:29 pm on July 3, 2020:

I had tried that, but thought that it might be useful to be able to do for (dep = 0; dep < MAX_VERSIONBITS_DEPLOYMENTS; ++dep) { ... DeploymentActiveAt(pindex, consParams, dep) .. }. I think in all the cases where that currently comes up you want to know more fine grained details though. (Changing BuriedForkDescPushBack to take a BuriedDeployment instead of a height would make use of that, eg)

I don’t think the type checking is any better with the template though, as unfortunately you can invoke DeploymentHeight<(BuriedDeployment)9000>() just as easily as DeploymentHeight((BuriedDeployment)9000) with both g++ and clang++.

in src/deploymentstatus.h:1 in 5834cc8654 outdated

0@@ -0,0 +1,53 @@
1+// Copyright (c) 2016-2019 The Bitcoin Core developers

JeremyRubin commented at 8:15 pm on July 3, 2020:

dates

in src/deploymentstatus.h:17 in 5834cc8654 outdated

15+
16+/** Global cache for versionbits deployment status */
17+extern VersionBitsCache versionbitscache GUARDED_BY(cs_main);
18+
19+/** Determine if a deployment is active for the next block */
20+inline bool DeploymentActiveAfter(const CBlockIndex* pindexPrev, const Consensus::Params& params, Consensus::BuriedDeployment dep)

JeremyRubin commented at 8:18 pm on July 3, 2020:

This dispatch scares me a bit unless we use enum class – enums could coerce poorly right?

ajtowns commented at 9:38 pm on July 3, 2020:

Nope; int (and the like) won’t automatically coerce to enum, so with int x = Consensus::DEPLOYMENT_SEGWIT; return DeploymentActiveAt(pindex, params, x) you get errors:

0validation.cpp:1921:12: error: no matching function for call to 'DeploymentActiveAt'
1    return DeploymentActiveAt(pindex, consensusparams, x);
2           ^~~~~~~~~~~~~~~~~~
3./deploymentstatus.h:32:13: note: candidate function not viable: no known conversion from 'int' to 'Consensus::BuriedDeployment' for 3rd argument
4inline bool DeploymentActiveAt(const CBlockIndex* pindex, const Consensus::Params& params, Consensus::BuriedDeployment dep)
5            ^
6./deploymentstatus.h:37:13: note: candidate function not viable: no known conversion from 'int' to 'Consensus::DeploymentPos' for 3rd argument
7inline bool DeploymentActiveAt(const CBlockIndex* pindex, const Consensus::Params& params, Consensus::DeploymentPos dep) EXCLUSIVE_LOCKS_REQUIRED(cs_main)
8            ^

(even if it did, which enum it coerced to is ambiguous between the two types, resulting in a failure like error: call to 'DeploymentActiveAt' is ambiguous)

JeremyRubin dismissed

JeremyRubin commented at 8:24 pm on July 3, 2020: contributor

Concept ACK and some lite code review feedback.

ajtowns force-pushed on Jul 3, 2020

DrahtBot added the label Needs rebase on Sep 7, 2020

ajtowns force-pushed on Sep 8, 2020

DrahtBot removed the label Needs rebase on Sep 8, 2020

DrahtBot added the label Needs rebase on Sep 8, 2020

ajtowns force-pushed on Sep 8, 2020

DrahtBot removed the label Needs rebase on Sep 8, 2020

DrahtBot added the label Needs rebase on Sep 21, 2020

ajtowns force-pushed on Sep 22, 2020

DrahtBot removed the label Needs rebase on Sep 22, 2020

DrahtBot added the label Needs rebase on Oct 15, 2020

ajtowns force-pushed on Oct 17, 2020

DrahtBot removed the label Needs rebase on Oct 17, 2020

in src/deploymentstatus.h:14 in 60de491a03 outdated

 9+#include <sync.h>
10+#include <versionbits.h>
11+
12+#include <limits>
13+
14+extern RecursiveMutex cs_main;

sipa commented at 5:00 am on December 27, 2020:

Any chance of just introducing a new lock for the versionbitscache, to avoid this semantically-cyclic dependency between validation and deploymentstatus?

ajtowns commented at 0:45 am on December 30, 2020:

Yeah, can make VersionBitsCache be natively threadsafe, rather than having the caller manage a mutex (which should help if it ever ends up having some sort of shared lock too). I’ve done that, though I’ve added the lock as the first commit, since I think that makes the impact of the change clearer.

sipa commented at 5:01 am on December 27, 2020: member

utACK 60de491a030011f3caeb42f0b650a1028905c54d

ajtowns force-pushed on Dec 30, 2020

in src/deploymentstatus.h:24 in 318370aa0e outdated

19+inline bool DeploymentActiveAt(const CBlockIndex* pindex, const Consensus::Params& params, Consensus::BuriedDeployment dep)
20+{
21+    return pindex->nHeight >= params.DeploymentHeight(dep);
22+}
23+
24+/** Determine if a deployment is enabled (can ever be active */

sipa commented at 7:16 am on December 30, 2020:

In commit “[refactor] Add deploymentstatus.h”:

YUUUUGE nit: missing parenthesis in comment

in src/validation.cpp:3451 in 318370aa0e outdated

3447@@ -3461,13 +3448,13 @@ static bool ContextualCheckBlockHeader(const CBlockHeader& block, BlockValidatio
3448  *  in ConnectBlock().
3449  *  Note that -reindex-chainstate skips the validation that happens here!
3450  */
3451-static bool ContextualCheckBlock(const CBlock& block, BlockValidationState& state, const Consensus::Params& consensusParams, const CBlockIndex* pindexPrev)
3452+static bool ContextualCheckBlock(const CBlock& block, BlockValidationState& state, const Consensus::Params& consensusParams, const CBlockIndex* pindexPrev) EXCLUSIVE_LOCKS_REQUIRED(cs_main)

sipa commented at 7:20 am on December 30, 2020:

In commit “[refactor] Add deploymentstatus.h”:

Is this “EXCLUSIVE_LOCKS_REQUIRED(cs_main)” still needed?

ajtowns commented at 7:58 am on January 2, 2021:

No, don’t think so. Removed.

in src/deploymentstatus.h:24 in a323cdf30e outdated

19     return (pindexPrev == nullptr ? 0 : pindexPrev->nHeight + 1) >= params.DeploymentHeight(dep);
20 }
21 
22+inline bool DeploymentActiveAfter(const CBlockIndex* pindexPrev, const Consensus::Params& params, Consensus::DeploymentPos dep)
23+{
24+    return ThresholdState::ACTIVE == VersionBitsState(pindexPrev, params, dep, versionbitscache);

sipa commented at 7:25 am on December 30, 2020:

In commit “[refactor] Add versionbits deployments to deploymentstatus.h”:

Perhaps assert that dep >= 0 in these functions (and dep < 0 for the buried ones)?

ajtowns commented at 7:58 am on January 2, 2021:

I’ve added a ValidDeployment() function for checking this with a bit more precision.

Might be worth revisiting @JeremyRubin’s suggestion of using templated functions so it can be a static assertion that’s resolved at compile-time? Having looked at it again, I think the only thing that would really end up making awkward is the rpc/blockchain.cpp stuff (ie, it would mean templating the SoftForkPushBack function and having the compiler duplicate the code and possibly not de-duplicate it when optimising) but that’s not necessarily a big deal.

I’ve added a draft commit that switches to templated functions and static_asserts to see what that’s like. It does make rpc/blockchain a bit bigger (after compiling with clang) so might need be worth some tweaking to avoid that, and the syntax is a bit clunky, but it seems plausible. Thoughts?

ajtowns commented at 3:42 am on January 11, 2021:

Okay, dropped the templated approach as too unwieldy for not enough benefit, so think this is resolved via ValidDeployment().

sipa commented at 7:28 am on December 30, 2020: member

utACK 545930482da83953829308c131f173504374c569

You may want to update the PR description regarding the locking change.

sipa commented at 8:55 pm on December 30, 2020: member

@JeremyRubin Have your comments been addressed?

DrahtBot added the label Needs rebase on Dec 31, 2020

ajtowns force-pushed on Jan 2, 2021

ajtowns commented at 8:07 am on January 2, 2021: member

Rebased to after the copyright bump, addressed comments, added a draft commit that switches from runtime to static checks.

DrahtBot removed the label Needs rebase on Jan 2, 2021

ajtowns force-pushed on Jan 11, 2021

ajtowns commented at 3:41 am on January 11, 2021: member

Dropped draft commit that made the ValidDeployment checks static_asserts.

DrahtBot added the label Needs rebase on Feb 1, 2021

ajtowns force-pushed on Feb 2, 2021

ajtowns commented at 9:11 am on February 2, 2021: member

Rebased on top of #20749 due to conflict from both PRs changing nearby sections of validation.h

DrahtBot removed the label Needs rebase on Feb 2, 2021

DrahtBot added the label Needs rebase on Feb 18, 2021

ajtowns force-pushed on Feb 25, 2021

DrahtBot removed the label Needs rebase on Feb 25, 2021

DrahtBot added the label Needs rebase on Mar 4, 2021

ajtowns force-pushed on Mar 4, 2021

DrahtBot removed the label Needs rebase on Mar 4, 2021

in src/deploymentstatus.cpp:17 in 073ae5b33c outdated

12+/* Basic sanity checking for BuriedDeployment/DeploymentPos enums and
13+ * ValidDeployment check */
14+
15+static_assert(ValidDeployment(Consensus::DEPLOYMENT_TESTDUMMY), "sanity check of DeploymentPos failed (TESTDUMMY not valid)");
16+static_assert(!ValidDeployment(Consensus::MAX_VERSION_BITS_DEPLOYMENTS), "sanity check of DeploymentPos failed (MAX value considered valid)");
17+static_assert(!ValidDeployment(static_cast<Consensus::BuriedDeployment>(Consensus::DEPLOYMENT_TESTDUMMY)), "sanity check of BuridedDeployment failed (overlaps with DeploymentPos)");

amitiuttarwar commented at 1:15 am on March 9, 2021:

typo: BuridedDeployment

jnewbery commented at 6:22 pm on March 9, 2021: member

Concept ACK. I really like that deployments can be changed from versionbits to buried without any changes to the code in validation and other places.

in src/consensus/params.h:16 in 3a32ec4a05 outdated

10@@ -11,13 +11,25 @@
11 
12 namespace Consensus {
13 
14-enum DeploymentPos
15+enum BuriedDeployment : int16_t
16+{
17+    // buried deployments get negative values to avoid overlap with SignalledDeployment

amitiuttarwar commented at 10:01 pm on March 10, 2021:

SignalledDeployment == DeploymentPos?

although, I’d probably be down for a rename. I’ve been wondering what Pos stands for? position?

ajtowns commented at 0:02 am on March 11, 2021:

“position in the vDeployments” array, I presume. Yeah, I think I changed it to “SignalledDeployment” at some point then dropped it as being too annoying to keep rebasing

in src/consensus/params.h:29 in 3a32ec4a05 outdated

25+
26+enum DeploymentPos : uint16_t
27 {
28     DEPLOYMENT_TESTDUMMY,
29     DEPLOYMENT_TAPROOT, // Deployment of Schnorr/Taproot (BIPs 340-342)
30     // NOTE: Also add new deployments to VersionBitsDeploymentInfo in versionbits.cpp

amitiuttarwar commented at 10:11 pm on March 10, 2021:

I think this comment was previously incorrect (VersionBitsDeploymentInfo was in versionbitsinfo.cpp), but should definitely be updated since as of this PR it is defined in deploymentinfo.cpp

amitiuttarwar commented at 10:13 pm on March 10, 2021: contributor

just a couple doc notes for now. I’ve started reviewing, but it’s going to take a while :)

ajtowns commented at 4:09 am on March 11, 2021: member

Probably a good idea to review #21380 (Fuzzing harness for versionbits) before this one – ~it has some refactors that will cause this to get rebased, and both the “speedy trial” implementations are based on it.~

ajtowns force-pushed on Mar 11, 2021

ajtowns commented at 6:21 am on March 11, 2021: member

Split deploymentinfo rename, DeploymentName and SoftForkPushBack into separate commits. (EDIT: and fix the comments amiti noted)

ajtowns force-pushed on Mar 11, 2021

in src/versionbits.h:78 in eb211ea0af outdated

76- *  keyed by the bit position used to signal support. */
77-struct VersionBitsCache
78+/** BIP 9 allows multiple softforks to be deployed in parallel. We cache
79+ *  per-period state for every one of them. */
80+
81+class VersionBitsCache

jnewbery commented at 10:08 am on March 15, 2021:

It seems a bit strange for this interface to be called ‘cache’. I’d say this has a cache, not is a cache.

ajtowns commented at 4:22 am on March 17, 2021:

I don’t think I agree – if there wasn’t a cache, there would be no need for an object, these things would just be standalone functions, so calling the object a cache makes sense to me.

There’s some impedence mismatch though – apart from having to have the object, the api is otherwise pretty independent of whether there’s a cache or not.

jnewbery commented at 9:23 am on March 17, 2021:

apart from having to have the object, the api is otherwise pretty independent of whether there’s a cache or not.

Indeed. The client doesn’t care whether there’s a cache or not (and for the static functions, there isn’t even a cache). Generally, I think interfaces should be named according to the client usage, not the implementation specifics. Not a big deal, and shouldn’t hold up this PR.

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

The client does care a little about whether there’s a cache – without it, checking the state is potentially linear in the height of the block being tested, with it, it’s amortized constant. Particularly for deployments with short periods (like bip91) that could be prohibitive.

Perhaps it’s worth considering renaming it to SignalledDeploymentManager or similar along with renaming from BIP9 to VBits or renaming DeploymentPos or a deeper refactoring of AbstractThresholdConditionChecker etc.

jnewbery commented at 9:53 am on March 17, 2021:

I’d prefer a generic name like SignalledDeploymentManager, but it definitely shouldn’t hold up this PR.

jnewbery commented at 9:55 am on March 17, 2021:

The client does care a little about whether there’s a cache

Sorry, I meant in terms of correctness/observability. Of course a cache has implications for computational complexity.

in src/versionbits.h:82 in eb211ea0af outdated

81+class VersionBitsCache
82 {
83-    ThresholdConditionCache caches[Consensus::MAX_VERSION_BITS_DEPLOYMENTS];
84+private:
85+    Mutex cs;
86+    ThresholdConditionCache caches[Consensus::MAX_VERSION_BITS_DEPLOYMENTS] GUARDED_BY(cs);

jnewbery commented at 10:12 am on March 15, 2021:

Couple of suggested changes:

use m_ naming convention (and cs -> mutex)
mark these mutable so the functions can all be const

0    mutable Mutex m_mutex;
1    mutable ThresholdConditionCache m_caches[Consensus::MAX_VERSION_BITS_DEPLOYMENTS] GUARDED_BY(cs);

ajtowns commented at 5:42 am on March 17, 2021:

Names changed; left as non-mutable and non-const.

in src/versionbits.h:88 in eb211ea0af outdated

87+
88+public:
89+    /** Get the numerical statistics for the BIP9 state for a given deployment as at pindexPrev. */
90+    static BIP9Stats Statistics(const CBlockIndex* pindexPrev, const Consensus::Params& params, Consensus::DeploymentPos pos);
91+
92+    static uint32_t Mask(const Consensus::Params& params, Consensus::DeploymentPos pos);

jnewbery commented at 10:15 am on March 15, 2021:

What’s the benefit of specifically these two functions being static? There will only ever be one instance of VersionBitsCache so theoretically everything here could be static. I’d suggest just dropping the static here.

sipa commented at 11:32 pm on March 16, 2021:

@jnewbery The class does have members variables, and the non-static member function do access them, so unless those variables become static too (which would be rather ugly), the other member functions can’t be.

I think it doesn’t hurt to mark the function that don’t need state static.

ajtowns commented at 5:26 am on March 17, 2021:

I think this is the same impedence mismatch as “calling it a cache” – they’re static because they don’t use the cache.

in src/versionbits.cpp:220 in eb211ea0af outdated

216+int32_t VersionBitsCache::ComputeBlockVersion(const CBlockIndex* pindexPrev, const Consensus::Params& params)
217+{
218+    LOCK(cs);
219+    int32_t nVersion = VERSIONBITS_TOP_BITS;
220+
221+    for (int i = 0; i < (int)Consensus::MAX_VERSION_BITS_DEPLOYMENTS; i++) {

jnewbery commented at 10:44 am on March 15, 2021:

You can use the underlying type of the enum rather than casting it to an int:

0    for (std::underlying_type<Consensus::DeploymentPos>::type i = 0; i < Consensus::MAX_VERSION_BITS_DEPLOYMENTS; i++) {

ajtowns commented at 4:50 am on March 17, 2021:

We cast to an int to iterate in a few places, and changing them feels gratuitous – we can’t make the underlying type something that won’t fit in an int because the fixed arrays we index into via the enum value would break too.

jnewbery commented at 9:21 am on March 17, 2021:

ok

in src/validation.cpp:3460 in eb211ea0af outdated

3457@@ -3491,11 +3458,12 @@ static bool ContextualCheckBlockHeader(const CBlockHeader& block, BlockValidatio
3458 
3459     // Reject outdated version blocks when 95% (75% on testnet) of the network has upgraded:
3460     // check for version 2, 3 and 4 upgrades

jnewbery commented at 10:49 am on March 15, 2021:

Comment can be removed. We’re no longer interested in the activation method now that these deployments are buried.

in src/validation.cpp:1876 in eb211ea0af outdated

1875-    if (pindex->nHeight >= consensusparams.CSVHeight) {
1876+    if (DeploymentActiveAt(pindex, consensusparams, Consensus::DEPLOYMENT_CSV)) {
1877         flags |= SCRIPT_VERIFY_CHECKSEQUENCEVERIFY;
1878     }
1879 
1880     // Start enforcing Taproot using versionbits logic.

jnewbery commented at 10:50 am on March 15, 2021:

0    // Start enforcing Taproot.

Actually, all of these comments can have s/Start enforcing/Enforce/.

in src/deploymentstatus.cpp:7 in eb211ea0af outdated

0@@ -0,0 +1,17 @@
1+// Copyright (c) 2020 The Bitcoin Core developers
2+// Distributed under the MIT software license, see the accompanying
3+// file COPYING or http://www.opensource.org/licenses/mit-license.php.
4+
5+#include <deploymentstatus.h>
6+#include <versionbits.h>
7+
8+#include <consensus/params.h>

jnewbery commented at 11:27 am on March 15, 2021:

0
1#include <consensus/params.h>
2#include <versionbits.h>

in src/deploymentinfo.h:10 in eb211ea0af outdated

 5+#ifndef BITCOIN_DEPLOYMENTINFO_H
 6+#define BITCOIN_DEPLOYMENTINFO_H
 7+
 8+#include <consensus/params.h>
 9+
10+#include <string.h>

jnewbery commented at 11:32 am on March 15, 2021:

0#include <string>

(string.h is the c string library: https://en.cppreference.com/w/cpp/header/cstring)

jnewbery commented at 11:46 am on March 15, 2021: member

ACK 58257b9a7ba841c1b569127bb05ca606780f853b

A few minor suggestions inline.

amitiuttarwar commented at 8:27 pm on March 16, 2021: contributor

approach ACK, I like the strategy of using enums with safety checks + overloaded functions to abstract buried vs future deployments

in src/versionbits.h:84 in 898d54177d outdated

78@@ -79,11 +79,11 @@ struct VersionBitsCache
79     void Clear();
80 };
81 
82-/** Get the BIP9 state for a given deployment at the current tip. */
83+/** Get the BIP9 state for a given deployment for the block after pindexPrev. */
84 ThresholdState VersionBitsState(const CBlockIndex* pindexPrev, const Consensus::Params& params, Consensus::DeploymentPos pos, VersionBitsCache& cache);
85-/** Get the numerical statistics for the BIP9 state for a given deployment at the current tip. */
86+/** Get the numerical statistics for the BIP9 state for a given deployment as at pindexPrev. */

sipa commented at 11:04 pm on March 16, 2021:

In “versionbits: correct doxygen comments”

Nit: perhaps it’s worth stopping using pindexPrev here if it’s giving statistics for the specified block rather than its successor?

ajtowns commented at 11:55 pm on March 16, 2021:

It’s giving the statistics for the retarget period that includes pindexPrev’s successor, up to and including pindexPrev if pindexPrev is in the same retarget period, but not if pindexPrev was the last block in the previous retarget period. (That means you can’t actually query the statistics for a fully completed retarget period – you’ll always get statistics for between 0 and 2015 blocks worth of signalling, never the full 2016 blocks. Or whatever the period is)

I’ve been confused about this until recently, so not sure how to describe it well

sipa commented at 4:29 am on March 17, 2021:

It could just say “Get statistics for the BIP9 state for the measuring window that inlcudes pindexPrev’s successor.” ?

Feel free to disregard this.

sipa commented at 11:35 pm on March 16, 2021: member

Concept/approach/code review ACK; there are a bunch of nits by @jnewbery and @amitiuttarwar left that seem worth addressing.

ajtowns force-pushed on Mar 17, 2021

ajtowns commented at 5:41 am on March 17, 2021: member

Nits picked.

jnewbery commented at 9:24 am on March 17, 2021: member

code review ACK e72e062e5a8279864d746776dc9072c112ddc014

in src/deploymentstatus.h:23 in e72e062e5a outdated

18+{
19+    assert(Consensus::ValidDeployment(dep));
20+    return (pindexPrev == nullptr ? 0 : pindexPrev->nHeight + 1) >= params.DeploymentHeight(dep);
21+}
22+
23+inline bool DeploymentActiveAfter(const CBlockIndex* pindexPrev, const Consensus::Params& params, Consensus::DeploymentPos dep)

GeneFerneau commented at 2:22 am on March 22, 2021:

nit: With all these new functions taking a const CBlockIndex* ... argument, maybe use a const CBlockIndex& ... instead. Safer API, and can still use &pindexPrev to get a pointer (if needed).

ajtowns commented at 3:38 am on March 22, 2021:

Could make sense to have DeploymentActiveAt(const CBlockIndex&, ...) but DeploymentActiveAfter can be called with nullptr, so can’t be a reference.

GeneFerneau commented at 2:22 am on March 22, 2021: none

code review ACK

0xB10C commented at 5:23 pm on March 22, 2021: member

Concept ACK. Started reviewing

in src/consensus/params.h:32 in 8b2aa89f9f outdated

28     DEPLOYMENT_TESTDUMMY,
29     DEPLOYMENT_TAPROOT, // Deployment of Schnorr/Taproot (BIPs 340-342)
30     // NOTE: Also add new deployments to VersionBitsDeploymentInfo in versionbits.cpp
31     MAX_VERSION_BITS_DEPLOYMENTS
32 };
33+constexpr bool ValidDeployment(DeploymentPos dep) { return DEPLOYMENT_TESTDUMMY <= dep && dep <= DEPLOYMENT_TAPROOT; }

0xB10C commented at 11:44 am on March 24, 2021:

in 8b2aa89f9f6366cfd3a3dfccf2f7678740b4d0ea

nit: couldn’t this be < MAX_VERSION_BITS_DEPLOYMENTS to avoid changing this line when adding/burring a deployment?

0constexpr bool ValidDeployment(DeploymentPos dep) { return DEPLOYMENT_TESTDUMMY <= dep && dep < MAX_VERSION_BITS_DEPLOYMENTS; }

in src/consensus/params.h:23 in 994b0ce58f outdated

18+    DEPLOYMENT_CLTV,
19+    DEPLOYMENT_DERSIG,
20+    DEPLOYMENT_CSV,
21+    DEPLOYMENT_SEGWIT,
22+};
23+constexpr bool ValidDeployment(BuriedDeployment dep) { return DEPLOYMENT_HEIGHTINCB <= dep && dep <= DEPLOYMENT_SEGWIT; }

fjahr commented at 4:09 pm on April 3, 2021:

very minor nit: If you add a DEPLOYMENT_BURIED_MAX and then use dep < DEPLOYMENT_BURIED_MAX instead of dep <= DEPLOYMENT_SEGWIT then this line wouldn’t need to change when something is buried. But I am not sure if it’s an improvement overall.

ajtowns commented at 9:36 am on April 16, 2021:

Didn’t want to add a DEPLOYMENT_BURIED_MAX since that would then need to be included in any switch statements.

in src/deploymentstatus.h:20 in 994b0ce58f outdated

11+
12+/** Determine if a deployment is active for the next block */
13+inline bool DeploymentActiveAfter(const CBlockIndex* pindexPrev, const Consensus::Params& params, Consensus::BuriedDeployment dep)
14+{
15+    assert(Consensus::ValidDeployment(dep));
16+    return (pindexPrev == nullptr ? 0 : pindexPrev->nHeight + 1) >= params.DeploymentHeight(dep);

fjahr commented at 4:28 pm on April 3, 2021:

nit: You could do !pindexPrev instead of == nullptr I think

in src/consensus/params.h:130 in 994b0ce58f outdated

114+        case DEPLOYMENT_CSV:
115+            return CSVHeight;
116+        case DEPLOYMENT_SEGWIT:
117+            return SegwitHeight;
118+        } // no default case, so the compiler can warn about missing cases
119+        return std::numeric_limits<int>::max();

fjahr commented at 4:34 pm on April 3, 2021:

nit: Could use an optional as return value instead of this to signal if no height was found?

ajtowns commented at 9:56 am on April 5, 2021:

That would mean checking and dereferencing the optional at every call site, which doesn’t seem useful.

MarcoFalke commented at 6:06 pm on July 1, 2021:

Or maybe turn this dead code into an assert(false)? Crashing the node should be preferred over a consensus failure.

in src/deploymentstatus.h:45 in 994b0ce58f outdated

24+}
25+
26+/** Determine if a deployment is enabled (can ever be active) */
27+inline bool DeploymentEnabled(const Consensus::Params& params, Consensus::BuriedDeployment dep)
28+{
29+    assert(Consensus::ValidDeployment(dep));

fjahr commented at 4:36 pm on April 3, 2021:

nit: This check is repeated three times only to protect DeploymentHeight() afaict, maybe move it into DeploymentHeight()?

in src/versionbits.h:78 in e72e062e5a outdated

71@@ -70,21 +72,32 @@ class AbstractThresholdConditionChecker {
72     int GetStateSinceHeightFor(const CBlockIndex* pindexPrev, const Consensus::Params& params, ThresholdConditionCache& cache) const;
73 };
74 
75-/** BIP 9 allows multiple softforks to be deployed in parallel. We cache per-period state for every one of them
76- *  keyed by the bit position used to signal support. */
77-struct VersionBitsCache
78+/** BIP 9 allows multiple softforks to be deployed in parallel. We cache
79+ *  per-period state for every one of them. */
80+

fjahr commented at 4:59 pm on April 3, 2021:

nit: accidental empty line?

fjahr commented at 5:29 pm on April 3, 2021: member

Code review ACK e72e062e5a8279864d746776dc9072c112ddc014

Left a couple of nits, feel free to ignore unless you have to retouch.

fanquake deleted a comment on Apr 10, 2021

DrahtBot added the label Needs rebase on Apr 13, 2021

ajtowns force-pushed on Apr 16, 2021

DrahtBot removed the label Needs rebase on Apr 16, 2021

ajtowns commented at 9:34 am on April 16, 2021: member

Rebased on top of #21666 (neighbouring line changed) and the ComputeBlockVersion tests in #21377. Changed DeploymentActiveAt() to take a reference to a block rather than a pointer, since nullptr isn’t valid. Also split the move of ComputeBlockVersion into two steps, one for the move-only part and the other for the refactoring, since that seems a bit easier to review.

DrahtBot added the label Needs rebase on Apr 17, 2021

ajtowns force-pushed on Apr 19, 2021

ajtowns commented at 3:00 am on April 19, 2021: member

Rebased on top of #21391 due to ChainActive().Tip() changes.

(Maybe consider reviewing conflicting pr #21691 first, since it’s easy)

ajtowns force-pushed on Apr 19, 2021

DrahtBot removed the label Needs rebase on Apr 19, 2021

DrahtBot added the label Needs rebase on Apr 20, 2021

ajtowns force-pushed on Apr 20, 2021

DrahtBot removed the label Needs rebase on Apr 20, 2021

ajtowns force-pushed on Apr 20, 2021

in src/deploymentinfo.h:26 in 37458fdd10 outdated

21+std::string DeploymentName(Consensus::BuriedDeployment dep);
22+
23+inline std::string DeploymentName(Consensus::DeploymentPos pos)
24+{
25+    assert(Consensus::ValidDeployment(pos));
26+    return VersionBitsDeploymentInfo[pos].name;

jnewbery commented at 11:39 am on April 26, 2021:

Consider using std::array and at() for bounds checking.

ajtowns commented at 8:41 am on April 30, 2021:

Same could be said for vDeployments in Consensus::Params; think both are better left for some other refactoring.

jnewbery commented at 12:19 pm on April 26, 2021: member

utACK 37458fdd1033cb30ec0591d0932e7d6a6b0dee32

I’m not convinced that a VersionBitsCache class is necessary. I think just as good would be to have a VersionBits namespace with free functions. The caching could all be hidden in the .cpp file, and only the functional interface would be exposed to client code.

DrahtBot added the label Needs rebase on Apr 27, 2021

ajtowns force-pushed on Apr 30, 2021

ajtowns commented at 8:45 am on April 30, 2021: member

Rebased past #21009 ~(and added some comment updates to reflect that Rewind is gone now)~

in src/chain.h:185 in b24e85139e outdated

181@@ -182,7 +182,7 @@ class CBlockIndex
182     //!
183     //! Note: this value is modified to show BLOCK_OPT_WITNESS during UTXO snapshot
184     //! load to avoid the block index being spuriously rewound.
185-    //! @sa RewindBlockIndex
186+    //! @sa NeedsRedownload

MarcoFalke commented at 9:02 am on April 30, 2021:

How is this change different from fa49430914fd16fbc34078bd8929b6ef635a8c42 in #21789 ?

ajtowns commented at 9:42 am on April 30, 2021:

:eyes: What change? There’s no change here… No change at all…

ajtowns force-pushed on Apr 30, 2021

DrahtBot removed the label Needs rebase on Apr 30, 2021

DrahtBot added the label Needs rebase on May 5, 2021

ajtowns force-pushed on May 6, 2021

ajtowns commented at 2:43 pm on May 6, 2021: member

Rebased past #21727.

I’m not convinced that a VersionBitsCache class is necessary. @jnewbery - You’re right it’s not necessary; but I think minimising the number of globals and keeping the lock and the things the lock is guarding together (which then allows stronger locking assertions, see #20272 (comment)) is worthwhile enough. If you’re implying the ComputeBlockVersion to versionbits.ComputeBlockVersion change is a bit ugly, that could be fixed just by adding an inline int32_t ComputeBlockVersion(pindexPrev, params) { return versionbits.ComputeBlockVersion(pindexPrev, params); } in deploymentstatus.h (where the versionbits global is declared).

DrahtBot removed the label Needs rebase on May 6, 2021

jnewbery commented at 7:47 am on May 10, 2021: member

See https://github.com/jnewbery/bitcoin/commit/000899c6753b69f141ffa07f7d2a12bd235824e3#diff-73b381667b1bb315180fc7e7a66992e79ad742972de5d0d2c1b8404d3d67e1b0 for the proposed changes in versionbits.h. The benefit is not exposing anything about the internal implementation outside of the .cpp file.

I think minimising the number of globals and keeping the lock and the things the lock is guarding together (which then allows stronger locking assertions […]

I think it’s fine to have a global with internal linkage in a single implementation file. The problems with globals all come when they’re externally linked and accessed from many places in the code. The point about locking assertions also isn’t relevant here since we’re not using EXCLUSIVE_LOCKS_REQUIRED(!) or LOCKS_EXCLUDED() for this mutex.

jnewbery commented at 7:53 am on May 10, 2021: member

utACK 63a6f6e5e695ca18c78a0dfb4d2261d1eb15ef4d.

Verified git range-diff master 37458fdd10 HEAD. Only difference is resolving the merge conflict with #21009.

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

DrahtBot added the label Needs rebase on Jun 4, 2021

ajtowns force-pushed on Jun 8, 2021

ajtowns commented at 4:17 am on June 8, 2021: member

Rebased past #22121 ; split the last commit to reduce the ComputeBlockVersion to versionbits.ComputeBlockVersion noise.

DrahtBot removed the label Needs rebase on Jun 8, 2021

jnewbery commented at 8:45 am on June 8, 2021: member

Code review ACK 3696abb45e

Verified the rebase using git range-diff.

in src/consensus/params.h:115 in 8695986585 outdated

110@@ -100,7 +111,25 @@ struct Params {
111      */
112     bool signet_blocks{false};
113     std::vector<uint8_t> signet_challenge;
114+
115+    inline int DeploymentHeight(BuriedDeployment dep) const

MarcoFalke commented at 12:31 pm on June 8, 2021:

8695986585d8717a9ab484877d25325fbe78a3e8: The compiler already knows this is inline because the member function has a definition. Thus, the inline can be removed for brevity.

in src/deploymentstatus.h:26 in 9773579504 outdated

21 }
22 
23+inline bool DeploymentActiveAfter(const CBlockIndex* pindexPrev, const Consensus::Params& params, Consensus::DeploymentPos dep)
24+{
25+    assert(Consensus::ValidDeployment(dep));
26+    return ThresholdState::ACTIVE == VersionBitsState(pindexPrev, params, dep, versionbitscache);

MarcoFalke commented at 1:32 pm on June 8, 2021:

97735795048b999acf5c79085ecdeb5df0413532: Any reason to not use ::versionbitscache? Especially since the g_ prefix is missing?

in src/deploymentinfo.h:19 in 0529efe7ce outdated

15     /** Whether GBT clients can safely ignore this rule in simplified usage */
16     bool gbt_force;
17 };
18 
19-extern const struct VBDeploymentInfo VersionBitsDeploymentInfo[];
20+extern const struct VBDeploymentInfo VersionBitsDeploymentInfo[Consensus::MAX_VERSION_BITS_DEPLOYMENTS];

MarcoFalke commented at 1:36 pm on June 8, 2021:

0529efe7ce918e564672a77da891d5e7a3c9657f: The compiler already knows that VBDeploymentInfo is a struct, so while touching this line it would be nice to remove this confusing extraneous keyword.

in src/deploymentstatus.h:26 in 5eecf96666 outdated

22@@ -23,7 +23,7 @@ inline bool DeploymentActiveAfter(const CBlockIndex* pindexPrev, const Consensus
23 inline bool DeploymentActiveAfter(const CBlockIndex* pindexPrev, const Consensus::Params& params, Consensus::DeploymentPos dep)
24 {
25     assert(Consensus::ValidDeployment(dep));
26-    return ThresholdState::ACTIVE == VersionBitsState(pindexPrev, params, dep, versionbitscache);
27+    return ThresholdState::ACTIVE == versionbitscache.State(pindexPrev, params, dep);

MarcoFalke commented at 1:46 pm on June 8, 2021:

5eecf966664df05afb8bb8ea9d5a18c7f9d13acf: Maybe add the :: now, if editing previous commits would be too much rebase hassle?

in src/miner.cpp:133 in 0967181ee8 outdated

129@@ -130,7 +130,7 @@ std::unique_ptr<CBlockTemplate> BlockAssembler::CreateNewBlock(const CScript& sc
130     assert(pindexPrev != nullptr);
131     nHeight = pindexPrev->nHeight + 1;
132 
133-    pblock->nVersion = ComputeBlockVersion(pindexPrev, chainparams.GetConsensus());
134+    pblock->nVersion = versionbitscache.ComputeBlockVersion(pindexPrev, chainparams.GetConsensus());

MarcoFalke commented at 1:54 pm on June 8, 2021:

0967181ee84d6c0f1a5c07d6e366d2a48c548bde: :: :smile_cat:

in src/test/versionbits_tests.cpp:412 in 3696abb45e outdated

412         lastBlock = secondChain.Mine(min_activation_height, nTime, VERSIONBITS_LAST_OLD_BLOCK_VERSION).Tip();
413     }
414 
415     // Check that we don't signal after activation
416-    BOOST_CHECK_EQUAL(ComputeBlockVersion(lastBlock, params) & (1<<bit), 0);
417+    BOOST_CHECK_EQUAL(versionbitscache.ComputeBlockVersion(lastBlock, params) & (1<<bit), 0);

MarcoFalke commented at 2:03 pm on June 8, 2021:

3696abb45e3d9b09d027fffd3fc9f19a15e21691: Could add :: and apply clang-format-diff to the touched lines?

in src/rpc/blockchain.cpp:1346 in 32cf12a896 outdated

1347@@ -1345,25 +1348,25 @@ static RPCHelpMan verifychain()
1348     };
1349 }
1350 
1351-static void BuriedForkDescPushBack(UniValue& softforks, const std::string &name, int softfork_height, int tip_height) EXCLUSIVE_LOCKS_REQUIRED(cs_main)

MarcoFalke commented at 2:05 pm on June 8, 2021:

32cf12a89619688182fcb175a63ba0356f170d9c: When removing the lock annotation here, maybe add a comment to LOCK(cs_main), that it must be held while all soft forks are pushed back even though there is no annotation requiring that. Otherwise the rpc might be racy.

Alternatively keep the annotation?

ajtowns commented at 1:56 am on June 11, 2021:

I don’t think it should be racy? The versionbits structures are protected by VersionBitsCache::m_mutex and should be stable when provided a particular block, even if it’s no longer the tip or a reorg is currently happening?

MarcoFalke commented at 6:41 am on June 11, 2021:

Oh correct. active_chain_tip is kept unchanged while pushing back all soft forks.

MarcoFalke approved

MarcoFalke commented at 2:06 pm on June 8, 2021: member

review ACK 3696abb45e3d9b09d027fffd3fc9f19a15e21691 🏌

Signature:

 0-----BEGIN PGP SIGNED MESSAGE-----
 1Hash: SHA512
 2
 3review ACK 3696abb45e3d9b09d027fffd3fc9f19a15e21691 🏌
 4-----BEGIN PGP SIGNATURE-----
 5
 6iQGzBAEBCgAdFiEE+rVPoUahrI9sLGYTzit1aX5ppUgFAlwqrYAACgkQzit1aX5p
 7pUgjMAwAiCgBhWC+XZ97dymn6KaEOyw0mJa0Yd1hVK7XAO6dUZzAVdpIJ4LFzzxv
 8K8wscByQ8ZbyM0jzaZYxj+KXNj0Mxc8nAooXKyL74myWD63Nc4yaXYLG5zDqAB3g
 9guSg6JNAhb/efy1XTA/RhnVJ4q6n0LKhSxE4AuEl4X2m8QeDJ1nh5YYF+BYhx9gd
10mtbjEuWhWiUOVqI0vs2nEDp+UwR9K0saYIcXemPJJ0JKarQDcfyvTP1saxoRqc3S
11RJ++Y2QZy8tCAZnVIz3TAYfnJ+8TnTsKOnc/NUC5d0dpVxvt+bgNsUY5mV6EWU2o
12GNOnea8u55Va0l8QuU2A7jK7QXAdCKgT7jqi9+On3qnGINcGUpNvrGMvldNllBfz
13s6i/rqng6gAcHGdMofJ8MEKnZ2KpdE5G60CheXlNk0FDMBDW+iAehyQP5PfD2w4j
14hjJ9V5EH2PqibtGZlCB8H8Rf45FB2laTTBtWpejLJFPrtVor9zOeV2vxFhOSWCqh
15goBp4TmI
16=ly6S
17-----END PGP SIGNATURE-----

Traceback (most recent call last): File “”, line 1, in FileNotFoundError: [Errno 2] No such file or directory: ‘/tmp/ghs_sig_71f8dc5f312c2dc020bbeb544189f15f -.ots’

DrahtBot added the label Needs rebase on Jun 10, 2021

ajtowns force-pushed on Jun 11, 2021

ajtowns commented at 2:27 am on June 11, 2021: member

Rebased past #22141 ; renamed versionbitscache to g_versionbitscache ; addressed @MarcoFalke’s comments

DrahtBot removed the label Needs rebase on Jun 11, 2021

MarcoFalke approved

MarcoFalke commented at 6:36 am on June 11, 2021: member

re-ACK f41c075cd2b1f0a38e645b58615d5f8a1bb8af71 😲

only changes:

remove inline and struct keywords where not needed
rebase
Add g_ prefix to versionbitscache

Signature:

 0-----BEGIN PGP SIGNED MESSAGE-----
 1Hash: SHA512
 2
 3re-ACK f41c075cd2b1f0a38e645b58615d5f8a1bb8af71 😲
 4
 5only changes:
 6* remove inline and struct keywords where not needed
 7* rebase
 8* Add g_ prefix to versionbitscache
 9-----BEGIN PGP SIGNATURE-----
10
11iQGzBAEBCgAdFiEE+rVPoUahrI9sLGYTzit1aX5ppUgFAlwqrYAACgkQzit1aX5p
12pUhv0gwAgxHsyNWf9+Zb59KLD/mAw/JOdA9SvkU/f/QI4qXoslOAaD5MdE546paw
13qM5g00MVUaLsgwoLqIgI5pfAiUpT/wSRChM2fIWzK0QqZPaq/9/y6az8Q4vT1ukM
14a0+zp86egBrWn/bB2rJuF5KAP7yzqQ3V+rWJ5NOit7DEuZ7bZbBerWxUO+zy9W8j
15rKAR3peiyl0Yk06J4fftH4OnUBnN4t9o/GZQjWrIUxTMtiSLvOprmjJsdWeObwP8
16Qxd7ANtZypHSwrqBqnl3+sTJCrx7pt1CI/8y4WvbXRTVlJihg46mh7IrY0wDv2D/
17susYpZy6kXTLaDG2G6IZ2u79iC/t8GCCCQjWwEM8z05/ay3MBAyAJtVUM+FaDtxs
18ye1E2jOOgdfieeWrxqATCxpl+ChsQPcARm8ksrhIrfJECV7rczvy0wXpTUfCM+F3
19L6jEdNwh0D0nFBNd0mz7/djxbHdSsEZqSlDaJp3goz8F/KflcriGQ9U0Y7Zt1M2I
20rZZfXr4D
21=oC8X
22-----END PGP SIGNATURE-----

Timestamp of file with hash ef1b7f61a9b0e9fe191ad81738ce361724107d785fda2888ecfb8e54fd721e5b -

jnewbery commented at 4:10 pm on June 11, 2021: member

reACK f41c075cd2b1f0a38e645b58615d5f8a1bb8af71

DrahtBot added the label Needs rebase on Jun 12, 2021

ajtowns force-pushed on Jun 12, 2021

ajtowns commented at 9:09 am on June 12, 2021: member

Rebased past #21866

DrahtBot removed the label Needs rebase on Jun 12, 2021

MarcoFalke commented at 4:01 pm on June 13, 2021: member

re-ACK 01c156677e34e21591e71fb979a041f19c060018 🔵

Signature:

 0-----BEGIN PGP SIGNED MESSAGE-----
 1Hash: SHA512
 2
 3re-ACK 01c156677e34e21591e71fb979a041f19c060018 🔵
 4-----BEGIN PGP SIGNATURE-----
 5
 6iQGzBAEBCgAdFiEE+rVPoUahrI9sLGYTzit1aX5ppUgFAlwqrYAACgkQzit1aX5p
 7pUimtAwAxwW2oHhca0vBquCwKsoXJ1HPQCWPUS0yXCXiKL8J/5ojI8gXEw7Pv1Ug
 8yzYIlNsr30ixEWXZCOZ4O+aRW8E9WvDy4JKesZ/xrBt37I66JLnOu95futuAcIfG
 96+L4rMRTqPnX+6wHx5WzVfBFhwN7eVjQbhu/Mia7FJsXHjeiPs6mjKYXUQzx7Ipi
10xYy2UxRfLBk98vABaw3969QTytAKxxwln8w3plPV6HVqonroUjGKQqlWsmFkPn9C
11yXFJwwWSsJQhJZgbyayhujK2HPaGIIO4pN2lrTdk5CnHS8vaA6b06OAvLhTNKtSA
12l+IRRHwtRToIp8/bngTkTB4vvccB2o50/ty+aVk7mlP4FmMCHxfXAa7o7g6WykQW
13efobheQdOAhpC1aV5EtlvCnQYlD3BMB806fxRQCzCa1FDJnG6jmH7+zkmKUbD9dx
140+b6xQIY1ROnz9cHqT1K+0w/9S2X4O4GBJn27TE4lCfuTwfoE/tkQI1xbGgaNN/t
15YKXw60G8
16=L+EA
17-----END PGP SIGNATURE-----

Timestamp of file with hash 3467c5ea999c8549d102168d19450d004f51c5fbdba853fb143538b01ab1893c -

in src/deploymentstatus.h:52 in e3a93b5095 outdated

47 }
48 
49+inline bool DeploymentEnabled(const Consensus::Params& params, Consensus::DeploymentPos dep)
50+{
51+    assert(Consensus::ValidDeployment(dep));
52+    return params.vDeployments[dep].nTimeout != 0;

MarcoFalke commented at 6:53 am on June 14, 2021:

e3a93b5095522f9315942600cbeb394a902bc5ce: This is currently unused, so there are no bugs. However, I am wondering if the implementation is the most meaningful. This is the first place where a magic value of 0 for the timeout is defined. Be aware that nTimeout is mostly an unsanitized [1] “don’t care value”, as the enabled status is read from nStartTime (NEVER_ACTIVE).

Finally, I am wondering what the use of this helper might be. Maybe delete the function until there is a use case?

0template <class> inline constexpr bool ALWAYS_FALSE{false};
1template <class Dep> bool DeploymentEnabled(const Consensus::Params& params, Dep dep)
2{
3static_assert(ALWAYS_FALSE<Dep>, "DeploymentEnabled only meaningful for buried deployments");

[1] There was a favour to not sanitize this value across all ST implementations. #21392 (review)

(Obviously non-blocking nit, can be fixed up later)

ajtowns commented at 4:30 am on June 15, 2021:

This is the first place where a magic value of 0 for the timeout is defined.

!= 0 matches the behaviour used for IsScriptWitnessEnabled prior to 0328dcdcfcb56dc8918697716d7686be048ad0b3

Finally, I am wondering what the use of this helper might be. Maybe delete the function until there is a use case?

Having it defined makes it (relatively) easy to switch DEPLOYMENT_SEGWIT back to a signaled deployment (ie updating consensus/params.h and chainparams.cpp) and checking that the change does what it’s supposed to (ie not require updating validation.cpp etc).

the enabled status is read from nStartTime (NEVER_ACTIVE).

I think it would make sense to switch it to this – should be possible to have a “fake” NEVER_ACTIVE that still passes DeploymentEnabled by setting the starttime to greater than uint32_t::max(). (Might do it if there’s another rebase needed, otherwise will leave for a later PR)

MarcoFalke commented at 6:09 pm on July 1, 2021:

Fixed in https://github.com/bitcoin/bitcoin/pull/22385

jnewbery commented at 8:29 am on June 14, 2021: member

ACK 01c156677e

ajtowns added this to the milestone 22.0 on Jun 21, 2021

michaelfolkson commented at 11:38 am on June 21, 2021: contributor

Concept ACK, Approach ACK.

Looked at this at the PR review club session back in March 2021 and changes since then appear to be minimal. This PR doesn’t appear to be urgent for 22.0 as Taproot deployment won’t be buried anytime soon but this PR has been open for a while so it would definitely be nice to get it in.

ajtowns commented at 6:49 am on June 23, 2021: member

Including this in 22.0 makes it easier to switch taproot to being a buried deployment, which would allow removing the speedy trial code, which in turn may make it easier to backport new activation code for any new soft forks that may be ready for deployment prior to version 22 reaching end-of-life (perhaps some of the consensus cleanup ideas eg).

MarcoFalke commented at 7:07 am on June 23, 2021: member

backport new activation code for any new soft forks that may be ready for deployment prior to version 22 reaching end-of-life

I think generally we only backport soft-forks to the previous major version. Though I have a slight preference to merge this for 22 to minimize potential conflicts when backporting (other) consensus changes.

in src/deploymentinfo.cpp:35 in 80b5b91752 outdated

30+    case Consensus::DEPLOYMENT_CSV:
31+        return "csv";
32+    case Consensus::DEPLOYMENT_SEGWIT:
33+        return "segwit";
34+    } // no default case, so the compiler can warn about missing cases
35+    return "";

fjahr commented at 3:40 pm on June 26, 2021:

nit: Could have given a default that provides more context like “unknown”

MarcoFalke commented at 5:23 pm on June 26, 2021:

Shouldn’t matter too much, as it is dead code. Empty string might even make it easier to run CHECK_NONFATAL(!name.empty()); in rpc code.

MarcoFalke commented at 7:29 am on June 30, 2021:

Or just assert(false) for symmetry with the assert at the beginning of the function?

in src/versionbits.cpp:220 in 0802af87b7 outdated

216+
217+int32_t ComputeBlockVersion(const CBlockIndex* pindexPrev, const Consensus::Params& params)
218+{
219+    int32_t nVersion = VERSIONBITS_TOP_BITS;
220+
221+    for (int i = 0; i < (int)Consensus::MAX_VERSION_BITS_DEPLOYMENTS; i++) {

fjahr commented at 3:57 pm on June 26, 2021:

nit: could have upgraded to static_cast<int>(Consensus::MAX_VERSION_BITS_DEPLOYMENTS)

MarcoFalke commented at 5:25 pm on June 26, 2021:

u{Consensus::MAX_VERSION_BITS_DEPLOYMENTS} is even more preferable when this is changed. (with u being the underlying type)

MarcoFalke commented at 7:44 am on June 28, 2021:

Introduce deploymentstatus #19438

Conflicts