feat: Port wat based wasm tests to new wasm design - #8114
Conversation
|
This PR has conflicts, please resolve them in order for the PR to be reviewed. |
|
All conflicts have been resolved. Assigned reviewers can now start or resume their review. |
Codecov Report❌ Patch coverage is
📢 Thoughts on this report? Let us know! |
![xrplf-ai-reviewer[bot]](https://avatars.githubusercontent.com/u/67929741?s=60&v=4){kind=small}
There was a problem hiding this comment.
This is a large but low-risk test-porting change: it retires the old Beast-based wasm test suite (Wasm_test.cpp, wasm_fixtures/) and replaces it with a layered Rust (.wat + FakeHost) and C++ (MockVmTest/RealVmTest) test structure, adding a detailed README documenting the migration mapping. No production/runtime code is touched — only test fixtures, test harnesses, and new test cases. I reviewed the new Rust unit tests (host_calls.rs, preflight.rs, vm_limits.rs) and the new C++ e2e/RealVmTest/WasmRun harness files for logic errors, buffer/offset mistakes, and resource issues, and did not find any high-confidence correctness or security problems in the added lines. Memory offsets in the new e2e WAT contracts (HostFunctionTour, CurrentLedgerObjField, TxField, SetData, LedgerSqn) stay within the single allocated page and don't overlap in ways that would corrupt results, and the new Rust tests exercise the described marshalling/refusal behavior consistently with the surrounding test style.
There was a problem hiding this comment.
This is a large, well-organized test-migration PR (WAT-based tests replacing the old Beast wasm suite) with no production code changes — only test infrastructure (crates/xrpl-wasm-vm tests, C++ wasm test fixtures/harnesses) and documentation. I reviewed the new Rust host_calls/preflight/vm_limits tests and the new C++ RealVmTest/WasmRun/e2e fixtures for correctness (byte-offset math, keylet sequence usage, gas defaults, memory reuse across host calls). The changes are internally consistent and I did not find any high-confidence bugs, security issues, or resource-leak problems in the added lines. One minor, low-value maintainability note below.
There was a problem hiding this comment.
This diff is almost entirely documentation (a detailed README explaining the new WASM test layering/benchmark methodology) plus one small new test-harness header (RealVmTest.h). The WasmBench.h diff body is empty, so there's nothing to review there. The RealVmTest.h code is a straightforward test fixture (default-argument wrapper around makeHost + runWat) with no apparent correctness or security issues — default arguments, the moved std::function parameter, and the std::expected return type are all used correctly. No changed lines warrant a flag.
There was a problem hiding this comment.
This MR ports a large batch of WAT-based wasm host-function tests and benchmarks (e2e tests + host_functions benchmarks) to the new wasm test design. The files are highly mechanical: each benchmark defines a WAT snippet, encodes host-function arguments via literal offsets, and calls a matching benchmarkImpl/benchmarkThroughVm helper. I checked memory offset/length arithmetic in the WAT bodies against the accompanying dataSegment calls (e.g. EscrowKeylet, CheckSignature, FloatAdd/FloatPower, TxField, TxNestedField) and found the offsets/lengths line up consistently with each other. I did not find any clear correctness, security, or resource-leak bugs introduced by the added lines within the visible diff; the pattern is repeated correctly across the many near-identical benchmark files. No comments to post.
There was a problem hiding this comment.
This MR adds ~30 new Google Benchmark files under src/tests/libxrpl/tx/wasm/host_functions/, porting existing wat-based wasm host-function benchmarks to the new benchmarkImpl/benchmarkThroughVm harness. The files are highly repetitive and mechanical, following a consistent, established pattern (Fixtures::instance(), UseManualTime(), kBenchIterations). I checked for copy-paste bugs across the set (mismatched account/param wiring, incorrect wasm names, budget miscalculations) and did not find any correctness or security issues in the added code. The two hand-crafted ones (Sha512Half, TxNestedField, UpdateData) that build raw byte payloads/imports look internally consistent with their described call signatures.
There was a problem hiding this comment.
This MR adds a large batch of new Google Benchmark (.bench.cpp) files under src/tests/libxrpl/tx/wasm/host_functions/, each following the same well-established template (benchmarkImpl / benchmarkThroughVm wrappers around Fixtures::instance()). The code is boilerplate, internally consistent across files, and I didn't find correctness, security, or resource-management issues in the added lines. No comments to post.
TimothyBanks
changed the title
There was a problem hiding this comment.
This MR ports WAT-based WASM host-function tests to the new layered test design (host_context/host_calls/host_functions/e2e) and adds an extensive README documenting the layering and gas-benchmark methodology. The diff shown is almost entirely new test/fixture files and documentation; I did not find correctness, security, or resource-leak issues in the added lines. The include-path update in WasmVM.cpp (WasmFixture.h -> fixtures/WasmFixture.h, plus fixtures/WasmRun.h) and the base-class rename (WasmTest -> MockVmTest) look consistent with the new fixture layout described in the README. The new e2e tests (CacheLedgerObj.cpp, CurrentLedgerObjField.cpp) build WAT contracts and real ledger fixtures that appear internally consistent (keylet computed from the owner's pre-submit sequence, byte-length assertions matching AccountID size, etc.).
There was a problem hiding this comment.
This MR is almost entirely mechanical: it relocates test fixture headers into a new tx/wasm/fixtures/ subdirectory and updates all #include paths accordingly, and adds a large batch of new .bench.cpp benchmark files exercising WASM host functions/keylets. I reviewed the new benchmark files for correctness (offset/param mismatches, buffer overlaps, endianness handling) and the include changes for consistency, and did not find any clear bugs, security issues, or logic errors in the added code. The through-VM benchmarks (e.g. CheckSignature, EscrowKeylet) construct WAT data segments and call bodies with offsets that line up correctly with the corresponding import signatures and encoded byte layouts.
|
All conflicts have been resolved. Assigned reviewers can now start or resume their review. |
There was a problem hiding this comment.
This MR is largely a mechanical port: (1) updating include paths from tx/wasm/*.h to tx/wasm/fixtures/*.h across all existing wasm host_context/host_functions tests, and (2) adding a large set of new .bench.cpp benchmark files that mirror the pattern used by existing benchmarks (constructing a Fixtures instance, running benchmarkImpl/benchmarkThroughVm with a wasm import/body). I reviewed the include-path changes for correctness (all consistently repoint to the new fixtures/ subdirectory with no missed instances) and the new benchmark files for correctness of offsets/lengths/params passed into the wat-level function calls (e.g. EscrowKeylet, CheckSignature). Everything is internally consistent with existing patterns in the codebase and I did not find correctness, security, or resource-management issues in the changed/added lines.
|
Thanks for the review @kuznetsss. The comments have been addressed. |
| } | ||
| else | ||
| { | ||
| auto result = call(*host); |
There was a problem hiding this comment.
Shouldn't result be checked to make sure the call was successfull?
| if(benchmark AND TARGET benchmark::benchmark) | ||
| target_include_directories( | ||
| xrpl_tests | ||
| PRIVATE | ||
| $<TARGET_PROPERTY:benchmark::benchmark,INTERFACE_INCLUDE_DIRECTORIES> | ||
| ) | ||
| endif() |
There was a problem hiding this comment.
-Dbenchmark=OFF breaks the verify-headers target:
This guard silently no-ops in exactly the configuration that needs it, leaving
verify-headers unbuildable.
Mechanism
verify_target_headers(xrpl_tests "${CMAKE_CURRENT_SOURCE_DIR}")compiles every header undersrc/tests/libxrplas its own translation unit -cmake/verify_headers.cmake:65globs recursively:file(GLOB_RECURSE headers CONFIGURE_DEPENDS "${dir}/*.h" "${dir}/*.hpp").- That sweep now picks up
tx/wasm/fixtures/WasmBench.h, which includes<benchmark/benchmark.h>at line 7. It's the first header in this tree to depend on an optional package. - Google Benchmark's include dirs only reach
xrpl_teststhrough the block above, which requiresTARGET benchmark::benchmark. - But
find_package(benchmark REQUIRED)itself sits insideif(benchmark)(CMakeLists.txt:153-155), so withbenchmark=OFFthe target never exists, theifis false, and the include dirs are never added.
| - cmake/** | ||
| - LICENSE.md | ||
| - .clang-tidy | ||
| - src/test/app/wasm_fixtures/**/*.wat |
There was a problem hiding this comment.
This directory was removed
|
|
||
| constexpr std::string_view kWasmName = "sha512_half"; | ||
|
|
||
| constexpr std::int16_t kMaxBytes = 1024; |
There was a problem hiding this comment.
Better to usekMaxWasmDataLength from include/xrpl/protocol/Protocol.h to avoid duplication
| gasDelta = std::max(std::int64_t{1}, hot.gas - cold.gas); | ||
| } | ||
|
|
||
| return (total / kCalibrationPairs) / static_cast<double>(gasDelta); |
There was a problem hiding this comment.
The comment above says:
a mean over
kBenchIterationspairs
but kCalibrationPairs is used instead
| /// fixtures); the plainer "bad magic / wrong version" shapes are covered by `garbage_does_not_pass`. | ||
| #[test] | ||
| fn parser_abuse_shapes_are_refused() { | ||
| fn hex(s: &str) -> Vec<u8> { |
There was a problem hiding this comment.
nit: hex() is defined the same here and in the test below. Maybe move it into a free function?
| # | ||
| # Each `*.bench.cpp` sits beside the test that covers the same host function, under | ||
| # `src/tests/libxrpl/tx/wasm/`. Keeping the benchmark next to the test means the | ||
| # two move together and share one fixture; a separate executable means benchmark |
There was a problem hiding this comment.
I still think it would be better to put *.bench.cpp, WasmBench.* and BenchFixtures.* into src/benchmarks/libxrpl/tx/wasm/. There are already some benchmarks in src/benchmarks. I think we should follow the same pattern.
The move would simplify cmake code because no exclusion regexp would be needed. And xrpl_tests target would not depend on Google Benchmark.
There was a problem hiding this comment.
This diff is purely mechanical — updating include paths from tx/wasm/*.h to tx/wasm/fixtures/*.h (and adding WasmLedger.h includes) across many test files, consistent with a header reorganization. No logic changes are present, so there is nothing substantive to flag.
There was a problem hiding this comment.
This diff is largely mechanical: a new WasmRun.h fixture header is added, and dozens of test files are updated to move include paths from tx/wasm/.h to tx/wasm/fixtures/.h following a directory reorganization. No behavioral changes are visible in the diff. Nothing concerning stands out in the new header or the include-path updates.
kuznetsss
left a comment
kuznetsss
left a comment
There was a problem hiding this comment.
The code looks good. Just one question about the measurement approach
| Calibration const& | ||
| Calibration::instance() | ||
| { | ||
| static Calibration const kValue; |
There was a problem hiding this comment.
To get statistically correct values we should call the benchmark with the flag --benchmark_repetitions=<some number>. That will provide mean, meadian, stddev which will show the error bar for the measurement. But because Calibration is a singleton it will be measured only once even with --benchmark_repetitions flag. Measured once it doesn't provide the error to value ratio for gas per second value and because of that we couldn't correctly estimate the error bar for the result gas price of a host function.
There was a problem hiding this comment.
Mechanical include-path update (tx/wasm/X.h -> tx/wasm/fixtures/X.h) applied uniformly across ~60 wasm test files as part of porting wat-based tests to the new wasm test design. No logic, behavior, or test-assertion changes are present in this diff — every hunk is a one-line #include path swap. No correctness, security, or maintainability issues found in the changed lines.
2 participants
High Level Overview of Change
Context of Change
API Impact
libxrplchange (any change that may affectlibxrplor dependents oflibxrpl)