Merge #20932: refactor: Replace fs::absolute calls with AbsPathJoin calls

da9caa1cedd69702aea44cb44b2fd0a2d6d56916 Replace fs::absolute calls with AbsPathJoin calls (Kiminuo)
66576c4fd532ac18b8b355ea93d25581a2c15654 test: Clear forced -walletdir setting after wallet init_tests (Kiminuo)

Pull request description:

  This adds better test coverage and will make it easier in #20744 to remove our dependency on the two-argument boost::filesystem::absolute() function which does not have a direct equivalent in C++17.

  This PR doesn't change behavior aside from adding an assert and fixing a test bug.

ACKs for top commit:
  jonatack:
    Code review ACK da9caa1cedd69702aea44cb44b2fd0a2d6d56916 only doxygen improvements since my last review per `git diff d867d7a da9caa1`
  MarcoFalke:
    review ACK da9caa1cedd69702aea44cb44b2fd0a2d6d56916 📯
  ryanofsky:
    Code review ACK da9caa1cedd69702aea44cb44b2fd0a2d6d56916. Just comment and test tweaks since previous review.

Tree-SHA512: c940ee60f3ba374d4927cf34cf12d27c4c735c94af591fbc0ca408c641b30f8f8fbcfe521d66bfbddf9877a1fc8cd99bd8a47ebcd2fa59789de6bd87a7b9cf4d

9 files changed, 45 insertions, 7 deletions

diff --git a/src/fs.cpp b/src/fs.cpp
index 7eb0f5ca9f..4f20ca4d28 100644
--- a/src/fs.cpp
+++ b/src/fs.cpp
@@ -31,6 +31,12 @@ FILE *fopen(const fs::path& p, const char *mode)
 #endif
 }
 
+fs::path AbsPathJoin(const fs::path& base, const fs::path& path)
+{
+    assert(base.is_absolute());
+    return fs::absolute(path, base);
+}
+
 #ifndef WIN32
 
 static std::string GetErrorReason()
diff --git a/src/fs.h b/src/fs.h
index dfbecc18e6..d77b90be66 100644
--- a/src/fs.h
+++ b/src/fs.h
@@ -21,6 +21,17 @@ namespace fs = boost::filesystem;
 namespace fsbridge {
     FILE *fopen(const fs::path& p, const char *mode);
 
+    /**
+     * Helper function for joining two paths
+     *
+     * @param[in] base  Base path
+     * @param[in] path  Path to combine with base
+     * @returns path unchanged if it is an absolute path, otherwise returns base joined with path. Returns base unchanged if path is empty.
+     * @pre  Base path must be absolute
+     * @post Returned path will always be absolute
+     */
+    fs::path AbsPathJoin(const fs::path& base, const fs::path& path);
+
     class FileLock
     {
     public:
diff --git a/src/rpc/blockchain.cpp b/src/rpc/blockchain.cpp
index 3f97f6f3d9..7a336c1ad6 100644
--- a/src/rpc/blockchain.cpp
+++ b/src/rpc/blockchain.cpp
@@ -2399,10 +2399,10 @@ static RPCHelpMan dumptxoutset()
         },
         [&](const RPCHelpMan& self, const JSONRPCRequest& request) -> UniValue
 {
-    fs::path path = fs::absolute(request.params[0].get_str(), GetDataDir());
+    const fs::path path = fsbridge::AbsPathJoin(GetDataDir(), request.params[0].get_str());
     // Write to a temporary path and then move into `path` on completion
     // to avoid confusion due to an interruption.
-    fs::path temppath = fs::absolute(request.params[0].get_str() + ".incomplete", GetDataDir());
+    const fs::path temppath = fsbridge::AbsPathJoin(GetDataDir(), request.params[0].get_str() + ".incomplete");
 
     if (fs::exists(path)) {
         throw JSONRPCError(
diff --git a/src/test/fs_tests.cpp b/src/test/fs_tests.cpp
index d02c3613ba..ec487aa3ff 100644
--- a/src/test/fs_tests.cpp
+++ b/src/test/fs_tests.cpp
@@ -52,6 +52,23 @@ BOOST_AUTO_TEST_CASE(fsbridge_fstream)
         file >> input_buffer;
         BOOST_CHECK_EQUAL(input_buffer, "bitcoin");
     }
+    {
+        // Join an absolute path and a relative path.
+        fs::path p = fsbridge::AbsPathJoin(tmpfolder, "fs_tests_₿_🏃");
+        BOOST_CHECK(p.is_absolute());
+        BOOST_CHECK_EQUAL(tmpfile1, p);
+    }
+    {
+        // Join two absolute paths.
+        fs::path p = fsbridge::AbsPathJoin(tmpfile1, tmpfile2);
+        BOOST_CHECK(p.is_absolute());
+        BOOST_CHECK_EQUAL(tmpfile2, p);
+    }
+    {
+        // Ensure joining with empty paths does not add trailing path components.
+        BOOST_CHECK_EQUAL(tmpfile1, fsbridge::AbsPathJoin(tmpfile1, ""));
+        BOOST_CHECK_EQUAL(tmpfile1, fsbridge::AbsPathJoin(tmpfile1, {}));
+    }
 }
 
 BOOST_AUTO_TEST_SUITE_END()
diff --git a/src/util/system.cpp b/src/util/system.cpp
index 917d37ccc7..d1fb921642 100644
--- a/src/util/system.cpp
+++ b/src/util/system.cpp
@@ -398,7 +398,7 @@ bool ArgsManager::GetSettingsPath(fs::path* filepath, bool temp) const
     }
     if (filepath) {
         std::string settings = GetArg("-settings", BITCOIN_SETTINGS_FILENAME);
-        *filepath = fs::absolute(temp ? settings + ".tmp" : settings, GetDataDir(/* net_specific= */ true));
+        *filepath = fsbridge::AbsPathJoin(GetDataDir(/* net_specific= */ true), temp ? settings + ".tmp" : settings);
     }
     return true;
 }
