fix(gnovm): make the consensus type-check verdict and error deterministic#5893
Draft
jaekwon wants to merge 14 commits into
Draft
fix(gnovm): make the consensus type-check verdict and error deterministic#5893jaekwon wants to merge 14 commits into
jaekwon wants to merge 14 commits into
Conversation
Store MP*All packages as two IAVL blobs instead of one: production files (non-.gno plus non-test .gno) under the canonical pkg:<path> key, and the test/filetest complement under a pkg:<path>#allbutprod sibling. GetMemPackage returns prod-only -- the import/preprocess/typecheck hot path -- so its IAVL read and amino-decode gas is charged on production bytes only, never on the test sources that importers discard. A new GetMemPackageAll merges both blobs for query paths (vm/qfile, debugger) that must still see test files. The complement blob carries the original MP*All type as an inert sentinel: it is written and read only by the store and never enters MemPackageType dispatch, so no new MemPackageType is introduced. A package with no production .gno files (e.g. xxx_test-only) has no prod blob; all its files (including non-.gno such as gnomod.toml) fold into the GetMemPackage result on restart. GetMemFile/QueryFile listing and debugger.isMemPackage consult the full package so test files stay retrievable. Design decisions (new chain, so no migration concerns): - No boot-time format guard (no pre-existing old-format data to detect). - Stdlib boot type-check now sees prod-only; stdlib tests still run via the test machinery (TestStdlibs green). Accepted. - QueryDoc stays on prod (the documentable/importable surface). Goldens: apphash_crossrealm38 updated (stored byte-set changed); addpkg gas +17 (prod blob Type "MPUserAll" -> "MPUserProd" is +1 amino byte: encode 3 + iavl-write 14). Co-Authored-By: Claude Opus 4.8 (1M context) <noreply@anthropic.com>
AddMemPackage stores an MP*All package as a prod blob (pkg:<path>) plus a #allbutprod sibling, writing each conditionally. Unlike the prior single-key layout, a re-add no longer fully replaces state across both keys: redeploying a private package (the only re-add VMKeeper.AddPackage permits) with a changed file set left a stale sibling -- so qfile/GetMemPackageAll served deleted test files -- or, if redeployed prod-less, a stale prod blob served by GetMemPackage. Add Store.DeleteMemPackage(path) (removes both keys) and call it from VMKeeper.AddPackage before re-running a private package, restoring the clean-overwrite invariant. The cost lands only on the rare redeploy path; fresh adds and the gas/apphash goldens are unchanged. Co-Authored-By: Claude Opus 4.8 (1M context) <noreply@anthropic.com>
- FindPathsByPrefix: de-dup the prod key and its #allbutprod sibling (strip the suffix) instead of skipping all "#" keys, so an empty-prod package (sibling only) is still listed in vm/qpaths exactly once. Packages with a prod blob are unaffected (the path was already listed once). - QueryDoc: use GetMemPackageAll for parity with QueryFile, so doc generation can see test files (future test-derived examples). - debugger fileContent: nil-check GetMemFile before deref; a name absent from an in-store package previously nil-panicked instead of falling through to disk. Co-Authored-By: Claude Opus 4.8 (1M context) <noreply@anthropic.com>
…first Harden against a future caller: an MP*All package is stored as a prod blob plus a #allbutprod sibling with conditional writes, so re-adding at an existing path without a preceding DeleteMemPackage can leave a stale blob. The keeper's AddPackage already does this on private redeploy; document the contract. Co-Authored-By: Claude Opus 4.8 (1M context) <noreply@anthropic.com>
…plit Add an integration fixture proving a dependency's _test.gno bytes do not inflate the gas of importing it. depa and depb have identical production code; depb also carries a large lib_test.gno. usea imports depa and useb imports depb (otherwise byte-identical), and both addpkg to the SAME gas (3172401), because type-checking an importer decodes only the dependency's prod blob -- depb's test file lives in the pkg:<path>#allbutprod sibling and is never decoded. On master (single-blob layout) useb costs 3212984 vs usea's 3172364: importing depb decodes its _test.gno too, +40620 gas. The split removes that import penalty; the equal GAS USED assertions guard against regressing it. Co-Authored-By: Claude Opus 4.8 (1M context) <noreply@anthropic.com>
…ckage A package whose only .gno files are _test.gno/_filetest.gno files passed every AddPackage check and deployed. Under the mempackage split it stores no prod blob (only the #allbutprod sibling), so on restart PreprocessAllFilesAndSaveBlockNodes skips it and rebuilds no PackageNode, while a non-restarted node still holds the deploy-time node in RAM. A MsgCall into such a path then fails on both, but with different gas consumed -- and since consumed gas feeds the shared block gas meter, a near-full block can push a later tx into out-of-gas on one node class and not the other, diverging the results hash. Reject such packages up front (a test-only package has no on-chain use), using the store split's own prod predicate so the check cannot drift from what gets stored. The machine-side nil skip stays as a defensive path for non-chain stores. Co-Authored-By: Claude Fable 5 <noreply@anthropic.com>
…t tests Review follow-ups from the mempackage-split branch review: - FindPathsByPrefix: a prefix containing '#' (impossible in a valid package path, but reachable from raw query input via vm/qpaths) could range over a #allbutprod sibling key and yield a path that does not carry the requested prefix. Re-check the trimmed key against the requested range in key space (path space would wrongly drop stdlib matches, whose "_/" key marker is stripped on decode). - Document on the Store interface that IterMemPackage yields prod-only mempackages, and nil for a package with no production .gno files. - Add a structural gas-equality test: importing a dep that carries a _test.gno costs exactly the same gas as importing one without. The addpkg_import_testdep_gas.txtar pins two equal literals, which a bulk gas re-pin could silently de-equalize; the Go test asserts the equality itself. Co-Authored-By: Claude Fable 5 <noreply@anthropic.com>
Rebasing the split onto current master shifts the genesis app hash (stdlib/genesis set moved) and nudges the hello deploy gas +20 (storage encoding). Re-derived against the split-only state. Co-Authored-By: Claude Fable 5 <noreply@anthropic.com>
MsgAddPackage and MsgRun run two native passes over the submitted package -- go/types type-checking (all .gno files) and gnovm preprocessing (the prod subset) -- with zero gas charged for either: unmetered validator CPU, linear in the submitted source bytes. Add a governable vm param PreprocessGasPerByte (proto field 14, default 1250, measured ~1250 gas/byte on realistic example packages at 1 gas == 1ns on the reference machine; validated positive, capped at 100_000) and charge it per .gno source byte (prod + _test + _filetest) in AddPackage and Run, up front, so an oversized package is rejected by the gas meter before the native work runs. The byte base includes test files because the type-check pass checks them too; charging prod-only bytes would leave test-heavy packages unmetered. The non-zero default serializes into genesis vm params state, so the pinned genesis app hash moves (apphash_crossrealm38_test.go). Txtar fixtures take -gas-wanted bumps; exact gas pins are re-derived (addpkg_import_testdep_gas, gnokey_gasfee, restart_gas, maketx_run). The charge prices the typical case and is deliberately linear; it is not a worst-case bound for adversarial type-system inputs, and it does not yet price transitively re-type-checked dependency source (a planned follow-up adds a per-dep-byte term). Co-Authored-By: Claude Fable 5 <noreply@anthropic.com>
A vm params blob written before the field existed has no vm:p:preprocess_gas_per_byte key, so GetStruct left the field zero. That had two consequences on such legacy state: the type-check/ preprocess charge was silently disabled (0 gas per byte), and -- because WillSetParam re-validates the whole struct read via GetParams -- the first governance update of ANY other vm param panicked with "PreprocessGasPerByte must be positive" until the new key was set first. Zero is otherwise unrepresentable (Validate rejects it on every write path: SetParams, WillSetParam, ValidateGenesis), so a decoded zero can only mean pre-field state. Default it to preprocessGasPerByteDefault in GetParams: the charge stays active on legacy chains and unrelated param updates validate cleanly. State self-heals on the next SetParams or genesis export. NB the sibling IterNextCostFlat has the same latent trap from its own rollout (pre-field-13 state); left as-is since changing its legacy behavior (0 -> 1000 per iter step) is out of scope here. Co-Authored-By: Claude Fable 5 <noreply@anthropic.com>
Two review follow-ups on the mempackage-split branch: - Genesis/GetParams asymmetry: GetParams defaults a missing (zero) PreprocessGasPerByte to 1250 for legacy running state, but ValidateGenesis called Params.Validate() directly, which rejects zero. A relaunch genesis exported by a binary predating the field omits it (decodes as zero), so InitChain would panic — despite the branch's migration model being a genesis relaunch. Extract Params.applyLegacyDefaults and apply it in GetParams, ValidateGenesis (covering the standalone gnoland genesis-validate path too), and InitGenesis (so the stored value is the default, not zero). An explicitly out-of-range genesis value still fails. - Drop an allocation in the prod-less AddPackage guard: replace MPFProd.FilterMemPackage(memPkg).IsEmpty() (which allocated a full filtered copy) with hasProdGnoFile, scanning via MPFProd's own per-file FilterGno predicate — same source of truth as the storage split, no drift, no copy. Co-Authored-By: Claude Fable 5 <noreply@anthropic.com>
The consensus type-check (gotypecheck.go) built its types.Config with no GoVersion, so go/types gated syntax at whatever Go version the validator binary was compiled with. A package using a version-straddling construct (e.g. `for range 10`, go1.22) would then be accepted by a node built with a newer Go and rejected by an older one -- and the error text differs too -- diverging the tx result and app hash across a heterogeneous validator set. No attacker needed. Pin GoVersion = "go1.18". This is a syntax-acceptance floor only, NOT a statement of Gno's runtime semantics: Gno matches no single Go version -- no generics (like <go1.18) yet go1.22 per-iteration loopvars, which live in the interpreter and are unaffected by this value (verified: nothing in the interpreter reads a Go version). go1.18 is the minimum the injected .gnobuiltins shim needs (it uses any/type params) and it rejects features Gno cannot run (min/max, range-over-int/func) at the type-check stage. Empirically the entire stdlib+examples corpus (187 pkgs) type-checks identically at go1.18 vs unpinned, so no stored package, app hash, or gas pin moves. range9/range12 filetests now report the rejection at the type-check stage (TypeCheckError) in addition to preprocess (Error). Co-Authored-By: Claude Fable 5 <noreply@anthropic.com>
TypeCheckError carried the raw go/types (and go/parser) diagnostic strings in an amino-encoded Errors field. ABCIResult.Error is merkle-hashed into the block's LastResultsHash, and those diagnostics are not a stable API: wording, count, and order vary across Go toolchains. Two validators that both (correctly) reject the same package could therefore commit different result hashes -- the second axis of the type-check consensus fork, alongside the GoVersion pin. Make TypeCheckError an empty sentinel (like every other vm abci error) and have ErrTypeCheck wrap the full messages around it as the error's msg trace. Only the sentinel is amino-encoded into the hashed result; the full diagnostics still reach the user through the unhashed Result.Log (%#v msg trace), which gnokey already prints, and gno.TypeCheckMemPackage still returns complete errors, so filetest TypeCheckError directives and tooling are unaffected. Hashing raw .Error() strings into the results hash is a standalone determinism liability; this removes the one live instance. Co-Authored-By: Claude Fable 5 <noreply@anthropic.com>
github-actions
Bot
added
📦 🤖 gnovm
Collaborator
🛠 PR Checks SummaryAll Automated Checks passed. ✅ Manual Checks (for Reviewers):
Read More🤖 This bot helps streamline PR reviews by verifying automated checks and providing guidance for contributors and reviewers. ✅ Automated Checks (for Contributors):No automated checks match this pull request. ☑️ Contributor Actions:
☑️ Reviewer Actions:
📚 Resources:Debug
|
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
3 participants
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.
What this does
When the chain processes a deploy or run, it type-checks the submitted code, and
the outcome affects consensus — so every node must reach the same outcome.
Right now two nodes can disagree in two ways, and either one forks the chain with
no attacker needed:
Whether borderline code is accepted at all. The type checker was never
told which Go language version to enforce, so it accepted or rejected
version-sensitive code based on whichever Go version each validator's binary
happened to be built with. Example: a loop like
for range 10(a newer-Gofeature) could be accepted on one node and rejected on another.
The exact wording of a rejection. When a type-check fails, the raw error
text from Go's type checker was put into the transaction result that gets
hashed into the block. That wording (and the number/order of messages) drifts
between Go versions, so two nodes that both correctly reject the same
package could still produce different block hashes.
This PR closes both:
Pin the accepted Go language version (to
go1.18), so the accept/rejectdecision is identical on every node regardless of how the binary was built.
It's a floor for accepting syntax only — it does not change how code runs
(for example, loop-variable behavior is decided by the interpreter and is
unaffected). The whole existing package set type-checks the same under it.
Replace the raw error text in the hashed result with a fixed placeholder.
The full, detailed type-check errors still reach the user through the normal
(unhashed) transaction log, and test tooling still sees them because it reads
them from a different path. Only the part that goes into the block hash is made
constant.
Heads up: consensus behavior, but no genesis app-hash change
Unlike PR1 and PR2, this PR does not change the genesis app hash. But it does
change type-check behavior and the results of rejected transactions, so it must
roll out uniformly — it's part of the same coordinated release as PR1/PR2.
Testing
for range 10) is rejected no matterwhat Go version the binary was built with, while an ordinary loop that closes
over its loop variable stays accepted.
results, while the full error detail is still present in the unhashed form.
range9,range12) updated to show the rejection happening atthe type-check stage.
err_metadata.txtarupdated: the hashed error is the placeholder, and the fulldetail appears in the log.
gno.land/pkg/sdk/vm, the txtar integration suite,and the gnovm filetests.
Files changed
gotypecheck.go(pin the Go version),gotypecheck_test.go, and therange9.gno/range12.gnofiletestserrors.go(fixed placeholder instead ofraw text),
errors_test.go,pb3_gen.go+vm.proto(drop the now-unusederror-strings field),
err_metadata.txtarNotes: