bug: Fix Download location of Nitro TensorRT on Windows

[Van-QA]

Avoid unexpected run-time crashes from string formatting by enforcing compile-time format string checks for most* tfm::format and strprintf usage, and by returning an error string instead of throwing for a run-time tfm::format_error so callsites don't need to implement this error handling.

This PR should introduce no behaviour change. The main changes are:

• remove the const std::string& tfm::format overload since it's not necessary. Usage of this overload is removed by one of:
  • replacing string concatenation in the format string with just an extra parameter
  • using the bilingual_str overload
  • using the tfm::format_raw functions (only for tinyformat implementation or tests)
• rename the non-compile-time-validated tfm::format overloads to tfm::format_raw, so existing callsites by default use the safer util::ConstevalFormatString tfm::format overloads. Callsites that for some reason don't pass the compile-time checks (such as in bitcoin-cli.cpp) can use tfm::format_raw.
• make tfm::format_error internal to tinyformat and just return error strings instead, so callsites don't need to implement error handling to prevent crashing.

*The bilingual_str format(const bilingual_str& fmt, const Args&... args) is not yet compile-time checked, which means the lint-format-strings.py test can't be removed yet.

Follow-up on https://github.com/bitcoin/bitcoin/pull/30889#discussion_r1766633746, an alternative to #15926.

[DrahtBot]

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

Code Coverage

For detailed information about the code coverage, see the test coverage report.

Reviews

See the guideline for information on the review process.

Type	Reviewers
ACK	maflcko, l0rinc
Concept ACK	hodlinator

If your review is incorrectly listed, please react with 👎 to this comment and the bot will ignore it on the next update.

Conflicts

Reviewers, this pull request conflicts with the following ones:

• #31061 (refactor: Check translatable format strings at compile-time by maflcko)
• #31042 (build: Rename PACKAGE_* variables to CLIENT_* by hebasto)
• #30997 (build: Switch to Qt 6 by hebasto)
• #30930 (netinfo: add peer services column and outbound-only option by jonatack)
• #30221 (wallet: Ensure best block matches wallet scan state by achow101)
• #29936 (fuzz: wallet: add target for CreateTransaction by brunoerg)
• #29418 (rpc: provide per message stats for global traffic via new RPC 'getnetmsgstats' by vasild)
• #28710 (Remove the legacy wallet and BDB dependency by achow101)

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.

[DrahtBot]

