From 76bc2c20c447d6992e59f27f4406a0ddfb449224 Mon Sep 17 00:00:00 2001 From: Bronek Kozicki <823856+Bronek@users.noreply.github.com> Date: Thu, 12 Oct 2023 16:27:57 +0100 Subject: [PATCH 01/13] Add handler performance report --- Builds/CMake/RippledCore.cmake | 1 + src/test/rpc/Handler_test.cpp | 114 +++++++++++++++++++++++++++++++++ 2 files changed, 115 insertions(+) create mode 100644 src/test/rpc/Handler_test.cpp diff --git a/Builds/CMake/RippledCore.cmake b/Builds/CMake/RippledCore.cmake index 8d2ff6cbaef..7c9ad7fdc57 100644 --- a/Builds/CMake/RippledCore.cmake +++ b/Builds/CMake/RippledCore.cmake @@ -1079,6 +1079,7 @@ if (tests) src/test/rpc/ValidatorInfo_test.cpp src/test/rpc/ValidatorRPC_test.cpp src/test/rpc/Version_test.cpp + src/test/rpc/Handler_test.cpp #[===============================[ test sources: subdir: server diff --git a/src/test/rpc/Handler_test.cpp b/src/test/rpc/Handler_test.cpp new file mode 100644 index 00000000000..d7dd37a3813 --- /dev/null +++ b/src/test/rpc/Handler_test.cpp @@ -0,0 +1,114 @@ +//------------------------------------------------------------------------------ +/* + This file is part of rippled: https://github.com/ripple/rippled + Copyright (c) 2023 Ripple Labs Inc. + + Permission to use, copy, modify, and/or distribute this software for any + purpose with or without fee is hereby granted, provided that the above + copyright notice and this permission notice appear in all copies. + + THE SOFTWARE IS PROVIDED "AS IS" AND THE AUTHOR DISCLAIMS ALL WARRANTIES + WITH REGARD TO THIS SOFTWARE INCLUDING ALL IMPLIED WARRANTIES OF + MERCHANTABILITY AND FITNESS. IN NO EVENT SHALL THE AUTHOR BE LIABLE FOR + ANY SPECIAL , DIRECT, INDIRECT, OR CONSEQUENTIAL DAMAGES OR ANY DAMAGES + WHATSOEVER RESULTING FROM LOSS OF USE, DATA OR PROFITS, WHETHER IN AN + ACTION OF CONTRACT, NEGLIGENCE OR OTHER TORTIOUS ACTION, ARISING OUT OF + OR IN CONNECTION WITH THE USE OR PERFORMANCE OF THIS SOFTWARE. +*/ +//============================================================================== + +#include +#include "ripple/overlay/Peer.h" +#include +#include + +#include +#include +#include +#include + +namespace ripple::RPC { + +// NOTE This is a rather naiive effort at a microbenchmark. Ideally we want +// Google Benchmark, or something similar. Also, this actually does not belong +// to unit tests, as it makes little sense to run it in conditions very +// dissimilar to how rippled will normally work. +// TODO as https://github.com/XRPLF/rippled/issues/4765 + +class Handler_test : public beast::unit_test::suite +{ + std::tuple + time(std::size_t n, auto f, auto prng) + { + assert(n > 0); + double sum = 0; + double sum_squared = 0; + std::size_t j = 0; + while (j < n) + { + // Generate 20 inputs upfront, separated from the inner loop + std::array inputs = {}; + for (auto& i : inputs) + { + i = prng(); + } + + // Take 20 samples and throw away 7 from each end, using middle 6 + std::array samples = {}; + for (std::size_t k = 0; k < 20; ++k) + { + auto start = std::chrono::steady_clock::now(); + f(inputs[k]); + samples[k] = (std::chrono::steady_clock::now() - start).count(); + } + + std::sort(samples.begin(), samples.end()); + for (std::size_t k = 7; k < 13; ++k) + { + j += 1; + sum += samples[k]; + sum_squared += (samples[k] * samples[k]); + } + } + + double const mean = sum / j; + return {mean, std::sqrt((sum_squared / j) - (mean * mean)), j}; + } + + void + reportLookupPerformance() + { + testcase("Handler lookup performance"); + + std::random_device dev; + std::ranlux48 prng(dev()); + + auto const names = getHandlerNames(); + std::uniform_int_distribution distr{0, names.size() - 1}; + + std::size_t dummy = 0; + auto const [mean, stdev, n] = time( + 1'000'000, + [&](std::size_t i) { + auto const d = getHandler(1, false, names[i]); + dummy = dummy + i + (int)d->role_; + }, + [&]() -> std::size_t { return distr(prng); }); + + std::cout << "mean=" << mean << " stdev=" << stdev << " N=" << n + << '\n'; + + BEAST_EXPECT(dummy != 0); + } + +public: + void + run() override + { + reportLookupPerformance(); + } +}; + +BEAST_DEFINE_TESTSUITE(Handler, rpc, ripple); + +} // namespace ripple::RPC \ No newline at end of file From fdd6c78a6375706bd1ad9c29d2c73202b0ba0c1c Mon Sep 17 00:00:00 2001 From: Bronek Kozicki <823856+Bronek@users.noreply.github.com> Date: Wed, 11 Oct 2023 15:39:53 +0100 Subject: [PATCH 02/13] Add support for handlers version selection --- src/ripple/rpc/handlers/LedgerHandler.h | 23 +++---- src/ripple/rpc/handlers/Version.h | 23 +++---- src/ripple/rpc/impl/Handler.cpp | 83 ++++++++++++++++++++----- src/ripple/rpc/impl/Handler.h | 4 ++ src/ripple/rpc/impl/RPCHelpers.h | 2 + src/test/rpc/Handler_test.cpp | 4 +- 6 files changed, 91 insertions(+), 48 deletions(-) diff --git a/src/ripple/rpc/handlers/LedgerHandler.h b/src/ripple/rpc/handlers/LedgerHandler.h index 77b361d3466..efa54187b76 100644 --- a/src/ripple/rpc/handlers/LedgerHandler.h +++ b/src/ripple/rpc/handlers/LedgerHandler.h @@ -30,6 +30,7 @@ #include #include #include +#include namespace Json { class Object; @@ -58,23 +59,15 @@ class LedgerHandler void writeResult(Object&); - static char const* - name() - { - return "ledger"; - } + static constexpr char const name[] = "ledger"; - static Role - role() - { - return Role::USER; - } + static constexpr unsigned minApiVer = 1; - static Condition - condition() - { - return NO_CONDITION; - } + static constexpr unsigned maxApiVer = RPC::apiMaximumValidVersion; + + static constexpr Role role = Role::USER; + + static constexpr Condition condition = NO_CONDITION; private: JsonContext& context_; diff --git a/src/ripple/rpc/handlers/Version.h b/src/ripple/rpc/handlers/Version.h index a9f42b94993..8c320749553 100644 --- a/src/ripple/rpc/handlers/Version.h +++ b/src/ripple/rpc/handlers/Version.h @@ -20,6 +20,7 @@ #ifndef RIPPLED_RIPPLE_RPC_HANDLERS_VERSION_H #define RIPPLED_RIPPLE_RPC_HANDLERS_VERSION_H +#include #include namespace ripple { @@ -46,23 +47,15 @@ class VersionHandler setVersion(obj, apiVersion_, betaEnabled_); } - static char const* - name() - { - return "version"; - } + static constexpr char const* name = "version"; - static Role - role() - { - return Role::USER; - } + static constexpr unsigned minApiVer = 1; - static Condition - condition() - { - return NO_CONDITION; - } + static constexpr unsigned maxApiVer = RPC::apiMaximumValidVersion; + + static constexpr Role role = Role::USER; + + static constexpr Condition condition = NO_CONDITION; private: unsigned int apiVersion_; diff --git a/src/ripple/rpc/impl/Handler.cpp b/src/ripple/rpc/impl/Handler.cpp index dd898ee8722..df7ac4de098 100644 --- a/src/ripple/rpc/impl/Handler.cpp +++ b/src/ripple/rpc/impl/Handler.cpp @@ -22,6 +22,8 @@ #include #include +#include + namespace ripple { namespace RPC { namespace { @@ -57,6 +59,19 @@ handle(JsonContext& context, Object& object) return status; }; +template +Handler +handlerFrom() +{ + return { + HandlerImpl::name, + &handle, + HandlerImpl::role, + HandlerImpl::condition, + HandlerImpl::minApiVer, + HandlerImpl::maxApiVer}; +} + Handler const handlerArray[]{ // Some handlers not specified here are added to the table via addHandler() // Request-response methods @@ -174,14 +189,39 @@ Handler const handlerArray[]{ class HandlerTable { private: + using handler_table_t = std::multimap; + + // Use with equal_range to enforce that API range of a newly added handler + // does not overlap with API range of an existing handler with same name + bool + overlappingApiVersion( + std::pair range, + unsigned minVer, + unsigned maxVer) + { + assert(minVer <= maxVer); + assert(maxVer <= RPC::apiMaximumValidVersion); + + for (; range.first != range.second; range.first++) + { + if (range.first->second.minApiVer_ <= maxVer && + range.first->second.maxApiVer_ >= minVer) + return true; + } + return false; + } + template explicit HandlerTable(const Handler (&entries)[N]) { - for (std::size_t i = 0; i < N; ++i) + for (auto const& entry : entries) { - auto const& entry = entries[i]; - assert(table_.find(entry.name_) == table_.end()); - table_[entry.name_] = entry; + assert(!overlappingApiVersion( + table_.equal_range(entry.name_), + entry.minApiVer_, + entry.maxApiVer_)); + + table_.insert({entry.name_, entry}); } // This is where the new-style handlers are added. @@ -205,36 +245,47 @@ class HandlerTable version > (betaEnabled ? RPC::apiBetaVersion : RPC::apiMaximumSupportedVersion)) return nullptr; - auto i = table_.find(name); - return i == table_.end() ? nullptr : &i->second; + + auto const range = table_.equal_range(name); + auto const i = std::find_if( + range.first, range.second, [version](auto const& entry) { + return version >= entry.second.minApiVer_ && + version <= entry.second.maxApiVer_; + }); + + return i == range.second ? nullptr : &i->second; } std::vector getHandlerNames() const { std::vector ret; - ret.reserve(table_.size()); for (auto const& i : table_) - ret.push_back(i.second.name_); + { + // Note, table_ is always ordered, allowing such a simple check + if (ret.empty() || std::strcmp(ret.back(), i.second.name_) != 0) + ret.push_back(i.second.name_); + } + return ret; } private: - std::map table_; + handler_table_t table_; template void addHandler() { - assert(table_.find(HandlerImpl::name()) == table_.end()); + static_assert(HandlerImpl::minApiVer <= HandlerImpl::maxApiVer); + static_assert(HandlerImpl::maxApiVer <= RPC::apiMaximumValidVersion); - Handler h; - h.name_ = HandlerImpl::name(); - h.valueMethod_ = &handle; - h.role_ = HandlerImpl::role(); - h.condition_ = HandlerImpl::condition(); + assert(!overlappingApiVersion( + table_.equal_range(HandlerImpl::name), + HandlerImpl::minApiVer, + HandlerImpl::maxApiVer)); - table_[HandlerImpl::name()] = h; + table_.insert({HandlerImpl::name, handlerFrom()}); } }; diff --git a/src/ripple/rpc/impl/Handler.h b/src/ripple/rpc/impl/Handler.h index e2188ef51e7..9c4008bb6e7 100644 --- a/src/ripple/rpc/impl/Handler.h +++ b/src/ripple/rpc/impl/Handler.h @@ -25,6 +25,7 @@ #include #include #include +#include #include #include @@ -52,6 +53,9 @@ struct Handler Method valueMethod_; Role role_; RPC::Condition condition_; + + unsigned minApiVer_ = 1; + unsigned maxApiVer_ = apiMaximumValidVersion; }; Handler const* diff --git a/src/ripple/rpc/impl/RPCHelpers.h b/src/ripple/rpc/impl/RPCHelpers.h index eb02e6ea37a..516f66fc620 100644 --- a/src/ripple/rpc/impl/RPCHelpers.h +++ b/src/ripple/rpc/impl/RPCHelpers.h @@ -242,10 +242,12 @@ constexpr unsigned int apiVersionIfUnspecified = 1; constexpr unsigned int apiMinimumSupportedVersion = 1; constexpr unsigned int apiMaximumSupportedVersion = 1; constexpr unsigned int apiBetaVersion = 2; +constexpr unsigned int apiMaximumValidVersion = apiBetaVersion; static_assert(apiMinimumSupportedVersion >= apiVersionIfUnspecified); static_assert(apiMaximumSupportedVersion >= apiMinimumSupportedVersion); static_assert(apiBetaVersion >= apiMaximumSupportedVersion); +static_assert(apiMaximumValidVersion >= apiMaximumSupportedVersion); template void diff --git a/src/test/rpc/Handler_test.cpp b/src/test/rpc/Handler_test.cpp index d7dd37a3813..c8ee2cac96f 100644 --- a/src/test/rpc/Handler_test.cpp +++ b/src/test/rpc/Handler_test.cpp @@ -17,13 +17,13 @@ */ //============================================================================== +#include #include -#include "ripple/overlay/Peer.h" -#include #include #include #include +#include #include #include From a6e12fab300816509c3ff4d2ab73839420f3d79a Mon Sep 17 00:00:00 2001 From: Bronek Kozicki <823856+Bronek@users.noreply.github.com> Date: Wed, 11 Oct 2023 17:10:56 +0100 Subject: [PATCH 03/13] Remove tx_history in API v2 --- src/ripple/rpc/impl/Handler.cpp | 2 +- src/test/rpc/TransactionHistory_test.cpp | 16 ++++++++++++++++ 2 files changed, 17 insertions(+), 1 deletion(-) diff --git a/src/ripple/rpc/impl/Handler.cpp b/src/ripple/rpc/impl/Handler.cpp index df7ac4de098..f618af5756c 100644 --- a/src/ripple/rpc/impl/Handler.cpp +++ b/src/ripple/rpc/impl/Handler.cpp @@ -167,7 +167,7 @@ Handler const handlerArray[]{ {"stop", byRef(&doStop), Role::ADMIN, NO_CONDITION}, {"transaction_entry", byRef(&doTransactionEntry), Role::USER, NO_CONDITION}, {"tx", byRef(&doTxJson), Role::USER, NEEDS_NETWORK_CONNECTION}, - {"tx_history", byRef(&doTxHistory), Role::USER, NO_CONDITION}, + {"tx_history", byRef(&doTxHistory), Role::USER, NO_CONDITION, 1, 1}, {"tx_reduce_relay", byRef(&doTxReduceRelay), Role::USER, NO_CONDITION}, {"unl_list", byRef(&doUnlList), Role::ADMIN, NO_CONDITION}, {"validation_create", diff --git a/src/test/rpc/TransactionHistory_test.cpp b/src/test/rpc/TransactionHistory_test.cpp index 3f9b5792744..862eaaee507 100644 --- a/src/test/rpc/TransactionHistory_test.cpp +++ b/src/test/rpc/TransactionHistory_test.cpp @@ -54,6 +54,21 @@ class TransactionHistory_test : public beast::unit_test::suite } } + void + testCommandRetired() + { + testcase("Command retired from API v2"); + using namespace test::jtx; + Env env{*this, envconfig(no_admin)}; + + Json::Value params{Json::objectValue}; + params[jss::api_version] = 2; + auto const result = + env.client().invoke("tx_history", params)[jss::result]; + BEAST_EXPECT(result[jss::error] == "unknownCmd"); + BEAST_EXPECT(result[jss::status] == "error"); + } + void testRequest() { @@ -148,6 +163,7 @@ class TransactionHistory_test : public beast::unit_test::suite { testBadInput(); testRequest(); + testCommandRetired(); } }; From b8071aa204aef31f4fe7810c738558e7b56b5995 Mon Sep 17 00:00:00 2001 From: Bronek Kozicki <823856+Bronek@users.noreply.github.com> Date: Fri, 13 Oct 2023 16:52:01 +0100 Subject: [PATCH 04/13] Remove ledger_header in API v2 --- Builds/CMake/RippledCore.cmake | 1 + src/ripple/rpc/impl/Handler.cpp | 2 +- src/test/rpc/LedgerHeader_test.cpp | 92 ++++++++++++++++++++++++++++++ 3 files changed, 94 insertions(+), 1 deletion(-) create mode 100644 src/test/rpc/LedgerHeader_test.cpp diff --git a/Builds/CMake/RippledCore.cmake b/Builds/CMake/RippledCore.cmake index 7c9ad7fdc57..89a418f7eaa 100644 --- a/Builds/CMake/RippledCore.cmake +++ b/Builds/CMake/RippledCore.cmake @@ -1056,6 +1056,7 @@ if (tests) src/test/rpc/KeyGeneration_test.cpp src/test/rpc/LedgerClosed_test.cpp src/test/rpc/LedgerData_test.cpp + src/test/rpc/LedgerHeader_test.cpp src/test/rpc/LedgerRPC_test.cpp src/test/rpc/LedgerRequestRPC_test.cpp src/test/rpc/ManifestRPC_test.cpp diff --git a/src/ripple/rpc/impl/Handler.cpp b/src/ripple/rpc/impl/Handler.cpp index f618af5756c..d316c6b69db 100644 --- a/src/ripple/rpc/impl/Handler.cpp +++ b/src/ripple/rpc/impl/Handler.cpp @@ -124,7 +124,7 @@ Handler const handlerArray[]{ NEEDS_CURRENT_LEDGER}, {"ledger_data", byRef(&doLedgerData), Role::USER, NO_CONDITION}, {"ledger_entry", byRef(&doLedgerEntry), Role::USER, NO_CONDITION}, - {"ledger_header", byRef(&doLedgerHeader), Role::USER, NO_CONDITION}, + {"ledger_header", byRef(&doLedgerHeader), Role::USER, NO_CONDITION, 1, 1}, {"ledger_request", byRef(&doLedgerRequest), Role::ADMIN, NO_CONDITION}, {"log_level", byRef(&doLogLevel), Role::ADMIN, NO_CONDITION}, {"logrotate", byRef(&doLogRotate), Role::ADMIN, NO_CONDITION}, diff --git a/src/test/rpc/LedgerHeader_test.cpp b/src/test/rpc/LedgerHeader_test.cpp new file mode 100644 index 00000000000..11151311747 --- /dev/null +++ b/src/test/rpc/LedgerHeader_test.cpp @@ -0,0 +1,92 @@ +//------------------------------------------------------------------------------ +/* + This file is part of rippled: https://github.com/ripple/rippled + Copyright (c) 2023 Ripple Labs Inc. + + Permission to use, copy, modify, and/or distribute this software for any + purpose with or without fee is hereby granted, provided that the above + copyright notice and this permission notice appear in all copies. + + THE SOFTWARE IS PROVIDED "AS IS" AND THE AUTHOR DISCLAIMS ALL WARRANTIES + WITH REGARD TO THIS SOFTWARE INCLUDING ALL IMPLIED WARRANTIES OF + MERCHANTABILITY AND FITNESS. IN NO EVENT SHALL THE AUTHOR BE LIABLE FOR + ANY SPECIAL , DIRECT, INDIRECT, OR CONSEQUENTIAL DAMAGES OR ANY DAMAGES + WHATSOEVER RESULTING FROM LOSS OF USE, DATA OR PROFITS, WHETHER IN AN + ACTION OF CONTRACT, NEGLIGENCE OR OTHER TORTIOUS ACTION, ARISING OUT OF + OR IN CONNECTION WITH THE USE OR PERFORMANCE OF THIS SOFTWARE. +*/ +//============================================================================== + +#include +#include +#include +#include + +namespace ripple { + +class LedgerHeader_test : public beast::unit_test::suite +{ + void + testSimpleCurrent() + { + testcase("Current ledger"); + using namespace test::jtx; + Env env{*this, envconfig(no_admin)}; + + Json::Value params{Json::objectValue}; + params[jss::api_version] = 1; + params[jss::ledger_index] = "current"; + auto const result = + env.client().invoke("ledger_header", params)[jss::result]; + BEAST_EXPECT(result[jss::status] == "success"); + BEAST_EXPECT(result.isMember("ledger")); + BEAST_EXPECT(result[jss::ledger][jss::closed] == false); + BEAST_EXPECT(result[jss::validated] == false); + } + + void + testSimpleValidated() + { + testcase("Validated ledger"); + using namespace test::jtx; + Env env{*this, envconfig(no_admin)}; + + Json::Value params{Json::objectValue}; + params[jss::api_version] = 1; + params[jss::ledger_index] = "validated"; + auto const result = + env.client().invoke("ledger_header", params)[jss::result]; + BEAST_EXPECT(result[jss::status] == "success"); + BEAST_EXPECT(result.isMember("ledger")); + BEAST_EXPECT(result[jss::ledger][jss::closed] == true); + BEAST_EXPECT(result[jss::validated] == true); + } + + void + testCommandRetired() + { + testcase("Command retired from API v2"); + using namespace test::jtx; + Env env{*this, envconfig(no_admin)}; + + Json::Value params{Json::objectValue}; + params[jss::api_version] = 2; + auto const result = + env.client().invoke("ledger_header", params)[jss::result]; + BEAST_EXPECT(result[jss::error] == "unknownCmd"); + BEAST_EXPECT(result[jss::status] == "error"); + } + +public: + void + run() override + { + testSimpleCurrent(); + testSimpleValidated(); + testCommandRetired(); + } +}; + +BEAST_DEFINE_TESTSUITE(LedgerHeader, rpc, ripple); + +} // namespace ripple From aa68b61690fde2b3770bc440a9782bf434e8dd8e Mon Sep 17 00:00:00 2001 From: Bronek Kozicki <823856+Bronek@users.noreply.github.com> Date: Fri, 13 Oct 2023 17:57:09 +0100 Subject: [PATCH 05/13] Update API-CHANGELOG.md --- API-CHANGELOG.md | 7 +++++++ 1 file changed, 7 insertions(+) diff --git a/API-CHANGELOG.md b/API-CHANGELOG.md index 5115eee0855..634d2a514ef 100644 --- a/API-CHANGELOG.md +++ b/API-CHANGELOG.md @@ -125,6 +125,13 @@ Currently (prior to the release of 2.0), it is available as a "beta" version, me Since `api_version` 2 is in "beta", breaking changes to V2 can currently be made at any time. +#### Retired methods + +In API version 2, the following methods are no longer available: + +- `tx_history` - please use other appropriate method, e.g. `tx` or `account_tx` or `ledger` with `transactions` field set to `true` +- `ledger_header` - please use `ledger` with `binary` field set to `true` + #### Modifications to account_info response in V2 - `signer_lists` is returned in the root of the response. Previously, in API version 1, it was nested under `account_data`. (https://github.com/XRPLF/rippled/pull/3770) From f88e58b880325dd96def16b9cf43611f85dd04d7 Mon Sep 17 00:00:00 2001 From: Bronek Kozicki Date: Fri, 13 Oct 2023 20:14:24 +0100 Subject: [PATCH 06/13] Update API-CHANGELOG.md Co-authored-by: Rome Reginelli --- API-CHANGELOG.md | 6 +++--- 1 file changed, 3 insertions(+), 3 deletions(-) diff --git a/API-CHANGELOG.md b/API-CHANGELOG.md index 634d2a514ef..0808d5a3b6e 100644 --- a/API-CHANGELOG.md +++ b/API-CHANGELOG.md @@ -125,12 +125,12 @@ Currently (prior to the release of 2.0), it is available as a "beta" version, me Since `api_version` 2 is in "beta", breaking changes to V2 can currently be made at any time. -#### Retired methods +#### Removed methods In API version 2, the following methods are no longer available: -- `tx_history` - please use other appropriate method, e.g. `tx` or `account_tx` or `ledger` with `transactions` field set to `true` -- `ledger_header` - please use `ledger` with `binary` field set to `true` +- `tx_history` - Instead, use other methods such as `account_tx` or `ledger` with the `transactions` field set to `true`. +- `ledger_header` - Instead, use the `ledger` method. #### Modifications to account_info response in V2 From 22eab9e049b046374b762678418e883b8b94f6bc Mon Sep 17 00:00:00 2001 From: Chenna Keshava Date: Mon, 16 Oct 2023 15:02:35 -0700 Subject: [PATCH 07/13] use std::set instead of std::vector to store unique RPC handler names --- src/ripple/perflog/impl/PerfLogImp.cpp | 2 +- src/ripple/perflog/impl/PerfLogImp.h | 4 +--- src/ripple/rpc/handlers/Version.h | 1 - src/ripple/rpc/impl/Handler.cpp | 14 +++++--------- src/ripple/rpc/impl/Handler.h | 2 +- src/test/basics/PerfLog_test.cpp | 4 +++- src/test/rpc/Handler_test.cpp | 5 ++++- 7 files changed, 15 insertions(+), 17 deletions(-) diff --git a/src/ripple/perflog/impl/PerfLogImp.cpp b/src/ripple/perflog/impl/PerfLogImp.cpp index db5a188fc3e..3d07d0ed625 100644 --- a/src/ripple/perflog/impl/PerfLogImp.cpp +++ b/src/ripple/perflog/impl/PerfLogImp.cpp @@ -43,7 +43,7 @@ namespace ripple { namespace perf { PerfLogImp::Counters::Counters( - std::vector const& labels, + std::set const& labels, JobTypes const& jobTypes) { { diff --git a/src/ripple/perflog/impl/PerfLogImp.h b/src/ripple/perflog/impl/PerfLogImp.h index 493c1dc1a18..4904126d95f 100644 --- a/src/ripple/perflog/impl/PerfLogImp.h +++ b/src/ripple/perflog/impl/PerfLogImp.h @@ -113,9 +113,7 @@ class PerfLogImp : public PerfLog std::unordered_map methods_; mutable std::mutex methodsMutex_; - Counters( - std::vector const& labels, - JobTypes const& jobTypes); + Counters(std::set const& labels, JobTypes const& jobTypes); Json::Value countersJson() const; Json::Value diff --git a/src/ripple/rpc/handlers/Version.h b/src/ripple/rpc/handlers/Version.h index 8c320749553..69578727fab 100644 --- a/src/ripple/rpc/handlers/Version.h +++ b/src/ripple/rpc/handlers/Version.h @@ -20,7 +20,6 @@ #ifndef RIPPLED_RIPPLE_RPC_HANDLERS_VERSION_H #define RIPPLED_RIPPLE_RPC_HANDLERS_VERSION_H -#include #include namespace ripple { diff --git a/src/ripple/rpc/impl/Handler.cpp b/src/ripple/rpc/impl/Handler.cpp index d316c6b69db..590a1a3de9b 100644 --- a/src/ripple/rpc/impl/Handler.cpp +++ b/src/ripple/rpc/impl/Handler.cpp @@ -256,16 +256,12 @@ class HandlerTable return i == range.second ? nullptr : &i->second; } - std::vector + std::set getHandlerNames() const { - std::vector ret; + std::set ret; for (auto const& i : table_) - { - // Note, table_ is always ordered, allowing such a simple check - if (ret.empty() || std::strcmp(ret.back(), i.second.name_) != 0) - ret.push_back(i.second.name_); - } + ret.insert(i.second.name_); return ret; } @@ -297,11 +293,11 @@ getHandler(unsigned version, bool betaEnabled, std::string const& name) return HandlerTable::instance().getHandler(version, betaEnabled, name); } -std::vector +std::set getHandlerNames() { return HandlerTable::instance().getHandlerNames(); -}; +} } // namespace RPC } // namespace ripple diff --git a/src/ripple/rpc/impl/Handler.h b/src/ripple/rpc/impl/Handler.h index 9c4008bb6e7..d7ce679d7b9 100644 --- a/src/ripple/rpc/impl/Handler.h +++ b/src/ripple/rpc/impl/Handler.h @@ -74,7 +74,7 @@ makeObjectValue( } /** Return names of all methods. */ -std::vector +std::set getHandlerNames(); template diff --git a/src/test/basics/PerfLog_test.cpp b/src/test/basics/PerfLog_test.cpp index 79944e0ed71..50587c33382 100644 --- a/src/test/basics/PerfLog_test.cpp +++ b/src/test/basics/PerfLog_test.cpp @@ -309,7 +309,9 @@ class PerfLog_test : public beast::unit_test::suite // Get the all the labels we can use for RPC interfaces without // causing an assert. - std::vector labels{ripple::RPC::getHandlerNames()}; + std::set handlerNames = ripple::RPC::getHandlerNames(); + std::vector labels( + handlerNames.begin(), handlerNames.end()); std::shuffle(labels.begin(), labels.end(), default_prng()); // Get two IDs to associate with each label. Errors tend to happen at diff --git a/src/test/rpc/Handler_test.cpp b/src/test/rpc/Handler_test.cpp index c8ee2cac96f..4b1d8238978 100644 --- a/src/test/rpc/Handler_test.cpp +++ b/src/test/rpc/Handler_test.cpp @@ -83,7 +83,10 @@ class Handler_test : public beast::unit_test::suite std::random_device dev; std::ranlux48 prng(dev()); - auto const names = getHandlerNames(); + std::set handlerNames = getHandlerNames(); + std::vector names( + handlerNames.begin(), handlerNames.end()); + std::uniform_int_distribution distr{0, names.size() - 1}; std::size_t dummy = 0; From 6de4b6c175d80880901f7f95c9f05421152adfb7 Mon Sep 17 00:00:00 2001 From: Bronek Kozicki Date: Tue, 17 Oct 2023 15:38:37 +0100 Subject: [PATCH 08/13] Minor improvements Use `LogicError` instead of `assert`; increase test batch to 100 samples and show time units (temporary workaround until we get a better compiler); remove redundant `include` and `const` --- src/ripple/rpc/handlers/LedgerHandler.h | 2 +- src/ripple/rpc/impl/Handler.cpp | 25 +++++++++++------ src/test/rpc/Handler_test.cpp | 36 ++++++++++++++++++------- 3 files changed, 44 insertions(+), 19 deletions(-) diff --git a/src/ripple/rpc/handlers/LedgerHandler.h b/src/ripple/rpc/handlers/LedgerHandler.h index efa54187b76..5c33c1392a4 100644 --- a/src/ripple/rpc/handlers/LedgerHandler.h +++ b/src/ripple/rpc/handlers/LedgerHandler.h @@ -59,7 +59,7 @@ class LedgerHandler void writeResult(Object&); - static constexpr char const name[] = "ledger"; + static constexpr char name[] = "ledger"; static constexpr unsigned minApiVer = 1; diff --git a/src/ripple/rpc/impl/Handler.cpp b/src/ripple/rpc/impl/Handler.cpp index 590a1a3de9b..e17a7e0f226 100644 --- a/src/ripple/rpc/impl/Handler.cpp +++ b/src/ripple/rpc/impl/Handler.cpp @@ -17,12 +17,15 @@ */ //============================================================================== +#include #include #include #include #include +#include #include +#include namespace ripple { namespace RPC { @@ -216,10 +219,13 @@ class HandlerTable { for (auto const& entry : entries) { - assert(!overlappingApiVersion( - table_.equal_range(entry.name_), - entry.minApiVer_, - entry.maxApiVer_)); + if (overlappingApiVersion( + table_.equal_range(entry.name_), + entry.minApiVer_, + entry.maxApiVer_)) + LogicError( + std::string("Handler for ") + entry.name_ + + " overlaps with an existing handler"); table_.insert({entry.name_, entry}); } @@ -276,10 +282,13 @@ class HandlerTable static_assert(HandlerImpl::minApiVer <= HandlerImpl::maxApiVer); static_assert(HandlerImpl::maxApiVer <= RPC::apiMaximumValidVersion); - assert(!overlappingApiVersion( - table_.equal_range(HandlerImpl::name), - HandlerImpl::minApiVer, - HandlerImpl::maxApiVer)); + if (overlappingApiVersion( + table_.equal_range(HandlerImpl::name), + HandlerImpl::minApiVer, + HandlerImpl::maxApiVer)) + LogicError( + std::string("Handler for ") + HandlerImpl::name + + " overlaps with an existing handler"); table_.insert({HandlerImpl::name, handlerFrom()}); } diff --git a/src/test/rpc/Handler_test.cpp b/src/test/rpc/Handler_test.cpp index 4b1d8238978..78a64f983a5 100644 --- a/src/test/rpc/Handler_test.cpp +++ b/src/test/rpc/Handler_test.cpp @@ -29,6 +29,16 @@ namespace ripple::RPC { +// NOTE: there should be no need for this function; +// `std::cout << some_duration` should just work if built with a compliant +// C++20 compiler. Sadly, we are not using one, as of today +// TODO: remove this operator<< overload when we bump compiler version +std::ostream& +operator<<(std::ostream& os, std::chrono::nanoseconds ns) +{ + return (os << ns.count() << "ns"); +} + // NOTE This is a rather naiive effort at a microbenchmark. Ideally we want // Google Benchmark, or something similar. Also, this actually does not belong // to unit tests, as it makes little sense to run it in conditions very @@ -37,25 +47,27 @@ namespace ripple::RPC { class Handler_test : public beast::unit_test::suite { - std::tuple - time(std::size_t n, auto f, auto prng) + auto + time(std::size_t n, auto f, auto prng) -> auto { + using clock = std::chrono::steady_clock; assert(n > 0); double sum = 0; double sum_squared = 0; std::size_t j = 0; while (j < n) { - // Generate 20 inputs upfront, separated from the inner loop - std::array inputs = {}; + // Generate 100 inputs upfront, separated from the inner loop + std::array inputs = {}; for (auto& i : inputs) { i = prng(); } - // Take 20 samples and throw away 7 from each end, using middle 6 - std::array samples = {}; - for (std::size_t k = 0; k < 20; ++k) + // Take 100 samples, then sort and throw away 35 from each end, + // using only middle 30. This helps to reduce measurement noise. + std::array samples = {}; + for (std::size_t k = 0; k < 100; ++k) { auto start = std::chrono::steady_clock::now(); f(inputs[k]); @@ -63,7 +75,7 @@ class Handler_test : public beast::unit_test::suite } std::sort(samples.begin(), samples.end()); - for (std::size_t k = 7; k < 13; ++k) + for (std::size_t k = 35; k < 65; ++k) { j += 1; sum += samples[k]; @@ -71,8 +83,12 @@ class Handler_test : public beast::unit_test::suite } } - double const mean = sum / j; - return {mean, std::sqrt((sum_squared / j) - (mean * mean)), j}; + const double mean_squared = (sum * sum) / (j * j); + return std::make_tuple( + clock::duration{static_cast(sum / j)}, + clock::duration{ + static_cast(std::sqrt((sum_squared / j) - mean_squared))}, + j); } void From 5b845b6590799d09273d84c5ecf2a55f897b7284 Mon Sep 17 00:00:00 2001 From: Bronek Kozicki Date: Wed, 18 Oct 2023 11:40:11 +0100 Subject: [PATCH 09/13] More minor improvements --- src/ripple/rpc/impl/Handler.cpp | 13 +++++++------ src/test/rpc/Handler_test.cpp | 2 +- src/test/rpc/LedgerHeader_test.cpp | 1 - 3 files changed, 8 insertions(+), 8 deletions(-) diff --git a/src/ripple/rpc/impl/Handler.cpp b/src/ripple/rpc/impl/Handler.cpp index e17a7e0f226..a3fc6efb46c 100644 --- a/src/ripple/rpc/impl/Handler.cpp +++ b/src/ripple/rpc/impl/Handler.cpp @@ -23,9 +23,7 @@ #include #include -#include #include -#include namespace ripple { namespace RPC { @@ -52,6 +50,9 @@ template Status handle(JsonContext& context, Object& object) { + assert( + context.apiVersion >= HandlerImpl.minApiVer && + context.apiVersion <= HandlerImpl.maxApiVer); HandlerImpl handler(context); auto status = handler.check(); @@ -60,7 +61,7 @@ handle(JsonContext& context, Object& object) else handler.writeResult(object); return status; -}; +} template Handler @@ -196,7 +197,7 @@ class HandlerTable // Use with equal_range to enforce that API range of a newly added handler // does not overlap with API range of an existing handler with same name - bool + [[nodiscard]] bool overlappingApiVersion( std::pair range, unsigned minVer, @@ -243,7 +244,7 @@ class HandlerTable return handlerTable; } - Handler const* + [[nodiscard]] Handler const* getHandler(unsigned version, bool betaEnabled, std::string const& name) const { @@ -262,7 +263,7 @@ class HandlerTable return i == range.second ? nullptr : &i->second; } - std::set + [[nodiscard]] std::set getHandlerNames() const { std::set ret; diff --git a/src/test/rpc/Handler_test.cpp b/src/test/rpc/Handler_test.cpp index 78a64f983a5..16a87d949a5 100644 --- a/src/test/rpc/Handler_test.cpp +++ b/src/test/rpc/Handler_test.cpp @@ -130,4 +130,4 @@ class Handler_test : public beast::unit_test::suite BEAST_DEFINE_TESTSUITE(Handler, rpc, ripple); -} // namespace ripple::RPC \ No newline at end of file +} // namespace ripple::RPC diff --git a/src/test/rpc/LedgerHeader_test.cpp b/src/test/rpc/LedgerHeader_test.cpp index 11151311747..d6c0652d5a2 100644 --- a/src/test/rpc/LedgerHeader_test.cpp +++ b/src/test/rpc/LedgerHeader_test.cpp @@ -18,7 +18,6 @@ //============================================================================== #include -#include #include #include From d934db025bad82866cfde3b42fb9b4ebdfc341b7 Mon Sep 17 00:00:00 2001 From: Bronek Kozicki Date: Wed, 18 Oct 2023 21:54:40 +0100 Subject: [PATCH 10/13] Fix assert compilation error --- src/ripple/rpc/impl/Handler.cpp | 4 ++-- 1 file changed, 2 insertions(+), 2 deletions(-) diff --git a/src/ripple/rpc/impl/Handler.cpp b/src/ripple/rpc/impl/Handler.cpp index a3fc6efb46c..98f4a533d4d 100644 --- a/src/ripple/rpc/impl/Handler.cpp +++ b/src/ripple/rpc/impl/Handler.cpp @@ -51,8 +51,8 @@ Status handle(JsonContext& context, Object& object) { assert( - context.apiVersion >= HandlerImpl.minApiVer && - context.apiVersion <= HandlerImpl.maxApiVer); + context.apiVersion >= HandlerImpl::minApiVer && + context.apiVersion <= HandlerImpl::maxApiVer); HandlerImpl handler(context); auto status = handler.check(); From 033629abd7610f53f3b78e4bd430abf4421bf6d5 Mon Sep 17 00:00:00 2001 From: Bronek Kozicki Date: Fri, 20 Oct 2023 22:38:04 +0100 Subject: [PATCH 11/13] Minor improvements --- src/ripple/rpc/handlers/LedgerHandler.h | 2 +- src/ripple/rpc/handlers/Version.h | 2 +- src/ripple/rpc/impl/Handler.cpp | 18 ++++++++++-------- src/test/rpc/Handler_test.cpp | 12 ++++++------ 4 files changed, 18 insertions(+), 16 deletions(-) diff --git a/src/ripple/rpc/handlers/LedgerHandler.h b/src/ripple/rpc/handlers/LedgerHandler.h index 5c33c1392a4..b0bca8e6635 100644 --- a/src/ripple/rpc/handlers/LedgerHandler.h +++ b/src/ripple/rpc/handlers/LedgerHandler.h @@ -61,7 +61,7 @@ class LedgerHandler static constexpr char name[] = "ledger"; - static constexpr unsigned minApiVer = 1; + static constexpr unsigned minApiVer = RPC::apiMinimumSupportedVersion; static constexpr unsigned maxApiVer = RPC::apiMaximumValidVersion; diff --git a/src/ripple/rpc/handlers/Version.h b/src/ripple/rpc/handlers/Version.h index 69578727fab..8f33b62f1cf 100644 --- a/src/ripple/rpc/handlers/Version.h +++ b/src/ripple/rpc/handlers/Version.h @@ -48,7 +48,7 @@ class VersionHandler static constexpr char const* name = "version"; - static constexpr unsigned minApiVer = 1; + static constexpr unsigned minApiVer = RPC::apiMinimumSupportedVersion; static constexpr unsigned maxApiVer = RPC::apiMaximumValidVersion; diff --git a/src/ripple/rpc/impl/Handler.cpp b/src/ripple/rpc/impl/Handler.cpp index 98f4a533d4d..c31619efbf8 100644 --- a/src/ripple/rpc/impl/Handler.cpp +++ b/src/ripple/rpc/impl/Handler.cpp @@ -206,13 +206,13 @@ class HandlerTable assert(minVer <= maxVer); assert(maxVer <= RPC::apiMaximumValidVersion); - for (; range.first != range.second; range.first++) - { - if (range.first->second.minApiVer_ <= maxVer && - range.first->second.maxApiVer_ >= minVer) - return true; - } - return false; + return std::any_of( + range.first, + range.second, // + [minVer, maxVer](auto const& item) { + return item.second.minApiVer_ <= maxVer && + item.second.maxApiVer_ >= minVer; + }); } template @@ -256,7 +256,7 @@ class HandlerTable auto const range = table_.equal_range(name); auto const i = std::find_if( range.first, range.second, [version](auto const& entry) { - return version >= entry.second.minApiVer_ && + return entry.second.minApiVer_ <= version && version <= entry.second.maxApiVer_; }); @@ -282,6 +282,8 @@ class HandlerTable { static_assert(HandlerImpl::minApiVer <= HandlerImpl::maxApiVer); static_assert(HandlerImpl::maxApiVer <= RPC::apiMaximumValidVersion); + static_assert( + RPC::apiMinimumSupportedVersion <= HandlerImpl::minApiVer); if (overlappingApiVersion( table_.equal_range(HandlerImpl::name), diff --git a/src/test/rpc/Handler_test.cpp b/src/test/rpc/Handler_test.cpp index 16a87d949a5..ba26a27c806 100644 --- a/src/test/rpc/Handler_test.cpp +++ b/src/test/rpc/Handler_test.cpp @@ -27,7 +27,7 @@ #include #include -namespace ripple::RPC { +namespace ripple::test { // NOTE: there should be no need for this function; // `std::cout << some_duration` should just work if built with a compliant @@ -39,7 +39,7 @@ operator<<(std::ostream& os, std::chrono::nanoseconds ns) return (os << ns.count() << "ns"); } -// NOTE This is a rather naiive effort at a microbenchmark. Ideally we want +// NOTE This is a rather naive effort at a microbenchmark. Ideally we want // Google Benchmark, or something similar. Also, this actually does not belong // to unit tests, as it makes little sense to run it in conditions very // dissimilar to how rippled will normally work. @@ -99,7 +99,7 @@ class Handler_test : public beast::unit_test::suite std::random_device dev; std::ranlux48 prng(dev()); - std::set handlerNames = getHandlerNames(); + std::set handlerNames = RPC::getHandlerNames(); std::vector names( handlerNames.begin(), handlerNames.end()); @@ -109,7 +109,7 @@ class Handler_test : public beast::unit_test::suite auto const [mean, stdev, n] = time( 1'000'000, [&](std::size_t i) { - auto const d = getHandler(1, false, names[i]); + auto const d = RPC::getHandler(1, false, names[i]); dummy = dummy + i + (int)d->role_; }, [&]() -> std::size_t { return distr(prng); }); @@ -128,6 +128,6 @@ class Handler_test : public beast::unit_test::suite } }; -BEAST_DEFINE_TESTSUITE(Handler, rpc, ripple); +BEAST_DEFINE_TESTSUITE_MANUAL(Handler, test, ripple); -} // namespace ripple::RPC +} // namespace ripple::test From dff81138d2da8ccaf04a8228be51bfc1538f0b61 Mon Sep 17 00:00:00 2001 From: Bronek Kozicki Date: Mon, 23 Oct 2023 14:08:00 +0000 Subject: [PATCH 12/13] Add make_vector to tests --- src/test/basics/PerfLog_test.cpp | 6 +++--- src/test/jtx/TestHelpers.h | 9 +++++++++ src/test/rpc/Handler_test.cpp | 5 ++--- 3 files changed, 14 insertions(+), 6 deletions(-) diff --git a/src/test/basics/PerfLog_test.cpp b/src/test/basics/PerfLog_test.cpp index 50587c33382..f0a6645195b 100644 --- a/src/test/basics/PerfLog_test.cpp +++ b/src/test/basics/PerfLog_test.cpp @@ -25,6 +25,7 @@ #include #include #include +#include #include #include @@ -309,9 +310,8 @@ class PerfLog_test : public beast::unit_test::suite // Get the all the labels we can use for RPC interfaces without // causing an assert. - std::set handlerNames = ripple::RPC::getHandlerNames(); - std::vector labels( - handlerNames.begin(), handlerNames.end()); + std::vector labels = + test::jtx::make_vector(ripple::RPC::getHandlerNames()); std::shuffle(labels.begin(), labels.end(), default_prng()); // Get two IDs to associate with each label. Errors tend to happen at diff --git a/src/test/jtx/TestHelpers.h b/src/test/jtx/TestHelpers.h index ff681ffa50b..2bee47a6411 100644 --- a/src/test/jtx/TestHelpers.h +++ b/src/test/jtx/TestHelpers.h @@ -27,10 +27,19 @@ #include #include +#include + namespace ripple { namespace test { namespace jtx { +// Helper to make vector from iterable +auto +make_vector(auto const& input) requires std::ranges::range +{ + return std::vector(std::ranges::begin(input), std::ranges::end(input)); +} + // Functions used in debugging Json::Value getAccountOffers(Env& env, AccountID const& acct, bool current = false); diff --git a/src/test/rpc/Handler_test.cpp b/src/test/rpc/Handler_test.cpp index ba26a27c806..5160a68aac2 100644 --- a/src/test/rpc/Handler_test.cpp +++ b/src/test/rpc/Handler_test.cpp @@ -99,9 +99,8 @@ class Handler_test : public beast::unit_test::suite std::random_device dev; std::ranlux48 prng(dev()); - std::set handlerNames = RPC::getHandlerNames(); - std::vector names( - handlerNames.begin(), handlerNames.end()); + std::vector names = + test::jtx::make_vector(ripple::RPC::getHandlerNames()); std::uniform_int_distribution distr{0, names.size() - 1}; From 6f7ad0cfc3abf4b0e999b04061c8e81f7d02ccda Mon Sep 17 00:00:00 2001 From: Bronek Kozicki Date: Mon, 23 Oct 2023 18:58:11 +0000 Subject: [PATCH 13/13] Minor improvement --- src/ripple/rpc/impl/Handler.h | 2 +- 1 file changed, 1 insertion(+), 1 deletion(-) diff --git a/src/ripple/rpc/impl/Handler.h b/src/ripple/rpc/impl/Handler.h index d7ce679d7b9..28d1ee60c85 100644 --- a/src/ripple/rpc/impl/Handler.h +++ b/src/ripple/rpc/impl/Handler.h @@ -54,7 +54,7 @@ struct Handler Role role_; RPC::Condition condition_; - unsigned minApiVer_ = 1; + unsigned minApiVer_ = apiMinimumSupportedVersion; unsigned maxApiVer_ = apiMaximumValidVersion; };