Rename MAX_MONEY to AMOUNT_OVERFLOW_PROTECTION_THRESHOLD#6197
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.