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.

Sign up for GitHub

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

test: env unit test RPC errors return a unique result: #4887

Merged
merged 1 commit into from
Mar 19, 2024
Merged

test: env unit test RPC errors return a unique result: #4887

Show file tree
Hide file tree
Changes from all commits
Commits
Show all changes
1 commit
Select commit
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
3 changes: 2 additions & 1 deletion src/ripple/protocol/TER.h
View file
Open in desktop
Original file line number Diff line number Diff line change
Expand Up @@ -64,7 +64,8 @@ enum TELcodes : TERUnderlyingType {
telCAN_NOT_QUEUE_FULL,
telWRONG_NETWORK,
telREQUIRES_NETWORK_ID,
telNETWORK_ID_MAKES_TX_NON_CANONICAL
telNETWORK_ID_MAKES_TX_NON_CANONICAL,
telENV_RPC_FAILED
};

//------------------------------------------------------------------------------
Expand Down
1 change: 1 addition & 0 deletions src/ripple/protocol/impl/TER.cpp
View file
Open in desktop
Original file line number Diff line number Diff line change
Expand Up @@ -154,6 +154,7 @@ transResults()
MAKE_ERROR(telWRONG_NETWORK, "Transaction specifies a Network ID that differs from that of the local node."),
MAKE_ERROR(telREQUIRES_NETWORK_ID, "Transactions submitted to this node/network must include a correct NetworkID field."),
MAKE_ERROR(telNETWORK_ID_MAKES_TX_NON_CANONICAL, "Transactions submitted to this node/network must NOT include a NetworkID field."),
MAKE_ERROR(telENV_RPC_FAILED, "Unit test RPC failure."),

MAKE_ERROR(temMALFORMED, "Malformed transaction."),
MAKE_ERROR(temBAD_AMM_TOKENS, "Malformed: Invalid LPTokens."),
Expand Down
12 changes: 9 additions & 3 deletions src/test/app/MultiSign_test.cpp
View file
Open in desktop
Original file line number Diff line number Diff line change
Expand Up @@ -247,7 +247,10 @@ class MultiSign_test : public beast::unit_test::suite

// Duplicate signers should fail.
aliceSeq = env.seq(alice);
env(noop(alice), msig(demon, demon), fee(3 * baseFee), ter(temINVALID));
env(noop(alice),
msig(demon, demon),
fee(3 * baseFee),
ter(telENV_RPC_FAILED));
env.close();
BEAST_EXPECT(env.seq(alice) == aliceSeq);

Expand Down Expand Up @@ -358,7 +361,7 @@ class MultiSign_test : public beast::unit_test::suite
msig phantoms{bogie, demon};
std::reverse(phantoms.signers.begin(), phantoms.signers.end());
std::uint32_t const aliceSeq = env.seq(alice);
env(noop(alice), phantoms, ter(temINVALID));
env(noop(alice), phantoms, ter(telENV_RPC_FAILED));
env.close();
BEAST_EXPECT(env.seq(alice) == aliceSeq);
}
Expand Down Expand Up @@ -1634,7 +1637,10 @@ class MultiSign_test : public beast::unit_test::suite

// Duplicate signers should fail.
aliceSeq = env.seq(alice);
env(noop(alice), msig(demon, demon), fee(3 * baseFee), ter(temINVALID));
env(noop(alice),
msig(demon, demon),
fee(3 * baseFee),
ter(telENV_RPC_FAILED));
Copy link
Collaborator

Choose a reason for hiding this comment

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

@ximinez Only the error code in Env.cpp's Env::parseResult should be returning telENV_RPC_FAILED. Why are all the other unit tests using this new error code?

For instance, if I use duplicate signers in an RPC call to a mainnet validator (non-unit test scenario), I should get temINVALID error code, not a telRNV_RPC_FAILED

Copy link
Collaborator Author

Choose a reason for hiding this comment

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

Why are all the other unit tests using this new error code?

Excellent question. The reason is that the RPC layer does some validation before ever invoking the transaction engine. While some of these checks are redundant, catching them early prevents a lot more work from being done later, and so saves time overall. In this example, I think the duplicate signer check is done before any signature verification, which is way more expensive.

Either way, this request is failing on those RPC checks, and so the result has no TER code. You can verify this yourself by changing the result to something else (e.g. tesSUCCESS), and noting that the new log message will show an RPC-level error, and not a transactor-level error.

