From 047a3f5cb8aa24e9a32b87ad9826f9458d640d50 Mon Sep 17 00:00:00 2001 From: Sergey Kuznetsov Date: Mon, 3 Aug 2026 14:05:21 +0100 Subject: [PATCH] Update docs --- crates/xrpl-wasm-vm/src/abi.rs | 262 +++++---------------- crates/xrpl-wasm-vm/src/lib.rs | 11 +- crates/xrpl-wasm-vm/src/register.rs | 41 +--- crates/xrpl-wasm-vm/src/vm.rs | 203 +++++----------- crates/xrpl-wasm-vm/tests/budgets.rs | 10 +- crates/xrpl-wasm-vm/tests/memory_policy.rs | 13 +- crates/xrpl-wasm-vm/tests/vm_limits.rs | 7 +- docs/claude/redesign_impl.md | 78 ++++-- 8 files changed, 200 insertions(+), 425 deletions(-) diff --git a/crates/xrpl-wasm-vm/src/abi.rs b/crates/xrpl-wasm-vm/src/abi.rs index e579d41f04..94b3ad6540 100644 --- a/crates/xrpl-wasm-vm/src/abi.rs +++ b/crates/xrpl-wasm-vm/src/abi.rs @@ -2,42 +2,18 @@ use crate::vm::{MAX_FIELD_BYTES, VmState}; use wasmi::{Caller, Memory}; use xrpl_host_functions::{HostError, HostFunctionSpec, HostFunctions, HostResult}; -// --------------------------------------------------------------------------- -// ABI marshaling: charge a call's gas at one point (`charged`) so every -// registered closure pays for itself exactly once, and hand the result to the -// engine on one of the two channels a host call answers on — a return code the -// guest reads, or a trap it cannot observe. -// --------------------------------------------------------------------------- - -/// A host-fatal [`HostError`] on its way out of a host call as a wasmi trap. -/// -/// wasmi takes an arbitrary payload out of a host function as long as it -/// implements `wasmi::errors::HostError`, a trait with no methods and no blanket -/// impl. Carrying the `HostError` itself is what lets [`crate::vm::run`] name the -/// condition with `downcast_ref` rather than string-comparing a message, as the -/// C++ path did with its `hfErrOutOfGas` trap strings. #[derive(Debug, Clone, Copy, PartialEq, Eq)] pub(crate) struct FatalHostError(pub(crate) HostError); impl wasmi::errors::HostError for FatalHostError {} impl core::fmt::Display for FatalHostError { - /// A fixed prefix and the variant's name. wasmi folds this text into its own - /// `Error`'s `Display`, which is the only place it surfaces, so keep it - /// stable and greppable. fn fmt(&self, f: &mut core::fmt::Formatter<'_>) -> core::fmt::Result { write!(f, "host call refused: {:?}", self.0) } } -/// Whether a [`HostError`] is host-fatal: the host could not serve the call at -/// all, so the guest is stopped where it stands rather than handed a code it may -/// ignore. Everything else is guest-visible — `OutOfTransferLimit` included, -/// which was the one soft failure in C++ too. -/// -/// Spelled variant by variant rather than as a range over `code()`, so which -/// channel a `HostError` added to the ABI later takes is a choice someone makes -/// here rather than one its number makes for it. +/// Whether a [`HostError`] stops the run instead of reaching the guest as a code. pub(crate) fn is_fatal(error: HostError) -> bool { matches!( error, @@ -45,9 +21,8 @@ pub(crate) fn is_fatal(error: HostError) -> bool { ) } -/// Charge a host call's gas from its spec, run its body, and hand the result to -/// the engine. Every registered closure goes through here, so gas cannot be -/// forgotten. +/// Charge the call's gas, run its body, put the result on the wire. The one path +/// every registered closure takes, so gas cannot be forgotten. pub(crate) fn charged( caller: &mut Caller<'_, VmState<'_>>, op: HostFunctionSpec, @@ -56,14 +31,6 @@ pub(crate) fn charged( to_wire(charge(caller, op.gas()).and_then(|()| body(caller))) } -/// Put a host-function result on one of the two channels a host call answers on. -/// -/// A value, or an error the guest is meant to act on, is the `i32` the wasm -/// function returns: `>= 0` a value, `< 0` a [`HostError`] code. A host-fatal -/// error ([`is_fatal`]) leaves as a `wasmi::Error` instead, unwinding the guest -/// at the call — C++ threw for exactly these, and a guest handed `OutOfGas` as a -/// code runs on to the end of its current basic block, a stopping point wasmi's -/// `ConsumeFuel` placement decides rather than the protocol. fn to_wire(result: HostResult) -> Result { match result { Ok(value) => Ok(value), @@ -72,39 +39,18 @@ fn to_wire(result: HostResult) -> Result { } } -// --------------------------------------------------------------------------- -// Gas + memory helpers. Every guest access is a checked wasmi slice op. -// --------------------------------------------------------------------------- - -/// Deduct `cost` fuel for a host call; `OutOfGas` if it would go negative. +/// Deduct `cost` fuel; `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)?; match remaining.checked_sub(cost) { Some(left) => caller.set_fuel(left).map_err(|_| HostError::Internal), None => { - // Spending what is left makes the run's reported cost the whole gas - // limit, as C++ reports it on out-of-gas. The store outlives the - // trap `OutOfGas` becomes, and `run` reads the cost off it. let _ = caller.set_fuel(0); Err(HostError::OutOfGas) } } } -/// Deduct `n` bytes from the per-run transfer-limit budget -/// ([`crate::vm::TRANSFER_LIMIT_BYTES`], separate from gas); -/// `OutOfTransferLimit` if it would go negative. -/// -/// The budget counts bytes that **cross the boundary as copies**: host→guest -/// writes (C++'s `setData`, here [`write_into`], this function's one call site) -/// and typed reads that materialize a host object out of guest bytes (C++ charged -/// uint256, AccountID, Currency, Asset). Plain borrowed reads are not charged, -/// because nothing is copied — the host is handed a slice aliasing guest memory. -/// This ABI has no typed reads yet; the rule is here for the ones that arrive, -/// which will charge the object they materialize. -/// -/// What bounds how many reads a run can make is gas, charged per host call before -/// its body runs ([`charged`]) — the same property C++ relied on. fn charge_transfer(state: &VmState<'_>, n: usize) -> Result<(), HostError> { let n = n as u64; let remaining = state.transfer_budget.get(); @@ -117,34 +63,12 @@ fn charge_transfer(state: &VmState<'_>, n: usize) -> Result<(), HostError> { } } -/// The guest's linear memory, as [`crate::vm::run`] resolved it from the -/// instance's exports. -/// -/// A field read: the resolution happens once per run, so no call pays to look an -/// export up, and every call in a run works in the same memory. `NoMemExported` -/// covers both ways the field is empty — a module that exports no memory, and a -/// call made from a start section, which runs before there is an instance to -/// resolve from. fn memory(caller: &Caller<'_, VmState<'_>>) -> Result { caller.data().memory.ok_or(HostError::NoMemExported) } -/// Bounds-check `[ptr, ptr + len)` against `data` and return that slice of it — -/// no allocation, no copy. -/// -/// Checks the params, then the [`MAX_FIELD_BYTES`] cap (`DataFieldTooLarge`), -/// then the bounds, before the slice is formed — C++'s `getDataSlice` order -/// (`HostFuncWrapper.cpp:150-176` at `b7059deb9f^`). The transfer budget is not -/// among them: there are no copied bytes to charge, which is why C++ left plain -/// slice/string reads (`trace`'s msg/data, `sha512_half`'s input) free of it — see -/// [`charge_transfer`]. -/// -/// Takes the memory's bytes rather than a [`Caller`], so a call can borrow as many -/// input regions as its signature has: they are shared borrows of one slice. -/// [`scratch_write`] is what supplies that slice. +/// Validate `[ptr, ptr + len)` against `data` and return that slice of it. pub(crate) fn region(data: &[u8], ptr: i32, len: i32) -> HostResult<&[u8]> { - // A guest's pointer and length are `i32` on the wire and indices here, so the - // conversion is the validity check: it fails on exactly the negative values. let (Ok(ptr), Ok(len)) = (usize::try_from(ptr), usize::try_from(len)) else { return Err(HostError::InvalidParams); }; @@ -155,11 +79,6 @@ pub(crate) fn region(data: &[u8], ptr: i32, len: i32) -> HostResult<&[u8]> { data.get(ptr..end).ok_or(HostError::PointerOutOfBounds) } -/// [`region`] of the guest's linear memory, for a call that reads it and writes -/// nothing back (`trace`, `trace_num`). -/// -/// 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. pub(crate) fn read_borrowed<'a>( caller: &'a Caller<'_, VmState<'_>>, ptr: i32, @@ -169,65 +88,37 @@ pub(crate) fn read_borrowed<'a>( region(mem.data(caller), ptr, len) } -/// 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. +/// Service a call whose answer is bytes, written straight into the guest's output +/// region. /// -/// **`fill`'s `usize` is the value's true length, not the number of bytes it -/// wrote.** A host holding a 64-byte value, handed a 4-byte region, writes nothing -/// and answers `64` — which is how the guest learns the size to ask for next time. -/// The count is therefore bounded by neither the region nor the cap, and that is -/// what makes both checks below reachable. -/// -/// The engine owns 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, which is why the region `fill` -/// receives is clamped to the cap: the checks decide the *status*, and the clamp -/// is what keeps an over-cap value's bytes out of guest memory regardless. -/// -/// A refusal says nothing about what is in the guest's buffer, and the guest must -/// not read it on a negative status. Over the cap, the clamp does bound what could -/// have landed. Under it the clamp is a no-op — `fill` holds exactly the region the -/// guest asked for — so whether a host that cannot fit a value leaves a prefix -/// behind is that host's choice, not something the engine can enforce. -/// -/// The bounds check covers the guest's whole declared `cap`, not the clamped -/// length, so a buffer running past memory is `PointerOutOfBounds` even when its -/// first [`MAX_FIELD_BYTES`] bytes would have been in bounds — the guest is told -/// its pointer is wrong rather than being served a truncated prefix of it. +/// **`fill` returns the value's true length, not what it wrote**: a host holding 64 +/// bytes and offered room for 4 writes nothing and answers `64`, which is how the +/// guest learns the size to ask for. So `n` is bounded by neither the region nor the +/// cap, and both checks below are reachable. pub(crate) fn write_into( caller: &mut Caller<'_, VmState<'_>>, dst: i32, cap: i32, fill: impl FnOnce(&dyn HostFunctions, &mut [u8]) -> HostResult, ) -> HostResult { - // As in `read_borrowed`: the conversion to an index is the validity check. let (Ok(dst), Ok(cap)) = (usize::try_from(dst), usize::try_from(cap)) else { return Err(HostError::InvalidParams); }; let mem = memory(caller)?; - // Copy the shared `&dyn HostFunctions` out of the store data (references are - // Copy) so the data borrow ends before we borrow guest memory mutably. let host: &dyn HostFunctions = caller.data().host; let end = dst.checked_add(cap).ok_or(HostError::PointerOutOfBounds)?; - // The guest's whole declared region, so the bounds rule is about what it asked - // for… + // Bounds-checked over the guest's whole declared region, so a buffer running + // past memory is a wrong pointer rather than a truncated prefix being served… let out = mem .data_mut(&mut *caller) .get_mut(dst..end) .ok_or(HostError::PointerOutOfBounds)?; - // …of which at most the field cap is writable. Narrowed here rather than at the - // call below, so no call can put more than MAX_FIELD_BYTES into guest memory - // whatever the guest declared, and the wider slice cannot be reached again. + // …of which only the field cap is writable, so no call can exceed it whatever + // the guest declared. let out = &mut out[..cap.min(MAX_FIELD_BYTES)]; let n = fill(host, out)?; - // Not subsumed by the clamp: `fill` reports the value's *true* length, which - // can exceed the region it was offered, and this is how the guest learns the - // value was too large rather than merely unwritten. The clamp bounds the - // bytes; this bounds the status. if n > MAX_FIELD_BYTES { return Err(HostError::DataFieldTooLarge); } @@ -235,8 +126,6 @@ pub(crate) fn write_into( return Err(HostError::BufferTooSmall); } charge_transfer(caller.data(), n)?; - // The cap check above bounds `n`, so the count reaches the wire whole: an - // `i32` the guest reads as a byte count, never a truncation of a larger one. #[expect( clippy::cast_possible_truncation, clippy::cast_possible_wrap, @@ -246,35 +135,22 @@ pub(crate) fn write_into( Ok(n) } -/// Service a host call that reads guest memory and writes a value back into it -/// (`sha512_half`): the host fills the run's scratch buffer -/// ([`VmState::scratch`](crate::vm::VmState::scratch)), and the engine copies the -/// result into the guest's output region once every rule has passed. +/// Service a call that reads guest memory and writes bytes back to it: the host +/// fills the run's output buffer, which is copied to the guest once every rule has +/// passed. /// -/// `call` is handed the guest's whole linear memory, so it borrows **as many** -/// input regions as it needs with [`region`]. That is the difference from -/// [`write_into`]: a `&mut` view of guest memory admits no simultaneous `&` view, -/// so a host writing straight into the guest can only be given inputs that were -/// copied out first, one buffer per input. Reading many and writing one is the -/// common shape in this ABI — the two-argument keylets, the float arithmetic ops — -/// and it is this helper that generalizes to it. +/// `call` gets the guest's whole memory, so it can borrow any number of input +/// regions with [`region`] — which a `&mut` view of that memory would forbid. That +/// is why the answer goes through a buffer instead of straight into the guest as +/// [`write_into`]'s does. /// -/// **The host is never told the guest's capacity.** It is offered the whole -/// [`MAX_FIELD_BYTES`] scratch, and reports the value's true length; the fit is -/// decided here. Two consequences worth the indirection: +/// **The host is never told the guest's capacity**: it is offered the whole buffer +/// and reports the value's true length, so the fit is decided here, with nothing yet +/// in guest memory. A refused value therefore reaches it in no part. /// -/// - Nothing reaches guest memory until the length, the bounds, the fit and the -/// budget have all passed, so a refused call cannot leave part of a value in the -/// guest's buffer. [`write_into`] can only bound what is *writable*. -/// - The checks then run in C++'s `setData` order — params, cap, bounds, fit, -/// transfer, copy (`HostFuncWrapper.cpp:115-148` at `b7059deb9f^`) — *after* the -/// value exists, which is the order C++ could use for exactly this reason. -/// -/// So the output region is validated after the inputs, and a call with both bad -/// reports the input's verdict, as C++'s `getDataSlice`-then-`setData` sequence -/// did. `NoMemExported` is the one verdict that precedes both: it is a fact about -/// the instance rather than about this call's arguments, and there is no memory to -/// validate a region against. +/// The output is judged after the inputs, so a call with both bad reports the +/// input's verdict. `NoMemExported` precedes both: there is no memory to validate a +/// region against. pub(crate) fn scratch_write( caller: &mut Caller<'_, VmState<'_>>, dst: i32, @@ -282,28 +158,20 @@ pub(crate) fn scratch_write( call: impl FnOnce(&dyn HostFunctions, &[u8], &mut [u8]) -> HostResult, ) -> HostResult { let mem = memory(caller)?; - // One borrow, split in two: the guest's bytes, which the inputs are slices of, - // and the store data holding the scratch the output goes to. Taking them - // together is what keeps the inputs borrowed instead of copied. + // One borrow split in two: the guest's bytes for the inputs, the store data for + // the output buffer. Taking them together is what keeps the inputs borrowed + // rather than copied out. let (data, state) = mem.data_and_store_mut(&mut *caller); - // `&dyn HostFunctions` is `Copy` and outlives the store data, so taking it here - // does not hold a borrow of `state` across the call below. let host: &dyn HostFunctions = state.host; - let n = call(host, data, &mut state.scratch[..])?; + let n = call(host, data, &mut state.out_buffer[..])?; - // As in `region`: the conversion to an index is the validity check. let (Ok(dst), Ok(cap)) = (usize::try_from(dst), usize::try_from(cap)) else { return Err(HostError::InvalidParams); }; - // `n` is the value's true length, which `call` reports whether or not it wrote - // it, so it is bounded by neither the scratch nor the guest's buffer. This is - // how the guest learns a value was too large rather than merely unwritten. if n > MAX_FIELD_BYTES { return Err(HostError::DataFieldTooLarge); } - // The guest's whole declared region, so a buffer running past memory is its - // pointer being wrong rather than a truncated prefix of it being served. let end = dst.checked_add(cap).ok_or(HostError::PointerOutOfBounds)?; let out = data .get_mut(dst..end) @@ -312,9 +180,7 @@ pub(crate) fn scratch_write( return Err(HostError::BufferTooSmall); } charge_transfer(state, n)?; - out[..n].copy_from_slice(&state.scratch[..n]); - // The cap check above bounds `n`, so the count reaches the wire whole: an - // `i32` the guest reads as a byte count, never a truncation of a larger one. + out[..n].copy_from_slice(&state.out_buffer[..n]); #[expect( clippy::cast_possible_truncation, clippy::cast_possible_wrap, @@ -324,15 +190,9 @@ pub(crate) fn scratch_write( Ok(n) } -// --------------------------------------------------------------------------- -// Unit tests -// -// A `Caller` exists only for the duration of a host call, so `read_borrowed`, -// `write_into`, `scratch_write` and `memory` are unreachable from here; `tests/` -// covers them by running real modules against a fake host. `region` is the -// exception, taking bytes rather than a `Caller`, but the same tests reach it -// through every call that borrows an input. -// --------------------------------------------------------------------------- +// A `Caller` exists only during a host call, so everything above that takes one is +// unreachable from here; `tests/` covers those by running real modules against a +// fake host. #[cfg(test)] mod tests { @@ -341,8 +201,7 @@ mod tests { use std::cell::Cell; use wasmi::StoreLimitsBuilder; - /// A host no test here calls; `charge_transfer` takes the store data, which - /// has to hold one. + /// `charge_transfer` takes the store data, which has to hold a host. struct UncalledHost; impl HostFunctions for UncalledHost { @@ -363,27 +222,23 @@ mod tests { } } - /// 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), memory: None, - scratch: [0u8; MAX_FIELD_BYTES], + out_buffer: [0u8; MAX_FIELD_BYTES], } } - /// The status a result reaches the guest as. `wasmi::Error` is not `PartialEq`, - /// so a test that expects the guest-visible channel says so here. + /// `wasmi::Error` is not `PartialEq`, so a test expecting the guest-visible + /// channel says so by going through here. fn wire(result: HostResult) -> i32 { to_wire(result) .unwrap_or_else(|trap| panic!("expected a guest-visible status, got a trap: {trap}")) } - /// The guest-visible channel: a value passes through, and a soft error - /// arrives as its negative wire code — a call the engine served either way, - /// because the guest is the one who decides what to do about it. #[test] fn a_success_becomes_the_value_and_an_error_becomes_its_code() { assert_eq!(wire(Ok(0)), 0); @@ -391,19 +246,17 @@ mod tests { assert_eq!(wire(Err(HostError::BufferTooSmall)), -3); } - /// The three conditions the host cannot serve a call under, as the tests - /// *expect* them rather than as [`is_fatal`] reports them — deriving this from - /// `is_fatal` would make both tests below vacuous, since a condition wrongly - /// classified as soft would simply be skipped. Named once, so the two are one - /// statement about the same set. + /// The fatal set as the tests *expect* it, not as [`is_fatal`] reports it: + /// deriving it from `is_fatal` would make both tests below vacuous, since a + /// condition wrongly classified as soft would simply be skipped. const MUST_TRAP: [HostError; 3] = [ HostError::OutOfGas, HostError::Internal, HostError::NoMemExported, ]; - /// The fatal channel: a trap, carrying the condition so `run` can name the - /// outcome without parsing a message. + /// The trap carries the condition, so `run` can name the outcome without + /// parsing a message. #[test] fn a_host_fatal_error_becomes_a_trap_carrying_it() { for error in MUST_TRAP { @@ -416,17 +269,12 @@ mod tests { } } - /// Which errors take which channel, as a deliberate change-detector: the - /// three in [`MUST_TRAP`] trap, and everything else is a code the guest acts on. + /// Over `HostError::ALL`, so it is the whole ABI and not a sample: a code added + /// to the ABI arrives already asserted to be guest-visible, and making it fatal + /// is then a change someone has to come and make. /// - /// Over `HostError::ALL`, so it is the whole ABI and not a sample: a code - /// added to the ABI arrives here already asserted to be guest-visible, and - /// making it fatal is then a change someone has to come and make. - /// - /// `OutOfTransferLimit` is the row worth reading twice. It is the one budget - /// a contract can be expected to handle — C++ made it the single soft failure - /// among these, and this fork keeps that — so a guest asking for more than - /// the run's remaining 1 MiB is told no, not killed. + /// `OutOfTransferLimit` is the row worth reading twice: the one budget a + /// contract can be expected to handle, so it is told no rather than killed. #[test] fn only_the_host_fatal_errors_trap() { for &error in HostError::ALL { @@ -449,8 +297,8 @@ mod tests { 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. + /// The budget bounds the total, so the transfer that would overrun it is + /// refused whole rather than partially charged. #[test] fn a_transfer_past_the_budget_is_refused_and_charges_nothing() { let state = state(100); @@ -479,9 +327,9 @@ mod tests { 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`. + /// The field cap holds 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!( diff --git a/crates/xrpl-wasm-vm/src/lib.rs b/crates/xrpl-wasm-vm/src/lib.rs index fbe31c9660..f6934705b3 100644 --- a/crates/xrpl-wasm-vm/src/lib.rs +++ b/crates/xrpl-wasm-vm/src/lib.rs @@ -1,11 +1,10 @@ //! The escrow wasm VM: compile a contract, meter it, and serve its host calls. //! -//! Every guest access goes through `abi.rs`, which reaches linear memory only by -//! wasmi's bounds-checked slice operations — `forbid(unsafe_code)` is what makes -//! that a property of the crate rather than a claim in a comment. The cast lints -//! are on for the same reason: a truncating or sign-losing cast on a consensus -//! path changes what a contract is charged or told, so each one has to be argued -//! for at its site. +//! Every guest access goes through `abi.rs` and reaches linear memory only by +//! wasmi's bounds-checked slice operations; `forbid(unsafe_code)` makes that a +//! property rather than a claim. The cast lints are on for the same reason — on a +//! consensus path a truncating or sign-losing cast changes what a contract is +//! charged or told, so each one is argued for at its site. #![forbid(unsafe_code)] #![deny(rustdoc::broken_intra_doc_links)] #![deny(unreachable_pub)] diff --git a/crates/xrpl-wasm-vm/src/register.rs b/crates/xrpl-wasm-vm/src/register.rs index 9063d26739..a56906b037 100644 --- a/crates/xrpl-wasm-vm/src/register.rs +++ b/crates/xrpl-wasm-vm/src/register.rs @@ -3,29 +3,22 @@ use crate::vm::VmState; use wasmi::{Caller, Linker}; use xrpl_host_functions::{HostError, HostFunctionSpec}; -/// Import module namespace the guest imports host functions from -/// (`(import "host_lib" "ldgr_index" ...)`). The guest SDK and this fork's -/// fixtures spell it this way. +/// The module name the guest imports under (`(import "host_lib" "ldgr_index" …)`), +/// as the guest SDK and this fork's fixtures spell it. const HOST_MODULE: &str = "host_lib"; -// --------------------------------------------------------------------------- -// Import registration -// --------------------------------------------------------------------------- - -/// Register the PoC's host functions on `linker`, one per [`HostFunctionSpec`] -/// variant. +/// Register the host functions on `linker`, one per [`HostFunctionSpec`] variant. /// -/// Driven by an exhaustive `match` over [`HostFunctionSpec::ALL`]: adding a -/// variant to the ABI won't compile until it has an arm here (that's the "can't -/// forget to register" guarantee). Each arm is one [`charged`] call — the sole -/// entry point for `charge`, and the one place a result is split between the -/// status the guest reads and the trap it cannot — wrapping a body that calls -/// straight into the [`xrpl_host_functions::HostFunctions`] trait object held in -/// the [`wasmi::Store`]. +/// The `match` is exhaustive over [`HostFunctionSpec::ALL`], so a variant added to +/// the ABI will not compile until it has an arm here — the "cannot forget to +/// register" guarantee. Every arm goes through [`charged`], which is what makes the +/// gas charge and the wire encoding unforgettable too. pub(crate) fn register_host_functions( linker: &mut Linker>, ) -> Result<(), wasmi::errors::LinkerError> { - // TODO: think on how to make it better + // The arms are hand-written and repetitive by decision, not by neglect: + // generating them needs the typed `link_*` shims, deferred until the C header + // is generated from the same table (docs/claude/redesign_impl.md). for &op in HostFunctionSpec::ALL { match op { HostFunctionSpec::GetLedgerSqn => linker.func_wrap( @@ -36,9 +29,6 @@ pub(crate) fn register_host_functions( out_len: i32| -> Result { charged(&mut caller, HostFunctionSpec::GetLedgerSqn, |c| { - // The host writes the serialized sequence number - // straight into the guest output region; `write_into` - // owns the bounds/cap/buffer/transfer policy. write_into(c, out_ptr, out_len, |host, out| host.get_ledger_sqn(out)) }) }, @@ -55,9 +45,6 @@ pub(crate) fn register_host_functions( &mut caller, HostFunctionSpec::GetCurrentLedgerObjField, |c| { - // The host writes the field's bytes straight into - // the guest output region (no owned `Vec` in - // between); `write_into` owns the policy. write_into(c, out_ptr, out_len, |host, out| { host.get_current_ledger_obj_field(field, out) }) @@ -75,10 +62,6 @@ pub(crate) fn register_host_functions( out_len: i32| -> Result { charged(&mut caller, HostFunctionSpec::Sha512Half, |c| { - // The input is borrowed straight out of guest memory and - // the digest is written to the run's scratch, which - // `scratch_write` copies to the guest after its - // bounds/cap/buffer/transfer policy passes. scratch_write(c, out_ptr, out_len, |host, data, out| { let input = region(data, data_ptr, data_len)?; host.sha512_half(input, out) @@ -97,9 +80,6 @@ pub(crate) fn register_host_functions( as_hex: i32| -> Result { charged(&mut caller, HostFunctionSpec::Trace, |c| { - // Read `msg`/`data` straight out of guest memory — the - // slices alias linear memory, no owned copy (`trace` - // returns nothing, so there's no output-aliasing worry). let host = c.data().host; let msg = read_borrowed(c, msg_ptr, msg_len)?; let data = read_borrowed(c, data_ptr, data_len)?; @@ -118,7 +98,6 @@ pub(crate) fn register_host_functions( number: i64| -> Result { charged(&mut caller, HostFunctionSpec::TraceNum, |c| { - // `msg` aliases guest memory — no owned copy. let host = c.data().host; let msg = read_borrowed(c, msg_ptr, msg_len)?; let msg = core::str::from_utf8(msg).map_err(|_| HostError::Decoding)?; diff --git a/crates/xrpl-wasm-vm/src/vm.rs b/crates/xrpl-wasm-vm/src/vm.rs index d438ff4129..c311c98a47 100644 --- a/crates/xrpl-wasm-vm/src/vm.rs +++ b/crates/xrpl-wasm-vm/src/vm.rs @@ -16,79 +16,53 @@ const WASM_PAGE_BYTES: u32 = 64 * 1024; /// Linear-memory page cap. pub const MAX_MEMORY_PAGES: u32 = 128; -/// Byte form of [`MAX_MEMORY_PAGES`]: `128 * 65536 = 8_388_608` (8 MiB). +/// [`MAX_MEMORY_PAGES`] in bytes: 8 MiB. 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 during one [`run`] invocation. Separate from gas. +/// Total bytes that may cross the host/guest boundary in one [`run`], 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`. +/// Size cap on any single value crossing the boundary, in either direction; over +/// it is `DataFieldTooLarge`. /// -/// Mirrors `kMaxWasmDataLength = 1 * 1024` in -/// `include/xrpl/protocol/Protocol.h:261`, enforced there by `getDataSlice` / -/// `setData` (`src/libxrpl/tx/wasm/HostFuncWrapper.cpp`). +/// A protocol limit: `kMaxWasmDataLength` in `include/xrpl/protocol/Protocol.h`. pub const MAX_FIELD_BYTES: usize = 1024; /// State threaded through every host call, stored in the wasmi [`Store`]. pub(crate) struct VmState<'h> { pub(crate) host: &'h dyn HostFunctions, - /// Enforces [`MAX_MEMORY_BYTES`] via `Store::limiter`. It lives here because - /// the limiter callback wasmi holds has to produce a `&mut` into it from - /// `&mut VmState`. + /// Enforces [`MAX_MEMORY_BYTES`] via `Store::limiter`, which needs a `&mut` + /// into it from `&mut VmState` — hence a field rather than a local. pub(crate) mem_limits: StoreLimits, - /// Remaining transfer-limit budget for this run ([`TRANSFER_LIMIT_BYTES`]), - /// decremented in `abi.rs` by the bytes actually moved. + /// Remaining transfer budget for this run ([`TRANSFER_LIMIT_BYTES`]). /// - /// 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. + /// A `Cell` because it is decremented from a shared `&Caller`. One thread per + /// invocation touches the store, so the lack of `Sync` costs nothing. /// - /// TODO: the C++ `unalignedGas` alignment-copy charge - /// (`HostFuncWrapper.cpp:44,390-397`) has no `FieldLocator` host function - /// here to attach to. + /// TODO: the extra charge for an unaligned field copy has nothing to attach to + /// until this ABI gains a `FieldLocator` host function. pub(crate) transfer_budget: Cell, - /// The guest's linear memory, every host call's frame of reference for a - /// pointer. Resolved once by [`run`], after instantiation, and read from here on, - /// so no call pays for an export lookup. + /// The guest's linear memory, resolved once by [`run`] after instantiation so + /// no host call pays for an export lookup. /// - /// Holding the handle across calls is sound because a [`Memory`] is an arena - /// index into the store rather than a pointer to the bytes: it survives - /// `memory.grow`, and `data`/`data_mut` re-derive the slice per call. C++ - /// memoized the same resolution, as `memIdx_` on the instance wrapper - /// (`InstanceWrapper::getMem`, `WasmiVM.cpp:224-249` at `b7059deb9f^`). + /// Caching the handle is sound because a [`Memory`] is an arena index, not a + /// pointer to the bytes: it survives `memory.grow`, and `data`/`data_mut` + /// re-derive the slice per call. /// - /// `None` before `run` resolves it and for a module that exports no memory, - /// which is a legal module right up to its first host call — so the absence is - /// `NoMemExported` at that call rather than a refused instantiation. - /// - /// Not a `Cell`: `run` writes it once through `Store::data_mut` before the - /// entry point runs, and every reader afterwards holds only a `&Caller`. - /// - /// The handle is scoped to one store, so the field assumes **one module, one - /// instance, one store per `run`** — which is what `run` builds, and nothing - /// lets a guest instantiate a second module. Module linking or nested contract - /// execution would have to resolve per instance instead: a cached handle would - /// then serve a host call against the wrong instance's memory, which is a wrong - /// answer rather than an error anyone sees. + /// The handle is scoped to one store, so this assumes **one module, one + /// instance, one store per `run`**. Module linking or nested execution would + /// have to resolve per instance: a cached handle would serve a call against the + /// wrong instance's memory, which is a wrong answer rather than an error. pub(crate) memory: Option, - /// Where a host writes a value before the engine copies it to the guest, for - /// the calls that read guest memory and write it in the same breath - /// ([`crate::abi::scratch_write`]). + /// Where a host writes a value before [`crate::abi::scratch_write`] copies it + /// to the guest. One buffer per run, so no call zero-fills one of its own. /// - /// One buffer per run, reused by every call, so no call zero-fills one of its - /// own. Sized to [`MAX_FIELD_BYTES`], which is what lets a host be offered the - /// whole cap and report the value's true length while the fit against the - /// guest's buffer is decided afterwards — with nothing yet in guest memory. - /// - /// Inline rather than boxed: the store's data is built once per run and then - /// only borrowed, so a kilobyte in it costs one move at construction, where a - /// `Box` would cost an allocation. A local in - /// [`scratch_write`](crate::abi::scratch_write) would cost neither, but - /// `forbid(unsafe_code)` means a stack buffer is zero-filled, and that lands - /// back on every call — which is the cost this field exists to remove. - pub(crate) scratch: [u8; MAX_FIELD_BYTES], + /// Inline rather than boxed: the store's data is built once and then only + /// borrowed, so a kilobyte in it costs a move where a `Box` costs an + /// allocation. A local would cost neither, but `forbid(unsafe_code)` means a + /// stack buffer is zero-filled — per call, which is the cost this removes. + pub(crate) out_buffer: [u8; MAX_FIELD_BYTES], } /// Outcome of running an escrow contract to completion. @@ -161,9 +135,8 @@ impl fmt::Display for RunFailure { } impl RunFailure { - /// A failure that costs nothing: it stopped the run at or before the point the - /// guest first gets to execute, or it stopped it under a store with no meter to - /// read, which comes to the same thing — no fuel was accounted either way. + /// A failure with no fuel accounted: it stopped the run at or before the guest's + /// first instruction, or under a store with no meter to read. fn owing_nothing(error: RunError) -> RunFailure { RunFailure { error, @@ -175,15 +148,11 @@ impl RunFailure { /// Fuel spent out of `gas`: the one place a run's cost is measured, so success, /// trap and refusal all report it the same way. /// -/// `Store::get_fuel` fails on exactly one condition — a store whose engine was -/// built without fuel metering — and that is a property of -/// [`build_wasm_engine`], which turns metering on, and one `run` has already -/// established for this store by the time anything is measured: its `set_fuel` -/// fails under the same condition and returns first. So a failure here is a defect -/// in this crate, and the one thing it must not become is a number: `0` would -/// forgive a run its whole cost and `gas` would charge an untouched one for -/// everything. It leaves as [`RunError::Internal`] instead, which is what the -/// caller maps a defect to. +/// `Store::get_fuel` fails only on a store without fuel metering, which +/// [`build_wasm_engine`] rules out and `run`'s `set_fuel` would already have +/// caught — so a failure here is a defect in this crate. It must not become a +/// number: `0` forgives a run its whole cost, `gas` charges an untouched one for +/// everything. [`RunError::Internal`] instead. fn fuel_used(store: &Store>, gas: u64) -> Result { store .get_fuel() @@ -191,10 +160,8 @@ fn fuel_used(store: &Store>, gas: u64) -> Result { .map_err(|_| RunError::Internal) } -/// Report `error` with the run's cost attached, from the one point that reads it. -/// -/// A cost that cannot be read replaces the outcome rather than being invented, -/// because the cost is what the caller charges for — see [`fuel_used`]. +/// Report `error` with the run's cost attached. A cost that cannot be read replaces +/// the outcome rather than being invented — see [`fuel_used`]. fn failed(store: &Store>, gas: u64, error: RunError) -> RunFailure { match fuel_used(store, gas) { Ok(fuel_used) => RunFailure { error, fuel_used }, @@ -202,18 +169,16 @@ fn failed(store: &Store>, gas: u64, error: RunError) -> RunFailure { } } -/// The outcome a `wasmi::Error` names for itself, if it names one, rather than -/// leaving it to the stage that raised it. +/// The outcome a `wasmi::Error` names for itself, if any, rather than leaving it to +/// the stage that raised it. /// -/// A host call the host could not serve traps with a [`FatalHostError`] payload, -/// which says which condition it was, so check for that before treating the error -/// as the guest's own doing. wasmi raises `OutOfFuel` when the guest's -/// *instructions* exhaust the meter — the same outcome by a different route, and -/// `as_trap_code` reports it whichever error kind carried it. +/// Two ways a run halts mid-flight: a host call that could not be served, which +/// carries a [`FatalHostError`] saying which condition it was, and the guest's own +/// instructions exhausting the meter, which wasmi raises as `OutOfFuel`. /// -/// Both arise anywhere the guest executes, and a start section is guest code -/// running during instantiation, so every stage from there on asks this before -/// naming a failure after itself. +/// Both can happen anywhere the guest executes — including a start section, which +/// is guest code running during instantiation — so every stage from there on asks +/// this before naming a failure after itself. fn guest_halted(error: &wasmi::Error) -> Option { if let Some(fatal) = error.downcast_ref::() { return Some(host_fatal(fatal.0)); @@ -223,12 +188,11 @@ fn guest_halted(error: &wasmi::Error) -> Option { /// The outcome a host-fatal `HostError` is. /// -/// Exhaustive over `HostError` rather than closed with a wildcard, so a variant -/// added to the ABI has to be placed here before this compiles. That covers one -/// direction of the agreement with [`crate::abi::is_fatal`], which decides the -/// channel; the other — an existing variant moved into `is_fatal`'s set, which -/// would land in the soft arm here and report `Internal` instead of the condition -/// it was — is covered by `tests::every_fatal_error_has_an_outcome_of_its_own`. +/// Exhaustive rather than closed with a wildcard, so a variant added to the ABI +/// must be placed here before this compiles. That is one direction of the agreement +/// with [`crate::abi::is_fatal`], which picks the channel; the other — an existing +/// variant moved into `is_fatal`'s set, landing in the soft arm and reported as +/// `Internal` — is `tests::every_fatal_error_has_an_outcome_of_its_own`. /// /// The soft arm is otherwise unreachable: a guest-visible error is a return code /// and never becomes a trap for [`guest_halted`] to unwrap. @@ -262,9 +226,9 @@ fn host_fatal(error: HostError) -> RunError { /// The process-wide wasmi engine, built once on first use. /// -/// 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. +/// 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(crate) fn wasm_engine() -> &'static Engine { static ENGINE: LazyLock = LazyLock::new(build_wasm_engine); &ENGINE @@ -289,7 +253,7 @@ fn build_wasm_engine() -> Engine { config.wasm_custom_page_sizes(false); config.wasm_memory64(false); config.wasm_wide_arithmetic(false); - // TODO: enable option to reject wasm code containing start section after next wasmi release + // TODO: enable option to reject wasm code containing start section after wasmi 2.0 release Engine::new(&config) } @@ -316,26 +280,19 @@ pub fn run<'h>( mem_limits, transfer_budget: Cell::new(TRANSFER_LIMIT_BYTES), memory: None, - scratch: [0u8; MAX_FIELD_BYTES], + out_buffer: [0u8; MAX_FIELD_BYTES], }, ); - // A store that will not take fuel, or imports that will not register, are - // defects in the engine configuration or in this crate, not in the module: - // nothing the contract did could have caused either. + store .set_fuel(gas) .map_err(|_| RunFailure::owing_nothing(RunError::Internal))?; - // The memory-page cap applies at instantiation too: an initial memory - // declared past it fails to instantiate, as a `memory.grow` past it traps. store.limiter(|state| &mut state.mem_limits); let mut linker = Linker::>::new(engine); register_host_functions(&mut linker) .map_err(|_| RunFailure::owing_nothing(RunError::Internal))?; - // Instantiation is the first point the guest can execute, through a start - // section, so from here on the cost comes off the store rather than being - // known to be nothing. let instance = match linker.instantiate_and_start(&mut store, &module) { Ok(instance) => instance, Err(e) => { @@ -343,21 +300,10 @@ pub fn run<'h>( return Err(failed(&store, gas, error)); } }; - // Every host call reads the memory out of the store, so resolve it before the - // guest can make one. - // - // By *kind*, never by name: nothing in the wasm spec attaches meaning to - // "memory", so a toolchain that names it otherwise still produces a contract. - // C++ matched the same way (`InstanceWrapper::getMem` scanned for - // `wasm_extern_kind(e) == WASM_EXTERN_MEMORY`, `WasmiVM.cpp:224-249` at - // `b7059deb9f^`). "The first" names one thing because `build_wasm_engine` sets - // `wasm_multi_memory(false)`: a module has at most one memory, and exporting it - // under several names yields that same handle each time, so the order - // `Instance::exports` walks its map in cannot change the answer. store.data_mut().memory = instance.exports(&store).find_map(Export::into_memory); - let finish = match instance.get_typed_func::<(), i32>(&store, function_name) { - Ok(finish) => finish, + let function = match instance.get_typed_func::<(), i32>(&store, function_name) { + Ok(function) => function, Err(e) => { let error = RunError::EntryPoint(entry_point_detail( instance.get_export(&store, function_name), @@ -368,7 +314,7 @@ pub fn run<'h>( } }; - let result = match finish.call(&mut store, ()) { + let result = match function.call(&mut store, ()) { Ok(result) => result, Err(e) => { let error = guest_halted(&e).unwrap_or_else(|| RunError::Trap(e.to_string())); @@ -380,13 +326,6 @@ pub fn run<'h>( Ok(RunOutcome { result, fuel_used }) } -/// Why `get_typed_func` would not hand over the entry point, told apart by what -/// the module exports under that name. -/// -/// wasmi answers all three cases with one error, so the message would otherwise -/// read "no entry point" for a contract that exports the name with the wrong -/// signature — a diagnostic that sends the author looking for a missing export -/// they already have. `export` is what [`wasmi::Instance::get_export`] found. fn entry_point_detail(export: Option, name: &str, error: &wasmi::Error) -> String { match export { Some(Extern::Func(_)) => { @@ -402,23 +341,11 @@ mod tests { use super::*; use crate::abi::is_fatal; - /// The engine is built once and shared, so two invocations must not compile - /// their modules against different engines. #[test] fn the_engine_is_one_engine() { assert!(Engine::same(wasm_engine(), wasm_engine())); } - /// The two lists that decide a host error's fate must name the same set. - /// [`is_fatal`] picks the channel; [`host_fatal`] names the outcome of the - /// fatal one. Only one direction of that agreement is compiler-enforced — a - /// *new* variant fails to compile until `host_fatal` places it — so a variant - /// moved into `is_fatal`'s set would trap and then be reported as `Internal`, - /// losing the condition it was. This is the other direction. - /// - /// The soft arm's answer is `Internal`, which `HostError::Internal` also - /// answers, so "named in its own right" is the outcome being anything else, - /// with `Internal` itself asked about by name. #[test] fn every_fatal_error_has_an_outcome_of_its_own() { for &error in HostError::ALL { @@ -442,14 +369,12 @@ mod tests { } } - /// 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`. + /// The only place these numbers appear as literals; every other test derives + /// them from the constants. #[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_MEMORY_PAGES, 128, "linear-memory page cap"); + assert_eq!(MAX_MEMORY_BYTES, 8 * 1024 * 1024, "page cap 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 index e7e606599c..9b7ac613ed 100644 --- a/crates/xrpl-wasm-vm/tests/budgets.rs +++ b/crates/xrpl-wasm-vm/tests/budgets.rs @@ -168,9 +168,8 @@ fn fuel_used_is_what_was_spent_not_what_was_supplied() { assert_eq!(outcome.fuel_used, cost, "gas {gas}"); } - // One fuel short: the run ends at the call it cannot pay for, and still owes - // the gas — the whole limit, because `charge` spends what is left, which is - // what makes the reported cost the full budget as in C++. + // One fuel short: the run ends at the call it cannot pay for and still owes the + // whole limit, because `charge` spends what is left. let short = run_with_gas(&wat, cost - 1, &host).expect_err("one fuel short must not complete"); assert!( matches!(short.error, RunError::OutOfGas), @@ -351,9 +350,8 @@ fn a_modest_run_never_meets_the_budget() { } /// Reads leave the budget alone: `read_borrowed` hands the host a slice *aliasing* -/// guest memory, so there are no copied bytes to charge — the rule C++ applied to -/// `trace`'s msg and data. What bounds how many reads a run can make is gas, which -/// every host call pays before its body runs. +/// guest memory, so there are no copied bytes to charge. What bounds how many reads +/// a run can make is gas, which every host call pays before its body runs. /// /// The observation is the write at the end, not the reads: the module reads four /// times the whole budget first, so a rule that charged reads would have nothing diff --git a/crates/xrpl-wasm-vm/tests/memory_policy.rs b/crates/xrpl-wasm-vm/tests/memory_policy.rs index f1ee38b41f..28f265bb78 100644 --- a/crates/xrpl-wasm-vm/tests/memory_policy.rs +++ b/crates/xrpl-wasm-vm/tests/memory_policy.rs @@ -301,11 +301,10 @@ fn both_of_traces_regions_are_checked() { /// before anything about the output, so a bad input is reported however the output /// region is wrong — out of bounds, or a pointer that is not one at all. /// -/// The whole output region, params included, is judged after the host has answered, -/// which is `getDataSlice`-then-`setData` (`HostFuncWrapper.cpp:115-176` at -/// `b7059deb9f^`). Hoisting any part of it above the call would put the output's -/// verdict first for these cases, and there is no half of it that can be hoisted on -/// a principle the other half shares. +/// The whole output region, params included, is judged after the host has answered. +/// Hoisting any part of that above the call would put the output's verdict first for +/// these cases, and there is no half of it that can be hoisted on a principle the +/// other half shares. #[test] fn a_read_write_checks_its_input_before_its_output() { let host = FakeHost::new(); @@ -495,9 +494,7 @@ fn no_memory_is_answered_before_a_calls_arguments_are() { /// The memory's export *name* is not part of the contract: the engine takes the /// module's memory whatever it is called. Nothing in the wasm spec attaches meaning -/// to `"memory"` — it is a toolchain convention — and C++ read no name either -/// (`InstanceWrapper::getMem`, `WasmiVM.cpp:224-249` at `b7059deb9f^`, matched -/// `wasm_extern_kind(e) == WASM_EXTERN_MEMORY`). +/// to `"memory"` — it is a toolchain convention, so the kind decides. #[test] fn a_memory_exported_under_any_name_is_the_guests_memory() { let host = FakeHost::new(); diff --git a/crates/xrpl-wasm-vm/tests/vm_limits.rs b/crates/xrpl-wasm-vm/tests/vm_limits.rs index a2d6f6d3eb..34b458321d 100644 --- a/crates/xrpl-wasm-vm/tests/vm_limits.rs +++ b/crates/xrpl-wasm-vm/tests/vm_limits.rs @@ -424,11 +424,8 @@ fn a_start_section_that_exhausts_gas_is_out_of_gas_not_an_instantiation_failure( /// and instantiation is what produces the instance, so a call made while it is /// still running has no memory to work in and ends the run. /// -/// This is the C++ path's behaviour and for the same reason: `wasm_instance_new` -/// (`WasmiVM.cpp:154` at `b7059deb9f^`) ran the start section, and -/// `wasm_instance_exports` (line 161) filled the export table only after it -/// returned — so the scan `InstanceWrapper::getMem` performs found nothing during a -/// start section either. +/// Not a choice: `Module::instantiate` is `pub(crate)` in wasmi, so instantiation +/// cannot be split from the start section to resolve the memory in between. #[test] fn a_start_section_cannot_make_a_host_call() { let host = FakeHost::new(); diff --git a/docs/claude/redesign_impl.md b/docs/claude/redesign_impl.md index 8869e41b73..2407f2fcbd 100644 --- a/docs/claude/redesign_impl.md +++ b/docs/claude/redesign_impl.md @@ -1038,41 +1038,73 @@ instead of error text to parse. D16's `gas = 0` decision belongs there too, sinc a TER choice. Deferred as before: macro-emitted `link_*` shims, the generated C header, the probe-module test. -## Once the crate is finished: cut the comments back +## The comment cut-back (done, 2026-08-03) -**`xrpl-wasm-vm`'s comments are too verbose, and they should be edited down in one pass -once the crate stops moving.** Do not do it while findings are still landing — several of -them turned on a rationale that only existed in a comment, and losing those mid-flight -costs more than the reading time. +**Done over `src/` and `tests/`, after C11 and with C12 deferred to a benchmark** — so no +behaviour finding was still in flight, which was the gate. Density in `abi.rs` went from +42% comment lines to 16%, `vm.rs` from 40% to 28%, `register.rs` from 23% to 9% (its +per-arm comments only restated the helper each arm calls). -Why they got this way is worth knowing, because it tells you what to keep. Each finding -was argued out in its doc comment as it landed: why a rule exists, which C++ line it -mirrors, why the obvious simplification is wrong. That was the right thing to write at the -time — the review found real bugs precisely where the code had asserted something no -comment justified — but the accumulation now reads as an essay per function. `write_into` -and `VmState::memory` are the clearest cases. +Two things made it safe to do in bulk. Nothing but comments changed — verified by +stripping comment and blank lines from before and after and diffing, per file, to +byte-identical code. And the 79 tests, `clippy --all-targets`, `fmt` and +`cargo doc --no-deps` all stayed green, the last of these load-bearing because +`deny(rustdoc::broken_intra_doc_links)` catches a link broken by a deleted paragraph. +One caveat learned the hard way: that lint does **not** cover private modules, which are +not documented by default, so the dead `VmState::scratch` link left by the +`scratch` → `out_buffer` rename passed `cargo doc` silently. Grep for renamed fields; do +not rely on the lint inside `abi.rs`. -What the pass should keep, roughly in order of value: +**The rule that overrode this document: no references to C++ that will not survive the +merge.** They read as evidence but will point at deleted files — `WasmiVM.cpp`, +`HostFuncWrapper.cpp`, anything pinned at `b7059deb9f^` — so the crate now has none, in +`src/` or `tests/`. Two exceptions stand, both live: `Protocol.h`'s `kMaxWasmDataLength` +and `kWasmTransferLimit`, which are where the numbers are defined for the rest of the +system and are named in `the_limits_are_the_protocol_limits` for that reason. The parity +evidence itself is not lost — it is in this document, and this is where it belongs. + +The `scratch` field is now `VmState::out_buffer`. `abi::scratch_write` keeps its name; +if that reads inconsistently, the rename is a one-liner. + +Two TODOs were made honest rather than deleted, since both read as gaps and neither is +one: the start-section TODO now says why wasmi 1.1 cannot close it and that the section is +metered regardless (D17), and `register.rs`'s "think on how to make it better" now says the +repetition is the deferred `link_*`-shim decision. `transfer_budget`'s `unalignedGas` TODO +stays a TODO — it is a real obligation, now stated as blocked on the ABI gaining a +`FieldLocator` function. + +Why the comments got that way is worth knowing, because it tells you what to keep. Each +finding was argued out in its doc comment as it landed: why a rule exists, which C++ line +it mirrored, why the obvious simplification is wrong. That was right at the time — the +review found real bugs precisely where the code asserted something no comment justified — +but it accumulated into an essay per function, `write_into` and `VmState::memory` worst. + +What the pass kept: -- **The C++ reference points.** `WasmiVM.cpp:224-249`, `HostFuncWrapper.cpp:497`, - `Protocol.h`'s names. These are consensus parity evidence and cannot be recovered from - the code. - **Why an apparent redundancy is not one.** The `n > MAX_FIELD_BYTES` check beside the clamp; `is_fatal` and `host_fatal` being two lists; `MUST_TRAP` restating the fatal set rather than deriving it. Every one of these has been "simplified" wrongly at least once in a mutation test, so each earns its sentence. - **Load-bearing invariants**, like `VmState::memory`'s one-instance-per-run assumption. +- **Hidden contracts a signature cannot state**, chiefly that a byte-output host function + returns the value's *true length* rather than what it wrote. +- **wasmi facts that decide a design**, like a `Memory` being an arena index (so caching + the handle survives `memory.grow`) and `Module::instantiate` being `pub(crate)` (so + instantiation cannot be split from the start section). -What it should cut: +What it cut: -- Prose restating what the next line plainly does. -- The same rationale on a field and on the function that sets it — pick the one a reader - reaches first. `WasmiVM.cpp:224-249` is currently cited twice for two different facts. -- Paragraphs duplicating this document. A pointer here beats a retelling in `abi.rs`. -- The worked examples that have served their purpose, where a sentence now does. +- Prose restating what the next line plainly does — every per-arm comment in + `register.rs`, `write_into`'s "the engine owns the policy" paragraph, and the several + notes explaining a borrow the compiler already enforces. +- The same rationale on a field and on the function that reads it: `VmState::memory` keeps + the arena-index invariant and `abi::memory` keeps only the two ways it is absent. +- Paragraphs duplicating this document — `scratch_write`'s case for its design went from + five paragraphs to three short ones, the ABI-shape argument left here. +- Every C++ citation, per the rule above. -A rule of thumb that fits what actually paid off: a comment should say something the -compiler cannot check and the code cannot show. Everything else is a candidate. +The rule of thumb that fits what paid off: a comment should say something the compiler +cannot check and the code cannot show. Everything else was a candidate. One incidental constraint found while checking the guest target: `crates/hello_world` cannot be checked for `wasm32-unknown-unknown` — it depends on `cxx` →