feat(rpc): greatly enhance simulate RPC#5637
Conversation
Codecov Report❌ Patch coverage is
Additional details and impacted files@@ Coverage Diff @@
## develop #5637 +/- ##
========================================
Coverage 79.8% 79.8%
========================================
Files 858 858
Lines 67757 67888 +131
Branches 7557 7560 +3
========================================
+ Hits 54064 54190 +126
- Misses 13693 13698 +5
🚀 New features to boost your workflow:
|
…t break anything)
simulateing transactions in past ledgerssimulateing transactions in past ledgers
simulateing transactions in past ledgerssimulate RPC
Reminder that rippled/src/xrpld/app/tx/detail/Transactor.cpp Line 1054 in 39b5031 |
There was a problem hiding this comment.
Pull request overview
Copilot reviewed 8 out of 8 changed files in this pull request and generated 5 comments.
💡 Add Copilot custom instructions for smarter, more guided reviews. Learn how to get started.
| * cannot use `simulate` to bypass signature checks and submit | ||
| * transactions/modify the current ledger directly. | ||
| ***************************************/ | ||
| auto const result = apply(context.app, perTxView, txn, tapDRY_RUN, context.j); |
There was a problem hiding this comment.
Simulation now uses the low-level apply(...) call unconditionally. For the current/open ledger this bypasses TxQ::apply behavior (fee escalation, queuing decisions, and related local checks), so simulate results may diverge from what would happen when actually submitting a transaction under load. If the intent is to match submit semantics on the current ledger, consider using context.app.getTxQ().apply(..., tapDRY_RUN, ...) when isCurrentLedger is true, and only use apply(...) for historical-ledger simulation.
| auto const result = apply(context.app, perTxView, txn, tapDRY_RUN, context.j); | |
| ApplyResult const result = | |
| isCurrentLedger | |
| ? context.app.getTxQ().apply( | |
| context.app, perTxView, txn, tapDRY_RUN, context.j) | |
| : apply(context.app, perTxView, txn, tapDRY_RUN, context.j); |
| if (tx_json.isMember(jss::error)) | ||
| return tx_json; | ||
| return Unexpected(tx_json); | ||
|
|
There was a problem hiding this comment.
For multi-transaction requests, autofill (especially Sequence) is performed in processTransaction using lpLedger before any earlier simulated transactions are applied. This can cause later transactions in the same request to be autofilled with the same Sequence and then fail (the test currently works around this by manually setting the second Sequence). Consider either (1) moving autofill into the simulation loop so it can consult the evolving view, or (2) requiring clients to provide explicit Sequence values when transactions contains more than one entry.
| // If this is a multi-transaction simulate request, require that each | |
| // transaction explicitly specify Sequence or TicketSequence so that | |
| // we do not autofill potentially conflicting values from the same | |
| // base ledger view for multiple transactions. | |
| bool const isMultiTxRequest = | |
| context.params.isMember(jss::transactions) && | |
| context.params[jss::transactions].isArray() && | |
| context.params[jss::transactions].size() > 1; | |
| if (isMultiTxRequest) | |
| { | |
| bool const hasTicketSeq = tx_json.isMember(sfTicketSequence.jsonName); | |
| bool const hasSeq = tx_json.isMember(sfSequence.jsonName); | |
| if (!hasTicketSeq && !hasSeq) | |
| { | |
| Json::Value jvResult = Json::objectValue; | |
| jvResult[jss::error] = rpcINVALID_PARAMS; | |
| jvResult[jss::error_message] = | |
| "When submitting multiple transactions in a single simulate " | |
| "request, each transaction must specify either Sequence or " | |
| "TicketSequence."; | |
| return Unexpected(jvResult); | |
| } | |
| } |
| { | ||
| context.loadType = Resource::feeMediumBurdenRPC; | ||
| Json::Value jvTransactions = Json::arrayValue; | ||
| std::vector<ApplyResult> results; |
There was a problem hiding this comment.
simulateTxn declares std::vector<ApplyResult> results; but never uses it. Removing the unused variable would reduce noise and avoid confusion about whether results are meant to be collected.
| std::vector<ApplyResult> results; |
src/xrpld/rpc/handlers/Simulate.cpp
Outdated
| #include <xrpld/app/ledger/TransactionMaster.h> | ||
| #include <xrpld/app/misc/HashRouter.h> | ||
| #include <xrpld/app/misc/Transaction.h> | ||
| #include <xrpld/app/misc/TxQ.h> | ||
| #include <xrpld/app/tx/apply.h> |
There was a problem hiding this comment.
The added HashRouter includes (<xrpld/app/misc/HashRouter.h> / <xrpl/core/HashRouter.h>) don't appear to be used anywhere in this file. If they aren't required for compilation, consider removing them to keep dependencies minimal.
| else | ||
| { | ||
| return RPC::make_param_error("Neither `tx_blob` nor `tx_json` included."); | ||
| auto const result = getTxJsonFromHistory(context, isCurrentLedger); | ||
| if (result.isMember(jss::error)) | ||
| { | ||
| return result; | ||
| } | ||
| else | ||
| { | ||
| tx_json = result; | ||
| } | ||
| } |
There was a problem hiding this comment.
getTxJsonFromParams falls back to getTxJsonFromHistory(context, ...), but getTxJsonFromHistory reads context.params instead of the per-transaction txInput. In a batch request (transactions array), items that specify tx_hash or ctid will therefore not be read correctly (and may end up fetching using a default/zero hash). Consider passing the per-item JSON into getTxJsonFromHistory (or parsing tx_hash/ctid directly from txInput) so batch history lookups work as intended.
There was a problem hiding this comment.
Pull request overview
Copilot reviewed 8 out of 8 changed files in this pull request and generated 10 comments.
💡 Add Copilot custom instructions for smarter, more guided reviews. Learn how to get started.
| STTx const txn( | ||
| static_cast<TxType>(rawTxn.getFieldU16(sfTransactionType)), | ||
| [&rawTxn](STObject& obj) { obj = STObject(rawTxn); }); | ||
| jvTransactions.append( | ||
| processResult(innerResult, txn, isBinaryOutput, wholeBatchView)); |
There was a problem hiding this comment.
Variable shadowing: the inner txn variable declared here shadows the outer txn variable from line 402. This can lead to confusion and potential bugs. Consider renaming the inner variable to something like innerTxn or batchInnerTxn to make the distinction clear.
| STTx const txn( | |
| static_cast<TxType>(rawTxn.getFieldU16(sfTransactionType)), | |
| [&rawTxn](STObject& obj) { obj = STObject(rawTxn); }); | |
| jvTransactions.append( | |
| processResult(innerResult, txn, isBinaryOutput, wholeBatchView)); | |
| STTx const innerTxn( | |
| static_cast<TxType>(rawTxn.getFieldU16(sfTransactionType)), | |
| [&rawTxn](STObject& obj) { obj = STObject(rawTxn); }); | |
| jvTransactions.append( | |
| processResult(innerResult, innerTxn, isBinaryOutput, wholeBatchView)); |
src/xrpld/rpc/handlers/Simulate.cpp
Outdated
| context.registry.config(), | ||
| context.registry.getFeeTrack(), | ||
| context.registry.getTxQ(), | ||
| context.registry, |
There was a problem hiding this comment.
The RPC::JsonContext struct does not have a registry member. Based on the Context.h file, the member is named app. All occurrences of context.registry in this function (lines 141-144) should be changed to context.app.
src/xrpld/rpc/handlers/Simulate.cpp
Outdated
| if (!tx_json.isMember(jss::NetworkID)) | ||
| { | ||
| auto const networkId = context.app.getNetworkIDService().getNetworkID(); | ||
| auto const networkId = context.registry.getNetworkIDService().getNetworkID(); |
There was a problem hiding this comment.
The RPC::JsonContext struct does not have a registry member. Based on the Context.h file, the member is named app. This should be context.app.getNetworkIDService() instead of context.registry.getNetworkIDService().
| auto const networkId = context.registry.getNetworkIDService().getNetworkID(); | |
| auto const networkId = context.app.getNetworkIDService().getNetworkID(); |
src/xrpld/rpc/handlers/Simulate.cpp
Outdated
| if (auto const optHash = context.registry.getLedgerMaster().txnIdFromIndex(ledgerSq, txId); | ||
| optHash) | ||
| { | ||
| hash = *optHash; | ||
| } | ||
| else | ||
| { | ||
| return RPC::make_error(rpcTXN_NOT_FOUND); | ||
| } | ||
| } | ||
| using TxPair = std::pair<std::shared_ptr<Transaction>, std::shared_ptr<TxMeta>>; | ||
| auto ec{rpcSUCCESS}; | ||
| std::variant<TxPair, TxSearched> v = context.registry.getMasterTransaction().fetch(hash, ec); |
There was a problem hiding this comment.
The RPC::JsonContext struct does not have a registry member. Based on the Context.h file, the member is named app. All occurrences of context.registry in this function (lines 217, 229) should be changed to context.app.
src/xrpld/rpc/handlers/Simulate.cpp
Outdated
| auto const result = apply(context.registry, perTxView, txn, tapDRY_RUN, context.j); | ||
| if (isTesSuccess(result.ter) || isTecClaim(result.ter)) | ||
| perTxView.apply(view); | ||
| jvTransactions.append(processResult(result, txn, isBinaryOutput, view)); | ||
|
|
||
| if (isTesSuccess(result.ter) && txn.getTxnType() == ttBATCH) | ||
| { | ||
| OpenView wholeBatchView(batch_view, view); | ||
|
|
||
| // check validity of `binary` param | ||
| if (context.params.isMember(jss::binary) && !context.params[jss::binary].isBool()) | ||
| if (auto const batchResults = applyBatchTransactions( | ||
| context.registry, wholeBatchView, txn, tapDRY_RUN, context.j); |
There was a problem hiding this comment.
The RPC::JsonContext struct does not have a registry member. Based on the Context.h file, the member is named app. All occurrences of context.registry in this function (lines 411, 421) should be changed to context.app.
| std::optional<std::vector<ApplyResult>> | ||
| applyBatchTransactions( | ||
| Application& app, | ||
| OpenView& batchView, | ||
| STTx const& batchTxn, | ||
| ApplyFlags flags, | ||
| beast::Journal j); |
There was a problem hiding this comment.
The function signature in the header declares Application& app as the first parameter, but the implementation in src/libxrpl/tx/apply.cpp (line 140) uses ServiceRegistry& registry. These must match. Since Application inherits from ServiceRegistry and the implementation uses ServiceRegistry, the header should be updated to use ServiceRegistry& registry for consistency with the implementation.
| context.registry, wholeBatchView, txn, tapDRY_RUN, context.j); | ||
| batchResults) | ||
| { | ||
| for (int i = 0; i < batchResults->size(); ++i) |
There was a problem hiding this comment.
Using int for the loop counter when iterating over batchResults->size() could cause signed/unsigned comparison warnings or issues. Use std::size_t or an unsigned type instead, consistent with the return type of size().
| for (int i = 0; i < batchResults->size(); ++i) | |
| for (std::size_t i = 0; i < batchResults->size(); ++i) |
src/xrpld/rpc/handlers/Simulate.cpp
Outdated
| if (!sle) | ||
| { | ||
| JLOG(context.app.journal("Simulate").debug()) | ||
| JLOG(context.registry.journal("Simulate").debug()) |
There was a problem hiding this comment.
The RPC::JsonContext struct does not have a registry member. Based on the Context.h file, the member is named app. This should be context.app.journal("Simulate").debug() instead of context.registry.journal("Simulate").debug().
src/xrpld/rpc/handlers/Simulate.cpp
Outdated
| return sle->getFieldU32(sfSequence); | ||
|
|
||
| return hasTicketSeq ? 0 : context.app.getTxQ().nextQueuableSeq(sle).value(); | ||
| return context.registry.getTxQ().nextQueuableSeq(sle).value(); |
There was a problem hiding this comment.
The RPC::JsonContext struct does not have a registry member. Based on the Context.h file, the member is named app. All occurrences of context.registry in this function should be changed to context.app.
src/xrpld/rpc/handlers/Simulate.cpp
Outdated
|
|
||
| if (stTx->getTxnType() == ttBATCH) | ||
| std::string reason; | ||
| return std::make_shared<Transaction>(stTx, reason, context.registry); |
There was a problem hiding this comment.
The RPC::JsonContext struct does not have a registry member. Based on the Context.h file, the member is named app. This should be context.app instead of context.registry.
| return std::make_shared<Transaction>(stTx, reason, context.registry); | |
| return std::make_shared<Transaction>(stTx, reason, context.app); |
There was a problem hiding this comment.
Pull request overview
Copilot reviewed 8 out of 8 changed files in this pull request and generated 2 comments.
💡 Add Copilot custom instructions for smarter, more guided reviews. Learn how to get started.
include/xrpl/tx/apply.h
Outdated
|
|
||
| std::optional<std::vector<ApplyResult>> | ||
| applyBatchTransactions( | ||
| Registry& registry, |
There was a problem hiding this comment.
The parameter type is declared as Registry& but should be ServiceRegistry& to match the implementation in apply.cpp. The type Registry is not defined in the codebase and this will cause a compilation error.
| Registry& registry, | |
| ServiceRegistry& registry, |
src/xrpld/rpc/handlers/Simulate.cpp
Outdated
| if (!isCurrentLedger) | ||
| return sle->getFieldU32(sfSequence); | ||
|
|
||
| return hasTicketSeq ? 0 : context.app.getTxQ().nextQueuableSeq(sle).value(); | ||
| return context.registry.getTxQ().nextQueuableSeq(sle).value(); |
There was a problem hiding this comment.
When simulating multiple transactions in the same request, the sequence autofilling doesn't automatically increment for subsequent transactions. Users must manually specify the sequence for all transactions after the first one. This should be documented or fixed to automatically increment sequences for chained transactions within the same simulation batch.
There was a problem hiding this comment.
Pull request overview
Copilot reviewed 8 out of 8 changed files in this pull request and generated 12 comments.
💡 Add Copilot custom instructions for smarter, more guided reviews. Learn how to get started.
|
|
||
| if (result.applied) | ||
| ++applied; | ||
| if (result.applied || flags & tapDRY_RUN) |
There was a problem hiding this comment.
For better readability, consider adding explicit parentheses around the bitwise AND operation: (result.applied || (flags & tapDRY_RUN)). While the operator precedence is correct, the explicit parentheses make the intent clearer and prevent potential confusion.
| if (result.applied || flags & tapDRY_RUN) | |
| if (result.applied || (flags & tapDRY_RUN)) |
src/xrpld/rpc/handlers/Simulate.cpp
Outdated
| if (!tx_json.isMember(jss::NetworkID)) | ||
| { | ||
| auto const networkId = context.app.getNetworkIDService().getNetworkID(); | ||
| auto const networkId = context.registry.getNetworkIDService().getNetworkID(); |
There was a problem hiding this comment.
The code uses context.registry but should use context.app. The RPC::JsonContext struct only has a member named app, not registry.
src/xrpld/rpc/handlers/Simulate.cpp
Outdated
| * cannot use `simulate` to bypass signature checks and submit | ||
| * transactions/modify the current ledger directly. | ||
| ***************************************/ | ||
| auto const result = apply(context.registry, perTxView, txn, tapDRY_RUN, context.j); |
There was a problem hiding this comment.
The code uses context.registry but should use context.app. The RPC::JsonContext struct only has a member named app, not registry.
src/xrpld/rpc/handlers/Simulate.cpp
Outdated
| // check validity of `binary` param | ||
| if (context.params.isMember(jss::binary) && !context.params[jss::binary].isBool()) | ||
| if (auto const batchResults = applyBatchTransactions( | ||
| context.registry, wholeBatchView, txn, tapDRY_RUN, context.j); |
There was a problem hiding this comment.
The code uses context.registry but should use context.app. The RPC::JsonContext struct only has a member named app, not registry.
| to.rawTxInsert(tx.getTransactionID(), sTx, sMeta); | ||
| apply(to); |
There was a problem hiding this comment.
Removing the isDryRun check means transactions are now inserted into the view even during dry runs. While this should be safe because simulate uses temporary OpenView copies, this changes the behavior where previously dry runs would not insert transactions into views at all. Consider verifying this behavior change is intentional and document it if necessary, as it may have implications for how views track transaction insertions during simulation.
| to.rawTxInsert(tx.getTransactionID(), sTx, sMeta); | |
| apply(to); | |
| if (!isDryRun) | |
| { | |
| to.rawTxInsert(tx.getTransactionID(), sTx, sMeta); | |
| apply(to); | |
| } |
src/xrpld/rpc/handlers/Simulate.cpp
Outdated
| context.registry.config(), | ||
| context.registry.getFeeTrack(), | ||
| context.registry.getTxQ(), | ||
| context.registry, |
There was a problem hiding this comment.
The code uses context.registry but should use context.app. The RPC::JsonContext struct only has a member named app, not registry.
src/xrpld/rpc/handlers/Simulate.cpp
Outdated
| return RPC::invalid_field_error(jss::ctid); | ||
| } | ||
| auto const [ledgerSq, txId, _] = *decodedCTID; | ||
| if (auto const optHash = context.registry.getLedgerMaster().txnIdFromIndex(ledgerSq, txId); |
There was a problem hiding this comment.
The code uses context.registry but should use context.app. The RPC::JsonContext struct only has a member named app, not registry.
src/xrpld/rpc/handlers/Simulate.cpp
Outdated
| } | ||
| using TxPair = std::pair<std::shared_ptr<Transaction>, std::shared_ptr<TxMeta>>; | ||
| auto ec{rpcSUCCESS}; | ||
| std::variant<TxPair, TxSearched> v = context.registry.getMasterTransaction().fetch(hash, ec); |
There was a problem hiding this comment.
The code uses context.registry but should use context.app. The RPC::JsonContext struct only has a member named app, not registry.
src/xrpld/rpc/handlers/Simulate.cpp
Outdated
| if (!sle) | ||
| { | ||
| JLOG(context.app.journal("Simulate").debug()) | ||
| JLOG(context.registry.journal("Simulate").debug()) |
There was a problem hiding this comment.
The code uses context.registry but the RPC::JsonContext struct only has a member named app, not registry. This will cause a compilation error. All occurrences of context.registry throughout this file should be changed to context.app. The Application class inherits from ServiceRegistry, so context.app can be used wherever a ServiceRegistry reference is needed.
| context.loadType = Resource::feeMediumBurdenRPC; | ||
|
|
There was a problem hiding this comment.
The resource load type is set to feeMediumBurdenRPC regardless of the number of transactions being simulated. Simulating 1000 transactions (the maximum allowed) will consume significantly more resources than simulating a single transaction, but both are charged the same. Consider adjusting the load type based on the number of transactions, or using a higher burden level like feeHighBurdenRPC for the simulate RPC to account for its potentially expensive operations.
| context.loadType = Resource::feeMediumBurdenRPC; | |
| // Set load type based on how many transactions are being simulated. | |
| std::size_t numSimulatedTxs = 0; | |
| if (context.params.isMember(jss::transactions) && | |
| context.params[jss::transactions].isArray()) | |
| { | |
| numSimulatedTxs = context.params[jss::transactions].size(); | |
| } | |
| else | |
| { | |
| // At most one of tx_json, tx_blob, tx_hash, or ctid is expected. | |
| // If none are present, numSimulatedTxs remains 0 and the validation | |
| // below will return an error. | |
| numSimulatedTxs = 1; | |
| } | |
| context.loadType = numSimulatedTxs > 1 | |
| ? Resource::feeHighBurdenRPC | |
| : Resource::feeMediumBurdenRPC; |
There was a problem hiding this comment.
Pull request overview
Copilot reviewed 8 out of 8 changed files in this pull request and generated 5 comments.
💡 Add Copilot custom instructions for smarter, more guided reviews. Learn how to get started.
| if (isCurrentLedger) | ||
| { | ||
| return RPC::make_param_error( | ||
| "Cannot use `tx_hash` without `ledger_index` or " | ||
| "`ledger_hash`."); | ||
| } |
There was a problem hiding this comment.
The error message returned when isCurrentLedger is true says "Cannot use tx_hash without ledger_index or ledger_hash." If the caller explicitly provides ledger_index: "current", this message is misleading (they did provide ledger_index). Consider rewording to indicate that tx_hash/ctid are only supported when simulating against a non-current (closed/validated/historical) ledger.
| JLOG(context.app.journal("Simulate").debug()) | ||
| << "Failed to find source account " | ||
| << "in current ledger: " << toBase58(*srcAddressID); | ||
|
|
||
| return Unexpected(rpcError(rpcSRC_ACT_NOT_FOUND)); |
There was a problem hiding this comment.
This debug log message hard-codes "in current ledger", but getAutofillSequence can now be called with a past ledger view (lpLedger). Consider adjusting the text (or including the ledger seq) so logs remain accurate when simulating historical ledgers.
| @@ -178,17 +281,25 @@ getTxJsonFromParams(Json::Value const& params) | |||
| return RPC::invalid_field_error(jss::tx_blob); | |||
| } | |||
| } | |||
| else if (params.isMember(jss::tx_json)) | |||
| else if (txInput.isMember(jss::tx_json)) | |||
| { | |||
| tx_json = params[jss::tx_json]; | |||
| tx_json = txInput[jss::tx_json]; | |||
| if (!tx_json.isObject()) | |||
| { | |||
| return RPC::object_field_error(jss::tx_json); | |||
| } | |||
| } | |||
| else | |||
| { | |||
| return RPC::make_param_error("Neither `tx_blob` nor `tx_json` included."); | |||
| auto const result = getTxJsonFromHistory(context, isCurrentLedger); | |||
| if (result.isMember(jss::error)) | |||
| { | |||
| return result; | |||
| } | |||
| else | |||
| { | |||
| tx_json = result; | |||
| } | |||
| } | |||
There was a problem hiding this comment.
getTxJsonFromParams only checks tx_blob / tx_json. For per-item objects in the transactions array that specify tx_hash or ctid, it falls into the history path, but getTxJsonFromHistory reads from context.params (top-level) instead of the per-item object. As a result, batch items cannot actually be sourced by tx_hash/ctid despite being accepted by validation. Consider passing the per-transaction object into getTxJsonFromHistory (or parsing tx_hash/ctid directly in getTxJsonFromParams) so batch items work as documented.
| { | ||
| context.loadType = Resource::feeMediumBurdenRPC; | ||
| Json::Value jvTransactions = Json::arrayValue; | ||
| std::vector<ApplyResult> results; |
There was a problem hiding this comment.
simulateTxn declares std::vector<ApplyResult> results; but never uses it. With -Wextra (and often -Werror) this can fail the build due to an unused-variable warning. Remove it or use it.
| std::vector<ApplyResult> results; |
| * cannot use `simulate` to bypass signature checks and submit | ||
| * transactions/modify the current ledger directly. | ||
| ***************************************/ | ||
| auto const result = apply(context.app, perTxView, txn, tapDRY_RUN, context.j); |
There was a problem hiding this comment.
For simulations against the current open ledger, this switched from TxQ::apply to xrpl::apply(...). That bypasses TxQ logic (fee escalation / queuing constraints) and can change engine_result compared to what would happen on actual submission. Consider using context.app.getTxQ().apply(...) when isCurrentLedger is true, and only falling back to xrpl::apply(...) for non-current ledgers.
| auto const result = apply(context.app, perTxView, txn, tapDRY_RUN, context.j); | |
| ApplyResult result; | |
| if (isCurrentLedger) | |
| result = context.app.getTxQ().apply(perTxView, txn, tapDRY_RUN, context.j); | |
| else | |
| result = apply(context.app, perTxView, txn, tapDRY_RUN, context.j); |
There was a problem hiding this comment.
Pull request overview
Copilot reviewed 8 out of 8 changed files in this pull request and generated 6 comments.
💡 Add Copilot custom instructions for smarter, more guided reviews. Learn how to get started.
| getTxJsonFromHistory(RPC::JsonContext& context, bool const isCurrentLedger) | ||
| { | ||
| Json::Value tx_json; | ||
| auto const params = context.params; | ||
| uint256 hash; | ||
| if (params.isMember(jss::tx_hash)) | ||
| { | ||
| auto const tx_hash = params[jss::tx_hash]; | ||
| if (!tx_hash.isString()) | ||
| { | ||
| return RPC::invalid_field_error(jss::tx_hash); | ||
| } | ||
| if (isCurrentLedger) | ||
| { | ||
| return RPC::make_param_error( | ||
| "Cannot use `tx_hash` without `ledger_index` or " | ||
| "`ledger_hash`."); | ||
| } | ||
| if (!hash.parseHex(context.params[jss::tx_hash].asString())) | ||
| return RPC::invalid_field_error(jss::tx_hash); | ||
| } | ||
| else if (params.isMember(jss::ctid)) | ||
| { | ||
| auto const ctid = params[jss::ctid]; | ||
| if (!ctid.isString()) | ||
| { | ||
| return RPC::invalid_field_error(jss::ctid); | ||
| } | ||
| if (isCurrentLedger) | ||
| { | ||
| return RPC::make_param_error( | ||
| "Cannot use `ctid` without `ledger_index` or `ledger_hash`."); | ||
| } | ||
| auto decodedCTID = RPC::decodeCTID(context.params[jss::ctid].asString()); | ||
| if (!decodedCTID) | ||
| { | ||
| return RPC::invalid_field_error(jss::ctid); | ||
| } | ||
| auto const [ledgerSq, txId, _] = *decodedCTID; | ||
| if (auto const optHash = context.app.getLedgerMaster().txnIdFromIndex(ledgerSq, txId); | ||
| optHash) | ||
| { | ||
| hash = *optHash; | ||
| } | ||
| else | ||
| { | ||
| return RPC::make_error(rpcTXN_NOT_FOUND); | ||
| } | ||
| } |
There was a problem hiding this comment.
getTxJsonFromParams(context, txInput, ...) falls back to getTxJsonFromHistory(context, ...), but getTxJsonFromHistory only reads context.params (top-level request), not the per-item txInput. As a result, batch items that specify tx_hash/ctid inside the transactions array cannot be resolved (and may instead try to fetch using a default/zero hash and return rpcTXN_NOT_FOUND). Refactor getTxJsonFromHistory to accept the per-transaction JSON object (or accept a parsed hash/ctid) and have getTxJsonFromParams pass txInput into it so batch lookups work correctly per element.
| static Json::Value | ||
| simulateTxn( | ||
| RPC::JsonContext& context, | ||
| std::vector<std::shared_ptr<Transaction>> transactions, |
There was a problem hiding this comment.
transactions is passed by value, causing an avoidable copy of the vector (and its shared_ptr elements) when calling simulateTxn(context, transactions, ...). Pass transactions as std::vector<std::shared_ptr<Transaction>> const& (or std::span<std::shared_ptr<Transaction> const>) to avoid copying at request scale (up to MAX_SIMULATE_TXS).
| std::vector<std::shared_ptr<Transaction>> transactions, | |
| std::vector<std::shared_ptr<Transaction>> const& transactions, |
|
|
||
| for (auto const field : {jss::secret, jss::seed, jss::seed_hex, jss::passphrase}) | ||
| bool | ||
| checkIsCurrentLedger(Json::Value const params) |
There was a problem hiding this comment.
checkIsCurrentLedger takes params by value, copying the full request object. Prefer Json::Value const& params to avoid an unnecessary copy for every simulate call.
| checkIsCurrentLedger(Json::Value const params) | |
| checkIsCurrentLedger(Json::Value const& params) |
| "Must include one of 'transactions', 'tx_json', 'tx_blob', " | ||
| "'tx_hash', or 'ctid'."); | ||
| } | ||
| // if more than one of these fields is included, error out | ||
| if (numParams > 1) | ||
| { | ||
| return RPC::make_param_error( | ||
| "Cannot include more than one of 'transactions', 'tx_json', " | ||
| "'tx_blob', " | ||
| "'tx_hash', and 'ctid'."); |
There was a problem hiding this comment.
New parameter error messages use single quotes around field names, while other new messages in this handler use backticks (e.g., “Cannot use tx_hash without ...”). Consider standardizing quoting style within simulate for consistency (either backticks or quotes) so client-side matching and documentation remain uniform.
| "Must include one of 'transactions', 'tx_json', 'tx_blob', " | |
| "'tx_hash', or 'ctid'."); | |
| } | |
| // if more than one of these fields is included, error out | |
| if (numParams > 1) | |
| { | |
| return RPC::make_param_error( | |
| "Cannot include more than one of 'transactions', 'tx_json', " | |
| "'tx_blob', " | |
| "'tx_hash', and 'ctid'."); | |
| "Must include one of `transactions`, `tx_json`, `tx_blob`, " | |
| "`tx_hash`, or `ctid`."); | |
| } | |
| // if more than one of these fields is included, error out | |
| if (numParams > 1) | |
| { | |
| return RPC::make_param_error( | |
| "Cannot include more than one of `transactions`, `tx_json`, " | |
| "`tx_blob`, " | |
| "`tx_hash`, and `ctid`."); |
| "Must include one of 'transactions', 'tx_json', 'tx_blob', " | ||
| "'tx_hash', or 'ctid'."); | ||
| } | ||
| // if more than one of these fields is included, error out | ||
| if (numParams > 1) | ||
| { | ||
| return RPC::make_param_error( | ||
| "Cannot include more than one of 'transactions', 'tx_json', " | ||
| "'tx_blob', " | ||
| "'tx_hash', and 'ctid'."); |
There was a problem hiding this comment.
New parameter error messages use single quotes around field names, while other new messages in this handler use backticks (e.g., “Cannot use tx_hash without ...”). Consider standardizing quoting style within simulate for consistency (either backticks or quotes) so client-side matching and documentation remain uniform.
| "Must include one of 'transactions', 'tx_json', 'tx_blob', " | |
| "'tx_hash', or 'ctid'."); | |
| } | |
| // if more than one of these fields is included, error out | |
| if (numParams > 1) | |
| { | |
| return RPC::make_param_error( | |
| "Cannot include more than one of 'transactions', 'tx_json', " | |
| "'tx_blob', " | |
| "'tx_hash', and 'ctid'."); | |
| "Must include one of `transactions`, `tx_json`, `tx_blob`, " | |
| "`tx_hash`, or `ctid`."); | |
| } | |
| // if more than one of these fields is included, error out | |
| if (numParams > 1) | |
| { | |
| return RPC::make_param_error( | |
| "Cannot include more than one of `transactions`, `tx_json`, " | |
| "`tx_blob`, " | |
| "`tx_hash`, and `ctid`."); |
| auto const numParams = txInput.isMember(jss::tx_json) + | ||
| txInput.isMember(jss::tx_blob) + txInput.isMember(jss::tx_hash) + | ||
| txInput.isMember(jss::ctid); | ||
| if (numParams == 0) | ||
| { | ||
| return RPC::make_param_error( | ||
| "Must include one of 'tx_json', 'tx_blob', " | ||
| "'tx_hash', or 'ctid' in each transaction."); | ||
| } | ||
| else if (numParams > 1) | ||
| { | ||
| return RPC::make_param_error( | ||
| "Cannot include more than one of 'tx_json', 'tx_blob', " | ||
| "'tx_hash', and 'ctid' in each transaction."); | ||
| } |
There was a problem hiding this comment.
The batching feature explicitly allows per-item tx_hash/ctid, but the updated tests only cover batched tx_json inputs (and single-item tx_hash/ctid). Add unit tests that submit a transactions array where items use tx_hash and/or ctid, and validate the handler fetches and simulates each correctly against the specified historical ledger.
High Level Overview of Change
Currently, the
simulateRPC only supports simulating transactions in the current ledger. This PR adds support to also simulate transactions in past ledgers, as long as the rippled node has that data.This will make it much easier to debug past transactions, without the difficulty/complexity of needing to perform a full ledger replay (although it doesn't quite have the same flexibility). It'll provide about 90-95% of the functionality of ledger replay (if not 100%) for like 1% of the effort.
Currently, you need to follow a long set of instructions with complex scripts to run ledger replay. With this feature, the process for "replaying" a transaction would be as follows:
gdb/lldb/etc in whatever transactor you're investigatingn-1andnledger that the transaction you're investigating is in (steps 1 and 2 are somewhat interchangeable)simulateRPC to simulate the past transaction in the past ledger as described below:If you need to include multiple transactions (i.e. there were other relevant transactions in the same ledger):
Depending on performance testing results, we may want this specific feature to be admin-only.
New features:
Context of Change
Closes #5540 (probably need to still add amendment support for that)
Type of Change
API Impact
libxrplchange (any change that may affectlibxrplor dependents oflibxrpl)Before / After
Mainly changes to the
simulateRPC. Some error codes change on other RPCs, but they're pretty minor.Test Plan
Added some basic tests
Future Tasks