From d7a20b3ee6882049858a958f2e32efa85978eb93 Mon Sep 17 00:00:00 2001 From: Samuel Dobson Date: Thu, 19 Aug 2021 10:03:18 +1200 Subject: [PATCH 1/3] Merge bitcoin/bitcoin#22217: refactor: Avoid wallet code writing node settings file 49ee2a0ad88e0e656234b769d806987784ff1e28 Avoid wallet code writing node settings file (Russell Yanofsky) Pull request description: Change wallet loading code to access settings through the Chain interface instead of writing settings.json directly. This is for running wallet and node in separate processes, since multiprocess code wouldn't easily work with different processes updating the same file. --- This PR is part of the [process separation project](https://github.com/bitcoin/bitcoin/projects/10). The commit was first part of larger PR #10102. ACKs for top commit: jamesob: ACK 49ee2a0ad88e0e656234b769d806987784ff1e28 ([`jamesob/ackr/22217.1.ryanofsky.refactor_avoid_wallet_co`](https://github.com/jamesob/bitcoin/tree/ackr/22217.1.ryanofsky.refactor_avoid_wallet_co)) ryanofsky: > ACK [49ee2a0](https://github.com/bitcoin/bitcoin/commit/49ee2a0ad88e0e656234b769d806987784ff1e28) ([`jamesob/ackr/22217.1.ryanofsky.refactor_avoid_wallet_co`](https://github.com/jamesob/bitcoin/tree/ackr/22217.1.ryanofsky.refactor_avoid_wallet_co)) Zero-1729: crACK 49ee2a0ad88e0e656234b769d806987784ff1e28 meshcollider: Code review ACK 49ee2a0ad88e0e656234b769d806987784ff1e28 Tree-SHA512: a81c63b87816f739e02e3992808f314294d6c7213babaafdaaf3c4650ebc97ee4f98f9a4684ce4ff87372df59989b8ad5929159c5686293a7cce04e97e2fabba --- src/interfaces/chain.h | 11 +++++++++-- src/node/interfaces.cpp | 12 ++++++++++-- src/util/system.h | 2 +- src/wallet/load.cpp | 17 ++++++++++------- 4 files changed, 30 insertions(+), 12 deletions(-) diff --git a/src/interfaces/chain.h b/src/interfaces/chain.h index 37f434591d..ea5fe712ab 100644 --- a/src/interfaces/chain.h +++ b/src/interfaces/chain.h @@ -285,11 +285,18 @@ public: //! Run function after given number of seconds. Cancel any previous calls with same name. virtual void rpcRunLater(const std::string& name, std::function fn, int64_t seconds) = 0; + //! Get settings value. + virtual util::SettingsValue getSetting(const std::string& arg) = 0; + + //! Get list of settings values. + virtual std::vector getSettingsList(const std::string& arg) = 0; + //! Return /settings.json setting value. virtual util::SettingsValue getRwSetting(const std::string& name) = 0; - //! Write a setting to /settings.json. - virtual bool updateRwSetting(const std::string& name, const util::SettingsValue& value) = 0; + //! Write a setting to /settings.json. Optionally just update the + //! setting in memory and do not write the file. + virtual bool updateRwSetting(const std::string& name, const util::SettingsValue& value, bool write=true) = 0; //! Synchronously send transactionAddedToMempool notifications about all //! current mempool transactions to the specified handler and return after diff --git a/src/node/interfaces.cpp b/src/node/interfaces.cpp index d29e95a3e4..d680ab8488 100644 --- a/src/node/interfaces.cpp +++ b/src/node/interfaces.cpp @@ -971,6 +971,14 @@ public: { RPCRunLater(name, std::move(fn), seconds); } + util::SettingsValue getSetting(const std::string& name) override + { + return gArgs.GetSetting(name); + } + std::vector getSettingsList(const std::string& name) override + { + return gArgs.GetSettingsList(name); + } util::SettingsValue getRwSetting(const std::string& name) override { util::SettingsValue result; @@ -981,7 +989,7 @@ public: }); return result; } - bool updateRwSetting(const std::string& name, const util::SettingsValue& value) override + bool updateRwSetting(const std::string& name, const util::SettingsValue& value, bool write) override { gArgs.LockSettings([&](util::Settings& settings) { if (value.isNull()) { @@ -990,7 +998,7 @@ public: settings.rw_settings[name] = value; } }); - return gArgs.WriteSettingsFile(); + return !write || gArgs.WriteSettingsFile(); } void requestMempoolTransactions(Notifications& notifications) override { diff --git a/src/util/system.h b/src/util/system.h index 591d50099f..85c46eaa50 100644 --- a/src/util/system.h +++ b/src/util/system.h @@ -225,6 +225,7 @@ protected: */ bool UseDefaultSection(const std::string& arg) const EXCLUSIVE_LOCKS_REQUIRED(cs_args); + public: /** * Get setting value. * @@ -239,7 +240,6 @@ protected: */ std::vector GetSettingsList(const std::string& arg) const; -public: ArgsManager(); ~ArgsManager(); diff --git a/src/wallet/load.cpp b/src/wallet/load.cpp index f5091b3794..bef1c57471 100644 --- a/src/wallet/load.cpp +++ b/src/wallet/load.cpp @@ -57,18 +57,20 @@ bool VerifyWallets(interfaces::Chain& chain) options.require_existing = true; options.verify = false; if (MakeWalletDatabase("", options, status, error_string)) { - gArgs.LockSettings([&](util::Settings& settings) { - util::SettingsValue wallets(util::SettingsValue::VARR); - wallets.push_back(""); // Default wallet name is "" - settings.rw_settings["wallet"] = wallets; - }); + util::SettingsValue wallets(util::SettingsValue::VARR); + wallets.push_back(""); // Default wallet name is "" + // Pass write=false because no need to write file and probably + // better not to. If unnamed wallet needs to be added next startup + // and the setting is empty, this code will just run again. + chain.updateRwSetting("wallet", wallets, /* write= */ false); } } // Keep track of each wallet absolute path to detect duplicates. std::set wallet_paths; - for (const auto& wallet_file : gArgs.GetArgs("-wallet")) { + for (const auto& wallet : chain.getSettingsList("wallet")) { + const auto& wallet_file = wallet.get_str(); const fs::path path = fsbridge::AbsPathJoin(GetWalletDir(), fs::PathFromString(wallet_file)); if (!wallet_paths.insert(path).second) { @@ -98,7 +100,8 @@ bool LoadWallets(interfaces::Chain& chain, interfaces::CoinJoin::Loader& coinjoi { try { std::set wallet_paths; - for (const std::string& name : gArgs.GetArgs("-wallet")) { + for (const auto& wallet : chain.getSettingsList("wallet")) { + const auto& name = wallet.get_str(); if (!wallet_paths.insert(fs::PathFromString(name)).second) { continue; } From 8b3a4867027f2d637107972c8736d0944f2775a1 Mon Sep 17 00:00:00 2001 From: "W. J. van der Laan" Date: Fri, 24 Sep 2021 17:10:06 +0200 Subject: [PATCH 2/3] Merge bitcoin/bitcoin#23025: bench: update nanobench add `-min_time` e148a5233292d156cda76cb20afb6641fc20f25e bench: fixed ubsan implicit conversion (Martin Ankerl) da4e2f1da0388d424659fa8c853fcaf37b4b5959 bench: various args improvements (Jon Atack) d312fd94a1083cdbf071f2888aab43c62d358151 bench: clean up includes (Jon Atack) 1f10f1663e53474038b9111c4264a250cffe7501 bench: add usage description and documentation (Martin Ankerl) d3c6f8bfa12f78635752878b28e66cec0c85d4a9 bench: introduce -min_time argument (Martin Ankerl) 9fef8329322277d9c14c8df1867cb3c61477c431 bench: make EvictionProtection.* work with any number of iterations (Martin Ankerl) 153e6860e84df0a3d52e5a3b2fe9c37b5e0b029a bench: change AddrManGood to AddrManAddThenGood (Martin Ankerl) 468b232f71562280aae16876bc257ec24f5fcccb bench: remove unnecessary & incorrect multiplication in MuHashDiv (Martin Ankerl) eed99cf272426e5957bee35dc8e7d0798aec8ec0 bench: update nanobench from 4.3.4 to 4.3.6 (Martin Ankerl) Pull request description: This PR updates the nanobench with the latest release from upstream, v4.3.6. It fixes the missing performance counters. Due to discussions on #22999 I have done some work that should make the benchmark results more reliable. It introduces a new flag `-min_time` that allows to run a benchmark for much longer then the default. When results are unreliable, choosing a large timeframe here should usually get repeatable results even when frequency scaling cannot be disabled. The default is now 10ms. For this to work I have changed the `AddrManGood` and `EvictionProtection` benchmarks so they work with any number of iterations. Also, this adds more usage documentation to `bench_bitcoin -h` and I've cherry-picked two changes from #22999 authored by Jon Atack ACKs for top commit: jonatack: re-ACK e148a5233292d156cda76cb20afb6641fc20f25e laanwj: Code review ACK e148a5233292d156cda76cb20afb6641fc20f25e Tree-SHA512: 2da6de19a5c85ac234b190025e195c727546166dbb75e3f9267e667a73677ba1e29b7765877418a42b1407b65df901e0130763936525e6f1450f18f08837c40c --- src/bench/bench.cpp | 15 +++++++-- src/bench/bench.h | 3 +- src/bench/bench_bitcoin.cpp | 64 ++++++++++++++++++++++++++++++++++--- src/bench/crypto_hash.cpp | 8 ++--- src/bench/peer_eviction.cpp | 10 +++--- src/bench/rollingbloom.cpp | 16 +++++----- 6 files changed, 88 insertions(+), 28 deletions(-) diff --git a/src/bench/bench.cpp b/src/bench/bench.cpp index 02ae5de7fd..d6ed76a1f4 100644 --- a/src/bench/bench.cpp +++ b/src/bench/bench.cpp @@ -4,15 +4,20 @@ #include -#include #include #include -#include #include +#include #include +#include #include +#include #include +#include +#include + +using namespace std::chrono_literals; const std::function G_TEST_LOG_FUN{}; @@ -66,6 +71,12 @@ void benchmark::BenchRunner::RunAll(const Args& args) Bench bench; bench.name(p.first); + if (args.min_time > 0ms) { + // convert to nanos before dividing to reduce rounding errors + std::chrono::nanoseconds min_time_ns = args.min_time; + bench.minEpochTime(min_time_ns / bench.epochs()); + } + if (args.asymptote.empty()) { p.second(bench); } else { diff --git a/src/bench/bench.h b/src/bench/bench.h index 6804273d52..4a838d1fe7 100644 --- a/src/bench/bench.h +++ b/src/bench/bench.h @@ -42,11 +42,12 @@ using ankerl::nanobench::Bench; typedef std::function BenchFunction; struct Args { - std::string regex_filter; bool is_list_only; + std::chrono::milliseconds min_time; std::vector asymptote; fs::path output_csv; fs::path output_json; + std::string regex_filter; }; class BenchRunner diff --git a/src/bench/bench_bitcoin.cpp b/src/bench/bench_bitcoin.cpp index 07bc619bd6..b12707dbf1 100644 --- a/src/bench/bench_bitcoin.cpp +++ b/src/bench/bench_bitcoin.cpp @@ -4,6 +4,7 @@ #include +#include #include #include #include @@ -11,16 +12,23 @@ #include #include +#include +#include +#include +#include +#include static const char* DEFAULT_BENCH_FILTER = ".*"; +static constexpr int64_t DEFAULT_MIN_TIME_MS{10}; static void SetupBenchArgs(ArgsManager& argsman) { SetupHelpOptions(argsman); - argsman.AddArg("-asymptote=n1,n2,n3,...", "Test asymptotic growth of the runtime of an algorithm, if supported by the benchmark", ArgsManager::ALLOW_ANY, OptionsCategory::OPTIONS); + argsman.AddArg("-asymptote=", "Test asymptotic growth of the runtime of an algorithm, if supported by the benchmark", ArgsManager::ALLOW_ANY, OptionsCategory::OPTIONS); argsman.AddArg("-filter=", strprintf("Regular expression filter to select benchmark by name (default: %s)", DEFAULT_BENCH_FILTER), ArgsManager::ALLOW_ANY, OptionsCategory::OPTIONS); - argsman.AddArg("-list", "List benchmarks without executing them", ArgsManager::ALLOW_ANY, OptionsCategory::OPTIONS); + argsman.AddArg("-list", "List benchmarks without executing them", ArgsManager::ALLOW_BOOL, OptionsCategory::OPTIONS); + argsman.AddArg("-min_time=", strprintf("Minimum runtime per benchmark, in milliseconds (default: %d)", DEFAULT_MIN_TIME_MS), ArgsManager::ALLOW_INT, OptionsCategory::OPTIONS); argsman.AddArg("-output_csv=", "Generate CSV file with the most important benchmark results", ArgsManager::ALLOW_ANY, OptionsCategory::OPTIONS); argsman.AddArg("-output_json=", "Generate JSON file with all benchmark results", ArgsManager::ALLOW_ANY, OptionsCategory::OPTIONS); } @@ -50,16 +58,62 @@ int main(int argc, char** argv) } if (HelpRequested(argsman)) { - std::cout << argsman.GetHelpMessage(); + std::cout << "Usage: bench_dash [options]\n" + "\n" + << argsman.GetHelpMessage() + << "Description:\n" + "\n" + " bench_dash executes microbenchmarks. The quality of the benchmark results\n" + " highly depend on the stability of the machine. It can sometimes be difficult\n" + " to get stable, repeatable results, so here are a few tips:\n" + "\n" + " * Use pyperf [1] to disable frequency scaling, turbo boost etc. For best\n" + " results, use CPU pinning and CPU isolation (see [2]).\n" + "\n" + " * Each call of run() should do exactly the same work. E.g. inserting into\n" + " a std::vector doesn't do that as it will reallocate on certain calls. Make\n" + " sure each run has exactly the same preconditions.\n" + "\n" + " * If results are still not reliable, increase runtime with e.g.\n" + " -min_time=5000 to let a benchmark run for at least 5 seconds.\n" + "\n" + " * bench_dash uses nanobench [3] for which there is extensive\n" + " documentation available online.\n" + "\n" + "Environment Variables:\n" + "\n" + " To attach a profiler you can run a benchmark in endless mode. This can be\n" + " done with the environment variable NANOBENCH_ENDLESS. E.g. like so:\n" + "\n" + " NANOBENCH_ENDLESS=MuHash ./bench_dash -filter=MuHash\n" + "\n" + " In rare cases it can be useful to suppress stability warnings. This can be\n" + " done with the environment variable NANOBENCH_SUPPRESS_WARNINGS, e.g:\n" + "\n" + " NANOBENCH_SUPPRESS_WARNINGS=1 ./bench_dash\n" + "\n" + "Notes:\n" + "\n" + " 1. pyperf\n" + " https://github.com/psf/pyperf\n" + "\n" + " 2. CPU pinning & isolation\n" + " https://pyperf.readthedocs.io/en/latest/system.html\n" + "\n" + " 3. nanobench\n" + " https://github.com/martinus/nanobench\n" + "\n"; + return EXIT_SUCCESS; } benchmark::Args args; - args.regex_filter = argsman.GetArg("-filter", DEFAULT_BENCH_FILTER); - args.is_list_only = argsman.GetBoolArg("-list", false); args.asymptote = parseAsymptote(argsman.GetArg("-asymptote", "")); + args.is_list_only = argsman.GetBoolArg("-list", false); + args.min_time = std::chrono::milliseconds(argsman.GetArg("-min_time", DEFAULT_MIN_TIME_MS)); args.output_csv = fs::PathFromString(argsman.GetArg("-output_csv", "")); args.output_json = fs::PathFromString(argsman.GetArg("-output_json", "")); + args.regex_filter = argsman.GetArg("-filter", DEFAULT_BENCH_FILTER); benchmark::BenchRunner::RunAll(args); diff --git a/src/bench/crypto_hash.cpp b/src/bench/crypto_hash.cpp index cb00185b2d..056ccf0e19 100644 --- a/src/bench/crypto_hash.cpp +++ b/src/bench/crypto_hash.cpp @@ -249,9 +249,9 @@ static void MuHash(benchmark::Bench& bench) { MuHash3072 acc; unsigned char key[32] = {0}; - int i = 0; + uint32_t i = 0; bench.run([&] { - key[0] = ++i; + key[0] = ++i & 0xFF; acc *= MuHash3072(key); }); } @@ -273,10 +273,6 @@ static void MuHashDiv(benchmark::Bench& bench) FastRandomContext rng(true); MuHash3072 muhash{rng.randbytes(32)}; - for (size_t i = 0; i < bench.epochIterations(); ++i) { - acc *= muhash; - } - bench.run([&] { acc /= muhash; }); diff --git a/src/bench/peer_eviction.cpp b/src/bench/peer_eviction.cpp index d5086bff85..f05f5e8f64 100644 --- a/src/bench/peer_eviction.cpp +++ b/src/bench/peer_eviction.cpp @@ -20,19 +20,17 @@ static void EvictionProtectionCommon( { using Candidates = std::vector; FastRandomContext random_context{true}; - bench.warmup(100).epochIterations(1100); Candidates candidates{GetRandomNodeEvictionCandidates(num_candidates, random_context)}; for (auto& c : candidates) { candidate_setup_fn(c); } - std::vector copies{ - static_cast(bench.epochs() * bench.epochIterations()), candidates}; - size_t i{0}; + bench.run([&] { - ProtectEvictionCandidatesByRatio(copies.at(i)); - ++i; + // creating a copy has an overhead of about 3%, so it does not influence the benchmark results much. + auto copy = candidates; + ProtectEvictionCandidatesByRatio(copy); }); } diff --git a/src/bench/rollingbloom.cpp b/src/bench/rollingbloom.cpp index 997ab56549..28167767db 100644 --- a/src/bench/rollingbloom.cpp +++ b/src/bench/rollingbloom.cpp @@ -13,16 +13,16 @@ static void RollingBloom(benchmark::Bench& bench) uint32_t count = 0; bench.run([&] { count++; - data[0] = count; - data[1] = count >> 8; - data[2] = count >> 16; - data[3] = count >> 24; + data[0] = count & 0xFF; + data[1] = (count >> 8) & 0xFF; + data[2] = (count >> 16) & 0xFF; + data[3] = (count >> 24) & 0xFF; filter.insert(data); - data[0] = count >> 24; - data[1] = count >> 16; - data[2] = count >> 8; - data[3] = count; + data[0] = (count >> 24) & 0xFF; + data[1] = (count >> 16) & 0xFF; + data[2] = (count >> 8) & 0xFF; + data[3] = count & 0xFF; filter.contains(data); }); } From bd67f806343314962198b463758f7dca3fd56d54 Mon Sep 17 00:00:00 2001 From: "W. J. van der Laan" Date: Wed, 15 Dec 2021 11:48:21 +0100 Subject: [PATCH 3/3] Merge bitcoin/bitcoin#22362: Drop only invalid entries when reading banlist.json faa6c3d44c861c0486c1369e1d098b7645ab07cd net: Drop only invalid entries when reading banlist.json (MarcoFalke) Pull request description: All entries will be dropped when there is at least one invalid one in `banlist.json`. Fix this by only dropping invalid ones. Also suggested in https://github.com/bitcoin/bitcoin/pull/20966#issuecomment-861150204 ACKs for top commit: laanwj: Re-ACK faa6c3d44c861c0486c1369e1d098b7645ab07cd Tree-SHA512: 5a58e7f1dcabf78d0c65d8c6d5d997063af1efeaa50ca7730fc00056fda7e0061b6f7a38907ea045fe667c9f61d392e01e556b425a95e6b126e3c41cd33deb83 --- src/Makefile.test.include | 1 + src/net_types.cpp | 17 ++++++++++++---- src/test/banman_tests.cpp | 43 +++++++++++++++++++++++++++++++++++++++ 3 files changed, 57 insertions(+), 4 deletions(-) create mode 100644 src/test/banman_tests.cpp diff --git a/src/Makefile.test.include b/src/Makefile.test.include index 471c38be44..92e2f086ff 100644 --- a/src/Makefile.test.include +++ b/src/Makefile.test.include @@ -77,6 +77,7 @@ BITCOIN_TESTS =\ test/addrman_tests.cpp \ test/amount_tests.cpp \ test/allocator_tests.cpp \ + test/banman_tests.cpp \ test/base32_tests.cpp \ test/base58_tests.cpp \ test/base64_tests.cpp \ diff --git a/src/net_types.cpp b/src/net_types.cpp index c8f57fe6c6..e4101a9876 100644 --- a/src/net_types.cpp +++ b/src/net_types.cpp @@ -4,12 +4,16 @@ #include +#include #include #include #include +static const char* BANMAN_JSON_VERSION_KEY{"version"}; + CBanEntry::CBanEntry(const UniValue& json) - : nVersion(json["version"].get_int()), nCreateTime(json["ban_created"].get_int64()), + : nVersion(json[BANMAN_JSON_VERSION_KEY].get_int()), + nCreateTime(json["ban_created"].get_int64()), nBanUntil(json["banned_until"].get_int64()) { } @@ -17,7 +21,7 @@ CBanEntry::CBanEntry(const UniValue& json) UniValue CBanEntry::ToJson() const { UniValue json(UniValue::VOBJ); - json.pushKV("version", nVersion); + json.pushKV(BANMAN_JSON_VERSION_KEY, nVersion); json.pushKV("ban_created", nCreateTime); json.pushKV("banned_until", nBanUntil); return json; @@ -54,11 +58,16 @@ UniValue BanMapToJson(const banmap_t& bans) void BanMapFromJson(const UniValue& bans_json, banmap_t& bans) { for (const auto& ban_entry_json : bans_json.getValues()) { + const int version{ban_entry_json[BANMAN_JSON_VERSION_KEY].get_int()}; + if (version != CBanEntry::CURRENT_VERSION) { + LogPrintf("Dropping entry with unknown version (%s) from ban list\n", version); + continue; + } CSubNet subnet; const auto& subnet_str = ban_entry_json[BANMAN_JSON_ADDR_KEY].get_str(); if (!LookupSubNet(subnet_str, subnet)) { - throw std::runtime_error( - strprintf("Cannot parse banned address or subnet: %s", subnet_str)); + LogPrintf("Dropping entry with unparseable address or subnet (%s) from ban list\n", subnet_str); + continue; } bans.insert_or_assign(subnet, CBanEntry{ban_entry_json}); } diff --git a/src/test/banman_tests.cpp b/src/test/banman_tests.cpp new file mode 100644 index 0000000000..27ce9ad638 --- /dev/null +++ b/src/test/banman_tests.cpp @@ -0,0 +1,43 @@ +// Copyright (c) 2021 The Bitcoin Core developers +// Distributed under the MIT software license, see the accompanying +// file COPYING or http://www.opensource.org/licenses/mit-license.php. + +#include +#include +#include +#include +#include +#include +#include + + +#include + +BOOST_FIXTURE_TEST_SUITE(banman_tests, BasicTestingSetup) + +BOOST_AUTO_TEST_CASE(file) +{ + SetMockTime(777s); + const fs::path banlist_path{m_args.GetDataDirBase() / "banlist_test"}; + { + const std::string entries_write{ + "{ \"banned_nets\": [" + " { \"version\": 1, \"ban_created\": 0, \"banned_until\": 778, \"address\": \"aaaaaaaaa\" }," + " { \"version\": 2, \"ban_created\": 0, \"banned_until\": 778, \"address\": \"bbbbbbbbb\" }," + " { \"version\": 1, \"ban_created\": 0, \"banned_until\": 778, \"address\": \"1.0.0.0/8\" }" + "] }", + }; + assert(WriteBinaryFile(banlist_path + ".json", entries_write)); + { + // The invalid entries will be dropped, but the valid one remains + ASSERT_DEBUG_LOG("Dropping entry with unparseable address or subnet (aaaaaaaaa) from ban list"); + ASSERT_DEBUG_LOG("Dropping entry with unknown version (2) from ban list"); + BanMan banman{banlist_path, /*client_interface=*/nullptr, /*default_ban_time=*/0}; + banmap_t entries_read; + banman.GetBanned(entries_read); + assert(entries_read.size() == 1); + } + } +} + +BOOST_AUTO_TEST_SUITE_END()