mirror of
https://github.com/XRPLF/rippled.git
synced 2026-08-23 07:10:53 +00:00
Don't charge for reading in host functions
This commit is contained in:
@@ -94,6 +94,17 @@ fn charge<T>(caller: &mut Caller<'_, T>, cost: u64) -> Result<(), HostError> {
|
||||
/// Deduct `n` bytes from the per-run transfer-limit budget
|
||||
/// ([`crate::vm::TRANSFER_LIMIT_BYTES`], separate from gas);
|
||||
/// `OutOfTransferLimit` if it would go negative.
|
||||
///
|
||||
/// The budget counts bytes that **cross the boundary as copies**: host→guest
|
||||
/// writes (C++'s `setData`, here [`write_into`], this function's one call site)
|
||||
/// and typed reads that materialize a host object out of guest bytes (C++ charged
|
||||
/// uint256, AccountID, Currency, Asset). Plain borrowed reads are not charged,
|
||||
/// because nothing is copied — the host is handed a slice aliasing guest memory.
|
||||
/// This ABI has no typed reads yet; the rule is here for the ones that arrive,
|
||||
/// which will charge the object they materialize.
|
||||
///
|
||||
/// What bounds how many reads a run can make is gas, charged per host call before
|
||||
/// its body runs ([`charged`]) — the same property C++ relied on.
|
||||
fn charge_transfer(state: &VmState<'_>, n: usize) -> Result<(), HostError> {
|
||||
let n = n as u64;
|
||||
let remaining = state.transfer_budget.get();
|
||||
@@ -119,8 +130,11 @@ fn memory<T>(caller: &Caller<'_, T>) -> Result<Memory, HostError> {
|
||||
/// as long as the host call it feeds; host functions never re-enter the guest and
|
||||
/// move its memory.
|
||||
///
|
||||
/// Checks params validity, the [`MAX_FIELD_BYTES`] cap (`DataFieldTooLarge`) and
|
||||
/// the transfer budget, in that order, before the slice is formed.
|
||||
/// Checks params validity then the [`MAX_FIELD_BYTES`] cap
|
||||
/// (`DataFieldTooLarge`), in that order, before the slice is formed. The transfer
|
||||
/// budget is not among them: there are no copied bytes to charge, which is why
|
||||
/// C++ left plain slice/string reads (`trace`'s msg/data, `sha512_half`'s input)
|
||||
/// free of it — see [`charge_transfer`].
|
||||
pub(crate) fn read_borrowed<'a>(
|
||||
caller: &'a Caller<'_, VmState<'_>>,
|
||||
ptr: i32,
|
||||
@@ -133,7 +147,6 @@ pub(crate) fn read_borrowed<'a>(
|
||||
if len > MAX_FIELD_BYTES {
|
||||
return Err(HostError::DataFieldTooLarge);
|
||||
}
|
||||
charge_transfer(caller.data(), len)?;
|
||||
let end = ptr.checked_add(len).ok_or(HostError::PointerOutOfBounds)?;
|
||||
memory(caller)?
|
||||
.data(caller)
|
||||
@@ -145,12 +158,29 @@ pub(crate) fn read_borrowed<'a>(
|
||||
/// region `[dst, dst + cap)` and hand the host a `&mut [u8]` aliasing it, so the
|
||||
/// host writes straight into guest linear memory. Returns the byte count.
|
||||
///
|
||||
/// `fill` reports the value's true length and writes only what fits, leaving the
|
||||
/// engine the policy the guest observes: the [`MAX_FIELD_BYTES`] cap
|
||||
/// **`fill`'s `usize` is the value's true length, not the number of bytes it
|
||||
/// wrote.** A host holding a 64-byte value, handed a 4-byte region, writes nothing
|
||||
/// and answers `64` — which is how the guest learns the size to ask for next time.
|
||||
/// The count is therefore bounded by neither the region nor the cap, and that is
|
||||
/// what makes both checks below reachable.
|
||||
///
|
||||
/// The engine owns the policy the guest observes: the [`MAX_FIELD_BYTES`] cap
|
||||
/// (`DataFieldTooLarge`), the buffer fit (`BufferTooSmall`), then the transfer
|
||||
/// budget — the order the C++ `setData` path uses. Those checks follow `fill`,
|
||||
/// since the length is unknown before it runs, so a refused value may leave bytes
|
||||
/// in the guest's own buffer; a negative status tells the guest not to read it.
|
||||
/// since the length is unknown before it runs, which is why the region `fill`
|
||||
/// receives is clamped to the cap: the checks decide the *status*, and the clamp
|
||||
/// is what keeps an over-cap value's bytes out of guest memory regardless.
|
||||
///
|
||||
/// A refusal says nothing about what is in the guest's buffer, and the guest must
|
||||
/// not read it on a negative status. Over the cap, the clamp does bound what could
|
||||
/// have landed. Under it the clamp is a no-op — `fill` holds exactly the region the
|
||||
/// guest asked for — so whether a host that cannot fit a value leaves a prefix
|
||||
/// behind is that host's choice, not something the engine can enforce.
|
||||
///
|
||||
/// The bounds check covers the guest's whole declared `cap`, not the clamped
|
||||
/// length, so a buffer running past memory is `PointerOutOfBounds` even when its
|
||||
/// first [`MAX_FIELD_BYTES`] bytes would have been in bounds — the guest is told
|
||||
/// its pointer is wrong rather than being served a truncated prefix of it.
|
||||
pub(crate) fn write_into(
|
||||
caller: &mut Caller<'_, VmState<'_>>,
|
||||
dst: i32,
|
||||
@@ -166,13 +196,23 @@ pub(crate) fn write_into(
|
||||
// Copy) so the data borrow ends before we borrow guest memory mutably.
|
||||
let host: &dyn HostFunctions = caller.data().host;
|
||||
let end = dst.checked_add(cap).ok_or(HostError::PointerOutOfBounds)?;
|
||||
// The guest's whole declared region, so the bounds rule is about what it asked
|
||||
// for…
|
||||
let out = mem
|
||||
.data_mut(&mut *caller)
|
||||
.get_mut(dst..end)
|
||||
.ok_or(HostError::PointerOutOfBounds)?;
|
||||
// …of which at most the field cap is writable. Narrowed here rather than at the
|
||||
// call below, so no call can put more than MAX_FIELD_BYTES into guest memory
|
||||
// whatever the guest declared, and the wider slice cannot be reached again.
|
||||
let out = &mut out[..cap.min(MAX_FIELD_BYTES)];
|
||||
|
||||
let n = fill(host, out)?;
|
||||
|
||||
// Not subsumed by the clamp: `fill` reports the value's *true* length, which
|
||||
// can exceed the region it was offered, and this is how the guest learns the
|
||||
// value was too large rather than merely unwritten. The clamp bounds the
|
||||
// bytes; this bounds the status.
|
||||
if n > MAX_FIELD_BYTES {
|
||||
return Err(HostError::DataFieldTooLarge);
|
||||
}
|
||||
@@ -198,7 +238,10 @@ const _: () = assert!(
|
||||
/// The input is copied into a stack buffer bounded by [`MAX_FIELD_BYTES`], so it
|
||||
/// stays valid while [`write_into`] borrows guest memory mutably for the output —
|
||||
/// no aliasing reasoning, at the price of zero-filling the buffer each call. The
|
||||
/// output half is [`write_into`], so it obeys the same policy as a plain write.
|
||||
/// copy is host-private scratch rather than a value crossing the boundary, so it
|
||||
/// costs no transfer budget, as C++'s `sha512_half` input did not
|
||||
/// ([`charge_transfer`]). The output half is [`write_into`], so it obeys the same
|
||||
/// policy as a plain write — including the charge.
|
||||
pub(crate) fn read_write(
|
||||
caller: &mut Caller<'_, VmState<'_>>,
|
||||
src: i32,
|
||||
@@ -214,7 +257,6 @@ pub(crate) fn read_write(
|
||||
if len > MAX_FIELD_BYTES {
|
||||
return Err(HostError::DataFieldTooLarge);
|
||||
}
|
||||
charge_transfer(caller.data(), len)?;
|
||||
|
||||
// Copy the input out before `write_into` borrows guest memory mutably.
|
||||
let mut buf = [0u8; MAX_FIELD_BYTES];
|
||||
|
||||
@@ -5,7 +5,7 @@
|
||||
mod support;
|
||||
|
||||
use support::{Answer, FakeHost, ONE_PAGE, PLENTY_OF_GAS, code, import, module, run, run_with_gas};
|
||||
use xrpl_host_functions::{HostError, HostFunctionSpec};
|
||||
use xrpl_host_functions::{HASH_LEN, HostError, HostFunctionSpec};
|
||||
use xrpl_wasm_vm::{MAX_FIELD_BYTES, RunError, TRANSFER_LIMIT_BYTES};
|
||||
|
||||
// ---------------------------------------------------------------------------
|
||||
@@ -296,8 +296,6 @@ fn until_refused(imports: &str, call: &str, keep_going: &str) -> String {
|
||||
|
||||
/// For a call whose success is a positive byte count.
|
||||
const WHILE_POSITIVE: &str = "(i32.gt_s (local.get $r) (i32.const 0))";
|
||||
/// For a call whose success is a status of 0.
|
||||
const WHILE_ZERO: &str = "(i32.eqz (local.get $r))";
|
||||
|
||||
/// Bytes written into guest memory are charged against the run's budget, and the
|
||||
/// budget is a per-run total: 1 MiB of 1 KiB values exhausts it.
|
||||
@@ -352,28 +350,89 @@ fn a_modest_run_never_meets_the_budget() {
|
||||
assert_eq!(outcome.result, MAX_FIELD_BYTES as i32);
|
||||
}
|
||||
|
||||
/// **Pins current behaviour, not a decision.** `read_borrowed` hands the host a
|
||||
/// slice aliasing guest memory, copying nothing, yet charges the bytes against the
|
||||
/// transfer budget. Finding A4 in `docs/claude/redesign_impl.md` says the rule
|
||||
/// should be settled.
|
||||
/// Reads leave the budget alone: `read_borrowed` hands the host a slice *aliasing*
|
||||
/// guest memory, so there are no copied bytes to charge — the rule C++ applied to
|
||||
/// `trace`'s msg and data. What bounds how many reads a run can make is gas, which
|
||||
/// every host call pays before its body runs.
|
||||
///
|
||||
/// The observation is the write at the end, not the reads: the module reads four
|
||||
/// times the whole budget first, so a rule that charged reads would have nothing
|
||||
/// left, and the write would answer `OutOfTransferLimit` instead of a byte count.
|
||||
#[test]
|
||||
fn reads_currently_spend_the_transfer_budget_too() {
|
||||
let host = FakeHost::new();
|
||||
let wat = until_refused(
|
||||
import::TRACE_NUM,
|
||||
&format!("(call $trace_num (i32.const 0) (i32.const {MAX_FIELD_BYTES}) (i64.const 0))"),
|
||||
WHILE_ZERO,
|
||||
fn reads_do_not_spend_the_transfer_budget() {
|
||||
/// 1 KiB reads, four times over the budget.
|
||||
const READS: u64 = 4 * TRANSFER_LIMIT_BYTES / MAX_FIELD_BYTES as u64;
|
||||
|
||||
let host = FakeHost::new().answering_field(1, Answer::filler(MAX_FIELD_BYTES));
|
||||
let wat = module(
|
||||
&[import::TRACE_NUM, import::HOME_LE_FIELD, ONE_PAGE],
|
||||
&format!(
|
||||
"(local $i i32)
|
||||
(loop $l
|
||||
(drop (call $trace_num (i32.const 0) (i32.const {MAX_FIELD_BYTES}) (i64.const 0)))
|
||||
(local.set $i (i32.add (local.get $i) (i32.const 1)))
|
||||
(br_if $l (i32.lt_u (local.get $i) (i32.const {READS}))))
|
||||
(call $home_le_field (i32.const 1) (i32.const 0) (i32.const {MAX_FIELD_BYTES}))"
|
||||
),
|
||||
);
|
||||
|
||||
let outcome = run(&wat, &host).expect("the module should run");
|
||||
assert_eq!(
|
||||
outcome.result,
|
||||
code(HostError::OutOfTransferLimit),
|
||||
"a read of aliased bytes is charged as though it were copied"
|
||||
host.traces().len() as u64,
|
||||
READS,
|
||||
"every read should have been served"
|
||||
);
|
||||
assert_eq!(
|
||||
host.traces().len() as u64,
|
||||
TRANSFER_LIMIT_BYTES / MAX_FIELD_BYTES as u64,
|
||||
"the budget ran out after 1 MiB of reads that copied nothing"
|
||||
outcome.result, MAX_FIELD_BYTES as i32,
|
||||
"the write after {READS} reads of {MAX_FIELD_BYTES} bytes should still have its budget"
|
||||
);
|
||||
}
|
||||
|
||||
/// Only the output half of a read-write call spends the budget. `sha512_half`'s
|
||||
/// input is a borrowed read like any other — the stack copy `read_write` takes is
|
||||
/// host-private scratch, not a value crossing the boundary — so a run may hash far
|
||||
/// more bytes than the budget holds as long as the digests it writes fit inside it.
|
||||
///
|
||||
/// The two totals are asserted, so the arithmetic that makes the case is in the
|
||||
/// test rather than in a comment: the inputs alone would overrun the budget, the
|
||||
/// digests alone are a small fraction of it.
|
||||
#[test]
|
||||
fn only_the_output_half_of_a_read_write_spends_the_budget() {
|
||||
/// Enough 1 KiB inputs to overrun the budget twice over.
|
||||
const CALLS: u64 = 2 * TRANSFER_LIMIT_BYTES / MAX_FIELD_BYTES as u64;
|
||||
|
||||
assert!(
|
||||
CALLS * MAX_FIELD_BYTES as u64 > TRANSFER_LIMIT_BYTES,
|
||||
"the inputs alone must overrun the budget"
|
||||
);
|
||||
assert!(
|
||||
CALLS * HASH_LEN as u64 <= TRANSFER_LIMIT_BYTES / 2,
|
||||
"the digests alone must stay well inside it"
|
||||
);
|
||||
|
||||
let host = FakeHost::new().answering_digest(Answer::filler(HASH_LEN));
|
||||
let wat = module(
|
||||
&[import::SHA512_HALF, ONE_PAGE],
|
||||
&format!(
|
||||
"(local $i i32)
|
||||
(local $r i32)
|
||||
(loop $l
|
||||
(local.set $r (call $sha512_half (i32.const 0) (i32.const {MAX_FIELD_BYTES})
|
||||
(i32.const 0) (i32.const {HASH_LEN})))
|
||||
(local.set $i (i32.add (local.get $i) (i32.const 1)))
|
||||
(br_if $l (i32.lt_u (local.get $i) (i32.const {CALLS}))))
|
||||
(local.get $r)"
|
||||
),
|
||||
);
|
||||
|
||||
let outcome = run(&wat, &host).expect("the module should run");
|
||||
assert_eq!(
|
||||
host.digested.borrow().len() as u64,
|
||||
CALLS,
|
||||
"every call should have been served"
|
||||
);
|
||||
assert_eq!(
|
||||
outcome.result, HASH_LEN as i32,
|
||||
"only the digests are charged, and they fit"
|
||||
);
|
||||
}
|
||||
|
||||
@@ -121,37 +121,51 @@ fn a_value_past_the_field_cap_is_refused() {
|
||||
assert_eq!(status(&wat, &host), CAP as i32, "the cap itself is allowed");
|
||||
}
|
||||
|
||||
/// **Pins current behaviour, not a decision.** `write_into` checks the field cap
|
||||
/// after `fill` has written, so an over-cap value reaches the guest's own buffer
|
||||
/// and is then refused. Finding A4 in `docs/claude/redesign_impl.md` says the write
|
||||
/// should be clamped instead.
|
||||
/// A refused over-cap value leaves nothing behind. `write_into` hands the host at
|
||||
/// most [`MAX_FIELD_BYTES`] of the guest's buffer however much room the guest
|
||||
/// declared, so a value past the cap does not fit the region it is offered and no
|
||||
/// prefix of it can reach guest memory either.
|
||||
///
|
||||
/// The host answers with a real over-cap value: [`Answer::claiming`] writes
|
||||
/// nothing and so could not show the bytes landing.
|
||||
/// nothing whatever the engine does, so it could not tell the two apart. The
|
||||
/// second module folds the *whole* declared buffer rather than one byte, so the
|
||||
/// claim is about the region and not about its first byte.
|
||||
#[test]
|
||||
fn an_over_cap_value_is_written_before_it_is_refused() {
|
||||
fn an_over_cap_value_is_refused_without_reaching_guest_memory() {
|
||||
/// The buffer the guest declares: well over the cap, so the clamp bites.
|
||||
const BUFFER: usize = 4096;
|
||||
|
||||
let over_cap = vec![0xff; MAX_FIELD_BYTES + 1];
|
||||
let host = FakeHost::new().answering_field(1, Answer::bytes(over_cap));
|
||||
let call = format!("(call $home_le_field (i32.const 1) (i32.const 0) (i32.const {BUFFER}))");
|
||||
|
||||
let wat = module(
|
||||
&[import::HOME_LE_FIELD, ONE_PAGE],
|
||||
"(drop (call $home_le_field (i32.const 1) (i32.const 0) (i32.const 4096)))
|
||||
(i32.load8_u (i32.const 0))",
|
||||
);
|
||||
// The status the guest sees, from a module that returns it directly.
|
||||
let refusing = module(
|
||||
&[import::HOME_LE_FIELD, ONE_PAGE],
|
||||
"(call $home_le_field (i32.const 1) (i32.const 0) (i32.const 4096))",
|
||||
);
|
||||
let refusing = module(&[import::HOME_LE_FIELD, ONE_PAGE], &call);
|
||||
assert_eq!(
|
||||
status(&refusing, &host),
|
||||
code(HostError::DataFieldTooLarge),
|
||||
"the value is refused"
|
||||
);
|
||||
|
||||
// Every byte of the buffer, or-ed together: guest memory starts zero-filled,
|
||||
// so any byte the host wrote shows up here.
|
||||
let reading = module(
|
||||
&[import::HOME_LE_FIELD, ONE_PAGE],
|
||||
&format!(
|
||||
"(local $i i32)
|
||||
(local $seen i32)
|
||||
(drop {call})
|
||||
(loop $l
|
||||
(local.set $seen (i32.or (local.get $seen) (i32.load8_u (local.get $i))))
|
||||
(local.set $i (i32.add (local.get $i) (i32.const 1)))
|
||||
(br_if $l (i32.lt_u (local.get $i) (i32.const {BUFFER}))))
|
||||
(local.get $seen)"
|
||||
),
|
||||
);
|
||||
assert_eq!(
|
||||
status(&wat, &host),
|
||||
0xff,
|
||||
"but its bytes are already in guest memory"
|
||||
status(&reading, &host),
|
||||
0,
|
||||
"and not one of its bytes is in the guest's buffer"
|
||||
);
|
||||
}
|
||||
|
||||
|
||||
@@ -446,15 +446,41 @@ matter. Items marked ✓ are done.
|
||||
**Decided: `host_lib`**, matching the SDK and the fixtures. `the_import_module_name_must_match`
|
||||
now rejects `host`, `env` and the empty name, so the choice is pinned rather than
|
||||
incidental.
|
||||
4. **The transfer budget is charged for bytes that are never copied, and charged
|
||||
4. ✓ **The transfer budget is charged for bytes that are never copied, and charged
|
||||
before validation.** `read_borrowed` *aliases* guest memory — zero copies — yet
|
||||
calls `charge_transfer` (`abi.rs:135`); C++ deliberately did not charge plain
|
||||
called `charge_transfer` (`abi.rs:135`); C++ deliberately did not charge plain
|
||||
slice/string reads (`trace` msg/data, `sha512_half` input — see "Reference points"
|
||||
below). The charge also precedes the bounds check, so a guest can drain the 1 MiB
|
||||
budget with out-of-bounds pointers. Related, in `write_into`: `fill` gets a slice
|
||||
of the guest's full `cap`, uncapped by `MAX_WASM_DATA_LEN`, so an over-cap value
|
||||
lands in guest memory before `n > MAX_WASM_DATA_LEN` rejects it — clamping `out` to
|
||||
`min(cap, MAX_WASM_DATA_LEN)` makes that post-check unreachable by construction.
|
||||
|
||||
Landed, with **one claim in the paragraph above corrected**: the clamp does *not*
|
||||
make the post-check unreachable, and the check is load-bearing. `fill` reports the
|
||||
value's *true* length, which can exceed the region it was handed, so `n >
|
||||
MAX_FIELD_BYTES` is what turns an over-cap value into `DataFieldTooLarge` instead
|
||||
of a silently-accepted 1025-byte count. Deleting it fails three tests. The clamp
|
||||
and the check bound different things: the clamp bounds the **bytes** that can reach
|
||||
guest memory, the check bounds the **status** the guest is given.
|
||||
|
||||
Both read charges are gone — `read_borrowed`'s and `read_write`'s input — so
|
||||
`charge_transfer` has exactly one call site, in `write_into`, and the budget means
|
||||
what C++ meant by it: bytes actually copied host→guest. Removing the read charge
|
||||
also dissolves the out-of-bounds drain rather than reordering around it. Nothing
|
||||
replaces those charges: gas already bounds how many reads a run can make, since
|
||||
every host call pays its spec's gas before its body runs, which is the property
|
||||
C++ relied on. The bounds check still spans the guest's **whole declared `cap`**,
|
||||
not the clamped length, so a buffer running past memory is `PointerOutOfBounds`
|
||||
even when its first `MAX_FIELD_BYTES` bytes would have been valid.
|
||||
|
||||
*Partly a host contract, not an engine guarantee.* The engine guarantees at most
|
||||
`min(cap, MAX_FIELD_BYTES)` bytes are **writable**. That a refused over-cap value
|
||||
leaves *nothing* behind additionally relies on the host writing only when the whole
|
||||
value fits `out` — what `setData` did and what `Answer::bytes` does. A host impl
|
||||
that scribbled the clamped prefix and then reported a larger `n` would still leave
|
||||
bytes behind. Worth stating in the `HostFunctions` declaration's doc comment; the
|
||||
ABI crate was not touched here.
|
||||
5. ✓ **`Module::new` accepted WAT text — a behaviour the rewrite introduced by
|
||||
accident.** wasmi's default features include `wat`, and `Module::new` runs
|
||||
`wat::parse_bytes` over its input (`module/mod.rs:228`), so the VM compiled
|
||||
@@ -611,21 +637,25 @@ useful for comparison and for the gas assertions in `Wasm_test.cpp` — not gosp
|
||||
## Current state (2026-07-30)
|
||||
|
||||
**`crates/` compiles**, and the whole workspace is green — `cargo test --workspace`,
|
||||
`clippy --workspace --all-targets`, `fmt`. 114 tests: 33 macro, 9 facade, 1 doctest, and
|
||||
**71 in `xrpl-wasm-vm`** (9 unit; 62 integration — 12 `host_calls`, 19 `memory_policy`,
|
||||
12 `budgets`, 19 `vm_limits`).
|
||||
`clippy --workspace --all-targets`, `fmt`. 115 tests: 33 macro, 9 facade, 1 doctest, and
|
||||
**72 in `xrpl-wasm-vm`** (9 unit; 63 integration — 12 `host_calls`, 19 `memory_policy`,
|
||||
13 `budgets`, 19 `vm_limits`).
|
||||
|
||||
**Findings A1, A2, A3 and B6, B7 are done** (2026-07-30). `run` is
|
||||
**Section A is closed, and B6/B7 with it** (2026-07-30). `run` is
|
||||
`Result<RunOutcome, RunFailure>` over a typed `RunError`; host-fatal errors trap
|
||||
instead of answering the guest a code; the import module is `host_lib`; the `i64`
|
||||
pipeline and `AbiRet` are gone. See those entries for what landed and why. Two
|
||||
decisions were taken to get there and are recorded at their findings:
|
||||
**`OutOfTransferLimit` stays soft** (A1) and **the module name is `host_lib`** (A3).
|
||||
pipeline and `AbiRet` are gone; the transfer budget counts only bytes actually copied
|
||||
host→guest, and no more than the field cap can reach guest memory. See those entries
|
||||
for what landed and why. Two decisions were taken to get there and are recorded at
|
||||
their findings: **`OutOfTransferLimit` stays soft** (A1) and **the module name is
|
||||
`host_lib`** (A3).
|
||||
|
||||
The two tests that existed only to pin behaviour A1 changed are gone, replaced by
|
||||
tests of the new behaviour (`a_host_call_refused_its_gas_stops_the_run`,
|
||||
`an_endless_loop_is_stopped_by_gas`). `the_wire_conversion_truncates` went with the
|
||||
cast it pinned. What the rewrite turned up that reading the code did not:
|
||||
Every test that existed only to pin behaviour a finding said should change is gone,
|
||||
replaced by a test of the new behaviour: `a_host_call_refused_its_gas_stops_the_run`
|
||||
and `an_endless_loop_is_stopped_by_gas` for A1, `reads_do_not_spend_the_transfer_budget`
|
||||
and `an_over_cap_value_is_refused_without_reaching_guest_memory` for A4.
|
||||
`the_wire_conversion_truncates` went with the cast it pinned. What the work turned up
|
||||
that reading the code did not:
|
||||
|
||||
- **An endless guest loop and a refused host charge are the same outcome**, and both
|
||||
report the whole limit as spent — the loop because wasmi's meter reaches zero, the
|
||||
@@ -648,6 +678,15 @@ cast it pinned. What the rewrite turned up that reading the code did not:
|
||||
instantiation) a move rather than a behaviour change — its failure is already a
|
||||
run-ender, so hoisting it to an instantiation-time `RunError::NoMemory` only
|
||||
changes which stage reports it.
|
||||
- **A clamp and a check that look redundant are not.** See A4: the clamp bounds the
|
||||
bytes, the `MAX_FIELD_BYTES` check bounds the status. The finding's own text claimed
|
||||
the clamp made the check unreachable; a mutation proved otherwise, which is the
|
||||
argument for mutating rather than reasoning about a test's value.
|
||||
- **Nothing in the suite reached the budget through `sha512_half`.** Removing
|
||||
`read_write`'s input charge would have been invisible, so
|
||||
`only_the_output_half_of_a_read_write_spends_the_budget` was written to catch it —
|
||||
2048 calls hashing twice the budget while writing a sixteenth of it. A finding whose
|
||||
fix no test can notice is a finding with no net under it.
|
||||
|
||||
**How the suite was checked.** A code review of the diff mutation-tested it, and the
|
||||
result is worth recording because it found a test that pinned nothing: the multi-value
|
||||
@@ -717,9 +756,8 @@ split the VM restated all five values, so a legitimate gas change meant editing
|
||||
files. (Corollary: `every_variant_appears_in_all_exactly_once` is now subsumed by the
|
||||
table comparison and could go.)
|
||||
|
||||
One test still **pins behaviour a finding says should change**, and says so in its name
|
||||
and doc comment: `reads_currently_spend_the_transfer_budget_too` (A4). It is meant to be
|
||||
rewritten when that decision lands, not preserved. Its A1 counterpart already was.
|
||||
No test now **pins behaviour a finding says should change**. The two that did were
|
||||
rewritten when their findings landed, which is what they were for.
|
||||
|
||||
The trait is settled, and every part of it is written in the declaration rather than
|
||||
synthesized: `&self`, `HostResult<T>`, and byte outputs as explicit
|
||||
@@ -733,21 +771,32 @@ Consequences worth remembering:
|
||||
- The ABI crate is now guest-linkable (`no_std`, no allocator, no runtime deps, checks
|
||||
for `wasm32-unknown-unknown`) — see "The ABI crate is a library both sides link".
|
||||
|
||||
Next, in rough order, from the findings above: **A4** — the transfer budget charged for
|
||||
bytes never copied and charged before validation, plus `write_into`'s
|
||||
`min(cap, MAX_FIELD_BYTES)` clamp. It is the last correctness item, and `is_fatal`
|
||||
answers the question it used to raise: a mis-charge cannot end a run, only mis-report a
|
||||
byte count, because `OutOfTransferLimit` is soft. Then the remaining B and D cleanups as
|
||||
one pass (B8's unused `cxx` dep, B9's stale links, D14's `forbid(unsafe_code)`, D16's
|
||||
three papercuts — `get_fuel().unwrap_or(0)` now appears once, in `vm::fuel_used`), then
|
||||
the cached `Memory` (C10). The scratch-buffer decision (C11) and real `ApplyContext`
|
||||
wiring plus the cxx bridge (`xrpl-wasm-vm-ffi` is still `mod ffi {}`) follow. Deferred as
|
||||
before: macro-emitted `link_*` shims, the generated C header, the probe-module test.
|
||||
Next, in rough order, from the findings above: **the remaining B and D cleanups as one
|
||||
pass** — B8's unused `cxx` dep, B9's stale links and historical comments, D14's
|
||||
`forbid(unsafe_code)` plus `unreachable_pub` and the cast lints, and D16's papercuts.
|
||||
Then the cached `Memory` (C10), which `NoMemExported` being fatal has already made a
|
||||
move rather than a behaviour change. The scratch-buffer decision (C11) and real
|
||||
`ApplyContext` wiring plus the cxx bridge (`xrpl-wasm-vm-ffi` is still `mod ffi {}`)
|
||||
follow. C12 (per-run `Linker` and no module cache) stays last: `VmState<'h>`'s lifetime
|
||||
is the blocker. Deferred as before: macro-emitted `link_*` shims, the generated C
|
||||
header, the probe-module test.
|
||||
|
||||
`register_host_functions` still returns `Result<(), String>` and `run` now discards that
|
||||
string (a linker failure is `RunError::Internal`, which carries nothing), so its
|
||||
`format!` is dead. `Result<(), wasmi::errors::LinkerError>` is the honest signature —
|
||||
small enough to fold into the B/D pass.
|
||||
Four small things belong in that B/D pass, each found by a slice rather than by the
|
||||
original read:
|
||||
|
||||
- `register_host_functions` returns `Result<(), String>` and `run` discards the string
|
||||
(a linker failure is `RunError::Internal`, which carries nothing), so its `format!`
|
||||
is dead. `Result<(), wasmi::errors::LinkerError>` is the honest signature.
|
||||
- `only_the_host_fatal_errors_trap`'s soft list is representative, not exhaustive —
|
||||
nothing in the ABI crate enumerates `HostError`. A `HostError::ALL` there would close
|
||||
it, and this pins a consensus-relevant channel split, so it is worth closing.
|
||||
- D16's `gas = 0` item has shifted from a bug to a decision: it no longer passes
|
||||
silently but fails with a typed `OutOfGas`, so the question is whether it deserves
|
||||
C++'s `temBAD_AMOUNT` at the caller instead. `gas` is `u64`, so C++'s negative case
|
||||
cannot arise.
|
||||
- The `HostFunctions` declaration should say that a host writes into `out` only when
|
||||
the whole value fits — see A4's last paragraph, where that is what makes "a refused
|
||||
value leaves nothing behind" hold end to end.
|
||||
|
||||
Deferred to a later refactor, once there is working code: macro-emitted `link_*`
|
||||
shims, the generated C header, and the probe-module conformance test.
|
||||
|
||||
Reference in New Issue
Block a user