Copy link
Collaborator

Choose a reason for hiding this comment

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

Hmm, I understand.

From a code readability point of view, it doesn't help that telENV_RPC_FAILED is scattered across a few unit tests (unrelated to the unit testing skeleton code).

If performance were not an issue, I'd love to clean up these instances of RPC-level errors. alas ://

thanks for the explanation

Copy link
Collaborator Author

Choose a reason for hiding this comment

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

You know.... It might not be a bad idea to introduce a new "requirement" parameter, similar to ter() for RPC result code values.... Then the test could explicitly specify which kind of RPC failure it's expecting.

Copy link
Collaborator

Choose a reason for hiding this comment

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

Potentially something not called ter() but rpcErr()? So if a particular RPC error was expected we could check for that. It might have the side effect of preventing the check for tesSUCCESS?

How hard would it be to code something like that up?

Copy link
Collaborator

Choose a reason for hiding this comment

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

Hello Ed,
You have done a great job! your branch passes all the tests, I don't think I can do better 😅

I have a minor nit about the placement of a comment, you can find it here: https://github.com/ckeshava/rippled/pull/new/minorNit

Having said that, I wanted to discuss three things:

  1. I feel we can avoid using exception as a variable name (to avoid confusing with std::exception) here - develop...ximinez:rippled:ci/jtx_rpc#diff-19c35fddcebc11f5dd0b6b647315df1340bdddc6f9a79c3a6de4b6a98a43a5a5R49

  2. Regarding the presentation of RPC errors, you have structured it as one of two alternatives: <rpcCode, epcError> or <rpcError, epcExceptionMessage>. This does capture all the existing unit tests.
    But I came across three instances where <rpcCode, epcError, rpcExceptionMessage> are all returned by rippled. Here are some examples:

As part of future work, it would be nice if we can have consistency across all such instances. Would it be too hard to specify an rpcErrorCode in all such instances? I don't know.

  1. I realised that we don't unit-test rpc<error_code> nearly as much as the tem/tec error codes. A handful of unit tests expect rpc-related errors. I know that Bronek is working to add more unit tests. I feel such unit tests are useful to detect breaking of Public APIs. What do you think?

Copy link
Collaborator Author

Choose a reason for hiding this comment

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

Awesome. Thanks! I'll make those changes and open a PR!

Copy link
Collaborator

Choose a reason for hiding this comment

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

I have tried to do a PoC for point-2 in my previous comment. Please check the top two commits: ximinez/rippled@ci/jtx_rpc...ckeshava:minorNit

I think it would be nice if all the error cases would return a jss::error_code, instead of only returning a jss::error or jss::error_message. What do you think?

I think this will be a non-trivial amount of work.

The Developer Growth team is working on an OpenAPI specification. I believe it would be useful if we can have a consistent set of error codes -- it would be easier to model the RPC error cases. (This is my read of the situation, I haven't discussed this with the team yet)

Copy link
Collaborator Author

@ximinez ximinez Apr 12, 2024 •
edited
Loading

Choose a reason for hiding this comment

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

This looks like it would be a valuable addition, but I think adding new RPC codes is beyond the scope of my change, partly because it could change the RPC API.

Copy link
Collaborator

Choose a reason for hiding this comment

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

yeah, that's true

env.close();
BEAST_EXPECT(env.seq(alice) == aliceSeq);

Expand Down
2 changes: 1 addition & 1 deletion src/test/app/Regression_test.cpp
View file
Open in desktop
Original file line number Diff line number Diff line change
Expand Up @@ -149,7 +149,7 @@ struct Regression_test : public beast::unit_test::suite
secp256r1Sig->setFieldVL(sfSigningPubKey, *pubKeyBlob);
jt.stx.reset(secp256r1Sig.release());

env(jt, ter(temINVALID));
env(jt, ter(telENV_RPC_FAILED));
};

Account const alice{"alice", KeyType::secp256k1};
Expand Down
3 changes: 3 additions & 0 deletions src/test/app/ValidatorSite_test.cpp
View file
Open in desktop
Original file line number Diff line number Diff line change
Expand Up @@ -237,7 +237,10 @@ class ValidatorSite_test : public beast::unit_test::suite

