ARCHIVE/rippled
1
0
Fork 0
mirror of https://github.com/XRPLF/rippled.git synced 2026-08-21 22:30:57 +00:00
Code Issues Packages Projects Releases Wiki Activity

Update docs

Browse Source
This commit is contained in:
Sergey Kuznetsov
2026-08-03 14:05:21 +01:00
parent e484a2902c
commit 047a3f5cb8
8 changed files with 200 additions and 425 deletions
Download Patch File Download Diff File Expand all files Collapse all files

262
crates/xrpl-wasm-vm/src/abi.rs
View File

@@ -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<i32>) -> Result<i32, wasmi::Error> {
match result {
Ok(value) => Ok(value),
@@ -72,39 +39,18 @@ fn to_wire(result: HostResult<i32>) -> Result<i32, wasmi::Error> {
}
}
// ---------------------------------------------------------------------------
// 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<T>(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<Memory, HostError> {
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<usize>,
) -> HostResult<i32> {
// 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<usize>,
) -> HostResult<i32> {
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>) -> 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!(

11
crates/xrpl-wasm-vm/src/lib.rs
View File

@@ -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)]

41
crates/xrpl-wasm-vm/src/register.rs
View File

@@ -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<VmState<'_>>,
) -> 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<i32, wasmi::Error> {
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<i32, wasmi::Error> {
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<i32, wasmi::Error> {
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<i32, wasmi::Error> {
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)?;

203
crates/xrpl-wasm-vm/src/vm.rs
View File

@@ -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<u64>,
/// 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<Memory>,
/// 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<VmState<'_>>, gas: u64) -> Result<u64, RunError> {
store
.get_fuel()
@@ -191,10 +160,8 @@ fn fuel_used(store: &Store<VmState<'_>>, gas: u64) -> Result<u64, RunError> {
.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<VmState<'_>>, 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<VmState<'_>>, 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<RunError> {
if let Some(fatal) = error.downcast_ref::<FatalHostError>() {
return Some(host_fatal(fatal.0));
@@ -223,12 +188,11 @@ fn guest_halted(error: &wasmi::Error) -> Option<RunError> {
/// 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<Engine> = 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::<VmState<'h>>::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<Extern>, 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");
}

10
crates/xrpl-wasm-vm/tests/budgets.rs
View File

@@ -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

13
crates/xrpl-wasm-vm/tests/memory_policy.rs
View File

@@ -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();

7
crates/xrpl-wasm-vm/tests/vm_limits.rs
View File

@@ -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();

78
docs/claude/redesign_impl.md
View File

@@ -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` →