Skip to content
New issue

Have a question about this project? Sign up for a free GitHub account to open an issue and contact its maintainers and the community.

By clicking “Sign up for GitHub”, you agree to our terms of service and privacy statement. We’ll occasionally send you account related emails.

Already on GitHub? Sign in to your account

Consolidate "Not Synced" Error Messages #3418

Closed
wants to merge 4 commits into from
Closed
Show file tree
Hide file tree
Changes from all commits
Commits
File filter

Filter by extension

Filter by extension

Conversations
Failed to load comments.
Loading
Jump to
Jump to file
Failed to load files.
Loading
Diff view
Diff view
5 changes: 5 additions & 0 deletions RELEASENOTES.md
Original file line number Diff line number Diff line change
Expand Up @@ -7,6 +7,11 @@ This document contains the release notes for `rippled`, the reference server imp

Have new ideas? Need help with setting up your node? Come visit us [here](https://github.com/ripple/rippled/issues/new/choose)

# Change Log

- 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 `invalidLgrRange` 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 always uses the latest verison.

# Releases

## Version 1.5.0
Expand Down
8 changes: 5 additions & 3 deletions src/ripple/app/main/Application.cpp
Original file line number Diff line number Diff line change
Expand Up @@ -1759,9 +1759,11 @@ ApplicationImp::setup()
getOPs(),
getLedgerMaster(),
c,
Role::ADMIN},
jvCommand,
RPC::ApiMaximumSupportedVersion};
Role::ADMIN,
{},
{},
RPC::ApiMaximumSupportedVersion},
jvCommand};

Json::Value jvResult;
RPC::doCommand(context, jvResult);
Expand Down
3 changes: 2 additions & 1 deletion src/ripple/app/main/GRPCServer.cpp
Original file line number Diff line number Diff line change
Expand Up @@ -142,7 +142,8 @@ GRPCServerImpl::CallData<Request, Response>::process(
usage,
role,
coro,
InfoSub::pointer()},
InfoSub::pointer(),
apiVersion},
request_};

// Make sure we can currently handle the rpc
Expand Down
2 changes: 2 additions & 0 deletions src/ripple/app/main/GRPCServer.h
Original file line number Diff line number Diff line change
Expand Up @@ -105,6 +105,8 @@ class GRPCServerImpl final
template <class Request, class Response>
using Handler = std::function<std::pair<Response, grpc::Status>(
RPC::GRPCContext<Request>&)>;
// This implementation is currently limited to v1 of the API
static unsigned constexpr apiVersion = 1;

public:
explicit GRPCServerImpl(Application& app);
Expand Down
10 changes: 8 additions & 2 deletions src/ripple/net/impl/RPCCall.cpp
Original file line number Diff line number Diff line change
Expand Up @@ -314,7 +314,10 @@ class RPCParser

if (uLedgerMax != -1 && uLedgerMax < uLedgerMin)
{
return rpcError(rpcLGR_IDXS_INVALID);
// The command line always follows ApiMaximumSupportedVersion
if (RPC::ApiMaximumSupportedVersion == 1)
return rpcError(rpcLGR_IDXS_INVALID);
return rpcError(rpcNOT_SYNCED);
}

jvRequest[jss::ledger_index_min] = jvParams[1u].asInt();
Expand Down Expand Up @@ -384,7 +387,10 @@ class RPCParser

if (uLedgerMax != -1 && uLedgerMax < uLedgerMin)
{
return rpcError(rpcLGR_IDXS_INVALID);
// The command line always follows ApiMaximumSupportedVersion
if (RPC::ApiMaximumSupportedVersion == 1)
return rpcError(rpcLGR_IDXS_INVALID);
return rpcError(rpcNOT_SYNCED);
}

