nit: that's a small method, but I don't see a reason to move it to the top of cpp file. especially in the next commit (rename MNSubsidy to CSHelper) most of code removed from this file.

Only most of the header contents of masternode/payments.h are moved to evo/cshelper.h, the contents of masternode/payments.cpp and evo/specialtxman.cpp remain where they are and are only modified to the extent that the new name was being used.

The new header exists somewhat document that ChainstateHelper is just an vessel for logic implemented in those two files and the logic implemented in specialtxman.cpp should not be considered a part of masternode payment code (which would've been in the implication had the special transaction functions been defined in masternode/payments.h).

I kept it at the top because it's a relatively small function (and now additionally, because it's the only function actually defined in its corresponding header, the rest being implementations of the definitions now in cshelper.h)

I don't like constructor of BlockAssembler to accept many-many arguments; this commit make list of arguments longer --- I don't feel like that is a useful refactoring, moreover I feel like that's getting worse.

If that's a staging temporary changes - I'd like to know what is a next step, but BlockAssembler having +1 more argument as it is done now that is defactoring, not refactoring in my opinion

IMO much better to let BlockAssembler to use mnhfman, creditpool, from chainparams or from cs_helper rather than have that long list of arguments in constructor

we could simply pass node context here I guess c764c09255

I don't know how I feel about making the managers available from cs_helper (everywhere else we accept the references for the classes' use but don't actually expose it) but I do agree, the long arguments are frustration-inducing (I'd know, reordering commits and cleaning up required me to fix up the argument list manually multiple times).

But the truth of the matter is, we have a lot of managers. Excluding CoinJoin and LLMQ managers (which are managed by their own context), we have ~10 managers and I don't think that's all of them. There's no real way to avoid it without consolidating them. There's a reason I got rid of CGovernanceTriggerManager, we don't need more managers.

I did think about creating DashContext but then accessing something like CBLSWorker would require doing something like node.dash_ctx->llmq_ctx->bls_worker and that, at least on initial impression, seems like too much nesting.

Using CChainState seems unideal imo, I've tried experimenting with aliasing the CDeterministicMNList query function to CChainState (similar to what was done with CMNHFManager) but it seemed unwieldly to do so, not only was I adding yet another argument to pass through ChainstateManager and CChainState, for a function that CChainState doesn't really need, it's more likely than not that if CDeterministicMNList is passed upwards enough, we'll eventually reach a point where CDeterministicMNManager would be needed.

The reason why I did the refactoring for CMNHFManager is because I'd prefer if CChainState accepts the fewest possible arguments. It's the reason ChainstateHelper exists, because the alternative would be piping CGovernanceManager, CSporkManager and others.

I'm not quite sure of adding NodeContext as an argument, it doesn't really seem to be meant to be used that way. Especially since, unlike LLMQContext, where the constructor initializes its members, there's no such guarantee for NodeContext.

I'm not quite sure of adding NodeContext as an argument, it doesn't really seem to be meant to be used that way.

Don't know, for me passing node context instead of individual members make perfect sense. Check this for example.

Especially since, unlike LLMQContext, where the constructor initializes its members, there's no such guarantee for NodeContext.

Check that diff a bit closer ;) and you'll notice that we do the same pointer dereference without checking anything already so there should be no side effects if we move these dereferences somewhere else. But it's true, it would probably make sense to have some (inline) asserts, see f5b3875578.

Cherry-picked and squashed in latest push

with credit pool here that's getting even worse. Need to think how to wrap things up here. Credit pool and mnhfman and mn_subsidy - that's a things that should be inside m_chainstate, as by design and by idea.

why don't move all of that directly to CChainState instead?

Because I'd prefer CChainState to be as close to upstream as possible, not only to make backports and maintenance easier, but to also maintain a soft barrier between Dash and Bitcoin code.

ChainstateHelper lets primarily Bitcoin logic access predominantly Dash logic, acting as a vessel that abstracts the inner machinations from CChainState, letting us change and update Dash-specific code as we see fit without having to make too many changes to Bitcoin-maintained code.

nit (not a blocker to get merged):

#include <vector>
#include <memory>
#include <optional>

why there's used inheritance instead aggregation? Why do we even need CMNPaymentProcessor?