-
Notifications
You must be signed in to change notification settings - Fork 38.8k
Rename MAX_MONEY to AMOUNT_OVERFLOW_PROTECTION_THRESHOLD #6197
New issue
Have a question about this project? Sign up for a free GitHub account to open an issue and contact its maintainers and the community.
By clicking “Sign up for GitHub”, you agree to our terms of service and privacy statement. We’ll occasionally send you account related emails.
Already on GitHub? Sign in to your account
Conversation
|
NAK, see #6198, less invasive. |
|
Either is fine for me.
|
|
I also prefer Peter's comment over changing so many files. |
|
The diff is only 12 lines over 7 files. Concept ACK, although it's currently failing Travis... |
|
Keep in mind that this constant is already named "MAX_MONEY" in many other 3rd party libraries, including bitcoinj, libbitcoin, python-bitcoinlib, etc. etc. Renaming it is needlessly confusing. |
|
re: other implementations, that includes a fair number of Bitcoin clients as well. For example: https://github.com/vinumeris/lighthouse/blob/63b35ca3abc6ec18e346d9eeb56462b5f35c6066/client/src/main/java/lighthouse/utils/BitcoinValue.java Also, it's good practice when you do things like take others' commits and add them to your own pull-reqs to credit appropriately. git cherry-pick is useful for that purpose. |
|
@petertodd The more reason to correct it, and disrupt the ripple effect — no pun intended. The credit for the comment is yours, of course, but it could not be adopted verbatim, due to the const reference. I expect the two to be squashed anyhow. |
|
Either is fine with me too, although I have a slight preference for @petertodd 's solution due to lower impact. |
|
The word "money" doesn't bother me at all, but I'd there was consensus to change it, the phrase MAX_VALUE is much less awkward. "Max upper bound on the value you will see in any one place; used as a (consensus critical) sanity check in various places, e.g. the total sum out of transaction txout" |
|
Heh. We had that discussion once; MAX_VALUE and MAX_AMOUNT are too generic, could refer to everything, and could easily collide with other things. At least this long awkward constant name is impossible to mix up. |
|
I prefer the less invasive approach of adding a comment as well. The name |
|
@davecgh I'd rather view invasiveness (12 lines over 7 files, as maaku pointed out) as no excuse for canonizing bugs, and that's what MAX_MONEY is. sipa's const and petertodd's comment are an imperfect step, but one in the right direction. As a matter of fact, sipa's choice of words was masterful here, and may survive the test of time as the const gets refactored as its taint inevitably will. |
|
Tested ACK. |
|
@21E14 BTW, |
|
Needs rebase |
|
Rebased. |
|
The new name really irks me. @petertodd's comment update is good enough, everyone that wonders what the constant does can now easily look it up, without having to embed it all into the identifier. |
Implemented as per sipa's suggestion. Was meant to be merged via #6191 had it not be closed out a little too expeditiously. It's not perfect, but it's good enough.