Repository: bitcoin/bitcoin
File: src/wallet/wallet.h (C/C++)

Code before:

    /** check if a certain wallet flag is set */
    bool IsWalletFlagSet(uint64_t flag) const override;

    /** overwrite all flags by the given uint64_t
       flags must be uninitialised (or 0)
       only known flags may be present */
    void InitWalletFlags(uint64_t flags);
    /** Loads the flags into the wallet. (used by LoadWallet) */
    bool LoadWalletFlags(uint64_t flags);

    /** Determine if we are a legacy wallet */
    bool IsLegacy() const;

    /** Returns a bracketed wallet name for displaying in logs, will return [default wallet] if the wallet has no name */
    std::string GetDisplayName() const override
    {
        std::string wallet_name = GetName().length() == 0 ? "default wallet" : GetName();
        return strprintf("[%s]", wallet_name);
    };

    /** Prepends the wallet name in logging output to ease debugging in multi-wallet use cases */
    template <typename... Params>
    void WalletLogPrintf(const char* fmt, Params... parameters) const
    {
        LogPrintf(("%s " + std::string{fmt}).c_str(), GetDisplayName(), parameters...);
    };

    /** Upgrade the wallet */
    bool UpgradeWallet(int version, bilingual_str& error);

    //! Returns all unique ScriptPubKeyMans in m_internal_spk_managers and m_external_spk_managers
    std::set<ScriptPubKeyMan*> GetActiveScriptPubKeyMans() const;
    bool IsActiveScriptPubKeyMan(const ScriptPubKeyMan& spkm) const;

    //! Returns all unique ScriptPubKeyMans
    std::set<ScriptPubKeyMan*> GetAllScriptPubKeyMans() const;

    //! Get the ScriptPubKeyMan for the given OutputType and internal/external chain.
    ScriptPubKeyMan* GetScriptPubKeyMan(const OutputType& type, bool internal) const;

    //! Get all the ScriptPubKeyMans for a script
    std::set<ScriptPubKeyMan*> GetScriptPubKeyMans(const CScript& script) const;
    //! Get the ScriptPubKeyMan by id
    ScriptPubKeyMan* GetScriptPubKeyMan(const uint256& id) const;

    //! Get the SigningProvider for a script
    std::unique_ptr<SigningProvider> GetSolvingProvider(const CScript& script) const;
    std::unique_ptr<SigningProvider> GetSolvingProvider(const CScript& script, SignatureData& sigdata) const;

    //! Get the wallet descriptors for a script.

Review comment (bug):
By not letting `LogInfo` do the formatting, there's no `tinyformat::format_error` error handling anymore. This makes the code less robust to the malformed format strings we do not catch at compile time. For example, with this diff:

```diff
diff --git a/src/wallet/wallet.cpp b/src/wallet/wallet.cpp
index 83e96adf07..e6a2576f0a 100644
--- a/src/wallet/wallet.cpp
+++ b/src/wallet/wallet.cpp
@@ -3822,7 +3822,7 @@ void CWallet::LoadActiveScriptPubKeyMan(uint256 id, OutputType type, bool intern
     // Legacy wallets have only one ScriptPubKeyManager and it's active for all output and change types.
     Assert(IsWalletFlagSet(WALLET_FLAG_DESCRIPTORS));
 
-    WalletLogPrintf("Setting spkMan to active: id = %s, type = %s, internal = %s\n", id.ToString(), FormatOutputType(type), internal ? "true" : "false");
+    WalletLogPrintf("Setting spkMan to active: id = %s, type = %s, internal = %*s\n", id.ToString(), FormatOutputType(type), internal ? "true" : "false");
     auto& spk_mans = internal ? m_internal_spk_managers : m_external_spk_managers;
     auto& spk_mans_other = internal ? m_external_spk_managers : m_internal_spk_managers;
     auto spk_man = m_spk_managers.at(id).get();

```

on `master`, this would still allow a wallet to be loaded (with just the logging failing):

