From 52da29d2290b032fb6dc5c2cc7f4044d3d2598d1 Mon Sep 17 00:00:00 2001 From: Luke Dashjr Date: Fri, 4 Sep 2020 18:27:02 +0000 Subject: [PATCH 01/13] Flush dbcache early if system is under memory pressure No point forcing memory to get pushed out to swap just to cache db changes when we can write the db changes out instead --- configure.ac | 15 +++++++++++++++ src/util/system.cpp | 30 ++++++++++++++++++++++++++++++ src/util/system.h | 2 ++ src/validation.cpp | 11 +++++++++-- 4 files changed, 56 insertions(+), 2 deletions(-) diff --git a/configure.ac b/configure.ac index b5116922f9..82af1655d0 100644 --- a/configure.ac +++ b/configure.ac @@ -1061,6 +1061,21 @@ AC_COMPILE_IFELSE([AC_LANG_PROGRAM([[#include ]], [ AC_MSG_RESULT([no])] ) +AC_MSG_CHECKING(for compatible sysinfo call) +AC_COMPILE_IFELSE([AC_LANG_PROGRAM([[#include ]], + [[ + struct sysinfo info; + int rv = sysinfo(&info); + unsigned long test = info.freeram + info.bufferram + info.mem_unit; + ]])], + [ + AC_MSG_RESULT(yes); + AC_DEFINE(HAVE_LINUX_SYSINFO, 1, [Define this symbol if you have a Linux-compatible sysinfo call]) + ],[ + AC_MSG_RESULT(no) + ] +) + dnl Check for posix_fallocate AC_MSG_CHECKING([for posix_fallocate]) AC_COMPILE_IFELSE([AC_LANG_PROGRAM([[ diff --git a/src/util/system.cpp b/src/util/system.cpp index 630eff99e0..2e4f18be31 100644 --- a/src/util/system.cpp +++ b/src/util/system.cpp @@ -42,6 +42,10 @@ #include #endif +#ifdef HAVE_LINUX_SYSINFO +#include +#endif + #include #include @@ -1130,4 +1134,30 @@ std::pair WinCmdLineArgs::get() return std::make_pair(argc, argv); } #endif + +bool SystemNeedsMemoryReleased() +{ + constexpr size_t low_memory_threshold = 10 * 1024 * 1024 /* 10 MB */; +#ifdef WIN32 + MEMORYSTATUSEX mem_status; + mem_status.dwLength = sizeof(mem_status); + if (GlobalMemoryStatusEx(&mem_status)) { + if (mem_status.dwMemoryLoad >= 99) return true; + if (mem_status.ullAvailPhys < low_memory_threshold) return true; + if (mem_status.ullAvailVirtual < low_memory_threshold) return true; + } +#endif +#ifdef HAVE_LINUX_SYSINFO + struct sysinfo sys_info; + if (!sysinfo(&sys_info)) { + // Explicitly 64-bit in case of 32-bit userspace on 64-bit kernel + const uint64_t free_ram = uint64_t(sys_info.freeram) * sys_info.mem_unit; + const uint64_t buffer_ram = uint64_t(sys_info.bufferram) * sys_info.mem_unit; + if (free_ram + buffer_ram < low_memory_threshold) return true; + } +#endif + // NOTE: sysconf(_SC_AVPHYS_PAGES) doesn't account for caches on at least Linux, so not safe to use here + return false; +} + } // namespace util diff --git a/src/util/system.h b/src/util/system.h index ff3155b498..55d41b028f 100644 --- a/src/util/system.h +++ b/src/util/system.h @@ -511,6 +511,8 @@ private: }; #endif +bool SystemNeedsMemoryReleased(); + } // namespace util #endif // BITCOIN_UTIL_SYSTEM_H diff --git a/src/validation.cpp b/src/validation.cpp index 1fd8f0e326..c8dd71e89c 100644 --- a/src/validation.cpp +++ b/src/validation.cpp @@ -2501,8 +2501,15 @@ bool Chainstate::FlushStateToDisk( } // The cache is large and we're within 10% and 10 MiB of the limit, but we have time now (not in the middle of a block processing). bool fCacheLarge = mode == FlushStateMode::PERIODIC && cache_state >= CoinsCacheSizeState::LARGE; - // The cache is over the limit, we have to write now. - bool fCacheCritical = mode == FlushStateMode::IF_NEEDED && cache_state >= CoinsCacheSizeState::CRITICAL; + bool fCacheCritical = false; + if (mode == FlushStateMode::IF_NEEDED) { + if (cache_state >= CoinsCacheSizeState::CRITICAL) { + // The cache is over the limit, we have to write now. + fCacheCritical = true; + } else if (util::SystemNeedsMemoryReleased()) { + fCacheCritical = true; + } + } // It's been a while since we wrote the block index to disk. Do this frequently, so we don't need to redownload after a crash. bool fPeriodicWrite = mode == FlushStateMode::PERIODIC && nNow > m_last_write + DATABASE_WRITE_INTERVAL; // It's been very long since we flushed the cache. Do this infrequently, to optimize cache usage. From 3b3d347866998b54dc6e9615230e01bc77846f1d Mon Sep 17 00:00:00 2001 From: Luke Dashjr Date: Sat, 24 Oct 2020 14:16:00 +0000 Subject: [PATCH 02/13] util: Log reasoning when returning true from SystemNeedsMemoryReleased --- src/util/system.cpp | 14 ++++++++++---- 1 file changed, 10 insertions(+), 4 deletions(-) diff --git a/src/util/system.cpp b/src/util/system.cpp index 2e4f18be31..288c6cceed 100644 --- a/src/util/system.cpp +++ b/src/util/system.cpp @@ -1142,9 +1142,12 @@ bool SystemNeedsMemoryReleased() MEMORYSTATUSEX mem_status; mem_status.dwLength = sizeof(mem_status); if (GlobalMemoryStatusEx(&mem_status)) { - if (mem_status.dwMemoryLoad >= 99) return true; - if (mem_status.ullAvailPhys < low_memory_threshold) return true; - if (mem_status.ullAvailVirtual < low_memory_threshold) return true; + if (mem_status.dwMemoryLoad >= 99 || + mem_status.ullAvailPhys < low_memory_threshold || + mem_status.ullAvailVirtual < low_memory_threshold) { + LogPrintf("%s: YES: %s%% memory load; %s available physical memory; %s available virtual memory\n", __func__, int(mem_status.dwMemoryLoad), size_t(mem_status.ullAvailPhys), size_t(mem_status.ullAvailVirtual)); + return true; + } } #endif #ifdef HAVE_LINUX_SYSINFO @@ -1153,7 +1156,10 @@ bool SystemNeedsMemoryReleased() // Explicitly 64-bit in case of 32-bit userspace on 64-bit kernel const uint64_t free_ram = uint64_t(sys_info.freeram) * sys_info.mem_unit; const uint64_t buffer_ram = uint64_t(sys_info.bufferram) * sys_info.mem_unit; - if (free_ram + buffer_ram < low_memory_threshold) return true; + if (free_ram + buffer_ram < low_memory_threshold) { + LogPrintf("%s: YES: %s free RAM + %s buffer RAM\n", __func__, free_ram, buffer_ram); + return true; + } } #endif // NOTE: sysconf(_SC_AVPHYS_PAGES) doesn't account for caches on at least Linux, so not safe to use here From 6399e4c638a5e59a2c4116fc34508b585bda1039 Mon Sep 17 00:00:00 2001 From: Luke Dashjr Date: Sat, 24 Oct 2020 14:43:26 +0000 Subject: [PATCH 03/13] Make lowmem threshold configurable --- src/init.cpp | 8 ++++++++ src/util/system.cpp | 13 +++++++++---- src/util/system.h | 2 ++ 3 files changed, 19 insertions(+), 4 deletions(-) diff --git a/src/init.cpp b/src/init.cpp index 1122496539..7860509a09 100644 --- a/src/init.cpp +++ b/src/init.cpp @@ -441,6 +441,7 @@ void SetupServerArgs(ArgsManager& argsman) argsman.AddArg("-dbcache=", strprintf("Maximum database cache size MiB (%d to %d, default: %d). In addition, unused mempool memory is shared for this cache (see -maxmempool).", nMinDbCache, nMaxDbCache, nDefaultDbCache), ArgsManager::ALLOW_ANY, OptionsCategory::OPTIONS); argsman.AddArg("-includeconf=", "Specify additional configuration file, relative to the -datadir path (only useable from configuration file, not command line)", ArgsManager::ALLOW_ANY, OptionsCategory::OPTIONS); argsman.AddArg("-loadblock=", "Imports blocks from external file on startup", ArgsManager::ALLOW_ANY, OptionsCategory::OPTIONS); + argsman.AddArg("-lowmem=", strprintf("If system available memory falls below MiB, flush caches (0 to disable, default: %s)", util::g_low_memory_threshold / 1024 / 1024), ArgsManager::ALLOW_ANY, OptionsCategory::OPTIONS); argsman.AddArg("-maxmempool=", strprintf("Keep the transaction memory pool below megabytes (default: %u)", DEFAULT_MAX_MEMPOOL_SIZE_MB), ArgsManager::ALLOW_ANY, OptionsCategory::OPTIONS); argsman.AddArg("-maxorphantx=", strprintf("Keep at most unconnectable transactions in memory (default: %u)", DEFAULT_MAX_ORPHAN_TRANSACTIONS), ArgsManager::ALLOW_ANY, OptionsCategory::OPTIONS); argsman.AddArg("-mempoolexpiry=", strprintf("Do not keep transactions in the mempool longer than hours (default: %u)", DEFAULT_MEMPOOL_EXPIRY_HOURS), ArgsManager::ALLOW_ANY, OptionsCategory::OPTIONS); @@ -1473,6 +1474,13 @@ bool AppInitMain(NodeContext& node, interfaces::BlockAndHeaderTipInfo* tip_info) } LogPrintf("* Using %.1f MiB for in-memory UTXO set (plus up to %.1f MiB of unused mempool space)\n", cache_sizes.coins * (1.0 / 1024 / 1024), mempool_opts.max_size_bytes * (1.0 / 1024 / 1024)); + if (gArgs.IsArgSet("-lowmem")) { + util::g_low_memory_threshold = gArgs.GetIntArg("-lowmem", 0 /* not used */) * 1024 * 1024; + } + if (util::g_low_memory_threshold > 0) { + LogPrintf("* Flushing caches if available system memory drops below %s MiB\n", util::g_low_memory_threshold / 1024 / 1024); + } + for (bool fLoaded = false; !fLoaded && !ShutdownRequested();) { node.mempool = std::make_unique(mempool_opts); diff --git a/src/util/system.cpp b/src/util/system.cpp index 288c6cceed..ce88206e35 100644 --- a/src/util/system.cpp +++ b/src/util/system.cpp @@ -1135,16 +1135,21 @@ std::pair WinCmdLineArgs::get() } #endif +size_t g_low_memory_threshold = 10 * 1024 * 1024 /* 10 MB */; + bool SystemNeedsMemoryReleased() { - constexpr size_t low_memory_threshold = 10 * 1024 * 1024 /* 10 MB */; + if (g_low_memory_threshold <= 0) { + // Intentionally bypass other metrics when disabled entirely + return false; + } #ifdef WIN32 MEMORYSTATUSEX mem_status; mem_status.dwLength = sizeof(mem_status); if (GlobalMemoryStatusEx(&mem_status)) { if (mem_status.dwMemoryLoad >= 99 || - mem_status.ullAvailPhys < low_memory_threshold || - mem_status.ullAvailVirtual < low_memory_threshold) { + mem_status.ullAvailPhys < g_low_memory_threshold || + mem_status.ullAvailVirtual < g_low_memory_threshold) { LogPrintf("%s: YES: %s%% memory load; %s available physical memory; %s available virtual memory\n", __func__, int(mem_status.dwMemoryLoad), size_t(mem_status.ullAvailPhys), size_t(mem_status.ullAvailVirtual)); return true; } @@ -1156,7 +1161,7 @@ bool SystemNeedsMemoryReleased() // Explicitly 64-bit in case of 32-bit userspace on 64-bit kernel const uint64_t free_ram = uint64_t(sys_info.freeram) * sys_info.mem_unit; const uint64_t buffer_ram = uint64_t(sys_info.bufferram) * sys_info.mem_unit; - if (free_ram + buffer_ram < low_memory_threshold) { + if (free_ram + buffer_ram < g_low_memory_threshold) { LogPrintf("%s: YES: %s free RAM + %s buffer RAM\n", __func__, free_ram, buffer_ram); return true; } diff --git a/src/util/system.h b/src/util/system.h index 55d41b028f..d8a18d413e 100644 --- a/src/util/system.h +++ b/src/util/system.h @@ -511,6 +511,8 @@ private: }; #endif +extern size_t g_low_memory_threshold; + bool SystemNeedsMemoryReleased(); } // namespace util From 27d43142a0d2a914295dd9fa4695de63a61f30ea Mon Sep 17 00:00:00 2001 From: Luke Dashjr Date: Sat, 24 Oct 2020 14:43:51 +0000 Subject: [PATCH 04/13] Disable lowmem flushing in test that needs determinism --- src/test/validation_chainstate_tests.cpp | 2 ++ src/wallet/test/wallet_tests.cpp | 3 +++ 2 files changed, 5 insertions(+) diff --git a/src/test/validation_chainstate_tests.cpp b/src/test/validation_chainstate_tests.cpp index 2078fcd8f8..b9854ec428 100644 --- a/src/test/validation_chainstate_tests.cpp +++ b/src/test/validation_chainstate_tests.cpp @@ -24,6 +24,8 @@ BOOST_FIXTURE_TEST_SUITE(validation_chainstate_tests, ChainTestingSetup) //! BOOST_AUTO_TEST_CASE(validation_chainstate_resize_caches) { + util::g_low_memory_threshold = 0; // disable to get deterministic flushing + ChainstateManager& manager = *Assert(m_node.chainman); CTxMemPool& mempool = *Assert(m_node.mempool); Chainstate& c1 = WITH_LOCK(cs_main, return manager.InitializeChainstate(&mempool)); diff --git a/src/wallet/test/wallet_tests.cpp b/src/wallet/test/wallet_tests.cpp index edcfaa24e5..c9a987b270 100644 --- a/src/wallet/test/wallet_tests.cpp +++ b/src/wallet/test/wallet_tests.cpp @@ -766,6 +766,9 @@ BOOST_FIXTURE_TEST_CASE(wallet_descriptor_test, BasicTestingSetup) //! rescanning where new transactions in new blocks could be lost. BOOST_FIXTURE_TEST_CASE(CreateWallet, TestChain100Setup) { + // FIXME: this test fails for some reason if there's a flush + util::g_low_memory_threshold = 0; + m_args.ForceSetArg("-unsafesqlitesync", "1"); // Create new wallet with known key and unload it. WalletContext context; From c624d1d4a320b3ab55ae40d82411ed261bc2d175 Mon Sep 17 00:00:00 2001 From: Luke Dashjr Date: Wed, 30 Oct 2019 11:23:57 +0000 Subject: [PATCH 05/13] Restore -mempoolreplacement option to allow disabling opt-in RBF This partially reverts commit 8053e5cdade87550f0381d51feab81dedfec6c46. --- src/init.cpp | 9 +++++++++ src/policy/policy.h | 2 ++ src/policy/settings.cpp | 1 + src/policy/settings.h | 1 + src/validation.cpp | 10 +++++++++- test/functional/feature_rbf.py | 22 +++++++++++++++++++--- 6 files changed, 41 insertions(+), 4 deletions(-) diff --git a/src/init.cpp b/src/init.cpp index 7860509a09..21904ec9ad 100644 --- a/src/init.cpp +++ b/src/init.cpp @@ -584,6 +584,7 @@ void SetupServerArgs(ArgsManager& argsman) argsman.AddArg("-datacarrier", strprintf("Relay and mine data carrier transactions (default: %u)", DEFAULT_ACCEPT_DATACARRIER), ArgsManager::ALLOW_ANY, OptionsCategory::NODE_RELAY); argsman.AddArg("-datacarriersize", strprintf("Maximum size of data in data carrier transactions we relay and mine (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 (default: %u)", DEFAULT_ENABLE_REPLACEMENT), ArgsManager::ALLOW_ANY, OptionsCategory::NODE_RELAY); argsman.AddArg("-permitbaremultisig", strprintf("Relay non-P2SH multisig (default: %u)", DEFAULT_PERMIT_BAREMULTISIG), ArgsManager::ALLOW_ANY, OptionsCategory::NODE_RELAY); argsman.AddArg("-minrelaytxfee=", strprintf("Fees (in %s/kvB) smaller than this are considered zero fee for relaying, mining and transaction creation (default: %s)", @@ -993,6 +994,14 @@ bool AppInitParameterInteraction(const ArgsManager& args, bool use_syscall_sandb } } + gEnableReplacement = args.GetBoolArg("-mempoolreplacement", DEFAULT_ENABLE_REPLACEMENT); + if ((!gEnableReplacement) && args.IsArgSet("-mempoolreplacement")) { + // Minimal effort at forwards compatibility + std::string replacement_opt = args.GetArg("-mempoolreplacement", ""); // default is impossible + std::vector replacement_modes = SplitString(replacement_opt, ","); + gEnableReplacement = (std::find(replacement_modes.begin(), replacement_modes.end(), "fee") != replacement_modes.end()); + } + #if defined(USE_SYSCALL_SANDBOX) if (args.IsArgSet("-sandbox") && !args.IsArgNegated("-sandbox")) { const std::string sandbox_arg{args.GetArg("-sandbox", "")}; diff --git a/src/policy/policy.h b/src/policy/policy.h index 7dd4205b74..d7a8f5a749 100644 --- a/src/policy/policy.h +++ b/src/policy/policy.h @@ -36,6 +36,8 @@ 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 */ diff --git a/src/policy/settings.cpp b/src/policy/settings.cpp index 722b12acf5..4a47ebdfea 100644 --- a/src/policy/settings.cpp +++ b/src/policy/settings.cpp @@ -7,4 +7,5 @@ #include +bool gEnableReplacement = DEFAULT_ENABLE_REPLACEMENT; unsigned int nBytesPerSigOp = DEFAULT_BYTES_PER_SIGOP; diff --git a/src/policy/settings.h b/src/policy/settings.h index 145454b0dd..da33c53575 100644 --- a/src/policy/settings.h +++ b/src/policy/settings.h @@ -6,6 +6,7 @@ #ifndef BITCOIN_POLICY_SETTINGS_H #define BITCOIN_POLICY_SETTINGS_H +extern bool gEnableReplacement; extern unsigned int nBytesPerSigOp; #endif // BITCOIN_POLICY_SETTINGS_H diff --git a/src/validation.cpp b/src/validation.cpp index 8cc223d6bb..5904822290 100644 --- a/src/validation.cpp +++ b/src/validation.cpp @@ -769,7 +769,15 @@ bool MemPoolAccept::PreChecks(ATMPArgs& args, Workspace& ws) // // If replaceability signaling is ignored due to node setting, // replacement is always allowed. - if (!(m_pool.m_full_rbf || ignore_rejects.count("txn-mempool-conflict")) && !SignalsOptInRBF(*ptxConflicting)) { + bool allow_replacement; + if (m_pool.m_full_rbf || ignore_rejects.count("txn-mempool-conflict")) { + allow_replacement = true; + } else if (!gEnableReplacement) { + allow_replacement = false; + } else { + allow_replacement = SignalsOptInRBF(*ptxConflicting); + } + if (!allow_replacement) { return state.Invalid(TxValidationResult::TX_MEMPOOL_POLICY, "txn-mempool-conflict"); } diff --git a/test/functional/feature_rbf.py b/test/functional/feature_rbf.py index 947d2e8273..8e63674caa 100755 --- a/test/functional/feature_rbf.py +++ b/test/functional/feature_rbf.py @@ -25,7 +25,7 @@ class ReplaceByFeeTest(BitcoinTestFramework): self.add_wallet_options(parser) def set_test_params(self): - self.num_nodes = 2 + self.num_nodes = 3 self.extra_args = [ [ "-maxorphantx=1000", @@ -37,6 +37,10 @@ class ReplaceByFeeTest(BitcoinTestFramework): # second node has default mempool parameters [ ], + [ + "-acceptnonstdtxn=1", + "-mempoolreplacement=0", + ], ] self.supports_cli = False @@ -112,17 +116,24 @@ class ReplaceByFeeTest(BitcoinTestFramework): # we use MiniWallet to create a transaction template with inputs correctly set, # and modify the output (amount, scriptPubKey) according to our needs tx = self.wallet.create_self_transfer()["tx"] - tx1a_txid = self.nodes[0].sendrawtransaction(tx.serialize().hex()) + tx1a_hex = tx.serialize().hex() + tx1a_txid = self.nodes[0].sendrawtransaction(tx1a_hex) + assert_equal(tx1a_txid, self.nodes[2].sendrawtransaction(tx1a_hex)) # Should fail because we haven't changed the fee tx.vout[0].scriptPubKey[-1] ^= 1 # This will raise an exception due to insufficient fee - assert_raises_rpc_error(-26, "insufficient fee", self.nodes[0].sendrawtransaction, tx.serialize().hex(), 0) + tx1b_hex = tx.serialize().hex() + assert_raises_rpc_error(-26, "insufficient fee", self.nodes[0].sendrawtransaction, tx1b_hex, 0) + # This will raise an exception due to transaction replacement being disabled + assert_raises_rpc_error(-26, "txn-mempool-conflict", self.nodes[2].sendrawtransaction, tx1b_hex, 0) # Extra 0.1 BTC fee tx.vout[0].nValue -= int(0.1 * COIN) tx1b_hex = tx.serialize().hex() + # Replacement still disabled even with "enough fee" + assert_raises_rpc_error(-26, "txn-mempool-conflict", self.nodes[2].sendrawtransaction, tx1b_hex, 0) # Works when enabled tx1b_txid = self.nodes[0].sendrawtransaction(tx1b_hex, 0) @@ -133,6 +144,11 @@ class ReplaceByFeeTest(BitcoinTestFramework): assert_equal(tx1b_hex, self.nodes[0].getrawtransaction(tx1b_txid)) + # Third node is running mempoolreplacement=0, will not replace originally-seen txn + mempool = self.nodes[2].getrawmempool() + assert tx1a_txid in mempool + assert tx1b_txid not in mempool + def test_doublespend_chain(self): """Doublespend of a long chain""" From 57053063fd3ed25019cc9fc2fe468215d8bbc04a Mon Sep 17 00:00:00 2001 From: Luke Dashjr Date: Sat, 13 Feb 2016 01:46:40 +0000 Subject: [PATCH 06/13] Make it possible to unconditionally RBF with mempoolreplacement=fee,-optin --- src/init.cpp | 19 +++++++++++++++++-- 1 file changed, 17 insertions(+), 2 deletions(-) diff --git a/src/init.cpp b/src/init.cpp index 21904ec9ad..4562111db9 100644 --- a/src/init.cpp +++ b/src/init.cpp @@ -584,7 +584,7 @@ void SetupServerArgs(ArgsManager& argsman) argsman.AddArg("-datacarrier", strprintf("Relay and mine data carrier transactions (default: %u)", DEFAULT_ACCEPT_DATACARRIER), ArgsManager::ALLOW_ANY, OptionsCategory::NODE_RELAY); argsman.AddArg("-datacarriersize", strprintf("Maximum size of data in data carrier transactions we relay and mine (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 (default: %u)", DEFAULT_ENABLE_REPLACEMENT), 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("-permitbaremultisig", strprintf("Relay non-P2SH multisig (default: %u)", DEFAULT_PERMIT_BAREMULTISIG), ArgsManager::ALLOW_ANY, OptionsCategory::NODE_RELAY); argsman.AddArg("-minrelaytxfee=", strprintf("Fees (in %s/kvB) smaller than this are considered zero fee for relaying, mining and transaction creation (default: %s)", @@ -836,6 +836,8 @@ bool AppInitBasicSetup(const ArgsManager& args) return true; } +static bool gReplacementHonourOptOut; // FIXME: Get rid of this + bool AppInitParameterInteraction(const ArgsManager& args, bool use_syscall_sandbox) { const CChainParams& chainparams = Params(); @@ -995,11 +997,23 @@ bool AppInitParameterInteraction(const ArgsManager& args, bool use_syscall_sandb } 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 replacement_modes = SplitString(replacement_opt, ","); + std::vector replacement_modes = 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; + } } #if defined(USE_SYSCALL_SANDBOX) @@ -1471,6 +1485,7 @@ bool AppInitMain(NodeContext& node, interfaces::BlockAndHeaderTipInfo* tip_info) CTxMemPool::Options mempool_opts{ .estimator = node.fee_estimator.get(), .check_ratio = chainparams.DefaultConsistencyChecks() ? 1 : 0, + .full_rbf = !gReplacementHonourOptOut, }; if (const auto err{ApplyArgsManOptions(args, chainparams, mempool_opts)}) { return InitError(*err); From 04ba461d6a943166bfde4609e10ed08c0cb37385 Mon Sep 17 00:00:00 2001 From: Luke Dashjr Date: Thu, 23 Jul 2020 17:51:37 +0000 Subject: [PATCH 07/13] QA: feature_rbf: Test full RBF mode --- test/functional/feature_rbf.py | 43 +++++++++++++++++++++++++--------- 1 file changed, 32 insertions(+), 11 deletions(-) diff --git a/test/functional/feature_rbf.py b/test/functional/feature_rbf.py index 8e63674caa..84bc5c9e89 100755 --- a/test/functional/feature_rbf.py +++ b/test/functional/feature_rbf.py @@ -25,7 +25,7 @@ class ReplaceByFeeTest(BitcoinTestFramework): self.add_wallet_options(parser) def set_test_params(self): - self.num_nodes = 3 + self.num_nodes = 4 self.extra_args = [ [ "-maxorphantx=1000", @@ -42,6 +42,12 @@ class ReplaceByFeeTest(BitcoinTestFramework): "-mempoolreplacement=0", ], ] + self.extra_args.append( + [ + *self.extra_args[0], + "-mempoolreplacement=fee,-optin", + ], + ) self.supports_cli = False def run_test(self): @@ -72,7 +78,10 @@ class ReplaceByFeeTest(BitcoinTestFramework): self.test_too_many_replacements_with_default_mempool_params() self.log.info("Running test opt-in...") - self.test_opt_in() + self.test_opt_in(fullrbf=False) + self.nodes[0], self.nodes[-1] = self.nodes[-1], self.nodes[0] + self.test_opt_in(fullrbf=True) + self.nodes[0], self.nodes[-1] = self.nodes[-1], self.nodes[0] self.log.info("Running test RPC...") self.test_rpc() @@ -484,7 +493,7 @@ class ReplaceByFeeTest(BitcoinTestFramework): self.generate(normal_node, 1) self.wallet.rescan_utxos() - def test_opt_in(self): + def test_opt_in(self, fullrbf): """Replacing should only work if orig tx opted in""" tx0_outpoint = self.make_utxo(self.nodes[0], int(1.1 * COIN)) @@ -500,14 +509,19 @@ class ReplaceByFeeTest(BitcoinTestFramework): assert_equal(self.nodes[0].getmempoolentry(tx1a_utxo["txid"])['bip125-replaceable'], False) # Shouldn't be able to double-spend - tx1b_hex = self.wallet.create_self_transfer( + tx1b_st = self.wallet.create_self_transfer( utxo_to_spend=tx0_outpoint, sequence=0, fee=Decimal("0.2"), - )["hex"] + ) + tx1b_hex = tx1b_st["hex"] - # This will raise an exception - assert_raises_rpc_error(-26, "txn-mempool-conflict", self.nodes[0].sendrawtransaction, tx1b_hex, 0) + if fullrbf: + self.nodes[0].sendrawtransaction(tx1b_hex, 0) + tx1a_utxo = tx1b_st["new_utxo"] + else: + # This will raise an exception + assert_raises_rpc_error(-26, "txn-mempool-conflict", self.nodes[0].sendrawtransaction, tx1b_hex, 0) tx1_outpoint = self.make_utxo(self.nodes[0], int(1.1 * COIN)) @@ -520,14 +534,19 @@ class ReplaceByFeeTest(BitcoinTestFramework): )["new_utxo"] # Still shouldn't be able to double-spend - tx2b_hex = self.wallet.create_self_transfer( + tx2b_st = self.wallet.create_self_transfer( utxo_to_spend=tx1_outpoint, sequence=0, fee=Decimal("0.2"), - )["hex"] + ) + tx2b_hex = tx2b_st["hex"] - # This will raise an exception - assert_raises_rpc_error(-26, "txn-mempool-conflict", self.nodes[0].sendrawtransaction, tx2b_hex, 0) + if fullrbf: + self.nodes[0].sendrawtransaction(tx2b_hex, 0) + tx2a_utxo = tx2b_st["new_utxo"] + else: + # This will raise an exception + assert_raises_rpc_error(-26, "txn-mempool-conflict", self.nodes[0].sendrawtransaction, tx2b_hex, 0) # Now create a new transaction that spends from tx1a and tx2a # opt-in on one of the inputs @@ -559,6 +578,8 @@ class ReplaceByFeeTest(BitcoinTestFramework): fee=Decimal("0.4"), ) + self.generate(self.nodes[0], 1) # clean mempool + def test_prioritised_transactions(self): # Ensure that fee deltas used via prioritisetransaction are # correctly used by replacement logic From a7594a61e4d208f8876ca05098b8d5c4df010e29 Mon Sep 17 00:00:00 2001 From: Luke Dashjr Date: Fri, 12 Feb 2016 23:40:02 +0000 Subject: [PATCH 08/13] Recognise temporary REPLACE_BY_FEE service bit --- src/protocol.cpp | 1 + src/protocol.h | 2 ++ 2 files changed, 3 insertions(+) diff --git a/src/protocol.cpp b/src/protocol.cpp index aa59bae6ff..49784ecf14 100644 --- a/src/protocol.cpp +++ b/src/protocol.cpp @@ -198,6 +198,7 @@ static std::string serviceFlagToStr(size_t bit) case NODE_WITNESS: return "WITNESS"; case NODE_COMPACT_FILTERS: return "COMPACT_FILTERS"; case NODE_NETWORK_LIMITED: return "NETWORK_LIMITED"; + case NODE_REPLACE_BY_FEE: return "REPLACE_BY_FEE?"; // Not using default, so we get warned when a case is missing } diff --git a/src/protocol.h b/src/protocol.h index cbcd400fef..bbce407d75 100644 --- a/src/protocol.h +++ b/src/protocol.h @@ -299,6 +299,8 @@ enum ServiceFlags : uint64_t { // collisions and other cases where nodes may be advertising a service they // do not actually support. Other service bits should be allocated via the // BIP process. + + NODE_REPLACE_BY_FEE = (1 << 26), }; /** From 430f823a6409a02c8001cb5090ec263142d71f70 Mon Sep 17 00:00:00 2001 From: Luke Dashjr Date: Fri, 8 Oct 2021 02:25:46 +0000 Subject: [PATCH 09/13] Advertise temporary REPLACE_BY_FEE service bit (when appropriate) --- src/init.cpp | 4 ++++ 1 file changed, 4 insertions(+) diff --git a/src/init.cpp b/src/init.cpp index 4562111db9..4c548baa5d 100644 --- a/src/init.cpp +++ b/src/init.cpp @@ -1505,6 +1505,10 @@ bool AppInitMain(NodeContext& node, interfaces::BlockAndHeaderTipInfo* tip_info) LogPrintf("* Flushing caches if available system memory drops below %s MiB\n", util::g_low_memory_threshold / 1024 / 1024); } + if (mempool_opts.full_rbf) { + nLocalServices = ServiceFlags(nLocalServices | NODE_REPLACE_BY_FEE); + } + for (bool fLoaded = false; !fLoaded && !ShutdownRequested();) { node.mempool = std::make_unique(mempool_opts); From eefc1844ca3626844b38a3991380af8b590c10cb Mon Sep 17 00:00:00 2001 From: Luke Dashjr Date: Fri, 11 Aug 2023 22:55:38 +0000 Subject: [PATCH 10/13] Rework mempoolreplacement option handling --- src/init.cpp | 29 ++----------------- src/kernel/mempool_options.h | 8 ++++-- src/node/mempool_args.cpp | 55 +++++++++++++++++++++++++++++++++++- src/policy/policy.h | 2 -- src/policy/settings.cpp | 1 - src/policy/settings.h | 1 - src/rpc/mempool.cpp | 2 +- src/txmempool.cpp | 2 +- src/txmempool.h | 2 +- src/validation.cpp | 4 +-- 10 files changed, 67 insertions(+), 39 deletions(-) diff --git a/src/init.cpp b/src/init.cpp index 4c548baa5d..16633c7a42 100644 --- a/src/init.cpp +++ b/src/init.cpp @@ -583,8 +583,8 @@ void SetupServerArgs(ArgsManager& argsman) argsman.AddArg("-bytespersigop", strprintf("Equivalent bytes per sigop in transactions for relay and mining (default: %u)", DEFAULT_BYTES_PER_SIGOP), ArgsManager::ALLOW_ANY, OptionsCategory::NODE_RELAY); argsman.AddArg("-datacarrier", strprintf("Relay and mine data carrier transactions (default: %u)", DEFAULT_ACCEPT_DATACARRIER), ArgsManager::ALLOW_ANY, OptionsCategory::NODE_RELAY); argsman.AddArg("-datacarriersize", strprintf("Maximum size of data in data carrier transactions we relay and mine (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 non-P2SH multisig (default: %u)", DEFAULT_PERMIT_BAREMULTISIG), ArgsManager::ALLOW_ANY, OptionsCategory::NODE_RELAY); argsman.AddArg("-minrelaytxfee=", strprintf("Fees (in %s/kvB) smaller than this are considered zero fee for relaying, mining and transaction creation (default: %s)", @@ -836,8 +836,6 @@ bool AppInitBasicSetup(const ArgsManager& args) return true; } -static bool gReplacementHonourOptOut; // FIXME: Get rid of this - bool AppInitParameterInteraction(const ArgsManager& args, bool use_syscall_sandbox) { const CChainParams& chainparams = Params(); @@ -996,26 +994,6 @@ bool AppInitParameterInteraction(const ArgsManager& args, bool use_syscall_sandb } } - 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 replacement_modes = 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; - } - } - #if defined(USE_SYSCALL_SANDBOX) if (args.IsArgSet("-sandbox") && !args.IsArgNegated("-sandbox")) { const std::string sandbox_arg{args.GetArg("-sandbox", "")}; @@ -1485,7 +1463,6 @@ bool AppInitMain(NodeContext& node, interfaces::BlockAndHeaderTipInfo* tip_info) CTxMemPool::Options mempool_opts{ .estimator = node.fee_estimator.get(), .check_ratio = chainparams.DefaultConsistencyChecks() ? 1 : 0, - .full_rbf = !gReplacementHonourOptOut, }; if (const auto err{ApplyArgsManOptions(args, chainparams, mempool_opts)}) { return InitError(*err); @@ -1505,7 +1482,7 @@ bool AppInitMain(NodeContext& node, interfaces::BlockAndHeaderTipInfo* tip_info) LogPrintf("* Flushing caches if available system memory drops below %s MiB\n", util::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); } diff --git a/src/kernel/mempool_options.h b/src/kernel/mempool_options.h index beb5fca5e9..92b63d45eb 100644 --- a/src/kernel/mempool_options.h +++ b/src/kernel/mempool_options.h @@ -16,14 +16,16 @@ class CBlockPolicyEstimator; +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{false}; +/** Default for -mempoolreplacement; must update docs in init.cpp manually */ +static constexpr RBFPolicy DEFAULT_MEMPOOL_RBF_POLICY{RBFPolicy::OptIn}; namespace kernel { /** @@ -54,7 +56,7 @@ struct MemPoolOptions { std::optional 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}; MemPoolLimits limits{}; }; } // namespace kernel diff --git a/src/node/mempool_args.cpp b/src/node/mempool_args.cpp index a0a2e43107..740bcb76b7 100644 --- a/src/node/mempool_args.cpp +++ b/src/node/mempool_args.cpp @@ -10,6 +10,7 @@ #include #include #include +#include #include #include #include