Remove reporting mode #5092

thejohnfreeman · 2024-08-03T16:25:21Z

Builds and passes all tests, but I have not yet tested compatibility with Clio. Asking for help from the Clio team with that.

codecov · 2024-08-03T22:45:35Z

Codecov Report

Attention: Patch coverage is 77.61194% with 30 lines in your changes missing coverage. Please review.

Project coverage is 76.1%. Comparing base (d9bd75e) to head (47644d5).
Report is 1 commits behind head on develop.

Files with missing lines	Patch %	Lines
src/xrpld/app/main/Application.cpp	75.5%	13 Missing ⚠️
src/xrpld/app/main/GRPCServer.cpp	0.0%	5 Missing ⚠️
src/xrpld/app/misc/NetworkOPs.cpp	90.0%	5 Missing ⚠️
src/xrpld/app/rdb/detail/RelationalDatabase.cpp	37.5%	5 Missing ⚠️
src/xrpld/app/ledger/Ledger.cpp	0.0%	1 Missing ⚠️
src/xrpld/rpc/detail/DeliveredAmount.cpp	75.0%	1 Missing ⚠️

Additional details and impacted files

@@            Coverage Diff            @@
##           develop   #5092     +/-   ##
=========================================
+ Coverage     74.9%   76.1%   +1.3%     
=========================================
  Files          768     760      -8     
  Lines        63134   61489   -1645     
  Branches      8846    8135    -711     
=========================================
- Hits         47269   46810    -459     
+ Misses       15865   14679   -1186

Files with missing lines	Coverage Δ
include/xrpl/protocol/ErrorCodes.h	`100.0% <ø> (ø)`
src/libxrpl/protocol/ErrorCodes.cpp	`71.4% <ø> (ø)`
src/xrpld/app/ledger/AcceptedLedger.cpp	`100.0% <100.0%> (+16.7%)`	⬆️
src/xrpld/app/ledger/Ledger.h	`100.0% <ø> (+6.2%)`	⬆️
src/xrpld/app/ledger/LedgerMaster.h	`75.0% <ø> (+13.1%)`	⬆️
src/xrpld/app/ledger/detail/LedgerMaster.cpp	`41.6% <ø> (+0.4%)`	⬆️
src/xrpld/app/ledger/detail/LedgerToJson.cpp	`76.8% <100.0%> (+0.3%)`	⬆️
src/xrpld/app/main/Application.h	`100.0% <ø> (ø)`
src/xrpld/app/main/Main.cpp	`79.1% <ø> (-0.3%)`	⬇️
src/xrpld/app/misc/NetworkOPs.h	`100.0% <ø> (ø)`
... and 44 more

... and 14 files with indirect coverage changes

godexsoft

Great effort 🙇 I'm leaving just a few questions to absolutely make sure we don't break Clio 👍

godexsoft · 2024-08-05T11:01:32Z

include/xrpl/protocol/ErrorCodes.h

