Refactor: Replace fprintf with tfm::format #16205

This should be a refactor except in the cases where we use the wrong format specifier [1], in which case this patch is a bug fix.

[1] : e.g. depends: Add libevent compatibility patch for windows #8730

Nice. Concept ACK. Wasn't aware we had so many uses of fprintf. We should probably change the linter to prevent it from being used again.

This might do it:

diff --git a/test/lint/lint-locale-dependence.sh b/test/lint/lint-locale-dependence.sh
index 2b6c78c2c841b7cab90ce11b831ea1051118029e..b0c5ce39b1aba1c52b9b23c7451ac46d33cc6c67 100755
--- a/test/lint/lint-locale-dependence.sh
+++ b/test/lint/lint-locale-dependence.sh
@@ -8,7 +8,6 @@ KNOWN_VIOLATIONS=(
     "src/dbwrapper.cpp:.*vsnprintf"
     "src/httprpc.cpp.*trim"
     "src/init.cpp:.*atoi"
-    "src/init.cpp:.*fprintf"
     "src/qt/rpcconsole.cpp:.*atoi"
     "src/rest.cpp:.*strtol"
     "src/test/dbwrapper_tests.cpp:.*snprintf"
@@ -189,8 +188,7 @@ GIT_GREP_OUTPUT=$(git grep -E "[^a-zA-Z0-9_\`'\"<>](${REGEXP_LOCALE_DEPENDENT_FU
 EXIT_CODE=0
 for LOCALE_DEPENDENT_FUNCTION in "${LOCALE_DEPENDENT_FUNCTIONS[@]}"; do
     MATCHES=$(grep -E "[^a-zA-Z0-9_\`'\"<>]${LOCALE_DEPENDENT_FUNCTION}(_r|_s)?[^a-zA-Z0-9_\`'\"<>]" <<< "${GIT_GREP_OUTPUT}" | \
-        grep -vE "\.(c|cpp|h):\s*(//|\*|/\*|\").*${LOCALE_DEPENDENT_FUNCTION}" | \
-        grep -vE 'fprintf\(.*(stdout|stderr)')
+        grep -vE "\.(c|cpp|h):\s*(//|\*|/\*|\").*${LOCALE_DEPENDENT_FUNCTION}")
     if [[ ${REGEXP_IGNORE_EXTERNAL_DEPENDENCIES} != "" ]]; then
         MATCHES=$(grep -vE "${REGEXP_IGNORE_EXTERNAL_DEPENDENCIES}" <<< "${MATCHES}")
     fi

Concept ACK

<!--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:

• #15936 (WIP: Unify bitcoin-qt and bitcoind persistent settings by ryanofsky)
• #15894 (Remove duplicated "Error: " prefix in logs by hebasto)
• #15864 (Fix datadir handling by hebasto)
• #14866 (Improve property evaluation way in bitcoin.conf by AkioNak)
• #10102 ([experimental] Multiprocess bitcoin 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.

Concept ACK, will test in a bit.

ACK fa8f195195945ce6258199af0461e3fbfbc1236d. Ideally this should be rebased before merge.

utACK fa8f195195945ce6258199af0461e3fbfbc1236d

When reviewing please note that tfm::format in contrast to fprintf may throw (tinyformat::format_error). That is not necessarily a problem obviously, but it is worth to have in mind when analysing the change in possible code paths from this change.

ACK https://github.com/bitcoin/bitcoin/pull/16205/commits/fa8f195195945ce6258199af0461e3fbfbc1236d

When reviewing please note that tfm::format in contrast to fprintf may throw (tinyformat::format_error). That is not necessarily a problem obviously, but it is worth to have in mind when analysing the change in possible code paths from this change.

Another thing to be careful of is to not use stderr and std::cerr in a mixed way due to potential buffering issues. So it's essential to do this switch in one go (which is the intent so that's fine).

code review and lightly tested ACK fa8f195195945ce6258199af0461e3fbfbc1236d

