test: add subsidy sum test, iterating every block #30699

l0rinc commented at 5:39 PM on August 22, 2024: contributor

We iterate through every single block until the subsidy reaches zero. After that we assert that it remains strictly zero (checking the next ~60 years).

Note: originally I modified subsidy_limit_test - now it's a separate test by popular demand

DrahtBot commented at 5:39 PM on August 22, 2024: contributor

The following sections might be updated with supplementary metadata relevant to reviewers and maintainers.

Code Coverage

For detailed information about the code coverage, see the test coverage report.

Reviews

See the guideline for information on the review process.

Type	Reviewers
ACK	hodlinator
Approach NACK	brunoerg, fjahr

If your review is incorrectly listed, please react with 👎 to this comment and the bot will ignore it on the next update.

DrahtBot added the label Tests on Aug 22, 2024

hodlinator commented at 7:16 PM on August 22, 2024: contributor

Nice to check the total subsidy without relying on nSubsidyHalvingInterval being evenly divisible by 1000. Neat to have the height of the first zero-subsidy block.

I do not like that the test no longer checks against max subsidy size and overflow/unreasonableness of the sum. What is your reasoning around that?

The old test went on for ~2x the subsidy-granting height, seems like a limit as least as good as 10m?

The minimum improvement I would like to see in the old test would be checking there is no remainder when dividing nSubsidyHalvingInterval by 1000. Lifting out the GetConsensus()-call from the loop is welcome too.

