diff --git a/crates/xrpl-wasm-vm-ffi/src/lib.rs b/crates/xrpl-wasm-vm-ffi/src/lib.rs index 9c0c3480f8..5a4c77048e 100644 --- a/crates/xrpl-wasm-vm-ffi/src/lib.rs +++ b/crates/xrpl-wasm-vm-ffi/src/lib.rs @@ -95,6 +95,8 @@ mod ffi { Import, /// No export of that name with signature `() -> i32`. EntryPoint, + /// The module asks for more linear memory than the engine grants. + Memory, /// The engine panicked. A defect in this crate or the one below it, and /// not a fault in the module — which is why it is a status of its own /// rather than one more way a contract can be malformed. @@ -377,6 +379,7 @@ impl From<&CheckError> for ffi::CheckStatus { CheckError::Compile(_) => ffi::CheckStatus::Compile, CheckError::Import(_) => ffi::CheckStatus::Import, CheckError::EntryPoint(_) => ffi::CheckStatus::EntryPoint, + CheckError::Memory(_) => ffi::CheckStatus::Memory, } } } @@ -559,6 +562,7 @@ mod tests { CheckError::Compile(String::new()), CheckError::Import(String::new()), CheckError::EntryPoint(String::new()), + CheckError::Memory(String::new()), ] } diff --git a/crates/xrpl-wasm-vm/src/preflight.rs b/crates/xrpl-wasm-vm/src/preflight.rs index a8d80a16d1..026d69b721 100644 --- a/crates/xrpl-wasm-vm/src/preflight.rs +++ b/crates/xrpl-wasm-vm/src/preflight.rs @@ -7,18 +7,21 @@ //! makes it callable from a transaction's preflight, which has no ledger to serve //! host calls from. //! -//! Two things it deliberately does not screen. A module exporting no linear +//! Two things it deliberately does not screen. A module exporting **no** linear //! memory passes: a contract that makes no host call needs none, and one that //! does is refused at the call and charged for what it burned. A start section -//! passes: it is guest code, and executing it is the one thing a check must not -//! do. +//! passes: it is guest code, and executing it is the one thing a check must not do +//! — a trap in one is charged to the contract like any other trap. +//! +//! One thing it screens that a run can only discover: an exported memory larger +//! than the engine grants. See [`check_memory`] for what stays invisible. use std::fmt; use wasmi::{ExternType, FuncType, Module, ValType}; use xrpl_host_functions::HostFunctionSpec; use crate::register::HOST_MODULE; -use crate::vm::compile; +use crate::vm::{MAX_MEMORY_PAGES, compile}; /// Why a module cannot be run. One variant per stage, since the caller maps the /// stages separately. @@ -32,6 +35,8 @@ pub enum CheckError { Import(String), /// No export named `function_name` with signature `() -> i32`. EntryPoint(String), + /// The module asks for more linear memory than the engine grants. + Memory(String), } impl fmt::Display for CheckError { @@ -42,16 +47,23 @@ impl fmt::Display for CheckError { // The detail says which of the entry point's failures this is, since // "no entry point" would be wrong for an export of the wrong type. CheckError::EntryPoint(detail) => write!(f, "{detail}"), + CheckError::Memory(detail) => write!(f, "memory: {detail}"), } } } -/// Screen `wasm`: it must compile, import only what the engine serves, and export -/// `function_name` as `() -> i32`. +/// Screen `wasm`: it must compile, import only what the engine serves, export +/// `function_name` as `() -> i32`, and ask for no more memory than it may have. +/// +/// The stages are ordered by how much of the module each explains. An import fault +/// is reported before a missing entry point because the imports are what the rest of +/// the module is built on; memory comes last, being a resource request rather than a +/// mistake about the ABI. pub fn check(wasm: &[u8], function_name: &str) -> Result<(), CheckError> { let module = compile(wasm).map_err(CheckError::Compile)?; check_imports(&module)?; - check_entry_point(&module, function_name) + check_entry_point(&module, function_name)?; + check_memory(&module) } /// Every import must be one the linker defines. The first that is not ends the @@ -104,6 +116,38 @@ fn is_entry_point(ty: &FuncType) -> bool { ty.params().is_empty() && matches!(ty.results(), [ValType::I32]) } +/// A module may not declare more linear memory than the engine grants. +/// +/// Only what it *exports* is visible here. A memory a module keeps to itself is not +/// in its exports, and the store's limiter is what refuses that one — at +/// instantiation, where the run is charged nothing and the caller cannot tell it +/// from any other resource failure. Screening the exported case covers every +/// contract built against the guest SDK, since a contract needs an exported memory +/// to make a host call at all. +fn check_memory(module: &Module) -> Result<(), CheckError> { + for export in module.exports() { + if let ExternType::Memory(ty) = export.ty() { + check_initial_pages(ty.minimum()).map_err(CheckError::Memory)?; + } + } + Ok(()) +} + +/// Whether the engine will grant a memory of this declared initial size. +/// +/// The *minimum* only: a declared maximum past the cap is legal and simply +/// unreachable, which `vm_limits::a_declared_maximum_past_the_cap_is_allowed_but_ +/// unreachable` pins on the run side. Refusing it here would turn a runnable +/// contract away. +fn check_initial_pages(pages: u64) -> Result<(), String> { + if pages > u64::from(MAX_MEMORY_PAGES) { + return Err(format!( + "initial memory of {pages} pages is past the {MAX_MEMORY_PAGES}-page cap" + )); + } + Ok(()) +} + /// How an entry-point lookup failed, in the words both stages use: a check and a /// run describe the same module the same way, and "no entry point" would send a /// contract author looking for a function they already have. @@ -250,6 +294,22 @@ mod tests { ); } + /// The cap itself is granted; one page past it is not. The boundary is the whole + /// rule, and it is the same boundary the store's limiter applies at + /// instantiation. + #[test] + fn the_initial_memory_may_reach_the_cap_but_not_pass_it() { + assert_eq!(check_initial_pages(0), Ok(())); + assert_eq!(check_initial_pages(u64::from(MAX_MEMORY_PAGES)), Ok(())); + + let past = u64::from(MAX_MEMORY_PAGES) + 1; + let refusal = check_initial_pages(past).expect_err("one page past the cap"); + assert_eq!( + refusal, + format!("initial memory of {past} pages is past the {MAX_MEMORY_PAGES}-page cap") + ); + } + /// The bridge logs this string and the C++ tests match on it, so the stage's /// prefix is part of the interface rather than a debugging aid. #[test] @@ -258,6 +318,10 @@ mod tests { CheckError::Compile("bad magic".to_string()).to_string(), "compile: bad magic" ); + assert_eq!( + CheckError::Memory("initial memory of 129 pages".to_string()).to_string(), + "memory: initial memory of 129 pages" + ); assert_eq!( CheckError::Import("no host function 'x'".to_string()).to_string(), "import: no host function 'x'" diff --git a/crates/xrpl-wasm-vm/src/vm.rs b/crates/xrpl-wasm-vm/src/vm.rs index 1b07a5c108..28643a819e 100644 --- a/crates/xrpl-wasm-vm/src/vm.rs +++ b/crates/xrpl-wasm-vm/src/vm.rs @@ -83,8 +83,9 @@ pub struct RunOutcome { pub enum RunError { /// `wasm` is not a valid module under this engine's configuration. Compile(String), - /// The module compiled but would not instantiate: an import the linker does - /// not define, an initial memory past the page cap, a trapping start section. + /// The module compiled but the engine would not accept it: an import the + /// linker does not define, or an initial memory past the page cap. Not guest + /// code failing — a start section that traps is [`RunError::Trap`]. Instantiate(String), /// No export named `function_name` with signature `() -> i32`: absent, not a /// function, or a function of another type — which the detail tells apart. @@ -99,7 +100,8 @@ pub enum RunError { /// 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. + /// access, or `memory.grow` past the page cap. Wherever the guest was + /// executing, including a start section during instantiation. Trap(String), } @@ -187,6 +189,22 @@ fn guest_halted(error: &wasmi::Error) -> Option { (error.as_trap_code() == Some(TrapCode::OutOfFuel)).then_some(RunError::OutOfGas) } +/// Why instantiation failed, once [`guest_halted`] has ruled out the two conditions +/// that can arise anywhere. +/// +/// A start section is guest code, so it can trap on its own — `unreachable`, a +/// division by zero, an out-of-bounds access — and a trap is the guest's fault +/// wherever it happens. Naming that after the *stage* would file it beside the +/// module faults a caller treats as its own defect, and charge nothing for +/// instructions the contract burned. What is left for [`RunError::Instantiate`] is a +/// module the linker or the store would not accept at all. +fn instantiation_failure(error: &wasmi::Error) -> RunError { + match error.as_trap_code() { + Some(_) => RunError::Trap(error.to_string()), + None => RunError::Instantiate(error.to_string()), + } +} + /// The outcome a host-fatal `HostError` is. /// /// Exhaustive rather than closed with a wildcard, so a variant added to the ABI @@ -306,7 +324,7 @@ pub fn run<'h>( let instance = match linker.instantiate_and_start(&mut store, &module) { Ok(instance) => instance, Err(e) => { - let error = guest_halted(&e).unwrap_or_else(|| RunError::Instantiate(e.to_string())); + let error = guest_halted(&e).unwrap_or_else(|| instantiation_failure(&e)); return Err(failed(&store, gas, error)); } }; diff --git a/crates/xrpl-wasm-vm/tests/preflight.rs b/crates/xrpl-wasm-vm/tests/preflight.rs index e8ad8c15cd..cfa29883da 100644 --- a/crates/xrpl-wasm-vm/tests/preflight.rs +++ b/crates/xrpl-wasm-vm/tests/preflight.rs @@ -375,42 +375,86 @@ fn screening_and_a_run_agree() { } } -/// The gap, listed rather than described. A start section is guest code, and -/// running it is what screening must not do; a memory the module keeps to itself -/// is not in its exports. Both leave a module that passes screening and then fails -/// to instantiate, which is why a run's own refusal cannot be treated as the -/// node's fault. +/// A module asking for more memory than the engine grants is refused, so the +/// contract that could never run does not reach the ledger. The cap itself passes. +#[test] +fn an_exported_memory_past_the_cap_does_not_pass() { + let wat = module( + &[&format!( + r#"(memory (export "memory") {})"#, + MAX_MEMORY_PAGES + 1 + )], + "(i32.const 0)", + ); + let refusal = assert_stage!(refusal(&wat), CheckError::Memory(_)).to_string(); + assert!(refusal.contains("past the 128-page cap"), "{refusal}"); + + passes(&module( + &[&format!(r#"(memory (export "memory") {MAX_MEMORY_PAGES})"#)], + "(i32.const 0)", + )); +} + +/// A declared *maximum* past the cap is legal and simply unreachable, so screening +/// must not turn it away: `vm_limits` runs this very module to completion. +#[test] +fn a_declared_maximum_past_the_cap_still_passes() { + passes(&module( + &[&format!( + r#"(memory (export "memory") 1 {})"#, + MAX_MEMORY_PAGES + 1 + )], + "(i32.const 0)", + )); +} + +/// The gap, listed rather than described, and now one entry long. A memory a module +/// keeps to itself is not in its exports, so this is the one module that passes +/// screening and then fails to *instantiate* — which is why a run's refusal at that +/// stage cannot be read as the node's fault. +/// +/// A contract needs an exported memory to make any host call, so a module of this +/// shape can do nothing but compute; the SDK does not produce one. #[test] fn what_static_screening_cannot_see() { let host = FakeHost::new(); + let wat = format!( + r#"(module (memory {}) + (func (export "finish") (result i32) (i32.const 0)))"#, + MAX_MEMORY_PAGES + 1 + ); - for (label, wat) in [ - ( - "a start section that traps", - format!( - r#"(module {ONE_PAGE} - (func $init (unreachable)) - (start $init) - (func (export "finish") (result i32) (i32.const 0)))"# - ), - ), - ( - "an unexported memory past the cap", - format!( - r#"(module (memory {}) - (func (export "finish") (result i32) (i32.const 0)))"#, - MAX_MEMORY_PAGES + 1 - ), - ), - ] { - let wasm = assemble(&wat); - passes(&wat); + passes(&wat); - let failure = xrpl_wasm_vm::run(&wasm, PLENTY_OF_GAS, &host, ENTRY) - .expect_err(&format!("{label}: expected the run to refuse it")); - assert!( - matches!(failure.error, RunError::Instantiate(_)), - "{label}: {failure}" - ); - } + let failure = xrpl_wasm_vm::run(&assemble(&wat), PLENTY_OF_GAS, &host, ENTRY) + .expect_err("the store's limiter must refuse the memory"); + assert!( + matches!(failure.error, RunError::Instantiate(_)), + "{failure}" + ); +} + +/// A start section is guest code, so screening cannot see whether it traps — but it +/// no longer has to. A trap is the guest's fault wherever it happens, so the run +/// charges the contract for what it burned instead of reporting a module the node +/// should have screened. +#[test] +fn a_start_section_screening_cannot_see_is_charged_as_a_trap() { + let host = FakeHost::new(); + let wat = format!( + r#"(module {ONE_PAGE} + (func $init (unreachable)) + (start $init) + (func (export "finish") (result i32) (i32.const 0)))"# + ); + + passes(&wat); + + let failure = xrpl_wasm_vm::run(&assemble(&wat), PLENTY_OF_GAS, &host, ENTRY) + .expect_err("a start section that traps must not complete the run"); + assert!(matches!(failure.error, RunError::Trap(_)), "{failure}"); + assert!( + failure.fuel_used > 0, + "charged for what it burned: {failure}" + ); } diff --git a/crates/xrpl-wasm-vm/tests/vm_limits.rs b/crates/xrpl-wasm-vm/tests/vm_limits.rs index 34b458321d..4fa60e8014 100644 --- a/crates/xrpl-wasm-vm/tests/vm_limits.rs +++ b/crates/xrpl-wasm-vm/tests/vm_limits.rs @@ -372,8 +372,14 @@ fn an_unused_import_is_still_linked() { /// is even looked up, and `set_fuel` and the memory limiter are both installed by /// then — so it is metered like any other guest code, and a run it stops is /// charged for what it burned. +/// +/// Reported as a **trap**, not as a module that would not instantiate: a trap is the +/// guest's fault wherever it happens, and the stage a run stopped at is not what the +/// caller maps. Filing it under the stage would put a contract's own defect among the +/// faults a caller treats as the node's, and charge nothing for the instructions the +/// contract burned reaching it. #[test] -fn a_trapping_start_section_fails_instantiation_and_is_charged() { +fn a_trapping_start_section_is_a_guest_trap_and_is_charged() { let host = FakeHost::new(); let wat = format!( @@ -384,8 +390,8 @@ fn a_trapping_start_section_fails_instantiation_and_is_charged() { ); let failure = assert_stage!( run_with_gas(&wat, PLENTY_OF_GAS, &host) - .expect_err("a start section that traps must not instantiate"), - RunError::Instantiate(_) + .expect_err("a start section that traps must not complete the run"), + RunError::Trap(_) ); assert!( failure.fuel_used > 0, @@ -393,6 +399,31 @@ fn a_trapping_start_section_fails_instantiation_and_is_charged() { ); } +/// What `RunError::Instantiate` is left to mean: a module the linker or the store +/// would not accept, rather than one whose guest code failed. Its two shapes, so the +/// variant is not left standing for nothing. +#[test] +fn instantiation_failure_is_a_module_the_engine_will_not_accept() { + let host = FakeHost::new(); + + // The linker defines no such import. + let wat = module( + &[ + r#"(import "host_lib" "no_such_function" (func $f (result i32)))"#, + ONE_PAGE, + ], + "(call $f)", + ); + assert_stage!(failure(&wat, &host), RunError::Instantiate(_)); + + // The store's limiter will not grant the memory, and does not trap to say so. + let wat = module( + &[&format!("(memory {})", MAX_MEMORY_PAGES + 1)], + "(i32.const 0)", + ); + assert_stage!(failure(&wat, &host), RunError::Instantiate(_)); +} + /// A start section that runs out of gas is reported as out of gas, not as a module /// that would not instantiate. The stage a run stopped at is not what the caller /// maps — the reason is — and gas exhaustion is one outcome wherever the guest diff --git a/docs/claude/wasm-vm/bridge.md b/docs/claude/wasm-vm/bridge.md index f72810e1a8..97d874f62d 100644 --- a/docs/claude/wasm-vm/bridge.md +++ b/docs/claude/wasm-vm/bridge.md @@ -59,14 +59,20 @@ node's, and charging a transaction for a node's defect would write that defect i |---|---|---| | `Ok` | — | `gas_used` | | `OutOfGas` | `tecOUT_OF_GAS` | `gas_used` | -| `Trap`, `NoMemory` | `tecFAILED_PROCESSING` | `gas_used` | -| `Compile`, `Instantiate`, `EntryPoint` | `tecINTERNAL` | none | +| `Trap`, `NoMemory`, `Instantiate` | `tecFAILED_PROCESSING` | `gas_used` | +| `Compile`, `EntryPoint` | `tecINTERNAL` | none | | `Internal`, `Panic` | `tecINTERNAL` | none | -The `Compile`/`Instantiate`/`EntryPoint` row is `tecINTERNAL` because preflight is meant to -have refused such a module with `temBAD_WASM` long before apply. **That row is now known to -be wrong for `Instantiate`** — see below. `NoMemory` had no old TER to match (it used to -reach the guest as code -14); `tecFAILED_PROCESSING` treats it as the contract fault it is. +`Compile` and `EntryPoint` are `tecINTERNAL` because preflight decides both from the same +bytes and the same engine, so agreement is not a matter of degree: reaching apply means the +screening did not happen. `Instantiate` is **not** in that row, and the reason is the point of +the whole arrangement — see below. `NoMemory` had no old TER to match (it used to reach the +guest as code -14); `tecFAILED_PROCESSING` treats it as the contract fault it is. + +One thing to settle before a long-lived escrow exists: `Compile` is only a node fault while +the engine's configuration never changes. A contract created under one feature set and +finished under another could legitimately fail to compile at apply, so either the config is +amendment-gated or `Compile` joins the charged row. `gas <= 0` is refused as `temBAD_AMOUNT` before the engine is called, restoring what `WasmiEngine::run` did — see [open-questions.md](open-questions.md). @@ -79,7 +85,7 @@ one answer, because a caller's only decision is whether the transaction may proc | `CheckStatus` | `NotTEC` | |---|---| | `Ok` | `tesSUCCESS` | -| `Compile`, `Import`, `EntryPoint` | `temBAD_WASM` | +| `Compile`, `Import`, `EntryPoint`, `Memory` | `temBAD_WASM` | | `Panic` | `telFAILED_PROCESSING` | The statuses stay distinct anyway: the *detail* is what a contract author needs, and one @@ -99,19 +105,23 @@ std::string_view) -> NotTEC`, with no `HostFunctions&`. The deleted `preflightEs took one and could therefore never have been called from a real `preflight()` — `PreflightContext` has no view to build a host over. -## Why `Instantiate` should stop being `tecINTERNAL` +## Why `Instantiate` is the contract's fault -`check` closes compile, imports and the entry point, but two ways instantiation fails are -invisible to it: a start section that traps, and a linear memory over the page cap that the -module does not export ([engine.md](engine.md)). Both are deterministic properties of the -module, so a contract can pass preflight, be escrowed, and then fail to instantiate at -apply — where the map currently blames the node and charges nothing. +The map must not depend on preflight being exhaustive, because it cannot be. `check` closes +compile, imports, the entry point and an exported memory over the page cap — but a memory a +module *keeps to itself* is absent from its exports, so such a module passes screening and +then fails to instantiate ([engine.md](engine.md)). That is a deterministic property of the +code, identical on every node, and nothing this node did; charging it as +`tecFAILED_PROCESSING` says so, where `tecINTERNAL` would blame the node and forgive the gas. -The fix is two lines and its own change: report `Instantiate` as `tecFAILED_PROCESSING` -with its gas, and in `vm::run` classify a failure carrying a trap code (`e.as_trap_code()`) -as `Trap` rather than `Instantiate`, since a start section trapping is guest code trapping. -`tecINTERNAL` then means what it says — `Internal` and `Panic`, the node's own defects — and -the map stops depending on preflight's completeness for its correctness. +The other half is in the engine rather than the map: `vm::instantiation_failure` reports a +failure carrying a trap code as `RunError::Trap`, because a start section that traps is guest +code trapping, and a trap is the guest's fault wherever it happens. What is left for +`Instantiate` is a module the linker or the store would not accept at all — +`vm_limits::instantiation_failure_is_a_module_the_engine_will_not_accept` pins both shapes. + +So `tecINTERNAL` now means what it says: `Internal` and `Panic`, the node's own defects, plus +the two stages preflight decides exactly. ## The one copy left on the byte path, and why it needs `HostFunctions` to change diff --git a/docs/claude/wasm-vm/engine.md b/docs/claude/wasm-vm/engine.md index 7d1636096a..4c7a51932b 100644 --- a/docs/claude/wasm-vm/engine.md +++ b/docs/claude/wasm-vm/engine.md @@ -30,7 +30,9 @@ cost that cannot be read becomes `RunError::Internal` rather than a number — ` forgive a run its whole cost and `gas` would charge an untouched one for everything. `guest_halted` asks "did the guest halt?" at *every* stage from instantiation on, so a start section that burns the limit is `OutOfGas`, not `Instantiate`: the stage a run stopped at is -not what the caller maps. +not what the caller maps. `instantiation_failure` finishes the thought — a failure carrying a +trap code is `Trap`, since a start section that traps is guest code trapping — leaving +`Instantiate` to mean a module the linker or the store would not accept. **Two ways a byte answer reaches the guest**, both taking a `Region`: @@ -129,12 +131,20 @@ unit tests state each rule, its precedence and its wording on inputs built direc an import breaking two rules reports the namespace, which is what explains the module's other imports too. -What it cannot see is guest behaviour and anything absent from the module's exports: a start -section that traps, and a linear memory over the page cap that the module keeps to itself. -Both pass the check and then fail instantiation, which is why a run's own refusal at that -stage cannot be read as the node's fault. `what_static_screening_cannot_see` lists them and -`screening_and_a_run_agree` pins the equivalence everywhere else — in both directions, so a -rule that refused a contract the engine would have served fails too. +A fourth stage screens what a module *declares*: an exported linear memory whose initial size +is past the page cap is refused, since the store's limiter would refuse it anyway. The +**minimum** only — a declared maximum past the cap is legal and simply unreachable. Only the +exported memory is visible, which is enough for every contract the guest SDK produces, since +a contract needs an exported memory to make a host call at all. + +What stays invisible is one module: a memory the module keeps to itself, over the cap, which +passes the check and then fails instantiation — the reason a run's refusal at that stage is +charged to the contract rather than blamed on the node ([bridge.md](bridge.md)). A start +section is invisible too, but no longer matters: a trap in one is reported as +`RunError::Trap` and charged like any other trap. +`what_static_screening_cannot_see` is that one module, and `screening_and_a_run_agree` pins +the equivalence everywhere else — in both directions, so a rule that refused a contract the +engine would have served fails too. Import **signatures** are the deliberate gap: `check` compares names and kinds, not types, so a mistyped import still parts a module from the engine at instantiation. Closing it needs the diff --git a/docs/claude/wasm-vm/index.md b/docs/claude/wasm-vm/index.md index 30d083b835..c8ac87f9a3 100644 --- a/docs/claude/wasm-vm/index.md +++ b/docs/claude/wasm-vm/index.md @@ -46,7 +46,8 @@ than guessing. See [history.md](history.md) for what is worth recovering. - `xrpl-host-functions-macros/` — the proc macro. An implementation detail of the crate above, deliberately not re-exported: the ABI has one declaration site. - `xrpl-wasm-vm/` — the wasmi wrapper. `vm.rs` (engine, store, `run`), `preflight.rs` - (`check` — compile, imports, entry point, with no host, store or gas), `abi.rs` (gas, + (`check` — compile, imports, entry point, declared memory, with no host, store or + gas), `abi.rs` (gas, transfer budget, guest-memory marshaling), `region.rs` (the `(ptr, len)` type), `register.rs` (one `func_wrap` per host function). See [engine.md](engine.md). - `xrpl-wasm-vm-ffi/` — the cxx bridge, all three crossings. `RunStatus`/`RunResult` and @@ -70,10 +71,10 @@ than guessing. See [history.md](history.md) for what is worth recovering. ## Current state (2026-08-04) **The whole workspace is green**: `cargo test --workspace`, `clippy --workspace ---all-targets`, `fmt`, and `cargo doc -p xrpl-wasm-vm --no-deps`. **168 tests** — 33 macro, -12 facade, 1 doctest, **105 in `xrpl-wasm-vm`** (19 unit; 86 integration — 13 `budgets`, -12 `host_calls`, 23 `memory_policy`, 17 `preflight`, 21 `vm_limits`), 15 in -`xrpl-wasm-vm-ffi`, 2 in `xrpl-wasm-testkit`. On the C++ side, **37 tests over the whole +--all-targets`, `fmt`, and `cargo doc -p xrpl-wasm-vm --no-deps`. **173 tests** — 33 macro, +12 facade, 1 doctest, **110 in `xrpl-wasm-vm`** (20 unit; 90 integration — 13 `budgets`, +12 `host_calls`, 23 `memory_policy`, 20 `preflight`, 22 `vm_limits`), 15 in +`xrpl-wasm-vm-ffi`, 2 in `xrpl-wasm-testkit`. On the C++ side, **40 tests over the whole loop** in seven fixtures: `./xrpl_tests --gtest_filter='WasmVMTest.*:*Call.*:PreflightTest.*'`. @@ -89,12 +90,7 @@ functions are registered (`ldgr_index`, `home_le_field`, `sha512_half`, `trace`, ## Next -1. **Move `Instantiate` off `tecINTERNAL`**, and report a trapping start section as `Trap` - rather than as a module that would not instantiate. Two lines plus their tests, and it is - what actually removes the papering-over: `check` cannot see either of the two remaining - instantiate faults, so the apply-side map must stop depending on preflight's - completeness. [bridge.md](bridge.md) has the reasoning. -2. **A caller.** `EscrowFinish.cpp` still has no wasm reference, so `runEscrowWasm` and +1. **A caller.** `EscrowFinish.cpp` still has no wasm reference, so `runEscrowWasm` and `preflightEscrowWasm` are reached only from `src/tests/libxrpl/tx/wasm/`. Wiring it up is what makes `WasmHostFunctionsImpl` (over a real `ApplyContext`) the host in production rather than in principle. **Blocked on the protocol fields**: `FinishFunction` and @@ -102,16 +98,16 @@ functions are registered (`ldgr_index`, `home_le_field`, `sha512_half`, `trace`, `FinishFunction` also has to answer the **contract code-size cap** — there is none, and preflight's cost is linear in the blob (a 249 KB module of duplicate imports measures 1.5 ms, mostly wasmi's own parse). -3. **Import signatures at preflight.** `check` compares an import's namespace, name and +2. **Import signatures at preflight.** `check` compares an import's namespace, name and kind, not its type, so a mistyped import still parts a module from the engine at instantiation. Deferred to when `host_functions!` generates the wasm-level lowering, which the C header and the typed `link_*` shims in [abi.md](abi.md) both want anyway. Note the deleted C++ `check` did not compare signatures either, so this is inherited rather than new. -4. **A gas parity oracle.** `Wasm_test.cpp` asserts exact gas numbers (e.g. 29'502) and is +3. **A gas parity oracle.** `Wasm_test.cpp` asserts exact gas numbers (e.g. 29'502) and is the best oracle we have, but it is commented out and its fixtures cannot run on this engine — see the `env` finding in [testing.md](testing.md). -5. **The `Bytes`-by-value copy in `HostFunctions`** — [bridge.md](bridge.md). A +4. **The `Bytes`-by-value copy in `HostFunctions`** — [bridge.md](bridge.md). A 49-signature sweep, so it wants a caller to measure against first. Also open: the two performance items and the ABI questions in diff --git a/src/libxrpl/tx/wasm/WasmVM.cpp b/src/libxrpl/tx/wasm/WasmVM.cpp index ce5965827b..f8ba707a58 100644 --- a/src/libxrpl/tx/wasm/WasmVM.cpp +++ b/src/libxrpl/tx/wasm/WasmVM.cpp @@ -51,14 +51,20 @@ outcome(rs::wasm_vm::RunResult const& run) // its host calls need - so it is charged for what it burned reaching that point. case RunStatus::Trap: case RunStatus::NoMemory: + // A module that will not instantiate is the contract's fault too. Screening + // cannot see every way this happens - a linear memory the module keeps to itself + // is absent from its exports - so a module can pass preflight and still be + // refused here. It is a deterministic property of the code either way, and one + // this node's own conduct had no part in. + case RunStatus::Instantiate: return std::unexpected(WasmTER{.ter = tecFAILED_PROCESSING, .cost = cost}); - // A module that will not compile, instantiate, or expose the entry point should - // have been refused at preflight with `temBAD_WASM`. Reaching apply means the - // screening did not happen, which is a node-side fault rather than the - // transaction's. + // A module that will not compile, or does not expose the entry point, should have + // been refused at preflight with `temBAD_WASM`: screening decides both from the + // same bytes and the same engine, so agreeing here is not a matter of degree. + // Reaching apply means the screening did not happen, which is a node-side fault + // rather than the transaction's. case RunStatus::Compile: - case RunStatus::Instantiate: case RunStatus::EntryPoint: // The host could not serve a call, or it threw and `HostContext` caught it. case RunStatus::Internal: @@ -117,11 +123,13 @@ verdict(CheckStatus status) case CheckStatus::Ok: return tesSUCCESS; - // The module will not compile, imports what no engine of this ABI serves, or - // does not export the entry point as `() -> i32`. + // The module will not compile, imports what no engine of this ABI serves, does + // not export the entry point as `() -> i32`, or asks for more linear memory than + // it may have. case CheckStatus::Compile: case CheckStatus::Import: case CheckStatus::EntryPoint: + case CheckStatus::Memory: return temBAD_WASM; // The engine panicked: a defect in the engine, reported rather than fatal to diff --git a/src/tests/libxrpl/tx/wasm/Preflight.cpp b/src/tests/libxrpl/tx/wasm/Preflight.cpp index 296b5063aa..21eddcb003 100644 --- a/src/tests/libxrpl/tx/wasm/Preflight.cpp +++ b/src/tests/libxrpl/tx/wasm/Preflight.cpp @@ -103,6 +103,28 @@ TEST_F(PreflightTest, ImportFromAnotherModuleIsRefused) EXPECT_THAT(logged(), testing::HasSubstr("is not from 'host_lib'")); } +// A contract asking for more linear memory than the engine grants can never run, so it is +// refused before it can be escrowed. The cap itself is granted. +TEST_F(PreflightTest, MemoryPastTheCapIsRefused) +{ + constexpr std::string_view tooMuch = R"wat( + (module + (memory (export "memory") 129) + (func (export "escrow_finish") (result i32) (i32.const 0))) + )wat"; + + EXPECT_EQ(preflight(tooMuch), temBAD_WASM); + EXPECT_THAT(logged(), testing::HasSubstr("memory: initial memory of 129 pages")); + + constexpr std::string_view atTheCap = R"wat( + (module + (memory (export "memory") 128) + (func (export "escrow_finish") (result i32) (i32.const 0))) + )wat"; + + EXPECT_EQ(preflight(atTheCap), tesSUCCESS); +} + TEST_F(PreflightTest, MissingEntryPointIsRefused) { constexpr std::string_view wat = R"wat( diff --git a/src/tests/libxrpl/tx/wasm/WasmVM.cpp b/src/tests/libxrpl/tx/wasm/WasmVM.cpp index 4d80d07e70..a77bbc4555 100644 --- a/src/tests/libxrpl/tx/wasm/WasmVM.cpp +++ b/src/tests/libxrpl/tx/wasm/WasmVM.cpp @@ -120,6 +120,49 @@ TEST_F(WasmVMTest, HostCallWithNoExportedMemoryFails) EXPECT_TRUE(outcome.error().cost.has_value()); } +// A module that will not instantiate is the contract's fault and is charged, not the node's. +// Screening does not see every way this happens - a linear memory the module keeps to itself +// is absent from its exports - so such a module can pass preflight and still be refused here. +TEST_F(WasmVMTest, ModuleThatWillNotInstantiateIsChargedToTheContract) +{ + // 129 pages, not exported, so nothing outside the module declares it. + constexpr std::string_view wat = R"wat( + (module + (memory 129) + (func (export "escrow_finish") (result i32) (i32.const 0))) + )wat"; + + EXPECT_EQ(preflightEscrowWasm(assembleWat(wat), beast::Journal{sink_}), tesSUCCESS) + << "screening cannot see an unexported memory"; + + auto const outcome = run(wat); + + ASSERT_FALSE(outcome.has_value()); + EXPECT_EQ(outcome.error().ter, tecFAILED_PROCESSING); + EXPECT_TRUE(outcome.error().cost.has_value()); +} + +// A start section is guest code, so a trap in one is the contract's fault wherever it +// happens - charged for what it burned, rather than reported as a module the node should +// have screened. +TEST_F(WasmVMTest, TrappingStartSectionIsChargedToTheContract) +{ + constexpr std::string_view wat = R"wat( + (module + (memory (export "memory") 1) + (func $init (unreachable)) + (start $init) + (func (export "escrow_finish") (result i32) (i32.const 0))) + )wat"; + + auto const outcome = run(wat); + + ASSERT_FALSE(outcome.has_value()); + EXPECT_EQ(outcome.error().ter, tecFAILED_PROCESSING); + ASSERT_TRUE(outcome.error().cost.has_value()); + EXPECT_GT(*outcome.error().cost, 0) << "the start section's instructions are metered"; +} + // Preflight is meant to refuse these with `temBAD_WASM`; reaching apply means the screening // did not happen, which is the node's fault and not the transaction's. TEST_F(WasmVMTest, UnrunnableModuleIsNodeSideFault)