finish fn result refactor

src/libxrpl/tx/ApplyContext.cpp

@@ -58,14 +58,19 @@ ApplyContext::discard()
 std::optional<TxMeta>
 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);
 }
 

src/libxrpl/tx/transactors/escrow/EscrowFinish.cpp

@@ -35,7 +35,6 @@
 #include <xrpl/tx/wasm/WasmVM.h>
 
 #include <cstdint>
-#include <limits>
 #include <memory>
 #include <optional>
 #include <string>
@@ -417,42 +416,49 @@ 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";
 
-        if (auto const& data = ledgerDataProvider.getData(); data.has_value())
+        // 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<std::int64_t> const cost = re.has_value() ? re->cost : re.error().cost;
+        if (cost.has_value())
         {
-            if (data->size() > kMaxWasmDataLength)
-            {
-                // should already be checked in the updateData host function
+            // 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
-            }
-            slep->setFieldVL(sfData, makeSlice(*data));
-            ctx_.view().update(slep);
+            ctx_.setGasUsed(static_cast<std::uint32_t>(*cost));
         }
 
-        if (re.has_value())
+        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;
+            // 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;
+        }
 
-            ctx_.setVMReturnCode(reValue);
+        auto const reValue = re->result;
+        JLOG(j_.debug()) << "WASM Success: " + std::to_string(reValue) << ", cost: " << re->cost;
 
-            if (reCost < 0 || reCost > std::numeric_limits<uint32_t>::max())
-                return tecINTERNAL;  // LCOV_EXCL_LINE
-            ctx_.setGasUsed(static_cast<uint32_t>(reCost));
+        ctx_.setVMReturnCode(reValue);
 
-            if (reValue <= 0)
+        // 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)
             {
-                return tecBYTECODE_REJECTED;
+                // should already be checked in the updateData host function
+                return tecINTERNAL;  // LCOV_EXCL_LINE
             }
+            slep->setFieldVL(sfData, makeSlice(*data));
+            ctx_.view().update(slep);
         }
-        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];

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