From cd5366dab78363796df51b26788eb8addb607857 Mon Sep 17 00:00:00 2001 From: Mayukha Vadari Date: Tue, 15 Sep 2026 19:10:16 -0400 Subject: [PATCH] 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 | 34 ++++++++++++++----- .../tx/wasm/host_functions/EmitBuiltTxn.cpp | 31 ++++++++++++----- .../tx/wasm/host_functions/EmitTxn.cpp | 22 +++++------- 3 files changed, 56 insertions(+), 31 deletions(-) diff --git a/src/libxrpl/tx/wasm/ContractHostFuncImpl.cpp b/src/libxrpl/tx/wasm/ContractHostFuncImpl.cpp index aac7f91616..f528e0e435 100644 --- a/src/libxrpl/tx/wasm/ContractHostFuncImpl.cpp +++ b/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 const stx = - std::make_shared(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 stx; try { + STObject built = contractCtx.built_txns[index]; + stx = std::make_shared(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 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 txPtr = stxPtr; if (!stxPtr->isFlag(tfInnerBatchTxn)) { - STObject obj(static_cast(*stxPtr)); - obj.setFlag(tfInnerBatchTxn); - txPtr = std::make_shared(std::move(obj)); + auto flagged = std::make_shared(*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 diff --git a/src/tests/libxrpl/tx/wasm/host_functions/EmitBuiltTxn.cpp b/src/tests/libxrpl/tx/wasm/host_functions/EmitBuiltTxn.cpp index d4e27fd78e..491a4f6fa6 100644 --- a/src/tests/libxrpl/tx/wasm/host_functions/EmitBuiltTxn.cpp +++ b/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); diff --git a/src/tests/libxrpl/tx/wasm/host_functions/EmitTxn.cpp b/src/tests/libxrpl/tx/wasm/host_functions/EmitTxn.cpp index 8ad8e3ab5d..af6fe9151b 100644 --- a/src/tests/libxrpl/tx/wasm/host_functions/EmitTxn.cpp +++ b/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