From a1245a5f933ea9eaf704c205bb8c2c94e2f18304 Mon Sep 17 00:00:00 2001 From: fanquake Date: Mon, 27 Mar 2023 14:22:37 +0100 Subject: [PATCH 1/5] Merge bitcoin/bitcoin#26642: clang-tidy: Add more `performance-*` checks and related fixes 03ec5b6f9ca3af28c9ce25cf2393e28ae852d808 clang-tidy: Exclude `performance-*` checks rather including them (Hennadii Stepanov) 24004372302adfc0e7cb36f8db6830694bf050e9 clang-tidy: Add `performance-type-promotion-in-math-fn` check (Hennadii Stepanov) 7e975e6cf86617346c1d8e2568f74a0252c03857 clang-tidy: Add `performance-inefficient-vector-operation` check (Hennadii Stepanov) 516b75f66ec3ba7495fc028c750937bd66cc9bba clang-tidy: Add `performance-faster-string-find` check (Hennadii Stepanov) Pull request description: ACKs for top commit: martinus: ACK 03ec5b6f9ca3af28c9ce25cf2393e28ae852d808 TheCharlatan: re-ACK [03ec5b6](https://github.com/bitcoin/bitcoin/pull/26642/commits/03ec5b6f9ca3af28c9ce25cf2393e28ae852d808) Tree-SHA512: 2dfa52f9131da88826f32583bfd534a56a998477db9804b7333c0e7ac0b6b36141009755c7163b9f95d0ecbf5c2cb63f8a69ce4b114bb83423faed21b50cec67 --- src/.clang-tidy | 9 +++++---- src/bench/lockedpool.cpp | 4 +--- src/bitcoin-cli.cpp | 1 + src/bitcoin-util.cpp | 1 + src/node/interfaces.cpp | 1 + src/qt/trafficgraphwidget.cpp | 4 ++-- src/rpc/server.cpp | 2 ++ src/rpc/util.cpp | 5 +++-- src/test/allocator_tests.cpp | 1 + src/test/fuzz/tx_pool.cpp | 1 + src/test/getarg_tests.cpp | 1 + src/test/policyestimator_tests.cpp | 1 + src/test/scheduler_tests.cpp | 2 ++ src/test/script_p2sh_tests.cpp | 4 +++- src/test/util/net.cpp | 1 + src/test/util_threadnames_tests.cpp | 1 + src/test/validation_block_tests.cpp | 1 + src/util/bip32.cpp | 2 +- src/wallet/bdb.cpp | 1 + src/wallet/rpc/backup.cpp | 1 + src/wallet/scriptpubkeyman.cpp | 1 + src/zmq/zmqpublishnotifier.cpp | 2 +- 22 files changed, 33 insertions(+), 14 deletions(-) diff --git a/src/.clang-tidy b/src/.clang-tidy index b363353d2fc4..720a45cc500d 100644 --- a/src/.clang-tidy +++ b/src/.clang-tidy @@ -6,10 +6,11 @@ misc-unused-using-decls, modernize-use-default-member-init, modernize-use-emplace, modernize-use-nullptr, -performance-for-range-copy, -performance-move-const-arg, -performance-no-automatic-move, -performance-unnecessary-copy-initialization, +performance-*, +-performance-inefficient-string-concatenation, +-performance-no-int-to-ptr, +-performance-noexcept-move-constructor, +-performance-unnecessary-value-param, readability-const-return-type, readability-redundant-declaration, readability-redundant-string-init, diff --git a/src/bench/lockedpool.cpp b/src/bench/lockedpool.cpp index ac8262654c1f..af8169f4affe 100644 --- a/src/bench/lockedpool.cpp +++ b/src/bench/lockedpool.cpp @@ -17,9 +17,7 @@ static void BenchLockedPool(benchmark::Bench& bench) const size_t synth_size = 1024*1024; Arena b(synth_base, synth_size, 16); - std::vector addr; - for (int x=0; x addr{ASIZE, nullptr}; uint32_t s = 0x12345678; bench.run([&] { int idx = s & (addr.size() - 1); diff --git a/src/bitcoin-cli.cpp b/src/bitcoin-cli.cpp index 3b0873970e9f..e1f25c73d0be 100644 --- a/src/bitcoin-cli.cpp +++ b/src/bitcoin-cli.cpp @@ -1054,6 +1054,7 @@ static void ParseGetInfoResult(UniValue& result) } std::vector formatted_proxies; + formatted_proxies.reserve(ordered_proxies.size()); for (const std::string& proxy : ordered_proxies) { formatted_proxies.emplace_back(strprintf("%s (%s)", proxy, Join(proxy_networks.find(proxy)->second, ", "))); } diff --git a/src/bitcoin-util.cpp b/src/bitcoin-util.cpp index 15d24fe7908a..869fe6c11602 100644 --- a/src/bitcoin-util.cpp +++ b/src/bitcoin-util.cpp @@ -123,6 +123,7 @@ static int Grind(const std::vector& args, std::string& strPrint) std::vector threads; int n_tasks = std::max(1u, std::thread::hardware_concurrency()); + threads.reserve(n_tasks); for (int i = 0; i < n_tasks; ++i) { threads.emplace_back(grind_task, nBits, header, i, n_tasks, std::ref(found), std::ref(proposed_nonce)); } diff --git a/src/node/interfaces.cpp b/src/node/interfaces.cpp index 385ee4aefa78..6962b34976aa 100644 --- a/src/node/interfaces.cpp +++ b/src/node/interfaces.cpp @@ -847,6 +847,7 @@ class NodeImpl : public Node if (command == "") return {}; ExternalSigner::Enumerate(command, signers, Params().NetworkIDString()); std::vector> result; + result.reserve(signers.size()); for (auto& signer : signers) { result.emplace_back(std::make_unique(std::move(signer))); } diff --git a/src/qt/trafficgraphwidget.cpp b/src/qt/trafficgraphwidget.cpp index 1c204448d5cd..a20746dabb17 100644 --- a/src/qt/trafficgraphwidget.cpp +++ b/src/qt/trafficgraphwidget.cpp @@ -88,8 +88,8 @@ void TrafficGraphWidget::paintEvent(QPaintEvent *) painter.drawLine(XMARGIN, YMARGIN + h, width() - XMARGIN, YMARGIN + h); // decide what order of magnitude we are - int base = floor(log10(fMax)); - float val = pow(10.0f, base); + int base = std::floor(std::log10(fMax)); + float val = std::pow(10.0f, base); float val2 = val; const QString units = tr("kB/s"); diff --git a/src/rpc/server.cpp b/src/rpc/server.cpp index ab8a4da314a0..70e0c411c981 100644 --- a/src/rpc/server.cpp +++ b/src/rpc/server.cpp @@ -85,6 +85,7 @@ std::string CRPCTable::help(const std::string& strCommand, const JSONRPCRequest& std::string category; std::set setDone; std::vector > vCommands; + vCommands.reserve(mapCommands.size()); for (const auto& entry : mapCommands) vCommands.emplace_back(entry.second.front()->category + entry.first, entry.second.front()); @@ -540,6 +541,7 @@ static bool ExecuteCommand(const CRPCCommand& command, const JSONRPCRequest& req std::vector CRPCTable::listCommands() const { std::vector commandList; + commandList.reserve(mapCommands.size()); for (const auto& i : mapCommands) commandList.emplace_back(i.first); return commandList; } diff --git a/src/rpc/util.cpp b/src/rpc/util.cpp index 02be3a9b80a4..a130c2621e14 100644 --- a/src/rpc/util.cpp +++ b/src/rpc/util.cpp @@ -593,6 +593,7 @@ bool RPCHelpMan::IsValidNumArgs(size_t num_args) const std::vector RPCHelpMan::GetArgNames() const { std::vector ret; + ret.reserve(m_args.size()); for (const auto& arg : m_args) { ret.emplace_back(arg.m_names); } @@ -671,12 +672,12 @@ UniValue RPCHelpMan::GetArgMap() const std::string RPCArg::GetFirstName() const { - return m_names.substr(0, m_names.find("|")); + return m_names.substr(0, m_names.find('|')); } std::string RPCArg::GetName() const { - CHECK_NONFATAL(std::string::npos == m_names.find("|")); + CHECK_NONFATAL(std::string::npos == m_names.find('|')); return m_names; } diff --git a/src/test/allocator_tests.cpp b/src/test/allocator_tests.cpp index 0b3b60991b7f..53978764e1f0 100644 --- a/src/test/allocator_tests.cpp +++ b/src/test/allocator_tests.cpp @@ -77,6 +77,7 @@ BOOST_AUTO_TEST_CASE(arena_tests) b.walk(); #endif // Sweeping allocate all memory + addr.reserve(2048); for (int x=0; x<1024; ++x) addr.push_back(b.alloc(1024)); BOOST_CHECK(b.stats().free == 0); diff --git a/src/test/fuzz/tx_pool.cpp b/src/test/fuzz/tx_pool.cpp index 8799b1bad806..f6631e64b8f1 100644 --- a/src/test/fuzz/tx_pool.cpp +++ b/src/test/fuzz/tx_pool.cpp @@ -311,6 +311,7 @@ FUZZ_TARGET(tx_pool, .init = initialize_tx_pool) SetMempoolConstraints(*node.args, fuzzed_data_provider); std::vector txids; + txids.reserve(g_outpoints_coinbase_init_mature.size()); for (const auto& outpoint : g_outpoints_coinbase_init_mature) { txids.push_back(outpoint.hash); } diff --git a/src/test/getarg_tests.cpp b/src/test/getarg_tests.cpp index 4240a5583e77..a2fdd1e19c20 100644 --- a/src/test/getarg_tests.cpp +++ b/src/test/getarg_tests.cpp @@ -29,6 +29,7 @@ void ResetArgs(ArgsManager& local_args, const std::string& strArg) // Convert to char*: std::vector vecChar; + vecChar.reserve(vecArg.size()); for (const std::string& s : vecArg) vecChar.push_back(s.c_str()); diff --git a/src/test/policyestimator_tests.cpp b/src/test/policyestimator_tests.cpp index 76852d66f7e9..fb4aafc978c0 100644 --- a/src/test/policyestimator_tests.cpp +++ b/src/test/policyestimator_tests.cpp @@ -24,6 +24,7 @@ BOOST_AUTO_TEST_CASE(BlockPolicyEstimates) CAmount basefee(2000); CAmount deltaFee(100); std::vector feeV; + feeV.reserve(10); // Populate vectors of increasing fees for (int j = 0; j < 10; j++) { diff --git a/src/test/scheduler_tests.cpp b/src/test/scheduler_tests.cpp index b0f4a33928a7..c2d990b8ff58 100644 --- a/src/test/scheduler_tests.cpp +++ b/src/test/scheduler_tests.cpp @@ -71,6 +71,7 @@ BOOST_AUTO_TEST_CASE(manythreads) // As soon as these are created they will start running and servicing the queue std::vector microThreads; + microThreads.reserve(10); for (int i = 0; i < 5; i++) microThreads.emplace_back(std::bind(&CScheduler::serviceQueue, µTasks)); @@ -136,6 +137,7 @@ BOOST_AUTO_TEST_CASE(singlethreadedscheduler_ordered) // the extra threads should effectively be doing nothing // if they don't we'll get out of order behaviour std::vector threads; + threads.reserve(5); for (int i = 0; i < 5; ++i) { threads.emplace_back([&] { scheduler.serviceQueue(); }); } diff --git a/src/test/script_p2sh_tests.cpp b/src/test/script_p2sh_tests.cpp index c424f255e8a7..fe5dbe61a3d6 100644 --- a/src/test/script_p2sh_tests.cpp +++ b/src/test/script_p2sh_tests.cpp @@ -154,6 +154,7 @@ BOOST_AUTO_TEST_CASE(set) FillableSigningProvider keystore; CKey key[4]; std::vector keys; + keys.reserve(4); for (int i = 0; i < 4; i++) { key[i].MakeNewKey(true); @@ -268,12 +269,13 @@ BOOST_AUTO_TEST_CASE(AreInputsStandard) CCoinsViewCache coins(&coinsDummy); FillableSigningProvider keystore; CKey key[6]; - std::vector keys; for (int i = 0; i < 6; i++) { key[i].MakeNewKey(true); BOOST_CHECK(keystore.AddKey(key[i])); } + std::vector keys; + keys.reserve(3); for (int i = 0; i < 3; i++) keys.push_back(key[i].GetPubKey()); diff --git a/src/test/util/net.cpp b/src/test/util/net.cpp index 963abca9631f..ad8ddad8a259 100644 --- a/src/test/util/net.cpp +++ b/src/test/util/net.cpp @@ -116,6 +116,7 @@ CNode* ConnmanTestMsg::ConnectNodePublic(PeerManager& peerman, const char* pszDe std::vector GetRandomNodeEvictionCandidates(int n_candidates, FastRandomContext& random_context) { std::vector candidates; + candidates.reserve(n_candidates); for (int id = 0; id < n_candidates; ++id) { candidates.push_back({ /*id=*/id, diff --git a/src/test/util_threadnames_tests.cpp b/src/test/util_threadnames_tests.cpp index 53987ae856fa..174052d5fa66 100644 --- a/src/test/util_threadnames_tests.cpp +++ b/src/test/util_threadnames_tests.cpp @@ -34,6 +34,7 @@ std::set RenameEnMasse(int num_threads) names.insert(util::ThreadGetInternalName()); }; + threads.reserve(num_threads); for (int i = 0; i < num_threads; ++i) { threads.emplace_back(RenameThisThread, i); } diff --git a/src/test/validation_block_tests.cpp b/src/test/validation_block_tests.cpp index 1525d645c196..ce5ef6c993d8 100644 --- a/src/test/validation_block_tests.cpp +++ b/src/test/validation_block_tests.cpp @@ -173,6 +173,7 @@ BOOST_AUTO_TEST_CASE(processnewblock_signals_ordering) // this will create parallelism and randomness inside validation - the ValidationInterface // will subscribe to events generated during block validation and assert on ordering invariance std::vector threads; + threads.reserve(10); for (int i = 0; i < 10; i++) { threads.emplace_back([&]() { bool ignored; diff --git a/src/util/bip32.cpp b/src/util/bip32.cpp index 441c4f460d75..934ca7768267 100644 --- a/src/util/bip32.cpp +++ b/src/util/bip32.cpp @@ -25,7 +25,7 @@ bool ParseHDKeypath(const std::string& keypath_str, std::vector& keypa } // Finds whether it is hardened uint32_t path = 0; - size_t pos = item.find("'"); + size_t pos = item.find('\''); if (pos != std::string::npos) { // The hardened tick can only be in the last index of the string if (pos != item.size() - 1) { diff --git a/src/wallet/bdb.cpp b/src/wallet/bdb.cpp index 6fb3dafde76f..d4eb23e1963b 100644 --- a/src/wallet/bdb.cpp +++ b/src/wallet/bdb.cpp @@ -443,6 +443,7 @@ void BerkeleyEnvironment::ReloadDbEnv() }); std::vector filenames; + filenames.reserve(m_databases.size()); for (const auto& it : m_databases) { filenames.push_back(it.first); } diff --git a/src/wallet/rpc/backup.cpp b/src/wallet/rpc/backup.cpp index c3892170501f..ac0589ce2de9 100644 --- a/src/wallet/rpc/backup.cpp +++ b/src/wallet/rpc/backup.cpp @@ -994,6 +994,7 @@ RPCHelpMan dumpwallet() // sort time/key pairs std::vector > vKeyBirth; + vKeyBirth.reserve(mapKeyBirth.size()); for (const auto& entry : mapKeyBirth) { vKeyBirth.emplace_back(entry.second, entry.first); } diff --git a/src/wallet/scriptpubkeyman.cpp b/src/wallet/scriptpubkeyman.cpp index d25077328019..fcd9bbd5c6aa 100644 --- a/src/wallet/scriptpubkeyman.cpp +++ b/src/wallet/scriptpubkeyman.cpp @@ -1687,6 +1687,7 @@ std::vector GetAffectedKeys(const CScript& spk, const SigningProvider& p FlatSigningProvider out; InferDescriptor(spk, provider)->Expand(0, DUMMY_SIGNING_PROVIDER, dummy, out); std::vector ret; + ret.reserve(out.pubkeys.size()); for (const auto& entry : out.pubkeys) { ret.push_back(entry.first); } diff --git a/src/zmq/zmqpublishnotifier.cpp b/src/zmq/zmqpublishnotifier.cpp index 054dc6e55857..6a1bd06f5b99 100644 --- a/src/zmq/zmqpublishnotifier.cpp +++ b/src/zmq/zmqpublishnotifier.cpp @@ -116,7 +116,7 @@ static bool IsZMQAddressIPV6(const std::string &zmq_address) { const std::string tcp_prefix = "tcp://"; const size_t tcp_index = zmq_address.rfind(tcp_prefix); - const size_t colon_index = zmq_address.rfind(":"); + const size_t colon_index = zmq_address.rfind(':'); if (tcp_index == 0 && colon_index != std::string::npos) { const std::string ip = zmq_address.substr(tcp_prefix.length(), colon_index - tcp_prefix.length()); const std::optional addr{LookupHost(ip, false)}; From 405782a6a3555d10152e140fce061b726470761e Mon Sep 17 00:00:00 2001 From: fanquake Date: Tue, 28 Mar 2023 11:49:36 +0100 Subject: [PATCH 2/5] Merge bitcoin/bitcoin#27344: fuzz: Remove legacy int parse fuzz tests faf8dc496e761a15956f8226d727f4bbab8dff82 fuzz: Remove legacy int parse fuzz tests (MarcoFalke) Pull request description: The fuzz tests checked that the result of the new function was equal to the legacy function. (Side note: The checks were incomplete, as evident by the follow-up fix in commit b5c9bb5cb9f4a8db57b33ef7399310c7d6de5822). Given that they haven't found any issues in years (beside missing the above issue, that they couldn't catch), it seems time to remove them. They may come in handy in the rare case that someone would want to modify `LocaleIndependentAtoi()` or `Parse*Int*()`, however that seems unlikely. Also, appropriate checks can be added then. ACKs for top commit: fanquake: ACK faf8dc496e761a15956f8226d727f4bbab8dff82 dergoegge: ACK faf8dc496e761a15956f8226d727f4bbab8dff82 Tree-SHA512: 4ec88b9fa8ba49a923b0604016f0f471b3c9b9e0ba6c5c3dc4e20503c6994789921e7221d9ec467a2a37a73f21a70ba51ba3370ed5ad311dee989e218290b29a --- ci/dash/lint-tidy.sh | 1 + src/test/fuzz/string.cpp | 158 +--------------------------- test/lint/lint-locale-dependence.py | 1 - 3 files changed, 5 insertions(+), 155 deletions(-) diff --git a/ci/dash/lint-tidy.sh b/ci/dash/lint-tidy.sh index 7b5aeab03258..cae9acab09ef 100755 --- a/ci/dash/lint-tidy.sh +++ b/ci/dash/lint-tidy.sh @@ -96,6 +96,7 @@ iwyu_tool.py \ "src/random.cpp" \ "src/rpc/fees.cpp" \ "src/rpc/signmessage.cpp" \ + "src/test/fuzz/string.cpp" \ "src/test/fuzz/txorphan.cpp" \ "src/util/bip32.cpp" \ "src/util/bytevectorhash.cpp" \ diff --git a/src/test/fuzz/string.cpp b/src/test/fuzz/string.cpp index 33490b0c1f28..d9150b5a6329 100644 --- a/src/test/fuzz/string.cpp +++ b/src/test/fuzz/string.cpp @@ -5,8 +5,6 @@ #include #include #include -#include -#include #include #include #include @@ -26,107 +24,16 @@ #include #include #include -#include +#include #include #include +#include +#include #include #include -namespace { -bool LegacyParsePrechecks(const std::string& str) -{ - if (str.empty()) // No empty string allowed - return false; - if (str.size() >= 1 && (IsSpace(str[0]) || IsSpace(str[str.size() - 1]))) // No padding allowed - return false; - if (!ContainsNoNUL(str)) // No embedded NUL characters allowed - return false; - return true; -} - -bool LegacyParseInt32(const std::string& str, int32_t* out) -{ - if (!LegacyParsePrechecks(str)) - return false; - char* endp = nullptr; - errno = 0; // strtol will not set errno if valid - long int n = strtol(str.c_str(), &endp, 10); - if (out) *out = (int32_t)n; - // Note that strtol returns a *long int*, so even if strtol doesn't report an over/underflow - // we still have to check that the returned value is within the range of an *int32_t*. On 64-bit - // platforms the size of these types may be different. - return endp && *endp == 0 && !errno && - n >= std::numeric_limits::min() && - n <= std::numeric_limits::max(); -} - -bool LegacyParseInt64(const std::string& str, int64_t* out) -{ - if (!LegacyParsePrechecks(str)) - return false; - char* endp = nullptr; - errno = 0; // strtoll will not set errno if valid - long long int n = strtoll(str.c_str(), &endp, 10); - if (out) *out = (int64_t)n; - // Note that strtoll returns a *long long int*, so even if strtol doesn't report an over/underflow - // we still have to check that the returned value is within the range of an *int64_t*. - return endp && *endp == 0 && !errno && - n >= std::numeric_limits::min() && - n <= std::numeric_limits::max(); -} - -bool LegacyParseUInt32(const std::string& str, uint32_t* out) -{ - if (!LegacyParsePrechecks(str)) - return false; - if (str.size() >= 1 && str[0] == '-') // Reject negative values, unfortunately strtoul accepts these by default if they fit in the range - return false; - char* endp = nullptr; - errno = 0; // strtoul will not set errno if valid - unsigned long int n = strtoul(str.c_str(), &endp, 10); - if (out) *out = (uint32_t)n; - // Note that strtoul returns a *unsigned long int*, so even if it doesn't report an over/underflow - // we still have to check that the returned value is within the range of an *uint32_t*. On 64-bit - // platforms the size of these types may be different. - return endp && *endp == 0 && !errno && - n <= std::numeric_limits::max(); -} - -bool LegacyParseUInt8(const std::string& str, uint8_t* out) -{ - uint32_t u32; - if (!LegacyParseUInt32(str, &u32) || u32 > std::numeric_limits::max()) { - return false; - } - if (out != nullptr) { - *out = static_cast(u32); - } - return true; -} - -bool LegacyParseUInt64(const std::string& str, uint64_t* out) -{ - if (!LegacyParsePrechecks(str)) - return false; - if (str.size() >= 1 && str[0] == '-') // Reject negative values, unfortunately strtoull accepts these by default if they fit in the range - return false; - char* endp = nullptr; - errno = 0; // strtoull will not set errno if valid - unsigned long long int n = strtoull(str.c_str(), &endp, 10); - if (out) *out = (uint64_t)n; - // Note that strtoull returns a *unsigned long long int*, so even if it doesn't report an over/underflow - // we still have to check that the returned value is within the range of an *uint64_t*. - return endp && *endp == 0 && !errno && - n <= std::numeric_limits::max(); -} - -// For backwards compatibility checking. -int64_t atoi64_legacy(const std::string& str) -{ - return strtoll(str.c_str(), nullptr, 10); -} -}; // namespace +enum class FeeEstimateMode; FUZZ_TARGET(string) { @@ -229,61 +136,4 @@ FUZZ_TARGET(string) const bilingual_str bs2{random_string_2, random_string_1}; (void)(bs1 + bs2); } - { - int32_t i32; - int64_t i64; - uint32_t u32; - uint64_t u64; - uint8_t u8; - const bool ok_i32 = ParseInt32(random_string_1, &i32); - const bool ok_i64 = ParseInt64(random_string_1, &i64); - const bool ok_u32 = ParseUInt32(random_string_1, &u32); - const bool ok_u64 = ParseUInt64(random_string_1, &u64); - const bool ok_u8 = ParseUInt8(random_string_1, &u8); - - int32_t i32_legacy; - int64_t i64_legacy; - uint32_t u32_legacy; - uint64_t u64_legacy; - uint8_t u8_legacy; - const bool ok_i32_legacy = LegacyParseInt32(random_string_1, &i32_legacy); - const bool ok_i64_legacy = LegacyParseInt64(random_string_1, &i64_legacy); - const bool ok_u32_legacy = LegacyParseUInt32(random_string_1, &u32_legacy); - const bool ok_u64_legacy = LegacyParseUInt64(random_string_1, &u64_legacy); - const bool ok_u8_legacy = LegacyParseUInt8(random_string_1, &u8_legacy); - - assert(ok_i32 == ok_i32_legacy); - assert(ok_i64 == ok_i64_legacy); - assert(ok_u32 == ok_u32_legacy); - assert(ok_u64 == ok_u64_legacy); - assert(ok_u8 == ok_u8_legacy); - - if (ok_i32) { - assert(i32 == i32_legacy); - } - if (ok_i64) { - assert(i64 == i64_legacy); - } - if (ok_u32) { - assert(u32 == u32_legacy); - } - if (ok_u64) { - assert(u64 == u64_legacy); - } - if (ok_u8) { - assert(u8 == u8_legacy); - } - } - - { - const int locale_independent_atoi_result = LocaleIndependentAtoi(random_string_1); - const int64_t atoi64_result = atoi64_legacy(random_string_1); - assert(locale_independent_atoi_result == std::clamp(atoi64_result, std::numeric_limits::min(), std::numeric_limits::max())); - } - - { - const int64_t atoi64_result = atoi64_legacy(random_string_1); - const int64_t locale_independent_atoi_result = LocaleIndependentAtoi(random_string_1); - assert(atoi64_result == locale_independent_atoi_result); - } } diff --git a/test/lint/lint-locale-dependence.py b/test/lint/lint-locale-dependence.py index 32cef3cb1248..0ee4c4566b0a 100755 --- a/test/lint/lint-locale-dependence.py +++ b/test/lint/lint-locale-dependence.py @@ -48,7 +48,6 @@ "src/bitcoin-tx.cpp.*stoul", "src/dbwrapper.cpp:.*vsnprintf", "src/test/fuzz/locale.cpp", - "src/test/fuzz/string.cpp", "src/test/util_tests.cpp", "src/util/strencodings.cpp:.*strtoll", "src/util/system.cpp:.*fprintf", From 103966701dcfb0e33505b6789510c234a7387f86 Mon Sep 17 00:00:00 2001 From: fanquake Date: Sat, 15 Apr 2023 12:56:42 +0100 Subject: [PATCH 3/5] Merge bitcoin/bitcoin#27465: doc: fix typo in developer-notes.md f24f4fa3f114b1bd5b7e7ff9ebb0ef73bfc81eda Update developer-notes.md (Riahiamirreza) Pull request description: ACKs for top commit: fanquake: ACK f24f4fa3f114b1bd5b7e7ff9ebb0ef73bfc81eda Tree-SHA512: 10301170dff6f2f7b47a229ba99f4a4f4953c361be24996b6dc70343ad118879cd90ebb54d78cd31c852f577fb17f9726582fdd02ed5b6fd7b71566942e8b408 --- doc/developer-notes.md | 4 ++-- 1 file changed, 2 insertions(+), 2 deletions(-) diff --git a/doc/developer-notes.md b/doc/developer-notes.md index c343f3a7e007..1d851191625e 100644 --- a/doc/developer-notes.md +++ b/doc/developer-notes.md @@ -112,8 +112,8 @@ code. - Align pointers and references to the left i.e. use `type& var` and not `type &var`. - Use a named cast or functional cast, not a C-Style cast. When casting between integer types, use functional casts such as `int(x)` or `int{x}` - instead of `(int) x`. When casting between more complex types, use static_cast. - Use reinterpret_cast and const_cast as appropriate. + instead of `(int) x`. When casting between more complex types, use `static_cast`. + Use `reinterpret_cast` and `const_cast` as appropriate. - Prefer [`list initialization ({})`](https://isocpp.github.io/CppCoreGuidelines/CppCoreGuidelines#Res-list) where possible. For example `int x{0};` instead of `int x = 0;` or `int x(0);` From 45bd83f0712e0dda5d481b2633a95218909e1909 Mon Sep 17 00:00:00 2001 From: Andrew Chow Date: Wed, 15 Mar 2023 19:33:21 -0400 Subject: [PATCH 4/5] Merge bitcoin/bitcoin#26207: rest: add verbose and mempool_sequence query params for mempool/contents 1ff5d61dfdaf8987e5619162662e4c760af76a43 doc: add mempool/contents rest verbose and mempool_sequence args (Andrew Toth) 52a31dccc92366efb36db3b94920bdf8b05b264c tests: mempool/contents verbose and mempool_sequence query params tests (Andrew Toth) a518fff0f2e479064dd4cff6c29fb54c72c1407b rest: add verbose and mempool_sequence query params for mempool/contents (Andrew Toth) Pull request description: The verbose mempool json response can get very large. This adds an option to return the non-verbose response of just the txids. It is identical to the rpc response so the diff here is minimal. This also adds the mempool_sequence parameter for rpc consistency. Verbose defaults to true to remain backwards compatible. It uses query parameters to be compatible with the efforts in https://github.com/bitcoin/bitcoin/issues/25752. ACKs for top commit: achow101: ACK 1ff5d61dfdaf8987e5619162662e4c760af76a43 stickies-v: re-ACK [1ff5d61](https://github.com/bitcoin/bitcoin/pull/26207/commits/1ff5d61dfdaf8987e5619162662e4c760af76a43) pablomartin4btc: tested ACK 1ff5d61dfdaf8987e5619162662e4c760af76a43. Tree-SHA512: 1bf08a7ffde2e7db14dc746e421feedf17d84c4b3f1141e79e36feb6014811dfde80e1d8dbc476c15ff705de2d3c967b3081dcd80536d76b7edf888f1a92e9d1 --- doc/REST-interface.md | 8 ++++++-- src/rest.cpp | 15 ++++++++++++++- test/functional/interface_rest.py | 28 ++++++++++++++++++++++++++++ 3 files changed, 48 insertions(+), 3 deletions(-) diff --git a/doc/REST-interface.md b/doc/REST-interface.md index 4b95ec1e3210..7aacecc9b859 100644 --- a/doc/REST-interface.md +++ b/doc/REST-interface.md @@ -125,11 +125,15 @@ Returns various information about the transaction mempool. Only supports JSON as output format. Refer to the `getmempoolinfo` RPC help for details. -`GET /rest/mempool/contents.json` +`GET /rest/mempool/contents.json?verbose=&mempool_sequence=` Returns the transactions in the mempool. Only supports JSON as output format. -Refer to the `getrawmempool` RPC help for details. +Refer to the `getrawmempool` RPC help for details. Defaults to setting +`verbose=true` and `mempool_sequence=false`. + +*Query parameters for `verbose` and `mempool_sequence` available in 23.1.5 and up.* + Risks ------------- diff --git a/src/rest.cpp b/src/rest.cpp index 70c61e40fe25..4b95a67d0ba8 100644 --- a/src/rest.cpp +++ b/src/rest.cpp @@ -645,7 +645,20 @@ static bool rest_mempool(const CoreContext& context, HTTPRequest* req, const std std::string str_json; if (param == "contents") { - str_json = MempoolToJSON(*mempool, llmq_ctx->isman.get(), true).write() + "\n"; + const std::string raw_verbose{req->GetQueryParameter("verbose").value_or("true")}; + if (raw_verbose != "true" && raw_verbose != "false") { + return RESTERR(req, HTTP_BAD_REQUEST, "The \"verbose\" query parameter must be either \"true\" or \"false\"."); + } + const std::string raw_mempool_sequence{req->GetQueryParameter("mempool_sequence").value_or("false")}; + if (raw_mempool_sequence != "true" && raw_mempool_sequence != "false") { + return RESTERR(req, HTTP_BAD_REQUEST, "The \"mempool_sequence\" query parameter must be either \"true\" or \"false\"."); + } + const bool verbose{raw_verbose == "true"}; + const bool mempool_sequence{raw_mempool_sequence == "true"}; + if (verbose && mempool_sequence) { + return RESTERR(req, HTTP_BAD_REQUEST, "Verbose results cannot contain mempool sequence values. (hint: set \"verbose=false\")"); + } + str_json = MempoolToJSON(*mempool, llmq_ctx->isman.get(), verbose, mempool_sequence).write() + "\n"; } else { str_json = MempoolInfoToJSON(*mempool, *llmq_ctx->isman).write() + "\n"; } diff --git a/test/functional/interface_rest.py b/test/functional/interface_rest.py index a2186bf57ea1..ea3f7b50743f 100755 --- a/test/functional/interface_rest.py +++ b/test/functional/interface_rest.py @@ -349,6 +349,34 @@ def run_test(self): assert_equal(json_obj[tx]['spentby'], txs[i + 1:i + 2]) assert_equal(json_obj[tx]['depends'], txs[i - 1:i]) + # Check the mempool response for explicit parameters + json_obj = self.test_rest_request("/mempool/contents", query_params={"verbose": "true", "mempool_sequence": "false"}) + assert_equal(json_obj, raw_mempool_verbose) + + # Check the mempool response for not verbose + json_obj = self.test_rest_request("/mempool/contents", query_params={"verbose": "false"}) + raw_mempool = self.nodes[0].getrawmempool(verbose=False) + + assert_equal(json_obj, raw_mempool) + + # Check the mempool response for sequence + json_obj = self.test_rest_request("/mempool/contents", query_params={"verbose": "false", "mempool_sequence": "true"}) + raw_mempool = self.nodes[0].getrawmempool(verbose=False, mempool_sequence=True) + + assert_equal(json_obj, raw_mempool) + + # Check for error response if verbose=true and mempool_sequence=true + resp = self.test_rest_request("/mempool/contents", ret_type=RetType.OBJ, status=400, query_params={"verbose": "true", "mempool_sequence": "true"}) + assert_equal(resp.read().decode('utf-8').strip(), 'Verbose results cannot contain mempool sequence values. (hint: set "verbose=false")') + + # Check for error response if verbose is not "true" or "false" + resp = self.test_rest_request("/mempool/contents", ret_type=RetType.OBJ, status=400, query_params={"verbose": "TRUE"}) + assert_equal(resp.read().decode('utf-8').strip(), 'The "verbose" query parameter must be either "true" or "false".') + + # Check for error response if mempool_sequence is not "true" or "false" + resp = self.test_rest_request("/mempool/contents", ret_type=RetType.OBJ, status=400, query_params={"verbose": "false", "mempool_sequence": "TRUE"}) + assert_equal(resp.read().decode('utf-8').strip(), 'The "mempool_sequence" query parameter must be either "true" or "false".') + # Now mine the transactions newblockhash = self.generate(self.nodes[1], 1) From 48179141b1a4f67321251089fd443e3cd9874a01 Mon Sep 17 00:00:00 2001 From: fanquake Date: Mon, 17 Apr 2023 14:59:04 +0100 Subject: [PATCH 5/5] Merge bitcoin/bitcoin#27468: bugfix: rest: avoid segfault for invalid URI 11422cc5720c8d73a87600de8fe8abb156db80dc bugfix: rest: avoid segfault for invalid URI (pablomartin4btc) Pull request description: Minimal fix to get it promptly into 25.0 release (suggested by [stickies-v](https://github.com/bitcoin/bitcoin/pull/27253#pullrequestreview-1385130381) and supported by [vasild](https://github.com/bitcoin/bitcoin/pull/27253#pullrequestreview-1385842606) ) Please check #27253 for reviewers comments and acks regarding this PR and read the commit comment message body for more details about the fix. ACKs for top commit: achow101: ACK 11422cc5720c8d73a87600de8fe8abb156db80dc stickies-v: re-ACK 11422cc Tree-SHA512: 5af6b53fb266a12b463f960910556d5e97bc88b3c2a4f437ffa343886b38749e1eb058cf7bc64d62e82e1acf6232a186bddacd8f3b4500c87bf9e550a0153386 --- src/httpserver.cpp | 3 +++ src/rest.cpp | 26 ++++++++++++++++++++++---- src/test/httpserver_tests.cpp | 4 ++++ test/functional/interface_rest.py | 4 ++++ 4 files changed, 33 insertions(+), 4 deletions(-) diff --git a/src/httpserver.cpp b/src/httpserver.cpp index fc0132133ff3..d325b19c47ad 100644 --- a/src/httpserver.cpp +++ b/src/httpserver.cpp @@ -759,6 +759,9 @@ std::optional HTTPRequest::GetQueryParameter(const std::string& key std::optional GetQueryParameterFromUri(const char* uri, const std::string& key) { evhttp_uri* uri_parsed{evhttp_uri_parse(uri)}; + if (!uri_parsed) { + throw std::runtime_error("URI parsing failed, it likely contained RFC 3986 invalid characters"); + } const char* query{evhttp_uri_get_query(uri_parsed)}; std::optional result; diff --git a/src/rest.cpp b/src/rest.cpp index 4b95a67d0ba8..c12271e0907e 100644 --- a/src/rest.cpp +++ b/src/rest.cpp @@ -224,7 +224,11 @@ static bool rest_headers(const CoreContext& context, } else if (path.size() == 1) { // new path with query parameter: /rest/headers/?count= hashStr = path[0]; - raw_count = req->GetQueryParameter("count").value_or("5"); + try { + raw_count = req->GetQueryParameter("count").value_or("5"); + } catch (const std::runtime_error& e) { + return RESTERR(req, HTTP_BAD_REQUEST, e.what()); + } } else { return RESTERR(req, HTTP_BAD_REQUEST, "Invalid URI format. Expected /rest/headers/.?count="); } @@ -403,7 +407,11 @@ static bool rest_filter_header(const CoreContext& context, HTTPRequest* req, con } else if (uri_parts.size() == 2) { // new path with query parameter: /rest/blockfilterheaders//?count= raw_blockhash = uri_parts[1]; - raw_count = req->GetQueryParameter("count").value_or("5"); + try { + raw_count = req->GetQueryParameter("count").value_or("5"); + } catch (const std::runtime_error& e) { + return RESTERR(req, HTTP_BAD_REQUEST, e.what()); + } } else { return RESTERR(req, HTTP_BAD_REQUEST, "Invalid URI format. Expected /rest/blockfilterheaders//.?count="); } @@ -645,11 +653,21 @@ static bool rest_mempool(const CoreContext& context, HTTPRequest* req, const std std::string str_json; if (param == "contents") { - const std::string raw_verbose{req->GetQueryParameter("verbose").value_or("true")}; + std::string raw_verbose; + try { + raw_verbose = req->GetQueryParameter("verbose").value_or("true"); + } catch (const std::runtime_error& e) { + return RESTERR(req, HTTP_BAD_REQUEST, e.what()); + } if (raw_verbose != "true" && raw_verbose != "false") { return RESTERR(req, HTTP_BAD_REQUEST, "The \"verbose\" query parameter must be either \"true\" or \"false\"."); } - const std::string raw_mempool_sequence{req->GetQueryParameter("mempool_sequence").value_or("false")}; + std::string raw_mempool_sequence; + try { + raw_mempool_sequence = req->GetQueryParameter("mempool_sequence").value_or("false"); + } catch (const std::runtime_error& e) { + return RESTERR(req, HTTP_BAD_REQUEST, e.what()); + } if (raw_mempool_sequence != "true" && raw_mempool_sequence != "false") { return RESTERR(req, HTTP_BAD_REQUEST, "The \"mempool_sequence\" query parameter must be either \"true\" or \"false\"."); } diff --git a/src/test/httpserver_tests.cpp b/src/test/httpserver_tests.cpp index ee59ec6967fc..c95a777e80c1 100644 --- a/src/test/httpserver_tests.cpp +++ b/src/test/httpserver_tests.cpp @@ -34,5 +34,9 @@ BOOST_AUTO_TEST_CASE(test_query_parameters) // Invalid query string syntax is the same as not having parameters uri = "/rest/endpoint/someresource.json&p1=v1&p2=v2"; BOOST_CHECK(!GetQueryParameterFromUri(uri.c_str(), "p1").has_value()); + + // URI with invalid characters (%) raises a runtime error regardless of which query parameter is queried + uri = "/rest/endpoint/someresource.json&p1=v1&p2=v2%"; + BOOST_CHECK_EXCEPTION(GetQueryParameterFromUri(uri.c_str(), "p1"), std::runtime_error, HasReason("URI parsing failed, it likely contained RFC 3986 invalid characters")); } BOOST_AUTO_TEST_SUITE_END() diff --git a/test/functional/interface_rest.py b/test/functional/interface_rest.py index ea3f7b50743f..ce830bf8b977 100755 --- a/test/functional/interface_rest.py +++ b/test/functional/interface_rest.py @@ -278,6 +278,10 @@ def run_test(self): assert_equal(len(json_obj), 1) # ensure that there is one header in the json response assert_equal(json_obj[0]['hash'], bb_hash) # request/response hash should be the same + # Check invalid uri (% symbol at the end of the request) + resp = self.test_rest_request(f"/headers/{bb_hash}%", ret_type=RetType.OBJ, status=400) + assert_equal(resp.read().decode('utf-8').rstrip(), "URI parsing failed, it likely contained RFC 3986 invalid characters") + # Compare with normal RPC block response rpc_block_json = self.nodes[0].getblock(bb_hash) for key in ['hash', 'confirmations', 'height', 'version', 'merkleroot', 'time', 'nonce', 'bits', 'difficulty', 'chainwork', 'previousblockhash']: