From 30270430d44c76dfe6266eb6b9cc8d40edb3d8e3 Mon Sep 17 00:00:00 2001 From: MacroFake Date: Mon, 1 Aug 2022 11:18:26 +0200 Subject: [PATCH 01/10] Merge bitcoin/bitcoin#25651: refactor: make all NodeImpl/ChainImpl/ExternalSignerImpl members public, rm temporaries, simplify 4bedfd702ad878645c51bea6ee8ce40d8c0bd3da refactor: remove unneeded temporaries in node/interfaces, simplify code (Jon Atack) b27ba169ebd4a8e4ec29be590f03a4d0da61a0cc refactor: make all NodeImpl/ChainImpl/ExternalSignerImpl members public (Jon Atack) Pull request description: - Make all `NodeImpl`, `ChainImpl` and `ExternalSignerImpl` class members `public` (and document why), to be consistent in all the `*Impl` classes in `src/node/interfaces.cpp` and `src/wallet/interfaces.cpp` and to help future reviewers and contributors. - Remove unneeded temporaries in `NodeImpl` and `ChainImpl` methods in `src/node/interfaces.cpp` and simplify, to make the code easier to read and understand and to improve performance by avoiding unnecessary move operations. ACKs for top commit: ryanofsky: Code review ACK 4bedfd702ad878645c51bea6ee8ce40d8c0bd3da. Changes since last review, applying suggested style & simplifiying first commit. Also avoiding another lock in second commit. Tree-SHA512: 112f7cad5e2838c94c5b79d61328f42fe75fdb97f401ab49eccf696fc2c6a8a0c0ee55ec974c0602acf7423f78bb82e90eb8a0cc531e1d3347f73b7c83685504 --- src/node/interfaces.cpp | 48 +++++++++++---------------------------- src/wallet/interfaces.cpp | 2 ++ 2 files changed, 15 insertions(+), 35 deletions(-) diff --git a/src/node/interfaces.cpp b/src/node/interfaces.cpp index c89bc48ec61b..d6a57df15695 100644 --- a/src/node/interfaces.cpp +++ b/src/node/interfaces.cpp @@ -101,6 +101,8 @@ using interfaces::Node; using interfaces::WalletLoader; namespace node { +// All members of the classes in this namespace are intentionally public, as the +// classes themselves are private. namespace { std::vector GetOwnerPayoutScripts(const CDeterministicMNState& state) { @@ -681,15 +683,12 @@ class ExternalSignerImpl : public interfaces::ExternalSigner public: ExternalSignerImpl(::ExternalSigner signer) : m_signer(std::move(signer)) {} std::string getName() override { return m_signer.m_name; } -private: ::ExternalSigner m_signer; }; #endif class NodeImpl : public Node { -private: - ChainstateManager& chainman() { return *Assert(m_context->chainman); } public: EVOImpl m_evo; GOVImpl m_gov; @@ -929,12 +928,7 @@ class NodeImpl : public Node } double getVerificationProgress() override { - const CBlockIndex* tip; - { - LOCK(::cs_main); - tip = chainman().ActiveChain().Tip(); - } - return GuessVerificationProgress(chainman().GetParams().TxData(), tip); + return GuessVerificationProgress(chainman().GetParams().TxData(), WITH_LOCK(::cs_main, return chainman().ActiveChain().Tip())); } bool isInitialBlockDownload() override { return chainman().ActiveChainstate().IsInitialBlockDownload(); @@ -1073,6 +1067,7 @@ class NodeImpl : public Node m_llmq.setContext(context); m_masternodeSync.setContext(context); } + ChainstateManager& chainman() { return *Assert(m_context->chainman); } NodeContext* m_context{nullptr}; }; @@ -1193,33 +1188,22 @@ class RpcHandlerImpl : public Handler class ChainImpl : public Chain { -private: - ChainstateManager& chainman() { return *Assert(m_node.chainman); } public: explicit ChainImpl(NodeContext& node) : m_node(node) {} std::optional getHeight() override { - LOCK(::cs_main); - const CChain& active = chainman().ActiveChain(); - int height = active.Height(); - if (height >= 0) { - return height; - } - return std::nullopt; + const int height{WITH_LOCK(::cs_main, return chainman().ActiveChain().Height())}; + return height >= 0 ? std::optional{height} : std::nullopt; } uint256 getBlockHash(int height) override { LOCK(::cs_main); - const CChain& active = chainman().ActiveChain(); - CBlockIndex* block = active[height]; - assert(block != nullptr); - return block->GetBlockHash(); + return Assert(chainman().ActiveChain()[height])->GetBlockHash(); } bool haveBlockOnDisk(int height) override { LOCK(::cs_main); - const CChain& active = chainman().ActiveChain(); - CBlockIndex* block = active[height]; + const CBlockIndex* block{chainman().ActiveChain()[height]}; return block && ((block->nStatus & BLOCK_HAVE_DATA) != 0) && block->nTx > 0; } std::optional findFork(const uint256& hash, std::optional* height) override @@ -1255,8 +1239,7 @@ class ChainImpl : public Chain std::optional findLocatorFork(const CBlockLocator& locator) override { LOCK(::cs_main); - const Chainstate& active = chainman().ActiveChainstate(); - if (const CBlockIndex* fork = active.FindForkInGlobalIndex(locator)) { + if (const CBlockIndex* fork = chainman().ActiveChainstate().FindForkInGlobalIndex(locator)) { return fork->nHeight; } return std::nullopt; @@ -1304,8 +1287,7 @@ class ChainImpl : public Chain bool findBlock(const uint256& hash, const FoundBlock& block) override { WAIT_LOCK(cs_main, lock); - const CChain& active = chainman().ActiveChain(); - return FillBlock(chainman().m_blockman.LookupBlockIndex(hash), block, lock, active); + return FillBlock(chainman().m_blockman.LookupBlockIndex(hash), block, lock, chainman().ActiveChain()); } bool findFirstBlockWithTimeAndHeight(int64_t min_time, int min_height, const FoundBlock& block) override { @@ -1327,11 +1309,10 @@ class ChainImpl : public Chain bool findAncestorByHash(const uint256& block_hash, const uint256& ancestor_hash, const FoundBlock& ancestor_out) override { WAIT_LOCK(cs_main, lock); - const CChain& active = chainman().ActiveChain(); const CBlockIndex* block = chainman().m_blockman.LookupBlockIndex(block_hash); const CBlockIndex* ancestor = chainman().m_blockman.LookupBlockIndex(ancestor_hash); if (block && ancestor && block->GetAncestor(ancestor->nHeight) != ancestor) ancestor = nullptr; - return FillBlock(ancestor, ancestor_out, lock, active); + return FillBlock(ancestor, ancestor_out, lock, chainman().ActiveChain()); } bool findCommonAncestor(const uint256& block_hash1, const uint256& block_hash2, const FoundBlock& ancestor_out, const FoundBlock& block1_out, const FoundBlock& block2_out) override { @@ -1462,11 +1443,7 @@ class ChainImpl : public Chain } void waitForNotificationsIfTipChanged(const uint256& old_tip) override { - if (!old_tip.IsNull()) { - LOCK(::cs_main); - const CChain& active = chainman().ActiveChain(); - if (old_tip == active.Tip()->GetBlockHash()) return; - } + if (!old_tip.IsNull() && old_tip == WITH_LOCK(::cs_main, return chainman().ActiveChain().Tip()->GetBlockHash())) return; SyncWithValidationInterfaceQueue(); } std::unique_ptr handleRpc(const CRPCCommand& command) override @@ -1521,6 +1498,7 @@ class ChainImpl : public Chain } NodeContext* context() override { return &m_node; } + ChainstateManager& chainman() { return *Assert(m_node.chainman); } NodeContext& m_node; }; } // namespace diff --git a/src/wallet/interfaces.cpp b/src/wallet/interfaces.cpp index d79475001c88..099437d099a3 100644 --- a/src/wallet/interfaces.cpp +++ b/src/wallet/interfaces.cpp @@ -60,6 +60,8 @@ using interfaces::WalletValueMap; using node::NodeContext; namespace wallet { +// All members of the classes in this namespace are intentionally public, as the +// classes themselves are private. namespace { //! Construct wallet tx struct. WalletTx MakeWalletTx(CWallet& wallet, const CWalletTx& wtx) From 83ba97c4a83f2e6bfe043fc94b4badc11bb6db57 Mon Sep 17 00:00:00 2001 From: Konstantin Akimov Date: Mon, 22 Jun 2026 14:32:35 +0700 Subject: [PATCH 02/10] refactor: remove make-up code from backport bitcoin#26933 by pasta-claw --- src/test/txpackage_tests.cpp | 10 ---------- 1 file changed, 10 deletions(-) diff --git a/src/test/txpackage_tests.cpp b/src/test/txpackage_tests.cpp index 06f6fff6b423..780a4b222cc2 100644 --- a/src/test/txpackage_tests.cpp +++ b/src/test/txpackage_tests.cpp @@ -375,16 +375,6 @@ BOOST_FIXTURE_TEST_CASE(package_cpfp_tests, TestChain100Setup) CTransactionRef tx_child = MakeTransactionRef(mtx_child); package_cpfp.push_back(tx_child); - // Verify that the low-fee parent individually meets the min relay fee requirement. - // This is important because Dash transactions are larger than Bitcoin's (no SegWit), - // so we need a higher low_fee_amt to ensure the parent's fee exceeds minRelayTxFee. - BOOST_CHECK_MESSAGE(::minRelayTxFee.GetFee(GetVirtualTransactionSize(*tx_parent)) <= low_fee_amt, - strprintf("low_fee_amt %d is below minRelayTxFee %d for parent vsize %d", - low_fee_amt, ::minRelayTxFee.GetFee(GetVirtualTransactionSize(*tx_parent)), - GetVirtualTransactionSize(*tx_parent))); - // But the parent's fee should be below the mempool minimum feerate. - BOOST_CHECK(m_node.mempool->GetMinFee().GetFee(GetVirtualTransactionSize(*tx_parent)) > low_fee_amt); - // Package feerate is calculated using modified fees, and prioritisetransaction accepts negative // fee deltas. This should be taken into account. De-prioritise the parent transaction // to bring the package feerate to 0. From e16afadba2ccc0787e8dc24053bf7f4ea2763ff7 Mon Sep 17 00:00:00 2001 From: glozow Date: Wed, 3 Aug 2022 09:43:59 +0100 Subject: [PATCH 03/10] Merge bitcoin/bitcoin#25648: refactor: Remove all policy globals BACKPORT NOTE: Moving the -acceptnonstdtxn check out of AppInitParameterInteraction and into ApplyArgsManOptions() means it now runs from AppInitMain, after StartLogging() has already opened the log file. The mainnet node that feature_config_args.py starts to trigger that error therefore leaves a debug.log behind, and a mainnet datadir has no chain subdirectory to name. A chain directory always sits directly under the datadir, so match exactly one level instead. Both nested cases disappear and the escape hatch is no longer needed. This restores the glob depth bitcoin#16973 changed while keeping what that PR actually fixed: pathlib plus as_posix(), so the regex still matches on Windows separators. ----- ddddd6913b1bdee1cad89a32d363306ea1f7b8d7 sort after scripted-diff (MacroFake) fac812ca835e0d843aba1d4db0e49d183018a29e scripted-diff: Move mempool_args to src/node (MacroFake) 66664384a6fec39ecb4d8d06db66a4f193a06e33 Remove ::g_max_datacarrier_bytes global (MacroFake) fad0b4fab849eb5f1f0aa54ebc290f85a473ec91 Pass datacarrier setting into IsStandard (MacroFake) fa2a6b8516b24d7e9ca11926a49cf2b07f661e81 Combine datacarrier globals into one (MacroFake) fa477d32eefcc3dd2f06b452066290d9936d8c5d Remove ::GetVirtualTransactionSize() alias (MacroFake) fa2f6c1a611dffe5a3f63fe1b453f1dd420371b1 Remove ::fIsBareMultisigStd global (MacroFake) fadc14e4f514e7167723285e0ac3d4a7149bbee6 Remove ::dustRelayFee (MacroFake) fa8a7f01fe1b6db98097021276ed5d929faadbec Remove ::IsStandardTx(tx, reason) alias (MacroFake) fa7a9114e59b81b50584311a4ab2b3e9a8d956bd test: Remove unused cs_main (MacroFake) fa9cba7afb73c01bd2c8fefd662dfc80dd98c5e8 Remove ::incrementalRelayFee and ::minRelayTxFee globals (MacroFake) fa148602e67fe035b1b21eff6c0b656919ac2d45 Remove ::fRequireStandard global (MacroFake) fa468bdfb62dec286cb977db78d3e47b64dafeba 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 ddddd6913b1bdee1cad89a32d363306ea1f7b8d7 ariard: Light Code Review ACK ddddd69 Tree-SHA512: 9de2ce601cfcaa4dfd7d1c92270568895ce8702ccdffb59829fbe9618eab0fd88d738afef33ed66988c66861115e0340e881056bfb71e2aed4af2440bd37eb1e --- ci/dash/lint-tidy.sh | 1 + src/Makefile.am | 4 +- src/init.cpp | 46 +---------- src/kernel/mempool_options.h | 19 +++++ src/mempool_args.cpp | 37 --------- src/node/interfaces.cpp | 24 +++++- src/node/mempool_args.cpp | 96 +++++++++++++++++++++++ src/{ => node}/mempool_args.h | 13 ++- src/node/psbt.cpp | 2 +- src/policy/policy.cpp | 13 +-- src/policy/policy.h | 8 +- src/policy/settings.cpp | 7 +- src/policy/settings.h | 29 ------- src/rpc/fees.cpp | 3 +- src/rpc/mempool.cpp | 4 +- src/rpc/net.cpp | 7 +- src/script/standard.cpp | 3 - src/script/standard.h | 12 +-- src/test/evo_assetlocks_tests.cpp | 16 ++-- src/test/evo_deterministicmns_tests.cpp | 2 +- src/test/fuzz/integer.cpp | 3 - src/test/fuzz/key.cpp | 4 +- src/test/fuzz/script.cpp | 2 +- src/test/fuzz/transaction.cpp | 6 +- src/test/fuzz/tx_pool.cpp | 11 ++- src/test/fuzz/validation_load_mempool.cpp | 2 +- src/test/multisig_tests.cpp | 22 ++++-- src/test/script_p2sh_tests.cpp | 8 +- src/test/transaction_tests.cpp | 26 +++--- src/test/txpackage_tests.cpp | 7 +- src/test/util/setup_common.cpp | 8 +- src/test/util/txmempool.cpp | 5 +- src/txmempool.cpp | 14 +++- src/txmempool.h | 16 ++-- src/validation.cpp | 18 +++-- src/validation.h | 1 - src/wallet/fees.cpp | 2 +- src/wallet/test/coinjoin_tests.cpp | 8 -- src/wallet/test/wallet_tests.cpp | 2 - test/functional/combine_logs.py | 6 +- 40 files changed, 277 insertions(+), 240 deletions(-) delete mode 100644 src/mempool_args.cpp create mode 100644 src/node/mempool_args.cpp rename src/{ => node}/mempool_args.h (57%) diff --git a/ci/dash/lint-tidy.sh b/ci/dash/lint-tidy.sh index d7d63656d842..757009610a9c 100755 --- a/ci/dash/lint-tidy.sh +++ b/ci/dash/lint-tidy.sh @@ -90,6 +90,7 @@ iwyu_tool.py \ "src/init" \ "src/kernel" \ "src/node/chainstate.cpp" \ + "src/node/mempool_args.cpp" \ "src/node/minisketchwrapper.cpp" \ "src/policy/feerate.cpp" \ "src/policy/packages.cpp" \ diff --git a/src/Makefile.am b/src/Makefile.am index 501a695cf852..1c2d3d57f68a 100644 --- a/src/Makefile.am +++ b/src/Makefile.am @@ -320,7 +320,6 @@ BITCOIN_CORE_H = \ masternode/payments.h \ masternode/sync.h \ masternode/utils.h \ - mempool_args.h \ memusage.h \ merkleblock.h \ messagesigner.h \ @@ -342,6 +341,7 @@ BITCOIN_CORE_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 \ @@ -596,7 +596,6 @@ libbitcoin_node_a_SOURCES = \ masternode/payments.cpp \ masternode/sync.cpp \ masternode/utils.cpp \ - mempool_args.cpp \ net.cpp \ net_processing.cpp \ netfulfilledman.cpp \ @@ -610,6 +609,7 @@ libbitcoin_node_a_SOURCES = \ 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 \ diff --git a/src/init.cpp b/src/init.cpp index 04f026e2463c..3832e0502175 100644 --- a/src/init.cpp +++ b/src/init.cpp @@ -40,7 +40,6 @@ #include #include #include -#include #include #include #include @@ -51,6 +50,7 @@ #include #include #include +#include #include #include #include @@ -1313,15 +1313,6 @@ bool AppInitParameterInteraction(const ArgsManager& args) LogPrintf("Warning: nMinimumChainWork set below default value of %s\n", chainparams.GetConsensus().nMinimumChainWork.GetHex()); } - // incremental relay fee sets the amount the mempool min fee increases above the feerate of txs evicted due to mempool limiting. - if (args.IsArgSet("-incrementalrelayfee")) { - if (std::optional 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) { @@ -1357,19 +1348,6 @@ bool AppInitParameterInteraction(const ArgsManager& args) return InitError(Untranslated("peertimeout must be a positive integer.")); } - if (args.IsArgSet("-minrelaytxfee")) { - if (std::optional 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")) { @@ -1378,28 +1356,10 @@ bool AppInitParameterInteraction(const ArgsManager& args) } } - // 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 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 @@ -1987,7 +1947,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(mempool_opts.check_ratio, 0, 1'000'000); int64_t descendant_limit_bytes = mempool_opts.limits.descendant_size_vbytes * 40; diff --git a/src/kernel/mempool_options.h b/src/kernel/mempool_options.h index a14abb662890..861ff4f2465c 100644 --- a/src/kernel/mempool_options.h +++ b/src/kernel/mempool_options.h @@ -6,8 +6,13 @@ #include +#include +#include +#include