Consolidate "Not Synced" Error Messages #3418

HowardHinnant · 2020-05-28T15:16:32Z

Fixes Consolidate "Not Synced" Error Messages #3269
The error noClosed as been renamed to notSynced.
Uses of the error noCurrent, noNetwork and lgrIdxsInvalid
now use notSynced.

Reviewers should consider whether or not this is something we want to do.

cjcobb23 · 2020-05-28T16:23:27Z

src/ripple/rpc/handlers/AccountTx.cpp

@@ -236,7 +236,7 @@ getLedgerRange(
 uLedgerMax = ls.max;
 }
 if (uLedgerMax < uLedgerMin)
- return rpcLGR_IDXS_INVALID;


Unless I am mistaken, if the user passes invalid ledger indices, where the min is greater than the max, the error returned will be rpcNOT_SYNCED. But that's not an accurate description of the error, because the server could actually be synced.

Yes, let's not return rpcNOT_SYNCED in the case where the server actually is synced but the user submits invalid parameters.

lgrIdxsInvalid (the code that actually gets returned in the API for rpcLGR_IDXS_INVALID) is very hard to read, though, and redundant with other errors as well.

rpcINVALID_LGR_RANGE is a sensible code also used in some other places that would work instead for the case where the user supplied an invalid range. Or you could use invalidParams, which is returned in some cases as well.

I've changed this to rpcINVALID_LGR_RANGE.

mDuo13

I am in favor of the changes in a broad sense. I consider them a small but potentially breaking change. I worry that it might set a bad example to introduce these without using API versioning, even though the chances are low that these changes significantly break anyone's workflow and it would be kind of a shame to maintain the older version just for that.

Perhaps it would be a good exercise to try introducing API version 2 starting with these changes, to see how much effort it actually is to support both old and new behavior for this sort of thing.

Also, I think we should keep a changelog (to go in the release notes) to explicitly catalog these sorts of breaking changes, even though they are very minor. The entry for this one might be:

The error codes noClosed, noCurrent, and noNetwork have been removed. Instead, the API returns the code notSynced in all of these cases.

The error code lgrIdxInvalid has been removed. Instead, the server returns either notSynced, if the server isn't synced to the network, or invalidParams, if the supplied ledger indexes are not valid.

EDIT: To add more complexity to the "do we use API versioning for this or not?" question, v1.6.0-b7 already has another change that is, theoretically, a breaking change - to the connect method's default port.

mDuo13 · 2020-06-05T20:27:00Z

src/ripple/rpc/impl/RPCHandler.cpp

