Rework mempoolreplacement option handling

Author: luke-jr

src/init.cpp

@@ -648,8 +648,8 @@ void SetupServerArgs(ArgsManager& argsman)
                              "is of this size or less (default: %u)",
                              MAX_OP_RETURN_RELAY),
                    ArgsManager::ALLOW_ANY, OptionsCategory::NODE_RELAY);
-    argsman.AddArg("-mempoolfullrbf", strprintf("Accept transaction replace-by-fee without requiring replaceability signaling (default: %u)", DEFAULT_MEMPOOL_FULL_RBF), ArgsManager::ALLOW_ANY, OptionsCategory::NODE_RELAY);
-    argsman.AddArg("-mempoolreplacement", strprintf("Enable transaction replacement in the memory pool (\"fee,-optin\" = ignore opt-out flag, default: %u)", DEFAULT_ENABLE_REPLACEMENT), ArgsManager::ALLOW_ANY, OptionsCategory::NODE_RELAY);
+    argsman.AddArg("-mempoolfullrbf", strprintf("Accept transaction replace-by-fee without requiring replaceability signaling (default: %u)", (DEFAULT_MEMPOOL_RBF_POLICY == RBFPolicy::Always)), ArgsManager::ALLOW_ANY, OptionsCategory::NODE_RELAY);
+    argsman.AddArg("-mempoolreplacement", strprintf("Enable transaction replacement in the memory pool (\"fee,-optin\" = ignore opt-out flag, default: %u)", "fee,-optin"), ArgsManager::ALLOW_ANY, OptionsCategory::NODE_RELAY);
     argsman.AddArg("-permitbaremultisig", strprintf("Relay transactions creating non-P2SH multisig outputs (default: %u)", DEFAULT_PERMIT_BAREMULTISIG), ArgsManager::ALLOW_ANY,
                    OptionsCategory::NODE_RELAY);
     argsman.AddArg("-minrelaytxfee=<amt>", strprintf("Fees (in %s/kvB) smaller than this are considered zero fee for relaying, mining and transaction creation (default: %s)",
@@ -894,8 +894,6 @@ bool AppInitBasicSetup(const ArgsManager& args, std::atomic<int>& exit_status)
     return true;
 }
 
-static bool gReplacementHonourOptOut;  // FIXME: Get rid of this
-
 bool AppInitParameterInteraction(const ArgsManager& args)
 {
     const CChainParams& chainparams = Params();
@@ -1061,26 +1059,6 @@ bool AppInitParameterInteraction(const ArgsManager& args)
         }
     }
 
-    gEnableReplacement = args.GetBoolArg("-mempoolreplacement", DEFAULT_ENABLE_REPLACEMENT);
-    gReplacementHonourOptOut = !args.GetBoolArg("-mempoolfullrbf", DEFAULT_MEMPOOL_FULL_RBF);
-    if ((!gEnableReplacement) && args.IsArgSet("-mempoolreplacement")) {
-        // Minimal effort at forwards compatibility
-        std::string replacement_opt = args.GetArg("-mempoolreplacement", "");  // default is impossible
-        std::vector<std::string> replacement_modes = util::SplitString(replacement_opt, ",+");
-        gEnableReplacement = (std::find(replacement_modes.begin(), replacement_modes.end(), "fee") != replacement_modes.end());
-        if (gEnableReplacement) {
-            for (auto& opt : replacement_modes) {
-                if (opt == "optin") {
-                    gReplacementHonourOptOut = true;
-                } else if (opt == "-optin") {
-                    gReplacementHonourOptOut = false;
-                }
-            }
-        } else {
-            gReplacementHonourOptOut = true;
-        }
-    }
-
     // Also report errors from parsing before daemonization
     {
         kernel::Notifications notifications{};
@@ -1554,7 +1532,6 @@ bool AppInitMain(NodeContext& node, interfaces::BlockAndHeaderTipInfo* tip_info)
 
     CTxMemPool::Options mempool_opts{
         .check_ratio = chainparams.DefaultConsistencyChecks() ? 1 : 0,
-        .full_rbf = !gReplacementHonourOptOut,
         .signals = &validation_signals,
     };
     auto result{ApplyArgsManOptions(args, chainparams, mempool_opts)};
@@ -1572,7 +1549,7 @@ bool AppInitMain(NodeContext& node, interfaces::BlockAndHeaderTipInfo* tip_info)
         LogPrintf("* Flushing caches if available system memory drops below %s MiB\n", g_low_memory_threshold / 1024 / 1024);
     }
 