ACK fa8f195195945ce6258199af0461e3fbfbc1236d from light code review, building, and running linter/unit tests/extended functional tests.

Post-merge utACK

Looks like this accidentally fixed multiple test failures with the cross-compiled windows binaries:

• feature_config_args:

Traceback (most recent call last):
  File "C:\Users\travis\build\MarcoFalke\btc_nightly\bitcoin\test\functional\test_framework\test_framework.py", line 193, in main
    self.run_test()
  File "C:\Users\travis\build\MarcoFalke\btc_nightly\bitcoin\/test/functional/feature_config_args.py", line 67, in run_test
    self.test_config_file_parser()
  File "C:\Users\travis\build\MarcoFalke\btc_nightly\bitcoin\/test/functional/feature_config_args.py", line 57, in test_config_file_parser
    self.nodes[0].stop_node(expected_stderr='Warning: ' + inc_conf_file_path + ':1 Section [testnot] is not recognized.' + os.linesep + 'Warning: ' + inc_conf_file2_path + ':1 Section [testnet] is not recognized.')
  File "C:\Users\travis\build\MarcoFalke\btc_nightly\bitcoin\test\functional\test_framework\test_node.py", line 276, in stop_node
    raise AssertionError("Unexpected stderr {} != {}".format(stderr, expected_stderr))
AssertionError: Unexpected stderr  != Warning: C:\Users\travis\AppData\Local\Temp\test_runner_₿_🏃_20190617_121557\feature_config_args_3\node0\include.conf:1 Section [testnot] is not recognized.

• feature_includeconf

Traceback (most recent call last):
  File "C:\Users\travis\build\MarcoFalke\btc_nightly\bitcoin\test\functional\test_framework\test_framework.py", line 193, in main
    self.run_test()
  File "C:\Users\travis\build\MarcoFalke\btc_nightly\bitcoin\/test/functional/feature_includeconf.py", line 55, in run_test
    self.stop_node(0, expected_stderr="warning: -includeconf cannot be used from included files; ignoring -includeconf=relative2.conf")
  File "C:\Users\travis\build\MarcoFalke\btc_nightly\bitcoin\test\functional\test_framework\test_framework.py", line 390, in stop_node
    self.nodes[i].stop_node(expected_stderr, wait=wait)
  File "C:\Users\travis\build\MarcoFalke\btc_nightly\bitcoin\test\functional\test_framework\test_node.py", line 276, in stop_node
    raise AssertionError("Unexpected stderr {} != {}".format(stderr, expected_stderr))
AssertionError: Unexpected stderr  != warning: -includeconf cannot be used from included files; ignoring -includeconf=relative2.conf

• tool_wallet

Traceback (most recent call last):
  File "C:\Users\travis\build\MarcoFalke\btc_nightly\bitcoin\test\functional\test_framework\test_framework.py", line 193, in main
    self.run_test()
  File "C:\Users\travis\build\MarcoFalke\btc_nightly\bitcoin\/test/functional/tool_wallet.py", line 61, in run_test
    self.assert_tool_output(out, '-wallet=wallet.dat', 'info')
  File "C:\Users\travis\build\MarcoFalke\btc_nightly\bitcoin\/test/functional/tool_wallet.py", line 37, in assert_tool_output
    assert_equal(stdout, output)
  File "C:\Users\travis\build\MarcoFalke\btc_nightly\bitcoin\test\functional\test_framework\util.py", line 39, in assert_equal
    raise AssertionError("not(%s)" % " == ".join(str(arg) for arg in (thing1, thing2) + args))
AssertionError: not(Wallet info
===========
Encrypted: no
HD (hd seed available): yes
Keypool Size: 2
Transactions: zu
Address Book: zu
 == Wallet info
===========
Encrypted: no
HD (hd seed available): yes
Keypool Size: 2
Transactions: 0
Address Book: 3
)

I am going to backport this, since it turned out to be a bugfix