From d9e4009e33c32aa5162ab7f6392faf51a5542e9b Mon Sep 17 00:00:00 2001 From: Mayukha Vadari Date: Fri, 7 Feb 2025 12:17:37 -0800 Subject: [PATCH] fix: issues in `simulate` RPC (#5265) Make `simulate` RPC easier to use: * Prevent the use of `seed`, `secret`, `seed_hex`, and `passphrase` fields (to avoid confusing with the signing methods). * Add autofilling of the `NetworkID` field. --- API-CHANGELOG.md | 2 +- src/test/jtx/JTx.h | 1 + src/test/jtx/impl/Env.cpp | 9 +- src/test/rpc/JSONRPC_test.cpp | 31 +++++ src/test/rpc/Simulate_test.cpp | 155 +++++++++++++++++++++-- src/xrpld/rpc/detail/TransactionSign.cpp | 7 + src/xrpld/rpc/handlers/Simulate.cpp | 16 +++ 7 files changed, 203 insertions(+), 18 deletions(-) diff --git a/API-CHANGELOG.md b/API-CHANGELOG.md index 9f99b4ab9a4..fda03c2d00a 100644 --- a/API-CHANGELOG.md +++ b/API-CHANGELOG.md @@ -92,7 +92,7 @@ As of 2025-01-28, version 2.4.0 is in development. You can use a pre-release ver - `ledger_entry`: `state` is added an alias for `ripple_state`. - `validators`: Added new field `validator_list_threshold` in response. - `simulate`: A new RPC that executes a [dry run of a transaction submission](https://github.com/XRPLF/XRPL-Standards/tree/master/XLS-0069d-simulate#2-rpc-simulate) -- Signing methods autofill fees better and properly handle transactions that don't have a base fee. +- Signing methods autofill fees better and properly handle transactions that don't have a base fee, and will also autofill the `NetworkID` field. ## XRP Ledger server version 2.3.0 diff --git a/src/test/jtx/JTx.h b/src/test/jtx/JTx.h index a5a4a9eb1b9..81ba1b406a0 100644 --- a/src/test/jtx/JTx.h +++ b/src/test/jtx/JTx.h @@ -51,6 +51,7 @@ struct JTx bool fill_fee = true; bool fill_seq = true; bool fill_sig = true; + bool fill_netid = true; std::shared_ptr stx; std::function signer; diff --git a/src/test/jtx/impl/Env.cpp b/src/test/jtx/impl/Env.cpp index c87b1260244..43286ab7824 100644 --- a/src/test/jtx/impl/Env.cpp +++ b/src/test/jtx/impl/Env.cpp @@ -493,9 +493,12 @@ Env::autofill(JTx& jt) if (jt.fill_seq) jtx::fill_seq(jv, *current()); - uint32_t networkID = app().config().NETWORK_ID; - if (!jv.isMember(jss::NetworkID) && networkID > 1024) - jv[jss::NetworkID] = std::to_string(networkID); + if (jt.fill_netid) + { + uint32_t networkID = app().config().NETWORK_ID; + if (!jv.isMember(jss::NetworkID) && networkID > 1024) + jv[jss::NetworkID] = std::to_string(networkID); + } // Must come last try diff --git a/src/test/rpc/JSONRPC_test.cpp b/src/test/rpc/JSONRPC_test.cpp index 6841eb2af72..6e97301fc3e 100644 --- a/src/test/rpc/JSONRPC_test.cpp +++ b/src/test/rpc/JSONRPC_test.cpp @@ -2049,6 +2049,7 @@ class JSONRPC_test : public beast::unit_test::suite void testBadRpcCommand() { + testcase("bad RPC command"); test::jtx::Env env(*this); Json::Value const result{ env.rpc("bad_command", R"({"MakingThisUp": 0})")}; @@ -2061,6 +2062,7 @@ class JSONRPC_test : public beast::unit_test::suite void testAutoFillFees() { + testcase("autofill fees"); test::jtx::Env env(*this); auto ledger = env.current(); auto const& feeTrack = env.app().getFeeTrack(); @@ -2207,6 +2209,7 @@ class JSONRPC_test : public beast::unit_test::suite void testAutoFillEscalatedFees() { + testcase("autofill escalated fees"); using namespace test::jtx; Env env{*this, envconfig([](std::unique_ptr cfg) { cfg->loadFromString("[" SECTION_SIGNING_SUPPORT "]\ntrue"); @@ -2538,6 +2541,32 @@ class JSONRPC_test : public beast::unit_test::suite } } + void + testAutoFillNetworkID() + { + testcase("autofill NetworkID"); + using namespace test::jtx; + Env env{*this, envconfig([&](std::unique_ptr cfg) { + cfg->NETWORK_ID = 1025; + return cfg; + })}; + + { + Json::Value toSign; + toSign[jss::tx_json] = noop(env.master); + + BEAST_EXPECT(!toSign[jss::tx_json].isMember(jss::NetworkID)); + toSign[jss::secret] = "masterpassphrase"; + auto rpcResult = env.rpc("json", "sign", to_string(toSign)); + auto result = rpcResult[jss::result]; + + BEAST_EXPECT(!RPC::contains_error(result)); + BEAST_EXPECT( + result[jss::tx_json].isMember(jss::NetworkID) && + result[jss::tx_json][jss::NetworkID] == 1025); + } + } + // A function that can be called as though it would process a transaction. static void fakeProcessTransaction( @@ -2552,6 +2581,7 @@ class JSONRPC_test : public beast::unit_test::suite void testTransactionRPC() { + testcase("sign/submit RPCs"); using namespace std::chrono_literals; // Use jtx to set up a ledger so the tests will do the right thing. test::jtx::Account const a{"a"}; // rnUy2SHTrB9DubsPmkJZUXTf5FcNDGrYEA @@ -2678,6 +2708,7 @@ class JSONRPC_test : public beast::unit_test::suite testBadRpcCommand(); testAutoFillFees(); testAutoFillEscalatedFees(); + testAutoFillNetworkID(); testTransactionRPC(); } }; diff --git a/src/test/rpc/Simulate_test.cpp b/src/test/rpc/Simulate_test.cpp index 636f1d04cd0..656e5f0577e 100644 --- a/src/test/rpc/Simulate_test.cpp +++ b/src/test/rpc/Simulate_test.cpp @@ -93,13 +93,17 @@ class Simulate_test : public beast::unit_test::suite validate, bool testSerialized = true) { + env.close(); + Json::Value params; params[jss::tx_json] = tx; validate(env.rpc("json", "simulate", to_string(params)), tx); + params[jss::binary] = true; validate(env.rpc("json", "simulate", to_string(params)), tx); validate(env.rpc("simulate", to_string(tx)), tx); validate(env.rpc("simulate", to_string(tx), "binary"), tx); + if (testSerialized) { // This cannot be tested in the multisign autofill scenario @@ -119,6 +123,10 @@ class Simulate_test : public beast::unit_test::suite validate(env.rpc("simulate", tx_blob), tx); validate(env.rpc("simulate", tx_blob, "binary"), tx); } + + BEAST_EXPECTS( + env.current()->txCount() == 0, + std::to_string(env.current()->txCount())); } Json::Value @@ -235,6 +243,58 @@ class Simulate_test : public beast::unit_test::suite resp[jss::result][jss::error_message] == "Invalid field 'tx_json', not object."); } + { + // `seed` field included + Json::Value params = Json::objectValue; + params[jss::seed] = "doesnt_matter"; + Json::Value tx_json = Json::objectValue; + tx_json[jss::TransactionType] = jss::AccountSet; + tx_json[jss::Account] = env.master.human(); + params[jss::tx_json] = tx_json; + auto const resp = env.rpc("json", "simulate", to_string(params)); + BEAST_EXPECT( + resp[jss::result][jss::error_message] == + "Invalid field 'seed'."); + } + { + // `secret` field included + Json::Value params = Json::objectValue; + params[jss::secret] = "doesnt_matter"; + Json::Value tx_json = Json::objectValue; + tx_json[jss::TransactionType] = jss::AccountSet; + tx_json[jss::Account] = env.master.human(); + params[jss::tx_json] = tx_json; + auto const resp = env.rpc("json", "simulate", to_string(params)); + BEAST_EXPECT( + resp[jss::result][jss::error_message] == + "Invalid field 'secret'."); + } + { + // `seed_hex` field included + Json::Value params = Json::objectValue; + params[jss::seed_hex] = "doesnt_matter"; + Json::Value tx_json = Json::objectValue; + tx_json[jss::TransactionType] = jss::AccountSet; + tx_json[jss::Account] = env.master.human(); + params[jss::tx_json] = tx_json; + auto const resp = env.rpc("json", "simulate", to_string(params)); + BEAST_EXPECT( + resp[jss::result][jss::error_message] == + "Invalid field 'seed_hex'."); + } + { + // `passphrase` field included + Json::Value params = Json::objectValue; + params[jss::passphrase] = "doesnt_matter"; + Json::Value tx_json = Json::objectValue; + tx_json[jss::TransactionType] = jss::AccountSet; + tx_json[jss::Account] = env.master.human(); + params[jss::tx_json] = tx_json; + auto const resp = env.rpc("json", "simulate", to_string(params)); + BEAST_EXPECT( + resp[jss::result][jss::error_message] == + "Invalid field 'passphrase'."); + } { // Invalid transaction Json::Value params = Json::objectValue; @@ -412,7 +472,10 @@ class Simulate_test : public beast::unit_test::suite testcase("Successful transaction"); using namespace jtx; - Env env(*this); + Env env{*this, envconfig([&](std::unique_ptr cfg) { + cfg->NETWORK_ID = 0; + return cfg; + })}; static auto const newDomain = "123ABC"; { @@ -473,8 +536,6 @@ class Simulate_test : public beast::unit_test::suite // test without autofill testTx(env, tx, validateOutput); - - // TODO: check that the ledger wasn't affected } } @@ -523,8 +584,6 @@ class Simulate_test : public beast::unit_test::suite // test without autofill testTx(env, tx, testSimulation); - - // TODO: check that the ledger wasn't affected } } @@ -604,8 +663,6 @@ class Simulate_test : public beast::unit_test::suite // test without autofill testTx(env, tx, testSimulation); - - // TODO: check that the ledger wasn't affected } } @@ -625,6 +682,7 @@ class Simulate_test : public beast::unit_test::suite // set up valid multisign env(signers(alice, 1, {{becky, 1}, {carol, 1}})); + env.close(); { auto validateOutput = [&](Json::Value const& resp, @@ -662,7 +720,7 @@ class Simulate_test : public beast::unit_test::suite BEAST_EXPECT(finalFields[sfDomain] == newDomain); } } - BEAST_EXPECT(metadata[sfTransactionIndex.jsonName] == 1); + BEAST_EXPECT(metadata[sfTransactionIndex.jsonName] == 0); BEAST_EXPECT( metadata[sfTransactionResult.jsonName] == "tesSUCCESS"); } @@ -697,8 +755,6 @@ class Simulate_test : public beast::unit_test::suite // test without autofill testTx(env, tx, validateOutput); - - // TODO: check that the ledger wasn't affected } } @@ -754,8 +810,6 @@ class Simulate_test : public beast::unit_test::suite // test without autofill testTx(env, tx, testSimulation); - - // TODO: check that the ledger wasn't affected } } @@ -825,8 +879,6 @@ class Simulate_test : public beast::unit_test::suite // test without autofill testTx(env, tx, validateOutput); - - // TODO: check that the ledger wasn't affected } } @@ -948,6 +1000,80 @@ class Simulate_test : public beast::unit_test::suite BEAST_EXPECT(ownerCount(env, subject) == 0); } + void + testSuccessfulTransactionNetworkID() + { + testcase("Successful transaction with a custom network ID"); + + using namespace jtx; + Env env{*this, envconfig([&](std::unique_ptr cfg) { + cfg->NETWORK_ID = 1025; + return cfg; + })}; + static auto const newDomain = "123ABC"; + + { + auto validateOutput = [&](Json::Value const& resp, + Json::Value const& tx) { + auto result = resp[jss::result]; + checkBasicReturnValidity( + result, tx, 1, env.current()->fees().base); + + BEAST_EXPECT(result[jss::engine_result] == "tesSUCCESS"); + BEAST_EXPECT(result[jss::engine_result_code] == 0); + BEAST_EXPECT( + result[jss::engine_result_message] == + "The simulated transaction would have been applied."); + + if (BEAST_EXPECT( + result.isMember(jss::meta) || + result.isMember(jss::meta_blob))) + { + Json::Value const metadata = getJsonMetadata(result); + + if (BEAST_EXPECT( + metadata.isMember(sfAffectedNodes.jsonName))) + { + BEAST_EXPECT( + metadata[sfAffectedNodes.jsonName].size() == 1); + auto node = metadata[sfAffectedNodes.jsonName][0u]; + if (BEAST_EXPECT( + node.isMember(sfModifiedNode.jsonName))) + { + auto modifiedNode = node[sfModifiedNode]; + BEAST_EXPECT( + modifiedNode[sfLedgerEntryType] == + "AccountRoot"); + auto finalFields = modifiedNode[sfFinalFields]; + BEAST_EXPECT(finalFields[sfDomain] == newDomain); + } + } + BEAST_EXPECT(metadata[sfTransactionIndex.jsonName] == 0); + BEAST_EXPECT( + metadata[sfTransactionResult.jsonName] == "tesSUCCESS"); + } + }; + + Json::Value tx; + + tx[jss::Account] = env.master.human(); + tx[jss::TransactionType] = jss::AccountSet; + tx[sfDomain] = newDomain; + + // test with autofill + testTx(env, tx, validateOutput); + + tx[sfSigningPubKey] = ""; + tx[sfTxnSignature] = ""; + tx[sfSequence] = 1; + tx[sfFee] = env.current()->fees().base.jsonClipped().asString(); + tx[sfNetworkID] = 1025; + + // test without autofill + testTx(env, tx, validateOutput); + } + } + public: void run() override @@ -961,6 +1087,7 @@ class Simulate_test : public beast::unit_test::suite testTransactionSigningFailure(); testMultisignedBadPubKey(); testDeleteExpiredCredentials(); + testSuccessfulTransactionNetworkID(); } }; diff --git a/src/xrpld/rpc/detail/TransactionSign.cpp b/src/xrpld/rpc/detail/TransactionSign.cpp index d8e4758ddd3..376a0ce24a5 100644 --- a/src/xrpld/rpc/detail/TransactionSign.cpp +++ b/src/xrpld/rpc/detail/TransactionSign.cpp @@ -467,6 +467,13 @@ transactionPreProcessImpl( if (!tx_json.isMember(jss::Flags)) tx_json[jss::Flags] = tfFullyCanonicalSig; + + if (!tx_json.isMember(jss::NetworkID)) + { + auto const networkId = app.config().NETWORK_ID; + if (networkId > 1024) + tx_json[jss::NetworkID] = to_string(networkId); + } } { diff --git a/src/xrpld/rpc/handlers/Simulate.cpp b/src/xrpld/rpc/handlers/Simulate.cpp index 538f6803f8c..7d391497f63 100644 --- a/src/xrpld/rpc/handlers/Simulate.cpp +++ b/src/xrpld/rpc/handlers/Simulate.cpp @@ -144,6 +144,13 @@ autofillTx(Json::Value& tx_json, RPC::JsonContext& context) tx_json[sfSequence.jsonName] = *seq; } + if (!tx_json.isMember(jss::NetworkID)) + { + auto const networkId = context.app.config().NETWORK_ID; + if (networkId > 1024) + tx_json[jss::NetworkID] = to_string(networkId); + } + return std::nullopt; } @@ -299,6 +306,15 @@ doSimulate(RPC::JsonContext& context) return RPC::invalid_field_error(jss::binary); } + for (auto const field : + {jss::secret, jss::seed, jss::seed_hex, jss::passphrase}) + { + if (context.params.isMember(field)) + { + return RPC::invalid_field_error(field); + } + } + // get JSON equivalent of transaction tx_json = getTxJsonFromParams(context.params); if (tx_json.isMember(jss::error))