-    if (mempool_opts.full_rbf) {
+    if (mempool_opts.rbf_policy == RBFPolicy::Always) {
         nLocalServices = ServiceFlags(nLocalServices | NODE_REPLACE_BY_FEE);
     }
 

src/kernel/mempool_options.h

@@ -15,14 +15,16 @@
 
 class ValidationSignals;
 
+enum class RBFPolicy { Never, OptIn, Always };
+
 /** Default for -maxmempool, maximum megabytes of mempool memory usage */
 static constexpr unsigned int DEFAULT_MAX_MEMPOOL_SIZE_MB{300};
 /** Default for -maxmempool when blocksonly is set */
 static constexpr unsigned int DEFAULT_BLOCKSONLY_MAX_MEMPOOL_SIZE_MB{5};
 /** Default for -mempoolexpiry, expiration time for mempool transactions in hours */
 static constexpr unsigned int DEFAULT_MEMPOOL_EXPIRY_HOURS{336};
-/** Default for -mempoolfullrbf, if the transaction replaceability signaling is ignored */
-static constexpr bool DEFAULT_MEMPOOL_FULL_RBF{true};
+/** Default for -mempoolreplacement; must update docs in init.cpp manually */
+static constexpr RBFPolicy DEFAULT_MEMPOOL_RBF_POLICY{RBFPolicy::Always};
 /** Whether to fall back to legacy V1 serialization when writing mempool.dat */
 static constexpr bool DEFAULT_PERSIST_V1_DAT{false};
 /** Default for -acceptnonstdtxn */
@@ -55,7 +57,7 @@ struct MemPoolOptions {
     std::optional<unsigned> max_datacarrier_bytes{DEFAULT_ACCEPT_DATACARRIER ? std::optional{MAX_OP_RETURN_RELAY} : std::nullopt};
     bool permit_bare_multisig{DEFAULT_PERMIT_BAREMULTISIG};
     bool require_standard{true};
-    bool full_rbf{DEFAULT_MEMPOOL_FULL_RBF};
+    RBFPolicy rbf_policy{DEFAULT_MEMPOOL_RBF_POLICY};
     bool persist_v1_dat{DEFAULT_PERSIST_V1_DAT};
     MemPoolLimits limits{};
 

src/node/mempool_args.cpp

@@ -12,10 +12,12 @@
 #include <consensus/amount.h>
 #include <kernel/chainparams.h>
 #include <logging.h>
+#include <node/interface_ui.h>
 #include <policy/feerate.h>
 #include <policy/policy.h>
 #include <tinyformat.h>
 #include <util/moneystr.h>
+#include <util/string.h>
 #include <util/translation.h>
 
 #include <chrono>
@@ -92,7 +94,59 @@ util::Result<void> ApplyArgsManOptions(const ArgsManager& argsman, const CChainP
         return util::Error{strprintf(Untranslated("acceptnonstdtxn is not currently supported for %s chain"), chainparams.GetChainTypeString())};
     }
 
-    mempool_opts.full_rbf = argsman.GetBoolArg("-mempoolfullrbf", mempool_opts.full_rbf);
+    if (argsman.IsArgSet("-mempoolreplacement") || argsman.IsArgSet("-mempoolfullrbf")) {
+        // Generally, mempoolreplacement overrides mempoolfullrbf, but the latter is used to infer intent in some cases
+        std::optional<bool> optin_flag;
+        bool fee_flag{false};
+        if (argsman.GetBoolArg("-mempoolreplacement", false)) {
+            fee_flag = true;
+        } else {
+            for (auto& opt : util::SplitString(argsman.GetArg("-mempoolreplacement", ""), ",+")) {
+                if (opt == "optin") {
+                    optin_flag = true;
+                } else if (opt == "-optin") {
+                    optin_flag = false;
+                } else if (opt == "fee") {
+                    fee_flag = true;
+                }
+            }
+        }
+        if (optin_flag.value_or(false)) {
+            // "optin" is explicitly specified
+            mempool_opts.rbf_policy = RBFPolicy::OptIn;
+        } else if (argsman.GetBoolArg("-mempoolfullrbf", false)) {
+            const bool mempoolreplacement_false{argsman.IsArgSet("-mempoolreplacement") && !(fee_flag || optin_flag.has_value())};
+            if (mempoolreplacement_false) {
+                // This is a contradiction, but override rather than error
+                InitWarning(_("False mempoolreplacement option contradicts true mempoolfullrbf; disallowing all RBF"));
+                mempool_opts.rbf_policy = RBFPolicy::Never;
+            } else {
+                mempool_opts.rbf_policy = RBFPolicy::Always;
+            }
+        } else if (!optin_flag.value_or(true)) {
+            // "-optin" is explicitly specified
+            mempool_opts.rbf_policy = fee_flag ? RBFPolicy::Always : RBFPolicy::Never;
+        } else if (fee_flag) {
+            // Just "fee" by itself
+            if (!argsman.GetBoolArg("-mempoolfullrbf", true)) {
+                mempool_opts.rbf_policy = RBFPolicy::OptIn;
+            } else {
+                // Fallback to default, unless it's been changed to Never
+                if (mempool_opts.rbf_policy == RBFPolicy::Never) {
+                    mempool_opts.rbf_policy = RBFPolicy::Always;
+                }
+            }
+        } else if (!argsman.IsArgSet("-mempoolreplacement")) {
+            // mempoolfullrbf is always explicitly false here
+            // Fallback to default, as long as it isn't Always
+            if (mempool_opts.rbf_policy == RBFPolicy::Always) {
+                mempool_opts.rbf_policy = RBFPolicy::OptIn;
+            }
+        } else {
+            // mempoolreplacement is explicitly false here
+            mempool_opts.rbf_policy = RBFPolicy::Never;
+        }
+    }
 
     mempool_opts.persist_v1_dat = argsman.GetBoolArg("-persistmempoolv1", mempool_opts.persist_v1_dat);
 

src/policy/policy.h

@@ -36,8 +36,6 @@ static constexpr unsigned int MAX_STANDARD_TX_SIGOPS_COST{MAX_BLOCK_SIGOPS_COST/
 static constexpr unsigned int DEFAULT_INCREMENTAL_RELAY_FEE{1000};
 /** Default for -bytespersigop */
 static constexpr unsigned int DEFAULT_BYTES_PER_SIGOP{20};
-/** Default for -mempoolreplacement */
-static constexpr bool DEFAULT_ENABLE_REPLACEMENT{true};
 /** Default for -permitbaremultisig */
 static constexpr bool DEFAULT_PERMIT_BAREMULTISIG{true};
 /** The maximum number of witness stack items in a standard P2WSH script */

src/policy/settings.cpp

@@ -7,5 +7,4 @@
 
 #include <policy/policy.h>
 
-bool gEnableReplacement = DEFAULT_ENABLE_REPLACEMENT;
 unsigned int nBytesPerSigOp = DEFAULT_BYTES_PER_SIGOP;

src/policy/settings.h

@@ -6,7 +6,6 @@
 #ifndef BITCOIN_POLICY_SETTINGS_H
 #define BITCOIN_POLICY_SETTINGS_H
 
-extern bool gEnableReplacement;
 extern unsigned int nBytesPerSigOp;
 
 #endif // BITCOIN_POLICY_SETTINGS_H

src/rpc/mempool.cpp

@@ -726,7 +726,7 @@ UniValue MempoolInfoToJSON(const CTxMemPool& pool, const std::optional<MempoolHi
     ret.pushKV("minrelaytxfee", ValueFromAmount(pool.m_opts.min_relay_feerate.GetFeePerK()));
     ret.pushKV("incrementalrelayfee", ValueFromAmount(pool.m_opts.incremental_relay_feerate.GetFeePerK()));
     ret.pushKV("unbroadcastcount", uint64_t{pool.GetUnbroadcastTxs().size()});
-    ret.pushKV("fullrbf", pool.m_opts.full_rbf);
+    ret.pushKV("fullrbf", (pool.m_opts.rbf_policy == RBFPolicy::Always));
 
     if (histogram_floors) {
         const MempoolHistogramFeeRates& floors{histogram_floors.value()};

src/validation.cpp

@@ -859,9 +859,9 @@ bool MemPoolAccept::PreChecks(ATMPArgs& args, Workspace& ws)
                 // Replaceability signaling of the original transactions may be
                 // ignored due to node setting.
                 bool allow_rbf;
-                if (m_pool.m_opts.full_rbf || ignore_rejects.count("txn-mempool-conflict")) {
+                if (m_pool.m_opts.rbf_policy == RBFPolicy::Always || ignore_rejects.count("txn-mempool-conflict")) {
                     allow_rbf = true;
-                } else if (!gEnableReplacement) {
+                } else if (m_pool.m_opts.rbf_policy == RBFPolicy::Never) {
                     allow_rbf = false;
                 } else {
                     allow_rbf = SignalsOptInRBF(*ptxConflicting) || ptxConflicting->version == TRUC_VERSION;