From 406d585d1c742b7313d37d4a60615aa3b954d953 Mon Sep 17 00:00:00 2001 From: Konstantin Akimov Date: Thu, 27 Jul 2023 11:22:24 -0400 Subject: [PATCH 1/8] Merge bitcoin/bitcoin#28148: refactor: consistently use ApplyArgsManOptions for PeerManager::Options 8a3159728ae84cb8093e2e9fa5d2c2b0a7d545da refactor: deduplicate ignores_incoming_txs (stickies-v) 5f41afcc46913dbd4b5f08e622c5f74cd1eb50a5 refactor: set ignore_incoming_txs in ApplyArgsManOptions (stickies-v) Pull request description: Consistently use `ApplyArgsManOptions` for `PeerManager::Options`, and initialize `PeerManager::Options` early to avoid reading `"-blocksonly"` twice. Suggested in https://github.com/bitcoin/bitcoin/pull/27499#discussion_r1268400386 and also requested in https://github.com/bitcoin/bitcoin/pull/27499#discussion_r1273346189. No behaviour change, but the [`TestingSetup`](https://github.com/bitcoin/bitcoin/blob/e35fb7bc48d360585b80d0c7f89ac5087c1d405e/src/test/util/setup_common.cpp#L255-L256) is now also able to access `"-blocksonly"`. ACKs for top commit: MarcoFalke: lgtm ACK 8a3159728ae84cb8093e2e9fa5d2c2b0a7d545da achow101: ACK 8a3159728ae84cb8093e2e9fa5d2c2b0a7d545da TheCharlatan: ACK 8a3159728ae84cb8093e2e9fa5d2c2b0a7d545da dergoegge: utACK 8a3159728ae84cb8093e2e9fa5d2c2b0a7d545da Tree-SHA512: 6cb489d79ac2a87e8faedb76c96973ab3fc597426f274a90a3ffd0bc5fe3f2b25db9c7ec2e55a0c806c2bcbc0fdded6e228adb43d2cd81f14fd6552863847698 Co-authored-by: Andrew Chow --- src/init.cpp | 13 +++++-------- src/node/peerman_args.cpp | 2 ++ 2 files changed, 7 insertions(+), 8 deletions(-) diff --git a/src/init.cpp b/src/init.cpp index 4c4892f7054b..793e558f20d7 100644 --- a/src/init.cpp +++ b/src/init.cpp @@ -1648,7 +1648,9 @@ bool AppInitMain(NodeContext& node, interfaces::BlockAndHeaderTipInfo* tip_info) fListen = args.GetBoolArg("-listen", DEFAULT_LISTEN); fDiscover = args.GetBoolArg("-discover", true); - const bool ignores_incoming_txs{args.GetBoolArg("-blocksonly", DEFAULT_BLOCKSONLY)}; + + PeerManager::Options peerman_opts{}; + ApplyArgsManOptions(args, peerman_opts); { @@ -1713,7 +1715,7 @@ bool AppInitMain(NodeContext& node, interfaces::BlockAndHeaderTipInfo* tip_info) assert(!node.fee_estimator); // Don't initialize fee estimation with old data if we don't relay transactions, // as they would never get updated. - if (!ignores_incoming_txs) { + if (!peerman_opts.ignore_incoming_txs) { bool read_stale_estimates = args.GetBoolArg("-acceptstalefeeestimates", DEFAULT_ACCEPT_STALE_FEE_ESTIMATES); if (read_stale_estimates && (chainparams.GetChainType() != ChainType::REGTEST)) { return InitError(strprintf(_("acceptstalefeeestimates is not supported on %s chain."), chainparams.GetChainTypeString())); @@ -2233,15 +2235,10 @@ bool AppInitMain(NodeContext& node, interfaces::BlockAndHeaderTipInfo* tip_info) if (!node.active_ctx) { // Only constructed in wallet-enabled builds; stays null otherwise, must check before use node.cj_walletman = CJWalletManager::make(chainman, *node.dmnman, *node.mn_metaman, *node.mempool, *node.mn_sync, - *node.isman, !ignores_incoming_txs); + *node.isman, !peerman_opts.ignore_incoming_txs); } #endif - PeerManager::Options peerman_opts{ - .ignore_incoming_txs = ignores_incoming_txs, - }; - ApplyArgsManOptions(args, peerman_opts); - assert(!node.peerman); node.peerman = PeerManager::make(*node.connman, *node.addrman, node.banman.get(), *node.dstxman, chainman, *node.mempool, *node.mn_metaman, *node.mn_sync, diff --git a/src/node/peerman_args.cpp b/src/node/peerman_args.cpp index c3cdbcc44b06..55570cdfc7ec 100644 --- a/src/node/peerman_args.cpp +++ b/src/node/peerman_args.cpp @@ -23,6 +23,8 @@ void ApplyArgsManOptions(const ArgsManager& argsman, PeerManager::Options& optio if (auto value{argsman.GetBoolArg("-capturemessages")}) options.capture_messages = *value; if (auto value{argsman.GetIntArg("-maxtimeadjustment")}) options.max_time_adjustment = *value; + + if (auto value{argsman.GetBoolArg("-blocksonly")}) options.ignore_incoming_txs = *value; } } // namespace node From b5ba6b2ece04f110d834fd45a85369b171a206bf Mon Sep 17 00:00:00 2001 From: Konstantin Akimov Date: Mon, 7 Aug 2023 17:06:42 +0200 Subject: [PATCH 2/8] Merge bitcoin/bitcoin#26296: ci: Integrate `bitcoin-tidy` clang-tidy plugin MIME-Version: 1.0 Content-Type: text/plain; charset=UTF-8 Content-Transfer-Encoding: 8bit BACKPORT NOTE: lint-logs.py is gone and the bitcoin-unterminated-logprintf clang-tidy check that replaces it knows nothing about /* Continued */. Unlike the linter it also sees headers and judges the format string itself, so it flags 22 Dash log calls whose newline comes from an argument or from a later call. Mark those NOLINT, as bitcoin#28237 does for CommitTransaction, and remove the now meaningless markers from the remaining calls. ci/test/06_script_b.sh hunk is adopted for Dash runs of clang-tidy from ci/dash/lint-tidy.sh. Without the plugin loaded the bitcoin-* checks in src/.clang-tidy match nothing, and with lint-logs.py removed unterminated log strings are not checked at all. --------------------- 1c976c691cc4b20f43071aabf36c7afed1571057 tidy: Integrate bicoin-tidy clang-tidy plugin (fanquake) 7de23cceb8ac13fcc709453ef0fa14fb93c460b0 refactor: fix unterminated LogPrintf()s (fanquake) 0a1029aa2946787bfe91a84de882c2dd0de076e9 lint: remove /* Continued */ markers from codebase (fanquake) 910007995d8603ffc466878856227153a638caff lint: remove lint-logs.py (fanquake) d86a83d6b8587b0971e66c6910af23dd8c042969 lint: drop DIR_IWYU global (fanquake) Pull request description: Demo of integrating the [bitcoin-tidy](https://github.com/theuni/bitcoin-tidy-plugin), [clang-tidy plugin](https://clang.llvm.org/extra/clang-tidy/) written by theuni into our tidy CI job. The plugin currently has a single check, `bitcoin-unterminated-logprintf`. This would replace our current Python driven, `git-grep`-based, `.cpp` file only, lint-logs linter. ACKs for top commit: TheCharlatan: ACK 1c976c691cc4b20f43071aabf36c7afed1571057 theuni: ACK 1c976c691cc4b20f43071aabf36c7afed1571057 MarcoFalke: re-ACK 1c976c691cc4b20f43071aabf36c7afed1571057 👠 Tree-SHA512: 725b45c70e431d48e6f276671e05c694e10b6047cae1a31906ac3ee9093bc8105fb226b36a5bac6709557526ca6007222112d66aecec05a574434edc4897e4b8 Co-authored-by: fanquake --- .github/workflows/build.yml | 6 +- ci/dash/lint-tidy.sh | 8 ++ contrib/devtools/bitcoin-tidy/CMakeLists.txt | 45 +++++++++ contrib/devtools/bitcoin-tidy/README | 8 ++ .../devtools/bitcoin-tidy/bitcoin-tidy.cpp | 22 +++++ .../bitcoin-tidy/example_logprintf.cpp | 91 +++++++++++++++++++ contrib/devtools/bitcoin-tidy/logprintf.cpp | 62 +++++++++++++ contrib/devtools/bitcoin-tidy/logprintf.h | 28 ++++++ src/.clang-tidy | 1 + src/chainlock/signing.cpp | 2 +- src/coinjoin/client.cpp | 22 ++--- src/coinjoin/coinjoin.cpp | 6 +- src/coinjoin/server.cpp | 32 +++---- src/coinjoin/walletman.cpp | 2 +- src/dbwrapper.cpp | 2 +- src/evo/creditpool.cpp | 2 +- src/evo/specialtxman.cpp | 4 +- src/flat-database.h | 2 +- src/governance/governance.cpp | 2 +- src/governance/net_governance.cpp | 2 +- src/governance/object.cpp | 2 +- src/governance/signing.cpp | 4 +- src/governance/superblock.cpp | 4 +- src/gsl/assert.cpp | 2 +- src/index/base.cpp | 6 +- src/init.cpp | 4 +- src/instantsend/net_instantsend.cpp | 4 +- src/llmq/blockprocessor.cpp | 10 +- src/llmq/net_dkg.cpp | 2 +- src/llmq/net_quorum.cpp | 2 +- src/llmq/signing_shares.cpp | 16 ++-- src/net_processing.cpp | 2 +- src/node/blockstorage.cpp | 2 +- src/node/miner.cpp | 2 +- src/node/sync_manager.cpp | 2 +- src/node/utxo_snapshot.cpp | 4 +- src/rpc/governance.cpp | 2 +- src/rpc/util.cpp | 2 +- src/stacktraces.cpp | 2 +- src/sync.cpp | 2 +- src/test/logging_tests.cpp | 12 +-- src/test/util/chainstate.h | 2 +- src/validation.cpp | 24 ++--- src/wallet/coinjoin.cpp | 2 +- src/wallet/init.cpp | 2 +- src/wallet/wallet.cpp | 2 +- test/lint/lint-format-strings.py | 2 +- test/lint/lint-include-guards.py | 3 +- test/lint/lint-includes.py | 5 +- test/lint/lint-logs.py | 34 ------- 50 files changed, 374 insertions(+), 139 deletions(-) create mode 100644 contrib/devtools/bitcoin-tidy/CMakeLists.txt create mode 100644 contrib/devtools/bitcoin-tidy/README create mode 100644 contrib/devtools/bitcoin-tidy/bitcoin-tidy.cpp create mode 100644 contrib/devtools/bitcoin-tidy/example_logprintf.cpp create mode 100644 contrib/devtools/bitcoin-tidy/logprintf.cpp create mode 100644 contrib/devtools/bitcoin-tidy/logprintf.h delete mode 100755 test/lint/lint-logs.py diff --git a/.github/workflows/build.yml b/.github/workflows/build.yml index e30687d774d5..25f3f506afef 100644 --- a/.github/workflows/build.yml +++ b/.github/workflows/build.yml @@ -512,9 +512,9 @@ jobs: uses: actions/cache/restore@v5 with: path: /cache/ctcache - key: ctcache-${{ hashFiles('contrib/containers/ci/ci.Dockerfile', 'depends/packages/*') }}-linux64_multiprocess-${{ github.sha }} + key: ctcache-${{ hashFiles('contrib/containers/ci/ci.Dockerfile', 'contrib/devtools/bitcoin-tidy/*', 'depends/packages/*') }}-linux64_multiprocess-${{ github.sha }} restore-keys: | - ctcache-${{ hashFiles('contrib/containers/ci/ci.Dockerfile', 'depends/packages/*') }}-linux64_multiprocess- + ctcache-${{ hashFiles('contrib/containers/ci/ci.Dockerfile', 'contrib/devtools/bitcoin-tidy/*', 'depends/packages/*') }}-linux64_multiprocess- - name: Run linters run: | @@ -530,7 +530,7 @@ jobs: uses: actions/cache/save@v5 with: path: /cache/ctcache - key: ctcache-${{ hashFiles('contrib/containers/ci/ci.Dockerfile', 'depends/packages/*') }}-linux64_multiprocess-${{ github.sha }} + key: ctcache-${{ hashFiles('contrib/containers/ci/ci.Dockerfile', 'contrib/devtools/bitcoin-tidy/*', 'depends/packages/*') }}-linux64_multiprocess-${{ github.sha }} test-linux64: name: linux64-test diff --git a/ci/dash/lint-tidy.sh b/ci/dash/lint-tidy.sh index be2639e1d5ab..36df742b3ac4 100755 --- a/ci/dash/lint-tidy.sh +++ b/ci/dash/lint-tidy.sh @@ -43,6 +43,8 @@ fi # Setup ctcache for clang-tidy result caching export CTCACHE_SAVE_OUTPUT=1 +# The cache key is the preprocessed source: keep comments so that a NOLINT edit invalidates it +export CTCACHE_KEEP_COMMENTS=1 export CTCACHE_CLANG_TIDY="${CLANG_TIDY_BIN}" mkdir -p "${CTCACHE_DIR}" @@ -64,6 +66,11 @@ fi # Zero stats before run to get accurate statistics for this run only python3 "${CLANG_TIDY_CACHE_PY}" --zero-stats 2>&1 || true +TIDY_BUILD_DIR="${BASE_ROOT_DIR}/build-ci/tidy-build" +cmake -B "${TIDY_BUILD_DIR}" -DLLVM_DIR="$(llvm-config --cmakedir)" -DCMAKE_BUILD_TYPE=Release -S "${BASE_ROOT_DIR}/contrib/devtools/bitcoin-tidy" +cmake --build "${TIDY_BUILD_DIR}" "${MAKEJOBS}" +cmake --build "${TIDY_BUILD_DIR}" --target bitcoin-tidy-tests "${MAKEJOBS}" + cd "${BASE_ROOT_DIR}/build-ci/dashcore-${BUILD_TARGET}/src" # Filter out files by regex here, because regex may not be @@ -84,6 +91,7 @@ if ! ( run-clang-tidy \ -checks=clang-diagnostic-old-style-cast,google-readability-casting \ -warnings-as-errors=-clang-diagnostic-old-style-cast,-google-readability-casting \ -clang-tidy-binary="${CLANG_TIDY_CACHE}" \ + -load="${TIDY_BUILD_DIR}/libbitcoin-tidy.so" \ -p "${CAST_LINT_DB}" \ -quiet "${MAKEJOBS}" | \ python3 "${BASE_ROOT_DIR}/ci/dash/lint-cstyle-casts.py" filter --source-root "${BASE_ROOT_DIR}" | \ diff --git a/contrib/devtools/bitcoin-tidy/CMakeLists.txt b/contrib/devtools/bitcoin-tidy/CMakeLists.txt new file mode 100644 index 000000000000..9ed18696d41b --- /dev/null +++ b/contrib/devtools/bitcoin-tidy/CMakeLists.txt @@ -0,0 +1,45 @@ +cmake_minimum_required(VERSION 3.9) + +project(bitcoin-tidy VERSION 1.0.0 DESCRIPTION "clang-tidy checks for Bitcoin Core") + +include(GNUInstallDirs) + +set(CMAKE_CXX_STANDARD 17) +set(CMAKE_CXX_STANDARD_REQUIRED True) +set(CMAKE_CXX_EXTENSIONS False) + +# TODO: Figure out how to avoid the terminfo check +find_package(LLVM REQUIRED CONFIG) +find_program(CLANG_TIDY_EXE NAMES "clang-tidy-${LLVM_VERSION_MAJOR}" "clang-tidy" HINTS ${LLVM_TOOLS_BINARY_DIR}) +message(STATUS "Found LLVM ${LLVM_PACKAGE_VERSION}") +message(STATUS "Found clang-tidy: ${CLANG_TIDY_EXE}") + +add_library(bitcoin-tidy MODULE bitcoin-tidy.cpp logprintf.cpp) +target_include_directories(bitcoin-tidy SYSTEM PRIVATE ${LLVM_INCLUDE_DIRS}) + +# Disable RTTI and exceptions as necessary +if (MSVC) + target_compile_options(bitcoin-tidy PRIVATE /GR-) +else() + target_compile_options(bitcoin-tidy PRIVATE -fno-rtti) + target_compile_options(bitcoin-tidy PRIVATE -fno-exceptions) +endif() + +# Add warnings +if (MSVC) + target_compile_options(bitcoin-tidy PRIVATE /W4) +else() + target_compile_options(bitcoin-tidy PRIVATE -Wall) + target_compile_options(bitcoin-tidy PRIVATE -Wextra) +endif() + +set(CLANG_TIDY_COMMAND "${CLANG_TIDY_EXE}" "--load=${CMAKE_BINARY_DIR}/${CMAKE_SHARED_LIBRARY_PREFIX}bitcoin-tidy${CMAKE_SHARED_LIBRARY_SUFFIX}" "-checks=-*,bitcoin-*") + +# Create a dummy library that runs clang-tidy tests as a side-effect of building +add_library(bitcoin-tidy-tests OBJECT EXCLUDE_FROM_ALL example_logprintf.cpp) +add_dependencies(bitcoin-tidy-tests bitcoin-tidy) + +set_target_properties(bitcoin-tidy-tests PROPERTIES CXX_CLANG_TIDY "${CLANG_TIDY_COMMAND}") + + +install(TARGETS bitcoin-tidy LIBRARY DESTINATION ${CMAKE_INSTALL_LIBDIR}) diff --git a/contrib/devtools/bitcoin-tidy/README b/contrib/devtools/bitcoin-tidy/README new file mode 100644 index 000000000000..1669fe98f57f --- /dev/null +++ b/contrib/devtools/bitcoin-tidy/README @@ -0,0 +1,8 @@ +# Bitcoin Tidy + +Example Usage: + +```bash +cmake -S . -B build -DLLVM_DIR=/path/to/lib/cmake/llvm -DCMAKE_BUILD_TYPE=Release +make -C build -j$(nproc) +``` diff --git a/contrib/devtools/bitcoin-tidy/bitcoin-tidy.cpp b/contrib/devtools/bitcoin-tidy/bitcoin-tidy.cpp new file mode 100644 index 000000000000..0f34d3779387 --- /dev/null +++ b/contrib/devtools/bitcoin-tidy/bitcoin-tidy.cpp @@ -0,0 +1,22 @@ +// Copyright (c) 2023 Bitcoin Developers +// Distributed under the MIT software license, see the accompanying +// file COPYING or http://www.opensource.org/licenses/mit-license.php. + +#include "logprintf.h" + +#include +#include + +class BitcoinModule final : public clang::tidy::ClangTidyModule +{ +public: + void addCheckFactories(clang::tidy::ClangTidyCheckFactories& CheckFactories) override + { + CheckFactories.registerCheck("bitcoin-unterminated-logprintf"); + } +}; + +static clang::tidy::ClangTidyModuleRegistry::Add + X("bitcoin-module", "Adds bitcoin checks."); + +volatile int BitcoinModuleAnchorSource = 0; diff --git a/contrib/devtools/bitcoin-tidy/example_logprintf.cpp b/contrib/devtools/bitcoin-tidy/example_logprintf.cpp new file mode 100644 index 000000000000..a3d2768964cc --- /dev/null +++ b/contrib/devtools/bitcoin-tidy/example_logprintf.cpp @@ -0,0 +1,91 @@ +// Copyright (c) 2023 Bitcoin Developers +// Distributed under the MIT software license, see the accompanying +// file COPYING or http://www.opensource.org/licenses/mit-license.php. + +// Warn about any use of LogPrintf that does not end with a newline. +#include + +enum LogFlags { + NONE +}; + +enum Level { + None +}; + +template +static inline void LogPrintf_(const std::string& logging_function, const std::string& source_file, const int source_line, const LogFlags flag, const Level level, const char* fmt, const Args&... args) +{ +} + +#define LogPrintLevel_(category, level, ...) LogPrintf_(__func__, __FILE__, __LINE__, category, level, __VA_ARGS__) +#define LogPrintf(...) LogPrintLevel_(LogFlags::NONE, Level::None, __VA_ARGS__) + +// Use a macro instead of a function for conditional logging to prevent +// evaluating arguments when logging for the category is not enabled. +#define LogPrint(category, ...) \ + do { \ + LogPrintf(__VA_ARGS__); \ + } while (0) + + +class CWallet +{ + std::string GetDisplayName() const + { + return "default wallet"; + } + +public: + template + void WalletLogPrintf(std::string fmt, Params... parameters) const + { + LogPrintf(("%s " + fmt).c_str(), GetDisplayName(), parameters...); + }; +}; + +void good_func() +{ + LogPrintf("hello world!\n"); +} +void good_func2() +{ + CWallet wallet; + wallet.WalletLogPrintf("hi\n"); + + const CWallet& walletref = wallet; + walletref.WalletLogPrintf("hi\n"); + + auto* walletptr = new CWallet(); + walletptr->WalletLogPrintf("hi\n"); + delete walletptr; +} +void bad_func() +{ + LogPrintf("hello world!"); +} +void bad_func2() +{ + LogPrintf(""); +} +void bad_func3() +{ + // Ending in "..." has no special meaning. + LogPrintf("hello world!..."); +} +void bad_func4_ignored() +{ + LogPrintf("hello world!"); // NOLINT(bitcoin-unterminated-logprintf) +} +void bad_func5() +{ + CWallet wallet; + wallet.WalletLogPrintf("hi"); + + const CWallet& walletref = wallet; + walletref.WalletLogPrintf("hi"); + + auto* walletptr = new CWallet(); + walletptr->WalletLogPrintf("hi"); + delete walletptr; +} diff --git a/contrib/devtools/bitcoin-tidy/logprintf.cpp b/contrib/devtools/bitcoin-tidy/logprintf.cpp new file mode 100644 index 000000000000..1690c8fde0a6 --- /dev/null +++ b/contrib/devtools/bitcoin-tidy/logprintf.cpp @@ -0,0 +1,62 @@ +// Copyright (c) 2023 Bitcoin Developers +// Distributed under the MIT software license, see the accompanying +// file COPYING or http://www.opensource.org/licenses/mit-license.php. + +#include "logprintf.h" + +#include +#include + + +namespace { +AST_MATCHER(clang::StringLiteral, unterminated) +{ + size_t len = Node.getLength(); + if (len > 0 && Node.getCodeUnit(len - 1) == '\n') { + return false; + } + return true; +} +} // namespace + +namespace bitcoin { + +void LogPrintfCheck::registerMatchers(clang::ast_matchers::MatchFinder* finder) +{ + using namespace clang::ast_matchers; + + /* + Logprintf(..., ..., ..., ..., ..., "foo", ...) + */ + + finder->addMatcher( + callExpr( + callee(functionDecl(hasName("LogPrintf_"))), + hasArgument(5, stringLiteral(unterminated()).bind("logstring"))), + this); + + /* + CWallet wallet; + auto walletptr = &wallet; + wallet.WalletLogPrintf("foo"); + wallet->WalletLogPrintf("foo"); + */ + finder->addMatcher( + cxxMemberCallExpr( + thisPointerType(qualType(hasDeclaration(cxxRecordDecl(hasName("CWallet"))))), + callee(cxxMethodDecl(hasName("WalletLogPrintf"))), + hasArgument(0, stringLiteral(unterminated()).bind("logstring"))), + this); +} + +void LogPrintfCheck::check(const clang::ast_matchers::MatchFinder::MatchResult& Result) +{ + if (const clang::StringLiteral* lit = Result.Nodes.getNodeAs("logstring")) { + const clang::ASTContext& ctx = *Result.Context; + const auto user_diag = diag(lit->getEndLoc(), "Unterminated format string used with LogPrintf"); + const auto& loc = lit->getLocationOfByte(lit->getByteLength(), *Result.SourceManager, ctx.getLangOpts(), ctx.getTargetInfo()); + user_diag << clang::FixItHint::CreateInsertion(loc, "\\n"); + } +} + +} // namespace bitcoin diff --git a/contrib/devtools/bitcoin-tidy/logprintf.h b/contrib/devtools/bitcoin-tidy/logprintf.h new file mode 100644 index 000000000000..466849ef0114 --- /dev/null +++ b/contrib/devtools/bitcoin-tidy/logprintf.h @@ -0,0 +1,28 @@ +// Copyright (c) 2023 Bitcoin Developers +// Distributed under the MIT software license, see the accompanying +// file COPYING or http://www.opensource.org/licenses/mit-license.php. + +#ifndef LOGPRINTF_CHECK_H +#define LOGPRINTF_CHECK_H + +#include + +namespace bitcoin { + +class LogPrintfCheck final : public clang::tidy::ClangTidyCheck +{ +public: + LogPrintfCheck(clang::StringRef Name, clang::tidy::ClangTidyContext* Context) + : clang::tidy::ClangTidyCheck(Name, Context) {} + + bool isLanguageVersionSupported(const clang::LangOptions& LangOpts) const override + { + return LangOpts.CPlusPlus; + } + void registerMatchers(clang::ast_matchers::MatchFinder* Finder) override; + void check(const clang::ast_matchers::MatchFinder::MatchResult& Result) override; +}; + +} // namespace bitcoin + +#endif // LOGPRINTF_CHECK_H diff --git a/src/.clang-tidy b/src/.clang-tidy index d127a4a60f52..13466b17c43f 100644 --- a/src/.clang-tidy +++ b/src/.clang-tidy @@ -1,5 +1,6 @@ Checks: ' -*, +bitcoin-unterminated-logprintf, bugprone-argument-comment, bugprone-use-after-move, bugprone-lambda-function-name, diff --git a/src/chainlock/signing.cpp b/src/chainlock/signing.cpp index 121b164c760e..ba1802ad452b 100644 --- a/src/chainlock/signing.cpp +++ b/src/chainlock/signing.cpp @@ -149,7 +149,7 @@ void ChainLockSigner::TrySignChainTip() for (const auto& txid : *txids) { if (!m_clhandler.IsTxSafeForMining(txid) && !m_isman.IsLocked(txid)) { - LogPrint(BCLog::CHAINLOCKS, /* Continued */ + LogPrint(BCLog::CHAINLOCKS, "%s -- not signing block %s due to TX %s not being islocked and not old enough.\n", __func__, pindexWalk->GetBlockHash().ToString(), txid.ToString()); return; diff --git a/src/coinjoin/client.cpp b/src/coinjoin/client.cpp index 3ba3b6bdbeba..58e0d0dbfec6 100644 --- a/src/coinjoin/client.cpp +++ b/src/coinjoin/client.cpp @@ -118,7 +118,7 @@ void CCoinJoinClientSession::ProcessMessage(CNode& peer, Chainstate& active_chai return; } - WalletCJLogPrint(m_wallet, "DSFINALTX -- txNew %s", txNew.ToString()); /* Continued */ + WalletCJLogPrint(m_wallet, "DSFINALTX -- txNew %s", txNew.ToString()); // NOLINT(bitcoin-unterminated-logprintf) // check to see if input is spent already? (and probably not confirmed) SignFinalTransaction(peer, active_chainstate, connman, mempool, txNew); @@ -479,7 +479,7 @@ bool CCoinJoinClientSession::SignFinalTransaction(CNode& peer, Chainstate& activ LOCK(cs_coinjoin); finalMutableTransaction = CMutableTransaction{finalTransactionNew}; - WalletCJLogPrint(m_wallet, "CCoinJoinClientSession::%s -- finalMutableTransaction=%s", __func__, finalMutableTransaction.ToString()); /* Continued */ + WalletCJLogPrint(m_wallet, "CCoinJoinClientSession::%s -- finalMutableTransaction=%s", __func__, finalMutableTransaction.ToString()); // NOLINT(bitcoin-unterminated-logprintf) // STEP 1: check final transaction general rules @@ -622,7 +622,7 @@ bool CCoinJoinClientSession::SignFinalTransaction(CNode& peer, Chainstate& activ } // push all of our signatures to the Masternode - WalletCJLogPrint(m_wallet, "CCoinJoinClientSession::%s -- pushing signed inputs to the masternode, finalMutableTransaction=%s", __func__, finalMutableTransaction.ToString()); /* Continued */ + WalletCJLogPrint(m_wallet, "CCoinJoinClientSession::%s -- pushing signed inputs to the masternode, finalMutableTransaction=%s", __func__, finalMutableTransaction.ToString()); // NOLINT(bitcoin-unterminated-logprintf) CNetMsgMaker msgMaker(peer.GetCommonVersion()); connman.PushMessage(&peer, msgMaker.Make(NetMsgType::DSSIGNFINALTX, signed_inputs)); SetState(POOL_STATE_SIGNING); @@ -735,7 +735,7 @@ void CCoinJoinClientManager::AddPendingObservation(const std::vector& // would make them selectable again while a valid mixing transaction spending them // may already be in flight. Nothing more we can do about it here beyond making // the failure loud - the wallet database is broken. - LogPrintf("CCoinJoinClientManager::%s -- ERROR: failed to persist locks for %d successfully mixed input(s), " /* Continued */ + LogPrintf("CCoinJoinClientManager::%s -- ERROR: failed to persist locks for %d successfully mixed input(s), " "they will not survive a restart until this succeeds\n", __func__, outpoints.size()); } @@ -769,7 +769,7 @@ void CCoinJoinClientManager::LoadPendingObservations(wallet::WalletBatch& batch) // never becomes readable, and the retry runs for as long as this node does. if (!m_pending_obs_load_failed) { m_pending_obs_load_failed = true; - LogPrintf("CCoinJoinClientManager::%s -- ERROR: failed to read the pending observation record, will keep " /* Continued */ + LogPrintf("CCoinJoinClientManager::%s -- ERROR: failed to read the pending observation record, will keep " "retrying silently. The inputs it tracks stay locked until it can be read, `lockunspent` " "releases them manually\n", __func__); @@ -847,7 +847,7 @@ void CCoinJoinClientManager::CheckPendingObservations(const CTxMemPool& mempool) // Still unspent in chain and mempool long after the session completed - // the finalized transaction most likely never propagated, release the // input so the wallet does not lose it forever - LogPrintf("CCoinJoinClientManager::%s -- WARNING: never observed finalized mixing transaction for %s, " /* Continued */ + LogPrintf("CCoinJoinClientManager::%s -- WARNING: never observed finalized mixing transaction for %s, " "releasing lock after %d seconds\n", __func__, outpoint.ToStringShort(), COINJOIN_PENDING_OBSERVATION_TIMEOUT); m_wallet->UnlockCoin(outpoint, &get_batch()); @@ -863,7 +863,7 @@ void CCoinJoinClientManager::CheckPendingObservations(const CTxMemPool& mempool) // selected for another session - so only refresh the timer to keep the check // above from running on every pass. The refresh is deliberately not persisted, // the worst a restart can do is re-run the check once. - WalletCJLogPrint(m_wallet, "CCoinJoinClientManager::%s -- %s is spent in chain/mempool but not according to " /* Continued */ + WalletCJLogPrint(m_wallet, "CCoinJoinClientManager::%s -- %s is spent in chain/mempool but not according to " "the wallet, keeping it locked\n", __func__, outpoint.ToStringShort()); it->second = nNow; @@ -878,7 +878,7 @@ void CCoinJoinClientManager::CheckPendingObservations(const CTxMemPool& mempool) const bool fPersisted{PersistPendingObservations(*m_wallet, get_batch(), m_pending_obs)}; // Retried on every pass until it succeeds, only complain when it starts failing if (!fPersisted && !m_pending_obs_dirty) { - LogPrintf("CCoinJoinClientManager::%s -- ERROR: failed to persist %d pending observation(s), " /* Continued */ + LogPrintf("CCoinJoinClientManager::%s -- ERROR: failed to persist %d pending observation(s), " "will keep retrying\n", __func__, m_pending_obs.size()); } @@ -1437,7 +1437,7 @@ bool CCoinJoinClientSession::JoinExistingQueue(CAmount nBalanceNeedsAnonymized, m_mn_metaman.AddUsedMasternode(dmn->proTxHash); if (connman.IsMasternodeOrDisconnectRequested(dmn->pdmnState->netInfo->GetPrimary())) { - WalletCJLogPrint(m_wallet, /* Continued */ + WalletCJLogPrint(m_wallet, "CCoinJoinClientSession::JoinExistingQueue -- skipping connection, masternode=%s\n", dmn->proTxHash.ToString()); continue; } @@ -1512,7 +1512,7 @@ bool CCoinJoinClientSession::StartNewQueue(CAmount nBalanceNeedsAnonymized, CCon } if (m_mn_metaman.IsMixingThresholdExceeded(dmn->proTxHash, nMnCount)) { - WalletCJLogPrint(m_wallet, /* Continued */ + WalletCJLogPrint(m_wallet, "CCoinJoinClientSession::StartNewQueue -- too early to mix with node masternode=%s\n", dmn->proTxHash.ToString()); nTries++; @@ -1543,7 +1543,7 @@ bool CCoinJoinClientSession::StartNewQueue(CAmount nBalanceNeedsAnonymized, CCon pendingDsaRequest = CPendingDsaRequest(dmn->proTxHash, CCoinJoinAccept(nSessionDenom, txMyCollateral)); SetState(POOL_STATE_QUEUE); nTimeLastSuccessfulStep = GetTime(); - WalletCJLogPrint(/* Continued */ + WalletCJLogPrint( m_wallet, "CCoinJoinClientSession::StartNewQueue -- pending connection, masternode=%s, nSessionDenom=%d " "(%s)\n", diff --git a/src/coinjoin/coinjoin.cpp b/src/coinjoin/coinjoin.cpp index 5fe5de73a1ab..04b7f70da612 100644 --- a/src/coinjoin/coinjoin.cpp +++ b/src/coinjoin/coinjoin.cpp @@ -469,7 +469,7 @@ bool CoinJoin::IsCollateralValid(ChainstateManager& chainman, const llmq::CInsta nValueOut += txout.nValue; if (!txout.scriptPubKey.IsPayToPublicKeyHash() && !txout.scriptPubKey.IsUnspendable()) { - LogPrint(BCLog::COINJOIN, "CoinJoin::IsCollateralValid -- Invalid Script, txCollateral=%s", txCollateral.ToString()); /* Continued */ + LogPrint(BCLog::COINJOIN, "CoinJoin::IsCollateralValid -- Invalid Script, txCollateral=%s", txCollateral.ToString()); // NOLINT(bitcoin-unterminated-logprintf) return false; } } @@ -489,11 +489,11 @@ bool CoinJoin::IsCollateralValid(ChainstateManager& chainman, const llmq::CInsta //collateral transactions are required to pay out a small fee to the miners if (nValueIn - nValueOut < GetCollateralAmount()) { - LogPrint(BCLog::COINJOIN, "CoinJoin::IsCollateralValid -- did not include enough fees in transaction: fees: %d, txCollateral=%s", nValueOut - nValueIn, txCollateral.ToString()); /* Continued */ + LogPrint(BCLog::COINJOIN, "CoinJoin::IsCollateralValid -- did not include enough fees in transaction: fees: %d, txCollateral=%s", nValueOut - nValueIn, txCollateral.ToString()); // NOLINT(bitcoin-unterminated-logprintf) return false; } - LogPrint(BCLog::COINJOIN, "CoinJoin::IsCollateralValid -- %s", txCollateral.ToString()); /* Continued */ + LogPrint(BCLog::COINJOIN, "CoinJoin::IsCollateralValid -- %s", txCollateral.ToString()); // NOLINT(bitcoin-unterminated-logprintf) if (!ATMPIfSaneFee(chainman, MakeTransactionRef(txCollateral), /*test_accept=*/true)) { LogPrint(BCLog::COINJOIN, "CoinJoin::IsCollateralValid -- didn't pass ATMPIfSaneFee()\n"); diff --git a/src/coinjoin/server.cpp b/src/coinjoin/server.cpp index b9cbb197e414..8ce61f1dea16 100644 --- a/src/coinjoin/server.cpp +++ b/src/coinjoin/server.cpp @@ -77,7 +77,7 @@ void CCoinJoinServer::ProcessDSACCEPT(CNode& peer, CDataStream& vRecv) CCoinJoinAccept dsa; vRecv >> dsa; - LogPrint(BCLog::COINJOIN, "DSACCEPT -- nDenom %d (%s) txCollateral %s", dsa.nDenom, CoinJoin::DenominationToString(dsa.nDenom), dsa.txCollateral.ToString()); /* Continued */ + LogPrint(BCLog::COINJOIN, "DSACCEPT -- nDenom %d (%s) txCollateral %s", dsa.nDenom, CoinJoin::DenominationToString(dsa.nDenom), dsa.txCollateral.ToString()); // NOLINT(bitcoin-unterminated-logprintf) auto mnList = m_dmnman.GetListAtChainTip(); auto dmn = mnList.GetValidMNByCollateral(m_mn_activeman.GetOutPoint()); @@ -209,7 +209,7 @@ void CCoinJoinServer::ProcessDSVIN(CNode& peer, CDataStream& vRecv) CCoinJoinEntry entry; vRecv >> entry; - LogPrint(BCLog::COINJOIN, "DSVIN -- txCollateral %s", entry.txCollateral->ToString()); /* Continued */ + LogPrint(BCLog::COINJOIN, "DSVIN -- txCollateral %s", entry.txCollateral->ToString()); // NOLINT(bitcoin-unterminated-logprintf) // Note: unbalanced (promotion/demotion) entries are only valid post-V24; AddEntry -> // IsValidInOuts rejects them pre-V24 and consumes the collateral to keep spam costly @@ -437,7 +437,7 @@ void CCoinJoinServer::CreateFinalTransaction(int session_id, bool charge_fees) sort(txNew.vout.begin(), txNew.vout.end(), CompareOutputBIP69()); finalMutableTransaction = txNew; - LogPrint(BCLog::COINJOIN, "CCoinJoinServer::CreateFinalTransaction -- finalMutableTransaction=%s", /* Continued */ + LogPrint(BCLog::COINJOIN, "CCoinJoinServer::CreateFinalTransaction -- finalMutableTransaction=%s", // NOLINT(bitcoin-unterminated-logprintf) txNew.ToString()); // request signatures from clients @@ -472,7 +472,7 @@ void CCoinJoinServer::CommitFinalTransaction(int session_id) } uint256 hashTx = finalTransaction->GetHash(); - LogPrint(BCLog::COINJOIN, "CCoinJoinServer::CommitFinalTransaction -- finalTransaction=%s", /* Continued */ + LogPrint(BCLog::COINJOIN, "CCoinJoinServer::CommitFinalTransaction -- finalTransaction=%s", // NOLINT(bitcoin-unterminated-logprintf) finalTransaction->ToString()); { @@ -480,7 +480,7 @@ void CCoinJoinServer::CommitFinalTransaction(int session_id) TRY_LOCK(::cs_main, lockMain); mempool.PrioritiseTransaction(hashTx, 0.1 * COIN); if (!lockMain || !ATMPIfSaneFee(m_chainman, finalTransaction)) { - LogPrint(BCLog::COINJOIN, /* Continued */ + LogPrint(BCLog::COINJOIN, "CCoinJoinServer::CommitFinalTransaction -- ATMPIfSaneFee() error: Transaction not valid\n"); // not much we can do in this case, just notify clients RelayCompletedTransaction(session_id, participants, ERR_INVALID_TX); @@ -552,7 +552,7 @@ CTransactionRef CCoinJoinServer::SelectCollateralToCharge() // This queue entry didn't send us the promised transaction if (!fFound) { - LogPrint(BCLog::COINJOIN, /* Continued */ + LogPrint(BCLog::COINJOIN, "CCoinJoinServer::SelectCollateralToCharge -- found uncooperative node (didn't send " "transaction), found offence\n"); vecOffendersCollaterals.push_back(txCollateral); @@ -563,7 +563,7 @@ CTransactionRef CCoinJoinServer::SelectCollateralToCharge() for (const auto& entry : vecEntries) { for (const auto& txdsin : entry.vecTxDSIn) { if (!txdsin.fHasSig) { - LogPrint(BCLog::COINJOIN, /* Continued */ + LogPrint(BCLog::COINJOIN, "CCoinJoinServer::SelectCollateralToCharge -- found uncooperative node (didn't sign), " "found offence\n"); vecOffendersCollaterals.push_back(entry.txCollateral); @@ -584,9 +584,9 @@ CTransactionRef CCoinJoinServer::SelectCollateralToCharge() //charge one of the offenders randomly Shuffle(vecOffendersCollaterals.begin(), vecOffendersCollaterals.end(), FastRandomContext()); - LogPrint(BCLog::COINJOIN, /* Continued */ + LogPrint(BCLog::COINJOIN, "CCoinJoinServer::SelectCollateralToCharge -- found uncooperative node (didn't %s transaction), charging " - "fees: %s", + "fees: %s", // NOLINT(bitcoin-unterminated-logprintf) (state == POOL_STATE_SIGNING) ? "sign" : "send", vecOffendersCollaterals[0]->ToString()); m_collaterals_being_charged.push_back(vecOffendersCollaterals[0]); return vecOffendersCollaterals[0]; @@ -610,8 +610,8 @@ void CCoinJoinServer::ChargeRandomFees(const std::vector& colla for (const auto& txCollateral : collaterals) { if (GetRand(/*nMax=*/100) > 10) return; - LogPrint(BCLog::COINJOIN, /* Continued */ - "CCoinJoinServer::ChargeRandomFees -- charging random fees, txCollateral=%s", txCollateral->ToString()); + LogPrint(BCLog::COINJOIN, + "CCoinJoinServer::ChargeRandomFees -- charging random fees, txCollateral=%s", txCollateral->ToString()); // NOLINT(bitcoin-unterminated-logprintf) ConsumeCollateral(txCollateral); } } @@ -894,7 +894,7 @@ bool CCoinJoinServer::AddEntry(const CCoinJoinEntry& entry, PoolMessage& nMessag // PROMOTION_RATIO outputs; pre-V24 entries are capped at COINJOIN_ENTRY_MAX_SIZE (9) const size_t nMaxEntrySize = fRebalanceSession ? static_cast(CoinJoin::PROMOTION_RATIO) : COINJOIN_ENTRY_MAX_SIZE; if (entry.vecTxDSIn.size() > nMaxEntrySize || entry.vecTxOut.size() > nMaxEntrySize) { - LogPrint(BCLog::COINJOIN, /* Continued */ + LogPrint(BCLog::COINJOIN, "CCoinJoinServer::%s -- ERROR: too many inputs or outputs! inputs=%s/%s, outputs=%s/%s\n", __func__, entry.vecTxDSIn.size(), nMaxEntrySize, entry.vecTxOut.size(), nMaxEntrySize); nMessageIDRet = ERR_MAXIMUM; @@ -1147,7 +1147,7 @@ bool CCoinJoinServer::CreateNewSession(const CCoinJoinAccept& dsa, int nPeerVers m_queueman.AddQueue(std::move(dsq)); } - LogPrint(BCLog::COINJOIN, /* Continued */ + LogPrint(BCLog::COINJOIN, "CCoinJoinServer::CreateNewSession -- new session created, nSessionID: %d nSessionDenom: %d (%s) " "participants: %d CoinJoin::GetMaxPoolParticipants(): %d\n", nSessionID, nSessionDenom, CoinJoin::DenominationToString(nSessionDenom), nParticipants, @@ -1246,7 +1246,7 @@ bool CCoinJoinServer::AddUserToExistingSession(const CCoinJoinAccept& dsa, int n // A resent or replayed dsa must not be counted as a new participant; see SessionCollaterals. if (const auto prevout = m_session_collaterals.FindCommittedPrevout(dsa.txCollateral)) { - LogPrint(BCLog::COINJOIN, /* Continued */ + LogPrint(BCLog::COINJOIN, "CCoinJoinServer::AddUserToExistingSession -- collateral %s spends prevout %s already committed to " "this session\n", dsa.txCollateral.GetHash().ToString(), prevout->ToStringShort()); @@ -1269,7 +1269,7 @@ bool CCoinJoinServer::AddUserToExistingSession(const CCoinJoinAccept& dsa, int n m_mapDeclaredShapes.emplace(dsa.txCollateral.GetHash(), DeclaredShape(dsa)); m_session_collaterals.Add(dsa.txCollateral); - LogPrint(BCLog::COINJOIN, /* Continued */ + LogPrint(BCLog::COINJOIN, "CCoinJoinServer::AddUserToExistingSession -- new user accepted, nSessionID: %d nSessionDenom: %d (%s) " "participants: %d CoinJoin::GetMaxPoolParticipants(): %d\n", nSessionID, nSessionDenom, CoinJoin::DenominationToString(nSessionDenom), m_session_collaterals.size(), @@ -1397,7 +1397,7 @@ void CCoinJoinServer::ResetSigningSessionIfCurrent(int session_id) AssertLockNotHeld(cs_coinjoin); LOCK(cs_coinjoin); if (nSessionID != session_id || nState != POOL_STATE_SIGNING) { - LogPrint(BCLog::COINJOIN, /* Continued */ + LogPrint(BCLog::COINJOIN, "CCoinJoinServer::%s -- signing session %d is no longer current, not resetting\n", __func__, session_id); return; } diff --git a/src/coinjoin/walletman.cpp b/src/coinjoin/walletman.cpp index dc774c18bf87..1717929e6551 100644 --- a/src/coinjoin/walletman.cpp +++ b/src/coinjoin/walletman.cpp @@ -280,7 +280,7 @@ MessageProcessingResult CJWalletManagerImpl::ProcessDSQueue(NodeId from, CConnma if (m_queueman->HasQueueFromMasternode(dsq.masternodeOutpoint, dsq.fReady)) { // no way the same mn can send another dsq with the same readiness this soon - LogPrint(BCLog::COINJOIN, /* Continued */ + LogPrint(BCLog::COINJOIN, "DSQUEUE -- Peer %d is sending WAY too many dsq messages for a masternode with collateral %s\n", from, dsq.masternodeOutpoint.ToStringShort()); return ret; diff --git a/src/dbwrapper.cpp b/src/dbwrapper.cpp index ecc406699b32..8d07afbc56f2 100644 --- a/src/dbwrapper.cpp +++ b/src/dbwrapper.cpp @@ -102,7 +102,7 @@ class CBitcoinLevelDBLogger : public leveldb::Logger { assert(p <= limit); base[std::min(bufsize - 1, (int)(p - base))] = '\0'; - LogPrintLevel(BCLog::LEVELDB, BCLog::Level::Debug, "%s", base); /* Continued */ + LogPrintLevel(BCLog::LEVELDB, BCLog::Level::Debug, "%s", base); // NOLINT(bitcoin-unterminated-logprintf) if (base != buffer) { delete[] base; } diff --git a/src/evo/creditpool.cpp b/src/evo/creditpool.cpp index 3f0aeb52e665..980ebab3c236 100644 --- a/src/evo/creditpool.cpp +++ b/src/evo/creditpool.cpp @@ -227,7 +227,7 @@ CCreditPool CCreditPoolManager::ConstructCreditPool(const gsl::not_null 0 || blockData.credit_pool > 0) { - LogPrint(BCLog::CREDITPOOL, /* Continued */ + LogPrint(BCLog::CREDITPOOL, "CCreditPoolManager: asset unlock limits on height: %d locked: %d.%08d limit: %d.%08d " "unlocked-in-window: %d.%08d locked-at-window-start: %d.%08d\n", block_index->nHeight, blockData.credit_pool / COIN, blockData.credit_pool % COIN, currentLimit / COIN, diff --git a/src/evo/specialtxman.cpp b/src/evo/specialtxman.cpp index eb2f68cb730a..c792004828e4 100644 --- a/src/evo/specialtxman.cpp +++ b/src/evo/specialtxman.cpp @@ -430,7 +430,7 @@ bool CSpecialTxProcessor::ApplyTxToList(const CTransaction& tx, gsl::not_nullproTxHash); if (debugLogs) { - LogPrintf("%s -- MN %s removed from list because collateral was used for " /* Continued */ + LogPrintf("%s -- MN %s removed from list because collateral was used for " "a new ProRegTx. collateralOutpoint=%s, nHeight=%d, mapCurMNs.allMNsCount=%d\n", __func__, replacedDmn->proTxHash.ToString(), dmn->collateralOutpoint.ToStringShort(), nHeight, newList.GetCounts().total()); @@ -794,7 +794,7 @@ bool CSpecialTxProcessor::FinishListForBlock(const CBlock& block, gsl::not_null< newList.RemoveMN(dmn->proTxHash); if (debugLogs) { - LogPrintf("%s -- MN %s removed from list because collateral was spent. " /* Continued */ + LogPrintf("%s -- MN %s removed from list because collateral was spent. " "collateralOutpoint=%s, nHeight=%d, mapCurMNs.allMNsCount=%d\n", __func__, dmn->proTxHash.ToString(), dmn->collateralOutpoint.ToStringShort(), nHeight, newList.GetCounts().total()); diff --git a/src/flat-database.h b/src/flat-database.h index f0fb0209ab72..427696543c04 100644 --- a/src/flat-database.h +++ b/src/flat-database.h @@ -164,7 +164,7 @@ class CFlatDB if (readResult == ReadResult::FileError) LogPrintf("Missing file %s, will try to recreate\n", strFilename); else if (readResult != ReadResult::Ok) { - LogPrintf("ERROR: CFlatDB::Read Error reading %s: ", strFilename); + LogPrintf("ERROR: CFlatDB::Read Error reading %s: ", strFilename); // NOLINT(bitcoin-unterminated-logprintf) if (readResult == ReadResult::IncorrectFormat) { LogPrintf("%s: Magic is ok but data has invalid format, will try to recreate\n", __func__); } else { diff --git a/src/governance/governance.cpp b/src/governance/governance.cpp index c9559ec039c3..419818fcec6c 100644 --- a/src/governance/governance.cpp +++ b/src/governance/governance.cpp @@ -261,7 +261,7 @@ bool CGovernanceManager::ProcessObject(const std::string& peer_str, const uint25 bool unused_rcb; if (fRateCheckBypassed && fIsValid && !MasternodeRateCheck(govobj, true, true, unused_rcb)) { - LogPrint(BCLog::GOBJECT, /* Continued */ + LogPrint(BCLog::GOBJECT, "MNGOVERNANCEOBJECT -- masternode rate check failed (after signature verification) - %s - (current " "block height %d)\n", strHash, nCachedBlockHeight.load()); diff --git a/src/governance/net_governance.cpp b/src/governance/net_governance.cpp index 4fd4d898468a..520078e1c7e6 100644 --- a/src/governance/net_governance.cpp +++ b/src/governance/net_governance.cpp @@ -233,7 +233,7 @@ void NetGovernance::ProcessMessage(CNode& peer, const std::string& msg_type, CDa ::cs_main, return m_peer_manager->PeerConsumeObjectRequest(peer.GetId(), CInv{MSG_GOVERNANCE_OBJECT_VOTE, nHash})); if (!announced_or_requested) { - LogPrint(BCLog::GOBJECT, /* Continued */ + LogPrint(BCLog::GOBJECT, "MNGOVERNANCEOBJECTVOTE -- Received unrequested vote object: %s, hash: %s, peer = %d\n", vote.ToString(tip_mn_list), strHash, peer.GetId()); return; diff --git a/src/governance/object.cpp b/src/governance/object.cpp index 6e7f161d55f2..716326cecdad 100644 --- a/src/governance/object.cpp +++ b/src/governance/object.cpp @@ -513,7 +513,7 @@ std::set CGovernanceObject::RemoveInvalidVotes(const CDeterministicMNLi for (const auto& h : removedVotes) { removedStr += strprintf(" %s\n", h.ToString()); } - LogPrintf("CGovernanceObject::%s -- Removed %d invalid votes for %s from MN %s:\n%s", __func__, removedVotes.size(), nParentHash.ToString(), mnOutpoint.ToString(), removedStr); /* Continued */ + LogPrintf("CGovernanceObject::%s -- Removed %d invalid votes for %s from MN %s:\n%s", __func__, removedVotes.size(), nParentHash.ToString(), mnOutpoint.ToString(), removedStr); // NOLINT(bitcoin-unterminated-logprintf) fDirtyCache = true; return removedVotes; diff --git a/src/governance/signing.cpp b/src/governance/signing.cpp index edb492c5b23b..b5c43f4a970f 100644 --- a/src/governance/signing.cpp +++ b/src/governance/signing.cpp @@ -202,7 +202,7 @@ void GovernanceSigner::VoteGovernanceTriggers(const std::optionalGetDataAsPlainString()); if (nBlockHeight != entry.sb->GetBlockHeight()) { - LogPrint(BCLog::GOBJECT, /* Continued */ + LogPrint(BCLog::GOBJECT, "IsSuperblockTriggered -- block height doesn't match nBlockHeight = %d, blockStart = %d, " "continuing\n", nBlockHeight, entry.sb->GetBlockHeight()); diff --git a/src/gsl/assert.cpp b/src/gsl/assert.cpp index df98f8a1aa70..e1a6186cc436 100644 --- a/src/gsl/assert.cpp +++ b/src/gsl/assert.cpp @@ -33,7 +33,7 @@ namespace details << loc.column() << ":" << loc.function_name() << std::endl; std::cerr << s.str() << std::flush; - LogPrintf("%s", s.str()); /* Continued */ + LogPrintf("%s", s.str()); // NOLINT(bitcoin-unterminated-logprintf) std::terminate(); #endif // defined(GSL_MSVC_USE_STL_NOEXCEPTION_WORKAROUND) } diff --git a/src/index/base.cpp b/src/index/base.cpp index 7c9b9056b2ce..f0edc24fb7b8 100644 --- a/src/index/base.cpp +++ b/src/index/base.cpp @@ -395,7 +395,7 @@ void BaseIndex::BlockConnected(const std::shared_ptr& block, const // in the ValidationInterface queue backlog even after the sync thread has caught up to the // new chain tip. In this unlikely event, log a warning and let the queue clear. if (best_block_index->GetAncestor(pindex->nHeight - 1) != pindex->pprev) { - LogPrintf("%s: WARNING: Block %s does not connect to an ancestor of " /* Continued */ + LogPrintf("%s: WARNING: Block %s does not connect to an ancestor of " "known best chain (tip=%s); not updating index\n", __func__, pindex->GetBlockHash().ToString(), best_block_index->GetBlockHash().ToString()); @@ -434,7 +434,7 @@ void BaseIndex::BlockDisconnected(const std::shared_ptr& block, co // a parent — otherwise same-height siblings would incorrectly trigger a rewind. if (best_block_index && best_block_index->nHeight >= pindex->nHeight && pindex->pprev) { if (best_block_index->GetAncestor(pindex->nHeight) != pindex) { - LogPrintf("%s: WARNING: Block %s is not on the indexed chain " /* Continued */ + LogPrintf("%s: WARNING: Block %s is not on the indexed chain " "(tip=%s); not updating index\n", __func__, pindex->GetBlockHash().ToString(), best_block_index->GetBlockHash().ToString()); return; @@ -471,7 +471,7 @@ void BaseIndex::ChainStateFlushed(const CBlockLocator& locator) // event, log a warning and let the queue clear. const CBlockIndex* best_block_index = m_best_block_index.load(); if (best_block_index->GetAncestor(locator_tip_index->nHeight) != locator_tip_index) { - LogPrintf("%s: WARNING: Locator contains block (hash=%s) not on known best " /* Continued */ + LogPrintf("%s: WARNING: Locator contains block (hash=%s) not on known best " "chain (tip=%s); not writing index locator\n", __func__, locator_tip_hash.ToString(), best_block_index->GetBlockHash().ToString()); diff --git a/src/init.cpp b/src/init.cpp index 793e558f20d7..1d4e541d2c70 100644 --- a/src/init.cpp +++ b/src/init.cpp @@ -1530,7 +1530,7 @@ bool AppInitMain(NodeContext& node, interfaces::BlockAndHeaderTipInfo* tip_info) // Warn about relative -datadir path. if (args.IsArgSet("-datadir") && !args.GetPathArg("-datadir").is_absolute()) { - LogPrintf("Warning: relative datadir option '%s' specified, which will be interpreted relative to the " /* Continued */ + LogPrintf("Warning: relative datadir option '%s' specified, which will be interpreted relative to the " "current working directory '%s'. This is fragile, because if Dash Core is started in the future " "from a different location, it will be unable to locate the current data files. There could " "also be data loss if Dash Core is started while in a temporary directory.\n", @@ -2591,7 +2591,7 @@ bool AppInitMain(NodeContext& node, interfaces::BlockAndHeaderTipInfo* tip_info) if (!result.repair_errors.empty()) { // Critical errors occurred - reindex required - LogPrintf("Failed to repair masternode list diffs. Database corruption detected. " /* Continued */ + LogPrintf("Failed to repair masternode list diffs. Database corruption detected. " "Please restart with -reindex to rebuild the database.\n" "Errors:\n%s\n", Join(result.repair_errors, "\n")); diff --git a/src/instantsend/net_instantsend.cpp b/src/instantsend/net_instantsend.cpp index edbe314aadab..2c0dcef517a0 100644 --- a/src/instantsend/net_instantsend.cpp +++ b/src/instantsend/net_instantsend.cpp @@ -213,7 +213,7 @@ Uint256HashSet NetInstantSend::ApplyVerificationResults( if (it != data.recSigs.end()) { auto recSig = std::make_shared(std::move(it->second)); if (!m_sigman.HasRecoveredSigForId(llmq_params.type, recSig->getId())) { - LogPrint(BCLog::INSTANTSEND, /* Continued */ + LogPrint(BCLog::INSTANTSEND, "NetInstantSend::%s -- txid=%s, islock=%s: " "passing reconstructed recSig to signing mgr, peer=%d\n", __func__, islock->txid.ToString(), hash.ToString(), nodeId); @@ -388,7 +388,7 @@ void NetInstantSend::ProcessInstantSendLock(NodeId from, const uint256& hash, co // Let's see if the TX that was locked by this islock is already mined in a ChainLocked block. If yes, // we can simply ignore the islock, as the ChainLock implies locking of all TXs in that chain if (minedHeight.has_value() && m_chainlocks.HasChainLock(*minedHeight, hashBlock)) { - LogPrint(BCLog::INSTANTSEND, /* Continued */ + LogPrint(BCLog::INSTANTSEND, "NetInstantSend::%s -- txlock=%s, islock=%s: dropping islock as it already got a " "ChainLock in block %s, peer=%d\n", __func__, islock->txid.ToString(), hash.ToString(), hashBlock.ToString(), from); diff --git a/src/llmq/blockprocessor.cpp b/src/llmq/blockprocessor.cpp index 71038471930d..99edf5dafe3a 100644 --- a/src/llmq/blockprocessor.cpp +++ b/src/llmq/blockprocessor.cpp @@ -336,7 +336,7 @@ bool CQuorumBlockProcessor::ProcessCommitment(Chainstate& chainstate, int nHeigh uint256 quorumHash = GetQuorumBlockHash(llmq_params, chainstate.m_chain, nHeight, qc.quorumIndex); - LogPrint(BCLog::LLMQ, /* Continued */ + LogPrint(BCLog::LLMQ, "%s -- processing commitment for block height=%d, type=%d, quorumIndex=%d, quorumHash=%s, signers=%s, " "validMembers=%d, quorumPublicKey=%s " "fJustCheck[%d] processing commitment from block.\n", @@ -349,7 +349,7 @@ bool CQuorumBlockProcessor::ProcessCommitment(Chainstate& chainstate, int nHeigh } if (quorumHash.IsNull()) { - LogPrint(BCLog::LLMQ, /* Continued */ + LogPrint(BCLog::LLMQ, "%s -- height=%d, type=%d, quorumIndex=%d, quorumHash=%s, signers=%s, validMembers=%d, " "quorumPublicKey=%s quorumHash is null.\n", __func__, nHeight, std23::to_underlying(qc.llmqType), qc.quorumIndex, quorumHash.ToString(), qc.CountSigners(), @@ -357,7 +357,7 @@ bool CQuorumBlockProcessor::ProcessCommitment(Chainstate& chainstate, int nHeigh return state.Invalid(BlockValidationResult::BLOCK_CONSENSUS, "bad-qc-block"); } if (quorumHash != qc.quorumHash) { - LogPrint(BCLog::LLMQ, /* Continued */ + LogPrint(BCLog::LLMQ, "%s -- height=%d, type=%d, quorumIndex=%d, quorumHash=%s, qc.quorumHash=%s signers=%s, " "validMembers=%d, quorumPublicKey=%s non equal quorumHash.\n", __func__, nHeight, std23::to_underlying(qc.llmqType), qc.quorumIndex, quorumHash.ToString(), @@ -367,7 +367,7 @@ bool CQuorumBlockProcessor::ProcessCommitment(Chainstate& chainstate, int nHeigh if (qc.IsNull()) { if (!qc.VerifyNull()) { - LogPrint(BCLog::LLMQ, /* Continued */ + LogPrint(BCLog::LLMQ, "%s -- height=%d, type=%d, quorumIndex=%d, quorumHash=%s, signers=%s, validMembers=%dqc " "verifynull failed.\n", __func__, nHeight, std23::to_underlying(qc.llmqType), qc.quorumIndex, quorumHash.ToString(), @@ -405,7 +405,7 @@ bool CQuorumBlockProcessor::ProcessCommitment(Chainstate& chainstate, int nHeigh // we don't validate signatures here; they already validated on previous step if (!qc.Verify({m_dmnman, m_qsnapman, m_chainman, pQuorumBaseBlockIndex}, /*checksigs=*/false)) { - LogPrint(BCLog::LLMQ, /* Continued */ + LogPrint(BCLog::LLMQ, "%s -- height=%d, type=%d, quorumIndex=%d, quorumHash=%s, signers=%s, validMembers=%d, " "quorumPublicKey=%s qc verify failed.\n", __func__, nHeight, std23::to_underlying(qc.llmqType), qc.quorumIndex, quorumHash.ToString(), qc.CountSigners(), diff --git a/src/llmq/net_dkg.cpp b/src/llmq/net_dkg.cpp index d320c234af90..22158bd9cccd 100644 --- a/src/llmq/net_dkg.cpp +++ b/src/llmq/net_dkg.cpp @@ -836,7 +836,7 @@ static void AddQuorumProbeConnections(const Consensus::LLMQParams& llmqParams, C dmn->pdmnState->netInfo->GetPrimary().ToStringAddrPort()); } } - LogPrint(BCLog::NET_NETCONN, debugMsg.c_str()); /* Continued */ + LogPrint(BCLog::NET_NETCONN, debugMsg.c_str()); } connman.AddPendingProbeConnections(probeConnections); } diff --git a/src/llmq/net_quorum.cpp b/src/llmq/net_quorum.cpp index cde8776470b1..eaf67ff7e833 100644 --- a/src/llmq/net_quorum.cpp +++ b/src/llmq/net_quorum.cpp @@ -824,7 +824,7 @@ bool EnsureQuorumConnections(const Consensus::LLMQParams& llmqParams, CConnman& dmn->pdmnState->netInfo->GetPrimary().ToStringAddrPort()); } } - LogPrint(BCLog::NET_NETCONN, debugMsg.c_str()); /* Continued */ + LogPrint(BCLog::NET_NETCONN, debugMsg.c_str()); } connman.SetMasternodeQuorumNodes(llmqParams.type, util_params.m_base_index->GetBlockHash(), connections); } diff --git a/src/llmq/signing_shares.cpp b/src/llmq/signing_shares.cpp index 5aa2df60dea1..d1998ffced08 100644 --- a/src/llmq/signing_shares.cpp +++ b/src/llmq/signing_shares.cpp @@ -474,7 +474,7 @@ bool CSigSharesManager::ProcessMessageSigShare(NodeId fromId, const CSigShare& s LOCK(cs); if (alreadyRecovered) { - LogPrint(BCLog::LLMQ_SIGS, /* Continued */ + LogPrint(BCLog::LLMQ_SIGS, "CSigSharesManager::%s -- dropping sigShare for recovered session. signHash=%s, id=%s, " "msgHash=%s, member=%d, node=%d\n", __func__, signHash.ToString(), sigShare.getId().ToString(), sigShare.getMsgHash().ToString(), @@ -699,7 +699,7 @@ std::shared_ptr CSigSharesManager::TryRecoverSig(const CQuorum& q std::shared_ptr singleMemberRecoveredSig; if (quorum.params.is_single_member()) { if (sigSharesForSignHash->empty()) { - LogPrint(BCLog::LLMQ_SIGS, /* Continued */ + LogPrint(BCLog::LLMQ_SIGS, "CSigSharesManager::%s -- impossible to recover single-node signature - no shares yet. id=%s, " "msgHash=%s\n", __func__, id.ToString(), msgHash.ToString()); @@ -835,12 +835,12 @@ bool CSigSharesManager::AsyncSignIfMember(Consensus::LLMQType llmqType, const ui db.GetVoteForId(llmqType, id, prevMsgHash); if (msgHash != prevMsgHash) { if (allowDiffMsgHashSigning) { - LogPrintf("%s -- already voted for id=%s and msgHash=%s. Signing for different " /* Continued */ + LogPrintf("%s -- already voted for id=%s and msgHash=%s. Signing for different " "msgHash=%s\n", __func__, id.ToString(), prevMsgHash.ToString(), msgHash.ToString()); hasVoted = false; } else { - LogPrintf("%s -- already voted for id=%s and msgHash=%s. Not voting on " /* Continued */ + LogPrintf("%s -- already voted for id=%s and msgHash=%s. Not voting on " "conflicting msgHash=%s\n", __func__, id.ToString(), prevMsgHash.ToString(), msgHash.ToString()); return false; @@ -1053,7 +1053,7 @@ void CSigSharesManager::CollectSigSharesToSendConcentrated(std::unordered_mapproTxHash; if (quorum.IsValidMember(proTxHash) && (proTxHash == myProTxHash || proTxToNode.contains(proTxHash))) { if (attempt != signedSession.attempt) { - LogPrint(BCLog::LLMQ_SIGS, /* Continued */ + LogPrint(BCLog::LLMQ_SIGS, "CSigSharesManager::%s -- signHash=%s, skipped %d unreachable recovery members\n", __func__, signedSession.sigShare.GetSignHash().ToString(), attempt - signedSession.attempt); } @@ -1412,13 +1412,13 @@ void CSigSharesManager::Cleanup() } } - LogPrintLevel(BCLog::LLMQ_SIGS, BCLog::Level::Info, /* Continued */ + LogPrintLevel(BCLog::LLMQ_SIGS, BCLog::Level::Info, "CSigSharesManager::%s -- signing session timed out. signHash=%s, id=%s, msgHash=%s, " "sigShareCount=%d, missingMembers=%s\n", __func__, signHash.ToString(), oneSigShare.getId().ToString(), oneSigShare.getMsgHash().ToString(), count, strMissingMembers); } else { - LogPrintLevel(BCLog::LLMQ_SIGS, BCLog::Level::Info, /* Continued */ + LogPrintLevel(BCLog::LLMQ_SIGS, BCLog::Level::Info, "CSigSharesManager::%s -- signing session timed out. signHash=%s, sigShareCount=%d\n", __func__, signHash.ToString(), count); } @@ -1581,7 +1581,7 @@ std::optional CSigSharesManager::CreateSigShareForSingleMember(const sigShare.UpdateKey(); - LogPrint(BCLog::LLMQ_SIGS, /* Continued */ + LogPrint(BCLog::LLMQ_SIGS, "CSigSharesManager::%s -- created sigShare. signHash=%s, id=%s, msgHash=%s, llmqType=%d, quorum=%s, " "time=%s\n", __func__, signHash.ToString(), sigShare.getId().ToString(), sigShare.getMsgHash().ToString(), diff --git a/src/net_processing.cpp b/src/net_processing.cpp index a38b79088cec..26ac1e5b3afe 100644 --- a/src/net_processing.cpp +++ b/src/net_processing.cpp @@ -3538,7 +3538,7 @@ bool PeerManagerImpl::PrepareBlockFilterRequest(CNode& node, Peer& peer, uint32_t stop_height = stop_index->nHeight; if (start_height > stop_height) { - LogPrint(BCLog::NET, "peer %d sent invalid getcfilters/getcfheaders with " /* Continued */ + LogPrint(BCLog::NET, "peer %d sent invalid getcfilters/getcfheaders with " "start height %d and stop height %d\n", node.GetId(), start_height, stop_height); node.fDisconnect = true; diff --git a/src/node/blockstorage.cpp b/src/node/blockstorage.cpp index 6c27552bcd89..dda05b7f5f40 100644 --- a/src/node/blockstorage.cpp +++ b/src/node/blockstorage.cpp @@ -727,7 +727,7 @@ bool BlockManager::FindBlockPos(FlatFilePos& pos, unsigned int nAddSize, unsigne // a reindex. A flush error might also leave some of the data files // untrimmed. if (!FlushBlockFile(!fKnown, finalize_undo)) { - LogPrintLevel(BCLog::BLOCKSTORAGE, BCLog::Level::Warning, /* Continued */ + LogPrintLevel(BCLog::BLOCKSTORAGE, BCLog::Level::Warning, "Failed to flush previous block file %05i (finalize=%i, finalize_undo=%i) before opening new block file %05i\n", m_last_blockfile, !fKnown, finalize_undo, nFile); } diff --git a/src/node/miner.cpp b/src/node/miner.cpp index 1def21c59a01..a7ccaf6a1f65 100644 --- a/src/node/miner.cpp +++ b/src/node/miner.cpp @@ -214,7 +214,7 @@ void BlockAssembler::FillBlockPayments(CMutableTransaction& txNew, const CBlockI voutMasternodeStr += txout.ToString(); } - LogPrint(BCLog::MNPAYMENTS, "%s -- nBlockHeight %d blockReward %lld voutMasternodePaymentsRet \"%s\" txNew %s", __func__, /* Continued */ + LogPrint(BCLog::MNPAYMENTS, "%s -- nBlockHeight %d blockReward %lld voutMasternodePaymentsRet \"%s\" txNew %s", __func__, // NOLINT(bitcoin-unterminated-logprintf) nBlockHeight, blockSubsidy + feeReward, voutMasternodeStr, txNew.ToString()); } diff --git a/src/node/sync_manager.cpp b/src/node/sync_manager.cpp index 1573e25c21d0..64d8b5598080 100644 --- a/src/node/sync_manager.cpp +++ b/src/node/sync_manager.cpp @@ -226,7 +226,7 @@ void SyncManager::ProcessTick() if (!pNodeTmp->IsInboundConn() && !fRequestedEarlier && !pNodeTmp->IsBlockRelayOnly()) { m_netfulfilledman.AddFulfilledRequest(pNodeTmp->addr, "mempool-sync"); m_connman.PushMessage(pNodeTmp, msgMaker.Make(NetMsgType::MEMPOOL)); - LogPrint(BCLog::MNSYNC, /* Continued */ + LogPrint(BCLog::MNSYNC, "Sync Tick -- nTick %d asset_id %d -- syncing mempool from peer=%d\n", nTick, asset_id, pNodeTmp->GetId()); } diff --git a/src/node/utxo_snapshot.cpp b/src/node/utxo_snapshot.cpp index 3498e86203e6..479dd6598b57 100644 --- a/src/node/utxo_snapshot.cpp +++ b/src/node/utxo_snapshot.cpp @@ -58,7 +58,7 @@ bool WriteSnapshotBaseBlockhash(Chainstate& snapshot_chainstate) std::optional ReadSnapshotBaseBlockhash(fs::path chaindir) { if (!fs::exists(chaindir)) { - LogPrintf("[snapshot] cannot read base blockhash: no chainstate dir " /* Continued */ + LogPrintf("[snapshot] cannot read base blockhash: no chainstate dir " "exists at path %s\n", fs::PathToString(chaindir)); return std::nullopt; } @@ -66,7 +66,7 @@ std::optional ReadSnapshotBaseBlockhash(fs::path chaindir) const std::string read_from_str = fs::PathToString(read_from); if (!fs::exists(read_from)) { - LogPrintf("[snapshot] snapshot chainstate dir is malformed! no base blockhash file " /* Continued */ + LogPrintf("[snapshot] snapshot chainstate dir is malformed! no base blockhash file " "exists at path %s. Try deleting %s and calling loadtxoutset again?\n", fs::PathToString(chaindir), read_from_str); return std::nullopt; diff --git a/src/rpc/governance.cpp b/src/rpc/governance.cpp index 1dc0ecdba30f..861c0ceea0d1 100644 --- a/src/rpc/governance.cpp +++ b/src/rpc/governance.cpp @@ -384,7 +384,7 @@ static RPCHelpMan gobject_submit() if (node.active_ctx) { const auto& mn_activeman{*node.active_ctx->nodeman}; const bool fMnFound = mnList.HasValidMNByCollateral(mn_activeman.GetOutPoint()); - LogPrint(BCLog::GOBJECT, /* Continued */ + LogPrint(BCLog::GOBJECT, "gobject_submit -- pubKeyOperator = %s, outpoint = %s, params.size() = %lld, fMnFound = %d\n", mn_activeman.GetPubKey().ToString(false), mn_activeman.GetOutPoint().ToStringShort(), request.params.size(), fMnFound); diff --git a/src/rpc/util.cpp b/src/rpc/util.cpp index d76f94cab6c1..7ac6af392fb9 100644 --- a/src/rpc/util.cpp +++ b/src/rpc/util.cpp @@ -133,7 +133,7 @@ bool ParseBoolV(const UniValue& v, const std::string &strName) } if (!v.isBool()) { - throw JSONRPCError(RPC_INVALID_PARAMETER, strprintf("%s must be a JSON boolean. " /* Continued */ + throw JSONRPCError(RPC_INVALID_PARAMETER, strprintf("%s must be a JSON boolean. " "Pass -deprecatedrpc=permissive_bool to allow legacy boolean parsing.", strName)); } return v.get_bool(); diff --git a/src/stacktraces.cpp b/src/stacktraces.cpp index a1d59b966941..b0e35c97dcb3 100644 --- a/src/stacktraces.cpp +++ b/src/stacktraces.cpp @@ -532,7 +532,7 @@ static std::string GetCrashInfoStr(const crash_info& ci, size_t spaces) static void PrintCrashInfo(const crash_info& ci) { auto str = GetCrashInfoStr(ci); - LogPrintf("%s", str); /* Continued */ + LogPrintf("%s", str); // NOLINT(bitcoin-unterminated-logprintf) tfm::format(std::cerr, "%s", str); fflush(stderr); } diff --git a/src/sync.cpp b/src/sync.cpp index 79453ed009bf..48799b35fdbb 100644 --- a/src/sync.cpp +++ b/src/sync.cpp @@ -134,7 +134,7 @@ static void double_lock_detected(const void* mutex, const LockStack& lock_stack) LogPrintf("Lock order:\n"); for (const LockStackItem& i : lock_stack) { if (i.first == mutex) { - LogPrintf(" (*)"); /* Continued */ + LogPrintf(" (*)"); // NOLINT(bitcoin-unterminated-logprintf) } LogPrintf(" %s\n", i.second.ToString()); } diff --git a/src/test/logging_tests.cpp b/src/test/logging_tests.cpp index f65c69e2ad37..cd6e6d1c57a1 100644 --- a/src/test/logging_tests.cpp +++ b/src/test/logging_tests.cpp @@ -83,12 +83,12 @@ BOOST_AUTO_TEST_CASE(logging_timer) 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::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"); + LogPrintf_("fn1", "src1", 1, BCLog::LogFlags::NET, BCLog::Level::Debug, "foo1: %s\n", "bar1"); + LogPrintf_("fn2", "src2", 2, BCLog::LogFlags::NET, BCLog::Level::Info, "foo2: %s\n", "bar2"); + LogPrintf_("fn3", "src3", 3, BCLog::LogFlags::ALL, BCLog::Level::Debug, "foo3: %s\n", "bar3"); + LogPrintf_("fn4", "src4", 4, BCLog::LogFlags::ALL, BCLog::Level::Info, "foo4: %s\n", "bar4"); + LogPrintf_("fn5", "src5", 5, BCLog::LogFlags::NONE, BCLog::Level::Debug, "foo5: %s\n", "bar5"); + LogPrintf_("fn6", "src6", 6, BCLog::LogFlags::NONE, BCLog::Level::Info, "foo6: %s\n", "bar6"); std::ifstream file{tmp_log_path}; std::vector log_lines; for (std::string log; std::getline(file, log);) { diff --git a/src/test/util/chainstate.h b/src/test/util/chainstate.h index 4bd17e0efb63..2ac5cffca389 100644 --- a/src/test/util/chainstate.h +++ b/src/test/util/chainstate.h @@ -50,7 +50,7 @@ CreateAndActivateUTXOSnapshot( UniValue result = CreateUTXOSnapshot( node, node.chainman->ActiveChainstate(), auto_outfile, snapshot_path, snapshot_path); LogPrintf( - "Wrote UTXO snapshot to %s: %s", fs::PathToString(snapshot_path.make_preferred()), result.write()); + "Wrote UTXO snapshot to %s: %s\n", fs::PathToString(snapshot_path.make_preferred()), result.write()); // Read the written snapshot in and then activate it. // diff --git a/src/validation.cpp b/src/validation.cpp index bd394b24d84a..1e325472c142 100644 --- a/src/validation.cpp +++ b/src/validation.cpp @@ -2616,7 +2616,7 @@ bool Chainstate::IsBlockValueValid(const CBlock& block, const CBlockIndex* pinde const bool is_v24{DeploymentActiveAfter(pindex->pprev, m_chainman, Consensus::DEPLOYMENT_V24)}; if (!IsSuperblockValid(superblock, *block.vtx[0], blockReward, is_v24, nSuperblockPaymentsLimit)) { // triggered but invalid? that's weird - LogPrintf("%s -- ERROR! Invalid superblock detected at height %d: %s", __func__, nBlockHeight, block.vtx[0]->ToString()); /* Continued */ + LogPrintf("%s -- ERROR! Invalid superblock detected at height %d: %s", __func__, nBlockHeight, block.vtx[0]->ToString()); // NOLINT(bitcoin-unterminated-logprintf) // should NOT allow invalid superblocks, when superblocks are enabled strErrorRet = strprintf("invalid superblock detected at height %d", nBlockHeight); return false; @@ -2659,10 +2659,10 @@ bool Chainstate::IsBlockPayeeValid(const CBlock& block, const CBlockIndex* pinde } LogPrintf("%s -- ERROR! Failed to find expected payee %s amount=%lld height=%d\n", __func__, str_payout, txout.nValue, nBlockHeight); - LogPrintf("%s -- ERROR! Invalid masternode payment detected at height %d: %s", __func__, nBlockHeight, txNew.ToString()); /* Continued */ + LogPrintf("%s -- ERROR! Invalid masternode payment detected at height %d: %s", __func__, nBlockHeight, txNew.ToString()); // NOLINT(bitcoin-unterminated-logprintf) return false; } - LogPrint(BCLog::MNPAYMENTS, "%s -- Valid masternode payment at height %d: %s", __func__, nBlockHeight, txNew.ToString()); /* Continued */ + LogPrint(BCLog::MNPAYMENTS, "%s -- Valid masternode payment at height %d: %s", __func__, nBlockHeight, txNew.ToString()); // NOLINT(bitcoin-unterminated-logprintf) return true; } @@ -3930,7 +3930,7 @@ bool Chainstate::ActivateBestChain(BlockValidationState& state, std::shared_ptr< // Belt-and-suspenders check that we aren't attempting to advance the background // chainstate past the snapshot base block. if (WITH_LOCK(::cs_main, return m_disabled)) { - LogPrintf("m_disabled is set - this chainstate should not be in operation. " /* Continued */ + LogPrintf("m_disabled is set - this chainstate should not be in operation. " "Please report this as a bug. %s\n", PACKAGE_BUGREPORT); return false; } @@ -6419,7 +6419,7 @@ bool ChainstateManager::PopulateAndValidateSnapshot( auto maybe_au_data = ExpectedAssumeutxo(base_height, GetParams()); if (!maybe_au_data) { - LogPrintf("[snapshot] assumeutxo height in snapshot metadata not recognized " /* Continued */ + LogPrintf("[snapshot] assumeutxo height in snapshot metadata not recognized " "(%d) - refusing to load snapshot\n", base_height); return false; } @@ -6744,8 +6744,8 @@ SnapshotCompletionResult ChainstateManager::MaybeCompleteSnapshotValidation( }; if (index_new.GetBlockHash() != snapshot_blockhash) { - LogPrintf("[snapshot] supposed base block %s does not match the " /* Continued */ - "snapshot base block %s (height %d). Snapshot is not valid.", + LogPrintf("[snapshot] supposed base block %s does not match the " + "snapshot base block %s (height %d). Snapshot is not valid.\n", index_new.ToString(), snapshot_blockhash.ToString(), snapshot_base_height); handle_invalid_snapshot(); return SnapshotCompletionResult::BASE_BLOCKHASH_MISMATCH; @@ -6763,7 +6763,7 @@ SnapshotCompletionResult ChainstateManager::MaybeCompleteSnapshotValidation( auto maybe_au_data = ExpectedAssumeutxo(curr_height, m_options.chainparams); if (!maybe_au_data) { - LogPrintf("[snapshot] assumeutxo data not found for height " /* Continued */ + LogPrintf("[snapshot] assumeutxo data not found for height " "(%d) - refusing to validate snapshot\n", curr_height); handle_invalid_snapshot(); return SnapshotCompletionResult::MISSING_CHAINPARAMS; @@ -6771,7 +6771,7 @@ SnapshotCompletionResult ChainstateManager::MaybeCompleteSnapshotValidation( const AssumeutxoData& au_data = *maybe_au_data; std::optional maybe_ibd_stats; - LogPrintf("[snapshot] computing UTXO stats for background chainstate to validate " /* Continued */ + LogPrintf("[snapshot] computing UTXO stats for background chainstate to validate " "snapshot - this could take a few minutes\n"); try { maybe_ibd_stats = ComputeUTXOStats( @@ -7149,7 +7149,7 @@ bool ChainstateManager::ValidatedSnapshotCleanup() // is in-memory, in which case we can't do on-disk cleanup. You'd better be // in a unittest! if (!ibd_chainstate_path_maybe || !snapshot_chainstate_path_maybe) { - LogPrintf("[snapshot] snapshot chainstate cleanup cannot happen with " /* Continued */ + LogPrintf("[snapshot] snapshot chainstate cleanup cannot happen with " "in-memory chainstates. You are testing, right?\n"); return false; } @@ -7196,7 +7196,7 @@ bool ChainstateManager::ValidatedSnapshotCleanup() throw; } - LogPrintf("[snapshot] moving snapshot chainstate (%s) to " /* Continued */ + LogPrintf("[snapshot] moving snapshot chainstate (%s) to " "default chainstate directory (%s)\n", fs::PathToString(snapshot_chainstate_path), fs::PathToString(ibd_chainstate_path)); @@ -7217,7 +7217,7 @@ bool ChainstateManager::ValidatedSnapshotCleanup() if (!DeleteCoinsDBFromDisk(tmp_old, /*is_snapshot=*/false)) { // No need to AbortNode because once the unneeded bg chainstate data is // moved, it will not interfere with subsequent initialization. - LogPrintf("Deletion of %s failed. Please remove it manually, as the " /* Continued */ + LogPrintf("Deletion of %s failed. Please remove it manually, as the " "directory is now unnecessary.\n", fs::PathToString(tmp_old)); } else { diff --git a/src/wallet/coinjoin.cpp b/src/wallet/coinjoin.cpp index 8effeb405daf..3fa51d021684 100644 --- a/src/wallet/coinjoin.cpp +++ b/src/wallet/coinjoin.cpp @@ -223,7 +223,7 @@ std::vector CWallet::SelectCoinsGroupedByAddresses(bool fSkipD for (const auto& item : vecTallyRet) { strMessage += strprintf(" %s %f\n", EncodeDestination(item.txdest), float(item.nAmount) / COIN); } - LogPrint(BCLog::SELECTCOINS, "%s", strMessage); /* Continued */ + LogPrint(BCLog::SELECTCOINS, "%s", strMessage); // NOLINT(bitcoin-unterminated-logprintf) } return vecTallyRet; diff --git a/src/wallet/init.cpp b/src/wallet/init.cpp index f50eb4733360..bee31c7e1788 100644 --- a/src/wallet/init.cpp +++ b/src/wallet/init.cpp @@ -216,7 +216,7 @@ void WalletInit::InitCoinJoinSettings(CCoinJoinClientManager& mgr) const if (fAutoStart) { mgr.startMixing(); } - LogPrintf("CoinJoin: autostart=%d, multisession=%d," /* Continued */ + LogPrintf("CoinJoin: autostart=%d, multisession=%d," "sessions=%d, rounds=%d, amount=%d, denoms_goal=%d, denoms_hardcap=%d\n", fAutoStart, CCoinJoinClientOptions::IsMultiSessionEnabled(), CCoinJoinClientOptions::GetSessions(), CCoinJoinClientOptions::GetRounds(), diff --git a/src/wallet/wallet.cpp b/src/wallet/wallet.cpp index d3598fd130e1..4178b3582e34 100644 --- a/src/wallet/wallet.cpp +++ b/src/wallet/wallet.cpp @@ -2581,7 +2581,7 @@ bool CWallet::SignGovernanceVote(const CKeyID& keyID, CGovernanceVote& vote) con util::Result CWallet::CommitTransaction(CTransactionRef tx, mapValue_t mapValue, std::vector> orderForm) { LOCK(cs_wallet); - WalletLogPrintf("CommitTransaction:\n%s", tx->ToString()); /* Continued */ + WalletLogPrintf("CommitTransaction:\n%s", tx->ToString()); // What this commit records about the transaction. const auto record_commit{[&](CWalletTx& wtx, bool new_tx) { diff --git a/test/lint/lint-format-strings.py b/test/lint/lint-format-strings.py index 6992343d6cf6..f42cbd278aa1 100755 --- a/test/lint/lint-format-strings.py +++ b/test/lint/lint-format-strings.py @@ -82,7 +82,7 @@ def main(): matching_files_filtered = [] for matching_file in matching_files: - if not re.search('^src/(dashbls|leveldb|secp256k1|minisketch|tinyformat|test/fuzz/strprintf.cpp|clientversion.cpp)', matching_file): + if not re.search('^src/(dashbls|leveldb|secp256k1|minisketch|tinyformat|test/fuzz/strprintf.cpp|clientversion.cpp)|contrib/devtools/bitcoin-tidy/example_logprintf.cpp', matching_file): matching_files_filtered.append(matching_file) matching_files_filtered.sort() diff --git a/test/lint/lint-include-guards.py b/test/lint/lint-include-guards.py index c5de3b4394a5..e5dfb0ca6b80 100755 --- a/test/lint/lint-include-guards.py +++ b/test/lint/lint-include-guards.py @@ -19,7 +19,8 @@ HEADER_ID_PREFIX = 'BITCOIN_' HEADER_ID_SUFFIX = '_H' -EXCLUDE_FILES_WITH_PREFIX = ['src/crypto/ctaes', +EXCLUDE_FILES_WITH_PREFIX = ['contrib/devtools/bitcoin-tidy', + 'src/crypto/ctaes', 'src/tinyformat.h', 'src/bench/nanobench.h', 'src/test/fuzz/FuzzedDataProvider.h', diff --git a/test/lint/lint-includes.py b/test/lint/lint-includes.py index b09d8993e99f..ab685933affc 100755 --- a/test/lint/lint-includes.py +++ b/test/lint/lint-includes.py @@ -16,6 +16,9 @@ from lint_ignore_dirs import SHARED_EXCLUDED_SUBTREES +EXCLUDED_DIRS = ["contrib/devtools/bitcoin-tidy/", + ] + SHARED_EXCLUDED_SUBTREES + EXPECTED_BOOST_INCLUDES = ["boost/date_time/posix_time/posix_time.hpp", "boost/hana/for_each.hpp", "boost/hana/tuple.hpp", @@ -100,7 +103,7 @@ def find_extra_boosts(): def find_quote_syntax_inclusions(): - exclude_args = [":(exclude)" + dir for dir in SHARED_EXCLUDED_SUBTREES] + exclude_args = [":(exclude)" + dir for dir in EXCLUDED_DIRS] quote_syntax_inclusions = list() try: diff --git a/test/lint/lint-logs.py b/test/lint/lint-logs.py deleted file mode 100755 index de04a1aecaf5..000000000000 --- a/test/lint/lint-logs.py +++ /dev/null @@ -1,34 +0,0 @@ -#!/usr/bin/env python3 -# -# Copyright (c) 2018-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. -# -# Check that all logs are terminated with '\n' -# -# Some logs are continued over multiple lines. They should be explicitly -# commented with /* Continued */ - -import re -import sys - -from subprocess import check_output - - -def main(): - logs_list = check_output(["git", "grep", "--extended-regexp", r"(LogPrintLevel|LogPrintfCategory|LogPrintf?)\(", "--", "*.cpp"], text=True, encoding="utf8").splitlines() - - unterminated_logs = [line for line in logs_list if not re.search(r'(\\n"|/\* Continued \*/)', line)] - - if unterminated_logs != []: - print("All calls to LogPrintf(), LogPrintfCategory(), LogPrint(), LogPrintLevel(), and WalletLogPrintf() should be terminated with \"\\n\".") - print("") - - for line in unterminated_logs: - print(line) - - sys.exit(1) - - -if __name__ == "__main__": - main() From af2fcc3a233c5c21e4006a7f15fbd2b6bd26cfca Mon Sep 17 00:00:00 2001 From: Konstantin Akimov Date: Fri, 11 Aug 2023 11:36:15 +0200 Subject: [PATCH 3/8] Merge bitcoin/bitcoin#28245: doc: use llvm-config for bitcoin-tidy example d82bb90a5b9dc1fd48b10514bdcd8f425aced256 doc: use llvm-config for bitcoin-tidy example (fanquake) Pull request description: An LLVM installation will have `llvm-config` available to query for info. Ask it for the `--cmakedir`, and use that in our bitcoin-tidy example, rather than listing multiple different (potential) paths per distro/OS etc. ACKs for top commit: theuni: ACK d82bb90a5b9dc1fd48b10514bdcd8f425aced256. jonatack: ACK d82bb90a5b9dc1fd48b10514bdcd8f425aced256 TheCharlatan: Nice, Re-ACK d82bb90a5b9dc1fd48b10514bdcd8f425aced256 Tree-SHA512: e07e979231f8f000deafce0751bed4b73ff0eff995bec49e90f579c9051cf5859dac5e49554b8219d33b00c81192db979eed98fee1c643a9205ea8babfce2c5d Co-authored-by: fanquake --- contrib/devtools/bitcoin-tidy/README | 7 +++++-- 1 file changed, 5 insertions(+), 2 deletions(-) diff --git a/contrib/devtools/bitcoin-tidy/README b/contrib/devtools/bitcoin-tidy/README index 1669fe98f57f..c15e07c4ede5 100644 --- a/contrib/devtools/bitcoin-tidy/README +++ b/contrib/devtools/bitcoin-tidy/README @@ -3,6 +3,9 @@ Example Usage: ```bash -cmake -S . -B build -DLLVM_DIR=/path/to/lib/cmake/llvm -DCMAKE_BUILD_TYPE=Release -make -C build -j$(nproc) +cmake -S . -B build -DLLVM_DIR=$(llvm-config --cmakedir) -DCMAKE_BUILD_TYPE=Release + +cmake --build build -j$(nproc) + +cmake --build build --target bitcoin-tidy-tests -j$(nproc) ``` From 7481613d2fbfb3a94976a54e693c98be325ec431 Mon Sep 17 00:00:00 2001 From: Konstantin Akimov Date: Mon, 14 Aug 2023 12:56:20 +0100 Subject: [PATCH 4/8] Merge bitcoin/bitcoin#28258: bitcoin-tidy: fix macOS build bb3263d3e3d9f9d4db86dde679f469e7278bf737 bitcoin-tidy: fix macOS build (Cory Fields) Pull request description: [LLVM uses these options](https://github.com/llvm/llvm-project/blob/main/llvm/cmake/modules/HandleLLVMOptions.cmake#L178) for building as well, so there's precedent. Also fix the shared library extension which was incorrectly being set to dylib. Thanks to jonatack for reporting and debugging. ACKs for top commit: jonatack: ACK bb3263d3e3d9f9d4db86dde679f469e7278bf737 tested with arm64 macos 13.5, llvm 16.0.6 and cmake 3.27.2 Tree-SHA512: de7bfd497f38f1565a14d217d0b057cbfa788bdda702b5942b7f0b55947ae5e1c05af13e7d6a073ed036bc4db57035868f180034508b6e084ab9b901a5baaf2f Co-authored-by: fanquake --- contrib/devtools/bitcoin-tidy/CMakeLists.txt | 13 ++++++++++++- 1 file changed, 12 insertions(+), 1 deletion(-) diff --git a/contrib/devtools/bitcoin-tidy/CMakeLists.txt b/contrib/devtools/bitcoin-tidy/CMakeLists.txt index 9ed18696d41b..35e60d1d87e2 100644 --- a/contrib/devtools/bitcoin-tidy/CMakeLists.txt +++ b/contrib/devtools/bitcoin-tidy/CMakeLists.txt @@ -25,6 +25,12 @@ else() target_compile_options(bitcoin-tidy PRIVATE -fno-exceptions) endif() +if(CMAKE_HOST_APPLE) + # ld64 expects no undefined symbols by default + target_link_options(bitcoin-tidy PRIVATE -Wl,-flat_namespace) + target_link_options(bitcoin-tidy PRIVATE -Wl,-undefined -Wl,suppress) +endif() + # Add warnings if (MSVC) target_compile_options(bitcoin-tidy PRIVATE /W4) @@ -33,7 +39,12 @@ else() target_compile_options(bitcoin-tidy PRIVATE -Wextra) endif() -set(CLANG_TIDY_COMMAND "${CLANG_TIDY_EXE}" "--load=${CMAKE_BINARY_DIR}/${CMAKE_SHARED_LIBRARY_PREFIX}bitcoin-tidy${CMAKE_SHARED_LIBRARY_SUFFIX}" "-checks=-*,bitcoin-*") +if(CMAKE_VERSION VERSION_LESS 3.27) + set(CLANG_TIDY_COMMAND "${CLANG_TIDY_EXE}" "--load=${CMAKE_BINARY_DIR}/${CMAKE_SHARED_MODULE_PREFIX}bitcoin-tidy${CMAKE_SHARED_MODULE_SUFFIX}" "-checks=-*,bitcoin-*") +else() + # CLANG_TIDY_COMMAND supports generator expressions as of 3.27 + set(CLANG_TIDY_COMMAND "${CLANG_TIDY_EXE}" "--load=$" "-checks=-*,bitcoin-*") +endif() # Create a dummy library that runs clang-tidy tests as a side-effect of building add_library(bitcoin-tidy-tests OBJECT EXCLUDE_FROM_ALL example_logprintf.cpp) From d2cc64f750d4426731dd80fe99e5dc9e03f5b6ef Mon Sep 17 00:00:00 2001 From: Konstantin Akimov Date: Tue, 10 Nov 2026 18:26:32 +0700 Subject: [PATCH 5/8] fix: check masternode peers after a list change outside of validation locks ConnectBlock and DisconnectBlock fire the signal while ActivateBestChain holds cs_main and the mempool lock, and CMNAuth's handler iterates the nodes, so m_nodes_mutex is taken under mempool.cs. Together with SendMessages (m_block_inv_mutex -> m_tx_inventory_mutex -> mempool.cs) and PeerManagerImpl::UpdatedBlockTip (m_nodes_mutex -> m_block_inv_mutex) that closes a lock-order cycle. bitcoin#27675 takes mempool.cs on every inventory pass, which makes ThreadSanitizer report it. --- src/dsnotificationinterface.cpp | 5 ++++- 1 file changed, 4 insertions(+), 1 deletion(-) diff --git a/src/dsnotificationinterface.cpp b/src/dsnotificationinterface.cpp index 43c9345e172a..2db2dc7e91e6 100644 --- a/src/dsnotificationinterface.cpp +++ b/src/dsnotificationinterface.cpp @@ -89,7 +89,10 @@ void CDSNotificationInterface::BlockDisconnected(const std::shared_ptr Date: Thu, 17 Aug 2023 10:35:57 +0100 Subject: [PATCH 6/8] Merge bitcoin/bitcoin#27675: p2p: Drop m_recently_announced_invs bloom filter fb02ba3c5f5bcd96b5e3622ef001b8e57ce63fc0 mempool_entry: improve struct packing (Anthony Towns) 1a118062fbc4ec8f645f4ec4298d869a869c3344 net_processing: Clean up INVENTORY_BROADCAST_MAX constants (Anthony Towns) 6fa49937e488d0924044786c76b42324b659f351 test: Check tx from disconnected block is immediately requestable (glozow) e4ffabbffacc4b890d393aafcc8286916ef887d8 net_processing: don't add txids to m_tx_inventory_known_filter (Anthony Towns) 6ec1809d33bfc42b80cb6f35625dccd56be8d507 net_processing: drop m_recently_announced_invs bloom filter (Anthony Towns) a70beafdb22564043dc24fc98133fdadbaf77d8a validation: when adding txs due to a block reorg, allow immediate relay (Anthony Towns) 1e9684f39fba909b3501e9402d5b61f4bf744ff2 mempool_entry: add mempool entry sequence number (Anthony Towns) Pull request description: This PR replaces the `m_recently_announced_invs` bloom filter with a simple sequence number tracking the mempool state when we last considered sending an INV message to a node. This saves 33kB per peer (or more if we raise the rate at which we relay transactions over the network, in which case we would need to increase the size of the bloom filter proportionally). The philosophy here (compare with #18861 and #19109) is that we consider the rate limiting on INV messages to only be about saving bandwidth and not protecting privacy, and therefore after you receive an INV message, it's immediately fair game to request any transaction that was in the mempool at the time the INV message was sent. We likewise consider the BIP 133 feefilter and BIP 37 bloom filters to be bandwidth optimisations here, and treat transactions as requestable if they would have been announced without those filters. Given that philosophy, tracking the timestamp of the last INV message and comparing that against the mempool entry time allows removal of each of `m_recently_announced_invs`, `m_last_mempool_req` and `UNCONDITIONAL_RELAY_DELAY` and associated logic. ACKs for top commit: naumenkogs: ACK fb02ba3c5f5bcd96b5e3622ef001b8e57ce63fc0 amitiuttarwar: review ACK fb02ba3c5f5 glozow: reACK fb02ba3c5f5bcd96b5e3622ef001b8e57ce63fc0 Tree-SHA512: cbba5ee04c86df26b6057f3654c00a2b45ec94d354f4f157a769cecdaa0b509edaac02b3128afba39b023e82473fc5e28c915a787f84457ffe66638c6ac9c2d4 Co-authored-by: fanquake --- src/bench/mempool_eviction.cpp | 3 +- src/bench/mempool_stress.cpp | 3 +- src/bench/rpc_mempool.cpp | 2 +- src/kernel/mempool_entry.h | 5 +- src/net_processing.cpp | 79 +++++++-------------------- src/node/interfaces.cpp | 2 +- src/test/fuzz/util/mempool.cpp | 3 +- src/test/util/setup_common.cpp | 4 +- src/test/util/txmempool.cpp | 2 +- src/test/util/txmempool.h | 2 + src/txmempool.cpp | 11 ++++ src/txmempool.h | 4 ++ src/validation.cpp | 5 +- src/validation.h | 3 +- test/functional/mempool_limit.py | 2 +- test/functional/mempool_reorg.py | 91 +++++++++++++++++++++++++++++++- 16 files changed, 147 insertions(+), 74 deletions(-) diff --git a/src/bench/mempool_eviction.cpp b/src/bench/mempool_eviction.cpp index 46ecab50a1b9..e1c3c8ef896e 100644 --- a/src/bench/mempool_eviction.cpp +++ b/src/bench/mempool_eviction.cpp @@ -13,11 +13,12 @@ static void AddTx(const CTransactionRef& tx, const CAmount& nFee, CTxMemPool& po { int64_t nTime = 0; unsigned int nHeight = 1; + uint64_t sequence = 0; bool spendsCoinbase = false; unsigned int sigOps = 1; LockPoints lp; pool.addUnchecked(CTxMemPoolEntry( - tx, nFee, nTime, nHeight, + tx, nFee, nTime, nHeight, sequence, spendsCoinbase, sigOps, lp)); } diff --git a/src/bench/mempool_stress.cpp b/src/bench/mempool_stress.cpp index 827d36e60463..b073c61b5882 100644 --- a/src/bench/mempool_stress.cpp +++ b/src/bench/mempool_stress.cpp @@ -16,10 +16,11 @@ static void AddTx(const CTransactionRef& tx, CTxMemPool& pool) EXCLUSIVE_LOCKS_R { int64_t nTime = 0; unsigned int nHeight = 1; + uint64_t sequence = 0; bool spendsCoinbase = false; unsigned int sigOpCost = 4; LockPoints lp; - pool.addUnchecked(CTxMemPoolEntry(tx, 1000, nTime, nHeight, spendsCoinbase, sigOpCost, lp)); + pool.addUnchecked(CTxMemPoolEntry(tx, 1000, nTime, nHeight, sequence, spendsCoinbase, sigOpCost, lp)); } struct Available { diff --git a/src/bench/rpc_mempool.cpp b/src/bench/rpc_mempool.cpp index 37fd8e203e1e..84f6e32ecbb3 100644 --- a/src/bench/rpc_mempool.cpp +++ b/src/bench/rpc_mempool.cpp @@ -16,7 +16,7 @@ static void AddTx(const CTransactionRef& tx, const CAmount& fee, CTxMemPool& pool) EXCLUSIVE_LOCKS_REQUIRED(cs_main, pool.cs) { LockPoints lp; - pool.addUnchecked(CTxMemPoolEntry(tx, fee, /*time=*/0, /*entry_height=*/1, /*spends_coinbase=*/false, /*sigops_count=*/1, lp)); + pool.addUnchecked(CTxMemPoolEntry(tx, fee, /*time=*/0, /*entry_height=*/1, /*entry_sequence=*/0, /*spends_coinbase=*/false, /*sigops_count=*/1, lp)); } static void RpcMempool(benchmark::Bench& bench) diff --git a/src/kernel/mempool_entry.h b/src/kernel/mempool_entry.h index a44d9de9f7b4..9123b8d5dbbb 100644 --- a/src/kernel/mempool_entry.h +++ b/src/kernel/mempool_entry.h @@ -83,6 +83,7 @@ class CTxMemPoolEntry const int32_t nTxSize; //!< ... and avoid recomputing tx size const size_t nUsageSize; //!< ... and total memory usage const int64_t nTime; //!< Local time when entering the mempool + const uint64_t entry_sequence; //!< Sequence number used to determine whether this transaction is too recent for relay const unsigned int entryHeight; //!< Chain height when entering the mempool const bool spendsCoinbase; //!< keep track of transactions that spend a coinbase const int64_t sigOpCount; //!< Legacy sig ops plus P2SH sig op count @@ -106,7 +107,7 @@ class CTxMemPoolEntry public: CTxMemPoolEntry(const CTransactionRef& tx, CAmount fee, - int64_t time, unsigned int entry_height, + int64_t time, unsigned int entry_height, uint64_t entry_sequence, bool spends_coinbase, int64_t sigops_count, LockPoints lp) : tx{tx}, @@ -114,6 +115,7 @@ class CTxMemPoolEntry nTxSize(tx->GetTotalSize()), nUsageSize{RecursiveDynamicUsage(tx)}, nTime{time}, + entry_sequence{entry_sequence}, entryHeight{entry_height}, spendsCoinbase{spends_coinbase}, sigOpCount{sigops_count}, @@ -145,6 +147,7 @@ class CTxMemPoolEntry } std::chrono::seconds GetTime() const { return std::chrono::seconds{nTime}; } unsigned int GetHeight() const { return entryHeight; } + uint64_t GetSequence() const { return entry_sequence; } int64_t GetSigOpCount() const { return sigOpCount; } CAmount GetModifiedFee() const { return m_modified_fee; } size_t DynamicMemoryUsage() const { return nUsageSize; } diff --git a/src/net_processing.cpp b/src/net_processing.cpp index 26ac1e5b3afe..b1cf73382e5f 100644 --- a/src/net_processing.cpp +++ b/src/net_processing.cpp @@ -112,8 +112,6 @@ static constexpr auto GETDATA_TX_INTERVAL{60s}; /** Limit to avoid sending big packets. Not used in processing incoming GETDATA for compatibility */ static const unsigned int MAX_GETDATA_SZ = 1000; -/** How long a transaction has to be in the mempool before it can unconditionally be relayed. */ -static constexpr auto UNCONDITIONAL_RELAY_DELAY = 2min; /** Headers download timeout. * Timeout = base + per_header * (expected number of headers) */ static constexpr auto HEADERS_DOWNLOAD_TIMEOUT_BASE = 15min; @@ -194,13 +192,9 @@ static constexpr auto OUTBOUND_INVENTORY_BROADCAST_INTERVAL{2s}; static constexpr unsigned int INVENTORY_BROADCAST_PER_SECOND = 7; /** Maximum number of inventory items to send per transmission. */ static constexpr unsigned int INVENTORY_BROADCAST_MAX_PER_1MB_BLOCK = 4 * INVENTORY_BROADCAST_PER_SECOND * count_seconds(INBOUND_INVENTORY_BROADCAST_INTERVAL); -/** The number of most recently announced transactions a peer can request. */ -static constexpr unsigned int INVENTORY_MAX_RECENT_RELAY = 3500; -/** Verify that INVENTORY_MAX_RECENT_RELAY is enough to cache everything typically - * relayed before unconditional relay from the mempool kicks in. This is only a - * lower bound, and it should be larger to account for higher inv rate to outbound - * peers, and random variations in the broadcast mechanism. */ -static_assert(INVENTORY_MAX_RECENT_RELAY >= INVENTORY_BROADCAST_PER_SECOND * UNCONDITIONAL_RELAY_DELAY / std::chrono::seconds{1}, "INVENTORY_RELAY_MAX too low"); +static constexpr unsigned int INVENTORY_BROADCAST_MAX = 1000; +static_assert(INVENTORY_BROADCAST_MAX >= INVENTORY_BROADCAST_MAX_PER_1MB_BLOCK, "INVENTORY_BROADCAST_MAX too low"); +static_assert(INVENTORY_BROADCAST_MAX <= MAX_PEER_OBJECT_ANNOUNCEMENTS, "INVENTORY_BROADCAST_MAX too high"); /** Maximum number of compact filters that may be requested with one getcfilters. See BIP 157. */ static constexpr uint32_t MAX_GETCFILTERS_SIZE = 1000; /** Maximum number of cf hashes that may be requested with one getcfheaders. See BIP 157. */ @@ -313,9 +307,6 @@ struct Peer { /** A bloom filter for which transactions to announce to the peer. See BIP37. */ std::unique_ptr m_bloom_filter PT_GUARDED_BY(m_bloom_filter_mutex) GUARDED_BY(m_bloom_filter_mutex){nullptr}; - /** A rolling bloom filter of all announced tx CInvs to this peer */ - CRollingBloomFilter m_recently_announced_invs GUARDED_BY(NetEventsInterface::g_msgproc_mutex){INVENTORY_MAX_RECENT_RELAY, 0.000001}; - mutable RecursiveMutex m_tx_inventory_mutex; /** A filter of all the txids that the peer has announced to * us or we have announced to the peer. We use this to avoid announcing @@ -331,11 +322,12 @@ struct Peer { * permitted if the peer has NetPermissionFlags::Mempool or we advertise * NODE_BLOOM. See BIP35. */ bool m_send_mempool GUARDED_BY(m_tx_inventory_mutex){false}; - /** The last time a BIP35 `mempool` request was serviced. */ - std::atomic m_last_mempool_req{0s}; /** The next time after which we will send an `inv` message containing * transaction announcements to this peer. */ std::chrono::microseconds m_next_inv_send_time GUARDED_BY(m_tx_inventory_mutex){0}; + /** The mempool sequence num at which we sent the last `inv` message to this peer. + * Can relay txs with lower sequence numbers than this (see CTxMempool::info_for_relay). */ + uint64_t m_last_inv_sequence GUARDED_BY(NetEventsInterface::g_msgproc_mutex){1}; }; /** @@ -1114,7 +1106,7 @@ class PeerManagerImpl final : public PeerManager std::atomic m_last_tip_update{0s}; /** Determine whether or not a peer can request a transaction, and return it (or nullptr if not found or not allowed). */ - CTransactionRef FindTxForGetData(const Peer::TxRelay& tx_relay, const uint256& txid, const std::chrono::seconds mempool_req, const std::chrono::seconds now) + CTransactionRef FindTxForGetData(const Peer::TxRelay& tx_relay, const uint256& txid) EXCLUSIVE_LOCKS_REQUIRED(!m_most_recent_block_mutex, NetEventsInterface::g_msgproc_mutex); void ProcessGetData(CNode& pfrom, Peer& peer, const std::atomic& interruptMsgProc) @@ -2971,28 +2963,20 @@ void PeerManagerImpl::ProcessGetBlockData(CNode& pfrom, Peer& peer, const CInv& } //! Determine whether or not a peer can request a transaction, and return it (or nullptr if not found or not allowed). -CTransactionRef PeerManagerImpl::FindTxForGetData(const Peer::TxRelay& tx_relay, const uint256& txid, const std::chrono::seconds mempool_req, const std::chrono::seconds now) +CTransactionRef PeerManagerImpl::FindTxForGetData(const Peer::TxRelay& tx_relay, const uint256& txid) { - auto txinfo = m_mempool.info(txid); + // If a tx was in the mempool prior to the last INV for this peer, permit the request. + auto txinfo = m_mempool.info_for_relay(txid, tx_relay.m_last_inv_sequence); if (!txinfo.tx) { // A MSG_ASSET_UNLOCK getdata identifies a version 2 asset unlock by its instance hash if (const auto unlock_tx = m_mempool.GetAssetUnlockByInstanceHash(txid)) { - txinfo = m_mempool.info(unlock_tx->GetHash()); + txinfo = m_mempool.info_for_relay(unlock_tx->GetHash(), tx_relay.m_last_inv_sequence); } } if (txinfo.tx) { - // If a TX could have been INVed in reply to a MEMPOOL request, - // or is older than UNCONDITIONAL_RELAY_DELAY, permit the request - // unconditionally. - if ((mempool_req.count() && txinfo.m_time <= mempool_req) || txinfo.m_time <= now - UNCONDITIONAL_RELAY_DELAY) { - return std::move(txinfo.tx); - } + return std::move(txinfo.tx); } - // Otherwise, the transaction might have been announced recently. - bool recent = tx_relay.m_recently_announced_invs.contains(txid); - if (recent && txinfo.tx) return std::move(txinfo.tx); - // Or it might be from the most recent block { LOCK(m_most_recent_block_mutex); @@ -3015,11 +2999,6 @@ void PeerManagerImpl::ProcessGetData(CNode& pfrom, Peer& peer, const std::atomic std::vector vNotFound; const CNetMsgMaker msgMaker(pfrom.GetCommonVersion()); - const auto now{GetTime()}; - // Get last mempool request time - const auto mempool_req = tx_relay != nullptr ? tx_relay->m_last_mempool_req.load() - : std::chrono::seconds::min(); - // Process as many TX items from the front of the getdata queue as // possible, since they're common and it's efficient to batch process // them. @@ -3047,7 +3026,7 @@ void PeerManagerImpl::ProcessGetData(CNode& pfrom, Peer& peer, const std::atomic bool push = false; if (inv.IsGenTxMsg()) { - CTransactionRef tx = FindTxForGetData(*tx_relay, inv.hash, mempool_req, now); + CTransactionRef tx = FindTxForGetData(*tx_relay, inv.hash); if (tx) { CCoinJoinBroadcastTx dstx; if (inv.IsMsgDstx()) { @@ -3060,29 +3039,6 @@ void PeerManagerImpl::ProcessGetData(CNode& pfrom, Peer& peer, const std::atomic } m_mempool.RemoveUnbroadcastTx(tx->GetHash()); push = true; - - // As we're going to send tx, make sure its unconfirmed parents are made requestable. - std::vector parent_ids_to_add; - { - LOCK(m_mempool.cs); - auto tx_iter = m_mempool.GetIter(tx->GetHash()); - if (tx_iter) { - const CTxMemPoolEntry::Parents& parents = (*tx_iter)->GetMemPoolParentsConst(); - parent_ids_to_add.reserve(parents.size()); - for (const CTxMemPoolEntry& parent : parents) { - if (parent.GetTime() > now - UNCONDITIONAL_RELAY_DELAY) { - parent_ids_to_add.push_back(parent.GetTx().GetHash()); - } - } - } - } - - for (const uint256& parent_txid : parent_ids_to_add) { - // Relaying a transaction with a recent but unconfirmed parent. - if (WITH_LOCK(tx_relay->m_tx_inventory_mutex, return !tx_relay->m_tx_inventory_known_filter.contains(parent_txid))) { - tx_relay->m_recently_announced_invs.insert(parent_txid); - } - } } } @@ -6713,7 +6669,6 @@ bool PeerManagerImpl::SendMessages(CNode* pto) auto queueAndMaybePushInv = [this, pto, peer, &vInv, &msgMaker](const CInv& invIn) { LogPrint(BCLog::NET, "SendMessages -- queued inv: %s index=%d peer=%d\n", invIn.ToString(), vInv.size(), pto->GetId()); - // Responses to MEMPOOL requests bypass the m_recently_announced_invs filter. vInv.push_back(invIn); if (vInv.size() == MAX_INV_SZ) { LogPrint(BCLog::NET, "SendMessages -- pushing invs: count=%d peer=%d\n", vInv.size(), pto->GetId()); @@ -6787,7 +6742,6 @@ bool PeerManagerImpl::SendMessages(CNode* pto) tx_relay->m_tx_inventory_known_filter.insert(chainlockHash); queueAndMaybePushInv(CInv(MSG_CLSIG, chainlockHash)); } - tx_relay->m_last_mempool_req = std::chrono::duration_cast(current_time); } // Determine transactions to relay @@ -6808,7 +6762,7 @@ bool PeerManagerImpl::SendMessages(CNode* pto) // especially since we have many peers and some will draw much shorter delays. unsigned int nRelayedTransactions = 0; size_t broadcast_max{INVENTORY_BROADCAST_MAX_PER_1MB_BLOCK * MaxBlockSize() / 1000000 + (tx_relay->m_tx_inventory_to_send.size()/1000)*5}; - broadcast_max = std::min(1000, broadcast_max); + broadcast_max = std::min(INVENTORY_BROADCAST_MAX, broadcast_max); while (!vInvTx.empty() && nRelayedTransactions < broadcast_max) { // Fetch the top element from the heap @@ -6836,11 +6790,14 @@ bool PeerManagerImpl::SendMessages(CNode* pto) nInvType = MSG_DSTX; } // Send - tx_relay->m_recently_announced_invs.insert(announce_hash); nRelayedTransactions++; tx_relay->m_tx_inventory_known_filter.insert(announce_hash); queueAndMaybePushInv(CInv(nInvType, announce_hash)); } + + // Ensure we'll respond to GETDATA requests for anything we've just announced + LOCK(m_mempool.cs); + tx_relay->m_last_inv_sequence = m_mempool.GetSequence(); } } { diff --git a/src/node/interfaces.cpp b/src/node/interfaces.cpp index 5e7f32d36184..c0878655a70a 100644 --- a/src/node/interfaces.cpp +++ b/src/node/interfaces.cpp @@ -1597,7 +1597,7 @@ class ChainImpl : public Chain { if (!m_node.mempool) return {}; LockPoints lp; - CTxMemPoolEntry entry(tx, 0, 0, 0, false, 0, lp); + CTxMemPoolEntry entry(tx, 0, 0, 0, 0, false, 0, lp); const CTxMemPool::Limits& limits{m_node.mempool->m_limits}; LOCK(m_node.mempool->cs); auto ancestors{m_node.mempool->CalculateMemPoolAncestors(entry, limits)}; diff --git a/src/test/fuzz/util/mempool.cpp b/src/test/fuzz/util/mempool.cpp index 4d94196a7f64..6fbc98673204 100644 --- a/src/test/fuzz/util/mempool.cpp +++ b/src/test/fuzz/util/mempool.cpp @@ -23,9 +23,10 @@ CTxMemPoolEntry ConsumeTxMemPoolEntry(FuzzedDataProvider& fuzzed_data_provider, const CAmount fee{ConsumeMoney(fuzzed_data_provider, /*max=*/std::numeric_limits::max() / CAmount{100'000})}; assert(MoneyRange(fee)); const int64_t time = fuzzed_data_provider.ConsumeIntegral(); + const uint64_t entry_sequence{fuzzed_data_provider.ConsumeIntegral()}; const unsigned int entry_height = fuzzed_data_provider.ConsumeIntegral(); const bool spends_coinbase = fuzzed_data_provider.ConsumeBool(); const bool dip1_status = fuzzed_data_provider.ConsumeBool(); const unsigned int sig_op_cost = fuzzed_data_provider.ConsumeIntegralInRange(0, MaxBlockSigOps(dip1_status)); - return CTxMemPoolEntry{MakeTransactionRef(tx), fee, time, entry_height, spends_coinbase, sig_op_cost, {}}; + return CTxMemPoolEntry{MakeTransactionRef(tx), fee, time, entry_height, entry_sequence, spends_coinbase, sig_op_cost, {}}; } diff --git a/src/test/util/setup_common.cpp b/src/test/util/setup_common.cpp index dccfbefccd41..35407c665f43 100644 --- a/src/test/util/setup_common.cpp +++ b/src/test/util/setup_common.cpp @@ -777,7 +777,7 @@ std::vector TestChainSetup::PopulateMempool(FastRandomContext& LOCK2(cs_main, m_node.mempool->cs); LockPoints lp; m_node.mempool->addUnchecked(CTxMemPoolEntry(ptx, /*fee=*/(total_in - num_outputs * amount_per_output), - /*time=*/0, /*entry_height=*/1, + /*time=*/0, /*entry_height=*/1, /*entry_sequence=*/0, /*spends_coinbase=*/false, /*sigops_count=*/4, lp)); } --num_transactions; @@ -807,7 +807,7 @@ void TestChainSetup::MockMempoolMinFee(const CFeeRate& target_feerate) const auto tx_fee = target_feerate.GetFee(GetVirtualTransactionSize(*tx)) - m_node.mempool->m_incremental_relay_feerate.GetFee(GetVirtualTransactionSize(*tx)); m_node.mempool->addUnchecked(CTxMemPoolEntry(tx, /*fee=*/tx_fee, - /*time=*/0, /*entry_height=*/1, + /*time=*/0, /*entry_height=*/1, /*entry_sequence=*/0, /*spends_coinbase=*/true, /*sigops_count=*/1, lp)); m_node.mempool->TrimToSize(0); assert(m_node.mempool->GetMinFee() == target_feerate); diff --git a/src/test/util/txmempool.cpp b/src/test/util/txmempool.cpp index 23aff3f2e2d4..2733fa21c234 100644 --- a/src/test/util/txmempool.cpp +++ b/src/test/util/txmempool.cpp @@ -36,6 +36,6 @@ CTxMemPoolEntry TestMemPoolEntryHelper::FromTx(const CMutableTransaction& tx) co CTxMemPoolEntry TestMemPoolEntryHelper::FromTx(const CTransactionRef& tx) const { - return CTxMemPoolEntry(tx, nFee, TicksSinceEpoch(time), nHeight, + return CTxMemPoolEntry(tx, nFee, TicksSinceEpoch(time), nHeight, m_sequence, spendsCoinbase, sigOpCount, lp); } diff --git a/src/test/util/txmempool.h b/src/test/util/txmempool.h index c99ee4933ebd..0304a2111505 100644 --- a/src/test/util/txmempool.h +++ b/src/test/util/txmempool.h @@ -19,6 +19,7 @@ struct TestMemPoolEntryHelper { CAmount nFee{0}; NodeSeconds time{}; unsigned int nHeight{1}; + uint64_t m_sequence{0}; bool spendsCoinbase{false}; unsigned int sigOpCount{1}; LockPoints lp; @@ -30,6 +31,7 @@ struct TestMemPoolEntryHelper { TestMemPoolEntryHelper &Fee(CAmount _fee) { nFee = _fee; return *this; } TestMemPoolEntryHelper& Time(NodeSeconds tp) { time = tp; return *this; } TestMemPoolEntryHelper &Height(unsigned int _height) { nHeight = _height; return *this; } + TestMemPoolEntryHelper& Sequence(uint64_t _seq) { m_sequence = _seq; return *this; } TestMemPoolEntryHelper &SpendsCoinbase(bool _flag) { spendsCoinbase = _flag; return *this; } TestMemPoolEntryHelper& SigOps(unsigned int _sigops) { sigOpCount = _sigops; return *this; } }; diff --git a/src/txmempool.cpp b/src/txmempool.cpp index 4b1199e69114..7511e73d029f 100644 --- a/src/txmempool.cpp +++ b/src/txmempool.cpp @@ -1592,6 +1592,17 @@ TxMempoolInfo CTxMemPool::info(const uint256& hash) const return GetInfo(i); } +TxMempoolInfo CTxMemPool::info_for_relay(const uint256& hash, uint64_t last_sequence) const +{ + LOCK(cs); + indexed_transaction_set::const_iterator i = mapTx.find(hash); + if (i != mapTx.end() && i->GetSequence() < last_sequence) { + return GetInfo(i); + } else { + return TxMempoolInfo(); + } +} + bool CTxMemPool::existsProviderTxCrossSchemeConflict(const CTransaction& tx) const { LOCK(cs); diff --git a/src/txmempool.h b/src/txmempool.h index 3fd0fb4e170b..328a0b13b0f5 100644 --- a/src/txmempool.h +++ b/src/txmempool.h @@ -720,6 +720,10 @@ class CTxMemPool CTransactionRef get(const uint256& hash) const; TxMempoolInfo info(const uint256& hash) const; + + /** Returns info for a transaction if its entry_sequence < last_sequence */ + TxMempoolInfo info_for_relay(const uint256& hash, uint64_t last_sequence) const; + std::vector infoAll() const; /** Get the transaction holding this version 2 asset unlock instance hash, if any. */ diff --git a/src/validation.cpp b/src/validation.cpp index 1e325472c142..f901c654adeb 100644 --- a/src/validation.cpp +++ b/src/validation.cpp @@ -1045,7 +1045,10 @@ bool MemPoolAccept::PreChecks(ATMPArgs& args, Workspace& ws) } } - entry.reset(new CTxMemPoolEntry(ptx, ws.m_base_fees, nAcceptTime, m_active_chainstate.m_chain.Height(), + // Set entry_sequence to 0 when bypass_limits is used; this allows txs from a block + // reorg to be marked earlier than any child txs that were already in the mempool. + const uint64_t entry_sequence = bypass_limits ? 0 : m_pool.GetSequence(); + entry.reset(new CTxMemPoolEntry(ptx, ws.m_base_fees, nAcceptTime, m_active_chainstate.m_chain.Height(), entry_sequence, fSpendsCoinbase, nSigOps, lock_points.value())); ws.m_vsize = entry->GetTxSize(); diff --git a/src/validation.h b/src/validation.h index 80a4149e9166..995d034aec7d 100644 --- a/src/validation.h +++ b/src/validation.h @@ -255,7 +255,8 @@ struct PackageMempoolAcceptResult * @param[in] tx The transaction to submit for mempool acceptance. * @param[in] accept_time The timestamp for adding the transaction to the mempool. * It is also used to determine when the entry expires. - * @param[in] bypass_limits When true, don't enforce mempool fee and capacity limits. + * @param[in] bypass_limits When true, don't enforce mempool fee and capacity limits, + * and set entry_sequence to zero. * @param[in] test_accept When true, run validation checks but don't submit to mempool. * * @returns a MempoolAcceptResult indicating whether the transaction was accepted/rejected with reason. diff --git a/test/functional/mempool_limit.py b/test/functional/mempool_limit.py index cef1a86e19ca..8f440dea5dad 100755 --- a/test/functional/mempool_limit.py +++ b/test/functional/mempool_limit.py @@ -125,7 +125,7 @@ def run_test(self): self.log.info("Check a package that passes mempoolminfee but is evicted immediately after submission") mempoolmin_feerate = node.getmempoolinfo()["mempoolminfee"] target_weight_each = 50000 - assert_greater_than(target_weight_each * 2, node.getmempoolinfo()["maxmempool"] - node.getmempoolinfo()["bytes"]) + assert_greater_than(target_weight_each * 2, node.getmempoolinfo()["maxmempool"] - node.getmempoolinfo()["usage"]) parent_fee = (mempoolmin_feerate / 1000) * (target_weight_each // 4) - Decimal("0.00001") miniwallet.rescan_utxos() tx_parent_just_below = miniwallet.create_self_transfer(fee=parent_fee, target_weight=target_weight_each) diff --git a/test/functional/mempool_reorg.py b/test/functional/mempool_reorg.py index 3a5bc1ebcd02..18b97a044c47 100755 --- a/test/functional/mempool_reorg.py +++ b/test/functional/mempool_reorg.py @@ -8,6 +8,17 @@ that spend (directly or indirectly) coinbase transactions. """ +import time + +from test_framework.messages import ( + CInv, + MSG_TX, + msg_getdata, +) +from test_framework.p2p import ( + P2PTxInvStore, + p2p_lock, +) from test_framework.test_framework import BitcoinTestFramework from test_framework.util import assert_equal, assert_raises_rpc_error from test_framework.wallet import MiniWallet @@ -22,8 +33,84 @@ def set_test_params(self): [] ] + def test_reorg_relay(self): + self.log.info("Test that transactions from disconnected blocks are available for relay immediately") + # Prevent time from moving forward + self.nodes[1].setmocktime(int(time.time())) + self.connect_nodes(0, 1) + self.generate(self.wallet, 3) + + # Disconnect node0 and node1 to create different chains. + self.disconnect_nodes(0, 1) + # Connect a peer to node1, which doesn't have immediate tx relay + peer1 = self.nodes[1].add_p2p_connection(P2PTxInvStore()) + + # Create a transaction that is included in a block. + tx_disconnected = self.wallet.send_self_transfer(from_node=self.nodes[1]) + self.generate(self.nodes[1], 1, sync_fun=self.no_op) + + # Create a transaction and submit it to node1's mempool. + tx_before_reorg = self.wallet.send_self_transfer(from_node=self.nodes[1]) + + # Create a child of that transaction and submit it to node1's mempool. + tx_child = self.wallet.send_self_transfer(utxo_to_spend=tx_disconnected["new_utxo"], from_node=self.nodes[1]) + assert_equal(self.nodes[1].getmempoolentry(tx_child["txid"])["ancestorcount"], 1) + assert_equal(len(peer1.get_invs()), 0) + + # node0 has a longer chain in which tx_disconnected was not confirmed. + self.generate(self.nodes[0], 3, sync_fun=self.no_op) + + # Reconnect the nodes and sync chains. node0's chain should win. + self.connect_nodes(0, 1) + self.sync_blocks() + + # Child now has an ancestor from the disconnected block + assert_equal(self.nodes[1].getmempoolentry(tx_child["txid"])["ancestorcount"], 2) + assert_equal(self.nodes[1].getmempoolentry(tx_before_reorg["txid"])["ancestorcount"], 1) + + # peer1 should not have received an inv for any of the transactions during this time, as not + # enough time has elapsed for those transactions to be announced. Likewise, it cannot + # request very recent, unanounced transactions. + assert_equal(len(peer1.get_invs()), 0) + # It's too early to request these two transactions + requests_too_recent = msg_getdata([CInv(t=MSG_TX, h=int(tx["txid"], 16)) for tx in [tx_before_reorg, tx_child]]) + peer1.send_and_ping(requests_too_recent) + for _ in range(len(requests_too_recent.inv)): + peer1.sync_with_ping() + with p2p_lock: + assert "tx" not in peer1.last_message + assert "notfound" in peer1.last_message + + # Request the tx from the disconnected block + request_disconnected_tx = msg_getdata([CInv(t=MSG_TX, h=int(tx_disconnected["txid"], 16))]) + peer1.send_and_ping(request_disconnected_tx) + + # The tx from the disconnected block was never announced, and it entered the mempool later + # than the transactions that are too recent. + assert_equal(len(peer1.get_invs()), 0) + with p2p_lock: + # However, the node will answer requests for the tx from the recently-disconnected block. + assert_equal(peer1.last_message["tx"].tx.rehash(), tx_disconnected["txid"]) + + self.nodes[1].setmocktime(int(time.time()) + 30) + peer1.sync_with_ping() + # the transactions are now announced + assert_equal(len(peer1.get_invs()), 3) + for _ in range(3): + # make sure all tx requests have been responded to + peer1.sync_with_ping() + last_tx_received = peer1.last_message["tx"] + + tx_after_reorg = self.wallet.send_self_transfer(from_node=self.nodes[1]) + request_after_reorg = msg_getdata([CInv(t=MSG_TX, h=int(tx_after_reorg["txid"], 16))]) + assert tx_after_reorg["txid"] in self.nodes[1].getrawmempool() + peer1.send_and_ping(request_after_reorg) + with p2p_lock: + assert_equal(peer1.last_message["tx"], last_tx_received) + def run_test(self): - wallet = MiniWallet(self.nodes[0]) + self.wallet = MiniWallet(self.nodes[0]) + wallet = self.wallet # Start with a 200 block chain assert_equal(self.nodes[0].getblockcount(), 200) @@ -103,6 +190,8 @@ def run_test(self): assert_equal(set(self.nodes[0].getrawmempool()), set()) self.sync_all() + self.test_reorg_relay() + if __name__ == '__main__': MempoolCoinbaseTest().main() From 3b49282e6a58755db29ed63b73bb52d3c7f1259b Mon Sep 17 00:00:00 2001 From: Konstantin Akimov Date: Thu, 24 Aug 2023 10:01:50 +0100 Subject: [PATCH 7/8] Merge bitcoin/bitcoin#28321: test: Fix intermittent issue in mempool_reorg fa5cc3ccfb15d05775ace9c1827b06203ce17116 test: Fix intermittent issue in mempool_reorg (MarcoFalke) Pull request description: Currently the test case may fail intermittently, see https://github.com/bitcoin/bitcoin/issues/28313 Fix this by changing a number and reducing the failure rate a bit. ACKs for top commit: glozow: ACK fa5cc3ccfb15d05775ace9c1827b06203ce17116 Tree-SHA512: ff552111b434ca712c7dbdc5ba32a986a6fa4512cba5a756234eae69428063bf6ecfdc8f350ee84226ed4d3e4262b4639dbe49162a722e8da85f0d61e5690c51 Co-authored-by: fanquake --- test/functional/mempool_reorg.py | 6 +++--- 1 file changed, 3 insertions(+), 3 deletions(-) diff --git a/test/functional/mempool_reorg.py b/test/functional/mempool_reorg.py index 18b97a044c47..425da6e75228 100755 --- a/test/functional/mempool_reorg.py +++ b/test/functional/mempool_reorg.py @@ -68,8 +68,8 @@ def test_reorg_relay(self): assert_equal(self.nodes[1].getmempoolentry(tx_child["txid"])["ancestorcount"], 2) assert_equal(self.nodes[1].getmempoolentry(tx_before_reorg["txid"])["ancestorcount"], 1) - # peer1 should not have received an inv for any of the transactions during this time, as not - # enough time has elapsed for those transactions to be announced. Likewise, it cannot + # peer1 should not have received an inv for any of the transactions during this time, as no + # mocktime has elapsed for those transactions to be announced. Likewise, it cannot # request very recent, unanounced transactions. assert_equal(len(peer1.get_invs()), 0) # It's too early to request these two transactions @@ -92,7 +92,7 @@ def test_reorg_relay(self): # However, the node will answer requests for the tx from the recently-disconnected block. assert_equal(peer1.last_message["tx"].tx.rehash(), tx_disconnected["txid"]) - self.nodes[1].setmocktime(int(time.time()) + 30) + self.nodes[1].setmocktime(int(time.time()) + 300) peer1.sync_with_ping() # the transactions are now announced assert_equal(len(peer1.get_invs()), 3) From ac324cfdfaf4bf37d4279671cd5599b6ae78e975 Mon Sep 17 00:00:00 2001 From: Konstantin Akimov Date: Fri, 18 Aug 2023 11:21:52 +0100 Subject: [PATCH 8/8] Merge bitcoin/bitcoin#28237: refactor: Enforce C-str fmt strings in WalletLogPrintf() fa60fa3b0cba4a30726af8e0e9d1e84e14849eda bitcoin-tidy: Apply bitcoin-unterminated-logprintf to spkm as well (MarcoFalke) faa11434fe38aa82892802adb6d879d112ae1675 refactor: Enable all clang-tidy plugin bitcoin tests (MarcoFalke) fa6dc57760e0a04dbb2e365ca7ad9fd8171ebfdb refactor: Enforce C-str fmt strings in WalletLogPrintf() (MarcoFalke) fa244f3321de7884f530bb38493a8d0a0cec86ab doc: Fix bitcoin-unterminated-logprintf tidy comments (MarcoFalke) Pull request description: All fmt functions only accept a raw C-string as argument. There should never be a need to pass a format string that is not a compile-time string literal, so disallow it in `WalletLogPrintf()` to avoid accidentally introducing it. Apart from consistency, this also fixes the clang-tidy plugin bug https://github.com/bitcoin/bitcoin/pull/26296#discussion_r1286821141. ACKs for top commit: theuni: ACK fa60fa3b0cba4a30726af8e0e9d1e84e14849eda Tree-SHA512: fa6f4984c50f9b34e850bdfee7236706af586e512d866cc869cf0cdfaf9aa707029c210ca72d91f85e75fcbd8efe0d77084701de8c3d2004abfd7e46b6fa9072 Co-authored-by: fanquake --- .../bitcoin-tidy/example_logprintf.cpp | 27 +++++++++++++++---- contrib/devtools/bitcoin-tidy/logprintf.cpp | 2 -- contrib/devtools/bitcoin-tidy/logprintf.h | 1 + src/.clang-tidy | 2 +- src/wallet/scriptpubkeyman.h | 7 ++--- src/wallet/wallet.cpp | 2 +- src/wallet/wallet.h | 7 ++--- test/lint/run-lint-format-strings.py | 8 +++--- 8 files changed, 37 insertions(+), 19 deletions(-) diff --git a/contrib/devtools/bitcoin-tidy/example_logprintf.cpp b/contrib/devtools/bitcoin-tidy/example_logprintf.cpp index a3d2768964cc..a12a666c086d 100644 --- a/contrib/devtools/bitcoin-tidy/example_logprintf.cpp +++ b/contrib/devtools/bitcoin-tidy/example_logprintf.cpp @@ -2,9 +2,10 @@ // Distributed under the MIT software license, see the accompanying // file COPYING or http://www.opensource.org/licenses/mit-license.php. -// Warn about any use of LogPrintf that does not end with a newline. #include +// Test for bitcoin-unterminated-logprintf + enum LogFlags { NONE }; @@ -21,8 +22,6 @@ static inline void LogPrintf_(const std::string& logging_function, const std::st #define LogPrintLevel_(category, level, ...) LogPrintf_(__func__, __FILE__, __LINE__, category, level, __VA_ARGS__) #define LogPrintf(...) LogPrintLevel_(LogFlags::NONE, Level::None, __VA_ARGS__) -// Use a macro instead of a function for conditional logging to prevent -// evaluating arguments when logging for the category is not enabled. #define LogPrint(category, ...) \ do { \ LogPrintf(__VA_ARGS__); \ @@ -38,9 +37,23 @@ class CWallet public: template - void WalletLogPrintf(std::string fmt, Params... parameters) const + void WalletLogPrintf(const char* fmt, Params... parameters) const + { + LogPrintf(("%s " + std::string{fmt}).c_str(), GetDisplayName(), parameters...); + }; +}; + +struct ScriptPubKeyMan +{ + std::string GetDisplayName() const + { + return "default wallet"; + } + + template + void WalletLogPrintf(const char* fmt, Params... parameters) const { - LogPrintf(("%s " + fmt).c_str(), GetDisplayName(), parameters...); + LogPrintf(("%s " + std::string{fmt}).c_str(), GetDisplayName(), parameters...); }; }; @@ -52,6 +65,8 @@ void good_func2() { CWallet wallet; wallet.WalletLogPrintf("hi\n"); + ScriptPubKeyMan spkm; + spkm.WalletLogPrintf("hi\n"); const CWallet& walletref = wallet; walletref.WalletLogPrintf("hi\n"); @@ -81,6 +96,8 @@ void bad_func5() { CWallet wallet; wallet.WalletLogPrintf("hi"); + ScriptPubKeyMan spkm; + spkm.WalletLogPrintf("hi"); const CWallet& walletref = wallet; walletref.WalletLogPrintf("hi"); diff --git a/contrib/devtools/bitcoin-tidy/logprintf.cpp b/contrib/devtools/bitcoin-tidy/logprintf.cpp index 1690c8fde0a6..36beac28c86c 100644 --- a/contrib/devtools/bitcoin-tidy/logprintf.cpp +++ b/contrib/devtools/bitcoin-tidy/logprintf.cpp @@ -36,14 +36,12 @@ void LogPrintfCheck::registerMatchers(clang::ast_matchers::MatchFinder* finder) this); /* - CWallet wallet; auto walletptr = &wallet; wallet.WalletLogPrintf("foo"); wallet->WalletLogPrintf("foo"); */ finder->addMatcher( cxxMemberCallExpr( - thisPointerType(qualType(hasDeclaration(cxxRecordDecl(hasName("CWallet"))))), callee(cxxMethodDecl(hasName("WalletLogPrintf"))), hasArgument(0, stringLiteral(unterminated()).bind("logstring"))), this); diff --git a/contrib/devtools/bitcoin-tidy/logprintf.h b/contrib/devtools/bitcoin-tidy/logprintf.h index 466849ef0114..db95dfe143e8 100644 --- a/contrib/devtools/bitcoin-tidy/logprintf.h +++ b/contrib/devtools/bitcoin-tidy/logprintf.h @@ -9,6 +9,7 @@ namespace bitcoin { +// Warn about any use of LogPrintf that does not end with a newline. class LogPrintfCheck final : public clang::tidy::ClangTidyCheck { public: diff --git a/src/.clang-tidy b/src/.clang-tidy index 13466b17c43f..ce43637d0490 100644 --- a/src/.clang-tidy +++ b/src/.clang-tidy @@ -1,6 +1,6 @@ Checks: ' -*, -bitcoin-unterminated-logprintf, +bitcoin-*, bugprone-argument-comment, bugprone-use-after-move, bugprone-lambda-function-name, diff --git a/src/wallet/scriptpubkeyman.h b/src/wallet/scriptpubkeyman.h index 4fae8272ebbc..08225ce2d65d 100644 --- a/src/wallet/scriptpubkeyman.h +++ b/src/wallet/scriptpubkeyman.h @@ -260,9 +260,10 @@ class ScriptPubKeyMan virtual std::unordered_set GetScriptPubKeys() const { return {}; }; /** Prepends the wallet name in logging output to ease debugging in multi-wallet use cases */ - template - void WalletLogPrintf(std::string fmt, Params... parameters) const { - LogPrintf(("%s " + fmt).c_str(), m_storage.GetDisplayName(), parameters...); + template + void WalletLogPrintf(const char* fmt, Params... parameters) const + { + LogPrintf(("%s " + std::string{fmt}).c_str(), m_storage.GetDisplayName(), parameters...); }; /** Watch-only address added */ diff --git a/src/wallet/wallet.cpp b/src/wallet/wallet.cpp index 4178b3582e34..358855ad592d 100644 --- a/src/wallet/wallet.cpp +++ b/src/wallet/wallet.cpp @@ -2581,7 +2581,7 @@ bool CWallet::SignGovernanceVote(const CKeyID& keyID, CGovernanceVote& vote) con util::Result CWallet::CommitTransaction(CTransactionRef tx, mapValue_t mapValue, std::vector> orderForm) { LOCK(cs_wallet); - WalletLogPrintf("CommitTransaction:\n%s", tx->ToString()); + WalletLogPrintf("CommitTransaction:\n%s", tx->ToString()); // NOLINT(bitcoin-unterminated-logprintf) // What this commit records about the transaction. const auto record_commit{[&](CWalletTx& wtx, bool new_tx) { diff --git a/src/wallet/wallet.h b/src/wallet/wallet.h index 3130774a8c49..bba26b669640 100644 --- a/src/wallet/wallet.h +++ b/src/wallet/wallet.h @@ -1183,9 +1183,10 @@ class CWallet final : public WalletStorage, public interfaces::Chain::Notificati }; /** Prepends the wallet name in logging output to ease debugging in multi-wallet use cases */ - template - void WalletLogPrintf(std::string fmt, Params... parameters) const { - LogPrintf(("%s " + fmt).c_str(), GetDisplayName(), parameters...); + template + void WalletLogPrintf(const char* fmt, Params... parameters) const + { + LogPrintf(("%s " + std::string{fmt}).c_str(), GetDisplayName(), parameters...); }; /** Upgrade the wallet */ diff --git a/test/lint/run-lint-format-strings.py b/test/lint/run-lint-format-strings.py index d6adb50c38ab..ed9f5f98e5a1 100755 --- a/test/lint/run-lint-format-strings.py +++ b/test/lint/run-lint-format-strings.py @@ -24,10 +24,10 @@ ("src/util/system.cpp", "strprintf(_(COPYRIGHT_HOLDERS).translated, COPYRIGHT_HOLDERS_SUBSTITUTION)"), ("src/test/translation_tests.cpp", "strprintf(format, arg)"), ("src/validationinterface.cpp", "LogPrint(BCLog::VALIDATION, fmt \"\\n\", __VA_ARGS__)"), - ("src/wallet/wallet.h", "WalletLogPrintf(std::string fmt, Params... parameters)"), - ("src/wallet/wallet.h", "LogPrintf((\"%s \" + fmt).c_str(), GetDisplayName(), parameters...)"), - ("src/wallet/scriptpubkeyman.h", "WalletLogPrintf(std::string fmt, Params... parameters)"), - ("src/wallet/scriptpubkeyman.h", "LogPrintf((\"%s \" + fmt).c_str(), m_storage.GetDisplayName(), parameters...)"), + ("src/wallet/wallet.h", "WalletLogPrintf(const char* fmt, Params... parameters)"), + ("src/wallet/wallet.h", "LogPrintf((\"%s \" + std::string{fmt}).c_str(), GetDisplayName(), parameters...)"), + ("src/wallet/scriptpubkeyman.h", "WalletLogPrintf(const char* fmt, Params... parameters)"), + ("src/wallet/scriptpubkeyman.h", "LogPrintf((\"%s \" + std::string{fmt}).c_str(), m_storage.GetDisplayName(), parameters...)"), ] def parse_function_calls(function_name, source_code):