consensus/params: simplify ValidDeployment check to avoid gcc warning

[ajtowns]

Simplifies the ValidDeployment check to only check the upperbound, relying on the lower bound to be trivially true due to enum values starting at the minimum value of the underlying type (which is checked at compile time in deploymentstatus.cpp). Avoids a "comparison always true" warning in some versions of gcc.

Fixes #22587

[DrahtBot]

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

Conflicts

Reviewers, this pull request conflicts with the following ones:

• refactor: Move mutable globals cleared in ::UnloadBlockIndex to BlockManager #22564 by dongcarl

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.

[maflcko]

Left a review comment. Also, unrelated:

DeploymentActiveAt could be a template as well:

diff --git a/src/deploymentstatus.h b/src/deploymentstatus.h
index f95c5996f5..f133d6ec6a 100644
--- a/src/deploymentstatus.h
+++ b/src/deploymentstatus.h
@@ -27,13 +27,8 @@ inline bool DeploymentActiveAfter(const CBlockIndex* pindexPrev, const Consensus
 }
 
 /** Determine if a deployment is active for this block */
-inline bool DeploymentActiveAt(const CBlockIndex& index, const Consensus::Params& params, Consensus::BuriedDeployment dep)
-{
-    assert(Consensus::ValidDeployment(dep));
-    return index.nHeight >= params.DeploymentHeight(dep);
-}
-
-inline bool DeploymentActiveAt(const CBlockIndex& index, const Consensus::Params& params, Consensus::DeploymentPos dep)
+template <typename Dep>
+bool DeploymentActiveAt(const CBlockIndex& index, const Consensus::Params& params, Dep dep)
 {
     assert(Consensus::ValidDeployment(dep));
     return DeploymentActiveAfter(index.pprev, params, dep);

The benefit (apart from less code) would also be a way to enforce that the two function have the same signature. See #22564 (comment)

[maflcko]

Is there a reason to use three template parameters when a single one would be sufficient?

template <typename X>
static constexpr bool is_minimum(const X x)
{
    using U = typename std::underlying_type<X>::type;
    return x == std::numeric_limits<U>::min();
}

static_assert(is_minimum(Consensus::DEPLOYMENT_HEIGHTINCB), "heightincb is not minimum value for BuriedDeployment");
static_assert(is_minimum(Consensus::DEPLOYMENT_TESTDUMMY), "testdummy is not minimum value for DeploymentPos");

This seems verbose, confusing and dangerous. For example the following asserts are wrong, but don't fail:

static_assert(is_minimum<Consensus::BuriedDeployment, Consensus::BuriedDeployment, signed short>(Consensus::DEPLOYMENT_HEIGHTINCB), "heightincb is not minimum value for BuriedDeployment");
static_assert(is_minimum<Consensus::DeploymentPos, Consensus::DeploymentPos, unsigned short>(Consensus::DEPLOYMENT_TESTDUMMY), "testdummy is not minimum value for DeploymentPos");

[ajtowns]

Changed. Better?

[practicalswift]

Concept ACK

[tryphe]

tested ACK 9bf43b0

It solves the warnings on gcc 8.3.0 (Debian 10).

[tryphe]

I think it's important to let other reviewers know that MAX_VERSION_BITS_DEPLOYMENTS is not intended to be assigned, hence we only check if it's less, unlike on line 26 where dep might equal DEPLOYMENT_SEGWIT.

At least that was my interpretation.

[maflcko]

review ACK 0591710

[maflcko]

Added for rc3 (if there is one)

[Rspigler]

After @tryphe showed me how to reproduce, I tested 0591710 and this does solve the problem!

[tryphe]

retested ACK 0591710
approach ACK 0591710

gcc 8.3.0

[maflcko]

tested on aarch64:

gcc (Ubuntu/Linaro 7.5.0-3ubuntu1~18.04) 7.5.0

[hebasto]

Backported in #22629.