refactor: remove unused c-string variant of atoi64() #19750

This is another micro-PR "removing old cruft with potentially sharp edges" (quote by practicalswift, see #19739). Gets rid of the c-string variant of the function atoi64(), which is only used in fuzzers and on one place with wallet/wallet.h (where it is originally a std::string anyways and uses .c_str() -- this method call can simply be removed.)

ACK 71e0f07e9c5f0aef532b85c04807dcbedd04e0af -- diff looks correct

Thanks for removing legacy code!

Oh wow, how could I have missed this one. ACK 71e0f07e9c5f0aef532b85c04807dcbedd04e0af

Another thing I tried once (see #17385) was to replace all uses of atoi64 with ParseInt64, which does error checking. It's somewhat more involved though, and I eventually closed it due to lack of interest. (that was last year? wow, it feels like a million years ago now)

@laanwj Getting rid of atoi64 would be really nice: atoi64 is surprisingly liberal in what inputs it tries to take on :)

It's a bit sad that your PR #17385 was closed - that PR would have helped us get rid of quite a few usages of locale dependent functions.

(Talking about locale dependence: please consider reviewing #18817 which documents why avoidance of locale dependent functions is crucial in order to not accidentally cause diverge between Bitcoin Core (which does not call setlocale(LC_ALL, "")) and Bitcoin Qt (which does call setlocale(LC_ALL, "") via Qt(!). I think that is a WTH worth documenting at least :))

If someone wants to try tackle the atoi64/ParseInt64 issue the following fuzzer might be helpful:

$ src/test/fuzz/atoi
ParseInt64 failed, but atoi64 parsed "6!@" (length: 3) as 6
ParseInt64 failed, but atoi64 parsed "^K6@3^K" (length: 5) as 6
ParseInt64 failed, but atoi64 parsed "^M1^M" (length: 3) as 1
ParseInt64 failed, but atoi64 parsed "33333333333333333333333333333333" (length: 32) as 9223372036854775807
…

diff --git a/src/Makefile.test.include b/src/Makefile.test.include
index d0f3107e2..277b51090 100644
--- a/src/Makefile.test.include
+++ b/src/Makefile.test.include
@@ -3,6 +3,7 @@
 # file COPYING or http://www.opensource.org/licenses/mit-license.php.

 FUZZ_TARGETS = \
+  test/fuzz/atoi \
   test/fuzz/addition_overflow \
   test/fuzz/addr_info_deserialize \
   test/fuzz/addrdb \
@@ -1137,6 +1138,12 @@ test_fuzz_uint256_deserialize_LDADD = $(FUZZ_SUITE_LD_COMMON)
 test_fuzz_uint256_deserialize_LDFLAGS = $(RELDFLAGS) $(AM_LDFLAGS) $(LIBTOOL_APP_LDFLAGS)
 test_fuzz_uint256_deserialize_SOURCES = test/fuzz/deserialize.cpp

+test_fuzz_atoi_CPPFLAGS = $(AM_CPPFLAGS) $(BITCOIN_INCLUDES)
+test_fuzz_atoi_CXXFLAGS = $(AM_CXXFLAGS) $(PIE_FLAGS)
+test_fuzz_atoi_LDADD = $(FUZZ_SUITE_LD_COMMON)
+test_fuzz_atoi_LDFLAGS = $(RELDFLAGS) $(AM_LDFLAGS) $(LIBTOOL_APP_LDFLAGS)
+test_fuzz_atoi_SOURCES = test/fuzz/atoi.cpp
+
 endif # ENABLE_FUZZ

 nodist_test_test_bitcoin_SOURCES = $(GENERATED_TEST_FILES)
diff --git a/src/test/fuzz/atoi.cpp b/src/test/fuzz/atoi.cpp
new file mode 100644
index 000000000..69fc2845f
--- /dev/null
+++ b/src/test/fuzz/atoi.cpp
@@ -0,0 +1,25 @@
+// Copyright (c) 2020 The Bitcoin Core developers
+// Distributed under the MIT software license, see the accompanying
+// file COPYING or http://www.opensource.org/licenses/mit-license.php.
+
+#include <test/fuzz/fuzz.h>
+#include <test/fuzz/FuzzedDataProvider.h>
+#include <test/fuzz/util.h>
+#include <util/strencodings.h>
+
+#include <iostream>
+#include <cstdint>
+#include <vector>
+
+void test_one_input(const std::vector<uint8_t>& buffer)
+{
+    FuzzedDataProvider fuzzed_data_provider{buffer.data(), buffer.size()};
+    const std::string s = fuzzed_data_provider.ConsumeRandomLengthString(32);
+    const int64_t parsed_using_atoi64 = atoi64(s);
+    int64_t parsed_using_parseint64;
+    if (ParseInt64(s, &parsed_using_parseint64)) {
+        assert(parsed_using_atoi64 == parsed_using_parseint64);
+    } else if (parsed_using_atoi64 != 0) {
+        std::cout << "ParseInt64 failed, but atoi64 parsed \"" << s << "\" (length: " << s.length() << ") as " << parsed_using_atoi64 << std::endl;
+    }
+}

It's a bit sad that your PR #17385 was closed - that PR would have helped us get rid of quite a few usages of locale dependent functions.

Maybe it's sad. I don't know. I've kind of given up on that myself. I think we've eliminated all the places where locale dependence is really bad, and the low-hanging fruit. I'm not sure dwelling on it too much is useful. At some point it becomes just baseless pedantry. E.g.:

I love the clarity of Rust's approach with a 100% deterministic standard library with no locale dependence. You can opt-in to locale specific things for user interaction. But I also know we'll never get there with C++.

That said, I'll try to get around to reviewing your PR.