@@ -65,7 +65,7 @@ namespace {
 Failure:
 {
 "result" : {
- "error" : "noNetwork",
+ "error" : "notSynced",
 "error_code" : 16,
 "error_message" : "Not synced to Ripple network.",


Let's change this message to reflect the message change in ErrorCodes.cpp

mDuo13 · 2020-06-05T20:27:23Z

src/ripple/rpc/impl/RPCHandler.cpp

@@ -95,7 +95,7 @@ namespace {

 Failure:
 {
- "error" : "noNetwork",
+ "error" : "notSynced",
 "error_code" : 16,
 "error_message" : "Not synced to Ripple network.",


Let's change this message to reflect ErrorCodes.cpp as well

mDuo13 · 2020-06-05T20:28:45Z

src/ripple/protocol/impl/ErrorCodes.cpp

@@ -88,10 +87,8 @@ constexpr static ErrorInfo unorderedErrorInfos[]{
 {rpcNOT_IMPL, "notImpl", "Not implemented."},
 {rpcNOT_READY, "notReady", "Not ready to handle this request."},
 {rpcNOT_SUPPORTED, "notSupported", "Operation not supported."},
- {rpcNO_CLOSED, "noClosed", "Closed ledger is unavailable."},
- {rpcNO_CURRENT, "noCurrent", "Current ledger is unavailable."},
+ {rpcNOT_SYNCED, "notSynced", "Not synced to XRP network."},


I would say maybe remove the word "XRP" here and just say "to the network"? Since that way the message isn't tied to any specific branding and it doesn't have to say which network you're syncing to.

mDuo13 · 2020-06-05T20:35:36Z

src/ripple/rpc/handlers/AccountTx.cpp

@@ -236,7 +236,7 @@ getLedgerRange(
 uLedgerMax = ls.max;
 }
 if (uLedgerMax < uLedgerMin)
- return rpcLGR_IDXS_INVALID;


Yes, let's not return rpcNOT_SYNCED in the case where the server actually is synced but the user submits invalid parameters.

lgrIdxsInvalid (the code that actually gets returned in the API for rpcLGR_IDXS_INVALID) is very hard to read, though, and redundant with other errors as well.

rpcINVALID_LGR_RANGE is a sensible code also used in some other places that would work instead for the case where the user supplied an invalid range. Or you could use invalidParams, which is returned in some cases as well.

HowardHinnant · 2020-06-15T13:28:25Z

Rebased to 1.6.0-b7.

HowardHinnant · 2020-06-15T15:55:55Z

Perhaps it would be a good exercise to try introducing API version 2 starting with these changes, to see how much effort it actually is to support both old and new behavior for this sort of thing.

I'm very hesitant for multiple reasons to expand the scope of this PR.

Also, I think we should keep a changelog (to go in the release notes) to explicitly catalog these sorts of breaking changes, even though they are very minor. The entry for this one might be:

I've added a "Change Log" section to the release notes. The intent is to rename the "Change Log" prior to releasing.

nbougalis

Consider this a 👍 from me. Left one minor comment, which is at @HowardHinnant's discretion. Please note that this change may break existing tools.

nbougalis · 2020-06-15T19:28:30Z

src/ripple/rpc/impl/RPCHelpers.cpp

@@ -358,13 +358,13 @@ Status
 getLedger(T& ledger, LedgerShortcut shortcut, Context& context)
 {
 if (isValidatedOld(context.ledgerMaster, context.app.config().standalone()))
- return {rpcNO_NETWORK, "InsufficientNetworkMode"};
+ return {rpcNOT_SYNCED, "InsufficientNetworkMode"};


Is there a point to returning a text string that's different from the description of rpcNOT_SYNCED here?

Good catch. Fixed this in several places in RPCHelpers.cpp

mDuo13

The revised changes look good to me, except the Change Log typo I mentioned. 👍

One thing to keep in mind when consolidating the changes is to update the commit message. This statement is no longer precisely accurate:

* Uses of the error noCurrent, noNetwork and lgrIdxsInvalid
  now use notSynced.

(It should be adjusted to mention invalidLgrRange, as in the ChangeLog.)

mDuo13 · 2020-06-16T22:15:24Z

RELEASENOTES.md

+# Change Log
+
+- The error codes `noClosed`, `noCurrent`, and `noNetwork` have been removed. Instead, the API returns the code `notSynced` in all of these cases.
+- The error code `lgrIdxInvalid` has been removed. Instead, the server returns either `notSynced`, if the server isn't synced to the network, or `invalidParams`, if the supplied ledger indexes are not valid.


Instead of invalidParams, this should say invalidLgrRange (that's the user-facing code for rpcINVALID_LGR_RANGE).

cjcobb23 · 2020-06-18T17:38:21Z

src/ripple/rpc/handlers/AccountTx.cpp

@@ -214,7 +214,7 @@ getLedgerRange(
 if (!bValidated)
 {
 // Don't have a validated ledger range.
- return rpcLGR_IDXS_INVALID;
+ return rpcNOT_SYNCED;


Sorry, I missed this the first time around. The gRPC methods need to be changed to handle this new error. rpcNOT_SYNCED should be FAILED_PRECONDITION and rpcINVALID_LEDGER_RANGE should be INVALID_ARGUMENT. The errors are parsed in populateProtoResponse(). More details about the gRPC error codes can be found here: https://grpc.github.io/grpc/core/md_doc_statuscodes.html

We've changed the approach of this PR, but I've not yet checked it in (hopefully later today). Behavior for gRPC is not going to change at all. More details later...

We need to make the same changes to gRPC as we did for the JSON API. We can forget about versioning for gRPC, but gRPC needs to handle this new error.

Here is a commit that has the changes I am requesting: cjcobb23@022605f

* Fixes XRPLF#3269 * Work on a version 2 of the XRP Network API has begun. The new API returns the code `notSynced` in place of `noClosed`, `noCurrent`, and `noNetwork`. And `invalidParams` is returned in place of `lgrIdxInvalid`. * The version 2 API can be specified by adding "api_version" = 2 to your json request. The default version remains 1 (if unspecified), except for the command line interface which tracks version 2.

HowardHinnant · 2020-06-22T18:58:28Z

We changed direction in this PR to put these new changes only under API version 2. Because the path from develop to here is shorter than the path taken by moving from one design to another, the commits between master and this have been squashed down to one. The command line interface is hardwired to use V2.

cjcobb23 · 2020-06-22T19:32:05Z

@HowardHinnant (ignore if you are already working on this) please cherry-pick the changes from this commit: cjcobb23@022605f
We can ignore dealing with versioning for gRPC for now, but the gRPC handler needs to handle the new error condition. Even if we aren't hitting this code path at the moment (since the gRPC API is pinned to version 1), we still need to add code to parse the new error type. Else we will be accumulating technical debt that will need to be solved (and remembered) for when we add versioning support to gRPC.

HowardHinnant · 2020-06-22T20:02:44Z

Thanks CJ. cherry-picked and will upload as soon as I test it out.

HowardHinnant · 2020-06-22T20:58:29Z

Tests out fine.

mDuo13

Aside from a typo in the changelog, the revised version looks good.

Now, not to be a downer or anything... if we're close to a feature freeze for v1.6.0, maybe we should hold off on these changes until v1.7.0-b1 so that these don't become the entirety of API v2.

As soon as an API version is in a full release, it should be fully frozen for all future releases. So, if we release v1.6.0 with only these changes, any further breaking changes would need to be in API v3.

Since there is likely to be a migration cost for clients with each new API version, I would rather lump more API changes together into a single API upgrade than to release numerous small API version upgrades. Maybe that's an unjustified concern?

Personally, I feel like it makes more sense to have a slightly bigger upgrade/migration path less often than to have a really small migration path every release. Then again, having a new API version for every (or most) releases seems... fine, especially if they're usually so small that clients can choose to migrate only once every few releases without missing much in the way of changes.

RELEASENOTES.md

intelliot · 2020-06-22T22:26:54Z

I agree with Rome. This is a nice cleanup and we should certainly do it, but there's no need for this to be in 1.6. I think it would be great to have this be one of the first commits in the 1.7 branch. If other breaking API changes are completed in time for 1.7, then they can be part of api_version 2. If not, it's also fine for this to be the only change in api_version 2, and for subsequent changes to be in api_version 3. The api_version makes life easier for developers. It is not for marketing purposes.

(On the other hand, it's also fine to include this in 1.6. Just keep in mind that any breaking API changes in 1.7 must be on api_version 3.)

carlhua · 2020-06-22T22:37:02Z

Aside from a typo in the changelog, the revised version looks good.

Now, not to be a downer or anything... if we're close to a feature freeze for v1.6.0, maybe we should hold off on these changes until v1.7.0-b1 so that these don't become the entirety of API v2.

As soon as an API version is in a full release, it should be fully frozen for all future releases. So, if we release v1.6.0 with only these changes, any further breaking changes would need to be in API v3.

Since there is likely to be a migration cost for clients with each new API version, I would rather lump more API changes together into a single API upgrade than to release numerous small API version upgrades. Maybe that's an unjustified concern?

Personally, I feel like it makes more sense to have a slightly bigger upgrade/migration path less often than to have a really small migration path every release. Then again, having a new API version for every (or most) releases seems... fine, especially if they're usually so small that clients can choose to migrate only once every few releases without missing much in the way of changes.

The norm is usually that unless API v2 is officially released, then breaking changes can and will be added into API v2. At this point, I think this is the first of many APIs we intend to update for v2 and it is in an "unstable" state. Generally speaking, API versions go through "betas" as well.

cjcobb23

Looks good to me

pwang200 · 2020-06-23T22:24:44Z

(Maybe it is too far from code changed, I could not comment inline. )
beast::SemanticVersion const lastVersion("1.0.0"); in RPCHelpers.cpp should be changed to
beast::SemanticVersion const lastVersion("2.0.0");

Here is some code for you to consider. It updated the Version RPC handler since we have V2 now.
pwang200@c201d3b

The code basically implemented the Future work in the API versioning spec.
https:/pwang200/RippledRPCDesign/blob/API_versioning/design/design.md#the-version-rpc-function

I tried it out with a rippled server running locally.
COMMAND:
{"method": "batch", "params": [{"method": "version", "params": [], "jsonrpc": "2.0", "id": "t_2"}, {"method": "version", "params": [{"api_version": 2}], "jsonrpc": "2.0", "id": "t_3"}], "jsonrpc": "2.0", "id": "t_2"}

RESPONSE:
[{"id":"t_2","jsonrpc":"2.0","result":{"status":"success","version":{"first":"1.0.0","good":"1.0.0","last":"2.0.0"}}},{"id":"t_3","jsonrpc":"2.0","result":{"status":"success","version":{"api_version_lower_limit":1,"api_version_upper_limit":2}}}]

pwang200

Looks great! I have only one comment for you to consider, about the Version RPC call handler. But I feel it might change the scope of the task again. So I am OK if we want to leave it as a separate task.

@nbougalis and @carlhua what do you think?
#3418 (comment)

mDuo13 · 2020-06-24T00:01:17Z

The norm is usually that unless API v2 is officially released, then breaking changes can and will be added into API v2. At this point, I think this is the first of many APIs we intend to update for v2 and it is in an "unstable" state. Generally speaking, API versions go through "betas" as well.

I don't think our API versioning implementation supports a distinction between "unstable" and "stable" API versions. (Especially since the commandline has to always use the latest API version.) Given that, if an API version is part of a stable software release, then the API itself should be considered stable.

carlhua · 2020-06-24T00:10:50Z

The norm is usually that unless API v2 is officially released, then breaking changes can and will be added into API v2. At this point, I think this is the first of many APIs we intend to update for v2 and it is in an "unstable" state. Generally speaking, API versions go through "betas" as well.

I don't think our API versioning implementation supports a distinction between "unstable" and "stable" API versions. (Especially since the commandline has to always use the latest API version.) Given that, if an API version is part of a stable software release, then the API itself should be considered stable.

I would like to decouple the notion of software version vs API version. In this case, for the upcoming 1.6 release, I would ask us to consider that API v1 (the existing API) is the stable version. This is the default API version when no version is specified. Then for any changes, we would like to make, that would be considered a v2 beta - and we make no promise on backward compatibility for a beta API. I agree our software does not provide native stability distinction - but this can be advertised via documentation IMO. It makes our job very difficult if we have to increment the version each time we have a breaking change during the development of a new API version. Often time, we would like users to try out the new version of the APIs and incrementally improve upon that, and until we mark it final - breaking changes should be expected.

We can also consider the V2 as alpha, and breaking changes should be both expected and allowed. The commandline is a potential issue - but I don't think we officially guarantee the compatibility on commandline? (correct me if I am wrong)

cjcobb23 · 2020-06-24T02:01:00Z

Just want to say that I agree that we need some way to accumulate changes for v2 (or any version), before we consider v2 stable. I also agree that we should decouple API version from software version.

HowardHinnant · 2020-06-24T14:31:43Z

I don't have strong feelings about how we deploy versioning. However I have coded and tested (but not uploaded here) another option for everyone's consideration. We could set things up such that all the work to-date for API V2 is done and shipped, but RPC::ApiMaximumSupportedVersion remains set at 1 (https:/ripple/rippled/blob/develop/src/ripple/rpc/impl/RPCHelpers.h#L214). This would mean that clients can not yet use API V2, but developers can easily turn on API V2 for further development of API V2 as it matures.

mDuo13 · 2020-06-24T16:28:15Z

@HowardHinnant How does that work for the commandline?

HowardHinnant · 2020-06-24T16:37:18Z

How does that work for the commandline?

The command line reads RPC::ApiMaximumSupportedVersion and does whatever is appropriate for that version.

--- a/src/ripple/net/impl/RPCCall.cpp
+++ b/src/ripple/net/impl/RPCCall.cpp
@@ -314,8 +314,9 @@ private:
 
             if (uLedgerMax != -1 && uLedgerMax < uLedgerMin)
             {
-                // This is an api_version 2 response
-                // because it is a command line request
+                // The command line always follows ApiMaximumSupportedVersion
+                if (RPC::ApiMaximumSupportedVersion == 1)
+                    return rpcError(rpcLGR_IDXS_INVALID);
                 return rpcError(rpcNOT_SYNCED);
             }
 
@@ -386,8 +387,9 @@ private:
 
             if (uLedgerMax != -1 && uLedgerMax < uLedgerMin)
             {
-                // This is an api_version 2 response
-                // because it is a command line request
+                // The command line always follows ApiMaximumSupportedVersion
+                if (RPC::ApiMaximumSupportedVersion == 1)
+                    return rpcError(rpcLGR_IDXS_INVALID);
                 return rpcError(rpcNOT_SYNCED);
             }

mDuo13 · 2020-06-24T21:30:55Z

I am fine with any of the following:

adding this to v1.6.0 as part of a stable API v2, then putting any post-v1.6.0 changes in API v3
adding this to v1.6.0 but disabling API v2 for now (clearly marking v2 as unstable)
postponing this to v1.7.0 so that API v2 can have more changes overall

I am not fine with:

releasing v1.6.0 with this change, then adding more changes to API v2 in releases > v1.6.0

In my opinion, if two stable server releases serve a different API under the "v2" name, we are doing API versioning all wrong. That completely defeats the purpose of letting the client request a specific, stable API version. Let's be honest, most people don't RTFM, so just saying in documentation that v2 is unstable is not sufficient.

I don't think we should release "unstable" API versions. Are we afraid of using up too many numbers? The code maintenance is not really that different if one place checks for "version > 1" and someplace else checks for "version > 2". The user experience is straightforward, dependable, and easy to understand: "API v2" means the same thing in every release. Betas and pre-releases have unstable API versions; that makes sense. And if we go through 4 API versions a year... who cares? The user can still migrate to the latest version, stick to it until they're ready to migrate again, and we can still maintain a rolling window for supporting old API versions (say, 1 year?). If anything, having each new stable release introduce a new API version seems pretty easy to figure out.

cjcobb23 · 2020-06-24T21:40:50Z

* adding this to v1.6.0 but disabling API v2 for now (clearly marking v2 as unstable)

I vote for this option.

* It can be re-enabled by setting ApiMaximumSupportedVersion to 2.

mDuo13

👍 for v2 being added and disabled for now.

pwang200 · 2020-06-24T22:04:18Z

* adding this to v1.6.0 but disabling API v2 for now (clearly marking v2 as unstable)
I vote for this option too.

HowardHinnant requested review from nbougalis and mDuo13 May 28, 2020 15:16

HowardHinnant assigned nbougalis and mDuo13 May 28, 2020

cjcobb23 reviewed May 28, 2020

View reviewed changes

carlhua requested a review from cjcobb23 June 5, 2020 14:47

mDuo13 requested changes Jun 5, 2020

View reviewed changes

HowardHinnant force-pushed the consolidate branch from 4b9e23e to c2b5e38 Compare June 15, 2020 13:27

nbougalis reviewed Jun 15, 2020

View reviewed changes

mDuo13 approved these changes Jun 16, 2020

View reviewed changes

carlhua assigned cjcobb23 Jun 18, 2020

cjcobb23 suggested changes Jun 18, 2020

View reviewed changes

HowardHinnant force-pushed the consolidate branch from 1033724 to 554aa89 Compare June 22, 2020 18:55

HowardHinnant requested a review from pwang200 June 22, 2020 19:07

HowardHinnant assigned pwang200 Jun 22, 2020

handle notSynced in gRPC code

7760766

mDuo13 reviewed Jun 22, 2020

View reviewed changes

RELEASENOTES.md Outdated Show resolved Hide resolved

RELEASENOTES.md Outdated Show resolved Hide resolved

RELEASENOTES.md Outdated Show resolved Hide resolved

cjcobb23 approved these changes Jun 23, 2020

View reviewed changes

[FOLD] Address reviewer comments

ceaa645

pwang200 reviewed Jun 23, 2020

View reviewed changes

carlhua requested review from pwang200 and nbougalis June 24, 2020 12:25

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

HowardHinnant removed the Passed Passed code review & PR owner thinks it's ready to merge. Perf sign-off may still be required. label Jun 24, 2020

[FOLD] Disable the v2 API by ApiMaximumSupportedVersion to 1.

1e1a9ee

* It can be re-enabled by setting ApiMaximumSupportedVersion to 2.

HowardHinnant requested review from mDuo13 and cjcobb23 June 24, 2020 21:55

mDuo13 approved these changes Jun 24, 2020

View reviewed changes

pwang200 approved these changes Jun 24, 2020

View reviewed changes

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

cjcobb23 approved these changes Jun 25, 2020

View reviewed changes

This was referenced Jun 25, 2020

Proposed 1.6.0-b8 #3459

Closed

Proposed 1.6.0-b8 #3461

Closed

Proposed 1.6.0-b8 #3462

Closed

Proposed 1.6.0-b8 #3483

Merged

manojsdoshi closed this in #3483 Jun 30, 2020

Consolidate "Not Synced" Error Messages #3418

Consolidate "Not Synced" Error Messages #3418

Conversation

HowardHinnant commented May 28, 2020

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

mDuo13 left a comment • 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

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

HowardHinnant commented Jun 15, 2020

HowardHinnant commented Jun 15, 2020

nbougalis left a comment • edited Loading

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

mDuo13 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

HowardHinnant commented Jun 22, 2020

cjcobb23 commented Jun 22, 2020

HowardHinnant commented Jun 22, 2020

HowardHinnant commented Jun 22, 2020

mDuo13 left a comment

Choose a reason for hiding this comment

intelliot commented Jun 22, 2020

carlhua commented Jun 22, 2020

cjcobb23 left a comment

Choose a reason for hiding this comment

pwang200 commented Jun 23, 2020

pwang200 left a comment • edited Loading

Choose a reason for hiding this comment

mDuo13 commented Jun 24, 2020

carlhua commented Jun 24, 2020 • edited Loading

cjcobb23 commented Jun 24, 2020

HowardHinnant commented Jun 24, 2020

mDuo13 commented Jun 24, 2020

HowardHinnant commented Jun 24, 2020 • edited Loading

mDuo13 commented Jun 24, 2020 • edited Loading

cjcobb23 commented Jun 24, 2020

mDuo13 left a comment

Choose a reason for hiding this comment

pwang200 commented Jun 24, 2020

mDuo13 left a comment •

edited

Loading

nbougalis left a comment •

edited

Loading

pwang200 left a comment •

edited

Loading

carlhua commented Jun 24, 2020 •

edited

Loading

HowardHinnant commented Jun 24, 2020 •

edited

Loading

mDuo13 commented Jun 24, 2020 •

edited

Loading