From faea30227ba633da5ab257d0247853e0927244bb Mon Sep 17 00:00:00 2001 From: MarcoFalke <*~=`'#}+{/-|&$^_@721217.xyz> Date: Fri, 8 Dec 2023 17:15:16 +0100 Subject: [PATCH 1/6] refactor: Use C++20 std::chrono::days --- src/qt/guiutil.cpp | 3 +-- 1 file changed, 1 insertion(+), 2 deletions(-) diff --git a/src/qt/guiutil.cpp b/src/qt/guiutil.cpp index eb9c65caf28..b8ac187ff98 100644 --- a/src/qt/guiutil.cpp +++ b/src/qt/guiutil.cpp @@ -723,8 +723,7 @@ QString ConnectionTypeToQString(ConnectionType conn_type, bool prepend_direction QString formatDurationStr(std::chrono::seconds dur) { - using days = std::chrono::duration>; // can remove this line after C++20 - const auto d{std::chrono::duration_cast(dur)}; + const auto d{std::chrono::duration_cast(dur)}; const auto h{std::chrono::duration_cast(dur - d)}; const auto m{std::chrono::duration_cast(dur - d - h)}; const auto s{std::chrono::duration_cast(dur - d - h - m)}; From fa2bac08c22182e738a8cabf1b24a9dbf3b092d2 Mon Sep 17 00:00:00 2001 From: MarcoFalke <*~=`'#}+{/-|&$^_@721217.xyz> Date: Thu, 26 Oct 2023 13:24:47 +0200 Subject: [PATCH 2/6] refactor: Avoid copy/move in fs.h The operator accepts a const& reference, so no copy or move is needed. See https://en.cppreference.com/w/cpp/filesystem/path/append --- src/util/fs.h | 6 +++--- 1 file changed, 3 insertions(+), 3 deletions(-) diff --git a/src/util/fs.h b/src/util/fs.h index 7e2803b6aa1..f71869f3498 100644 --- a/src/util/fs.h +++ b/src/util/fs.h @@ -35,7 +35,7 @@ public: // Allow path objects arguments for compatibility. path(std::filesystem::path path) : std::filesystem::path::path(std::move(path)) {} path& operator=(std::filesystem::path path) { std::filesystem::path::operator=(std::move(path)); return *this; } - path& operator/=(std::filesystem::path path) { std::filesystem::path::operator/=(path); return *this; } + path& operator/=(const std::filesystem::path& path) { std::filesystem::path::operator/=(path); return *this; } // Allow literal string arguments, which are safe as long as the literals are ASCII. path(const char* c) : std::filesystem::path(c) {} @@ -97,9 +97,9 @@ static inline auto quoted(const std::string& s) } // Allow safe path append operations. -static inline path operator/(path p1, path p2) +static inline path operator/(path p1, const path& p2) { - p1 /= std::move(p2); + p1 /= p2; return p1; } static inline path operator/(path p1, const char* p2) From fa00098e1a493aa3cce20335d18e7f5f2fb7a4a8 Mon Sep 17 00:00:00 2001 From: MarcoFalke <*~=`'#}+{/-|&$^_@721217.xyz> Date: Sat, 9 Dec 2023 13:22:59 +0100 Subject: [PATCH 3/6] Add tests for C++20 std::u8string Also, add missing includes: #include // for error_code #include // for is_same #include // for errno --- src/test/fs_tests.cpp | 4 +++- src/util/fs.cpp | 5 +++-- src/util/fs.h | 4 +++- 3 files changed, 9 insertions(+), 4 deletions(-) diff --git a/src/test/fs_tests.cpp b/src/test/fs_tests.cpp index 7cfecb2b22f..f6fab7f733b 100644 --- a/src/test/fs_tests.cpp +++ b/src/test/fs_tests.cpp @@ -18,8 +18,10 @@ BOOST_FIXTURE_TEST_SUITE(fs_tests, BasicTestingSetup) BOOST_AUTO_TEST_CASE(fsbridge_pathtostring) { std::string u8_str = "fs_tests_₿_🏃"; + std::u8string str8{u8"fs_tests_₿_🏃"}; BOOST_CHECK_EQUAL(fs::PathToString(fs::PathFromString(u8_str)), u8_str); BOOST_CHECK_EQUAL(fs::u8path(u8_str).u8string(), u8_str); + BOOST_CHECK_EQUAL(fs::path(str8).u8string(), u8_str); BOOST_CHECK_EQUAL(fs::PathFromString(u8_str).u8string(), u8_str); BOOST_CHECK_EQUAL(fs::PathToString(fs::u8path(u8_str)), u8_str); #ifndef WIN32 @@ -47,7 +49,7 @@ BOOST_AUTO_TEST_CASE(fsbridge_fstream) fs::path tmpfolder = m_args.GetDataDirBase(); // tmpfile1 should be the same as tmpfile2 fs::path tmpfile1 = tmpfolder / fs::u8path("fs_tests_₿_🏃"); - fs::path tmpfile2 = tmpfolder / fs::u8path("fs_tests_₿_🏃"); + fs::path tmpfile2 = tmpfolder / fs::path(u8"fs_tests_₿_🏃"); { std::ofstream file{tmpfile1}; file << "bitcoin"; diff --git a/src/util/fs.cpp b/src/util/fs.cpp index 14f7a446611..348c1b33837 100644 --- a/src/util/fs.cpp +++ b/src/util/fs.cpp @@ -1,4 +1,4 @@ -// Copyright (c) 2017-2022 The Bitcoin Core developers +// Copyright (c) 2017-present The Bitcoin Core developers // Distributed under the MIT software license, see the accompanying // file COPYING or http://www.opensource.org/licenses/mit-license.php. @@ -18,6 +18,7 @@ #endif #include +#include #include namespace fsbridge { @@ -130,4 +131,4 @@ std::string get_filesystem_error_message(const fs::filesystem_error& e) #endif } -} // fsbridge +} // namespace fsbridge diff --git a/src/util/fs.h b/src/util/fs.h index f71869f3498..3e69a540d23 100644 --- a/src/util/fs.h +++ b/src/util/fs.h @@ -1,4 +1,4 @@ -// Copyright (c) 2017-2022 The Bitcoin Core developers +// Copyright (c) 2017-present The Bitcoin Core developers // Distributed under the MIT software license, see the accompanying // file COPYING or http://www.opensource.org/licenses/mit-license.php. @@ -14,6 +14,8 @@ #include #include #include +#include +#include #include /** Filesystem operations and types */ From fa3d9304e80c214c8b073f12a7f4b08c5a94af04 Mon Sep 17 00:00:00 2001 From: MarcoFalke <*~=`'#}+{/-|&$^_@721217.xyz> Date: Sat, 9 Dec 2023 13:00:45 +0100 Subject: [PATCH 4/6] refactor: Remove pre-C++20 fs code Treating std::string as UTF-8 is deprecated in std::filesystem::path since C++20. However, it makes this codebase easier to read and maintain to retain the ability for std::string to hold UTF-8. --- src/util/fs.h | 10 ++-------- 1 file changed, 2 insertions(+), 8 deletions(-) diff --git a/src/util/fs.h b/src/util/fs.h index 3e69a540d23..4eee97c3380 100644 --- a/src/util/fs.h +++ b/src/util/fs.h @@ -56,10 +56,8 @@ public: std::string u8string() const { - const auto& utf8_str{std::filesystem::path::u8string()}; - // utf8_str might either be std::string (C++17) or std::u8string - // (C++20). Convert both to std::string. This method can be removed - // after switching to C++20. + const std::u8string& utf8_str{std::filesystem::path::u8string()}; + // Convert to std::string as a convenience for use in RPC code. return std::string{utf8_str.begin(), utf8_str.end()}; } @@ -71,11 +69,7 @@ public: static inline path u8path(const std::string& utf8_str) { -#if __cplusplus < 202002L - return std::filesystem::u8path(utf8_str); -#else return std::filesystem::path(std::u8string{utf8_str.begin(), utf8_str.end()}); -#endif } // Disallow implicit std::string conversion for absolute to avoid From 856c88776f8486446602476a1c9e133ac0cff510 Mon Sep 17 00:00:00 2001 From: Vasil Dimov Date: Thu, 2 Nov 2023 13:20:17 +0100 Subject: [PATCH 5/6] ArgsManager: return path by value from GetBlocksDirPath() `ArgsManager::m_cached_blocks_path` is protected by `ArgsManager::cs_args` and returning a reference to it after releasing the mutex is unsafe. To resolve this, return a copy of the path. This has some performance penalty which is presumably ok, given that paths are a few 100s bytes at most and `GetBlocksDirPath()` is not called often. This silences the following (clang 18): ``` common/args.cpp:288:31: error: returning variable 'm_cached_blocks_path' by reference requires holding mutex 'cs_args' [-Werror,-Wthread-safety-reference-return] 288 | if (!path.empty()) return path; | ^ ``` Do the same with `ArgsManager::GetDataDir()`, `ArgsManager::GetDataDirBase()` and `ArgsManager::GetDataDirNet()`. --- src/common/args.cpp | 4 ++-- src/common/args.h | 8 ++++---- 2 files changed, 6 insertions(+), 6 deletions(-) diff --git a/src/common/args.cpp b/src/common/args.cpp index 1f25d13bee9..a9108e59162 100644 --- a/src/common/args.cpp +++ b/src/common/args.cpp @@ -277,7 +277,7 @@ fs::path ArgsManager::GetPathArg(std::string arg, const fs::path& default_value) return result.has_filename() ? result : result.parent_path(); } -const fs::path& ArgsManager::GetBlocksDirPath() const +fs::path ArgsManager::GetBlocksDirPath() const { LOCK(cs_args); fs::path& path = m_cached_blocks_path; @@ -302,7 +302,7 @@ const fs::path& ArgsManager::GetBlocksDirPath() const return path; } -const fs::path& ArgsManager::GetDataDir(bool net_specific) const +fs::path ArgsManager::GetDataDir(bool net_specific) const { LOCK(cs_args); fs::path& path = net_specific ? m_cached_network_datadir_path : m_cached_datadir_path; diff --git a/src/common/args.h b/src/common/args.h index 1c5db718f4d..6451b194d1c 100644 --- a/src/common/args.h +++ b/src/common/args.h @@ -215,21 +215,21 @@ protected: * * @return Blocks path which is network specific */ - const fs::path& GetBlocksDirPath() const; + fs::path GetBlocksDirPath() const; /** * Get data directory path * * @return Absolute path on success, otherwise an empty path when a non-directory path would be returned */ - const fs::path& GetDataDirBase() const { return GetDataDir(false); } + fs::path GetDataDirBase() const { return GetDataDir(false); } /** * Get data directory path with appended network identifier * * @return Absolute path on success, otherwise an empty path when a non-directory path would be returned */ - const fs::path& GetDataDirNet() const { return GetDataDir(true); } + fs::path GetDataDirNet() const { return GetDataDir(true); } /** * Clear cached directory paths @@ -420,7 +420,7 @@ private: * @param net_specific Append network identifier to the returned path * @return Absolute path on success, otherwise an empty path when a non-directory path would be returned */ - const fs::path& GetDataDir(bool net_specific) const; + fs::path GetDataDir(bool net_specific) const; /** * Return -regtest/-signet/-testnet/-chain= setting as a ChainType enum if a From 66667130416b86208e01a0eb5541a15ea805ac26 Mon Sep 17 00:00:00 2001 From: MarcoFalke <*~=`'#}+{/-|&$^_@721217.xyz> Date: Thu, 14 Dec 2023 13:23:32 +0100 Subject: [PATCH 6/6] refactor: Rename fs::path::u8string() to fs::path::utf8string() --- doc/developer-notes.md | 2 +- src/bench/wallet_create.cpp | 2 +- src/qt/guiutil.cpp | 2 +- src/rpc/blockchain.cpp | 10 +++++----- src/rpc/mempool.cpp | 2 +- src/rpc/server.cpp | 2 +- src/test/fs_tests.cpp | 7 ++++--- src/util/fs.h | 17 +++++++++++------ src/wallet/rpc/backup.cpp | 4 ++-- src/wallet/rpc/wallet.cpp | 4 ++-- 10 files changed, 29 insertions(+), 23 deletions(-) diff --git a/doc/developer-notes.md b/doc/developer-notes.md index 791c049a4ab..00a3b2ea6df 100644 --- a/doc/developer-notes.md +++ b/doc/developer-notes.md @@ -1406,7 +1406,7 @@ A few guidelines for introducing and reviewing new RPC interfaces: - *Rationale*: User-facing consistency. -- Use `fs::path::u8string()` and `fs::u8path()` functions when converting path +- Use `fs::path::u8string()`/`fs::path::utf8string()` and `fs::u8path()` functions when converting path to JSON strings, not `fs::PathToString` and `fs::PathFromString` - *Rationale*: JSON strings are Unicode strings, not byte strings, and diff --git a/src/bench/wallet_create.cpp b/src/bench/wallet_create.cpp index ba3c25d25e8..993c4c4b3fd 100644 --- a/src/bench/wallet_create.cpp +++ b/src/bench/wallet_create.cpp @@ -34,7 +34,7 @@ static void WalletCreate(benchmark::Bench& bench, bool encrypted) fs::path wallet_path = test_setup->m_path_root / strprintf("test_wallet_%d", random.rand32()).c_str(); bench.run([&] { - auto wallet = CreateWallet(context, wallet_path.u8string(), /*load_on_start=*/std::nullopt, options, status, error_string, warnings); + auto wallet = CreateWallet(context, wallet_path.utf8string(), /*load_on_start=*/std::nullopt, options, status, error_string, warnings); assert(status == DatabaseStatus::SUCCESS); assert(wallet != nullptr); diff --git a/src/qt/guiutil.cpp b/src/qt/guiutil.cpp index b8ac187ff98..ebfd14cc254 100644 --- a/src/qt/guiutil.cpp +++ b/src/qt/guiutil.cpp @@ -669,7 +669,7 @@ fs::path QStringToPath(const QString &path) QString PathToQString(const fs::path &path) { - return QString::fromStdString(path.u8string()); + return QString::fromStdString(path.utf8string()); } QString NetworkToQString(Network net) diff --git a/src/rpc/blockchain.cpp b/src/rpc/blockchain.cpp index be6a8c47fd3..7556366692a 100644 --- a/src/rpc/blockchain.cpp +++ b/src/rpc/blockchain.cpp @@ -2603,7 +2603,7 @@ static RPCHelpMan dumptxoutset() if (fs::exists(path)) { throw JSONRPCError( RPC_INVALID_PARAMETER, - path.u8string() + " already exists. If you are sure this is what you want, " + path.utf8string() + " already exists. If you are sure this is what you want, " "move it out of the way first"); } @@ -2612,7 +2612,7 @@ static RPCHelpMan dumptxoutset() if (afile.IsNull()) { throw JSONRPCError( RPC_INVALID_PARAMETER, - "Couldn't open file " + temppath.u8string() + " for writing."); + "Couldn't open file " + temppath.utf8string() + " for writing."); } NodeContext& node = EnsureAnyNodeContext(request.context); @@ -2620,7 +2620,7 @@ static RPCHelpMan dumptxoutset() node, node.chainman->ActiveChainstate(), afile, path, temppath); fs::rename(temppath, path); - result.pushKV("path", path.u8string()); + result.pushKV("path", path.utf8string()); return result; }, }; @@ -2692,7 +2692,7 @@ UniValue CreateUTXOSnapshot( result.pushKV("coins_written", maybe_stats->coins_count); result.pushKV("base_hash", tip->GetBlockHash().ToString()); result.pushKV("base_height", tip->nHeight); - result.pushKV("path", path.u8string()); + result.pushKV("path", path.utf8string()); result.pushKV("txoutset_hash", maybe_stats->hashSerialized.ToString()); result.pushKV("nchaintx", tip->nChainTx); return result; @@ -2745,7 +2745,7 @@ static RPCHelpMan loadtxoutset() if (afile.IsNull()) { throw JSONRPCError( RPC_INVALID_PARAMETER, - "Couldn't open file " + path.u8string() + " for reading."); + "Couldn't open file " + path.utf8string() + " for reading."); } SnapshotMetadata metadata; diff --git a/src/rpc/mempool.cpp b/src/rpc/mempool.cpp index 04bd7fa928e..f6d9d42f0f4 100644 --- a/src/rpc/mempool.cpp +++ b/src/rpc/mempool.cpp @@ -806,7 +806,7 @@ static RPCHelpMan savemempool() } UniValue ret(UniValue::VOBJ); - ret.pushKV("filename", dump_path.u8string()); + ret.pushKV("filename", dump_path.utf8string()); return ret; }, diff --git a/src/rpc/server.cpp b/src/rpc/server.cpp index 7adc28f416e..ea5271f4344 100644 --- a/src/rpc/server.cpp +++ b/src/rpc/server.cpp @@ -243,7 +243,7 @@ static RPCHelpMan getrpcinfo() UniValue result(UniValue::VOBJ); result.pushKV("active_commands", active_commands); - const std::string path = LogInstance().m_file_path.u8string(); + const std::string path = LogInstance().m_file_path.utf8string(); UniValue log_path(UniValue::VSTR, path); result.pushKV("logpath", log_path); diff --git a/src/test/fs_tests.cpp b/src/test/fs_tests.cpp index f6fab7f733b..1581a37a07f 100644 --- a/src/test/fs_tests.cpp +++ b/src/test/fs_tests.cpp @@ -20,9 +20,10 @@ BOOST_AUTO_TEST_CASE(fsbridge_pathtostring) std::string u8_str = "fs_tests_₿_🏃"; std::u8string str8{u8"fs_tests_₿_🏃"}; BOOST_CHECK_EQUAL(fs::PathToString(fs::PathFromString(u8_str)), u8_str); - BOOST_CHECK_EQUAL(fs::u8path(u8_str).u8string(), u8_str); - BOOST_CHECK_EQUAL(fs::path(str8).u8string(), u8_str); - BOOST_CHECK_EQUAL(fs::PathFromString(u8_str).u8string(), u8_str); + BOOST_CHECK_EQUAL(fs::u8path(u8_str).utf8string(), u8_str); + BOOST_CHECK_EQUAL(fs::path(str8).utf8string(), u8_str); + BOOST_CHECK(fs::path(str8).u8string() == str8); + BOOST_CHECK_EQUAL(fs::PathFromString(u8_str).utf8string(), u8_str); BOOST_CHECK_EQUAL(fs::PathToString(fs::u8path(u8_str)), u8_str); #ifndef WIN32 // On non-windows systems, verify that arbitrary byte strings containing diff --git a/src/util/fs.h b/src/util/fs.h index 4eee97c3380..f841e0d76c5 100644 --- a/src/util/fs.h +++ b/src/util/fs.h @@ -54,10 +54,15 @@ public: // Disallow std::string conversion method to avoid locale-dependent encoding on windows. std::string string() const = delete; - std::string u8string() const + /** + * Return a UTF-8 representation of the path as a std::string, for + * compatibility with code using std::string. For code using the newer + * std::u8string type, it is more efficient to call the inherited + * std::filesystem::path::u8string method instead. + */ + std::string utf8string() const { const std::u8string& utf8_str{std::filesystem::path::u8string()}; - // Convert to std::string as a convenience for use in RPC code. return std::string{utf8_str.begin(), utf8_str.end()}; } @@ -136,7 +141,7 @@ static inline bool copy_file(const path& from, const path& to, copy_options opti * Because \ref PathToString and \ref PathFromString functions don't specify an * encoding, they are meant to be used internally, not externally. They are not * appropriate to use in applications requiring UTF-8, where - * fs::path::u8string() and fs::u8path() methods should be used instead. Other + * fs::path::u8string() / fs::path::utf8string() and fs::u8path() methods should be used instead. Other * applications could require still different encodings. For example, JSON, XML, * or URI applications might prefer to use higher-level escapes (\uXXXX or * &XXXX; or %XX) instead of multibyte encoding. Rust, Python, Java applications @@ -150,13 +155,13 @@ static inline std::string PathToString(const path& path) // use here, because these methods encode the path using C++'s narrow // multibyte encoding, which on Windows corresponds to the current "code // page", which is unpredictable and typically not able to represent all - // valid paths. So fs::path::u8string() and + // valid paths. So fs::path::utf8string() and // fs::u8path() functions are used instead on Windows. On - // POSIX, u8string/u8path functions are not safe to use because paths are + // POSIX, u8string/utf8string/u8path functions are not safe to use because paths are // not always valid UTF-8, so plain string methods which do not transform // the path there are used. #ifdef WIN32 - return path.u8string(); + return path.utf8string(); #else static_assert(std::is_same::value, "PathToString not implemented on this platform"); return path.std::filesystem::path::string(); diff --git a/src/wallet/rpc/backup.cpp b/src/wallet/rpc/backup.cpp index 3e1ec667e0a..99c548bf1d5 100644 --- a/src/wallet/rpc/backup.cpp +++ b/src/wallet/rpc/backup.cpp @@ -734,7 +734,7 @@ RPCHelpMan dumpwallet() * It may also avoid other security issues. */ if (fs::exists(filepath)) { - throw JSONRPCError(RPC_INVALID_PARAMETER, filepath.u8string() + " already exists. If you are sure this is what you want, move it out of the way first"); + throw JSONRPCError(RPC_INVALID_PARAMETER, filepath.utf8string() + " already exists. If you are sure this is what you want, move it out of the way first"); } std::ofstream file; @@ -828,7 +828,7 @@ RPCHelpMan dumpwallet() file.close(); UniValue reply(UniValue::VOBJ); - reply.pushKV("filename", filepath.u8string()); + reply.pushKV("filename", filepath.utf8string()); return reply; }, diff --git a/src/wallet/rpc/wallet.cpp b/src/wallet/rpc/wallet.cpp index 164ce9afedb..4417f8aa6a3 100644 --- a/src/wallet/rpc/wallet.cpp +++ b/src/wallet/rpc/wallet.cpp @@ -165,7 +165,7 @@ static RPCHelpMan listwalletdir() UniValue wallets(UniValue::VARR); for (const auto& path : ListDatabases(GetWalletDir())) { UniValue wallet(UniValue::VOBJ); - wallet.pushKV("name", path.u8string()); + wallet.pushKV("name", path.utf8string()); wallets.push_back(wallet); } @@ -802,7 +802,7 @@ static RPCHelpMan migratewallet() if (res->solvables_wallet) { r.pushKV("solvables_name", res->solvables_wallet->GetName()); } - r.pushKV("backup_path", res->backup_path.u8string()); + r.pushKV("backup_path", res->backup_path.utf8string()); return r; },