From 25afc04420bb704fd86c9648340d31ad91081b9d Mon Sep 17 00:00:00 2001 From: Sergey Kuznetsov Date: Thu, 30 Jul 2026 16:06:00 +0100 Subject: [PATCH] More fixes --- crates/Cargo.lock | 1 - crates/xrpl-host-functions-macros/src/lib.rs | 20 +++ crates/xrpl-host-functions/src/lib.rs | 85 ++++++----- .../xrpl-host-functions/tests/host_errors.rs | 70 +++++++++ crates/xrpl-wasm-vm/Cargo.toml | 1 - crates/xrpl-wasm-vm/src/abi.rs | 69 +++++---- crates/xrpl-wasm-vm/src/lib.rs | 18 +++ crates/xrpl-wasm-vm/src/register.rs | 27 ++-- crates/xrpl-wasm-vm/src/vm.rs | 140 ++++++++++++++---- crates/xrpl-wasm-vm/tests/vm_limits.rs | 44 +++++- docs/claude/redesign_impl.md | 129 ++++++++++++---- 11 files changed, 449 insertions(+), 155 deletions(-) create mode 100644 crates/xrpl-host-functions/tests/host_errors.rs diff --git a/crates/Cargo.lock b/crates/Cargo.lock index ae8ccef07c..17db72ea3a 100644 --- a/crates/Cargo.lock +++ b/crates/Cargo.lock @@ -481,7 +481,6 @@ dependencies = [ name = "xrpl-wasm-vm" version = "0.1.0" dependencies = [ - "cxx", "wasmi", "wat", "xrpl-host-functions", diff --git a/crates/xrpl-host-functions-macros/src/lib.rs b/crates/xrpl-host-functions-macros/src/lib.rs index 3f3d9c1828..80761f9420 100644 --- a/crates/xrpl-host-functions-macros/src/lib.rs +++ b/crates/xrpl-host-functions-macros/src/lib.rs @@ -136,6 +136,26 @@ fn generate(functions: &[ParsedHostFunction]) -> TokenStream { /// Each method is one declaration from the `host_functions!` block, as /// written; its `&self` receiver is not part of the ABI the guest sees, /// so a host that must mutate does so behind interior mutability. + /// + /// # The output contract + /// + /// A method handed an `out` buffer **writes into it only when the whole + /// value fits, and returns the value's true length whether it fitted or + /// not.** + /// + /// The length is the value's, not the number of bytes written, because it + /// is how a guest that asked with too small a buffer learns the size to + /// ask for next time. The engine turns a length past the buffer into + /// `BufferTooSmall`, and one past the field cap into `DataFieldTooLarge`, + /// so a host needs to know neither. + /// + /// Writing nothing unless the value fits is the half only a host can hold + /// up. An engine can bound how many bytes are *writable* — and does, by + /// handing over a region clamped to the field cap — but it cannot take + /// back what a method already put there. A host that wrote a truncated + /// prefix and then reported the larger length would leave those bytes in + /// guest memory behind a refusal the guest is told to ignore. C++'s + /// `setData` is the reference point: it wrote only on a value that fit. pub trait HostFunctions { #(#trait_methods)* } diff --git a/crates/xrpl-host-functions/src/lib.rs b/crates/xrpl-host-functions/src/lib.rs index 5113381d76..c2082a9d0f 100644 --- a/crates/xrpl-host-functions/src/lib.rs +++ b/crates/xrpl-host-functions/src/lib.rs @@ -14,16 +14,53 @@ // Not re-exported: the ABI is declared once, here, and this is the only call site. use xrpl_host_functions_macros::host_functions; -/// Error codes a host function may return. +/// Declares [`HostError`] from one list: the variants, [`HostError::ALL`] and +/// [`HostError::from_code`]'s table all expand from the codes below. /// -/// The discriminants mirror `HostFunctionError` in -/// `include/xrpl/tx/wasm/WasmCommon.h`, so a negative `i32` crossing the wasm -/// boundary means the same thing to the guest, the Rust host, and the existing -/// C++ code. The full set is kept (not just the ones the PoC uses today) to -/// preserve that shared meaning. -#[derive(Debug, Clone, Copy, PartialEq, Eq)] -#[repr(i32)] -pub enum HostError { +/// One list is what makes `ALL` complete. Rust cannot enumerate an enum's +/// variants — an exhaustive `match` forces an arm per variant but gives nothing to +/// iterate — so a hand-written `ALL` beside a hand-written enum could only be kept +/// in step by review, and `ALL`'s whole purpose is to be the set a test can trust. +/// A code added below gains its `ALL` entry and its `from_code` arm by +/// construction. `HostFunctionSpec::ALL` is complete the same way, from the +/// `host_functions!` block. +macro_rules! host_errors { + ($($variant:ident = $code:literal,)+) => { + /// Error codes a host function may return. + /// + /// The discriminants mirror `HostFunctionError` in + /// `include/xrpl/tx/wasm/WasmCommon.h`, so a negative `i32` crossing the wasm + /// boundary means the same thing to the guest, the Rust host, and the existing + /// C++ code. The full set is kept (not just the ones the PoC uses today) to + /// preserve that shared meaning. + #[derive(Debug, Clone, Copy, PartialEq, Eq)] + #[repr(i32)] + pub enum HostError { + $($variant = $code,)+ + } + + impl HostError { + /// Every error a host function may return, in code order. + /// + /// The complete set, and complete by construction: a wasm engine's + /// split between the codes it hands the guest and the conditions it + /// traps on is a decision per variant, so the test that checks the + /// split iterates this and a code added to the ABI cannot slip past it. + pub const ALL: &'static [HostError] = &[$(HostError::$variant,)+]; + + /// Reconstruct a `HostError` from its wire code; unknown/positive values + /// map to `Internal`. + pub const fn from_code(code: i32) -> HostError { + match code { + $($code => HostError::$variant,)+ + _ => HostError::Internal, + } + } + } + }; +} + +host_errors! { Internal = -1, FieldNotFound = -2, BufferTooSmall = -3, @@ -55,36 +92,6 @@ impl HostError { pub const fn code(self) -> i32 { self as i32 } - - /// Reconstruct a `HostError` from its wire code; unknown/positive values map to `Internal`. - pub const fn from_code(code: i32) -> HostError { - match code { - -1 => HostError::Internal, - -2 => HostError::FieldNotFound, - -3 => HostError::BufferTooSmall, - -4 => HostError::NoArray, - -5 => HostError::NotLeafField, - -6 => HostError::LocatorMalformed, - -7 => HostError::SlotOutRange, - -8 => HostError::SlotsFull, - -9 => HostError::EmptySlot, - -10 => HostError::LedgerObjNotFound, - -11 => HostError::Decoding, - -12 => HostError::DataFieldTooLarge, - -13 => HostError::PointerOutOfBounds, - -14 => HostError::NoMemExported, - -15 => HostError::InvalidParams, - -16 => HostError::InvalidAccount, - -17 => HostError::InvalidField, - -18 => HostError::IndexOutOfBounds, - -19 => HostError::FloatInputMalformed, - -20 => HostError::FloatComputationError, - -21 => HostError::NoRuntime, - -22 => HostError::OutOfGas, - -23 => HostError::OutOfTransferLimit, - _ => HostError::Internal, - } - } } /// Convenience alias for the trait's fallible returns. diff --git a/crates/xrpl-host-functions/tests/host_errors.rs b/crates/xrpl-host-functions/tests/host_errors.rs new file mode 100644 index 0000000000..7815f5c1a6 --- /dev/null +++ b/crates/xrpl-host-functions/tests/host_errors.rs @@ -0,0 +1,70 @@ +//! Exercises what `host_errors!` generates: the wire codes, the set +//! [`HostError::ALL`] names, and the round trip between them. +//! +//! The codes are consensus input — they are what a guest reads off a failed host +//! call — so they are pinned here as literals and derived everywhere else. + +use xrpl_host_functions::HostError; + +/// The whole set, written out in the order `ALL` gives it: the one place the wire +/// codes appear as literals, and a deliberate change-detector, since a code that +/// moves changes what every deployed guest is told. +#[test] +fn the_error_table_matches_the_declarations() { + let table: Vec<(HostError, i32)> = HostError::ALL + .iter() + .map(|&error| (error, error.code())) + .collect(); + + assert_eq!( + table, + [ + (HostError::Internal, -1), + (HostError::FieldNotFound, -2), + (HostError::BufferTooSmall, -3), + (HostError::NoArray, -4), + (HostError::NotLeafField, -5), + (HostError::LocatorMalformed, -6), + (HostError::SlotOutRange, -7), + (HostError::SlotsFull, -8), + (HostError::EmptySlot, -9), + (HostError::LedgerObjNotFound, -10), + (HostError::Decoding, -11), + (HostError::DataFieldTooLarge, -12), + (HostError::PointerOutOfBounds, -13), + (HostError::NoMemExported, -14), + (HostError::InvalidParams, -15), + (HostError::InvalidAccount, -16), + (HostError::InvalidField, -17), + (HostError::IndexOutOfBounds, -18), + (HostError::FloatInputMalformed, -19), + (HostError::FloatComputationError, -20), + (HostError::NoRuntime, -21), + (HostError::OutOfGas, -22), + (HostError::OutOfTransferLimit, -23), + ] + ); +} + +/// Every code a guest can be handed comes back as the error that produced it, so a +/// caller reading a negative return value recovers the condition and not a +/// neighbouring one. The table above pins the numbers; this adds only the round +/// trip. +#[test] +fn every_wire_code_round_trips_back_to_its_error() { + for &error in HostError::ALL { + assert_eq!(HostError::from_code(error.code()), error, "{error:?}"); + } +} + +/// A code from outside the set is `Internal`: a host that answers something this +/// ABI does not define has failed in a way the caller cannot act on, and success is +/// not an error at all. +#[test] +fn a_code_outside_the_set_is_internal() { + let unassigned = -(HostError::ALL.len() as i32) - 1; + + for code in [unassigned, i32::MIN, 0, 1, i32::MAX] { + assert_eq!(HostError::from_code(code), HostError::Internal, "{code}"); + } +} diff --git a/crates/xrpl-wasm-vm/Cargo.toml b/crates/xrpl-wasm-vm/Cargo.toml index fcc8e8f180..21a5a6f608 100644 --- a/crates/xrpl-wasm-vm/Cargo.toml +++ b/crates/xrpl-wasm-vm/Cargo.toml @@ -5,7 +5,6 @@ edition.workspace = true [dependencies] wasmi = { version = "1.1.0", default-features = false, features = ["std"] } -cxx.workspace = true xrpl-host-functions = { path = "../xrpl-host-functions" } [dev-dependencies] diff --git a/crates/xrpl-wasm-vm/src/abi.rs b/crates/xrpl-wasm-vm/src/abi.rs index 350019b729..7db1cdb2bb 100644 --- a/crates/xrpl-wasm-vm/src/abi.rs +++ b/crates/xrpl-wasm-vm/src/abi.rs @@ -140,10 +140,11 @@ pub(crate) fn read_borrowed<'a>( ptr: i32, len: i32, ) -> HostResult<&'a [u8]> { - if ptr < 0 || len < 0 { + // A guest's pointer and length are `i32` on the wire and indices here, so the + // conversion is the validity check: it fails on exactly the negative values. + let (Ok(ptr), Ok(len)) = (usize::try_from(ptr), usize::try_from(len)) else { return Err(HostError::InvalidParams); - } - let (ptr, len) = (ptr as usize, len as usize); + }; if len > MAX_FIELD_BYTES { return Err(HostError::DataFieldTooLarge); } @@ -187,10 +188,10 @@ pub(crate) fn write_into( cap: i32, fill: impl FnOnce(&dyn HostFunctions, &mut [u8]) -> HostResult, ) -> HostResult { - if dst < 0 || cap < 0 { + // As in `read_borrowed`: the conversion to an index is the validity check. + let (Ok(dst), Ok(cap)) = (usize::try_from(dst), usize::try_from(cap)) else { return Err(HostError::InvalidParams); - } - let (dst, cap) = (dst as usize, cap as usize); + }; let mem = memory(caller)?; // Copy the shared `&dyn HostFunctions` out of the store data (references are // Copy) so the data borrow ends before we borrow guest memory mutably. @@ -222,7 +223,13 @@ pub(crate) fn write_into( charge_transfer(caller.data(), n)?; // 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) + #[expect( + clippy::cast_possible_truncation, + clippy::cast_possible_wrap, + reason = "`n > MAX_FIELD_BYTES` returned above, and the cap is far inside i32" + )] + let n = n as i32; + Ok(n) } // The input buffer in `read_write` lives on the stack, sized to the field cap. @@ -250,10 +257,10 @@ pub(crate) fn read_write( cap: i32, call: impl FnOnce(&dyn HostFunctions, &[u8], &mut [u8]) -> HostResult, ) -> HostResult { - if src < 0 || src_len < 0 { + // As in `read_borrowed`: the conversion to an index is the validity check. + let (Ok(src), Ok(len)) = (usize::try_from(src), usize::try_from(src_len)) else { return Err(HostError::InvalidParams); - } - let len = src_len as usize; + }; if len > MAX_FIELD_BYTES { return Err(HostError::DataFieldTooLarge); } @@ -261,7 +268,7 @@ pub(crate) fn read_write( // Copy the input out before `write_into` borrows guest memory mutably. let mut buf = [0u8; MAX_FIELD_BYTES]; memory(caller)? - .read(&*caller, src as usize, &mut buf[..len]) + .read(&*caller, src, &mut buf[..len]) .map_err(|_| HostError::PointerOutOfBounds)?; let input = &buf[..len]; @@ -331,9 +338,12 @@ mod tests { 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] = [ + /// The three conditions the host cannot serve a call under, as the tests + /// *expect* them rather than as [`is_fatal`] reports them — deriving this from + /// `is_fatal` would make both tests below vacuous, since a condition wrongly + /// classified as soft would simply be skipped. Named once, so the two are one + /// statement about the same set. + const MUST_TRAP: [HostError; 3] = [ HostError::OutOfGas, HostError::Internal, HostError::NoMemExported, @@ -343,7 +353,7 @@ mod tests { /// outcome without parsing a message. #[test] fn a_host_fatal_error_becomes_a_trap_carrying_it() { - for error in FATAL { + for error in MUST_TRAP { 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(|| { @@ -354,7 +364,11 @@ mod tests { } /// 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. + /// three in [`MUST_TRAP`] trap, and everything else is a code the guest acts on. + /// + /// Over `HostError::ALL`, so it is the whole ABI and not a sample: a code + /// added to the ABI arrives here already asserted to be guest-visible, and + /// making it fatal is then a change someone has to come and make. /// /// `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 @@ -362,22 +376,13 @@ mod tests { /// 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()); + for &error in HostError::ALL { + if MUST_TRAP.contains(&error) { + assert!(is_fatal(error), "{error:?} must stop the run"); + } else { + assert!(!is_fatal(error), "{error:?} must reach the guest as a code"); + assert_eq!(wire(Err(error)), error.code()); + } } } diff --git a/crates/xrpl-wasm-vm/src/lib.rs b/crates/xrpl-wasm-vm/src/lib.rs index 35bdbac236..fbe31c9660 100644 --- a/crates/xrpl-wasm-vm/src/lib.rs +++ b/crates/xrpl-wasm-vm/src/lib.rs @@ -1,3 +1,21 @@ +//! The escrow wasm VM: compile a contract, meter it, and serve its host calls. +//! +//! Every guest access goes through `abi.rs`, which reaches linear memory only by +//! wasmi's bounds-checked slice operations — `forbid(unsafe_code)` is what makes +//! that a property of the crate rather than a claim in a comment. The cast lints +//! are on for the same reason: a truncating or sign-losing cast on a consensus +//! path changes what a contract is charged or told, so each one has to be argued +//! for at its site. +#![forbid(unsafe_code)] +#![deny(rustdoc::broken_intra_doc_links)] +#![deny(unreachable_pub)] +#![deny( + clippy::cast_possible_truncation, + clippy::cast_possible_wrap, + clippy::cast_sign_loss, + clippy::cast_lossless +)] + mod abi; mod register; mod vm; diff --git a/crates/xrpl-wasm-vm/src/register.rs b/crates/xrpl-wasm-vm/src/register.rs index b96d3ebddc..b4066cd073 100644 --- a/crates/xrpl-wasm-vm/src/register.rs +++ b/crates/xrpl-wasm-vm/src/register.rs @@ -12,19 +12,19 @@ const HOST_MODULE: &str = "host_lib"; // Import registration // --------------------------------------------------------------------------- -/// Register the PoC's host functions on `linker`, one per [`HostFn`] variant. +/// Register the PoC's host functions on `linker`, one per [`HostFunctionSpec`] +/// variant. /// -/// 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 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}") - } - +/// Driven by an exhaustive `match` over [`HostFunctionSpec::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 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 [`xrpl_host_functions::HostFunctions`] trait object held in +/// the [`wasmi::Store`]. +pub(crate) fn register_host_functions( + linker: &mut Linker>, +) -> Result<(), wasmi::errors::LinkerError> { // TODO: think on how to make it better for &op in HostFunctionSpec::ALL { match op { @@ -130,8 +130,7 @@ pub(crate) fn register_host_functions(linker: &mut Linker>) -> Resul }) }, ), - } - .map_err(link_err)?; + }?; } Ok(()) } diff --git a/crates/xrpl-wasm-vm/src/vm.rs b/crates/xrpl-wasm-vm/src/vm.rs index 57fc520cfe..7abdc12b63 100644 --- a/crates/xrpl-wasm-vm/src/vm.rs +++ b/crates/xrpl-wasm-vm/src/vm.rs @@ -1,7 +1,9 @@ use std::cell::Cell; use std::fmt; use std::sync::LazyLock; -use wasmi::{Config, Engine, Linker, Module, Store, StoreLimits, StoreLimitsBuilder, TrapCode}; +use wasmi::{ + Config, Engine, Extern, Linker, Module, Store, StoreLimits, StoreLimitsBuilder, TrapCode, +}; use xrpl_host_functions::{HostError, HostFunctions}; use crate::abi::FatalHostError; @@ -29,7 +31,7 @@ pub const TRANSFER_LIMIT_BYTES: u64 = 1 << 20; pub const MAX_FIELD_BYTES: usize = 1024; /// State threaded through every host call, stored in the wasmi [`Store`]. -pub struct VmState<'h> { +pub(crate) struct VmState<'h> { pub(crate) host: &'h dyn HostFunctions, /// Enforces [`MAX_MEMORY_BYTES`] via `Store::limiter`. It lives here because /// the limiter callback wasmi holds has to produce a `&mut` into it from @@ -68,7 +70,8 @@ pub enum RunError { /// 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`. + /// No export named `function_name` with signature `() -> i32`: absent, not a + /// function, or a function of another type — which the detail tells apart. EntryPoint(String), /// Gas exhausted — by the guest's own instructions or by a host call's /// charge. [`RunFailure::fuel_used`] is the whole limit. @@ -87,7 +90,9 @@ impl fmt::Display for RunError { 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}"), + // The detail says which of the entry point's failures this is, since + // "no entry point" would be wrong for an export of the wrong type. + RunError::EntryPoint(detail) => write!(f, "{detail}"), RunError::OutOfGas => write!(f, "out of gas"), RunError::Internal => write!(f, "internal error"), RunError::NoMemory => write!(f, "no exported memory"), @@ -113,8 +118,9 @@ impl fmt::Display for RunFailure { } 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. + /// A failure that costs nothing: it stopped the run at or before the point the + /// guest first gets to execute, or it stopped it under a store with no meter to + /// read, which comes to the same thing — no fuel was accounted either way. fn owing_nothing(error: RunError) -> RunFailure { RunFailure { error, @@ -125,8 +131,32 @@ impl RunFailure { /// 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)) +/// +/// `Store::get_fuel` fails on exactly one condition — a store whose engine was +/// built without fuel metering — and that is a property of +/// [`build_wasm_engine`], which turns metering on, and one `run` has already +/// established for this store by the time anything is measured: its `set_fuel` +/// fails under the same condition and returns first. So a failure here is a defect +/// in this crate, and the one thing it must not become is a number: `0` would +/// forgive a run its whole cost and `gas` would charge an untouched one for +/// everything. It leaves as [`RunError::Internal`] instead, which is what the +/// caller maps a defect to. +fn fuel_used(store: &Store>, gas: u64) -> Result { + store + .get_fuel() + .map(|remaining| gas.saturating_sub(remaining)) + .map_err(|_| RunError::Internal) +} + +/// Report `error` with the run's cost attached, from the one point that reads it. +/// +/// A cost that cannot be read replaces the outcome rather than being invented, +/// because the cost is what the caller charges for — see [`fuel_used`]. +fn failed(store: &Store>, gas: u64, error: RunError) -> RunFailure { + match fuel_used(store, gas) { + Ok(fuel_used) => RunFailure { error, fuel_used }, + Err(unmetered) => RunFailure::owing_nothing(unmetered), + } } /// The outcome a `wasmi::Error` names for itself, if it names one, rather than @@ -151,11 +181,14 @@ fn guest_halted(error: &wasmi::Error) -> Option { /// 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. +/// added to the ABI has to be placed here before this compiles. That covers one +/// direction of the agreement with [`crate::abi::is_fatal`], which decides the +/// channel; the other — an existing variant moved into `is_fatal`'s set, which +/// would land in the soft arm here and report `Internal` instead of the condition +/// it was — is covered by `tests::every_fatal_error_has_an_outcome_of_its_own`. +/// +/// 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, @@ -189,7 +222,7 @@ fn host_fatal(error: HostError) -> RunError { /// The configuration is consensus-fixed and identical for every invocation, and /// an [`Engine`] is an internally `Arc`ed `Send + Sync` handle, so one shared /// engine serves concurrent [`run`] calls. -pub fn wasm_engine() -> &'static Engine { +pub(crate) fn wasm_engine() -> &'static Engine { static ENGINE: LazyLock = LazyLock::new(build_wasm_engine); &ENGINE } @@ -261,41 +294,55 @@ pub fn run<'h>( 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 error = guest_halted(&e).unwrap_or_else(|| RunError::Instantiate(e.to_string())); + return Err(failed(&store, gas, error)); } }; 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 error = RunError::EntryPoint(entry_point_detail( + instance.get_export(&store, function_name), + function_name, + &e, + )); + return Err(failed(&store, gas, error)); } }; 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 error = guest_halted(&e).unwrap_or_else(|| RunError::Trap(e.to_string())); + return Err(failed(&store, gas, error)); } }; - Ok(RunOutcome { - result, - fuel_used: fuel_used(&store, gas), - }) + let fuel_used = fuel_used(&store, gas).map_err(RunFailure::owing_nothing)?; + Ok(RunOutcome { result, fuel_used }) +} + +/// Why `get_typed_func` would not hand over the entry point, told apart by what +/// the module exports under that name. +/// +/// wasmi answers all three cases with one error, so the message would otherwise +/// read "no entry point" for a contract that exports the name with the wrong +/// signature — a diagnostic that sends the author looking for a missing export +/// they already have. `export` is what [`wasmi::Instance::get_export`] found. +fn entry_point_detail(export: Option, name: &str, error: &wasmi::Error) -> String { + match export { + Some(Extern::Func(_)) => { + format!("entry point '{name}' has the wrong signature, expected '() -> i32': {error}") + } + Some(_) => format!("export '{name}' is not a function: {error}"), + None => format!("no entry point '{name}': {error}"), + } } #[cfg(test)] mod tests { use super::*; + use crate::abi::is_fatal; /// The engine is built once and shared, so two invocations must not compile /// their modules against different engines. @@ -304,6 +351,39 @@ mod tests { assert!(Engine::same(wasm_engine(), wasm_engine())); } + /// The two lists that decide a host error's fate must name the same set. + /// [`is_fatal`] picks the channel; [`host_fatal`] names the outcome of the + /// fatal one. Only one direction of that agreement is compiler-enforced — a + /// *new* variant fails to compile until `host_fatal` places it — so a variant + /// moved into `is_fatal`'s set would trap and then be reported as `Internal`, + /// losing the condition it was. This is the other direction. + /// + /// The soft arm's answer is `Internal`, which `HostError::Internal` also + /// answers, so "named in its own right" is the outcome being anything else, + /// with `Internal` itself asked about by name. + #[test] + fn every_fatal_error_has_an_outcome_of_its_own() { + for &error in HostError::ALL { + let named = !matches!(host_fatal(error), RunError::Internal) + || matches!(error, HostError::Internal); + assert_eq!( + is_fatal(error), + named, + "{error:?}: abi::is_fatal {}, host_fatal {}", + if is_fatal(error) { + "traps it" + } else { + "passes it to the guest" + }, + if named { + "names its outcome" + } else { + "groups it with the soft errors" + } + ); + } + } + /// The four protocol limits against the C++ values they mirror: a deliberate /// change-detector, and the only place these numbers appear as literals — /// every other test derives from the constants. The C++ names make the parity diff --git a/crates/xrpl-wasm-vm/tests/vm_limits.rs b/crates/xrpl-wasm-vm/tests/vm_limits.rs index b64c0945c4..c1a973c37f 100644 --- a/crates/xrpl-wasm-vm/tests/vm_limits.rs +++ b/crates/xrpl-wasm-vm/tests/vm_limits.rs @@ -449,9 +449,10 @@ fn the_entry_point_is_the_name_the_caller_gives() { assert_eq!(outcome.result, 9); } -/// The entry point must take nothing and return an `i32`. A wrongly-typed export is -/// reported as a missing entry point, which reads as though it were absent — -/// finding D16 in `docs/claude/redesign_impl.md`. +/// The entry point must take nothing and return an `i32`. A module that exports the +/// name with another signature is told so, rather than being told the export is +/// missing: wasmi answers both cases with one error, and "no entry point" would send +/// a contract author looking for a function they already have. #[test] fn an_entry_point_of_the_wrong_type_fails() { let host = FakeHost::new(); @@ -467,13 +468,42 @@ fn an_entry_point_of_the_wrong_type_fails() { let wat = format!( 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") - .to_string(); - assert!(failure.contains("no entry point"), "{signature}: {failure}"); + let failure = assert_stage!( + run_with_gas(&wat, PLENTY_OF_GAS, &host) + .expect_err("a wrongly-typed entry point must not run"), + RunError::EntryPoint(_) + ) + .to_string(); + assert!( + failure.contains("entry point 'finish' has the wrong signature"), + "{signature}: {failure}" + ); + assert!( + !failure.contains("no entry point"), + "a present export must not be reported as absent — {signature}: {failure}" + ); } } +/// An export of the entry point's name that is not a function at all is a third +/// case, and named as such: nothing is missing and no signature is wrong. +#[test] +fn an_entry_point_that_is_not_a_function_fails() { + let host = FakeHost::new(); + + let wat = + r#"(module (memory (export "memory") 1) (global (export "finish") i32 (i32.const 0)))"#; + let failure = assert_stage!( + run_with_gas(wat, PLENTY_OF_GAS, &host).expect_err("a non-function export must not run"), + RunError::EntryPoint(_) + ) + .to_string(); + assert!( + failure.contains("export 'finish' is not a function"), + "{failure}" + ); +} + /// A guest that traps fails the run rather than returning a value. #[test] fn a_trapping_guest_fails_the_run() { diff --git a/docs/claude/redesign_impl.md b/docs/claude/redesign_impl.md index 82a5c60463..b88faeb557 100644 --- a/docs/claude/redesign_impl.md +++ b/docs/claude/redesign_impl.md @@ -519,8 +519,8 @@ matter. Items marked ✓ are done. 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: +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`), `run_escrow` (`vm.rs:19,64`). And `abi.rs:147-150` / `vm.rs:71` are historical comments ("used to pay", "The `CxxHost` path additionally used to marshal … that @@ -528,6 +528,13 @@ matter. Items marked ✓ are done. no-historical-comments convention. `#![deny(rustdoc::broken_intra_doc_links)]` stops the links from rotting again. + The links are fixed and the `deny` is in. **The historical comments were already + gone** — the A1–A4 slices rewrote those lines. A sweep for `used to` / `no longer` / + `formerly` / `originally` / `unchanged from` / `previously` across `crates/` found + nothing but present-tense prose and C++ reference points, which the convention + allows. The one stale comment left was in `vm_limits.rs`, citing D16 as an open bug; + it went with D16. + ### C. Performance 10. **The `"memory"` export is a string hash lookup on every host call.** `memory()` @@ -557,15 +564,46 @@ matter. Items marked ✓ are done. `1024`/`1025` as literals twenty-one times. Now `vm::MAX_FIELD_BYTES`, beside the others — **renamed**, so a search for the old name (or for C++'s `kMaxWasmDataLength`, which its doc comment still cites) lands here. -14. `#![forbid(unsafe_code)]` — `abi.rs:64` *claims* every access is a checked wasmi +14. ✓ `#![forbid(unsafe_code)]` — `abi.rs:64` *claims* every access is a checked wasmi slice op; let the compiler enforce the claim. Plus `unreachable_pub` and clippy's cast lints. + + All three are on, and the two warning lints each paid for themselves. + `unreachable_pub` found `VmState` and `wasm_engine`: `pub` inside a private module + and never re-exported, so unreachable from outside the crate — now `pub(crate)`, + with nothing silenced. The cast lints found **8 sites, all in `abi.rs`**. Six were + `cast_sign_loss` on the guest's `i32` pointers and lengths, and the fix removed + code rather than adding it: `let (Ok(ptr), Ok(len)) = (usize::try_from(ptr), + usize::try_from(len)) else { … }` — **the conversion is the negativity check**, so + the separate `ptr < 0 || len < 0` guards are gone rather than duplicated. The + remaining two are the one `n as i32` in `write_into`, bounded by the + `MAX_FIELD_BYTES` return directly above it, under a scoped `#[expect]` — `expect` + rather than `allow`, so it fires if a later restructure makes it unnecessary. 15. ✓ **Zero tests.** Nothing checked the bounds/cap/transfer/gas policy, and every item above edits exactly that policy. Closed first, for that reason. 16. Minor: `gas = 0` is accepted silently (C++ rejected it as `temBAD_AMOUNT`); `store.get_fuel().unwrap_or(0)` (`vm.rs:137`) swallows an error into a plausible-looking number; `get_typed_func` failure reports "no entry point" when the export exists with the wrong signature. + + **Two of the three are done.** `fuel_used` returns `Result`, folded + through a `failed(store, gas, error)` helper so all four report sites read the + meter in one place. Worth recording *why* it is not a document-and-assert: the + `unwrap_or(0)` did not merely swallow an error, it reported `gas - 0`, **the whole + limit** — an untouched contract charged for everything. No fallback is defensible + (`0` forgives the run, `gas` overcharges), so a cost that cannot be read replaces + the outcome with `Internal` rather than being invented. No panic on a consensus + path. And the entry-point diagnostic is now three cases, told apart by + `Instance::get_export`: no such export, an export of the wrong signature, and an + export that is not a function at all — the last two used to claim "no entry point" + about an export that was right there. `RunError::EntryPoint`'s `Display` carries the + detail bare for that reason, the one variant without a `stage:` prefix. + + **Still open, and now a decision rather than a bug:** `gas = 0` no longer passes + silently — it fails with a typed `OutOfGas`. Whether the caller should instead + reject it up front as C++'s `temBAD_AMOUNT` is a TER question, so it belongs with + the cxx bridge, where the mapping gets written. (`gas` is `u64`, so C++'s negative + case cannot arise.) 17. The start-section TODO (`vm.rs:90`) **cannot** be closed with wasmi 1.1's public API: there is no `InstancePre`/`ensure_no_start`, and `ModuleHeader::start` is private, so only a byte-level section scan would do it. But `set_fuel` and @@ -637,11 +675,14 @@ useful for comparison and for the gas assertions in `Wasm_test.cpp` — not gosp ## Current state (2026-07-30) **`crates/` compiles**, and the whole workspace is green — `cargo test --workspace`, -`clippy --workspace --all-targets`, `fmt`. 115 tests: 33 macro, 9 facade, 1 doctest, and -**72 in `xrpl-wasm-vm`** (9 unit; 63 integration — 12 `host_calls`, 19 `memory_policy`, -13 `budgets`, 19 `vm_limits`). +`clippy --workspace --all-targets`, `fmt`, and `cargo doc -p xrpl-wasm-vm --no-deps` +(which `deny(rustdoc::broken_intra_doc_links)` now makes load-bearing). 120 tests: 33 +macro, 12 facade, 1 doctest, and **74 in `xrpl-wasm-vm`** (10 unit; 64 integration — 12 +`host_calls`, 19 `memory_policy`, 13 `budgets`, 20 `vm_limits`). -**Section A is closed, and B6/B7 with it** (2026-07-30). `run` is +**Section A is closed, B6/B7 with it, and the B/D cleanup after that** (2026-07-30) — +B8, B9, D14 and two thirds of D16. Only C10/C11/C12, D17 and D16's `gas = 0` decision +are left of the seventeen. `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; the transfer budget counts only bytes actually copied @@ -771,32 +812,58 @@ 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 remaining B and D cleanups as one -pass** — B8's unused `cxx` dep, B9's stale links and historical comments, D14's -`forbid(unsafe_code)` plus `unreachable_pub` and the cast lints, and D16's papercuts. -Then the cached `Memory` (C10), which `NoMemExported` being fatal has already made a -move rather than a behaviour change. The scratch-buffer decision (C11) and real -`ApplyContext` wiring plus the cxx bridge (`xrpl-wasm-vm-ffi` is still `mod ffi {}`) -follow. C12 (per-run `Linker` and no module cache) stays last: `VmState<'h>`'s lifetime -is the blocker. Deferred as before: macro-emitted `link_*` shims, the generated C -header, the probe-module test. +Next, from the findings above, only section C is left. **The cached `Memory` (C10)** +first: it is the one item with a measurable payoff, the benchmark can show it, and +`NoMemExported` being fatal has already made it a move rather than a behaviour change — +only `assert_no_memory`'s expected stage shifts from a trap to instantiation. Then +**C11**, which needs the scratch-buffer decision made before it is codeable, and C10 +makes either answer easier. **C12** (per-run `Linker`, no module cache) stays last: +`VmState<'h>`'s lifetime forces `Linker>` to be per-run, so it is a design +change rather than a tweak, and the cxx bridge will force that lifetime question anyway. -Four small things belong in that B/D pass, each found by a slice rather than by the -original read: +The real remaining work is not in the findings list: **the cxx bridge** +(`xrpl-wasm-vm-ffi` is still `mod ffi {}`) and **real `ApplyContext` wiring**. A1 and A2 +were sequenced first so the bridge has a typed `RunError` and a `fuel_used` to marshal +instead of error text to parse. D16's `gas = 0` decision belongs there too, since it is +a TER choice. Deferred as before: macro-emitted `link_*` shims, the generated C header, +the probe-module test. -- `register_host_functions` returns `Result<(), String>` and `run` discards the string - (a linker failure is `RunError::Internal`, which carries nothing), so its `format!` - is dead. `Result<(), wasmi::errors::LinkerError>` is the honest signature. -- `only_the_host_fatal_errors_trap`'s soft list is representative, not exhaustive — - nothing in the ABI crate enumerates `HostError`. A `HostError::ALL` there would close - it, and this pins a consensus-relevant channel split, so it is worth closing. -- D16's `gas = 0` item has shifted from a bug to a decision: it no longer passes - silently but fails with a typed `OutOfGas`, so the question is whether it deserves - C++'s `temBAD_AMOUNT` at the caller instead. `gas` is `u64`, so C++'s negative case - cannot arise. -- The `HostFunctions` declaration should say that a host writes into `out` only when - the whole value fits — see A4's last paragraph, where that is what makes "a refused - value leaves nothing behind" hold end to end. +One incidental constraint found while checking the guest target: `crates/hello_world` +cannot be checked for `wasm32-unknown-unknown` — it depends on `cxx` → +`link-cplusplus`, which wants a C++ toolchain for the target. Pre-existing, but it means +the guest-linkability check has to name the ABI crate rather than being a blanket +workspace command. + +Four things the slices turned up rather than the original read went into that pass, and +one of them is worth more than its size: + +- `register_host_functions` now returns `Result<(), wasmi::errors::LinkerError>`; its + `format!` was dead once `run` began discarding the string. +- **`HostError::ALL` exists, and how it had to be built is the interesting part.** + `only_the_host_fatal_errors_trap` checked a hand-listed sample, so a variant added to + the ABI was not covered. The obvious fix — a wildcard-free `match`, the trick + `vm::host_fatal` uses — **cannot close this**, and the reason generalizes: an + exhaustive `match` forces you to *write an arm*, but checking "every variant is in + `ALL`" requires *enumerating* variants, and Rust has no stable way to do that + (`mem::variant_count` is unstable). Every const-assertion scheme over `ALL` is beaten + by "add the variant, give its arm a value, leave `ALL` + alone", because the assertion only ever iterates `ALL` — the very thing missing the + variant. So the airtight mechanism is a **single declaration site**: a `host_errors!` + macro emits the enum, `ALL` and `from_code` from one list of codes. + `HostFunctionSpec::ALL` is complete for exactly the same reason. It also retired a + hand-duplicated 23-arm `from_code` table that nothing tested; `tests/host_errors.rs` + now pins the 23 wire codes as literals, which is where a consensus-visible number + belongs. +- With `ALL` in hand, `every_fatal_error_has_an_outcome_of_its_own` closes the + `is_fatal`/`host_fatal` coupling gap the other direction — an existing variant moved + into `is_fatal` without `host_fatal` gaining an arm now fails a test instead of + silently reporting `Internal`. (Its wrinkle: `RunError::Internal` is both the soft + arm's answer and `HostError::Internal`'s own, so the test asks about that one by + name.) +- The generated `HostFunctions` trait now carries an output contract: a host writes into + `out` only when the whole value fits, and returns the value's true length either way. + That is what makes A4's "a refused value leaves nothing behind" hold end to end, + since `write_into` can only bound what is *writable*. Deferred to a later refactor, once there is working code: macro-emitted `link_*` shims, the generated C header, and the probe-module conformance test.