From ef438aec26bf08e8126556dbe0935c20cd1b9986 Mon Sep 17 00:00:00 2001 From: pwang200 <354723+pwang200@users.noreply.github.com> Date: Sat, 8 Aug 2026 23:13:33 -0400 Subject: [PATCH] finish fn result refactor (#7947) --- src/libxrpl/tx/ApplyContext.cpp | 13 +++-- .../tx/transactors/escrow/EscrowFinish.cpp | 54 ++++++++++--------- src/test/app/EscrowSmart_test.cpp | 38 +++++++++++++ 3 files changed, 77 insertions(+), 28 deletions(-) diff --git a/src/libxrpl/tx/ApplyContext.cpp b/src/libxrpl/tx/ApplyContext.cpp index 05c6425634..4c839acef2 100644 --- a/src/libxrpl/tx/ApplyContext.cpp +++ b/src/libxrpl/tx/ApplyContext.cpp @@ -58,14 +58,19 @@ ApplyContext::discard() std::optional ApplyContext::apply(TER ter) { - if (vmReturnCode_.has_value()) + // tecINTERNAL reports an xrpld bug, not a result: nothing the VM recorded + // before we hit it belongs in the metadata. + if (ter != tecINTERNAL) { + if (vmReturnCode_.has_value()) + { + // NOLINTNEXTLINE(bugprone-unchecked-optional-access) view_ emplaced in constructor + view_->setVMReturnCode(*vmReturnCode_); + } // NOLINTNEXTLINE(bugprone-unchecked-optional-access) view_ emplaced in constructor - view_->setVMReturnCode(*vmReturnCode_); + view_->setGasUsed(gasUsed_); } // NOLINTNEXTLINE(bugprone-unchecked-optional-access) view_ emplaced in constructor - view_->setGasUsed(gasUsed_); - // NOLINTNEXTLINE(bugprone-unchecked-optional-access) view_ emplaced in constructor return view_->apply(base_, tx, ter, parentBatchId_, (flags_ & TapDryRun) != 0u, journal); } diff --git a/src/libxrpl/tx/transactors/escrow/EscrowFinish.cpp b/src/libxrpl/tx/transactors/escrow/EscrowFinish.cpp index a44b77c006..1d09d55b89 100644 --- a/src/libxrpl/tx/transactors/escrow/EscrowFinish.cpp +++ b/src/libxrpl/tx/transactors/escrow/EscrowFinish.cpp @@ -35,7 +35,6 @@ #include #include -#include #include #include #include @@ -417,9 +416,35 @@ EscrowFinish::doApply() return tecINTERNAL; } std::uint32_t const allowance = ctx_.tx[sfGas]; - auto re = runEscrowWasm(wasm, ledgerDataProvider, allowance, escrowFunctionName); + auto const re = runEscrowWasm(wasm, ledgerDataProvider, allowance, escrowFunctionName); JLOG(j_.trace()) << "Escrow WASM ran"; + // Gas consumed, reported in the tx metadata whenever the engine has a + // trustworthy number: a completed run, out of gas, or a wasm fault. + std::optional const cost = re.has_value() ? re->cost : re.error().cost; + if (cost.has_value()) + { + // The engine cannot spend more than it was given, and pins the cost + // to the allowance when it runs out. + if (*cost < 0 || *cost > allowance) + return tecINTERNAL; // LCOV_EXCL_LINE + ctx_.setGasUsed(static_cast(*cost)); + } + + if (!re.has_value()) + { + // No return code, and any data it wrote goes away with the view. + JLOG(j_.debug()) << "WASM Failure: " + transHuman(re.error().ter); + return re.error().ter; + } + + auto const reValue = re->result; + JLOG(j_.debug()) << "WASM Success: " + std::to_string(reValue) << ", cost: " << re->cost; + + ctx_.setVMReturnCode(reValue); + + // Only matters on a reject, where the escrow survives: + // Transactor::processPersistentChanges replays this after the reset. if (auto const& data = ledgerDataProvider.getData(); data.has_value()) { if (data->size() > kMaxWasmDataLength) @@ -431,28 +456,9 @@ EscrowFinish::doApply() ctx_.view().update(slep); } - if (re.has_value()) - { - auto const reValue = re.value().result; - auto const reCost = re.value().cost; - JLOG(j_.debug()) << "WASM Success: " + std::to_string(reValue) << ", cost: " << reCost; - - ctx_.setVMReturnCode(reValue); - - if (reCost < 0 || reCost > std::numeric_limits::max()) - return tecINTERNAL; // LCOV_EXCL_LINE - ctx_.setGasUsed(static_cast(reCost)); - - if (reValue <= 0) - { - return tecBYTECODE_REJECTED; - } - } - else - { - JLOG(j_.debug()) << "WASM Failure: " + transHuman(re.error().ter); - return re.error().ter; - } + // 0 or negative is a contract-defined reject code, reported as sfVMReturnCode. + if (reValue <= 0) + return tecBYTECODE_REJECTED; } AccountID const account = (*slep)[sfAccount]; diff --git a/src/test/app/EscrowSmart_test.cpp b/src/test/app/EscrowSmart_test.cpp index e85cbb30b3..f8c2c3f2cf 100644 --- a/src/test/app/EscrowSmart_test.cpp +++ b/src/test/app/EscrowSmart_test.cpp @@ -492,6 +492,18 @@ struct EscrowSmart_test : public beast::unit_test::Suite Fee(finishFee), escrow::Gas(2), Ter(tecOUT_OF_GAS)); + + // Running out of gas still reports the gas consumed, which is the + // whole allowance. The function did not run to completion, so + // there is no return code to report. + auto const txMeta = env.meta(); + if (BEAST_EXPECT(txMeta && txMeta->isFieldPresent(sfGasUsed))) + { + BEAST_EXPECTS( + txMeta->getFieldU32(sfGasUsed) == 2, + std::to_string(txMeta->getFieldU32(sfGasUsed))); + } + BEAST_EXPECT(txMeta && !txMeta->isFieldPresent(sfVMReturnCode)); } { @@ -510,6 +522,32 @@ struct EscrowSmart_test : public beast::unit_test::Suite escrow::Gas(allowance), Ter(tefNO_BYTECODE)); } + + { + // a trap in the wasm code reports the gas it burned, which is only + // part of the allowance + auto const trapSeq = env.seq(alice); + env(escrow::create(alice, carol, XRP(500)), + escrow::Bytecode(kTrapUnreachableHex), + escrow::kCancelTime(env.now() + 100s), + Fee(env.current()->fees().base * 10 + kTrapUnreachableHex.size() / 2 * 5)); + env.close(); + + std::uint32_t const allowance = 1000; + env(escrow::finish(carol, alice, trapSeq), + Fee(env.current()->fees().base + + (allowance * env.current()->fees().gasPrice) / microDropsPerDrop + 1), + escrow::Gas(allowance), + Ter(tecFAILED_PROCESSING)); + + auto const txMeta = env.meta(); + if (BEAST_EXPECT(txMeta && txMeta->isFieldPresent(sfGasUsed))) + { + auto const gasUsed = txMeta->getFieldU32(sfGasUsed); + BEAST_EXPECTS(gasUsed < allowance, std::to_string(gasUsed)); + } + BEAST_EXPECT(txMeta && !txMeta->isFieldPresent(sfVMReturnCode)); + } } void