From 0cb26b057d6fe00d982eee1b225dbef331f5dcb7 Mon Sep 17 00:00:00 2001 From: Ed Hennis Date: Wed, 3 Apr 2024 18:57:47 -0400 Subject: [PATCH 1/4] Add RPC error checking support to unit tests --- src/test/app/MultiSign_test.cpp | 11 +++- src/test/app/Regression_test.cpp | 4 +- src/test/app/TxQ_test.cpp | 9 ++- src/test/jtx.h | 1 + src/test/jtx/Env.h | 18 +++++- src/test/jtx/Env_test.cpp | 8 ++- src/test/jtx/JTx.h | 3 + src/test/jtx/impl/Env.cpp | 100 +++++++++++++++++++++++-------- src/test/jtx/rpc.h | 86 ++++++++++++++++++++++++++ src/test/protocol/Memo_test.cpp | 27 +++++++-- 10 files changed, 222 insertions(+), 45 deletions(-) create mode 100644 src/test/jtx/rpc.h diff --git a/src/test/app/MultiSign_test.cpp b/src/test/app/MultiSign_test.cpp index 6e918e36c79..0e151b38d0a 100644 --- a/src/test/app/MultiSign_test.cpp +++ b/src/test/app/MultiSign_test.cpp @@ -250,7 +250,8 @@ class MultiSign_test : public beast::unit_test::suite env(noop(alice), msig(demon, demon), fee(3 * baseFee), - ter(telENV_RPC_FAILED)); + rpc("invalidTransaction", + "fails local checks: Duplicate Signers not allowed.")); env.close(); BEAST_EXPECT(env.seq(alice) == aliceSeq); @@ -361,7 +362,10 @@ 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(telENV_RPC_FAILED)); + env(noop(alice), + phantoms, + rpc("invalidTransaction", + "fails local checks: Unsorted Signers array.")); env.close(); BEAST_EXPECT(env.seq(alice) == aliceSeq); } @@ -1640,7 +1644,8 @@ class MultiSign_test : public beast::unit_test::suite env(noop(alice), msig(demon, demon), fee(3 * baseFee), - ter(telENV_RPC_FAILED)); + rpc("invalidTransaction", + "fails local checks: Duplicate Signers not allowed.")); env.close(); BEAST_EXPECT(env.seq(alice) == aliceSeq); diff --git a/src/test/app/Regression_test.cpp b/src/test/app/Regression_test.cpp index e2c4b355a9d..f743a30f079 100644 --- a/src/test/app/Regression_test.cpp +++ b/src/test/app/Regression_test.cpp @@ -149,7 +149,9 @@ struct Regression_test : public beast::unit_test::suite secp256r1Sig->setFieldVL(sfSigningPubKey, *pubKeyBlob); jt.stx.reset(secp256r1Sig.release()); - env(jt, ter(telENV_RPC_FAILED)); + env(jt, + rpc("invalidTransaction", + "fails local checks: Invalid signature.")); }; Account const alice{"alice", KeyType::secp256k1}; diff --git a/src/test/app/TxQ_test.cpp b/src/test/app/TxQ_test.cpp index 31e0b7b8124..50134a351e0 100644 --- a/src/test/app/TxQ_test.cpp +++ b/src/test/app/TxQ_test.cpp @@ -1058,16 +1058,15 @@ class TxQPosNegFlows_test : public beast::unit_test::suite auto const& jt = env.jt(noop(alice)); BEAST_EXPECT(jt.stx); - bool didApply; - TER ter; + Env::ParsedResult parsed; env.app().openLedger().modify( [&](OpenView& view, beast::Journal j) { - std::tie(ter, didApply) = ripple::apply( + std::tie(parsed.ter, parsed.didApply) = ripple::apply( env.app(), view, *jt.stx, tapNONE, env.journal); - return didApply; + return parsed.didApply; }); - env.postconditions(jt, ter, didApply); + env.postconditions(jt, parsed); } checkMetrics(__LINE__, env, 1, std::nullopt, 4, 2, 256); diff --git a/src/test/jtx.h b/src/test/jtx.h index 03bbf154e63..e6651fc1f0d 100644 --- a/src/test/jtx.h +++ b/src/test/jtx.h @@ -54,6 +54,7 @@ #include #include #include +#include #include #include #include diff --git a/src/test/jtx/Env.h b/src/test/jtx/Env.h index 72cac29040a..1d62669c33a 100644 --- a/src/test/jtx/Env.h +++ b/src/test/jtx/Env.h @@ -120,6 +120,19 @@ class Env Account const& master = Account::master; + /// Used by parseResult() and postConditions() + struct ParsedResult + { + TER ter; + // One way that RPC errors are returned + error_code_i rpcCode = rpcSUCCESS; + std::string rpcMessage; + // Another way that RPC errors are returned + std::string rpcError; + std::string rpcException; + bool didApply; + }; + private: struct AppBundle { @@ -493,7 +506,7 @@ class Env /** Gets the TER result and `didApply` flag from a RPC Json result object. */ - static std::pair + static ParsedResult parseResult(Json::Value const& jr); /** Submit an existing JTx. @@ -514,8 +527,7 @@ class Env void postconditions( JTx const& jt, - TER ter, - bool didApply, + ParsedResult const& parsed, Json::Value const& jr = Json::Value()); /** Apply funclets and submit. */ diff --git a/src/test/jtx/Env_test.cpp b/src/test/jtx/Env_test.cpp index 8a42b554b8e..f1cc4c37fc9 100644 --- a/src/test/jtx/Env_test.cpp +++ b/src/test/jtx/Env_test.cpp @@ -748,8 +748,12 @@ class Env_test : public beast::unit_test::suite params[jss::fee_mult_max] = 1; params[jss::fee_div_max] = 2; // RPC errors result in telENV_RPC_FAILED - envs(noop(alice), fee(none), seq(none), ter(telENV_RPC_FAILED))( - params); + envs( + noop(alice), + fee(none), + seq(none), + rpc(rpcHIGH_FEE, + "Fee of 10 exceeds the requested tx limit of 5"))(params); auto tx = env.tx(); BEAST_EXPECT(!tx); diff --git a/src/test/jtx/JTx.h b/src/test/jtx/JTx.h index 5f73c25f4e7..06b68dda336 100644 --- a/src/test/jtx/JTx.h +++ b/src/test/jtx/JTx.h @@ -44,6 +44,9 @@ struct JTx Json::Value jv; requires_t require; std::optional ter = TER{tesSUCCESS}; + std::optional> rpcCode = std::nullopt; + std::optional>> + rpcException = std::nullopt; bool fill_fee = true; bool fill_seq = true; bool fill_sig = true; diff --git a/src/test/jtx/impl/Env.cpp b/src/test/jtx/impl/Env.cpp index 6496f7df1d2..4f659e454a7 100644 --- a/src/test/jtx/impl/Env.cpp +++ b/src/test/jtx/impl/Env.cpp @@ -272,24 +272,46 @@ Env::trust(STAmount const& amount, Account const& account) test.expect(balance(account) == start); } -std::pair +Env::ParsedResult Env::parseResult(Json::Value const& jr) { - TER ter; - if (jr.isObject() && jr.isMember(jss::result) && - jr[jss::result].isMember(jss::engine_result_code)) - ter = TER::fromInt(jr[jss::result][jss::engine_result_code].asInt()); + auto error = [](ParsedResult& parsed, Json::Value const& object) { + // Use an error code that is not used anywhere in the transaction + // engine to distinguish this case. + parsed.ter = telENV_RPC_FAILED; + // Extract information about the error + if (!object.isObject()) + return; + if (object.isMember(jss::error_code)) + parsed.rpcCode = + safe_cast(object[jss::error_code].asInt()); + if (object.isMember(jss::error_message)) + parsed.rpcMessage = object[jss::error_message].asString(); + if (object.isMember(jss::error)) + parsed.rpcError = object[jss::error].asString(); + if (object.isMember(jss::error_exception)) + parsed.rpcException = object[jss::error_exception].asString(); + }; + ParsedResult parsed; + if (jr.isObject() && jr.isMember(jss::result)) + { + auto const& result = jr[jss::result]; + if (result.isMember(jss::engine_result_code)) + parsed.ter = TER::fromInt(result[jss::engine_result_code].asInt()); + else + error(parsed, result); + } else - // 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)); + error(parsed, jr); + + parsed.didApply = isTesSuccess(parsed.ter) || isTecClaim(parsed.ter); + return parsed; } void Env::submit(JTx const& jt) { - bool didApply; + ParsedResult parsedResult; auto const jr = [&]() { if (jt.stx) { @@ -298,7 +320,8 @@ Env::submit(JTx const& jt) jt.stx->add(s); auto const jr = rpc("submit", strHex(s.slice())); - std::tie(ter_, didApply) = parseResult(jr); + parsedResult = parseResult(jr); + ter_ = parsedResult.ter; return jr; } @@ -306,20 +329,18 @@ Env::submit(JTx const& jt) { // Parsing failed or the JTx is // otherwise missing the stx field. - ter_ = temMALFORMED; - didApply = false; + parsedResult.ter = ter_ = temMALFORMED; + parsedResult.didApply = false; return Json::Value(); } }(); - return postconditions(jt, ter_, didApply, jr); + return postconditions(jt, parsedResult, jr); } void Env::sign_and_submit(JTx const& jt, Json::Value params) { - bool didApply; - auto const account = lookup(jt.jv[jss::Account].asString()); auto const& passphrase = account.name(); @@ -348,24 +369,51 @@ Env::sign_and_submit(JTx const& jt, Json::Value params) if (!txid_.parseHex(jr[jss::result][jss::tx_json][jss::hash].asString())) txid_.zero(); - std::tie(ter_, didApply) = parseResult(jr); + ParsedResult const parsedResult = parseResult(jr); + ter_ = parsedResult.ter; - return postconditions(jt, ter_, didApply, jr); + return postconditions(jt, parsedResult, jr); } void Env::postconditions( JTx const& jt, - TER ter, - bool didApply, + ParsedResult const& parsed, Json::Value const& jr) { - if (jt.ter && - !test.expect( - ter == *jt.ter, - "apply: Got " + transToken(ter) + " (" + transHuman(ter) + - "); Expected " + transToken(*jt.ter) + " (" + - transHuman(*jt.ter) + ")")) + bool bad = + (jt.ter && + !test.expect( + parsed.ter == *jt.ter, + "apply: Got " + transToken(parsed.ter) + " (" + + transHuman(parsed.ter) + "); Expected " + transToken(*jt.ter) + + " (" + transHuman(*jt.ter) + ")")); + using namespace std::string_literals; + bad = (jt.rpcCode && + !test.expect( + parsed.rpcCode == jt.rpcCode->first && + parsed.rpcMessage == jt.rpcCode->second, + "apply: Got RPC result "s + + RPC::get_error_info(parsed.rpcCode).token.c_str() + " (" + + parsed.rpcMessage + "); Expected " + + RPC::get_error_info(jt.rpcCode->first).token.c_str() + " (" + + jt.rpcCode->second + ")")) || + bad; + // If we have an rpcCode (just checked), then the rpcException check is + // optional - the 'error' field may not be defined, but if it is, it must + // match rpcError. + bad = + (jt.rpcException && + !test.expect( + (jt.rpcCode && parsed.rpcError.empty()) || + (parsed.rpcError == jt.rpcException->first && + (!jt.rpcException->second || + parsed.rpcException == *jt.rpcException->second)), + "apply: Got RPC result "s + parsed.rpcError + " (" + + parsed.rpcException + "); Expected " + jt.rpcException->first + + " (" + jt.rpcException->second.value_or("n/a") + ")")) || + bad; + if (bad) { test.log << pretty(jt.jv) << std::endl; if (jr) diff --git a/src/test/jtx/rpc.h b/src/test/jtx/rpc.h new file mode 100644 index 00000000000..0d533909ab1 --- /dev/null +++ b/src/test/jtx/rpc.h @@ -0,0 +1,86 @@ +//------------------------------------------------------------------------------ +/* + This file is part of rippled: https://github.com/ripple/rippled + Copyright (c) 2012, 2013 Ripple Labs Inc. + + Permission to use, copy, modify, and/or distribute this software for any + purpose with or without fee is hereby granted, provided that the above + copyright notice and this permission notice appear in all copies. + + THE SOFTWARE IS PROVIDED "AS IS" AND THE AUTHOR DISCLAIMS ALL WARRANTIES + WITH REGARD TO THIS SOFTWARE INCLUDING ALL IMPLIED WARRANTIES OF + MERCHANTABILITY AND FITNESS. IN NO EVENT SHALL THE AUTHOR BE LIABLE FOR + ANY SPECIAL , DIRECT, INDIRECT, OR CONSEQUENTIAL DAMAGES OR ANY DAMAGES + WHATSOEVER RESULTING FROM LOSS OF USE, DATA OR PROFITS, WHETHER IN AN + ACTION OF CONTRACT, NEGLIGENCE OR OTHER TORTIOUS ACTION, ARISING OUT OF + OR IN CONNECTION WITH THE USE OR PERFORMANCE OF THIS SOFTWARE. +*/ +//============================================================================== + +#ifndef RIPPLE_TEST_JTX_RPC_H_INCLUDED +#define RIPPLE_TEST_JTX_RPC_H_INCLUDED + +#include +#include + +namespace ripple { +namespace test { +namespace jtx { + +/** Set the expected result code for a JTx + The test will fail if the code doesn't match. +*/ +class rpc +{ +private: + std::optional code_; + std::optional errorMessage_; + std::optional error_; + std::optional errorException_; + +public: + /// If there's an error code, we expect an error message + explicit rpc(error_code_i code, std::optional m = {}) + : code_(code), errorMessage_(m) + { + } + + /// If there is not a code, we expect an exception message + explicit rpc( + std::string error, + std::optional exceptionMessage = {}) + : error_(error), errorException_(exceptionMessage) + { + } + + void + operator()(Env&, JTx& jt) const + { + // The RPC request should fail + jt.ter = telENV_RPC_FAILED; + if (code_) + { + auto const& errorInfo = RPC::get_error_info(*code_); + // When an RPC request returns an error code ('error_code'), it + // always includes an error message ('error_message'), and sometimes + // includes an error token ('error'). If it does, the error token is + // always obtained from the lookup into the ErrorInfo lookup table. + // + // Take advantage of that fact to populate jt.rpcException. The + // check will be aware of whether the rpcExcpetion can be safely + // ignored. + jt.rpcCode = { + *code_, + errorMessage_ ? *errorMessage_ : errorInfo.message.c_str()}; + jt.rpcException = {errorInfo.token.c_str(), std::nullopt}; + } + if (error_) + jt.rpcException = {*error_, errorException_}; + } +}; + +} // namespace jtx +} // namespace test +} // namespace ripple + +#endif diff --git a/src/test/protocol/Memo_test.cpp b/src/test/protocol/Memo_test.cpp index 89ae6dfe18a..e119ee76eec 100644 --- a/src/test/protocol/Memo_test.cpp +++ b/src/test/protocol/Memo_test.cpp @@ -56,7 +56,10 @@ 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(telENV_RPC_FAILED)); + env(memoSize, + rpc("invalidTransaction", + "fails local checks: The memo exceeds the maximum allowed " + "size.")); // This memo is just barely small enough. memoSize.jv[sfMemos.jsonName][0u][sfMemo.jsonName] @@ -72,7 +75,10 @@ class Memo_test : public beast::unit_test::suite auto& m = mi[sfCreatedNode.jsonName]; // CreatedNode in Memos m[sfMemoData.jsonName] = "3030303030"; - env(memoNonMemo, ter(telENV_RPC_FAILED)); + env(memoNonMemo, + rpc("invalidTransaction", + "fails local checks: A memo array may contain only Memo " + "objects.")); } { // Put an invalid field in a Memo object. @@ -80,7 +86,10 @@ class Memo_test : public beast::unit_test::suite memoExtra .jv[sfMemos.jsonName][0u][sfMemo.jsonName][sfFlags.jsonName] = 13; - env(memoExtra, ter(telENV_RPC_FAILED)); + env(memoExtra, + rpc("invalidTransaction", + "fails local checks: A memo may contain only MemoType, " + "MemoData or MemoFormat fields.")); } { // Put a character that is not allowed in a URL in a MemoType field. @@ -88,7 +97,11 @@ class Memo_test : public beast::unit_test::suite memoBadChar.jv[sfMemos.jsonName][0u][sfMemo.jsonName] [sfMemoType.jsonName] = strHex(std::string_view("ONE Date: Mon, 15 Apr 2024 13:30:39 -0400 Subject: [PATCH 2/4] Update copyright data in src/test/jtx/rpc.h Co-authored-by: Chenna Keshava B S <21219765+ckeshava@users.noreply.github.com> --- src/test/jtx/rpc.h | 2 +- 1 file changed, 1 insertion(+), 1 deletion(-) diff --git a/src/test/jtx/rpc.h b/src/test/jtx/rpc.h index 0d533909ab1..129e0d6be0b 100644 --- a/src/test/jtx/rpc.h +++ b/src/test/jtx/rpc.h @@ -1,7 +1,7 @@ //------------------------------------------------------------------------------ /* This file is part of rippled: https://github.com/ripple/rippled - Copyright (c) 2012, 2013 Ripple Labs Inc. + Copyright (c) 2024 Ripple Labs Inc. Permission to use, copy, modify, and/or distribute this software for any purpose with or without fee is hereby granted, provided that the above From f58d42de8bc52f480f418ef971e45c26fee836c3 Mon Sep 17 00:00:00 2001 From: Ed Hennis Date: Mon, 15 Apr 2024 13:40:04 -0400 Subject: [PATCH 3/4] Review feedback from @ckeshava: Clean up comments --- src/test/jtx/Env_test.cpp | 1 - src/test/jtx/rpc.h | 2 +- 2 files changed, 1 insertion(+), 2 deletions(-) diff --git a/src/test/jtx/Env_test.cpp b/src/test/jtx/Env_test.cpp index f1cc4c37fc9..5c08469e0b8 100644 --- a/src/test/jtx/Env_test.cpp +++ b/src/test/jtx/Env_test.cpp @@ -747,7 +747,6 @@ 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 telENV_RPC_FAILED envs( noop(alice), fee(none), diff --git a/src/test/jtx/rpc.h b/src/test/jtx/rpc.h index 129e0d6be0b..f9962849698 100644 --- a/src/test/jtx/rpc.h +++ b/src/test/jtx/rpc.h @@ -56,7 +56,7 @@ class rpc void operator()(Env&, JTx& jt) const { - // The RPC request should fail + // The RPC request should fail. RPC errors result in telENV_RPC_FAILED. jt.ter = telENV_RPC_FAILED; if (code_) { From 66646ca4e76253d44dc05592302828783b59f864 Mon Sep 17 00:00:00 2001 From: Ed Hennis Date: Thu, 18 Apr 2024 19:25:42 -0400 Subject: [PATCH 4/4] Review feedback from @scottschurr: * Make ter and rpcCode optional in ParsedResult * Remove didApply from ParsedResult --- src/test/app/TxQ_test.cpp | 6 ++++-- src/test/jtx/Env.h | 11 ++++++----- src/test/jtx/impl/Env.cpp | 30 ++++++++++++++++++------------ 3 files changed, 28 insertions(+), 19 deletions(-) diff --git a/src/test/app/TxQ_test.cpp b/src/test/app/TxQ_test.cpp index 50134a351e0..0ed1ae499cf 100644 --- a/src/test/app/TxQ_test.cpp +++ b/src/test/app/TxQ_test.cpp @@ -1062,9 +1062,11 @@ class TxQPosNegFlows_test : public beast::unit_test::suite env.app().openLedger().modify( [&](OpenView& view, beast::Journal j) { - std::tie(parsed.ter, parsed.didApply) = ripple::apply( + // No need to initialize, since it's about to get set + bool didApply; + std::tie(parsed.ter, didApply) = ripple::apply( env.app(), view, *jt.stx, tapNONE, env.journal); - return parsed.didApply; + return didApply; }); env.postconditions(jt, parsed); } diff --git a/src/test/jtx/Env.h b/src/test/jtx/Env.h index 1d62669c33a..55d96edbf0d 100644 --- a/src/test/jtx/Env.h +++ b/src/test/jtx/Env.h @@ -123,14 +123,15 @@ class Env /// Used by parseResult() and postConditions() struct ParsedResult { - TER ter; - // One way that RPC errors are returned - error_code_i rpcCode = rpcSUCCESS; + std::optional ter{}; + // RPC errors tend to return either a "code" and a "message" (sometimes + // with an "error" that corresponds to the "code"), or with an "error" + // and an "exception". However, this structure allows all possible + // combinations. + std::optional rpcCode{}; std::string rpcMessage; - // Another way that RPC errors are returned std::string rpcError; std::string rpcException; - bool didApply; }; private: diff --git a/src/test/jtx/impl/Env.cpp b/src/test/jtx/impl/Env.cpp index 4f659e454a7..b8322f0d90d 100644 --- a/src/test/jtx/impl/Env.cpp +++ b/src/test/jtx/impl/Env.cpp @@ -297,14 +297,16 @@ Env::parseResult(Json::Value const& jr) { auto const& result = jr[jss::result]; if (result.isMember(jss::engine_result_code)) + { parsed.ter = TER::fromInt(result[jss::engine_result_code].asInt()); + parsed.rpcCode.emplace(rpcSUCCESS); + } else error(parsed, result); } else error(parsed, jr); - parsed.didApply = isTesSuccess(parsed.ter) || isTecClaim(parsed.ter); return parsed; } @@ -321,7 +323,8 @@ Env::submit(JTx const& jt) auto const jr = rpc("submit", strHex(s.slice())); parsedResult = parseResult(jr); - ter_ = parsedResult.ter; + test.expect(parsedResult.ter, "ter uninitialized!"); + ter_ = parsedResult.ter.value_or(telENV_RPC_FAILED); return jr; } @@ -330,7 +333,6 @@ Env::submit(JTx const& jt) // Parsing failed or the JTx is // otherwise missing the stx field. parsedResult.ter = ter_ = temMALFORMED; - parsedResult.didApply = false; return Json::Value(); } @@ -370,7 +372,8 @@ Env::sign_and_submit(JTx const& jt, Json::Value params) txid_.zero(); ParsedResult const parsedResult = parseResult(jr); - ter_ = parsedResult.ter; + test.expect(parsedResult.ter, "ter uninitialized!"); + ter_ = parsedResult.ter.value_or(telENV_RPC_FAILED); return postconditions(jt, parsedResult, jr); } @@ -381,21 +384,24 @@ Env::postconditions( ParsedResult const& parsed, Json::Value const& jr) { - bool bad = - (jt.ter && + bool bad = !test.expect(parsed.ter, "apply: No ter result!"); + bad = + (jt.ter && parsed.ter && !test.expect( - parsed.ter == *jt.ter, - "apply: Got " + transToken(parsed.ter) + " (" + - transHuman(parsed.ter) + "); Expected " + transToken(*jt.ter) + - " (" + transHuman(*jt.ter) + ")")); + *parsed.ter == *jt.ter, + "apply: Got " + transToken(*parsed.ter) + " (" + + transHuman(*parsed.ter) + "); Expected " + + transToken(*jt.ter) + " (" + transHuman(*jt.ter) + ")")); using namespace std::string_literals; bad = (jt.rpcCode && !test.expect( parsed.rpcCode == jt.rpcCode->first && parsed.rpcMessage == jt.rpcCode->second, "apply: Got RPC result "s + - RPC::get_error_info(parsed.rpcCode).token.c_str() + " (" + - parsed.rpcMessage + "); Expected " + + (parsed.rpcCode + ? RPC::get_error_info(*parsed.rpcCode).token.c_str() + : "NO RESULT") + + " (" + parsed.rpcMessage + "); Expected " + RPC::get_error_info(jt.rpcCode->first).token.c_str() + " (" + jt.rpcCode->second + ")")) || bad;