backport: trivial 2024 08 14

src/Makefile.am

@@ -410,7 +410,6 @@ obj/build.h: FORCE
 	  "$(abs_top_srcdir)"
 libbitcoin_util_a-clientversion.$(OBJEXT): obj/build.h
 
-ipc/capnp/libbitcoin_ipc_a-ipc.$(OBJEXT): $(libbitcoin_ipc_mpgen_input:=.h)
 
 # server: shared between dashd and dash-qt
 # Contains code accessing mempool and chain state that is meant to be separated
@@ -1015,12 +1014,18 @@ libbitcoin_ipc_mpgen_input = \
 EXTRA_DIST += $(libbitcoin_ipc_mpgen_input)
 %.capnp:
 
+# Explicitly list dependencies on generated headers as described in
+# https://www.gnu.org/software/automake/manual/html_node/Built-Sources-Example.html#Recording-Dependencies-manually
+ipc/capnp/libbitcoin_ipc_a-protocol.$(OBJEXT): $(libbitcoin_ipc_mpgen_input:=.h)
+
 if BUILD_MULTIPROCESS
 LIBBITCOIN_IPC=libbitcoin_ipc.a
 libbitcoin_ipc_a_SOURCES = \
+  ipc/capnp/context.h \
   ipc/capnp/init-types.h \
   ipc/capnp/protocol.cpp \
   ipc/capnp/protocol.h \
+  ipc/context.h \
   ipc/exception.h \
   ipc/interfaces.cpp \
   ipc/process.cpp \

src/fs.h

@@ -154,6 +154,28 @@ static inline path PathFromString(const std::string& string)
     return std::filesystem::path(string);
 #endif
 }
+
+/**
+ * Create directory (and if necessary its parents), unless the leaf directory
+ * already exists or is a symlink to an existing directory.
+ * This is a temporary workaround for an issue in libstdc++ that has been fixed
+ * upstream [PR101510].
+ */
+static inline bool create_directories(const std::filesystem::path& p)
+{
+    if (std::filesystem::is_symlink(p) && std::filesystem::is_directory(p)) {
+        return false;
+    }
+    return std::filesystem::create_directories(p);
+}
+
+/**
+ * This variant is not used. Delete it to prevent it from accidentally working
+ * around the workaround. If it is needed, add a workaround in the same pattern
+ * as above.
+ */
+bool create_directories(const std::filesystem::path& p, std::error_code& ec) = delete;
+
 } // namespace fs
 
 /** Bridge operations to C stdio */

src/interfaces/ipc.h

@@ -9,6 +9,10 @@
 #include <memory>
 #include <typeindex>
 
+namespace ipc {
+struct Context;
+} // namespace ipc
+
 namespace interfaces {
 class Init;
 
@@ -58,6 +62,9 @@ class Ipc
         addCleanup(typeid(Interface), &iface, std::move(cleanup));
     }
 
+    //! IPC context struct accessor (see struct definition for more description).
+    virtual ipc::Context& context() = 0;
+
 protected:
     //! Internal implementation of public addCleanup method (above) as a
     //! type-erased virtual function, since template functions can't be virtual.

src/ipc/capnp/context.h

@@ -0,0 +1,23 @@
+// Copyright (c) 2021 The Bitcoin Core developers
+// Distributed under the MIT software license, see the accompanying
+// file COPYING or http://www.opensource.org/licenses/mit-license.php.
+
+#ifndef BITCOIN_IPC_CAPNP_CONTEXT_H
+#define BITCOIN_IPC_CAPNP_CONTEXT_H
+
+#include <ipc/context.h>
+
+namespace ipc {
+namespace capnp {
+//! Cap'n Proto context struct. Generally the parent ipc::Context struct should
+//! be used instead of this struct to give all IPC protocols access to
+//! application state, so there aren't unnecessary differences between IPC
+//! protocols. But this specialized struct can be used to pass capnp-specific
+//! function and object types to capnp hooks.
+struct Context : ipc::Context
+{
+};
+} // namespace capnp
+} // namespace ipc
+
+#endif // BITCOIN_IPC_CAPNP_CONTEXT_H

src/ipc/capnp/protocol.cpp

@@ -3,6 +3,7 @@
 // file COPYING or http://www.opensource.org/licenses/mit-license.php.
 
 #include <interfaces/init.h>
