Updated 4a15ddc -> 6184764 (pr/green.1 -> pr/green.2, compare) fixing a comment typo.

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

Conflicts

No conflicts as of last run.

Concept ACK

Concept ACK, if jamesob is ok that you are taking this over.

I will never not be honored by Ryan Ofsky taking over one of my changes.

But here's the thing - the reason I didn't go this route in the original PR is because it's doing what I was trying to avoid in the first place, which is changing behavior. This is evidenced by the changes to existing tests that accompany this PR.

IMO we shouldn't change number parsing without some kind of deprecation warning. But I guess it's okay if we can make some good arguments that we're reasonably sure there are no existing uses of non-int64 atoi parsing that will fail if saturation behavior changes?

Updated 6184764 -> d5c0e2d (pr/green.2 -> pr/green.3, compare) updating fuzz test, and fixing include error https://cirrus-ci.com/task/4726914576285696 and MSVC error https://cirrus-ci.com/task/5712076994772992

But here's the thing - the reason I didn't go this route in the original PR is because it's doing what I was trying to avoid in the first place, which is changing behavior.

Oh, I guess atoi out of range behavior is UB (unlike stoll) so maybe this isn't actually changing any defined behavior, in which case I'm ACK.

But here's the thing - the reason I didn't go this route in the original PR is because it's doing what I was trying to avoid in the first place, which is changing behavior. This is evidenced by the changes to existing tests that accompany this PR.

It's definitely changing behavior relative to #20452 and the new tests added in #20452. I'm actually not sure if this is changing behavior from before #20452. If it is changing behavior from before #20452, I think it's just platform specific behavior.

Updated d5c0e2d -> 389c47a (pr/green.3 -> pr/green.4, compare) to fix CI errors about unused variable in fuzz test https://cirrus-ci.com/task/6214569847685120, https://cirrus-ci.com/task/6724743242973184, https://cirrus-ci.com/task/5598843336130560. This is an expedient fix since I'm not totally sure what kind of fuzz coverage is useful. Maybe it is good to keep compare three integer parsing functions against each other instead of only comparing two.

ACK pending CI pass

Updated 389c47a -> 1b0b1dd (pr/green.4 -> pr/green.5, compare) fixing typo in fuzz test that led to unfixed CI errors https://cirrus-ci.com/task/6099332351918080 https://cirrus-ci.com/task/5817857375207424 https://cirrus-ci.com/task/4691957468364800

i dont know enough about string parsing to be super useful, but based on the breaking changes analysis & trusting the implementations are OK this approach has my concept ACK.

Updated 1b0b1dd -> b5c9bb5 (pr/green.5 -> pr/green.6, compare) dropping fuzz check to avoid depending on undefined atoi behavior https://cirrus-ci.com/task/5686396143796224

Could update OP to clarify how the intermediate behavior changes cancel each other out?

(initial state) ----------------> (midstate) --------------------> (end result)
atoi UB         ----- 20452 ----> atoi 0     ----- this pull ----> atoi saturate
atoi64 saturate ----- 20452 ----> atoi64 0   ----- this pull ----> atoi64 saturate

Could update OP to clarify how the intermediate behavior changes cancel each other out?

I'm not sure I followed the diagram, but I added a sentence to the OP about what previous behavior was and how this restores it. Feel free to edit OP directly if you were asking for something different or want to change anything.

Github ACK b5c9bb5

There is still another behaviour change:

123a would be parsed as 123.

Now it is mapped to 0.

Edit: Wrong. See discussion below.

@MarcoFalke that is due to #20452, correct?

Yes, everything is due to 20452

There is still another behaviour change:

123a would be parsed as 123.

Now it is mapped to 0.

This is surprising. std::from_chars "expects the pattern identical to the one used by std::strtol" and returns the "first character not matching the pattern." And std::strtol accepts a pattern that "takes as many characters as possible to form a valid base-n (where n=base) integer number representation". So I'd expect 123a to be parsed as 123. But we should have test coverage for this, whether or not there is a bug.

strtol has error reporting for this case, but atoi64 disables that by setting nullptr. I expect you'll have to call from_chars twice on the string (the second time passing in the returned string) to emulate that.

strtol has error reporting for this case

Where do you see that strtol would return an error for this case? That seems counter to the documentation https://en.cppreference.com/w/cpp/string/byte/strtol#Return_value

I'm also not able to reproduce a bug. For example the following test passes on master (290ff5e)

diff --git a/src/test/getarg_tests.cpp b/src/test/getarg_tests.cpp
index d5142c8d748..5286aa6d812 100644
--- a/src/test/getarg_tests.cpp
+++ b/src/test/getarg_tests.cpp
@@ -154,6 +154,9 @@ BOOST_AUTO_TEST_CASE(intarg)
     BOOST_CHECK_EQUAL(m_local_args.GetIntArg("-foo", 0), 11);
     BOOST_CHECK_EQUAL(m_local_args.GetIntArg("-bar", 11), 12);
 
+    ResetArgs("-foo=123a");
+    BOOST_CHECK_EQUAL(m_local_args.GetIntArg("-foo", 456), 123);
+
     ResetArgs("-foo=NaN -bar=NotANumber");
     BOOST_CHECK_EQUAL(m_local_args.GetIntArg("-foo", 1), 0);
     BOOST_CHECK_EQUAL(m_local_args.GetIntArg("-bar", 11), 0);

make -C src test/test_bitcoin && src/test/test_bitcoin -l test_suite -t getarg_tests

Oh, right. I had a typo in my own test and it didn't show the wrong line due to the std::map indirection, but now it passes:

diff --git a/src/test/util_tests.cpp b/src/test/util_tests.cpp
index 20d27a237d..280ea7cbd3 100644
--- a/src/test/util_tests.cpp
+++ b/src/test/util_tests.cpp
@@ -1668,6 +1668,9 @@ BOOST_AUTO_TEST_CASE(test_LocaleIndependentAtoi)
         {"-9223372036854775808", -9'223'372'036'854'775'807LL - 1LL},
         {"9223372036854775807", 9'223'372'036'854'775'807},
         {"9223372036854775808", std::numeric_limits<int64_t>::max()},
+        {"+123a", 123},
+        {"123a", 123},
+        {"-123a", -123},
         {"+-", 0},
         {"0x1", 0},
         {"ox1", 0},

<luke-jr> ryanofsky: what is the meaning of "green" in #24041 ?

@luke-jr, it's just a nonsense branch name from frogs are green tangent in previous PR