Stop a contract's own mistake reading as a node fault when it emits

Both emit host functions rebuilt an `STTx`, and both were caught by the
outer guard when the rebuild threw, so a contract that got its own
transaction wrong was answered `InternalFatal`: the run stopped and the
transaction reported `tecINTERNAL`. Either was reachable from any guest.

`emit_built_txn` constructed the transaction before its try, so a field
the type requires but the contract never set threw there. The
construction moves inside the guard and answers `SubmitTxnFailure`, which
the contract can act on. It also copies rather than moves out of the
builder, so a contract told its transaction is malformed can supply what
was missing and emit the same index again.

`emit_txn` could not emit anything a contract had not already marked
`tfInnerBatchTxn`. Setting the flag meant rebuilding the transaction from
its fields, and that re-runs the format check, which rejects the defaulted
`sfPaths` an ordinary Payment carries harmlessly. The flag now goes on a
copy made through `STTx`'s own copy constructor, which does not re-check,
and through `setFieldU32` rather than `setFlag`: a transaction built
against a template holds `sfFlags` as a placeholder until something gives
it a value, and `setFlag` can only reach one that already has it.

The tests that pinned the old behavior now pin the new.

src/libxrpl/tx/wasm/ContractHostFuncImpl.cpp

@@ -950,17 +950,26 @@ ContractHostFunctionsImpl::emitBuiltTxn(std::uint32_t const& index)
         // Ensure tfInnerBatchTxn is always set, even if the contract
         // overwrote sfFlags via addTxnField.
         contractCtx.built_txns[index].setFlag(tfInnerBatchTxn);
-        std::shared_ptr<STTx const> const stx =
-            std::make_shared<STTx>(std::move(contractCtx.built_txns[index]));
 
+        // `STTx`'s constructor checks the transaction against its format, and reports a
+        // field the type requires but the contract never set by throwing. That is the
+        // contract's own mistake, so it is caught here and answered rather than left to
+        // the catch below, which would report a node fault and stop the run.
+        //
+        // The built transaction is copied rather than moved: a run that is told its
+        // transaction is malformed can correct it and emit the same index again. `STTx`
+        // takes the object by rvalue, so the copy is made here.
+        std::shared_ptr<STTx const> stx;
         try
         {
+            STObject built = contractCtx.built_txns[index];
+            stx = std::make_shared<STTx const>(std::move(built));
             (void)stx->getTransactionID();
         }
         catch (std::exception const& e)
         {
             JLOG(j.trace()) << "WasmTrace[" << parentBatchId << "]: "
-                            << "emitBuiltTxn: Failed to decode transaction: " << e.what();
+                            << "emitBuiltTxn: Failed to build transaction: " << e.what();
             return std::unexpected(HostFunctionError::SubmitTxnFailure);
         }
 
@@ -1003,14 +1012,23 @@ ContractHostFunctionsImpl::emitTxn(std::shared_ptr<STTx const> const& stxPtr)
 
     try
     {
-        // Ensure tfInnerBatchTxn is always set on emitted transactions.
-        // Since STTx is const, create a mutable copy if the flag is missing.
+        // Ensure tfInnerBatchTxn is always set on emitted transactions. `STTx` is held
+        // const, so the flag goes on a copy.
+        //
+        // The copy is made through `STTx`'s own copy constructor rather than by rebuilding
+        // one from its fields. Rebuilding re-runs the format check, which rejects the
+        // defaulted `sfPaths` an ordinary Payment carries harmlessly — so a contract that
+        // simply did not set the flag itself could not emit anything at all.
         std::shared_ptr<STTx const> txPtr = stxPtr;
         if (!stxPtr->isFlag(tfInnerBatchTxn))
         {
-            STObject obj(static_cast<STObject const&>(*stxPtr));
-            obj.setFlag(tfInnerBatchTxn);
-            txPtr = std::make_shared<STTx const>(std::move(obj));
+            auto flagged = std::make_shared<STTx>(*stxPtr);
+
+            // `setFlag` reaches an existing `sfFlags` and cannot add one: a transaction
+            // built against a template holds the field as a placeholder until something
+            // gives it a value, and that is what `setFieldU32` does.
+            flagged->setFieldU32(sfFlags, flagged->getFlags() | tfInnerBatchTxn);
+            txPtr = std::move(flagged);
         }
 
         try

src/tests/libxrpl/tx/wasm/host_functions/EmitBuiltTxn.cpp

@@ -84,24 +84,37 @@ TEST_F(EmitBuiltTxnImpl, APaymentTheAccountCannotAffordIsReportedAsItsTec)
         << "a fee-claiming transaction belongs on the ledger";
 }
 
