feat: More verbose forwarding errors (#1523)

Fixes #1516.

src/etl/ETLState.hpp

@@ -23,6 +23,7 @@
 
 #include <boost/json.hpp>
 #include <boost/json/conversion.hpp>
+#include <boost/json/object.hpp>
 #include <boost/json/value.hpp>
 #include <boost/json/value_to.hpp>
 
@@ -46,8 +47,11 @@ struct ETLState {
     static std::optional<ETLState>
     fetchETLStateFromSource(Forward& source) noexcept
     {
-        auto const serverInfoRippled = data::synchronous([&source](auto yield) {
-            return source.forwardToRippled({{"command", "server_info"}}, std::nullopt, {}, yield);
+        auto const serverInfoRippled = data::synchronous([&source](auto yield) -> std::optional<boost::json::object> {
+            if (auto result = source.forwardToRippled({{"command", "server_info"}}, std::nullopt, {}, yield)) {
+                return std::move(result).value();
+            }
+            return std::nullopt;
         });
 
         if (serverInfoRippled)

src/etl/LoadBalancer.cpp

@@ -24,6 +24,7 @@
 #include "etl/NetworkValidatedLedgersInterface.hpp"
 #include "etl/Source.hpp"
 #include "feed/SubscriptionManagerInterface.hpp"
+#include "rpc/Errors.hpp"
 #include "util/Assert.hpp"
 #include "util/Random.hpp"
 #include "util/log/Logger.hpp"
@@ -39,6 +40,7 @@
 #include <chrono>
 #include <cstddef>
 #include <cstdint>
+#include <expected>
 #include <memory>
 #include <optional>
 #include <stdexcept>
@@ -211,7 +213,7 @@ LoadBalancer::fetchLedger(
     return response;
 }
 
-std::optional<boost::json::object>
+std::expected<boost::json::object, rpc::ClioError>
 LoadBalancer::forwardToRippled(
     boost::json::object const& request,
     std::optional<std::string> const& clientIp,
@@ -221,7 +223,7 @@ LoadBalancer::forwardToRippled(
 {
     if (forwardingCache_) {
         if (auto cachedResponse = forwardingCache_->get(request); cachedResponse) {
-            return cachedResponse;
+            return std::move(cachedResponse).value();
         }
     }
 
@@ -233,20 +235,26 @@ LoadBalancer::forwardToRippled(
     auto xUserValue = isAdmin ? ADMIN_FORWARDING_X_USER_VALUE : USER_FORWARDING_X_USER_VALUE;
 
     std::optional<boost::json::object> response;
+    rpc::ClioError error = rpc::ClioError::etlCONNECTION_ERROR;
     while (numAttempts < sources_.size()) {
-        if (auto res = sources_[sourceIdx]->forwardToRippled(request, clientIp, xUserValue, yield)) {
-            response = std::move(res);
+        auto res = sources_[sourceIdx]->forwardToRippled(request, clientIp, xUserValue, yield);
+        if (res) {
+            response = std::move(res).value();
             break;
         }
+        error = std::max(error, res.error());  // Choose the best result between all sources
 
         sourceIdx = (sourceIdx + 1) % sources_.size();
         ++numAttempts;
     }
 
-    if (response and forwardingCache_ and not response->contains("error"))
-        forwardingCache_->put(request, *response);
+    if (response) {
+        if (forwardingCache_ and not response->contains("error"))
+            forwardingCache_->put(request, *response);
+        return std::move(response).value();
+    }
 
-    return response;
+    return std::unexpected{error};
 }
 
 boost::json::value

src/etl/LoadBalancer.hpp

@@ -25,6 +25,7 @@
 #include "etl/Source.hpp"
 #include "etl/impl/ForwardingCache.hpp"
 #include "feed/SubscriptionManagerInterface.hpp"
+#include "rpc/Errors.hpp"
 #include "util/config/Config.hpp"
 #include "util/log/Logger.hpp"
 
@@ -41,6 +42,7 @@
 #include <atomic>
 #include <chrono>
 #include <cstdint>
+#include <expected>
 #include <memory>
 #include <optional>
 #include <string>
@@ -181,9 +183,9 @@ class LoadBalancer {
      * @param clientIp The IP address of the peer, if known
      * @param isAdmin Whether the request is from an admin
      * @param yield The coroutine context
-     * @return Response received from rippled node as JSON object on success; nullopt on failure
+     * @return Response received from rippled node as JSON object on success or error on failure
      */
-    std::optional<boost::json::object>
+    std::expected<boost::json::object, rpc::ClioError>
     forwardToRippled(
         boost::json::object const& request,
         std::optional<std::string> const& clientIp,

src/etl/Source.hpp

@@ -22,6 +22,7 @@
 #include "data/BackendInterface.hpp"
 #include "etl/NetworkValidatedLedgersInterface.hpp"
 #include "feed/SubscriptionManagerInterface.hpp"
+#include "rpc/Errors.hpp"
 #include "util/config/Config.hpp"
 #include "util/log/Logger.hpp"
 
@@ -34,6 +35,7 @@
 
 #include <chrono>
 #include <cstdint>
+#include <expected>
 #include <functional>
 #include <memory>
 #include <optional>
@@ -131,9 +133,9 @@ class SourceBase {
      * @param forwardToRippledClientIp IP of the client forwarding this request if known
      * @param xUserValue Value of the X-User header
      * @param yield The coroutine context
-     * @return Response wrapped in an optional on success; nullopt otherwise
+     * @return Response on success or error on failure
      */
-    virtual std::optional<boost::json::object>
+    virtual std::expected<boost::json::object, rpc::ClioError>
     forwardToRippled(
         boost::json::object const& request,
         std::optional<std::string> const& forwardToRippledClientIp,

src/etl/impl/ForwardingSource.cpp

@@ -19,6 +19,7 @@
 
 #include "etl/impl/ForwardingSource.hpp"
 
+#include "rpc/Errors.hpp"
 #include "util/log/Logger.hpp"
 
 #include <boost/asio/spawn.hpp>
@@ -55,7 +56,7 @@ ForwardingSource::ForwardingSource(
         );
 }
 
-std::optional<boost::json::object>
+std::expected<boost::json::object, rpc::ClioError>
 ForwardingSource::forwardToRippled(
     boost::json::object const& request,
     std::optional<std::string> const& forwardToRippledClientIp,
@@ -74,18 +75,26 @@ ForwardingSource::forwardToRippled(
 
     auto expectedConnection = connectionBuilder.connect(yield);
     if (not expectedConnection) {
-        return std::nullopt;
+        LOG(log_.debug()) << "Couldn't connect to rippled to forward request.";
+        return std::unexpected{rpc::ClioError::etlCONNECTION_ERROR};
     }
     auto& connection = expectedConnection.value();
 
     auto writeError = connection->write(boost::json::serialize(request), yield, forwardingTimeout_);
     if (writeError) {
-        return std::nullopt;
+        LOG(log_.debug()) << "Error sending request to rippled to forward request.";
+        return std::unexpected{rpc::ClioError::etlREQUEST_ERROR};
     }
 
     auto response = connection->read(yield, forwardingTimeout_);
     if (not response) {
-        return std::nullopt;
+        if (auto errorCode = response.error().errorCode();
+            errorCode.has_value() and errorCode->value() == boost::system::errc::timed_out) {
+            LOG(log_.debug()) << "Request to rippled timed out";
+            return std::unexpected{rpc::ClioError::etlREQUEST_TIMEOUT};
+        }
+        LOG(log_.debug()) << "Error sending request to rippled to forward request.";
+        return std::unexpected{rpc::ClioError::etlREQUEST_ERROR};
     }
 
     boost::json::value parsedResponse;
@@ -94,8 +103,8 @@ ForwardingSource::forwardToRippled(
         if (not parsedResponse.is_object())
             throw std::runtime_error("response is not an object");
     } catch (std::exception const& e) {
-        LOG(log_.error()) << "Error parsing response from rippled: " << e.what() << ". Response: " << *response;
-        return std::nullopt;
+        LOG(log_.debug()) << "Error parsing response from rippled: " << e.what() << ". Response: " << *response;
+        return std::unexpected{rpc::ClioError::etlINVALID_RESPONSE};
     }
 
     auto responseObject = parsedResponse.as_object();

src/etl/impl/ForwardingSource.hpp

@@ -19,13 +19,15 @@
 
 #pragma once
 
+#include "rpc/Errors.hpp"
 #include "util/log/Logger.hpp"
 #include "util/requests/WsConnection.hpp"
 
 #include <boost/asio/spawn.hpp>
 #include <boost/json/object.hpp>
 
 #include <chrono>
+#include <expected>
 #include <optional>
 #include <string>
 #include <string_view>
@@ -54,9 +56,9 @@ class ForwardingSource {
      * @param forwardToRippledClientIp IP of the client forwarding this request if known
      * @param xUserValue Optional value for X-User header
      * @param yield The coroutine context
-     * @return Response wrapped in an optional on success; nullopt otherwise
+     * @return Response on success or error on failure
      */
-    std::optional<boost::json::object>
+    std::expected<boost::json::object, rpc::ClioError>
     forwardToRippled(
         boost::json::object const& request,
         std::optional<std::string> const& forwardToRippledClientIp,

src/etl/impl/SourceImpl.hpp

@@ -23,6 +23,7 @@
 #include "etl/impl/ForwardingSource.hpp"
 #include "etl/impl/GrpcSource.hpp"
 #include "etl/impl/SubscriptionSource.hpp"
+#include "rpc/Errors.hpp"
 
 #include <boost/asio/spawn.hpp>
 #include <boost/json/object.hpp>
@@ -31,12 +32,14 @@
 
 #include <chrono>
 #include <cstdint>
+#include <expected>
 #include <memory>
 #include <optional>
 #include <string>
 #include <string_view>
 #include <utility>
 #include <vector>
+
 namespace etl::impl {
 
 /**
@@ -205,9 +208,9 @@ class SourceImpl : public SourceBase {
      * @param forwardToRippledClientIp IP of the client forwarding this request if known
      * @param xUserValue Optional value of the X-User header
      * @param yield The coroutine context
-     * @return Response wrapped in an optional on success; nullopt otherwise
+     * @return Response or ClioError
      */
-    std::optional<boost::json::object>
+    std::expected<boost::json::object, rpc::ClioError>
     forwardToRippled(
         boost::json::object const& request,
         std::optional<std::string> const& forwardToRippledClientIp,

src/rpc/Errors.cpp

@@ -99,6 +99,11 @@ getErrorInfo(ClioError code)
         {ClioError::rpcCOMMAND_NOT_STRING, "commandNotString", "Method is not a string."},
         {ClioError::rpcCOMMAND_IS_EMPTY, "emptyCommand", "Method is an empty string."},
         {ClioError::rpcPARAMS_UNPARSEABLE, "paramsUnparseable", "Params must be an array holding exactly one object."},
+        // etl related errors
+        {ClioError::etlCONNECTION_ERROR, "connectionError", "Couldn't connect to rippled."},
+        {ClioError::etlREQUEST_ERROR, "requestError", "Error sending request to rippled."},
+        {ClioError::etlREQUEST_TIMEOUT, "timeout", "Request to rippled timed out."},
+        {ClioError::etlINVALID_RESPONSE, "invalidResponse", "Rippled returned an invalid response."}
     };
 
     auto matchByCode = [code](auto const& info) { return info.code == code; };

src/rpc/Errors.hpp

@@ -50,6 +50,14 @@ enum class ClioError {
     rpcCOMMAND_NOT_STRING = 6002,
     rpcCOMMAND_IS_EMPTY = 6003,
     rpcPARAMS_UNPARSEABLE = 6004,
+
+    // TODO: Since it is not only rpc errors here now, we should move it to util
+    // etl related errors start with 7000
+    // Higher value in this errors means better progress in the forwarding
+    etlCONNECTION_ERROR = 7000,
+    etlREQUEST_ERROR = 7001,
+    etlREQUEST_TIMEOUT = 7002,
+    etlINVALID_RESPONSE = 7003,
 };
 
 /** @brief Holds info about a particular @ref ClioError. */

src/rpc/common/impl/ForwardingProxy.hpp

@@ -96,7 +96,7 @@ class ForwardingProxy {
         auto res = balancer_->forwardToRippled(toForward, ctx.clientIp, ctx.isAdmin, ctx.yield);
         if (not res) {
             notifyFailedToForward(ctx.method);
-            return Result{Status{RippledError::rpcFAILED_TO_FORWARD}};
+            return Result{Status{CombinedError{res.error()}}};
         }
 
         notifyForwarded(ctx.method);

src/web/impl/ErrorHandling.hpp

@@ -90,6 +90,10 @@ class ErrorHelper {
                     case rpc::ClioError::rpcINVALID_HOT_WALLET:
                     case rpc::ClioError::rpcFIELD_NOT_FOUND_TRANSACTION:
                     case rpc::ClioError::rpcMALFORMED_ORACLE_DOCUMENT_ID:
+                    case rpc::ClioError::etlCONNECTION_ERROR:
+                    case rpc::ClioError::etlREQUEST_ERROR:
+                    case rpc::ClioError::etlREQUEST_TIMEOUT:
+                    case rpc::ClioError::etlINVALID_RESPONSE:
                         ASSERT(
                             false, "Unknown rpc error code {}", static_cast<int>(*clioCode)
                         );  // this should never happen

tests/common/util/MockLoadBalancer.hpp

@@ -19,6 +19,7 @@
 
 #pragma once
 
+#include "rpc/Errors.hpp"
 #include "util/FakeFetchResponse.hpp"
 
 #include <boost/asio/spawn.hpp>
@@ -28,6 +29,7 @@
 #include <gmock/gmock.h>
 
 #include <cstdint>
+#include <expected>
 #include <optional>
 #include <string>
 
@@ -37,8 +39,10 @@ struct MockLoadBalancer {
     MOCK_METHOD(void, loadInitialLedger, (std::uint32_t, bool), ());
     MOCK_METHOD(std::optional<FakeFetchResponse>, fetchLedger, (uint32_t, bool, bool), ());
     MOCK_METHOD(boost::json::value, toJson, (), (const));
+
+    using ForwardToRippledReturnType = std::expected<boost::json::object, rpc::ClioError>;
     MOCK_METHOD(
-        std::optional<boost::json::object>,
+        ForwardToRippledReturnType,
         forwardToRippled,
         (boost::json::object const&, std::optional<std::string> const&, bool, boost::asio::yield_context),
         (const)

tests/common/util/MockSource.hpp

@@ -19,10 +19,10 @@
 #pragma once
 
 #include "data/BackendInterface.hpp"
-#include "etl/ETLHelpers.hpp"
 #include "etl/NetworkValidatedLedgersInterface.hpp"
 #include "etl/Source.hpp"
 #include "feed/SubscriptionManagerInterface.hpp"
+#include "rpc/Errors.hpp"
 #include "util/config/Config.hpp"
 
 #include <boost/asio/io_context.hpp>
@@ -38,6 +38,7 @@
 #include <chrono>
 #include <cstddef>
 #include <cstdint>
+#include <expected>
 #include <memory>
 #include <optional>
 #include <string>
@@ -59,8 +60,10 @@ struct MockSource : etl::SourceBase {
         (override)
     );
     MOCK_METHOD((std::pair<std::vector<std::string>, bool>), loadInitialLedger, (uint32_t, uint32_t, bool), (override));
+
+    using ForwardToRippledReturnType = std::expected<boost::json::object, rpc::ClioError>;
     MOCK_METHOD(
-        std::optional<boost::json::object>,
+        ForwardToRippledReturnType,
         forwardToRippled,
         (boost::json::object const&, std::optional<std::string> const&, std::string_view, boost::asio::yield_context),
         (const, override)
@@ -127,7 +130,7 @@ class MockSourceWrapper : public etl::SourceBase {
         return mock_->loadInitialLedger(sequence, maxLedger, getObjects);
     }
 
-    std::optional<boost::json::object>
+    std::expected<boost::json::object, rpc::ClioError>
     forwardToRippled(
         boost::json::object const& request,
         std::optional<std::string> const& forwardToRippledClientIp,

tests/unit/etl/ETLStateTests.cpp

@@ -18,6 +18,7 @@
 //==============================================================================
 
 #include "etl/ETLState.hpp"
+#include "rpc/Errors.hpp"
 #include "util/LoggerFixtures.hpp"
 #include "util/MockSource.hpp"
 
@@ -37,7 +38,7 @@ struct ETLStateTest : public NoLoggerFixture {
 
 TEST_F(ETLStateTest, Error)
 {
-    EXPECT_CALL(source, forwardToRippled).WillOnce(Return(std::nullopt));
+    EXPECT_CALL(source, forwardToRippled).WillOnce(Return(std::unexpected{rpc::ClioError::etlINVALID_RESPONSE}));
     auto const state = etl::ETLState::fetchETLStateFromSource(source);
     EXPECT_FALSE(state);
 }