+#include <ipc/capnp/context.h>
 #include <ipc/capnp/init.capnp.h>
 #include <ipc/capnp/init.capnp.proxy.h>
 #include <ipc/capnp/protocol.h>
@@ -54,7 +55,7 @@ class CapnpProtocol : public Protocol
     {
         assert(!m_loop);
         mp::g_thread_context.thread_name = mp::ThreadName(exe_name);
-        m_loop.emplace(exe_name, &IpcLogFn, nullptr);
+        m_loop.emplace(exe_name, &IpcLogFn, &m_context);
         mp::ServeStream<messages::Init>(*m_loop, fd, init);
         m_loop->loop();
         m_loop.reset();
@@ -63,13 +64,14 @@ class CapnpProtocol : public Protocol
     {
         mp::ProxyTypeRegister::types().at(type)(iface).cleanup.emplace_back(std::move(cleanup));
     }
+    Context& context() override { return m_context; }
     void startLoop(const char* exe_name)
     {
         if (m_loop) return;
         std::promise<void> promise;
         m_loop_thread = std::thread([&] {
             util::ThreadRename("capnp-loop");
-            m_loop.emplace(exe_name, &IpcLogFn, nullptr);
+            m_loop.emplace(exe_name, &IpcLogFn, &m_context);
             {
                 std::unique_lock<std::mutex> lock(m_loop->m_mutex);
                 m_loop->addClient(lock);
@@ -80,6 +82,7 @@ class CapnpProtocol : public Protocol
         });
         promise.get_future().wait();
     }
+    Context m_context;
     std::thread m_loop_thread;
     std::optional<mp::EventLoop> m_loop;
 };

src/ipc/context.h

@@ -0,0 +1,19 @@
+// Copyright (c) 2021 The Bitcoin Core developers
+// Distributed under the MIT software license, see the accompanying
+// file COPYING or http://www.opensource.org/licenses/mit-license.php.
+
+#ifndef BITCOIN_IPC_CONTEXT_H
+#define BITCOIN_IPC_CONTEXT_H
+
+namespace ipc {
+//! Context struct used to give IPC protocol implementations or implementation
+//! hooks access to application state, in case they need to run extra code that
+//! isn't needed within a single process, like code copying global state from an
+//! existing process to a new process when it's initialized, or code dealing
+//! with shared objects that are created or destroyed remotely.
+struct Context
+{
+};
+} // namespace ipc
+
+#endif // BITCOIN_IPC_CONTEXT_H

src/ipc/interfaces.cpp

@@ -60,6 +60,7 @@ class IpcImpl : public interfaces::Ipc
     {
         m_protocol->addCleanup(type, iface, std::move(cleanup));
     }
+    Context& context() override { return m_protocol->context(); }
     const char* m_exe_name;
     const char* m_process_argv0;
     interfaces::Init& m_init;

src/ipc/protocol.h

@@ -12,6 +12,8 @@
 #include <typeindex>
 
 namespace ipc {
+struct Context;
+
 //! IPC protocol interface for calling IPC methods over sockets.
 //!
 //! There may be different implementations of this interface for different IPC
@@ -33,6 +35,9 @@ class Protocol
     //! Add cleanup callback to interface that will run when the interface is
     //! deleted.
     virtual void addCleanup(std::type_index type, void* iface, std::function<void()> cleanup) = 0;
+
+    //! Context accessor.
+    virtual Context& context() = 0;
 };
 } // namespace ipc
 

src/test/dbwrapper_tests.cpp

@@ -203,7 +203,7 @@ BOOST_AUTO_TEST_CASE(existing_data_no_obfuscate)
 {
     // We're going to share this fs::path between two wrappers
     fs::path ph = m_args.GetDataDirBase() / "existing_data_no_obfuscate";
-    create_directories(ph);
+    fs::create_directories(ph);
 
     // Set up a non-obfuscated wrapper to write some initial data.
     std::unique_ptr<CDBWrapper> dbw = std::make_unique<CDBWrapper>(ph, (1 << 10), false, false, false);
@@ -244,7 +244,7 @@ BOOST_AUTO_TEST_CASE(existing_data_reindex)
 {
     // We're going to share this fs::path between two wrappers
     fs::path ph = m_args.GetDataDirBase() / "existing_data_reindex";
-    create_directories(ph);
+    fs::create_directories(ph);
 
     // Set up a non-obfuscated wrapper to write some initial data.
     std::unique_ptr<CDBWrapper> dbw = std::make_unique<CDBWrapper>(ph, (1 << 10), false, false, false);

src/test/fs_tests.cpp

@@ -118,5 +118,29 @@ BOOST_AUTO_TEST_CASE(fsbridge_fstream)
     }
 }
 