🚧 At least one of the CI tasks failed. _{Debug: https://github.com/bitcoin/bitcoin/runs/30382700932}

Hints

Make sure to run all tests locally, according to the documentation.

The failure may happen due to a number of reasons, for example:

• Possibly due to a silent merge conflict (the changes in this pull request being incompatible with the current code in the target branch). If so, make sure to rebase on the latest commit of the target branch.

• A sanitizer issue, which can only be found by compiling with the sanitizer and running the affected test.

• An intermittent issue.

Leave a comment here, if you need help tracking down a confusing failure.

[DrahtBot]

🚧 At least one of the CI tasks failed. _{Debug: https://github.com/bitcoin/bitcoin/runs/30701048188}

Hints

Make sure to run all tests locally, according to the documentation.

The failure may happen due to a number of reasons, for example:

• Possibly due to a silent merge conflict (the changes in this pull request being incompatible with the current code in the target branch). If so, make sure to rebase on the latest commit of the target branch.

• A sanitizer issue, which can only be found by compiling with the sanitizer and running the affected test.

• An intermittent issue.

Leave a comment here, if you need help tracking down a confusing failure.

[stickies-v]

Latest force-push increases the scope of this PR, see updated PR description for an overview. In a nutshell:

• all non-bilingual_str format/strprintf usage is now compile-time checked. Format errors are returned as strings instead of thrown as error, but throwing behaviour can be maintained through tfm::format_raw.
• the const std::string& format overload is removed because it's unnecessary

Thanks a lot @maflcko and @l0rinc for your review!

[stickies-v]

Force-pushed to address @maflcko's comment about generally not letting tinyformat throw format errors, instead of just for the util::ConstevalFormatString overloads. Most notably, this changes behaviour for the bilingual_str format overload to also not throw.

[stickies-v]

Force-pushed to address two nits from @l0rinc - thanks for the review!

[maflcko]

Only changes are some minor style-only test-only fixups.

re-ACK 2e33dbadcf69838c8ea591dccdc666f809c3134a 🔰

Show signature

Signature:

untrusted comment: signature from minisign secret key on empty file; verify via: minisign -Vm "${path_to_any_empty_file}" -P RWTRmVTMeKV5noAMqVlsMugDDCyyTSbA3Re5AkUrhvLVln0tSaFWglOw -x "${path_to_this_whole_four_line_signature_blob}"
RUTRmVTMeKV5npGrKx1nqXCw5zeVHdtdYURB/KlyA/LMFgpNCs+SkW9a8N95d+U4AP1RJMi+krxU1A3Yux4bpwZNLvVBKy0wLgM=
trusted comment: re-ACK 2e33dbadcf69838c8ea591dccdc666f809c3134a 🔰
h0UdH750a2MwfQDDQ6yAM8tgC42014X4xvJwvJx2c8A+MKy2Ic1ul1lWUqsX8+xhu3EQRJWsZpa3GZbGzbsGCg==

[l0rinc]

Most of my observations should rather be addressed in a separate PR (can be merged before or after, I don't mind rebasing, would appreciate a review there). I'm fine with merging this one as is, thanks for the improvement.

ACK 2e33dbadcf69838c8ea591dccdc666f809c3134a

[stickies-v]

Force-pushed to revert the changed behaviour that wraps the format string into quotes, which doesn't play nice with newlines.

[l0rinc]

ACK 4f33e9a85c3a8f546deddc2f78cf74bceff30315

[maflcko]

Only change is https://github.com/bitcoin/bitcoin/pull/30928#discussion_r1781022698

re-ACK 4f33e9a85c3a8f546deddc2f78cf74bceff30315 👾

Show signature

Signature:

untrusted comment: signature from minisign secret key on empty file; verify via: minisign -Vm "${path_to_any_empty_file}" -P RWTRmVTMeKV5noAMqVlsMugDDCyyTSbA3Re5AkUrhvLVln0tSaFWglOw -x "${path_to_this_whole_four_line_signature_blob}"
RUTRmVTMeKV5npGrKx1nqXCw5zeVHdtdYURB/KlyA/LMFgpNCs+SkW9a8N95d+U4AP1RJMi+krxU1A3Yux4bpwZNLvVBKy0wLgM=
trusted comment: re-ACK 4f33e9a85c3a8f546deddc2f78cf74bceff30315 👾
ouSYrq7MNfE0e0Yqx0hbAqK94p/zuOqik+Tjz2hCaVlVZ5IdU1RQWwbMq6dKpL6cn6Szqf0JvwuGbX7mt2ZYAA==

[stickies-v]

It would be cleaner in the implementation to use util::Result instead of overwriting the returned value with the error message

I don't think that's a good approach for our codebase, even if there was minimal code churn. All format strings are known at compile-time, so any formatting errors are programming errors. Handling that with util::Result would be both a cumbersome interface and bad practice imo. We don't need to let callsites handle formatting errors, they should never occur, but if/when they do after all we don't want them to crash the application because they are harmless to program execution, until we fix the error in the code.

The last commit makes it so that none of the format_raw functions throw either?

Sorry, missed that line when updating the PR description, fixed now!

[maflcko]

I agree that util::Result doesn't make any sense here. Apart from the reasons mentioned, it would also make it harder to switch to std::format, as well as being confusing, because no other format lib I am aware of is doing that.

• using the tfm::format_raw functions (only for tinyformat implementation or tests)

In the PR description: I think this should also say or bitcoin-cli.

[stickies-v]

In the PR description: I think this should also say or bitcoin-cli.

bitcoin-cli wasn't using the const std::string& overload which that bulletpoint is about. It is mentioned in the next bulletpoint about compile-time checks:

Callsites that for some reason don't pass the compile-time checks (such as in bitcoin-cli.cpp) can use tfm::format_raw.

[maflcko]

This is a good argument except we do have some strings only known at runtime, notably translations, which probably don't get full testing. Maybe there's something that automatically compares format strings for equivalence across translations though.

If translations are malformed to induce a tinyformat error where one wouldn't be present in the non-translated string, I don't think the solution would to use Result. This seems no different from an otherwise fully tinyformat-valid string that is completely malformed by the translation. Trying to recover from this error in the code at runtime is the wrong approach and it was specifically changed for this reason, see https://github.com/bitcoin/bitcoin/pull/30928#discussion_r1776965132. A better approach would be to do it when updating the translations, or at build-time. But this can be done in a follow-up and shouldn't be a blocker here.

If you are referring to the fact that the non-translated format string in a translation may be malformed: They are still checked by the current python linter, so there shouldn't be an issue either. If there is an issue, it should be fixed in the format string, not with a Result.

One could argue that if we returned util::Result, most call sites would probably opt to print an error + the offending format string to the log anyway, so doing that as a blanket rule for everything is acceptable.

Yes, indeed. I also can't see a reason for a call-site to do anything else here.

[stickies-v]

This is a good argument except we do have some strings only known at runtime, notably translations

I chose my wording "known at compile-time" carefully for exactly this reason: they're known at compile-time, we just choose not to implement the (complete) validation logic because it much more complex than just returning an error string. It's not worth arguing semantics too much, but in my view that's very much still a programming error and not something that callsites should be aware of, let alone handle.

[ryanofsky]

Will not nack this pr since I don't think consequences are great, but I would prefer not to see this PR merged.

I don't think putting "error while formatting log message" in a std::string and then returning that std::string is a good way for a C++ API to return errors, especially errors that are almost certainly bugs in the code. Throwing an exception seems far more appropriate, and more likely to result in bugs being detected and fixed. (Also nit: the wording of that message doesn't make sense because tinyformat is used to format things other than log messages, and is also useful for low level string manipulation and things like formatting numbers or constructing urls)

I understand that practically speaking impact of this change should be small because the new compile time checking is pretty good, but that just means to my mind that there should be less of a need for this PR.

EDIT: I also think the new uses of .c_str() are gross, though I didn't look into why those are necessary and that is not my main objection to this PR.

[hodlinator]

My Concept ACK accepted the current concept, while stating that I see it as one of the least worst alternatives (implying I don't see the error handling part as blocking). I'm happy to get feedback on that but my intent was not to start a debate.

@ryanofsky how would you feel about adding an Assume() statement that could fail Debug-builds when formatting fails?

[ryanofsky]

Thanks, I didn't read the previous discussion history, so my feedback is only based on the implementation of this PR. not on earlier comments.

I think tinyformat's original decision to throw exceptions when invalid format strings were specified was flexible and sound, and the additional consteval checking we were able to add on top of that was a nice improvement that should be extended more places.

I just don't see a need to rip out tinyformat's simple handling mechanism and replace it with more complicated and unusual one, whether it's a self-referencing string apologizing for not being formatted correctly, or an inconsistently enforced check that behaves differently based on build flags.

To put it another way:

• I think adding consteval format string checking was a great improvement that should improve developer experience and catch bugs.
• I think the consteval string checking should be extended more places, for example supporting the _ function to check translated original strings, and maybe even the imported translations of those strings.
• I don't like the runtime behavior change implemented in the fourth commit of this PR, and I'm not sure about the other commits. The only cited benefit in the PR description is "Avoid unexpected run-time crashes from string formatting" but I feel like the way to achieve that is to extend compile time checks more places, not to take a detour into changing runtime behavior.

[hodlinator]

My Concept ACK accepted the current concept, while stating that I see it as one of the least worst alternatives (implying I don't see the error handling part as blocking). I'm happy to get feedback on that but my intent was not to start a debate.

(This part my previous comment was more for maflcko & stickies-v. I think they were interpreting me as being more critical of the error-handling than I intended).

That said, I also see ryanofsky's perspective. The PR is borderline over-incremental instead of extending build time checks.

Worst case example that could help motivate the current PR approach:

A formatting error in a rarely used codepath in the Icelandic translation causes an unhandled exception, leading a lightning node to becoming unable to broadcast a penalty transaction in time. => Angry angry vikings. :crossed_swords:

[DrahtBot]

🚧 At least one of the CI tasks failed. _{Debug: https://github.com/bitcoin/bitcoin/runs/31237946658}

Hints

Try to run the tests locally, according to the documentation. However, a CI failure may still happen due to a number of reasons, for example:

• Possibly due to a silent merge conflict (the changes in this pull request being incompatible with the current code in the target branch). If so, make sure to rebase on the latest commit of the target branch.

• A sanitizer issue, which can only be found by compiling with the sanitizer and running the affected test.

• An intermittent issue.

Leave a comment here, if you need help tracking down a confusing failure.

[stickies-v]

Force-pushed to rebase addressing a merge conflict, preserve tfm::format_error throwing behaviour for DEBUG builds to improve visibility into programming errors before they manifest, and improve an error string.

---

Thank you for your thoughtful review @ryanofsky. tl;dr I share your concerns, but I think I have a different opinion on the trade-offs made.

I think everyone here is in agreement that compile-time checks (ideally with std::format) are the proper way forward, and everything else is a temporary patch. I think there are differing views on what we do in the meanwhile. It appears, especially without std::format and having to deal with unverified translated strings, that implementing bulletproof compile-time checking on format strings is not really feasible or the best use of our time.

I don't think putting "error while formatting log message" in a std::string and then returning that std::string is a good way for a C++ API to return errors, especially errors that are almost certainly bugs in the code.

I agree that returning error strings is not a good way to handle errors in general, but I don't think throwing a std::runtime_error is a good way to deal with programming errors either. Typically, these would be handled with assert statements (which is also tinyformat's default), but crashing the node for malformed (usually) informational strings seems perhaps unnecessarily strict, in similar vein to our usage of CHECK_NONFATAL instead of assert?

Can you agree that the concept of not exposing tfm::format_error to callsites makes sense in our codebase, at least philosophically? Fomat errors should never occur at run-time, even if they still can at present. If you agree with that, then I think it all comes down to what we do before we have full compile-time checking? It appears you prefer keeping the increased visibility when errors occur, whereas in this PR I prioritize not crashing the software in case we do miss it or if it's part of an erroneously translated format string.

I've force-pushed to preserve tfm::format_error throwing behaviour in DEBUG builds to keep more of the visibility without reducing stability. If you still feel like that new behaviour is still not an improvement over master (which is a view I can absolutely understand), I will drop the last commit and rework the first ones a bit. Even though the intent of the original version of this PR was to fix a regression, I'd be happy to change course and keep just the first commits that force more compile-time checks, without changing run-time behaviour, which I think everyone here agrees is an improvement.

EDIT: I also think the new uses of .c_str() are gross, though I didn't look into why those are necessary and that is not my main objection to this PR.

The const std::string& overload was dropped so that the util::ConstevalFormatString overload could be forced for all string literals. Although I don't like the additional .c_str() usage this introduced, it is mostly limited to fuzz and bitcoin-cli, and I think is worth the trade-off?

I think tinyformat's original decision to throw exceptions when invalid format strings were specified was flexible and sound

Note: tinyformat natively asserts that there are no formatting errors, the tfm::format_error is Bitcoin Core-specific.

or an inconsistently enforced check that behaves differently based on build flags.

I don't think this is correct: https://github.com/bitcoin/bitcoin/blob/caf44e500eb257376c36c0e043f71f72b949f743/src/tinyformat.h#L139

(Also nit: the wording of that message doesn't make sense because tinyformat is used to format things other than log messages, and is also useful for low level string manipulation and things like formatting numbers or constructing urls)

Oops, good catch, thanks. This string was lifted from logging.h and I didn't properly update it - done now.

how would you feel about adding an Assume() statement that could fail Debug-builds when formatting fails?

Adding util/check.h to tinyformat.h introduced a couple of circular dependencies so instead I'm using #ifdef DEBUG, but the effect should be the same! (We could expand on this adding a TFM_THROW_FORMAT_ERROR flag (that defaults to set in DEBUG) so it can be added to more CI jobs without overhead if people would like that).

[maflcko]

EDIT: I also think the new uses of .c_str() are gross, though I didn't look into why those are necessary and that is not my main objection to this PR.

I removed tfm::format_raw(fmt.original.c_str(), ... in https://github.com/bitcoin/bitcoin/pull/31061, where it remains just tfm::format(fmt.original, .... Obviously the two pulls conflict a bit, but the conflicts are trivial to solve either way.

[hodlinator]

Worth bringing back the try/catch in src/test/fuzz/strprintf.cpp for Debug builds?

₿ cmake --preset=libfuzzer -DCMAKE_BUILD_TYPE=Debug
...
₿ cmake --build build_fuzz -j<X>
...
₿ FUZZ=str_printf build_fuzz/src/test/fuzz/fuzz ../qa-assets/fuzz_seed_corpus/str_printf/
INFO: Running with entropic power schedule (0xFF, 100).
INFO: Seed: 734467281
INFO: Loaded 1 modules   (1292656 inline 8-bit counters): 1292656 [0x5d5503ee60a0, 0x5d5504021a10), 
INFO: Loaded 1 PC tables (1292656 PCs): 1292656 [0x5d5504021a10,0x5d55053db110), 
INFO:     1133 files found in ../qa-assets/fuzz_seed_corpus/str_printf/
INFO: -max_len is not provided; libFuzzer will not generate inputs larger than 16446 bytes
terminate called after throwing an instance of 'tinyformat::format_error'
  what():  tinyformat: Not enough conversion specifiers in format string
==59713== ERROR: libFuzzer: deadly signal
    #0 0x5d54f8395fea  (/home/hodlinator/bitcoin/build_fuzz/src/test/fuzz/fuzz+0x5bd1fea)
...
    #30 0x5d54f8245174  (/home/hodlinator/bitcoin/build_fuzz/src/test/fuzz/fuzz+0x5a81174)

NOTE: libFuzzer has rudimentary signal handlers.
      Combine libFuzzer with AddressSanitizer or similar for better crash reports.
SUMMARY: libFuzzer: deadly signal
MS: 0 ; base unit: 0000000000000000000000000000000000000000


artifact_prefix='./'; Test unit written to ./crash-da39a3ee5e6b4b0d3255bfef95601890afd80709
Base64: 

[l0rinc]

I'm not in love with the DEBUG separation, but I'm fine with it if others are.

ACK 49f73071a8ec4131595090a619d6198a5f8b7c3c

git range-diff 4f33e9a85c3a8f546deddc2f78cf74bceff30315~4..4f33e9a85c3a8f546deddc2f78cf74bceff30315 aba9ce0eb6e67945f2209a17676f356fe9748 7e6..928538e3782ccbb22f54f4a3b1152b1faba95471

Diff

1:  1f2d0ffd56 < -:  ---------- move-only: ConstevalFormatString tfm::format to tinyformat.h
2:  c937ebf1ce = 1:  efa6f3f372 tinyformat: remove std::string overload
3:  be7de807c8 = 2:  9329e334eb tinyformat: force compile-time checks for format string literals
4:  4f33e9a85c ! 3:  928538e378 tinyformat: don't throw format string errors
    @@ Commit message
         behaviour, so suppress run-time format errors by returning the error as
         a string instead of throwing a tinyformat::format_error.
     
    +    In DEBUG builds, tinyformat::format_error throwing behaviour is
    +    preserved.
    +
         Note: most format string literals are partially validated at compile-time
         through util::ConstevalFormatString. Notably, bilingual_str format strings
         do not have this compile-time validation.
    @@ src/logging.h: template <typename... Args>
     -        try {
     -            log_msg = tfm::format(fmt, args...);
     -        } catch (tinyformat::format_error& fmterr) {
    --            /* Original format string will have newline so don't add one here */
     -            log_msg = "Error \"" + std::string{fmterr.what()} + "\" while formatting log message: " + fmt.fmt;
     -        }
     +        const auto log_msg{tfm::format(fmt, args...)};
    @@ src/test/util_string_tests.cpp
      #include <util/string.h>
      
      #include <boost/test/unit_test.hpp>
    + #include <test/util/setup_common.h>
      
     +#include <sstream>
     +
    @@ src/test/util_string_tests.cpp: BOOST_AUTO_TEST_CASE(ConstevalFormatString_NumSp
      
     +BOOST_AUTO_TEST_CASE(tinyformat_ConstevalFormatString)
     +{
    -+    // Ensure invalid format strings don't throw at run-time.
    -+    BOOST_CHECK_EQUAL(tfm::format("%*c", "dummy"), R"(Error "tinyformat: Cannot convert from argument type to integer for use as variable width or precision" while formatting log message: %*c)");
    -+    BOOST_CHECK_EQUAL(tfm::format("%2$*3$d", "dummy", "value"), R"(Error "tinyformat: Positional argument out of range" while formatting log message: %2$*3$d)");
    ++    // Ensure invalid format strings don't throw at run-time, when not in DEBUG
    ++#ifndef DEBUG
    ++    BOOST_CHECK_EQUAL(tfm::format("%*c", "dummy"), R"(Error "tinyformat: Cannot convert from argument type to integer for use as variable width or precision" while formatting: %*c)");
    ++    BOOST_CHECK_EQUAL(tfm::format("%2$*3$d", "dummy", "value"), R"(Error "tinyformat: Positional argument out of range" while formatting: %2$*3$d)");
     +    std::ostringstream oss;
     +    tfm::format(oss, "%.*f", 5);
    -+    BOOST_CHECK_EQUAL(oss.str(), R"(Error "tinyformat: Too many conversion specifiers in format string" while formatting log message: %.*f)");
    ++    BOOST_CHECK_EQUAL(oss.str(), R"(Error "tinyformat: Too many conversion specifiers in format string" while formatting: %.*f)");
    ++#endif
     +}
     +
     +
    @@ src/tinyformat.h: TINYFORMAT_FOREACH_ARGNUM(TINYFORMAT_MAKE_MAKEFORMATLIST)
     +    try {
     +        detail::formatImpl(out, fmt, list.m_args, list.m_N);
     +    } catch (tinyformat::format_error& fmterr) {
    -+        out << "Error \"" + std::string{fmterr.what()} + "\" while formatting log message: " + fmt;
    ++        out << "Error \"" + std::string{fmterr.what()} + "\" while formatting: " + fmt;
    ++#ifdef DEBUG
    ++        throw;
    ++#endif
     +    }
      }

Worth bringing back the try/catch in src/test/fuzz/strprintf.cpp for Debug builds?

We should definitely be able to see easily what the invalid values were (and add unit tests for those cases)

[stickies-v]

I've carved out the first 3 commits into https://github.com/bitcoin/bitcoin/pull/31149, since other PRs (e.g. #31061 and #31072) somewhat rely on this, and everyone seems in agreement that adding compile-time checks is good. Existing ACKs should be trivially transferable.

Converting this PR to draft until #31149 is merged to keep the controversial error suppression discussion until later.

[hodlinator]

Worth bringing back the try/catch in src/test/fuzz/strprintf.cpp for Debug builds?

We should definitely be able to see easily what the invalid values were (and add unit tests for those cases)

(Not sure that would add much value, we know the fuzzing will produce mostly garbage format strings that will trigger errors).

[DrahtBot]

🐙 This pull request conflicts with the target branch and needs rebase.

[DrahtBot]

⌛ There hasn't been much activity lately and the patch still needs rebase. What is the status here?

• Is it still relevant? ➡️ Please solve the conflicts to make it ready for review and to ensure the CI passes.
• Is it no longer relevant? ➡️ Please close.
• Did the author lose interest or time to work on this? ➡️ Please close it and mark it 'Up for grabs' with the label, so that it can be picked up in the future.

[stickies-v]

Closing this PR. Thanks to related PRs like #31174, #31072 and #31061, tfm::format error throwing should be significantly reduced, so there's increasingly little point in making this behaviour change.