deploymentstatus: move g_versionbitscache global to ChainstateManager #24595

Could you explain a bit why m_versionbitscache is marked mutable?

It's a cache, so its existence doesn't impact the logical API of the class -- if we didn't use a cache, you could have a const ChainstateManager and get answers as to whether a deployment is active/enabled and what version to use for new blocks; so adding a cache shouldn't mean that you can only do that with a non-const ChainstateManager.

ie, I'm treating const ChainstateManager as being "logically" constant, not bitwise constant.

Sounds good!

Perhaps these additions could be made in the previous commit where we introduce the private m_chainparams member

By this, do you mean all references to (CChainState::)m_params by CChainState member functions should instead be references to m_chainman.GetParams()?

Yeah. Or perhaps add a GetParams() and GetConsensus() to CChainState that just call m_chainman.GetParams() or m_chainman.GetConsensus().

(I didn't start on this here since I imagine it would have a bunch of conflicts with other PRs)

Sounds good!

nit: I'm not sure that this change is 100% necessary. The change seems to trade one reference for another, and since ChainstateManager is much larger in scope, it is harder to see after the change that this is a function that is limited in scope.

However, this is not at all a bit deal and up to subjective taste so feel free to ignore!

It needs to call DeploymentActive(.., SEGWIT), and that's something that the ChainstateManager is responsible for being able to answer. At least that was my logic.

Note that it's a const member function, so it's not doing anything to change the chain state, if that counts as reducing the scope?

Sounds good!

Perhaps we can use a const VersionBitsCache& here?

Could, but it should all get optimised away since WarningBitsConditionChecker is only briefly instantiated while it's being checked.

Sure!

4cbc4e94b764ba2b4a79610fadbb092a7bdd076e: What about a public m_params and m_consensus? There shouldn't be any risk associated with read-only references. And a public member removes the need for a function wrapper that only returns the member.

6 to one, half a dozen the other? I thought accessors were more common than extra variables, and make it a bit clearer that they can't get out of sync? shrug

0fc617a406232e21bdc8b39b2f8308f965775825 Any reason to add this include when the existing fwd-decl is enough?

Adding #include <chainparams.h> in the previous commit already brings consensus/params.h in so the fwd declaration isn't saving anything. Later, DeploymentActive* use info from consensus/params.h too.

I think for passing references or pointers, a fwd-decl is enough.

iwyu seems to agree:

validation.h should remove these lines:
- #include <config/bitcoin-config.h>  // lines 10-10
- #include <consensus/params.h>  // lines 18-18
- #include <stdint.h>  // lines 39-39
- #include <util/hasher.h>  // lines 30-30
- class CBlockTreeDB;  // lines 46-46
- class CTxMemPool;  // lines 47-47
- struct AssumeutxoData;  // lines 53-53
- struct ChainTxData;  // lines 49-49
- struct DisconnectedBlockTransactions;  // lines 50-50
- struct LockPoints;  // lines 52-52

(Not saying you should fix all includes in this pull here, but I think not making it worse is a good compromise)

Ah, iwyu treats the DeploymentActive stuff as recommending that deploymentstatus.h add the #include <consensus/params.h> not validation.h. I've got some later commits that move those functions into validation.h which updates iwyu's recommendations to a #include.

Since it's already included indirectly by chainparams.h I don't really see the include as making it worse, but it's not really better either, so will change it back if there are other fixes to bundle it with.

Done now

5c67e84d37d452e9186a6357e5405fabeff241c7 nit: .m_chainman.GetParams()

Seems odd to add a TODO and violate it in the same commit ( /* TODO: replace with m_chainman.GetParams() */)

eca22c726ac48b4216bb68cc0f0bbd655c43ac12 nit: Later diff could be smaller by a few lines if you added a VersionBitsCache& vbcache{g_versionbitscache}; in this commit already.

Removed chainman completely from this test in https://github.com/bitcoin/bitcoin/pull/25125