Looks OK from quick glance. Test plan?

utACK.

One way to test this would be creating a unit test with the old GetBlockValue() implementation, and calling it for some arbitrary heights/fees, and comparing with the new. It looks trivially correct to me, though.

Needed rebase.
I also think it is trivial enough: passing Consensus::Params, adding the fees outside of the function and not calculating the initial subsidy if it's going to return 0.

In any case, the current main_tests.cpp (which just tests this function) could be improved, it only checks that the subsidy is always <= 50 * COIN. Probably a dataset with pairs height -> subsidy would be better.

We could also test for other subsidy halving intervals (now that Consensus::Params it's passed, it's trivial to set another Consensus::Params::nSubsidyHalvingInterval in the tests, even without calling Params()).
Does that sound good enough? Suggestions for improving main_tests (sat some point they should be renamed subsidy_tests and moved to src/consensus/tests but not yet)?

Needed rebase again.
I also added some more tests.
Note that if the constants...

    int maxHalvings = 64;
    CAmount nInitialSubsidy = 50 * COIN;

...were part of Consensus::Params, we could easily adapt my tests to test different values for them.
But I'm not sure it's worth it.
And even less sure if that belongs to this PR, I mean to get rid of CChainParams::SubsidyHalvingInterval() I could have simply added the Consensus::Params parameter to GetBlockValue without removing the nFees parameter...

Rebased

I mean to get rid of CChainParams::SubsidyHalvingInterval() I could have simply added the Consensus::Params parameter to GetBlockValue without removing the nFees parameter...

If you had done that, this would have already been merged. I'm a bit wary as this changes the fee computation code and is not simple substitution like the other "Remove redundant getter" pulls.

That's fine.
Libconsensus-wise, this is only required for VerifyBlock, which can only come after both VerifyTx and VerifyHeader, so this isn't really blocking anything in the short term. Therefore I prefer to do it in a way that I think is right rather than in the fastest way possible.

This doesn't "change the way fees are computed": fees are computed outside of this function. The only difference is that the subsidy and the fees were added together inside the function and now they will be added outside it. If the previous function had a more descriptive declaration like Camount CalculateSubsidyAndAddPreviouslyCalculatedFeesToIt(nHeight, previouslyCalucaletedFees) I doubt anybody would have complained about replacing CalculateSubsidyAndAddPreviouslyCalculatedFeesToIt(nHeight, nFees) with GetSubsidy(nHeight) + nFees, which is far more readable IMO (and more aligned with general principles about reusability, but let's not go there).

Calculating the initial subsidy to then ignore when halvings >= 64 it's stupid, so I would have also felt bad if I had touched this function without fixing that. I can always remove that tiny optimization if people insist on keep doing the wrong thing.

Maybe I should have done this in 3 commits and only squash them after people have seen this as the obviously equivalent-but-clearer thing I see. If someone else wants to do a version without the optimization and keeping the addition inside the function (because somehow they thing , nFees) is clearer than + nFees), I'm totally fine with it. But it will change exactly 2 lines less (the 2 lines required for the micro-optimization, that is, moving CAmount nSubsidy = 50 * COIN; after ````if halvings >= 64`).
If the creator of the competitor PR likes the micro-optimization but still wants to do the addition inside the function, the new PR will change exactly the same lines.
But I'm sorry, I'm not proposing that alternative myself because I don't like it, my change is free diff-wise and I consider rejecting that change another form of "DoS against development" that IMO makes less sense than asking people to order includes alphabetically (which at least has some minimal advantages rather than minimal disadvantages).

@jjtimon I didn't intend to argue, or demand that you have to change this code now after all that time, just tried to explain why this one requires more thorough scrutiny than those other mentioned pulls.

That said, the change looks good to me. utACK

I understood that and that's why this one got tests.
Re-reading my comment it sounds overly defensive and almost rude, my apologies.