refactor: Wasm vm redesign - #7952
Conversation
chore: Fix clang version in devshell
| // non-positive limit means is a transaction-validity rule; the engine's own budget is | ||
| // therefore an unsigned quantity with no invalid value to represent. | ||
| if (gasLimit <= 0) | ||
| return std::unexpected(WasmTER{.ter = temBAD_AMOUNT, .cost = std::nullopt}); |
There was a problem hiding this comment.
nit:
| return std::unexpected(WasmTER{.ter = temBAD_AMOUNT, .cost = std::nullopt}); | |
| return std::unexpected{WasmTER{.ter = temBAD_AMOUNT, .cost = std::nullopt}}; |
| if (gasLimit <= 0) | ||
| return std::unexpected(WasmTER{.ter = temBAD_AMOUNT, .cost = std::nullopt}); | ||
|
|
||
| auto const nodeSideFault = std::unexpected(WasmTER{.ter = tecINTERNAL, .cost = std::nullopt}); |
There was a problem hiding this comment.
nit:
| auto const nodeSideFault = std::unexpected(WasmTER{.ter = tecINTERNAL, .cost = std::nullopt}); | |
| auto const nodeSideFault = std::unexpected{WasmTER{.ter = tecINTERNAL, .cost = std::nullopt}}; |
| HostContext ctx{hfs}; | ||
| auto const run = rs::wasm_vm::run_escrow( | ||
| ctx, | ||
| rust::Slice<std::uint8_t const>(wasmCode.data(), wasmCode.size()), |
There was a problem hiding this comment.
nit:
| rust::Slice<std::uint8_t const>(wasmCode.data(), wasmCode.size()), | |
| rust::Slice<std::uint8_t const>{wasmCode.data(), wasmCode.size()}, |
| ctx, | ||
| rust::Slice<std::uint8_t const>(wasmCode.data(), wasmCode.size()), | ||
| static_cast<std::uint64_t>(gasLimit), | ||
| rust::Str(funcName.data(), funcName.size())); |
There was a problem hiding this comment.
nit:
| rust::Str(funcName.data(), funcName.size())); | |
| rust::Str{funcName.data(), funcName.size())}; |
| if (!result) | ||
| { | ||
| JLOG(hfs.getJournal().warn()) | ||
| << "wasm: " << std::string_view(run.detail.data(), run.detail.size()) |
There was a problem hiding this comment.
nit:
| << "wasm: " << std::string_view(run.detail.data(), run.detail.size()) | |
| << "wasm: " << std::string_view{run.detail.data(), run.detail.size()} |
| return impl_->getJournal(); | ||
| return guarded(j, NotTEC{telFAILED_PROCESSING}, [&]() { | ||
| auto const checked = rs::wasm_vm::check_escrow( | ||
| rust::Slice<std::uint8_t const>(wasmCode.data(), wasmCode.size()), |
There was a problem hiding this comment.
nit:
| rust::Slice<std::uint8_t const>(wasmCode.data(), wasmCode.size()), | |
| rust::Slice<std::uint8_t const>{wasmCode.data(), wasmCode.size()}, |
| return guarded(j, NotTEC{telFAILED_PROCESSING}, [&]() { | ||
| auto const checked = rs::wasm_vm::check_escrow( | ||
| rust::Slice<std::uint8_t const>(wasmCode.data(), wasmCode.size()), | ||
| rust::Str(funcName.data(), funcName.size())); |
There was a problem hiding this comment.
nit:
| rust::Str(funcName.data(), funcName.size())); | |
| rust::Str{funcName.data(), funcName.size())}; |
| if (!isTesSuccess(ter)) | ||
| { | ||
| JLOG(j.warn()) << "wasm: " | ||
| << std::string_view(checked.detail.data(), checked.detail.size()) |
There was a problem hiding this comment.
nit:
| << std::string_view(checked.detail.data(), checked.detail.size()) | |
| << std::string_view{checked.detail.data(), checked.detail.size()} |
| // ldgr_index — no input, one scalar output | ||
| // --------------------------------------------------------------------------------------- | ||
|
|
||
| class LedgerSqnCall : public HostCallTest |
There was a problem hiding this comment.
I kind of feel like we should split these tests out into their own source files. There will be a decent amount of host functions and this file will get pretty large.
| // an expectation can say *what* the guest asked the host to work on. | ||
| MATCHER_P(BytesAre, expected, "") | ||
| { | ||
| return std::string_view(reinterpret_cast<char const*>(arg.data()), arg.size()) == |
There was a problem hiding this comment.
nit:
| return std::string_view(reinterpret_cast<char const*>(arg.data()), arg.size()) == | |
| return std::string_view{reinterpret_cast<char const*>(arg.data()), arg.size()} == |
| MATCHER_P(BytesAre, expected, "") | ||
| { | ||
| return std::string_view(reinterpret_cast<char const*>(arg.data()), arg.size()) == | ||
| std::string_view(expected); |
There was a problem hiding this comment.
nit:
| std::string_view(expected); | |
| std::string_view{expected}; |
| std::string text_; | ||
|
|
||
| public: | ||
| CapturingSink() : Sink(beast::Severity::Warning, false) |
There was a problem hiding this comment.
nit:
| CapturingSink() : Sink(beast::Severity::Warning, false) | |
| CapturingSink() : Sink{beast::Severity::Warning, false} |
|
|
||
| // Keeps what a run logged. The host's default journal is a null sink, which would let a | ||
| // swallowed condition pass a test that only checks the TER. | ||
| class CapturingSink : public beast::Journal::Sink |
There was a problem hiding this comment.
This seems like a type that should already be available within the repo.
There was a problem hiding this comment.
There is a CaptureSink in CaptureLogs.h.
| inline Bytes | ||
| assembleWat(std::string_view wat) | ||
| { | ||
| auto const wasm = rs::wasm_testkit::compile_wat(rust::Str(wat.data(), wat.size())); |
There was a problem hiding this comment.
nit:
| auto const wasm = rs::wasm_testkit::compile_wat(rust::Str(wat.data(), wat.size())); | |
| auto const wasm = rs::wasm_testkit::compile_wat(rust::Str{wat.data(), wat.size()}); |
| } | ||
|
|
||
| static Bytes | ||
| assemble(std::string_view wat) |
There was a problem hiding this comment.
What does this function provide over just calling assembleWat?
There was a problem hiding this comment.
It also converts rust::Vec into Bytes
| char const* what; | ||
| Bytes code; | ||
| std::string_view entryPoint; | ||
| } const cases[] = { |
There was a problem hiding this comment.
While this is valid C++ code, it seems like a rarely used way to declare a type and use it in a variable.
| // have screened. | ||
| TEST_F(WasmVMTest, TrappingStartSectionIsChargedToTheContract) | ||
| { | ||
| constexpr std::string_view wat = R"wat( |
There was a problem hiding this comment.
nit: All of these wat variables could be static as well.
| // asked to run anything. | ||
| TEST_F(WasmVMTest, NoGasIsRefusedAsMalformedRatherThanRun) | ||
| { | ||
| for (std::int64_t const gas : {std::int64_t{0}, std::int64_t{-1}}) |
There was a problem hiding this comment.
nit:
| for (std::int64_t const gas : {std::int64_t{0}, std::int64_t{-1}}) | |
| for (auto const gas : {std::int64_t{0}, std::int64_t{-1}}) |
TimothyBanks
left a comment
There was a problem hiding this comment.
The pending conversations are all something that can be done at a later time. I think this is a great start so no reason to hold it up from us flushing out the rest of the host functions.
| SlotsFull = -8, | ||
| EmptySlot = -9, | ||
| LedgerObjNotFound = -10, | ||
| Decoding = -11, |
There was a problem hiding this comment.
🟡 Severity: MEDIUM
The Rust HostError enum assigns Decoding = -11, but the C++ HostFunctionError in WasmCommon.h assigns OutOfTransferLimit = -11. The doc comment here claims the discriminants "mirror" the C++ enum, but they diverge at this code. When a C++ host function returns error code -11 (meaning OutOfTransferLimit), the Rust engine via HostError::from_code(-11) interprets it as Decoding — a different semantic. Since error codes are consensus-relevant (contracts branch on them), this mismatch can cause incorrect guest behavior or divergence between the two ABI definitions.
Helpful? Add 👍 / 👎
💡 Fix Suggestion
Suggestion: Realign the Rust HostError enum discriminants to match the C++ HostFunctionError enum in WasmCommon.h. Specifically:
- In
crates/xrpl-host-functions/src/lib.rs, changeDecoding = -11toOutOfTransferLimit = -11(matching C++ line 37), and moveDecodingto a new code beyond the shared C++ range (e.g.,-21or another unused value). - Remove or renumber the current
OutOfTransferLimit = -23(line 86) since it will now be at-11. - Also align
Internal = -1with C++'sUnimplemented = -1— either rename the Rust variant or addUnimplementedas an alias. - Update the C++ enum in
WasmCommon.hto include the Rust-only variants (Decoding,NoRuntime,OutOfGas) so both sides have the same complete set. - Update all downstream uses of these variants:
crates/xrpl-wasm-vm/src/register.rslines 90 and 107 (usesHostError::Decoding)crates/xrpl-wasm-vm/src/vm.rsline 232 (matches onHostError::Decoding)crates/xrpl-wasm-vm/src/abi.rsline 63, 288, 298 (usesHostError::OutOfTransferLimit)crates/xrpl-host-functions/tests/host_errors.rslines 32 and 44 (expected code values)crates/xrpl-wasm-vm/tests/host_calls.rsline 208 andtests/budgets.rsline 311, 331
- Update the test comment at
src/tests/libxrpl/tx/wasm/WasmVM.cpp:236that documents the known drift.
| if (size <= out.size()) | ||
| std::memcpy(out.data(), value, size); | ||
| return static_cast<std::int32_t>(size); |
There was a problem hiding this comment.
⚪ Severity: LOW
The answer() helper casts std::size_t to std::int32_t without range check. If a buggy HostFunctions implementation returns a Bytes vector larger than INT32_MAX, this wraps to a negative value, which the Rust engine interprets as a HostError code — causing a misclassified error on a consensus-critical path.
Helpful? Add 👍 / 👎
💡 Fix Suggestion
Suggestion: Add a range check before the static_cast<std::int32_t>(size) to guard against values larger than INT32_MAX. If size exceeds INT32_MAX, return kHostInternal (the existing fatal-error sentinel already defined in this anonymous namespace) instead of allowing the cast to wrap to a negative value. INT32_MAX is available from the already-included <cstdint>.
⚠️ Experimental Feature: This code suggestion is automatically generated. Please review carefully.
| if (size <= out.size()) | |
| std::memcpy(out.data(), value, size); | |
| return static_cast<std::int32_t>(size); | |
| if (size > static_cast<std::size_t>(INT32_MAX)) | |
| return kHostInternal; | |
| if (size <= out.size()) | |
| std::memcpy(out.data(), value, size); | |
| return static_cast<std::int32_t>(size); |
|
This PR has conflicts, please resolve them in order for the PR to be reviewed. |
eaa7344 to
b5a90e7
Compare
High Level Overview of Change
This PR replaces the wasmi C-API integration with a Rust wasmi wrapper reached over a cxx
bridge.
What's here
crates/— cargo workspace, built in via corrosion.xrpl-host-functions— the ABI's single declaration site.host_functions! { … }generates the
HostFunctionstrait and the spec enum (import name + gas per function).no_std, no allocator, zero runtime deps, builds forwasm32— so the guest SDK canimplement the same trait from the same declaration.
xrpl-wasm-vm— the engine:run, andcheck(compile / imports / entry point /declared memory, with no host, store or gas, because a transaction's
preflight()has noledger to serve a host call from).
xrpl-wasm-vm-ffi— the bridge. Three crossings:run_escrowin, host calls back out,check_escrowin.xrpl-wasm-testkit— test-onlycompile_wat, a separate crate so no assembler can reachthe shipped node.
C++ —
HostContext.{h,cpp}, an ABI-shapednoexceptview ofHostFunctions; andWasmVM.{h,cpp}withrunEscrowWasm/preflightEscrowWasmand both TER maps.Not in this PR
ldgr_index,home_le_field,sha512_half,trace,trace_num). The remainingHostFuncImpl*.cppare untouched and still compile.src/test/app/HostFuncImpl_test.cppis commented out: its assertions ran through thedeleted C lowering.
Base branches
The PR is based on 2 branches:
Context of Change
API Impact
libxrplchange (any change that may affectlibxrplor dependents oflibxrpl)