From 011235f9a79f61f5ed5a7b8936e2692a0edcbcdd Mon Sep 17 00:00:00 2001 From: Sergey Kuznetsov Date: Wed, 29 Jul 2026 16:26:03 +0100 Subject: [PATCH] Writing tests for vm --- crates/Cargo.lock | 9 +- .../tests/generated_abi.rs | 62 +-- crates/xrpl-wasm-vm/Cargo.toml | 5 +- crates/xrpl-wasm-vm/src/abi.rs | 239 ++++++---- crates/xrpl-wasm-vm/src/lib.rs | 4 +- crates/xrpl-wasm-vm/src/vm.rs | 80 ++-- crates/xrpl-wasm-vm/tests/budgets.rs | 363 +++++++++++++++ crates/xrpl-wasm-vm/tests/host_calls.rs | 245 ++++++++++ crates/xrpl-wasm-vm/tests/memory_policy.rs | 422 +++++++++++++++++ crates/xrpl-wasm-vm/tests/support/mod.rs | 268 +++++++++++ crates/xrpl-wasm-vm/tests/vm_limits.rs | 423 ++++++++++++++++++ docs/claude/redesign_impl.md | 234 +++++++++- 12 files changed, 2207 insertions(+), 147 deletions(-) create mode 100644 crates/xrpl-wasm-vm/tests/budgets.rs create mode 100644 crates/xrpl-wasm-vm/tests/host_calls.rs create mode 100644 crates/xrpl-wasm-vm/tests/memory_policy.rs create mode 100644 crates/xrpl-wasm-vm/tests/support/mod.rs create mode 100644 crates/xrpl-wasm-vm/tests/vm_limits.rs diff --git a/crates/Cargo.lock b/crates/Cargo.lock index 20affc58f0..ae8ccef07c 100644 --- a/crates/Cargo.lock +++ b/crates/Cargo.lock @@ -236,6 +236,12 @@ version = "1.0.9" source = "registry+https://github.com/rust-lang/crates.io-index" checksum = "d68f2ec51b097e4c1a75b681a8bec621909b5e91f15bb7b840c4f2f7b01148b2" +[[package]] +name = "semver" +version = "1.0.28" +source = "registry+https://github.com/rust-lang/crates.io-index" +checksum = "8a7852d02fc848982e0c167ef163aaff9cd91dc640ba85e263cb1ce46fae51cd" + [[package]] name = "serde" version = "1.0.228" @@ -358,7 +364,6 @@ dependencies = [ "wasmi_core", "wasmi_ir", "wasmparser 0.239.0", - "wat", ] [[package]] @@ -406,6 +411,7 @@ checksum = "d5769a29f799fbab136aaf65b4fe5384cd7d93fe6fc9ba0dcb6c8382a1f16e27" dependencies = [ "bitflags", "indexmap", + "semver", ] [[package]] @@ -477,6 +483,7 @@ version = "0.1.0" dependencies = [ "cxx", "wasmi", + "wat", "xrpl-host-functions", ] diff --git a/crates/xrpl-host-functions/tests/generated_abi.rs b/crates/xrpl-host-functions/tests/generated_abi.rs index 31b3cd445a..a760a2b3af 100644 --- a/crates/xrpl-host-functions/tests/generated_abi.rs +++ b/crates/xrpl-host-functions/tests/generated_abi.rs @@ -2,6 +2,7 @@ //! the spec table agrees with the declarations in `src/lib.rs`. use std::cell::RefCell; +use std::collections::HashSet; use xrpl_host_functions::{HASH_LEN, HostError, HostFunctionSpec, HostFunctions, HostResult}; @@ -117,42 +118,53 @@ fn the_trait_is_callable_through_a_shared_trait_object() { assert_eq!(*fake.traced.borrow(), ["count=1"]); } +/// The whole table, written out: the one place the ABI's wire names and gas costs +/// appear as literals, and a deliberate change-detector, since both are consensus +/// input. Everything else reads `HostFunctionSpec::gas()` instead. +/// +/// `ALL` is in declaration order, so comparing the whole vec pins the order and the +/// membership too. #[test] fn the_spec_table_matches_the_declarations() { - assert_eq!(HostFunctionSpec::ALL.len(), 5); - assert_eq!(HostFunctionSpec::GetLedgerSqn.wasm_name(), "ldgr_index"); - assert_eq!(HostFunctionSpec::GetLedgerSqn.gas(), 60); - assert_eq!(HostFunctionSpec::Sha512Half.gas(), 2000); - assert_eq!( - HostFunctionSpec::GetCurrentLedgerObjField.wasm_name(), - "home_le_field" - ); -} - -/// `ALL` is what a wasm engine iterates to register imports, so it must be complete. -#[test] -fn every_variant_appears_in_all_exactly_once() { - let mut names: Vec<&str> = HostFunctionSpec::ALL + let table: Vec<(&str, u64)> = HostFunctionSpec::ALL .iter() - .map(|function| function.wasm_name()) + .map(|function| (function.wasm_name(), function.gas())) .collect(); - names.sort_unstable(); assert_eq!( - names, + table, [ - "home_le_field", - "ldgr_index", - "sha512_half", - "trace", - "trace_num" + ("ldgr_index", 60), + ("home_le_field", 70), + ("sha512_half", 2000), + ("trace", 500), + ("trace_num", 500), ] ); } -/// The generated `spec` is `const`, so gas costs are available at compile time. +/// `ALL` is what a wasm engine iterates to register imports, so no two declarations +/// may collapse to the same wire name. The table above pins membership and order; +/// this adds only uniqueness, and restates nothing. +#[test] +fn every_variant_appears_in_all_exactly_once() { + let names: HashSet<&str> = HostFunctionSpec::ALL + .iter() + .map(|function| function.wasm_name()) + .collect(); + + assert_eq!(names.len(), HostFunctionSpec::ALL.len()); +} + +/// Both accessors are `const`, so an engine can build its import and gas tables at +/// compile time rather than on every invocation. The assertions sit in `const` +/// blocks so they are checked while compiling, which is the claim; the values +/// themselves are pinned above. #[test] fn the_table_is_usable_in_const_context() { - const TRACE_GAS: u64 = HostFunctionSpec::Trace.gas(); - assert_eq!(TRACE_GAS, 500); + const NAME: &str = HostFunctionSpec::Trace.wasm_name(); + const GAS: u64 = HostFunctionSpec::Trace.gas(); + + const { assert!(!NAME.is_empty()) }; + const { assert!(GAS > 0) }; } diff --git a/crates/xrpl-wasm-vm/Cargo.toml b/crates/xrpl-wasm-vm/Cargo.toml index b6865b8c4f..fcc8e8f180 100644 --- a/crates/xrpl-wasm-vm/Cargo.toml +++ b/crates/xrpl-wasm-vm/Cargo.toml @@ -4,6 +4,9 @@ version = "0.1.0" edition.workspace = true [dependencies] -wasmi = "1.1.0" +wasmi = { version = "1.1.0", default-features = false, features = ["std"] } cxx.workspace = true xrpl-host-functions = { path = "../xrpl-host-functions" } + +[dev-dependencies] +wat = "1" diff --git a/crates/xrpl-wasm-vm/src/abi.rs b/crates/xrpl-wasm-vm/src/abi.rs index b49ee89138..39236203c6 100644 --- a/crates/xrpl-wasm-vm/src/abi.rs +++ b/crates/xrpl-wasm-vm/src/abi.rs @@ -1,22 +1,16 @@ -use crate::vm::VmState; +use crate::vm::{MAX_FIELD_BYTES, VmState}; use wasmi::{Caller, Extern, Memory}; use xrpl_host_functions::{HostError, HostFunctionSpec, HostFunctions, HostResult}; // --------------------------------------------------------------------------- -// ABI marshaling traits: decode a host-function argument from wasm scalars + -// guest memory (`AbiArg`), encode a result back into guest memory and a wasm -// return status (`AbiRet`), and a single-point gas-charging wrapper -// (`charged`) so every registered closure pays for its call exactly once. +// 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. // --------------------------------------------------------------------------- -/// Encode a *scalar or unit* host-function result into the status the wasm fn -/// returns (>= 0 success — a value; < 0 a HostError code, via `to_wasm_*`). -/// `Out` is the extra wasm scalars for output — always `()` here, since these -/// returns need no guest buffer. -/// -/// Value-producing returns (`Vec` / `[u8; N]`) do *not* go through this -/// trait: they are serviced by [`write_into`], where the host writes straight -/// into guest linear memory with no owned buffer to encode. +/// 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; @@ -35,8 +29,8 @@ impl AbiRet for u32 { } } -/// Charge a host call's gas once (from the enum's spec) then run its body. -/// Because every registered closure goes through here, gas can't be forgotten. +/// Charge a host call's gas from its spec, then run its body. Every registered +/// closure goes through here, so gas cannot be forgotten. pub(crate) fn charged( caller: &mut Caller<'_, VmState<'_>>, op: HostFunctionSpec, @@ -61,18 +55,9 @@ pub(crate) fn to_wasm_i64(r: HostResult) -> i64 { } // --------------------------------------------------------------------------- -// Gas + bounds-checked memory helpers (the crate's only "unsafe surface", -// concentrated and safe: every access is a checked wasmi slice op) +// Gas + memory helpers. Every guest access is a checked wasmi slice op. // --------------------------------------------------------------------------- -/// Per-field size cap for any single value crossing the host/guest boundary. -/// -/// Mirrors `kMaxWasmDataLength = 1 * 1024` in -/// `include/xrpl/protocol/Protocol.h:261`, enforced by `getDataSlice`/ -/// `setData` (`src/libxrpl/tx/wasm/HostFuncWrapper.cpp`) returning -/// `DataFieldTooLarge`. -const MAX_WASM_DATA_LEN: usize = 1024; - /// Deduct `cost` fuel for a host call; `OutOfGas` if it would go negative. fn charge(caller: &mut Caller<'_, T>, cost: u64) -> Result<(), HostError> { let remaining = caller.get_fuel().map_err(|_| HostError::Internal)?; @@ -85,9 +70,9 @@ fn charge(caller: &mut Caller<'_, T>, cost: u64) -> Result<(), HostError> { } } -/// Deduct `n` bytes from the per-run transfer-limit budget (see -/// [`crate::vm::TRANSFER_LIMIT_BYTES`]); `OutOfTransferLimit` if it would go -/// negative. A separate budget from gas — see `VmState::transfer_budget`. +/// Deduct `n` bytes from the per-run transfer-limit budget +/// ([`crate::vm::TRANSFER_LIMIT_BYTES`], separate from gas); +/// `OutOfTransferLimit` if it would go negative. fn charge_transfer(state: &VmState<'_>, n: usize) -> Result<(), HostError> { let n = n as u64; let remaining = state.transfer_budget.get(); @@ -108,18 +93,13 @@ fn memory(caller: &Caller<'_, T>) -> Result { } } -/// Bounds-check `[ptr, ptr + len)` and return a `&[u8]` **aliasing guest linear -/// memory** — no allocation, no copy. The read analog of [`write_into`]: where -/// `write_into` hands the host a `&mut [u8]` into guest memory, this hands it a -/// `&[u8]`, so a *read-only* host call touches the guest's bytes in place. +/// Bounds-check `[ptr, ptr + len)` and return a `&[u8]` aliasing guest linear +/// memory — no allocation, no copy. The slice borrows `caller`, so it lives only +/// as long as the host call it feeds; host functions never re-enter the guest and +/// move its memory. /// -/// The returned slice borrows `caller`, so it is valid only for the duration of -/// the host call it feeds — the same leaf-call invariant `write_into` relies on -/// (our host functions don't re-enter the guest and move its memory). -/// -/// Checks, in order: params validity, the [`MAX_WASM_DATA_LEN`] size cap -/// (`DataFieldTooLarge`), then the transfer-limit budget — all before the -/// slice is formed. +/// Checks params validity, the [`MAX_FIELD_BYTES`] cap (`DataFieldTooLarge`) and +/// the transfer budget, in that order, before the slice is formed. pub(crate) fn read_borrowed<'a>( caller: &'a Caller<'_, VmState<'_>>, ptr: i32, @@ -129,7 +109,7 @@ pub(crate) fn read_borrowed<'a>( return Err(HostError::InvalidParams); } let (ptr, len) = (ptr as usize, len as usize); - if len > MAX_WASM_DATA_LEN { + if len > MAX_FIELD_BYTES { return Err(HostError::DataFieldTooLarge); } charge_transfer(caller.data(), len)?; @@ -140,28 +120,16 @@ pub(crate) fn read_borrowed<'a>( .ok_or(HostError::PointerOutOfBounds) } -/// Service a "fill-the-caller's-buffer" host call: bounds-check the guest -/// output region `[dst, dst + cap)`, hand the host a `&mut [u8]` aliasing it, -/// and let the host write **straight into guest linear memory** — the single -/// copy, with no owned buffer intermediate (this is what removes the extra copy -/// the value-producing host functions used to pay: a `Vec` / `[u8; N]` -/// materialized on the host side, then copied into guest memory. The `CxxHost` -/// path additionally used to marshal C++ `Bytes` through a `rust::Vec` / -/// `HashResult`; that too is gone). +/// Service a "fill-the-caller's-buffer" host call: bounds-check the guest output +/// region `[dst, dst + cap)` and hand the host a `&mut [u8]` aliasing it, so the +/// host writes straight into guest linear memory. Returns the byte count. /// -/// `fill` returns the value's *true* length (it writes only when the value fits -/// in `dst`), so the engine keeps ownership of the policy the guest observes: -/// the [`MAX_WASM_DATA_LEN`] field-size cap (`DataFieldTooLarge`), the -/// buffer-fit check (`BufferTooSmall`), and the transfer-limit budget — checked -/// here, in the same order as the C++ `setData` path (size cap precedes the -/// transfer charge). On success returns the byte count. -/// -/// Ordering note: because the byte count isn't known until `fill` runs, the -/// transfer budget is charged *after* the write rather than before it (the -/// pre-write gas charge in [`charged`] still bounds how often this runs). A -/// value rejected for being over-cap/over-budget may leave bytes in the guest -/// buffer, but they sit within the guest's own bounds and the guest must treat -/// a negative status as "don't read the buffer". +/// `fill` reports the value's true length and writes only what fits, leaving the +/// engine the policy the guest observes: the [`MAX_FIELD_BYTES`] cap +/// (`DataFieldTooLarge`), the buffer fit (`BufferTooSmall`), then the transfer +/// budget — the order the C++ `setData` path uses. Those checks follow `fill`, +/// since the length is unknown before it runs, so a refused value may leave bytes +/// in the guest's own buffer; a negative status tells the guest not to read it. pub(crate) fn write_into( caller: &mut Caller<'_, VmState<'_>>, dst: i32, @@ -184,7 +152,7 @@ pub(crate) fn write_into( let n = fill(host, out)?; - if n > MAX_WASM_DATA_LEN { + if n > MAX_FIELD_BYTES { return Err(HostError::DataFieldTooLarge); } if n > cap { @@ -197,27 +165,17 @@ pub(crate) fn write_into( // The input buffer in `read_write` lives on the stack, sized to the field cap. // Guard the assumption that the cap stays small enough for that to be fine. const _: () = assert!( - MAX_WASM_DATA_LEN <= 8 * 1024, - "read_write's input buffer is a stack array; keep MAX_WASM_DATA_LEN small" + MAX_FIELD_BYTES <= 8 * 1024, + "read_write's input buffer is a stack array; keep MAX_FIELD_BYTES small" ); -/// Service a host call that reads an input region *and* writes an output region -/// of guest memory (e.g. `sha512_half`). +/// Service a host call that reads one region of guest memory and writes another +/// (e.g. `sha512_half`). /// -/// The input is copied into a fixed **stack** buffer — no heap allocation. It's -/// bounded by [`MAX_WASM_DATA_LEN`] (the 1 KiB field cap, checked before the -/// copy), so a plain `[u8; MAX_WASM_DATA_LEN]` array always fits; `&buf[..len]` -/// carries the length, so no wrapper type is needed. Keeping the input in a -/// stack local — rather than a borrow of the wasmi store — is what lets it -/// coexist with the output `&mut [u8]`: [`write_into`] can borrow guest memory -/// mutably for the output while `input` (borrowing the local) stays valid, with -/// no aliasing/split reasoning. The output half reuses [`write_into`] verbatim, -/// so the field-cap / buffer-fit / transfer policy is unchanged. -/// -/// (The stack buffer is zero-initialized each call — one `memset` of the cap -/// size. That's the deliberately-simple PoC trade: it drops the per-call heap -/// allocation the old `Vec` path paid, at the price of a small fixed -/// zero-fill; a `MaybeUninit`/arrayvec buffer could drop that too.) +/// The input is copied into a stack buffer bounded by [`MAX_FIELD_BYTES`], so it +/// stays valid while [`write_into`] borrows guest memory mutably for the output — +/// no aliasing reasoning, at the price of zero-filling the buffer each call. The +/// output half is [`write_into`], so it obeys the same policy as a plain write. pub(crate) fn read_write( caller: &mut Caller<'_, VmState<'_>>, src: i32, @@ -230,14 +188,13 @@ pub(crate) fn read_write( return Err(HostError::InvalidParams); } let len = src_len as usize; - if len > MAX_WASM_DATA_LEN { + if len > MAX_FIELD_BYTES { return Err(HostError::DataFieldTooLarge); } charge_transfer(caller.data(), len)?; - // Copy the input into a stack buffer, then release the (shared) store - // borrow before `write_into` takes it mutably for the output. - let mut buf = [0u8; MAX_WASM_DATA_LEN]; + // 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]) .map_err(|_| HostError::PointerOutOfBounds)?; @@ -245,3 +202,117 @@ pub(crate) fn read_write( write_into(caller, dst, cap, |host, out| call(host, input, out)) } + +// --------------------------------------------------------------------------- +// Unit tests +// +// A `Caller` exists only for the duration of a host call, so `read_borrowed`, +// `write_into`, `read_write` and `memory` are unreachable from here; `tests/` +// covers them by running real modules against a fake host. +// --------------------------------------------------------------------------- + +#[cfg(test)] +mod tests { + use super::*; + use crate::vm::TRANSFER_LIMIT_BYTES; + use std::cell::Cell; + use wasmi::StoreLimitsBuilder; + + /// A host no test here calls; `charge_transfer` takes the store data, which + /// has to hold one. + struct UncalledHost; + + impl HostFunctions for UncalledHost { + fn get_ledger_sqn(&self, _out: &mut [u8]) -> HostResult { + unreachable!("no unit test in this module calls the host") + } + fn get_current_ledger_obj_field(&self, _field: i32, _out: &mut [u8]) -> HostResult { + unreachable!("no unit test in this module calls the host") + } + fn sha512_half(&self, _data: &[u8], _out: &mut [u8]) -> HostResult { + unreachable!("no unit test in this module calls the host") + } + fn trace(&self, _msg: &str, _data: &[u8], _as_hex: bool) -> HostResult<()> { + unreachable!("no unit test in this module calls the host") + } + fn trace_num(&self, _msg: &str, _number: i64) -> HostResult<()> { + unreachable!("no unit test in this module calls the host") + } + } + + /// A `VmState` whose transfer budget starts at `budget`. + fn state(budget: u64) -> VmState<'static> { + VmState { + host: &UncalledHost, + mem_limits: StoreLimitsBuilder::new().build(), + transfer_budget: Cell::new(budget), + } + } + + #[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); + } + + /// `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. + #[test] + fn the_wire_conversion_truncates() { + assert_eq!(to_wasm_i32(Ok(i64::from(i32::MAX) + 1)), i32::MIN); + } + + #[test] + fn a_transfer_spends_the_budget() { + let state = state(100); + + assert_eq!(charge_transfer(&state, 30), Ok(())); + assert_eq!(state.transfer_budget.get(), 70); + assert_eq!(charge_transfer(&state, 70), Ok(())); + assert_eq!(state.transfer_budget.get(), 0); + } + + /// The budget bounds the total, so the last transfer that fits is allowed and + /// the one that would overrun is refused whole — never partially charged. + #[test] + fn a_transfer_past_the_budget_is_refused_and_charges_nothing() { + let state = state(100); + + assert_eq!( + charge_transfer(&state, 101), + Err(HostError::OutOfTransferLimit) + ); + assert_eq!( + state.transfer_budget.get(), + 100, + "a refusal must not charge" + ); + assert_eq!(charge_transfer(&state, 100), Ok(())); + assert_eq!( + charge_transfer(&state, 1), + Err(HostError::OutOfTransferLimit) + ); + } + + #[test] + fn transferring_nothing_costs_nothing() { + let state = state(0); + + assert_eq!(charge_transfer(&state, 0), Ok(())); + assert_eq!(state.transfer_budget.get(), 0); + } + + /// The field cap holds any one call to a small share of the run's budget, so + /// the budget bounds a run rather than a call. An inequality, not the two + /// values: those are pinned in `vm.rs`. + #[test] + fn no_single_value_can_exhaust_the_run_budget() { + assert!( + (MAX_FIELD_BYTES as u64) * 64 <= TRANSFER_LIMIT_BYTES, + "one {MAX_FIELD_BYTES}-byte value against a {TRANSFER_LIMIT_BYTES}-byte budget" + ); + } +} diff --git a/crates/xrpl-wasm-vm/src/lib.rs b/crates/xrpl-wasm-vm/src/lib.rs index 50936e7036..3d4aa1290e 100644 --- a/crates/xrpl-wasm-vm/src/lib.rs +++ b/crates/xrpl-wasm-vm/src/lib.rs @@ -2,4 +2,6 @@ mod abi; mod register; mod vm; -pub use vm::run; +pub use vm::{ + MAX_FIELD_BYTES, MAX_MEMORY_BYTES, MAX_MEMORY_PAGES, RunOutcome, TRANSFER_LIMIT_BYTES, run, +}; diff --git a/crates/xrpl-wasm-vm/src/vm.rs b/crates/xrpl-wasm-vm/src/vm.rs index 6cbae96719..2934c7e963 100644 --- a/crates/xrpl-wasm-vm/src/vm.rs +++ b/crates/xrpl-wasm-vm/src/vm.rs @@ -15,32 +15,34 @@ pub const MAX_MEMORY_PAGES: u32 = 128; pub const MAX_MEMORY_BYTES: usize = (MAX_MEMORY_PAGES * WASM_PAGE_BYTES) as usize; /// Per-run transfer-limit budget: total bytes that may cross the host/guest -/// boundary (via the `read_bytes` / `write_into` helpers in `abi.rs`) during -/// one [`run_escrow`] invocation. A budget separate from gas. +/// boundary during one [`run`] invocation. Separate from gas. pub const TRANSFER_LIMIT_BYTES: u64 = 1 << 20; +/// Size cap on any single value crossing the host/guest boundary, in either +/// direction. A value over it is refused with `DataFieldTooLarge`. +/// +/// Mirrors `kMaxWasmDataLength = 1 * 1024` in +/// `include/xrpl/protocol/Protocol.h:261`, enforced there by `getDataSlice` / +/// `setData` (`src/libxrpl/tx/wasm/HostFuncWrapper.cpp`). +pub const MAX_FIELD_BYTES: usize = 1024; + /// State threaded through every host call, stored in the wasmi [`Store`]. pub struct VmState<'h> { pub(crate) host: &'h dyn HostFunctions, - /// Enforces [`MAX_MEMORY_BYTES`] via `Store::limiter` (see `run_escrow`). - /// Lives in `VmState` (rather than as a standalone local) because the - /// limiter callback wasmi holds must be able to produce a `&mut` into it - /// from `&mut VmState`. + /// Enforces [`MAX_MEMORY_BYTES`] via `Store::limiter`. It lives here because + /// the limiter callback wasmi holds has to produce a `&mut` into it from + /// `&mut VmState`. pub(crate) mem_limits: StoreLimits, - /// Remaining transfer-limit budget for this run (see - /// [`TRANSFER_LIMIT_BYTES`]); decremented in `abi.rs`'s `read_bytes` / - /// `write_into` by the number of bytes actually moved. + /// Remaining transfer-limit budget for this run ([`TRANSFER_LIMIT_BYTES`]), + /// decremented in `abi.rs` by the bytes actually moved. /// - /// A `Cell`, not a plain `u64`: `AbiArg::read` (the guest -> host read - /// path) only has a shared `&Caller`, while `write_into` (the host -> - /// guest write path) has `&mut Caller` — both need to decrement this - /// counter, so it can't be an ordinary field mutated only through - /// `&mut`. The store (and this counter) is only ever touched from one - /// thread per invocation, so `Cell`'s lack of `Sync` is not an issue. + /// A `Cell` because the read path holds only a shared `&Caller` while the + /// write path holds `&mut Caller`, and both decrement it. One thread per + /// invocation touches the store, so `Cell`'s lack of `Sync` is no issue. /// - /// NOTE: the C++ `unalignedGas`/`FieldLocator` alignment-copy charge - /// (`HostFuncWrapper.cpp:44,390-397`) is deferred — the PoC has no - /// `FieldLocator` host functions yet to attach it to. + /// TODO: the C++ `unalignedGas` alignment-copy charge + /// (`HostFuncWrapper.cpp:44,390-397`) has no `FieldLocator` host function + /// here to attach to. pub(crate) transfer_budget: Cell, } @@ -57,19 +59,16 @@ pub struct RunOutcome { /// The process-wide wasmi engine, built once on first use. /// -/// The engine's configuration is consensus-fixed and identical for every -/// invocation, so there is no reason to rebuild it per finish. A wasmi -/// [`Engine`] is an `Arc` internally (cheap to share, `Send + Sync`), and -/// modules compiled against it are per-invocation, so a single shared engine is -/// safe to reuse across concurrent [`run_escrow`] calls. +/// 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 { static ENGINE: LazyLock = LazyLock::new(build_wasm_engine); &ENGINE } -/// Build the wasmi engine with the sandboxing knobs the escrow VM requires. -/// (Unchanged from the original skeleton: a deterministic, minimal-feature -/// configuration with fuel metering on.) +/// Build the wasmi engine the escrow VM requires: deterministic, minimal +/// features, fuel metering on. fn build_wasm_engine() -> Engine { let mut config = Config::default(); config.consume_fuel(true); @@ -115,9 +114,8 @@ pub fn run<'h>( }, ); store.set_fuel(gas).map_err(|e| format!("set_fuel: {e}"))?; - // Registers the memory-page cap; also applied at instantiation time (an - // initial memory declared past the cap fails instantiation, same as a - // `memory.grow` past it traps at runtime). + // 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); @@ -140,3 +138,27 @@ pub fn run<'h>( fuel_used: gas.saturating_sub(remaining), }) } + +#[cfg(test)] +mod tests { + use super::*; + + /// The engine is built once and shared, so two invocations must not compile + /// their modules against different engines. + #[test] + fn the_engine_is_one_engine() { + assert!(Engine::same(wasm_engine(), wasm_engine())); + } + + /// 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 + /// greppable against `include/xrpl/protocol/Protocol.h`. + #[test] + fn the_limits_are_the_protocol_limits() { + assert_eq!(MAX_MEMORY_PAGES, 128, "maxPages"); + assert_eq!(MAX_MEMORY_BYTES, 8 * 1024 * 1024, "maxPages, in bytes"); + assert_eq!(MAX_FIELD_BYTES, 1024, "kMaxWasmDataLength"); + assert_eq!(TRANSFER_LIMIT_BYTES, 1 << 20, "kWasmTransferLimit"); + } +} diff --git a/crates/xrpl-wasm-vm/tests/budgets.rs b/crates/xrpl-wasm-vm/tests/budgets.rs new file mode 100644 index 0000000000..95f9e7ac6b --- /dev/null +++ b/crates/xrpl-wasm-vm/tests/budgets.rs @@ -0,0 +1,363 @@ +//! The two budgets a run spends: gas (fuel), and the transfer limit on bytes +//! crossing the boundary. Both are consensus input, so several of these tests +//! assert exact numbers. + +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}; + +// --------------------------------------------------------------------------- +// Gas +// --------------------------------------------------------------------------- + +/// The fuel a module of `body` burns, given gas to spare. +fn fuel_for(body: &str, parts: &[&str], host: &FakeHost) -> u64 { + let wat = module(parts, body); + run(&wat, host).expect("the module should run").fuel_used +} + +/// The fuel a module burns doing nothing but returning a constant; every figure +/// below builds on it. wasmi's number, pinned deliberately because wasmi's fuel +/// table is consensus input. +const EMPTY_MODULE_FUEL: u64 = 30; + +/// wasmi's own fuel for a host call whose operands are all constants under 64: 14 +/// per `*.const`, plus 1 for the call. Our gas sits on top. +/// +/// The formula holds only under 64, because wasmi widens a constant's encoding +/// above that, each tier costing 7 more. Every call in [`call_for`] keeps its +/// operands small for that reason; one with a larger constant fails here by a +/// multiple of 7. +fn wasmi_call_fuel(small_const_operands: u64) -> u64 { + 14 * small_const_operands + 1 +} + +/// wasmi's fuel for one `(drop …)`, which is how a module makes more than one call +/// and keeps only the last result. Pinned like the two above. +const WASMI_DROP_FUEL: u64 = 21; + +/// The wasm a test needs in order to call one host function: the `(import …)` +/// declaration, a call with small-constant operands, and how many it pushes. +struct Call { + import: &'static str, + call: &'static str, + operands: u64, +} + +/// The test wasm for each host function. The `match` is exhaustive, so a function +/// added to the ABI fails to compile until it has wasm here, and iterating +/// [`HostFunctionSpec::ALL`] then covers the whole ABI. +fn call_for(op: HostFunctionSpec) -> Call { + let (import, call, operands) = match op { + HostFunctionSpec::GetLedgerSqn => ( + import::LDGR_INDEX, + "(call $ldgr_index (i32.const 0) (i32.const 4))", + 2, + ), + HostFunctionSpec::GetCurrentLedgerObjField => ( + import::HOME_LE_FIELD, + "(call $home_le_field (i32.const 1) (i32.const 0) (i32.const 4))", + 3, + ), + HostFunctionSpec::Sha512Half => ( + import::SHA512_HALF, + "(call $sha512_half (i32.const 0) (i32.const 4) (i32.const 0) (i32.const 32))", + 4, + ), + HostFunctionSpec::Trace => ( + import::TRACE, + "(call $trace (i32.const 0) (i32.const 0) (i32.const 0) (i32.const 0) (i32.const 0))", + 5, + ), + HostFunctionSpec::TraceNum => ( + import::TRACE_NUM, + "(call $trace_num (i32.const 0) (i32.const 0) (i64.const 0))", + 3, + ), + }; + Call { + import, + call, + operands, + } +} + +#[test] +fn an_empty_module_burns_a_fixed_amount_of_fuel() { + let fuel = fuel_for("(i32.const 0)", &[ONE_PAGE], &FakeHost::new()); + assert_eq!(fuel, EMPTY_MODULE_FUEL); +} + +/// Calling a host function `n` times costs `n` times its gas, to the unit. Every +/// other term is known — the module's floor, wasmi's fuel per call, one `drop` +/// between consecutive calls — so the total is a closed form, with the gas read +/// from the spec table rather than restated. `n = 1` pins the charge, `n > 1` pins +/// that it lands on every call rather than once per run. +#[test] +fn a_host_call_costs_its_gas_every_time_it_is_called() { + let host = FakeHost::new().answering_field(1, Answer::bytes([0xaa])); + + for &op in HostFunctionSpec::ALL { + let Call { + import, + call, + operands, + } = call_for(op); + let per_call = wasmi_call_fuel(operands) + op.gas(); + + for n in 1..=3 { + let body = format!("{}{call}", format!("(drop {call}) ").repeat(n - 1)); + let n = n as u64; + + assert_eq!( + fuel_for(&body, &[import, ONE_PAGE], &host), + EMPTY_MODULE_FUEL + n * per_call + (n - 1) * WASMI_DROP_FUEL, + "{n} x {call}" + ); + } + } +} + +/// The gas charge precedes the call's body, so a failing call costs exactly what a +/// successful one costs. Field 1 is answered and field 7 is not; the two modules +/// are otherwise identical, so their totals are comparable. +#[test] +fn a_failing_host_call_costs_exactly_what_a_successful_one_costs() { + let host = FakeHost::new().answering_field(1, Answer::bytes([0xaa])); + let call = |field: i32| { + module( + &[import::HOME_LE_FIELD, ONE_PAGE], + &format!("(call $home_le_field (i32.const {field}) (i32.const 0) (i32.const 4))"), + ) + }; + + let answered = run(&call(1), &host).expect("the module should run"); + let refused = run(&call(7), &host).expect("the module should run"); + + assert_eq!(answered.result, 1); + assert_eq!(refused.result, code(HostError::FieldNotFound)); + assert_eq!(refused.fuel_used, answered.fuel_used); +} + +/// `fuel_used` is `gas - remaining`: what the run spent, not what was left or what +/// it was handed. The gas figures are derived from the run's cost, so the boundary +/// — exactly enough, and one short — is among the cases. +#[test] +fn fuel_used_is_what_was_spent_not_what_was_supplied() { + let host = FakeHost::new(); + let op = HostFunctionSpec::GetLedgerSqn; + let Call { + import, + call, + operands, + } = call_for(op); + let wat = module(&[import, ONE_PAGE], call); + 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. + for gas in [cost, cost + 1, cost * 100, PLENTY_OF_GAS] { + let outcome = run_with_gas(&wat, gas, &host).expect("should run"); + assert_eq!( + outcome.result, 4, + "gas {gas}: the call should have succeeded" + ); + 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++" + ); +} + +/// Fuel is metered, so the same module burns the same fuel every time — a +/// property consensus depends on. +#[test] +fn the_same_run_burns_the_same_fuel() { + let wat = module( + &[import::TRACE, ONE_PAGE], + "(call $trace (i32.const 0) (i32.const 0) (i32.const 0) (i32.const 0) (i32.const 0))", + ); + + let first = run(&wat, &FakeHost::new()).expect("should run").fuel_used; + for _ in 0..4 { + assert_eq!( + run(&wat, &FakeHost::new()).expect("should run").fuel_used, + first + ); + } + assert!(first > HostFunctionSpec::Trace.gas()); +} + +/// Too little gas to finish stops the run. +#[test] +fn a_run_that_cannot_afford_itself_fails() { + let host = FakeHost::new(); + let wat = module( + &[import::LDGR_INDEX, ONE_PAGE], + "(call $ldgr_index (i32.const 0) (i32.const 4))", + ); + + for gas in [0, 1, 10] { + let outcome = run_with_gas(&wat, gas, &host); + assert!( + outcome.is_err(), + "gas {gas} should not have completed: {outcome:?}" + ); + } +} + +/// A guest looping forever is stopped by gas rather than running away. +#[test] +fn an_endless_loop_is_stopped_by_gas() { + 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}"); +} + +/// **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. +#[test] +fn out_of_gas_in_a_host_call_currently_reaches_the_guest_as_a_code() { + let host = FakeHost::new(); + + // 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; + } + } + 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"); +} + +// --------------------------------------------------------------------------- +// The transfer limit +// --------------------------------------------------------------------------- + +/// A module that repeats `call` while `keep_going` holds, then returns the last +/// status, so a budget can be run to exhaustion inside one invocation. +fn until_refused(imports: &str, call: &str, keep_going: &str) -> String { + module( + &[imports, ONE_PAGE], + &format!( + "(local $r i32) + (loop $l + (local.set $r {call}) + (br_if $l {keep_going})) + (local.get $r)" + ), + ) +} + +/// For a call whose success is a positive byte count. +const WHILE_POSITIVE: &str = "(i32.gt_s (local.get $r) (i32.const 0))"; +/// For a call whose success is a status of 0. +const WHILE_ZERO: &str = "(i32.eqz (local.get $r))"; + +/// Bytes written into guest memory are charged against the run's budget, and the +/// budget is a per-run total: 1 MiB of 1 KiB values exhausts it. +#[test] +fn writes_spend_the_transfer_budget() { + let host = FakeHost::new().answering_field(1, Answer::filler(MAX_FIELD_BYTES)); + let wat = until_refused( + import::HOME_LE_FIELD, + &format!("(call $home_le_field (i32.const 1) (i32.const 0) (i32.const {MAX_FIELD_BYTES}))"), + WHILE_POSITIVE, + ); + + let outcome = run(&wat, &host).expect("the module should run"); + assert_eq!(outcome.result, code(HostError::OutOfTransferLimit)); + assert_eq!( + host.fields_asked.borrow().len() as u64, + TRANSFER_LIMIT_BYTES / MAX_FIELD_BYTES as u64 + 1, + "one call per 1 KiB of budget, plus the one that was refused" + ); +} + +/// The budget is per run, not per call: a fresh run starts with a full budget. +#[test] +fn each_run_gets_its_own_budget() { + let wat = until_refused( + import::HOME_LE_FIELD, + &format!("(call $home_le_field (i32.const 1) (i32.const 0) (i32.const {MAX_FIELD_BYTES}))"), + WHILE_POSITIVE, + ); + + for _ in 0..2 { + let host = FakeHost::new().answering_field(1, Answer::filler(MAX_FIELD_BYTES)); + let outcome = run(&wat, &host).expect("the module should run"); + assert_eq!(outcome.result, code(HostError::OutOfTransferLimit)); + assert_eq!( + host.fields_asked.borrow().len() as u64, + TRANSFER_LIMIT_BYTES / MAX_FIELD_BYTES as u64 + 1 + ); + } +} + +/// A run well inside the budget never sees it. +#[test] +fn a_modest_run_never_meets_the_budget() { + let host = FakeHost::new().answering_field(1, Answer::filler(MAX_FIELD_BYTES)); + let wat = module( + &[import::HOME_LE_FIELD, ONE_PAGE], + &format!("(call $home_le_field (i32.const 1) (i32.const 0) (i32.const {MAX_FIELD_BYTES}))"), + ); + + let outcome = run(&wat, &host).expect("the module should run"); + assert_eq!(outcome.result, MAX_FIELD_BYTES as i32); +} + +/// **Pins current behaviour, not a decision.** `read_borrowed` hands the host a +/// slice aliasing guest memory, copying nothing, yet charges the bytes against the +/// transfer budget. Finding A4 in `docs/claude/redesign_impl.md` says the rule +/// should be settled. +#[test] +fn reads_currently_spend_the_transfer_budget_too() { + let host = FakeHost::new(); + let wat = until_refused( + import::TRACE_NUM, + &format!("(call $trace_num (i32.const 0) (i32.const {MAX_FIELD_BYTES}) (i64.const 0))"), + WHILE_ZERO, + ); + + let outcome = run(&wat, &host).expect("the module should run"); + assert_eq!( + outcome.result, + code(HostError::OutOfTransferLimit), + "a read of aliased bytes is charged as though it were copied" + ); + assert_eq!( + host.traces().len() as u64, + TRANSFER_LIMIT_BYTES / MAX_FIELD_BYTES as u64, + "the budget ran out after 1 MiB of reads that copied nothing" + ); +} diff --git a/crates/xrpl-wasm-vm/tests/host_calls.rs b/crates/xrpl-wasm-vm/tests/host_calls.rs new file mode 100644 index 0000000000..d9987d58ea --- /dev/null +++ b/crates/xrpl-wasm-vm/tests/host_calls.rs @@ -0,0 +1,245 @@ +//! What each registered host function passes in each direction: the scalars the +//! guest supplies reach the host unchanged, and the bytes the host produces land +//! where the guest asked for them. + +mod support; + +use support::{FakeHost, ONE_PAGE, Trace, code, import, module, run, status}; +use xrpl_host_functions::{HASH_LEN, HostError}; + +/// A value the host writes must be readable by the guest at the pointer it gave, +/// and the call's status is the byte count. +#[test] +fn ldgr_index_writes_the_sequence_number_where_the_guest_asked() { + let host = FakeHost::new(); + + let wat = module( + &[import::LDGR_INDEX, ONE_PAGE], + "(drop (call $ldgr_index (i32.const 64) (i32.const 4))) + (i32.load (i32.const 64))", + ); + assert_eq!(status(&wat, &host), 7, "the 4 LE bytes the host wrote"); + + let wat = module( + &[import::LDGR_INDEX, ONE_PAGE], + "(call $ldgr_index (i32.const 64) (i32.const 4))", + ); + assert_eq!(status(&wat, &host), 4, "the byte count"); +} + +/// The output region is wherever the guest points, not a fixed address. +#[test] +fn the_output_region_is_the_pointer_the_guest_gave() { + let host = FakeHost::new(); + + for offset in [0, 1, 7, 4096, 65532] { + let wat = module( + &[import::LDGR_INDEX, ONE_PAGE], + &format!( + "(drop (call $ldgr_index (i32.const {offset}) (i32.const 4))) + (i32.load (i32.const {offset}))" + ), + ); + assert_eq!(status(&wat, &host), 7, "at offset {offset}"); + } +} + +/// A leading scalar parameter reaches the host as declared. +#[test] +fn home_le_field_passes_the_field_selector_through() { + let host = FakeHost::new().answering_field(17, support::Answer::bytes([0xab, 0xcd])); + + let wat = module( + &[import::HOME_LE_FIELD, ONE_PAGE], + "(call $home_le_field (i32.const 17) (i32.const 0) (i32.const 64))", + ); + assert_eq!(status(&wat, &host), 2); + assert_eq!(*host.fields_asked.borrow(), vec![17]); +} + +/// A host error reaches the guest as its negative wire code, and the output +/// region is left as the guest had it. +#[test] +fn a_host_error_becomes_its_wire_code() { + let host = FakeHost::new(); + const UNTOUCHED: i32 = 7; + + // Field 99 is unanswered, so the host returns `FieldNotFound`. The guest + // stamps its buffer first, then checks the byte survived the failed call. + let wat = module( + &[import::HOME_LE_FIELD, ONE_PAGE], + &format!( + "(i32.store8 (i32.const 0) (i32.const {UNTOUCHED})) + (drop (call $home_le_field (i32.const 99) (i32.const 0) (i32.const 64))) + (i32.load8_u (i32.const 0))" + ), + ); + assert_eq!(status(&wat, &host), UNTOUCHED, "nothing was written"); + + let wat = module( + &[import::HOME_LE_FIELD, ONE_PAGE], + "(call $home_le_field (i32.const 99) (i32.const 0) (i32.const 64))", + ); + assert_eq!(status(&wat, &host), code(HostError::FieldNotFound)); +} + +/// `sha512_half` reads one region and writes another in the same call. +#[test] +fn sha512_half_carries_bytes_in_and_out() { + const MARKER: u8 = 99; + + let host = FakeHost::new().answering_digest(support::Answer::bytes([MARKER; HASH_LEN])); + + let wat = module( + &[ + import::SHA512_HALF, + ONE_PAGE, + r#"(data (i32.const 0) "hello wasm")"#, + ], + &format!( + "(drop (call $sha512_half (i32.const 0) (i32.const 10) + (i32.const 128) (i32.const {HASH_LEN}))) + (i32.load8_u (i32.const 128))" + ), + ); + assert_eq!( + status(&wat, &host), + i32::from(MARKER), + "the first digest byte" + ); + assert_eq!( + *host.digested.borrow(), + vec![b"hello wasm".to_vec()], + "the input the host saw" + ); +} + +/// An empty input region is a legal read, not an error. +#[test] +fn sha512_half_accepts_an_empty_input() { + let host = FakeHost::new(); + + let wat = module( + &[import::SHA512_HALF, ONE_PAGE], + "(call $sha512_half (i32.const 0) (i32.const 0) (i32.const 128) (i32.const 32))", + ); + assert_eq!(status(&wat, &host), 32); + assert_eq!(*host.digested.borrow(), vec![Vec::::new()]); +} + +/// `trace` reads two regions and a flag, and yields a status of 0. +#[test] +fn trace_passes_its_message_data_and_flag_through() { + let host = FakeHost::new(); + + let wat = module( + &[ + import::TRACE, + ONE_PAGE, + r#"(data (i32.const 0) "note")"#, + r#"(data (i32.const 16) "\01\02\03")"#, + ], + "(call $trace (i32.const 0) (i32.const 4) (i32.const 16) (i32.const 3) (i32.const 1))", + ); + assert_eq!(status(&wat, &host), 0, "trace yields a status of 0"); + assert_eq!( + host.traces(), + vec![Trace::Message { + msg: "note".to_owned(), + data: vec![1, 2, 3], + as_hex: true, + }] + ); +} + +/// The flag is `bool` in the declaration and `i32` on the wire: nonzero is true. +#[test] +fn any_nonzero_flag_is_true() { + for (flag, expected) in [("0", false), ("1", true), ("2", true), ("-1", true)] { + let host = FakeHost::new(); + let wat = module( + &[import::TRACE, ONE_PAGE], + &format!( + "(call $trace (i32.const 0) (i32.const 0) (i32.const 0) (i32.const 0) (i32.const {flag}))" + ), + ); + assert_eq!(status(&wat, &host), 0); + let Some(Trace::Message { as_hex, .. }) = host.traces().first().cloned() else { + panic!("expected one traced message"); + }; + assert_eq!(as_hex, expected, "flag {flag}"); + } +} + +/// An `i64` parameter crosses as an `i64`, full width. +#[test] +fn trace_num_carries_a_full_width_i64() { + for number in [0, 1, -1, i64::MAX, i64::MIN] { + let host = FakeHost::new(); + let wat = module( + &[import::TRACE_NUM, ONE_PAGE], + &format!("(call $trace_num (i32.const 0) (i32.const 0) (i64.const {number}))"), + ); + assert_eq!(status(&wat, &host), 0); + assert_eq!( + host.traces(), + vec![Trace::Number { + msg: String::new(), + number, + }] + ); + } +} + +/// A `&str` parameter is a byte region the engine validates: the host is handed +/// a `&str`, so bytes that are not UTF-8 cannot be passed on. +#[test] +fn a_message_that_is_not_utf8_is_refused() { + let host = FakeHost::new(); + + let wat = module( + &[ + import::TRACE_NUM, + ONE_PAGE, + r#"(data (i32.const 0) "\ff\fe")"#, + ], + "(call $trace_num (i32.const 0) (i32.const 2) (i64.const 0))", + ); + assert_eq!(status(&wat, &host), code(HostError::Decoding)); + assert!(host.traces().is_empty(), "the host must not be called"); +} + +/// Several host calls in one run each see their own arguments: the two fields answer +/// with distinct marker bytes and `finish` returns their sum, so a value landing in +/// the wrong place gives a different total. +#[test] +fn calls_do_not_bleed_into_each_other() { + const FIRST: u8 = 11; + const SECOND: u8 = 22; + + let host = FakeHost::new() + .answering_field(1, support::Answer::bytes([FIRST])) + .answering_field(2, support::Answer::bytes([SECOND, SECOND])); + + let wat = module( + &[import::HOME_LE_FIELD, ONE_PAGE], + "(drop (call $home_le_field (i32.const 1) (i32.const 0) (i32.const 64))) + (drop (call $home_le_field (i32.const 2) (i32.const 64) (i32.const 64))) + (i32.add (i32.load8_u (i32.const 0)) (i32.load8_u (i32.const 64)))", + ); + assert_eq!(status(&wat, &host), i32::from(FIRST) + i32::from(SECOND)); + assert_eq!(*host.fields_asked.borrow(), vec![1, 2]); +} + +/// The run's outcome carries the entry point's return value, and that value is +/// the guest's own — the engine does not interpret it. +#[test] +fn the_outcome_carries_whatever_the_guest_returned() { + let host = FakeHost::new(); + + for value in [0, 1, -1, i32::MAX, i32::MIN] { + let wat = module(&[ONE_PAGE], &format!("(i32.const {value})")); + let outcome = run(&wat, &host).expect("the module should run"); + assert_eq!(outcome.result, value); + } +} diff --git a/crates/xrpl-wasm-vm/tests/memory_policy.rs b/crates/xrpl-wasm-vm/tests/memory_policy.rs new file mode 100644 index 0000000000..41dc555a97 --- /dev/null +++ b/crates/xrpl-wasm-vm/tests/memory_policy.rs @@ -0,0 +1,422 @@ +//! The bounds, field-cap and buffer-fit rules `abi.rs` enforces on every region +//! crossing the boundary. This is the policy the guest observes, so each rule is +//! pinned to the code it answers with. + +mod support; + +use support::{Answer, FakeHost, ONE_PAGE, code, import, module, status}; +use xrpl_host_functions::{HASH_LEN, HostError}; +use xrpl_wasm_vm::MAX_FIELD_BYTES; + +/// One page, so anything at or past 65536 is out of bounds. +const PAGE: i64 = 64 * 1024; + +/// The per-field size cap, as a wasm operand. +const CAP: i64 = MAX_FIELD_BYTES as i64; +/// One byte over the cap: the smallest value the engine must refuse. +const OVER_CAP: i64 = CAP + 1; + +// --------------------------------------------------------------------------- +// Output regions (`write_into`) +// --------------------------------------------------------------------------- + +/// The whole output region must be in bounds, not merely its start — the engine +/// checks `[dst, dst + cap)` before the host is allowed to write. +#[test] +fn an_output_region_running_past_memory_is_refused() { + let host = FakeHost::new(); + + for (dst, cap) in [(PAGE, 4), (PAGE - 3, 4), (PAGE + 1024, 4), (0, PAGE + 1)] { + let wat = module( + &[import::LDGR_INDEX, ONE_PAGE], + &format!("(call $ldgr_index (i32.const {dst}) (i32.const {cap}))"), + ); + assert_eq!( + status(&wat, &host), + code(HostError::PointerOutOfBounds), + "dst {dst} cap {cap}" + ); + } +} + +/// A region ending exactly at the last byte of memory is in bounds. +#[test] +fn an_output_region_ending_at_the_last_byte_is_allowed() { + let host = FakeHost::new(); + + let wat = module( + &[import::LDGR_INDEX, ONE_PAGE], + &format!("(call $ldgr_index (i32.const {}) (i32.const 4))", PAGE - 4), + ); + assert_eq!(status(&wat, &host), 4); +} + +/// The wire carries `i32`, so a guest can present a negative pointer or length. +#[test] +fn a_negative_output_pointer_or_length_is_refused() { + let host = FakeHost::new(); + + for (dst, cap) in [(-1, 4), (0, -1), (-1, -1), (i32::MIN, 4)] { + let wat = module( + &[import::LDGR_INDEX, ONE_PAGE], + &format!("(call $ldgr_index (i32.const {dst}) (i32.const {cap}))"), + ); + assert_eq!( + status(&wat, &host), + code(HostError::InvalidParams), + "dst {dst} cap {cap}" + ); + } +} + +/// The host reports a value's true length whether or not it fitted; a value that +/// did not fit is the guest's error, not the host's. +#[test] +fn a_value_larger_than_the_buffer_is_refused() { + let host = FakeHost::new().answering_field(1, Answer::filler(64)); + + let wat = module( + &[import::HOME_LE_FIELD, ONE_PAGE], + "(call $home_le_field (i32.const 1) (i32.const 0) (i32.const 63))", + ); + assert_eq!(status(&wat, &host), code(HostError::BufferTooSmall)); + + let wat = module( + &[import::HOME_LE_FIELD, ONE_PAGE], + "(call $home_le_field (i32.const 1) (i32.const 0) (i32.const 64))", + ); + assert_eq!(status(&wat, &host), 64, "exactly enough room is enough"); +} + +/// A zero-length output region is in bounds and simply cannot hold anything. +#[test] +fn a_zero_length_output_region_is_in_bounds_but_too_small() { + let host = FakeHost::new(); + + let wat = module( + &[import::LDGR_INDEX, ONE_PAGE], + "(call $ldgr_index (i32.const 0) (i32.const 0))", + ); + assert_eq!(status(&wat, &host), code(HostError::BufferTooSmall)); +} + +/// A host that reports more than the per-field cap is refused even when the +/// guest offered room for it: the cap is the engine's rule, not the buffer's. +#[test] +fn a_value_past_the_field_cap_is_refused() { + let host = FakeHost::new() + .answering_field(1, Answer::claiming(OVER_CAP as usize)) + .answering_field(2, Answer::claiming(MAX_FIELD_BYTES)); + + let wat = module( + &[import::HOME_LE_FIELD, ONE_PAGE], + "(call $home_le_field (i32.const 1) (i32.const 0) (i32.const 4096))", + ); + assert_eq!(status(&wat, &host), code(HostError::DataFieldTooLarge)); + + let wat = module( + &[import::HOME_LE_FIELD, ONE_PAGE], + "(call $home_le_field (i32.const 2) (i32.const 0) (i32.const 4096))", + ); + assert_eq!(status(&wat, &host), CAP as i32, "the cap itself is allowed"); +} + +/// **Pins current behaviour, not a decision.** `write_into` checks the field cap +/// after `fill` has written, so an over-cap value reaches the guest's own buffer +/// and is then refused. Finding A4 in `docs/claude/redesign_impl.md` says the write +/// should be clamped instead. +/// +/// The host answers with a real over-cap value: [`Answer::claiming`] writes +/// nothing and so could not show the bytes landing. +#[test] +fn an_over_cap_value_is_written_before_it_is_refused() { + let over_cap = vec![0xff; MAX_FIELD_BYTES + 1]; + let host = FakeHost::new().answering_field(1, Answer::bytes(over_cap)); + + let wat = module( + &[import::HOME_LE_FIELD, ONE_PAGE], + "(drop (call $home_le_field (i32.const 1) (i32.const 0) (i32.const 4096))) + (i32.load8_u (i32.const 0))", + ); + // The status the guest sees, from a module that returns it directly. + let refusing = module( + &[import::HOME_LE_FIELD, ONE_PAGE], + "(call $home_le_field (i32.const 1) (i32.const 0) (i32.const 4096))", + ); + assert_eq!( + status(&refusing, &host), + code(HostError::DataFieldTooLarge), + "the value is refused" + ); + assert_eq!( + status(&wat, &host), + 0xff, + "but its bytes are already in guest memory" + ); +} + +/// The field cap is checked before the buffer-fit rule, so a value that breaks both +/// is reported as over-cap. The guest branches on the code, and the two rules +/// answer different questions, so the order is worth pinning. +#[test] +fn the_field_cap_precedes_the_buffer_fit_check() { + let host = FakeHost::new().answering_field(1, Answer::claiming(MAX_FIELD_BYTES + 1)); + + // A 63-byte buffer: the value is both over the cap and far too big to fit. + let wat = module( + &[import::HOME_LE_FIELD, ONE_PAGE], + "(call $home_le_field (i32.const 1) (i32.const 0) (i32.const 63))", + ); + assert_eq!(status(&wat, &host), code(HostError::DataFieldTooLarge)); +} + +// --------------------------------------------------------------------------- +// Input regions (`read_borrowed`, via `trace`) +// --------------------------------------------------------------------------- + +/// An input region is bounds-checked the same way an output region is. Every case +/// here stays within the field cap, which on an input is checked first. +#[test] +fn an_input_region_running_past_memory_is_refused() { + let host = FakeHost::new(); + + for (ptr, len) in [(PAGE, 1), (PAGE - 3, 4), (PAGE - 1, CAP)] { + let wat = module( + &[import::TRACE_NUM, ONE_PAGE], + &format!("(call $trace_num (i32.const {ptr}) (i32.const {len}) (i64.const 0))"), + ); + assert_eq!( + status(&wat, &host), + code(HostError::PointerOutOfBounds), + "ptr {ptr} len {len}" + ); + assert!(host.traces().is_empty(), "the host must not be called"); + } +} + +#[test] +fn a_negative_input_pointer_or_length_is_refused() { + let host = FakeHost::new(); + + for (ptr, len) in [(-1, 1), (0, -1), (i32::MIN, 1)] { + let wat = module( + &[import::TRACE_NUM, ONE_PAGE], + &format!("(call $trace_num (i32.const {ptr}) (i32.const {len}) (i64.const 0))"), + ); + assert_eq!( + status(&wat, &host), + code(HostError::InvalidParams), + "ptr {ptr} len {len}" + ); + } +} + +/// The field cap bounds what the guest may hand *in*, too. +#[test] +fn an_input_past_the_field_cap_is_refused() { + let host = FakeHost::new(); + + let wat = module( + &[import::TRACE_NUM, ONE_PAGE], + &format!("(call $trace_num (i32.const 0) (i32.const {OVER_CAP}) (i64.const 0))"), + ); + assert_eq!(status(&wat, &host), code(HostError::DataFieldTooLarge)); + assert!(host.traces().is_empty()); + + let wat = module( + &[import::TRACE_NUM, ONE_PAGE], + &format!("(call $trace_num (i32.const 0) (i32.const {CAP}) (i64.const 0))"), + ); + assert_eq!(status(&wat, &host), 0, "the cap itself is allowed"); +} + +/// The two directions check in opposite orders: an input's length is known before +/// the read, so the cap comes first, while an output's region has to be resolved +/// before the host can produce a value, so bounds come first there. +#[test] +fn the_field_cap_precedes_the_bounds_check_on_an_input() { + let host = FakeHost::new(); + + let reading = module( + &[import::TRACE_NUM, ONE_PAGE], + &format!( + "(call $trace_num (i32.const 0) (i32.const {}) (i64.const 0))", + PAGE + 1 + ), + ); + assert_eq!(status(&reading, &host), code(HostError::DataFieldTooLarge)); + + let writing = module( + &[import::LDGR_INDEX, ONE_PAGE], + &format!("(call $ldgr_index (i32.const 0) (i32.const {}))", PAGE + 1), + ); + assert_eq!(status(&writing, &host), code(HostError::PointerOutOfBounds)); +} + +/// `trace` reads two regions, and either one being bad refuses the call. +#[test] +fn both_of_traces_regions_are_checked() { + let host = FakeHost::new(); + + let bad_msg = module( + &[import::TRACE, ONE_PAGE], + &format!( + "(call $trace (i32.const {PAGE}) (i32.const 1) (i32.const 0) (i32.const 1) (i32.const 0))" + ), + ); + assert_eq!(status(&bad_msg, &host), code(HostError::PointerOutOfBounds)); + + let bad_data = module( + &[import::TRACE, ONE_PAGE], + &format!( + "(call $trace (i32.const 0) (i32.const 1) (i32.const {PAGE}) (i32.const 1) (i32.const 0))" + ), + ); + assert_eq!( + status(&bad_data, &host), + code(HostError::PointerOutOfBounds) + ); + assert!(host.traces().is_empty()); +} + +// --------------------------------------------------------------------------- +// Both at once (`read_write`, via `sha512_half`) +// --------------------------------------------------------------------------- + +/// A call with an input and an output region checks the input first, so a bad +/// input is reported even when the output region is also bad. +#[test] +fn a_read_write_checks_its_input_before_its_output() { + let host = FakeHost::new(); + let digest = |src: i64, src_len: i64, dst: i64| { + module( + &[import::SHA512_HALF, ONE_PAGE], + &format!( + "(call $sha512_half (i32.const {src}) (i32.const {src_len}) + (i32.const {dst}) (i32.const {HASH_LEN}))" + ), + ) + }; + + let over_cap = digest(0, OVER_CAP, 0); + assert_eq!(status(&over_cap, &host), code(HostError::DataFieldTooLarge)); + + let out_of_bounds = digest(PAGE, 4, 0); + assert_eq!( + status(&out_of_bounds, &host), + code(HostError::PointerOutOfBounds) + ); + + // A bad input and a bad output: the input's verdict is the one reported. + let both_bad = digest(0, OVER_CAP, PAGE); + assert_eq!(status(&both_bad, &host), code(HostError::DataFieldTooLarge)); + assert!(host.digested.borrow().is_empty(), "the host is not reached"); +} + +/// The output half of a read-write call obeys the same rules as a plain write. +#[test] +fn a_read_write_output_obeys_the_write_rules() { + let host = FakeHost::new().answering_digest(Answer::filler(32)); + + let wat = module( + &[import::SHA512_HALF, ONE_PAGE], + "(call $sha512_half (i32.const 0) (i32.const 4) (i32.const 0) (i32.const 31))", + ); + assert_eq!(status(&wat, &host), code(HostError::BufferTooSmall)); + + let wat = module( + &[import::SHA512_HALF, ONE_PAGE], + &format!( + "(call $sha512_half (i32.const 0) (i32.const 4) (i32.const {PAGE}) (i32.const 32))" + ), + ); + assert_eq!(status(&wat, &host), code(HostError::PointerOutOfBounds)); +} + +/// An input region may overlap the output region: the engine copies the input out +/// of guest memory before the host writes back into it. The marker is any byte +/// distinct from the input's first (`a`), so `finish` returning it proves the write +/// landed. +#[test] +fn an_input_may_overlap_the_output() { + const MARKER: u8 = 99; + + let host = FakeHost::new().answering_digest(Answer::bytes([MARKER; HASH_LEN])); + + let wat = module( + &[ + import::SHA512_HALF, + ONE_PAGE, + r#"(data (i32.const 0) "abcd")"#, + ], + &format!( + "(drop (call $sha512_half (i32.const 0) (i32.const 4) + (i32.const 0) (i32.const {HASH_LEN}))) + (i32.load8_u (i32.const 0))" + ), + ); + assert_eq!( + status(&wat, &host), + i32::from(MARKER), + "the output overwrote the input" + ); + assert_eq!( + *host.digested.borrow(), + vec![b"abcd".to_vec()], + "the host saw the input as it was" + ); +} + +// --------------------------------------------------------------------------- +// The memory export itself +// --------------------------------------------------------------------------- + +/// Every region is relative to the guest's exported memory, so a module without +/// one cannot make a host call at all. +#[test] +fn a_module_that_exports_no_memory_cannot_call_the_host() { + let host = FakeHost::new(); + + let wat = module( + &[import::LDGR_INDEX, "(memory 1)"], + "(call $ldgr_index (i32.const 0) (i32.const 4))", + ); + assert_eq!(status(&wat, &host), code(HostError::NoMemExported)); +} + +/// The export has to be named `memory`, and it has to *be* a memory — a global +/// under that name is not a near miss the engine tolerates. +#[test] +fn the_memory_export_must_be_a_memory_named_memory() { + let host = FakeHost::new(); + + // The right kind under the wrong name. + let misnamed = module( + &[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)); + + // The right name on the wrong kind, which is the other arm of the match. + let wrong_kind = module( + &[ + import::LDGR_INDEX, + "(memory 1)", + r#"(global (export "memory") i32 (i32.const 0))"#, + ], + "(call $ldgr_index (i32.const 0) (i32.const 4))", + ); + assert_eq!(status(&wrong_kind, &host), code(HostError::NoMemExported)); +} + +/// Bounds follow the memory the module actually declared, not a fixed page. +#[test] +fn bounds_follow_the_declared_memory_size() { + let host = FakeHost::new(); + + let wat = module( + &[import::LDGR_INDEX, r#"(memory (export "memory") 2)"#], + &format!("(call $ldgr_index (i32.const {PAGE}) (i32.const 4))"), + ); + assert_eq!(status(&wat, &host), 4, "the second page is in bounds"); +} diff --git a/crates/xrpl-wasm-vm/tests/support/mod.rs b/crates/xrpl-wasm-vm/tests/support/mod.rs new file mode 100644 index 0000000000..e58caa0b3a --- /dev/null +++ b/crates/xrpl-wasm-vm/tests/support/mod.rs @@ -0,0 +1,268 @@ +//! Shared scaffolding for the integration tests: a host whose every answer the +//! test sets, and the pieces of a wasm module to run against it. +//! +//! `abi.rs`'s guest-memory marshaling is reachable only from a live host call, so +//! each test assembles the smallest module that exercises one rule and reads the +//! verdict out of `finish`'s return value. + +#![allow(dead_code)] // Each test binary uses a different part of this module. + +use std::cell::RefCell; +use std::collections::HashMap; + +use xrpl_host_functions::{HostError, HostFunctions, HostResult}; +use xrpl_wasm_vm::RunOutcome; + +/// The entry point every test module exports. +pub const ENTRY: &str = "finish"; + +/// Gas for a test that is not about gas: enough that nothing runs out. +pub const PLENTY_OF_GAS: u64 = 100_000_000; + +// --------------------------------------------------------------------------- +// The fake host +// --------------------------------------------------------------------------- + +/// What the host does when asked for a value. +#[derive(Clone, Debug)] +pub enum Answer { + /// Writes `bytes` into the output region if they fit, and reports `len` as + /// the true length either way. `len` is separate from `bytes.len()` so a + /// test can reach the over-cap and buffer-fit rules without a value that + /// large. + Value { bytes: Vec, len: usize }, + /// Fails without touching the output region. + Fail(HostError), +} + +impl Answer { + /// Writes `bytes` and reports their true length. + pub fn bytes(bytes: impl Into>) -> Answer { + let bytes = bytes.into(); + Answer::Value { + len: bytes.len(), + bytes, + } + } + + /// Writes nothing and claims a value of `len` bytes. It under-writes relative + /// to a real host, which writes whenever the value fits `out`, so a test about + /// what lands in guest memory wants [`Answer::bytes`] instead. + pub fn claiming(len: usize) -> Answer { + Answer::Value { + bytes: Vec::new(), + len, + } + } + + /// `len` bytes counting up from 0, written and reported. + pub fn filler(len: usize) -> Answer { + Answer::bytes((0..len).map(|i| i as u8).collect::>()) + } + + fn fill(&self, out: &mut [u8]) -> HostResult { + match self { + Answer::Value { bytes, len } => { + if bytes.len() <= out.len() { + out[..bytes.len()].copy_from_slice(bytes); + } + Ok(*len) + } + Answer::Fail(error) => Err(*error), + } + } +} + +/// One `trace` or `trace_num` call, as the host received it. +#[derive(Clone, Debug, PartialEq, Eq)] +pub enum Trace { + Message { + msg: String, + data: Vec, + as_hex: bool, + }, + Number { + msg: String, + number: i64, + }, +} + +/// A `HostFunctions` implementation that answers from what the test put in it and +/// records what it was asked. The ABI's receiver is `&self`, so the recording goes +/// behind `RefCell`, as a real mutating host's would. +pub struct FakeHost { + /// What `get_ledger_sqn` answers. + pub ledger_sqn: Answer, + /// What `get_current_ledger_obj_field` answers, by field selector. An + /// unlisted selector answers `FieldNotFound`. + pub fields: HashMap, + /// What `sha512_half` answers, whatever it is given. + pub digest: Answer, + /// Every field selector `get_current_ledger_obj_field` was asked for. + pub fields_asked: RefCell>, + /// Every input `sha512_half` was given. + pub digested: RefCell>>, + /// Every `trace`/`trace_num` call, in order. + pub traces: RefCell>, +} + +impl Default for FakeHost { + fn default() -> FakeHost { + FakeHost { + // 4 little-endian bytes, as the declaration's doc comment specifies. + ledger_sqn: Answer::bytes(7u32.to_le_bytes()), + fields: HashMap::new(), + digest: Answer::filler(32), + fields_asked: RefCell::new(Vec::new()), + digested: RefCell::new(Vec::new()), + traces: RefCell::new(Vec::new()), + } + } +} + +impl FakeHost { + pub fn new() -> FakeHost { + FakeHost::default() + } + + pub fn answering_sqn(mut self, answer: Answer) -> FakeHost { + self.ledger_sqn = answer; + self + } + + pub fn answering_field(mut self, field: i32, answer: Answer) -> FakeHost { + self.fields.insert(field, answer); + self + } + + pub fn answering_digest(mut self, answer: Answer) -> FakeHost { + self.digest = answer; + self + } + + pub fn traces(&self) -> Vec { + self.traces.borrow().clone() + } +} + +impl HostFunctions for FakeHost { + fn get_ledger_sqn(&self, out: &mut [u8]) -> HostResult { + self.ledger_sqn.fill(out) + } + + fn get_current_ledger_obj_field(&self, field: i32, out: &mut [u8]) -> HostResult { + self.fields_asked.borrow_mut().push(field); + match self.fields.get(&field) { + Some(answer) => answer.fill(out), + None => Err(HostError::FieldNotFound), + } + } + + fn sha512_half(&self, data: &[u8], out: &mut [u8]) -> HostResult { + self.digested.borrow_mut().push(data.to_vec()); + self.digest.fill(out) + } + + fn trace(&self, msg: &str, data: &[u8], as_hex: bool) -> HostResult<()> { + self.traces.borrow_mut().push(Trace::Message { + msg: msg.to_owned(), + data: data.to_vec(), + as_hex, + }); + Ok(()) + } + + fn trace_num(&self, msg: &str, number: i64) -> HostResult<()> { + self.traces.borrow_mut().push(Trace::Number { + msg: msg.to_owned(), + number, + }); + Ok(()) + } +} + +// --------------------------------------------------------------------------- +// 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. +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)))"#; + pub const TRACE: &str = + r#"(import "host" "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)))"#; +} + +/// One page of linear memory, exported under the name the engine looks for. +pub const ONE_PAGE: &str = r#"(memory (export "memory") 1)"#; + +/// A module of `parts`, wrapping `body` in an exported `finish` returning `i32`. +pub fn module(parts: &[&str], body: &str) -> String { + format!( + "(module {parts}\n (func (export \"{ENTRY}\") (result i32)\n {body}))", + parts = parts.join("\n ") + ) +} + +// --------------------------------------------------------------------------- +// Running +// +// The tests write their modules as text and assemble them here: the VM takes +// binaries only, so the crate builds `wasmi` without its `wat` feature. +// --------------------------------------------------------------------------- + +/// Assembles a text-format module into the binary the VM takes. +/// +/// Panics rather than returning an error: text that will not assemble is a +/// mistake in the test, not a case under test. +pub fn assemble(wat: &str) -> Vec { + wat::parse_str(wat) + .unwrap_or_else(|e| panic!("this test's module does not assemble: {e}\n{wat}")) +} + +/// Runs `wat`'s `finish` against `host` with gas to spare. +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 { + 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 { + xrpl_wasm_vm::run(&assemble(wat), PLENTY_OF_GAS, host, entry) +} + +/// The value `finish` returned, for a run expected to complete: the host call's +/// status, so a byte count on success or a negative [`HostError`] code. +pub fn status(wat: &str, host: &FakeHost) -> i32 { + run(wat, host) + .unwrap_or_else(|e| panic!("expected the module to run, but: {e}\n{wat}")) + .result +} + +/// The wire code a `HostError` reaches the guest as, for readable assertions. +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 { + match run(wat, host) { + Err(message) => message, + 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 new file mode 100644 index 0000000000..492ba47a59 --- /dev/null +++ b/crates/xrpl-wasm-vm/tests/vm_limits.rs @@ -0,0 +1,423 @@ +//! What the engine refuses outright: modules it will not compile, will not +//! instantiate, or cannot find an entry point in — plus the linear-memory cap. +//! +//! These are the sandbox's outer wall. Everything here fails the run rather than +//! returning a code to the guest, so each test reads the failure's message. + +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; + +/// 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}" + ); +} + +// --------------------------------------------------------------------------- +// Linear memory +// --------------------------------------------------------------------------- + +/// A module declaring more than the cap fails to instantiate — the limit applies +/// to the initial memory, not only to growth. +#[test] +fn an_initial_memory_past_the_cap_is_refused() { + let host = FakeHost::new(); + + let wat = module( + &[&format!( + r#"(memory (export "memory") {})"#, + MAX_MEMORY_PAGES + 1 + )], + "(i32.const 0)", + ); + assert_stage(&failure(&wat, &host), "instantiate"); +} + +/// The cap itself is allowed. +#[test] +fn an_initial_memory_at_the_cap_is_allowed() { + let host = FakeHost::new(); + + let wat = module( + &[&format!(r#"(memory (export "memory") {MAX_MEMORY_PAGES})"#)], + "(i32.const 0)", + ); + assert_eq!(run(&wat, &host).expect("should run").result, 0); +} + +/// Growth up to the cap succeeds; growth past it traps rather than answering -1 as +/// `memory.grow` otherwise would, because the engine's limiter sets +/// `trap_on_grow_failure(true)`. +#[test] +fn growth_stops_at_the_cap() { + let host = FakeHost::new(); + + let wat = module( + &[ONE_PAGE], + &format!("(memory.grow (i32.const {}))", MAX_MEMORY_PAGES - 1), + ); + assert_eq!( + run(&wat, &host).expect("should run").result, + 1, + "growing to exactly the cap answers the previous size" + ); + + let wat = module( + &[ONE_PAGE], + &format!("(memory.grow (i32.const {MAX_MEMORY_PAGES}))"), + ); + assert_stage(&failure(&wat, &host), "trap"); +} + +/// A module may declare a maximum above the cap: the cap is enforced on the initial +/// memory and on growth, not on the memory type's declared bound. +#[test] +fn a_declared_maximum_past_the_cap_is_allowed_but_unreachable() { + let host = FakeHost::new(); + let memory = format!(r#"(memory (export "memory") 1 {})"#, MAX_MEMORY_PAGES + 1); + + let wat = module(&[&memory], "(i32.const 0)"); + assert_eq!(run(&wat, &host).expect("should run").result, 0); + + let wat = module( + &[&memory], + &format!("(memory.grow (i32.const {MAX_MEMORY_PAGES}))"), + ); + assert_stage(&failure(&wat, &host), "trap"); +} + +// --------------------------------------------------------------------------- +// Engine configuration +// --------------------------------------------------------------------------- + +/// One row per feature `build_wasm_engine` turns off: the smallest module that uses +/// it, and the fragment of wasmi's refusal that names the feature. A row declaring +/// its own memory omits [`ONE_PAGE`], or it is refused for having two memories +/// instead. +fn disabled_features() -> Vec<(&'static str, Vec<&'static str>, &'static str, &'static str)> { + vec![ + ( + "wasm_multi_value", + vec![ + ONE_PAGE, + "(func $two (result i32 i32) (i32.const 1) (i32.const 2))", + ], + "(call $two) (drop) (drop) (i32.const 0)", + "multi-value", + ), + ( + "wasm_sign_extension", + vec![ONE_PAGE], + "(i32.extend8_s (i32.const 1))", + "sign extension", + ), + ( + "wasm_bulk_memory", + vec![ONE_PAGE], + "(memory.fill (i32.const 0) (i32.const 0) (i32.const 1)) (i32.const 0)", + "bulk memory", + ), + ( + "wasm_reference_types", + vec![ONE_PAGE, "(table 1 externref)"], + "(i32.const 0)", + "reference types", + ), + // The proposal covers mutable globals crossing the module boundary; an + // internal one is core wasm and stays allowed — see the test below. + ( + "wasm_mutable_global", + vec![ONE_PAGE, r#"(global (export "g") (mut i32) (i32.const 0))"#], + "(i32.const 0)", + "mutable global", + ), + ( + "wasm_tail_call", + vec![ONE_PAGE, "(func $f (result i32) (i32.const 0))"], + "(return_call $f)", + "tail call", + ), + // Arithmetic in a constant initialiser. wasmi names the operator rather + // than the proposal here. + ( + "wasm_extended_const", + vec![ + ONE_PAGE, + "(global $g i32 (i32.add (i32.const 1) (i32.const 2)))", + ], + "(global.get $g)", + "non-constant operator", + ), + ( + "wasm_multi_memory", + vec![ONE_PAGE, "(memory 1)"], + "(i32.const 0)", + "multiple memories", + ), + ( + "wasm_memory64", + vec![r#"(memory (export "memory") i64 1)"#], + "(i32.const 0)", + "memory64", + ), + ( + "wasm_custom_page_sizes", + vec![r#"(memory (export "memory") 1 (pagesize 1))"#], + "(i32.const 0)", + "custom page sizes", + ), + ( + "wasm_wide_arithmetic", + vec![ONE_PAGE], + "(drop (i64.add128 (i64.const 1) (i64.const 2) (i64.const 3) (i64.const 4))) + (i32.const 0)", + "wide arithmetic", + ), + // Determinism across nodes is the reason floats are off. + ( + "floats", + vec![ONE_PAGE], + "(drop (f64.add (f64.const 1) (f64.const 2))) (i32.const 0)", + "floating-point", + ), + ] +} + +/// Every feature the engine disables is refused, and refused for that reason. +/// +/// `wasm_custom_page_sizes` and `wasm_wide_arithmetic` are off by default in wasmi +/// 1.1 (`engine/config.rs:72,74`), so their rows guard against wasmi changing that +/// default rather than against our own config. +#[test] +fn every_disabled_feature_is_refused_by_name() { + let host = FakeHost::new(); + + for (knob, parts, body, expected) in disabled_features() { + let wat = module(&parts, body); + let failure = failure(&wat, &host); + + assert_stage(&failure, "compile"); + assert!( + failure.contains(expected), + "{knob}: expected a refusal mentioning {expected:?}, got: {failure}" + ); + } +} + +/// The three knobs [`every_disabled_feature_is_refused_by_name`] cannot cover. The +/// engine is a process-wide `LazyLock`, so a test observes the one configuration we +/// build: a knob masked by another, or with no caller-visible effect, has no +/// distinguishing module. +#[test] +fn the_knobs_without_a_module_of_their_own() { + let host = FakeHost::new(); + + // `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); + assert!(refusal.contains("floating-point"), "{refusal}"); + assert!(!refusal.contains("saturating"), "{refusal}"); + + // `ignore_custom_sections(true)`: governs whether wasmi retains custom + // sections, not accept/reject, so this pins only that one is harmless. + let wat = module( + &[ONE_PAGE, r#"(@custom "note" "ignored")"#], + "(i32.const 0)", + ); + assert_eq!(run(&wat, &host).expect("should run").result, 0); + + // `consume_fuel(true)`: with it off, `Store::set_fuel` fails and `run` returns + // before instantiating, so every test in the suite fails. + let wat = module(&[ONE_PAGE], "(i32.const 0)"); + assert!(run(&wat, &host).expect("should run").fuel_used > 0); +} + +/// A mutable global the module keeps to itself is core wasm, so the disabled +/// proposal does not reach it: a guest can still have mutable state. +#[test] +fn an_internal_mutable_global_is_still_allowed() { + let host = FakeHost::new(); + + let wat = module( + &[ONE_PAGE, "(global $g (mut i32) (i32.const 0))"], + "(global.set $g (i32.const 7)) (global.get $g)", + ); + assert_eq!(run(&wat, &host).expect("should run").result, 7); +} + +/// Bytes that are not a wasm module at all. +#[test] +fn garbage_does_not_compile() { + let host = FakeHost::new(); + + 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"); + } +} + +/// The VM takes wasm binaries, and text is not one. wasmi's `wat` feature is on by +/// default and would have `Module::new` assemble text too, so the crate builds +/// wasmi without it; turning it back on would make this transaction blob valid. +#[test] +fn the_vm_refuses_a_text_format_module() { + let host = FakeHost::new(); + let text = module(&[ONE_PAGE], "(i32.const 0)"); + + 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"); + + // The same module, assembled first, runs: the text is sound and only the + // format was refused. + assert_eq!(run(&text, &host).expect("should run").result, 0); +} + +// --------------------------------------------------------------------------- +// Imports +// --------------------------------------------------------------------------- + +/// A module may import fewer host functions than are registered, but not more: +/// an import the linker does not define fails instantiation. +#[test] +fn an_unknown_import_fails_instantiation() { + let host = FakeHost::new(); + + let wat = module( + &[ + r#"(import "host" "no_such_function" (func $f (param i32) (result i32)))"#, + ONE_PAGE, + ], + "(call $f (i32.const 0))", + ); + assert_stage(&failure(&wat, &host), "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`. +#[test] +fn the_import_module_name_must_match() { + let host = FakeHost::new(); + + for module_name in ["host_lib", "env", ""] { + let wat = module( + &[ + &format!( + r#"(import "{module_name}" "ldgr_index" (func $f (param i32 i32) (result i32)))"# + ), + ONE_PAGE, + ], + "(call $f (i32.const 0) (i32.const 4))", + ); + assert_stage(&failure(&wat, &host), "instantiate"); + } +} + +/// An import spelled with the wrong signature does not link even under the right +/// name, which is what makes the registered signatures load-bearing. +#[test] +fn an_import_with_the_wrong_signature_fails_instantiation() { + let host = FakeHost::new(); + + for signature in [ + "(param i32) (result i32)", // too few parameters + "(param i32 i32 i32) (result i32)", // too many + "(param i64 i64) (result i32)", // wrong parameter types + "(param i32 i32) (result i64)", // wrong result type + "(param i32 i32)", // no result + ] { + let wat = module( + &[ + &format!(r#"(import "host" "ldgr_index" (func $f {signature}))"#), + ONE_PAGE, + ], + "(i32.const 0)", + ); + assert_stage(&failure(&wat, &host), "instantiate"); + } +} + +/// A module that imports a host function it never calls still has to link. +#[test] +fn an_unused_import_is_still_linked() { + let host = FakeHost::new(); + + let wat = module( + &[import::LDGR_INDEX, import::TRACE, ONE_PAGE], + "(i32.const 0)", + ); + assert_eq!(run(&wat, &host).expect("should run").result, 0); +} + +// --------------------------------------------------------------------------- +// The entry point +// --------------------------------------------------------------------------- + +#[test] +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}"); +} + +/// The entry point is looked up by the name the caller asks for. +#[test] +fn the_entry_point_is_the_name_the_caller_gives() { + let host = FakeHost::new(); + + let wat = r#"(module (memory (export "memory") 1) (func (export "other") (result i32) (i32.const 9)))"#; + let outcome = run_entry(wat, &host, "other").expect("should run"); + 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`. +#[test] +fn an_entry_point_of_the_wrong_type_fails() { + let host = FakeHost::new(); + + for signature in ["(result i64)", "(param i32) (result i32)", ""] { + let body = if signature.contains("result i64") { + "(i64.const 0)" + } else if signature.is_empty() { + "(nop)" + } else { + "(i32.const 0)" + }; + 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"); + assert!(failure.contains("no entry point"), "{signature}: {failure}"); + } +} + +/// A guest that traps fails the run rather than returning a value. +#[test] +fn a_trapping_guest_fails_the_run() { + let host = FakeHost::new(); + + let wat = module(&[ONE_PAGE], "(unreachable)"); + assert_stage(&failure(&wat, &host), "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"); +} diff --git a/docs/claude/redesign_impl.md b/docs/claude/redesign_impl.md index f22622cc1a..0ae5322fb5 100644 --- a/docs/claude/redesign_impl.md +++ b/docs/claude/redesign_impl.md @@ -383,6 +383,134 @@ long name `get_ledger_sqn` where the code registered `ldgr_index`, and it refere `detail/WasmVM.cpp`, `detail/HostFuncWrapper.cpp` and `ParamsHelper.h`, none of which exist (the helper is `WasmImportsHelper.h`). +## `xrpl-wasm-vm` review findings (2026-07-29) + +A read of all three files (`vm.rs`, `abi.rs`, `register.rs`) against the vendored +wasmi 1.1.0 source. Grouped by kind and ordered within each group by how much they +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 + `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 + the current basic block before it traps. The stopping point is a function of + wasmi's block layout — implementation-defined behaviour on a consensus path. + XLS-0102 requires immediate halting and C++ trapped (`hfErrOutOfGas`); `-22` is + also outside the range the SDK's `transmute` accepts (open question 3). Fix is the + two-channel design: host-fatal errors (`OutOfGas`, `Internal`, `NoMemExported`) + 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 + 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 + 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). +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 + slice/string reads (`trace` msg/data, `sha512_half` input — see "Reference points" + below). The charge also precedes the bounds check, so a guest can drain the 1 MiB + budget with out-of-bounds pointers. Related, in `write_into`: `fill` gets a slice + of the guest's full `cap`, uncapped by `MAX_WASM_DATA_LEN`, so an over-cap value + lands in guest memory before `n > MAX_WASM_DATA_LEN` rejects it — clamping `out` to + `min(cap, MAX_WASM_DATA_LEN)` makes that post-check unreachable by construction. +5. ✓ **`Module::new` accepted WAT text — a behaviour the rewrite introduced by + accident.** wasmi's default features include `wat`, and `Module::new` runs + `wat::parse_bytes` over its input (`module/mod.rs:228`), so the VM compiled + text-format modules straight from a transaction blob and `wat`/`wast`/`bumpalo` sat + in the release build. + + **The C++ path did not do this**, and the reason is worth recording, because it is + the whole finding. `ModuleWrapper::init` called `wasm_module_new` with the raw + transaction bytes (`WasmiVM.cpp:314-318` at `b7059deb9f^`), which wraps + `Module::new` (`crates/c_api/src/module.rs:54` of the `wasmi/1.0.9` conan package). + wasmi 1.0.9 carries the *same* `#[cfg(feature = "wat")]` parse and the same + `default = ["std", "wat"]` — but the C-API crate takes wasmi with + `default-features = false` (wasmi workspace `Cargo.toml:34`) and never re-enables + `wat` (`wasmi_c_api_impl` has only `std`, `prefix-symbols`, `simd`). So that line was + compiled out of the C++ build, and the C-API exposes no wat2wasm entry point either — + unlike wasmtime's, `wasmi.h` has nothing of the kind. Binary only, and no mention of + WAT anywhere in the deleted C++ wasm sources. + + Linking the wasmi *Rust* crate directly is what picked the default up: the feature + the C-API had already turned off upstream came back silently. `default-features = + false, features = ["std"]` restores parity — it is not a new policy. (The secondary + argument still holds: it also keeps a module's validity a protocol rule rather than a + function of a cargo flag.) The tests assemble text themselves from a dev-dependency, + so nothing of ours is needed to keep them working — see "Build / test loop". + +### B. Dead weight — pure simplification, no behaviour change + +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. +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 + too is gone", "Unchanged from the original skeleton"), against the + no-historical-comments convention. `#![deny(rustdoc::broken_intra_doc_links)]` + stops the links from rotting again. + +### C. Performance + +10. **The `"memory"` export is a string hash lookup on every host call.** `memory()` + (`abi.rs:104`) → `Caller::get_export` → `InstanceEntity::exports: Map, + Extern>`. Resolve it once after instantiation and keep the `Memory` in `VmState`. + Two bonuses: `NoMemExported` becomes an instantiation-time error, where it + belongs, and a per-call failure path disappears. Cheapest real win in the crate, + and the benchmark can measure it. +11. `read_write` memsets 1 KiB of stack per call and does not generalize past one byte + input — that is the scratch-buffer decision already open above. #10 makes either + choice easier. +12. `Linker` is rebuilt per `run` (five `func_wrap`s plus string interning) and the + module is compiled per run with no cache. Lower priority. The blocker worth + recording: `VmState<'h>`'s lifetime forces `Linker>` to be per-run. + +### D. Hardening + +13. ✓ **The public surface was accidental.** `lib.rs` was `pub use vm::run` alone, so + `RunOutcome` was `pub` inside a private module and unreachable: a caller could + invoke `run` but not name its return type, and `MAX_MEMORY_PAGES` / + `TRANSFER_LIMIT_BYTES` / `MAX_MEMORY_BYTES` were likewise unreachable. Exported + with the test work, since the tests need to name them. + + The 1 KiB per-field cap was a further case, and the odd one out: three of the four + protocol limits lived in `vm.rs` and were `pub`, while this one sat private in + `abi.rs` as `MAX_WASM_DATA_LEN`. Being unreachable is why the tests had restated + `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 + slice op; let the compiler enforce the claim. Plus `unreachable_pub` and clippy's + cast lints. +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. +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 + `limiter` are both installed *before* `instantiate_and_start`, so start-section + work is already metered and memory-capped. Recorded because the TODO reads like an + open hole and is closer to a preference. + ## Reference points from the deleted C++ path Import names and gas costs are ABI; the rest below is *evidence of prior behaviour*, @@ -417,6 +545,25 @@ useful for comparison and for the gas assertions in `Wasm_test.cpp` — not gosp - Fast: `cd crates && cargo check --workspace --all-targets`, `cargo test --workspace`, `cargo clippy --workspace --all-targets`. +- `xrpl-wasm-vm`'s tests come in two kinds, and the split is forced rather than + stylistic. A wasmi `Caller` exists only for the duration of a host call, so + `read_borrowed` / `write_into` / `read_write` / `memory` **cannot be reached from a + unit test**. The unit tests in `src/` therefore cover only what needs no live + instance (the wire conversions, the transfer-budget arithmetic, the limits), and the + guest-memory policy is covered by integration tests in `tests/`, which run real + modules against a configurable fake host. +- Those integration tests write their modules as **WAT text** and assemble it + themselves — `wat` is a plain `[dev-dependencies]` entry and `support::assemble` is the + only caller, so the assembler never enters the library. `run` takes binaries; there is + no `run_wat` and no cargo feature for one. What makes that hold is `wasmi = { + default-features = false, features = ["std"] }`: wasmi's `wat` feature is **on by + default** and makes `Module::new` accept text as readily as binary (finding A5), which + would put the text assembler in the consensus path and make a transaction's validity a + build flag. `the_vm_refuses_a_text_format_module` in `vm_limits.rs` is what catches + that feature coming back. +- `tests/support/mod.rs` holds the fake host and the import declarations. `Answer` + separates *what the host writes* from *what length it reports*, which is what makes + the over-cap and buffer-fit rules testable without values that large existing. - Guest-linkability of the ABI crate (needs `rustup target add wasm32-unknown-unknown`): `cargo check -p xrpl-host-functions --target wasm32-unknown-unknown`. Worth keeping green — the guest stdlib links this crate, so a `std`/`alloc`/dependency creep here @@ -428,8 +575,82 @@ useful for comparison and for the gas assertions in `Wasm_test.cpp` — not gosp ## Current state (2026-07-29) **`crates/` compiles**, and the whole workspace is green — `cargo test --workspace`, -`clippy --workspace --all-targets`, `fmt`. 33 macro tests, 9 facade tests, 1 doctest; -`xrpl-wasm-vm` has no tests of its own yet. +`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`). + +**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 +row of the old `the_disabled_proposals_do_not_compile` left a stray value on the wasm +stack, so the module was refused as a *type error* rather than for the proposal, and +`config.wasm_multi_value(false)` could be deleted with the whole suite still green. The +general lesson — a stage-only `assert_stage(…, "compile")` cannot tell "refused for the +reason under test" from "my wasm was malformed" — is now the design of +`every_disabled_feature_is_refused_by_name`: one row per disabled feature, each +asserting the *fragment of wasmi's message that names the feature*. Verified by deleting +each knob in turn: ten of twelve rows fail when their knob goes. The two that don't are +`wasm_custom_page_sizes` and `wasm_wide_arithmetic`, which wasmi 1.1 already defaults to +off (`engine/config.rs:72,74`), so those calls are redundant and no test can notice them +going — their rows guard against wasmi changing that default instead. + +Three knobs have no module of their own, and `the_knobs_without_a_module_of_their_own` +records why rather than leaving it to a comment: `wasm_saturating_float_to_int` is masked +by `floats(false)` (every saturating conversion takes a float operand, and the test +asserts the message proves which knob answered); `ignore_custom_sections` is not +observable through accept/reject at all, since a module carrying a custom section +compiles either way; `consume_fuel` is covered by construction, because with it off +`Store::set_fuel` fails and every test in the suite breaks. + +The other findings acted on: `MAX_MEMORY_PAGES` had **no** golden pin (it could be halved +with nothing failing), so `the_limits_are_the_protocol_limits` in `vm.rs` now pins all +four limits against their `Protocol.h` names, and the misnamed +`the_field_cap_is_far_below_the_run_budget` became the inequality its name promised. +`generated_abi.rs`'s "one place for literals" claim was false twice in its own file — the +subsumed name list and a restated `500` are gone. And `Answer::claiming` writes nothing, +which hid finding A4's actual hazard: `an_over_cap_value_is_written_before_it_is_refused` +now uses a real over-cap value and shows the bytes reaching guest memory before the +refusal. Verified by applying the `min(cap, MAX_FIELD_BYTES)` clamp — the test flips, as +its comment says it should. + +Those 65 are review finding 15, closed: the bounds / field-cap / buffer-fit / gas / +transfer policy now has a net under it, which is what the rest of the findings need +before they can be acted on. Writing them turned up things reading the code did not: + +- **wasmi parses WAT by default** (finding A5), so the VM compiled text-format modules + straight from a transaction blob. The C++ path did not — its C-API took wasmi with + `default-features = false` — so this was an accidental behaviour change, not a choice. + Fixed, and back at parity. +- `wasm_mutable_global(false)` does **not** forbid a guest's own mutable globals — the + proposal is about mutable globals crossing the module boundary. An internal one is + core wasm and still compiles. +- A declared memory *maximum* above the 128-page cap instantiates fine; only the size + actually reached is capped. And growth past the cap **traps** rather than answering + `-1`, because the limiter is built with `trap_on_grow_failure(true)`. +- The two directions check in opposite orders, observably: an over-long *input* reports + `DataFieldTooLarge` (the cap precedes the bounds check) while an over-long *output* + reports `PointerOutOfBounds` (bounds precede the cap). +- wasmi's guest-side fuel for a host call is exactly `14 × operands + 1` (29/43/57/71 + for 2/3/4/5 operands, across all five functions; operand *type* is irrelevant — an + `i64` costs what an `i32` does). With that and the 30-fuel empty-module floor known, a + one-call run's total is known to the unit, so `a_host_call_costs_its_gas_every_time_it_is_called` + asserts each function's charge directly rather than by differencing. + +**Where the gas numbers live.** `the_spec_table_matches_the_declarations` in the ABI +crate's `generated_abi.rs` is the **one** place wire names and gas costs appear as +literals, as a whole-table comparison — a deliberate change-detector on consensus input, +which also pins `ALL`'s order and membership. Everything else reads +`HostFunctionSpec::gas()`. That split matters because the two properties are different: +*what the table says* is the ABI crate's business, while *whether the engine charges the +row the table holds* is the VM's. Verified by mutating `#[gas = 70]` to `71` — exactly +one test fails, the table one, and the VM's fuel tests follow the new value. Before the +split the VM restated all five values, so a legitimate gas change meant editing three +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. The trait is settled, and every part of it is written in the declaration rather than synthesized: `&self`, `HostResult`, and byte outputs as explicit @@ -442,11 +663,12 @@ Consequences worth remembering: `read_write` already took `FnOnce(&dyn HostFunctions, …, &mut [u8]) -> HostResult`. - 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". -- `xrpl-wasm-vm` has **no tests**, so nothing would catch a mistake in `abi.rs`'s - bounds/cap/transfer policy. That is the gap to close before refactoring it. -Next, in rough order: the scratch-buffer decision, then real `ApplyContext` wiring and -the cxx bridge (`xrpl-wasm-vm-ffi` is still `mod ffi {}`). Deferred as before: +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. Deferred to a later refactor, once there is working code: macro-emitted `link_*`