Merge bitcoin/bitcoin#28237: refactor: Enforce C-str fmt strings in WalletLogPrintf()

fa60fa3 bitcoin-tidy: Apply bitcoin-unterminated-logprintf to spkm as well (MarcoFalke)
faa1143 refactor: Enable all clang-tidy plugin bitcoin tests (MarcoFalke)
fa6dc57 refactor: Enforce C-str fmt strings in WalletLogPrintf() (MarcoFalke)
fa244f3 doc: Fix bitcoin-unterminated-logprintf tidy comments (MarcoFalke)

Pull request description:

  All fmt functions only accept a raw C-string as argument.

  There should never be a need to pass a format string that is not a compile-time string literal, so disallow it in `WalletLogPrintf()` to avoid accidentally introducing it.

  Apart from consistency, this also fixes the clang-tidy plugin bug bitcoin/bitcoin#26296 (comment).

ACKs for top commit:
  theuni:
    ACK fa60fa3

Tree-SHA512: fa6f4984c50f9b34e850bdfee7236706af586e512d866cc869cf0cdfaf9aa707029c210ca72d91f85e75fcbd8efe0d77084701de8c3d2004abfd7e46b6fa9072

Author: fanquake

contrib/devtools/bitcoin-tidy/example_logprintf.cpp

@@ -2,9 +2,10 @@
 // Distributed under the MIT software license, see the accompanying
 // file COPYING or http://www.opensource.org/licenses/mit-license.php.
 
-// Warn about any use of LogPrintf that does not end with a newline.
 #include <string>
 
+// Test for bitcoin-unterminated-logprintf
+
 enum LogFlags {
     NONE
 };