in src/test/validation_tests.cpp:62 in dd967d4a1a outdated

  54 | @@ -55,15 +55,22 @@ BOOST_AUTO_TEST_CASE(block_subsidy_test)
  55 |  
  56 |  BOOST_AUTO_TEST_CASE(subsidy_limit_test)
  57 |  {
  58 | -    const auto chainParams = CreateChainParams(*m_node.args, ChainType::MAIN);
  59 | -    CAmount nSum = 0;
  60 | -    for (int nHeight = 0; nHeight < 14000000; nHeight += 1000) {
  61 | -        CAmount nSubsidy = GetBlockSubsidy(nHeight, chainParams->GetConsensus());
  62 | -        BOOST_CHECK(nSubsidy <= 50 * COIN);

fjahr commented at 7:17 PM on August 22, 2024:

Why would you remove this check?

l0rinc commented at 7:35 PM on August 22, 2024:

I do not like that the test no longer checks against max subsidy size and overflow/unreasonableness of the sum. What is your reasoning around that?

Thanks for the review! I aimed to view the code from the perspective of layman Bitcoin enthusiasts, trying to get an answer to their question, "How do we know the limit isn't greater than 21 million?". I prioritized a high signal-to-noise ratio, omitting checks already covered by other tests. This test was about validating limits, not GetBlockSubsidy behavior, which was already tested elsewhere (e.g., TestBlockSubsidyHalvings).

Here we're checking the sum in the end, so validating intermediary values was considered noise. But if the reviewers insist, I can add it back.

fjahr commented at 7:44 PM on August 22, 2024:

I aimed to view the code from the perspective of layman Bitcoin enthusiasts

I don't think that many of them will look at our unit tests and if they do it's probably easier if you stick to writing a comment that is understandable. But I don't think they are the target audience of our unit tests.

I prioritized a high signal-to-noise ratio

I don't think that's the right strategy for our unit tests in general. I don't think we should remove checks unless there is a clear reason to do so. Having a bit of duplication is fine as this is arguably the most important feature of bitcoin.

l0rinc commented at 7:48 PM on August 22, 2024:

I've added back the intermediary asserts.

in src/test/validation_tests.cpp:64 in dd967d4a1a outdated

  59 | -    CAmount nSum = 0;
  60 | -    for (int nHeight = 0; nHeight < 14000000; nHeight += 1000) {
  61 | -        CAmount nSubsidy = GetBlockSubsidy(nHeight, chainParams->GetConsensus());
  62 | -        BOOST_CHECK(nSubsidy <= 50 * COIN);
  63 | -        nSum += nSubsidy * 1000;
  64 | -        BOOST_CHECK(MoneyRange(nSum));

fjahr commented at 7:18 PM on August 22, 2024:

And why remove this?

l0rinc commented at 7:35 PM on August 22, 2024:

Please see: #30699 (review)

fjahr commented at 7:23 PM on August 22, 2024: contributor

Your changed version doesn't check the same things as the old version and overall this doesn't look like an improvement to me. I would be neutral on the change if you just add some comments, that should be enough to improve clarity. The refactoring doesn't seem necessary to improve clarity and given the importance of this test I don't think many will be keen to invest their time to review sweeping code changes here just for improving clarity.

l0rinc force-pushed on Aug 22, 2024

l0rinc commented at 7:38 PM on August 22, 2024: contributor

nSubsidyHalvingInterval being evenly divisible by 1000

That shouldn't be important in the new implementation anymore since we're counting every block until the block reward exists.

The old test went on for ~2x the subsidy-granting height, seems like a limit as least as good as 10m?

Reverted to 14m, thanks.

fjahr commented at 7:46 PM on August 22, 2024: contributor

As I indicated above, even with the changes, approach NACK from me. I think clarification can be achieved without a complete refactoring of this test. I'm neutral on adding comments and more checks.

l0rinc force-pushed on Aug 22, 2024

kevkevinpal commented at 11:40 PM on August 22, 2024: contributor

I don't understand why we need to refactor the whole test, I think additional asserts I'm also neutral on

I think adding asserts without refactoring the test would be better

l0rinc commented at 6:09 AM on August 23, 2024: contributor

don't understand why we need to refactor the whole test

Because we assume less in the test if we iterate over every block (until we still have a block reward), not just every 1000 blocks

maflcko commented at 6:26 AM on August 23, 2024: member

don't understand why we need to refactor the whole test

Because we assume less in the test

I think this is a statement that is not trivial to check and probably not a good use of reviewers time. I haven't checked the history of the test case, but I presume it was intentionally written the way it looks now. For example, the MoneyRange check that you removed may or may not have been added to detect signed integer overflow, which is undefined behavior. I know that the undefined sanitizer exists now, but it may not have existed at the time when the test was written. The same goes for the other constructs in the test. No objection to adding new coverage or tests. Conceptually this looks similar to #30035, which refactored existing tests and added some new. It went through review cycles for months and then was closed.

l0rinc commented at 6:45 AM on August 23, 2024: contributor

I think this is a statement that is not trivial to check

We were checking every 1000th block, now we're checking every block.

For example, the MoneyRange check that you removed

I have added the asserts back yesterday.

brunoerg commented at 6:36 PM on August 23, 2024: contributor

Approach NACK

Unnecessary refactoring and tbh I could not clear understand the motivation and improvement.

l0rinc commented at 6:39 PM on August 23, 2024: contributor

not clear understand the motivation and improvement.

The motivation was to add every block's reward when calculating total, not just every 1000th (multiplied)

hodlinator changes_requested

hodlinator commented at 6:33 PM on August 26, 2024: contributor

Think a possible way forward would be to undo most changes to subsidy_limit_test.

The change from 2099999997690000 to using '-separators is nice. Have you considered the slightly more BTC-native 20'999'999'9769'0000 or 20'999'999'97'690'000? (Only found some vague precedence for the former in util_ParseMoney test case and none for the latter).
As I already mentioned, I think it would be an improvement to add a check before the loop that there is no remainder when dividing nSubsidyHalvingInterval by 1000 (the "step" of the loop).

On top of that, there may be enough support for adding a separate test case which does what your initial PR did (summing subsidy of every block, no checks inside the loop for efficiency, skipping to the next loop once subsidy goes to zero, with a check for the block height between the loops etc).

l0rinc force-pushed on Aug 29, 2024

l0rinc renamed this:
~~test: Improve clarity of subsidy limit test~~
test: Add subsidy sum test, iterating every block
on Aug 29, 2024

l0rinc force-pushed on Aug 29, 2024

l0rinc commented at 7:18 AM on August 29, 2024: contributor

Thanks for the reviews @fjahr, @brunoerg, @hodlinator, @maflcko, @kevkevinpal. If I understood you correctly, the objection was the modification of the existing test, so I added a new one which counts every single block subsidy until it drops to zero.

Have you considered the slightly more BTC-native 20'999'999'9769'0000

of course, but I really disliked that option.

no remainder when dividing nSubsidyHalvingInterval by 1000

done, thanks.

l0rinc renamed this:
~~test: Add subsidy sum test, iterating every block~~
test: add subsidy sum test, iterating every block
on Aug 29, 2024

l0rinc force-pushed on Aug 29, 2024

in src/test/validation_tests.cpp:60 in 30b528fb16 outdated

  56 | @@ -57,15 +57,31 @@ BOOST_AUTO_TEST_CASE(subsidy_limit_test)
  57 |  {
  58 |      const auto chainParams = CreateChainParams(*m_node.args, ChainType::MAIN);
  59 |      CAmount nSum = 0;
  60 | -    for (int nHeight = 0; nHeight < 14000000; nHeight += 1000) {
  61 | +    auto step = 1000;

hodlinator commented at 7:30 AM on August 29, 2024:

nit: Might consider it a constant:

    constexpr auto STEP = 1000;

l0rinc commented at 10:29 AM on August 29, 2024:

I would do this in prod code, here it just seems like noise. If you feel strongly about it, let me know.

in src/test/validation_tests.cpp:81 in 30b528fb16 outdated

  78 | +    int block_height{0};
  79 | +    while (auto subsidy = GetBlockSubsidy(block_height, main_consensus)) {
  80 | +        total_subsidy_sats += subsidy;
  81 | +        block_height++;
  82 | +    }
  83 | +    BOOST_CHECK_EQUAL(total_subsidy_sats, CAmount{2'099'999'997'690'000});

hodlinator commented at 7:35 AM on August 29, 2024:

Since 21 million BTC is such a meme and you expressed intent to make the test more accessible to novices, I still think this number format could be made clearer. You didn't like my suggestion with the sats-part having ' after the fourth digit, so this is my final suggestion on the number format:

    BOOST_CHECK_EQUAL(total_subsidy_sats, CAmount{20'999'999'97690000});

l0rinc commented at 10:29 AM on August 29, 2024:

so this is my final suggestion on the number format

perfect, it's the best of both worlds!

in src/test/validation_tests.cpp:78 in 30b528fb16 outdated

  74 | +BOOST_AUTO_TEST_CASE(subsidy_sum_test)
  75 | +{
  76 | +    auto main_consensus{CreateChainParams(*m_node.args, ChainType::MAIN)->GetConsensus()};
  77 | +    CAmount total_subsidy_sats{0};
  78 | +    int block_height{0};
  79 | +    while (auto subsidy = GetBlockSubsidy(block_height, main_consensus)) {

hodlinator commented at 7:52 AM on August 29, 2024:

nit: Did you intentionally make it like this for clarity over

    while (auto subsidy{GetBlockSubsidy(block_height, main_consensus)}) {

?

l0rinc commented at 10:29 AM on August 29, 2024:

yeah, I'd use the subsidy{} in prod code, here I wanted the simpler syntax that laymen would also understand when looking at the test - I consider this the entrypoint for their don't-trust-verify journey.

hodlinator commented at 12:00 PM on August 29, 2024:

There's still the implicit int -> bool conversion. Can't come up with anything more elegant though.

l0rinc commented at 1:49 PM on August 29, 2024:

Would you consider this more readable?

for (CAmount subsidy; (subsidy = GetBlockSubsidy(block_height, main_consensus)) > 0; block_height++) {
    total_subsidy_sats += subsidy;
}

hodlinator commented at 6:52 PM on August 29, 2024:

Nah, to me that's more complicated. Better keep it as-is.

hodlinator commented at 7:58 AM on August 29, 2024: contributor

Reviewed 30b528fb16920d5b64add0d53187527a30073040

Why did you decide to drop this from the added test?

    // Ensure subsidy remains zero.
    for (; block_height < 14'000'000; block_height += 1000) {
        BOOST_CHECK_EQUAL(GetBlockSubsidy(block_height, main_consensus), CAmount{0});
    }

I think it does provide some added value.

test: Add subsidy sum test, iterating every block

We iterate through every single block until the subsidy reaches zero, after which we assert that it remains zero.

Co-authored-by: Hodlinator <172445034+hodlinator@users.noreply.github.com>

3da293a192

l0rinc force-pushed on Aug 29, 2024

l0rinc commented at 10:29 AM on August 29, 2024: contributor

Why did you decide to drop this from the added test?

Since you see value in that, I've added it back - but also iterating every block, since it seems to be cheap (until 10m, I think that should suffice in this case):

time build/src/test/test_bitcoin --run_test=validation_tests/subsidy_sum_test 0.55s user 0.02s system 54% cpu 1.029 total

hodlinator approved

hodlinator commented at 12:32 PM on August 29, 2024: contributor

ACK 3da293a1922b94e9ad7c41e13fe9cca459ad9fbb

Ensures subsidy_limit_test isn't miscalculating due to out-of phase step <-> nSubsidyHalvingInterval.
Adds a more thorough test than with a cheaper loop but step of 1.
- Test increases confidence in the very important property of 21m.
- Documents the precise block height at which the subsidy goes to zero.
- Verifies that subsidy stays zero for another 3 069 999 blocks.

Performance measurements

</summary>

With variable CPU performance:

time src/test/test_bitcoin -t validation_tests/subsidy_sum_test
...
real	0m0.722s
user	0m0.705s
sys	0m0.016s

With fixed CPU perfomance:

time src/test/test_bitcoin -t validation_tests/subsidy_sum_test
...
real	0m0.872s
user	0m0.848s
sys	0m0.022s

hyperfine "src/test/test_bitcoin -t validation_tests/subsidy_sum_test"
Benchmark 1: src/test/test_bitcoin -t validation_tests/subsidy_sum_test
  Time (mean ± σ):     872.2 ms ±   2.9 ms    [User: 848.4 ms, System: 21.3 ms]
  Range (min … max):   869.2 ms … 877.7 ms    10 runs

</details>

DrahtBot requested review from brunoerg on Aug 29, 2024

l0rinc requested review from fjahr on Sep 9, 2024

l0rinc commented at 7:55 PM on September 9, 2024: contributor

@fjahr, @brunoerg, please reevaluate the concept NACKs. I'm no longer modifying the original test. The point of the new one is to avoid skipping any blocks and to validate strict monotonicity after reaching 0 — i.e., a more thorough validation than before.

fjahr commented at 8:08 PM on September 9, 2024: contributor

I'm no longer modifying the original test.

You are still modifying the original test and even if you would remove those modifications I still think this is not worth the effort of review. Your new test doesn't add anything of substantial value and I would not be surprised if in a few weeks someone opens a PR to remove the first test because there is already another test that does the same thing which leads to the same result as a rewrite. Then we will have to have another argument about the same topic. It's a waste of time.

l0rinc commented at 8:22 PM on September 9, 2024: contributor

You are still modifying the original test

No, it does the same steps as before.

I still think this is not worth the effort of review

Your NACK will prevent others from exerting that effort, so what's the real reason for treating this area differently than the rest of the codebase? I've explained my reasoning for why I think we need to be more thorough, hence my PR.

fjahr commented at 8:51 PM on September 9, 2024: contributor

Since I didn't make it explicit before: Approach NACK to having duplicate tests with minor differences in the codebase. Aside from duplication generally being bad I explained above how this will just introduce new discussion-heavy PRs and might lead to the same outcome as the direct refactoring with an indirection in between. This means if the refactoring is rejected this is not a viable approach either.

I still think this is not worth the effort of review

Your NACK will prevent others from exerting that effort, so what's the real reason for treating this area differently than the rest of the codebase? I've explained my reasoning for why I think we need to be more thorough, hence my PR.

You have given your reasoning and then you have seen multiple seasoned contributors here giving feedback that this is not an effort worthwhile. Yet you seem to be determined to make changes to this very critical test somehow. But the good news is: my NACK does not prevent anyone from reviewing this and supporting you. And if there is enough support it will get merged eventually. This is rough consensus.

so what's the real reason for treating this area differently than the rest of the codebase?

What do you mean by this? Where else in the code do we have duplicate functions or tests with just minor differences? PRs get closed all the time because they don't provide enough value. Please give specific examples and I can tell you why my review was different in these other cases.

This is nothing personal by the way. I have talked about how minor refactoring PRs are slowing down the project a long time ago, for example: https://btctranscripts.com/bitcoin-core-dev-tech/2023-04/2023-04-25-refactors/

l0rinc commented at 8:58 PM on September 9, 2024: contributor

Ok, thanks for taking the time to explain it. I don't want others to approve it if you've NACK-ed it. While I don't fully understand the reasoning, I respect your decision, so I'll just close this off instead.

l0rinc closed this on Sep 9, 2024

l0rinc deleted the branch on Sep 9, 2024

bitcoin locked this on Sep 9, 2025