Add RPC/WS ports to server_info #4427

drlongle · 2023-02-20T16:52:40Z

High Level Overview of Change

This PR resolves #2837. It adds info about WebSocket/RPC port(s) of the local rippledserver to the response of server_info. The function processing requests to the /crawl endpoint actually calls server_info internally. Hence, this PR enables a rippled server to advertise info about its WebSocket/RPC port(s) to its peers via the endpoint /crawl. This allows the peers to build a richer topology instead of port-scanning all reachable nodes. For non-admin users (which is true for peers), the extended server_info method does not advertise info about admin RPC/WS poirts.

Context of Change

The response of server_info includes a new array ports. Each array element contains a value port for the listening port and an array protocols for the supported protocol(s). The array protocol is sorted in increasing order.

./rippled server_info

2023-Feb-20 17:20:22.585574554 UTC HTTPClient:NFO Connecting to 127.0.0.1:5005
{
   "result" : {
      "info" : {
         "build_version" : "1.10.0-rc1"
         ...
         "ports" : [
            {
               "port" : "5005",
               "protocol" : [ "http", "ws2" ]
            },
            {
               "port" : "51235",
               "protocol" : [ "peer" ]
            },
            {
               "port" : "6006",
               "protocol" : [ "ws" ]
            },
            {
               "port" : "50051",
               "protocol" : [ "port_grpc" ]
            }
         ],
         ...
      },
      "status" : "success"
   }
}

Type of Change

Bug fix (non-breaking change which fixes an issue)
New feature (non-breaking change which adds functionality)
Breaking change (fix or feature that would cause existing functionality to not work as expected)
Refactor (non-breaking change that only restructures code)
Tests (You added tests for code that already exists, or your new feature included in this PR)
Needs Documentation Updates
Release

Fix XRPLF#2837.

MarkusTeufelberger · 2023-02-21T15:53:12Z

Maybe gRPC and peer ports should also be in that list? Still, thanks for tackling this issue, the code looks ok to me, but I don't feel confident to properly review it, so I can only thank you for implementing it in general. :-)

intelliot · 2023-02-21T18:00:21Z

~~I noticed the Breaking change checkbox is checked. How is this a breaking change?~~ (resolved)

justinr1234 · 2023-02-21T18:02:52Z

src/ripple/app/misc/NetworkOPs.cpp

@@ -2661,6 +2662,38 @@ NetworkOPsImp::getServerInfo(bool human, bool admin, bool counters)
        info["reporting"] = app_.getReportingETL().getInfo();
    }

+    // This array must be sorted in increasing order.


I'm assuming this is because of std::set_intersection and Elements are compared using operator< and the ranges must be sorted with respect to the same.

Is there a way to static assert that it is sorted?

Great question, and a good idea. Since we're running C++20, std::is_sorted() is now constexpr. So, if all of our standard libraries have that support, it should be easy to check at compile time. Otherwise it's still possible to check, but a bit more code to write.

Great idea. Thanks!

Nice job with the static_assert. Looks great!

drlongle · 2023-02-22T10:10:34Z

I noticed the Breaking change checkbox is checked. How is this a breaking change?

@intelliot This PR introduces a new array in the JSON response. Does it break clients that do not expect the new array? If not, I can remove the checkbox `Breaking change.

justinr1234 · 2023-02-22T10:57:05Z

I noticed the Breaking change checkbox is checked. How is this a breaking change?

@intelliot This PR introduces a new array in the JSON response. Does it break clients that do not expect the new array? If not, I can remove the checkbox `Breaking change.

In general adding to a response shouldn’t be considered a breaking change. If you modified an existing field by doing things like changing value type, removing fields, or changing names, these would be breaking changes.

drlongle · 2023-02-22T14:22:10Z

Maybe gRPC and peer ports should also be in that list? Still, thanks for tackling this issue, the code looks ok to me, but I don't feel confident to properly review it, so I can only thank you for implementing it in general. :-)

