feat(numscript): execute transactions on the bytecode VM #1597
There are no files selected for viewing
| Original file line number | Diff line number | Diff line change |
|---|---|---|
|
|
@@ -5,7 +5,7 @@ import ( | |
| "context" | ||
| "sync" | ||
|
|
||
| "github.com/zeebo/xxh3" | ||
| "go.opentelemetry.io/otel/metric" | ||
|
|
||
| numscriptlib "github.com/formancehq/numscript" | ||
|
|
@@ -20,7 +20,7 @@ import ( | |
| // write-locking on the hot path. | ||
| type NumscriptCache struct { | ||
| mu sync.RWMutex | ||
| cache map[[16]byte]*list.Element | ||
| order *list.List | ||
| maxSize int | ||
|
|
||
|
|
@@ -30,14 +30,27 @@ type NumscriptCache struct { | |
|
|
||
| // lruEntry holds the cache key and value for an LRU list element. | ||
| type lruEntry struct { | ||
| hash [16]byte | ||
| script parsedScript | ||
| } | ||
|
|
||
| // parsedScript wraps a parsed Numscript program with any parsing errors, plus | ||
| // lazily-populated bytecode-compilation artifacts (the VM execution path). The | ||
| // vars encoder and the machine are only produced on the first GetOrCompileVM | ||
| // call for a given script and memoized for reuse, mirroring how the parsed AST | ||
| // is memoized for the interpreter path. | ||
| type parsedScript struct { | ||
| program numscriptlib.ParseResult | ||
| err domain.Describable | ||
|
|
||
| compiledDone bool | ||
| encoder numscriptlib.VarsEncoder | ||
| compiledErr domain.Describable | ||
| // vm is a reusable machine bound to the compiled program (nil when | ||
| // compilation failed). It holds mutable per-run register banks + runstate | ||
| // that numscriptlib.ExecVm resets on every call, so it may be reused across | ||
| // executions — but MUST NOT be executed concurrently (see GetOrCompileVM). | ||
| vm *numscriptlib.Vm | ||
| } | ||
|
|
||
| // NewNumscriptCache creates a new NumscriptCache with the given maximum size. | ||
|
|
@@ -48,23 +61,19 @@ func NewNumscriptCache(maxSize int) *NumscriptCache { | |
| } | ||
|
|
||
| return &NumscriptCache{ | ||
| cache: make(map[[16]byte]*list.Element, maxSize), | ||
| order: list.New(), | ||
| maxSize: maxSize, | ||
| } | ||
| } | ||
|
|
||
| // HashScript computes the xxh3 128-bit hash of the script content, used as the | ||
| // cache key. xxh3 is a fast non-cryptographic hash; the cache key needs no | ||
| // 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 { | ||
|
NumaryBot marked this conversation as resolved.
NumaryBot marked this conversation as resolved.
Contributor
There was a problem hiding this comment. 🟠 [major] Non-cryptographic xxh3 hash used as sole cache key risks script collision Both 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. |
||
| return xxh3.HashString128(script).Bytes() | ||
| } | ||
|
|
||
| // GetOrParse retrieves a parsed script from the cache or parses it if not found. | ||
|
|
@@ -131,6 +140,75 @@ func (c *NumscriptCache) GetOrParse(script string) (numscriptlib.ParseResult, do | |
| return parsed, parseErr | ||
| } | ||
|
|
||
| // GetOrCompileVM returns the vars encoder and a reusable bytecode VM for a | ||
| // script, compiling the program and building the machine once and memoizing | ||
| // both. Compilation subsumes parsing, so a parse error is surfaced here too. | ||
| // The compile + VM build happen outside the write lock; the result is stored on | ||
| // the shared cache entry so subsequent runs of the same script skip both. | ||
| // | ||
| // The returned *Vm is SHARED and holds mutable per-run state (register banks + | ||
| // runstate); numscriptlib.ExecVm resets it on every call, so it may be reused | ||
| // across executions but MUST NOT be executed concurrently. In the ledger this | ||
| // is safe because numscript VM execution only happens on the single-threaded | ||
| // FSM apply path — admission dependency discovery uses the interpreter, not the | ||
| // VM. Reuse avoids reallocating the machine's register banks | ||
| // ([256]big.Int/big.Rat/monetary/string — the dominant per-transaction | ||
| // allocation) on every call. | ||
| func (c *NumscriptCache) GetOrCompileVM(script string) (numscriptlib.VarsEncoder, *numscriptlib.Vm, domain.Describable) { | ||
| // Ensure the script is parsed and cached first; a parse failure is terminal. | ||
| parsed, parseErr := c.GetOrParse(script) | ||
| if parseErr != nil { | ||
| return numscriptlib.VarsEncoder{}, nil, parseErr | ||
| } | ||
|
|
||
| hash := HashScript(script) | ||
|
|
||
| // Fast path: encoder + machine already memoized on the entry. | ||
| c.mu.RLock() | ||
| if elem, ok := c.cache[hash]; ok { | ||
| if entry, _ := elem.Value.(*lruEntry); entry.script.compiledDone { | ||
| enc, machine, cErr := entry.script.encoder, entry.script.vm, entry.script.compiledErr | ||
| c.mu.RUnlock() | ||
|
|
||
| return enc, machine, cErr | ||
| } | ||
| } | ||
| c.mu.RUnlock() | ||
|
|
||
| // Compile + build the machine outside the lock — the expensive operations. | ||
| enc, prog, err := parsed.Compile() | ||
|
|
||
| var ( | ||
| compileErr domain.Describable | ||
| machine *numscriptlib.Vm | ||
| ) | ||
|
|
||
| if err != nil { | ||
| compileErr = &domain.ErrNumscriptParse{Details: err.Error()} | ||
| } else { | ||
| machine = numscriptlib.NewVm(prog) | ||
|
Contributor
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)
So a freshly-booted node (fresh Fix: until the library documents/guarantees register hygiene, build a fresh
NumaryBot marked this conversation as resolved.
Contributor
There was a problem hiding this comment. 🟠 [major] Reused VM instance carries uncleared register state across transactions
Suggestion: Until the library explicitly documents and guarantees that register banks are safe to reuse across invocations, allocate a fresh |
||
| } | ||
|
|
||
| // Store the artifacts on the shared entry (if it still exists). | ||
| c.mu.Lock() | ||
| defer c.mu.Unlock() | ||
|
|
||
| if elem, ok := c.cache[hash]; ok { | ||
| entry, _ := elem.Value.(*lruEntry) | ||
| if !entry.script.compiledDone { | ||
| entry.script.encoder = enc | ||
| entry.script.compiledErr = compileErr | ||
| entry.script.vm = machine | ||
| entry.script.compiledDone = true | ||
| } | ||
|
|
||
| return entry.script.encoder, entry.script.vm, entry.script.compiledErr | ||
| } | ||
|
|
||
| // The entry was evicted between parse and compile; return the fresh result. | ||
| return enc, machine, compileErr | ||
| } | ||
|
|
||
| // InitCacheMetrics initializes the cache metrics on the NumscriptCache. | ||
| func (c *NumscriptCache) InitCacheMetrics(m metric.Meter) error { | ||
| size, err := m.Int64Gauge( | ||
|
|
||
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
🟠 [major] Invalid numscript-engine values silently fall back to interpreter
When
--numscript-engine/NUMSCRIPT_ENGINEis 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
interpreterorvmat startup and return a configuration error otherwise, preventing silent misconfiguration.