diff options
Diffstat (limited to 'src')
45 files changed, 673 insertions, 232 deletions
diff --git a/src/Makefile.am b/src/Makefile.am index 2562c4cc65..4a1973aa87 100644 --- a/src/Makefile.am +++ b/src/Makefile.am @@ -196,6 +196,7 @@ BITCOIN_CORE_H = \ kernel/messagestartchars.h \ kernel/notifications_interface.h \ kernel/validation_cache_sizes.h \ + kernel/warning.h \ key.h \ key_io.h \ logging.h \ @@ -239,6 +240,7 @@ BITCOIN_CORE_H = \ node/types.h \ node/utxo_snapshot.h \ node/validation_cache_args.h \ + node/warnings.h \ noui.h \ outputtype.h \ policy/v3_policy.h \ @@ -367,7 +369,6 @@ BITCOIN_CORE_H = \ wallet/wallettool.h \ wallet/walletutil.h \ walletinitinterface.h \ - warnings.h \ zmq/zmqabstractnotifier.h \ zmq/zmqnotificationinterface.h \ zmq/zmqpublishnotifier.h \ @@ -444,6 +445,7 @@ libbitcoin_node_a_SOURCES = \ node/txreconciliation.cpp \ node/utxo_snapshot.cpp \ node/validation_cache_args.cpp \ + node/warnings.cpp \ noui.cpp \ policy/v3_policy.cpp \ policy/fees.cpp \ @@ -721,7 +723,6 @@ libbitcoin_common_a_SOURCES = \ script/sign.cpp \ script/signingprovider.cpp \ script/solver.cpp \ - warnings.cpp \ $(BITCOIN_CORE_H) # @@ -996,8 +997,7 @@ libbitcoinkernel_la_SOURCES = \ util/tokenpipe.cpp \ validation.cpp \ validationinterface.cpp \ - versionbits.cpp \ - warnings.cpp + versionbits.cpp # Required for obj/build.h to be generated first. # More details: https://www.gnu.org/software/automake/manual/html_node/Built-Sources-Example.html diff --git a/src/Makefile.test.include b/src/Makefile.test.include index fa7cadb726..633d0776f5 100644 --- a/src/Makefile.test.include +++ b/src/Makefile.test.include @@ -118,6 +118,7 @@ BITCOIN_TESTS =\ test/net_peer_eviction_tests.cpp \ test/net_tests.cpp \ test/netbase_tests.cpp \ + test/node_warnings_tests.cpp \ test/orphanage_tests.cpp \ test/peerman_tests.cpp \ test/pmt_tests.cpp \ diff --git a/src/bitcoin-chainstate.cpp b/src/bitcoin-chainstate.cpp index ca4434a882..ecbdcd48bb 100644 --- a/src/bitcoin-chainstate.cpp +++ b/src/bitcoin-chainstate.cpp @@ -16,6 +16,7 @@ #include <kernel/checks.h> #include <kernel/context.h> #include <kernel/validation_cache_sizes.h> +#include <kernel/warning.h> #include <consensus/validation.h> #include <core_io.h> @@ -28,6 +29,7 @@ #include <util/fs.h> #include <util/signalinterrupt.h> #include <util/task_runner.h> +#include <util/translation.h> #include <validation.h> #include <validationinterface.h> @@ -86,9 +88,13 @@ int main(int argc, char* argv[]) { std::cout << "Progress: " << title.original << ", " << progress_percent << ", " << resume_possible << std::endl; } - void warning(const bilingual_str& warning) override + void warningSet(kernel::Warning id, const bilingual_str& message) override { - std::cout << "Warning: " << warning.original << std::endl; + std::cout << "Warning " << static_cast<int>(id) << " set: " << message.original << std::endl; + } + void warningUnset(kernel::Warning id) override + { + std::cout << "Warning " << static_cast<int>(id) << " unset" << std::endl; } void flushError(const bilingual_str& message) override { diff --git a/src/bitcoind.cpp b/src/bitcoind.cpp index 0b89aa42af..a09bb5c9da 100644 --- a/src/bitcoind.cpp +++ b/src/bitcoind.cpp @@ -17,6 +17,7 @@ #include <kernel/context.h> #include <node/context.h> #include <node/interface_ui.h> +#include <node/warnings.h> #include <noui.h> #include <util/check.h> #include <util/exception.h> @@ -181,6 +182,8 @@ static bool AppInit(NodeContext& node) return false; } + node.warnings = std::make_unique<node::Warnings>(); + node.kernel = std::make_unique<kernel::Context>(); node.ecc_context = std::make_unique<ECC_Context>(); if (!AppInitSanityChecks(*node.kernel)) diff --git a/src/coins.cpp b/src/coins.cpp index b62653e0de..a4e4d4ad32 100644 --- a/src/coins.cpp +++ b/src/coins.cpp @@ -361,7 +361,7 @@ static bool ExecuteBackedWrapper(Func func, const std::vector<std::function<void for (const auto& f : err_callbacks) { f(); } - LogPrintf("Error reading from database: %s\n", e.what()); + LogError("Error reading from database: %s\n", e.what()); // Starting the shutdown sequence and returning false to the caller would be // interpreted as 'entry not found' (as opposed to unable to read data), and // could lead to invalid interpretation. Just exit immediately, as we can't diff --git a/src/index/base.cpp b/src/index/base.cpp index e66c89f9e4..955d7b67c9 100644 --- a/src/index/base.cpp +++ b/src/index/base.cpp @@ -17,7 +17,6 @@ #include <util/thread.h> #include <util/translation.h> #include <validation.h> // For g_chainman -#include <warnings.h> #include <string> #include <utility> @@ -31,7 +30,7 @@ template <typename... Args> void BaseIndex::FatalErrorf(const char* fmt, const Args&... args) { auto message = tfm::format(fmt, args...); - node::AbortNode(m_chain->context()->shutdown, m_chain->context()->exit_status, Untranslated(message)); + node::AbortNode(m_chain->context()->shutdown, m_chain->context()->exit_status, Untranslated(message), m_chain->context()->warnings.get()); } CBlockLocator GetLocator(interfaces::Chain& chain, const uint256& block_hash) diff --git a/src/init.cpp b/src/init.cpp index dec391bb49..5bb82dc320 100644 --- a/src/init.cpp +++ b/src/init.cpp @@ -409,7 +409,7 @@ static void HandleSIGHUP(int) static BOOL WINAPI consoleCtrlHandler(DWORD dwCtrlType) { if (!(*Assert(g_shutdown))()) { - LogPrintf("Error: failed to send shutdown signal on Ctrl-C\n"); + LogError("Failed to send shutdown signal on Ctrl-C\n"); return false; } Sleep(INFINITE); @@ -840,7 +840,7 @@ std::set<BlockFilterType> g_enabled_filter_types; // Since LogPrintf may itself allocate memory, set the handler directly // to terminate first. std::set_new_handler(std::terminate); - LogPrintf("Error: Out of memory. Terminating.\n"); + LogError("Out of memory. Terminating.\n"); // The log was successful, terminate now. std::terminate(); @@ -1175,9 +1175,9 @@ bool AppInitMain(NodeContext& node, interfaces::BlockAndHeaderTipInfo* tip_info) scheduler.scheduleEvery([&args, &node]{ constexpr uint64_t min_disk_space = 50 << 20; // 50 MB if (!CheckDiskSpace(args.GetBlocksDirPath(), min_disk_space)) { - LogPrintf("Shutting down due to lack of disk space!\n"); + LogError("Shutting down due to lack of disk space!\n"); if (!(*Assert(node.shutdown))()) { - LogPrintf("Error: failed to send shutdown signal after disk space check\n"); + LogError("Failed to send shutdown signal after disk space check\n"); } } }, std::chrono::minutes{5}); @@ -1486,7 +1486,7 @@ bool AppInitMain(NodeContext& node, interfaces::BlockAndHeaderTipInfo* tip_info) // ********************************************************* Step 7: load block chain - node.notifications = std::make_unique<KernelNotifications>(*Assert(node.shutdown), node.exit_status); + node.notifications = std::make_unique<KernelNotifications>(*Assert(node.shutdown), node.exit_status, *Assert(node.warnings)); ReadNotificationArgs(args, *node.notifications); ChainstateManager::Options chainman_opts{ .chainparams = chainparams, @@ -1582,7 +1582,7 @@ bool AppInitMain(NodeContext& node, interfaces::BlockAndHeaderTipInfo* tip_info) try { return f(); } catch (const std::exception& e) { - LogPrintf("%s\n", e.what()); + LogError("%s\n", e.what()); return std::make_tuple(node::ChainstateLoadStatus::FAILURE, _("Error opening block database")); } }; @@ -1614,10 +1614,10 @@ bool AppInitMain(NodeContext& node, interfaces::BlockAndHeaderTipInfo* tip_info) if (fRet) { do_reindex = true; if (!Assert(node.shutdown)->reset()) { - LogPrintf("Internal error: failed to reset shutdown signal.\n"); + LogError("Internal error: failed to reset shutdown signal.\n"); } } else { - LogPrintf("Aborted block database rebuild. Exiting.\n"); + LogError("Aborted block database rebuild. Exiting.\n"); return false; } } else { @@ -1639,7 +1639,8 @@ bool AppInitMain(NodeContext& node, interfaces::BlockAndHeaderTipInfo* tip_info) assert(!node.peerman); node.peerman = PeerManager::make(*node.connman, *node.addrman, node.banman.get(), chainman, - *node.mempool, peerman_opts); + *node.mempool, *node.warnings, + peerman_opts); validation_signals.RegisterValidationInterface(node.peerman.get()); // ********************************************************* Step 8: start indexers @@ -1752,7 +1753,7 @@ bool AppInitMain(NodeContext& node, interfaces::BlockAndHeaderTipInfo* tip_info) if (args.GetBoolArg("-stopafterblockimport", DEFAULT_STOPAFTERBLOCKIMPORT)) { LogPrintf("Stopping after block import\n"); if (!(*Assert(node.shutdown))()) { - LogPrintf("Error: failed to send shutdown signal after finishing block import\n"); + LogError("Failed to send shutdown signal after finishing block import\n"); } return; } diff --git a/src/kernel/notifications_interface.h b/src/kernel/notifications_interface.h index 7283a88e86..8e090dd7db 100644 --- a/src/kernel/notifications_interface.h +++ b/src/kernel/notifications_interface.h @@ -16,6 +16,8 @@ namespace kernel { //! Result type for use with std::variant to indicate that an operation should be interrupted. struct Interrupted{}; +enum class Warning; + //! Simple result type for functions that need to propagate an interrupt status and don't have other return values. using InterruptResult = std::variant<std::monostate, Interrupted>; @@ -38,7 +40,8 @@ public: [[nodiscard]] virtual InterruptResult blockTip(SynchronizationState state, CBlockIndex& index) { return {}; } virtual void headerTip(SynchronizationState state, int64_t height, int64_t timestamp, bool presync) {} virtual void progress(const bilingual_str& title, int progress_percent, bool resume_possible) {} - virtual void warning(const bilingual_str& warning) {} + virtual void warningSet(Warning id, const bilingual_str& message) {} + virtual void warningUnset(Warning id) {} //! The flush error notification is sent to notify the user that an error //! occurred while flushing block data to disk. Kernel code may ignore flush diff --git a/src/kernel/warning.h b/src/kernel/warning.h new file mode 100644 index 0000000000..453f36c552 --- /dev/null +++ b/src/kernel/warning.h @@ -0,0 +1,14 @@ +// Copyright (c) 2024-present The Bitcoin Core developers +// Distributed under the MIT software license, see the accompanying +// file COPYING or http://www.opensource.org/licenses/mit-license.php. + +#ifndef BITCOIN_KERNEL_WARNING_H +#define BITCOIN_KERNEL_WARNING_H + +namespace kernel { +enum class Warning { + UNKNOWN_NEW_RULES_ACTIVATED, + LARGE_WORK_INVALID_CHAIN, +}; +} // namespace kernel +#endif // BITCOIN_KERNEL_WARNING_H diff --git a/src/leveldb/include/leveldb/status.h b/src/leveldb/include/leveldb/status.h index e3273144e4..68efe3001a 100644 --- a/src/leveldb/include/leveldb/status.h +++ b/src/leveldb/include/leveldb/status.h @@ -103,6 +103,8 @@ class LEVELDB_EXPORT Status { inline Status::Status(const Status& rhs) { state_ = (rhs.state_ == nullptr) ? nullptr : CopyState(rhs.state_); } + +// NOLINTBEGIN(bugprone-unhandled-self-assignment) inline Status& Status::operator=(const Status& rhs) { // The following condition catches both aliasing (when this == &rhs), // and the common case where both rhs and *this are ok. @@ -112,6 +114,8 @@ inline Status& Status::operator=(const Status& rhs) { } return *this; } +// NOLINTEND(bugprone-unhandled-self-assignment) + inline Status& Status::operator=(Status&& rhs) noexcept { std::swap(state_, rhs.state_); return *this; diff --git a/src/net_processing.cpp b/src/net_processing.cpp index 6374cb52c1..795b798f9c 100644 --- a/src/net_processing.cpp +++ b/src/net_processing.cpp @@ -25,6 +25,7 @@ #include <node/blockstorage.h> #include <node/timeoffsets.h> #include <node/txreconciliation.h> +#include <node/warnings.h> #include <policy/fees.h> #include <policy/policy.h> #include <policy/settings.h> @@ -489,7 +490,7 @@ class PeerManagerImpl final : public PeerManager public: PeerManagerImpl(CConnman& connman, AddrMan& addrman, BanMan* banman, ChainstateManager& chainman, - CTxMemPool& pool, Options opts); + CTxMemPool& pool, node::Warnings& warnings, Options opts); /** Overridden from CValidationInterface. */ void BlockConnected(ChainstateRole role, const std::shared_ptr<const CBlock>& pblock, const CBlockIndex* pindexConnected) override @@ -790,7 +791,8 @@ private: /** Next time to check for stale tip */ std::chrono::seconds m_stale_tip_check_time GUARDED_BY(cs_main){0s}; - TimeOffsets m_outbound_time_offsets; + node::Warnings& m_warnings; + TimeOffsets m_outbound_time_offsets{m_warnings}; const Options m_opts; @@ -2042,14 +2044,14 @@ std::optional<std::string> PeerManagerImpl::FetchBlock(NodeId peer_id, const CBl std::unique_ptr<PeerManager> PeerManager::make(CConnman& connman, AddrMan& addrman, BanMan* banman, ChainstateManager& chainman, - CTxMemPool& pool, Options opts) + CTxMemPool& pool, node::Warnings& warnings, Options opts) { - return std::make_unique<PeerManagerImpl>(connman, addrman, banman, chainman, pool, opts); + return std::make_unique<PeerManagerImpl>(connman, addrman, banman, chainman, pool, warnings, opts); } PeerManagerImpl::PeerManagerImpl(CConnman& connman, AddrMan& addrman, BanMan* banman, ChainstateManager& chainman, - CTxMemPool& pool, Options opts) + CTxMemPool& pool, node::Warnings& warnings, Options opts) : m_rng{opts.deterministic_rng}, m_fee_filter_rounder{CFeeRate{DEFAULT_MIN_RELAY_TX_FEE}, m_rng}, m_chainparams(chainman.GetParams()), @@ -2058,6 +2060,7 @@ PeerManagerImpl::PeerManagerImpl(CConnman& connman, AddrMan& addrman, m_banman(banman), m_chainman(chainman), m_mempool(pool), + m_warnings{warnings}, m_opts{opts} { // While Erlay support is incomplete, it must be enabled explicitly via -txreconciliation. diff --git a/src/net_processing.h b/src/net_processing.h index 85e399d948..e6430d390e 100644 --- a/src/net_processing.h +++ b/src/net_processing.h @@ -16,6 +16,10 @@ class CChainParams; class CTxMemPool; class ChainstateManager; +namespace node { +class Warnings; +} // namespace node + /** Whether transaction reconciliation protocol should be enabled by default. */ static constexpr bool DEFAULT_TXRECONCILIATION_ENABLE{false}; /** Default for -maxorphantx, maximum number of orphan transactions kept in memory */ @@ -73,7 +77,7 @@ public: static std::unique_ptr<PeerManager> make(CConnman& connman, AddrMan& addrman, BanMan* banman, ChainstateManager& chainman, - CTxMemPool& pool, Options opts); + CTxMemPool& pool, node::Warnings& warnings, Options opts); virtual ~PeerManager() { } /** diff --git a/src/node/abort.cpp b/src/node/abort.cpp index b727608384..8a17c41fd2 100644 --- a/src/node/abort.cpp +++ b/src/node/abort.cpp @@ -6,19 +6,18 @@ #include <logging.h> #include <node/interface_ui.h> +#include <node/warnings.h> #include <util/signalinterrupt.h> #include <util/translation.h> -#include <warnings.h> #include <atomic> #include <cstdlib> -#include <string> namespace node { -void AbortNode(util::SignalInterrupt* shutdown, std::atomic<int>& exit_status, const bilingual_str& message) +void AbortNode(util::SignalInterrupt* shutdown, std::atomic<int>& exit_status, const bilingual_str& message, node::Warnings* warnings) { - SetMiscWarning(message); + if (warnings) warnings->Set(Warning::FATAL_INTERNAL_ERROR, message); InitError(_("A fatal internal error occurred, see debug.log for details: ") + message); exit_status.store(EXIT_FAILURE); if (shutdown && !(*shutdown)()) { diff --git a/src/node/abort.h b/src/node/abort.h index 1092279142..c881af4634 100644 --- a/src/node/abort.h +++ b/src/node/abort.h @@ -14,7 +14,8 @@ class SignalInterrupt; } // namespace util namespace node { -void AbortNode(util::SignalInterrupt* shutdown, std::atomic<int>& exit_status, const bilingual_str& message); +class Warnings; +void AbortNode(util::SignalInterrupt* shutdown, std::atomic<int>& exit_status, const bilingual_str& message, node::Warnings* warnings); } // namespace node #endif // BITCOIN_NODE_ABORT_H diff --git a/src/node/context.cpp b/src/node/context.cpp index e32d21b383..da05fde6ee 100644 --- a/src/node/context.cpp +++ b/src/node/context.cpp @@ -13,6 +13,7 @@ #include <net_processing.h> #include <netgroup.h> #include <node/kernel_notifications.h> +#include <node/warnings.h> #include <policy/fees.h> #include <scheduler.h> #include <txmempool.h> diff --git a/src/node/context.h b/src/node/context.h index a7d92989dd..77838ea99b 100644 --- a/src/node/context.h +++ b/src/node/context.h @@ -39,6 +39,7 @@ class SignalInterrupt; namespace node { class KernelNotifications; +class Warnings; //! NodeContext struct containing references to chain state and connection //! state. @@ -81,6 +82,8 @@ struct NodeContext { //! Issues calls about blocks and transactions std::unique_ptr<ValidationSignals> validation_signals; std::atomic<int> exit_status{EXIT_SUCCESS}; + //! Manages all the node warnings + std::unique_ptr<node::Warnings> warnings; //! Declare default constructor and destructor that are not inline, so code //! instantiating the NodeContext struct doesn't need to #include class diff --git a/src/node/interfaces.cpp b/src/node/interfaces.cpp index 19f4aaf9c4..2b36f4ceae 100644 --- a/src/node/interfaces.cpp +++ b/src/node/interfaces.cpp @@ -32,6 +32,7 @@ #include <node/mini_miner.h> #include <node/transaction.h> #include <node/types.h> +#include <node/warnings.h> #include <policy/feerate.h> #include <policy/fees.h> #include <policy/policy.h> @@ -53,7 +54,6 @@ #include <util/translation.h> #include <validation.h> #include <validationinterface.h> -#include <warnings.h> #include <config/bitcoin-config.h> // IWYU pragma: keep @@ -93,7 +93,7 @@ public: explicit NodeImpl(NodeContext& context) { setContext(&context); } void initLogging() override { InitLogging(args()); } void initParameterInteraction() override { InitParameterInteraction(args()); } - bilingual_str getWarnings() override { return Join(GetWarnings(), Untranslated("<hr />")); } + bilingual_str getWarnings() override { return Join(Assert(m_context->warnings)->GetMessages(), Untranslated("<hr />")); } int getExitStatus() override { return Assert(m_context)->exit_status.load(); } uint32_t getLogCategories() override { return LogInstance().GetCategoryMask(); } bool baseInitialize() override @@ -101,6 +101,7 @@ public: if (!AppInitBasicSetup(args(), Assert(context())->exit_status)) return false; if (!AppInitParameterInteraction(args())) return false; + m_context->warnings = std::make_unique<node::Warnings>(); m_context->kernel = std::make_unique<kernel::Context>(); m_context->ecc_context = std::make_unique<ECC_Context>(); if (!AppInitSanityChecks(*m_context->kernel)) return false; @@ -125,7 +126,7 @@ public: void startShutdown() override { if (!(*Assert(Assert(m_context)->shutdown))()) { - LogPrintf("Error: failed to send shutdown signal\n"); + LogError("Failed to send shutdown signal\n"); } // Stop RPC for clean shutdown if any of waitfor* commands is executed. if (args().GetBoolArg("-server", false)) { diff --git a/src/node/kernel_notifications.cpp b/src/node/kernel_notifications.cpp index 1f07014ee2..9894052a3a 100644 --- a/src/node/kernel_notifications.cpp +++ b/src/node/kernel_notifications.cpp @@ -10,15 +10,16 @@ #include <common/args.h> #include <common/system.h> #include <kernel/context.h> +#include <kernel/warning.h> #include <logging.h> #include <node/abort.h> #include <node/interface_ui.h> +#include <node/warnings.h> #include <util/check.h> #include <util/signalinterrupt.h> #include <util/strencodings.h> #include <util/string.h> #include <util/translation.h> -#include <warnings.h> #include <cstdint> #include <string> @@ -28,7 +29,6 @@ using util::ReplaceAll; static void AlertNotify(const std::string& strMessage) { - uiInterface.NotifyAlertChanged(); #if HAVE_SYSTEM std::string strCmd = gArgs.GetArg("-alertnotify", ""); if (strCmd.empty()) return; @@ -46,16 +46,6 @@ static void AlertNotify(const std::string& strMessage) #endif } -static void DoWarning(const bilingual_str& warning) -{ - static bool fWarned = false; - SetMiscWarning(warning); - if (!fWarned) { - AlertNotify(warning.original); - fWarned = true; - } -} - namespace node { kernel::InterruptResult KernelNotifications::blockTip(SynchronizationState state, CBlockIndex& index) @@ -63,7 +53,7 @@ kernel::InterruptResult KernelNotifications::blockTip(SynchronizationState state uiInterface.NotifyBlockTip(state, &index); if (m_stop_at_height && index.nHeight >= m_stop_at_height) { if (!m_shutdown()) { - LogPrintf("Error: failed to send shutdown signal after reaching stop height\n"); + LogError("Failed to send shutdown signal after reaching stop height\n"); } return kernel::Interrupted{}; } @@ -80,20 +70,27 @@ void KernelNotifications::progress(const bilingual_str& title, int progress_perc uiInterface.ShowProgress(title.translated, progress_percent, resume_possible); } -void KernelNotifications::warning(const bilingual_str& warning) +void KernelNotifications::warningSet(kernel::Warning id, const bilingual_str& message) +{ + if (m_warnings.Set(id, message)) { + AlertNotify(message.original); + } +} + +void KernelNotifications::warningUnset(kernel::Warning id) { - DoWarning(warning); + m_warnings.Unset(id); } void KernelNotifications::flushError(const bilingual_str& message) { - AbortNode(&m_shutdown, m_exit_status, message); + AbortNode(&m_shutdown, m_exit_status, message, &m_warnings); } void KernelNotifications::fatalError(const bilingual_str& message) { node::AbortNode(m_shutdown_on_fatal_error ? &m_shutdown : nullptr, - m_exit_status, message); + m_exit_status, message, &m_warnings); } void ReadNotificationArgs(const ArgsManager& args, KernelNotifications& notifications) diff --git a/src/node/kernel_notifications.h b/src/node/kernel_notifications.h index f4d97a0fff..e37f4d4e1e 100644 --- a/src/node/kernel_notifications.h +++ b/src/node/kernel_notifications.h @@ -15,18 +15,24 @@ class CBlockIndex; enum class SynchronizationState; struct bilingual_str; +namespace kernel { +enum class Warning; +} // namespace kernel + namespace util { class SignalInterrupt; } // namespace util namespace node { +class Warnings; static constexpr int DEFAULT_STOPATHEIGHT{0}; class KernelNotifications : public kernel::Notifications { public: - KernelNotifications(util::SignalInterrupt& shutdown, std::atomic<int>& exit_status) : m_shutdown(shutdown), m_exit_status{exit_status} {} + KernelNotifications(util::SignalInterrupt& shutdown, std::atomic<int>& exit_status, node::Warnings& warnings) + : m_shutdown(shutdown), m_exit_status{exit_status}, m_warnings{warnings} {} [[nodiscard]] kernel::InterruptResult blockTip(SynchronizationState state, CBlockIndex& index) override; @@ -34,7 +40,9 @@ public: void progress(const bilingual_str& title, int progress_percent, bool resume_possible) override; - void warning(const bilingual_str& warning) override; + void warningSet(kernel::Warning id, const bilingual_str& message) override; + + void warningUnset(kernel::Warning id) override; void flushError(const bilingual_str& message) override; @@ -47,6 +55,7 @@ public: private: util::SignalInterrupt& m_shutdown; std::atomic<int>& m_exit_status; + node::Warnings& m_warnings; }; void ReadNotificationArgs(const ArgsManager& args, KernelNotifications& notifications); diff --git a/src/node/timeoffsets.cpp b/src/node/timeoffsets.cpp index 62f527be8a..002c00d245 100644 --- a/src/node/timeoffsets.cpp +++ b/src/node/timeoffsets.cpp @@ -3,13 +3,12 @@ // file COPYING or http://www.opensource.org/licenses/mit-license.php. #include <logging.h> -#include <node/interface_ui.h> #include <node/timeoffsets.h> +#include <node/warnings.h> #include <sync.h> #include <tinyformat.h> #include <util/time.h> #include <util/translation.h> -#include <warnings.h> #include <algorithm> #include <chrono> @@ -49,8 +48,7 @@ bool TimeOffsets::WarnIfOutOfSync() const // when median == std::numeric_limits<int64_t>::min(), calling std::chrono::abs is UB auto median{std::max(Median(), std::chrono::seconds(std::numeric_limits<int64_t>::min() + 1))}; if (std::chrono::abs(median) <= WARN_THRESHOLD) { - SetMedianTimeOffsetWarning(std::nullopt); - uiInterface.NotifyAlertChanged(); + m_warnings.Unset(node::Warning::CLOCK_OUT_OF_SYNC); return false; } @@ -63,7 +61,6 @@ bool TimeOffsets::WarnIfOutOfSync() const "RPC methods to get more info." ), Ticks<std::chrono::minutes>(WARN_THRESHOLD))}; LogWarning("%s\n", msg.original); - SetMedianTimeOffsetWarning(msg); - uiInterface.NotifyAlertChanged(); + m_warnings.Set(node::Warning::CLOCK_OUT_OF_SYNC, msg); return true; } diff --git a/src/node/timeoffsets.h b/src/node/timeoffsets.h index 2b12584e12..eba706ac1e 100644 --- a/src/node/timeoffsets.h +++ b/src/node/timeoffsets.h @@ -11,8 +11,16 @@ #include <cstddef> #include <deque> +namespace node { +class Warnings; +} // namespace node + class TimeOffsets { +public: + TimeOffsets(node::Warnings& warnings) : m_warnings{warnings} {} + +private: //! Maximum number of timeoffsets stored. static constexpr size_t MAX_SIZE{50}; //! Minimum difference between system and network time for a warning to be raised. @@ -23,6 +31,8 @@ class TimeOffsets * positive offset means our peer's clock is ahead of our local clock. */ std::deque<std::chrono::seconds> m_offsets GUARDED_BY(m_mutex){}; + node::Warnings& m_warnings; + public: /** Add a new time offset sample. */ void Add(std::chrono::seconds offset) EXCLUSIVE_LOCKS_REQUIRED(!m_mutex); diff --git a/src/node/warnings.cpp b/src/node/warnings.cpp new file mode 100644 index 0000000000..b99c845900 --- /dev/null +++ b/src/node/warnings.cpp @@ -0,0 +1,68 @@ +// Copyright (c) 2009-2010 Satoshi Nakamoto +// Copyright (c) 2009-2022 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 <config/bitcoin-config.h> // IWYU pragma: keep + +#include <node/warnings.h> + +#include <common/system.h> +#include <node/interface_ui.h> +#include <sync.h> +#include <univalue.h> +#include <util/translation.h> + +#include <utility> +#include <vector> + +namespace node { +Warnings::Warnings() +{ + // Pre-release build warning + if (!CLIENT_VERSION_IS_RELEASE) { + m_warnings.insert( + {Warning::PRE_RELEASE_TEST_BUILD, + _("This is a pre-release test build - use at your own risk - do not use for mining or merchant applications")}); + } +} +bool Warnings::Set(warning_type id, bilingual_str message) +{ + LOCK(m_mutex); + const auto& [_, inserted]{m_warnings.insert({id, std::move(message)})}; + if (inserted) uiInterface.NotifyAlertChanged(); + return inserted; +} + +bool Warnings::Unset(warning_type id) +{ + auto success{WITH_LOCK(m_mutex, return m_warnings.erase(id))}; + if (success) uiInterface.NotifyAlertChanged(); + return success; +} + +std::vector<bilingual_str> Warnings::GetMessages() const +{ + LOCK(m_mutex); + std::vector<bilingual_str> messages; + messages.reserve(m_warnings.size()); + for (const auto& [id, msg] : m_warnings) { + messages.push_back(msg); + } + return messages; +} + +UniValue GetWarningsForRpc(const Warnings& warnings, bool use_deprecated) +{ + if (use_deprecated) { + const auto all_messages{warnings.GetMessages()}; + return all_messages.empty() ? "" : all_messages.back().original; + } + + UniValue messages{UniValue::VARR}; + for (auto&& message : warnings.GetMessages()) { + messages.push_back(std::move(message.original)); + } + return messages; +} +} // namespace node diff --git a/src/node/warnings.h b/src/node/warnings.h new file mode 100644 index 0000000000..24aeb8a922 --- /dev/null +++ b/src/node/warnings.h @@ -0,0 +1,90 @@ +// Copyright (c) 2009-2010 Satoshi Nakamoto +// Copyright (c) 2009-2021 The Bitcoin Core developers +// Distributed under the MIT software license, see the accompanying +// file COPYING or http://www.opensource.org/licenses/mit-license.php. + +#ifndef BITCOIN_NODE_WARNINGS_H +#define BITCOIN_NODE_WARNINGS_H + +#include <sync.h> +#include <util/translation.h> + +#include <map> +#include <variant> +#include <vector> + +class UniValue; + +namespace kernel { +enum class Warning; +} // namespace kernel + +namespace node { +enum class Warning { + CLOCK_OUT_OF_SYNC, + PRE_RELEASE_TEST_BUILD, + FATAL_INTERNAL_ERROR, +}; + +/** + * @class Warnings + * @brief Manages warning messages within a node. + * + * The Warnings class provides mechanisms to set, unset, and retrieve + * warning messages. It updates the GUI when warnings are changed. + * + * This class is designed to be non-copyable to ensure warnings + * are managed centrally. + */ +class Warnings +{ + typedef std::variant<kernel::Warning, node::Warning> warning_type; + + mutable Mutex m_mutex; + std::map<warning_type, bilingual_str> m_warnings GUARDED_BY(m_mutex); + +public: + Warnings(); + //! A warnings instance should always be passed by reference, never copied. + Warnings(const Warnings&) = delete; + Warnings& operator=(const Warnings&) = delete; + /** + * @brief Set a warning message. If a warning with the specified + * `id` is already active, false is returned and the new + * warning is ignored. If `id` does not yet exist, the + * warning is set, the UI is updated, and true is returned. + * + * @param[in] id Unique identifier of the warning. + * @param[in] message Warning message to be shown. + * + * @returns true if the warning was indeed set (i.e. there is no + * active warning with this `id`), otherwise false. + */ + bool Set(warning_type id, bilingual_str message) EXCLUSIVE_LOCKS_REQUIRED(!m_mutex); + /** + * @brief Unset a warning message. If a warning with the specified + * `id` is active, it is unset, the UI is updated, and true + * is returned. Otherwise, no warning is unset and false is + * returned. + * + * @param[in] id Unique identifier of the warning. + * + * @returns true if the warning was indeed unset (i.e. there is an + * active warning with this `id`), otherwise false. + */ + bool Unset(warning_type id) EXCLUSIVE_LOCKS_REQUIRED(!m_mutex); + /** Return potential problems detected by the node, sorted by the + * warning_type id */ + std::vector<bilingual_str> GetMessages() const EXCLUSIVE_LOCKS_REQUIRED(!m_mutex); +}; + +/** + * RPC helper function that wraps warnings.GetMessages(). + * + * Returns a UniValue::VSTR with the latest warning if use_deprecated is + * set to true, or a UniValue::VARR with all warnings otherwise. + */ +UniValue GetWarningsForRpc(const Warnings& warnings, bool use_deprecated); +} // namespace node + +#endif // BITCOIN_NODE_WARNINGS_H diff --git a/src/policy/v3_policy.cpp b/src/policy/v3_policy.cpp index bcf0b2b236..6bd043b8e3 100644 --- a/src/policy/v3_policy.cpp +++ b/src/policy/v3_policy.cpp @@ -91,7 +91,6 @@ std::optional<std::string> PackageV3Checks(const CTransactionRef& ptx, int64_t v const auto parent_info = [&] { if (mempool_ancestors.size() > 0) { auto& mempool_parent = *mempool_ancestors.begin(); - Assume(mempool_parent->GetCountWithDescendants() == 1); return ParentInfo{mempool_parent->GetTx().GetHash(), mempool_parent->GetTx().GetWitnessHash(), mempool_parent->GetTx().version, @@ -135,10 +134,7 @@ std::optional<std::string> PackageV3Checks(const CTransactionRef& ptx, int64_t v } } - // It shouldn't be possible to have any mempool siblings at this point. SingleV3Checks - // catches mempool siblings and sibling eviction is not extended to packages. Also, if the package consists of connected transactions, - // any tx having a mempool ancestor would mean the package exceeds ancestor limits. - if (!Assume(!parent_info.m_has_mempool_descendant)) { + if (parent_info.m_has_mempool_descendant) { return strprintf("tx %s (wtxid=%s) would exceed descendant count limit", parent_info.m_txid.ToString(), parent_info.m_wtxid.ToString()); } diff --git a/src/rpc/blockchain.cpp b/src/rpc/blockchain.cpp index 46b0ae161f..e785678614 100644 --- a/src/rpc/blockchain.cpp +++ b/src/rpc/blockchain.cpp @@ -29,6 +29,7 @@ #include <node/context.h> #include <node/transaction.h> #include <node/utxo_snapshot.h> +#include <node/warnings.h> #include <primitives/transaction.h> #include <rpc/server.h> #include <rpc/server_util.h> @@ -1308,7 +1309,8 @@ RPCHelpMan getblockchaininfo() } } - obj.pushKV("warnings", GetNodeWarnings(IsDeprecatedRPCEnabled("warnings"))); + NodeContext& node = EnsureAnyNodeContext(request.context); + obj.pushKV("warnings", node::GetWarningsForRpc(*CHECK_NONFATAL(node.warnings), IsDeprecatedRPCEnabled("warnings"))); return obj; }, }; diff --git a/src/rpc/mining.cpp b/src/rpc/mining.cpp index 6412fb35ec..0f6853ef37 100644 --- a/src/rpc/mining.cpp +++ b/src/rpc/mining.cpp @@ -20,6 +20,7 @@ #include <net.h> #include <node/context.h> #include <node/miner.h> +#include <node/warnings.h> #include <pow.h> #include <rpc/blockchain.h> #include <rpc/mining.h> @@ -454,7 +455,7 @@ static RPCHelpMan getmininginfo() obj.pushKV("networkhashps", getnetworkhashps().HandleRequest(request)); obj.pushKV("pooledtx", (uint64_t)mempool.size()); obj.pushKV("chain", chainman.GetParams().GetChainTypeString()); - obj.pushKV("warnings", GetNodeWarnings(IsDeprecatedRPCEnabled("warnings"))); + obj.pushKV("warnings", node::GetWarningsForRpc(*CHECK_NONFATAL(node.warnings), IsDeprecatedRPCEnabled("warnings"))); return obj; }, }; diff --git a/src/rpc/net.cpp b/src/rpc/net.cpp index 1cc55f891a..1119a3e668 100644 --- a/src/rpc/net.cpp +++ b/src/rpc/net.cpp @@ -16,6 +16,7 @@ #include <netbase.h> #include <node/context.h> #include <node/protocol_version.h> +#include <node/warnings.h> #include <policy/settings.h> #include <protocol.h> #include <rpc/blockchain.h> @@ -715,7 +716,7 @@ static RPCHelpMan getnetworkinfo() } } obj.pushKV("localaddresses", std::move(localAddresses)); - obj.pushKV("warnings", GetNodeWarnings(IsDeprecatedRPCEnabled("warnings"))); + obj.pushKV("warnings", node::GetWarningsForRpc(*CHECK_NONFATAL(node.warnings), IsDeprecatedRPCEnabled("warnings"))); return obj; }, }; diff --git a/src/rpc/util.cpp b/src/rpc/util.cpp index bb1aef63f4..4df4466c49 100644 --- a/src/rpc/util.cpp +++ b/src/rpc/util.cpp @@ -5,17 +5,17 @@ #include <config/bitcoin-config.h> // IWYU pragma: keep #include <clientversion.h> -#include <core_io.h> #include <common/args.h> #include <common/messages.h> #include <common/types.h> #include <consensus/amount.h> -#include <script/interpreter.h> +#include <core_io.h> #include <key_io.h> #include <node/types.h> #include <outputtype.h> #include <rpc/util.h> #include <script/descriptor.h> +#include <script/interpreter.h> #include <script/signingprovider.h> #include <script/solver.h> #include <tinyformat.h> @@ -25,7 +25,6 @@ #include <util/strencodings.h> #include <util/string.h> #include <util/translation.h> -#include <warnings.h> #include <algorithm> #include <iterator> @@ -1382,17 +1381,3 @@ void PushWarnings(const std::vector<bilingual_str>& warnings, UniValue& obj) if (warnings.empty()) return; obj.pushKV("warnings", BilingualStringsToUniValue(warnings)); } - -UniValue GetNodeWarnings(bool use_deprecated) -{ - if (use_deprecated) { - const auto all_warnings{GetWarnings()}; - return all_warnings.empty() ? "" : all_warnings.back().original; - } - - UniValue warnings{UniValue::VARR}; - for (auto&& warning : GetWarnings()) { - warnings.push_back(std::move(warning.original)); - } - return warnings; -} diff --git a/src/rpc/util.h b/src/rpc/util.h index ca6ca6007b..23024376e0 100644 --- a/src/rpc/util.h +++ b/src/rpc/util.h @@ -503,6 +503,4 @@ private: void PushWarnings(const UniValue& warnings, UniValue& obj); void PushWarnings(const std::vector<bilingual_str>& warnings, UniValue& obj); -UniValue GetNodeWarnings(bool use_deprecated); - #endif // BITCOIN_RPC_UTIL_H diff --git a/src/test/blockmanager_tests.cpp b/src/test/blockmanager_tests.cpp index efe0983698..9eb7acc3ca 100644 --- a/src/test/blockmanager_tests.cpp +++ b/src/test/blockmanager_tests.cpp @@ -27,7 +27,7 @@ BOOST_FIXTURE_TEST_SUITE(blockmanager_tests, BasicTestingSetup) BOOST_AUTO_TEST_CASE(blockmanager_find_block_pos) { const auto params {CreateChainParams(ArgsManager{}, ChainType::MAIN)}; - KernelNotifications notifications{*Assert(m_node.shutdown), m_node.exit_status}; + KernelNotifications notifications{*Assert(m_node.shutdown), m_node.exit_status, *Assert(m_node.warnings)}; const BlockManager::Options blockman_opts{ .chainparams = *params, .blocks_dir = m_args.GetBlocksDirPath(), @@ -134,7 +134,7 @@ BOOST_FIXTURE_TEST_CASE(blockmanager_block_data_availability, TestChain100Setup) BOOST_AUTO_TEST_CASE(blockmanager_flush_block_file) { - KernelNotifications notifications{*Assert(m_node.shutdown), m_node.exit_status}; + KernelNotifications notifications{*Assert(m_node.shutdown), m_node.exit_status, *Assert(m_node.warnings)}; node::BlockManager::Options blockman_opts{ .chainparams = Params(), .blocks_dir = m_args.GetBlocksDirPath(), diff --git a/src/test/denialofservice_tests.cpp b/src/test/denialofservice_tests.cpp index 5e9ae78681..e493d532a5 100644 --- a/src/test/denialofservice_tests.cpp +++ b/src/test/denialofservice_tests.cpp @@ -141,7 +141,7 @@ BOOST_AUTO_TEST_CASE(stale_tip_peer_management) { NodeId id{0}; auto connman = std::make_unique<ConnmanTestMsg>(0x1337, 0x1337, *m_node.addrman, *m_node.netgroupman, Params()); - auto peerLogic = PeerManager::make(*connman, *m_node.addrman, nullptr, *m_node.chainman, *m_node.mempool, {}); + auto peerLogic = PeerManager::make(*connman, *m_node.addrman, nullptr, *m_node.chainman, *m_node.mempool, *m_node.warnings, {}); constexpr int max_outbound_full_relay = MAX_OUTBOUND_FULL_RELAY_CONNECTIONS; CConnman::Options options; @@ -239,7 +239,7 @@ BOOST_AUTO_TEST_CASE(block_relay_only_eviction) { NodeId id{0}; auto connman = std::make_unique<ConnmanTestMsg>(0x1337, 0x1337, *m_node.addrman, *m_node.netgroupman, Params()); - auto peerLogic = PeerManager::make(*connman, *m_node.addrman, nullptr, *m_node.chainman, *m_node.mempool, {}); + auto peerLogic = PeerManager::make(*connman, *m_node.addrman, nullptr, *m_node.chainman, *m_node.mempool, *m_node.warnings, {}); constexpr int max_outbound_block_relay{MAX_BLOCK_RELAY_ONLY_CONNECTIONS}; constexpr int64_t MINIMUM_CONNECT_TIME{30}; @@ -300,7 +300,7 @@ BOOST_AUTO_TEST_CASE(peer_discouragement) auto banman = std::make_unique<BanMan>(m_args.GetDataDirBase() / "banlist", nullptr, DEFAULT_MISBEHAVING_BANTIME); auto connman = std::make_unique<ConnmanTestMsg>(0x1337, 0x1337, *m_node.addrman, *m_node.netgroupman, Params()); - auto peerLogic = PeerManager::make(*connman, *m_node.addrman, banman.get(), *m_node.chainman, *m_node.mempool, {}); + auto peerLogic = PeerManager::make(*connman, *m_node.addrman, banman.get(), *m_node.chainman, *m_node.mempool, *m_node.warnings, {}); CNetAddr tor_netaddr; BOOST_REQUIRE( @@ -402,7 +402,7 @@ BOOST_AUTO_TEST_CASE(DoS_bantime) auto banman = std::make_unique<BanMan>(m_args.GetDataDirBase() / "banlist", nullptr, DEFAULT_MISBEHAVING_BANTIME); auto connman = std::make_unique<CConnman>(0x1337, 0x1337, *m_node.addrman, *m_node.netgroupman, Params()); - auto peerLogic = PeerManager::make(*connman, *m_node.addrman, banman.get(), *m_node.chainman, *m_node.mempool, {}); + auto peerLogic = PeerManager::make(*connman, *m_node.addrman, banman.get(), *m_node.chainman, *m_node.mempool, *m_node.warnings, {}); banman->ClearBanned(); int64_t nStartTime = GetTime(); diff --git a/src/test/fuzz/package_eval.cpp b/src/test/fuzz/package_eval.cpp index 122543ebad..53aedf23ea 100644 --- a/src/test/fuzz/package_eval.cpp +++ b/src/test/fuzz/package_eval.cpp @@ -314,7 +314,7 @@ FUZZ_TARGET(tx_package_eval, .init = initialize_tx_pool) // just use result_package.m_state here. This makes the expect_valid check meaningless, but // we can still verify that the contents of m_tx_results are consistent with m_state. const bool expect_valid{result_package.m_state.IsValid()}; - Assert(!CheckPackageMempoolAcceptResult(txs, result_package, expect_valid, nullptr)); + Assert(!CheckPackageMempoolAcceptResult(txs, result_package, expect_valid, &tx_pool)); } else { // This is empty if it fails early checks, or "full" if transactions are looked at deeper Assert(result_package.m_tx_results.size() == txs.size() || result_package.m_tx_results.empty()); diff --git a/src/test/fuzz/timeoffsets.cpp b/src/test/fuzz/timeoffsets.cpp index 019337a94a..dfa4dd705d 100644 --- a/src/test/fuzz/timeoffsets.cpp +++ b/src/test/fuzz/timeoffsets.cpp @@ -3,6 +3,7 @@ // file COPYING or http://www.opensource.org/licenses/mit-license.php. #include <node/timeoffsets.h> +#include <node/warnings.h> #include <test/fuzz/FuzzedDataProvider.h> #include <test/fuzz/fuzz.h> #include <test/util/setup_common.h> @@ -19,7 +20,8 @@ void initialize_timeoffsets() FUZZ_TARGET(timeoffsets, .init = initialize_timeoffsets) { FuzzedDataProvider fuzzed_data_provider(buffer.data(), buffer.size()); - TimeOffsets offsets{}; + node::Warnings warnings{}; + TimeOffsets offsets{warnings}; LIMITED_WHILE(fuzzed_data_provider.remaining_bytes() > 0, 4'000) { (void)offsets.Median(); offsets.Add(std::chrono::seconds{fuzzed_data_provider.ConsumeIntegral<std::chrono::seconds::rep>()}); diff --git a/src/test/net_peer_connection_tests.cpp b/src/test/net_peer_connection_tests.cpp index 00bc1fdb6a..5f38ce112c 100644 --- a/src/test/net_peer_connection_tests.cpp +++ b/src/test/net_peer_connection_tests.cpp @@ -84,7 +84,7 @@ static void AddPeer(NodeId& id, std::vector<CNode*>& nodes, PeerManager& peerman BOOST_AUTO_TEST_CASE(test_addnode_getaddednodeinfo_and_connection_detection) { auto connman = std::make_unique<ConnmanTestMsg>(0x1337, 0x1337, *m_node.addrman, *m_node.netgroupman, Params()); - auto peerman = PeerManager::make(*connman, *m_node.addrman, nullptr, *m_node.chainman, *m_node.mempool, {}); + auto peerman = PeerManager::make(*connman, *m_node.addrman, nullptr, *m_node.chainman, *m_node.mempool, *m_node.warnings, {}); NodeId id{0}; std::vector<CNode*> nodes; diff --git a/src/test/node_warnings_tests.cpp b/src/test/node_warnings_tests.cpp new file mode 100644 index 0000000000..2bcc2c95ed --- /dev/null +++ b/src/test/node_warnings_tests.cpp @@ -0,0 +1,52 @@ +// Copyright (c) 2024-present 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 <node/warnings.h> +#include <util/translation.h> + +#include <test/util/setup_common.h> + +#include <boost/test/unit_test.hpp> + +BOOST_FIXTURE_TEST_SUITE(node_warnings_tests, BasicTestingSetup) + +BOOST_AUTO_TEST_CASE(warnings) +{ + node::Warnings warnings; + // On pre-release builds, a warning is generated automatically + warnings.Unset(node::Warning::PRE_RELEASE_TEST_BUILD); + + // For these tests, we don't care what the exact warnings are, so + // just refer to them as warning_1 and warning_2 + const auto warning_1{node::Warning::CLOCK_OUT_OF_SYNC}; + const auto warning_2{node::Warning::FATAL_INTERNAL_ERROR}; + + // Ensure we start without any warnings + BOOST_CHECK(warnings.GetMessages().size() == 0); + // Add two warnings + BOOST_CHECK(warnings.Set(warning_1, _("warning 1"))); + BOOST_CHECK(warnings.Set(warning_2, _("warning 2"))); + // Unset the second one + BOOST_CHECK(warnings.Unset(warning_2)); + // Since it's already been unset, this should return false + BOOST_CHECK(!warnings.Unset(warning_2)); + // We should now be able to set w2 again + BOOST_CHECK(warnings.Set(warning_2, _("warning 2 - revision 1"))); + // Setting w2 again should return false since it's already set + BOOST_CHECK(!warnings.Set(warning_2, _("warning 2 - revision 2"))); + + // Verify messages are correct + const auto messages{warnings.GetMessages()}; + BOOST_CHECK(messages.size() == 2); + BOOST_CHECK(messages[0].original == "warning 1"); + BOOST_CHECK(messages[1].original == "warning 2 - revision 1"); + + // Clearing all warnings should also clear all messages + BOOST_CHECK(warnings.Unset(warning_1)); + BOOST_CHECK(warnings.Unset(warning_2)); + BOOST_CHECK(warnings.GetMessages().size() == 0); +} + +BOOST_AUTO_TEST_SUITE_END() diff --git a/src/test/peerman_tests.cpp b/src/test/peerman_tests.cpp index 28866695bc..397b1d8c2d 100644 --- a/src/test/peerman_tests.cpp +++ b/src/test/peerman_tests.cpp @@ -31,7 +31,7 @@ static void mineBlock(const node::NodeContext& node, std::chrono::seconds block_ // Verifying when network-limited peer connections are desirable based on the node's proximity to the tip BOOST_AUTO_TEST_CASE(connections_desirable_service_flags) { - std::unique_ptr<PeerManager> peerman = PeerManager::make(*m_node.connman, *m_node.addrman, nullptr, *m_node.chainman, *m_node.mempool, {}); + std::unique_ptr<PeerManager> peerman = PeerManager::make(*m_node.connman, *m_node.addrman, nullptr, *m_node.chainman, *m_node.mempool, *m_node.warnings, {}); auto consensus = m_node.chainman->GetParams().GetConsensus(); // Check we start connecting to full nodes diff --git a/src/test/timeoffsets_tests.cpp b/src/test/timeoffsets_tests.cpp index 008f1a9db9..5f85a5feeb 100644 --- a/src/test/timeoffsets_tests.cpp +++ b/src/test/timeoffsets_tests.cpp @@ -4,6 +4,7 @@ // #include <node/timeoffsets.h> +#include <node/warnings.h> #include <test/util/setup_common.h> #include <boost/test/unit_test.hpp> @@ -24,7 +25,8 @@ BOOST_FIXTURE_TEST_SUITE(timeoffsets_tests, BasicTestingSetup) BOOST_AUTO_TEST_CASE(timeoffsets) { - TimeOffsets offsets{}; + node::Warnings warnings{}; + TimeOffsets offsets{warnings}; BOOST_CHECK(offsets.Median() == 0s); AddMulti(offsets, {{0s, -1s, -2s, -3s}}); @@ -50,7 +52,8 @@ BOOST_AUTO_TEST_CASE(timeoffsets) static bool IsWarningRaised(const std::vector<std::chrono::seconds>& check_offsets) { - TimeOffsets offsets{}; + node::Warnings warnings{}; + TimeOffsets offsets{warnings}; AddMulti(offsets, check_offsets); return offsets.WarnIfOutOfSync(); } diff --git a/src/test/txpackage_tests.cpp b/src/test/txpackage_tests.cpp index d5f3412aed..478121cc6f 100644 --- a/src/test/txpackage_tests.cpp +++ b/src/test/txpackage_tests.cpp @@ -6,6 +6,7 @@ #include <key_io.h> #include <policy/packages.h> #include <policy/policy.h> +#include <policy/rbf.h> #include <primitives/transaction.h> #include <script/script.h> #include <serialize.h> @@ -938,4 +939,147 @@ BOOST_FIXTURE_TEST_CASE(package_cpfp_tests, TestChain100Setup) BOOST_CHECK_EQUAL(m_node.mempool->size(), expected_pool_size); } } + +BOOST_FIXTURE_TEST_CASE(package_rbf_tests, TestChain100Setup) +{ + mineBlocks(5); + LOCK(::cs_main); + size_t expected_pool_size = m_node.mempool->size(); + CKey child_key{GenerateRandomKey()}; + CScript parent_spk = GetScriptForDestination(WitnessV0KeyHash(child_key.GetPubKey())); + CKey grandchild_key{GenerateRandomKey()}; + CScript child_spk = GetScriptForDestination(WitnessV0KeyHash(grandchild_key.GetPubKey())); + + const CAmount coinbase_value{50 * COIN}; + // Test that de-duplication works. This is not actually package rbf. + { + // 1 parent paying 200sat, 1 child paying 300sat + Package package1; + // 1 parent paying 200sat, 1 child paying 500sat + Package package2; + // Package1 and package2 have the same parent. The children conflict. + auto mtx_parent = CreateValidMempoolTransaction(/*input_transaction=*/m_coinbase_txns[0], /*input_vout=*/0, + /*input_height=*/0, /*input_signing_key=*/coinbaseKey, + /*output_destination=*/parent_spk, + /*output_amount=*/coinbase_value - low_fee_amt, /*submit=*/false); + CTransactionRef tx_parent = MakeTransactionRef(mtx_parent); + package1.push_back(tx_parent); + package2.push_back(tx_parent); + + CTransactionRef tx_child_1 = MakeTransactionRef(CreateValidMempoolTransaction(tx_parent, 0, 101, child_key, child_spk, coinbase_value - low_fee_amt - 300, false)); + package1.push_back(tx_child_1); + CTransactionRef tx_child_2 = MakeTransactionRef(CreateValidMempoolTransaction(tx_parent, 0, 101, child_key, child_spk, coinbase_value - low_fee_amt - 500, false)); + package2.push_back(tx_child_2); + + LOCK(m_node.mempool->cs); + const auto submit1 = ProcessNewPackage(m_node.chainman->ActiveChainstate(), *m_node.mempool, package1, /*test_accept=*/false, std::nullopt); + if (auto err_1{CheckPackageMempoolAcceptResult(package1, submit1, /*expect_valid=*/true, m_node.mempool.get())}) { + BOOST_ERROR(err_1.value()); + } + + // Check precise ResultTypes and mempool size. We know it_parent_1 and it_child_1 exist from above call + auto it_parent_1 = submit1.m_tx_results.find(tx_parent->GetWitnessHash()); + auto it_child_1 = submit1.m_tx_results.find(tx_child_1->GetWitnessHash()); + BOOST_CHECK_EQUAL(it_parent_1->second.m_result_type, MempoolAcceptResult::ResultType::VALID); + BOOST_CHECK_EQUAL(it_child_1->second.m_result_type, MempoolAcceptResult::ResultType::VALID); + expected_pool_size += 2; + BOOST_CHECK_EQUAL(m_node.mempool->size(), expected_pool_size); + + const auto submit2 = ProcessNewPackage(m_node.chainman->ActiveChainstate(), *m_node.mempool, package2, /*test_accept=*/false, std::nullopt); + if (auto err_2{CheckPackageMempoolAcceptResult(package2, submit2, /*expect_valid=*/true, m_node.mempool.get())}) { + BOOST_ERROR(err_2.value()); + } + + // Check precise ResultTypes and mempool size. We know it_parent_2 and it_child_2 exist from above call + auto it_parent_2 = submit2.m_tx_results.find(tx_parent->GetWitnessHash()); + auto it_child_2 = submit2.m_tx_results.find(tx_child_2->GetWitnessHash()); + BOOST_CHECK_EQUAL(it_parent_2->second.m_result_type, MempoolAcceptResult::ResultType::MEMPOOL_ENTRY); + BOOST_CHECK_EQUAL(it_child_2->second.m_result_type, MempoolAcceptResult::ResultType::VALID); + BOOST_CHECK_EQUAL(m_node.mempool->size(), expected_pool_size); + + // child1 has been replaced + BOOST_CHECK(!m_node.mempool->exists(GenTxid::Txid(tx_child_1->GetHash()))); + } + + // Test package rbf. + { + CTransactionRef tx_parent_1 = MakeTransactionRef(CreateValidMempoolTransaction( + m_coinbase_txns[1], /*input_vout=*/0, /*input_height=*/0, + coinbaseKey, parent_spk, coinbase_value - 200, /*submit=*/false)); + CTransactionRef tx_child_1 = MakeTransactionRef(CreateValidMempoolTransaction( + tx_parent_1, /*input_vout=*/0, /*input_height=*/101, + child_key, child_spk, coinbase_value - 400, /*submit=*/false)); + + CTransactionRef tx_parent_2 = MakeTransactionRef(CreateValidMempoolTransaction( + m_coinbase_txns[1], /*input_vout=*/0, /*input_height=*/0, + coinbaseKey, parent_spk, coinbase_value - 800, /*submit=*/false)); + CTransactionRef tx_child_2 = MakeTransactionRef(CreateValidMempoolTransaction( + tx_parent_2, /*input_vout=*/0, /*input_height=*/101, + child_key, child_spk, coinbase_value - 800 - 200, /*submit=*/false)); + + CTransactionRef tx_parent_3 = MakeTransactionRef(CreateValidMempoolTransaction( + m_coinbase_txns[1], /*input_vout=*/0, /*input_height=*/0, + coinbaseKey, parent_spk, coinbase_value - 199, /*submit=*/false)); + CTransactionRef tx_child_3 = MakeTransactionRef(CreateValidMempoolTransaction( + tx_parent_3, /*input_vout=*/0, /*input_height=*/101, + child_key, child_spk, coinbase_value - 199 - 1300, /*submit=*/false)); + + // In all packages, the parents conflict with each other + BOOST_CHECK(tx_parent_1->GetHash() != tx_parent_2->GetHash() && tx_parent_2->GetHash() != tx_parent_3->GetHash()); + + // 1 parent paying 200sat, 1 child paying 200sat. + Package package1{tx_parent_1, tx_child_1}; + // 1 parent paying 800sat, 1 child paying 200sat. + Package package2{tx_parent_2, tx_child_2}; + // 1 parent paying 199sat, 1 child paying 1300sat. + Package package3{tx_parent_3, tx_child_3}; + + const auto submit1 = ProcessNewPackage(m_node.chainman->ActiveChainstate(), *m_node.mempool, package1, false, std::nullopt); + if (auto err_1{CheckPackageMempoolAcceptResult(package1, submit1, /*expect_valid=*/true, m_node.mempool.get())}) { + BOOST_ERROR(err_1.value()); + } + auto it_parent_1 = submit1.m_tx_results.find(tx_parent_1->GetWitnessHash()); + auto it_child_1 = submit1.m_tx_results.find(tx_child_1->GetWitnessHash()); + BOOST_CHECK_EQUAL(it_parent_1->second.m_result_type, MempoolAcceptResult::ResultType::VALID); + BOOST_CHECK_EQUAL(it_child_1->second.m_result_type, MempoolAcceptResult::ResultType::VALID); + expected_pool_size += 2; + BOOST_CHECK_EQUAL(m_node.mempool->size(), expected_pool_size); + + // This replacement is actually not package rbf; the parent carries enough fees + // to replace the entire package on its own. + const auto submit2 = ProcessNewPackage(m_node.chainman->ActiveChainstate(), *m_node.mempool, package2, false, std::nullopt); + if (auto err_2{CheckPackageMempoolAcceptResult(package2, submit2, /*expect_valid=*/true, m_node.mempool.get())}) { + BOOST_ERROR(err_2.value()); + } + auto it_parent_2 = submit2.m_tx_results.find(tx_parent_2->GetWitnessHash()); + auto it_child_2 = submit2.m_tx_results.find(tx_child_2->GetWitnessHash()); + BOOST_CHECK_EQUAL(it_parent_2->second.m_result_type, MempoolAcceptResult::ResultType::VALID); + BOOST_CHECK_EQUAL(it_child_2->second.m_result_type, MempoolAcceptResult::ResultType::VALID); + BOOST_CHECK_EQUAL(m_node.mempool->size(), expected_pool_size); + + // Package RBF, in which the replacement transaction's child sponsors the fees to meet RBF feerate rules + const auto submit3 = ProcessNewPackage(m_node.chainman->ActiveChainstate(), *m_node.mempool, package3, false, std::nullopt); + if (auto err_3{CheckPackageMempoolAcceptResult(package3, submit3, /*expect_valid=*/true, m_node.mempool.get())}) { + BOOST_ERROR(err_3.value()); + } + auto it_parent_3 = submit3.m_tx_results.find(tx_parent_3->GetWitnessHash()); + auto it_child_3 = submit3.m_tx_results.find(tx_child_3->GetWitnessHash()); + BOOST_CHECK_EQUAL(it_parent_3->second.m_result_type, MempoolAcceptResult::ResultType::VALID); + BOOST_CHECK_EQUAL(it_child_3->second.m_result_type, MempoolAcceptResult::ResultType::VALID); + + // package3 was considered as a package to replace both package2 transactions + BOOST_CHECK(it_parent_3->second.m_replaced_transactions.size() == 2); + BOOST_CHECK(it_child_3->second.m_replaced_transactions.empty()); + + std::vector<Wtxid> expected_package3_wtxids({tx_parent_3->GetWitnessHash(), tx_child_3->GetWitnessHash()}); + const auto package3_total_vsize{GetVirtualTransactionSize(*tx_parent_3) + GetVirtualTransactionSize(*tx_child_3)}; + BOOST_CHECK(it_parent_3->second.m_wtxids_fee_calculations.value() == expected_package3_wtxids); + BOOST_CHECK(it_child_3->second.m_wtxids_fee_calculations.value() == expected_package3_wtxids); + BOOST_CHECK_EQUAL(it_parent_3->second.m_effective_feerate.value().GetFee(package3_total_vsize), 199 + 1300); + BOOST_CHECK_EQUAL(it_child_3->second.m_effective_feerate.value().GetFee(package3_total_vsize), 199 + 1300); + + BOOST_CHECK_EQUAL(m_node.mempool->size(), expected_pool_size); + } + +} BOOST_AUTO_TEST_SUITE_END() diff --git a/src/test/util/setup_common.cpp b/src/test/util/setup_common.cpp index 9e9db32351..79e33eacec 100644 --- a/src/test/util/setup_common.cpp +++ b/src/test/util/setup_common.cpp @@ -31,6 +31,7 @@ #include <node/miner.h> #include <node/peerman_args.h> #include <node/validation_cache_args.h> +#include <node/warnings.h> #include <noui.h> #include <policy/fees.h> #include <policy/fees_args.h> @@ -182,6 +183,7 @@ BasicTestingSetup::BasicTestingSetup(const ChainType chainType, const std::vecto InitLogging(*m_node.args); AppInitParameterInteraction(*m_node.args); LogInstance().StartLogging(); + m_node.warnings = std::make_unique<node::Warnings>(); m_node.kernel = std::make_unique<kernel::Context>(); m_node.ecc_context = std::make_unique<ECC_Context>(); SetupEnvironment(); @@ -230,10 +232,11 @@ ChainTestingSetup::ChainTestingSetup(const ChainType chainType, const std::vecto bilingual_str error{}; m_node.mempool = std::make_unique<CTxMemPool>(MemPoolOptionsForTest(m_node), error); Assert(error.empty()); + m_node.warnings = std::make_unique<node::Warnings>(); m_cache_sizes = CalculateCacheSizes(m_args); - m_node.notifications = std::make_unique<KernelNotifications>(*Assert(m_node.shutdown), m_node.exit_status); + m_node.notifications = std::make_unique<KernelNotifications>(*Assert(m_node.shutdown), m_node.exit_status, *Assert(m_node.warnings)); const ChainstateManager::Options chainman_opts{ .chainparams = chainparams, @@ -322,7 +325,8 @@ TestingSetup::TestingSetup( peerman_opts.deterministic_rng = true; m_node.peerman = PeerManager::make(*m_node.connman, *m_node.addrman, m_node.banman.get(), *m_node.chainman, - *m_node.mempool, peerman_opts); + *m_node.mempool, *m_node.warnings, + peerman_opts); { CConnman::Options options; diff --git a/src/test/util/txmempool.cpp b/src/test/util/txmempool.cpp index 249ce9503c..94d50bba50 100644 --- a/src/test/util/txmempool.cpp +++ b/src/test/util/txmempool.cpp @@ -7,6 +7,7 @@ #include <chainparams.h> #include <node/context.h> #include <node/mempool_args.h> +#include <policy/rbf.h> #include <policy/v3_policy.h> #include <txmempool.h> #include <util/check.h> @@ -68,6 +69,28 @@ std::optional<std::string> CheckPackageMempoolAcceptResult(const Package& txns, return strprintf("tx %s unexpectedly failed: %s", wtxid.ToString(), atmp_result.m_state.ToString()); } + // Each subpackage is allowed MAX_REPLACEMENT_CANDIDATES replacements (only checking individually here) + if (atmp_result.m_replaced_transactions.size() > MAX_REPLACEMENT_CANDIDATES) { + return strprintf("tx %s result replaced too many transactions", + wtxid.ToString()); + } + + // Replacements can't happen for subpackages larger than 2 + if (!atmp_result.m_replaced_transactions.empty() && + atmp_result.m_wtxids_fee_calculations.has_value() && atmp_result.m_wtxids_fee_calculations.value().size() > 2) { + return strprintf("tx %s was part of a too-large package RBF subpackage", + wtxid.ToString()); + } + + if (!atmp_result.m_replaced_transactions.empty() && mempool) { + LOCK(mempool->cs); + // If replacements occurred and it used 2 transactions, this is a package RBF and should result in a cluster of size 2 + if (atmp_result.m_wtxids_fee_calculations.has_value() && atmp_result.m_wtxids_fee_calculations.value().size() == 2) { + const auto cluster = mempool->GatherClusters({tx->GetHash()}); + if (cluster.size() != 2) return strprintf("tx %s has too many ancestors or descendants for a package rbf", wtxid.ToString()); + } + } + // m_vsize and m_base_fees should exist iff the result was VALID or MEMPOOL_ENTRY const bool mempool_entry{atmp_result.m_result_type == MempoolAcceptResult::ResultType::MEMPOOL_ENTRY}; if (atmp_result.m_base_fees.has_value() != (valid || mempool_entry)) { @@ -108,6 +131,11 @@ std::optional<std::string> CheckPackageMempoolAcceptResult(const Package& txns, return strprintf("wtxid %s should not be in mempool", wtxid.ToString()); } } + for (const auto& tx_ref : atmp_result.m_replaced_transactions) { + if (mempool->exists(GenTxid::Txid(tx_ref->GetHash()))) { + return strprintf("tx %s should not be in mempool as it was replaced", tx_ref->GetWitnessHash().ToString()); + } + } } } return std::nullopt; diff --git a/src/test/validation_chainstatemanager_tests.cpp b/src/test/validation_chainstatemanager_tests.cpp index 1f6b7368a2..1641c4cd22 100644 --- a/src/test/validation_chainstatemanager_tests.cpp +++ b/src/test/validation_chainstatemanager_tests.cpp @@ -378,7 +378,7 @@ struct SnapshotTestSetup : TestChain100Setup { LOCK(::cs_main); chainman.ResetChainstates(); BOOST_CHECK_EQUAL(chainman.GetAll().size(), 0); - m_node.notifications = std::make_unique<KernelNotifications>(*Assert(m_node.shutdown), m_node.exit_status); + m_node.notifications = std::make_unique<KernelNotifications>(*Assert(m_node.shutdown), m_node.exit_status, *Assert(m_node.warnings)); const ChainstateManager::Options chainman_opts{ .chainparams = ::Params(), .datadir = chainman.m_options.datadir, diff --git a/src/validation.cpp b/src/validation.cpp index e066ee16cb..c34d60f137 100644 --- a/src/validation.cpp +++ b/src/validation.cpp @@ -27,14 +27,15 @@ #include <kernel/mempool_entry.h> #include <kernel/messagestartchars.h> #include <kernel/notifications_interface.h> +#include <kernel/warning.h> #include <logging.h> #include <logging/timer.h> #include <node/blockstorage.h> #include <node/utxo_snapshot.h> -#include <policy/v3_policy.h> #include <policy/policy.h> #include <policy/rbf.h> #include <policy/settings.h> +#include <policy/v3_policy.h> #include <pow.h> #include <primitives/block.h> #include <primitives/transaction.h> @@ -57,11 +58,11 @@ #include <util/result.h> #include <util/signalinterrupt.h> #include <util/strencodings.h> +#include <util/string.h> #include <util/time.h> #include <util/trace.h> #include <util/translation.h> #include <validationinterface.h> -#include <warnings.h> #include <algorithm> #include <cassert> @@ -524,7 +525,7 @@ public: /* m_bypass_limits */ false, /* m_coins_to_uncache */ coins_to_uncache, /* m_test_accept */ false, - /* m_allow_replacement */ false, + /* m_allow_replacement */ true, /* m_allow_sibling_eviction */ false, /* m_package_submission */ true, /* m_package_feerates */ true, @@ -602,8 +603,8 @@ public: /** * Submission of a subpackage. * If subpackage size == 1, calls AcceptSingleTransaction() with adjusted ATMPArgs to avoid - * package policy restrictions like no CPFP carve out (PackageMempoolChecks) and disabled RBF - * (m_allow_replacement), and creates a PackageMempoolAcceptResult wrapping the result. + * package policy restrictions like no CPFP carve out (PackageMempoolChecks) + * and creates a PackageMempoolAcceptResult wrapping the result. * * If subpackage size > 1, calls AcceptMultipleTransactions() with the provided ATMPArgs. * @@ -666,12 +667,13 @@ private: // only tests that are fast should be done here (to avoid CPU DoS). bool PreChecks(ATMPArgs& args, Workspace& ws) EXCLUSIVE_LOCKS_REQUIRED(cs_main, m_pool.cs); - // Run checks for mempool replace-by-fee. + // Run checks for mempool replace-by-fee, only used in AcceptSingleTransaction. bool ReplacementChecks(Workspace& ws) EXCLUSIVE_LOCKS_REQUIRED(cs_main, m_pool.cs); // Enforce package mempool ancestor/descendant limits (distinct from individual - // ancestor/descendant limits done in PreChecks). + // ancestor/descendant limits done in PreChecks) and run Package RBF checks. bool PackageMempoolChecks(const std::vector<CTransactionRef>& txns, + std::vector<Workspace>& workspaces, int64_t total_vsize, PackageValidationState& package_state) EXCLUSIVE_LOCKS_REQUIRED(cs_main, m_pool.cs); @@ -949,7 +951,7 @@ bool MemPoolAccept::PreChecks(ATMPArgs& args, Workspace& ws) ws.m_iters_conflicting = m_pool.GetIterSet(ws.m_conflicts); // Note that these modifications are only applicable to single transaction scenarios; - // carve-outs and package RBF are disabled for multi-transaction evaluations. + // carve-outs are disabled for multi-transaction evaluations. CTxMemPool::Limits maybe_rbf_limits = m_pool.m_opts.limits; // Calculate in-mempool ancestors, up to a limit. @@ -1088,10 +1090,9 @@ bool MemPoolAccept::ReplacementChecks(Workspace& ws) // descendant transaction of a direct conflict to pay a higher feerate than the transaction that // might replace them, under these rules. if (const auto err_string{PaysMoreThanConflicts(ws.m_iters_conflicting, newFeeRate, hash)}) { - // Even though this is a fee-related failure, this result is TX_MEMPOOL_POLICY, not - // TX_RECONSIDERABLE, because it cannot be bypassed using package validation. - // This must be changed if package RBF is enabled. - return state.Invalid(TxValidationResult::TX_MEMPOOL_POLICY, + // This fee-related failure is TX_RECONSIDERABLE because validating in a package may change + // the result. + return state.Invalid(TxValidationResult::TX_RECONSIDERABLE, strprintf("insufficient fee%s", ws.m_sibling_eviction ? " (including sibling eviction)" : ""), *err_string); } @@ -1116,16 +1117,15 @@ bool MemPoolAccept::ReplacementChecks(Workspace& ws) } if (const auto err_string{PaysForRBF(m_subpackage.m_conflicting_fees, ws.m_modified_fees, ws.m_vsize, m_pool.m_opts.incremental_relay_feerate, hash)}) { - // Even though this is a fee-related failure, this result is TX_MEMPOOL_POLICY, not - // TX_RECONSIDERABLE, because it cannot be bypassed using package validation. - // This must be changed if package RBF is enabled. - return state.Invalid(TxValidationResult::TX_MEMPOOL_POLICY, + // Result may change in a package context + return state.Invalid(TxValidationResult::TX_RECONSIDERABLE, strprintf("insufficient fee%s", ws.m_sibling_eviction ? " (including sibling eviction)" : ""), *err_string); } return true; } bool MemPoolAccept::PackageMempoolChecks(const std::vector<CTransactionRef>& txns, + std::vector<Workspace>& workspaces, const int64_t total_vsize, PackageValidationState& package_state) { @@ -1136,12 +1136,88 @@ bool MemPoolAccept::PackageMempoolChecks(const std::vector<CTransactionRef>& txn assert(std::all_of(txns.cbegin(), txns.cend(), [this](const auto& tx) { return !m_pool.exists(GenTxid::Txid(tx->GetHash()));})); + assert(txns.size() == workspaces.size()); + auto result = m_pool.CheckPackageLimits(txns, total_vsize); if (!result) { // This is a package-wide error, separate from an individual transaction error. return package_state.Invalid(PackageValidationResult::PCKG_POLICY, "package-mempool-limits", util::ErrorString(result).original); } - return true; + + // No conflicts means we're finished. Further checks are all RBF-only. + if (!m_subpackage.m_rbf) return true; + + // We're in package RBF context; replacement proposal must be size 2 + if (workspaces.size() != 2 || !Assume(IsChildWithParents(txns))) { + return package_state.Invalid(PackageValidationResult::PCKG_POLICY, "package RBF failed: package must be 1-parent-1-child"); + } + + // If the package has in-mempool ancestors, we won't consider a package RBF + // since it would result in a cluster larger than 2. + // N.B. To relax this constraint we will need to revisit how CCoinsViewMemPool::PackageAddTransaction + // is being used inside AcceptMultipleTransactions to track available inputs while processing a package. + for (const auto& ws : workspaces) { + if (!ws.m_ancestors.empty()) { + return package_state.Invalid(PackageValidationResult::PCKG_POLICY, "package RBF failed: new transaction cannot have mempool ancestors"); + } + } + + // Aggregate all conflicts into one set. + CTxMemPool::setEntries direct_conflict_iters; + for (Workspace& ws : workspaces) { + // Aggregate all conflicts into one set. + direct_conflict_iters.merge(ws.m_iters_conflicting); + } + + const auto& parent_ws = workspaces[0]; + const auto& child_ws = workspaces[1]; + + // Don't consider replacements that would cause us to remove a large number of mempool entries. + // This limit is not increased in a package RBF. Use the aggregate number of transactions. + if (const auto err_string{GetEntriesForConflicts(*child_ws.m_ptx, m_pool, direct_conflict_iters, + m_subpackage.m_all_conflicts)}) { + return package_state.Invalid(PackageValidationResult::PCKG_POLICY, + "package RBF failed: too many potential replacements", *err_string); + } + + for (CTxMemPool::txiter it : m_subpackage.m_all_conflicts) { + m_subpackage.m_conflicting_fees += it->GetModifiedFee(); + m_subpackage.m_conflicting_size += it->GetTxSize(); + } + + // Use the child as the transaction for attributing errors to. + const Txid& child_hash = child_ws.m_ptx->GetHash(); + if (const auto err_string{PaysForRBF(/*original_fees=*/m_subpackage.m_conflicting_fees, + /*replacement_fees=*/m_subpackage.m_total_modified_fees, + /*replacement_vsize=*/m_subpackage.m_total_vsize, + m_pool.m_opts.incremental_relay_feerate, child_hash)}) { + return package_state.Invalid(PackageValidationResult::PCKG_POLICY, + "package RBF failed: insufficient anti-DoS fees", *err_string); + } + + // Ensure this two transaction package is a "chunk" on its own; we don't want the child + // to be only paying anti-DoS fees + const CFeeRate parent_feerate(parent_ws.m_modified_fees, parent_ws.m_vsize); + const CFeeRate package_feerate(m_subpackage.m_total_modified_fees, m_subpackage.m_total_vsize); + if (package_feerate <= parent_feerate) { + return package_state.Invalid(PackageValidationResult::PCKG_POLICY, + "package RBF failed: package feerate is less than parent feerate", + strprintf("package feerate %s <= parent feerate is %s", package_feerate.ToString(), parent_feerate.ToString())); + } + + // Check if it's economically rational to mine this package rather than the ones it replaces. + // This takes the place of ReplacementChecks()'s PaysMoreThanConflicts() in the package RBF setting. + if (const auto err_tup{ImprovesFeerateDiagram(m_pool, direct_conflict_iters, m_subpackage.m_all_conflicts, m_subpackage.m_total_modified_fees, m_subpackage.m_total_vsize)}) { + return package_state.Invalid(PackageValidationResult::PCKG_POLICY, + "package RBF failed: " + err_tup.value().second, ""); + } + + LogPrint(BCLog::TXPACKAGES, "package RBF checks passed: parent %s (wtxid=%s), child %s (wtxid=%s)\n", + txns.front()->GetHash().ToString(), txns.front()->GetWitnessHash().ToString(), + txns.back()->GetHash().ToString(), txns.back()->GetWitnessHash().ToString()); + + + return true; } bool MemPoolAccept::PolicyScriptChecks(const ATMPArgs& args, Workspace& ws) @@ -1215,16 +1291,19 @@ bool MemPoolAccept::Finalize(const ATMPArgs& args, Workspace& ws) const bool bypass_limits = args.m_bypass_limits; std::unique_ptr<CTxMemPoolEntry>& entry = ws.m_entry; + if (!m_subpackage.m_all_conflicts.empty()) Assume(args.m_allow_replacement); // Remove conflicting transactions from the mempool for (CTxMemPool::txiter it : m_subpackage.m_all_conflicts) { - LogPrint(BCLog::MEMPOOL, "replacing tx %s (wtxid=%s) with %s (wtxid=%s) for %s additional fees, %d delta bytes\n", + LogPrint(BCLog::MEMPOOL, "replacing mempool tx %s (wtxid=%s, fees=%s, vsize=%s). New tx %s (wtxid=%s, fees=%s, vsize=%s)\n", it->GetTx().GetHash().ToString(), it->GetTx().GetWitnessHash().ToString(), + it->GetFee(), + it->GetTxSize(), hash.ToString(), tx.GetWitnessHash().ToString(), - FormatMoney(ws.m_modified_fees - m_subpackage.m_conflicting_fees), - (int)entry->GetTxSize() - (int)m_subpackage.m_conflicting_size); + entry->GetFee(), + entry->GetTxSize()); TRACE7(mempool, replaced, it->GetTx().GetHash().data(), it->GetTxSize(), @@ -1318,6 +1397,13 @@ bool MemPoolAccept::SubmitPackage(const ATMPArgs& args, std::vector<Workspace>& std::transform(workspaces.cbegin(), workspaces.cend(), std::back_inserter(all_package_wtxids), [](const auto& ws) { return ws.m_ptx->GetWitnessHash(); }); + if (!m_subpackage.m_replaced_transactions.empty()) { + LogPrint(BCLog::MEMPOOL, "replaced %u mempool transactions with %u new one(s) for %s additional fees, %d delta bytes\n", + m_subpackage.m_replaced_transactions.size(), workspaces.size(), + m_subpackage.m_total_modified_fees - m_subpackage.m_conflicting_fees, + m_subpackage.m_total_vsize - static_cast<int>(m_subpackage.m_conflicting_size)); + } + // Add successful results. The returned results may change later if LimitMempoolSize() evicts them. for (Workspace& ws : workspaces) { const auto effective_feerate = args.m_package_feerates ? ws.m_package_feerate : @@ -1361,7 +1447,13 @@ MempoolAcceptResult MemPoolAccept::AcceptSingleTransaction(const CTransactionRef return MempoolAcceptResult::Failure(ws.m_state); } - if (m_subpackage.m_rbf && !ReplacementChecks(ws)) return MempoolAcceptResult::Failure(ws.m_state); + if (m_subpackage.m_rbf && !ReplacementChecks(ws)) { + if (ws.m_state.GetResult() == TxValidationResult::TX_RECONSIDERABLE) { + // Failed for incentives-based fee reasons. Provide the effective feerate and which tx was included. + return MempoolAcceptResult::FeeFailure(ws.m_state, CFeeRate(ws.m_modified_fees, ws.m_vsize), single_wtxid); + } + return MempoolAcceptResult::Failure(ws.m_state); + } // Perform the inexpensive checks first and avoid hashing and signature verification unless // those checks pass, to mitigate CPU exhaustion denial-of-service attacks. @@ -1393,6 +1485,13 @@ MempoolAcceptResult MemPoolAccept::AcceptSingleTransaction(const CTransactionRef m_pool.m_opts.signals->TransactionAddedToMempool(tx_info, m_pool.GetAndIncrementSequence()); } + if (!m_subpackage.m_replaced_transactions.empty()) { + LogPrint(BCLog::MEMPOOL, "replaced %u mempool transactions with 1 new transaction for %s additional fees, %d delta bytes\n", + m_subpackage.m_replaced_transactions.size(), + ws.m_modified_fees - m_subpackage.m_conflicting_fees, + ws.m_vsize - static_cast<int>(m_subpackage.m_conflicting_size)); + } + return MempoolAcceptResult::Success(std::move(m_subpackage.m_replaced_transactions), ws.m_vsize, ws.m_base_fees, effective_feerate, single_wtxid); } @@ -1434,11 +1533,14 @@ PackageMempoolAcceptResult MemPoolAccept::AcceptMultipleTransactions(const std:: } // Make the coins created by this transaction available for subsequent transactions in the - // package to spend. Since we already checked conflicts in the package and we don't allow - // replacements, we don't need to track the coins spent. Note that this logic will need to be - // updated if package replace-by-fee is allowed in the future. - assert(!args.m_allow_replacement); - assert(!m_subpackage.m_rbf); + // package to spend. If there are no conflicts within the package, no transaction can spend a coin + // needed by another transaction in the package. We also need to make sure that no package + // tx replaces (or replaces the ancestor of) the parent of another package tx. As long as we + // check these two things, we don't need to track the coins spent. + // If a package tx conflicts with a mempool tx, PackageMempoolChecks() ensures later that any package RBF attempt + // has *no* in-mempool ancestors, so we don't have to worry about subsequent transactions in + // same package spending the same in-mempool outpoints. This needs to be revisited for general + // package RBF. m_viewmempool.PackageAddTransaction(ws.m_ptx); } @@ -1479,7 +1581,7 @@ PackageMempoolAcceptResult MemPoolAccept::AcceptMultipleTransactions(const std:: // Apply package mempool ancestor/descendant limits. Skip if there is only one transaction, // because it's unnecessary. - if (txns.size() > 1 && !PackageMempoolChecks(txns, m_subpackage.m_total_vsize, package_state)) { + if (txns.size() > 1 && !PackageMempoolChecks(txns, workspaces, m_subpackage.m_total_vsize, package_state)) { return PackageMempoolAcceptResult(package_state, std::move(results)); } @@ -1921,9 +2023,11 @@ void Chainstate::CheckForkWarningConditions() if (m_chainman.m_best_invalid && m_chainman.m_best_invalid->nChainWork > m_chain.Tip()->nChainWork + (GetBlockProof(*m_chain.Tip()) * 6)) { LogPrintf("%s: Warning: Found invalid chain at least ~6 blocks longer than our best chain.\nChain state database corruption likely.\n", __func__); - SetfLargeWorkInvalidChainFound(true); + m_chainman.GetNotifications().warningSet( + kernel::Warning::LARGE_WORK_INVALID_CHAIN, + _("Warning: We do not appear to fully agree with our peers! You may need to upgrade, or other nodes may need to upgrade.")); } else { - SetfLargeWorkInvalidChainFound(false); + m_chainman.GetNotifications().warningUnset(kernel::Warning::LARGE_WORK_INVALID_CHAIN); } } @@ -2847,13 +2951,6 @@ void Chainstate::PruneAndFlush() } } -/** Private helper function that concatenates warning messages. */ -static void AppendWarning(bilingual_str& res, const bilingual_str& warn) -{ - if (!res.empty()) res += Untranslated(", "); - res += warn; -} - static void UpdateTipLog( const CCoinsViewCache& coins_tip, const CBlockIndex* tip, @@ -2904,7 +3001,7 @@ void Chainstate::UpdateTip(const CBlockIndex* pindexNew) g_best_block_cv.notify_all(); } - bilingual_str warning_messages; + std::vector<bilingual_str> warning_messages; if (!m_chainman.IsInitialBlockDownload()) { const CBlockIndex* pindex = pindexNew; for (int bit = 0; bit < VERSIONBITS_NUM_BITS; bit++) { @@ -2913,14 +3010,15 @@ void Chainstate::UpdateTip(const CBlockIndex* pindexNew) if (state == ThresholdState::ACTIVE || state == ThresholdState::LOCKED_IN) { const bilingual_str warning = strprintf(_("Unknown new rules activated (versionbit %i)"), bit); if (state == ThresholdState::ACTIVE) { - m_chainman.GetNotifications().warning(warning); + m_chainman.GetNotifications().warningSet(kernel::Warning::UNKNOWN_NEW_RULES_ACTIVATED, warning); } else { - AppendWarning(warning_messages, warning); + warning_messages.push_back(warning); } } } } - UpdateTipLog(coins_tip, pindexNew, params, __func__, "", warning_messages.original); + UpdateTipLog(coins_tip, pindexNew, params, __func__, "", + util::Join(warning_messages, Untranslated(", ")).original); } /** Disconnect m_chain's tip. @@ -5967,8 +6065,8 @@ SnapshotCompletionResult ChainstateManager::MaybeCompleteSnapshotValidation() PACKAGE_NAME, snapshot_tip_height, snapshot_base_height, snapshot_base_height, PACKAGE_BUGREPORT ); - LogPrintf("[snapshot] !!! %s\n", user_error.original); - LogPrintf("[snapshot] deleting snapshot, reverting to validated chain, and stopping node\n"); + LogError("[snapshot] !!! %s\n", user_error.original); + LogError("[snapshot] deleting snapshot, reverting to validated chain, and stopping node\n"); m_active_chainstate = m_ibd_chainstate.get(); m_snapshot_chainstate->m_disabled = true; @@ -6320,7 +6418,7 @@ bool ChainstateManager::ValidatedSnapshotCleanup() fs::path p_old, fs::path p_new, const fs::filesystem_error& err) { - LogPrintf("Error renaming path (%s) -> (%s): %s\n", + LogError("[snapshot] Error renaming path (%s) -> (%s): %s\n", fs::PathToString(p_old), fs::PathToString(p_new), err.what()); GetNotifications().fatalError(strprintf(_( "Rename of '%s' -> '%s' failed. " diff --git a/src/wallet/feebumper.cpp b/src/wallet/feebumper.cpp index 1288d3b418..3184d0f3b0 100644 --- a/src/wallet/feebumper.cpp +++ b/src/wallet/feebumper.cpp @@ -93,7 +93,7 @@ static feebumper::Result CheckFeeRate(const CWallet& wallet, const CMutableTrans } CAmount new_total_fee = newFeerate.GetFee(maxTxSize) + combined_bump_fee.value(); - CFeeRate incrementalRelayFee = std::max(wallet.chain().relayIncrementalFee(), CFeeRate(WALLET_INCREMENTAL_RELAY_FEE)); + CFeeRate incrementalRelayFee = wallet.chain().relayIncrementalFee(); // Min total fee is old fee + relay fee CAmount minTotalFee = old_fee + incrementalRelayFee.GetFee(maxTxSize); diff --git a/src/warnings.cpp b/src/warnings.cpp deleted file mode 100644 index 38c0554cf2..0000000000 --- a/src/warnings.cpp +++ /dev/null @@ -1,65 +0,0 @@ -// Copyright (c) 2009-2010 Satoshi Nakamoto -// Copyright (c) 2009-2022 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 <config/bitcoin-config.h> // IWYU pragma: keep - -#include <warnings.h> - -#include <common/system.h> -#include <sync.h> -#include <util/translation.h> - -#include <optional> -#include <vector> - -static GlobalMutex g_warnings_mutex; -static bilingual_str g_misc_warnings GUARDED_BY(g_warnings_mutex); -static bool fLargeWorkInvalidChainFound GUARDED_BY(g_warnings_mutex) = false; -static std::optional<bilingual_str> g_timeoffset_warning GUARDED_BY(g_warnings_mutex){}; - -void SetMiscWarning(const bilingual_str& warning) -{ - LOCK(g_warnings_mutex); - g_misc_warnings = warning; -} - -void SetfLargeWorkInvalidChainFound(bool flag) -{ - LOCK(g_warnings_mutex); - fLargeWorkInvalidChainFound = flag; -} - -void SetMedianTimeOffsetWarning(std::optional<bilingual_str> warning) -{ - LOCK(g_warnings_mutex); - g_timeoffset_warning = warning; -} - -std::vector<bilingual_str> GetWarnings() -{ - std::vector<bilingual_str> warnings; - - LOCK(g_warnings_mutex); - - // Pre-release build warning - if (!CLIENT_VERSION_IS_RELEASE) { - warnings.emplace_back(_("This is a pre-release test build - use at your own risk - do not use for mining or merchant applications")); - } - - // Misc warnings like out of disk space and clock is wrong - if (!g_misc_warnings.empty()) { - warnings.emplace_back(g_misc_warnings); - } - - if (fLargeWorkInvalidChainFound) { - warnings.emplace_back(_("Warning: We do not appear to fully agree with our peers! You may need to upgrade, or other nodes may need to upgrade.")); - } - - if (g_timeoffset_warning) { - warnings.emplace_back(g_timeoffset_warning.value()); - } - - return warnings; -} diff --git a/src/warnings.h b/src/warnings.h deleted file mode 100644 index 79dc2ffabf..0000000000 --- a/src/warnings.h +++ /dev/null @@ -1,22 +0,0 @@ -// Copyright (c) 2009-2010 Satoshi Nakamoto -// Copyright (c) 2009-2021 The Bitcoin Core developers -// Distributed under the MIT software license, see the accompanying -// file COPYING or http://www.opensource.org/licenses/mit-license.php. - -#ifndef BITCOIN_WARNINGS_H -#define BITCOIN_WARNINGS_H - -#include <optional> -#include <string> -#include <vector> - -struct bilingual_str; - -void SetMiscWarning(const bilingual_str& warning); -void SetfLargeWorkInvalidChainFound(bool flag); -/** Pass std::nullopt to disable the warning */ -void SetMedianTimeOffsetWarning(std::optional<bilingual_str> warning); -/** Return potential problems detected by the node. */ -std::vector<bilingual_str> GetWarnings(); - -#endif // BITCOIN_WARNINGS_H |