std::vector<std::string> uris;
for (auto const& u : servers)
{
log << "Testing " << u.uri << std::endl;
uris.push_back(u.uri);
}
sites->load(uris);
sites->start();
sites->join();
Expand Down
6 changes: 5 additions & 1 deletion src/test/jtx/Env.h
View file
Open in desktop
Original file line number Diff line number Diff line change
Expand Up @@ -512,7 +512,11 @@ class Env
of JTx submission.
*/
void
postconditions(JTx const& jt, TER ter, bool didApply);
postconditions(
JTx const& jt,
TER ter,
bool didApply,
Json::Value const& jr = Json::Value());

/** Apply funclets and submit. */
/** @{ */
Expand Down
5 changes: 3 additions & 2 deletions src/test/jtx/Env_test.cpp
View file
Open in desktop
Original file line number Diff line number Diff line change
Expand Up @@ -747,8 +747,9 @@ class Env_test : public beast::unit_test::suite
// Force the factor low enough to fail
params[jss::fee_mult_max] = 1;
params[jss::fee_div_max] = 2;
// RPC errors result in temINVALID
envs(noop(alice), fee(none), seq(none), ter(temINVALID))(params);
// RPC errors result in telENV_RPC_FAILED
envs(noop(alice), fee(none), seq(none), ter(telENV_RPC_FAILED))(
params);

auto tx = env.tx();
BEAST_EXPECT(!tx);
Expand Down
52 changes: 33 additions & 19 deletions src/test/jtx/impl/Env.cpp
View file
Open in desktop
Original file line number Diff line number Diff line change
Expand Up @@ -280,31 +280,39 @@ Env::parseResult(Json::Value const& jr)
jr[jss::result].isMember(jss::engine_result_code))
ter = TER::fromInt(jr[jss::result][jss::engine_result_code].asInt());
else
ter = temINVALID;
// Use an error code that is not used anywhere in the transaction engine
// to distinguish this case.
ter = telENV_RPC_FAILED;
return std::make_pair(ter, isTesSuccess(ter) || isTecClaim(ter));
}

void
Env::submit(JTx const& jt)
{
bool didApply;
if (jt.stx)
{
txid_ = jt.stx->getTransactionID();
Serializer s;
jt.stx->add(s);
auto const jr = rpc("submit", strHex(s.slice()));
auto const jr = [&]() {
if (jt.stx)
{
txid_ = jt.stx->getTransactionID();
Serializer s;
jt.stx->add(s);
auto const jr = rpc("submit", strHex(s.slice()));

std::tie(ter_, didApply) = parseResult(jr);
}
else
{
// Parsing failed or the JTx is
// otherwise missing the stx field.
ter_ = temMALFORMED;
didApply = false;
}
return postconditions(jt, ter_, didApply);
std::tie(ter_, didApply) = parseResult(jr);

return jr;
}
else
{
// Parsing failed or the JTx is
// otherwise missing the stx field.
ter_ = temMALFORMED;
didApply = false;

return Json::Value();
}
}();
return postconditions(jt, ter_, didApply, jr);
Copy link
Collaborator

Choose a reason for hiding this comment

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

this is a useful change, thanks! it saves developers from printing out the suspicious outputs in debug sessions

}

void
Expand Down Expand Up @@ -342,11 +350,15 @@ Env::sign_and_submit(JTx const& jt, Json::Value params)

std::tie(ter_, didApply) = parseResult(jr);

return postconditions(jt, ter_, didApply);
return postconditions(jt, ter_, didApply, jr);
}

