log: Enforce trailing newline #30929

maflcko commented at 2:53 PM on September 19, 2024: member

There are many problems around missing a trailing newline while logging:

All log lines are currently terminated by a trailing newline. This means any runtime code trying to handle a "missing" newline is currently dead code.
Leaving a line unterminated is racy and can cause content corruption by mixing log lines from different sources.
It requires extra code like m_started_new_line to keep track of, which is annoying and pointless to maintain, because it is currently dead code, see #30386 (review).
It requires a standalone unterminated-logprintf clang-tidy plugin, which is unmaintained (no one updated it for the new log function names), probably harder to maintain than normal C++ code (because it requires clang AST matcher knowledge), brittle (it can fail to detect issues at any time, if it goes out-of-sync, or be explicitly disabled via NOLINT), and annoying for devs (it is slow and intricate to run locally and thus only effectively run on CI or via the CI scripts).

Fix all issues by enforcing the trailing newline in logs directly in the code. Then remove all the other stuff.

This refactor does not change behavior.

DrahtBot commented at 2:53 PM on September 19, 2024: contributor

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	stickies-v, ryanofsky, achow101
Stale ACK	l0rinc

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:

#30928 (tinyformat: refactor: increase compile-time checks and don't throw for tfm::format_error by stickies-v)
#29641 (scripted-diff: Use LogInfo over LogPrintf [WIP, NOMERGE, DRAFT] by maflcko)

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 renamed this:
~~log: Enforce trailing newline at compile time~~
log: Enforce trailing newline at compile time
on Sep 19, 2024

DrahtBot added the label Utils/log/libs on Sep 19, 2024

maflcko force-pushed on Sep 19, 2024

DrahtBot added the label CI failed on Sep 19, 2024

DrahtBot commented at 3:11 PM on September 19, 2024: contributor

🚧 At least one of the CI tasks failed. <sub>Debug: https://github.com/bitcoin/bitcoin/runs/30383147556</sub>

<details><summary>Hints</summary>

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.

</details>

