diff --git a/crates/xrpl-wasm-vm/src/lib.rs b/crates/xrpl-wasm-vm/src/lib.rs index d559d564c6..f0b5fdcfe6 100644 --- a/crates/xrpl-wasm-vm/src/lib.rs +++ b/crates/xrpl-wasm-vm/src/lib.rs @@ -22,7 +22,7 @@ mod preflight; mod register; mod vm; -pub use preflight::{CheckError, check}; +pub use preflight::{CheckError, check, check_all}; pub use vm::{ MAX_FIELD_BYTES, MAX_MEMORY_BYTES, MAX_MEMORY_PAGES, MAX_TABLE_ELEMENTS, RunError, RunFailure, RunOutcome, TRANSFER_LIMIT_BYTES, run, diff --git a/crates/xrpl-wasm-vm/src/preflight/mod.rs b/crates/xrpl-wasm-vm/src/preflight/mod.rs index 8919faf0fc..0b1811a74c 100644 --- a/crates/xrpl-wasm-vm/src/preflight/mod.rs +++ b/crates/xrpl-wasm-vm/src/preflight/mod.rs @@ -7,16 +7,19 @@ //! 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 -//! 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 -//! — a trap in one is charged to the contract like any other trap. +//! Two entry points over one pass: [`check`] stops at the first refusal, which is +//! all a consensus path can act on, and [`check_all`] reports every one. Both draw +//! from [`check_error_iter`], so they cannot disagree about which refusal is first. +//! +//! One thing it deliberately does not screen: a module exporting **no** linear +//! memory passes, since 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 needs +//! no rule of its own — the engine forbids one, so such a module fails to compile. //! //! Two things it screens that a run can only discover: an exported memory, or an //! exported table, larger than the engine grants. Both read the same export list, so -//! [`check_exported_resources`] is one pass — see it for what stays invisible, and -//! why the table case leaves much more of it there. +//! [`check_exported_resources_iter`] is one pass — see it for what stays invisible, +//! and why the table case leaves much more of it there. //! //! Every rule is here but one: [`signature`] holds the comparison of an import's //! type against the ABI's, which needs machinery the rest of the stage does not. @@ -70,26 +73,36 @@ impl fmt::Display for CheckError { /// Screen `wasm`: it must compile, import only what the engine serves, export /// `function_name` as `() -> i32`, and ask for no more memory or table 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; the resource caps come last, being a 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_exported_resources(&module) + check_error_iter(&module, function_name) + .next() + .map_or(Ok(()), Err) } -/// Every import must be one the linker defines, with the type it defines it as. The -/// first that is not ends the check, so a module with several faults reports the -/// earliest. -fn check_imports(module: &Module) -> Result<(), CheckError> { - for import in module.imports() { - check_import(import.module(), import.name(), import.ty())?; +/// [`check`], reporting every error found rather than stopping at the first. +pub fn check_all(wasm: &[u8], function_name: &str) -> Result<(), Vec> { + let module = compile(wasm).map_err(|detail| vec![CheckError::Compile(detail)])?; + let refusals: Vec = check_error_iter(&module, function_name).collect(); + if refusals.is_empty() { + return Ok(()); } - Ok(()) + Err(refusals) +} + +fn check_error_iter<'a>( + module: &'a Module, + function_name: &'a str, +) -> impl Iterator + 'a { + check_imports_iter(module) + .chain(check_entry_point_iter(module, function_name)) + .chain(check_exported_resources_iter(module)) +} + +fn check_imports_iter(module: &Module) -> impl Iterator + '_ { + module + .imports() + .filter_map(|import| check_import(import.module(), import.name(), import.ty()).err()) } /// Whether the engine defines this one import, as the guest declares it. @@ -130,11 +143,15 @@ fn imported_function<'ty>(name: &str, ty: &'ty ExternType) -> Result<&'ty FuncTy } } -fn check_entry_point(module: &Module, name: &str) -> Result<(), CheckError> { - match module.get_export(name) { - Some(ExternType::Func(ty)) if is_entry_point(&ty) => Ok(()), - found => Err(CheckError::EntryPoint(entry_point_fault(found, name))), - } +fn check_entry_point_iter<'a>( + module: &'a Module, + name: &'a str, +) -> impl Iterator + 'a { + std::iter::once_with(move || match module.get_export(name) { + Some(ExternType::Func(ty)) if is_entry_point(&ty) => None, + found => Some(CheckError::EntryPoint(entry_point_fault(found, name))), + }) + .flatten() } /// The entry point's type: nothing in, one `i32` out — what [`crate::run`]'s @@ -154,22 +171,19 @@ fn is_entry_point(ty: &FuncType) -> bool { /// normal shape — and narrow for memories, since a contract needs an exported one to /// make any host call at all. /// -/// A module faulting on both is reported by whichever it declares first. Neither -/// fault explains the other, so there is no precedence to preserve — only the need -/// for every node to reach the same verdict, which export order already gives. -fn check_exported_resources(module: &Module) -> Result<(), CheckError> { - for export in module.exports() { - match export.ty() { - ExternType::Memory(ty) => { - check_initial_pages(ty.minimum()).map_err(CheckError::Memory)?; - } - ExternType::Table(ty) => { - check_initial_elements(ty.minimum()).map_err(CheckError::Table)?; - } - _ => {} - } - } - Ok(()) +/// A module faulting on both yields both, in export order. Neither fault explains +/// the other, so there is no precedence to preserve — only the need for every node to +/// reach the same verdict, which export order already gives. +fn check_exported_resources_iter(module: &Module) -> impl Iterator + '_ { + module.exports().filter_map(|export| match export.ty() { + ExternType::Memory(ty) => check_initial_pages(ty.minimum()) + .err() + .map(CheckError::Memory), + ExternType::Table(ty) => check_initial_elements(ty.minimum()) + .err() + .map(CheckError::Table), + _ => None, + }) } /// Whether the engine will grant a memory of this declared initial size. @@ -216,10 +230,10 @@ pub(crate) fn entry_point_fault(found: Option, name: &str) -> String } /// The rules, one by one, on inputs built directly rather than parsed out of a -/// module. `tests/preflight.rs` runs real modules through [`check`]; what is here is -/// what a module cannot state precisely — which rule fires, in which order, and in -/// what words the caller logs it. The signature rule's derivation is tested beside -/// it, in [`signature`]. +/// module. `tests/preflight.rs` runs real modules through [`check`] and +/// [`check_all`]; what is here is what a module cannot state precisely — which rule +/// fires and in what words the caller logs it. The signature rule's derivation is +/// tested beside it, in [`signature`]. /// /// `wat` is a dev-dependency, so the one test here that does need a module writes it /// as text like every other test in the crate. What the library must not gain is a @@ -476,4 +490,15 @@ mod tests { "a module that compiles and imports nothing reaches the entry point" ); } + + /// Compiling is the one stage that ends the walk for [`check_all`] too: there is + /// no module to read the other rules off. + #[test] + fn a_failed_compile_is_reported_alone() { + let refusals = check_all(b"not wasm", "finish").expect_err("not a module"); + assert!( + matches!(refusals.as_slice(), [CheckError::Compile(_)]), + "{refusals:?}" + ); + } } diff --git a/crates/xrpl-wasm-vm/tests/preflight.rs b/crates/xrpl-wasm-vm/tests/preflight.rs index bd1499f8e3..0a3c70449b 100644 --- a/crates/xrpl-wasm-vm/tests/preflight.rs +++ b/crates/xrpl-wasm-vm/tests/preflight.rs @@ -3,6 +3,9 @@ //! `check` reaches its verdict from the compiled module alone, so these tests take //! no host — except the ones that put the same module through `run` to compare the //! two. +//! +//! These screen with `check`, which reports the earliest refusal; the last section +//! is what `check_all` adds. mod support; @@ -735,3 +738,66 @@ fn structurally_malformed_modules_are_refused() { assert_stage!(refusal, CheckError::Compile(_)); } } + +// --------------------------------------------------------------------------- +// Reporting every refusal +// --------------------------------------------------------------------------- + +/// A module that breaks every rule past compiling, once each. +fn a_module_faulting_at_every_stage() -> String { + format!( + r#"(module + (import "host_lib" "no_such_function" (func (param i32) (result i32))) + (import "host_lib" "ldgr_index" (func (param i64 i64) (result i32))) + (memory (export "memory") {pages}) + (table (export "t") {elements} funcref) + (func (export "{ENTRY}") (result i64) (i64.const 0)))"#, + pages = MAX_MEMORY_PAGES + 1, + elements = MAX_TABLE_ELEMENTS + 1, + ) +} + +#[test] +fn check_all_reports_a_refusal_from_every_stage() { + let refusals = xrpl_wasm_vm::check_all(&assemble(&a_module_faulting_at_every_stage()), ENTRY) + .expect_err("this module breaks every rule past compiling"); + + assert!( + matches!( + refusals.as_slice(), + [ + CheckError::Import(_), + CheckError::Signature(_), + CheckError::EntryPoint(_), + CheckError::Memory(_), + CheckError::Table(_), + ] + ), + "{refusals:?}" + ); +} + +/// What lets the consensus path keep fail-fast without a second implementation of +/// the stage order to drift from. +#[test] +fn check_reports_what_check_all_reports_first() { + let wasm = assemble(&a_module_faulting_at_every_stage()); + + let first = xrpl_wasm_vm::check(&wasm, ENTRY).expect_err("five faults"); + let all = xrpl_wasm_vm::check_all(&wasm, ENTRY).expect_err("five faults"); + + assert_eq!(first.to_string(), all[0].to_string()); +} + +/// Nothing to report is `Ok`, never an empty `Vec`. +#[test] +fn check_all_passes_a_runnable_contract() { + let wat = module( + &[import::LDGR_INDEX, ONE_PAGE], + "(call $ldgr_index (i32.const 0) (i32.const 4))", + ); + + if let Err(refusals) = xrpl_wasm_vm::check_all(&assemble(&wat), ENTRY) { + panic!("expected this module to pass, but: {refusals:?}\n{wat}"); + } +}