From 9a4b8f1773cfb5a14182891f62be2c8ecba76dd6 Mon Sep 17 00:00:00 2001 From: unknown Date: Sat, 26 Sep 2026 14:53:05 +0100 Subject: [PATCH 1/3] test(host-function-contract): harden fixture, tests, and harness docs MIME-Version: 1.0 Content-Type: text/plain; charset=UTF-8 Content-Transfer-Encoding: 8bit Resolve four maintenance issues in the host-function benchmark fixture: - #718: extract the duplicated identity-Map seeding loop shared by every map_* entry point into a documented private `support` submodule, leaving the public contract interface unchanged. - #716: cover every fixture entry point (ledger reads, crypto, allocation, Map ops) in both native and WASM registration, including the zero/empty edge cases. - #717: document the shared WASM loader's purpose, control flow, item behaviour, and panics. - #715: load the WASM artifact once per test through the shared helper and reuse it across sizes instead of re-reading the file for every measurement, removing the fragile hardcoded relative path. 🤖 Generated with Codebuff Co-Authored-By: Codebuff --- host-function-contract/src/lib.rs | 78 +++-- host-function-contract/tests/common/mod.rs | 69 ++++- host-function-contract/tests/contract.rs | 270 +++++++++++++++++- .../tests/measure_map_gap.rs | 135 +++++---- pr_body.md | 173 ++++++++++- 5 files changed, 630 insertions(+), 95 deletions(-) diff --git a/host-function-contract/src/lib.rs b/host-function-contract/src/lib.rs index 43ee73fd..717807c5 100644 --- a/host-function-contract/src/lib.rs +++ b/host-function-contract/src/lib.rs @@ -11,15 +11,54 @@ //! host-function overhead from other billing dimensions (such as read/write bytes or VM instructions). //! //! It also hosts the Map-operation fixtures (`map_insert`, `map_get`, `map_remove`, -//! `map_iterate`), which isolate Soroban [`Map`] host calls without storage, event, or -//! arithmetic side-effects, for the local-vs-network Map cost-gap measurement. +//! `map_iterate`), which isolate Soroban [`Map`](soroban_sdk::Map) host calls without +//! storage, event, or arithmetic side-effects, for the local-vs-network Map +//! cost-gap measurement. //! //! See `README.md` and `MEASUREMENTS.md` at the repository root for detailed methodology //! and captured figures. +//! +//! ## Module layout +//! +//! Each public entry point below performs exactly one repeated host-function +//! operation, so a measurement of that entry point isolates that operation. The +//! reusable Map setup is factored into the private `support` submodule so the +//! four `map_*` entry points do not each repeat the same seeding loop (#718). +//! The public contract interface is unchanged by that factoring. #![no_std] -use soroban_sdk::{contract, contractimpl, Bytes, Env, Map}; +use soroban_sdk::{contract, contractimpl, Bytes, Env}; + +/// Internal helper components shared by the contract entry points below. +/// +/// Factoring this logic out of the `#[contractimpl]` block keeps each public +/// entry point short and focused on the single host-function operation it +/// exists to measure, and gives the shared setup one place to be documented, +/// tested, and changed (#718). +mod support { + use soroban_sdk::{Env, Map}; + + /// Builds a fresh `Map` seeded with `size` identity entries. + /// + /// Entry `i` maps the key `i` to the value `i`, so the result is + /// deterministic and every key is distinct. Building the map performs + /// exactly `size` `Map::set` host calls. + /// + /// The `map_get`, `map_remove`, and `map_iterate` fixtures each build this + /// identical map before measuring their own operation, so subtracting the + /// cost of `map_insert(size)` from theirs yields the marginal cost of the + /// operation under measurement. + /// + /// A `size` of `0` returns an empty map without invoking any host call. + pub(super) fn seed_identity_map(env: &Env, size: u32) -> Map { + let mut map = Map::new(env); + for i in 0..size { + map.set(i, i); + } + map + } +} /// Benchmark contract fixture for measuring the gap between local budget estimates /// and live network simulation figures for repeated host-function calls. @@ -67,7 +106,7 @@ impl HostFunctionBenchmark { /// Hashes a small input buffer with SHA-256 for `iterations` count and /// returns the number of iterations completed. /// - /// Each iteration allocates an 8-byte `Bytes` value and passes it through + /// Each iteration passes the same pre-built input through /// `env.crypto().sha256()`, exercising the cryptographic host function /// category. The return value prevents dead-code elimination while /// keeping the function free of storage side-effects. @@ -102,11 +141,8 @@ impl HostFunctionBenchmark { /// arithmetic side-effects. The return value prevents dead-code /// elimination. `size` is the map size / insert count. pub fn map_insert(env: Env, size: u32) -> u32 { - let mut m: Map = Map::new(&env); - - for i in 0..size { - m.set(i, i); - } + // The seeding loop *is* the measurement; the map itself is discarded. + let _map = support::seed_identity_map(&env, size); size } @@ -118,13 +154,10 @@ impl HostFunctionBenchmark { /// the same `size`, so the marginal cost of the get loop is obtained by /// subtracting `map_insert(size)`. This isolates the cost of Map lookups. pub fn map_get(env: Env, size: u32) -> u32 { - let mut m: Map = Map::new(&env); - for i in 0..size { - m.set(i, i); - } + let map = support::seed_identity_map(&env, size); for i in 0..size { - let _v = m.get(i); + let _v = map.get(i); } size @@ -137,13 +170,10 @@ impl HostFunctionBenchmark { /// the same `size`, so the marginal cost of the remove loop is obtained by /// subtracting `map_insert(size)`. This isolates the cost of Map removals. pub fn map_remove(env: Env, size: u32) -> u32 { - let mut m: Map = Map::new(&env); - for i in 0..size { - m.set(i, i); - } + let mut map = support::seed_identity_map(&env, size); for i in 0..size { - let _ = m.remove(i); + let _ = map.remove(i); } size @@ -156,14 +186,14 @@ impl HostFunctionBenchmark { /// the same `size`, so the marginal cost of the iteration is obtained by /// subtracting `map_insert(size)`. This isolates the cost of Map /// iteration. + /// + /// Because the map is seeded with identity entries, the returned sum is + /// well-defined for tests (`0 + 1 + … + (size - 1)`). pub fn map_iterate(env: Env, size: u32) -> u32 { - let mut m: Map = Map::new(&env); - for i in 0..size { - m.set(i, i); - } + let map = support::seed_identity_map(&env, size); let mut sum: u32 = 0; - for (_k, v) in m.iter() { + for (_k, v) in map.iter() { sum = sum.wrapping_add(v); } diff --git a/host-function-contract/tests/common/mod.rs b/host-function-contract/tests/common/mod.rs index aa28c711..df09cd5b 100644 --- a/host-function-contract/tests/common/mod.rs +++ b/host-function-contract/tests/common/mod.rs @@ -1,14 +1,35 @@ -//! Shared helper for the host-function-contract integration tests. +//! Shared helper for the host-function-contract integration tests (#480, #717). //! -//! The tests exercise the contract as WASM — not just as native Rust — so the -//! fixture is verified to build for the `wasm32v1-none` target used by the -//! rest of the workspace (issue #480). This module resolves and loads the -//! pre-built `host_function_contract.wasm` artifact, building it on demand -//! when it is missing or stale so the tests run as part of `cargo test -//! --workspace` with no extra setup. +//! # Why this module exists +//! +//! The tests that exercise the benchmark fixture do not only run it as native +//! Rust: they also register the pre-built `host_function_contract.wasm` artifact +//! into the test environment, so the fixture is verified against the same +//! `wasm32v1-none` target the rest of the workspace uses for budget +//! measurement. Trusting a stale or hand-built artifact would let the tests +//! pass against code that is no longer in the tree, so this module resolves the +//! artifact path and loads the bytes, building the artifact on demand whenever +//! it is missing or older than the contract sources. +//! +//! # How it fits together +//! +//! 1. [`workspace_root`] locates the workspace root from this crate's +//! `CARGO_MANIFEST_DIR` (the crate sits directly under the root). +//! 2. [`wasm_path`] turns a target triple into the expected artifact path under +//! `target//release/`, honouring `CARGO_TARGET_DIR` when set. +//! 3. [`artifact_needs_rebuild`] compares the artifact's mtime against the +//! contract sources in [`STALE_SOURCES`]. +//! 4. [`load_contract_wasm`] serialises the check-then-build sequence under +//! [`BUILD_LOCK`] and returns the artifact bytes. //! //! The artifact path honours `CARGO_TARGET_DIR` when it is set and otherwise -//! falls back to the workspace-root `target/` directory. +//! falls back to the workspace-root `target/` directory, so the helper works +//! from any test working directory and with any custom target-dir setup. + +// Each integration-test binary in this directory declares `mod common;` and +// uses only the helpers it needs, so the rest can look unused to a single +// binary. Same idiom as `amm-pool-contract/tests/common/mod.rs`. +#![allow(dead_code)] use std::path::{Path, PathBuf}; use std::process::Command; @@ -25,9 +46,16 @@ const STALE_SOURCES: &[&str] = &["src/lib.rs", "Cargo.toml"]; /// Serialises the check-then-build sequence so tests running in parallel /// inside one test binary never race on the same artifact. +/// +/// Without this, two tests could both observe a missing artifact and spawn +/// concurrent `cargo build` invocations, or one could read the file while the +/// other is truncating it. static BUILD_LOCK: Mutex<()> = Mutex::new(()); /// The workspace root: this crate's manifest directory sits directly under it. +/// +/// `CARGO_MANIFEST_DIR` is expanded at compile time, so this requires no +/// runtime environment lookup and cannot fail for a missing variable. fn workspace_root() -> &'static Path { Path::new(env!("CARGO_MANIFEST_DIR")) .parent() @@ -36,7 +64,12 @@ fn workspace_root() -> &'static Path { /// Absolute path to the `host_function_contract.wasm` artifact for `target`, /// honouring `CARGO_TARGET_DIR` when it is set. +/// +/// The returned path is not guaranteed to exist: callers that need the bytes +/// should use [`load_contract_wasm`] instead. pub fn wasm_path(target: &str) -> PathBuf { + // Mirror Cargo's own precedence: an explicit `CARGO_TARGET_DIR` wins, + // otherwise the workspace-root `target/` directory is used. let target_dir = std::env::var_os("CARGO_TARGET_DIR") .map(PathBuf::from) .unwrap_or_else(|| workspace_root().join("target")); @@ -46,6 +79,11 @@ pub fn wasm_path(target: &str) -> PathBuf { /// Loads the `host_function_contract.wasm` bytes for `target`, building the /// artifact first when it is missing or stale. /// +/// The check and the build happen under [`BUILD_LOCK`], so concurrent callers +/// observe either a complete artifact or a single build, never a partial write. +/// +/// # Panics +/// /// Panics with a clear message if the build or the read fails. pub fn load_contract_wasm(target: &str) -> Vec { let _guard = BUILD_LOCK.lock().unwrap_or_else(PoisonError::into_inner); @@ -64,11 +102,18 @@ pub fn load_contract_wasm(target: &str) -> Vec { /// Whether the artifact at `wasm` is missing or older than the contract /// sources it was built from. +/// +/// A missing artifact or an artifact whose mtime cannot be read is treated as +/// stale, so the caller rebuilds. A source whose mtime cannot be read is +/// skipped: an unreadable source cannot make a readable artifact stale. fn artifact_needs_rebuild(wasm: &Path) -> bool { let Ok(wasm_meta) = std::fs::metadata(wasm) else { + // Artifact does not exist yet — must build. return true; }; let Ok(wasm_mtime) = wasm_meta.modified() else { + // The platform cannot report an mtime — rebuild rather than risk + // trusting an artifact we cannot compare. return true; }; STALE_SOURCES.iter().any(|source| { @@ -82,6 +127,14 @@ fn artifact_needs_rebuild(wasm: &Path) -> bool { } /// Builds the `host-function-contract` cdylib for `target` in release mode. +/// +/// Uses the `CARGO` executable Cargo set for this build (`env!("CARGO")`) +/// rather than assuming `cargo` is on `PATH`, so the correct toolchain is used +/// even when the tests are launched through a rustup proxy. +/// +/// # Panics +/// +/// Panics if the build command cannot be spawned or exits unsuccessfully. fn build_contract_wasm(target: &str) { let status = Command::new(env!("CARGO")) .args([ diff --git a/host-function-contract/tests/contract.rs b/host-function-contract/tests/contract.rs index 89578995..a3809abc 100644 --- a/host-function-contract/tests/contract.rs +++ b/host-function-contract/tests/contract.rs @@ -1,4 +1,4 @@ -//! Tests for the host-function measurement fixture (issue #480). +//! Integration tests for the host-function measurement fixture (issues #480, #716). //! //! The fixture exists so host-function costs can be measured against //! something small and predictable: `repeated_sequence` repeatedly calls the @@ -7,11 +7,24 @@ //! used by the host-function-call row in `MEASUREMENTS.md`, so it is not dead //! fixture code. //! +//! This file exercises **every** entry point on the fixture, not just +//! `repeated_sequence`, and covers the zero/empty edge case of each (#716): +//! +//! * ledger reads — `repeated_sequence`, `repeated_timestamp`; +//! * crypto — `repeated_hash`; +//! * allocation — `repeated_bytes_new`; +//! * Map operations — `map_insert`, `map_get`, `map_remove`, `map_iterate`. +//! //! Every assertion is run twice: once against the contract registered as //! native Rust (`env.register`) and once against the contract registered from //! its built `wasm32v1-none` artifact (`register_contract_wasm`), so the //! fixture is exercised at the same WASM level the rest of the workspace uses //! for budget measurement, not just as a native `cargo test`. +//! +//! The tests assert on the documented return values (iteration/size counts and +//! the identity-sum from `map_iterate`). They deliberately keep iteration +//! counts and map sizes small: this file checks correctness, while the +//! `measure_*` harnesses produce the cost figures. mod common; @@ -42,7 +55,13 @@ fn wasm_client(env: &Env) -> HostFunctionBenchmarkClient<'_> { HostFunctionBenchmarkClient::new(env, &contract_id) } -// ── Native registration ─────────────────────────────────────────────────── +/// Sum returned by `map_iterate(size)`: the map is seeded with identity entries +/// `i -> i` for `i` in `0..size`, so the total is `size * (size - 1) / 2`. +fn identity_sum(size: u32) -> u32 { + (u64::from(size) * u64::from(size).saturating_sub(1) / 2) as u32 +} + +// ── Native registration: ledger reads ───────────────────────────────────── #[test] fn repeated_sequence_returns_the_current_ledger_sequence() { @@ -75,7 +94,159 @@ fn repeated_sequence_is_deterministic_across_iteration_counts() { } } -// ── WASM registration ───────────────────────────────────────────────────── +#[test] +fn repeated_timestamp_returns_the_current_ledger_timestamp() { + let env = Env::default(); + let client = native_client(&env); + + let expected = env.ledger().timestamp(); + assert_eq!(client.repeated_timestamp(&1_000), expected); +} + +#[test] +fn repeated_timestamp_with_zero_iterations_returns_zero() { + let env = Env::default(); + let client = native_client(&env); + + assert_eq!(client.repeated_timestamp(&0), 0); +} + +#[test] +fn repeated_timestamp_is_deterministic_across_iteration_counts() { + let env = Env::default(); + let client = native_client(&env); + + // Reading the timestamp does not advance the ledger, so every iteration + // count must observe the same value. + let expected = env.ledger().timestamp(); + for iterations in [1, 10, 1_000, 10_000] { + assert_eq!(client.repeated_timestamp(&iterations), expected); + } +} + +// ── Native registration: crypto and allocation ──────────────────────────── + +#[test] +fn repeated_hash_returns_the_iteration_count() { + let env = Env::default(); + let client = native_client(&env); + + for iterations in [0, 1, 10, 100] { + assert_eq!(client.repeated_hash(&iterations), iterations); + } +} + +#[test] +fn repeated_hash_with_zero_iterations_returns_zero() { + let env = Env::default(); + let client = native_client(&env); + + // No digest is computed, and the counter stays at its initial value. + assert_eq!(client.repeated_hash(&0), 0); +} + +#[test] +fn repeated_bytes_new_returns_the_iteration_count() { + let env = Env::default(); + let client = native_client(&env); + + for iterations in [0, 1, 10, 100] { + assert_eq!(client.repeated_bytes_new(&iterations), iterations); + } +} + +#[test] +fn repeated_bytes_new_with_zero_iterations_returns_zero() { + let env = Env::default(); + let client = native_client(&env); + + assert_eq!(client.repeated_bytes_new(&0), 0); +} + +// ── Native registration: Map operations ─────────────────────────────────── + +#[test] +fn map_insert_returns_the_number_of_inserts() { + let env = Env::default(); + let client = native_client(&env); + + for size in [0, 1, 5, 100] { + assert_eq!(client.map_insert(&size), size); + } +} + +#[test] +fn map_insert_with_zero_size_returns_zero() { + let env = Env::default(); + let client = native_client(&env); + + assert_eq!(client.map_insert(&0), 0); +} + +#[test] +fn map_get_returns_the_number_of_lookups() { + let env = Env::default(); + let client = native_client(&env); + + for size in [0, 1, 5, 100] { + assert_eq!(client.map_get(&size), size); + } +} + +#[test] +fn map_get_with_zero_size_returns_zero() { + let env = Env::default(); + let client = native_client(&env); + + assert_eq!(client.map_get(&0), 0); +} + +#[test] +fn map_remove_returns_the_number_of_removals() { + let env = Env::default(); + let client = native_client(&env); + + for size in [0, 1, 5, 100] { + assert_eq!(client.map_remove(&size), size); + } +} + +#[test] +fn map_remove_with_zero_size_returns_zero() { + let env = Env::default(); + let client = native_client(&env); + + assert_eq!(client.map_remove(&0), 0); +} + +#[test] +fn map_iterate_returns_the_sum_of_all_values() { + let env = Env::default(); + let client = native_client(&env); + + for size in [0, 1, 5, 10] { + assert_eq!(client.map_iterate(&size), identity_sum(size)); + } +} + +#[test] +fn map_iterate_with_empty_map_returns_zero() { + let env = Env::default(); + let client = native_client(&env); + + assert_eq!(client.map_iterate(&0), 0); +} + +#[test] +fn map_iterate_with_single_zero_valued_entry_returns_zero() { + let env = Env::default(); + let client = native_client(&env); + + // The only entry is `0 -> 0`, so the accumulator remains at zero. + assert_eq!(client.map_iterate(&1), 0); +} + +// ── WASM registration: ledger reads ─────────────────────────────────────── #[test] fn wasm_repeated_sequence_returns_the_current_ledger_sequence() { @@ -104,3 +275,96 @@ fn wasm_repeated_sequence_is_deterministic_across_iteration_counts() { assert_eq!(client.repeated_sequence(&iterations), expected); } } + +#[test] +fn wasm_repeated_timestamp_returns_the_current_ledger_timestamp() { + let env = Env::default(); + let client = wasm_client(&env); + + let expected = env.ledger().timestamp(); + assert_eq!(client.repeated_timestamp(&1_000), expected); +} + +#[test] +fn wasm_repeated_timestamp_with_zero_iterations_returns_zero() { + let env = Env::default(); + let client = wasm_client(&env); + + assert_eq!(client.repeated_timestamp(&0), 0); +} + +// ── WASM registration: crypto and allocation ────────────────────────────── + +#[test] +fn wasm_repeated_hash_returns_the_iteration_count() { + let env = Env::default(); + let client = wasm_client(&env); + + for iterations in [0, 1, 10, 100] { + assert_eq!(client.repeated_hash(&iterations), iterations); + } +} + +#[test] +fn wasm_repeated_bytes_new_returns_the_iteration_count() { + let env = Env::default(); + let client = wasm_client(&env); + + for iterations in [0, 1, 10, 100] { + assert_eq!(client.repeated_bytes_new(&iterations), iterations); + } +} + +// ── WASM registration: Map operations ───────────────────────────────────── + +#[test] +fn wasm_map_insert_returns_the_number_of_inserts() { + let env = Env::default(); + let client = wasm_client(&env); + + for size in [0, 1, 5, 100] { + assert_eq!(client.map_insert(&size), size); + } +} + +#[test] +fn wasm_map_get_returns_the_number_of_lookups() { + let env = Env::default(); + let client = wasm_client(&env); + + for size in [0, 1, 5, 100] { + assert_eq!(client.map_get(&size), size); + } +} + +#[test] +fn wasm_map_remove_returns_the_number_of_removals() { + let env = Env::default(); + let client = wasm_client(&env); + + for size in [0, 1, 5, 100] { + assert_eq!(client.map_remove(&size), size); + } +} + +#[test] +fn wasm_map_iterate_returns_the_sum_of_all_values() { + let env = Env::default(); + let client = wasm_client(&env); + + for size in [0, 1, 5, 10] { + assert_eq!(client.map_iterate(&size), identity_sum(size)); + } +} + +#[test] +fn wasm_map_operations_with_zero_size_return_zero() { + let env = Env::default(); + let client = wasm_client(&env); + + // Every Map entry point must accept an empty map without erroring. + assert_eq!(client.map_insert(&0), 0); + assert_eq!(client.map_get(&0), 0); + assert_eq!(client.map_remove(&0), 0); + assert_eq!(client.map_iterate(&0), 0); +} diff --git a/host-function-contract/tests/measure_map_gap.rs b/host-function-contract/tests/measure_map_gap.rs index c1f39082..d0b5386c 100644 --- a/host-function-contract/tests/measure_map_gap.rs +++ b/host-function-contract/tests/measure_map_gap.rs @@ -1,64 +1,97 @@ -#[cfg(test)] -mod measure_map_gap { - use host_function_contract::HostFunctionBenchmarkClient; - use soroban_sdk::Env; +//! Local-vs-network cost-gap measurement for Soroban `Map` host operations +//! (issue #715). +//! +//! Inserts, lookups, removals, and iteration over a `Map` are all +//! billed by the host, but at different per-operation rates. This harness +//! registers the fixture WASM in a fresh environment for each measurement and +//! records the CPU instruction cost of a single `map_*` call, so the local +//! estimate can be compared against the matching `simulateTransaction` figure +//! recorded in `MEASUREMENTS.md`. +//! +//! Each non-insert operation first performs the same `size`-entry build that +//! the `map_insert` fixture performs, so the marginal per-operation cost is +//! `(map_(size) - map_insert(size)) / size`. +//! +//! # Running +//! +//! ```bash +//! cargo build --target wasm32v1-none --release -p host-function-contract +//! cargo test -p host-function-contract --test measure_map_gap -- --nocapture +//! ``` - /// Registers the WASM in a fresh `Env`, resets its budget to unlimited, - /// runs `call_fn`, and returns the resulting cumulative CPU instruction - /// cost. The WASM is re-registered per invocation so measurements reflect - /// a clean module instantiation, matching how the network figure is - /// captured by `simulateTransaction`. - fn measure_cpu(call_fn: impl FnOnce(&HostFunctionBenchmarkClient<'_>)) -> u64 { - let env = Env::default(); - let wasm_path = "../target/wasm32v1-none/release/host_function_contract.wasm"; - let wasm = std::fs::read(wasm_path).expect("WASM file not found, did you run cargo build?"); - let contract_id = env.register(wasm.as_slice(), ()); - let client = HostFunctionBenchmarkClient::new(&env, &contract_id); +mod common; - env.cost_estimate().budget().reset_unlimited(); +use host_function_contract::HostFunctionBenchmarkClient; +use soroban_sdk::Env; - call_fn(&client); +/// Map sizes measured for every operation. Small enough that the whole series +/// runs quickly, large enough to expose the difference between the +/// constant-time-per-call operations and the super-linear `remove`. +const SIZES: [u32; 3] = [100, 500, 1_000]; - env.cost_estimate().budget().cpu_instruction_cost() - } +/// Registers `wasm` in a fresh [`Env`], resets its budget to unlimited, runs +/// `call_fn`, and returns the resulting cumulative CPU instruction cost. +/// +/// A fresh `Env` — and therefore a clean module instantiation — is used per +/// measurement so the local figure is captured the same way the network figure +/// is captured by `simulateTransaction`. +/// +/// The already-loaded `wasm` bytes are passed in by the caller rather than +/// read here, so the artifact is read from disk once per test instead of once +/// per measured size. The staleness check and build-on-demand live in +/// [`common::load_contract_wasm`]. +fn measure_cpu(wasm: &[u8], call_fn: impl FnOnce(&HostFunctionBenchmarkClient<'_>)) -> u64 { + let env = Env::default(); + let contract_id = env.register(wasm, ()); + let client = HostFunctionBenchmarkClient::new(&env, &contract_id); + + env.cost_estimate().budget().reset_unlimited(); + + call_fn(&client); + + env.cost_estimate().budget().cpu_instruction_cost() +} - #[test] - fn measure_map_insert_across_sizes() { - for &size in &[100u32, 500, 1_000] { - let cpu = measure_cpu(|c| { - c.map_insert(&size); - }); - println!("INSERT size={:>5} local WASM CPU: {:>12}", size, cpu); - } +#[test] +fn measure_map_insert_across_sizes() { + let wasm = common::load_contract_wasm("wasm32v1-none"); + for &size in &SIZES { + let cpu = measure_cpu(&wasm, |c| { + c.map_insert(&size); + }); + println!("INSERT size={:>5} local WASM CPU: {:>12}", size, cpu); } +} - #[test] - fn measure_map_get_across_sizes() { - for &size in &[100u32, 500, 1_000] { - let cpu = measure_cpu(|c| { - c.map_get(&size); - }); - println!("GET size={:>5} local WASM CPU: {:>12}", size, cpu); - } +#[test] +fn measure_map_get_across_sizes() { + let wasm = common::load_contract_wasm("wasm32v1-none"); + for &size in &SIZES { + let cpu = measure_cpu(&wasm, |c| { + c.map_get(&size); + }); + println!("GET size={:>5} local WASM CPU: {:>12}", size, cpu); } +} - #[test] - fn measure_map_remove_across_sizes() { - for &size in &[100u32, 500, 1_000] { - let cpu = measure_cpu(|c| { - c.map_remove(&size); - }); - println!("REMOVE size={:>5} local WASM CPU: {:>12}", size, cpu); - } +#[test] +fn measure_map_remove_across_sizes() { + let wasm = common::load_contract_wasm("wasm32v1-none"); + for &size in &SIZES { + let cpu = measure_cpu(&wasm, |c| { + c.map_remove(&size); + }); + println!("REMOVE size={:>5} local WASM CPU: {:>12}", size, cpu); } +} - #[test] - fn measure_map_iterate_across_sizes() { - for &size in &[100u32, 500, 1_000] { - let cpu = measure_cpu(|c| { - c.map_iterate(&size); - }); - println!("ITERATE size={:>5} local WASM CPU: {:>12}", size, cpu); - } +#[test] +fn measure_map_iterate_across_sizes() { + let wasm = common::load_contract_wasm("wasm32v1-none"); + for &size in &SIZES { + let cpu = measure_cpu(&wasm, |c| { + c.map_iterate(&size); + }); + println!("ITERATE size={:>5} local WASM CPU: {:>12}", size, cpu); } } diff --git a/pr_body.md b/pr_body.md index e114a55d..1b4f62bf 100644 --- a/pr_body.md +++ b/pr_body.md @@ -1,9 +1,164 @@ -- closes #624 -- closes #625 -- closes #628 -- closes #633 - -- Improved inline documentation in env_file_missing_file.rs -- Optimized allocations in invalid_arg.rs -- Improved documentation in scaling_unknown_model.rs -- Optimized allocations in budget_lt_unknown_property.rs +# Harden and document the `host-function-contract` fixture + +Closes #715 +Closes #716 +Closes #717 +Closes #718 + +## Summary + +This PR resolves four `host-function-contract` maintenance issues in one focused +change set. All four concern the benchmark fixture crate — the Map cost-gap +harness, the fixture's integration tests, their shared helper, and the contract +entry points themselves. None of them changes the public contract interface or +any measured host-function operation; the fixture keeps measuring exactly what +it measured before. + +| Issue | Area | Change | +| --- | --- | --- | +| #715 | `tests/measure_map_gap.rs` | Reuse one loaded WASM artifact per test instead of re-reading it per measurement; drop the hardcoded relative path. | +| #716 | `tests/contract.rs` | Add native + WASM coverage for every fixture entry point and every zero/empty edge case. | +| #717 | `tests/common/mod.rs` | Document the module's purpose, control flow, and every public item. | +| #718 | `src/lib.rs` | Extract the duplicated Map-seeding logic into a private helper submodule. | + +## Issue #718 — Modularize `host-function-contract/src/lib.rs` + +The four `map_*` entry points (`map_insert`, `map_get`, `map_remove`, +`map_iterate`) each repeated the same "build a `size`-entry identity map" +loop before doing their own work. That loop is the shared measurement +baseline: every non-insert fixture subtracts `map_insert(size)` from its own +figure, so the build must be identical across all four. + +The build is now a single documented helper, `support::seed_identity_map`, +living in a private `support` submodule: + +```rust +mod support { + pub(super) fn seed_identity_map(env: &Env, size: u32) -> Map { ... } +} +``` + +- Each `map_*` entry point now reads as one line of setup plus the single + operation it exists to measure, making it obvious that the only difference + between the four fixtures is the operation under test. +- The `Map` import moved to where it is used, so the crate root no longer + carries an unused import. +- The public contract interface is byte-for-byte unchanged; `#[contractimpl]` + still exports exactly the same entry points. +- The `support` module is declared inline (no new file), so the + `scripts/check-module-reachability.sh` gate is satisfied without adding a + new `mod` file. + +An inaccurate comment on `repeated_hash` ("each iteration allocates an 8-byte +`Bytes` value") was corrected — the input is built once, before the loop. + +## Issue #715 — Optimize `tests/measure_map_gap.rs` + +The harness measured four operations at three map sizes (12 measurements) and +called `measure_cpu` once per measurement. Each call re-read +`../target/wasm32v1-none/release/host_function_contract.wasm` from disk into a +fresh `Vec`, i.e. 12 disk reads and 12 heap allocations of the artifact per +test run, and relied on a relative path that only resolves for one test working +directory. + +Changes: + +- The artifact is loaded **once per test** via the existing shared helper, + `common::load_contract_wasm("wasm32v1-none")`, and the same `&[u8]` is reused + across all measured sizes. Disk reads drop from 12 to 4 and the transient + `Vec` allocations drop from 12 to 4, without changing the measurement + semantics: `measure_cpu` still constructs a fresh `Env` and re-registers the + WASM per size, so each figure still reflects a clean module instantiation. +- The hardcoded relative path is gone. `load_contract_wasm` resolves the + artifact via `CARGO_TARGET_DIR`/workspace root and rebuilds it on demand when + missing or stale, which is the convention the sibling AMM harnesses already + follow (`amm-pool-contract/tests/measure_bytes_ops.rs`). This also means the + harness runs on a clean checkout instead of erroring with "WASM file not + found". +- The size series is a single named `const SIZES: [u32; 3]`, and the file now + carries a module-level doc comment (purpose, marginal-cost formula, and the + exact commands to run it), matching the documented harness style. +- The printed output lines are unchanged, so any captured measurement logs + remain comparable. + +## Issue #716 — Comprehensive tests for `tests/contract.rs` + +Previously the file tested only `repeated_sequence`. It now covers **every** +entry point on the fixture, in both registration modes: + +- **Ledger reads:** `repeated_sequence`, `repeated_timestamp` — return the + current ledger value, are deterministic across iteration counts, and return + `0` for zero iterations. +- **Crypto/allocation:** `repeated_hash`, `repeated_bytes_new` — return the + iteration count for `[0, 1, 10, 100]` and `0` for zero iterations. +- **Map operations:** `map_insert`, `map_get`, `map_remove` return their size; + `map_iterate` returns the documented identity sum with a small + `identity_sum(size)` helper (`size * (size - 1) / 2`), including the empty + map and the single `0 -> 0` entry. +- Every group has a native (`env.register`) and a WASM + (`register_contract_wasm`) counterpart, so the fixture is verified at the + same WASM level the workspace uses for budget measurement. + +Iteration counts and map sizes are kept deliberately small here, because this +file asserts correctness while the `measure_*` harnesses produce the cost data. +The tests assert only the documented return values, so they do not over-constrain +the fixture. + +## Issue #717 — Document `tests/common/mod.rs` + +The helper already worked but left new contributors to infer how it worked. +The module now explains, in order: + +- **Why it exists:** tests must run against a freshly built WASM artifact, not a + stale one; trusting a hand-built artifact would let tests pass against code + that is no longer in the tree. +- **How it fits together:** a numbered walk-through of `workspace_root` → + `wasm_path` → `artifact_needs_rebuild` → `load_contract_wasm`, so the reader + can follow the control flow without reading the bodies first. +- **Item docs:** every function now documents what it returns, its failure + behaviour, and its `# Panics` conditions. `wasm_path` notes that the path is + not guaranteed to exist, and `load_contract_wasm` documents the + [`BUILD_LOCK`] serialisation. +- **Inline comments:** the mtime comparison, the missing-artifact and + unreadable-mtime cases, and Cargo's `CARGO_TARGET_DIR` precedence are called + out where the logic is non-obvious. +- An `#![allow(dead_code)]` attribute with a rationale comment, matching + `amm-pool-contract/tests/common/mod.rs`, since each integration-test binary + uses only part of the module. + +The executable code is unchanged apart from comments. + +## Scope and non-goals + +- No change to the public contract interface or to any measured host-function + operation. +- No new dependencies, no new crate, and no new `src/` file (the helper module + is inline), so `cargo machete` and `check-module-reachability.sh` are + unaffected. +- The debug `failed_log.txt` and any unrelated scratch files are untouched. +- `MEASUREMENTS.md` figures are not regenerated: the measured operations and + their call sequences are identical. `regenerate-measurements.sh` discovery is + intentionally unchanged — `measure_map_gap.rs` still carries no `// @measure` + marker, exactly as before. + +## Verification + +This change set modifies only Rust test/fixture sources and is guarded by the +standard workspace gates. The expected CI ("Quality Checks") results are: + +```bash +cargo fmt --all -- --check # passes +cargo clippy --workspace --all-targets -- -D warnings # passes +cargo test --workspace # passes, including the new tests +``` + +Notes for reviewers: + +- The `host-function-contract` integration tests build the + `wasm32v1-none` artifact on demand through `tests/common/mod.rs`, so + `cargo test --workspace` needs no separate `cargo build` step, consistent + with `CONTRIBUTING.md`. +- To inspect the Map harness output directly: + `cargo test -p host-function-contract --test measure_map_gap -- --nocapture` +- To run only the new coverage: + `cargo test -p host-function-contract --test contract` From 3d0971b7d721179632e879b21e65cf1d84d580f2 Mon Sep 17 00:00:00 2001 From: unknown Date: Wed, 30 Sep 2026 07:48:15 +0100 Subject: [PATCH 2/3] test(host-function-contract): reset budget to unlimited in fixture tests The expanded entry-point coverage added for #716 creates a bare Env::default() and asserts only on return values, but Env::default() installs a finite Budget::default() (100M CPU instructions, 40MB memory) alongside the mainnet per-invocation resource limits. A correctness assertion can therefore fail with a budget error rather than a value mismatch, and the resulting exit code 101 is indistinguishable from a real assertion failure in CI. Every measure_* harness in this crate already calls reset_unlimited() for the same reason. Do it in the two client helpers so all tests in the file inherit it, including the zero/empty edge cases added by #716. --- host-function-contract/tests/contract.rs | 16 ++++++++++++++++ 1 file changed, 16 insertions(+) diff --git a/host-function-contract/tests/contract.rs b/host-function-contract/tests/contract.rs index a3809abc..9cb89d57 100644 --- a/host-function-contract/tests/contract.rs +++ b/host-function-contract/tests/contract.rs @@ -32,7 +32,17 @@ use host_function_contract::{HostFunctionBenchmark, HostFunctionBenchmarkClient} use soroban_sdk::Env; /// Registers the fixture as native Rust and returns a client for it. +/// +/// The budget is reset to unlimited so the assertions below observe return +/// values only. `Env::default()` installs a *finite* default budget +/// (`Budget::default()`: 100M CPU instructions and a 40 MB memory ceiling) plus +/// the mainnet per-invocation resource limits, so a correctness test that drives +/// an entry point hard enough can fail with a budget error rather than an +/// assertion. Every `measure_*` harness in this crate already resets the budget +/// for the same reason; these tests must not inherit a limit that has nothing +/// to do with what they assert. fn native_client(env: &Env) -> HostFunctionBenchmarkClient<'_> { + env.cost_estimate().budget().reset_unlimited(); let contract_id = env.register(HostFunctionBenchmark, ()); HostFunctionBenchmarkClient::new(env, &contract_id) } @@ -48,10 +58,16 @@ fn native_client(env: &Env) -> HostFunctionBenchmarkClient<'_> { /// exercise, `register_contract_wasm` with `#[allow(deprecated)]` remains /// necessary until soroban-sdk provides a non-deprecated replacement for raw /// WASM byte registration. +/// +/// The budget is reset to unlimited for the same reason as in +/// [`native_client`]: these assertions cover return values, not costs, and must +/// not depend on the finite default budget or the mainnet per-invocation +/// resource limits that `Env::default()` installs. fn wasm_client(env: &Env) -> HostFunctionBenchmarkClient<'_> { let wasm = common::load_contract_wasm("wasm32v1-none"); #[allow(deprecated)] let contract_id = env.register_contract_wasm(None, wasm.as_slice()); + env.cost_estimate().budget().reset_unlimited(); HostFunctionBenchmarkClient::new(env, &contract_id) } From 8f79dcc1ee0d2637dbe85a7762d0cb395201afe5 Mon Sep 17 00:00:00 2001 From: unknown Date: Wed, 30 Sep 2026 08:09:13 +0100 Subject: [PATCH 3/3] style(amm): restore rustfmt compliance at the pool_logic boundary main is currently red on the Code Quality workflow: \cargo fmt --all -- --check\ reports a diff in amm-pool-contract/src/lib.rs, which fails the lint gate on every open PR (PR 761 included) because pull_request runs check the merge with the base tip. The violation was introduced on main by the pool_logic refactor (c37e6f91, 831e3190, 59aa3042): the \/// Internal module for AMM pool logic\ doc comment was left abutting the preceding item's closing brace, and a second blank line was left after \mod pool_logic;\. rustfmt's default blank_lines_upper_bound is 1, so the extra blank line is the reported diff. Add the missing blank line before the doc comment and collapse the duplicate blank line. No behaviour change. --- amm-pool-contract/src/lib.rs | 2 +- 1 file changed, 1 insertion(+), 1 deletion(-) diff --git a/amm-pool-contract/src/lib.rs b/amm-pool-contract/src/lib.rs index 37e713e0..94206dd9 100644 --- a/amm-pool-contract/src/lib.rs +++ b/amm-pool-contract/src/lib.rs @@ -78,13 +78,13 @@ impl RelayContract { } } } + /// Internal module for AMM pool logic. /// /// Extracts the complex constant-product calculations and storage updates /// from the main contract implementation to improve readability and testability. mod pool_logic; - #[contract] pub struct ConstantProductPool;