in contrib/devtools/bitcoin-tidy/bitcoin-tidy.cpp:16 in fa30228869 outdated

  11 | @@ -13,7 +12,6 @@ class BitcoinModule final : public clang::tidy::ClangTidyModule
  12 |  public:
  13 |      void addCheckFactories(clang::tidy::ClangTidyCheckFactories& CheckFactories) override
  14 |      {
  15 | -        CheckFactories.registerCheck<bitcoin::LogPrintfCheck>("bitcoin-unterminated-logprintf");

l0rinc commented at 3:14 PM on September 19, 2024:

nice

in src/index/base.cpp:31 in fa30228869 outdated

  27 | @@ -28,7 +28,7 @@ constexpr auto SYNC_LOG_INTERVAL{30s};
  28 |  constexpr auto SYNC_LOCATOR_WRITE_INTERVAL{30s};
  29 |  
  30 |  template <typename... Args>
  31 | -void BaseIndex::FatalErrorf(util::ConstevalFormatString<sizeof...(Args)> fmt, const Args&... args)
  32 | +void BaseIndex::FatalErrorf(util::ConstevalFormatString<false, sizeof...(Args)> fmt, const Args&... args)

l0rinc commented at 3:15 PM on September 19, 2024:

nit: would it make sense to add a bool comment for these params?

void BaseIndex::FatalErrorf(util::ConstevalFormatString</*force_trailing_newline=*/false, sizeof...(Args)> fmt, const Args&... args)

l0rinc commented at 6:54 PM on September 19, 2024:

Even better, resolve it please

fanquake requested review from stickies-v on Sep 19, 2024

DrahtBot removed the label CI failed on Sep 19, 2024

maflcko added the label Refactoring on Sep 19, 2024

l0rinc commented at 6:01 PM on September 19, 2024: contributor

concept ACK

maflcko force-pushed on Sep 19, 2024

in src/logging.h:242 in fa7190db83 outdated

 238 | @@ -246,7 +239,7 @@ static inline bool LogAcceptCategory(BCLog::LogFlags category, BCLog::Level leve
 239 |  bool GetLogCategory(BCLog::LogFlags& flag, std::string_view str);
 240 |  
 241 |  template <typename... Args>
 242 | -inline void LogPrintFormatInternal(std::string_view logging_function, std::string_view source_file, const int source_line, const BCLog::LogFlags flag, const BCLog::Level level, util::ConstevalFormatString<sizeof...(Args)> fmt, const Args&... args)
 243 | +inline void LogPrintFormatInternal(std::string_view logging_function, std::string_view source_file, const int source_line, const BCLog::LogFlags flag, const BCLog::Level level, util::ConstevalFormatString<util::TrailingNewlineCheck, sizeof...(Args)> fmt, const Args&... args)

stickies-v commented at 10:49 AM on September 20, 2024:

nit: might be nice introducing a LogFmt alias here?

diff --git a/src/logging.h b/src/logging.h
index e0bd79f793..ed224565e6 100644
--- a/src/logging.h
+++ b/src/logging.h
@@ -239,7 +239,10 @@ static inline bool LogAcceptCategory(BCLog::LogFlags category, BCLog::Level leve
 bool GetLogCategory(BCLog::LogFlags& flag, std::string_view str);
 
 template <typename... Args>
-inline void LogPrintFormatInternal(std::string_view logging_function, std::string_view source_file, const int source_line, const BCLog::LogFlags flag, const BCLog::Level level, util::ConstevalFormatString<util::TrailingNewlineCheck, sizeof...(Args)> fmt, const Args&... args)
+using LogFmt = util::ConstevalFormatString<util::TrailingNewlineCheck, sizeof...(Args)>;
+
+template <typename... Args>
+inline void LogPrintFormatInternal(std::string_view logging_function, std::string_view source_file, const int source_line, const BCLog::LogFlags flag, const BCLog::Level level, LogFmt<Args...> fmt, const Args&... args)
 {
     if (LogInstance().Enabled()) {
         std::string log_msg;
diff --git a/src/wallet/scriptpubkeyman.h b/src/wallet/scriptpubkeyman.h
index e038c4c77a..a16f5c99d6 100644
--- a/src/wallet/scriptpubkeyman.h
+++ b/src/wallet/scriptpubkeyman.h
@@ -254,7 +254,7 @@ public:
 
     /** Prepends the wallet name in logging output to ease debugging in multi-wallet use cases */
     template <typename... Params>
-    void WalletLogPrintf(util::ConstevalFormatString<util::TrailingNewlineCheck, sizeof...(Params)> wallet_fmt, const Params&... params) const
+    void WalletLogPrintf(LogFmt<Params...> wallet_fmt, const Params&... params) const
     {
         LogInfo("%s %s\n", m_storage.GetDisplayName(), util::RemoveSuffixView(tfm::format(wallet_fmt, params...), "\n"));
     };
diff --git a/src/wallet/wallet.h b/src/wallet/wallet.h
index f85bebe179..807c95ae1c 100644
--- a/src/wallet/wallet.h
+++ b/src/wallet/wallet.h
@@ -927,7 +927,7 @@ public:
 
     /** Prepends the wallet name in logging output to ease debugging in multi-wallet use cases */
     template <typename... Params>
-    void WalletLogPrintf(util::ConstevalFormatString<util::TrailingNewlineCheck, sizeof...(Params)> wallet_fmt, const Params&... params) const
+    void WalletLogPrintf(LogFmt<Params...> wallet_fmt, const Params&... params) const
     {
         LogInfo("%s %s\n", GetDisplayName(), util::RemoveSuffixView(tfm::format(wallet_fmt, params...), "\n"));
     };

</details>

maflcko commented at 11:54 AM on September 20, 2024:

Sure, done!

stickies-v commented at 10:54 AM on September 20, 2024: contributor

Concept ACK on these checks being done at compile-time instead of through clang-tidy/linters/..., and the significant resulting code clean-up. Added verbosity can be hidden, e.g. by the alias suggested as a nit.

maflcko force-pushed on Sep 20, 2024

DrahtBot added the label Needs rebase on Sep 27, 2024

maflcko force-pushed on Sep 27, 2024

DrahtBot removed the label Needs rebase on Sep 27, 2024

maflcko commented at 1:45 PM on September 27, 2024: member

(rebased, trival)

<!-- ```diff diff --git a/src/logging.h b/src/logging.h index f465622d0b..9998f85c35 100644 --- a/src/logging.h +++ b/src/logging.h @@ -256,7 +256,7 @@ inline void LogPrintFormatInternal(std::string_view logging_function, std::strin /* 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; } + if (!log_msg.ends_with('\n')) log_msg.push_back('\n'); LogInstance().LogPrintStr(log_msg, logging_function, source_file, source_line, flag, level); } }

in src/test/util_string_tests.cpp:32 in fac4a34d0a outdated

  30 | +template <unsigned WrongNumArgs, util::EndCheck end_check = util::NoEndCheck>
  31 |  inline void FailFmtWithError(std::string_view wrong_fmt, std::string_view error)
  32 |  {
  33 | -    BOOST_CHECK_EXCEPTION(util::ConstevalFormatString<WrongNumArgs>::Detail_CheckNumFormatSpecifiers(wrong_fmt), const char*, HasReason(error));
  34 | +    using WrongFmt = util::ConstevalFormatString<end_check, WrongNumArgs>;
  35 | +    BOOST_CHECK_EXCEPTION(WrongFmt::Detail_CheckNumFormatSpecifiers(wrong_fmt), const char*, HasReason(error));

l0rinc commented at 1:59 PM on September 27, 2024:

nit: if you edit again (the spirit of @hodlinator is haunting me here):

    BOOST_CHECK_EXCEPTION(WrongFmt::Detail_CheckNumFormatSpecifiers(wrong_fmt), const char*, HasReason{error});

maflcko commented at 8:22 AM on September 30, 2024:

Ok, done

in src/wallet/scriptpubkeyman.h:257 in fac4a34d0a outdated

 253 | @@ -254,9 +254,9 @@ class ScriptPubKeyMan
 254 |  
 255 |      /** Prepends the wallet name in logging output to ease debugging in multi-wallet use cases */
 256 |      template <typename... Params>
 257 | -    void WalletLogPrintf(util::ConstevalFormatString<sizeof...(Params)> wallet_fmt, const Params&... params) const
 258 | +    void WalletLogPrintf(LogFmt<Params...> wallet_fmt, const Params&... params) const

l0rinc commented at 2:07 PM on September 27, 2024:

nit: in tinyformat the print variant that appends a \n is called printfln -> please consider a similar naming (I'm not necessarily recommending it, just bringing it to your attention):

    void WalletLogPrintfln(LogFmt<Params...> wallet_fmt, const Params&... params) const

If you disagree, just resolve it, no explanation needed.

maflcko commented at 3:00 PM on September 27, 2024:

It does not append a \n. It removes one and adds one, the effect of which is a nullopt.

I understand that this is a bit confusing and it would be better if there were no \n anywhere in log strings, but this can be done in the future. For example when doing other breaking changes, like switching to std::format in C++23 in a few years down the line. Closing for now.

in src/test/util_string_tests.cpp:37 in fac4a34d0a outdated

  35 | +    BOOST_CHECK_EXCEPTION(WrongFmt::Detail_CheckNumFormatSpecifiers(wrong_fmt), const char*, HasReason(error));
  36 | +}
  37 | +
  38 | +BOOST_AUTO_TEST_CASE(ConstevalFormatString_Newline)
  39 | +{
  40 | +    PassFmtNewline<0>("\n");

l0rinc commented at 2:11 PM on September 27, 2024:

nit: given that we're removing \n specifically in https://github.com/bitcoin/bitcoin/blob/fabf6dcdd1b1e1250936444a80ef25fdf737e2f2/src/wallet/scriptpubkeyman.h#L259 - should we document what we expect to happen with Windows newlines (which also end with \n, but we're not removing \r and adding back the \n later):

    PassFmtNewline<0>("\r\n");

maflcko commented at 3:06 PM on September 27, 2024:

There aren't any Windows newlines anywhere in log lines, and this pull requests isn't changing anything about that. So I'll leave this as-is for now.

in src/test/util_string_tests.cpp:41 in fac4a34d0a outdated

  38 | +BOOST_AUTO_TEST_CASE(ConstevalFormatString_Newline)
  39 | +{
  40 | +    PassFmtNewline<0>("\n");
  41 | +    PassFmtNewline<1>("%s\n");
  42 | +    PassFmtNewline<0>("%%\n");
  43 | +    PassFmtNewline<1>("%\n"); // tinyformat will use the newline as format specifier

l0rinc commented at 2:22 PM on September 27, 2024:

This was already tested in ConstevalFormatString_NumSpec - maybe we could move the comment or remove that example

maflcko commented at 3:07 PM on September 27, 2024:

Seems fine to document twice. I don't really see the risk or downside. Leaving as-is for now.

in src/test/util_string_tests.cpp:42 in fac4a34d0a outdated

  39 | +{
  40 | +    PassFmtNewline<0>("\n");
  41 | +    PassFmtNewline<1>("%s\n");
  42 | +    PassFmtNewline<0>("%%\n");
  43 | +    PassFmtNewline<1>("%\n"); // tinyformat will use the newline as format specifier
  44 | +

l0rinc commented at 2:22 PM on September 27, 2024:

we could add a few more corner cases, e.g. \n\n and \n -> otherwise the tests would pass for a contains("\n") or count("\n") == 1 as well

maflcko commented at 8:23 AM on September 30, 2024:

Sure, done

in src/logging.cpp:393 in fac4a34d0a outdated

 389 | @@ -389,23 +390,10 @@ void BCLog::Logger::LogPrintStr(std::string_view str, std::string_view logging_f
 390 |  
 391 |  void BCLog::Logger::LogPrintStr_(std::string_view str, std::string_view logging_function, std::string_view source_file, int source_line, BCLog::LogFlags category, BCLog::Level level)
 392 |  {
 393 | +    Assume(str.ends_with("\n")); // Checked at compile-time, so this can never fail

l0rinc commented at 2:30 PM on September 27, 2024:

Assume should be used to document assumptions when program execution can safely continue even if the assumption is violated.

Shouldn't this be an assert instead?

maflcko commented at 3:02 PM on September 27, 2024:

A missing \n is a harmless, albeit confusing, stylistic issue in the debug logs. I'd very much say that the program can safely continue and that this is not a fatal error.

Edit: It should be trivial to add the missing newline in this case at runtime, but I think playing code-golf on effectively dead code isn't too useful.

maflcko commented at 11:46 AM on September 30, 2024:

To clarify, with "code-golf on effectively dead code" I meant adding the newline while keeping the compile-time check. Either the newline is appended going forward, dropping the compile-time checks, or the compile-time checks are kept and the newline is appended at runtime. But doing both doesn't make sense.

ryanofsky commented at 12:16 PM on September 30, 2024:

It seems a little crazy for logging code to use \n in format strings when logger doesn't support ~multiline~ multipart log strings, and more crazy to add a compile time check enforcing presence of \n characters which serve no purpose, add noise, and give misleading impression that the logger supports ~multiline~ multipart strings when it doesn't. I think the best thing would be go to ignore trailing \n characters, stop using them going forward, and not require them.

(EDIT: s/multiline/multipart/ above to be more precise. I'm referring to logging feature which used to exist which allowed subsequent log messages to be joined together if they didn't end in \n)

maflcko commented at 1:44 PM on September 30, 2024:

I think the best thing would be go to ignore trailing \n characters, stop using them going forward, and not require them.

Thanks, pushed the diff from #30929 (comment)

in src/test/util_string_tests.cpp:43 in fac4a34d0a outdated

  41 | +    PassFmtNewline<1>("%s\n");
  42 | +    PassFmtNewline<0>("%%\n");
  43 | +    PassFmtNewline<1>("%\n"); // tinyformat will use the newline as format specifier
  44 | +
  45 | +    auto err_newline{"Format string must end with a newline"};
  46 | +    FailFmtWithError<1, util::TrailingNewlineCheck>("", err_newline);

l0rinc commented at 2:36 PM on September 27, 2024:

given that PassFmtNewline<1>("\n") would already fail - and this is an test for a missing newline, not invalid parameter count -, consider the following:

    FailFmtWithError<0, util::TrailingNewlineCheck>("", err_newline);

maflcko commented at 8:23 AM on September 30, 2024:

Sure, done

l0rinc commented at 2:41 PM on September 27, 2024: contributor

Thanks for automating these! I left a few naming notes, a few additional test recommendations and an Assume -> assert suggestion based on its description.

maflcko force-pushed on Sep 30, 2024

maflcko commented at 9:48 AM on September 30, 2024: member

Force pushed some trivial test-only fixups.

l0rinc approved

l0rinc commented at 10:38 AM on September 30, 2024: contributor

ACK facfdcef4a512d4249c63134e632f6154652f022

DrahtBot requested review from stickies-v on Sep 30, 2024

maflcko commented at 10:53 AM on September 30, 2024: member

It would be possible to drop the first commit completely and just replace it with a patch that appends a newline if it is missing. This means that no behavior is enforced at compile-time and the newline placement in code could be inconsistent, but the thing that matters is fixed: A trailing newline is ensured to be always present.

diff --git a/src/logging.h b/src/logging.h
index f465622d0b..9998f85c35 100644
--- a/src/logging.h
+++ b/src/logging.h
@@ -256,7 +256,7 @@ inline void LogPrintFormatInternal(std::string_view logging_function, std::strin
             /* 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;
         }
+        if (!log_msg.ends_with('\n')) log_msg.push_back('\n');
         LogInstance().LogPrintStr(log_msg, logging_function, source_file, source_line, flag, level);
     }
 }

l0rinc commented at 11:02 AM on September 30, 2024: contributor

replace it with a patch that appends a newline if it is missing

Didn't you call this code-golf in #30929 (review)? I'd prefer keeping this explicit (as it i now) - or renaming the method to end with ln if we're appending.

maflcko renamed this:
~~log: Enforce trailing newline at compile time~~
log: Enforce trailing newline
on Sep 30, 2024

maflcko force-pushed on Sep 30, 2024

ryanofsky commented at 2:49 PM on September 30, 2024: contributor

Approach ACK fadce8893d766a9ead7493c1a00a885066156b00. Two things:

Would be good to clarify in the description that the reason why "This refactor does not change behavior" is that the only unterminated log prints were removed in #30485, so there is no code relying on the old behavior. (Assuming this is the case)
I wonder if it would be possible to simplify the implementation by just making the newline part of log formatting:

--- a/src/logging.cpp
+++ b/src/logging.cpp
@@ -369,6 +369,8 @@ static size_t MemUsage(const BCLog::Logger::BufferedLog& buflog)
 
 void BCLog::Logger::FormatLogStrInPlace(std::string& str, BCLog::LogFlags category, BCLog::Level level, std::string_view source_file, int source_line, std::string_view logging_function, std::string_view threadname, SystemClock::time_point now, std::chrono::seconds mocktime) const
 {
+    if (!str.ends_with('\n')) str.push_back('\n');
+
     str.insert(0, GetLogPrefix(category, level));
 
     if (m_log_sourcelocations) {
@@ -390,7 +392,6 @@ void BCLog::Logger::LogPrintStr(std::string_view str, std::string_view logging_f
 
 void BCLog::Logger::LogPrintStr_(std::string_view str, std::string_view logging_function, std::string_view source_file, int source_line, BCLog::LogFlags category, BCLog::Level level)
 {
-    Assume(str.ends_with("\n")); // Enforced at the call-site, so this can never fail
     std::string str_prefixed = LogEscapeMessage(str);
 
     if (m_buffering) {
--- a/src/logging.h
+++ b/src/logging.h
@@ -249,10 +249,6 @@ inline void LogPrintFormatInternal(std::string_view logging_function, std::strin
             /* 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;
         }
-        // Some log messages have a trailing newline, but this is not required
-        // going forward. However, LogPrintStr requires it, so add one if it
-        // was missing.
-        if (!log_msg.ends_with('\n')) log_msg.push_back('\n');
         LogInstance().LogPrintStr(log_msg, logging_function, source_file, source_line, flag, level);
     }
 }

maflcko force-pushed on Sep 30, 2024

ryanofsky approved

ryanofsky commented at 3:26 PM on September 30, 2024: contributor

Code review ACK fa6444fe628fc33b77d45773c13b27a5a557cd2f. Nice simplification removing dead code after #30485.

DrahtBot requested review from l0rinc on Sep 30, 2024

in src/logging.cpp:372 in fa6444fe62 outdated

 368 | @@ -368,6 +369,8 @@ static size_t MemUsage(const BCLog::Logger::BufferedLog& buflog)
 369 |  
 370 |  void BCLog::Logger::FormatLogStrInPlace(std::string& str, BCLog::LogFlags category, BCLog::Level level, std::string_view source_file, int source_line, std::string_view logging_function, std::string_view threadname, SystemClock::time_point now, std::chrono::seconds mocktime) const
 371 |  {
 372 | +    if (!str.ends_with('\n')) str.push_back('\n');

stickies-v commented at 4:33 PM on September 30, 2024:

Would be nice to add test coverage to prevent regressions? Imo can be as simple as slightly modifying the existing logging_LogPrintMacros test. E.g. this would fail on fae943042f38012ec3410cc8988928896e924352:

diff --git a/src/test/logging_tests.cpp b/src/test/logging_tests.cpp
index 8217a2385c..9cfdf8293a 100644
--- a/src/test/logging_tests.cpp
+++ b/src/test/logging_tests.cpp
@@ -135,8 +135,8 @@ BOOST_FIXTURE_TEST_CASE(logging_LogPrintMacros, LogSetup)
 {
     LogTrace(BCLog::NET, "foo6: %s\n", "bar6"); // not logged
     LogDebug(BCLog::NET, "foo7: %s\n", "bar7");
-    LogInfo("foo8: %s\n", "bar8");
-    LogWarning("foo9: %s\n", "bar9");
+    LogInfo("foo8: %s", "bar8");
+    LogWarning("foo9: %s", "bar9");
     LogError("foo10: %s\n", "bar10");
     std::ifstream file{tmp_log_path};
     std::vector<std::string> log_lines;

</details>

stickies-v commented at 5:01 PM on September 30, 2024:

nit: I think this comment should now also be removed: https://github.com/bitcoin/bitcoin/blob/f3c74c4a7e120ea363abe4d4aa034b85c1d71919/src/logging.h#L256

maflcko commented at 9:35 AM on October 1, 2024:

Sure, done both

laanwj commented at 9:36 AM on October 3, 2024:

thanks for doing it in this way instead of making it an error/warning, i like that we can now just do LogPrintfs without typing \n every time

in contrib/devtools/bitcoin-tidy/example_logprintf.cpp:1 in fa6444fe62 outdated

   0 | @@ -1,108 +0,0 @@
   1 | -// Copyright (c) 2023 Bitcoin Developers

stickies-v commented at 4:37 PM on September 30, 2024:

Suppression for this file can be cleaned up now:

diff --git a/test/lint/lint-format-strings.py b/test/lint/lint-format-strings.py
index a809851ec6..86a17fb0f8 100755
--- a/test/lint/lint-format-strings.py
+++ b/test/lint/lint-format-strings.py
@@ -62,7 +62,7 @@ def main():
 
         matching_files_filtered = []
         for matching_file in matching_files:
-            if not re.search('^src/(leveldb|secp256k1|minisketch|tinyformat|test/fuzz/strprintf.cpp)|contrib/devtools/bitcoin-tidy/example_logprintf.cpp', matching_file):
+            if not re.search('^src/(leveldb|secp256k1|minisketch|tinyformat|test/fuzz/strprintf.cpp)', matching_file):
                 matching_files_filtered.append(matching_file)
         matching_files_filtered.sort()

</details>

maflcko commented at 9:35 AM on October 1, 2024:

Sure, done

stickies-v approved

stickies-v commented at 5:02 PM on September 30, 2024: contributor

ACK fa6444fe628fc33b77d45773c13b27a5a557cd2f , I much prefer this new approach with less code complexity and a more user-friendly logging interface, that allows for a gradual transition to not having \n-terminated log statements.

I wonder if it would make more sense to switch the commit order so the bitcoin-tidy check is removed after auto-appending the newline, instead of before?

As a quick belt-and-suspenders way to check that there's no behaviour change, I ran a -debug=all node for a while (as well as unit and functional test suite) with no issues with this diff applied:

diff --git a/src/logging.cpp b/src/logging.cpp
index 5f055566ef..917d3e9932 100644
--- a/src/logging.cpp
+++ b/src/logging.cpp
@@ -369,7 +369,7 @@ static size_t MemUsage(const BCLog::Logger::BufferedLog& buflog)
 
 void BCLog::Logger::FormatLogStrInPlace(std::string& str, BCLog::LogFlags category, BCLog::Level level, std::string_view source_file, int source_line, std::string_view logging_function, std::string_view threadname, SystemClock::time_point now, std::chrono::seconds mocktime) const
 {
-    if (!str.ends_with('\n')) str.push_back('\n');
+    if (!str.ends_with('\n')) assert(false);
 
     str.insert(0, GetLogPrefix(category, level));

</details>

log: Enforce trailing newline, Remove redundant m_started_new_line

All log lines already have a trailing newline, but enforcing it allows
to delete unused code.

bbbb2e43ee

Remove redundant unterminated-logprintf tidy check fa2b7d8d6b

maflcko force-pushed on Oct 1, 2024

stickies-v commented at 10:32 AM on October 1, 2024: contributor

re-ACK fa2b7d8d6b3f8d53199921e1e542072441b26fab

No changes except for rebase and addressing review comments (test/lint/doc updates and commit re-ordering).

DrahtBot requested review from ryanofsky on Oct 1, 2024

ryanofsky approved

ryanofsky commented at 12:51 PM on October 1, 2024: contributor

Code review ACK fa2b7d8d6b3f8d53199921e1e542072441b26fab. Just comment and test cleanup since last review

achow101 commented at 10:51 PM on October 2, 2024: member

ACK fa2b7d8d6b3f8d53199921e1e542072441b26fab

achow101 merged this on Oct 2, 2024

achow101 closed this on Oct 2, 2024

maflcko deleted the branch on Oct 7, 2024

TheCharlatan commented at 1:37 PM on October 10, 2024: contributor

Post-merge ACK

I think this really showed the limits of having our own clang-tidy checks. They don't lend themselves well to code that is evolving, may receive renames, or new methods.

TheCharlatan referenced this in commit 8bb47d4c2c on Nov 2, 2024

luke-jr referenced this in commit d8cb709033 on Feb 26, 2025

bug-castercv502 referenced this in commit 403bebd591 on Sep 28, 2025

bitcoin locked this on Oct 10, 2025