Merge bitcoin/bitcoin#28721: multiprocess compatibility updates

3b70f7b615 doc: fix broken doc/design/multiprocess.md links after #24352 (Ryan Ofsky)
6d43aad742 span: Make Span template deduction guides work in SFINAE context (Ryan Ofsky)
8062c3bdb9 util: Add ArgsManager SetConfigFilePath method (Ryan Ofsky)
441d00c60f interfaces: Rename CalculateBumpFees methods to be compatible with capn'proto (Ryan Ofsky)
156f49d682 interfaces: Change getUnspentOutput return type to avoid multiprocess segfault (Ryan Ofsky)
4978754c00 interfaces: Add schedulerMockForward method so mockscheduler RPC can work across processes (Ryan Ofsky)
924327eaf3 interfaces: Fix const virtual method that breaks multiprocess support (Ryan Ofsky)
82a379eca8 streams: Add SpanReader ignore method (Russell Yanofsky)

Pull request description:

  This is a collection of small changes to interfaces and code which were needed as part of multiprocess PR #10102, but have been moved here to make that PR smaller.

  All of these changes are refactoring changes which do not affect behavior of current code

  ---

  This PR is part of the [process separation project](https://github.com/bitcoin/bitcoin/issues/28722).

ACKs for top commit:
  achow101:
    ACK 3b70f7b615
  naumenkogs:
    ACK 3b70f7b615
  maflcko:
    re-ACK 3b70f7b615  🎆

Tree-SHA512: 2368772b887056ad8a9f84c299cfde76ba45943770e3b5353130580900afa9611302195b899ced7b6e303b11f053ff204cae7c28ff4e12c55562fcc81119ba4c

doc/design/multiprocess.md

@@ -19,7 +19,7 @@ The `-debug=ipc` command line option can be used to see requests and responses b
 
 ## Installation
 
-The multiprocess feature requires [Cap'n Proto](https://capnproto.org/) and [libmultiprocess](https://github.com/chaincodelabs/libmultiprocess) as dependencies. A simple way to get starting using it without installing these dependencies manually is to use the [depends system](../depends) with the `MULTIPROCESS=1` [dependency option](../depends#dependency-options) passed to make:
+The multiprocess feature requires [Cap'n Proto](https://capnproto.org/) and [libmultiprocess](https://github.com/chaincodelabs/libmultiprocess) as dependencies. A simple way to get starting using it without installing these dependencies manually is to use the [depends system](../../depends) with the `MULTIPROCESS=1` [dependency option](../../depends#dependency-options) passed to make:
 
 ```
 cd <BITCOIN_SOURCE_DIRECTORY>
@@ -32,12 +32,12 @@ BITCOIND=bitcoin-node test/functional/test_runner.py
 
 The configure script will pick up settings and library locations from the depends directory, so there is no need to pass `--enable-multiprocess` as a separate flag when using the depends system (it's controlled by the `MULTIPROCESS=1` option).
 
-Alternately, you can install [Cap'n Proto](https://capnproto.org/) and [libmultiprocess](https://github.com/chaincodelabs/libmultiprocess) packages on your system, and just run `./configure --enable-multiprocess` without using the depends system. The configure script will be able to locate the installed packages via [pkg-config](https://www.freedesktop.org/wiki/Software/pkg-config/). See [Installation](https://github.com/chaincodelabs/libmultiprocess#installation) section of the libmultiprocess readme for install steps. See [build-unix.md](build-unix.md) and [build-osx.md](build-osx.md) for information about installing dependencies in general.
+Alternately, you can install [Cap'n Proto](https://capnproto.org/) and [libmultiprocess](https://github.com/chaincodelabs/libmultiprocess) packages on your system, and just run `./configure --enable-multiprocess` without using the depends system. The configure script will be able to locate the installed packages via [pkg-config](https://www.freedesktop.org/wiki/Software/pkg-config/). See [Installation](https://github.com/chaincodelabs/libmultiprocess/blob/master/doc/install.md) section of the libmultiprocess readme for install steps. See [build-unix.md](../build-unix.md) and [build-osx.md](../build-osx.md) for information about installing dependencies in general.
 
 ## IPC implementation details
 
 Cross process Node, Wallet, and Chain interfaces are defined in
-[`src/interfaces/`](../src/interfaces/). These are C++ classes which follow
+[`src/interfaces/`](../../src/interfaces/). These are C++ classes which follow
 [conventions](../developer-notes.md#internal-interface-guidelines), like passing
 serializable arguments so they can be called from different processes, and
 making methods pure virtual so they can have proxy implementations that forward
@@ -58,7 +58,7 @@ actual serialization and socket communication.
 As much as possible, calls between processes are meant to work the same as
 calls within a single process without adding limitations or requiring extra
 implementation effort. Processes communicate with each other by calling regular
-[C++ interface methods](../src/interfaces/README.md). Method arguments and
+[C++ interface methods](../../src/interfaces/README.md). Method arguments and
 return values are automatically serialized and sent between processes. Object
 references and `std::function` arguments are automatically tracked and mapped
 to allow invoked code to call back into invoking code at any time, and there is

src/Makefile.test.include

@@ -146,6 +146,7 @@ BITCOIN_TESTS =\
   test/sigopcount_tests.cpp \
   test/skiplist_tests.cpp \
   test/sock_tests.cpp \
+  test/span_tests.cpp \
   test/streams_tests.cpp \
   test/sync_tests.cpp \
   test/system_tests.cpp \

src/common/args.cpp

@@ -720,6 +720,13 @@ fs::path ArgsManager::GetConfigFilePath() const
     return *Assert(m_config_path);
 }
 
+void ArgsManager::SetConfigFilePath(fs::path path)
+{
+    LOCK(cs_args);
+    assert(!m_config_path);
+    m_config_path = path;
+}
+
 ChainType ArgsManager::GetChainType() const
 {
     std::variant<ChainType, std::string> arg = GetChainArg();

src/common/args.h

@@ -180,6 +180,7 @@ protected:
      * Return config file path (read-only)
      */
     fs::path GetConfigFilePath() const;
+    void SetConfigFilePath(fs::path);
     [[nodiscard]] bool ReadConfigFiles(std::string& error, bool ignore_invalid_keys = false);
 
     /**

src/interfaces/chain.h

@@ -244,7 +244,7 @@ public:
     //  outputs in the same transaction) or have shared ancestry, the bump fees are calculated
     //  independently, i.e. as if only one of them is spent. This may result in double-fee-bumping. This
     //  caveat can be rectified per use of the sister-function CalculateCombinedBumpFee(…).
-    virtual std::map<COutPoint, CAmount> CalculateIndividualBumpFees(const std::vector<COutPoint>& outpoints, const CFeeRate& target_feerate) = 0;
+    virtual std::map<COutPoint, CAmount> calculateIndividualBumpFees(const std::vector<COutPoint>& outpoints, const CFeeRate& target_feerate) = 0;
 
     //! Calculate the combined bump fee for an input set per the same strategy
     //  as in CalculateIndividualBumpFees(…).
@@ -252,7 +252,7 @@ public:
     //  bump fees per outpoint, but a single bump fee for the shared ancestry.
     //  The combined bump fee may be used to correct overestimation due to
     //  shared ancestry by multiple UTXOs after coin selection.
-    virtual std::optional<CAmount> CalculateCombinedBumpFee(const std::vector<COutPoint>& outpoints, const CFeeRate& target_feerate) = 0;
+    virtual std::optional<CAmount> calculateCombinedBumpFee(const std::vector<COutPoint>& outpoints, const CFeeRate& target_feerate) = 0;
 
     //! Get the node's package limits.
     //! Currently only returns the ancestor and descendant count limits, but could be enhanced to
@@ -395,6 +395,9 @@ public:
 
     //! Set mock time.
     virtual void setMockTime(int64_t time) = 0;
+
+    //! Mock the scheduler to fast forward in time.
+    virtual void schedulerMockForward(std::chrono::seconds delta_seconds) = 0;
 };
 
 //! Return implementation of Chain interface.

src/interfaces/node.h

@@ -204,8 +204,8 @@ public:
     //! Unset RPC timer interface.
     virtual void rpcUnsetTimerInterface(RPCTimerInterface* iface) = 0;
 
-    //! Get unspent outputs associated with a transaction.
+    //! Get unspent output associated with a transaction.
-    virtual bool getUnspentOutput(const COutPoint& output, Coin& coin) = 0;
+    virtual std::optional<Coin> getUnspentOutput(const COutPoint& output) = 0;
 
     //! Broadcast transaction.
     virtual TransactionError broadcastTransaction(CTransactionRef tx, CAmount max_tx_fee, std::string& err_string) = 0;

src/interfaces/wallet.h

@@ -118,7 +118,7 @@ public:
         wallet::AddressPurpose* purpose) = 0;
 
     //! Get wallet address list.
-    virtual std::vector<WalletAddress> getAddresses() const = 0;
+    virtual std::vector<WalletAddress> getAddresses() = 0;
 
     //! Get receive requests.
     virtual std::vector<std::string> getAddressReceiveRequests() = 0;

src/node/interfaces.cpp

@@ -328,10 +328,12 @@ public:
     std::vector<std::string> listRpcCommands() override { return ::tableRPC.listCommands(); }
     void rpcSetTimerInterfaceIfUnset(RPCTimerInterface* iface) override { RPCSetTimerInterfaceIfUnset(iface); }
     void rpcUnsetTimerInterface(RPCTimerInterface* iface) override { RPCUnsetTimerInterface(iface); }
-    bool getUnspentOutput(const COutPoint& output, Coin& coin) override
+    std::optional<Coin> getUnspentOutput(const COutPoint& output) override
     {
         LOCK(::cs_main);
-        return chainman().ActiveChainstate().CoinsTip().GetCoin(output, coin);
+        Coin coin;
+        if (chainman().ActiveChainstate().CoinsTip().GetCoin(output, coin)) return coin;
+        return {};
     }
     TransactionError broadcastTransaction(CTransactionRef tx, CAmount max_tx_fee, std::string& err_string) override
     {
@@ -670,7 +672,7 @@ public:
         m_node.mempool->GetTransactionAncestry(txid, ancestors, descendants, ancestorsize, ancestorfees);
     }
 
-    std::map<COutPoint, CAmount> CalculateIndividualBumpFees(const std::vector<COutPoint>& outpoints, const CFeeRate& target_feerate) override
+    std::map<COutPoint, CAmount> calculateIndividualBumpFees(const std::vector<COutPoint>& outpoints, const CFeeRate& target_feerate) override
     {
         if (!m_node.mempool) {
             std::map<COutPoint, CAmount> bump_fees;
@@ -682,7 +684,7 @@ public:
         return MiniMiner(*m_node.mempool, outpoints).CalculateBumpFees(target_feerate);
     }
 
-    std::optional<CAmount> CalculateCombinedBumpFee(const std::vector<COutPoint>& outpoints, const CFeeRate& target_feerate) override
+    std::optional<CAmount> calculateCombinedBumpFee(const std::vector<COutPoint>& outpoints, const CFeeRate& target_feerate) override
     {
         if (!m_node.mempool) {
             return 0;

src/qt/transactiondesc.cpp

@@ -360,12 +360,10 @@ QString TransactionDesc::toHTML(interfaces::Node& node, interfaces::Wallet& wall
         {
             COutPoint prevout = txin.prevout;
 
-            Coin prev;
+            if (auto prev{node.getUnspentOutput(prevout)}) {
-            if(node.getUnspentOutput(prevout, prev))
-            {
                 {
                     strHTML += "<li>";
-                    const CTxOut &vout = prev.out;
+                    const CTxOut& vout = prev->out;
                     CTxDestination address;
                     if (ExtractDestination(vout.scriptPubKey, address))
                     {

src/rpc/node.cpp

@@ -91,6 +91,9 @@ static RPCHelpMan mockscheduler()
     const NodeContext& node_context{EnsureAnyNodeContext(request.context)};
     CHECK_NONFATAL(node_context.scheduler)->MockForward(std::chrono::seconds{delta_seconds});
     SyncWithValidationInterfaceQueue();
+    for (const auto& chain_client : node_context.chain_clients) {
+        chain_client->schedulerMockForward(std::chrono::seconds(delta_seconds));
+    }
 
     return UniValue::VNULL;
 },

src/span.h

@@ -222,15 +222,32 @@ public:
     template <typename O> friend class Span;
 };
 
+// Return result of calling .data() method on type T. This is used to be able to
+// write template deduction guides for the single-parameter Span constructor
+// below that will work if the value that is passed has a .data() method, and if
+// the data method does not return a void pointer.
+//
+// It is important to check for the void type specifically below, so the
+// deduction guides can be used in SFINAE contexts to check whether objects can
+// be converted to spans. If the deduction guides did not explicitly check for
+// void, and an object was passed that returned void* from data (like
+// std::vector<bool>), the template deduction would succeed, but the Span<void>
+// object instantiation would fail, resulting in a hard error, rather than a
+// SFINAE error.
+// https://stackoverflow.com/questions/68759148/sfinae-to-detect-the-explicitness-of-a-ctad-deduction-guide
+// https://stackoverflow.com/questions/16568986/what-happens-when-you-call-data-on-a-stdvectorbool
+template<typename T>
+using DataResult = std::remove_pointer_t<decltype(std::declval<T&>().data())>;
+
 // Deduction guides for Span
 // For the pointer/size based and iterator based constructor:
 template <typename T, typename EndOrSize> Span(T*, EndOrSize) -> Span<T>;
 // For the array constructor:
 template <typename T, std::size_t N> Span(T (&)[N]) -> Span<T>;
 // For the temporaries/rvalue references constructor, only supporting const output.
-template <typename T> Span(T&&) -> Span<std::enable_if_t<!std::is_lvalue_reference_v<T>, const std::remove_pointer_t<decltype(std::declval<T&&>().data())>>>;
+template <typename T> Span(T&&) -> Span<std::enable_if_t<!std::is_lvalue_reference_v<T> && !std::is_void_v<DataResult<T&&>>, const DataResult<T&&>>>;
 // For (lvalue) references, supporting mutable output.
-template <typename T> Span(T&) -> Span<std::remove_pointer_t<decltype(std::declval<T&>().data())>>;
+template <typename T> Span(T&) -> Span<std::enable_if_t<!std::is_void_v<DataResult<T&>>, DataResult<T&>>>;
 
 /** Pop the last element off a span, and return a reference to that element. */
 template <typename T>

src/streams.h

@@ -182,6 +182,11 @@ public:
         memcpy(dst.data(), m_data.data(), dst.size());
         m_data = m_data.subspan(dst.size());
     }
+
+    void ignore(size_t n)
+    {
+        m_data = m_data.subspan(n);
+    }
 };
 
 /** Double ended buffer combining vector and stream-like interfaces.

src/test/span_tests.cpp

@@ -0,0 +1,73 @@
+// Copyright (c) 2023 The Bitcoin Core developers
+// Distributed under the MIT software license, see the accompanying
+// file COPYING or http://www.opensource.org/licenses/mit-license.php.
+
+#include <span.h>
+
+#include <boost/test/unit_test.hpp>
+#include <array>
+#include <set>
+#include <vector>
+
+namespace {
+struct Ignore
+{
+    template<typename T> Ignore(T&&) {}
+};
+template<typename T>
+bool Spannable(T&& value, decltype(Span{value})* enable = nullptr)
+{
+    return true;
+}
+bool Spannable(Ignore)
+{
+    return false;
+}
+
+#if defined(__clang__)
+#    pragma clang diagnostic push
+#    pragma clang diagnostic ignored "-Wunneeded-member-function"
+#    pragma clang diagnostic ignored "-Wunused-member-function"
+#endif
+struct SpannableYes
+{
+    int* data();
+    size_t size();
+};
+struct SpannableNo
+{
+    void* data();
+    size_t size();
+};
+#if defined(__clang__)
+#    pragma clang diagnostic pop
+#endif
+} // namespace
+
+BOOST_AUTO_TEST_SUITE(span_tests)
+
+// Make sure template Span template deduction guides accurately enable calls to
+// Span constructor overloads that work, and disable calls to constructor overloads that
+// don't work. This makes it is possible to use the Span constructor in a SFINAE
+// contexts like in the Spannable function above to detect whether types are or
+// aren't compatible with Spans at compile time.
+//
+// Previously there was a bug where writing a SFINAE check for vector<bool> was
+// not possible, because in libstdc++ vector<bool> has a data() memeber
+// returning void*, and the Span template guide ignored the data() return value,
+// so the template substitution would succeed, but the constructor would fail,
+// resulting in a fatal compile error, rather than a SFINAE error that could be
+// handled.
+BOOST_AUTO_TEST_CASE(span_constructor_sfinae)
+{
+    BOOST_CHECK(Spannable(std::vector<int>{}));
+    BOOST_CHECK(!Spannable(std::set<int>{}));
+    BOOST_CHECK(!Spannable(std::vector<bool>{}));
+    BOOST_CHECK(Spannable(std::array<int, 3>{}));
+    BOOST_CHECK(Spannable(Span<int>{}));
+    BOOST_CHECK(Spannable("char array"));
+    BOOST_CHECK(Spannable(SpannableYes{}));
+    BOOST_CHECK(!Spannable(SpannableNo{}));
+}
+
+BOOST_AUTO_TEST_SUITE_END()

src/wallet/context.h

@@ -13,6 +13,7 @@
 #include <vector>
 
 class ArgsManager;
+class CScheduler;
 namespace interfaces {
 class Chain;
 class Wallet;
@@ -34,6 +35,7 @@ using LoadWalletFn = std::function<void(std::unique_ptr<interfaces::Wallet> wall
 //! behavior.
 struct WalletContext {
     interfaces::Chain* chain{nullptr};
+    CScheduler* scheduler{nullptr};
     ArgsManager* args{nullptr}; // Currently a raw pointer because the memory is not managed by this struct
     // It is unsafe to lock this after locking a CWallet::cs_wallet mutex because
     // this could introduce inconsistent lock ordering and cause deadlocks.

src/wallet/feebumper.cpp

@@ -86,7 +86,7 @@ static feebumper::Result CheckFeeRate(const CWallet& wallet, const CMutableTrans
         reused_inputs.push_back(txin.prevout);
     }
 
-    std::optional<CAmount> combined_bump_fee = wallet.chain().CalculateCombinedBumpFee(reused_inputs, newFeerate);
+    std::optional<CAmount> combined_bump_fee = wallet.chain().calculateCombinedBumpFee(reused_inputs, newFeerate);
     if (!combined_bump_fee.has_value()) {
         errors.push_back(strprintf(Untranslated("Failed to calculate bump fees, because unconfirmed UTXOs depend on enormous cluster of unconfirmed transactions.")));
     }

src/wallet/interfaces.cpp

@@ -11,6 +11,7 @@
 #include <policy/fees.h>
 #include <primitives/transaction.h>
 #include <rpc/server.h>
+#include <scheduler.h>
 #include <support/allocators/secure.h>
 #include <sync.h>
 #include <uint256.h>
@@ -212,7 +213,7 @@ public:
         }
         return true;
     }
-    std::vector<WalletAddress> getAddresses() const override
+    std::vector<WalletAddress> getAddresses() override
     {
         LOCK(m_wallet->cs_wallet);
         std::vector<WalletAddress> result;
@@ -585,10 +586,15 @@ public:
     }
     bool verify() override { return VerifyWallets(m_context); }
     bool load() override { return LoadWallets(m_context); }
-    void start(CScheduler& scheduler) override { return StartWallets(m_context, scheduler); }
+    void start(CScheduler& scheduler) override
+    {
+        m_context.scheduler = &scheduler;
+        return StartWallets(m_context);
+    }
     void flush() override { return FlushWallets(m_context); }
     void stop() override { return StopWallets(m_context); }
     void setMockTime(int64_t time) override { return SetMockTime(time); }
+    void schedulerMockForward(std::chrono::seconds delta) override { Assert(m_context.scheduler)->MockForward(delta); }
 
     //! WalletLoader methods
     util::Result<std::unique_ptr<Wallet>> createWallet(const std::string& name, const SecureString& passphrase, uint64_t wallet_creation_flags, std::vector<bilingual_str>& warnings) override

src/wallet/load.cpp

@@ -141,7 +141,7 @@ bool LoadWallets(WalletContext& context)
     }
 }
 
-void StartWallets(WalletContext& context, CScheduler& scheduler)
+void StartWallets(WalletContext& context)
 {
     for (const std::shared_ptr<CWallet>& pwallet : GetWallets(context)) {
         pwallet->postInitProcess();
@@ -149,9 +149,9 @@ void StartWallets(WalletContext& context, CScheduler& scheduler)
 
     // Schedule periodic wallet flushes and tx rebroadcasts
     if (context.args->GetBoolArg("-flushwallet", DEFAULT_FLUSHWALLET)) {
-        scheduler.scheduleEvery([&context] { MaybeCompactWalletDB(context); }, std::chrono::milliseconds{500});
+        context.scheduler->scheduleEvery([&context] { MaybeCompactWalletDB(context); }, 500ms);
     }
-    scheduler.scheduleEvery([&context] { MaybeResendWalletTxs(context); }, 1min);
+    context.scheduler->scheduleEvery([&context] { MaybeResendWalletTxs(context); }, 1min);
 }
 
 void FlushWallets(WalletContext& context)

src/wallet/load.h

@@ -26,7 +26,7 @@ bool VerifyWallets(WalletContext& context);
 bool LoadWallets(WalletContext& context);
 
 //! Complete startup of wallets.
-void StartWallets(WalletContext& context, CScheduler& scheduler);
+void StartWallets(WalletContext& context);
 
 //! Flush all wallets in preparation for shutdown.
 void FlushWallets(WalletContext& context);

src/wallet/spend.cpp

@@ -259,7 +259,7 @@ util::Result<PreSelectedInputs> FetchSelectedInputs(const CWallet& wallet, const
 {
     PreSelectedInputs result;
     const bool can_grind_r = wallet.CanGrindR();
-    std::map<COutPoint, CAmount> map_of_bump_fees = wallet.chain().CalculateIndividualBumpFees(coin_control.ListSelected(), coin_selection_params.m_effective_feerate);
+    std::map<COutPoint, CAmount> map_of_bump_fees = wallet.chain().calculateIndividualBumpFees(coin_control.ListSelected(), coin_selection_params.m_effective_feerate);
     for (const COutPoint& outpoint : coin_control.ListSelected()) {
         int input_bytes = -1;
         CTxOut txout;
@@ -453,7 +453,7 @@ CoinsResult AvailableCoins(const CWallet& wallet,
     }
 
     if (feerate.has_value()) {
-        std::map<COutPoint, CAmount> map_of_bump_fees = wallet.chain().CalculateIndividualBumpFees(outpoints, feerate.value());
+        std::map<COutPoint, CAmount> map_of_bump_fees = wallet.chain().calculateIndividualBumpFees(outpoints, feerate.value());
 
         for (auto& [_, outputs] : result.coins) {
             for (auto& output : outputs) {
@@ -725,7 +725,7 @@ util::Result<SelectionResult> ChooseSelectionResult(interfaces::Chain& chain, co
             outpoints.push_back(coin->outpoint);
             summed_bump_fees += coin->ancestor_bump_fees;
         }
-        std::optional<CAmount> combined_bump_fee = chain.CalculateCombinedBumpFee(outpoints, coin_selection_params.m_effective_feerate);
+        std::optional<CAmount> combined_bump_fee = chain.calculateCombinedBumpFee(outpoints, coin_selection_params.m_effective_feerate);
         if (!combined_bump_fee.has_value()) {
             return util::Error{_("Failed to calculate bump fees, because unconfirmed UTXOs depend on enormous cluster of unconfirmed transactions.")};
         }