@@ -136,9 +136,8 @@ enum error_code_i {
 rpcINVALID_LGR_RANGE = 79,
 rpcEXPIRED_VALIDATOR_LIST = 80,

- // Reporting
- rpcFAILED_TO_FORWARD = 90,
- rpcREPORTING_UNSUPPORTED = 91,


This is used by Clio.
We can either leave this code be or replace it in Clio with a Clio-native code instead.
The latter would slightly change the output of the 'subscribe' RPC which uses this error code.

Looks like we are going to use our own error code on Clio's side. 👍

I suggest to just keep these two in ErrorCodes.h, at least for the time being - since this is libxrpl include, there's a reasonable expectation that it defines the rippled API.

Also, these names belong to API version 2, even if unused. So removing them is an API breaking change. I'm happy to compromise to rename rpcREPORTING_UNSUPPORTED to a more generic name (keeping the error code 91).

Renaming the rpcREPORTING_UNSUPPORTED is an option but i suppose we want to keep the associated error strings as they are to avoid breaking the API.. in that case the output of Clio will remain with 'reportingMode' in the output. We can live with that. Hopefully can be properly addressed with OpenAPI later.

In this case @thejohnfreeman would it be OK to leave the above two definitions rpcFAILED_TO_FORWARD = 90 and rpcREPORTING_UNSUPPORTED = 91 , and just update the comment to indicate that they belong to API and are used by Clio ?

Ok. I'll restore these two values and add a comment.

On further research, it appears that Clio uses only rpcREPORTING_UNSUPPORTED, so I'll restore only that one.

src/xrpld/app/ledger/detail/LedgerMaster.cpp

src/xrpld/app/main/Application.cpp

godexsoft · 2024-08-05T11:23:11Z

src/xrpld/app/main/GRPCServer.cpp

@@ -381,8 +278,7 @@ GRPCServerImpl::CallData<Request, Response>::clientIsUnlimited()
 if (!getUser())
 return false;
 auto clientIp = getClientIpAddress();
- auto proxiedIp = getProxiedClientIpAddress();


Are we sure this is only used when in reporting mode? This wont break Clio's unlimited use of gRPC, right?

I can't prove it, but I think it is. I don't think there is any other gRPC client in practice. This will return true for all clients it did before, plus some more. In fact, this would have returned false for a reporting mode client acting on behalf of another client. If Clio is unlimited, that must be because it is not configured as a proxy, right?

Clio needs its IP in the secure_gateway of the [port_grpc]. I'm not sure if that makes it a proxy or what but without it we get tooBusy sometimes. Keep in mind that Clio uses the X-User header to get unlimited role. I'm not sure how any of that relates to this but from your explanation it sounds like some separate thing.

Clio is the client.

With reporting mode, the configuration looks like this:

non-reporting rippled <--> reporting rippled <--> app

In this scenario, when the app sends a request to the reporting server, and it doesn't have that information, it forwards the request to the non-reporting server. In that forwarded request, the reporting server is the client, and the app is the proxied client.

If Clio is not attaching the correct field (client_ip) to indicate it is forwarding a request on behalf of a proxied client, then it will never be throttled as long as its address (the client address) is in the list of secure gateways.

(Yes, this is confusing. Clio is the "client" in this code block, with its address in the clientIP variable, but the message's client_ip field names the "proxied client".)

For forwarding to rippled what we do with the real client's IP is

if (forwardToRippledClientIp) { connectionBuilder.addHeader( {boost::beast::http::field::forwarded, fmt::format("for={}", *forwardToRippledClientIp)} ); }

and we also set the X-User to either clio_admin or clio_user depending whether the connection is an admin one (in Clio terms).
However this has nothing to do with gRPC.

For the gRPC connection we don't set the client_ip field but we do set the user to ETL (probably does nothing).
What happens if you don't add Clio's IP to the list of secure gateways, Clio itself gets throttled. Makes sense.
Note that Clio is not doing any gRPC forwarding for Clio's clients, gRPC is only used by Clio to get the data for Clio's use.

thejohnfreeman · 2024-08-06T22:09:31Z

cfg/rippled-example.cfg

-# Each ETL source specified must have gRPC enabled (by adding a [port_grpc]
-# section to the config). It is recommended to add a secure_gateway entry to
-# the gRPC section, in order to bypass the server's rate limiting.
-# This section needs to be added to the config of the ETL source, not
-# the config of the reporting node. In the example below, the
-# reporting server is running at 127.0.0.1. Multiple IPs can be
-# specified in secure_gateway via a comma separated list.
-#
-# [port_grpc]
-# ip = 0.0.0.0
-# port = 50051
-# secure_gateway = 127.0.0.1


@godexsoft should this stanza be left in, but changed to refer to Clio instead of "reporting node"?

I think it would be helpful for operators setting up their ETL nodes, yes 👍

seelabs

👍

Remove reporting mode

65bb45c

thejohnfreeman requested a review from godexsoft August 3, 2024 16:25

Fix formatting

9d806cc

thejohnfreeman added the Remove unused code label Aug 4, 2024

godexsoft approved these changes Aug 5, 2024

View reviewed changes

thejohnfreeman commented Aug 6, 2024

View reviewed changes

thejohnfreeman added 5 commits August 6, 2024 17:38

Redocument port_grpc stanza

52bdd4b

Merge branch 'develop'

76f2144

Restore rpcREPORTING_UNSUPPORTED

30a9ab7

Remove more dead code

5808d06

Merge branch 'develop'

47644d5

seelabs approved these changes Aug 28, 2024

View reviewed changes

thejohnfreeman added the Passed Passed code review & PR owner thinks it's ready to merge. Perf sign-off may still be required. label Aug 28, 2024

thejohnfreeman merged commit 4d7aed8 into XRPLF:develop Aug 28, 2024
20 checks passed

thejohnfreeman deleted the reporting branch August 28, 2024 18:47

Provide feedback

Saved searches

Use saved searches to filter your results more quickly

Remove reporting mode #5092

Remove reporting mode #5092

thejohnfreeman commented Aug 3, 2024

codecov bot commented Aug 3, 2024 •

edited

Loading

godexsoft left a comment

godexsoft Aug 5, 2024

godexsoft Aug 5, 2024

Bronek Aug 6, 2024

godexsoft Aug 6, 2024

Bronek Aug 20, 2024

thejohnfreeman Aug 21, 2024

thejohnfreeman Aug 21, 2024

godexsoft Aug 5, 2024

thejohnfreeman Aug 5, 2024

godexsoft Aug 5, 2024

thejohnfreeman Aug 6, 2024 •

edited

Loading

godexsoft Aug 7, 2024

thejohnfreeman Aug 6, 2024

godexsoft Aug 7, 2024

seelabs left a comment

Remove reporting mode #5092

Remove reporting mode #5092

Conversation

thejohnfreeman commented Aug 3, 2024

codecov bot commented Aug 3, 2024 • edited Loading

Codecov Report

godexsoft 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

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

thejohnfreeman Aug 6, 2024 • 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

seelabs left a comment

Choose a reason for hiding this comment

codecov bot commented Aug 3, 2024 •

edited

Loading

thejohnfreeman Aug 6, 2024 •

edited

Loading