diff --git a/crates/xrpl-wasm-vm/src/abi.rs b/crates/xrpl-wasm-vm/src/abi.rs index 39236203c6..c520bcb4d5 100644 --- a/crates/xrpl-wasm-vm/src/abi.rs +++ b/crates/xrpl-wasm-vm/src/abi.rs @@ -3,54 +3,72 @@ use wasmi::{Caller, Extern, Memory}; use xrpl_host_functions::{HostError, HostFunctionSpec, HostFunctions, HostResult}; // --------------------------------------------------------------------------- -// ABI marshaling: encode a host-function result as a wasm return status -// (`AbiRet`), and charge a call's gas at one point (`charged`) so every -// registered closure pays for itself exactly once. +// ABI marshaling: charge a call's gas at one point (`charged`) so every +// registered closure pays for itself exactly once, and hand the result to the +// engine on one of the two channels a host call answers on — a return code the +// guest reads, or a trap it cannot observe. // --------------------------------------------------------------------------- -/// Encode a scalar or unit host-function result into the status the wasm fn -/// returns (>= 0 a value, < 0 a `HostError` code). Byte-valued results go -/// through [`write_into`] instead, which has nothing to encode. -pub(crate) trait AbiRet { - type Out; - fn write(self, caller: &mut Caller<'_, VmState<'_>>, out: Self::Out) -> HostResult; -} +/// A host-fatal [`HostError`] on its way out of a host call as a wasmi trap. +/// +/// wasmi takes an arbitrary payload out of a host function as long as it +/// implements `wasmi::errors::HostError`, a trait with no methods and no blanket +/// impl. Carrying the `HostError` itself is what lets [`crate::vm::run`] name the +/// condition with `downcast_ref` rather than string-comparing a message, as the +/// C++ path did with its `hfErrOutOfGas` trap strings. +#[derive(Debug, Clone, Copy, PartialEq, Eq)] +pub(crate) struct FatalHostError(pub(crate) HostError); -impl AbiRet for () { - type Out = (); - fn write(self, _c: &mut Caller<'_, VmState<'_>>, _o: ()) -> HostResult { - Ok(0) - } -} -impl AbiRet for u32 { - type Out = (); - fn write(self, _c: &mut Caller<'_, VmState<'_>>, _o: ()) -> HostResult { - Ok(self as i64) +impl wasmi::errors::HostError for FatalHostError {} + +impl core::fmt::Display for FatalHostError { + /// A fixed prefix and the variant's name. wasmi folds this text into its own + /// `Error`'s `Display`, which is the only place it surfaces, so keep it + /// stable and greppable. + fn fmt(&self, f: &mut core::fmt::Formatter<'_>) -> core::fmt::Result { + write!(f, "host call refused: {:?}", self.0) } } -/// Charge a host call's gas from its spec, then run its body. Every registered -/// closure goes through here, so gas cannot be forgotten. +/// Whether a [`HostError`] is host-fatal: the host could not serve the call at +/// all, so the guest is stopped where it stands rather than handed a code it may +/// ignore. Everything else is guest-visible — `OutOfTransferLimit` included, +/// which was the one soft failure in C++ too. +/// +/// Spelled variant by variant rather than as a range over `code()`, so which +/// channel a `HostError` added to the ABI later takes is a choice someone makes +/// here rather than one its number makes for it. +pub(crate) fn is_fatal(error: HostError) -> bool { + matches!( + error, + HostError::OutOfGas | HostError::Internal | HostError::NoMemExported + ) +} + +/// Charge a host call's gas from its spec, run its body, and hand the result to +/// the engine. Every registered closure goes through here, so gas cannot be +/// forgotten. pub(crate) fn charged( caller: &mut Caller<'_, VmState<'_>>, op: HostFunctionSpec, - body: impl FnOnce(&mut Caller<'_, VmState<'_>>) -> HostResult, -) -> HostResult { - charge(caller, op.gas())?; - body(caller) + body: impl FnOnce(&mut Caller<'_, VmState<'_>>) -> HostResult, +) -> Result { + to_wire(charge(caller, op.gas()).and_then(|()| body(caller))) } -pub(crate) fn to_wasm_i32(r: HostResult) -> i32 { - match r { - Ok(v) => v as i32, - Err(e) => e.code(), - } -} -#[allow(dead_code)] -pub(crate) fn to_wasm_i64(r: HostResult) -> i64 { - match r { - Ok(v) => v, - Err(e) => e.code() as i64, +/// Put a host-function result on one of the two channels a host call answers on. +/// +/// A value, or an error the guest is meant to act on, is the `i32` the wasm +/// function returns: `>= 0` a value, `< 0` a [`HostError`] code. A host-fatal +/// error ([`is_fatal`]) leaves as a `wasmi::Error` instead, unwinding the guest +/// at the call — C++ threw for exactly these, and a guest handed `OutOfGas` as a +/// code runs on to the end of its current basic block, a stopping point wasmi's +/// `ConsumeFuel` placement decides rather than the protocol. +fn to_wire(result: HostResult) -> Result { + match result { + Ok(value) => Ok(value), + Err(error) if is_fatal(error) => Err(wasmi::Error::host(FatalHostError(error))), + Err(error) => Ok(error.code()), } } @@ -64,6 +82,9 @@ fn charge(caller: &mut Caller<'_, T>, cost: u64) -> Result<(), HostError> { match remaining.checked_sub(cost) { Some(left) => caller.set_fuel(left).map_err(|_| HostError::Internal), None => { + // Spending what is left makes the run's reported cost the whole gas + // limit, as C++ reports it on out-of-gas. The store outlives the + // trap `OutOfGas` becomes, and `run` reads the cost off it. let _ = caller.set_fuel(0); Err(HostError::OutOfGas) } @@ -135,7 +156,7 @@ pub(crate) fn write_into( dst: i32, cap: i32, fill: impl FnOnce(&dyn HostFunctions, &mut [u8]) -> HostResult, -) -> HostResult { +) -> HostResult { if dst < 0 || cap < 0 { return Err(HostError::InvalidParams); } @@ -159,7 +180,9 @@ pub(crate) fn write_into( return Err(HostError::BufferTooSmall); } charge_transfer(caller.data(), n)?; - Ok(n as i64) + // The cap check above bounds `n`, so the count reaches the wire whole: an + // `i32` the guest reads as a byte count, never a truncation of a larger one. + Ok(n as i32) } // The input buffer in `read_write` lives on the stack, sized to the field cap. @@ -183,7 +206,7 @@ pub(crate) fn read_write( dst: i32, cap: i32, call: impl FnOnce(&dyn HostFunctions, &[u8], &mut [u8]) -> HostResult, -) -> HostResult { +) -> HostResult { if src < 0 || src_len < 0 { return Err(HostError::InvalidParams); } @@ -249,20 +272,71 @@ mod tests { } } - #[test] - fn a_success_becomes_the_value_and_an_error_becomes_its_code() { - assert_eq!(to_wasm_i32(Ok(0)), 0); - assert_eq!(to_wasm_i32(Ok(32)), 32); - assert_eq!(to_wasm_i32(Err(HostError::BufferTooSmall)), -3); - assert_eq!(to_wasm_i64(Ok(32)), 32); - assert_eq!(to_wasm_i64(Err(HostError::BufferTooSmall)), -3); + /// The status a result reaches the guest as. `wasmi::Error` is not `PartialEq`, + /// so a test that expects the guest-visible channel says so here. + fn wire(result: HostResult) -> i32 { + to_wire(result) + .unwrap_or_else(|trap| panic!("expected a guest-visible status, got a trap: {trap}")) } - /// `to_wasm_i32` narrows to the `i32` the wire carries. No host function - /// produces a value that wide, but the cast is silent, so pin it. + /// The guest-visible channel: a value passes through, and a soft error + /// arrives as its negative wire code — a call the engine served either way, + /// because the guest is the one who decides what to do about it. #[test] - fn the_wire_conversion_truncates() { - assert_eq!(to_wasm_i32(Ok(i64::from(i32::MAX) + 1)), i32::MIN); + fn a_success_becomes_the_value_and_an_error_becomes_its_code() { + assert_eq!(wire(Ok(0)), 0); + assert_eq!(wire(Ok(32)), 32); + assert_eq!(wire(Err(HostError::BufferTooSmall)), -3); + } + + /// The three conditions the host cannot serve a call under. Named once, so the + /// two tests below are one statement about the same set. + const FATAL: [HostError; 3] = [ + HostError::OutOfGas, + HostError::Internal, + HostError::NoMemExported, + ]; + + /// The fatal channel: a trap, carrying the condition so `run` can name the + /// outcome without parsing a message. + #[test] + fn a_host_fatal_error_becomes_a_trap_carrying_it() { + for error in FATAL { + let trap = + to_wire(Err(error)).expect_err("a fatal error must not reach the guest as a code"); + let payload = trap.downcast_ref::().unwrap_or_else(|| { + panic!("{error:?}: expected a FatalHostError payload, got: {trap}") + }); + assert_eq!(*payload, FatalHostError(error)); + } + } + + /// Which errors take which channel, as a deliberate change-detector: the + /// three in [`FATAL`] trap, and everything else is a code the guest acts on. + /// + /// `OutOfTransferLimit` is the row worth reading twice. It is the one budget + /// a contract can be expected to handle — C++ made it the single soft failure + /// among these, and this fork keeps that — so a guest asking for more than + /// the run's remaining 1 MiB is told no, not killed. + #[test] + fn only_the_host_fatal_errors_trap() { + for error in FATAL { + assert!(is_fatal(error), "{error:?} must stop the run"); + } + + for error in [ + HostError::OutOfTransferLimit, + HostError::DataFieldTooLarge, + HostError::BufferTooSmall, + HostError::PointerOutOfBounds, + HostError::InvalidParams, + HostError::FieldNotFound, + HostError::Decoding, + HostError::NoRuntime, + ] { + assert!(!is_fatal(error), "{error:?} must reach the guest as a code"); + assert_eq!(wire(Err(error)), error.code()); + } } #[test] diff --git a/crates/xrpl-wasm-vm/src/lib.rs b/crates/xrpl-wasm-vm/src/lib.rs index 3d4aa1290e..35bdbac236 100644 --- a/crates/xrpl-wasm-vm/src/lib.rs +++ b/crates/xrpl-wasm-vm/src/lib.rs @@ -3,5 +3,6 @@ mod register; mod vm; pub use vm::{ - MAX_FIELD_BYTES, MAX_MEMORY_BYTES, MAX_MEMORY_PAGES, RunOutcome, TRANSFER_LIMIT_BYTES, run, + MAX_FIELD_BYTES, MAX_MEMORY_BYTES, MAX_MEMORY_PAGES, RunError, RunFailure, RunOutcome, + TRANSFER_LIMIT_BYTES, run, }; diff --git a/crates/xrpl-wasm-vm/src/register.rs b/crates/xrpl-wasm-vm/src/register.rs index f1d9040031..b96d3ebddc 100644 --- a/crates/xrpl-wasm-vm/src/register.rs +++ b/crates/xrpl-wasm-vm/src/register.rs @@ -1,11 +1,12 @@ -use crate::abi::{AbiRet, charged, read_borrowed, read_write, to_wasm_i32, write_into}; +use crate::abi::{charged, read_borrowed, read_write, write_into}; use crate::vm::VmState; use wasmi::{Caller, Linker}; use xrpl_host_functions::{HostError, HostFunctionSpec}; /// Import module namespace the guest imports host functions from -/// (`(import "host" "ldgr_index" ...)`). -const HOST_MODULE: &str = "host"; +/// (`(import "host_lib" "ldgr_index" ...)`). The guest SDK and this fork's +/// fixtures spell it this way. +const HOST_MODULE: &str = "host_lib"; // --------------------------------------------------------------------------- // Import registration @@ -15,10 +16,10 @@ const HOST_MODULE: &str = "host"; /// /// Driven by an exhaustive `match` over [`HostFn::ALL`]: adding a variant to /// the ABI won't compile until it has an arm here (that's the "can't forget to -/// register" guarantee). Each arm charges gas once via [`charged`] — the sole -/// entry point for `charge` — and marshals its wasm scalars through -/// [`AbiArg`]/[`AbiRet`] before calling straight into the [`HostFunctions`] -/// trait object held in the [`Store`]. +/// register" guarantee). Each arm is one [`charged`] call — the sole entry point +/// for `charge`, and the one place a result is split between the status the guest +/// reads and the trap it cannot — wrapping a body that calls straight into the +/// [`HostFunctions`] trait object held in the [`Store`]. pub(crate) fn register_host_functions(linker: &mut Linker>) -> Result<(), String> { fn link_err(e: wasmi::errors::LinkerError) -> String { format!("register import: {e}") @@ -30,13 +31,16 @@ pub(crate) fn register_host_functions(linker: &mut Linker>) -> Resul HostFunctionSpec::GetLedgerSqn => linker.func_wrap( HOST_MODULE, op.wasm_name(), - |mut caller: Caller<'_, VmState<'_>>, out_ptr: i32, out_len: i32| -> i32 { - to_wasm_i32(charged(&mut caller, HostFunctionSpec::GetLedgerSqn, |c| { + |mut caller: Caller<'_, VmState<'_>>, + out_ptr: i32, + out_len: i32| + -> Result { + charged(&mut caller, HostFunctionSpec::GetLedgerSqn, |c| { // The host writes the serialized sequence number // straight into the guest output region; `write_into` // owns the bounds/cap/buffer/transfer policy. write_into(c, out_ptr, out_len, |host, out| host.get_ledger_sqn(out)) - })) + }) }, ), HostFunctionSpec::GetCurrentLedgerObjField => linker.func_wrap( @@ -46,8 +50,8 @@ pub(crate) fn register_host_functions(linker: &mut Linker>) -> Resul field: i32, out_ptr: i32, out_len: i32| - -> i32 { - to_wasm_i32(charged( + -> Result { + charged( &mut caller, HostFunctionSpec::GetCurrentLedgerObjField, |c| { @@ -58,7 +62,7 @@ pub(crate) fn register_host_functions(linker: &mut Linker>) -> Resul host.get_current_ledger_obj_field(field, out) }) }, - )) + ) }, ), HostFunctionSpec::Sha512Half => linker.func_wrap( @@ -69,8 +73,8 @@ pub(crate) fn register_host_functions(linker: &mut Linker>) -> Resul data_len: i32, out_ptr: i32, out_len: i32| - -> i32 { - to_wasm_i32(charged(&mut caller, HostFunctionSpec::Sha512Half, |c| { + -> Result { + charged(&mut caller, HostFunctionSpec::Sha512Half, |c| { // Input copied into a stack buffer (no heap), output // written straight into guest memory; `read_write` // owns the read/write bounds/cap/transfer policy. @@ -82,7 +86,7 @@ pub(crate) fn register_host_functions(linker: &mut Linker>) -> Resul out_len, |host, data, out| host.sha512_half(data, out), ) - })) + }) }, ), HostFunctionSpec::Trace => linker.func_wrap( @@ -94,8 +98,8 @@ pub(crate) fn register_host_functions(linker: &mut Linker>) -> Resul data_ptr: i32, data_len: i32, as_hex: i32| - -> i32 { - to_wasm_i32(charged(&mut caller, HostFunctionSpec::Trace, |c| { + -> Result { + charged(&mut caller, HostFunctionSpec::Trace, |c| { // Read `msg`/`data` straight out of guest memory — the // slices alias linear memory, no owned copy (`trace` // returns nothing, so there's no output-aliasing worry). @@ -104,8 +108,8 @@ pub(crate) fn register_host_functions(linker: &mut Linker>) -> Resul let data = read_borrowed(c, data_ptr, data_len)?; let msg = core::str::from_utf8(msg).map_err(|_| HostError::Decoding)?; host.trace(msg, data, as_hex != 0)?; - <() as AbiRet>::write((), c, ()) - })) + Ok(0) + }) }, ), HostFunctionSpec::TraceNum => linker.func_wrap( @@ -115,15 +119,15 @@ pub(crate) fn register_host_functions(linker: &mut Linker>) -> Resul msg_ptr: i32, msg_len: i32, number: i64| - -> i32 { - to_wasm_i32(charged(&mut caller, HostFunctionSpec::TraceNum, |c| { + -> Result { + charged(&mut caller, HostFunctionSpec::TraceNum, |c| { // `msg` aliases guest memory — no owned copy. let host = c.data().host; let msg = read_borrowed(c, msg_ptr, msg_len)?; let msg = core::str::from_utf8(msg).map_err(|_| HostError::Decoding)?; host.trace_num(msg, number)?; - <() as AbiRet>::write((), c, ()) - })) + Ok(0) + }) }, ), } diff --git a/crates/xrpl-wasm-vm/src/vm.rs b/crates/xrpl-wasm-vm/src/vm.rs index 2934c7e963..57fc520cfe 100644 --- a/crates/xrpl-wasm-vm/src/vm.rs +++ b/crates/xrpl-wasm-vm/src/vm.rs @@ -1,8 +1,10 @@ use std::cell::Cell; +use std::fmt; use std::sync::LazyLock; -use wasmi::{Config, Engine, Linker, Module, Store, StoreLimits, StoreLimitsBuilder}; -use xrpl_host_functions::HostFunctions; +use wasmi::{Config, Engine, Linker, Module, Store, StoreLimits, StoreLimitsBuilder, TrapCode}; +use xrpl_host_functions::{HostError, HostFunctions}; +use crate::abi::FatalHostError; use crate::register::register_host_functions; /// wasm linear-memory page size, fixed by the wasm spec (64 KiB). @@ -57,6 +59,131 @@ pub struct RunOutcome { pub fuel_used: u64, } +/// Why a run produced no result. Each variant is one outcome for the caller to +/// map to a TER. +#[derive(Debug)] +pub enum RunError { + /// `wasm` is not a valid module under this engine's configuration. + Compile(String), + /// The module compiled but would not instantiate: an import the linker does + /// not define, an initial memory past the page cap, a trapping start section. + Instantiate(String), + /// No export named `function_name` with signature `() -> i32`. + EntryPoint(String), + /// Gas exhausted — by the guest's own instructions or by a host call's + /// charge. [`RunFailure::fuel_used`] is the whole limit. + OutOfGas, + /// The host could not serve a call. + Internal, + /// The module exports no linear memory, so no host call can be served. + NoMemory, + /// The guest trapped: `unreachable`, division by zero, an out-of-bounds + /// access, or `memory.grow` past the page cap. + Trap(String), +} + +impl fmt::Display for RunError { + fn fmt(&self, f: &mut fmt::Formatter<'_>) -> fmt::Result { + match self { + RunError::Compile(detail) => write!(f, "compile: {detail}"), + RunError::Instantiate(detail) => write!(f, "instantiate: {detail}"), + RunError::EntryPoint(detail) => write!(f, "no entry point {detail}"), + RunError::OutOfGas => write!(f, "out of gas"), + RunError::Internal => write!(f, "internal error"), + RunError::NoMemory => write!(f, "no exported memory"), + RunError::Trap(detail) => write!(f, "trap: {detail}"), + } + } +} + +/// A failed run, with the gas it still owes: a contract that traps or exhausts +/// its gas is charged for what it burned. +#[derive(Debug)] +pub struct RunFailure { + pub error: RunError, + /// Fuel consumed before the failure. The whole limit when gas ran out; `0` + /// when the module never ran. + pub fuel_used: u64, +} + +impl fmt::Display for RunFailure { + fn fmt(&self, f: &mut fmt::Formatter<'_>) -> fmt::Result { + write!(f, "{} (fuel used: {})", self.error, self.fuel_used) + } +} + +impl RunFailure { + /// A failure the guest cannot have burned fuel before, because it stopped the + /// run at or before the point the guest first gets to execute. + fn owing_nothing(error: RunError) -> RunFailure { + RunFailure { + error, + fuel_used: 0, + } + } +} + +/// Fuel spent out of `gas`: the one place a run's cost is measured, so success, +/// trap and refusal all report it the same way. +fn fuel_used(store: &Store>, gas: u64) -> u64 { + gas.saturating_sub(store.get_fuel().unwrap_or(0)) +} + +/// The outcome a `wasmi::Error` names for itself, if it names one, rather than +/// leaving it to the stage that raised it. +/// +/// A host call the host could not serve traps with a [`FatalHostError`] payload, +/// which says which condition it was, so check for that before treating the error +/// as the guest's own doing. wasmi raises `OutOfFuel` when the guest's +/// *instructions* exhaust the meter — the same outcome by a different route, and +/// `as_trap_code` reports it whichever error kind carried it. +/// +/// Both arise anywhere the guest executes, and a start section is guest code +/// running during instantiation, so every stage from there on asks this before +/// naming a failure after itself. +fn guest_halted(error: &wasmi::Error) -> Option { + if let Some(fatal) = error.downcast_ref::() { + return Some(host_fatal(fatal.0)); + } + (error.as_trap_code() == Some(TrapCode::OutOfFuel)).then_some(RunError::OutOfGas) +} + +/// The outcome a host-fatal `HostError` is. +/// +/// Exhaustive over `HostError` rather than closed with a wildcard, so a variant +/// added to the ABI has to be placed here before this compiles. Moving an +/// existing variant into [`crate::abi::is_fatal`]'s set is not caught that way — +/// it lands in the soft arm and reports `Internal` — so the two are read +/// together. The soft arm is otherwise unreachable: a guest-visible error is a +/// return code and never becomes a trap for [`guest_halted`] to unwrap. +fn host_fatal(error: HostError) -> RunError { + match error { + HostError::OutOfGas => RunError::OutOfGas, + HostError::Internal => RunError::Internal, + HostError::NoMemExported => RunError::NoMemory, + HostError::FieldNotFound + | HostError::BufferTooSmall + | HostError::NoArray + | HostError::NotLeafField + | HostError::LocatorMalformed + | HostError::SlotOutRange + | HostError::SlotsFull + | HostError::EmptySlot + | HostError::LedgerObjNotFound + | HostError::Decoding + | HostError::DataFieldTooLarge + | HostError::PointerOutOfBounds + | HostError::InvalidParams + | HostError::InvalidAccount + | HostError::InvalidField + | HostError::IndexOutOfBounds + | HostError::FloatInputMalformed + | HostError::FloatComputationError + | HostError::NoRuntime + | HostError::OutOfTransferLimit => RunError::Internal, + } +} + /// The process-wide wasmi engine, built once on first use. /// /// The configuration is consensus-fixed and identical for every invocation, and @@ -97,9 +224,10 @@ pub fn run<'h>( gas: u64, host: &'h dyn HostFunctions, function_name: &str, -) -> Result { +) -> Result { let engine = wasm_engine(); - let module = Module::new(engine, wasm).map_err(|e| format!("compile: {e}"))?; + let module = Module::new(engine, wasm) + .map_err(|e| RunFailure::owing_nothing(RunError::Compile(e.to_string())))?; let mem_limits = StoreLimitsBuilder::new() .memory_size(MAX_MEMORY_BYTES) @@ -113,29 +241,55 @@ pub fn run<'h>( transfer_budget: Cell::new(TRANSFER_LIMIT_BYTES), }, ); - store.set_fuel(gas).map_err(|e| format!("set_fuel: {e}"))?; + // A store that will not take fuel, or imports that will not register, are + // defects in the engine configuration or in this crate, not in the module: + // nothing the contract did could have caused either. + store + .set_fuel(gas) + .map_err(|_| RunFailure::owing_nothing(RunError::Internal))?; // The memory-page cap applies at instantiation too: an initial memory // declared past it fails to instantiate, as a `memory.grow` past it traps. store.limiter(|state| &mut state.mem_limits); let mut linker = Linker::>::new(engine); - register_host_functions(&mut linker)?; + register_host_functions(&mut linker) + .map_err(|_| RunFailure::owing_nothing(RunError::Internal))?; - let instance = linker - .instantiate_and_start(&mut store, &module) - .map_err(|e| format!("instantiate: {e}"))?; - let finish = instance - .get_typed_func::<(), i32>(&store, function_name) - .map_err(|e| format!("no entry point '{function_name}': {e}"))?; + // Instantiation is the first point the guest can execute, through a start + // section, so from here on the cost comes off the store rather than being + // known to be nothing. + let instance = match linker.instantiate_and_start(&mut store, &module) { + Ok(instance) => instance, + Err(e) => { + return Err(RunFailure { + error: guest_halted(&e).unwrap_or_else(|| RunError::Instantiate(e.to_string())), + fuel_used: fuel_used(&store, gas), + }); + } + }; + let finish = match instance.get_typed_func::<(), i32>(&store, function_name) { + Ok(finish) => finish, + Err(e) => { + return Err(RunFailure { + error: RunError::EntryPoint(format!("'{function_name}': {e}")), + fuel_used: fuel_used(&store, gas), + }); + } + }; - let result = finish - .call(&mut store, ()) - .map_err(|e| format!("trap: {e}"))?; + let result = match finish.call(&mut store, ()) { + Ok(result) => result, + Err(e) => { + return Err(RunFailure { + error: guest_halted(&e).unwrap_or_else(|| RunError::Trap(e.to_string())), + fuel_used: fuel_used(&store, gas), + }); + } + }; - let remaining = store.get_fuel().unwrap_or(0); Ok(RunOutcome { result, - fuel_used: gas.saturating_sub(remaining), + fuel_used: fuel_used(&store, gas), }) } diff --git a/crates/xrpl-wasm-vm/tests/budgets.rs b/crates/xrpl-wasm-vm/tests/budgets.rs index 95f9e7ac6b..b9d5a63163 100644 --- a/crates/xrpl-wasm-vm/tests/budgets.rs +++ b/crates/xrpl-wasm-vm/tests/budgets.rs @@ -6,7 +6,7 @@ mod support; use support::{Answer, FakeHost, ONE_PAGE, PLENTY_OF_GAS, code, import, module, run, run_with_gas}; use xrpl_host_functions::{HostError, HostFunctionSpec}; -use xrpl_wasm_vm::{MAX_FIELD_BYTES, TRANSFER_LIMIT_BYTES}; +use xrpl_wasm_vm::{MAX_FIELD_BYTES, RunError, TRANSFER_LIMIT_BYTES}; // --------------------------------------------------------------------------- // Gas @@ -157,8 +157,8 @@ fn fuel_used_is_what_was_spent_not_what_was_supplied() { let cost = EMPTY_MODULE_FUEL + wasmi_call_fuel(operands) + op.gas(); // Exactly its cost is enough, and no amount above it changes the figure. The - // result is checked too: a refused call burns the whole limit, which at - // `gas == cost` is the same number. + // result is checked too, so the figure belongs to a run that did the work + // rather than to one that was cut short. for gas in [cost, cost + 1, cost * 100, PLENTY_OF_GAS] { let outcome = run_with_gas(&wat, gas, &host).expect("should run"); assert_eq!( @@ -168,16 +168,15 @@ fn fuel_used_is_what_was_spent_not_what_was_supplied() { assert_eq!(outcome.fuel_used, cost, "gas {gas}"); } - // One fuel short: the call is refused rather than fatal, so the run completes - // and the guest reads `OutOfGas` off the return (finding A1). - let short = run_with_gas(&wat, cost - 1, &host).expect("completes today; see finding A1"); - assert_eq!(short.result, code(HostError::OutOfGas)); - assert_eq!( - short.fuel_used, - cost - 1, - "a call it cannot afford burns the whole limit — `charge` zeroes the fuel, \ - which is what makes the reported cost the full budget as in C++" + // One fuel short: the run ends at the call it cannot pay for, and still owes + // the gas — the whole limit, because `charge` spends what is left, which is + // what makes the reported cost the full budget as in C++. + let short = run_with_gas(&wat, cost - 1, &host).expect_err("one fuel short must not complete"); + assert!( + matches!(short.error, RunError::OutOfGas), + "expected the run to end out of gas, got: {short}" ); + assert_eq!(short.fuel_used, cost - 1); } /// Fuel is metered, so the same module burns the same fuel every time — a @@ -199,7 +198,8 @@ fn the_same_run_burns_the_same_fuel() { assert!(first > HostFunctionSpec::Trace.gas()); } -/// Too little gas to finish stops the run. +/// Too little gas to finish stops the run: the meter refuses the guest's own +/// instructions before it ever reaches the host call. #[test] fn a_run_that_cannot_afford_itself_fails() { let host = FakeHost::new(); @@ -209,53 +209,69 @@ fn a_run_that_cannot_afford_itself_fails() { ); for gas in [0, 1, 10] { - let outcome = run_with_gas(&wat, gas, &host); + let Err(failure) = run_with_gas(&wat, gas, &host) else { + panic!("gas {gas} should not have completed"); + }; assert!( - outcome.is_err(), - "gas {gas} should not have completed: {outcome:?}" + matches!(failure.error, RunError::OutOfGas), + "gas {gas}: expected the run to end out of gas, got: {failure}" ); } } -/// A guest looping forever is stopped by gas rather than running away. +/// A guest looping forever is stopped by gas rather than running away, and owes +/// the gas it burned doing it. #[test] fn an_endless_loop_is_stopped_by_gas() { + const GAS: u64 = 100_000; + let host = FakeHost::new(); let wat = module(&[ONE_PAGE], "(loop $l (br $l)) (i32.const 0)"); - let failure = - run_with_gas(&wat, 100_000, &host).expect_err("an endless loop must not complete"); - assert!(failure.contains("trap"), "{failure}"); + let failure = run_with_gas(&wat, GAS, &host).expect_err("an endless loop must not complete"); + assert!( + matches!(failure.error, RunError::OutOfGas), + "expected the meter to stop it, got: {failure}" + ); + assert_eq!( + failure.fuel_used, GAS, + "a runaway guest burns the whole limit" + ); } -/// **Pins current behaviour, not a decision.** A host call that runs out of gas -/// returns `OutOfGas` to the guest as a negative code, and the guest keeps running. -/// Finding A1 in `docs/claude/redesign_impl.md` says this should become a trap. +/// A host call refused its gas stops the run: the guest never gets a chance to +/// ignore the refusal and carry on, and it is charged the whole limit. +/// +/// The gas range is every amount that reaches the call and cannot pay for it, so +/// the case is the whole boundary rather than one number. #[test] -fn out_of_gas_in_a_host_call_currently_reaches_the_guest_as_a_code() { +fn a_host_call_refused_its_gas_stops_the_run() { let host = FakeHost::new(); + let op = HostFunctionSpec::TraceNum; + let Call { + import, + call, + operands, + } = call_for(op); + let wat = module(&[import, ONE_PAGE], call); + // What the guest spends getting as far as the call. Below it the meter stops + // the guest's own instructions instead, which is + // `a_run_that_cannot_afford_itself_fails`'s case, not this one. + let reaching_the_call = EMPTY_MODULE_FUEL + wasmi_call_fuel(operands); - // Enough gas to enter the call and be refused its 500, then return. - let wat = module( - &[import::TRACE_NUM, ONE_PAGE], - "(call $trace_num (i32.const 0) (i32.const 0) (i64.const 0))", - ); - - let mut seen_as_code = false; - for gas in 20..500 { - if let Ok(outcome) = run_with_gas(&wat, gas, &host) { - assert_eq!( - outcome.result, - code(HostError::OutOfGas), - "gas {gas} completed with an unexpected status" - ); - seen_as_code = true; - } + for gas in reaching_the_call..reaching_the_call + op.gas() { + let Err(failure) = run_with_gas(&wat, gas, &host) else { + panic!("gas {gas}: the run completed, so the guest was handed the refusal"); + }; + assert!( + matches!(failure.error, RunError::OutOfGas), + "gas {gas}: expected the run to end out of gas, got: {failure}" + ); + assert_eq!( + failure.fuel_used, gas, + "gas {gas}: a call it cannot afford burns the whole limit" + ); } - assert!( - seen_as_code, - "expected some gas amount to let the guest observe OutOfGas as a return code" - ); assert!(host.traces().is_empty(), "the host body must not have run"); } diff --git a/crates/xrpl-wasm-vm/tests/memory_policy.rs b/crates/xrpl-wasm-vm/tests/memory_policy.rs index 41dc555a97..92330412fe 100644 --- a/crates/xrpl-wasm-vm/tests/memory_policy.rs +++ b/crates/xrpl-wasm-vm/tests/memory_policy.rs @@ -4,9 +4,9 @@ mod support; -use support::{Answer, FakeHost, ONE_PAGE, code, import, module, status}; +use support::{Answer, FakeHost, ONE_PAGE, code, failure, import, module, status}; use xrpl_host_functions::{HASH_LEN, HostError}; -use xrpl_wasm_vm::MAX_FIELD_BYTES; +use xrpl_wasm_vm::{MAX_FIELD_BYTES, RunError}; /// One page, so anything at or past 65536 is out of bounds. const PAGE: i64 = 64 * 1024; @@ -371,6 +371,19 @@ fn an_input_may_overlap_the_output() { // The memory export itself // --------------------------------------------------------------------------- +/// A host call with no memory to work in ends the run instead of answering the +/// guest: there is no buffer for a status to describe, and nothing the guest could +/// do about the answer — which is what puts this beside out-of-gas on the fatal +/// channel. What the guest burned getting there is still charged. +fn assert_no_memory(wat: &str, host: &FakeHost) { + let failure = failure(wat, host); + assert!( + matches!(failure.error, RunError::NoMemory), + "expected the run to end for want of a memory export, got: {failure}" + ); + assert!(failure.fuel_used > 0, "{failure}"); +} + /// Every region is relative to the guest's exported memory, so a module without /// one cannot make a host call at all. #[test] @@ -381,7 +394,7 @@ fn a_module_that_exports_no_memory_cannot_call_the_host() { &[import::LDGR_INDEX, "(memory 1)"], "(call $ldgr_index (i32.const 0) (i32.const 4))", ); - assert_eq!(status(&wat, &host), code(HostError::NoMemExported)); + assert_no_memory(&wat, &host); } /// The export has to be named `memory`, and it has to *be* a memory — a global @@ -395,7 +408,7 @@ fn the_memory_export_must_be_a_memory_named_memory() { &[import::LDGR_INDEX, r#"(memory (export "mem") 1)"#], "(call $ldgr_index (i32.const 0) (i32.const 4))", ); - assert_eq!(status(&misnamed, &host), code(HostError::NoMemExported)); + assert_no_memory(&misnamed, &host); // The right name on the wrong kind, which is the other arm of the match. let wrong_kind = module( @@ -406,7 +419,7 @@ fn the_memory_export_must_be_a_memory_named_memory() { ], "(call $ldgr_index (i32.const 0) (i32.const 4))", ); - assert_eq!(status(&wrong_kind, &host), code(HostError::NoMemExported)); + assert_no_memory(&wrong_kind, &host); } /// Bounds follow the memory the module actually declared, not a fixed page. diff --git a/crates/xrpl-wasm-vm/tests/support/mod.rs b/crates/xrpl-wasm-vm/tests/support/mod.rs index e58caa0b3a..813d049bea 100644 --- a/crates/xrpl-wasm-vm/tests/support/mod.rs +++ b/crates/xrpl-wasm-vm/tests/support/mod.rs @@ -11,7 +11,7 @@ use std::cell::RefCell; use std::collections::HashMap; use xrpl_host_functions::{HostError, HostFunctions, HostResult}; -use xrpl_wasm_vm::RunOutcome; +use xrpl_wasm_vm::{RunFailure, RunOutcome}; /// The entry point every test module exports. pub const ENTRY: &str = "finish"; @@ -185,20 +185,18 @@ impl HostFunctions for FakeHost { // Module pieces // --------------------------------------------------------------------------- -/// One `(import …)` declaration per host function, spelled with the signature it -/// is registered under and binding the `$name` call sites use. A wrong signature -/// fails instantiation. +/// One `(import …)` declaration per host function, spelled with the module name +/// and signature it is registered under and binding the `$name` call sites use. A +/// wrong module name or signature fails instantiation. pub mod import { pub const LDGR_INDEX: &str = - r#"(import "host" "ldgr_index" (func $ldgr_index (param i32 i32) (result i32)))"#; - pub const HOME_LE_FIELD: &str = - r#"(import "host" "home_le_field" (func $home_le_field (param i32 i32 i32) (result i32)))"#; - pub const SHA512_HALF: &str = - r#"(import "host" "sha512_half" (func $sha512_half (param i32 i32 i32 i32) (result i32)))"#; + r#"(import "host_lib" "ldgr_index" (func $ldgr_index (param i32 i32) (result i32)))"#; + pub const HOME_LE_FIELD: &str = r#"(import "host_lib" "home_le_field" (func $home_le_field (param i32 i32 i32) (result i32)))"#; + pub const SHA512_HALF: &str = r#"(import "host_lib" "sha512_half" (func $sha512_half (param i32 i32 i32 i32) (result i32)))"#; pub const TRACE: &str = - r#"(import "host" "trace" (func $trace (param i32 i32 i32 i32 i32) (result i32)))"#; + r#"(import "host_lib" "trace" (func $trace (param i32 i32 i32 i32 i32) (result i32)))"#; pub const TRACE_NUM: &str = - r#"(import "host" "trace_num" (func $trace_num (param i32 i32 i64) (result i32)))"#; + r#"(import "host_lib" "trace_num" (func $trace_num (param i32 i32 i64) (result i32)))"#; } /// One page of linear memory, exported under the name the engine looks for. @@ -229,17 +227,17 @@ pub fn assemble(wat: &str) -> Vec { } /// Runs `wat`'s `finish` against `host` with gas to spare. -pub fn run(wat: &str, host: &FakeHost) -> Result { +pub fn run(wat: &str, host: &FakeHost) -> Result { run_with_gas(wat, PLENTY_OF_GAS, host) } /// Runs `wat`'s `finish` against `host` with exactly `gas` to spend. -pub fn run_with_gas(wat: &str, gas: u64, host: &FakeHost) -> Result { +pub fn run_with_gas(wat: &str, gas: u64, host: &FakeHost) -> Result { xrpl_wasm_vm::run(&assemble(wat), gas, host, ENTRY) } /// Runs the export named `entry` rather than `finish`. -pub fn run_entry(wat: &str, host: &FakeHost, entry: &str) -> Result { +pub fn run_entry(wat: &str, host: &FakeHost, entry: &str) -> Result { xrpl_wasm_vm::run(&assemble(wat), PLENTY_OF_GAS, host, entry) } @@ -256,10 +254,10 @@ pub fn code(error: HostError) -> i32 { error.code() } -/// The error message from a run that was expected to fail. -pub fn failure(wat: &str, host: &FakeHost) -> String { +/// The failure from a run that was expected not to complete. +pub fn failure(wat: &str, host: &FakeHost) -> RunFailure { match run(wat, host) { - Err(message) => message, + Err(failure) => failure, Ok(outcome) => panic!( "expected a failure, but the module returned {}", outcome.result diff --git a/crates/xrpl-wasm-vm/tests/vm_limits.rs b/crates/xrpl-wasm-vm/tests/vm_limits.rs index 492ba47a59..b64c0945c4 100644 --- a/crates/xrpl-wasm-vm/tests/vm_limits.rs +++ b/crates/xrpl-wasm-vm/tests/vm_limits.rs @@ -9,15 +9,21 @@ mod support; use support::{ FakeHost, ONE_PAGE, PLENTY_OF_GAS, failure, import, module, run, run_entry, run_with_gas, }; -use xrpl_wasm_vm::MAX_MEMORY_PAGES; +use xrpl_wasm_vm::{MAX_MEMORY_PAGES, RunError}; -/// A failure message has to say which stage failed, because the caller maps the -/// stages to different outcomes. -fn assert_stage(message: &str, stage: &str) { - assert!( - message.starts_with(stage), - "expected a {stage:?} failure, got: {message}" - ); +/// Assert which stage a run failed at, because the caller maps the stages to +/// different outcomes. A stage is one `RunError` variant, so the expectation is a +/// pattern; the failure comes back out for the tests that also read its message. +macro_rules! assert_stage { + ($failure:expr, $stage:pat) => {{ + let failure = $failure; + assert!( + matches!(failure.error, $stage), + concat!("expected a ", stringify!($stage), " failure, got: {}"), + failure + ); + failure + }}; } // --------------------------------------------------------------------------- @@ -37,7 +43,7 @@ fn an_initial_memory_past_the_cap_is_refused() { )], "(i32.const 0)", ); - assert_stage(&failure(&wat, &host), "instantiate"); + assert_stage!(failure(&wat, &host), RunError::Instantiate(_)); } /// The cap itself is allowed. @@ -73,7 +79,7 @@ fn growth_stops_at_the_cap() { &[ONE_PAGE], &format!("(memory.grow (i32.const {MAX_MEMORY_PAGES}))"), ); - assert_stage(&failure(&wat, &host), "trap"); + assert_stage!(failure(&wat, &host), RunError::Trap(_)); } /// A module may declare a maximum above the cap: the cap is enforced on the initial @@ -90,7 +96,7 @@ fn a_declared_maximum_past_the_cap_is_allowed_but_unreachable() { &[&memory], &format!("(memory.grow (i32.const {MAX_MEMORY_PAGES}))"), ); - assert_stage(&failure(&wat, &host), "trap"); + assert_stage!(failure(&wat, &host), RunError::Trap(_)); } // --------------------------------------------------------------------------- @@ -201,9 +207,8 @@ fn every_disabled_feature_is_refused_by_name() { for (knob, parts, body, expected) in disabled_features() { let wat = module(&parts, body); - let failure = failure(&wat, &host); + let failure = assert_stage!(failure(&wat, &host), RunError::Compile(_)).to_string(); - assert_stage(&failure, "compile"); assert!( failure.contains(expected), "{knob}: expected a refusal mentioning {expected:?}, got: {failure}" @@ -222,7 +227,7 @@ fn the_knobs_without_a_module_of_their_own() { // `wasm_saturating_float_to_int(false)`: every saturating conversion takes a // float operand, so `floats(false)` refuses it first, as the message shows. let wat = module(&[ONE_PAGE], "(i32.trunc_sat_f32_s (f32.const 1))"); - let refusal = failure(&wat, &host); + let refusal = failure(&wat, &host).to_string(); assert!(refusal.contains("floating-point"), "{refusal}"); assert!(!refusal.contains("saturating"), "{refusal}"); @@ -261,7 +266,7 @@ fn garbage_does_not_compile() { for bytes in [b"".as_slice(), b"not wasm", &[0x00, 0x61, 0x73, 0x6d]] { let failure = xrpl_wasm_vm::run(bytes, PLENTY_OF_GAS, &host, support::ENTRY) .expect_err("garbage must not compile"); - assert_stage(&failure, "compile"); + assert_stage!(failure, RunError::Compile(_)); } } @@ -275,7 +280,7 @@ fn the_vm_refuses_a_text_format_module() { let failure = xrpl_wasm_vm::run(text.as_bytes(), PLENTY_OF_GAS, &host, support::ENTRY) .expect_err("text must not compile as a module"); - assert_stage(&failure, "compile"); + assert_stage!(failure, RunError::Compile(_)); // The same module, assembled first, runs: the text is sound and only the // format was refused. @@ -294,23 +299,22 @@ fn an_unknown_import_fails_instantiation() { let wat = module( &[ - r#"(import "host" "no_such_function" (func $f (param i32) (result i32)))"#, + r#"(import "host_lib" "no_such_function" (func $f (param i32) (result i32)))"#, ONE_PAGE, ], "(call $f (i32.const 0))", ); - assert_stage(&failure(&wat, &host), "instantiate"); + assert_stage!(failure(&wat, &host), RunError::Instantiate(_)); } -/// Host functions are registered under one module name, and a guest naming a -/// different one does not link. Which name is an open ABI question: this fork -/// registers `host`, the guest SDK and this repo's fixtures use `host_lib`, and -/// plain clang emits `env`. +/// Host functions are registered under one module name — `host_lib`, the name the +/// guest SDK and this repo's fixtures import from — and a guest naming a different +/// one does not link. `env` is in the list because that is what plain clang emits. #[test] fn the_import_module_name_must_match() { let host = FakeHost::new(); - for module_name in ["host_lib", "env", ""] { + for module_name in ["host", "env", ""] { let wat = module( &[ &format!( @@ -320,7 +324,7 @@ fn the_import_module_name_must_match() { ], "(call $f (i32.const 0) (i32.const 4))", ); - assert_stage(&failure(&wat, &host), "instantiate"); + assert_stage!(failure(&wat, &host), RunError::Instantiate(_)); } } @@ -339,12 +343,12 @@ fn an_import_with_the_wrong_signature_fails_instantiation() { ] { let wat = module( &[ - &format!(r#"(import "host" "ldgr_index" (func $f {signature}))"#), + &format!(r#"(import "host_lib" "ldgr_index" (func $f {signature}))"#), ONE_PAGE, ], "(i32.const 0)", ); - assert_stage(&failure(&wat, &host), "instantiate"); + assert_stage!(failure(&wat, &host), RunError::Instantiate(_)); } } @@ -360,6 +364,61 @@ fn an_unused_import_is_still_linked() { assert_eq!(run(&wat, &host).expect("should run").result, 0); } +// --------------------------------------------------------------------------- +// The start section +// --------------------------------------------------------------------------- + +/// A start section runs guest code during instantiation, before the entry point +/// is even looked up, and `set_fuel` and the memory limiter are both installed by +/// then — so it is metered like any other guest code, and a run it stops is +/// charged for what it burned. +#[test] +fn a_trapping_start_section_fails_instantiation_and_is_charged() { + let host = FakeHost::new(); + + let wat = format!( + r#"(module {ONE_PAGE} + (func $init (unreachable)) + (start $init) + (func (export "finish") (result i32) (i32.const 0)))"# + ); + let failure = assert_stage!( + run_with_gas(&wat, PLENTY_OF_GAS, &host) + .expect_err("a start section that traps must not instantiate"), + RunError::Instantiate(_) + ); + assert!( + failure.fuel_used > 0, + "the start section's instructions are metered: {failure}" + ); +} + +/// A start section that runs out of gas is reported as out of gas, not as a module +/// that would not instantiate. The stage a run stopped at is not what the caller +/// maps — the reason is — and gas exhaustion is one outcome wherever the guest +/// reaches it. +#[test] +fn a_start_section_that_exhausts_gas_is_out_of_gas_not_an_instantiation_failure() { + const GAS: u64 = 10_000; + + let host = FakeHost::new(); + let wat = format!( + r#"(module {ONE_PAGE} + (func $init (loop $l (br $l))) + (start $init) + (func (export "finish") (result i32) (i32.const 0)))"# + ); + + let failure = assert_stage!( + run_with_gas(&wat, GAS, &host).expect_err("an endless start section must not instantiate"), + RunError::OutOfGas + ); + assert_eq!( + failure.fuel_used, GAS, + "a runaway start section burns the whole limit" + ); +} + // --------------------------------------------------------------------------- // The entry point // --------------------------------------------------------------------------- @@ -369,9 +428,15 @@ fn a_missing_entry_point_fails() { let host = FakeHost::new(); let wat = r#"(module (memory (export "memory") 1) (func (export "other") (result i32) (i32.const 0)))"#; - let failure = run_with_gas(wat, PLENTY_OF_GAS, &host) - .expect_err("a module without the entry point must not run"); - assert!(failure.contains("no entry point 'finish'"), "{failure}"); + let failure = assert_stage!( + run_with_gas(wat, PLENTY_OF_GAS, &host) + .expect_err("a module without the entry point must not run"), + RunError::EntryPoint(_) + ); + assert!( + failure.to_string().contains("no entry point 'finish'"), + "{failure}" + ); } /// The entry point is looked up by the name the caller asks for. @@ -403,7 +468,8 @@ fn an_entry_point_of_the_wrong_type_fails() { r#"(module (memory (export "memory") 1) (func (export "finish") {signature} {body}))"# ); let failure = run_with_gas(&wat, PLENTY_OF_GAS, &host) - .expect_err("a wrongly-typed entry point must not run"); + .expect_err("a wrongly-typed entry point must not run") + .to_string(); assert!(failure.contains("no entry point"), "{signature}: {failure}"); } } @@ -414,10 +480,10 @@ fn a_trapping_guest_fails_the_run() { let host = FakeHost::new(); let wat = module(&[ONE_PAGE], "(unreachable)"); - assert_stage(&failure(&wat, &host), "trap"); + assert_stage!(failure(&wat, &host), RunError::Trap(_)); // An out-of-bounds guest access is a trap too, caught by the engine rather // than anything the host is asked about. let wat = module(&[ONE_PAGE], "(i32.load (i32.const 100000))"); - assert_stage(&failure(&wat, &host), "trap"); + assert_stage!(failure(&wat, &host), RunError::Trap(_)); } diff --git a/docs/claude/redesign_impl.md b/docs/claude/redesign_impl.md index 0ae5322fb5..ee35d07b64 100644 --- a/docs/claude/redesign_impl.md +++ b/docs/claude/redesign_impl.md @@ -344,8 +344,8 @@ Found while auditing the guest SDK (`~/Documents/rust/xrpl-wasm-stdlib`, checkou `wasm_importtype_module()` is commented out at `src/libxrpl/tx/wasm/WasmiVM.cpp:429-431` and only the field name is looked up. `register.rs:8` now enforces `"host"`. The SDK and the fork's own fixture (`src/test/app/wasm_fixtures/codecov_tests/src/host_bindings_loose.rs:20`) - use `"host_lib"`. Plain clang emits `"env"` unless annotated. `"host"` currently - matches nothing that exists. + use `"host_lib"`. Plain clang emits `"env"` unless annotated. `"host"` matched + nothing that exists. **Resolved: `host_lib`** (finding A3). 2. **Import name lineage.** The fixtures pin the SDK at `branch = renames` and use **short** wire names (`parent_ldgr_hash`, `cache_le`, `tx_inner_arr_len`, `accountroot_id`, `trustline_id`), matching rippled's `ldgr_index` / `home_le_field` @@ -365,6 +365,15 @@ Found while auditing the guest SDK (`~/Documents/rust/xrpl-wasm-stdlib`, checkou *Still open*: is `OutOfTransferLimit` soft or fatal? The guest has no code for it — `-11` is `InvalidDecoding` there but `OutOfTransferLimit` in `WasmCommon.h:47`. Fatal is the only resolution that needs no SDK change. + + **Partly resolved by A1**, and the remainder is sharper for it. The trap channel + exists, and `OutOfGas = -22` no longer reaches the guest at all. But the decision + was **`OutOfTransferLimit` stays soft** (C++ parity — it was the one soft failure + there), so `-23` still reaches a guest that transmutes it, and `NoRuntime = -21` + still would if anything returned it. So the guest-visible table is `-1..-20` plus + those two, not `-1..-20`: closing this needs either a range check in the SDK or + `OutOfTransferLimit` remapped onto an in-range code. The soft/fatal question is + settled; the encoding question is not. 4. **`-1` collides semantically**: host `Unimplemented` vs guest `InternalError`. 5. **`float_to_mant_exp` byte count.** Host returns **12** (8 mantissa + 4 exponent, `HostFuncWrapper.cpp:497` at `b7059deb9f^`); the guest doc says 8. The guest's @@ -391,8 +400,8 @@ matter. Items marked ✓ are done. ### A. Correctness — behaviour changes, land before the cxx bridge -1. **Out-of-gas is not a trap, and how much guest code runs after exhaustion is - wasmi's business.** `charge` (`abi.rs:77`) returns `HostError::OutOfGas`, which +1. ✓ **Out-of-gas is not a trap, and how much guest code runs after exhaustion is + wasmi's business.** `charge` (`abi.rs:77`) returned `HostError::OutOfGas`, which `to_wasm_i32` hands the guest as `-22` with fuel already at 0. wasmi meters by emitting `ConsumeFuel` instructions at *block boundaries* (`engine/translator/func/instrs.rs`), so the guest keeps executing to the end of @@ -404,16 +413,39 @@ matter. Items marked ✓ are done. return `Err(wasmi::Error)` from the closure and trap. The wasm signature is unchanged. This reshapes `abi.rs`'s return type, so it precedes any cosmetic work there. -2. **`run` discards gas accounting on every failure path.** `Result` (`vm.rs:96`) means a trap yields `Err(String)` with no `fuel_used` — but a + + Landed with A2 and A3. The fatal set is carried out of a closure as + `abi::FatalHostError(HostError)`, a payload `wasmi::Error::host` accepts and + `run` names again with `downcast_ref` — so the condition survives the crossing + as a value rather than as message text, which is what the C++ path had to + string-compare (`"HfOutOfGas"`). `is_fatal` spells the set variant by variant, + so which channel a new `HostError` takes is a choice someone makes rather than + one its number makes for it. **`OutOfTransferLimit` stays soft** — the decision + below, and C++'s behaviour. +2. ✓ **`run` discards gas accounting on every failure path.** `Result` (`vm.rs:96`) meant a trap yielded `Err(String)` with no `fuel_used` — but a contract that traps or exhausts gas still has to be charged (C++: full limit → `tecOUT_OF_GAS`; internal → `tecINTERNAL`). `String` also cannot be matched on, so the cxx bridge would end up string-comparing error text, which is exactly what the deleted C++ did with its `"HfOutOfGas"` trap strings. `fuel_used` belongs on both paths, and the error wants to be a typed enum C++ can map to a TER. -3. **`HOST_MODULE = "host"` (`register.rs:8`) matches no guest that exists** — the SDK + + Now `Result`, where `RunFailure` is `{ error: RunError, + fuel_used }` — so the gas is on both paths by construction rather than by + remembering. `RunError` is `Compile`/`Instantiate`/`EntryPoint`/`Trap`, each + carrying wasmi's diagnostic, plus `OutOfGas`/`Internal`/`NoMemory`, which carry + nothing because the variant *is* the information C++ needs. Gas exhaustion + reaches `run` by two routes — wasmi's own `OutOfFuel` for guest instructions, + our trap payload for a refused host charge — and both land on `OutOfGas`. + `guest_halted` asks that question at **every** stage from instantiation on, so a + start section that burns the limit is `OutOfGas` and not `Instantiate`: the stage + a run stopped at is not what the caller maps. +3. ✓ **`HOST_MODULE = "host"` (`register.rs:8`) matches no guest that exists** — the SDK and this fork's own fixtures use `host_lib`, plain clang emits `env`. A decision, not a code fix, but nothing real instantiates until it is made (open question 1). + **Decided: `host_lib`**, matching the SDK and the fixtures. `the_import_module_name_must_match` + now rejects `host`, `env` and the empty name, so the choice is pinned rather than + incidental. 4. **The transfer budget is charged for bytes that are never copied, and charged before validation.** `read_borrowed` *aliases* guest memory — zero copies — yet calls `charge_transfer` (`abi.rs:135`); C++ deliberately did not charge plain @@ -450,13 +482,17 @@ matter. Items marked ✓ are done. ### B. Dead weight — pure simplification, no behaviour change -6. **`AbiRet` is vestigial.** `type Out` is always `()`, `impl AbiRet for u32` is never +6. ✓ **`AbiRet` is vestigial.** `type Out` is always `()`, `impl AbiRet for u32` is never used, and the trait's only call site is `<() as AbiRet>::write((), c, ())` — nine - tokens for `Ok(0)`. Delete the trait and both impls. -7. **The `i64` pipeline is pointless and lossy.** Every host function returns `i32` on - the wire, but the internals thread `HostResult` and `to_wasm_i32` then does - `v as i32` — a silent truncating cast on a consensus path. `to_wasm_i64` is dead - code behind `#[allow]`. `HostResult` end to end removes both. + tokens for `Ok(0)`. Delete the trait and both impls. Done with A1, which rewrote + those call sites anyway. +7. ✓ **The `i64` pipeline is pointless and lossy.** Every host function returns `i32` on + the wire, but the internals threaded `HostResult` and `to_wasm_i32` then did + `v as i32` — a silent truncating cast on a consensus path. `to_wasm_i64` was dead + code behind `#[allow]`. `HostResult` end to end removed both. Done with A1 + for the same reason as B6: A1 rewrites exactly these signatures, and the `n as + i32` in `write_into` now sits after the `MAX_FIELD_BYTES` check, where it cannot + lose bits. 8. **`cxx` is an unused dependency** of this crate — the bridge lives in the ffi crate. 9. **Stale docs.** Seven broken intra-doc links name types that no longer exist: `AbiArg` (`register.rs:20`, `abi.rs:7`), `HostFn` (`register.rs:14,16`), @@ -572,12 +608,46 @@ useful for comparison and for the gas assertions in `Wasm_test.cpp` — not gosp `HostFuncImpl_test.cpp`). - VCS is **jj** (`jj st`, `jj log`), not raw git, for local work. -## Current state (2026-07-29) +## Current state (2026-07-30) **`crates/` compiles**, and the whole workspace is green — `cargo test --workspace`, -`clippy --workspace --all-targets`, `fmt`. 111 tests: 33 macro, 9 facade, 1 doctest, and -**68 in `xrpl-wasm-vm`** (8 unit; 60 integration — 12 `host_calls`, 19 `memory_policy`, -12 `budgets`, 17 `vm_limits`). +`clippy --workspace --all-targets`, `fmt`. 114 tests: 33 macro, 9 facade, 1 doctest, and +**71 in `xrpl-wasm-vm`** (9 unit; 62 integration — 12 `host_calls`, 19 `memory_policy`, +12 `budgets`, 19 `vm_limits`). + +**Findings A1, A2, A3 and B6, B7 are done** (2026-07-30). `run` is +`Result` over a typed `RunError`; host-fatal errors trap +instead of answering the guest a code; the import module is `host_lib`; the `i64` +pipeline and `AbiRet` are gone. See those entries for what landed and why. Two +decisions were taken to get there and are recorded at their findings: +**`OutOfTransferLimit` stays soft** (A1) and **the module name is `host_lib`** (A3). + +The two tests that existed only to pin behaviour A1 changed are gone, replaced by +tests of the new behaviour (`a_host_call_refused_its_gas_stops_the_run`, +`an_endless_loop_is_stopped_by_gas`). `the_wire_conversion_truncates` went with the +cast it pinned. What the rewrite turned up that reading the code did not: + +- **An endless guest loop and a refused host charge are the same outcome**, and both + report the whole limit as spent — the loop because wasmi's meter reaches zero, the + refused charge because `charge` spends what is left before it fails, which is what + makes C++'s "reported cost is the full limit" fall out rather than be arranged. +- **A start section is guest code, so the stage is not the reason.** Gas exhausted + during `instantiate_and_start` first reported `Instantiate`, hiding a + `tecOUT_OF_GAS`, because every error from that call was named after the stage. + `guest_halted` runs at both stages now, and + `a_start_section_that_exhausts_gas_is_out_of_gas_not_an_instantiation_failure` + pins it. `as_trap_code()` is what makes this work at all: it reports + `TrapCode::OutOfFuel` for whichever of wasmi's several error kinds carried the + exhaustion (`error.rs:236-252`). +- **The compile-time guarantee on the fatal set is narrower than it looks.** + `vm::host_fatal` is exhaustive over `HostError`, so a variant *added to the ABI* + cannot compile until it is placed. But moving an *existing* variant into + `abi::is_fatal`'s set is not caught: it falls into the grouped soft arm and + reports `Internal`. The two lists are read together, and the doc comment says so. +- `NoMemExported` being fatal makes C10 (resolve the `"memory"` export once at + instantiation) a move rather than a behaviour change — its failure is already a + run-ender, so hoisting it to an instantiation-time `RunError::NoMemory` only + changes which stage reports it. **How the suite was checked.** A code review of the diff mutation-tested it, and the result is worth recording because it found a test that pinned nothing: the multi-value @@ -647,10 +717,9 @@ split the VM restated all five values, so a legitimate gas change meant editing files. (Corollary: `every_variant_appears_in_all_exactly_once` is now subsumed by the table comparison and could go.) -Two tests **pin behaviour a finding says should change**, and say so in their names and -doc comments: `out_of_gas_in_a_host_call_currently_reaches_the_guest_as_a_code` (A1) and -`reads_currently_spend_the_transfer_budget_too` (A4). They are meant to be rewritten -when those decisions land, not to be preserved. +One test still **pins behaviour a finding says should change**, and says so in its name +and doc comment: `reads_currently_spend_the_transfer_budget_too` (A4). It is meant to be +rewritten when that decision lands, not preserved. Its A1 counterpart already was. The trait is settled, and every part of it is written in the declaration rather than synthesized: `&self`, `HostResult`, and byte outputs as explicit @@ -664,12 +733,21 @@ Consequences worth remembering: - The ABI crate is now guest-linkable (`no_std`, no allocator, no runtime deps, checks for `wasm32-unknown-unknown`) — see "The ABI crate is a library both sides link". -Next, in rough order, from the findings above: the two-channel error decision (A1 + A2, -which reshape `abi.rs`'s return type and `run`'s signature, so they go before any -cosmetic work there), then the B and D cleanups as one pass, then the cached `Memory` -(C10). The scratch-buffer decision (C11) and real `ApplyContext` wiring plus the cxx -bridge (`xrpl-wasm-vm-ffi` is still `mod ffi {}`) follow. Deferred as before: -macro-emitted `link_*` shims, the generated C header, the probe-module test. +Next, in rough order, from the findings above: **A4** — the transfer budget charged for +bytes never copied and charged before validation, plus `write_into`'s +`min(cap, MAX_FIELD_BYTES)` clamp. It is the last correctness item, and `is_fatal` +answers the question it used to raise: a mis-charge cannot end a run, only mis-report a +byte count, because `OutOfTransferLimit` is soft. Then the remaining B and D cleanups as +one pass (B8's unused `cxx` dep, B9's stale links, D14's `forbid(unsafe_code)`, D16's +three papercuts — `get_fuel().unwrap_or(0)` now appears once, in `vm::fuel_used`), then +the cached `Memory` (C10). The scratch-buffer decision (C11) and real `ApplyContext` +wiring plus the cxx bridge (`xrpl-wasm-vm-ffi` is still `mod ffi {}`) follow. Deferred as +before: macro-emitted `link_*` shims, the generated C header, the probe-module test. + +`register_host_functions` still returns `Result<(), String>` and `run` now discards that +string (a linker failure is `RunError::Internal`, which carries nothing), so its +`format!` is dead. `Result<(), wasmi::errors::LinkerError>` is the honest signature — +small enough to fold into the B/D pass. Deferred to a later refactor, once there is working code: macro-emitted `link_*` shims, the generated C header, and the probe-module conformance test.