jvRequest[jss::ledger_index_min] = jvParams[1u].asInt();
Expand Down
2 changes: 1 addition & 1 deletion src/ripple/protocol/ErrorCodes.h
Original file line number Diff line number Diff line change
Expand Up @@ -64,9 +64,9 @@ enum error_code_i {
rpcNO_CLOSED = 15,
rpcNO_CURRENT = 16,
rpcNO_NETWORK = 17,
rpcNOT_SYNCED = 18,

// Ledger state
// unused 18,
rpcACT_NOT_FOUND = 19,
// unused 20,
rpcLGR_NOT_FOUND = 21,
Expand Down
3 changes: 2 additions & 1 deletion src/ripple/protocol/impl/ErrorCodes.cpp
Original file line number Diff line number Diff line change
Expand Up @@ -90,8 +90,9 @@ constexpr static ErrorInfo unorderedErrorInfos[]{
{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 the network."},
{rpcNO_EVENTS, "noEvents", "Current transport does not support events."},
{rpcNO_NETWORK, "noNetwork", "Not synced to Ripple network."},
{rpcNO_NETWORK, "noNetwork", "Not synced to the network."},
{rpcNO_PERMISSION,
"noPermission",
"You don't have permission for this command."},
Expand Down
2 changes: 1 addition & 1 deletion src/ripple/rpc/Context.h
Original file line number Diff line number Diff line change
Expand Up @@ -47,6 +47,7 @@ struct Context
Role role;
std::shared_ptr<JobQueue::Coro> coro{};
InfoSub::pointer infoSub{};
unsigned int apiVersion;
};

struct JsonContext : public Context
Expand All @@ -62,7 +63,6 @@ struct JsonContext : public Context

Json::Value params;

unsigned int apiVersion;
Headers headers{};
};

Expand Down
14 changes: 12 additions & 2 deletions src/ripple/rpc/handlers/AccountTx.cpp
Original file line number Diff line number Diff line change
Expand Up @@ -214,7 +214,9 @@ getLedgerRange(
if (!bValidated)
{
// Don't have a validated ledger range.
return rpcLGR_IDXS_INVALID;
if (context.apiVersion == 1)
return rpcLGR_IDXS_INVALID;
return rpcNOT_SYNCED;
Copy link
Contributor

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

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

Copy link
Contributor Author

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

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...

Copy link
Contributor

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

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.

Copy link
Contributor

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

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

}

std::uint32_t uLedgerMin = uValidatedMin;
Expand All @@ -236,7 +238,11 @@ getLedgerRange(
uLedgerMax = ls.max;
}
if (uLedgerMax < uLedgerMin)
return rpcLGR_IDXS_INVALID;
Copy link
Contributor

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

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.

Copy link
Collaborator

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

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.

Copy link
Contributor Author

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

I've changed this to rpcINVALID_LGR_RANGE.

{
if (context.apiVersion == 1)
return rpcLGR_IDXS_INVALID;
return rpcINVALID_LGR_RANGE;
}
}
else
{
Expand Down Expand Up @@ -330,6 +336,10 @@ populateProtoResponse(
{
status = {grpc::StatusCode::NOT_FOUND, error.message()};
}
else if (error.toErrorCode() == rpcNOT_SYNCED)
{
status = {grpc::StatusCode::FAILED_PRECONDITION, error.message()};
}
else
{
status = {grpc::StatusCode::INVALID_ARGUMENT, error.message()};
Expand Down
8 changes: 6 additions & 2 deletions src/ripple/rpc/handlers/AccountTxOld.cpp
Original file line number Diff line number Diff line change
Expand Up @@ -105,15 +105,19 @@ doAccountTxOld(RPC::JsonContext& context)
if (!bValidated && (iLedgerMin == -1 || iLedgerMax == -1))
{
// Don't have a validated ledger range.
return rpcError(rpcLGR_IDXS_INVALID);
if (context.apiVersion == 1)
return rpcError(rpcLGR_IDXS_INVALID);
return rpcError(rpcNOT_SYNCED);
}

uLedgerMin = iLedgerMin == -1 ? uValidatedMin : iLedgerMin;
uLedgerMax = iLedgerMax == -1 ? uValidatedMax : iLedgerMax;

if (uLedgerMax < uLedgerMin)
{
return rpcError(rpcLGR_IDXS_INVALID);
if (context.apiVersion == 1)
return rpcError(rpcLGR_IDXS_INVALID);
return rpcError(rpcNOT_SYNCED);
}
}
else
Expand Down
6 changes: 5 additions & 1 deletion src/ripple/rpc/handlers/LedgerRequest.cpp
Original file line number Diff line number Diff line change
Expand Up @@ -67,7 +67,11 @@ doLedgerRequest(RPC::JsonContext& context)
// We need a validated ledger to get the hash from the sequence
if (ledgerMaster.getValidatedLedgerAge() >
RPC::Tuning::maxValidatedLedgerAge)
return rpcError(rpcNO_CURRENT);
{
if (context.apiVersion == 1)
return rpcError(rpcNO_CURRENT);
return rpcError(rpcNOT_SYNCED);
}

ledgerIndex = jsonIndex.asInt();
auto ledger = ledgerMaster.getValidatedLedger();
Expand Down
4 changes: 3 additions & 1 deletion src/ripple/rpc/handlers/RipplePathFind.cpp
Original file line number Diff line number Diff line change
Expand Up @@ -49,7 +49,9 @@ doRipplePathFind(RPC::JsonContext& context)
if (context.app.getLedgerMaster().getValidatedLedgerAge() >
RPC::Tuning::maxValidatedLedgerAge)
{
return rpcError(rpcNO_NETWORK);
if (context.apiVersion == 1)
return rpcError(rpcNO_NETWORK);
return rpcError(rpcNOT_SYNCED);
}

PathRequest::pointer request;
Expand Down
16 changes: 12 additions & 4 deletions src/ripple/rpc/impl/Handler.h
Original file line number Diff line number Diff line change
Expand Up @@ -83,7 +83,9 @@ conditionMet(Condition condition_required, T& context)
JLOG(context.j.info()) << "Insufficient network mode for RPC: "
<< context.netOps.strOperatingMode();

return rpcNO_NETWORK;
if (context.apiVersion == 1)
return rpcNO_NETWORK;
return rpcNOT_SYNCED;
}

if (context.app.getOPs().isAmendmentBlocked() &&
Expand All @@ -99,7 +101,9 @@ conditionMet(Condition condition_required, T& context)
if (context.ledgerMaster.getValidatedLedgerAge() >
Tuning::maxValidatedLedgerAge)
{
return rpcNO_CURRENT;
if (context.apiVersion == 1)
return rpcNO_CURRENT;
return rpcNOT_SYNCED;
}

auto const cID = context.ledgerMaster.getCurrentLedgerIndex();
Expand All @@ -110,14 +114,18 @@ conditionMet(Condition condition_required, T& context)
JLOG(context.j.debug())
<< "Current ledger ID(" << cID
<< ") is less than validated ledger ID(" << vID << ")";
return rpcNO_CURRENT;
if (context.apiVersion == 1)
return rpcNO_CURRENT;
return rpcNOT_SYNCED;
}
}

if ((condition_required & NEEDS_CLOSED_LEDGER) &&
!context.ledgerMaster.getClosedLedger())
{
return rpcNO_CLOSED;
if (context.apiVersion == 1)
return rpcNO_CLOSED;
return rpcNOT_SYNCED;
}

return rpcSUCCESS;
Expand Down
22 changes: 18 additions & 4 deletions src/ripple/rpc/impl/RPCHandler.cpp
Original file line number Diff line number Diff line change
Expand Up @@ -65,9 +65,16 @@ namespace {
Failure:
{
"result" : {
// api_version == 1
"error" : "noNetwork",
"error_code" : 16,
"error_message" : "Not synced to Ripple network.",
"error_code" : 17,
"error_message" : "Not synced to the network.",

// api_version == 2
"error" : "notSynced",
"error_code" : 18,
"error_message" : "Not synced to the network.",

"request" : {
"command" : "ledger",
"ledger_index" : 10300865
Expand Down Expand Up @@ -95,9 +102,16 @@ namespace {

Failure:
{
// api_version == 1
"error" : "noNetwork",
"error_code" : 16,
"error_message" : "Not synced to Ripple network.",
"error_code" : 17,
"error_message" : "Not synced to the network.",

// api_version == 2
"error" : "notSynced",
"error_code" : 18,
"error_message" : "Not synced to the network.",

"request" : {
"command" : "ledger",
"ledger_index" : 10300865
Expand Down
26 changes: 21 additions & 5 deletions src/ripple/rpc/impl/RPCHelpers.cpp
Original file line number Diff line number Diff line change
Expand Up @@ -347,7 +347,9 @@ getLedger(T& ledger, uint32_t ledgerIndex, Context& context)
isValidatedOld(context.ledgerMaster, context.app.config().standalone()))
{
ledger.reset();
return {rpcNO_NETWORK, "InsufficientNetworkMode"};
if (context.apiVersion == 1)
return {rpcNO_NETWORK, "InsufficientNetworkMode"};
return {rpcNOT_SYNCED, "notSynced"};
}

return Status::OK;
Expand All @@ -358,13 +360,21 @@ Status
getLedger(T& ledger, LedgerShortcut shortcut, Context& context)
{
if (isValidatedOld(context.ledgerMaster, context.app.config().standalone()))
return {rpcNO_NETWORK, "InsufficientNetworkMode"};
{
if (context.apiVersion == 1)
return {rpcNO_NETWORK, "InsufficientNetworkMode"};
return {rpcNOT_SYNCED, "notSynced"};
}

if (shortcut == LedgerShortcut::VALIDATED)
{
ledger = context.ledgerMaster.getValidatedLedger();
if (ledger == nullptr)
return {rpcNO_NETWORK, "InsufficientNetworkMode"};
{
if (context.apiVersion == 1)
return {rpcNO_NETWORK, "InsufficientNetworkMode"};
return {rpcNOT_SYNCED, "notSynced"};
}

assert(!ledger->open());
}
Expand All @@ -386,15 +396,21 @@ getLedger(T& ledger, LedgerShortcut shortcut, Context& context)
}

if (ledger == nullptr)
return {rpcNO_NETWORK, "InsufficientNetworkMode"};
{
if (context.apiVersion == 1)
return {rpcNO_NETWORK, "InsufficientNetworkMode"};
return {rpcNOT_SYNCED, "notSynced"};
}

static auto const minSequenceGap = 10;

if (ledger->info().seq + minSequenceGap <
context.ledgerMaster.getValidLedgerIndex())
{
ledger.reset();
return {rpcNO_NETWORK, "InsufficientNetworkMode"};
if (context.apiVersion == 1)
return {rpcNO_NETWORK, "InsufficientNetworkMode"};
return {rpcNOT_SYNCED, "notSynced"};
}
}
return Status::OK;
Expand Down
8 changes: 4 additions & 4 deletions src/ripple/rpc/impl/ServerHandlerImp.cpp
Original file line number Diff line number Diff line change
Expand Up @@ -443,9 +443,9 @@ ServerHandlerImp::processSession(
is->getConsumer(),
role,
coro,
is},
is,
apiVersion},
jv,
apiVersion,
{is->user(), is->forwarded_for()}};

RPC::doCommand(context, jr[jss::result]);
Expand Down Expand Up @@ -829,9 +829,9 @@ ServerHandlerImp::processRequest(
usage,
role,
coro,
InfoSub::pointer()},
InfoSub::pointer(),
apiVersion},
params,
apiVersion,
{user, forwardedFor}};
Json::Value result;
RPC::doCommand(context, result);
Expand Down
14 changes: 10 additions & 4 deletions src/ripple/rpc/impl/TransactionSign.cpp
Original file line number Diff line number Diff line change
Expand Up @@ -270,7 +270,8 @@ checkTxJsonFields(
bool const verify,
std::chrono::seconds validatedLedgerAge,
Config const& config,
LoadFeeTrack const& feeTrack)
LoadFeeTrack const& feeTrack,
unsigned apiVersion)
{
std::pair<Json::Value, AccountID> ret;

Expand Down Expand Up @@ -308,7 +309,10 @@ checkTxJsonFields(
if (verify && !config.standalone() &&
(validatedLedgerAge > Tuning::maxValidatedLedgerAge))
{
ret.first = rpcError(rpcNO_CURRENT);
if (apiVersion == 1)
ret.first = rpcError(rpcNO_CURRENT);
else
ret.first = rpcError(rpcNOT_SYNCED);
return ret;
}

Expand Down Expand Up @@ -384,7 +388,8 @@ transactionPreProcessImpl(
verify,
validatedLedgerAge,
app.config(),
app.getFeeTrack());
app.getFeeTrack(),
getAPIVersionNumber(params));

if (RPC::contains_error(txJsonResult))
return std::move(txJsonResult);
Expand Down Expand Up @@ -1068,7 +1073,8 @@ transactionSubmitMultiSigned(
true,
validatedLedgerAge,
app.config(),
app.getFeeTrack());
app.getFeeTrack(),
getAPIVersionNumber(jvRequest));

if (RPC::contains_error(txJsonResult))
return std::move(txJsonResult);
Expand Down
Loading