mirror of
https://github.com/XRPLF/rippled.git
synced 2026-08-21 14:20:56 +00:00
Add preflight to c++ code
This commit is contained in:
@@ -5,6 +5,11 @@
|
||||
`crates/xrpl-wasm-vm-ffi/src/lib.rs` is the whole of the Rust half; `HostContext.{h,cpp}` and
|
||||
`WasmVM.{h,cpp}` are the C++ half. Two decisions carry the design.
|
||||
|
||||
Three crossings, not two: `run_escrow` in, the host calls back out, and `check_escrow`
|
||||
in. The third goes one way only — screening a module needs no host — so it takes no
|
||||
`HostContext`, has no C++-exception half to contain, and is the one bridge function the
|
||||
crate's own tests can call outright.
|
||||
|
||||
**The result is total, not `Result<T>`.** cxx's `Result` sugar throws a `rust::Error` into
|
||||
C++; a status is the better interface for a condition the caller has to turn into a TER
|
||||
anyway. `RunResult { status, result, gas_used, detail }` flattens the engine's
|
||||
@@ -26,6 +31,12 @@ given a status *and* a TER.
|
||||
- The asymmetry is what makes each half sufficient: because the C++ shims never unwind,
|
||||
every frame between a panic and `catch_unwind` is Rust.
|
||||
|
||||
Both halves are named `guarded`, and each is one function that every crossing goes through:
|
||||
Rust's takes the panic arm as an argument (`ffi::RunResult::panicked`), C++'s takes the value
|
||||
to answer with if the call throws. Anything C++ catches there is xrpld's own — a bad
|
||||
allocation, or a `funcName` that is not valid UTF-8 and so cannot become a `rust::Str` —
|
||||
never a wasm outcome, since those arrive as statuses.
|
||||
|
||||
`HostContext` holds a `HostFunctions&` and lowers its typed `std::expected` onto the wire.
|
||||
The `&self`-vs-non-const worry was a non-issue: a `const` member function holding a
|
||||
non-const reference can still call `cacheLedgerObj`/`updateData`. `cxx_name` on each method
|
||||
@@ -53,13 +64,55 @@ node's, and charging a transaction for a node's defect would write that defect i
|
||||
| `Internal`, `Panic` | `tecINTERNAL` | none |
|
||||
|
||||
The `Compile`/`Instantiate`/`EntryPoint` row is `tecINTERNAL` because preflight is meant to
|
||||
have refused such a module with `temBAD_WASM` long before apply — which is why preflight is
|
||||
item 1 in [the roadmap](index.md#next). `NoMemory` had no old TER to match (it used to reach
|
||||
the guest as code -14); `tecFAILED_PROCESSING` treats it as the contract fault it is.
|
||||
have refused such a module with `temBAD_WASM` long before apply. **That row is now known to
|
||||
be wrong for `Instantiate`** — see below. `NoMemory` had no old TER to match (it used to
|
||||
reach the guest as code -14); `tecFAILED_PROCESSING` treats it as the contract fault it is.
|
||||
|
||||
`gas <= 0` is refused as `temBAD_AMOUNT` before the engine is called, restoring what
|
||||
`WasmiEngine::run` did — see [open-questions.md](open-questions.md).
|
||||
|
||||
## The preflight map
|
||||
|
||||
`preflightEscrowWasm` owns it, and it is deliberately flat: every fault in the module is
|
||||
one answer, because a caller's only decision is whether the transaction may proceed.
|
||||
|
||||
| `CheckStatus` | `NotTEC` |
|
||||
|---|---|
|
||||
| `Ok` | `tesSUCCESS` |
|
||||
| `Compile`, `Import`, `EntryPoint` | `temBAD_WASM` |
|
||||
| `Panic` | `telFAILED_PROCESSING` |
|
||||
|
||||
The statuses stay distinct anyway: the *detail* is what a contract author needs, and one
|
||||
status per stage keeps the map's arms reviewable and lets it grow without inventing
|
||||
distinctions later.
|
||||
|
||||
`Panic` is not `temBAD_WASM`. A defect in the engine teaches nothing about the module, and
|
||||
`tem` would record our bug as the transaction's malformation; `tel` is the preflight
|
||||
analogue of `tecINTERNAL`'s "the fault is the node's" — local, not forwarded, no fee. Two
|
||||
things follow that are worth stating: divergence between nodes is not what the code choice
|
||||
fixes (a panic in deterministic code is not node-local, and if it were, no TER would
|
||||
reconcile the two), and this arm has no test on the C++ side, because there is no reliable
|
||||
way to make the engine panic from a fixture.
|
||||
|
||||
**The signature the C++ front does not have is the point**: `(Bytes, beast::Journal,
|
||||
std::string_view) -> NotTEC`, with no `HostFunctions&`. The deleted `preflightEscrowWasm`
|
||||
took one and could therefore never have been called from a real `preflight()` —
|
||||
`PreflightContext` has no view to build a host over.
|
||||
|
||||
## Why `Instantiate` should stop being `tecINTERNAL`
|
||||
|
||||
`check` closes compile, imports and the entry point, but two ways instantiation fails are
|
||||
invisible to it: a start section that traps, and a linear memory over the page cap that the
|
||||
module does not export ([engine.md](engine.md)). Both are deterministic properties of the
|
||||
module, so a contract can pass preflight, be escrowed, and then fail to instantiate at
|
||||
apply — where the map currently blames the node and charges nothing.
|
||||
|
||||
The fix is two lines and its own change: report `Instantiate` as `tecFAILED_PROCESSING`
|
||||
with its gas, and in `vm::run` classify a failure carrying a trap code (`e.as_trap_code()`)
|
||||
as `Trap` rather than `Instantiate`, since a start section trapping is guest code trapping.
|
||||
`tecINTERNAL` then means what it says — `Internal` and `Panic`, the node's own defects — and
|
||||
the map stops depending on preflight's completeness for its correctness.
|
||||
|
||||
## The one copy left on the byte path, and why it needs `HostFunctions` to change
|
||||
|
||||
The engine's side of the byte path is copy-free by construction — `write_into` hands the host
|
||||
|
||||
@@ -122,6 +122,13 @@ so the configuration that decides validity cannot differ. The import set is
|
||||
host function extends the check and the linker at once. And the entry point's three faults are
|
||||
described by one `entry_point_fault`, called from `run` with wasmi's error appended.
|
||||
|
||||
The rules themselves are pure functions over what a module *declares* — `check_import` takes
|
||||
`(namespace, name, ExternType)`, `entry_point_fault` takes an `Option<ExternType>` — so the
|
||||
unit tests state each rule, its precedence and its wording on inputs built directly, and
|
||||
`tests/preflight.rs` is left to run real modules. Precedence is a decision, not an accident:
|
||||
an import breaking two rules reports the namespace, which is what explains the module's other
|
||||
imports too.
|
||||
|
||||
What it cannot see is guest behaviour and anything absent from the module's exports: a start
|
||||
section that traps, and a linear memory over the page cap that the module keeps to itself.
|
||||
Both pass the check and then fail instantiation, which is why a run's own refusal at that
|
||||
|
||||
@@ -49,8 +49,9 @@ than guessing. See [history.md](history.md) for what is worth recovering.
|
||||
(`check` — compile, imports, entry point, with no host, store or gas), `abi.rs` (gas,
|
||||
transfer budget, guest-memory marshaling), `region.rs` (the `(ptr, len)` type),
|
||||
`register.rs` (one `func_wrap` per host function). See [engine.md](engine.md).
|
||||
- `xrpl-wasm-vm-ffi/` — the cxx bridge, both crossings. `RunStatus`/`RunResult`,
|
||||
`run_escrow`, `CxxHost`, the panic guard. See [bridge.md](bridge.md).
|
||||
- `xrpl-wasm-vm-ffi/` — the cxx bridge, all three crossings. `RunStatus`/`RunResult` and
|
||||
`run_escrow`, `CheckStatus`/`CheckResult` and `check_escrow`, `CxxHost`, the panic
|
||||
guard. See [bridge.md](bridge.md).
|
||||
- `xrpl-wasm-testkit/` — **test-only**: `compile_wat`, so the C++ tests write their modules
|
||||
as WebAssembly text. A crate of its own so `wat` cannot reach the shipped node; see
|
||||
[testing.md](testing.md).
|
||||
@@ -58,7 +59,8 @@ than guessing. See [history.md](history.md) for what is worth recovering.
|
||||
`HostFunctions` interface), `HostFuncImpl*.cpp` (its implementations, over
|
||||
`ApplyContext&`), `WasmCommon.h` (`HostFunctionError`, `Wmem`, `WasmTER`, `FieldLocator`).
|
||||
The bridge's C++ half is `HostContext.{h,cpp}` (the ABI-shaped view of `HostFunctions`)
|
||||
and `WasmVM.{h,cpp}` (`runEscrowWasm`, gas validation, the TER map).
|
||||
and `WasmVM.{h,cpp}` (`runEscrowWasm`, `preflightEscrowWasm`, gas validation, both TER
|
||||
maps).
|
||||
- `src/tests/libxrpl/tx/wasm/` — the C++ tests, in the `xrpl_tests` gtest binary.
|
||||
- `include/xrpl/tx/wasm/README.md` is **stale**: it uses the long name `get_ledger_sqn`
|
||||
where the code registers `ldgr_index`, and references `detail/WasmVM.cpp`,
|
||||
@@ -68,15 +70,17 @@ than guessing. See [history.md](history.md) for what is worth recovering.
|
||||
## Current state (2026-08-04)
|
||||
|
||||
**The whole workspace is green**: `cargo test --workspace`, `clippy --workspace
|
||||
--all-targets`, `fmt`, and `cargo doc -p xrpl-wasm-vm --no-deps`. **154 tests** — 33 macro,
|
||||
12 facade, 1 doctest, **96 in `xrpl-wasm-vm`** (11 unit; 85 integration — 13 `budgets`,
|
||||
12 `host_calls`, 23 `memory_policy`, 16 `preflight`, 21 `vm_limits`), 10 in
|
||||
`xrpl-wasm-vm-ffi`, 2 in `xrpl-wasm-testkit`. On the C++ side, **27 tests over the whole
|
||||
loop** in six fixtures: `./xrpl_tests --gtest_filter='WasmVMTest.*:*Call.*'`.
|
||||
--all-targets`, `fmt`, and `cargo doc -p xrpl-wasm-vm --no-deps`. **168 tests** — 33 macro,
|
||||
12 facade, 1 doctest, **105 in `xrpl-wasm-vm`** (19 unit; 86 integration — 13 `budgets`,
|
||||
12 `host_calls`, 23 `memory_policy`, 17 `preflight`, 21 `vm_limits`), 15 in
|
||||
`xrpl-wasm-vm-ffi`, 2 in `xrpl-wasm-testkit`. On the C++ side, **37 tests over the whole
|
||||
loop** in seven fixtures: `./xrpl_tests
|
||||
--gtest_filter='WasmVMTest.*:*Call.*:PreflightTest.*'`.
|
||||
|
||||
**Both crossings are wired and a real contract runs through them**: C++ calls
|
||||
**All three crossings are wired and a real contract runs through them**: C++ calls
|
||||
`runEscrowWasm`, the engine services `ldgr_index` by calling back into
|
||||
`xrpl::HostFunctions`, and the guest reads the answer out of its own memory. Five host
|
||||
`xrpl::HostFunctions`, and the guest reads the answer out of its own memory;
|
||||
`preflightEscrowWasm` screens a module through the third, with no host at all. Five host
|
||||
functions are registered (`ldgr_index`, `home_le_field`, `sha512_half`, `trace`,
|
||||
`trace_num`) out of the ~65 the full ABI will carry.
|
||||
|
||||
@@ -85,23 +89,29 @@ functions are registered (`ldgr_index`, `home_le_field`, `sha512_half`, `trace`,
|
||||
|
||||
## Next
|
||||
|
||||
1. **`preflightEscrowWasm`.** The engine half is done — `check` in `preflight.rs`. What is
|
||||
left is the second bridge entry (`check_escrow`, a `CheckStatus`/`CheckResult` pair
|
||||
mirroring `RunStatus`/`RunResult`) and the C++ front, whose signature is
|
||||
`(Bytes, beast::Journal, std::string_view) -> NotTEC`: **no `HostFunctions&`**, since a
|
||||
check needs no host and a `PreflightContext` has no ledger to build one from.
|
||||
Two decisions are still open — what a *panic* at preflight returns
|
||||
(`telFAILED_PROCESSING` reads as the preflight analogue of `tecINTERNAL`'s "the fault is
|
||||
the node's"), and whether the apply-side map moves `Instantiate` off `tecINTERNAL` in the
|
||||
same change; see [bridge.md](bridge.md).
|
||||
2. **A caller.** `EscrowFinish.cpp` still has no wasm reference, so `runEscrowWasm` is
|
||||
reached only from `src/tests/libxrpl/tx/wasm/`. Wiring it up is what makes
|
||||
`WasmHostFunctionsImpl` (over a real `ApplyContext`) the host in production rather than
|
||||
in principle.
|
||||
3. **A gas parity oracle.** `Wasm_test.cpp` asserts exact gas numbers (e.g. 29'502) and is
|
||||
1. **Move `Instantiate` off `tecINTERNAL`**, and report a trapping start section as `Trap`
|
||||
rather than as a module that would not instantiate. Two lines plus their tests, and it is
|
||||
what actually removes the papering-over: `check` cannot see either of the two remaining
|
||||
instantiate faults, so the apply-side map must stop depending on preflight's
|
||||
completeness. [bridge.md](bridge.md) has the reasoning.
|
||||
2. **A caller.** `EscrowFinish.cpp` still has no wasm reference, so `runEscrowWasm` and
|
||||
`preflightEscrowWasm` are reached only from `src/tests/libxrpl/tx/wasm/`. Wiring it up is
|
||||
what makes `WasmHostFunctionsImpl` (over a real `ApplyContext`) the host in production
|
||||
rather than in principle. **Blocked on the protocol fields**: `FinishFunction` and
|
||||
`ComputationAllowance` exist nowhere in this fork or upstream, and adding
|
||||
`FinishFunction` also has to answer the **contract code-size cap** — there is none, and
|
||||
preflight's cost is linear in the blob (a 249 KB module of duplicate imports measures
|
||||
1.5 ms, mostly wasmi's own parse).
|
||||
3. **Import signatures at preflight.** `check` compares an import's namespace, name and
|
||||
kind, not its type, so a mistyped import still parts a module from the engine at
|
||||
instantiation. Deferred to when `host_functions!` generates the wasm-level lowering,
|
||||
which the C header and the typed `link_*` shims in [abi.md](abi.md) both want anyway.
|
||||
Note the deleted C++ `check` did not compare signatures either, so this is inherited
|
||||
rather than new.
|
||||
4. **A gas parity oracle.** `Wasm_test.cpp` asserts exact gas numbers (e.g. 29'502) and is
|
||||
the best oracle we have, but it is commented out and its fixtures cannot run on this
|
||||
engine — see the `env` finding in [testing.md](testing.md).
|
||||
4. **The `Bytes`-by-value copy in `HostFunctions`** — [bridge.md](bridge.md). A
|
||||
5. **The `Bytes`-by-value copy in `HostFunctions`** — [bridge.md](bridge.md). A
|
||||
49-signature sweep, so it wants a caller to measure against first.
|
||||
|
||||
Also open: the two performance items and the ABI questions in
|
||||
|
||||
@@ -39,6 +39,9 @@
|
||||
(`let f: fn(&ffi::HostContext) -> _ = ...`) fails with `Undefined symbols:
|
||||
_rs$wasm_vm$cxxbridge1$…`. So keep those tests on pure logic — the status map, the panic
|
||||
guard, the wire conversions — and put anything that needs a host in the gtest.
|
||||
**`check_escrow` is the exception**: it takes no `HostContext`, so its tests call the real
|
||||
bridge function, hand-writing the two modules they need as bytes (the eight-byte header is
|
||||
a valid module) rather than reaching for an assembler this crate does not have.
|
||||
|
||||
## The Rust tests
|
||||
|
||||
@@ -99,6 +102,13 @@ Then one fixture per host function — `LedgerSqnCall`, `CurrentLedgerObjFieldCa
|
||||
`Sha512HalfCall`, `TraceCall`, `TraceNumCall` — because the module *is* that function's shared
|
||||
setup. `WasmVMTest` keeps what belongs to the engine rather than to any function.
|
||||
|
||||
**`PreflightTest` deliberately derives from `testing::Test`, not from `WasmTest`**, and holds
|
||||
no mock: `preflightEscrowWasm` takes no host, and a fixture that supplied one would hide the
|
||||
signature that is the point. That is why `assembleWat` is a free function in `WasmFixture.h`
|
||||
rather than a `WasmTest` member. `PreflightTest.ScreeningAgreesWithARun` is the one test
|
||||
there that does build a host — it puts the same modules through `runEscrowWasm` so the two
|
||||
entry points do not have to be trusted to agree.
|
||||
|
||||
**The journal is captured, not sent to a null sink.**
|
||||
`WasmVMTest.ThrowingHostFunctionBecomesInternal` asserts the exception text *and* that the log
|
||||
names `getLedgerSqn`; without that, an exception silently swallowed with no log would pass, and
|
||||
|
||||
Reference in New Issue
Block a user