refactor: improve readability of numeric literals in consensus parameters and network settings #29444

l0rinc commented at 9:28 AM on February 18, 2024: contributor

This commit enhances the readability of large numeric literals across various parts of the Bitcoin Core codebase by introducing thousands separators ('), leveraging the C++14 digit separator feature.

This readability enhancement follows the precedent set in various places within the Bitcoin Core codebase and aligns with ongoing efforts to improve code quality and developer experience: delimiters were already used in a few places, even in comments: <img alt="image" src="https://github.com/bitcoin/bitcoin/assets/1841944/997032db-3595-4f32-a17e-62ec9d3ded82">

DrahtBot commented at 9:28 AM on February 18, 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. A summary of reviews will appear here.

Conflicts

Reviewers, this pull request conflicts with the following ones:

#29432 (Stratum v2 Template Provider (take 3) by Sjors)

If you consider this pull request important, please also help to review the conflicting pull requests. Ideally, start with the one that should be merged first.

Improve readability of numeric literals in consensus parameters

This commit enhances the readability of large numeric literals across various consensus-related constants by introducing thousands separators ('), as per the C++14 digit separator feature. Changes are made in chainparams.cpp, amount.h, and consensus.h to include separators in the definition of constants such as nSubsidyHalvingInterval, COIN, MAX_MONEY, MAX_BLOCK_SERIALIZED_SIZE, MAX_BLOCK_WEIGHT, and MAX_BLOCK_SIGOPS_COST. This update aims to improve code clarity without affecting functionality.

- Utilize the C++14 digit separator to make large numbers more readable.
- Update nSubsidyHalvingInterval in chainparams.cpp for MAIN, TESTNET, and SIGNET chains.
- Enhance clarity of COIN and MAX_MONEY in amount.h.
- Apply readability improvements to MAX_BLOCK_SERIALIZED_SIZE, MAX_BLOCK_WEIGHT, and MAX_BLOCK_SIGOPS_COST in consensus.h.

d3d41e202b

l0rinc force-pushed on Feb 18, 2024

l0rinc renamed this:
~~Improve readability of numeric literals in consensus parameters~~
Improve readability of numeric literals in consensus parameters and network settings
on Feb 18, 2024

fjahr commented at 4:33 PM on February 18, 2024: contributor

Hi @paplorinc, I'm not convinced we need to change the formatting in these particular places. We usually do such refactorings when the code in question is being touched in a PR anyway, otherwise we try to avoid the noise. But I would support it if you open a PR recommending the use of numeric literals in our coding style guidelines (doc/developer-notes.md).

Update developer-notes.md with digit separator recommendation 9d89358f34

l0rinc commented at 5:14 PM on February 18, 2024: contributor

Hey @fjahr, thanks for the quick feedback.

I have targeted these numbers, since they're quoted to normies all the time when asking "so how do you know there isn't an inflation bug in Bitcoin" and I can just show them: <img alt="image" src="https://github.com/bitcoin/bitcoin/assets/1841944/27aaed6f-7223-49a2-b40b-a5a6e71be434">

I think it looks more reassuring if it's organized as such. What do you think? I suspect that some lines will have to be refactored in dedicated PRs, since we won't accidentally just touch these otherwise...

l0rinc renamed this:
~~Improve readability of numeric literals in consensus parameters and network settings~~
refactor: improve readability of numeric literals in consensus parameters and network settings
on Feb 20, 2024

DrahtBot added the label Refactoring on Feb 20, 2024

fanquake commented at 3:31 PM on February 20, 2024: member

Thanks, however we don't generally refactor consensus code for the sake of readability, so we'll leave this as-is for now.

fanquake closed this on Feb 20, 2024

l0rinc commented at 4:02 PM on February 20, 2024: contributor

Hey @fanquake, thanks for your review. Can you tell me when you'd expect consensus code to be changed otherwise?

maflcko commented at 4:17 PM on February 20, 2024: member

Thank you for your contribution. While this stylistic change makes sense on its own, it comes at a cost and risk for the project as a whole. The weak motivation for the change does not justify the burden that it places on the project. A burden could be any of the following:

Time spent on review
Accidental introduction of bugs
(Silent) merge conflicts, either in the branch or a backport branch. Those conflicts demand further developer and reviewer time or introduce bugs.

For more information about refactoring changes and stylistic cleanup, see

Generally, if the style is not mentioned nor enforced by the developer notes, we leave it up to the original author to pick whatever fits them best personally and then leave it that way until the line is touched for other reasons.

Let us know if you have any questions.

l0rinc commented at 4:48 PM on February 20, 2024: contributor

Thanks @maflcko, understood, should we keep https://github.com/bitcoin/bitcoin/pull/29444/commits/9d89358f34416c97bbe6ce4b63b7eaac2c714c54?

l0rinc deleted the branch on Aug 9, 2024

bitcoin locked this on Aug 9, 2025