Merge bitcoin/bitcoin#24026: Block unsafe std::string fs::path conversion copy_file calls

3a45dc3 Change type of `backup_file` parameter in RestoreWallet/restoreWallet (Hennadii Stepanov)
213172c refactor: Block unsafe std::string fs::path conversion copy_file calls (Hennadii Stepanov)

Pull request description:

  This PR is an optional prerequisite for bitcoin/bitcoin#20744 "Use std::filesystem. Remove Boost Filesystem & System" which:
  - makes further code changes safer
  - prevents [some](https://cirrus-ci.com/task/6525835388649472) test failures on native Windows

ACKs for top commit:
  ryanofsky:
    Code review ACK 3a45dc3. Looks great! Thanks for debugging and fixing this and making #20744 smaller!

Tree-SHA512: c6dfaef6b45b9c268bc9ee9b943b9d679152c9d565ca4f86da8d33f8eb9b3cdbe9ba6df7b7578eacc0d00db6551048beff97419f86eb4b1d3182c43e2b4eb9a5

Author: fanquake

src/fs.h

@@ -92,6 +92,13 @@ static inline path operator+(path p1, path p2)
     return p1;
 }
 
+// Disallow implicit std::string conversion for copy_file
+// to avoid locale-dependent encoding on Windows.
+static inline void copy_file(const path& from, const path& to, copy_option options)
+{
+    boost::filesystem::copy_file(from, to, options);
+}
+
 /**
  * Convert path object to byte string. On POSIX, paths natively are byte
  * strings, so this is trivial. On Windows, paths natively are Unicode, so an

src/interfaces/wallet.h

@@ -6,6 +6,7 @@
 #define BITCOIN_INTERFACES_WALLET_H
 
 #include <consensus/amount.h>
+#include <fs.h>
 #include <interfaces/chain.h>          // For ChainClient
 #include <pubkey.h>                    // For CKeyID and CScriptID (definitions needed in CTxDestination instantiation)
 #include <script/standard.h>           // For CTxDestination
@@ -326,7 +327,7 @@ class WalletLoader : public ChainClient
    virtual std::string getWalletDir() = 0;
 
    //! Restore backup wallet
-   virtual std::unique_ptr<Wallet> restoreWallet(const std::string& backup_file, const std::string& wallet_name, bilingual_str& error, std::vector<bilingual_str>& warnings) = 0;
+   virtual std::unique_ptr<Wallet> restoreWallet(const fs::path& backup_file, const std::string& wallet_name, bilingual_str& error, std::vector<bilingual_str>& warnings) = 0;
 
    //! Return available wallets in wallet directory.
    virtual std::vector<std::string> listWalletDir() = 0;

src/wallet/interfaces.cpp

@@ -557,7 +557,7 @@ class WalletLoaderImpl : public WalletLoader
         options.require_existing = true;
         return MakeWallet(m_context, LoadWallet(m_context, name, true /* load_on_start */, options, status, error, warnings));
     }
-    std::unique_ptr<Wallet> restoreWallet(const std::string& backup_file, const std::string& wallet_name, bilingual_str& error, std::vector<bilingual_str>& warnings) override
+    std::unique_ptr<Wallet> restoreWallet(const fs::path& backup_file, const std::string& wallet_name, bilingual_str& error, std::vector<bilingual_str>& warnings) override
     {
         DatabaseStatus status;
 

src/wallet/rpc/backup.cpp

@@ -1887,7 +1887,7 @@ RPCHelpMan restorewallet()
     bilingual_str error;
     std::vector<bilingual_str> warnings;
 
-    const std::shared_ptr<CWallet> wallet = RestoreWallet(context, fs::PathToString(backup_file), wallet_name, load_on_start, status, error, warnings);
+    const std::shared_ptr<CWallet> wallet = RestoreWallet(context, backup_file, wallet_name, load_on_start, status, error, warnings);
 
     HandleWalletError(wallet, status, error);
 

src/wallet/wallet.cpp

@@ -357,12 +357,12 @@ std::shared_ptr<CWallet> CreateWallet(WalletContext& context, const std::string&
     return wallet;
 }
 
-std::shared_ptr<CWallet> RestoreWallet(WalletContext& context, const std::string& backup_file, const std::string& wallet_name, std::optional<bool> load_on_start, DatabaseStatus& status, bilingual_str& error, std::vector<bilingual_str>& warnings)
+std::shared_ptr<CWallet> RestoreWallet(WalletContext& context, const fs::path& backup_file, const std::string& wallet_name, std::optional<bool> load_on_start, DatabaseStatus& status, bilingual_str& error, std::vector<bilingual_str>& warnings)
 {
     DatabaseOptions options;
     options.require_existing = true;
 
-    if (!fs::exists(fs::u8path(backup_file))) {
+    if (!fs::exists(backup_file)) {
         error = Untranslated("Backup file does not exist");
         status = DatabaseStatus::FAILED_INVALID_BACKUP_FILE;
         return nullptr;

src/wallet/wallet.h

@@ -7,6 +7,7 @@
 #define BITCOIN_WALLET_WALLET_H
 
 #include <consensus/amount.h>
+#include <fs.h>
 #include <interfaces/chain.h>
 #include <interfaces/handler.h>
 #include <outputtype.h>
@@ -60,7 +61,7 @@ std::vector<std::shared_ptr<CWallet>> GetWallets(WalletContext& context);
 std::shared_ptr<CWallet> GetWallet(WalletContext& context, const std::string& name);
 std::shared_ptr<CWallet> LoadWallet(WalletContext& context, const std::string& name, std::optional<bool> load_on_start, const DatabaseOptions& options, DatabaseStatus& status, bilingual_str& error, std::vector<bilingual_str>& warnings);
 std::shared_ptr<CWallet> CreateWallet(WalletContext& context, const std::string& name, std::optional<bool> load_on_start, DatabaseOptions& options, DatabaseStatus& status, bilingual_str& error, std::vector<bilingual_str>& warnings);
-std::shared_ptr<CWallet> RestoreWallet(WalletContext& context, const std::string& backup_file, const std::string& wallet_name, std::optional<bool> load_on_start, DatabaseStatus& status, bilingual_str& error, std::vector<bilingual_str>& warnings);
+std::shared_ptr<CWallet> RestoreWallet(WalletContext& context, const fs::path& backup_file, const std::string& wallet_name, std::optional<bool> load_on_start, DatabaseStatus& status, bilingual_str& error, std::vector<bilingual_str>& warnings);
 std::unique_ptr<interfaces::Handler> HandleLoadWallet(WalletContext& context, LoadWalletFn load_wallet);
 std::unique_ptr<WalletDatabase> MakeWalletDatabase(const std::string& name, const DatabaseOptions& options, DatabaseStatus& status, bilingual_str& error);