-
Notifications
You must be signed in to change notification settings - Fork 790
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
Use max config difficulty internally #2218
Use max config difficulty internally #2218
Conversation
I'm not sure we should have floating point literals in config files, different parsers may treat them differently and it is likely to be locale dependent (also relevant with toml transition). Could we serialize as string (like we do with bignums) to a specific format that the node knows how to parse across all platforms/independent of locale/parser? |
@cryptocode can we probably use int then? |
I think for this multiplier it would be fine to use int but all our multipliers are floats. How would it work with |
@guilhermelawless it may or may not work in toml and it it'll depend on parser and locale. My locale has "," as decimal separator and of course it fails to parse that. Also, reading 1.35 and writing it out gets me 1.3500000000000001 as it cannot be represented accurately (a potential problem for future migrations where we wanna compare against defaults?) We don't use floats in other parts of the config and I personally think we should avoid it. Safest would be int or a string representation - the config comment will make it clear what the data type is. We can try to make it work with doubles, but I'm skeptical 😅 |
I'm ok with string and using the same for future floats. |
Does it make sense to use |
@guilhermelawless is to_string locale specific? I cannot confirm or deny it in tests. More important is if decoder is locale specific |
@SergiySW C++ defaults to the C classic locale unless explicitly set otherwise: http://www.cplusplus.com/reference/locale/locale/ And from http://www.cplusplus.com/reference/clocale/
@cryptocode what makes the parser you tested locale specific? |
@guilhermelawless I think that's the issue - this particular parser doesn't honor my locale (and it's not set up to pick up LC_NUMERIC on launch). Some people are naturally going to put decimal numbers in the config file using their locale's format, not C classic locale. So ideally the system locale should be used, but probably need ICU to get that right. Some systems allows both decimal point characters regardless of locale, which might be a good idea. Or... we can accept only C classic locale and ignore OS locale and have people deal with the parse error if they use the "wrong" format. In that case, we can likely use decimals directly instead of stringifying the setting. Ugh. |
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
edit: This now needs to (de)serialize toml instead (no json or upgrade)
b682ce2
to
9b4c96c
Compare
…enerate_multiplier
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
LGTM
…enerate_multiplier
nanocurrency/nano-node#2218 This change was made after TOML upgrade, updating config.json to provide setting overview
#121) * Use max config difficulty internally (PR#2218) nanocurrency/nano-node#2218 This change was made after TOML upgrade, updating config.json to provide setting overview * Add frontiers confirmation modes (PR#2175) nanocurrency/nano-node#2175 This change was made after TOML upgrade, updating config.json to provide setting overview
Instead of only RPC "work_generate"
Also max_difficulty converted to max_work_generate_multiplier for better readability