From 0a3ce6cf360af45a0c76cc01c620a5690c466597 Mon Sep 17 00:00:00 2001 From: Bronek Kozicki Date: Tue, 24 Oct 2023 23:57:49 +0100 Subject: [PATCH] APIv2: remove tx_history and ledger_header (#4759) Remove `tx_history` and `ledger_header` methods from API version 2. Update `RPC::Handler` to allow for methods (or method implementations) to be API version specific. This partially resolves #4727. We can now store multiple handlers with the same name, as long as they belong to different (non-overlapping) API versions. This necessarily impacts the handler lookup algorithm and its complexity; however, there is no performance loss on x86_64 architecture, and only minimal performance loss on arm64 (around 10ns). This design change gives us extra flexibility evolving the API in the future, including other parts of In API version 2, `tx_history` and `ledger_header` are no longer recognised; if they are called, `rippled` will return error `unknownCmd` Resolve #3638 Resolve #3539 --- Builds/CMake/RippledCore.cmake | 2 + src/ripple/perflog/impl/PerfLogImp.cpp | 2 +- src/ripple/perflog/impl/PerfLogImp.h | 4 +- src/ripple/rpc/handlers/LedgerHandler.h | 23 ++-- src/ripple/rpc/handlers/Version.h | 22 ++-- src/ripple/rpc/impl/Handler.cpp | 107 +++++++++++++----- src/ripple/rpc/impl/Handler.h | 6 +- src/ripple/rpc/impl/RPCHelpers.h | 2 + src/test/basics/PerfLog_test.cpp | 4 +- src/test/jtx/TestHelpers.h | 9 ++ src/test/rpc/Handler_test.cpp | 132 +++++++++++++++++++++++ src/test/rpc/LedgerHeader_test.cpp | 91 ++++++++++++++++ src/test/rpc/TransactionHistory_test.cpp | 16 +++ 13 files changed, 360 insertions(+), 60 deletions(-) create mode 100644 src/test/rpc/Handler_test.cpp create mode 100644 src/test/rpc/LedgerHeader_test.cpp diff --git a/Builds/CMake/RippledCore.cmake b/Builds/CMake/RippledCore.cmake index 53823d159..4eb3fbe4e 100644 --- a/Builds/CMake/RippledCore.cmake +++ b/Builds/CMake/RippledCore.cmake @@ -1114,6 +1114,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 @@ -1138,6 +1139,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/ripple/perflog/impl/PerfLogImp.cpp b/src/ripple/perflog/impl/PerfLogImp.cpp index db5a188fc..3d07d0ed6 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 493c1dc1a..4904126d9 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/LedgerHandler.h b/src/ripple/rpc/handlers/LedgerHandler.h index 77b361d34..b0bca8e66 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 @@ public: void writeResult(Object&); - static char const* - name() - { - return "ledger"; - } + static constexpr char name[] = "ledger"; - static Role - role() - { - return Role::USER; - } + static constexpr unsigned minApiVer = RPC::apiMinimumSupportedVersion; - 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 a9f42b949..8f33b62f1 100644 --- a/src/ripple/rpc/handlers/Version.h +++ b/src/ripple/rpc/handlers/Version.h @@ -46,23 +46,15 @@ public: 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 = RPC::apiMinimumSupportedVersion; - 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 ba3979447..d0de5beaf 100644 --- a/src/ripple/rpc/impl/Handler.cpp +++ b/src/ripple/rpc/impl/Handler.cpp @@ -17,11 +17,14 @@ */ //============================================================================== +#include #include #include #include #include +#include + namespace ripple { namespace RPC { namespace { @@ -47,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(); @@ -55,7 +61,20 @@ handle(JsonContext& context, Object& object) else handler.writeResult(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() @@ -110,7 +129,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}, @@ -158,7 +177,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", @@ -183,14 +202,42 @@ 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 + [[nodiscard]] bool + overlappingApiVersion( + std::pair range, + unsigned minVer, + unsigned maxVer) + { + assert(minVer <= maxVer); + assert(maxVer <= RPC::apiMaximumValidVersion); + + return std::any_of( + range.first, + range.second, // + [minVer, maxVer](auto const& item) { + return item.second.minApiVer_ <= maxVer && + item.second.maxApiVer_ >= minVer; + }); + } + 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; + 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}); } // This is where the new-style handlers are added. @@ -206,7 +253,7 @@ public: return handlerTable; } - Handler const* + [[nodiscard]] Handler const* getHandler(unsigned version, bool betaEnabled, std::string const& name) const { @@ -214,36 +261,48 @@ public: 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 entry.second.minApiVer_ <= version && + version <= entry.second.maxApiVer_; + }); + + return i == range.second ? nullptr : &i->second; } - std::vector + [[nodiscard]] std::set getHandlerNames() const { - std::vector ret; - ret.reserve(table_.size()); + std::set ret; for (auto const& i : table_) - ret.push_back(i.second.name_); + ret.insert(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); + static_assert( + RPC::apiMinimumSupportedVersion <= HandlerImpl::minApiVer); - Handler h; - h.name_ = HandlerImpl::name(); - h.valueMethod_ = &handle; - h.role_ = HandlerImpl::role(); - h.condition_ = HandlerImpl::condition(); + if (overlappingApiVersion( + table_.equal_range(HandlerImpl::name), + HandlerImpl::minApiVer, + HandlerImpl::maxApiVer)) + LogicError( + std::string("Handler for ") + HandlerImpl::name + + " overlaps with an existing handler"); - table_[HandlerImpl::name()] = h; + table_.insert({HandlerImpl::name, handlerFrom()}); } }; @@ -255,11 +314,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 e2188ef51..28d1ee60c 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_ = apiMinimumSupportedVersion; + unsigned maxApiVer_ = apiMaximumValidVersion; }; Handler const* @@ -70,7 +74,7 @@ makeObjectValue( } /** Return names of all methods. */ -std::vector +std::set getHandlerNames(); template diff --git a/src/ripple/rpc/impl/RPCHelpers.h b/src/ripple/rpc/impl/RPCHelpers.h index ad3cc218d..640a224b5 100644 --- a/src/ripple/rpc/impl/RPCHelpers.h +++ b/src/ripple/rpc/impl/RPCHelpers.h @@ -259,10 +259,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/basics/PerfLog_test.cpp b/src/test/basics/PerfLog_test.cpp index 79944e0ed..f0a664519 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,7 +310,8 @@ public: // Get the all the labels we can use for RPC interfaces without // causing an assert. - std::vector labels{ripple::RPC::getHandlerNames()}; + 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 74c6cbbaa..2703fc9f1 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 new file mode 100644 index 000000000..5160a68aa --- /dev/null +++ b/src/test/rpc/Handler_test.cpp @@ -0,0 +1,132 @@ +//------------------------------------------------------------------------------ +/* + 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 +#include +#include +#include +#include + +namespace ripple::test { + +// 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 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. +// TODO as https://github.com/XRPLF/rippled/issues/4765 + +class Handler_test : public beast::unit_test::suite +{ + 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 100 inputs upfront, separated from the inner loop + std::array inputs = {}; + for (auto& i : inputs) + { + i = prng(); + } + + // 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]); + samples[k] = (std::chrono::steady_clock::now() - start).count(); + } + + std::sort(samples.begin(), samples.end()); + for (std::size_t k = 35; k < 65; ++k) + { + j += 1; + sum += samples[k]; + sum_squared += (samples[k] * samples[k]); + } + } + + 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 + reportLookupPerformance() + { + testcase("Handler lookup performance"); + + std::random_device dev; + std::ranlux48 prng(dev()); + + std::vector names = + test::jtx::make_vector(ripple::RPC::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 = RPC::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_MANUAL(Handler, test, ripple); + +} // namespace ripple::test diff --git a/src/test/rpc/LedgerHeader_test.cpp b/src/test/rpc/LedgerHeader_test.cpp new file mode 100644 index 000000000..d6c0652d5 --- /dev/null +++ b/src/test/rpc/LedgerHeader_test.cpp @@ -0,0 +1,91 @@ +//------------------------------------------------------------------------------ +/* + 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 + +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 diff --git a/src/test/rpc/TransactionHistory_test.cpp b/src/test/rpc/TransactionHistory_test.cpp index 3f9b57927..862eaaee5 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 @@ public: { testBadInput(); testRequest(); + testCommandRetired(); } };