From 1eac4d2c07eaac04178ff53c7288bc46c6ce7da0 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 #4727. 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 --- API-CHANGELOG.md | 7 ++ 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 +++ 14 files changed, 367 insertions(+), 60 deletions(-) create mode 100644 src/test/rpc/Handler_test.cpp create mode 100644 src/test/rpc/LedgerHeader_test.cpp diff --git a/API-CHANGELOG.md b/API-CHANGELOG.md index 968a03c0784..b3c9b18d2f8 100644 --- a/API-CHANGELOG.md +++ b/API-CHANGELOG.md @@ -134,6 +134,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. +#### Removed methods + +In API version 2, the following methods are no longer available: + +- `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 - `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) diff --git a/Builds/CMake/RippledCore.cmake b/Builds/CMake/RippledCore.cmake index 4758d0791e8..269c107ca9e 100644 --- a/Builds/CMake/RippledCore.cmake +++ b/Builds/CMake/RippledCore.cmake @@ -1062,6 +1062,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 @@ -1085,6 +1086,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 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/LedgerHandler.h b/src/ripple/rpc/handlers/LedgerHandler.h index 77b361d3466..b0bca8e6635 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 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 a9f42b94993..8f33b62f1cf 100644 --- a/src/ripple/rpc/handlers/Version.h +++ b/src/ripple/rpc/handlers/Version.h @@ -46,23 +46,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 = 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 b69d2608b0e..d05c3279800 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}, @@ -156,7 +175,7 @@ Handler const handlerArray[]{ NEEDS_CURRENT_LEDGER}, {"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", @@ -178,14 +197,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. @@ -201,7 +248,7 @@ class HandlerTable return handlerTable; } - Handler const* + [[nodiscard]] Handler const* getHandler(unsigned version, bool betaEnabled, std::string const& name) const { @@ -209,36 +256,48 @@ 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 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()}); } }; @@ -250,11 +309,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 e2188ef51e7..28d1ee60c85 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 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/basics/PerfLog_test.cpp b/src/test/basics/PerfLog_test.cpp index 79944e0ed71..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,7 +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::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 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 new file mode 100644 index 00000000000..5160a68aac2 --- /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 00000000000..d6c0652d5a2 --- /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 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(); } };