Merge bitcoin/bitcoin#25648: refactor: Remove all policy globals

ddddd69 sort after scripted-diff (MacroFake)
fac812c scripted-diff: Move mempool_args to src/node (MacroFake)
6666438 Remove ::g_max_datacarrier_bytes global (MacroFake)
fad0b4f Pass datacarrier setting into IsStandard (MacroFake)
fa2a6b8 Combine datacarrier globals into one (MacroFake)
fa477d3 Remove ::GetVirtualTransactionSize() alias (MacroFake)
fa2f6c1 Remove ::fIsBareMultisigStd global (MacroFake)
fadc14e Remove ::dustRelayFee (MacroFake)
fa8a7f0 Remove ::IsStandardTx(tx, reason) alias (MacroFake)
fa7a911 test: Remove unused cs_main (MacroFake)
fa9cba7 Remove ::incrementalRelayFee and ::minRelayTxFee globals (MacroFake)
fa14860 Remove ::fRequireStandard global (MacroFake)
fa468bd Return optional error from ApplyArgsManOptions (MacroFake)

Pull request description:

  This change is good because:

  * It moves module-specific init-logic out of the bloated init.cpp
  * It removes a global from validation.cpp and places it into the data structure that needs it (mempool)

ACKs for top commit:
  glozow:
    re ACK ddddd69
  ryanofsky:
    Code review ACK ddddd69
  ariard:
    Light Code Review ACK ddddd69

Tree-SHA512: 9de2ce601cfcaa4dfd7d1c92270568895ce8702ccdffb59829fbe9618eab0fd88d738afef33ed66988c66861115e0340e881056bfb71e2aed4af2440bd37eb1e

Author: glozow

ci/test/06_script_b.sh

@@ -45,6 +45,7 @@ if [ "${RUN_TIDY}" = "true" ]; then
           " src/init"\
           " src/kernel"\
           " src/node/chainstate.cpp"\
+          " src/node/mempool_args.cpp"\
           " src/policy/feerate.cpp"\
           " src/policy/packages.cpp"\
           " src/policy/settings.cpp"\

doc/policy/README.md

@@ -3,7 +3,7 @@
 **Policy** (Mempool or Transaction Relay Policy) is the node's set of validation rules, in addition
 to consensus, enforced for unconfirmed transactions before submitting them to the mempool. These
 rules are local to the node and configurable (e.g. `-minrelaytxfee`, `-limitancestorsize`,
-`-incrementalRelayFee`). Policy may include restrictions on the transaction itself, the transaction
+`-incrementalrelayfee`). Policy may include restrictions on the transaction itself, the transaction
 in relation to the current chain tip, and the transaction in relation to the node's mempool
 contents. Policy is *not* applied to transactions in blocks.
 

doc/policy/mempool-replacements.md

@@ -71,7 +71,7 @@ This set of rules is similar but distinct from BIP125.
   Bitcoin Core implementation.
 
 * The incremental relay feerate used to calculate the required additional fees is distinct from
