APIv2: remove tx_history and ledger_header (XRPLF#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 XRPLF#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 XRPLF#4727. In API version 2, `tx_history` and `ledger_header` are no longer recognised; if they are called, `rippled` will return error `unknownCmd` Resolve XRPLF#3638 Resolve XRPLF#3539
gregtatcam · Oct 24, 2023 · 1eac4d2 · 1eac4d2
1 parent 3e5f770
commit 1eac4d2
Show file tree

Hide file tree

Showing 14 changed files with 367 additions and 60 deletions.
diff --git 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
@@ -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
@@ -43,7 +43,7 @@ namespace ripple {
 namespace perf {
 
 PerfLogImp::Counters::Counters(
-    std::vector<char const*> const& labels,
+    std::set<char const*> const& labels,
     JobTypes const& jobTypes)
 {
     {

diff --git 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<std::uint64_t, MethodStart> methods_;
         mutable std::mutex methodsMutex_;
 
-        Counters(
-            std::vector<char const*> const& labels,
-            JobTypes const& jobTypes);
+        Counters(std::set<char const*> 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
@@ -30,6 +30,7 @@
 #include <ripple/rpc/Role.h>
 #include <ripple/rpc/Status.h>
 #include <ripple/rpc/impl/Handler.h>
+#include <ripple/rpc/impl/RPCHelpers.h>
 
 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
@@ -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
@@ -17,11 +17,14 @@
 */
 //==============================================================================
 
+#include <ripple/basics/contract.h>
 #include <ripple/rpc/handlers/Handlers.h>
 #include <ripple/rpc/handlers/Version.h>
 #include <ripple/rpc/impl/Handler.h>
 #include <ripple/rpc/impl/RPCHelpers.h>
 
+#include <map>
+
 namespace ripple {
 namespace RPC {
 namespace {
@@ -47,6 +50,9 @@ template <class Object, class HandlerImpl>
 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 <typename HandlerImpl>
+Handler
+handlerFrom()
+{
+    return {
+        HandlerImpl::name,
+        &handle<Json::Value, HandlerImpl>,
+        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<std::string, Handler>;
+
+    // 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<handler_table_t::iterator, handler_table_t::iterator> 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 <std::size_t N>
     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,44 +248,56 @@ class HandlerTable
         return handlerTable;
     }
 
-    Handler const*
+    [[nodiscard]] Handler const*
     getHandler(unsigned version, bool betaEnabled, std::string const& name)
         const
     {
         if (version < RPC::apiMinimumSupportedVersion ||
             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<char const*>
+    [[nodiscard]] std::set<char const*>
     getHandlerNames() const
     {
-        std::vector<char const*> ret;
-        ret.reserve(table_.size());
+        std::set<char const*> ret;
         for (auto const& i : table_)
-            ret.push_back(i.second.name_);
+            ret.insert(i.second.name_);
+
         return ret;
     }
 
 private:
-    std::map<std::string, Handler> table_;
+    handler_table_t table_;
 
     template <class HandlerImpl>
     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<Json::Value, HandlerImpl>;
-        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<HandlerImpl>()});
     }
 };
 
@@ -250,11 +309,11 @@ getHandler(unsigned version, bool betaEnabled, std::string const& name)
     return HandlerTable::instance().getHandler(version, betaEnabled, name);
 }
 
-std::vector<char const*>
+std::set<char const*>
 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
@@ -25,6 +25,7 @@
 #include <ripple/core/Config.h>
 #include <ripple/rpc/RPCHandler.h>
 #include <ripple/rpc/Status.h>
+#include <ripple/rpc/impl/RPCHelpers.h>
 #include <ripple/rpc/impl/Tuning.h>
 #include <vector>
 
@@ -52,6 +53,9 @@ struct Handler
     Method<Json::Value> 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<char const*>
+std::set<char const*>
 getHandlerNames();
 
 template <class T>

diff --git 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 <class Object>
 void

diff --git a/src/test/basics/PerfLog_test.cpp b/src/test/basics/PerfLog_test.cpp
@@ -25,6 +25,7 @@
 #include <ripple/protocol/jss.h>
 #include <ripple/rpc/impl/Handler.h>
 #include <test/jtx/Env.h>
+#include <test/jtx/TestHelpers.h>
 
 #include <atomic>
 #include <chrono>
@@ -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<char const*> labels{ripple::RPC::getHandlerNames()};
+        std::vector<char const*> 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
@@ -27,10 +27,19 @@
 #include <ripple/protocol/jss.h>
 #include <test/jtx/Env.h>
 
+#include <ranges>
+
 namespace ripple {
 namespace test {
 namespace jtx {
 
+// Helper to make vector from iterable
+auto
+make_vector(auto const& input) requires std::ranges::range<decltype(input)>
+{
+    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);