feat(numscript): execute transactions on the bytecode VM#1597
Conversation
|
Important Review skippedAuto reviews are disabled on base/target branches other than the default branch. 🗂️ Base branches to auto review (1)
Please check the settings in the CodeRabbit UI or the ⚙️ Run configurationConfiguration used: Organization UI Review profile: CHILL Plan: Pro Run ID: You can disable this status message by setting the Use the checkbox below for a quick retry:
✨ Finishing Touches🧪 Generate unit tests (beta)
Thanks for using CodeRabbit! It's free for OSS, and your support helps us grow. If you like it, consider giving us a shout-out. Comment |
🛑 Changes requested — automated reviewThis PR routes live Numscript transaction execution through a new bytecode VM path. Several significant issues must be resolved before it can be safely merged. Blockers:
Major issues: Minor issues: Findings outside the diff🟡 [minor] Skipped tests leave VM-path metadata behaviour and the #322 security guard untested — Five Suggestion: Add a running test pinning the current 'metadata dropped' behaviour so the regression is intentional and visible. Keep at least one test exercising the validation guards via the interpreter producer so #322 coverage is not silently lost. |
NumaryBot
left a comment
NumaryBot
left a comment
There was a problem hiding this comment.
NumaryBot posted 2 new inline findings.
Summary: #1597 (comment)
NumaryBot
left a comment
NumaryBot
left a comment
There was a problem hiding this comment.
NumaryBot posted 2 new inline findings.
Summary: #1597 (comment)
NumaryBot
left a comment
NumaryBot
left a comment
There was a problem hiding this comment.
NumaryBot posted 3 new inline findings.
Summary: #1597 (comment)
Integrate the numscript compiler + register VM (formancehq/numscript#167, branch feat/exp/vm) and route the live create-transaction path through it instead of the tree-walking interpreter, to benchmark VM vs interpreter. - bump numscript to the feat/exp/vm pseudo-version (compiler + vm public API) - adapt the existing interpreter store adapter + discovery store to the new slice-based Store API (BalanceQuery/Balances/MetadataQuery/AccountsMetadata are now row slices carrying color/scope; set-metadata is a typed Value list) - add numscriptVMStoreAdapter (implements vm.Store: lazy per-key GetBalance / GetMetadata over the coverage-gated Scope) and numscriptVMPostingProducer - memoize compiled programs in NumscriptCache (GetOrCompile) and add SafeExecVM (recover + error mapping, mirror of SafeRun) - share applyNumscriptResult / numscriptBalance / numscriptAccountMetadata between the interpreter and VM producers (DRY) - restore verbatim metadata strings: the VM branch's Value.String() now quotes strings, so unwrap via numscript's tagged-JSON form Known limitation: the VM does not yet map set_tx_meta/set_account_meta output (numscript ExecVm TODO); the 5 metadata-asserting unit tests are skipped while the live path runs on the VM.
…transaction numscriptlib.ExecVm resets the machine's runstate on every call (Reset clears balances/sources/postings/currentAsset and rebinds the store) and register banks are overwritten by loads before use, so a *Vm is safe to reuse across executions. NewVm allocated the machine's register banks ([256]big.Int/big.Rat/monetary/string) on every transaction — the dominant per-tx allocation. Memoize the built *Vm in NumscriptCache (GetOrCompileVM, replacing GetOrCompile) and have the VM producer reuse it. Safe because numscript VM execution only happens on the single-threaded FSM apply path (admission discovery uses the interpreter). Adds a reuse test asserting the second run is not polluted by the first run's postings.
The numscript program cache key needs speed, not cryptographic collision resistance (the audit hash chain is the source of truth and stays on blake3). Switch HashScript to xxh3's 128-bit hash: allocation-free (no per-call hasher), faster than blake3, and 128 bits keeps accidental collisions negligible. Cache key type narrows from [32]byte to [16]byte.
gfyrag
force-pushed
the
feat/numscript-vm
branch
from
e1bafc9 to
1d34505
Compare
NumaryBot
left a comment
NumaryBot
left a comment
There was a problem hiding this comment.
NumaryBot posted 2 new inline findings.
Summary: #1597 (comment)
Codecov Report❌ Patch coverage is Additional details and impacted files@@ Coverage Diff @@
## release/v3.0 #1597 +/- ##
================================================
+ Coverage 74.72% 74.76% +0.03%
================================================
Files 430 431 +1
Lines 45722 45821 +99
================================================
+ Hits 34164 34256 +92
+ Misses 8510 8492 -18
- Partials 3048 3073 +25
Flags with carried forward coverage won't be shown. Click here to find out more. ☔ View full report in Codecov by Harness. 🚀 New features to boost your workflow:
|
paul-nicolas
left a comment
paul-nicolas
left a comment
There was a problem hiding this comment.
Multi-model PR review (Claude + Codex)
Both reviewers independently converged on a metadata-integrity regression and an error-contract regression introduced by routing the live create-transaction path through the numscript bytecode VM. Findings were verified against the current PR head (1d34505) and the pinned numscript library source.
Severity counts (posted inline): 1 Critical, 2 Major, 2 Medium.
Verification outcome
5 Critical/Major/Medium findings entered adversarial verification against the actual code; all 5 confirmed valid. One Codex finding was rejected: a claimed "VM producer drops the stale-inputs re-resolution guard (inputsResolutionHash)" — that guard does not exist on this PR branch (it belongs to a different, unrelated branch that briefly contaminated the review checkout), so numscriptVMPostingProducer drops nothing. Dropped, not posted.
Note
The PR description itself states this is a benchmarking branch, not intended to merge as-is (VM on a draft feat/exp/vm dependency). The findings below are what must be resolved before any of this reaches a release path; several are already acknowledged as known limitations in the description.
Minor / Nit (not posted inline)
internal/domain/processing/processor_transaction_numscript.go:237— [Minor]numscriptMetaValueToStringis correct but now dead on the live path (VM emits no metadata), and silently falls back to the quoted form if numscript's tagged-JSON shape ever changes; consider a guard/test if it stays.internal/domain/processing/processor_transaction_numscript_vm.go:26— [Nit]GetBalancecolor arg is ignored; fine for single-color volumes, but worth an assert/comment if numscript ever relies on color.go.mod:12— [Nit] numscript pinned to a draft-branch pseudo-version (bf5bbb96) on the live path; acknowledged in the PR description.
| isNumscript := script != nil && script.GetPlain() != "" | ||
| if isNumscript { | ||
| producer = &numscriptPostingProducer{cache: ctx.NumscriptCache, ledgerName: ledger, assetCache: ctx.AssetCache} | ||
| producer = &numscriptVMPostingProducer{cache: ctx.NumscriptCache, ledgerName: ledger} |
There was a problem hiding this comment.
[Critical] Live VM path silently drops set_tx_meta / set_account_meta — data loss with no error
This line routes every numscript create-transaction through numscriptVMPostingProducer. Its produce calls numscript.SafeExecVM → numscriptlib.ExecVm, and in the pinned library ExecVm returns only postings:
// numscript.go (v0.0.25-…-bf5bbb96)
// TODO map VM tx/account metadata (stringified) onto the typed contract
return ExecutionResult{Postings: res.Postings}, nilvm.Exec actually computes txMeta/accountsMeta internally, but the wrapper discards them, so result.Metadata and result.AccountsMetadata are always empty. applyNumscriptResult then guards on len(...) > 0 and skips the metadata blocks. Net effect: a script calling set_tx_meta/set_account_meta commits a successful transaction with the metadata silently missing — no error, no signal. This is squarely the situation CLAUDE.md invariant #7 forbids on the apply path, and it is a data-integrity regression versus the interpreter.
Fix: do not put the VM on the live path until ExecVm maps metadata; or make the VM producer fail loudly (ErrNumscriptRuntime) when the compiled program contains metadata opcodes, so such scripts are rejected rather than silently truncated. Re-enable the skipped metadata tests before this goes live.
| }() | ||
|
|
||
| result, execErr := numscriptlib.ExecVm(ctx, machine, vars, store) | ||
| err = convertNumscriptError(execErr) |
There was a problem hiding this comment.
[Major] Insufficient funds now surfaces as an internal runtime error instead of a business precondition error
convertNumscriptError only matches numscriptlib.MissingFundsErr (the interpreter's type, interpreter.MissingFundsErr). On the VM path, missing funds is raised as internal/vm.MissingFundsError / NegativeBalanceError — distinct types that are not exported at the library root, so errors.As misses and the error falls through to &domain.ErrNumscriptRuntime{} (KindInternal).
Result on the now-live path: a routine insufficient-funds outcome maps to an internal error kind instead of domain.ErrInsufficientFunds (INSUFFICIENT_FUNDS → KindPrecondition). Clients can no longer distinguish "not enough money" from "script/engine bug" — a contract regression for one of the most common ledger outcomes. It is also untested: the only non-force numscript funds tests exercise the interpreter/raw-posting paths.
Fix: detect the VM missing-funds error in convertNumscriptError and map it to ErrInsufficientFunds (push numscript to export the VM error, or match on its structured fields), and add a non-force numscript insufficient-funds test asserting INSUFFICIENT_FUNDS.
| if err != nil { | ||
| compileErr = &domain.ErrNumscriptParse{Details: err.Error()} | ||
| } else { | ||
| machine = numscriptlib.NewVm(prog) |
There was a problem hiding this comment.
[Major] Reused VM is execution-history-dependent state on the deterministic FSM path; determinism is unproven (invariant #2)
GetOrCompileVM builds one *numscriptlib.Vm per script and reuses it across every transaction. The Vm holds mutable register banks (stringsRegs/intsRegs/portionsRegs/monetariesRegs [256]…). On each run vm.Exec calls vm.runstate.Reset(...) which clears only the RunState (balances/sources/postings/currentAsset) — the register banks are never cleared.
So a freshly-booted node (fresh NewVm, zeroed regs) and a long-running node (reused VM carrying the previous transaction's register contents) run the same command against different starting register state. Output is identical only if numscript's codegen guarantees write-before-read for every register on every control-flow path — an unenforced, undocumented property of an experimental (feat/exp/vm) library. If it ever fails, two nodes diverge for the same applied index → cache/FSM divergence (invariants #1/#2). The burden of proving determinism is on this change, and there is no test that fresh-VM and reused-VM produce identical results.
Fix: until the library documents/guarantees register hygiene, build a fresh NewVm(prog) per apply (keep the compiled program + encoder memoized — those are immutable). Alternatively, add a differential determinism test (see the test-file comment).
| // optimization: two transactions run through the same producer/cache reuse the | ||
| // memoized *Vm, and numscript's runstate.Reset must clear the prior run's | ||
| // postings so the second result is not polluted by the first. | ||
| func TestNumscriptVMProducer_ReusesVMAcrossCalls(t *testing.T) { |
There was a problem hiding this comment.
[Medium] ReusesVMAcrossCalls does not actually guard the determinism claim it documents
The test runs the same script twice and asserts Len==1 + amount. That catches gross posting accumulation (a failed reset giving Len==2), but it never (a) compares a reused-VM result against a fresh-VM result, nor (b) runs a different script/vars through the memoized *Vm. So it cannot detect stale register-bank leakage between differing runs — which is exactly the determinism risk flagged on cache.go:189. TestNumscriptVMProducer_MatchesInterpreter also allocates a fresh cache per run(), so it doesn't exercise reuse either.
Fix: add a case that runs script A then a different script B (different postings/vars) through the same producer+cache and asserts B is independent of A; ideally also assert the reused-VM output equals a fresh-VM run for identical inputs.
|
|
||
| func TestProcessCreateTransaction_Numscript_SetTxMeta(t *testing.T) { | ||
| t.Parallel() | ||
| t.Skip("live numscript path now runs on the bytecode VM, which does not yet map set_tx_meta/set_account_meta output (numscript feat/exp/vm ExecVm TODO)") |
There was a problem hiding this comment.
[Medium] Five skipped tests leave the live-path metadata behavior — including the #322 security guard — untested
These t.Skips (712, 781, 828, 909, 998) remove the only coverage of numscript metadata on the create path. Two of them (RejectsEmptyMetadataKey, RejectsNullByteMetadataValue) guard the #322 fix (empty/NUL keys corrupting read-index entries). Because the VM now drops metadata entirely, domain.ValidateMetadataKey/ValidateMetadataString inside applyNumscriptResult are unreachable on the live path, and the only producer that still reaches them (numscriptPostingProducer) is exercised solely by MatchesInterpreter, whose script sets no metadata — so the #322 guard is now dead and untested. The metadata-drop behavior change itself is asserted by no running test (it lives only in the skip strings).
Fix: add a running test pinning the current "metadata dropped" behavior (so the regression is intentional and visible), and keep a test that still drives the validation guards via the interpreter producer so #322 isn't silently un-covered.
…er CRD Add a numscript-engine switch (interpreter | vm) instead of hardcoding the VM on the live path. Default is interpreter, so the metadata-complete tree-walker stays the production default and the VM is opt-in for benchmarking. - server: --numscript-engine flag (env NUMSCRIPT_ENGINE), parsed into cfg.NumscriptUseVM - thread the bool through NewMachine -> NewRequestProcessor -> Context; the producer selection in processCreateTransaction picks VM vs interpreter - operator: add spec.numscriptEngine to the Cluster CRD (+kubebuilder:validation:Enum=interpreter;vm), map it to NUMSCRIPT_ENGINE in envvars.go, regenerate CRD manifests - un-skip the 5 metadata unit tests (default interpreter maps set_*_meta again) - add TestProcessCreateTransaction_Numscript_VMEngine pinning flag routing and the VM's set_tx_meta drop; update all NewRequestProcessor/NewMachine callers - docs: document --numscript-engine in docs/ops/cli.md The engine is cluster-wide config and MUST match on every node: the FSM apply path must be deterministic and the two engines are not yet equivalent (VM drops set_tx_meta/set_account_meta output), so a mixed-engine cluster would diverge on metadata-setting scripts. Change it via a full cluster restart.
NumaryBot
left a comment
NumaryBot
left a comment
There was a problem hiding this comment.
NumaryBot posted 6 new inline findings.
Summary: #1597 (comment)
| // cryptographic collision resistance (the audit hash chain lives elsewhere), | ||
| // and 128 bits makes accidental collisions negligible. Lock-free and | ||
| // allocation-free. | ||
| func HashScript(script string) [16]byte { |
There was a problem hiding this comment.
🟠 [major] Non-cryptographic xxh3 hash used as sole cache key risks script collision
reported by NumaryBot, paul-nicolas
Both GetOrParse and GetOrCompileVM use an xxh3 hash as the sole cache lookup key. xxh3 is optimised for speed and is not collision-resistant. A hash collision between two distinct scripts causes the second to silently execute using the first script's compiled AST or VM program. Whether a collision is hit depends on cache warmth, making transaction semantics non-deterministic across nodes and restarts.
Suggestion: Use a collision-resistant key — either the full script text as the map key, or a SHA-256/SHA-512 hash — or verify the cached entry's original script text on every cache hit before returning the cached program.
NumaryBot
left a comment
NumaryBot
left a comment
There was a problem hiding this comment.
NumaryBot posted 6 new inline findings.
Summary: #1597 (comment)
| } else { | ||
| switch { | ||
| case isNumscript && ctx.NumscriptUseVM: | ||
| producer = &numscriptVMPostingProducer{cache: ctx.NumscriptCache, ledgerName: ledger} |
There was a problem hiding this comment.
🔴 [blocker] set_tx_meta / set_account_meta metadata silently dropped on VM path
reported by NumaryBot, paul-nicolas, codex
Every live Numscript transaction is now routed through numscriptVMPostingProducer. The underlying ExecVm wrapper returns only postings — ExecutionResult.Metadata and AccountsMetadata are always empty. applyNumscriptResult guards on len(...) > 0 and skips all metadata persistence and validation blocks entirely. A script calling set_tx_meta/set_account_meta therefore commits successfully with its postings but with no metadata, a silent data-loss regression. The ValidateMetadataKey/ValidateMetadataString guards added for issue #322 are also unreachable on this path.
Suggestion: Populate ExecutionResult.Metadata and AccountsMetadata from the VM execution result before returning, mirroring what the interpreter path does. Until ExecVm propagates metadata, either keep the VM off the live path or make the producer return an explicit error when the script contains metadata-setting instructions. Add test coverage asserting that metadata from set_tx_meta/set_account_meta is present in the committed transaction.
| if isNumscript { | ||
| producer = &numscriptPostingProducer{cache: ctx.NumscriptCache, ledgerName: ledger, assetCache: ctx.AssetCache} | ||
| } else { | ||
| switch { |
There was a problem hiding this comment.
🔴 [blocker] Engine selection is node-local and can violate FSM determinism in a mixed cluster
reported by NumaryBot, paul-nicolas
The numscript-engine flag is evaluated independently on each node and is not part of the replicated Raft order. In a partially-rolled-out or misconfigured cluster, different nodes may apply the same Raft index through different engines. Since the VM and interpreter already diverge on metadata-setting scripts, this can produce different ledger state on different nodes for the same committed log entry, silently violating FSM determinism.
Suggestion: Validate and replicate the engine selection as part of committed Raft configuration, or gate the feature flag so it is rejected at startup unless all nodes agree on the engine. At minimum, fail fast with a logged fatal or startup error if the flag differs from what the FSM state records as having been used previously.
| } | ||
| }() | ||
|
|
||
| result, execErr := numscriptlib.ExecVm(ctx, machine, vars, store) |
There was a problem hiding this comment.
🟠 [major] VM insufficient-funds errors not mapped to the INSUFFICIENT_FUNDS domain error
reported by NumaryBot, paul-nicolas
convertNumscriptError only matches numscriptlib.MissingFundsErr (the interpreter type). On the VM path, missing-funds conditions are raised as distinct VM-internal types not exported at the library root, so errors.As misses them and they fall through to ErrNumscriptRuntime (KindInternal). A routine 'not enough balance' outcome now surfaces as an internal/runtime error instead of the established INSUFFICIENT_FUNDS/KindPrecondition domain error, breaking existing client contracts and making it impossible to distinguish a balance shortfall from a genuine engine bug.
Suggestion: Extend the error-mapping logic to detect the VM-specific missing-funds error type and convert it to the same ErrInsufficientFunds domain error the interpreter path produces. Push the numscript library to export the VM error type, or match on its structured fields. Add a test that drives a non-force numscript script to an insufficient-funds outcome via the VM path and asserts the INSUFFICIENT_FUNDS error code.
| if err != nil { | ||
| compileErr = &domain.ErrNumscriptParse{Details: err.Error()} | ||
| } else { | ||
| machine = numscriptlib.NewVm(prog) |
There was a problem hiding this comment.
🟠 [major] Reused VM instance carries uncleared register state across transactions
reported by NumaryBot, paul-nicolas
GetOrCompileVM builds a single *numscriptlib.Vm per script and reuses it across every transaction. vm.Exec calls vm.runstate.Reset(...) which clears only the RunState; the underlying register banks (stringsRegs, intsRegs, portionsRegs, monetariesRegs) are never cleared. A freshly-booted node (zeroed registers) and a long-running node (registers contain previous-transaction state) therefore execute the same instruction stream against different starting register state. Output is identical only if numscript's codegen guarantees write-before-read for every register — an unenforced, undocumented property of an experimental library. If that guarantee fails, two nodes diverge for the same applied Raft index.
Suggestion: Until the library explicitly documents and guarantees that register banks are safe to reuse across invocations, allocate a fresh *Vm per transaction (or call the appropriate reset API if one exists). Add a test that runs a fresh VM and a reused VM with identical inputs and asserts identical outputs across varied scripts.
| // Numscript execution engine. Only "vm" enables the bytecode VM; any other | ||
| // value (including the default "interpreter") uses the tree-walking | ||
| // interpreter. Must match on every node (FSM determinism). | ||
| cfg.NumscriptUseVM = getString("numscript-engine", "interpreter") == "vm" |
There was a problem hiding this comment.
🟠 [major] Invalid numscript-engine values silently fall back to interpreter
When --numscript-engine / NUMSCRIPT_ENGINE is set to an unrecognised value (e.g. VM, vmm), the code silently falls back to the interpreter instead of failing startup. Because the selected engine affects the FSM apply path, a misconfigured node can diverge from correctly-configured peers, creating an avoidable mixed-engine cluster.
Suggestion: Validate the flag value as exactly interpreter or vm at startup and return a configuration error otherwise, preventing silent misconfiguration.
| // optimization: two transactions run through the same producer/cache reuse the | ||
| // memoized *Vm, and numscript's runstate.Reset must clear the prior run's | ||
| // postings so the second result is not polluted by the first. | ||
| func TestNumscriptVMProducer_ReusesVMAcrossCalls(t *testing.T) { |
There was a problem hiding this comment.
🟡 [minor] ReusesVMAcrossCalls does not guard the claimed determinism property
The test runs the same script twice and asserts Len==1 plus the amount. This catches gross posting accumulation but does not compare a reused-VM result against a fresh-VM result, nor does it run a different script through the memoized *Vm. It therefore cannot detect stale register-bank leakage between differing runs, which is the actual determinism risk.
Suggestion: Add a case that runs script A then a different script B through the same producer+cache and asserts B is independent of A. Ideally also assert that the reused-VM output equals a fresh-VM run for identical inputs.
3 participants
Context
Integrates the numscript compiler + register VM (formancehq/numscript#167, branch
feat/exp/vm) into the ledger and routes the live create-transaction path through the VM instead of the tree-walking interpreter, so we can benchmark VM vs interpreter under the perf harness.What changed
github.com/formancehq/numscript→feat/exp/vm(v0.0.25-…-bf5bbb96), which exposesCompile/NewVm/ExecVm/VMStore. The bump is a breaking API change on the interpreter side too, so the existing path had to be adapted.StoreAPI —BalanceQuery/Balances/MetadataQuery/AccountsMetadataare now row slices carryingcolor/scope;ExecutionResult.AccountsMetadatais a typedSetAccountMetadataRowlist. Both the store adapter and the dependency-discovery store were ported.numscriptVMStoreAdapterimplementsvm.Store(lazy per-keyGetBalance/GetMetadataover the coverage-gatedScope).numscriptVMPostingProducercompiles → encodes vars →NewVm→ExecVm.NumscriptCache.GetOrCompilememoizes the compiled bytecode (fair steady-state comparison, mirroring the memoized parse).SafeExecVMwrapsExecVmwith recover + error mapping (mirror ofSafeRun).applyNumscriptResult(postings→volumes) andnumscriptBalance/numscriptAccountMetadataare shared by both producers.Value.String()to quote strings;numscriptMetaValueToStringunwraps via numscript's tagged-JSON form so stored metadata stays verbatim.processor_transaction.gonow selects the VM producer for all numscript. The interpreter producer is kept (parity test / comparison).Known limitation
ExecVmdoes not yet mapset_tx_meta/set_account_metaoutput (numscript TODO). Since the VM is now on the live path, numscript that sets metadata produces none. Plain sends (e.g. theworld-to-bankperf script) are unaffected. Consequently 5 metadata-asserting unit tests aret.Skip-ed with an explicit reason. Other caveats: VM missing-funds maps toErrNumscriptRuntime(notErrInsufficientFunds);GetBalancecoloris ignored (no ledger equivalent).Verification
go build ./...,go vet ./...— cleangolangci-lint runon touched packages — 0 issuesgo test ./internal/domain/processing/...— green (incl. VM sanity + VM/interpreter parity on an allotment split)