Remove strtol in torcontrol

[maflcko]

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.

[katesalazar]

Concept ACK.

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

[fanquake]

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 0x557c2b741c35 in ParseTorReplyMapping(std::__cxx11::basic_string<char, std::char_traits<char>, std::allocator<char> > const&) src/torcontrol.cpp:279:49
    #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 0x557c2ad2efd8 in torcontrol_tests::util_ParseTorReplyMapping::test_method() src/test/torcontrol_tests.cpp:150:5
    #3 0x557c2ad2ceea in torcontrol_tests::util_ParseTorReplyMapping_invoker() src/test/torcontrol_tests.cpp:73:1
    #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 0x7fad31b494c1  (/lib/x86_64-linux-gnu/libboost_unit_test_framework.so.1.74.0+0x274c1)
    #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 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 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 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 0x7fad31b925a6  (/lib/x86_64-linux-gnu/libboost_unit_test_framework.so.1.74.0+0x705a6)
    #11 0x7fad31b92bf3  (/lib/x86_64-linux-gnu/libboost_unit_test_framework.so.1.74.0+0x70bf3)
    #12 0x7fad31b92bf3  (/lib/x86_64-linux-gnu/libboost_unit_test_framework.so.1.74.0+0x70bf3)
    #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 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 0x557c2a39d83d in main /usr/include/boost/test/unit_test.hpp:64:12
    #16 0x7fad311bffcf  (/lib/x86_64-linux-gnu/libc.so.6+0x2dfcf)
    #17 0x7fad311c007c in __libc_start_main (/lib/x86_64-linux-gnu/libc.so.6+0x2e07c)
    #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

[DrahtBot]

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

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.

[laanwj]

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 49a199b

[laanwj]

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.

[maflcko]

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.

[laanwj]

Code review and tested ACK fa186eb

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.

[laanwj]

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