Chainparams: Remove redundant getter CChainParams::SubsidyHalvingInterval()

jtimon commented at 3:45 pm on April 10, 2015: contributor

Decouple main::GetBlockValue() from Params() and rename it GetBlockSubsidy. Then remove redundant the getter CChainParams::SubsidyHalvingInterval().

jgarzik commented at 5:34 pm on April 10, 2015: contributor

Looks OK from quick glance. Test plan?

sipa commented at 8:42 am on April 11, 2015: member

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.

jtimon force-pushed on Apr 12, 2015

jtimon force-pushed on Apr 13, 2015

jtimon commented at 4:31 pm on April 13, 2015: contributor

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)?

jtimon force-pushed on Apr 15, 2015

jtimon commented at 2:03 pm on April 15, 2015: contributor

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

0    int maxHalvings = 64;
1    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…

Chainparams: Refactor: Decouple main::GetBlockValue() from Params() [renamed GetBlockSubsidy]

Remove redundant getter CChainParams::SubsidyHalvingInterval()

935bd0a447

jtimon force-pushed on May 15, 2015

jtimon commented at 2:13 pm on May 15, 2015: contributor

Rebased

laanwj commented at 8:13 am on May 16, 2015: member

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.

laanwj added the label Improvement on May 16, 2015

jtimon commented at 9:42 am on May 16, 2015: contributor

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).

laanwj commented at 6:27 am on May 18, 2015: member

@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

jtimon commented at 0:05 am on May 19, 2015: contributor

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

laanwj merged this on May 19, 2015

laanwj closed this on May 19, 2015

laanwj referenced this in commit 377711ff3f on May 19, 2015

in src/main.h: in 935bd0a447

201@@ -202,7 +202,7 @@ std::string GetWarnings(std::string strFor);
202 bool GetTransaction(const uint256 &hash, CTransaction &tx, uint256 &hashBlock, bool fAllowSlow = false);
203 /** Find the best known block, and make it the tip of the block chain */
204 bool ActivateBestChain(CValidationState &state, CBlock *pblock = NULL);
205-CAmount GetBlockValue(int nHeight, const CAmount& nFees);
206+CAmount GetBlockSubsidy(int nHeight, const Consensus::Params& consensusParams);

Diapolo commented at 1:50 pm on May 20, 2015:

Nit: You could have added a comment explaining the function AFAIK all main.h functions should include a comment.

jtimon commented at 8:19 pm on May 20, 2015:

That can still be done as an independent PR. Documentation is good, but there’s no reason why it has to be added when a function is touched. For example, should I complete the documentation of all functions touched in #6163 ? Seems orthogonal to me… Also I don’t see how functions declared main.h should be special in that regard.

MarcoFalke locked this on Sep 8, 2021

Chainparams: Remove redundant getter CChainParams::SubsidyHalvingInterval() #5996