Skip to content

Conversation

@ghost
Copy link

@ghost ghost commented May 27, 2015

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.

@petertodd
Copy link
Contributor

NAK, see #6198, less invasive.

@sipa
Copy link
Member

sipa commented May 28, 2015 via email

@paveljanik
Copy link
Contributor

I also prefer Peter's comment over changing so many files.

@maaku
Copy link
Contributor

maaku commented May 28, 2015

The diff is only 12 lines over 7 files. Concept ACK, although it's currently failing Travis...

@petertodd
Copy link
Contributor

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.

@ghost
Copy link
Author

ghost commented May 28, 2015

The #6198 comment will make a nice addition this PR (#6197), which has now been updated accordingly. Sipa's AMOUNT_OVERFLOW_PROTECTION_THRESHOLD is an elegant way to resolve the controversy. I see other implementations as no reason to perpetuate it.

@petertodd
Copy link
Contributor

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.

@ghost
Copy link
Author

ghost commented May 29, 2015

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

@laanwj
Copy link
Member

laanwj commented May 29, 2015

Either is fine with me too, although I have a slight preference for @petertodd 's solution due to lower impact.
The constant is an upper bound on the possible coins, not the limit value of the cumulative reward function, so MAX_xxx is sort of appropriate, the new name is a bit awkward and pedantic (but correct). Getting rid of the word money on the other hand is nice...

@petertodd
Copy link
Contributor

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"

@laanwj
Copy link
Member

laanwj commented May 29, 2015

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.

@davecgh
Copy link
Contributor

davecgh commented May 29, 2015

I prefer the less invasive approach of adding a comment as well. The name MAX_XYZ is appropriate given how the constant is used. Also, I find the name in this pr awkward and way long at 36 characters, which is completely contrary to elegant for me.

@ghost
Copy link
Author

ghost commented May 29, 2015

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

@maaku
Copy link
Contributor

maaku commented May 30, 2015

Tested ACK.

@paveljanik
Copy link
Contributor

@21E14 BTW, MoneyRange() is OK for you?

@maaku
Copy link
Contributor

maaku commented Jun 1, 2015

Needs rebase

@ghost
Copy link
Author

ghost commented Jun 2, 2015

Rebased.

@laanwj
Copy link
Member

laanwj commented Jun 12, 2015

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

@laanwj laanwj closed this Jun 12, 2015
@bitcoin bitcoin locked as resolved and limited conversation to collaborators Sep 8, 2021
Sign up for free to subscribe to this conversation on GitHub. Already have an account? Sign in.

Labels

None yet

Projects

None yet

Development

Successfully merging this pull request may close these issues.

6 participants