@@ -21,8 +22,6 @@ static inline void LogPrintf_(const std::string& logging_function, const std::st
 #define LogPrintLevel_(category, level, ...) LogPrintf_(__func__, __FILE__, __LINE__, category, level, __VA_ARGS__)
 #define LogPrintf(...) LogPrintLevel_(LogFlags::NONE, Level::None, __VA_ARGS__)
 
-// Use a macro instead of a function for conditional logging to prevent
-// evaluating arguments when logging for the category is not enabled.
 #define LogPrint(category, ...) \
     do {                        \
         LogPrintf(__VA_ARGS__); \
@@ -38,9 +37,23 @@ class CWallet
 
 public:
     template <typename... Params>
-    void WalletLogPrintf(std::string fmt, Params... parameters) const
+    void WalletLogPrintf(const char* fmt, Params... parameters) const
+    {
+        LogPrintf(("%s " + std::string{fmt}).c_str(), GetDisplayName(), parameters...);
+    };
+};
+
+struct ScriptPubKeyMan
+{
+    std::string GetDisplayName() const
+    {
+        return "default wallet";
+    }
+
+    template <typename... Params>
+    void WalletLogPrintf(const char* fmt, Params... parameters) const
     {
-        LogPrintf(("%s " + fmt).c_str(), GetDisplayName(), parameters...);
+        LogPrintf(("%s " + std::string{fmt}).c_str(), GetDisplayName(), parameters...);
     };
 };
 
@@ -52,6 +65,8 @@ void good_func2()
 {
     CWallet wallet;
     wallet.WalletLogPrintf("hi\n");
+    ScriptPubKeyMan spkm;
+    spkm.WalletLogPrintf("hi\n");
 
     const CWallet& walletref = wallet;
     walletref.WalletLogPrintf("hi\n");
@@ -81,6 +96,8 @@ void bad_func5()
 {
     CWallet wallet;
     wallet.WalletLogPrintf("hi");
+    ScriptPubKeyMan spkm;
+    spkm.WalletLogPrintf("hi");
 
     const CWallet& walletref = wallet;
     walletref.WalletLogPrintf("hi");

contrib/devtools/bitcoin-tidy/logprintf.cpp

@@ -36,14 +36,12 @@ void LogPrintfCheck::registerMatchers(clang::ast_matchers::MatchFinder* finder)
         this);
 
     /*
-      CWallet wallet;
       auto walletptr = &wallet;
       wallet.WalletLogPrintf("foo");
       wallet->WalletLogPrintf("foo");
     */
     finder->addMatcher(
         cxxMemberCallExpr(
-            thisPointerType(qualType(hasDeclaration(cxxRecordDecl(hasName("CWallet"))))),
             callee(cxxMethodDecl(hasName("WalletLogPrintf"))),
             hasArgument(0, stringLiteral(unterminated()).bind("logstring"))),
         this);

contrib/devtools/bitcoin-tidy/logprintf.h

@@ -9,6 +9,7 @@
 
 namespace bitcoin {
 
+// Warn about any use of LogPrintf that does not end with a newline.
 class LogPrintfCheck final : public clang::tidy::ClangTidyCheck
 {
 public:

src/.clang-tidy

@@ -1,6 +1,6 @@
 Checks: '
 -*,
-bitcoin-unterminated-logprintf,
+bitcoin-*,
 bugprone-argument-comment,
 bugprone-use-after-move,
 misc-unused-using-decls,

src/wallet/scriptpubkeyman.h

@@ -250,9 +250,10 @@ class ScriptPubKeyMan
     virtual std::unordered_set<CScript, SaltedSipHasher> GetScriptPubKeys() const { return {}; };
 
     /** Prepends the wallet name in logging output to ease debugging in multi-wallet use cases */
-    template<typename... Params>
-    void WalletLogPrintf(std::string fmt, Params... parameters) const {
-        LogPrintf(("%s " + fmt).c_str(), m_storage.GetDisplayName(), parameters...);
+    template <typename... Params>
+    void WalletLogPrintf(const char* fmt, Params... parameters) const
+    {
+        LogPrintf(("%s " + std::string{fmt}).c_str(), m_storage.GetDisplayName(), parameters...);
     };
 
     /** Watch-only address added */

src/wallet/wallet.cpp

@@ -2320,7 +2320,7 @@ OutputType CWallet::TransactionChangeType(const std::optional<OutputType>& chang
 void CWallet::CommitTransaction(CTransactionRef tx, mapValue_t mapValue, std::vector<std::pair<std::string, std::string>> orderForm)
 {
     LOCK(cs_wallet);
-    WalletLogPrintf("CommitTransaction:\n%s", tx->ToString());
+    WalletLogPrintf("CommitTransaction:\n%s", tx->ToString()); // NOLINT(bitcoin-unterminated-logprintf)
 
     // Add tx to wallet, because if it has change it's also ours,
     // otherwise just for transaction history.

src/wallet/wallet.h

@@ -891,9 +891,10 @@ class CWallet final : public WalletStorage, public interfaces::Chain::Notificati
     };
 
     /** Prepends the wallet name in logging output to ease debugging in multi-wallet use cases */
-    template<typename... Params>
-    void WalletLogPrintf(std::string fmt, Params... parameters) const {
-        LogPrintf(("%s " + fmt).c_str(), GetDisplayName(), parameters...);
+    template <typename... Params>
+    void WalletLogPrintf(const char* fmt, Params... parameters) const
+    {
+        LogPrintf(("%s " + std::string{fmt}).c_str(), GetDisplayName(), parameters...);
     };
 
     /** Upgrade the wallet */

test/lint/run-lint-format-strings.py

@@ -20,10 +20,10 @@
     ("src/clientversion.cpp", "strprintf(_(COPYRIGHT_HOLDERS).translated, COPYRIGHT_HOLDERS_SUBSTITUTION)"),
     ("src/test/translation_tests.cpp", "strprintf(format, arg)"),
     ("src/validationinterface.cpp", "LogPrint(BCLog::VALIDATION, fmt \"\\n\", __VA_ARGS__)"),
-    ("src/wallet/wallet.h",  "WalletLogPrintf(std::string fmt, Params... parameters)"),
-    ("src/wallet/wallet.h", "LogPrintf((\"%s \" + fmt).c_str(), GetDisplayName(), parameters...)"),
-    ("src/wallet/scriptpubkeyman.h",  "WalletLogPrintf(std::string fmt, Params... parameters)"),
-    ("src/wallet/scriptpubkeyman.h", "LogPrintf((\"%s \" + fmt).c_str(), m_storage.GetDisplayName(), parameters...)"),
+    ("src/wallet/wallet.h", "WalletLogPrintf(const char* fmt, Params... parameters)"),
+    ("src/wallet/wallet.h", "LogPrintf((\"%s \" + std::string{fmt}).c_str(), GetDisplayName(), parameters...)"),
+    ("src/wallet/scriptpubkeyman.h", "WalletLogPrintf(const char* fmt, Params... parameters)"),
+    ("src/wallet/scriptpubkeyman.h", "LogPrintf((\"%s \" + std::string{fmt}).c_str(), m_storage.GetDisplayName(), parameters...)"),
 ]