Please refer to the subsidy_cleanup branch (current tip, 3185dae)
There're 4 commits in that branch and here're my comments/response:

commit 1: kwvg@3fc0fe5
refactor: pass CChainParams instead of Consensus::Params to GetBlocksubsidy
commit 4: kwvg@3185dae
refactor: pass Consensus::Params to GetMasternodePayment

IMHO, better to do replacing Params() along backport bitcoin#25168 - out of scope of this tiny refactoring

commit 2: kwvg@812d63c
refactor: add zero-height block safe GetBlockSubsidy

This commit is very similar to my changes, but there're 2 functions GetBlockSubsidy that are both public but different only in argument name.

as doc/developer-nodes.md says:

• Try not to overload methods on argument type. E.g. don't make getblock(true) and getblock("hash")
  do different things.

• Rationale: This is impossible to use with dash-cli, and can be surprising to users.

• Exception: Some RPC calls can take both an int and bool, most notably when a bool was switched
  to a multi-value, or due to other historical reasons. Always have false map to 0 and
  true to 1 in this case.
  In this case GetBlockSubsidy behave differently depends from which function is called.

commit 3: kwvg@f4180b1
refactor: split underlying subsidy calculations to GetBaseSubsidy

I don't see a point to do it, because this underlying function is not used anywhere and just make implementation longer.

So, commit-2 - duplicate with this PR, commit 3 - to drop. Commit 1 and Commit 4 -> dedicated PR along with backport bitcoin#25168 (and may be some more backports)

• I feel that commits 1 and 4 should be a part of this PR, it reduces the amount of additional changes that need to be done when that PR needs to be backported and are relatively trivial in nature. It would also ease review when that PR is backported.

• Overloading has been employed in commit 2 so that switching the codebase over and then eventually removing the current variant with the proposed variant is easier.

  While doc/developer-notes.md does argue against overloading, it's under the section "RPC interface guidelines".

  The proposed changes have nothing to do with RPCs.

• Splitting was done in commit 4 because I felt doing so into smaller pieces will make it easier to read, separating the base calculation from the calculation with all parameters considered.

  But, the split out portion isn't being reused and is unlikely to be reused and exists only for the sake of readability. It can be omitted if desired.

Sorry, I quoted wrong part of doc/developer-notes when I tried to find it. This function is public interface from header even if it is not in virtual class. Anyway, it's 100% applied... But I'd prefer to use different name for this case, because logic is not same.

doc/developer-notes:

• Interface methods should not be overloaded.

  Rationale: consistency and friendliness to code generation tools.

  Example:

  // Good: method names are unique
  virtual bool disconnectByAddress(const CNetAddr& net_addr) = 0;
  virtual bool disconnectById(NodeId id) = 0;
  
  // Bad: methods are overloaded by type
  virtual bool disconnect(const CNetAddr& net_addr) = 0;
  virtual bool disconnect(NodeId id) = 0;

About refactoring of Params(). I just double-checked with current master of bitcoin - even if some parts are refactored, GetBlockSubsidy is not changed.

dc971be083 (Ryan Ofsky        2022-01-17 20:33:16 -0500 117)     const CAmount block_subsidy{GetBlockSubsidy(block.height, Params().GetConsensus())};

There's exactly same line in current master of bitcoin that still uses Params().GetConsensus(): https://github.com/bitcoin/bitcoin/blob/master/src/index/coinstatsindex.cpp#L117

I'd prefer to keep this PR as it is.

imo GetBlockSubsidyPrev name is confusing. How about 1270311 or smth similar?

• I don't feel GetBlockSubsidy is part of an interface but I won't insist, imo it's a matter of preference and I won't object to a non-overloaded variant.

• About refactoring of Params(). I just double-checked with current master of bitcoin - even if some parts are refactored, GetBlockSubsidy is not changed.

  The parts that are refactored to avoid calling the global are validation-specific, coinstatsindex doesn't fit that bill, it may be calling validation-specific code but it passes the global as an argument and GetBlockSubsidy doesn't itself invoke the global. I'm aware of the global invocations in index/ which is why I was okay with keeping them in #5501.

  I'm instead, proposing to replace the global invocation made here with one that takes in an argument, which means taking in CChainParams instead of Consensus::Params

• hm.... Probably here's some misusage of fSuperblockPartOnly - doesn't look as it is used anywhere

  I'm not sure I follow you, usage seems fine to me

The parts that are refactored to avoid calling the global are validation-specific, coinstatsindex doesn't fit that bill, it may be calling validation-specific code but it passes the global as an argument and GetBlockSubsidy doesn't itself invoke the global. I'm aware of the global invocations in index/ which is why I was okay with keeping them in #5501.

I'm instead, proposing to replace the global invocation made here with one that takes in an argument, which means taking in CChainParams instead of Consensus::Params

Sorry, I didn't understand this part.
I took random line from one of your commit from mentioned branch:

-    coinbaseTx.vout[0].nValue = GetBlockSubsidy(block.nBits, nHeight, chainparams.GetConsensus());
+   coinbaseTx.vout[0].nValue = GetBlockSubsidy(block.nBits, nHeight, chainparams);

I open same line in the bitcoin code (master):

    coinbaseTx.vout[0].nValue = GetBlockSubsidy(nHeight, chainparams.GetConsensus());

So, if this refactoring will be done, any backports in future would be more difficult to be done.
If there's any noticeable benefits that can out-weight potential conflicts while doing backports - please, bring them up.

Btw, if some code is dash-specific, let's consider changes without changing common code with bitcoin.

UPD: do you mean this particular call and only it? Params().NetworkIDString() == CBaseChainParams::MAIN ? hm, let me think how to update it then.

UPD: do you mean this particular call and only it? Params().NetworkIDString() == CBaseChainParams::MAIN? hm, let me think how to update it then.

Pretty much why I bothered with changing to CChainParams, yes

In defense of Params().NetworkIDString() I may say that bitcoin even get rid of that in validation.cpp still have direct calls of Params() in multiple places (I excluded tests, init.cpp and gui's related code and Params().GetConsensus()) but still:

src/wallet/external_signer_scriptpubkeyman.cpp:    ExternalSigner::Enumerate(command, signers, Params().GetChainTypeString());
src/torcontrol.cpp:        service = LookupNumeric(std::string(service_id+".onion"), Params().GetDefaultPort());
src/net.cpp:    return static_cast<uint16_t>(gArgs.GetIntArg("-port", Params().GetDefaultPort()));
src/rpc/mempool.cpp: if (!Params().IsMockableChain()) {

Note, that GetChainTypeString is equivalent of NetworkIDString
Also I'd like to note this one:

 /* Return the currently selected parameters. This won't change after app
 * startup, except for unit tests.
 */
const CChainParams &Params()

so, it doesn't actually critical to get-rid-of that call.