Merge bitcoin/bitcoin#28721: multiprocess compatibility updates

doc/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 <DASH_SOURCE_DIRECTORY>
@@ -32,12 +32,12 @@ DASHD=dash-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

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

src/interfaces/chain.h

@@ -206,6 +206,43 @@ class Chain
     //! Calculate mempool ancestor and descendant counts for the given transaction.
     virtual void getTransactionAncestry(const uint256& txid, size_t& ancestors, size_t& descendants, size_t* ancestorsize = nullptr, CAmount* ancestorfees = nullptr) = 0;
 
+    //! For each outpoint, calculate the fee-bumping cost to spend this outpoint at the specified
+    //  feerate, including bumping its ancestors. For example, if the target feerate is 10sat/vbyte
+    //  and this outpoint refers to a mempool transaction at 3sat/vbyte, the bump fee includes the
+    //  cost to bump the mempool transaction to 10sat/vbyte (i.e. 7 * mempooltx.vsize). If that
+    //  transaction also has, say, an unconfirmed parent with a feerate of 1sat/vbyte, the bump fee
+    //  includes the cost to bump the parent (i.e. 9 * parentmempooltx.vsize).
+    //
+    //  If the outpoint comes from an unconfirmed transaction that is already above the target
+    //  feerate or bumped by its descendant(s) already, it does not need to be bumped. Its bump fee
+    //  is 0. Likewise, if any of the transaction's ancestors are already bumped by a transaction
+    //  in our mempool, they are not included in the transaction's bump fee.
+    //
+    //  Also supported is bump-fee calculation in the case of replacements. If an outpoint
+    //  conflicts with another transaction in the mempool, it is assumed that the goal is to replace
+    //  that transaction. As such, the calculation will exclude the to-be-replaced transaction, but
+    //  will include the fee-bumping cost. If bump fees of descendants of the to-be-replaced
+    //  transaction are requested, the value will be 0. Fee-related RBF rules are not included as
+    //  they are logically distinct.
+    //
+    //  Any outpoints that are otherwise unavailable from the mempool (e.g. UTXOs from confirmed
+    //  transactions or transactions not yet broadcast by the wallet) are given a bump fee of 0.
+    //
+    //  If multiple outpoints come from the same transaction (which would be very rare because
+    //  it means that one transaction has multiple change outputs or paid the same wallet using multiple
+    //  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;
+
+    //! Calculate the combined bump fee for an input set per the same strategy
+    //  as in CalculateIndividualBumpFees(…).
+    //  Unlike CalculateIndividualBumpFees(…), this does not return individual
+    //  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;
+
     //! Get the node's package limits.
     //! Currently only returns the ancestor and descendant count limits, but could be enhanced to
     //! return more policy settings.
@@ -342,6 +379,9 @@ class ChainClient
 
     //! 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

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

src/interfaces/wallet.h

@@ -143,7 +143,7 @@ class Wallet
         std::string* 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

@@ -504,10 +504,12 @@ class NodeImpl : public Node
     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, bilingual_str& err_string) override
     {
@@ -916,6 +918,24 @@ class ChainImpl : public Chain
         if (!m_node.mempool) return;
         m_node.mempool->GetTransactionAncestry(txid, ancestors, descendants, ancestorsize, ancestorfees);
     }
+
+    std::map<COutPoint, CAmount> calculateIndividualBumpFees(const std::vector<COutPoint>& outpoints, const CFeeRate& target_feerate) override
+    {
+        // TODO: Dash doesn't have MiniMiner yet. Return 0 fees for all outpoints for now.
+        // This is needed for multiprocess compatibility but not actively used in Dash.
+        std::map<COutPoint, CAmount> bump_fees;
+        for (const auto& outpoint : outpoints) {
+            bump_fees.emplace(outpoint, 0);
+        }
+        return bump_fees;
+    }
+
+    std::optional<CAmount> calculateCombinedBumpFee(const std::vector<COutPoint>& outpoints, const CFeeRate& target_feerate) override
+    {
+        // TODO: Dash doesn't have MiniMiner yet. Return 0 for now.
+        // This is needed for multiprocess compatibility but not actively used in Dash.
+        return 0;
+    }
     void getPackageLimits(unsigned int& limit_ancestor_count, unsigned int& limit_descendant_count) override
     {
         limit_ancestor_count = gArgs.GetIntArg("-limitancestorcount", DEFAULT_ANCESTOR_LIMIT);

src/qt/transactiondesc.cpp

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

src/rpc/node.cpp

@@ -844,6 +844,10 @@ static RPCHelpMan mockscheduler()
     // protect against null pointer dereference
     CHECK_NONFATAL(node_context->scheduler);
     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 NullUniValue;
 },

src/span.h

@@ -222,15 +222,32 @@ class Span
     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

@@ -179,6 +179,11 @@ class SpanReader
         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/util/system.cpp

@@ -982,6 +982,15 @@ fs::path ArgsManager::GetConfigFilePath() const
     return GetConfigFile(GetPathArg("-conf", BITCOIN_CONF_FILENAME));
 }
 
+void ArgsManager::SetConfigFilePath(fs::path path)
+{
+    LOCK(cs_args);
+    // Note: Dash doesn't have m_config_path member, but this method is needed
+    // for multiprocess compatibility. The actual config file path is handled
+    // through GetConfigFile() with the -conf argument.
+    // TODO: Implement proper config path storage if multiprocess support is added
+}
+
 bool ArgsManager::ReadConfigFiles(std::string& error, bool ignore_invalid_keys)
 {
     {

src/util/system.h

@@ -262,6 +262,7 @@ class ArgsManager
      * 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/wallet/context.h

@@ -13,6 +13,7 @@
 #include <vector>
 
 class ArgsManager;
+class CScheduler;
 namespace interfaces {
 class Chain;
 namespace CoinJoin {
@@ -41,6 +42,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/interfaces.cpp

@@ -12,6 +12,7 @@
 #include <policy/fees.h>
 #include <primitives/transaction.h>
 #include <rpc/server.h>
+#include <scheduler.h>
 #include <script/standard.h>
 #include <support/allocators/secure.h>
 #include <sync.h>
@@ -242,7 +243,7 @@ class WalletImpl : public Wallet
         }
         return true;
     }
-    std::vector<WalletAddress> getAddresses() const override
+    std::vector<WalletAddress> getAddresses() override
     {
         LOCK(m_wallet->cs_wallet);
         std::vector<WalletAddress> result;
@@ -601,10 +602,15 @@ class WalletLoaderImpl : public WalletLoader
     }
     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
     void registerOtherRpcs(const Span<const CRPCCommand>& commands) override

src/wallet/load.cpp

@@ -144,17 +144,17 @@ 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();
     }
 
     // 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)