+#ifndef WIN32
+BOOST_AUTO_TEST_CASE(create_directories)
+{
+    // Test fs::create_directories workaround.
+    const fs::path tmpfolder{m_args.GetDataDirBase()};
+
+    const fs::path dir{GetUniquePath(tmpfolder)};
+    fs::create_directory(dir);
+    BOOST_CHECK(fs::exists(dir));
+    BOOST_CHECK(fs::is_directory(dir));
+    BOOST_CHECK(!fs::create_directories(dir));
+
+    const fs::path symlink{GetUniquePath(tmpfolder)};
+    fs::create_directory_symlink(dir, symlink);
+    BOOST_CHECK(fs::exists(symlink));
+    BOOST_CHECK(fs::is_symlink(symlink));
+    BOOST_CHECK(fs::is_directory(symlink));
+    BOOST_CHECK(!fs::create_directories(symlink));
+
+    fs::remove(symlink);
+    fs::remove(dir);
+}
+#endif // WIN32
+
 BOOST_AUTO_TEST_SUITE_END()
 

src/test/fuzz/rpc.cpp

@@ -41,13 +41,17 @@ struct RPCFuzzTestingSetup : public TestingSetup {
     {
     }
 
-    UniValue CallRPC(const std::string& rpc_method, const std::vector<std::string>& arguments)
+    void CallRPC(const std::string& rpc_method, const std::vector<std::string>& arguments)
     {
         JSONRPCRequest request;
         request.context = m_node;
         request.strMethod = rpc_method;
-        request.params = RPCConvertValues(rpc_method, arguments);
-        return tableRPC.execute(request);
+        try {
+            request.params = RPCConvertValues(rpc_method, arguments);
+        } catch (const std::runtime_error&) {
+            return;
+        }
+        tableRPC.execute(request);
     }
 
     std::vector<std::string> GetRPCCommands() const
@@ -353,7 +357,13 @@ FUZZ_TARGET_INIT(rpc, initialize_rpc)
     }
     try {
         rpc_testing_setup->CallRPC(rpc_command, arguments);
-    } catch (const UniValue&) {
-    } catch (const std::runtime_error&) {
+    } catch (const UniValue& json_rpc_error) {
+        const std::string error_msg{find_value(json_rpc_error, "message").get_str()};
+        // Once c++20 is allowed, starts_with can be used.
+        // if (error_msg.starts_with("Internal bug detected")) {
+        if (0 == error_msg.rfind("Internal bug detected", 0)) {
+            // Only allow the intentional internal bug
+            assert(error_msg.find("trigger_internal_bug") != std::string::npos);
+        }
     }
 }

src/test/util_tests.cpp

@@ -73,12 +73,9 @@ BOOST_AUTO_TEST_CASE(util_datadir)
     args.ClearPathCache();
     BOOST_CHECK_EQUAL(dd_norm, args.GetDataDirBase());
 
-#ifndef WIN32
-    // Windows does not consider "datadir/.//" to be a valid directory path.
     args.ForceSetArg("-datadir", fs::PathToString(dd_norm) + "/.//");
     args.ClearPathCache();
     BOOST_CHECK_EQUAL(dd_norm, args.GetDataDirBase());