void
Env::postconditions(JTx const& jt, TER ter, bool didApply)
Env::postconditions(
JTx const& jt,
TER ter,
bool didApply,
Json::Value const& jr)
{
if (jt.ter &&
!test.expect(
Expand All @@ -356,6 +368,8 @@ Env::postconditions(JTx const& jt, TER ter, bool didApply)
transHuman(*jt.ter) + ")"))
{
test.log << pretty(jt.jv) << std::endl;
if (jr)
test.log << pretty(jr) << std::endl;
// Don't check postconditions if
// we didn't get the expected result.
return;
Expand Down
7 changes: 7 additions & 0 deletions src/test/net/DatabaseDownloader_test.cpp
View file
Open in desktop
Original file line number Diff line number Diff line change
Expand Up @@ -147,6 +147,7 @@ class DatabaseDownloader_test : public beast::unit_test::suite
// server to request from. Use the /textfile endpoint
// to get a simple text file sent as response.
auto server = createServer(env);
log << "Downloading DB from " << server->local_endpoint() << std::endl;

ripple::test::detail::FileDirGuard const data{
*this, "downloads", "data", "", false, false};
Expand Down Expand Up @@ -225,6 +226,8 @@ class DatabaseDownloader_test : public beast::unit_test::suite
auto server = createServer(env);
auto host = server->local_endpoint().address().to_string();
auto port = std::to_string(server->local_endpoint().port());
log << "Downloading DB from " << server->local_endpoint()
<< std::endl;
server->stop();
BEAST_EXPECT(dl->download(
host,
Expand All @@ -249,6 +252,8 @@ class DatabaseDownloader_test : public beast::unit_test::suite
ripple::test::detail::FileDirGuard const datafile{
*this, "downloads", "data", "", false, false};
auto server = createServer(env, false);
log << "Downloading DB from " << server->local_endpoint()
<< std::endl;
BEAST_EXPECT(dl->download(
server->local_endpoint().address().to_string(),
std::to_string(server->local_endpoint().port()),
Expand All @@ -272,6 +277,8 @@ class DatabaseDownloader_test : public beast::unit_test::suite
ripple::test::detail::FileDirGuard const datafile{
*this, "downloads", "data", "", false, false};
auto server = createServer(env);
log << "Downloading DB from " << server->local_endpoint()
<< std::endl;
BEAST_EXPECT(dl->download(
server->local_endpoint().address().to_string(),
std::to_string(server->local_endpoint().port()),
Expand Down
10 changes: 5 additions & 5 deletions src/test/protocol/Memo_test.cpp
View file
Open in desktop
Original file line number Diff line number Diff line change
Expand Up @@ -56,7 +56,7 @@ class Memo_test : public beast::unit_test::suite
JTx memoSize = makeJtxWithMemo();
memoSize.jv[sfMemos.jsonName][0u][sfMemo.jsonName]
[sfMemoData.jsonName] = std::string(2020, '0');
env(memoSize, ter(temINVALID));
env(memoSize, ter(telENV_RPC_FAILED));

// This memo is just barely small enough.
memoSize.jv[sfMemos.jsonName][0u][sfMemo.jsonName]
Expand All @@ -72,23 +72,23 @@ class Memo_test : public beast::unit_test::suite
auto& m = mi[sfCreatedNode.jsonName]; // CreatedNode in Memos
m[sfMemoData.jsonName] = "3030303030";

env(memoNonMemo, ter(temINVALID));
env(memoNonMemo, ter(telENV_RPC_FAILED));
}
{
// Put an invalid field in a Memo object.
JTx memoExtra = makeJtxWithMemo();
memoExtra
.jv[sfMemos.jsonName][0u][sfMemo.jsonName][sfFlags.jsonName] =
13;
env(memoExtra, ter(temINVALID));
env(memoExtra, ter(telENV_RPC_FAILED));
Copy link
Collaborator

Choose a reason for hiding this comment

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

To emphasize my point: this test case underscores invalid fields in a memo object. It is more appropriate to return a temINVALID code, instead of the current change. That would help localize telENV_RPC_FAILED to a few places in the code base.

Copy link
Collaborator Author

Choose a reason for hiding this comment

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

My explanation above covers this test case as well.

}
{
// Put a character that is not allowed in a URL in a MemoType field.
JTx memoBadChar = makeJtxWithMemo();
memoBadChar.jv[sfMemos.jsonName][0u][sfMemo.jsonName]
[sfMemoType.jsonName] =
strHex(std::string_view("ONE<INFINITY"));
env(memoBadChar, ter(temINVALID));
env(memoBadChar, ter(telENV_RPC_FAILED));
}
{
// Put a character that is not allowed in a URL in a MemoData field.
Expand All @@ -105,7 +105,7 @@ class Memo_test : public beast::unit_test::suite
memoBadChar.jv[sfMemos.jsonName][0u][sfMemo.jsonName]
[sfMemoFormat.jsonName] =
strHex(std::string_view("NoBraces{}InURL"));
env(memoBadChar, ter(temINVALID));
env(memoBadChar, ter(telENV_RPC_FAILED));
}
}

Expand Down
Loading