Looks like it would be cleaner to remove this method completely? This is only used to check that the chain type is regtest, which can be seen in the error strings. See:

$ git grep -A1 IsMockableChain  src/rpc/
src/rpc/mempool.cpp:            if (!Params().IsMockableChain()) {
src/rpc/mempool.cpp-                throw std::runtime_error("submitpackage is for regression testing (-regtest mode) only");
--
src/rpc/node.cpp:    if (!Params().IsMockableChain()) {
src/rpc/node.cpp-        throw std::runtime_error("setmocktime is for regression testing (-regtest mode) only");
--
src/rpc/node.cpp:    if (!Params().IsMockableChain()) {
src/rpc/node.cpp-        throw std::runtime_error("mockscheduler is for regression testing (-regtest mode) only");

The rpc code would be easier to read if the chain type check was inlined and this method be removed, I'd say. For example, submitpackage has nothing to do with whether the time can be mocked.

For clarity, my comment is on IsMockableChain, not on IsTestChain.

I see. So keep IsTestChain and remove IsMockableChain method and replace all occurrences with inline checks like != ChainType::REGTEST?

Yes, I think it makes sense to keep the IsTestChain alias, because it is just one line of code and it makes code that uses it easier to read.

However, the IsMockableChain helper is used only in contexts to check for regtest, so I think it makes code easier to read to use a regtest chain-type check directly.

"mockable time" isn't a concept with libbitcoinkernel, so I think it makes sense to move IsMockableChain out of the kernel module. IsTestChain otoh seems like an important enough concept to still have in the kernel and be something we can ask about any chain we might have.

This change makes sense independently; submitpackage has no relationship to mocktime.

Not really convinced this makes sense though; having the IsMockable condition in one place (via an easily greppable name), rather than two places seems sensible. OTOH, could easily have that code just be in rpc/node.cpp rather than kernel/chainparams.h, eg

static bool IsMockable(const CChainParams& params)
{
    return params.GetChainType() == ChainType::REGTEST;
}

if (!IsMockable(Params())) { error...; }

May also make sense to add an error in init.cpp if -mocktime is specified with a different chain (in which case mocktime activates but you can't ever change it, so things will break pretty quickly), in which case perhaps putting IsMockable into src/chainparams.h (rather than kernel/chainparams.h) or similar would make sense.

May want to also replace Params() with EnsureAnyChainman(request.context).GetParams() to reduce global accesses?

If the function is kept, I'd prefer to keep it in the place it is now. Doesn't seem worth it to move a single function into another file (like src/chainparams.h). I guess this can be done in the future, if there is more than one function.

Keeping the location as-is should also make it a smaller diff and less controversial :)