util: Replace use of locale dependent atoi(…) with locale-independent std::from_chars(…) (C++17) #20452

practicalswift · 2020-11-22T12:05:49Z

Replace use of locale dependent atoi(…) with locale-independent std::from_chars(…) (C++17).

laanwj · 2020-11-22T12:15:15Z

Note that we have a function ParseInt32 (as well as 64 and UInt variants) for this and I once tried to use it in more places, see #17385.

However these didn't turn out to be actually locale-independent. Maybe this can replace them eventually.

src/util/strencodings.h

laanwj · 2020-11-22T12:23:57Z

locale-independent std::from_chars(…) (C++17).

This is good to hear at least. From https://en.cppreference.com/w/cpp/utility/from_chars#Notes :

Unlike other parsing functions in C++ and C libraries, std::from_chars is locale-independent, non-allocating, and non-throwing.

Concept ACK.

src/util/strencodings.h

DrahtBot · 2020-11-22T13:18:44Z

The following sections might be updated with supplementary metadata relevant to reviewers and maintainers.

Conflicts

Reviewers, this pull request conflicts with the following ones:

#15936 (Unify bitcoin-qt and bitcoind persistent settings by ryanofsky)

If you consider this pull request important, please also help to review the conflicting pull requests. Ideally, start with the one that should be merged first.

laanwj · 2020-11-22T13:29:25Z

I think we need to make a choice here (either one or both):

Make an emulation of all broken and surprising atoi behavior, but call it something else than ToIntegral, something like LocaleIndependentAtoi, make it clear it should not be used in new code, it's only meant for preserving atoi behavior for backward compatibility reasons.
Make a sane integer parsing function that can eventually replace Parse[U]IntXX or their contents in a locale-independent way. I'm fine with the name then.

practicalswift · 2020-11-22T14:07:33Z

@laanwj

I think we need to make a choice here (either one or both):

Make an emulation of all broken and surprising atoi behavior, but call it something else than ToIntegral, something like LocaleIndependentAtoi, make it clear it should not be used in new code, it's only meant for preserving atoi behavior for backward compatibility reasons.

Make a sane integer parsing function that can eventually replace Parse[U]IntXX or their contents in a locale-independent way. I'm fine with the name then.

Good point. I thought about those options as well. I think we should do both. This PR is meant as a pure refactoring: it is not meant to change any behaviour that is defined by atoi.

I've now made it more clear that this PR is meant as a quirk-by-quirk compatible replacement for atoi by calling the function LocaleIndependentAtoi :)

Good point about naming: we should keep the name ToIntegral for a future sane version that doesn't emulate atoi or any other legacy function.

laanwj · 2020-11-22T17:48:26Z