@@ -1311,7 +1311,7 @@ fs::path AbsPathForConfigVal(const fs::path& path, bool net_specific)
     if (path.is_absolute()) {
         return path;
     }
-    return fs::absolute(path, GetDataDir(net_specific));
+    return fsbridge::AbsPathJoin(GetDataDir(net_specific), path);
 }
 
 void ScheduleBatchPriority()
diff --git a/src/wallet/load.cpp b/src/wallet/load.cpp
index 036fd4956f..30832f983b 100644
--- a/src/wallet/load.cpp
+++ b/src/wallet/load.cpp
@@ -62,7 +62,7 @@ bool VerifyWallets(interfaces::Chain& chain)
     std::set<fs::path> wallet_paths;
 
     for (const auto& wallet_file : gArgs.GetArgs("-wallet")) {
-        const fs::path path = fs::absolute(wallet_file, GetWalletDir());
+        const fs::path path = fsbridge::AbsPathJoin(GetWalletDir(), wallet_file);
 
         if (!wallet_paths.insert(path).second) {
             chain.initWarning(strprintf(_("Ignoring duplicate -wallet %s."), wallet_file));
diff --git a/src/wallet/test/init_test_fixture.cpp b/src/wallet/test/init_test_fixture.cpp
index aa7f1c83d8..f035a70a20 100644
--- a/src/wallet/test/init_test_fixture.cpp
+++ b/src/wallet/test/init_test_fixture.cpp
@@ -3,6 +3,7 @@
 // file COPYING or http://www.opensource.org/licenses/mit-license.php.
 
 #include <fs.h>
+#include <univalue.h>
 #include <util/check.h>
 #include <util/system.h>
 
@@ -37,6 +38,9 @@ InitWalletDirTestingSetup::InitWalletDirTestingSetup(const std::string& chainNam
 
 InitWalletDirTestingSetup::~InitWalletDirTestingSetup()
 {
+    gArgs.LockSettings([&](util::Settings& settings) {
+        settings.forced_settings.erase("walletdir");
+    });
     fs::current_path(m_cwd);
 }
 
diff --git a/src/wallet/wallet.cpp b/src/wallet/wallet.cpp
index 918946f9e7..723552860a 100644
--- a/src/wallet/wallet.cpp
+++ b/src/wallet/wallet.cpp
@@ -3780,7 +3780,7 @@ std::unique_ptr<WalletDatabase> MakeWalletDatabase(const std::string& name, cons
     // 2. Path to an existing directory.
     // 3. Path to a symlink to a directory.
     // 4. For backwards compatibility, the name of a data file in -walletdir.
-    const fs::path& wallet_path = fs::absolute(name, GetWalletDir());
+    const fs::path wallet_path = fsbridge::AbsPathJoin(GetWalletDir(), name);
     fs::file_type path_type = fs::symlink_status(wallet_path).type();
     if (!(path_type == fs::file_not_found || path_type == fs::directory_file ||
           (path_type == fs::symlink_file && fs::is_directory(wallet_path)) ||
diff --git a/src/wallet/wallettool.cpp b/src/wallet/wallettool.cpp
index a1bb7343f4..bc90491a2c 100644
--- a/src/wallet/wallettool.cpp
+++ b/src/wallet/wallettool.cpp
@@ -105,7 +105,7 @@ static void WalletShowInfo(CWallet* wallet_instance)
 
 bool ExecuteWalletToolFunc(const ArgsManager& args, const std::string& command, const std::string& name)
 {
-    fs::path path = fs::absolute(name, GetWalletDir());
+    const fs::path path = fsbridge::AbsPathJoin(GetWalletDir(), name);
 
     if (args.IsArgSet("-format") && command != "createfromdump") {
         tfm::format(std::cerr, "The -format option can only be used with the \"createfromdump\" command.\n");