feat: Report all errors in wasm code check (#8225)

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,

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<CheckError>> {
+    let module = compile(wasm).map_err(|detail| vec![CheckError::Compile(detail)])?;
+    let refusals: Vec<CheckError> = 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<Item = CheckError> + '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<Item = CheckError> + '_ {
+    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<Item = CheckError> + '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<Item = CheckError> + '_ {
+    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<ExternType>, 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:?}"
+        );
+    }
 }

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}");
+    }
+}