Okay, agree. I guess it could replace atoi as well as atoi64 in that case (as it's parametrized on type)?

practicalswift · 2020-11-22T19:48:22Z

@laanwj Done! I've now replaced all instances of atoi64(s) with LocaleIndependentAtoi<int64_t>(s).

src/util/strencodings.h

laanwj · 2020-11-23T09:55:20Z

@laanwj Done! I've now replaced all instances of atoi64(s) with LocaleIndependentAtoi<int64_t>(s).

Thanks, looks good to me now except the documentation nit.

… leading +/-/0:s 05c1095 test: Add testing of ParseInt/ParseUInt edge cases with leading +/-/0:s (practicalswift) Pull request description: Add testing of `ParseInt`/`ParseUInt` edge cases with leading `+`/`-`/`0`:s. Context: While working on #20457 and #20452 I noticed some edge cases which our unit tests are currently not covering. ACKs for top commit: MarcoFalke: review ACK 05c1095 laanwj: Code review ACK 05c1095 jonatack: ACK 05c1095 promag: Code review ACK 05c1095. Tree-SHA512: bdfb94d8fa0293512dbba89907cb6dd0f8b1418d878267dd6d49c8c397a0e5b9714441345565d41a6a909a1cda052ef7cccece822f355ff604fcf85f2dc8136f

…es with leading +/-/0:s 05c1095 test: Add testing of ParseInt/ParseUInt edge cases with leading +/-/0:s (practicalswift) Pull request description: Add testing of `ParseInt`/`ParseUInt` edge cases with leading `+`/`-`/`0`:s. Context: While working on bitcoin#20457 and bitcoin#20452 I noticed some edge cases which our unit tests are currently not covering. ACKs for top commit: MarcoFalke: review ACK 05c1095 laanwj: Code review ACK 05c1095 jonatack: ACK 05c1095 promag: Code review ACK 05c1095. Tree-SHA512: bdfb94d8fa0293512dbba89907cb6dd0f8b1418d878267dd6d49c8c397a0e5b9714441345565d41a6a909a1cda052ef7cccece822f355ff604fcf85f2dc8136f

laanwj · 2020-11-25T14:28:35Z

I don't get the CI error. Is charconv non-standard in some way?

In file included from primitives/transaction.cpp:10:0:
./util/strencodings.h:16:10: fatal error: charconv: No such file or directory
 #include <charconv>
          ^~~~~~~~~~
compilation terminated.

…from_chars(…) (C++17) test: Add test cases for LocaleIndependentAtoi fuzz: Assert legacy atoi(s) == LocaleIndependentAtoi<int>(s) fuzz: Assert legacy atoi64(s) == LocaleIndependentAtoi<int64_t>(s)

jonatack · 2021-09-30T15:33:35Z

Concept ACK

laanwj · 2021-09-30T16:57:22Z

Code review ACK 4343f11

jonatack

Code review ACK 4343f11

src/rpc/mining.cpp

laanwj · 2021-10-01T07:01:23Z

src/util/strencodings.h

+// LocaleIndependentAtoi is provided for backwards compatibility reasons.
+//
+// New code should use the ParseInt64/ParseUInt64/ParseInt32/ParseUInt32 functions
+// which provide parse error feedback.


This is a really important comment. I hope we can get rid of these functions entirely at some point. The implicit error handling behavior of atoi and atoi64 is pretty much always undesirable.
(so I would normally comment "these function names are long and clunky, please shorten them" but it's good in this case, we want using them to be ugly 😄 )

…th loca…

maflcko

concept ack

src/core_read.cpp

src/qt/rpcconsole.cpp

src/util/strencodings.h

src/util/moneystr.cpp

maflcko · 2021-10-05T11:11:02Z

Follow up in #23184

maflcko

withdrawn my comments

src/core_read.cpp

src/qt/rpcconsole.cpp

src/util/moneystr.cpp

…es with leading +/-/0:s 05c1095 test: Add testing of ParseInt/ParseUInt edge cases with leading +/-/0:s (practicalswift) Pull request description: Add testing of `ParseInt`/`ParseUInt` edge cases with leading `+`/`-`/`0`:s. Context: While working on bitcoin#20457 and bitcoin#20452 I noticed some edge cases which our unit tests are currently not covering. ACKs for top commit: MarcoFalke: review ACK 05c1095 laanwj: Code review ACK 05c1095 jonatack: ACK 05c1095 promag: Code review ACK 05c1095. Tree-SHA512: bdfb94d8fa0293512dbba89907cb6dd0f8b1418d878267dd6d49c8c397a0e5b9714441345565d41a6a909a1cda052ef7cccece822f355ff604fcf85f2dc8136f

The new locale-independent atoi64 method introduced in bitcoin#20452 behaves differently for values passed which are greater than the uint64_t max. This commit is proof of that, meant to spur discussion on how to handle such an incompatibility. The change as committed in bitcoin#20542 may break some scripts which invoke bitcoind, but more than that may have consensus implications as this deserialization mechanism is used in CScript::ParseScript. I think this means it's possible that bitcoin#20542 could have been an accidental soft fork?

Introduce LocaleIndependentAtoi64 which behaves the same way that previous versions of Bitcoin Core has when faced with under- and overflow. This behavior was implicitly changed in bitcoin#20452, but has not yet been included in a release. Attempts to use LocaleIndependentAtoi for int64_t return values will result in a compilation error.

The new locale-independent atoi64 method introduced in bitcoin#20452 behaves differently for values passed which are greater than the uint64_t max. This commit is proof of that, meant to spur discussion on how to handle such an incompatibility. Introduce LocaleIndependentAtoi64 which behaves the same way that previous versions of Bitcoin Core has when faced with under- and overflow. This behavior was implicitly changed in bitcoin#20452, but has not yet been included in a release. Attempts to use LocaleIndependentAtoi for int64_t return values will result in a compilation error.

The new locale-independent atoi64 method introduced in bitcoin#20452 parses large integer values higher than maximum representable value as 0 instead of the maximum value, which breaks backwards compatibility. This commit restores compatibility and adds test coverage for this case. Co-authored-by: James O'Beirne <james.obeirne@pm.me>

The new locale-independent atoi64 method introduced in bitcoin#20452 parses large integer values higher than maximum representable value as 0 instead of the maximum value, which breaks backwards compatibility. This commit restores compatibility and adds test coverage for this case in terms of the related GetIntArg and strtoll functions. Co-authored-by: James O'Beirne <james.obeirne@pm.me>

The new locale-independent atoi64 method introduced in bitcoin#20452 parses large integer values higher than maximum representable value as 0 instead of the maximum value, which breaks backwards compatibility. This commit restores compatibility and adds test coverage for this case in terms of the related GetIntArg and strtoll functions. Co-authored-by: Ryan Ofsky <ryan@ofsky.org>

b5c9bb5 util: Restore GetIntArg saturating behavior (James O'Beirne) Pull request description: The new locale-independent atoi64 method introduced in #20452 parses large integer values higher than maximum representable value as 0 instead of the maximum value, which breaks backwards compatibility. This commit restores compatibility and adds test coverage for this case in terms of the related GetIntArg and strtoll functions. Specifically, command line or bitcoin.conf integer values greater than `9223372036854775807` (`2**63-1`) used to be parsed as `9223372036854775807` before #20452. Then #20452 caused them to be parsed as `0`. And after this PR they will be parsed as `9223372036854775807` again. This change is a stripped-down alternative version of #23841 by jamesob ACKs for top commit: jamesob: Github ACK b5c9bb5 vincenzopalazzo: ACK b5c9bb5 MarcoFalke: review ACK b5c9bb5 🌘 Tree-SHA512: 4e8abdbabf3cf4713cf5a7c5169539159f359ab4109a4e7e644cc2e9b2b0c3c532fad9f6b772daf015e1c5340ce59280cd9a41f2730afda6099cbf636b7d23ae

The new locale-independent atoi64 method introduced in bitcoin#20452 parses large integer values higher than maximum representable value as 0 instead of the maximum value, which breaks backwards compatibility. This commit restores compatibility and adds test coverage for this case in terms of the related GetIntArg and strtoll functions. Co-authored-by: Ryan Ofsky <ryan@ofsky.org>

laanwj reviewed Nov 22, 2020

View reviewed changes

src/util/strencodings.h Show resolved Hide resolved

laanwj reviewed Nov 22, 2020

View reviewed changes

src/util/strencodings.h Show resolved Hide resolved

DrahtBot added GUI P2P Utils/log/libs labels Nov 22, 2020

practicalswift force-pushed the remove-atoi branch from be0efcd to 25ad608 Compare November 22, 2020 14:00

DrahtBot mentioned this pull request Nov 22, 2020

Extend support for nested commands to bitcoin-cli #20273

Closed

practicalswift force-pushed the remove-atoi branch 2 times, most recently from f23f854 to 8c1142e Compare November 22, 2020 17:26

practicalswift force-pushed the remove-atoi branch from 8c1142e to 6a6a71e Compare November 22, 2020 19:36

DrahtBot mentioned this pull request Nov 22, 2020

Replace uses of boost::trim* with locale-independent alternatives #18130

Closed

DrahtBot mentioned this pull request Nov 23, 2020

refactor: Implement missing error checking for ArgsManager flags #16545

Draft

laanwj reviewed Nov 23, 2020

View reviewed changes

src/util/strencodings.h Show resolved Hide resolved

laanwj mentioned this pull request Nov 23, 2020

util: Make Parse{Int,UInt}{32,64} use locale independent std::from_chars(…) (C++17) instead of locale dependent strto{l,ll,ul,ull} #20457

Merged

practicalswift force-pushed the remove-atoi branch 2 times, most recently from f14668e to 04271a9 Compare November 23, 2020 15:50

practicalswift mentioned this pull request Nov 24, 2020

test: Add testing of ParseInt/ParseUInt edge cases with leading +/-/0:s #20472

Merged

practicalswift changed the title ~~Replace use of locale dependent atoi(…) with locale-independent std::from_chars(…) (C++17)~~ util: Replace use of locale dependent atoi(…) with locale-independent std::from_chars(…) (C++17) Nov 24, 2020

practicalswift force-pushed the remove-atoi branch from 04271a9 to 638cfd3 Compare November 24, 2020 15:08

practicalswift force-pushed the remove-atoi branch from 4747efa to 43434b1 Compare September 30, 2021 13:51

Replace use of locale dependent atoi(…) with locale-independent std::…

4343f11

…from_chars(…) (C++17) test: Add test cases for LocaleIndependentAtoi fuzz: Assert legacy atoi(s) == LocaleIndependentAtoi<int>(s) fuzz: Assert legacy atoi64(s) == LocaleIndependentAtoi<int64_t>(s)

practicalswift force-pushed the remove-atoi branch from 43434b1 to 4343f11 Compare September 30, 2021 14:29

jonatack reviewed Sep 30, 2021

View reviewed changes

src/rpc/mining.cpp Show resolved Hide resolved

laanwj reviewed Oct 1, 2021

View reviewed changes

laanwj merged commit cdb4dfc into bitcoin:master Oct 4, 2021

sidhujag pushed a commit to syscoin/syscoin that referenced this pull request Oct 4, 2021

Merge bitcoin#20452: util: Replace use of locale dependent atoi(…) wi…

9506227

…th loca…

maflcko reviewed Oct 5, 2021

View reviewed changes

src/core_read.cpp Show resolved Hide resolved

src/qt/rpcconsole.cpp Show resolved Hide resolved

src/util/strencodings.h Show resolved Hide resolved

src/util/moneystr.cpp Show resolved Hide resolved

maflcko reviewed Oct 5, 2021

View reviewed changes

src/core_read.cpp Show resolved Hide resolved

src/qt/rpcconsole.cpp Show resolved Hide resolved

src/util/moneystr.cpp Show resolved Hide resolved

jamesob mentioned this pull request Dec 22, 2021

Restore atoi64/GetIntArg saturating behavior #23841

Closed

ryanofsky mentioned this pull request Jan 11, 2022

util: Restore GetIntArg saturating behavior #24041

Merged

barton2526 mentioned this pull request Aug 8, 2022

util: Replace use of locale dependent atoi(…) with locale-independent std::from_chars(…) (C++17) gridcoin-community/Gridcoin-Research#2564

Merged

bitcoin locked and limited conversation to collaborators Aug 15, 2022

util: Replace use of locale dependent atoi(…) with locale-independent std::from_chars(…) (C++17) #20452

util: Replace use of locale dependent atoi(…) with locale-independent std::from_chars(…) (C++17) #20452

Uh oh!

Conversation

practicalswift commented Nov 22, 2020

Uh oh!

laanwj commented Nov 22, 2020 • edited Loading Uh oh! There was an error while loading. Please reload this page.

Uh oh!

Uh oh!

Uh oh!

laanwj commented Nov 22, 2020 • edited Loading Uh oh! There was an error while loading. Please reload this page.

Uh oh!

Uh oh!

Uh oh!

DrahtBot commented Nov 22, 2020 • edited Loading Uh oh! There was an error while loading. Please reload this page.

Uh oh!

Conflicts

Uh oh!

laanwj commented Nov 22, 2020 • edited Loading Uh oh! There was an error while loading. Please reload this page.

Uh oh!

Uh oh!

practicalswift commented Nov 22, 2020 • edited Loading Uh oh! There was an error while loading. Please reload this page.

Uh oh!

Uh oh!

laanwj commented Nov 22, 2020

Uh oh!

practicalswift commented Nov 22, 2020

Uh oh!

Uh oh!

laanwj commented Nov 23, 2020

Uh oh!

laanwj commented Nov 25, 2020

Uh oh!

jonatack commented Sep 30, 2021

Uh oh!

laanwj commented Sep 30, 2021

Uh oh!

jonatack left a comment

Choose a reason for hiding this comment

Uh oh!

Uh oh!

laanwj Oct 1, 2021

Choose a reason for hiding this comment

Uh oh!

maflcko left a comment

Choose a reason for hiding this comment

Uh oh!

Uh oh!

Uh oh!

Uh oh!

Uh oh!

maflcko commented Oct 5, 2021

Uh oh!

maflcko left a comment

Choose a reason for hiding this comment

Uh oh!

Uh oh!

Uh oh!

Uh oh!

Uh oh!

laanwj commented Nov 22, 2020 •

edited

Loading

laanwj commented Nov 22, 2020 •

edited

Loading

DrahtBot commented Nov 22, 2020 •

edited

Loading

laanwj commented Nov 22, 2020 •

edited

Loading

practicalswift commented Nov 22, 2020 •

edited

Loading