fix(engine)!: refuse engine calls from the response allocation

[sdbondi]

Closes #2442, which has the mechanism and the impact.

Why option 3 and not option 2

#2442 leaned to option 2 — take response allocation away from the template entirely. I designed it
out, and it does not survive contact:

• The region cannot be sized. ComponentManager::get_state() returns the whole component
  state, and the generated dispatcher issues it on every method call, so a host-owned region is
  sized by the largest state any template holds, on the hottest path. A template's entire linear
  memory is 2 MiB (max_memory_pages = 32), and max_substate_size is 1 MiB, so preserving today's
  semantics means handing half the memory budget to a buffer.
• The escape hatch does not work. A per-template #[template(io_region_kib = N)] was the answer
  to that, with a write-side check to stop a template writing state larger than the region that has
  to read it back — otherwise a component is written successfully and then permanently uncallable.
  But GetState can read another template's component, so the writer's region is not the
  reader's. The check has to be against a global ceiling, which puts a single fixed cap straight
  back and leaves the knob buying nothing.
• Template-supplied buffers need host-side state. The alternative — template passes the
  destination, host writes into it — requires the template to size the buffer before it knows the
  response length, and engine ops cannot be re-run to find out. The host has to hold the encoded
  response between two calls. Multipart is the same stash with more round trips.

So option 2 as scoped would have shipped a knob that does not knob, a new consensus check, and a
hard ABI break requiring every template to be republished — to remove a class of bugs that option 3
closes for three lines.

The performance argument does not carry it either. I measured the response allocation across the
engine suite: 6,341 of them, median 7 bytes, p99 692, max 1,964 — nothing near any plausible region
size. Each costs ~126 metered points, which is ~1% of a transaction's WASM points (0.4-1.2%
across sampled transactions), or ~2% counting the matching free. Real, but not what an ABI break is
for.

What option 3 does not do is retire the class: the engine still calls template-supplied tari_alloc
and tari_free, and the invariant lives in three hand-written brackets rather than at the boundary.
Worth its own issue if it is picked up later — the sizing data above says the right shape is a small
region (4 KiB covers everything observed) with a fallback, not a region sized for the worst case.

Implementation

handle writes each engine call's response through the template's tari_alloc, so servicing a call
runs template code while an invocation is in flight. That allocation moves into alloc_response,
which shuts the invocation window around it:

let was_open = env.data_mut().suspend_template_invocation();
let result = alloc_fn.call(&mut *env, len);
env.data_mut().restore_template_invocation(was_open);
take_refused_engine_call(env.data_mut())?;

The window is restored rather than reopened, so the bracket is correct wherever it is called from
rather than only behind the entrypoint guard.

Same mechanism #2440 used for the two entry points that already ran outside an invocation, so all
three places the engine drives template code are now closed to engine calls.

handle takes the FunctionEnvMut rather than a pre-split (&mut WasmEnv, StoreMut). It needs to
mutate the environment either side of a call that re-enters WASM, and the environment must not stay
borrowed across that call — the refusal is recorded through the engine's own &mut to it. The
dispatch sites in tari_engine_entrypoint pass &mut env, and the guard/meter split is scoped to a
block ahead of them.

WasmEnv::alloc is gone; alloc_response is the only caller left and it needs the borrow split.

Refusals are drained on both paths

A refusal now fails the call before the Ok/Err match rather than only on the trap path.
tari_engine_entrypoint can answer a refused call only with a null pointer, and a template is free
to ignore that and return normally — which is exactly what the new test template does. Checking only
the trap path let the refusal be dropped and the transaction commit.

This also picks up the case flagged in review on #2440: an engine error raised mid-invocation and
swallowed by a template no longer disappears.

Test

test_engine_call_in_response_alloc adds a third buggy variant whose tari_alloc calls the engine
once an invocation is under way — a static flag skips the CallInfo allocation, which #2440 already
refuses before the invocation begins, so the response allocation is the one that runs. It ignores the
null it gets back and returns normally.

Verified load-bearing: with the bracket removed the test aborts the process with
fatal runtime error: stack overflow (SIGABRT). With it, the transaction rejects with
EngineCallOutsideInvocation.

cargo test -p tari_engine passes in full (36 suites); cargo lints clippy clean.

Breaking

Consensus-affecting: a transaction whose template calls the engine from tari_alloc during an
invocation previously crashed the node and now rejects.

[sdbondi]

Checked out the branch, traced the control flow, and ran the tests. The fix is correct, and I verified the new test is load-bearing: with the exit/enter bracket removed from alloc_response, test_engine_call_in_response_alloc aborts with fatal runtime error: stack overflow (SIGABRT); with it, all three guard tests pass.

What I verified