-// A transaction missing a field its type requires never becomes a transaction at all.
-//
-// What the contract is told is `InternalFatal`, which stops the run and reports
-// `tecINTERNAL`: the transaction's format is checked where `STTx` is constructed, outside
-// the guard that would have answered `SubmitTxnFailure`. A contract's own mistake reading as
-// a node fault is worth revisiting; this pins what it does today.
-TEST_F(EmitBuiltTxnImpl, ATransactionMissingARequiredFieldStopsTheRun)
+// A transaction missing a field its type requires never becomes a transaction, and the
+// contract is told so: `SubmitTxnFailure` is a refusal it can act on, where the run-stopping
+// `InternalFatal` would have reported a node fault for a mistake of its own.
+TEST_F(EmitBuiltTxnImpl, ATransactionMissingARequiredFieldIsRefused)
 {
     auto const contractHost = host();
     auto const amount = WasmLedger::toBytes(STAmount{XRP(1)});
     ASSERT_TRUE(contractHost->buildTxn(ttPAYMENT));
     ASSERT_TRUE(contractHost->addTxnField(0, sfAmount, Slice{amount.data(), amount.size()}));
 
-    // No destination, so the payment cannot even be serialized as one.
-    expectError(contractHost->emitBuiltTxn(0), HostFunctionError::InternalFatal);
+    // No destination, so the payment cannot be built as one.
+    expectError(contractHost->emitBuiltTxn(0), HostFunctionError::SubmitTxnFailure);
     EXPECT_TRUE(contractHost.context().result.emittedTxns.empty());
 }
 
+// A refusal leaves the transaction where it was, so the contract can supply what was missing
+// and emit the same index again.
+TEST_F(EmitBuiltTxnImpl, ARefusedTransactionCanBeCorrectedAndEmitted)
+{
+    auto const contractHost = host();
+    auto const amount = WasmLedger::toBytes(STAmount{XRP(192)});
+    auto const destination = accountField(carol.id());
+    ASSERT_TRUE(contractHost->buildTxn(ttPAYMENT));
+    ASSERT_TRUE(contractHost->addTxnField(0, sfAmount, Slice{amount.data(), amount.size()}));
+    ASSERT_FALSE(contractHost->emitBuiltTxn(0));
+
+    ASSERT_TRUE(
+        contractHost->addTxnField(0, sfDestination, Slice{destination.data(), destination.size()}));
+    expectValue(contractHost->emitBuiltTxn(0), TERtoInt(tesSUCCESS));
+}
+
 TEST_F(EmitBuiltTxnImpl, AnIndexNamingNoTransactionIsOutOfBounds)
 {
     expectError(host()->emitBuiltTxn(0), HostFunctionError::IndexOutOfBounds);

src/tests/libxrpl/tx/wasm/host_functions/EmitTxn.cpp

@@ -63,23 +63,17 @@ TEST_F(EmitTxnImpl, APaymentTheLedgerAcceptsIsQueued)
     EXPECT_EQ(contractHost.context().result.emittedTxns.size(), 1U);
 }
 
-// A transaction the contract did not mark as an inner one cannot be emitted at all.
-//
-// The host means to set the flag for it, and rebuilds the transaction to do so — but
-// rebuilding an `STTx` from its own fields re-runs the format check, which rejects a
-// defaulted `sfPaths` that the original carried harmlessly. The contract is told
-// `InternalFatal`, so the run stops and the transaction reports `tecINTERNAL`.
-//
-// This pins what happens today. A contract's own choice reading as a node fault belongs
-// with the same fix as `EmitBuiltTxnImpl.ATransactionMissingARequiredFieldStopsTheRun`.
-TEST_F(EmitTxnImpl, ATransactionWithoutTheInnerFlagStopsTheRun)
+// A contract does not have to mark what it emits as an inner transaction: the host marks it,
+// on a copy, and the transaction is applied like any other.
+TEST_F(EmitTxnImpl, ATransactionWithoutTheInnerFlagIsMarkedAndEmitted)
 {
     auto const contractHost = host();
 
-    expectError(
-        contractHost->emitTxn(payment(XRP(192), contractSequence(), false)),
-        HostFunctionError::InternalFatal);
-    EXPECT_TRUE(contractHost.context().result.emittedTxns.empty());
+    expectValue(
+        contractHost->emitTxn(payment(XRP(192), contractSequence(), false)), TERtoInt(tesSUCCESS));
+
+    ASSERT_EQ(contractHost.context().result.emittedTxns.size(), 1U);
+    EXPECT_TRUE(contractHost.context().result.emittedTxns.front()->isFlag(tfInnerBatchTxn));
 }
 
 // One the contract did mark keeps the flag, which is what the transactor reads to know it is