Added Anan-AS PVM Interpreter#716
Conversation
📝 WalkthroughSummary by CodeRabbit
WalkthroughAdds a PVM backend abstraction and multiplatform PVM support across the repo. Introduces packages/core/pvm-interface and packages/core/pvm-interpreter-ananas, an IPvmInterpreter interface, and an Ananas WASM-backed interpreter implementation. Refactors the existing interpreter to implement the new interface, replaces many local gas/memory/register APIs with interface types (IGasCounter, IMemory, IRegisters, HostCallMemory, HostCallRegisters), and threads a new PvmBackend enum and CLI Estimated code review effort🎯 4 (Complex) | ⏱️ ~60 minutes Areas requiring extra attention:
Possibly related PRs
Suggested reviewers
Poem
Pre-merge checks and finishing touches❌ Failed checks (1 warning)
✅ Passed checks (2 passed)
✨ Finishing touches
🧪 Generate unit tests (beta)
Comment |
|
@fluffylabs-bot benchmark |
tomusdrw
left a comment
tomusdrw
left a comment
There was a problem hiding this comment.
looks like a good start!
Some potential for improvements:
- Passing & parsing service code is probably pretty heavy and could be done asynchronously.
- We currently can't have multiple instances of ananas due to API restrictions. We could consider instantiating multiple WASM modules though.
- We should run
stateTransitiontest vectors on both pvm backends. - We might consider adding pvm backend that executes on both pvms (in parallel!) and makes sure that they yield the same results.
|
@fluffylabs-bot benchmark |
|
✅ Benchmark workflow triggered successfully! 🎉 🔗 Check the Actions tab for workflow progress. |
![coderabbitai[bot]](https://avatars.githubusercontent.com/in/347564?s=60&v=4){kind=small}
There was a problem hiding this comment.
Actionable comments posted: 23
Caution
Some comments are outside the diff and can’t be posted inline due to platform limitations.
⚠️ Outside diff range comments (6)
packages/jam/jam-host-calls/fetch.ts (1)
15-239: Based on the shell output and web search results:The latest release is v0.7.1 (marked as "Latest"), not v0.7.2.
Align graypaper links to v0.7.1 (current latest version).
The code in
packages/jam/jam-host-calls/fetch.tscontains references to mixed versions:
- v0.6.6 (27 links)
- v0.7.2 (lines 183, 208) — This version does not exist; v0.7.1 is the latest release
- v0.6.7 (line 244)
All graypaper links in fetch.ts should reference v0.7.1, the current stable version. The references to v0.7.2 appear to be forward-references to a non-existent version and require correction.
packages/jam/jam-host-calls/externalities/refine-externalities.ts (2)
70-73: Avoid unchecked BigInt→Number cast; annotate or constrain.
Number(v)onnumber | bigintneeds an explicit safety note or guard since BigInt→Number is lossy. Here the valid range is 0..4.As per coding guidelines (BigInt→Number), add a comment and constrain the cast:
-export const toMemoryOperation = (v: number | bigint): MemoryOperation | null => - v <= MemoryOperation.Write && v >= MemoryOperation.Void ? Number(v) : null; +export const toMemoryOperation = (v: number | bigint): MemoryOperation | null => { + // Safe cast: MemoryOperation is 0..4 which fits in JS Number precisely. + const n = typeof v === "bigint" ? Number(v) : v; + return n <= MemoryOperation.Write && n >= MemoryOperation.Void ? n as MemoryOperation : null; +}
56-68: Fix typos in documentation.“possbile” → “possible”; “writeable” → “writable”.
-/** Types of possbile operations to request by Pages host call. */ +/** Types of possible operations to request by Pages host call. */ @@ - /** Set given range of pages as writeable and initialize them with zeros. */ + /** Set given range of pages as writable and initialize them with zeros. */Also applies to: 116-118
packages/jam/jam-host-calls/refine/invoke.ts (1)
117-124: Handle Status.OK; current code throws unexpectedly.
MachineStatusallowsOK, but the code throws for it. Treat it like other terminal statuses.- if ([Status.PANIC, Status.HALT, Status.OOG].includes(returnState.status)) { + if ([Status.PANIC, Status.HALT, Status.OOG, Status.OK].includes(returnState.status)) { regs.set(IN_OUT_REG_1, tryAsU64(returnState.status)); return; }packages/jam/transition/accumulate/pvm-executor.ts (1)
67-75: Nit: spelling.“PVM exectutor” → “PVM executor”.
- * PVM exectutor class that prepares PVM together with host call handlers to be run in requested context + * PVM executor class that prepares PVM together with host call handlers to be run in requested contextpackages/core/pvm-interpreter/interpreter.ts (1)
311-313: Typo in helper name.Rename
getNextIstructionIndextogetNextInstructionIndexfor clarity.- const getNextIstructionIndex = (index: number) => index + 1 + this.mask.getNoOfBytesToNextInstruction(index + 1); + const getNextInstructionIndex = (index: number) => + index + 1 + this.mask.getNoOfBytesToNextInstruction(index + 1); - for (let index = 0; index < codeLength; index = getNextIstructionIndex(index)) { + for (let index = 0; index < codeLength; index = getNextInstructionIndex(index)) {
♻️ Duplicate comments (2)
bin/test-runner/w3f/accumulate.ts (1)
144-149: Run both backends sequentially or gate via a flag; don’t leave Ananas commented out.Tests should cover BuiltIn and Ananas. Run in sequence to avoid interference, or gate with an env/CLI switch.
export async function runAccumulateTest(test: AccumulateTest, path: string) { - await runAccumulateInternal(test, path, PvmBackend.BuiltIn); - // TODO [MaSo] running separate passes the test, running both at the same time fails. - // - //await runAccumulateInternal(test, path, PVMBackend.Ananas); + const backend = process.env.TYPEBERRY_PVM_BACKENDS ?? 'both'; + if (backend === 'built-in') { + await runAccumulateInternal(test, path, PvmBackend.BuiltIn); + } else if (backend === 'ananas') { + await runAccumulateInternal(test, path, PvmBackend.Ananas); + } else { + // run sequentially to avoid interference + await runAccumulateInternal(test, path, PvmBackend.BuiltIn); + await runAccumulateInternal(test, path, PvmBackend.Ananas); + } }packages/core/pvm-interpreter-ananas/api.ts (1)
1-8: Raw-generated type surface is brittle; keep temporary and document.This mirrors
@fluffylabs/anan-as/rawinternals (__Record..,__Internref..). It’s OK short‑term, but prone to break on generator updates. Add a prominent TODO with the linked issue and consider a thin adapter that exposes a stable, minimal API surface.Also applies to: 10-182
🧹 Nitpick comments (27)
bin/test-runner/w3f/pvm.ts (1)
65-65: Follow-up issue needed for Ananas testing.The TODO comment indicates that W3F PVM tests should also run on the Ananas backend. Based on the past review comments, this was discussed and deferred to a follow-up issue, but the issue doesn't appear to have been created yet.
Would you like me to open a GitHub issue to track running W3F PVM tests on both
BuiltInandAnanasinterpreters?packages/core/pvm-host-calls/host-call-registers.ts (1)
38-40: Avoid leaking internal buffer fromgetEncoded().Returning
this.bytesexposes mutable internal storage to callers.Return a copy or clearly document mutability:
- getEncoded(): Uint8Array { - return this.bytes; - } + getEncoded(): Uint8Array { + return this.bytes.slice(); // return copy to prevent external mutation + }As per coding guidelines.
packages/core/pvm-interface/gas.ts (1)
25-55: MakeinitialGasimmutable and clarifyused()range.
initialGasis initialized once; make itreadonly. Also specifyused()is clamped to [0,initialGas] for consistency with implementations.Proposed tweaks:
-export interface IGasCounter { +export interface IGasCounter { /** * Set during initialization of GasCounter. * * NOTE: Needed to calculate `used()` gas. */ - initialGas: Gas; + readonly initialGas: Gas; @@ - /** - * Calculates used gas since creation of GasCounter. - * - * NOTE: Capped to `initialGas`. - */ + /** + * Calculates used gas since creation of GasCounter. + * + * Range: 0 … initialGas (clamped). + */ used(): Gas; }As per coding guidelines.
packages/jam/jam-host-calls/accumulate/assign.test.ts (1)
11-13: LGTM; align gasCounter import path for consistencyThe switch to emptyRegistersBuffer and single HostCallMemory is correct. To keep imports uniform (like in gas.test.ts), consider importing gasCounter from the package root.
-import { gasCounter } from "@typeberry/pvm-interpreter/gas.js"; +import { gasCounter } from "@typeberry/pvm-interpreter";Also applies to: 36-37, 57-60
packages/core/pvm-host-calls/host-call-memory.test.ts (1)
55-66: Verification passed; maintain optional refactoring for brittle assertionsMEMORY_SIZE is correctly exported from
@typeberry/pvm-interfaceand properly imported in the test file. The PageFault semantics align with the runtime implementation.The optional suggestion to reduce brittle string assertions remains valid. Replace full error message comparisons with type checks:
- deepEqual( - res, - Result.error(PageFault.fromMemoryIndex(0, true), () => "Page fault: attempted to access reserved page 0"), - ); + if (!res.isError) throw new Error("expected error"); + if (!(res.error instanceof PageFault)) throw new Error("expected PageFault");Also applies to: 105-116
packages/jam/jam-host-calls/read.ts (1)
21-25: Update graypaper link to the current version used elsewhere (v0.7.2) or confirm correctnessHost-call docs link points to v0.7.1, while related code references v0.7.2. Please align to the current spec version or confirm this section must stay at 0.7.1.
As per coding guidelines.
packages/jam/jam-host-calls/accumulate/query.test.ts (1)
52-58: Avoid BigInt→Number casts or add a safety commentCalls like tryAsU32(Number(w7)) / tryAsU32(Number(w8)) convert U64→number without an explicative comment. Either:
- Pass the literal constants directly (e.g., tryAsU32(2 ** 16), tryAsU32(32)), or
- Mask to lower 32 bits and add a brief “safe because…” note, or
- Provide a helper (e.g., getLowerU32) to avoid Number on bigint in tests.
This keeps tests aligned with the “Number(bigint) requires justification” rule.
As per coding guidelines.
Also applies to: 80-81, 105-106, 133-134, 162-163, 192-193
packages/core/pvm-host-calls/host-call-memory.ts (2)
31-34: Add explicit safety comment for Number() conversion.Per coding guidelines, casting a
bigintusingNumber(x)must have an explanation comment why it is safe. While the masking to 32 bits ensures the value is within safe integer range, please add an explicit comment.As per coding guidelines.
Apply this diff:
// NOTE: We always take lower 32 bits from register value. // // https://graypaper.fluffylabs.dev/#/ab2cdbd/25ed0025ed00?v=0.7.2 + // NOTE: It's safe to convert to number because the value is masked to 32 bits (max 0xFFFFFFFF). const address = tryAsU32(Number(regAddress & 0xffff_ffffn));
51-54: Add explicit safety comment for Number() conversion.Per coding guidelines, casting a
bigintusingNumber(x)must have an explanation comment why it is safe. While the masking to 32 bits ensures the value is within safe integer range, please add an explicit comment.As per coding guidelines.
Apply this diff:
// NOTE: We always take lower 32 bits from register value. // // https://graypaper.fluffylabs.dev/#/ab2cdbd/25ed0025ed00?v=0.7.2 + // NOTE: It's safe to convert to number because the value is masked to 32 bits (max 0xFFFFFFFF). const address = tryAsU32(Number(regAddress & 0xffff_ffffn));packages/core/pvm-interface/pvm.ts (1)
17-19: Consider a backend-neutral method name.
resetJamleaks JAM naming into a core interface.resetProgramorresetwould be clearer and backend-agnostic.packages/jam/transition/accumulate/accumulate.ts (1)
363-371: Simplify zero-work early-return condition.
const n = transfers.length + i + reports.length; if (n === 0) { ... }is equivalent to checking both collections empty.- const n = transfers.length + i + reports.length; - - if (n === 0) { + if (transfers.length === 0 && reports.length === 0) { return { accumulatedReports: tryAsU32(0), gasCost: tryAsServiceGas(0), state: stateUpdate, }; }packages/jam/transition/accumulate/accumulate.test.ts (2)
422-424: Optional: move fixture blob near top.preimageBlob is referenced inside test bodies; moving it above the describes improves readability and avoids TDZ surprises if this file is later refactored.
22-22: Verify PvmBackendNames index mapping.PvmBackendNames indexing by enum ordinal assumes the array order stays synchronized with enum member values. Current state: enum has exactly BuiltIn=0, Ananas=1 and array is ["built-in", "ananas"], so indices align correctly. However, if a new enum member is added or members reordered, the array indexing breaks silently. Deriving the mapping from the enum using a Record<PvmBackend, string> pattern would provide compile-time safety—TypeScript would error if enum changes without updating the Record.
packages/jam/jam-host-calls/accumulate/solicit.test.ts (1)
8-10: Align gas import path with other tests.This imports gasCounter from "@typeberry/pvm-interpreter/gas.js" while lookup.test.ts imports from "@typeberry/pvm-interpreter". Standardize on one.
packages/jam/transition/accumulate/pvm-executor.ts (1)
87-89: Pool size is magic.prepareBackend hardcodes pool size 4. Consider making it a parameter with a sensible default and documenting the rationale (CPU cores, throughput vs memory).
- private static async prepareBackend(pvm: PvmBackend) { - return PvmInstanceManager.new(4, pvm); + private static async prepareBackend(pvm: PvmBackend, poolSize = 4) { + return PvmInstanceManager.new(poolSize, pvm); }packages/jam/jam-host-calls/lookup.test.ts (1)
8-11: Unify gas import path across test files for consistency.The codebase inconsistently imports
gasCounter—some files use"@typeberry/pvm-interpreter"(e.g.,lookup.test.ts) while others use"@typeberry/pvm-interpreter/gas.js"(e.g.,solicit.test.ts). Pick one canonical import path and apply it consistently across all test files to avoid duplicate bundles at build time.packages/core/pvm-host-calls/bin.ts (1)
29-29: Minor: drop the IIFE; use TLA consistently.Since you already use TLA above, you can await runProgram at top level and simplify.
- (async () => { - await pvmHostCallExtension.runProgram(program, args, 5, tryAsGas(1000)); - })(); +await pvmHostCallExtension.runProgram(program, args, 5, tryAsGas(1000));packages/jam/jam-host-calls/accumulate/eject.test.ts (1)
8-11: Import migration to pvm-interface and utils — OK; consider consolidating gasCounter import.The move to
tryAsGasfrom@typeberry/pvm-interfaceandemptyRegistersBufferis correct. Optionally, importgasCounterfrom the package root for consistency with other tests.-import { MemoryBuilder } from "@typeberry/pvm-interpreter"; -import { gasCounter } from "@typeberry/pvm-interpreter/gas.js"; +import { gasCounter, MemoryBuilder } from "@typeberry/pvm-interpreter";Also applies to: 18-18
packages/core/pvm-host-calls/interpreter-instance-manager.ts (3)
15-33: Async factory is the right shape; add basic parameter validation.
new()now awaits backends (incl. Ananas) — good. ValidatenoOfPvmInstances >= 1to fail fast on misconfiguration.static async new(noOfPvmInstances: number, interpreter: PvmBackend): Promise<InterpreterInstanceManager> { + if (noOfPvmInstances < 1) { + throw new Error(`noOfPvmInstances must be >= 1 (got ${noOfPvmInstances})`); + } const instances: IPvmInterpreter[] = []; for (let i = 0; i < noOfPvmInstances; i++) { switch (interpreter) {
36-44: Nit: avoid Promise.resolve in async function.Just
return instance;— async will wrap it. Keeps code tidy.- if (instance !== undefined) { - return Promise.resolve(instance); - } + if (instance !== undefined) { + return instance; + }
9-9: Leftover TODO label.Either remove the TODO or open a tracking issue and reference it.
I can open a tracking issue for removal once the dual-backend path fully lands.
bin/test-runner/w3f/accumulate.ts (1)
168-169: Typo in failure message.“successfull” → “successful”.
- assert.fail(`Expected successfull accumulation for ${PvmBackendNames[pvm]}, got: ${result}`); + assert.fail(`Expected successful accumulation for ${PvmBackendNames[pvm]}, got: ${result}`);packages/core/pvm-interpreter/memory/errors.ts (1)
7-23: PageFault construction semantics: modulo wrap and default flag.
- Using
maybeMemoryIndex % MEMORY_SIZEwraps negatives and OOB indices; this may mask true OOB faults. Confirm intended behavior vs. reporting the actual offending address.- Private ctor defaults
isAccessFault = true, but both factories default tofalse; effectively all faults created via factories are not access faults. Confirm this is deliberate.If wrap is unintended, prefer validation and mapping to the start page without
%:- const memoryIndex = tryAsMemoryIndex(maybeMemoryIndex % MEMORY_SIZE); + const memoryIndex = tryAsMemoryIndex(maybeMemoryIndex);If you need clamping (not wrapping), clamp instead of modulo and add a short comment.
packages/core/pvm-interpreter-ananas/index.ts (2)
153-159: Status mapping needs rationale.
status < 0 => Status.OKis non-obvious. Add a short comment referencing Ananas status encoding or guard only for a known sentinel.
165-167: Exit param should be null unless HOST/FAULT.Interface docs imply the exit arg is meaningful only on HOST/FAULT. Consider returning
nullotherwise.- getExitParam(): U32 | null { - return tryAsU32(this.instance.getExitArg()); - } + getExitParam(): U32 | null { + const s = this.getStatus(); + if (s !== Status.HOST && s !== Status.FAULT) return null; + return tryAsU32(this.instance.getExitArg()); + }bin/pvm/index.ts (2)
29-30: Gas prints may be unreadable.
gas.get()returns an opaque Gas. Convert toBigIntfor consistent output.- console.info(`Gas: 🫐 ${pvmTb.gas.get()}| 🍍 ${pvmAnanas.gas.get()}`); + console.info(`Gas: 🫐 ${BigInt(pvmTb.gas.get())} | 🍍 ${BigInt(pvmAnanas.gas.get())}`);And same for the final log.
Also applies to: 46-47
21-39: Stepping loop semantics OK for demo; consider run-to-yield for Ananas.For performance, you can use
runProgram()on Ananas (it already batches withnSteps(INF)), then advance TB until yield to reduce JS↔WASM thrash.
There was a problem hiding this comment.
Actionable comments posted: 13
Caution
Some comments are outside the diff and can’t be posted inline due to platform limitations.
⚠️ Outside diff range comments (2)
packages/jam/transition/accumulate/deferred-transfers.ts (1)
75-77: Comparator may return BigInt — risk of TypeError in sort.If
sourceis a U64/bigint,a.source - b.sourceyields BigInt; engines compare against 0 (Number) internally, which can throw. Use relational compare returning Number.- .toSorted((a, b) => a.source - b.source); + .toSorted((a, b) => (a.source < b.source ? -1 : a.source > b.source ? 1 : 0));packages/jam/jam-host-calls/refine/invoke.test.ts (1)
56-56: Add safety note for bigint→Number cast.
const addressAsNumber = Number(address);converts U64 to Number; add a short comment explaining why it’s safe (address is bounded to 32‑bit memory index).As per coding guidelines.
Suggested change:
- const addressAsNumber = Number(address); + // Safe: addresses are constrained to 32-bit memory indices in tests, well within JS Number precision. + const addressAsNumber = Number(address);
♻️ Duplicate comments (1)
packages/core/pvm-interpreter-ananas/api.ts (1)
1-8: Interface breadth tightly couples to raw WASM types.Prefer narrowing to the minimal surface used by our wrapper to reduce churn when raw types change. I see this was already raised and tracked.
Also applies to: 10-182
🧹 Nitpick comments (18)
bin/jam/args.ts (1)
86-97: Avoidasconversion by using explicit switch statement.The
as PvmBackendcast on line 92 violates the coding guideline requiring tryAs methods instead ofasconversions. Although the cast is safe due to the indexOf validation, an explicit switch statement would eliminate the cast entirely and improve type safety.As per coding guidelines.
Apply this diff to replace the indexOf approach with an explicit switch:
const { pvm } = parseStringOption( args, "pvm", (v) => { - const pvm = PvmBackendNames.indexOf(v); - if (pvm >= 0) { - return pvm as PvmBackend; - } - throw Error(`Use one of ${PvmBackendNames.join(", ")}`); + switch (v) { + case "built-in": + return PvmBackend.BuiltIn; + case "ananas": + return PvmBackend.Ananas; + default: + throw Error(`Use one of ${PvmBackendNames.join(", ")}`); + } }, NODE_DEFAULTS.pvm, );packages/jam/node/jam-config.ts (1)
26-39: Consider consistent naming between parameter and field.The method parameter is named
nodeBackend(line 26, 34) but is stored as the fieldpvmBackend(line 50). While the field naming was correctly updated based on past review comments, using consistent naming throughout would improve clarity.Apply this diff to align parameter naming with the field:
static new({ isAuthoring, nodeName, nodeConfig, - nodeBackend, + pvmBackend, devConfig = null, networkConfig = null, ancestry = [], }: { isAuthoring?: boolean; nodeName: string; nodeConfig: NodeConfiguration; - nodeBackend: PvmBackend; + pvmBackend: PvmBackend; devConfig?: DevConfig | null; networkConfig?: NetworkConfig | null; ancestry?: [HeaderHash, TimeSlot][]; }) { - return new JamConfig(isAuthoring ?? false, nodeName, nodeConfig, nodeBackend, devConfig, networkConfig, ancestry); + return new JamConfig(isAuthoring ?? false, nodeName, nodeConfig, pvmBackend, devConfig, networkConfig, ancestry); }packages/jam/jam-host-calls/info.test.ts (1)
23-23: Test hermeticity: avoid shared gas counter across tests.Minor: create
const gas = gasCounter(tryAsGas(0))per test to prevent accidental state coupling in future changes.packages/jam/jam-host-calls/externalities/refine-externalities.ts (1)
71-73: Avoid Number(BigInt) cast; return enum safely without casts.Comply with guideline on bigint->number casts and eliminate need for
as.-export const toMemoryOperation = (v: number | bigint): MemoryOperation | null => - v <= MemoryOperation.Write && v >= MemoryOperation.Void ? Number(v) : null; +export const toMemoryOperation = (v: number | bigint): MemoryOperation | null => { + if (typeof v === "bigint") { + if (v === 0n) return MemoryOperation.Void; + if (v === 1n) return MemoryOperation.ZeroRead; + if (v === 2n) return MemoryOperation.ZeroWrite; + if (v === 3n) return MemoryOperation.Read; + if (v === 4n) return MemoryOperation.Write; + return null; + } + switch (v) { + case MemoryOperation.Void: + case MemoryOperation.ZeroRead: + case MemoryOperation.ZeroWrite: + case MemoryOperation.Read: + case MemoryOperation.Write: + return v; + default: + return null; + } +}As per coding guidelines.
packages/core/pvm-debugger-adapter/debugger-adapter.ts (1)
46-48: Handle memory write Result to avoid silent faults
memory.storeFrom(...)returns a Result; ignoring it may mask page faults in the debugger. Either unwrap and throw, or propagate an explicit error to the UI.- this.pvm.memory.storeFrom(tryAsMemoryIndex(address), value); + const r = this.pvm.memory.storeFrom(tryAsMemoryIndex(address), value); + // TODO: surface PageFault details in debugger instead of silently ignoring + // if (r.isError?.()) throw r.error; // adjust to your Result APIpackages/jam/jam-host-calls/accumulate/yield.test.ts (1)
8-10: Imports and test helpers: LGTM; avoid shared gas across tests
- Interface/import shifts and use of
emptyRegistersBuffer()and directHostCallMemoryare good.- Consider creating a fresh
gasCounter(tryAsGas(0))per test to avoid state leakage if gas is mutated.-const gas = gasCounter(tryAsGas(0)); +// In each test: +const gas = gasCounter(tryAsGas(0));Also applies to: 13-14, 25-37
packages/core/pvm-interpreter/memory/errors.ts (1)
19-23: Optional: normalize negative indices before moduloIf negatives can reach
fromMemoryIndex,(x % MEMORY_SIZE)is negative in JS. Either rely ontryAsMemoryIndexthrowing (current behavior) or normalize explicitly.-const memoryIndex = tryAsMemoryIndex(maybeMemoryIndex % MEMORY_SIZE); +const idx = ((maybeMemoryIndex % MEMORY_SIZE) + MEMORY_SIZE) % MEMORY_SIZE; +const memoryIndex = tryAsMemoryIndex(idx);bin/test-runner/w3f/pvm.ts (1)
65-65: Track TODO for dual-backend testing.This TODO aligns with past review discussions about running PVM tests on both Ananas and BuiltIn backends. Based on PR comments, a follow-up issue was requested to implement both PVM backends for w3f/pvm tests.
Would you like me to verify if a GitHub issue has been created to track this work, or help draft one?
packages/core/pvm-interpreter/interpreter.ts (1)
136-160: Avoid reassigninggasobject after making it readonly.After the change above, update resetGeneric to set the private field.
- this.gas = gasCounter(gas); + this._gas = gasCounter(gas);Optionally, expose a
reset(initialGas)on the counter to reuse the instance and avoid reallocations.packages/core/pvm-host-calls/interpreter-instance-manager.ts (2)
36-44: Tiny nit: avoid Promise.resolve in async function.You can just
return instance;here.- return Promise.resolve(instance); + return instance;
46-53: Release path OK. Consider FIFO for fairness.Optional: using a queue (shift from instances) can distribute wear on instances more evenly.
packages/core/pvm-host-calls/bin.ts (1)
8-8: Avoid hard-coding the backend; allow switching via env/CLI.Makes benchmarking Ananas vs BuiltIn trivial without code edits.
Apply:
-import { PvmBackend } from "@typeberry/config"; +import { PvmBackend } from "@typeberry/config"; +const pickBackend = () => + (process.env.PVM_BACKEND?.toLowerCase() === "ananas" ? PvmBackend.Ananas : PvmBackend.BuiltIn); ... -const pvmInstanceManager = await InterpreterInstanceManager.new(1, PvmBackend.BuiltIn); +const pvmInstanceManager = await InterpreterInstanceManager.new(1, pickBackend());bin/pvm/index.ts (1)
41-41: Guard against OOB access when printing the last instruction.Prevents
undefinedwheni === instructions.length.-console.info(`Instruction ${i}: ${instructions[i]}`); +const last = instructions[i] ?? "<end>"; +console.info(`Instruction ${i}: ${last}`);packages/core/pvm-interpreter-ananas/index.ts (4)
134-141: Avoid magic numbers for register layout.Use the imported constants for clarity and future‑proofing.
- const emptyRegisters = Array(13 * 8).fill(0); + const emptyRegisters = Array(NO_OF_REGISTERS * REGISTER_BYTE_SIZE).fill(0);
91-99: Use0nwhen comparingbigint.Keeps types consistent and avoids accidental number/bigint mixing.
- if (gasConsumed < 0) { + if (gasConsumed < 0n) {
39-43: Optional: assert 8‑byte alignment before creatingBigUint64Array.Defensive check to prevent accidental misaligned views.
getAllU64(): BigUint64Array { const bytes = this.getAllEncoded(); - return new BigUint64Array(bytes.buffer, bytes.byteOffset); + check`${bytes.byteOffset % REGISTER_BYTE_SIZE === 0} Unaligned register buffer.`; + return new BigUint64Array(bytes.buffer, bytes.byteOffset); }
170-177: Minor: simplify byte lowering.Array.from is clearer and V8 optimizes it well.
-function lowerBytes(data: Uint8Array): number[] { - const r = new Array<number>(data.length); - for (let i = 0; i < data.length; i++) { - r[i] = data[i]; - } - return r; -} +function lowerBytes(data: Uint8Array): number[] { + return Array.from(data); +}packages/jam/transition/accumulate/pvm-executor.ts (1)
87-89: Make instance pool size configurable.Expose via parameter or ChainSpec to tune throughput vs memory.
- private static async prepareBackend(pvm: PvmBackend) { - return PvmInstanceManager.new(4, pvm); - } + private static async prepareBackend(pvm: PvmBackend, poolSize = 4) { + return PvmInstanceManager.new(poolSize, pvm); + }
View all
Benchmarks summary: 63/63 OK ✅ |
There was a problem hiding this comment.
Actionable comments posted: 4
Caution
Some comments are outside the diff and can’t be posted inline due to platform limitations.
⚠️ Outside diff range comments (1)
packages/jam/jam-host-calls/fetch.ts (1)
269-272: Fix numeric bound comment (2^32) and optional clarity on conversion.The comment should reference 2^32, not 2*32. Suggest tightening the note.
- // NOTE: casting to `Number` is safe in both places, since we are always bounded - // by the actual length of the value returned which is smaller than `2*32`. + // NOTE: casting to Number is safe: indices are bounded by the actual value length, + // which is strictly less than 2^32 (TypedArray index range).Optionally, you can self-document the 32-bit bound:
- const chunk = value === null ? new Uint8Array() : value.raw.subarray(Number(offset), Number(offset + length)); + const start = Number(offset); + const end = Number(offset + length); + const chunk = value === null ? new Uint8Array() : value.raw.subarray(start, end);
♻️ Duplicate comments (7)
packages/jam/transition/accumulate/pvm-executor.ts (1)
80-84: Critical: HostCalls constructor contract still violated — issue from past review not resolved.The past review correctly identified that
HostCallsexpects two parameters(pvmInstanceManager, hostCallsManager), but lines 80-83 still pass an object config. Per the constructor signature in the relevant code snippets, you must first instantiateHostCallsManagerwith the config object, then pass both the manager andpvmInstanceManagertoHostCalls.Apply this diff to fix the constructor call:
+ const hostCallsManager = new HostCallsManager({ + missing: new Missing(), + handlers: hostCallHandlers, + }); - this.hostCalls = new HostCalls({ - missing: new Missing(), - handlers: hostCallHandlers, - }); + this.hostCalls = new HostCalls(pvmInstanceManager, hostCallsManager); this.pvm = new PvmHostCallExtension(pvmInstanceManager, this.hostCalls);You'll also need to add
HostCallsManagerto the imports on line 30.packages/core/pvm-interpreter-ananas/index.ts (1)
137-144: Replace magic number with constants.Line 139 uses the magic number
Array(13 * 8).fill(0)instead of the exported constantsNO_OF_REGISTERS * REGISTER_BYTE_SIZE.As per coding guidelines.
Apply this diff:
resetGeneric(program: Uint8Array, _pc: number, gas: Gas): void { const programArr = lowerBytes(program); - const emptyRegisters = Array(13 * 8).fill(0); + const emptyRegisters = Array(NO_OF_REGISTERS * REGISTER_BYTE_SIZE).fill(0); const pageMap = new Uint8Array(); const chunks = new Uint8Array(); this.gas.initialGas = gas; this.instance.resetGenericWithMemory(programArr, emptyRegisters, pageMap, chunks, BigInt(gas), false); }packages/core/pvm-interpreter/gas.ts (1)
34-43: Fix BigInt comparison bug and incorrect logic.Line 38 compares a bigint (
gasConsumed) to the number0, which will throw a TypeError at runtime. Additionally, the logic is incorrect: when current gas exceeds initial gas (negative consumption),used()should return 0, notinitialGas.Apply this diff:
used(): Gas { - const gasConsumed = tryAsU64(this.initialGas) - this.gas; - - // In we have less than zero left we assume that all gas has been consumed. - if (gasConsumed < 0) { - return this.initialGas; - } - - return tryAsGas(gasConsumed); + const init = tryAsU64(this.initialGas); + // Clamp to [0, initialGas] + if (this.gas >= init) { + return tryAsGas(0n); + } + return tryAsGas(init - this.gas); }packages/core/pvm-interface/memory.ts (1)
7-7: Address the PAGE_SIZE_SHIFT duplication flagged in past review.A previous review identified that
PAGE_SIZE_SHIFTis duplicated between this file andpackages/core/pvm-interpreter/memory/memory-consts.ts(line 4). The constant should be exported from this interface file to establish a single source of truth, then imported by the interpreter implementation.packages/core/pvm-host-calls/interpreter-instance-manager.ts (1)
9-10: Document Ananas concurrency constraints near this class.Add a brief note that Ananas must not run concurrently; this manager intentionally serializes access.
-// TODO [MaSo] Delete this & also make host calls independent from intepreters. +// TODO [MaSo] Delete this & make host calls independent from interpreters. +// NOTE: Ananas backend must not be instantiated/run concurrently. This manager +// serializes access (pool size = 1) to prevent parallel usage.packages/core/pvm-host-calls/host-call-memory.ts (2)
19-22: Add explicit safety justification forNumber()cast on bigint.The comment explains that the lower 32 bits are taken but doesn't explicitly state why the
Number()conversion is safe. Per coding guidelines, bigint-to-Number casts require a safety explanation.The mask
0xffff_ffffnensures the value is ≤ 0xFFFFFFFF (2³²-1), which is well within JavaScript's safe integer range (2⁵³-1), making theNumber()conversion safe.Apply this diff:
// NOTE: We always take lower 32 bits from register value. // // https://graypaper.fluffylabs.dev/#/ab2cdbd/25ed0025ed00?v=0.7.2 + // Safe to convert to Number: masking to 32 bits guarantees value ≤ 2^32-1, within JS safe integer range. const address = tryAsU32(Number(regAddress & 0xffff_ffffn));As per coding guidelines.
37-40: Add explicit safety justification forNumber()cast on bigint.The comment mentions "it's safe" but doesn't explain why. Per coding guidelines, bigint-to-Number casts require explicit safety reasoning.
Apply this diff:
// https://graypaper.fluffylabs.dev/#/ab2cdbd/25ed0025ed00?v=0.7.2 // - // NOTE we are taking the the lower U32 part of the register, hence it's safe. + // NOTE: We take the lower U32 part of the register (masked to 32 bits). + // Safe to convert to Number: value ≤ 2^32-1, within JS safe integer range. const address = tryAsU32(Number(regAddress & 0xffff_ffffn));As per coding guidelines.
🧹 Nitpick comments (6)
packages/core/pvm-interpreter-ananas/index.ts (3)
23-24: TODO: Track WASM inlining implementation.The TODO comment indicates this WASM file loading approach is temporary. Consider tracking this technical debt.
Would you like me to create an issue to track the implementation of WASM file inlining for the final build?
96-104: Add comment explaining negative gasConsumed handling.The check for negative gasConsumed at lines 99-100 handles an edge case where
set()might have been called with a value exceedinginitialGas. Since this is non-obvious, add a brief comment explaining why this defensive check is necessary.Apply this diff:
used(): Gas { const gasConsumed = BigInt(this.initialGas) - BigInt(this.get()); + // Handle edge case where set() was called with value > initialGas if (gasConsumed < 0) { return this.initialGas; }
156-163: Document why negative status maps to Status.OK.The logic at lines 158-160 returns
Status.OKwhen the status is negative, but there's no explanation for why Ananas returns negative status values or what they represent. Add a comment explaining this Ananas-specific behavior.Apply this diff:
getStatus(): Status { const status = this.instance.getStatus(); + // Ananas returns negative status values to indicate [TODO: explain what negative means] if (status < 0) { return Status.OK; }packages/jam/jam-host-calls/fetch.ts (3)
278-282: Trace log for null value.When
valueis null, the trace printsundefined. Consider logging an explicit sentinel for consistency withHostCallResult.NONE.- logger.trace`FETCH(${kind}) <- ${value?.toStringTruncated()}`; + logger.trace`FETCH(${kind}) <- ${value ? value.toStringTruncated() : 'NONE'}`;
284-379: getValue type update LGTM; optional readability refactor.Type change to
HostCallRegistersis consistent. Optionally, convert the if‑chain to aswitch (kind)for readability; no behavior change needed.
382-403: Duplicate enum values: add a note to avoid confusion.Sharing values between legacy and new variants (14, 15) is intentional due to GP version gating. Add a brief comment explaining the aliasing and the
Compatibility.isGreaterOrEqual(V0_7_1)guard.export enum FetchKind { @@ - LegacyAllOperands = 14, - AllTransfersAndOperands = 14, + // 14: legacy/new alias; gated by GP >= 0.7.1 + LegacyAllOperands = 14, + AllTransfersAndOperands = 14, @@ - LegacyOneOperand = 15, - OneTransferOrOperand = 15, + // 15: legacy/new alias; gated by GP >= 0.7.1 + LegacyOneOperand = 15, + OneTransferOrOperand = 15, }
📜 Review details
Configuration used: Path: .coderabbit.yaml
Review profile: CHILL
Plan: Pro
📒 Files selected for processing (61)
bin/jam/args.test.ts(3 hunks)bin/jam/build-for-npm.sh(2 hunks)bin/jam/index.ts(1 hunks)bin/rpc/test/e2e-setup.ts(1 hunks)bin/tci/index.test.ts(1 hunks)bin/tci/index.ts(1 hunks)bin/test-runner/w3f/accumulate.ts(3 hunks)bin/test-runner/w3f/pvm.ts(2 hunks)bin/test-runner/w3f/runners.ts(2 hunks)packages/README.md(2 hunks)packages/core/pvm-debugger-adapter/index.ts(2 hunks)packages/core/pvm-host-calls/bin.ts(1 hunks)packages/core/pvm-host-calls/host-call-handler.ts(3 hunks)packages/core/pvm-host-calls/host-call-memory.test.ts(5 hunks)packages/core/pvm-host-calls/host-call-memory.ts(1 hunks)packages/core/pvm-host-calls/host-call-registers.test.ts(1 hunks)packages/core/pvm-host-calls/host-call-registers.ts(1 hunks)packages/core/pvm-host-calls/host-calls-manager.ts(2 hunks)packages/core/pvm-host-calls/host-calls.ts(7 hunks)packages/core/pvm-host-calls/index.ts(1 hunks)packages/core/pvm-host-calls/interpreter-instance-manager.ts(2 hunks)packages/core/pvm-interface/gas.ts(1 hunks)packages/core/pvm-interface/memory.ts(1 hunks)packages/core/pvm-interpreter-ananas/index.ts(1 hunks)packages/core/pvm-interpreter-ananas/package.json(1 hunks)packages/core/pvm-interpreter/gas.ts(2 hunks)packages/jam/jam-host-calls/accumulate/assign.ts(2 hunks)packages/jam/jam-host-calls/accumulate/bless.ts(2 hunks)packages/jam/jam-host-calls/accumulate/checkpoint.ts(2 hunks)packages/jam/jam-host-calls/accumulate/designate.ts(2 hunks)packages/jam/jam-host-calls/accumulate/eject.ts(2 hunks)packages/jam/jam-host-calls/accumulate/forget.ts(2 hunks)packages/jam/jam-host-calls/accumulate/new.ts(2 hunks)packages/jam/jam-host-calls/accumulate/provide.ts(2 hunks)packages/jam/jam-host-calls/accumulate/query.ts(2 hunks)packages/jam/jam-host-calls/accumulate/solicit.ts(2 hunks)packages/jam/jam-host-calls/accumulate/transfer.ts(3 hunks)packages/jam/jam-host-calls/accumulate/upgrade.ts(2 hunks)packages/jam/jam-host-calls/accumulate/yield.ts(2 hunks)packages/jam/jam-host-calls/externalities/refine-externalities.test.ts(4 hunks)packages/jam/jam-host-calls/externalities/refine-externalities.ts(6 hunks)packages/jam/jam-host-calls/fetch.ts(3 hunks)packages/jam/jam-host-calls/gas.ts(2 hunks)packages/jam/jam-host-calls/info.ts(2 hunks)packages/jam/jam-host-calls/log.ts(2 hunks)packages/jam/jam-host-calls/lookup.ts(2 hunks)packages/jam/jam-host-calls/missing.ts(2 hunks)packages/jam/jam-host-calls/read.ts(2 hunks)packages/jam/jam-host-calls/refine/export.ts(2 hunks)packages/jam/jam-host-calls/refine/expunge.ts(2 hunks)packages/jam/jam-host-calls/refine/historical-lookup.ts(2 hunks)packages/jam/jam-host-calls/refine/invoke.ts(5 hunks)packages/jam/jam-host-calls/refine/machine.ts(2 hunks)packages/jam/jam-host-calls/refine/pages.ts(2 hunks)packages/jam/jam-host-calls/refine/peek.ts(2 hunks)packages/jam/jam-host-calls/refine/poke.ts(2 hunks)packages/jam/jam-host-calls/utils.ts(3 hunks)packages/jam/jam-host-calls/write.ts(2 hunks)packages/jam/node/jam-config.ts(3 hunks)packages/jam/transition/accumulate/pvm-executor.ts(4 hunks)packages/workers/importer/protocol.ts(2 hunks)
🚧 Files skipped from review as they are similar to previous changes (24)
- packages/README.md
- bin/jam/index.ts
- bin/jam/build-for-npm.sh
- packages/jam/jam-host-calls/accumulate/forget.ts
- packages/workers/importer/protocol.ts
- packages/jam/jam-host-calls/refine/historical-lookup.ts
- packages/jam/jam-host-calls/refine/expunge.ts
- packages/jam/jam-host-calls/externalities/refine-externalities.ts
- packages/core/pvm-interpreter-ananas/package.json
- packages/core/pvm-debugger-adapter/index.ts
- packages/core/pvm-interface/gas.ts
- packages/jam/jam-host-calls/refine/export.ts
- packages/jam/jam-host-calls/log.ts
- packages/jam/jam-host-calls/refine/machine.ts
- bin/test-runner/w3f/runners.ts
- bin/jam/args.test.ts
- packages/core/pvm-host-calls/index.ts
- packages/jam/jam-host-calls/accumulate/bless.ts
- packages/jam/jam-host-calls/accumulate/upgrade.ts
- packages/core/pvm-host-calls/host-call-registers.test.ts
- packages/jam/jam-host-calls/write.ts
- packages/jam/jam-host-calls/accumulate/transfer.ts
- packages/core/pvm-host-calls/host-call-registers.ts
- packages/jam/jam-host-calls/accumulate/assign.ts
🧰 Additional context used
📓 Path-based instructions (2)
**/*.ts
⚙️ CodeRabbit configuration file
**/*.ts: rules from./CODESTYLE.mdshould be adhered to.
**/*.ts: Any function whose documentation mention it must not be used in production code,
can be safely used in*.test.tsfiles. Other usage should be carefuly reviewed
and the comment must explain why it's safe to use there.
**/*.ts:asconversions must not be used. Suggest usingtryAsconversion methods.
**/*.ts: Classes withstatic Codecfield must have private constructor and staticcreatemethod.
**/*.ts: Casting abigint(orU64) usingNumber(x)must have an explanation comment why
it is safe.
**/*.ts: When making changes to code with comments containing links (in classes, constants, methods, etc.)
to graypaper.fluffylabs.dev, ensure those links point to the current version for this update.
Files:
packages/jam/jam-host-calls/lookup.tsbin/tci/index.test.tsbin/test-runner/w3f/accumulate.tspackages/jam/jam-host-calls/accumulate/new.tspackages/jam/jam-host-calls/accumulate/solicit.tspackages/jam/jam-host-calls/refine/peek.tspackages/jam/jam-host-calls/missing.tspackages/jam/jam-host-calls/accumulate/checkpoint.tsbin/rpc/test/e2e-setup.tspackages/jam/jam-host-calls/accumulate/query.tspackages/jam/jam-host-calls/info.tspackages/jam/jam-host-calls/refine/pages.tspackages/core/pvm-interpreter-ananas/index.tspackages/jam/jam-host-calls/externalities/refine-externalities.test.tspackages/jam/jam-host-calls/refine/invoke.tspackages/core/pvm-host-calls/host-calls.tspackages/core/pvm-host-calls/host-call-memory.tspackages/core/pvm-host-calls/host-call-handler.tspackages/jam/jam-host-calls/accumulate/provide.tspackages/jam/jam-host-calls/gas.tspackages/jam/jam-host-calls/accumulate/eject.tsbin/tci/index.tspackages/jam/jam-host-calls/accumulate/yield.tspackages/core/pvm-interpreter/gas.tsbin/test-runner/w3f/pvm.tspackages/jam/node/jam-config.tspackages/jam/jam-host-calls/accumulate/designate.tspackages/jam/jam-host-calls/utils.tspackages/core/pvm-host-calls/host-calls-manager.tspackages/core/pvm-host-calls/interpreter-instance-manager.tspackages/jam/jam-host-calls/refine/poke.tspackages/core/pvm-interface/memory.tspackages/jam/transition/accumulate/pvm-executor.tspackages/core/pvm-host-calls/host-call-memory.test.tspackages/core/pvm-host-calls/bin.tspackages/jam/jam-host-calls/fetch.tspackages/jam/jam-host-calls/read.ts
packages/core/**/*.ts
⚙️ CodeRabbit configuration file
packages/core/**/*.ts: Core packages must not import any JAM-related packages
(i.e. packages defined underpackages/jam/**)
Files:
packages/core/pvm-interpreter-ananas/index.tspackages/core/pvm-host-calls/host-calls.tspackages/core/pvm-host-calls/host-call-memory.tspackages/core/pvm-host-calls/host-call-handler.tspackages/core/pvm-interpreter/gas.tspackages/core/pvm-host-calls/host-calls-manager.tspackages/core/pvm-host-calls/interpreter-instance-manager.tspackages/core/pvm-interface/memory.tspackages/core/pvm-host-calls/host-call-memory.test.tspackages/core/pvm-host-calls/bin.ts
🧠 Learnings (1)
📚 Learning: 2025-05-16T16:23:08.685Z
Learnt from: tomusdrw
PR: FluffyLabs/typeberry#390
File: packages/jam/jam-host-calls/fetch.ts:220-226
Timestamp: 2025-05-16T16:23:08.685Z
Learning: In the typeberry codebase, host call (HC) implementations define a gas cost but don't need to explicitly consume gas in their execute() methods. Gas accounting is handled outside of the host call implementation.
Applied to files:
packages/jam/jam-host-calls/refine/peek.tspackages/jam/jam-host-calls/missing.tspackages/core/pvm-host-calls/host-call-handler.tspackages/jam/jam-host-calls/accumulate/yield.tspackages/jam/jam-host-calls/refine/poke.ts
🧬 Code graph analysis (37)
packages/jam/jam-host-calls/lookup.ts (3)
packages/core/pvm-interface/gas.ts (1)
IGasCounter(25-57)packages/core/pvm-host-calls/host-call-registers.ts (1)
HostCallRegisters(4-25)packages/core/pvm-host-calls/host-call-memory.ts (1)
HostCallMemory(5-43)
bin/tci/index.test.ts (1)
packages/jam/config-node/node-config.ts (1)
NODE_DEFAULTS(19-23)
bin/test-runner/w3f/accumulate.ts (4)
bin/test-runner/w3f/spec.ts (1)
getChainSpec(3-9)packages/core/hash/blake2b.ts (1)
Blake2b(8-44)packages/jam/transition/accumulate/accumulate-output.ts (1)
AccumulateOutput(14-19)packages/jam/config/pvm-backend.ts (1)
PvmBackendNames(2-2)
packages/jam/jam-host-calls/accumulate/new.ts (3)
packages/core/pvm-interface/gas.ts (1)
IGasCounter(25-57)packages/core/pvm-host-calls/host-call-registers.ts (1)
HostCallRegisters(4-25)packages/core/pvm-host-calls/host-call-memory.ts (1)
HostCallMemory(5-43)
packages/jam/jam-host-calls/accumulate/solicit.ts (3)
packages/core/pvm-interface/gas.ts (1)
IGasCounter(25-57)packages/core/pvm-host-calls/host-call-registers.ts (1)
HostCallRegisters(4-25)packages/core/pvm-host-calls/host-call-memory.ts (1)
HostCallMemory(5-43)
packages/jam/jam-host-calls/refine/peek.ts (3)
packages/core/pvm-interface/gas.ts (1)
IGasCounter(25-57)packages/core/pvm-host-calls/host-call-registers.ts (1)
HostCallRegisters(4-25)packages/core/pvm-host-calls/host-call-memory.ts (1)
HostCallMemory(5-43)
packages/jam/jam-host-calls/missing.ts (3)
packages/core/pvm-interface/gas.ts (1)
IGasCounter(25-57)packages/core/pvm-host-calls/host-call-registers.ts (1)
HostCallRegisters(4-25)packages/core/pvm-host-calls/host-call-memory.ts (1)
HostCallMemory(5-43)
packages/jam/jam-host-calls/accumulate/checkpoint.ts (2)
packages/core/pvm-interface/gas.ts (1)
IGasCounter(25-57)packages/core/pvm-host-calls/host-call-registers.ts (1)
HostCallRegisters(4-25)
bin/rpc/test/e2e-setup.ts (2)
packages/jam/node/jam-config.ts (1)
JamConfig(21-58)packages/jam/config-node/node-config.ts (1)
NODE_DEFAULTS(19-23)
packages/jam/jam-host-calls/accumulate/query.ts (3)
packages/core/pvm-interface/gas.ts (1)
IGasCounter(25-57)packages/core/pvm-host-calls/host-call-registers.ts (1)
HostCallRegisters(4-25)packages/core/pvm-host-calls/host-call-memory.ts (1)
HostCallMemory(5-43)
packages/jam/jam-host-calls/info.ts (3)
packages/core/pvm-interface/gas.ts (1)
IGasCounter(25-57)packages/core/pvm-host-calls/host-call-registers.ts (1)
HostCallRegisters(4-25)packages/core/pvm-host-calls/host-call-memory.ts (1)
HostCallMemory(5-43)
packages/jam/jam-host-calls/refine/pages.ts (2)
packages/core/pvm-interface/gas.ts (1)
IGasCounter(25-57)packages/core/pvm-host-calls/host-call-registers.ts (1)
HostCallRegisters(4-25)
packages/core/pvm-interpreter-ananas/index.ts (6)
packages/core/pvm-interface/registers.ts (3)
IRegisters(5-18)NO_OF_REGISTERS(1-1)REGISTER_BYTE_SIZE(2-2)packages/core/utils/debug.ts (1)
check(15-26)packages/core/pvm-interface/memory.ts (3)
IMemory(18-24)PageFault(9-11)getPageStartAddress(13-15)packages/core/numbers/index.ts (2)
U32(24-24)tryAsU32(48-51)packages/core/pvm-interface/gas.ts (4)
IGasCounter(25-57)Gas(9-9)tryAsGas(18-19)tryAsBigGas(15-15)packages/core/pvm-interface/pvm.ts (1)
IPvmInterpreter(7-31)
packages/jam/jam-host-calls/externalities/refine-externalities.test.ts (2)
packages/core/pvm-host-calls/host-call-memory.ts (1)
HostCallMemory(5-43)packages/core/pvm-host-calls/host-call-registers.ts (1)
HostCallRegisters(4-25)
packages/jam/jam-host-calls/refine/invoke.ts (5)
packages/core/pvm-interface/registers.ts (2)
NO_OF_REGISTERS(1-1)REGISTER_BYTE_SIZE(2-2)packages/core/pvm-interface/gas.ts (1)
IGasCounter(25-57)packages/core/pvm-host-calls/host-call-registers.ts (1)
HostCallRegisters(4-25)packages/core/pvm-host-calls/host-call-memory.ts (1)
HostCallMemory(5-43)packages/core/bytes/bytes.ts (1)
Bytes(176-242)
packages/core/pvm-host-calls/host-calls.ts (4)
packages/core/pvm-interface/pvm.ts (1)
IPvmInterpreter(7-31)packages/core/pvm-host-calls/host-call-registers.ts (1)
HostCallRegisters(4-25)packages/core/pvm-host-calls/host-call-memory.ts (1)
HostCallMemory(5-43)packages/core/utils/safe-alloc-uint8array.ts (1)
safeAllocUint8Array(7-13)
packages/core/pvm-host-calls/host-call-memory.ts (3)
packages/core/pvm-interface/memory.ts (2)
IMemory(18-24)PageFault(9-11)packages/core/numbers/index.ts (2)
U64(27-27)tryAsU32(48-51)packages/core/utils/result.ts (4)
Result(4-4)Result(81-111)OK(25-25)OK(26-26)
packages/core/pvm-host-calls/host-call-handler.ts (3)
packages/core/pvm-interface/gas.ts (2)
Gas(9-9)IGasCounter(25-57)packages/core/pvm-host-calls/host-call-registers.ts (1)
HostCallRegisters(4-25)packages/core/pvm-host-calls/host-call-memory.ts (1)
HostCallMemory(5-43)
packages/jam/jam-host-calls/accumulate/provide.ts (3)
packages/core/pvm-interface/gas.ts (1)
IGasCounter(25-57)packages/core/pvm-host-calls/host-call-registers.ts (1)
HostCallRegisters(4-25)packages/core/pvm-host-calls/host-call-memory.ts (1)
HostCallMemory(5-43)
packages/jam/jam-host-calls/gas.ts (2)
packages/core/pvm-interface/gas.ts (1)
IGasCounter(25-57)packages/core/pvm-host-calls/host-call-registers.ts (1)
HostCallRegisters(4-25)
packages/jam/jam-host-calls/accumulate/eject.ts (3)
packages/core/pvm-interface/gas.ts (1)
IGasCounter(25-57)packages/core/pvm-host-calls/host-call-registers.ts (1)
HostCallRegisters(4-25)packages/core/pvm-host-calls/host-call-memory.ts (1)
HostCallMemory(5-43)
bin/tci/index.ts (1)
packages/jam/config-node/node-config.ts (1)
NODE_DEFAULTS(19-23)
packages/jam/jam-host-calls/accumulate/yield.ts (3)
packages/core/pvm-interface/gas.ts (1)
IGasCounter(25-57)packages/core/pvm-host-calls/host-call-registers.ts (1)
HostCallRegisters(4-25)packages/core/pvm-host-calls/host-call-memory.ts (1)
HostCallMemory(5-43)
packages/core/pvm-interpreter/gas.ts (2)
packages/core/pvm-interface/gas.ts (3)
Gas(9-9)IGasCounter(25-57)tryAsGas(18-19)packages/core/numbers/index.ts (2)
tryAsU64(57-61)U64(27-27)
bin/test-runner/w3f/pvm.ts (1)
packages/core/pvm-interface/gas.ts (1)
tryAsGas(18-19)
packages/jam/node/jam-config.ts (2)
packages/jam/config-node/node-config.ts (1)
NodeConfiguration(44-73)packages/jam/block/common.ts (1)
TimeSlot(16-16)
packages/jam/jam-host-calls/accumulate/designate.ts (3)
packages/core/pvm-interface/gas.ts (1)
IGasCounter(25-57)packages/core/pvm-host-calls/host-call-registers.ts (1)
HostCallRegisters(4-25)packages/core/pvm-host-calls/host-call-memory.ts (1)
HostCallMemory(5-43)
packages/jam/jam-host-calls/utils.ts (3)
packages/core/pvm-host-calls/host-call-registers.ts (1)
HostCallRegisters(4-25)packages/core/utils/safe-alloc-uint8array.ts (1)
safeAllocUint8Array(7-13)packages/core/pvm-interface/registers.ts (1)
REGISTER_BYTE_SIZE(2-2)
packages/core/pvm-host-calls/host-calls-manager.ts (1)
packages/core/pvm-host-calls/host-call-registers.ts (1)
HostCallRegisters(4-25)
packages/core/pvm-host-calls/interpreter-instance-manager.ts (3)
packages/core/pvm-interface/pvm.ts (1)
IPvmInterpreter(7-31)packages/core/pvm-interpreter-ananas/index.ts (1)
AnanasInterpreter(107-172)packages/core/utils/debug.ts (1)
assertNever(36-38)
packages/jam/jam-host-calls/refine/poke.ts (3)
packages/core/pvm-interface/gas.ts (1)
IGasCounter(25-57)packages/core/pvm-host-calls/host-call-registers.ts (1)
HostCallRegisters(4-25)packages/core/pvm-host-calls/host-call-memory.ts (1)
HostCallMemory(5-43)
packages/core/pvm-interface/memory.ts (3)
packages/core/pvm-interpreter/memory/memory-consts.ts (1)
PAGE_SIZE_SHIFT(4-4)packages/core/pvm-interpreter/memory/errors.ts (1)
PageFault(7-24)packages/core/numbers/index.ts (2)
U32(24-24)tryAsU32(48-51)
packages/jam/transition/accumulate/pvm-executor.ts (2)
packages/core/pvm-host-calls/host-calls.ts (1)
HostCalls(37-144)packages/core/pvm-host-calls/index.ts (1)
HostCalls(4-4)
packages/core/pvm-host-calls/host-call-memory.test.ts (4)
packages/core/pvm-interface/memory.ts (4)
IMemory(18-24)PageFault(9-11)getPageStartAddress(13-15)MAX_MEMORY_INDEX(4-4)packages/core/numbers/index.ts (3)
U32(24-24)tryAsU32(48-51)tryAsU64(57-61)packages/core/utils/result.ts (4)
Result(4-4)Result(81-111)OK(25-25)OK(26-26)packages/core/pvm-host-calls/host-call-memory.ts (1)
HostCallMemory(5-43)
packages/core/pvm-host-calls/bin.ts (4)
packages/core/pvm-host-calls/host-calls-manager.ts (2)
HostCallsManager(16-59)NoopMissing(61-70)packages/core/pvm-host-calls/interpreter-instance-manager.ts (1)
InterpreterInstanceManager(10-51)packages/core/pvm-host-calls/host-calls.ts (1)
HostCalls(37-144)packages/core/pvm-interface/gas.ts (1)
tryAsGas(18-19)
packages/jam/jam-host-calls/fetch.ts (3)
packages/core/pvm-interface/gas.ts (1)
IGasCounter(25-57)packages/core/pvm-host-calls/host-call-registers.ts (1)
HostCallRegisters(4-25)packages/core/pvm-host-calls/host-call-memory.ts (1)
HostCallMemory(5-43)
packages/jam/jam-host-calls/read.ts (3)
packages/core/pvm-interface/gas.ts (1)
IGasCounter(25-57)packages/core/pvm-host-calls/host-call-registers.ts (1)
HostCallRegisters(4-25)packages/core/pvm-host-calls/host-call-memory.ts (1)
HostCallMemory(5-43)
⏰ Context from checks skipped due to timeout of 120000ms. You can increase the timeout in your CodeRabbit configuration to a maximum of 15 minutes (900000ms). (2)
- GitHub Check: run (22.x)
- GitHub Check: benchmarks (22.x)
🔇 Additional comments (46)
packages/jam/node/jam-config.ts (1)
8-8: LGTM! pvmBackend parameter properly integrated.The
pvmBackendparameter is consistently threaded through the import, factory method signature, constructor call, and class field declaration. The naming matches the previous review feedback, and the implementation follows the existing pattern for other JamConfig parameters.Also applies to: 26-26, 34-34, 39-39, 49-50
bin/tci/index.test.ts (1)
16-16: LGTM! Test properly updated for new signature.The test correctly adds the
pvmBackendparameter usingNODE_DEFAULTS.pvm, ensuring the test remains aligned with the updatedJamConfig.newsignature.bin/tci/index.ts (1)
83-88: LGTM! TCI entry point updated correctly.The
createJamConfigfunction now properly includespvmBackend: NODE_DEFAULTS.pvmin the JamConfig creation, ensuring the TCI uses the default built-in PVM backend.bin/rpc/test/e2e-setup.ts (1)
14-14: LGTM! E2E setup updated correctly.The end-to-end test setup now includes the
pvmBackendparameter with the default built-in backend, ensuring tests run with the expected PVM configuration.bin/test-runner/w3f/pvm.ts (2)
4-7: LGTM! Import reorganization follows the new abstraction layer.The imports correctly use the new
@typeberry/pvm-interfacepackage for interface types and constants, while keeping the concreteInterpreterfrom@typeberry/pvm-interpreter. This separation supports the multi-backend architecture introduced in this PR.
116-121: API migration to new interface looks correct.The changes properly migrate to the new PVM interface API:
resetGenericcorrectly passes all required arguments with proper type conversion viatryAsGaspvm.gas.get()provides the new gas access patternpvm.registers.getAllU64()uses the direct registers propertyNote: Running these tests on both Ananas and Built-in backends is tracked in issue #738 as per past review discussions.
packages/jam/transition/accumulate/pvm-executor.ts (4)
1-31: LGTM: Import updates support multi-backend architecture.The imports correctly bring in PvmBackend and Gas types to support the new PVM backend abstraction.
87-89: LGTM: Clean backend preparation abstraction.The async factory method correctly delegates to
PvmInstanceManager.new()and is appropriately used by the public factory methods.
140-150: LGTM: Factory method correctly supports async backend initialization.The async signature, PvmBackend parameter, and proper awaiting of
prepareBackend()correctly implement the multi-backend architecture.
153-162: LGTM: Factory method correctly implements async backend pattern.Consistent with
createAccumulateExecutor, this factory properly handles async backend initialization and multi-backend support.bin/test-runner/w3f/accumulate.ts (3)
15-15: LGTM: Backend imports added correctly.The new imports
PvmBackendandPvmBackendNamesare properly used throughout the file for backend parameterization and error messaging.
152-177: LGTM: Internal runner correctly parameterized by backend.The internal implementation properly threads the
pvmparameter through to theAccumulateconstructor and includes the backend name in error messages for clarity. The removal ofascasts (lines 162, 164) complies with coding guidelines.As per coding guidelines.
144-150: Both backends are correctly tested via runner registration.Verification confirms that
runAccumulateTestandrunAccumulateTestAnanasare properly imported inbin/test-runner/w3f/runners.tsand registered as separate test entries ("accumulate" and "accumulate_ananas"). The test infrastructure will invoke both functions, ensuring accumulate tests run on both BuiltIn and Ananas backends as intended.packages/core/pvm-interpreter-ananas/index.ts (3)
30-48: LGTM!The AnanasRegisters implementation is clean and correct. The size validation in setAllEncoded() properly uses the exported constants, and getAllU64() provides a convenient view over the register buffer.
50-71: LGTM!The AnanasMemory implementation correctly handles both store and read operations with proper error handling and PageFault reporting.
174-181: LGTM!The lowerBytes helper is straightforward and serves its purpose of converting Uint8Array to number[] for WASM interop.
packages/jam/jam-host-calls/fetch.ts (4)
256-282: Let me verify the complete HostCallHandler interface definition:Interface signature verified; no drift detected.
The
executemethod infetch.ts(line 256) correctly implements theHostCallHandlerinterface signature frompackages/core/pvm-host-calls/host-call-handler.ts(line 52). The parameter names match expected types, and in TypeScript, the names of the parameters do not need to match, so the underscore prefix on_gasis a valid TypeScript convention indicating an unused parameter. All 26+ implementations across the codebase follow this same pattern with no deviations.
6-6: IGasCounter migration verified successfully.The verification confirms the switch to
IGasCounterfrom@typeberry/pvm-interfaceis complete across all jam-host-calls files. No lingeringGasCounterimports or code usages exist—all 28 relevant files correctly importIGasCounter. The twoGasCounterreferences found are documentation comments in the interface source file (packages/core/pvm-interface/gas.ts), which is appropriate.
4-4: Import type swap verified; no runtime instanceof checks detected.Confirmed that
import type { HostCallHandler, HostCallMemory, HostCallRegisters }is correctly used without runtime dependencies, and noinstanceofchecks forHostCallRegistersorHostCallMemoryexist in the codebase.
12-239: Update Graypaper links to consistent version; clarify deprecated method policy.The code mixes v=0.6.6 (24 links), v=0.6.7 (1 link), and v=0.7.2 (2 links for new methods). Per coding guidelines, links should reflect the current version when the file is modified. Evidence suggests v=0.7.2 is current (used only for newly added methods).
Decide whether to:
- Update all links (including deprecated methods) to v=0.7.2, or
- Keep deprecated method links at v=0.6.6 with explanatory comments indicating intentional version preservation.
Then audit all 27 links accordingly.
packages/jam/jam-host-calls/lookup.ts (1)
5-7: LGTM: Clean migration to public interface types.The type updates correctly align with the new public API surface (HostCallRegisters, HostCallMemory, IGasCounter), and the async signature matches the HostCallHandler interface requirements.
Also applies to: 37-37
packages/jam/jam-host-calls/refine/invoke.ts (1)
6-19: LGTM: Public interface migration complete and past issue resolved.The type updates are consistent, and the previous review concern about using REGISTER_BYTE_SIZE instead of the literal 8 has been properly addressed at lines 30 and 90.
Also applies to: 47-47, 74-74, 90-90
packages/core/pvm-host-calls/bin.ts (1)
1-2: LGTM: Correct async instantiation with backend selection.The changes properly instantiate the interpreter manager asynchronously with PvmBackend.BuiltIn and update the runProgram call to match the new signature.
Also applies to: 8-8, 29-29
packages/jam/jam-host-calls/refine/peek.ts (1)
3-4: LGTM: Consistent type migration.The updates correctly adopt the public interface types (HostCallMemory, HostCallRegisters, IGasCounter) and async signature.
Also applies to: 9-9, 31-31
packages/jam/jam-host-calls/refine/pages.ts (1)
3-3: LGTM: Type updates consistent with public API.The changes correctly adopt HostCallRegisters and IGasCounter from the public interfaces.
Also applies to: 8-8, 36-36
packages/core/pvm-host-calls/host-calls-manager.ts (1)
3-3: LGTM: Consistent type migration.The updates correctly use Gas and HostCallRegisters types from the public interfaces.
Also applies to: 11-11, 44-44
packages/jam/jam-host-calls/gas.ts (1)
3-5: LGTM: Type updates align with public API.The signature correctly uses IGasCounter and HostCallRegisters from the public interfaces.
Also applies to: 22-22
packages/jam/jam-host-calls/utils.ts (2)
3-5: LGTM! Type migration aligns with the new public API surface.The import updates replacing
IHostCallRegisterswithHostCallRegistersand sourcing constants from@typeberry/pvm-interfaceare consistent with the broader refactor to establish a clean public interface boundary.Also applies to: 14-14
45-47: Well-implemented utility function.The
emptyRegistersBuffer()function correctly allocates a zeroed buffer usingsafeAllocUint8Array, which provides protection against excessive allocations. The size calculation (NO_OF_REGISTERS * REGISTER_BYTE_SIZE) is correct and aligns with the register structure.packages/core/pvm-interface/memory.ts (1)
1-24: Clean memory interface design.The
IMemoryinterface and supporting types (PageFault,getPageStartAddress) provide a well-structured abstraction for memory operations. The interface correctly usesResulttypes for error handling and properly documents the store/read operations.packages/jam/jam-host-calls/accumulate/checkpoint.ts (1)
2-4: LGTM! Consistent type migration to public interfaces.The updates replacing
IHostCallRegisterswithHostCallRegistersandGasCounterwithIGasCounteralign with the broader refactor. The explicit return statement on line 34 improves code clarity.Also applies to: 30-35
packages/jam/jam-host-calls/accumulate/eject.ts (1)
5-7: LGTM! Type updates are consistent.The migration to
HostCallMemory,HostCallRegisters, andIGasCounterfollows the established pattern across host-call implementations and maintains backward-compatible behavior.Also applies to: 31-31
packages/jam/jam-host-calls/accumulate/new.ts (1)
5-7: LGTM! Type migration follows the established pattern.The updates to use
HostCallMemory,HostCallRegisters, andIGasCounterare consistent with the interface standardization across the accumulate module.Also applies to: 32-32
packages/jam/jam-host-calls/info.ts (1)
6-8: LGTM! Consistent interface adoption.The type updates align with the standardized public API surface for host-call handlers, maintaining consistent behavior while improving type clarity.
Also applies to: 56-56
packages/jam/jam-host-calls/accumulate/query.ts (1)
7-8: LGTM! Type migration is consistent.The updates to
HostCallMemory,HostCallRegisters, andIGasCounterfollow the established pattern for host-call interface standardization.Also applies to: 13-13, 37-37
packages/jam/jam-host-calls/read.ts (1)
4-6: LGTM! Type migration completes the interface standardization.The updates to
HostCallMemory,HostCallRegisters, andIGasCountercomplete the consistent adoption of the new public API surface across host-call implementations.Also applies to: 35-35
packages/jam/jam-host-calls/externalities/refine-externalities.test.ts (1)
6-7: Tests aligned with public HostCall types: LGTM.Type updates for HostCallMemory/HostCallRegisters and Status look consistent with the new interfaces.
Also applies to: 101-107, 117-123, 133-137
packages/jam/jam-host-calls/accumulate/solicit.ts (1)
4-7: Confirm graypaper link version alignment with this update's target version.The link at line 17 uses v=0.6.7, consistent with most other accumulate module functions. However,
transfer.ts(same module) uses v=0.7.2, and the codebase contains references up to v=0.7.2. Without explicit knowledge of this update's target graypaper version, unable to definitively confirm if 0.6.7 is current. Please verify v=0.6.7 matches your update scope.packages/jam/jam-host-calls/refine/poke.ts (1)
1-31: LGTM! Clean migration to interface types.The type updates from
IHostCallMemory/IHostCallRegisters/GasCountertoHostCallMemory/HostCallRegisters/IGasCounterare consistent with the PR's interface abstraction objectives. The method logic remains unchanged.packages/jam/jam-host-calls/missing.ts (1)
1-19: LGTM! Consistent type migration.The updates from
IHostCallMemory/IHostCallRegisters/GasCountertoHostCallMemory/HostCallRegisters/IGasCounteralign with the interface abstraction introduced in this PR.packages/jam/jam-host-calls/accumulate/designate.ts (1)
4-32: LGTM! Type migration aligns with interface abstraction.The signature and import updates correctly adopt
HostCallMemory/HostCallRegisters/IGasCounterfrom the new PVM interface. Logic remains functionally identical.packages/core/pvm-host-calls/host-calls.ts (2)
43-68: Verify the IPvmInterpreter-based refactoring, particularly the memory load logic.The refactoring introduces encoded register/memory views and updates gas accounting to use
pvmInstance.gas.used(). The HALT path reconstruction looks correct.One concern: Line 54 performs a
Number()cast on a bigint without an explicit safety comment, violating the coding guidelines.Apply this diff to add the required safety comment:
const regs = new HostCallRegisters(pvmInstance.registers.getAllEncoded()); const memory = new HostCallMemory(pvmInstance.memory); const address = regs.get(7); - // NOTE we are taking the the lower U32 part of the register, hence it's safe. + // NOTE: We take the lower U32 part of the register (masked to 32 bits). + // Safe to convert to Number: value ≤ 2^32-1, within JS safe integer range. const length = Number(regs.get(8) & 0xffff_ffffn);As per coding guidelines.
70-143: LGTM! IPvmInterpreter integration is well-structured.The execution loop correctly:
- Creates encoded register/memory views for each host call
- Synchronizes register state back to the interpreter instance after host-call execution (line 108)
- Handles gas accounting via the new
IGasCounterinterface- Properly manages the PVM lifecycle with
resetJamandrunProgrampackages/core/pvm-host-calls/host-call-handler.ts (1)
1-52: LGTM! Interface definition properly migrated.The
HostCallHandlerinterface correctly adopts the new abstraction types (IGasCounter,HostCallMemory,HostCallRegisters) from@typeberry/pvm-interface. The execute signature and documentation are consistent with the broader refactoring.packages/core/pvm-host-calls/host-call-memory.test.ts (2)
13-38: LGTM! Test infrastructure properly migrated to IMemory interface.The
FakeMemorytest double correctly implementsIMemoryand returnsPageFaulterrors as expected. The test setup cleanly adopts the new interface abstraction.
74-84: LGTM! Address wrapping tests align with GP specification.The tests correctly verify that addresses exceeding
MAX_MEMORY_INDEXwrap (mod 2³²) and result in page faults at the wrapped address. This aligns with the Gray Paper specification for memory addressing.Also applies to: 121-131
2 participants
Created interface fo external interpreters.
Changed HostCallRegister, HCMemory and GasCounter API.
Added Ananas as second possible backend to run services on.
Added flag
--pvmto choose pvm backend for accumulate execution.Currently supported backends: ['built-in', 'ananas'].