prefer enum class to prevent leaking names.

Also helps with avoiding overlap with SignalledDeployment.

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.

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.

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

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.

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

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

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:

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

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

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

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.

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

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

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

See also #19438 (comment)

24972d9f49:

Shouldn't this say:

const bool taproot_active{DeploymentActiveAfter(m_active_chainstate.m_chain.Tip(), args.m_chainparams.GetConsensus(), Consensus::DEPLOYMENT_TAPROOT)};

Yes, it should, fixed.