I added grpc and peer ports as requested.

drlongle · 2023-02-22T19:32:52Z

src/ripple/app/main/GRPCServer.cpp

@@ -429,7 +429,7 @@ GRPCServerImpl::GRPCServerImpl(Application& app)
    // if present, get endpoint from config
    if (app_.config().exists("port_grpc"))
    {
-        Section section = app_.config().section("port_grpc");
+        const auto& section = app_.config().section("port_grpc");



Sneak in a minor improvement.

The change you made here definitely is an improvement. You're now taking a reference instead of making a copy. And a Section is a heavy weight thing to copy. Good spotting.

However, not everyone will agree that replacing an explicit type name with auto is an improvement. In my opinion it hides important information that could be visible explicitly in the source code. My personal opinion is that Herb Sutter was dead wrong when he said "almost always auto" years ago. So my preferred declaration here would look like...

Section const& section = ...

Folks who use an IDE with something like intellisense sometimes lose track of those of us who who develop code using just a text editor. Explicit type names (as opposed to auto) also are a big help when viewing code through browsers, like on Github or in code reviews.

I'm not asking for a change here. Just providing a different perspective for you to think about when you make other changes in the future.

Fix XRPLF#2837.

scottschurr

Looks very good! I especially like the new static_assert. I left a couple of comments, but they are not blockers.

I appreciate that you added the unit test. When I ran code coverage I noticed that here are couple of places in the new code of NetworkOPs.cpp that are not exercised by the new test. I flagged those areas with comments. It would be really nice if those two places could be exercised as well.

I'm hoping you'll look at how hard it would be to exercise those additional areas in your unit test and add that coverage if the effort is reasonable.

I run code coverage locally on my Mac. I don't know whether you use a Mac or not, but in case it helps, here are the command lines I use to run code coverage locally:

conan install ${SOURCE_DIR} --output-folder . --build missing --settings build_type=Debug
cmake -DCMAKE_BUILD_TYPE=Debug -Dunity=OFF -Dassert=ON -Dwerr=ON -Dwextra=ON -DCMAKE_TOOLCHAIN_FILE:FILEPATH=build/generators/conan_toolchain.cmake ${SOURCE_DIR}  -Dcoverage=ON -Dcoverage_core_only=OFF -Ucoverage_test ${SOURCE_DIR}
make -j3 coverage_report
open ./coverage/index.html

scottschurr · 2023-05-02T23:16:57Z

src/ripple/app/misc/NetworkOPs.cpp

+    static_assert(std::is_sorted(std::begin(protocols), std::end(protocols)));
+    {
+        Json::Value ports{Json::arrayValue};
+        std::vector<std::string> proto;


I'm guessing that proto, which is only used inside the for loop has been lofted out of the loop for a reason.

I'll guess that you want to avoid re-allocating the vector each time through the loop? If that's the case, consider really avoiding reallocation by calling...

proto.reserve(protocols.size());

before entering the loop.

However that's a really minor optimization given that there are lots of allocations for std::strings and the like taking place in this function. My preference would be to move the declaration of proto into the loop just above where it's used in the call to std::set_intersection(). That keeps the declaration closest to where it's needed and minimizes the scope of proto.

Your call. Neither of these changes is critical. I'm just offering a different perspective.

Thanks. Fixed.

scottschurr · 2023-05-03T20:50:03Z

src/ripple/app/misc/NetworkOPs.cpp

+        if (app_.config().exists("port_grpc"))
+        {
+            auto const& grpcSection = app_.config().section("port_grpc");
+            auto const optPort = grpcSection.get("port");
+            if (optPort && grpcSection.get("ip"))
+            {
+                auto& jv = ports.append(Json::Value(Json::objectValue));
+                jv[jss::port] = *optPort;
+                jv[jss::protocol] = Json::Value{Json::arrayValue};
+                jv[jss::protocol].append("port_grpc");
+            }
+        }


The code inside the if (app_.config().exists("port_grpc")) condition is not exercised by the unit tests. Is there a way to tweak your test to exercise that case?

Thanks. I modified my unit tests to cover that case.

scottschurr · 2023-05-03T20:51:15Z

src/ripple/app/misc/NetworkOPs.cpp

+            if (!admin &&
+                !(port.admin_nets_v4.empty() && port.admin_nets_v6.empty() &&
+                  port.admin_user.empty() && port.admin_password.empty()))
+                continue;


The continue inside the if (!admin && ...) condition is not exercised by the unit tests. Is there a way to tweak your test to exercise that case?

Thanks. I modified my unit tests to cover that case.

scottschurr · 2023-05-03T21:52:21Z

src/ripple/app/misc/NetworkOPs.cpp

@@ -2661,6 +2662,38 @@ NetworkOPsImp::getServerInfo(bool human, bool admin, bool counters)
        info["reporting"] = app_.getReportingETL().getInfo();
    }

+    // This array must be sorted in increasing order.


Nice job with the static_assert. Looks great!

scottschurr

👍 Looks good to me. Thanks for the added test coverage!

intelliot · 2023-05-23T03:53:10Z

Suggested commit message:

Add RPC/WS ports to server_info (#4427)

Enhance the /crawl endpoint by publishing WebSocket/RPC ports in the
server_info response. The function processing requests to the /crawl
endpoint actually calls server_info internally, so this change enables a
server to advertise its WebSocket/RPC port(s) to peers via the /crawl
endpoint. `grpc` and `peer` ports are included as well.

The new `ports` array contains objects, each containing a `port` for the
listening port (number string), and an array `protocol` listing the
supported protocol(s).

This allows crawlers to build a richer topology without needing to
port-scan nodes. For non-admin users (including peers), the info about
*admin* ports is excluded.

Also increase test coverage for RPC ServerInfo.

Fix #2837.

drlongle · 2023-05-23T09:22:39Z

@intelliot Thanks for the suggested commit message. It looks good!

intelliot · 2023-06-05T16:48:49Z

This is being held until after 1.11 has been released (~June 20)

* Commits 0b812cd (XRPLF#4427) and 11e914f (XRPLF#4516) conflict. The first added references to `ServerHandlerImp` in files outside of that class's organizational unit (which is technically incorrect). The second removed `ServerHandlerImp`, but was not up to date with develop. This results in the build failing in all circumstances. * Fixes the build by changing references to `ServerHandlerImp` to the more correct `ServerHandler`.

* Commits 0b812cd (#4427) and 11e914f (#4516) conflict. The first added references to `ServerHandlerImp` in files outside of that class's organizational unit (which is technically incorrect). The second removed `ServerHandlerImp`, but was not up to date with develop. This results in the build failing. * Fixes the build by changing references to `ServerHandlerImp` to the more correct `ServerHandler`.

Enhance the /crawl endpoint by publishing WebSocket/RPC ports in the server_info response. The function processing requests to the /crawl endpoint actually calls server_info internally, so this change enables a server to advertise its WebSocket/RPC port(s) to peers via the /crawl endpoint. `grpc` and `peer` ports are included as well. The new `ports` array contains objects, each containing a `port` for the listening port (number string), and an array `protocol` listing the supported protocol(s). This allows crawlers to build a richer topology without needing to port-scan nodes. For non-admin users (including peers), the info about *admin* ports is excluded. Also increase test coverage for RPC ServerInfo. Fix XRPLF#2837.

* Commits 0b812cd (XRPLF#4427) and 11e914f (XRPLF#4516) conflict. The first added references to `ServerHandlerImp` in files outside of that class's organizational unit (which is technically incorrect). The second removed `ServerHandlerImp`, but was not up to date with develop. This results in the build failing. * Fixes the build by changing references to `ServerHandlerImp` to the more correct `ServerHandler`.

Enhance the /crawl endpoint by publishing WebSocket/RPC ports in the server_info response. The function processing requests to the /crawl endpoint actually calls server_info internally, so this change enables a server to advertise its WebSocket/RPC port(s) to peers via the /crawl endpoint. `grpc` and `peer` ports are included as well. The new `ports` array contains objects, each containing a `port` for the listening port (number string), and an array `protocol` listing the supported protocol(s). This allows crawlers to build a richer topology without needing to port-scan nodes. For non-admin users (including peers), the info about *admin* ports is excluded. Also increase test coverage for RPC ServerInfo. Fix XRPLF#2837.

* Commits 0b812cd (XRPLF#4427) and 11e914f (XRPLF#4516) conflict. The first added references to `ServerHandlerImp` in files outside of that class's organizational unit (which is technically incorrect). The second removed `ServerHandlerImp`, but was not up to date with develop. This results in the build failing. * Fixes the build by changing references to `ServerHandlerImp` to the more correct `ServerHandler`.

Enhance the /crawl endpoint by publishing WebSocket/RPC ports in the server_info response. The function processing requests to the /crawl endpoint actually calls server_info internally, so this change enables a server to advertise its WebSocket/RPC port(s) to peers via the /crawl endpoint. `grpc` and `peer` ports are included as well. The new `ports` array contains objects, each containing a `port` for the listening port (number string), and an array `protocol` listing the supported protocol(s). This allows crawlers to build a richer topology without needing to port-scan nodes. For non-admin users (including peers), the info about *admin* ports is excluded. Also increase test coverage for RPC ServerInfo. Fix XRPLF#2837.

* Commits 0b812cd (XRPLF#4427) and 11e914f (XRPLF#4516) conflict. The first added references to `ServerHandlerImp` in files outside of that class's organizational unit (which is technically incorrect). The second removed `ServerHandlerImp`, but was not up to date with develop. This results in the build failing. * Fixes the build by changing references to `ServerHandlerImp` to the more correct `ServerHandler`.

drlongle added a commit to drlongle/rippled that referenced this pull request Feb 20, 2023

Add RPC/WebSocket ports to server_info (XRPLF#4427)

e0ad966

Fix XRPLF#2837.

drlongle force-pushed the add-ports-to-server_info branch from e94932b to e0ad966 Compare February 20, 2023 17:01

This was referenced Feb 20, 2023

Add RPC/WS ports to server_info XRPLF/xrpl.js#2222

Closed

Add RPC/WS ports to server_info XRPLF/xrpl-py#525

Closed

drlongle added a commit to drlongle/rippled that referenced this pull request Feb 20, 2023

Add RPC/WebSocket ports to server_info (XRPLF#4427)

41c3269

Fix XRPLF#2837.

drlongle force-pushed the add-ports-to-server_info branch from e0ad966 to 41c3269 Compare February 20, 2023 18:21

drlongle added a commit to drlongle/rippled that referenced this pull request Feb 20, 2023

Add RPC/WebSocket ports to server_info (XRPLF#4427)

41198ea

Fix XRPLF#2837.

drlongle force-pushed the add-ports-to-server_info branch from 41c3269 to 41198ea Compare February 20, 2023 18:37

intelliot requested review from MarkusTeufelberger, nbougalis, mtrippled and thejohnfreeman February 21, 2023 15:43

justinr1234 approved these changes Feb 21, 2023

View reviewed changes

drlongle commented Feb 22, 2023

View reviewed changes

intelliot added this to the 1.11.0 milestone Feb 23, 2023

drlongle added 2 commits February 23, 2023 08:52

Add RPC/WebSocket ports to server_info (XRPLF#4427)

1593d05

Fix XRPLF#2837.

Publish peer and grpc ports to server_info

7cad80a

drlongle force-pushed the add-ports-to-server_info branch from bac80f9 to 7cad80a Compare February 23, 2023 07:53

intelliot requested a review from scottschurr April 24, 2023 15:57

intelliot assigned thejohnfreeman and scottschurr Apr 24, 2023

scottschurr reviewed May 4, 2023

View reviewed changes

intelliot assigned drlongle and unassigned thejohnfreeman and scottschurr May 17, 2023

intelliot added the Will Need Documentation label May 17, 2023

intelliot modified the milestones: 1.11.0, 1.12 May 19, 2023

Increase test coverage for RPC ServerInfo

65ee0fd

intelliot requested a review from scottschurr May 22, 2023 22:55

intelliot unassigned drlongle May 22, 2023

scottschurr approved these changes May 23, 2023

View reviewed changes

intelliot added the Passed Passed code review & PR owner thinks it's ready to merge. Perf sign-off may still be required. label May 24, 2023

intelliot mentioned this pull request May 25, 2023

Add info about Websocket/RPC port(s) to the /crawl endpoint #2837

Closed

intelliot merged commit 0b812cd into XRPLF:develop Jun 23, 2023

ximinez mentioned this pull request Jun 28, 2023

[Trivial] Fix build references to deleted ServerHandlerImp #4592

Merged

ximinez mentioned this pull request Jun 28, 2023

Fix levelization / resolve circular dependencies #4593

Open

intelliot added a commit to intelliot/rippled that referenced this pull request Jul 9, 2023

API-CHANGELOG.md: XRPLF#4427

3d61d02

intelliot added Added to API Changelog and removed Passed Passed code review & PR owner thinks it's ready to merge. Perf sign-off may still be required. labels Jul 9, 2023

shawnxie999 mentioned this pull request Jul 18, 2023

Update docker to xrpllabsofficial/xrpld:1.12.0-b1 XRPLF/xrpl.js#2381

Merged

7 tasks

intelliot mentioned this pull request Aug 1, 2023

Add RPC/WS ports to server_info XRPLF/clio#797

Closed

intelliot added the Clio Reviewed label Aug 1, 2023

mDuo13 mentioned this pull request Sep 13, 2023

server_info/state: document new ports field XRPLF/xrpl-dev-portal#2130

Merged

Provide feedback

Saved searches

Use saved searches to filter your results more quickly

Add RPC/WS ports to server_info #4427

Add RPC/WS ports to server_info #4427

drlongle commented Feb 20, 2023 •

edited

Loading

MarkusTeufelberger commented Feb 21, 2023

intelliot commented Feb 21, 2023 •

edited

Loading

justinr1234 Feb 21, 2023

scottschurr Feb 21, 2023

drlongle Feb 22, 2023

scottschurr May 3, 2023

drlongle commented Feb 22, 2023

justinr1234 commented Feb 22, 2023

drlongle commented Feb 22, 2023

drlongle Feb 22, 2023

scottschurr Feb 23, 2023

scottschurr left a comment

scottschurr May 2, 2023

drlongle May 22, 2023

scottschurr May 3, 2023

drlongle May 22, 2023

scottschurr May 3, 2023

drlongle May 22, 2023

scottschurr May 3, 2023

scottschurr left a comment

intelliot commented May 23, 2023

drlongle commented May 23, 2023

intelliot commented Jun 5, 2023

Add RPC/WS ports to server_info #4427

Add RPC/WS ports to server_info #4427

Conversation

drlongle commented Feb 20, 2023 • edited Loading

High Level Overview of Change

Context of Change

Type of Change

MarkusTeufelberger commented Feb 21, 2023

intelliot commented Feb 21, 2023 • edited Loading

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

drlongle commented Feb 22, 2023

justinr1234 commented Feb 22, 2023

drlongle commented Feb 22, 2023

Choose a reason for hiding this comment

Choose a reason for hiding this comment

scottschurr left a comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

scottschurr left a comment

Choose a reason for hiding this comment

intelliot commented May 23, 2023

drlongle commented May 23, 2023

intelliot commented Jun 5, 2023

drlongle commented Feb 20, 2023 •

edited

Loading

intelliot commented Feb 21, 2023 •

edited

Loading