-#endif
 }
 
 namespace {

src/util/check.h

@@ -33,11 +33,11 @@ class NonFatalCheckError : public std::runtime_error
     do {                                                          \
         if (!(condition)) {                                       \
             throw NonFatalCheckError(                             \
-                strprintf("%s:%d (%s)\n"                          \
-                          "Internal bug detected: '%s'\n"         \
+                strprintf("Internal bug detected: '%s'\n"         \
+                          "%s:%d (%s)\n"                          \
                           "You may report this issue here: %s\n", \
-                    __FILE__, __LINE__, __func__,                 \
                     (#condition),                                 \
+                    __FILE__, __LINE__, __func__,                 \
                     PACKAGE_BUGREPORT));                          \
         }                                                         \
     } while (false)

src/util/system.cpp

@@ -80,6 +80,7 @@
 #include <map>
 #include <memory>
 #include <string>
+#include <system_error>
 #include <thread>
 #include <typeinfo>
 
@@ -268,7 +269,7 @@ fs::path StripRedundantLastElementsOfPath(const fs::path& path)
         result = result.parent_path();
     }
 
-    assert(fs::equivalent(result, path));
+    assert(fs::equivalent(result, path.lexically_normal()));
     return result;
 }
 } // namespace
@@ -1133,13 +1134,9 @@ std::vector<util::SettingsValue> ArgsManager::GetSettingsList(const std::string&
 
 bool RenameOver(fs::path src, fs::path dest)
 {
-#ifdef WIN32
-    return MoveFileExW(src.wstring().c_str(), dest.wstring().c_str(),
-                       MOVEFILE_REPLACE_EXISTING) != 0;
-#else
-    int rc = std::rename(src.c_str(), dest.c_str());
-    return (rc == 0);
-#endif /* WIN32 */
+    std::error_code error;
+    fs::rename(src, dest, error);
+    return !error;
 }
 
 /**

src/util/system.h

@@ -75,7 +75,13 @@ void DirectoryCommit(const fs::path &dirname);
 bool TruncateFile(FILE *file, unsigned int length);
 int RaiseFileDescriptorLimit(int nMinFD);
 void AllocateFileRange(FILE *file, unsigned int offset, unsigned int length);
+
+/**
+ * Rename src to dest.
+ * @return true if the rename was successful.
+ */
 [[nodiscard]] bool RenameOver(fs::path src, fs::path dest);
+
 bool LockDirectory(const fs::path& directory, const std::string lockfile_name, bool probe_only=false);
 void UnlockDirectory(const fs::path& directory, const std::string& lockfile_name);
 bool DirIsWritable(const fs::path& directory);

src/wallet/db.cpp

@@ -16,7 +16,6 @@
 
 std::vector<fs::path> ListDatabases(const fs::path& wallet_dir)
 {
-    const size_t offset = wallet_dir.native().size() + (wallet_dir == wallet_dir.root_name() ? 0 : 1);
     std::vector<fs::path> paths;
     std::error_code ec;
 
@@ -32,10 +31,7 @@ std::vector<fs::path> ListDatabases(const fs::path& wallet_dir)
         }
 
         try {
-            // Get wallet path relative to walletdir by removing walletdir from the wallet path.
-            // This can be replaced by boost::filesystem::lexically_relative once boost is bumped to 1.60.
-            const auto path_str = it->path().native().substr(offset);
-            const fs::path path{path_str.begin(), path_str.end()};
+            const fs::path path{it->path().lexically_relative(wallet_dir)};
 
             if (it->status().type() == fs::file_type::directory &&
                 (IsBDBFile(BDBDataFile(it->path())) || IsSQLiteFile(SQLiteDataFile(it->path())))) {

src/wallet/load.cpp

@@ -27,11 +27,13 @@ bool VerifyWallets(interfaces::Chain& chain)
         fs::path wallet_dir = fs::PathFromString(gArgs.GetArg("-walletdir", ""));
         std::error_code error;
         // The canonical path cleans the path, preventing >1 Berkeley environment instances for the same directory
+        // It also lets the fs::exists and fs::is_directory checks below pass on windows, since they return false
+        // if a path has trailing slashes, and it strips trailing slashes.
         fs::path canonical_wallet_dir = fs::canonical(wallet_dir, error);
-        if (error || !fs::exists(wallet_dir)) {
+        if (error || !fs::exists(canonical_wallet_dir)) {
             chain.initError(strprintf(_("Specified -walletdir \"%s\" does not exist"), fs::PathToString(wallet_dir)));
             return false;
-        } else if (!fs::is_directory(wallet_dir)) {
+        } else if (!fs::is_directory(canonical_wallet_dir)) {
             chain.initError(strprintf(_("Specified -walletdir \"%s\" is not a directory"), fs::PathToString(wallet_dir)));
             return false;
         // The canonical path transforms relative paths into absolute ones, so we check the non-canonical version