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

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

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<STTx const> stx;
     std::function<void(Env&, JTx&)> signer;
 

src/test/jtx/impl/Env.cpp

@@ -534,9 +534,12 @@ Env::autofill(JTx& jt)
     if (jt.fill_seq)
         jtx::fill_seq(jv, *current());
 
-    uint32_t networkID = app().config().NETWORK_ID;
+    if (jt.fill_netid)
-    if (!jv.isMember(jss::NetworkID) && networkID > 1024)
+    {
-        jv[jss::NetworkID] = std::to_string(networkID);
+        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

src/test/rpc/JSONRPC_test.cpp

@@ -2049,6 +2049,7 @@ public:
     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 @@ public:
     void
     testAutoFillFees()
     {
+        testcase("autofill fees");
         test::jtx::Env env(*this);
         auto ledger = env.current();
         auto const& feeTrack = env.app().getFeeTrack();
@@ -2207,6 +2209,7 @@ public:
     void
     testAutoFillEscalatedFees()
     {
+        testcase("autofill escalated fees");
         using namespace test::jtx;
         Env env{*this, envconfig([](std::unique_ptr<Config> cfg) {
                     cfg->loadFromString("[" SECTION_SIGNING_SUPPORT "]\ntrue");
@@ -2538,6 +2541,32 @@ public:
         }
     }
 
+    void
+    testAutoFillNetworkID()
+    {
+        testcase("autofill NetworkID");
+        using namespace test::jtx;
+        Env env{*this, envconfig([&](std::unique_ptr<Config> 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 @@ public:
     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 @@ public:
         testBadRpcCommand();
         testAutoFillFees();
         testAutoFillEscalatedFees();
+        testAutoFillNetworkID();
         testTransactionRPC();
     }
 };

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<Config> 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<Config> 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 @@ public:
         testTransactionSigningFailure();
         testMultisignedBadPubKey();
         testDeleteExpiredCredentials();
+        testSuccessfulTransactionNetworkID();
     }
 };
 

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);
+        }
     }
 
     {

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