Skip to content
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

Conversation

SergiySW
Copy link
Contributor

Instead of only RPC "work_generate"
Also max_difficulty converted to max_work_generate_multiplier for better readability

@SergiySW SergiySW added enhancement toml TOML related change labels Aug 15, 2019
@SergiySW SergiySW added this to the V20.0 milestone Aug 15, 2019
@SergiySW SergiySW self-assigned this Aug 15, 2019
@cryptocode
Copy link
Contributor

cryptocode commented Aug 16, 2019

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?

@SergiySW
Copy link
Contributor Author

SergiySW commented Aug 16, 2019

@cryptocode can we probably use int then?

@guilhermelawless
Copy link
Contributor

I think for this multiplier it would be fine to use int but all our multipliers are floats. How would it work with toml ?

@cryptocode
Copy link
Contributor

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

@guilhermelawless
Copy link
Contributor

I'm ok with string and using the same for future floats.

@guilhermelawless
Copy link
Contributor

Does it make sense to use nano::to_string here? https://github.com/nanocurrency/nano-node/blob/master/nano/lib/numbers.cpp#L810

@SergiySW
Copy link
Contributor Author

SergiySW commented Aug 18, 2019

@guilhermelawless is to_string locale specific? I cannot confirm or deny it in tests. More important is if decoder is locale specific

@SergiySW SergiySW added the incomplete This item is incomplete and should not be merged if it is a pull request label Aug 18, 2019
@guilhermelawless
Copy link
Contributor

guilhermelawless commented Aug 18, 2019

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

It is a rather neutral locale which has the same settings across all systems and compilers, and therefore the exact results of a program using this locale are predictable.

@cryptocode what makes the parser you tested locale specific?

@cryptocode
Copy link
Contributor

cryptocode commented Aug 19, 2019

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.

Copy link
Contributor

@cryptocode cryptocode left a 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)

@SergiySW SergiySW force-pushed the config/max_work_generate_multiplier branch from b682ce2 to 9b4c96c Compare August 23, 2019 22:49
@SergiySW SergiySW removed the incomplete This item is incomplete and should not be merged if it is a pull request label Aug 23, 2019
Copy link
Contributor

@guilhermelawless guilhermelawless left a comment

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

LGTM

@guilhermelawless guilhermelawless merged commit 6a9ba89 into nanocurrency:master Aug 24, 2019
SergiySW added a commit to SergiySW/nano-docs that referenced this pull request Sep 26, 2019
nanocurrency/nano-node#2218
This change was made after TOML upgrade, updating config.json to provide setting overview
zhyatt pushed a commit to nanocurrency/nano-docs that referenced this pull request Sep 27, 2019
#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
Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment
Labels
enhancement toml TOML related change
Projects
None yet
Development

Successfully merging this pull request may close these issues.

3 participants