• Coverage is complete. Four places the engine drives WASM: alloc_checked (CallInfo, before the window), free_checked (return ptr, after it), alloc_response (new), and func.call itself. Grepping .call( in process.rs finds no fifth, and with_alloc_and_mem_writer is only used for the CallInfo.
• enter_template_invocation() after the alloc restores rather than sets, because handle is only reachable past the entrypoint guard at crates/engine/src/wasm/process.rs:250, which returns null when the window is closed.
• Nesting cannot corrupt the bool. invoke_template (crates/engine/src/transaction/processor.rs:939) builds a fresh Store + WasmProcess per cross-template call, so each WasmEnv sees exactly one invocation.
• Draining last_engine_error on both paths is strictly additive. template_abi::call_engine panics on a null response (crates/template_abi/src/abi/mod.rs:67), so a swallowed engine error only arises for a template that ignores the ABI. No legitimate flow is newly rejected.
• Error precedence survives the hoist. The engine error is still checked ahead of the panic message and the out-of-gas classification. An out-of-gas trap inside alloc_response surfaces as WasmExecutionError::WasmRuntimeError, not RuntimeError, so it is not stored in last_engine_error and FeeIntentComputeExceeded/InsufficientFeesForCompute still win in the Err arm.
• Hardening not called out in the description: len as u32 became u32::try_from. The old WasmEnv::alloc call site truncated a usize from encoded_len, so a >=4 GiB response would have allocated a truncated buffer that encode_into_writer then wrote past. Unreachable in a wasm32 process, but good to have closed.

Worth resolving before merge

1. This implements option 3, but the issue leans to option 2. #2442 lists three directions, and the comment there is "leaning to option 2 because that is the more correct way (removes a class of bugs and slightly reduces WASM size to boot), we can tolerate ABI breaking changes at this stage." This PR does option 3 (guard the alloc) and closes the issue. Option 3's own stated downside is that it "does not bound other re-entrant shapes if any exist" — the invariant now lives in three near-duplicate hand-written brackets (alloc_checked, free_checked, alloc_response) rather than at the boundary, so a fourth host->WASM call site added later has to remember. Either keep #2442 open as the follow-up for option 2, or record in the PR body why option 3 won. As written, the decision is lost when the issue auto-closes.

Minor

2. handle is pub (crates/engine/src/wasm/process.rs:361) but the bracket assumes it is not reachable outside the window. Nothing outside process.rs calls it (grepped crates/ and applications/). Since the signature is changing anyway, making it private — or saving and restoring the flag instead of the unconditional enter_template_invocation() — turns "safe because of a guard three functions away" into "safe by construction".

3. take_refused_engine_call at crates/engine/src/wasm/process.rs:512 cannot fire. A refusal is only recorded while the window is closed, and every site that closes it (alloc_checked, free_checked, alloc_response) drains it before returning, including on the trap path since the drain precedes result?. In the new test the refusal reaches invoke as last_engine_error, not as a pending refusal. Harmless belt-and-braces, but the comment above it ("A refusal or engine error recorded during the invocation fails the call on both paths") reads as if the refusal arm is live.

4. rustfmt: three lines now exceed max_width = 120 in crates/engine/tests/templates/buggy/src/lib.rs (lines 27, 59 and 93, at 121-126 chars). CI will not catch it because that crate declares its own [workspace] and root cargo fmt --all does not reach it, but the file was inside the limit before.

5. Module name. engine_call_outside_invocation now hosts a variant that runs inside an invocation — the engine just closes the window around it. The doc comment explains this well; the name is the only thing left saying otherwise.

Nothing here blocks except deciding (1).

---

Generated by Claude Code

[sdbondi]

Reviewed bf680c1. All five points from the previous review are addressed, and the option-2 section in the description is substantive — I reproduced the sizing data it rests on. One new minor suggestion.

Verified on this commit

• suspend/restore genuinely generalizes the bracket. alloc_response no longer depends on the entrypoint guard at crates/engine/src/wasm/process.rs:245 for correctness, which is what I was after.
• handle is private and nothing outside process.rs referenced it; the crate builds.
• The check reorder in invoke is behaviourally identical today. A refusal recorded while the window is closed is always drained by the site that closed it, including on the trap path since the drain precedes result?, so refused_engine_call is None by the time invoke looks. The new test reaches invoke through last_engine_error, not through the refusal arm. The revised comment now says exactly that rather than implying the arm is live.
• Module rename is complete. grep -rn engine_call_outside_invocation over the tree is empty, and the header comment at the top of buggy/src/lib.rs tracks the new name.
• No line in buggy/src/lib.rs exceeds 120 any more. The two diffs cargo +nightly-2025-12-05 fmt --check still reports in that crate (the two _ABI_TEMPLATE_DEF array literals) are both pre-existing on development, so this PR now introduces none.
• cargo test -p tari_engine: 4 test binaries, 37 tests, 0 failures. cargo clippy -p tari_engine --all-targets: clean — the two too_many_arguments warnings come from crates/engine_types/src/resource.rs:68,99 and are pre-existing.

On the option-2 rationale

I checked the numbers that carry the argument:

• max_memory_pages: 32 (~2 MiB) and max_substate_size: 1024 * 1024 at crates/engine_types/src/limits.rs:19 and :128, so "half the memory budget to a buffer" is accurate.
• The generated dispatcher does issue component_manager.get_state::<..>() per method call (crates/template_macros/src/template/dispatcher.rs:144), so a host-owned region really would be sized by the largest component state on the hottest path.
• Reproduced the size distribution. Instrumented handle to record encoded_len(&resp) and ran the full engine suite: 6,341 samples, median 7 B, p90 43 B, p99 728 B, max 1,965 B. (I dropped 15 lines torn by concurrent test binaries appending to one file — the two apparent outliers above 2 KiB were both 39 repetitions.) That matches the median 7 / max 1,964 in the description, and confirms 4 KiB covers everything observed with room to spare.

The per-allocation point cost (~126) and the 0.4-1.2% of transaction WASM points I did not reproduce.

New (minor)

#[must_use] on suspend_template_invocation. The point of this commit is that the bracket is correct wherever it is called from — but a caller that drops the returned bool leaves the window closed for the remainder of the invocation, so every later engine call in it is refused and the transaction rejects. unused_must_use is denied in lints.toml:21, so the attribute turns that into a build failure instead of a silently wrong rejection. restore_template_invocation taking a plain bool means nothing else flags it.

---

Generated by Claude Code