Remove strtol in torcontrol #23538

The sequence of octal chars is fully validated before calling strtol, so it can be replaced by a simple loop. This removes the last "locale depended" strtol call. Also, removes some unused includes.

Concept ACK.

Cppreference.com: std::from_chars_result from_chars(...): locale independence refers to code initially proposed and later removed completely.

https://github.com/bitcoin/bitcoin/pull/23538/checks?check_run_id=4242747766

torcontrol.cpp:279:49: runtime error: implicit conversion from type 'unsigned char' of value 255 (8-bit, unsigned) to type 'char' changed the value to -1 (8-bit, signed)
    [#0](/bitcoin-bitcoin/0/) 0x557c2b741c35 in ParseTorReplyMapping(std::__cxx11::basic_string<char, std::char_traits<char>, std::allocator<char> > const&) src/torcontrol.cpp:279:49
    [#1](/bitcoin-bitcoin/1/) 0x557c2ad31efb in torcontrol_tests::CheckParseTorReplyMapping(std::__cxx11::basic_string<char, std::char_traits<char>, std::allocator<char> >, std::map<std::__cxx11::basic_string<char, std::char_traits<char>, std::allocator<char> >, std::__cxx11::basic_string<char, std::char_traits<char>, std::allocator<char> >, std::less<std::__cxx11::basic_string<char, std::char_traits<char>, std::allocator<char> > >, std::allocator<std::pair<std::__cxx11::basic_string<char, std::char_traits<char>, std::allocator<char> > const, std::__cxx11::basic_string<char, std::char_traits<char>, std::allocator<char> > > > >) src/test/torcontrol_tests.cpp:61:16
    [#2](/bitcoin-bitcoin/2/) 0x557c2ad2efd8 in torcontrol_tests::util_ParseTorReplyMapping::test_method() src/test/torcontrol_tests.cpp:150:5
    [#3](/bitcoin-bitcoin/3/) 0x557c2ad2ceea in torcontrol_tests::util_ParseTorReplyMapping_invoker() src/test/torcontrol_tests.cpp:73:1
    [#4](/bitcoin-bitcoin/4/) 0x557c2a46a89f in boost::detail::function::void_function_invoker0<void (*)(), void>::invoke(boost::detail::function::function_buffer&) /usr/include/boost/function/function_template.hpp:117:11
    [#5](/bitcoin-bitcoin/5/) 0x7fad31b494c1  (/lib/x86_64-linux-gnu/libboost_unit_test_framework.so.1.74.0+0x274c1)
    [#6](/bitcoin-bitcoin/6/) 0x7fad31b4f410 in boost::execution_monitor::catch_signals(boost::function<int ()> const&) (/lib/x86_64-linux-gnu/libboost_unit_test_framework.so.1.74.0+0x2d410)
    [#7](/bitcoin-bitcoin/7/) 0x7fad31b4f900 in boost::execution_monitor::execute(boost::function<int ()> const&) (/lib/x86_64-linux-gnu/libboost_unit_test_framework.so.1.74.0+0x2d900)
    [#8](/bitcoin-bitcoin/8/) 0x7fad31b4f9bb in boost::execution_monitor::vexecute(boost::function<void ()> const&) (/lib/x86_64-linux-gnu/libboost_unit_test_framework.so.1.74.0+0x2d9bb)
    [#9](/bitcoin-bitcoin/9/) 0x7fad31b73876 in boost::unit_test::unit_test_monitor_t::execute_and_translate(boost::function<void ()> const&, unsigned long) (/lib/x86_64-linux-gnu/libboost_unit_test_framework.so.1.74.0+0x51876)
    [#10](/bitcoin-bitcoin/10/) 0x7fad31b925a6  (/lib/x86_64-linux-gnu/libboost_unit_test_framework.so.1.74.0+0x705a6)
    [#11](/bitcoin-bitcoin/11/) 0x7fad31b92bf3  (/lib/x86_64-linux-gnu/libboost_unit_test_framework.so.1.74.0+0x70bf3)
    [#12](/bitcoin-bitcoin/12/) 0x7fad31b92bf3  (/lib/x86_64-linux-gnu/libboost_unit_test_framework.so.1.74.0+0x70bf3)
    [#13](/bitcoin-bitcoin/13/) 0x7fad31b5a79a in boost::unit_test::framework::run(unsigned long, bool) (/lib/x86_64-linux-gnu/libboost_unit_test_framework.so.1.74.0+0x3879a)
    [#14](/bitcoin-bitcoin/14/) 0x7fad31b6c6f9 in boost::unit_test::unit_test_main(bool (*)(), int, char**) (/lib/x86_64-linux-gnu/libboost_unit_test_framework.so.1.74.0+0x4a6f9)
    [#15](/bitcoin-bitcoin/15/) 0x557c2a39d83d in main /usr/include/boost/test/unit_test.hpp:64:12
    [#16](/bitcoin-bitcoin/16/) 0x7fad311bffcf  (/lib/x86_64-linux-gnu/libc.so.6+0x2dfcf)
    [#17](/bitcoin-bitcoin/17/) 0x7fad311c007c in __libc_start_main (/lib/x86_64-linux-gnu/libc.so.6+0x2e07c)
    [#18](/bitcoin-bitcoin/18/) 0x557c2a2ec974 in _start (/tmp/cirrus-ci-build/ci/scratch/build/bitcoin-x86_64-pc-linux-gnu/src/test/test_bitcoin+0x10d3974)

SUMMARY: UndefinedBehaviorSanitizer: implicit-integer-sign-change torcontrol.cpp:279:49 in 
make[3]: *** [Makefile:19175: test/torcontrol_tests.cpp.test] Error 1

<!--e57a25ab6845829454e8d69fc972939a-->

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

<!--174a7506f384e20aa4161008e828411d-->

Conflicts

Reviewers, this pull request conflicts with the following ones:

• #22953 (refactor: introduce single-separator split helper (boost::split replacement) by theStack)

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.

Concept ACK. I wondered how this ended up in the code (I definitely didn't remember writing it), but looks like the strtol was introduced in 49a199bb51fc00659f8134e5b16f5d36364b0554

honestly, I kind of wonder why this isn't implemented with a little

num *= 8;
num += val - '0';

kind of loop. Wrapping it in a std::string then passing it to a parsing function seems overkill, especially if no validation nor special whitespace handling is needed, and it already iterates over every character.

Wrapping it in a std::string then passing it to a parsing function seems overkill, especially if no validation nor special whitespace handling is needed, and it already iterates over every character.

The validation pass needs to be a different loop than the calculation loop, so I think a one-line function call is fine. If speed is a concern, ToIntegral could be changed to use std::string_view instead.

Though, changed to a while loop for now.

Code review and tested ACK fa186eb7f45a9d420503f96ff3a5f753afb75dec

If speed is a concern, ToIntegral could be changed to use std::string_view instead.

I don't think speed is a concern, the whole construction just looked fragile and bolted on to me, I think it's much clearer what the code does and what the intent is this way.

It looks like this code is sufficiently tested in util_ParseTorReplyMapping.

I've done some manual testing, too, in this way:

$ src/bitcoind -regtest -listenonion=1 -listen=1 -debug=tor
⋮ 
2021-11-25T10:34:45Z tor: Connected to Tor version 0.4.6.7
2021-11-25T10:34:45Z tor: Supported authentication method: SAFECOOKIE
2021-11-25T10:34:45Z tor: Using SAFECOOKIE authentication, reading cookie authentication from /tmp/💜
2021-11-25T10:34:45Z tor: Authentication cookie /tmp/💜 could not be opened (check permissions)
2021-11-25T10:34:56Z tor: End of stream

$ nc -l -C -p 9051
PROTOCOLINFO 1
250-PROTOCOLINFO 1    
250-AUTH METHODS=SAFECOOKIE COOKIEFILE="/tmp/\360\237\222\234"
250-VERSION Tor="0.4.6.7"
250 OK