From 041869ff3d3f82a9ad12647201d324078d549fbc Mon Sep 17 00:00:00 2001 From: Sergey Kuznetsov Date: Thu, 30 Jul 2026 17:42:34 +0100 Subject: [PATCH] Search memory in exports. Cache memory --- crates/xrpl-wasm-vm/src/abi.rs | 19 ++- crates/xrpl-wasm-vm/src/vm.rs | 45 ++++++- crates/xrpl-wasm-vm/tests/memory_policy.rs | 77 +++++++++-- crates/xrpl-wasm-vm/tests/vm_limits.rs | 33 +++++ docs/claude/redesign_impl.md | 144 ++++++++++++++++++--- 5 files changed, 282 insertions(+), 36 deletions(-) diff --git a/crates/xrpl-wasm-vm/src/abi.rs b/crates/xrpl-wasm-vm/src/abi.rs index 7db1cdb2bb..52c2f2d18d 100644 --- a/crates/xrpl-wasm-vm/src/abi.rs +++ b/crates/xrpl-wasm-vm/src/abi.rs @@ -1,5 +1,5 @@ use crate::vm::{MAX_FIELD_BYTES, VmState}; -use wasmi::{Caller, Extern, Memory}; +use wasmi::{Caller, Memory}; use xrpl_host_functions::{HostError, HostFunctionSpec, HostFunctions, HostResult}; // --------------------------------------------------------------------------- @@ -117,12 +117,16 @@ fn charge_transfer(state: &VmState<'_>, n: usize) -> Result<(), HostError> { } } -/// The guest's exported linear memory. -fn memory(caller: &Caller<'_, T>) -> Result { - match caller.get_export("memory") { - Some(Extern::Memory(mem)) => Ok(mem), - _ => Err(HostError::NoMemExported), - } +/// 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)` and return a `&[u8]` aliasing guest linear @@ -318,6 +322,7 @@ mod tests { host: &UncalledHost, mem_limits: StoreLimitsBuilder::new().build(), transfer_budget: Cell::new(budget), + memory: None, } } diff --git a/crates/xrpl-wasm-vm/src/vm.rs b/crates/xrpl-wasm-vm/src/vm.rs index 7abdc12b63..229361d604 100644 --- a/crates/xrpl-wasm-vm/src/vm.rs +++ b/crates/xrpl-wasm-vm/src/vm.rs @@ -2,7 +2,8 @@ use std::cell::Cell; use std::fmt; use std::sync::LazyLock; use wasmi::{ - Config, Engine, Extern, Linker, Module, Store, StoreLimits, StoreLimitsBuilder, TrapCode, + Config, Engine, Export, Extern, Linker, Memory, Module, Store, StoreLimits, StoreLimitsBuilder, + TrapCode, }; use xrpl_host_functions::{HostError, HostFunctions}; @@ -48,6 +49,30 @@ pub(crate) struct VmState<'h> { /// (`HostFuncWrapper.cpp:44,390-397`) has no `FieldLocator` host function /// here to attach to. 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. + /// + /// 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^`). + /// + /// `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. + pub(crate) memory: Option, } /// Outcome of running an escrow contract to completion. @@ -78,7 +103,9 @@ pub enum RunError { OutOfGas, /// The host could not serve a call. Internal, - /// The module exports no linear memory, so no host call can be served. + /// A host call had no linear memory to work in: the module exports none, or + /// the call came from a start section, which runs before there is an instance + /// to resolve the memory from. NoMemory, /// The guest trapped: `unreachable`, division by zero, an out-of-bounds /// access, or `memory.grow` past the page cap. @@ -272,6 +299,7 @@ pub fn run<'h>( host, mem_limits, transfer_budget: Cell::new(TRANSFER_LIMIT_BYTES), + memory: None, }, ); // A store that will not take fuel, or imports that will not register, are @@ -298,6 +326,19 @@ 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, Err(e) => { diff --git a/crates/xrpl-wasm-vm/tests/memory_policy.rs b/crates/xrpl-wasm-vm/tests/memory_policy.rs index 8c88663c03..3ea65a94c9 100644 --- a/crates/xrpl-wasm-vm/tests/memory_policy.rs +++ b/crates/xrpl-wasm-vm/tests/memory_policy.rs @@ -411,29 +411,84 @@ fn a_module_that_exports_no_memory_cannot_call_the_host() { assert_no_memory(&wat, &host); } -/// The export has to be named `memory`, and it has to *be* a memory — a global -/// under that name is not a near miss the engine tolerates. +/// 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`). #[test] -fn the_memory_export_must_be_a_memory_named_memory() { +fn a_memory_exported_under_any_name_is_the_guests_memory() { let host = FakeHost::new(); - // The right kind under the wrong name. - let misnamed = module( - &[import::LDGR_INDEX, r#"(memory (export "mem") 1)"#], - "(call $ldgr_index (i32.const 0) (i32.const 4))", - ); - assert_no_memory(&misnamed, &host); + for name in ["mem", "linear", "the memory"] { + let wat = module( + &[ + import::LDGR_INDEX, + &format!(r#"(memory (export "{name}") 1)"#), + ], + "(drop (call $ldgr_index (i32.const 64) (i32.const 4))) + (i32.load (i32.const 64))", + ); + assert_eq!( + status(&wat, &host), + 7, + "the host wrote into the memory exported as '{name}'" + ); + } +} + +/// One memory exported under several names is one memory. The engine resolves the +/// first export of kind memory, and with at most one memory per module every such +/// export is that memory, so the order the exports are walked in cannot change the +/// answer. +#[test] +fn one_memory_exported_under_several_names_is_still_that_memory() { + let host = FakeHost::new(); + + let wat = module( + &[ + import::LDGR_INDEX, + r#"(memory (export "memory") (export "mem") (export "linear") 1)"#, + ], + "(drop (call $ldgr_index (i32.const 64) (i32.const 4))) + (i32.load (i32.const 64))", + ); + assert_eq!(status(&wat, &host), 7); +} + +/// The export has to *be* a memory: a global named `memory` is not one, and it +/// neither serves as the guest's memory nor hides the memory the module really +/// exports. The kind decides, so the conventional name carries no weight on +/// either side. +#[test] +fn an_export_named_memory_that_is_not_a_memory_is_not_the_guests_memory() { + let host = FakeHost::new(); + + let call = "(call $ldgr_index (i32.const 0) (i32.const 4))"; - // The right name on the wrong kind, which is the other arm of the match. let wrong_kind = module( &[ import::LDGR_INDEX, "(memory 1)", r#"(global (export "memory") i32 (i32.const 0))"#, ], - "(call $ldgr_index (i32.const 0) (i32.const 4))", + call, ); assert_no_memory(&wrong_kind, &host); + + let shadowed = module( + &[ + import::LDGR_INDEX, + r#"(memory (export "mem") 1)"#, + r#"(global (export "memory") i32 (i32.const 0))"#, + ], + call, + ); + assert_eq!( + status(&shadowed, &host), + 4, + "the real memory is found past the global that took its name" + ); } /// Bounds follow the memory the module actually declared, not a fixed page. diff --git a/crates/xrpl-wasm-vm/tests/vm_limits.rs b/crates/xrpl-wasm-vm/tests/vm_limits.rs index c1a973c37f..a2d6f6d3eb 100644 --- a/crates/xrpl-wasm-vm/tests/vm_limits.rs +++ b/crates/xrpl-wasm-vm/tests/vm_limits.rs @@ -419,6 +419,39 @@ fn a_start_section_that_exhausts_gas_is_out_of_gas_not_an_instantiation_failure( ); } +/// A start section cannot make a host call that needs guest memory, even in a +/// module that exports one: the memory is resolved from the *instance's* exports, +/// 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. +#[test] +fn a_start_section_cannot_make_a_host_call() { + let host = FakeHost::new(); + + let wat = format!( + r#"(module {ldgr_index} {ONE_PAGE} + (func $init (drop (call $ldgr_index (i32.const 0) (i32.const 4)))) + (start $init) + (func (export "finish") (result i32) (i32.const 0)))"#, + ldgr_index = import::LDGR_INDEX + ); + + let failure = assert_stage!( + run_with_gas(&wat, PLENTY_OF_GAS, &host) + .expect_err("a host call from a start section must not be served"), + RunError::NoMemory + ); + assert!( + failure.fuel_used > 0, + "the start section is metered up to the refused call: {failure}" + ); +} + // --------------------------------------------------------------------------- // The entry point // --------------------------------------------------------------------------- diff --git a/docs/claude/redesign_impl.md b/docs/claude/redesign_impl.md index b88faeb557..2dfc2eb526 100644 --- a/docs/claude/redesign_impl.md +++ b/docs/claude/redesign_impl.md @@ -309,7 +309,7 @@ internal to `abi.rs`. Both `register.rs` and the trait are untouched by the choi (`mem.data_mut(&mut *caller).get_mut(dst..end)`), so the host writes straight into wasm memory with no copy. The cost is that this `&mut` borrow cannot coexist with a `&` borrow of guest memory for the inputs, which is the only reason `read_write` exists: it -memcpies the input into a `[0u8; MAX_WASM_DATA_LEN]` stack array first. That does not +memcpies the input into a `[0u8; MAX_FIELD_BYTES]` stack array first. That does not generalize — `credential_keylet`, `check_sig` and `paychan_keylet` each take three byte inputs, so each would need its own stack buffer. @@ -325,15 +325,24 @@ Cost is roughly a wash: - `get_tx_field` (no byte input, ≤1 KiB output) — today 1024 direct; scratch 1024 + 1024. **Worse.** -Scratch also fixes a real wart: `write_into` checks `n > cap` *after* `fill` has -already written, so a rejected call leaves bytes in the guest buffer. Its own doc -comment accepts this ("the guest must treat a negative status as don't read the -buffer"); C++ `setData` checked before the memcpy. +**One argument for scratch has since been spent, and it was the strongest one.** It used +to be that `write_into` checked `n > cap` *after* `fill` had already written, so a +refused call left bytes in the guest's buffer, and only a scratch buffer could check +before the copy the way C++'s `setData` did. **A4 closed most of that without scratch**: +`fill` receives at most `min(cap, MAX_FIELD_BYTES)`, so an over-cap value cannot reach +guest memory at all. What remains is narrower — under the cap the clamp is a no-op, so a +host that cannot fit a value could still leave a prefix behind, and a scratch buffer +would make that impossible rather than contractual. So the wart is now a **host-contract +question, not an engine defect**, and it should carry much less weight in the decision +than the paragraph above once implied. Judge C11 mainly on the cost table and on +`read_write` not generalizing past one byte input. -### Status: deferred +### Status: the live decision, after C10 -**Not a blocker.** Get the VM compiling and working first; the typed shims, generated -header and probe-module test are a follow-up refactor once there is working code. +The VM compiles and works, so the reason this was deferred is spent. It is **C11**, and +the order is C10 first: caching the `Memory` in `VmState` removes the per-call export +lookup that both designs otherwise pay, and makes either answer here easier to +implement. The typed shims, generated header and probe-module test stay deferred. ## Open ABI questions and interop risks (2026-07-29) @@ -506,6 +515,43 @@ matter. Items marked ✓ are done. function of a cargo flag.) The tests assemble text themselves from a dev-dependency, so nothing of ours is needed to keep them working — see "Build / test loop". +### A, addendum: the memory export's *name* is a rule the rewrite introduced (2026-07-30) + +Found while scoping C10, and it is the same class of item as A3 and A5: a behaviour +change nobody chose. + +`abi.rs` resolves guest memory with `caller.get_export("memory")` — **by name**. The C++ +path did not use the name at all. `InstanceWrapper::getMem` +(`WasmiVM.cpp:224-249` at `b7059deb9f^`) scanned the instance's exports for the first one +whose *kind* is `WASM_EXTERN_MEMORY`, whatever it was called: + +```cpp +if (wasm_extern_kind(e) == WASM_EXTERN_MEMORY) { memIdx_ = i; mem = ...; break; } +``` + +So a module exporting its memory as `"mem"` or `"linear"` worked under C++ and is refused +today — and `the_memory_export_must_be_a_memory_named_memory` in `memory_policy.rs` pins +the stricter rule. With `wasm_multi_memory(false)` the C++ scan was unambiguous: at most +one memory exists, so "the first memory export" names exactly one thing. + +Nothing in the wasm spec attaches meaning to the name `"memory"`, or requires a module to +export its memory at all; the name is a toolchain convention (LLVM, Rust's +`wasm32-unknown-unknown`, Emscripten and wasi all emit it), which is why matching on it +works in practice. **The decision to make**: keep the name as an ABI rule, or restore +C++'s match-by-kind. Either is defensible — but if the name stays, it is as much part of +the wire contract as `HOST_MODULE`, and unlike `HOST_MODULE` it is a bare literal inside a +private helper with no named constant and no mention in the ABI docs. That asymmetry is +the part to fix regardless of which way the decision goes. + +**Decided: match by kind**, restoring C++'s behaviour, which also dissolves the +asymmetry rather than fixing it — the name is no longer in the code at all. Landed with +C10; see finding 10 for what that forced about start sections. + +Second observation from the same code: **C++ already cached the resolution**, memoizing +`memIdx_` on first use. So C10 is not an optimization past the C++ path, it is restoring +something the rewrite dropped. `memIdx_` was a per-`InstanceWrapper` member, which is the +same one-instance-per-run assumption C10's cache would take on. + ### B. Dead weight — pure simplification, no behaviour change 6. ✓ **`AbiRet` is vestigial.** `type Out` is always `()`, `impl AbiRet for u32` is never @@ -537,18 +583,77 @@ matter. Items marked ✓ are done. ### C. Performance -10. **The `"memory"` export is a string hash lookup on every host call.** `memory()` +10. ✓ **The `"memory"` export is a string hash lookup on every host call.** `memory()` (`abi.rs:104`) → `Caller::get_export` → `InstanceEntity::exports: Map, Extern>`. Resolve it once after instantiation and keep the `Memory` in `VmState`. Two bonuses: `NoMemExported` becomes an instantiation-time error, where it belongs, and a per-call failure path disappears. Cheapest real win in the crate, and the benchmark can measure it. + + Caching is sound: `wasmi::Memory` is `Stored`, an arena index into the + store rather than a pointer (`memory/mod.rs:31`), so the handle survives + `memory.grow` — only the data slice is re-derived, per call, by `data`/`data_mut`. + It is also worth more than "one lookup per call": `trace` resolves the export twice + (two `read_borrowed`s) and `sha512_half` twice (`read_write`, then `write_into`). + + **Decline the first bonus.** Failing instantiation when there is no `"memory"` + export is a *behaviour change*, not a tidy-up: a module that exports no memory and + makes no host call runs today and would stop. C++ also only discovered this at the + call, since it resolved the export per call too. The version with identical + observable behaviour is to resolve eagerly into an `Option` in `VmState`, + leave it `None` when the export is absent, and have the accessor answer + `NoMemExported` — every call is then free of the lookup and nothing observable + moves. The residual "`None` after `run` set it" case is a defect in this crate, not + a guest one, so it belongs on `Internal` rather than `NoMemExported`. + + Consequence for the tests either way: `memory_policy.rs`'s `assert_no_memory` + asserts `fuel_used > 0`, which holds because the guest burns fuel reaching the + call. That stays true under the `Option` design and would become `== 0` under + instantiation-time failure — a useful tell for which design got built. + + **Landed, resolving by kind** (the addendum's decision), so the name is gone from + the resolution path: `instance.exports(store).find_map(Export::into_memory)`, once, + after `instantiate_and_start`, into a plain `Option` on `VmState`. Plain + rather than `Cell` because it is written once through `store.data_mut()` before + `finish.call` and only read after — unlike `transfer_budget`, whose read path holds + a shared borrow. + + **Kind-matching cannot be lazy, and that decides one behaviour.** + `Caller::get_export` is name-only and `Caller`'s `instance` field is private + (`func/caller.rs:13,32`), so exports cannot be enumerated from inside a host call; + and `Module::instantiate` is `pub(crate)`, so instantiation cannot be split from the + start section (D17's root cause again). The resolution therefore happens after the + start section runs, and **a start section can no longer make a host call needing + memory** — it gets `NoMemExported`. That is parity, not a regression: C++ ran + `wasm_instance_new` (start included, `WasmiVM.cpp:154`) and filled its export table + with `wasm_instance_exports` only afterwards (`:161`), so its scan found nothing + during a start section either. `a_start_section_cannot_make_a_host_call` pins it. + Today's lazy name-based lookup was the outlier on *both* axes. + + A residual `None` is therefore **not** the `Internal` case sketched above: with + resolution after instantiation, `None` is reachable for two legitimate guest-caused + reasons — no memory export, and a call from a start section — so `NoMemExported` is + the only correct answer. + + Two notes from writing the tests. wasmi's export map is a `BTreeMap` in this feature + configuration, so `"finish"` sorts first and a kind-blind "first export" resolution + fails 38 tests rather than a subtle few — cheap to catch. And a global exported as + `"memory"` **cannot on its own** pin kind-matching: a module with no memory export + answers `None` under both the correct and the kind-blind resolution, so that + assertion holds either way. The test needed a second half — a real memory exported + as `"mem"` *beside* a global named `"memory"`, asserting the call succeeds — which + states the rule in both directions: the conventional name neither qualifies a + non-memory nor hides the real one. 11. `read_write` memsets 1 KiB of stack per call and does not generalize past one byte - input — that is the scratch-buffer decision already open above. #10 makes either - choice easier. + input — that is the scratch-buffer decision already open above ("Open: where the + output region points"), which is now the live one and needs answering before this is + codeable. #10 makes either choice easier. Note A4 spent that section's + leaves-bytes-behind argument; read the amendment there before deciding. 12. `Linker` is rebuilt per `run` (five `func_wrap`s plus string interning) and the module is compiled per run with no cache. Lower priority. The blocker worth - recording: `VmState<'h>`'s lifetime forces `Linker>` to be per-run. + recording: `VmState<'h>`'s lifetime forces `Linker>` to be per-run — + a design change rather than a tweak, and one the bridge forces anyway, so it is + better done with that context than before it. ### D. Hardening @@ -568,7 +673,14 @@ matter. Items marked ✓ are done. slice op; let the compiler enforce the claim. Plus `unreachable_pub` and clippy's cast lints. - All three are on, and the two warning lints each paid for themselves. + All three are on, at `deny` — the whole lint block is uniform rather than half + advisory, so a violation fails the build rather than scrolling past. Verified: + making `wasm_engine` `pub` again fails `cargo build`, not merely `clippy`. The + `#[expect]` on the one remaining cast keeps working under `deny`, and being + `expect` rather than `allow` it also fires if a restructure makes the cast + unnecessary. + + Both of the new lints paid for themselves. `unreachable_pub` found `VmState` and `wasm_engine`: `pub` inside a private module and never re-exported, so unreachable from outside the crate — now `pub(crate)`, with nothing silenced. The cast lints found **8 sites, all in `abi.rs`**. Six were @@ -676,9 +788,9 @@ useful for comparison and for the gas assertions in `Wasm_test.cpp` — not gosp **`crates/` compiles**, and the whole workspace is green — `cargo test --workspace`, `clippy --workspace --all-targets`, `fmt`, and `cargo doc -p xrpl-wasm-vm --no-deps` -(which `deny(rustdoc::broken_intra_doc_links)` now makes load-bearing). 120 tests: 33 -macro, 12 facade, 1 doctest, and **74 in `xrpl-wasm-vm`** (10 unit; 64 integration — 12 -`host_calls`, 19 `memory_policy`, 13 `budgets`, 20 `vm_limits`). +(which `deny(rustdoc::broken_intra_doc_links)` now makes load-bearing). 123 tests: 33 +macro, 12 facade, 1 doctest, and **77 in `xrpl-wasm-vm`** (10 unit; 67 integration — 12 +`host_calls`, 21 `memory_policy`, 13 `budgets`, 21 `vm_limits`). **Section A is closed, B6/B7 with it, and the B/D cleanup after that** (2026-07-30) — B8, B9, D14 and two thirds of D16. Only C10/C11/C12, D17 and D16's `gas = 0` decision