From 89f4744e78f7a7bcc4465c89345d50b1cc995c57 Mon Sep 17 00:00:00 2001 From: Hennadii Stepanov <32963518+hebasto@users.noreply.github.com> Date: Thu, 31 Aug 2023 12:22:58 +0100 Subject: [PATCH 1/9] Merge bitcoin-core/gui#749: make '-min' minimize wallet loading dialog 32db15450a9ef2a45de29b3b2ae60491a38edbd6 gui: make '-min' minimize wallet loading dialog (furszy) Pull request description: Simple fix for #748. When '-min' is enabled, no loading dialog should be presented on screen during startup. ACKs for top commit: hebasto: ACK 32db15450a9ef2a45de29b3b2ae60491a38edbd6, tested on Debian 11 + XFCE. Tree-SHA512: d08060b044938c67e8309db77b49ca645850fc21fdd7d78d5368d336fb9f602dcc66ea398a7505b00bf7d43afa07108347c7260480319fad3ec84cb41332f780 --- src/qt/bitcoin.cpp | 9 ++++++--- src/qt/bitcoingui.cpp | 4 ++-- src/qt/bitcoingui.h | 2 +- src/qt/walletcontroller.cpp | 9 ++++++--- src/qt/walletcontroller.h | 4 ++-- 5 files changed, 17 insertions(+), 11 deletions(-) diff --git a/src/qt/bitcoin.cpp b/src/qt/bitcoin.cpp index 2cc498ab6228..f6988f649a56 100644 --- a/src/qt/bitcoin.cpp +++ b/src/qt/bitcoin.cpp @@ -403,18 +403,21 @@ void BitcoinApplication::initializeResult(bool success, interfaces::BlockAndHead qInfo() << "Platform customization:" << gArgs.GetArg("-uiplatform", BitcoinGUI::DEFAULT_UIPLATFORM).c_str(); clientModel = new ClientModel(node(), optionsModel); window->setClientModel(clientModel, &tip_info); + + // If '-min' option passed, start window minimized (iconified) or minimized to tray + bool start_minimized = gArgs.GetBoolArg("-min", false); #ifdef ENABLE_WALLET if (WalletModel::isWalletEnabled()) { m_wallet_controller = new WalletController(*clientModel, this); - window->setWalletController(m_wallet_controller); + window->setWalletController(m_wallet_controller, /*show_loading_minimized=*/start_minimized); if (paymentServer) { paymentServer->setOptionsModel(optionsModel); } } #endif // ENABLE_WALLET - // If -min option passed, start window minimized (iconified) or minimized to tray - if (!gArgs.GetBoolArg("-min", false)) { + // Show or minimize window + if (!start_minimized) { window->show(); } else if (clientModel->getOptionsModel()->getMinimizeToTray() && window->hasTrayIcon()) { // do nothing as the window is managed by the tray icon diff --git a/src/qt/bitcoingui.cpp b/src/qt/bitcoingui.cpp index 3921618469ea..8478c00a70b2 100644 --- a/src/qt/bitcoingui.cpp +++ b/src/qt/bitcoingui.cpp @@ -917,7 +917,7 @@ void BitcoinGUI::setClientModel(ClientModel *_clientModel, interfaces::BlockAndH } #ifdef ENABLE_WALLET -void BitcoinGUI::setWalletController(WalletController* wallet_controller) +void BitcoinGUI::setWalletController(WalletController* wallet_controller, bool show_loading_minimized) { assert(!m_wallet_controller); assert(wallet_controller); @@ -937,7 +937,7 @@ void BitcoinGUI::setWalletController(WalletController* wallet_controller) }); auto activity = new LoadWalletsActivity(m_wallet_controller, this); - activity->load(); + activity->load(show_loading_minimized); } WalletController* BitcoinGUI::getWalletController() diff --git a/src/qt/bitcoingui.h b/src/qt/bitcoingui.h index 08543872943e..26f73eda3e60 100644 --- a/src/qt/bitcoingui.h +++ b/src/qt/bitcoingui.h @@ -81,7 +81,7 @@ class BitcoinGUI : public QMainWindow */ void setClientModel(ClientModel *clientModel = nullptr, interfaces::BlockAndHeaderTipInfo* tip_info = nullptr); #ifdef ENABLE_WALLET - void setWalletController(WalletController* wallet_controller); + void setWalletController(WalletController* wallet_controller, bool show_loading_minimized); WalletController* getWalletController(); #endif diff --git a/src/qt/walletcontroller.cpp b/src/qt/walletcontroller.cpp index 55616a2f4914..18739bc22750 100644 --- a/src/qt/walletcontroller.cpp +++ b/src/qt/walletcontroller.cpp @@ -194,7 +194,7 @@ WalletControllerActivity::WalletControllerActivity(WalletController* wallet_cont connect(this, &WalletControllerActivity::finished, this, &QObject::deleteLater); } -void WalletControllerActivity::showProgressDialog(const QString& title_text, const QString& label_text) +void WalletControllerActivity::showProgressDialog(const QString& title_text, const QString& label_text, bool show_minimized) { auto progress_dialog = new QProgressDialog(m_parent_widget); progress_dialog->setAttribute(Qt::WA_DeleteOnClose); @@ -209,6 +209,8 @@ void WalletControllerActivity::showProgressDialog(const QString& title_text, con // The setValue call forces QProgressDialog to start the internal duration estimation. // See details in https://bugreports.qt.io/browse/QTBUG-47042. progress_dialog->setValue(0); + // When requested, launch dialog minimized + if (show_minimized) progress_dialog->showMinimized(); } CreateWalletActivity::CreateWalletActivity(WalletController* wallet_controller, QWidget* parent_widget) @@ -447,14 +449,15 @@ LoadWalletsActivity::LoadWalletsActivity(WalletController* wallet_controller, QW { } -void LoadWalletsActivity::load() +void LoadWalletsActivity::load(bool show_loading_minimized) { showProgressDialog( //: Title of progress window which is displayed when wallets are being loaded. tr("Load Wallets"), /*: Descriptive text of the load wallets progress window which indicates to the user that wallets are currently being loaded.*/ - tr("Loading wallets…")); + tr("Loading wallets…"), + /*show_minimized=*/show_loading_minimized); QTimer::singleShot(0, worker(), [this] { for (auto& wallet : node().walletLoader().getWallets()) { diff --git a/src/qt/walletcontroller.h b/src/qt/walletcontroller.h index ccb7dbc62d51..c8ce283c0ad7 100644 --- a/src/qt/walletcontroller.h +++ b/src/qt/walletcontroller.h @@ -96,7 +96,7 @@ class WalletControllerActivity : public QObject interfaces::Node& node() const { return m_wallet_controller->m_node; } QObject* worker() const { return m_wallet_controller->m_activity_worker; } - void showProgressDialog(const QString& title_text, const QString& label_text); + void showProgressDialog(const QString& title_text, const QString& label_text, bool show_minimized=false); WalletController* const m_wallet_controller; QWidget* const m_parent_widget; @@ -152,7 +152,7 @@ class LoadWalletsActivity : public WalletControllerActivity public: LoadWalletsActivity(WalletController* wallet_controller, QWidget* parent_widget); - void load(); + void load(bool show_loading_minimized); }; class RestoreWalletActivity : public WalletControllerActivity From 10ddb032d9520a6277da3bd654170b988eeccdd3 Mon Sep 17 00:00:00 2001 From: Ava Chow Date: Wed, 10 Jan 2024 14:00:09 -0500 Subject: [PATCH 2/9] Merge bitcoin/bitcoin#28318: logging: Simplify API for level based logging MIME-Version: 1.0 Content-Type: text/plain; charset=UTF-8 Content-Transfer-Encoding: 8bit e60fc7d5d34f23cccbff6e4f5f3d716fa8dad50c logging: Replace uses of LogPrintfCategory (Anthony Towns) f7ce5ac08c669ac763e275bb7c82dcfb2b1b6c33 logging: add LogError, LogWarning, LogInfo, LogDebug, LogTrace (Anthony Towns) fbd7642c8e5b70327e019382320f5ef0a651ecc5 logging: add -loglevelalways=1 option (Anthony Towns) 782bb6a05663ad7a53908e910d0f42b49b881e09 logging: treat BCLog::ALL like BCLog::NONE (Anthony Towns) 667ce3e3297645527b07314e1d5a82275fb25845 logging: Drop BCLog::Level::None (Anthony Towns) ab34dc6012351e7b8aab871dd9d2b38ade1cd9bc logging: Log Info messages unconditionally (Anthony Towns) dfe98b6874da04e45f68d17575c1e8a5431ca9bc logging: make [cat:debug] and [info] implicit (Anthony Towns) c5c76dc615677d226c9f6b3f2b66d833315d40da logging: refactor: pull prefix code out (Anthony Towns) Pull request description: Replace `LogPrint*` functions with severity based logging functions: * `LogInfo(...)`, `LogWarning(...)`, `LogError(...)` for unconditional (uncategorised) logging (replaces `LogPrintf`) * `LogDebug(CATEGORY, ...)` and `LogTrace(CATEGORY, ...)` for conditional logging (replaces `LogPrint`) * `LogPrintLevel(CATEGORY, LEVEL, ...)` for when the level isn't known in advance, or a category needs to be added for an info/warning/error log message (mostly unchanged, but rarely needed) Logs look roughly as they do now with `LogInfo` not having an `[info]` prefix, and `LogDebug` having a `[cat]` prefix, rather than a `[cat:debug]` prefix. This removes `BCLog::Level::None` entirely -- for `LogFlags::NONE` just use `Level::Info`, for any actual category, use `Level::Debug`. Adds docs to developer-notes about when to use which level. Adds `-loglevelalways=1` option so that you get `[net:debug]`, `[all:info]`, `[all:warning]` etc, which might be helpful for automated parsing, or just if you like everything to be consistent. Defaults to off to reduce noise in the default config, and to avoid unnecessary changes on upgrades. Changes the behaviour of `LogPrintLevel(CATEGORY, BCLog::Level::Info, ...)` to be logged unconditionally, rather than only being an additional optional logging level in addition to trace and debug. Does not change the behaviour of `LogPrintLevel(NONE, Debug, ...)` and `LogPrintLevel(NONE, Trace, ...)` being no-ops. ACKs for top commit: maflcko: re-ACK e60fc7d5d34f23cccbff6e4f5f3d716fa8dad50c 🌚 achow101: ACK e60fc7d5d34f23cccbff6e4f5f3d716fa8dad50c stickies-v: ACK e60fc7d5d34f23cccbff6e4f5f3d716fa8dad50c jamesob: ACK e60fc7d5d34f23cccbff6e4f5f3d716fa8dad50c ([`jamesob/ackr/28318.1.ajtowns.logging_simplify_api_for`](https://github.com/jamesob/bitcoin/tree/ackr/28318.1.ajtowns.logging_simplify_api_for)) Tree-SHA512: e7a4588779b148242495b7b6f64198a00c314cd57100affab11c43e9d39c9bbf85118ee2002792087fdcffdea08c84576e20844b3079f27083e26ddd7ca15d7f --- doc/developer-notes.md | 45 +++++++++++++++++++++++-- src/httpserver.cpp | 5 ++- src/init.cpp | 2 +- src/init/common.cpp | 2 ++ src/logging.cpp | 56 ++++++++++++++++++-------------- src/logging.h | 31 ++++++++++-------- src/net_processing.cpp | 2 +- src/test/logging_tests.cpp | 42 +++++++++++++++++++----- src/torcontrol.cpp | 2 +- test/lint/lint-format-strings.py | 5 +++ 10 files changed, 138 insertions(+), 54 deletions(-) diff --git a/doc/developer-notes.md b/doc/developer-notes.md index 28b58496057b..bd9ca2db7c3a 100644 --- a/doc/developer-notes.md +++ b/doc/developer-notes.md @@ -730,6 +730,47 @@ General Dash Core - *Explanation*: If the test suite is to be updated for a change, this has to be done first. +Logging +------- + +The macros `LogInfo`, `LogDebug`, `LogTrace`, `LogWarning` and `LogError` are available for +logging messages. They should be used as follows: + +- `LogDebug(BCLog::CATEGORY, fmt, params...)` is what you want + most of the time, and it should be used for log messages that are + useful for debugging and can reasonably be enabled on a production + system (that has sufficient free storage space). They will be logged + if the program is started with `-debug=category` or `-debug=1`. + Note that `LogPrint(BCLog::CATEGORY, fmt, params...)` is a deprecated + alias for `LogDebug`. + +- `LogInfo(fmt, params...)` should only be used rarely, eg for startup + messages or for infrequent and important events such as a new block tip + being found or a new outbound connection being made. These log messages + are unconditional so care must be taken that they can't be used by an + attacker to fill up storage. Note that `LogPrintf(fmt, params...)` is + a deprecated alias for `LogInfo`. + +- `LogError(fmt, params...)` should be used in place of `LogInfo` for + severe problems that require the node (or a subsystem) to shut down + entirely (eg, insufficient storage space). + +- `LogWarning(fmt, params...)` should be used in place of `LogInfo` for + severe problems that the node admin should address, but are not + severe enough to warrant shutting down the node (eg, system time + appears to be wrong, unknown soft fork appears to have activated). + +- `LogTrace(BCLog::CATEGORY, fmt, params...) should be used in place of + `LogDebug` for log messages that would be unusable on a production + system, eg due to being too noisy in normal use, or too resource + intensive to process. These will be logged if the startup + options `-debug=category -loglevel=category:trace` or `-debug=1 + -loglevel=trace` are selected. + +Note that the format strings and parameters of `LogDebug` and `LogTrace` +are only evaluated if the logging category is enabled, so you must be +careful to avoid side-effects in those expressions. + Wallet ------- @@ -899,7 +940,7 @@ Strings and formatting `wcstoll`, `wcstombs`, `wcstoul`, `wcstoull`, `wcstoumax`, `wcswidth`, `wcsxfrm`, `wctob`, `wctomb`, `wctrans`, `wctype`, `wcwidth`, `wprintf` -- For `strprintf`, `LogPrint`, `LogPrintf` formatting characters don't need size specifiers. +- For `strprintf`, `LogInfo`, `LogDebug`, etc formatting characters don't need size specifiers. - *Rationale*: Dash Core uses tinyformat, which is type safe. Leave them out to avoid confusion. @@ -911,7 +952,7 @@ Strings and formatting - *Rationale*: Although this is guaranteed to be safe starting with C++11, `.data()` communicates the intent better. - - Do not use it when passing strings to `tfm::format`, `strprintf`, `LogPrint[f]`. + - Do not use it when passing strings to `tfm::format`, `strprintf`, `LogInfo`, `LogDebug`, etc. - *Rationale*: This is redundant. Tinyformat handles strings. diff --git a/src/httpserver.cpp b/src/httpserver.cpp index f31f197c25e1..f4b27507c020 100644 --- a/src/httpserver.cpp +++ b/src/httpserver.cpp @@ -438,7 +438,7 @@ bool InitHTTPServer() workQueueDepthExternal = std::max((long)gArgs.GetIntArg("-rpcexternalworkqueue", DEFAULT_HTTP_WORKQUEUE), 1L); g_external_usernames = SplitString(rpc_externaluser, ','); } - LogPrintfCategory(BCLog::HTTP, "creating work queue of depth %d external_depth %d\n", workQueueDepth, workQueueDepthExternal); + LogDebug(BCLog::HTTP, "creating work queue of depth %d external_depth %d\n", workQueueDepth, workQueueDepthExternal); g_work_queue = std::make_unique>(workQueueDepth, workQueueDepthExternal); // transfer ownership to eventBase/HTTP via .release() eventBase = base_ctr.release(); @@ -459,9 +459,8 @@ static std::vector g_thread_http_workers; void StartHTTPServer() { - LogPrint(BCLog::HTTP, "Starting HTTP server\n"); int rpcThreads = std::max((long)gArgs.GetIntArg("-rpcthreads", DEFAULT_HTTP_THREADS), 1L); - LogPrintfCategory(BCLog::HTTP, "starting %d worker threads\n", rpcThreads); + LogInfo("Starting HTTP server with %d worker threads\n", rpcThreads); g_thread_http = std::thread(ThreadHTTP, eventBase); for (int i = 0; i < rpcThreads; i++) { diff --git a/src/init.cpp b/src/init.cpp index dcd0923bdd5d..b9b529600fb7 100644 --- a/src/init.cpp +++ b/src/init.cpp @@ -2101,7 +2101,7 @@ bool AppInitMain(NodeContext& node, interfaces::BlockAndHeaderTipInfo* tip_info) uiInterface.InitMessage(_("Verifying blocks…").translated); auto check_blocks = args.GetIntArg("-checkblocks", DEFAULT_CHECKBLOCKS); if (chainman.m_blockman.m_have_pruned && check_blocks > MIN_BLOCKS_TO_KEEP) { - LogPrintfCategory(BCLog::PRUNE, "pruned datadir may not have more than %d blocks; only checking available blocks\n", + LogWarning("pruned datadir may not have more than %d blocks; only checking available blocks\n", MIN_BLOCKS_TO_KEEP); } maybe_verify_error = VerifyLoadedChainstate(chainman, diff --git a/src/init/common.cpp b/src/init/common.cpp index d04764d0c73e..a1a9f0c31a51 100644 --- a/src/init/common.cpp +++ b/src/init/common.cpp @@ -70,6 +70,7 @@ void AddLoggingArgs(ArgsManager& argsman) argsman.AddArg("-logthreadnames", strprintf("Prepend debug output with name of the originating thread (only available on platforms supporting thread_local) (default: %u)", DEFAULT_LOGTHREADNAMES), ArgsManager::ALLOW_ANY | ArgsManager::DEBUG_ONLY, OptionsCategory::DEBUG_TEST); argsman.AddArg("-logsourcelocations", strprintf("Prepend debug output with name of the originating source location (source file, line number and function name) (default: %u)", DEFAULT_LOGSOURCELOCATIONS), ArgsManager::ALLOW_ANY, OptionsCategory::DEBUG_TEST); argsman.AddArg("-logtimemicros", strprintf("Add microsecond precision to debug timestamps (default: %u)", DEFAULT_LOGTIMEMICROS), ArgsManager::ALLOW_ANY | ArgsManager::DEBUG_ONLY, OptionsCategory::DEBUG_TEST); + argsman.AddArg("-loglevelalways", strprintf("Always prepend a category and level (default: %u)", DEFAULT_LOGLEVELALWAYS), ArgsManager::ALLOW_ANY, OptionsCategory::DEBUG_TEST); argsman.AddArg("-printtoconsole", "Send trace/debug info to console (default: 1 when no -daemon. To disable logging to file, set -nodebuglogfile)", ArgsManager::ALLOW_ANY, OptionsCategory::DEBUG_TEST); argsman.AddArg("-shrinkdebugfile", "Shrink debug.log file on client startup (default: 1 when no -debug)", ArgsManager::ALLOW_ANY, OptionsCategory::DEBUG_TEST); } @@ -83,6 +84,7 @@ void SetLoggingOptions(const ArgsManager& args) LogInstance().m_log_time_micros = args.GetBoolArg("-logtimemicros", DEFAULT_LOGTIMEMICROS); LogInstance().m_log_threadnames = args.GetBoolArg("-logthreadnames", DEFAULT_LOGTHREADNAMES); LogInstance().m_log_sourcelocations = args.GetBoolArg("-logsourcelocations", DEFAULT_LOGSOURCELOCATIONS); + LogInstance().m_always_print_category_level = args.GetBoolArg("-loglevelalways", DEFAULT_LOGLEVELALWAYS); fLogIPs = args.GetBoolArg("-logips", DEFAULT_LOGIPS); } diff --git a/src/logging.cpp b/src/logging.cpp index d59d1a86aa9b..3cf1b12a433e 100644 --- a/src/logging.cpp +++ b/src/logging.cpp @@ -125,9 +125,9 @@ bool BCLog::Logger::WillLogCategory(BCLog::LogFlags category) const bool BCLog::Logger::WillLogCategoryLevel(BCLog::LogFlags category, BCLog::Level level) const { - // Log messages at Warning and Error level unconditionally, so that + // Log messages at Info, Warning and Error level unconditionally, so that // important troubleshooting information doesn't get lost. - if (level >= BCLog::Level::Warning) return true; + if (level >= BCLog::Level::Info) return true; if (!WillLogCategory(category)) return false; @@ -215,7 +215,7 @@ bool GetLogCategory(BCLog::LogFlags& flag, const std::string& str) return false; } -std::string BCLog::Logger::LogLevelToStr(BCLog::Level level) const +std::string BCLog::Logger::LogLevelToStr(BCLog::Level level) { switch (level) { case BCLog::Level::Trace: @@ -228,8 +228,6 @@ std::string BCLog::Logger::LogLevelToStr(BCLog::Level level) const return "warning"; case BCLog::Level::Error: return "error"; - case BCLog::Level::None: - return ""; } assert(false); } @@ -346,8 +344,6 @@ static std::optional GetLogLevel(const std::string& level_str) return BCLog::Level::Warning; } else if (level_str == "error") { return BCLog::Level::Error; - } else if (level_str == "none") { - return BCLog::Level::None; } else { return std::nullopt; } @@ -382,7 +378,7 @@ static constexpr std::array LogLevelsList() std::string BCLog::Logger::LogLevelsString() const { const auto& levels = LogLevelsList(); - return Join(std::vector{levels.begin(), levels.end()}, ", ", [this](BCLog::Level level) { return LogLevelToStr(level); }); + return Join(std::vector{levels.begin(), levels.end()}, ", ", [](BCLog::Level level) { return LogLevelToStr(level); }); } std::string BCLog::Logger::LogTimestampStr(const std::string& str) @@ -432,29 +428,39 @@ namespace BCLog { } } // namespace BCLog -void BCLog::Logger::LogPrintStr(const std::string& str, const std::string& logging_function, const std::string& source_file, int source_line, BCLog::LogFlags category, BCLog::Level level) +std::string BCLog::Logger::GetLogPrefix(BCLog::LogFlags category, BCLog::Level level) const { - StdLockGuard scoped_lock(m_cs); - std::string str_prefixed = LogEscapeMessage(str); + if (category == LogFlags::NONE) category = LogFlags::ALL; - if ((category != LogFlags::NONE || level != Level::None) && m_started_new_line) { - std::string s{"["}; + const bool has_category{m_always_print_category_level || category != LogFlags::ALL}; - if (category != LogFlags::NONE) { - s += LogCategoryToStr(category); - } + // If there is no category, Info is implied + if (!has_category && level == Level::Info) return {}; - if (category != LogFlags::NONE && level != Level::None) { - // Only add separator if both flag and level are not NONE - s += ":"; - } + std::string s{"["}; + if (has_category) { + s += LogCategoryToStr(category); + } - if (level != Level::None) { - s += LogLevelToStr(level); - } + if (m_always_print_category_level || !has_category || level != Level::Debug) { + // If there is a category, Debug is implied, so don't add the level + + // Only add separator if we have a category + if (has_category) s += ":"; + s += Logger::LogLevelToStr(level); + } + + s += "] "; + return s; +} - s += "] "; - str_prefixed.insert(0, s); +void BCLog::Logger::LogPrintStr(const std::string& str, const std::string& logging_function, const std::string& source_file, int source_line, BCLog::LogFlags category, BCLog::Level level) +{ + StdLockGuard scoped_lock(m_cs); + std::string str_prefixed = LogEscapeMessage(str); + + if (m_started_new_line) { + str_prefixed.insert(0, GetLogPrefix(category, level)); } if (m_log_sourcelocations && m_started_new_line) { diff --git a/src/logging.h b/src/logging.h index 2488d056cd79..ff7f98223585 100644 --- a/src/logging.h +++ b/src/logging.h @@ -25,6 +25,7 @@ static const bool DEFAULT_LOGIPS = false; static const bool DEFAULT_LOGTIMESTAMPS = true; static const bool DEFAULT_LOGTHREADNAMES = false; static const bool DEFAULT_LOGSOURCELOCATIONS = false; +static constexpr bool DEFAULT_LOGLEVELALWAYS = false; extern const char * const DEFAULT_DEBUGLOGFILE; extern bool fLogThreadNames; @@ -98,7 +99,6 @@ namespace BCLog { Info, // Default Warning, Error, - None, // Internal use only }; constexpr auto DEFAULT_LOG_LEVEL{Level::Debug}; @@ -142,10 +142,13 @@ namespace BCLog { bool m_log_time_micros = DEFAULT_LOGTIMEMICROS; bool m_log_threadnames = DEFAULT_LOGTHREADNAMES; bool m_log_sourcelocations = DEFAULT_LOGSOURCELOCATIONS; + bool m_always_print_category_level = DEFAULT_LOGLEVELALWAYS; fs::path m_file_path; std::atomic m_reopen_file{false}; + std::string GetLogPrefix(LogFlags category, Level level) const; + /** Send a string to the log output */ void LogPrintStr(const std::string& str, const std::string& logging_function, const std::string& source_file, int source_line, BCLog::LogFlags category, BCLog::Level level); @@ -216,7 +219,7 @@ namespace BCLog { std::string LogLevelsString() const; //! Returns the string representation of a log level. - std::string LogLevelToStr(BCLog::Level level) const; + static std::string LogLevelToStr(BCLog::Level level); bool DefaultShrinkDebugFile() const; }; @@ -275,22 +278,17 @@ static inline void LogPrintf_(const std::string& logging_function, const std::st #define LogPrintLevel_(category, level, ...) LogPrintf_(__func__, __FILE__, __LINE__, category, level, __VA_ARGS__) // Log unconditionally. -#define LogPrintf(...) LogPrintLevel_(BCLog::LogFlags::NONE, BCLog::Level::None, __VA_ARGS__) +#define LogInfo(...) LogPrintLevel_(BCLog::LogFlags::ALL, BCLog::Level::Info, __VA_ARGS__) +#define LogWarning(...) LogPrintLevel_(BCLog::LogFlags::ALL, BCLog::Level::Warning, __VA_ARGS__) +#define LogError(...) LogPrintLevel_(BCLog::LogFlags::ALL, BCLog::Level::Error, __VA_ARGS__) -// Log unconditionally, prefixing the output with the passed category name. -#define LogPrintfCategory(category, ...) LogPrintLevel_(category, BCLog::Level::None, __VA_ARGS__) +// Deprecated unconditional logging. +#define LogPrintf(...) LogInfo(__VA_ARGS__) +#define LogPrintfCategory(category, ...) LogPrintLevel_(category, BCLog::Level::Info, __VA_ARGS__) // Use a macro instead of a function for conditional logging to prevent // evaluating arguments when logging for the category is not enabled. -// Log conditionally, prefixing the output with the passed category name. -#define LogPrint(category, ...) \ - do { \ - if (LogAcceptCategory((category), BCLog::Level::Debug)) { \ - LogPrintLevel_(category, BCLog::Level::None, __VA_ARGS__); \ - } \ - } while (0) - // Log conditionally, prefixing the output with the passed category name and severity level. #define LogPrintLevel(category, level, ...) \ do { \ @@ -299,4 +297,11 @@ static inline void LogPrintf_(const std::string& logging_function, const std::st } \ } while (0) +// Log conditionally, prefixing the output with the passed category name. +#define LogDebug(category, ...) LogPrintLevel(category, BCLog::Level::Debug, __VA_ARGS__) +#define LogTrace(category, ...) LogPrintLevel(category, BCLog::Level::Trace, __VA_ARGS__) + +// Deprecated conditional logging +#define LogPrint(category, ...) LogDebug(category, __VA_ARGS__) + #endif // BITCOIN_LOGGING_H diff --git a/src/net_processing.cpp b/src/net_processing.cpp index d69a44eeb4cb..2c08d178b87d 100644 --- a/src/net_processing.cpp +++ b/src/net_processing.cpp @@ -4806,7 +4806,7 @@ void PeerManagerImpl::ProcessMessage( } if (received_new_header) { - LogPrintfCategory(BCLog::NET, "Saw new cmpctblock header hash=%s peer=%d\n", + LogInfo("Saw new cmpctblock header hash=%s peer=%d\n", blockhash.ToString(), pfrom.GetId()); } diff --git a/src/test/logging_tests.cpp b/src/test/logging_tests.cpp index accc83824350..afaf78f00879 100644 --- a/src/test/logging_tests.cpp +++ b/src/test/logging_tests.cpp @@ -95,27 +95,32 @@ BOOST_FIXTURE_TEST_CASE(logging_LogPrintf_, LogSetup) { LogInstance().m_log_sourcelocations = true; LogPrintf_("fn1", "src1", 1, BCLog::LogFlags::NET, BCLog::Level::Debug, "foo1: %s", "bar1\n"); - LogPrintf_("fn2", "src2", 2, BCLog::LogFlags::NET, BCLog::Level::None, "foo2: %s", "bar2\n"); - LogPrintf_("fn3", "src3", 3, BCLog::LogFlags::NONE, BCLog::Level::Debug, "foo3: %s", "bar3\n"); - LogPrintf_("fn4", "src4", 4, BCLog::LogFlags::NONE, BCLog::Level::None, "foo4: %s", "bar4\n"); + LogPrintf_("fn2", "src2", 2, BCLog::LogFlags::NET, BCLog::Level::Info, "foo2: %s", "bar2\n"); + LogPrintf_("fn3", "src3", 3, BCLog::LogFlags::ALL, BCLog::Level::Debug, "foo3: %s", "bar3\n"); + LogPrintf_("fn4", "src4", 4, BCLog::LogFlags::ALL, BCLog::Level::Info, "foo4: %s", "bar4\n"); + LogPrintf_("fn5", "src5", 5, BCLog::LogFlags::NONE, BCLog::Level::Debug, "foo5: %s", "bar5\n"); + LogPrintf_("fn6", "src6", 6, BCLog::LogFlags::NONE, BCLog::Level::Info, "foo6: %s", "bar6\n"); std::ifstream file{tmp_log_path}; std::vector log_lines; for (std::string log; std::getline(file, log);) { log_lines.push_back(log); } std::vector expected = { - "[src1:1] [fn1] [net:debug] foo1: bar1", - "[src2:2] [fn2] [net] foo2: bar2", + "[src1:1] [fn1] [net] foo1: bar1", + "[src2:2] [fn2] [net:info] foo2: bar2", "[src3:3] [fn3] [debug] foo3: bar3", "[src4:4] [fn4] foo4: bar4", + "[src5:5] [fn5] [debug] foo5: bar5", + "[src6:6] [fn6] foo6: bar6", }; BOOST_CHECK_EQUAL_COLLECTIONS(log_lines.begin(), log_lines.end(), expected.begin(), expected.end()); } -BOOST_FIXTURE_TEST_CASE(logging_LogPrintMacros, LogSetup) +BOOST_FIXTURE_TEST_CASE(logging_LogPrintMacrosDeprecated, LogSetup) { LogPrintf("foo5: %s\n", "bar5"); LogPrint(BCLog::NET, "foo6: %s\n", "bar6"); + LogPrintLevel(BCLog::NET, BCLog::Level::Trace, "foo4: %s\n", "bar4"); // not logged LogPrintLevel(BCLog::NET, BCLog::Level::Debug, "foo7: %s\n", "bar7"); LogPrintLevel(BCLog::NET, BCLog::Level::Info, "foo8: %s\n", "bar8"); LogPrintLevel(BCLog::NET, BCLog::Level::Warning, "foo9: %s\n", "bar9"); @@ -129,11 +134,32 @@ BOOST_FIXTURE_TEST_CASE(logging_LogPrintMacros, LogSetup) std::vector expected = { "foo5: bar5", "[net] foo6: bar6", - "[net:debug] foo7: bar7", + "[net] foo7: bar7", "[net:info] foo8: bar8", "[net:warning] foo9: bar9", "[net:error] foo10: bar10", - "[validation] foo11: bar11", + "[validation:info] foo11: bar11", + }; + BOOST_CHECK_EQUAL_COLLECTIONS(log_lines.begin(), log_lines.end(), expected.begin(), expected.end()); +} + +BOOST_FIXTURE_TEST_CASE(logging_LogPrintMacros, LogSetup) +{ + LogTrace(BCLog::NET, "foo6: %s\n", "bar6"); // not logged + LogDebug(BCLog::NET, "foo7: %s\n", "bar7"); + LogInfo("foo8: %s\n", "bar8"); + LogWarning("foo9: %s\n", "bar9"); + LogError("foo10: %s\n", "bar10"); + std::ifstream file{tmp_log_path}; + std::vector log_lines; + for (std::string log; std::getline(file, log);) { + log_lines.push_back(log); + } + std::vector expected = { + "[net] foo7: bar7", + "foo8: bar8", + "[warning] foo9: bar9", + "[error] foo10: bar10", }; BOOST_CHECK_EQUAL_COLLECTIONS(log_lines.begin(), log_lines.end(), expected.begin(), expected.end()); } diff --git a/src/torcontrol.cpp b/src/torcontrol.cpp index 67958c521596..b03063857fe3 100644 --- a/src/torcontrol.cpp +++ b/src/torcontrol.cpp @@ -431,7 +431,7 @@ void TorController::add_onion_cb(TorControlConnection& _conn, const TorControlRe return; } service = LookupNumeric(std::string(service_id+".onion"), Params().GetDefaultPort()); - LogPrintfCategory(BCLog::TOR, "Got service ID %s, advertising service %s\n", service_id, service.ToStringAddrPort()); + LogInfo("Got tor service ID %s, advertising service %s\n", service_id, service.ToStringAddrPort()); if (WriteBinaryFile(GetPrivateKeyFile(), private_key)) { LogPrint(BCLog::TOR, "Cached service private key to %s\n", fs::PathToString(GetPrivateKeyFile())); } else { diff --git a/test/lint/lint-format-strings.py b/test/lint/lint-format-strings.py index f8ccef352af6..6992343d6cf6 100755 --- a/test/lint/lint-format-strings.py +++ b/test/lint/lint-format-strings.py @@ -20,6 +20,11 @@ 'fprintf,1', 'tfm::format,1', # Assuming tfm::::format(std::ostream&, ... 'LogConnectFailure,1', + 'LogError,0', + 'LogWarning,0', + 'LogInfo,0', + 'LogDebug,1', + 'LogTrace,1', 'LogPrint,1', 'LogPrintf,0', 'LogPrintfCategory,1', From 52f6409d6b2e583fb02fea7e639e9fe40ddcd98c Mon Sep 17 00:00:00 2001 From: fanquake Date: Mon, 15 Jan 2024 10:25:32 +0000 Subject: [PATCH 3/9] Merge bitcoin/bitcoin#29241: doc: Add missing backtick in developer notes logging section c003562120c193c296ac754b4bb8cff02bbbe4dc doc: Add missing backtick in developer notes logging section (Fabian Jahr) Pull request description: Newly added logging section from https://github.com/bitcoin/bitcoin/pull/28318 is missing a single backtick. Also fixes some minor punctuation errors in that section. ACKs for top commit: jonatack: ACK c003562120c193c296ac754b4bb8cff02bbbe4dc alfonsoromanz: ACK c003562120c193c296ac754b4bb8cff02bbbe4dc Tree-SHA512: 2f75f9472d212ce7c7ebf3e7404f86b3bd8c695f63e2a7447d7a55bb54dcdb5e3bfd15e8eac5b92efbdcf1216c4e8d699cae0250d021f72b9d1c32a7db91989d --- doc/developer-notes.md | 12 ++++++------ 1 file changed, 6 insertions(+), 6 deletions(-) diff --git a/doc/developer-notes.md b/doc/developer-notes.md index bd9ca2db7c3a..f8bb574015e1 100644 --- a/doc/developer-notes.md +++ b/doc/developer-notes.md @@ -744,25 +744,25 @@ logging messages. They should be used as follows: Note that `LogPrint(BCLog::CATEGORY, fmt, params...)` is a deprecated alias for `LogDebug`. -- `LogInfo(fmt, params...)` should only be used rarely, eg for startup +- `LogInfo(fmt, params...)` should only be used rarely, e.g. for startup messages or for infrequent and important events such as a new block tip being found or a new outbound connection being made. These log messages - are unconditional so care must be taken that they can't be used by an + are unconditional, so care must be taken that they can't be used by an attacker to fill up storage. Note that `LogPrintf(fmt, params...)` is a deprecated alias for `LogInfo`. - `LogError(fmt, params...)` should be used in place of `LogInfo` for severe problems that require the node (or a subsystem) to shut down - entirely (eg, insufficient storage space). + entirely (e.g., insufficient storage space). - `LogWarning(fmt, params...)` should be used in place of `LogInfo` for severe problems that the node admin should address, but are not - severe enough to warrant shutting down the node (eg, system time + severe enough to warrant shutting down the node (e.g., system time appears to be wrong, unknown soft fork appears to have activated). -- `LogTrace(BCLog::CATEGORY, fmt, params...) should be used in place of +- `LogTrace(BCLog::CATEGORY, fmt, params...)` should be used in place of `LogDebug` for log messages that would be unusable on a production - system, eg due to being too noisy in normal use, or too resource + system, e.g. due to being too noisy in normal use, or too resource intensive to process. These will be logged if the startup options `-debug=category -loglevel=category:trace` or `-debug=1 -loglevel=trace` are selected. From 5a6bcba7cf9a268cb5a32ed2977789e360cbcec2 Mon Sep 17 00:00:00 2001 From: Ava Chow Date: Tue, 6 Feb 2024 12:53:19 -0500 Subject: [PATCH 4/9] Merge bitcoin/bitcoin#29375: wallet: remove unused 'accept_no_keys' arg from decryption process 2bb25ce5023c4d56c8b11e9c75f9f8bd69894452 wallet: remove unused 'accept_no_keys' arg from decryption process (furszy) Pull request description: Found it while reviewing other PR. Couldn't contain myself from cleaning it up. The wallet decryption process (`CheckDecryptionKey()` and `Unlock()`) contains an arg 'accept_no_keys,' introduced in #13926, that has never been used. Additionally, this also removes the unimplemented `SplitWalletPath` function. ACKs for top commit: delta1: ACK 2bb25ce5023c4d56c8b11e9c75f9f8bd69894452 epiccurious: utACK 2bb25ce5023c4d56c8b11e9c75f9f8bd69894452. achow101: ACK 2bb25ce5023c4d56c8b11e9c75f9f8bd69894452 theStack: Code-review ACK 2bb25ce5023c4d56c8b11e9c75f9f8bd69894452 Tree-SHA512: e0537c994be19ca0032551d8a64cf1755c8997e04d21ee0522b31de26ad90b9eb02a8b415448257b60bced484b9d2a23b37586e12dc5ff6e35bdd8ff2165c6bf --- src/wallet/db.h | 1 - src/wallet/scriptpubkeyman.cpp | 8 ++++---- src/wallet/scriptpubkeyman.h | 6 +++--- src/wallet/wallet.cpp | 8 ++++---- src/wallet/wallet.h | 4 ++-- 5 files changed, 13 insertions(+), 14 deletions(-) diff --git a/src/wallet/db.h b/src/wallet/db.h index dba86c8a90bd..fbd0e989c59c 100644 --- a/src/wallet/db.h +++ b/src/wallet/db.h @@ -20,7 +20,6 @@ class ArgsManager; struct bilingual_str; namespace wallet { -void SplitWalletPath(const fs::path& wallet_path, fs::path& env_directory, std::string& database_filename); /** RAII class that provides access to a WalletDatabase */ class DatabaseBatch diff --git a/src/wallet/scriptpubkeyman.cpp b/src/wallet/scriptpubkeyman.cpp index 13889d68416f..9615bbf7e0f3 100644 --- a/src/wallet/scriptpubkeyman.cpp +++ b/src/wallet/scriptpubkeyman.cpp @@ -185,7 +185,7 @@ isminetype LegacyScriptPubKeyMan::IsMine(const CTxDestination& dest) const return IsMine(script); } -bool LegacyScriptPubKeyMan::CheckDecryptionKey(const CKeyingMaterial& master_key, bool accept_no_keys) +bool LegacyScriptPubKeyMan::CheckDecryptionKey(const CKeyingMaterial& master_key) { { LOCK(cs_KeyStore); @@ -221,7 +221,7 @@ bool LegacyScriptPubKeyMan::CheckDecryptionKey(const CKeyingMaterial& master_key if (keyFail) { return false; } - if (!keyPass && !accept_no_keys && (m_hd_chain.IsNull() || !m_hd_chain.IsCrypted())) { + if (!keyPass && (m_hd_chain.IsNull() || !m_hd_chain.IsCrypted())) { return false; } @@ -1868,7 +1868,7 @@ isminetype DescriptorScriptPubKeyMan::IsMine(const CTxDestination& dest) const return IsMine(script); } -bool DescriptorScriptPubKeyMan::CheckDecryptionKey(const CKeyingMaterial& master_key, bool accept_no_keys) +bool DescriptorScriptPubKeyMan::CheckDecryptionKey(const CKeyingMaterial& master_key) { LOCK(cs_desc_man); if (!m_map_keys.empty()) { @@ -1894,7 +1894,7 @@ bool DescriptorScriptPubKeyMan::CheckDecryptionKey(const CKeyingMaterial& master LogPrintf("The wallet is probably corrupted: Some keys decrypt but not all.\n"); throw std::runtime_error("Error unlocking wallet: some keys decrypt but not all. Your wallet file may be corrupt."); } - if (keyFail || (!keyPass && !accept_no_keys)) { + if (keyFail || !keyPass) { return false; } m_decryption_thoroughly_checked = true; diff --git a/src/wallet/scriptpubkeyman.h b/src/wallet/scriptpubkeyman.h index 5f577ea83b47..631b442e9ef4 100644 --- a/src/wallet/scriptpubkeyman.h +++ b/src/wallet/scriptpubkeyman.h @@ -181,7 +181,7 @@ class ScriptPubKeyMan virtual isminetype IsMine(const CTxDestination& dest) const { return ISMINE_NO; } //! Check that the given decryption key is valid for this ScriptPubKeyMan, i.e. it decrypts all of the keys handled by it. - virtual bool CheckDecryptionKey(const CKeyingMaterial& master_key, bool accept_no_keys = false) { return false; } + virtual bool CheckDecryptionKey(const CKeyingMaterial& master_key) { return false; } virtual bool Encrypt(const CKeyingMaterial& master_key, WalletBatch* batch) { return false; } virtual util::Result GetReservedDestination(bool internal, int64_t& index, CKeyPool& keypool) { return util::Error{Untranslated("Not supported")}; } @@ -341,7 +341,7 @@ class LegacyScriptPubKeyMan : public ScriptPubKeyMan, public FillableSigningProv isminetype IsMine(const CScript& script) const override; isminetype IsMine(const CTxDestination& dest) const override; - bool CheckDecryptionKey(const CKeyingMaterial& master_key, bool accept_no_keys = false) override; + bool CheckDecryptionKey(const CKeyingMaterial& master_key) override; bool Encrypt(const CKeyingMaterial& master_key, WalletBatch* batch) override; util::Result GetReservedDestination(bool internal, int64_t& index, CKeyPool& keypool) override; @@ -582,7 +582,7 @@ class DescriptorScriptPubKeyMan : public ScriptPubKeyMan isminetype IsMine(const CScript& script) const override; isminetype IsMine(const CTxDestination& dest) const override; - bool CheckDecryptionKey(const CKeyingMaterial& master_key, bool accept_no_keys = false) override; + bool CheckDecryptionKey(const CKeyingMaterial& master_key) override; bool Encrypt(const CKeyingMaterial& master_key, WalletBatch* batch) override; util::Result GetReservedDestination(bool internal, int64_t& index, CKeyPool& keypool) override; diff --git a/src/wallet/wallet.cpp b/src/wallet/wallet.cpp index 85adcf5ad527..7adee0f99aa4 100644 --- a/src/wallet/wallet.cpp +++ b/src/wallet/wallet.cpp @@ -3684,7 +3684,7 @@ bool CWallet::Lock(bool fAllowMixing) return true; } -bool CWallet::Unlock(const SecureString& strWalletPassphrase, bool fForMixingOnly, bool accept_no_keys) +bool CWallet::Unlock(const SecureString& strWalletPassphrase, bool fForMixingOnly) { if (!IsLocked()) // was already fully unlocked, not only for mixing return true; @@ -3700,7 +3700,7 @@ bool CWallet::Unlock(const SecureString& strWalletPassphrase, bool fForMixingOnl return false; if (!crypter.Decrypt(pMasterKey.second.vchCryptedKey, _vMasterKey)) continue; // try another master key - if (Unlock(_vMasterKey, fForMixingOnly, accept_no_keys)) { + if (Unlock(_vMasterKey, fForMixingOnly)) { // Now that we've unlocked, upgrade the key metadata UpgradeKeyMetadata(); // Now that we've unlocked, upgrade the descriptor cache @@ -3717,12 +3717,12 @@ bool CWallet::Unlock(const SecureString& strWalletPassphrase, bool fForMixingOnl return false; } -bool CWallet::Unlock(const CKeyingMaterial& vMasterKeyIn, bool fForMixingOnly, bool accept_no_keys) +bool CWallet::Unlock(const CKeyingMaterial& vMasterKeyIn, bool fForMixingOnly) { { LOCK(cs_wallet); for (const auto& spk_man_pair : m_spk_managers) { - if (!spk_man_pair.second->CheckDecryptionKey(vMasterKeyIn, accept_no_keys)) { + if (!spk_man_pair.second->CheckDecryptionKey(vMasterKeyIn)) { return false; } } diff --git a/src/wallet/wallet.h b/src/wallet/wallet.h index 8e0966d4bfae..812ad7ecb56c 100644 --- a/src/wallet/wallet.h +++ b/src/wallet/wallet.h @@ -262,7 +262,7 @@ class CWallet final : public WalletStorage, public interfaces::Chain::Notificati //! if fOnlyMixingAllowed is true, only mixing should be allowed in unlocked wallet bool fOnlyMixingAllowed; - bool Unlock(const CKeyingMaterial& vMasterKeyIn, bool fForMixingOnly = false, bool accept_no_keys = false); + bool Unlock(const CKeyingMaterial& vMasterKeyIn, bool fForMixingOnly = false); std::atomic fAbortRescan{false}; // reset by WalletRescanReserver::reserve() std::atomic fScanningWallet{false}; // controlled by WalletRescanReserver @@ -605,7 +605,7 @@ class CWallet final : public WalletStorage, public interfaces::Chain::Notificati // Used to prevent concurrent calls to walletpassphrase RPC. Mutex m_unlock_mutex; - bool Unlock(const SecureString& strWalletPassphrase, bool fForMixingOnly = false, bool accept_no_keys = false); + bool Unlock(const SecureString& strWalletPassphrase, bool fForMixingOnly = false); bool ChangeWalletPassphrase(const SecureString& strOldWalletPassphrase, const SecureString& strNewWalletPassphrase); bool EncryptWallet(const SecureString& strWalletPassphrase); From 1e7dc56e1303d6e39c8e1b8ca67e7cb245875db2 Mon Sep 17 00:00:00 2001 From: fanquake Date: Fri, 5 Jan 2024 17:35:18 +0000 Subject: [PATCH 5/9] Merge bitcoin/bitcoin#29117: wallettool: Always be able to dump a wallet's database d83bea42d1f0ffb0899a6de3556c489543468995 wallettool: Don't create CWallet when dumping DB (Andrew Chow) 40c80e36b1a204ed133acc403016a6cb1a92051e wallettool: Don't unilaterally reset wallet_instance if loading error (Ava Chow) Pull request description: https://github.com/bitcoin/bitcoin/issues/29109#issuecomment-1863449058 reports that a wallet with noncritical errors cannot be dumped with `bitcoin-wallet dump`. This was caused by an erroneous reset of the wallet pointer when the loading the wallet returns something other than `LOAD_OK`. Not all errors are errors that require aborting, so unilaterally resetting the pointer at that time is incorrect. The first commit resolves this issue. Furthermore, if a wallet has loading errors, that should not prevent the wallet tool from dumping the wallet. The wallet application logic should not get in the way of performing such a low level database operation, especially when it's primary usage is for debugging potentially corrupted wallets. The 2nd commit is taken from #28710 and changes the `dump` to stop at making a `WalletDatabase` rather than making a `CWallet` only to retrieve the underlying `WalletDatabase`. ACKs for top commit: furszy: Code review ACK d83bea42d1 BrandonOdiwuor: Code Review ACK d83bea42d1f0ffb0899a6de3556c489543468995 Tree-SHA512: 425d712dfff1002bd81272aca0bae1016f9126a3c89506f8cb7cf0a0ec9f33d0c03b8d03896394f3a45c2998e59047e19218dfd08dc8a5f40e8625134e886b0f --- src/wallet/dump.cpp | 7 ++----- src/wallet/dump.h | 4 ++-- src/wallet/wallettool.cpp | 12 ++++++++---- 3 files changed, 12 insertions(+), 11 deletions(-) diff --git a/src/wallet/dump.cpp b/src/wallet/dump.cpp index 8f86eb1b2ddc..9112cb0dc125 100644 --- a/src/wallet/dump.cpp +++ b/src/wallet/dump.cpp @@ -7,6 +7,7 @@ #include #include #include +#include #include #include @@ -19,7 +20,7 @@ namespace wallet { static const std::string DUMP_MAGIC = "BITCOIN_CORE_WALLET_DUMP"; uint32_t DUMP_VERSION = 1; -bool DumpWallet(const ArgsManager& args, CWallet& wallet, bilingual_str& error) +bool DumpWallet(const ArgsManager& args, WalletDatabase& db, bilingual_str& error) { // Get the dumpfile std::string dump_filename = args.GetArg("-dumpfile", ""); @@ -43,7 +44,6 @@ bool DumpWallet(const ArgsManager& args, CWallet& wallet, bilingual_str& error) HashWriter hasher{}; - WalletDatabase& db = wallet.GetDatabase(); std::unique_ptr batch = db.MakeBatch(); bool ret = true; @@ -88,9 +88,6 @@ bool DumpWallet(const ArgsManager& args, CWallet& wallet, bilingual_str& error) batch->CloseCursor(); batch.reset(); - // Close the wallet after we're done with it. The caller won't be doing this - wallet.Close(); - if (ret) { // Write the hash tfm::format(dump_file, "checksum,%s\n", HexStr(hasher.GetHash())); diff --git a/src/wallet/dump.h b/src/wallet/dump.h index ca2f3db03baf..0c0c05e3db80 100644 --- a/src/wallet/dump.h +++ b/src/wallet/dump.h @@ -14,9 +14,9 @@ struct bilingual_str; class ArgsManager; namespace wallet { -class CWallet; +class WalletDatabase; -bool DumpWallet(const ArgsManager& args, CWallet& wallet, bilingual_str& error); +bool DumpWallet(const ArgsManager& args, WalletDatabase& db, bilingual_str& error); bool CreateFromDump(const ArgsManager& args, const std::string& name, const fs::path& wallet_path, bilingual_str& error, std::vector& warnings); } // namespace wallet diff --git a/src/wallet/wallettool.cpp b/src/wallet/wallettool.cpp index b6e16fa0c4b2..1d0537f849dd 100644 --- a/src/wallet/wallettool.cpp +++ b/src/wallet/wallettool.cpp @@ -78,7 +78,6 @@ static std::shared_ptr MakeWallet(const std::string& name, const fs::pa } if (load_wallet_ret != DBErrors::LOAD_OK) { - wallet_instance = nullptr; if (load_wallet_ret == DBErrors::CORRUPT) { tfm::format(std::cerr, "Error loading %s: Wallet corrupted", name); return nullptr; @@ -214,10 +213,15 @@ bool ExecuteWalletToolFunc(const ArgsManager& args, const std::string& command) DatabaseOptions options; ReadDatabaseArgs(args, options); options.require_existing = true; - const std::shared_ptr wallet_instance = MakeWallet(name, path, args, options); - if (!wallet_instance) return false; + DatabaseStatus status; bilingual_str error; - bool ret = DumpWallet(args, *wallet_instance, error); + std::unique_ptr database = MakeDatabase(path, options, status, error); + if (!database) { + tfm::format(std::cerr, "%s\n", error.original); + return false; + } + + bool ret = DumpWallet(args, *database, error); if (!ret && !error.empty()) { tfm::format(std::cerr, "%s\n", error.original); return ret; From 85a3325b041eaed72a093faeacfb23945fb091a4 Mon Sep 17 00:00:00 2001 From: fanquake Date: Thu, 16 Nov 2023 10:35:30 +0000 Subject: [PATCH 6/9] Merge bitcoin/bitcoin#28605: Fix typos 43de4d3630274e1287179c86896ed4c2d8b9eff4 doc: fix typos (Sjors Provoost) Pull request description: This PR fixes typos found by lint-spelling.py using codespell 2.2.6. Our CI linter job uses codespell 2.2.5 and found fewer typos that I did locally. In any case it's happy now. ACKs for top commit: pablomartin4btc: re ACK 43de4d3630274e1287179c86896ed4c2d8b9eff4 Tree-SHA512: c032fe86cb49c924a468385653b31f309a9db68c478d70335bba3e65a1ff3826abe80284fe00a090ab5a509e1edbf17e476f6922fb15d055e50f1103dad2ccb0 --- ci/README.md | 2 +- depends/description.md | 2 +- src/net_processing.cpp | 4 ++-- src/qt/transactiontablemodel.cpp | 2 +- src/rpc/server.cpp | 2 +- src/test/fuzz/FuzzedDataProvider.h | 2 +- src/test/orphanage_tests.cpp | 2 +- src/test/script_tests.cpp | 2 +- src/test/versionbits_tests.cpp | 2 +- src/torcontrol.cpp | 2 +- src/txmempool.h | 2 +- src/wallet/spend.cpp | 4 ++-- src/wallet/wallet.h | 2 +- test/functional/test_framework/blockfilter.py | 2 +- test/lint/spelling.ignore-words.txt | 1 + 15 files changed, 17 insertions(+), 16 deletions(-) diff --git a/ci/README.md b/ci/README.md index 19228ffbaaf0..aa89a179f74b 100644 --- a/ci/README.md +++ b/ci/README.md @@ -61,5 +61,5 @@ in order. ### Cache In order to avoid rebuilding all dependencies for each build, the binaries are -cached and re-used when possible. Changes in the dependency-generator will +cached and reused when possible. Changes in the dependency-generator will trigger cache-invalidation and rebuilds as necessary. diff --git a/depends/description.md b/depends/description.md index cb69b81ae350..83b5bef859bb 100644 --- a/depends/description.md +++ b/depends/description.md @@ -27,7 +27,7 @@ etc), and as well as a hash of the same data for each recursive dependency. If any portion of a package's build recipe changes, it will be rebuilt as well as any other package that depends on it. If any of the main makefiles (Makefile, funcs.mk, etc) are changed, all packages will be rebuilt. After building, the -results are cached into a tarball that can be re-used and distributed. +results are cached into a tarball that can be reused and distributed. ### Package build results are (relatively) deterministic. diff --git a/src/net_processing.cpp b/src/net_processing.cpp index 2c08d178b87d..aa0504b8663e 100644 --- a/src/net_processing.cpp +++ b/src/net_processing.cpp @@ -3017,7 +3017,7 @@ void PeerManagerImpl::ProcessGetData(CNode& pfrom, Peer& peer, const std::atomic // of transactions relevant to them, without having to download the // entire memory pool. // Also, other nodes can use these messages to automatically request a - // transaction from some other peer that annnounced it, and stop + // transaction from some other peer that announced it, and stop // waiting for us to respond. // In normal operation, we often send NOTFOUND messages for parents of // transactions that we relay; if a peer is missing a parent, they may @@ -3956,7 +3956,7 @@ void PeerManagerImpl::ProcessMessage( return; } - // Log succesful connections unconditionally for outbound, but not for inbound as those + // Log successful connections unconditionally for outbound, but not for inbound as those // can be triggered by an attacker at high rate. if (!pfrom.IsInboundConn() || LogAcceptCategory(BCLog::NET, BCLog::Level::Debug)) { LogPrintf("New %s %s peer connected: version: %d, blocks=%d, peer=%d%s\n", diff --git a/src/qt/transactiontablemodel.cpp b/src/qt/transactiontablemodel.cpp index d337057b2fb6..296a9e88622d 100644 --- a/src/qt/transactiontablemodel.cpp +++ b/src/qt/transactiontablemodel.cpp @@ -237,7 +237,7 @@ class TransactionTablePriv // If a status update is needed (blocks came in since last check), // try to update the status of this transaction from the wallet. - // Otherwise, simply re-use the cached status. + // Otherwise, simply reuse the cached status. interfaces::WalletTxStatus wtx; int numBlocks; int64_t block_time; diff --git a/src/rpc/server.cpp b/src/rpc/server.cpp index 0ad0d236b287..d03a4995e7a6 100644 --- a/src/rpc/server.cpp +++ b/src/rpc/server.cpp @@ -424,7 +424,7 @@ static inline JSONRPCRequest transformNamedArguments(const JSONRPCRequest& in, c } // Process expected parameters. If any parameters were left unspecified in // the request before a parameter that was specified, null values need to be - // inserted at the unspecifed parameter positions, and the "hole" variable + // inserted at the unspecified parameter positions, and the "hole" variable // below tracks the number of null values that need to be inserted. // The "initial_hole_size" variable stores the size of the initial hole, // i.e. how many initial positional arguments were left unspecified. This is diff --git a/src/test/fuzz/FuzzedDataProvider.h b/src/test/fuzz/FuzzedDataProvider.h index 8a8214bd99fe..5903ed837917 100644 --- a/src/test/fuzz/FuzzedDataProvider.h +++ b/src/test/fuzz/FuzzedDataProvider.h @@ -158,7 +158,7 @@ FuzzedDataProvider::ConsumeRandomLengthString(size_t max_length) { // picking its contents. std::string result; - // Reserve the anticipated capaticity to prevent several reallocations. + // Reserve the anticipated capacity to prevent several reallocations. result.reserve(std::min(max_length, remaining_bytes_)); for (size_t i = 0; i < max_length && remaining_bytes_ != 0; ++i) { char next = ConvertUnsignedToSigned(data_ptr_[0]); diff --git a/src/test/orphanage_tests.cpp b/src/test/orphanage_tests.cpp index 5eb3f7379551..ed7aa50da3b7 100644 --- a/src/test/orphanage_tests.cpp +++ b/src/test/orphanage_tests.cpp @@ -109,7 +109,7 @@ BOOST_AUTO_TEST_CASE(DoS_mapOrphans) tx.vin[j].prevout.hash = txPrev->GetHash(); } BOOST_CHECK(SignSignature(keystore, *txPrev, tx, 0, SIGHASH_ALL)); - // Re-use same signature for other inputs + // Reuse same signature for other inputs // (they don't have to be valid for this test) for (unsigned int j = 1; j < tx.vin.size(); j++) tx.vin[j].scriptSig = tx.vin[0].scriptSig; diff --git a/src/test/script_tests.cpp b/src/test/script_tests.cpp index 4924c26e0af0..c8b3884b6d2d 100644 --- a/src/test/script_tests.cpp +++ b/src/test/script_tests.cpp @@ -1048,7 +1048,7 @@ BOOST_AUTO_TEST_CASE(script_CHECKMULTISIG23) BOOST_CHECK_MESSAGE(err == SCRIPT_ERR_OK, ScriptErrorString(err)); keys.clear(); - keys.push_back(key2); keys.push_back(key2); // Can't re-use sig + keys.push_back(key2); keys.push_back(key2); // Can't reuse sig CScript badsig1 = sign_multisig(scriptPubKey23, keys, CTransaction(txTo23)); BOOST_CHECK(!VerifyScript(badsig1, scriptPubKey23, gFlags, MutableTransactionSignatureChecker(&txTo23, 0, txFrom23.vout[0].nValue, MissingDataBehavior::ASSERT_FAIL), &err)); BOOST_CHECK_MESSAGE(err == SCRIPT_ERR_EVAL_FALSE, ScriptErrorString(err)); diff --git a/src/test/versionbits_tests.cpp b/src/test/versionbits_tests.cpp index fdcffaa22154..09abad134a43 100644 --- a/src/test/versionbits_tests.cpp +++ b/src/test/versionbits_tests.cpp @@ -430,7 +430,7 @@ BOOST_AUTO_TEST_CASE(versionbits_computeblockversion) uint32_t chain_all_vbits{0}; for (int i = 0; i < (int)Consensus::MAX_VERSION_BITS_DEPLOYMENTS; ++i) { const auto dep = static_cast(i); - // Check that no bits are re-used (within the same chain). This is + // Check that no bits are reused (within the same chain). This is // disallowed because the transition to FAILED (on timeout) does // not take precedence over STARTED/LOCKED_IN. So all softforks on // the same bit might overlap, even when non-overlapping start-end diff --git a/src/torcontrol.cpp b/src/torcontrol.cpp index b03063857fe3..f7c244687a96 100644 --- a/src/torcontrol.cpp +++ b/src/torcontrol.cpp @@ -249,7 +249,7 @@ std::map ParseTorReplyMapping(const std::string &s) /** * Unescape value. Per https://spec.torproject.org/control-spec section 2.1.1: * - * For future-proofing, controller implementors MAY use the following + * For future-proofing, controller implementers MAY use the following * rules to be compatible with buggy Tor implementations and with * future ones that implement the spec as intended: * diff --git a/src/txmempool.h b/src/txmempool.h index e28b6c856ff6..dec456a925c5 100644 --- a/src/txmempool.h +++ b/src/txmempool.h @@ -622,7 +622,7 @@ class CTxMemPool void removeRecursive(const CTransaction& tx, MemPoolRemovalReason reason) EXCLUSIVE_LOCKS_REQUIRED(cs); /** After reorg, filter the entries that would no longer be valid in the next block, and update * the entries' cached LockPoints if needed. The mempool does not have any knowledge of - * consensus rules. It just appplies the callable function and removes the ones for which it + * consensus rules. It just applies the callable function and removes the ones for which it * returns true. * @param[in] filter_final_and_mature Predicate that checks the relevant validation rules * and updates an entry's LockPoints. diff --git a/src/wallet/spend.cpp b/src/wallet/spend.cpp index 7687f88d8fad..945c5ff08c0b 100644 --- a/src/wallet/spend.cpp +++ b/src/wallet/spend.cpp @@ -1071,7 +1071,7 @@ static util::Result CreateTransactionInternal( } // Before we return success, we assume any change key will be used to prevent - // accidental re-use. + // accidental reuse. reservedest.KeepDestination(); wallet.WalletLogPrintf("Fee Calculation: Fee:%d Bytes:%u Tgt:%d (requested %d) Reason:\"%s\" Decay %.5f: Estimation: (%g - %g) %.2f%% %.1f/(%.1f %d mem %.1f out) Fail: (%g - %g) %.2f%% %.1f/(%.1f %d mem %.1f out)\n", @@ -1114,7 +1114,7 @@ util::Result CreateTransaction( CCoinControl tmp_cc = coin_control; tmp_cc.m_avoid_partial_spends = true; - // Re-use the change destination from the first creation attempt to avoid skipping BIP44 indexes + // Reuse the change destination from the first creation attempt to avoid skipping BIP44 indexes const int ungrouped_change_pos = txr_ungrouped.change_pos; if (ungrouped_change_pos != -1) { ExtractDestination(txr_ungrouped.tx->vout[ungrouped_change_pos].scriptPubKey, tmp_cc.destChange); diff --git a/src/wallet/wallet.h b/src/wallet/wallet.h index 812ad7ecb56c..1a4d570b36d4 100644 --- a/src/wallet/wallet.h +++ b/src/wallet/wallet.h @@ -167,7 +167,7 @@ extern const std::map WALLET_FLAG_CAVEATS; * Instantiating a ReserveDestination does not reserve an address. To do so, * GetReservedDestination() needs to be called on the object. Once an address has been * reserved, call KeepDestination() on the ReserveDestination object to make sure it is not - * returned. Call ReturnDestination() to return the address so it can be re-used (for + * returned. Call ReturnDestination() to return the address so it can be reused (for * example, if the address was used in a new transaction * and that transaction was not completed and needed to be aborted). * diff --git a/test/functional/test_framework/blockfilter.py b/test/functional/test_framework/blockfilter.py index 3d6b38a23d0c..a16aa3d34f3e 100644 --- a/test/functional/test_framework/blockfilter.py +++ b/test/functional/test_framework/blockfilter.py @@ -29,7 +29,7 @@ def bip158_basic_element_hash(script_pub_key, N, block_hash): def bip158_relevant_scriptpubkeys(node, block_hash): - """ Determines the basic filter relvant scriptPubKeys as defined in BIP158: + """ Determines the basic filter relevant scriptPubKeys as defined in BIP158: 'A basic filter MUST contain exactly the following items for each transaction in a block: - The previous output script (the script being spent) for each input, except for diff --git a/test/lint/spelling.ignore-words.txt b/test/lint/spelling.ignore-words.txt index 25cb5796e8fc..5ad51e3c2102 100644 --- a/test/lint/spelling.ignore-words.txt +++ b/test/lint/spelling.ignore-words.txt @@ -5,6 +5,7 @@ bu cachable clen crypted +debbugs fo fpr hights From 73244b249c6dc09cc74ab50fc8ac197d8c87fafe Mon Sep 17 00:00:00 2001 From: Andrew Chow Date: Fri, 1 Dec 2023 12:18:09 -0500 Subject: [PATCH 7/9] Merge bitcoin/bitcoin#28784: rpc: keep `.cookie` file if it was not generated 7cb9367157eb42ee06bc6fa024522cc14a80138d rpc: keep .cookie if it was not generated (Roman Zeyde) Pull request description: Otherwise, starting bitcoind twice may cause the `.cookie` file generated by the first instance to be deleted by the second instance shutdown (after failing to obtain a lock). ACKs for top commit: willcl-ark: re-ACK 7cb9367157eb42ee06bc6fa024522cc14a80138d achow101: ACK 7cb9367157eb42ee06bc6fa024522cc14a80138d kristapsk: re-ACK 7cb9367157eb42ee06bc6fa024522cc14a80138d stickies-v: ACK 7cb9367157eb42ee06bc6fa024522cc14a80138d Tree-SHA512: 0960dbc457975b0e0535f3d814824a879d7f85c9f1191537415b3fc253429a316a8e4badde56c8bc139778f132392983cec5fbe03891fb15ff61d3bc3f6e681b --- src/rpc/request.cpp | 8 +++++++- test/functional/feature_filelock.py | 5 +++++ 2 files changed, 12 insertions(+), 1 deletion(-) diff --git a/src/rpc/request.cpp b/src/rpc/request.cpp index f1cd256e3549..c00c0b552fc3 100644 --- a/src/rpc/request.cpp +++ b/src/rpc/request.cpp @@ -78,6 +78,8 @@ static fs::path GetAuthCookieFile(bool temp=false) return AbsPathForConfigVal(arg); } +static bool g_generated_cookie = false; + bool GenerateAuthCookie(std::string *cookie_out) { const size_t COOKIE_SIZE = 32; @@ -103,6 +105,7 @@ bool GenerateAuthCookie(std::string *cookie_out) LogPrintf("Unable to rename cookie authentication file %s to %s\n", fs::PathToString(filepath_tmp), fs::PathToString(filepath)); return false; } + g_generated_cookie = true; LogPrintf("Generated RPC authentication cookie %s\n", fs::PathToString(filepath)); if (cookie_out) @@ -129,7 +132,10 @@ bool GetAuthCookie(std::string *cookie_out) void DeleteAuthCookie() { try { - fs::remove(GetAuthCookieFile()); + if (g_generated_cookie) { + // Delete the cookie file if it was generated by this process + fs::remove(GetAuthCookieFile()); + } } catch (const fs::filesystem_error& e) { LogPrintf("%s: Unable to remove random auth cookie file: %s\n", __func__, fsbridge::get_filesystem_error_message(e)); } diff --git a/test/functional/feature_filelock.py b/test/functional/feature_filelock.py index 54c5956274f0..af5509a77db6 100755 --- a/test/functional/feature_filelock.py +++ b/test/functional/feature_filelock.py @@ -9,6 +9,8 @@ from test_framework.test_framework import BitcoinTestFramework from test_framework.test_node import ErrorMatch +from pathlib import Path + class FilelockTest(BitcoinTestFramework): def set_test_params(self): @@ -28,6 +30,9 @@ def run_test(self): expected_msg = f"Error: Cannot obtain a lock on data directory {datadir}. {self.config['environment']['PACKAGE_NAME']} is probably already running." self.nodes[1].assert_start_raises_init_error(extra_args=[f'-datadir={self.nodes[0].datadir}', '-noserver'], expected_msg=expected_msg) + cookie_file = Path(datadir) / ".cookie" + assert cookie_file.exists() # should not be deleted during the second bitcoind instance shutdown + if self.is_wallet_compiled(): def check_wallet_filelock(descriptors): wallet_name = ''.join([random.choice(string.ascii_lowercase) for _ in range(6)]) From 4cdd5dbb32170482ff86f2a59ca910e444938e4b Mon Sep 17 00:00:00 2001 From: fanquake Date: Mon, 11 Dec 2023 10:43:32 +0100 Subject: [PATCH 8/9] Merge bitcoin/bitcoin#29035: test: fix `addnode` functional test failure on OpenBSD fd0bde2793239bd6d60a2435fa28df915cedd7e6 test: fix `addnode` functional test failure on OpenBSD (Sebastian Falbesoner) Pull request description: This is the functional test counterpart of PR #28891 / commit 007d6f0e85bc329040bb405ef6016339518caa66 (unfortunately, I missed it back then and only ran the unit tests -- sorry for the noise). master branch on OpenBSD 7.4: ``` $ ./test/functional/rpc_net.py 2023-12-08T17:29:05.057000Z TestFramework (INFO): PRNG seed is: 6024296850131317403 2023-12-08T17:29:05.058000Z TestFramework (INFO): Initializing test directory /tmp/bitcoin_func_test_au3zchif 2023-12-08T17:29:05.618000Z TestFramework (INFO): Test getconnectioncount 2023-12-08T17:29:05.618000Z TestFramework (INFO): Test getpeerinfo 2023-12-08T17:29:06.643000Z TestFramework (INFO): Check getpeerinfo output before a version message was sent 2023-12-08T17:29:06.709000Z TestFramework (INFO): Test getnettotals 2023-12-08T17:29:06.773000Z TestFramework (INFO): Test getnetworkinfo 2023-12-08T17:29:06.978000Z TestFramework (INFO): Test addnode and getaddednodeinfo 2023-12-08T17:29:06.980000Z TestFramework (ERROR): Assertion failed Traceback (most recent call last): File "/home/thestack/bitcoin/test/functional/test_framework/test_framework.py", line 131, in main self.run_test() File "/home/thestack/bitcoin/./test/functional/rpc_net.py", line 65, in run_test self.test_addnode_getaddednodeinfo() File "/home/thestack/bitcoin/./test/functional/rpc_net.py", line 224, in test_addnode_getaddednodeinfo assert_raises_rpc_error(-23, "Node already added", self.nodes[0].addnode, node=ip_port2, command='add') File "/home/thestack/bitcoin/test/functional/test_framework/util.py", line 131, in assert_raises_rpc_error assert try_rpc(code, message, fun, *args, **kwds), "No exception raised" AssertionError: No exception raised ``` On the PR branch, the same call succeeds. ACKs for top commit: kevkevinpal: ACK [fd0bde2](https://github.com/bitcoin/bitcoin/pull/29035/commits/fd0bde2793239bd6d60a2435fa28df915cedd7e6) Tree-SHA512: ae20816fa4025c212e115ebd267b5e5784bfcdf0051219eb686faaade47ec4f91a3947af6d24258b159290000d2dcc3f6e65e788b83b8a9297282945dbdafbfb --- test/functional/rpc_net.py | 7 +++++-- 1 file changed, 5 insertions(+), 2 deletions(-) diff --git a/test/functional/rpc_net.py b/test/functional/rpc_net.py index 1e507be864b6..b70f1147fb9f 100755 --- a/test/functional/rpc_net.py +++ b/test/functional/rpc_net.py @@ -16,6 +16,7 @@ ) from itertools import product +import platform from test_framework.test_framework import DashTestFramework from test_framework.util import ( @@ -241,8 +242,10 @@ def test_addnode_getaddednodeinfo(self): ip_port = "127.0.0.1:{}".format(p2p_port(2)) self.nodes[0].addnode(node=ip_port, command='add') # try to add an equivalent ip - ip_port2 = "127.1:{}".format(p2p_port(2)) - assert_raises_rpc_error(-23, "Node already added", self.nodes[0].addnode, node=ip_port2, command='add') + # (note that OpenBSD doesn't support the IPv4 shorthand notation with omitted zero-bytes) + if platform.system() != "OpenBSD": + ip_port2 = "127.1:{}".format(p2p_port(2)) + assert_raises_rpc_error(-23, "Node already added", self.nodes[0].addnode, node=ip_port2, command='add') # check that the node has indeed been added added_nodes = self.nodes[0].getaddednodeinfo() assert_equal(len(added_nodes), 1) From 12a96f7c46cc390097cd7e20429810e81e5ace13 Mon Sep 17 00:00:00 2001 From: Andrew Chow Date: Thu, 2 Nov 2023 13:49:50 -0400 Subject: [PATCH 9/9] Merge bitcoin/bitcoin#24097: Replace RecursiveMutex m_cs_banned with Mutex, and rename it MIME-Version: 1.0 Content-Type: text/plain; charset=UTF-8 Content-Transfer-Encoding: 8bit 37d150d8c5ffcb2bddcd99951a739e97571194c7 refactor: Add more negative `!m_banned_mutex` thread safety annotations (Hennadii Stepanov) 0fb29087080a4e60d7c709ff5edf14e830ef3a69 refactor: replace RecursiveMutex m_banned_mutex with Mutex (w0xlt) 784c316f9cb664c9577cbfed1873bae573efd1b4 scripted-diff: rename m_cs_banned -> m_banned_mutex (w0xlt) 46709c5f27bf6cbc8eba1298b04bd079da2cdded refactor: Get rid of `BanMan::SetBannedSetDirty()` (Hennadii Stepanov) d88c0d8440cf640ef4f2c7a40b8b8b31bfd38f23 refactor: Get rid of `BanMan::BannedSetIsDirty()` (Hennadii Stepanov) Pull request description: This PR is an alternative to bitcoin/bitcoin#24092. Last two commit have been cherry-picked from the latter. ACKs for top commit: maflcko: ACK 37d150d8c5ffcb2bddcd99951a739e97571194c7 🎾 achow101: ACK 37d150d8c5ffcb2bddcd99951a739e97571194c7 theStack: Code-review ACK 37d150d8c5ffcb2bddcd99951a739e97571194c7 vasild: ACK 37d150d8c5ffcb2bddcd99951a739e97571194c7 Tree-SHA512: 5e9d40101a09af6e0645a6ede67432ea68631a1b960f9e6af0ad07415ca7718a30fcc1aad5182d1d5265dc54c26aba2008fc9973840255c09adbab8fedf10075 --- src/banman.cpp | 43 ++++++++++++++++--------------------------- src/banman.h | 39 ++++++++++++++++++--------------------- 2 files changed, 34 insertions(+), 48 deletions(-) diff --git a/src/banman.cpp b/src/banman.cpp index 28e333f5967d..abf459476132 100644 --- a/src/banman.cpp +++ b/src/banman.cpp @@ -27,7 +27,7 @@ BanMan::~BanMan() void BanMan::LoadBanlist() { - LOCK(m_cs_banned); + LOCK(m_banned_mutex); if (m_client_interface) m_client_interface->InitMessage(_("Loading banlist…").translated); @@ -51,16 +51,17 @@ void BanMan::DumpBanlist() banmap_t banmap; { - LOCK(m_cs_banned); + LOCK(m_banned_mutex); SweepBanned(); - if (!BannedSetIsDirty()) return; + if (!m_is_dirty) return; banmap = m_banned; - SetBannedSetDirty(false); + m_is_dirty = false; } const auto start{SteadyClock::now()}; if (!m_ban_db.Write(banmap)) { - SetBannedSetDirty(true); + LOCK(m_banned_mutex); + m_is_dirty = true; } LogPrint(BCLog::NET, "Flushed %d banned node addresses/subnets to disk %dms\n", banmap.size(), @@ -70,7 +71,7 @@ void BanMan::DumpBanlist() void BanMan::ClearBanned() { { - LOCK(m_cs_banned); + LOCK(m_banned_mutex); m_banned.clear(); m_is_dirty = true; } @@ -81,7 +82,7 @@ void BanMan::ClearBanned() void BanMan::ClearDiscouraged() { { - LOCK(m_cs_banned); + LOCK(m_banned_mutex); m_discouraged.reset(); m_is_dirty = true; } @@ -90,14 +91,14 @@ void BanMan::ClearDiscouraged() bool BanMan::IsDiscouraged(const CNetAddr& net_addr) { - LOCK(m_cs_banned); + LOCK(m_banned_mutex); return m_discouraged.contains(net_addr.GetAddrBytes()); } bool BanMan::IsBanned(const CNetAddr& net_addr) { auto current_time = GetTime(); - LOCK(m_cs_banned); + LOCK(m_banned_mutex); for (const auto& it : m_banned) { CSubNet sub_net = it.first; CBanEntry ban_entry = it.second; @@ -112,7 +113,7 @@ bool BanMan::IsBanned(const CNetAddr& net_addr) bool BanMan::IsBanned(const CSubNet& sub_net) { auto current_time = GetTime(); - LOCK(m_cs_banned); + LOCK(m_banned_mutex); banmap_t::iterator i = m_banned.find(sub_net); if (i != m_banned.end()) { CBanEntry ban_entry = (*i).second; @@ -131,7 +132,7 @@ void BanMan::Ban(const CNetAddr& net_addr, int64_t ban_time_offset, bool since_u void BanMan::Discourage(const CNetAddr& net_addr) { - LOCK(m_cs_banned); + LOCK(m_banned_mutex); m_discouraged.insert(net_addr.GetAddrBytes()); } @@ -148,7 +149,7 @@ void BanMan::Ban(const CSubNet& sub_net, int64_t ban_time_offset, bool since_uni ban_entry.nBanUntil = (normalized_since_unix_epoch ? 0 : GetTime()) + normalized_ban_time_offset; { - LOCK(m_cs_banned); + LOCK(m_banned_mutex); if (m_banned[sub_net].nBanUntil < ban_entry.nBanUntil) { m_banned[sub_net] = ban_entry; m_is_dirty = true; @@ -170,7 +171,7 @@ bool BanMan::Unban(const CNetAddr& net_addr) bool BanMan::Unban(const CSubNet& sub_net) { { - LOCK(m_cs_banned); + LOCK(m_banned_mutex); if (m_banned.erase(sub_net) == 0) return false; m_is_dirty = true; } @@ -181,7 +182,7 @@ bool BanMan::Unban(const CSubNet& sub_net) void BanMan::GetBanned(banmap_t& banmap) { - LOCK(m_cs_banned); + LOCK(m_banned_mutex); // Sweep the banlist so expired bans are not returned SweepBanned(); banmap = m_banned; //create a thread safe copy @@ -189,7 +190,7 @@ void BanMan::GetBanned(banmap_t& banmap) void BanMan::SweepBanned() { - AssertLockHeld(m_cs_banned); + AssertLockHeld(m_banned_mutex); int64_t now = GetTime(); bool notify_ui = false; @@ -212,15 +213,3 @@ void BanMan::SweepBanned() m_client_interface->BannedListChanged(); } } - -bool BanMan::BannedSetIsDirty() -{ - LOCK(m_cs_banned); - return m_is_dirty; -} - -void BanMan::SetBannedSetDirty(bool dirty) -{ - LOCK(m_cs_banned); //reuse m_banned lock for the m_is_dirty flag - m_is_dirty = dirty; -} diff --git a/src/banman.h b/src/banman.h index 652aa37efceb..c968fd99a2c0 100644 --- a/src/banman.h +++ b/src/banman.h @@ -60,41 +60,38 @@ class BanMan public: ~BanMan(); BanMan(fs::path ban_file, CClientUIInterface* client_interface, int64_t default_ban_time); - void Ban(const CNetAddr& net_addr, int64_t ban_time_offset = 0, bool since_unix_epoch = false); - void Ban(const CSubNet& sub_net, int64_t ban_time_offset = 0, bool since_unix_epoch = false); - void Discourage(const CNetAddr& net_addr); - void ClearBanned(); - void ClearDiscouraged(); + void Ban(const CNetAddr& net_addr, int64_t ban_time_offset = 0, bool since_unix_epoch = false) EXCLUSIVE_LOCKS_REQUIRED(!m_banned_mutex); + void Ban(const CSubNet& sub_net, int64_t ban_time_offset = 0, bool since_unix_epoch = false) EXCLUSIVE_LOCKS_REQUIRED(!m_banned_mutex); + void Discourage(const CNetAddr& net_addr) EXCLUSIVE_LOCKS_REQUIRED(!m_banned_mutex); + void ClearBanned() EXCLUSIVE_LOCKS_REQUIRED(!m_banned_mutex); + void ClearDiscouraged() EXCLUSIVE_LOCKS_REQUIRED(!m_banned_mutex); //! Return whether net_addr is banned - bool IsBanned(const CNetAddr& net_addr); + bool IsBanned(const CNetAddr& net_addr) EXCLUSIVE_LOCKS_REQUIRED(!m_banned_mutex); //! Return whether sub_net is exactly banned - bool IsBanned(const CSubNet& sub_net); + bool IsBanned(const CSubNet& sub_net) EXCLUSIVE_LOCKS_REQUIRED(!m_banned_mutex); //! Return whether net_addr is discouraged. - bool IsDiscouraged(const CNetAddr& net_addr); + bool IsDiscouraged(const CNetAddr& net_addr) EXCLUSIVE_LOCKS_REQUIRED(!m_banned_mutex); - bool Unban(const CNetAddr& net_addr); - bool Unban(const CSubNet& sub_net); - void GetBanned(banmap_t& banmap); - void DumpBanlist(); + bool Unban(const CNetAddr& net_addr) EXCLUSIVE_LOCKS_REQUIRED(!m_banned_mutex); + bool Unban(const CSubNet& sub_net) EXCLUSIVE_LOCKS_REQUIRED(!m_banned_mutex); + void GetBanned(banmap_t& banmap) EXCLUSIVE_LOCKS_REQUIRED(!m_banned_mutex); + void DumpBanlist() EXCLUSIVE_LOCKS_REQUIRED(!m_banned_mutex); private: - void LoadBanlist() EXCLUSIVE_LOCKS_REQUIRED(!m_cs_banned); - bool BannedSetIsDirty(); - //!set the "dirty" flag for the banlist - void SetBannedSetDirty(bool dirty = true); + void LoadBanlist() EXCLUSIVE_LOCKS_REQUIRED(!m_banned_mutex); //!clean unused entries (if bantime has expired) - void SweepBanned() EXCLUSIVE_LOCKS_REQUIRED(m_cs_banned); + void SweepBanned() EXCLUSIVE_LOCKS_REQUIRED(m_banned_mutex); - RecursiveMutex m_cs_banned; - banmap_t m_banned GUARDED_BY(m_cs_banned); - bool m_is_dirty GUARDED_BY(m_cs_banned){false}; + Mutex m_banned_mutex; + banmap_t m_banned GUARDED_BY(m_banned_mutex); + bool m_is_dirty GUARDED_BY(m_banned_mutex){false}; CClientUIInterface* m_client_interface = nullptr; CBanDB m_ban_db; const int64_t m_default_ban_time; - CRollingBloomFilter m_discouraged GUARDED_BY(m_cs_banned) {50000, 0.000001}; + CRollingBloomFilter m_discouraged GUARDED_BY(m_banned_mutex) {50000, 0.000001}; }; #endif // BITCOIN_BANMAN_H