feat: decode composite (vec/map) call arguments; raise recursion limit - #52
Open
RaoulSchaffranek wants to merge 6 commits into
Open
feat: decode composite (vec/map) call arguments; raise recursion limit#52RaoulSchaffranek wants to merge 6 commits into
RaoulSchaffranek wants to merge 6 commits into
Conversation
komet-node rejected any Vec/Map — and thus any user enum/struct/tuple —
contract call argument at admission: scval_to_json raised NotImplementedError
on SCV_VEC/SCV_MAP and #decodeArg had only scalar rules, so the transaction
never ran. Add recursive vec/map support on both sides so composite arguments
are decoded and executed:
- scval.py: scval_to_json emits {"type":"vec","value":[...]} and
{"type":"map","value":[{"key":..,"val":..},..]}, recursing element-wise.
- node.md: #decodeArg vec/map rules — ScVec(#decodeArgList(...)) and
ScMap(#decodeMapEntries(...)). Enums, structs, and tuples all reduce to
vec/map at the XDR level, so these cover every composite call argument.
- args.wat / test_server.py: a call carrying flat, nested (Vec<(enum,i128)>
with an Address variant and a negative i128), map, and map-in-vec arguments
reaches SUCCESS and its trace's callContract frame round-trips the exact
SCVals sent.
Also raise the Python recursion limit — large real contracts produce a KORE
world-state term far deeper than CPython's default 1000, which surfaced as a
RecursionError mid-request during pyk parsing / config traversal:
- __init__.py: sys.setrecursionlimit(10**7), matching the rest of the K
tooling (pyk sets 10**7; komet sets its own limit at import).
- server.py: run the blocking serve loop on a worker thread with a 512 MB
stack, so a deep term raises a catchable RecursionError instead of
overflowing the 8 MB default stack into a SIGSEGV.
- test_scval.py: unit tests pinning the vec/map JSON shape (order-sensitive,
since #decodeArg matches on member order) and that a deeply nested value
encodes without hitting the recursion limit.
…ract Reassembling the trace array in the semantics recursively copied the whole remaining tail once per line — O(n^2) time and memory that OOM-killed the interpreter on multi-hundred-MB traces. Serve traceTransaction directly from traces/trace_<hash>.jsonl in one linear pass instead, bypassing the interpreter. Each served record is stamped with an executingContract field — the contract executing at that record, reconstructed from the callContract/endWasm call-boundary markers — so a consumer can map each pos against the right contract binary. The field is named executingContract, not contract, to avoid clobbering the contractData record's own documented contract field.
#traceLedger writes the ledger scalars and every account's balance as the trace's first line, before any step runs, so a debugger can seed its view of chain state and replay the per-operation events on top of it instead of seeing only what a contract happened to touch. Balances are gathered one per rewrite step by #collectAccounts, since <accounts> is a cell collection that no function can take as an argument. Also records the executing module's globals on each instruction record. Co-Authored-By: Claude Opus 5 (1M context) <noreply@anthropic.com>
`generateLedgerTrace` and `AccountBalances2JSONs` lived in komet's `tracing.md` but had no caller there -- `collectAccounts-done` below is the only one. Keeping them upstream meant they were untested and undocumented where they lived, and made this module depend on a komet newer than the v0.1.86 it pins. `imports JSON-UTILS` is now explicit, for `Address2JSON`. It was previously reachable only through KASMER's import chain into `TRACING`, which sits behind komet's `k-tracing` md selector -- so relying on it would have made this module silently require a tracing-enabled komet build. Also corrects the rationale in the surrounding prose, which had it backwards: `<ledgerSequenceNumber>`, `<ledgerTimestamp>` and `<accounts>` are all declared in komet's `configuration.md`, not here. What belongs to komet-node is the record, not the cells. The same passage referred to komet's `#collectGlobals`, which no longer exists; it now points at `moduleGlobals` and notes that reading cells as function context would remove these rewrite steps here too. Co-Authored-By: Claude Opus 5 (1M context) <noreply@anthropic.com>
…format
komet v0.1.87 ("Clean up the trace format", #122) replaced the `pos`/`instr`
tagging of trace records with a top-level `kind` field and flattened each
record's own fields, and v0.1.88 (#126) added `globals` to every instruction
record. Bumping the pin alone would have broken the serve path silently, so
this migrates komet-node with it.
- pyproject.toml / uv.lock: v0.1.86 -> v0.1.88. pykwasm is unchanged (v0.1.155).
- server.py: `_annotate_trace_lines` keyed its call-boundary stack on
`instr[0]`, which no longer exists on `callContract`/`endWasm` records — it
would have tagged every served record `executingContract: null` without
raising. It now dispatches on `kind`, and the cheap substring prefilter
matches `"endWasm"` exactly rather than the `"endWasm` prefix: the trap
spelling it was guarding against (`endWasm-error`) is a K rule name, never a
record kind. komet emits one `endWasm` record for both outcomes, telling them
apart by `success`, so the pop keys on `kind` alone.
- node.md: `generateLedgerTrace` emits `{"kind": "ledger", ...}`, dropping the
`pos`/`instr` pair, so the baseline record komet-node contributes matches the
format of the komet records around it.
- README.md / docs: the trace format, record-by-record. The README trace
section also gains the `ledger` record and the `executingContract` tag, both
of which it predated.
- test_server.py: trace assertions and synthetic record fixtures move to
`kind`. Also fixes two lint errors that already failed `make check` on this
branch (an unused local and a quote-escaping warning).
Verified with `make check`, `make test-unit` (9 passed) and, against a kdist
rebuild of the v0.1.88 semantics, `make test-integration` (101 passed).
Co-Authored-By: Claude Opus 5 (1M context) <noreply@anthropic.com>
traceTransaction stamped every served record with an executingContract field naming the contract running at it, reconstructed by walking the trace's callContract/endWasm boundaries. That was the wrong layer. komet-node is a thin stateful wrapper around komet, and the field carried no information the trace did not already have: a callContract names its callee and an endWasm closes it, so a consumer folds it out of records it is walking anyway. Doing it here cost three things. It duplicated ~87 bytes of derivable data per record on traces that run to hundreds of megabytes — added by the very code path that exists to keep memory proportional to the trace. It made the served array differ from the stored file, so the RPC and the on-disk format disagreed about what a record is. And it coupled komet-node to komet's record semantics: the v0.1.87 format change broke exactly this function and nothing else, because it is the only place here that looks inside a record. The serve path is now a linear join of the file's lines with no JSON parsing at all. The debug adapter derives the executing contract itself (simbolik-komet, src/komet/executingContract.ts), where it also has the call-frame stack it needs for its own Ledger view. Co-Authored-By: Claude Opus 5 (1M context) <noreply@anthropic.com>
This file contains hidden or bidirectional Unicode text that may be interpreted or compiled differently than what appears below. To review, open the file in an editor that reveals hidden Unicode characters.
Learn more about bidirectional Unicode characters
Sign up for free
to join this conversation on GitHub.
Already have an account?
Sign in to comment
1 participant
Add this suggestion to a batch that can be applied as a single commit.This suggestion is invalid because no changes were made to the code.Suggestions cannot be applied while the pull request is closed.Suggestions cannot be applied while viewing a subset of changes.Only one suggestion per line can be applied in a batch.Add this suggestion to a batch that can be applied as a single commit.Applying suggestions on deleted lines is not supported.You must change the existing code in this line in order to create a valid suggestion.Outdated suggestions cannot be applied.This suggestion has been applied or marked resolved.Suggestions cannot be applied from pending reviews.Suggestions cannot be applied on multi-line comments.Suggestions cannot be applied while the pull request is queued to merge.Suggestion cannot be applied right now. Please check back later.
Summary
Four fixes from one debugging session against a real Soroban contract.
Composite call arguments.
Vec/Maparguments — and so any user enum, struct, or tuple — were rejected astxMALFORMEDbefore the transaction ran. Both the Python encoder and the K decoder now recurse, covering every composite argument rather than a list of special cases.traceTransactionno longer OOM-kills the interpreter. Traces were reassembled inside the semantics, copying the whole remainder once per line. The server now joins the stored file's lines in one pass with no JSON parsing and returns them verbatim; anything derivable is left to the consumer.Every trace opens with a ledger baseline. Sequence, timestamp, and all account balances as the transaction found them, so a debugger can show chain state it did not watch being written.
Large contracts no longer crash mid-request. Deep world-state terms blew CPython's recursion limit; it is raised to match the rest of the K tooling, with a large serve-thread stack so a deep term raises rather than segfaults.
komet v0.1.86 → v0.1.88 — breaking trace format change
Needed for the globals the debugger uses to resolve local variables. v0.1.87 also reorganised trace records: each now names itself with a
kindfield instead of encoding its type insidepos/instr, with type-specific fields spelled out. So{"instr": ["contractData", "put", "temporary"], …}becomes{"kind": "contractData", "operation": "put", "durability": "temporary", …}.komet-node is migrated to match, including the
ledgerrecord it emits itself.Downstream
The VS Code debugger must land after this, not before: it is migrated on its own branch and now requires v0.1.87 or newer, so either half released alone will not run. That branch also flips its composite-argument test from asserting komet-node cannot encode a
Vecto tracing one end to end.