-  `minRelayTxFee` and configurable using `-incrementalrelayfee`
+  `-minrelaytxfee` and configurable using `-incrementalrelayfee`
   ([PR #9380](https://github.com/bitcoin/bitcoin/pull/9380)).
 
 * RBF enabled by default in the wallet GUI as of **v0.18.1** ([PR

doc/policy/packages.md

@@ -81,7 +81,7 @@ If any transactions in the package are already in the mempool, they are not subm
 ("deduplicated") and are thus excluded from this calculation.
 
 To meet the two feerate requirements of a mempool, i.e., the pre-configured minimum relay feerate
-(`minRelayTxFee`) and the dynamic mempool minimum feerate, the total package feerate is used instead
+(`-minrelaytxfee`) and the dynamic mempool minimum feerate, the total package feerate is used instead
 of the individual feerate. The individual transactions are allowed to be below the feerate
 requirements if the package meets the feerate requirements. For example, the parent(s) in the
 package can pay no fees but be paid for by the child.

src/Makefile.am

@@ -139,7 +139,6 @@ BITCOIN_CORE_H = \
   compat/cpuid.h \
   compat/endian.h \
   compressor.h \
-  node/connection_types.h \
   consensus/consensus.h \
   consensus/tx_check.h \
   consensus/tx_verify.h \
@@ -149,7 +148,6 @@ BITCOIN_CORE_H = \
   dbwrapper.h \
   deploymentinfo.h \
   deploymentstatus.h \
-  node/eviction.h \
   external_signer.h \
   flatfile.h \
   fs.h \
@@ -184,7 +182,6 @@ BITCOIN_CORE_H = \
   logging.h \
   logging/timer.h \
   mapport.h \
-  mempool_args.h \
   memusage.h \
   merkleblock.h \
   net.h \
@@ -199,13 +196,16 @@ BITCOIN_CORE_H = \
   node/caches.h \
   node/chainstate.h \
   node/coin.h \
+  node/connection_types.h \
   node/context.h \
+  node/eviction.h \
+  node/interface_ui.h \
+  node/mempool_args.h \
   node/mempool_persist_args.h \
   node/miner.h \
   node/minisketchwrapper.h \
   node/psbt.h \
   node/transaction.h \
-  node/interface_ui.h \
   node/utxo_snapshot.h \
   noui.h \
   outputtype.h \
@@ -372,24 +372,24 @@ libbitcoin_node_a_SOURCES = \
   kernel/context.cpp \
   kernel/mempool_persist.cpp \
   mapport.cpp \
-  mempool_args.cpp \
   net.cpp \
-  netgroup.cpp \
   net_processing.cpp \
+  netgroup.cpp \
   node/blockstorage.cpp \
   node/caches.cpp \
   node/chainstate.cpp \
   node/coin.cpp \
   node/connection_types.cpp \
   node/context.cpp \
   node/eviction.cpp \
+  node/interface_ui.cpp \
   node/interfaces.cpp \
+  node/mempool_args.cpp \
   node/mempool_persist_args.cpp \
   node/miner.cpp \
   node/minisketchwrapper.cpp \
   node/psbt.cpp \
   node/transaction.cpp \
-  node/interface_ui.cpp \
   noui.cpp \
   policy/fees.cpp \
   policy/fees_args.cpp \
@@ -402,8 +402,8 @@ libbitcoin_node_a_SOURCES = \
   rpc/fees.cpp \
   rpc/mempool.cpp \
   rpc/mining.cpp \
-  rpc/node.cpp \
   rpc/net.cpp \
+  rpc/node.cpp \
   rpc/output_script.cpp \
   rpc/rawtransaction.cpp \
   rpc/server.cpp \

src/init.cpp

@@ -31,7 +31,6 @@
 #include <interfaces/init.h>
 #include <interfaces/node.h>
 #include <mapport.h>
-#include <mempool_args.h>
 #include <net.h>
 #include <net_permissions.h>
 #include <net_processing.h>
@@ -42,6 +41,7 @@
 #include <node/chainstate.h>
 #include <node/context.h>
 #include <node/interface_ui.h>
+#include <node/mempool_args.h>
 #include <node/mempool_persist_args.h>
 #include <node/miner.h>
 #include <policy/feerate.h>
@@ -935,16 +935,6 @@ bool AppInitParameterInteraction(const ArgsManager& args, bool use_syscall_sandb
         LogPrintf("Warning: nMinimumChainWork set below default value of %s\n", chainparams.GetConsensus().nMinimumChainWork.GetHex());
     }
 
-    // incremental relay fee sets the minimum feerate increase necessary for BIP 125 replacement in the mempool
-    // and the amount the mempool min fee increases above the feerate of txs evicted due to mempool limiting.
-    if (args.IsArgSet("-incrementalrelayfee")) {
-        if (std::optional<CAmount> inc_relay_fee = ParseMoney(args.GetArg("-incrementalrelayfee", ""))) {
-            ::incrementalRelayFee = CFeeRate{inc_relay_fee.value()};
-        } else {
-            return InitError(AmountErrMsg("incrementalrelayfee", args.GetArg("-incrementalrelayfee", "")));
-        }
-    }
-
     // block pruning; get the amount of disk space (in MiB) to allot for block & undo files
     int64_t nPruneArg = args.GetIntArg("-prune", 0);
     if (nPruneArg < 0) {
@@ -973,19 +963,6 @@ bool AppInitParameterInteraction(const ArgsManager& args, bool use_syscall_sandb
         return InitError(Untranslated("peertimeout must be a positive integer."));
     }
 
-    if (args.IsArgSet("-minrelaytxfee")) {
-        if (std::optional<CAmount> min_relay_fee = ParseMoney(args.GetArg("-minrelaytxfee", ""))) {
-            // High fee check is done afterward in CWallet::Create()
-            ::minRelayTxFee = CFeeRate{min_relay_fee.value()};
-        } else {
-            return InitError(AmountErrMsg("minrelaytxfee", args.GetArg("-minrelaytxfee", "")));
-        }
-    } else if (incrementalRelayFee > ::minRelayTxFee) {
-        // Allow only setting incrementalRelayFee to control both
-        ::minRelayTxFee = incrementalRelayFee;
-        LogPrintf("Increasing minrelaytxfee to %s to match incrementalrelayfee\n",::minRelayTxFee.ToString());
-    }
-
     // Sanity check argument for min fee for including tx in block
     // TODO: Harmonize which arguments need sanity checking and where that happens
     if (args.IsArgSet("-blockmintxfee")) {
@@ -994,28 +971,10 @@ bool AppInitParameterInteraction(const ArgsManager& args, bool use_syscall_sandb
         }
     }
 
-    // Feerate used to define dust.  Shouldn't be changed lightly as old
-    // implementations may inadvertently create non-standard transactions
-    if (args.IsArgSet("-dustrelayfee")) {
-        if (std::optional<CAmount> parsed = ParseMoney(args.GetArg("-dustrelayfee", ""))) {
-            dustRelayFee = CFeeRate{parsed.value()};
-        } else {
-            return InitError(AmountErrMsg("dustrelayfee", args.GetArg("-dustrelayfee", "")));
-        }
-    }
-
-    fRequireStandard = !args.GetBoolArg("-acceptnonstdtxn", !chainparams.RequireStandard());
-    if (!chainparams.IsTestChain() && !fRequireStandard) {
-        return InitError(strprintf(Untranslated("acceptnonstdtxn is not currently supported for %s chain"), chainparams.NetworkIDString()));
-    }
     nBytesPerSigOp = args.GetIntArg("-bytespersigop", nBytesPerSigOp);
 
     if (!g_wallet_init_interface.ParameterInteraction()) return false;
 
-    fIsBareMultisigStd = args.GetBoolArg("-permitbaremultisig", DEFAULT_PERMIT_BAREMULTISIG);
-    fAcceptDatacarrier = args.GetBoolArg("-datacarrier", DEFAULT_ACCEPT_DATACARRIER);
-    nMaxDatacarrierBytes = args.GetIntArg("-datacarriersize", nMaxDatacarrierBytes);
-
     // Option to startup with mocktime set (used for regression testing):
     SetMockTime(args.GetIntArg("-mocktime", 0)); // SetMockTime(0) is a no-op
 
@@ -1418,7 +1377,9 @@ bool AppInitMain(NodeContext& node, interfaces::BlockAndHeaderTipInfo* tip_info)
         .estimator = node.fee_estimator.get(),
         .check_ratio = chainparams.DefaultConsistencyChecks() ? 1 : 0,
     };
-    ApplyArgsManOptions(args, mempool_opts);
+    if (const auto err{ApplyArgsManOptions(args, chainparams, mempool_opts)}) {
+        return InitError(*err);
+    }
     mempool_opts.check_ratio = std::clamp<int>(mempool_opts.check_ratio, 0, 1'000'000);
 
     int64_t descendant_limit_bytes = mempool_opts.limits.descendant_size_vbytes * 40;

src/kernel/mempool_options.h

@@ -6,8 +6,13 @@
 
 #include <kernel/mempool_limits.h>
 
+#include <policy/feerate.h>
+#include <policy/policy.h>
+#include <script/standard.h>
+
 #include <chrono>
 #include <cstdint>
+#include <optional>
 
 class CBlockPolicyEstimator;
 
@@ -33,6 +38,20 @@ struct MemPoolOptions {
     int check_ratio{0};
     int64_t max_size_bytes{DEFAULT_MAX_MEMPOOL_SIZE_MB * 1'000'000};
     std::chrono::seconds expiry{std::chrono::hours{DEFAULT_MEMPOOL_EXPIRY_HOURS}};
+    CFeeRate incremental_relay_feerate{DEFAULT_INCREMENTAL_RELAY_FEE};
+    /** A fee rate smaller than this is considered zero fee (for relaying, mining and transaction creation) */
+    CFeeRate min_relay_feerate{DEFAULT_MIN_RELAY_TX_FEE};
+    CFeeRate dust_relay_feerate{DUST_RELAY_TX_FEE};
+    /**
+     * A data carrying output is an unspendable output containing data. The script
+     * type is designated as TxoutType::NULL_DATA.
+     *
+     * Maximum size of TxoutType::NULL_DATA scripts that this node considers standard.
+     * If nullopt, any size is nonstandard.
+     */
+    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};
     MemPoolLimits limits{};
 };

src/mempool_args.cpp

@@ -4759,8 +4759,8 @@ void PeerManagerImpl::MaybeSendFeefilter(CNode& pto, Peer& peer, std::chrono::mi
     }
     if (current_time > peer.m_next_send_feefilter) {
         CAmount filterToSend = g_filter_rounder.round(currentFilter);
-        // We always have a fee filter of at least minRelayTxFee
-        filterToSend = std::max(filterToSend, ::minRelayTxFee.GetFeePerK());
+        // We always have a fee filter of at least the min relay fee
+        filterToSend = std::max(filterToSend, m_mempool.m_min_relay_feerate.GetFeePerK());
         if (filterToSend != peer.m_fee_filter_sent) {
             m_connman.PushMessage(&pto, CNetMsgMaker(pto.GetCommonVersion()).Make(NetMsgType::FEEFILTER, filterToSend));
             peer.m_fee_filter_sent = filterToSend;

src/net_processing.cpp

@@ -301,7 +301,11 @@ class NodeImpl : public Node
         }
     }
     bool getNetworkActive() override { return m_context->connman && m_context->connman->GetNetworkActive(); }
-    CFeeRate getDustRelayFee() override { return ::dustRelayFee; }
+    CFeeRate getDustRelayFee() override
+    {
+        if (!m_context->mempool) return CFeeRate{DUST_RELAY_TX_FEE};
+        return m_context->mempool->m_dust_relay_feerate;
+    }
     UniValue executeRpc(const std::string& command, const UniValue& params, const std::string& uri) override
     {
         JSONRPCRequest req;
@@ -676,9 +680,21 @@ class ChainImpl : public Chain
         if (!m_node.mempool) return {};
         return m_node.mempool->GetMinFee();
     }
-    CFeeRate relayMinFee() override { return ::minRelayTxFee; }
-    CFeeRate relayIncrementalFee() override { return ::incrementalRelayFee; }
-    CFeeRate relayDustFee() override { return ::dustRelayFee; }
+    CFeeRate relayMinFee() override
+    {
+        if (!m_node.mempool) return CFeeRate{DEFAULT_MIN_RELAY_TX_FEE};
+        return m_node.mempool->m_min_relay_feerate;
+    }
+    CFeeRate relayIncrementalFee() override
+    {
+        if (!m_node.mempool) return CFeeRate{DEFAULT_INCREMENTAL_RELAY_FEE};
+        return m_node.mempool->m_incremental_relay_feerate;
+    }
+    CFeeRate relayDustFee() override
+    {
+        if (!m_node.mempool) return CFeeRate{DUST_RELAY_TX_FEE};
+        return m_node.mempool->m_dust_relay_feerate;
+    }
     bool havePruned() override
     {
         LOCK(::cs_main);