```
% bitcoin-cli loadwallet test
{
  "name": "test"
}

...

2024-09-19T10:32:57Z init message: Loading wallet…
2024-09-19T10:32:57Z [test] Wallet file version = 10500, last client version = 289900
2024-09-19T10:32:57Z [test] Descriptors: 8, Descriptor Keys: 1 plaintext, 0 encrypted, 1 total.
2024-09-19T10:32:57Z Error "tinyformat: Cannot convert from argument type to integer for use as variable width or precision" while formatting log message: %s Setting spkMan to active: id = %s, type = %s, internal = %*s
2024-09-19T10:32:57Z Error "tinyformat: Cannot convert from argument type to integer for use as variable width or precision" while formatting log message: %s Setting spkMan to active: id = %s, type = %s, internal = %*s
2024-09-19T10:32:57Z Error "tinyformat: Cannot convert from argument type to integer for use as variable width or precision" while formatting log message: %s Setting spkMan to active: id = %s, type = %s, internal = %*s
2024-09-19T10:32:57Z Error "tinyformat: Cannot convert from argument type to integer for use as variable width or precision" while formatting log message: %s Setting spkMan to active: id = %s, type = %s, internal = %*s
2024-09-19T10:32:57Z Error "tinyformat: Cannot convert from argument type to integer for use as variable width or precision" while formatting log message: %s Setting spkMan to active: id = %s, type = %s, internal = %*s
2024-09-19T10:32:57Z Error "tinyformat: Cannot convert from argument type to integer for use as variable width or precision" while formatting log message: %s Setting spkMan to active: id = %s, type = %s, internal = %*s
2024-09-19T10:32:57Z Error "tinyformat: Cannot convert from argument type to integer for use as variable width or precision" while formatting log message: %s Setting spkMan to active: id = %s, type = %s, internal = %*s
2024-09-19T10:32:57Z Error "tinyformat: Cannot convert from argument type to integer for use as variable width or precision" while formatting log message: %s Setting spkMan to active: id = %s, type = %s, internal = %*s
2024-09-19T10:32:57Z [test] Wallet completed loading in             281ms
```
 
whereas on facbcd4cef8890ae18976fb53b67ea56b3c04454, you get:

```
% bitcoin-cli loadwallet test
error code: -4
error message:
Wallet loading failed. Error loading <...>/signet/wallets/test/wallet.dat: Wallet corrupted

...

2024-09-19T11:00:08Z init message: Loading wallet…
2024-09-19T11:00:08Z [test] Wallet file version = 10500, last client version = 289900
2024-09-19T11:00:09Z [test] Descriptors: 8, Descriptor Keys: 1 plaintext, 0 encrypted, 1 total.
2024-09-19T11:00:09Z [test] Releasing wallet test..
```

Similar concern in `scriptpubkeyman.h`.

Code after:

    /** check if a certain wallet flag is set */
    bool IsWalletFlagSet(uint64_t flag) const override;

    /** overwrite all flags by the given uint64_t
       flags must be uninitialised (or 0)
       only known flags may be present */
    void InitWalletFlags(uint64_t flags);
    /** Loads the flags into the wallet. (used by LoadWallet) */
    bool LoadWalletFlags(uint64_t flags);

    /** Determine if we are a legacy wallet */
    bool IsLegacy() const;

    /** Returns a bracketed wallet name for displaying in logs, will return [default wallet] if the wallet has no name */
    std::string GetDisplayName() const override
    {
        std::string wallet_name = GetName().length() == 0 ? "default wallet" : GetName();
        return strprintf("[%s]", wallet_name);
    };

    /** Prepends the wallet name in logging output to ease debugging in multi-wallet use cases */
    template <typename... Params>
    void WalletLogPrintf(util::ConstevalFormatString<sizeof...(Params)> wallet_fmt, const Params&... params) const
    {
        LogInfo("%s %s", GetDisplayName(), tfm::format(wallet_fmt, params...));
    };

    /** Upgrade the wallet */
    bool UpgradeWallet(int version, bilingual_str& error);

    //! Returns all unique ScriptPubKeyMans in m_internal_spk_managers and m_external_spk_managers
    std::set<ScriptPubKeyMan*> GetActiveScriptPubKeyMans() const;
    bool IsActiveScriptPubKeyMan(const ScriptPubKeyMan& spkm) const;

    //! Returns all unique ScriptPubKeyMans
    std::set<ScriptPubKeyMan*> GetAllScriptPubKeyMans() const;

    //! Get the ScriptPubKeyMan for the given OutputType and internal/external chain.
    ScriptPubKeyMan* GetScriptPubKeyMan(const OutputType& type, bool internal) const;

    //! Get all the ScriptPubKeyMans for a script
    std::set<ScriptPubKeyMan*> GetScriptPubKeyMans(const CScript& script) const;
    //! Get the ScriptPubKeyMan by id
    ScriptPubKeyMan* GetScriptPubKeyMan(const uint256& id) const;

    //! Get the SigningProvider for a script
    std::unique_ptr<SigningProvider> GetSolvingProvider(const CScript& script) const;
    std::unique_ptr<SigningProvider> GetSolvingProvider(const CScript& script, SignatureData& sigdata) const;

    //! Get the wallet descriptors for a script.