feat(gnovm): split mempackage storage into prod and test blobs#5891
Draft
jaekwon wants to merge 9 commits into
Draft
feat(gnovm): split mempackage storage into prod and test blobs#5891jaekwon wants to merge 9 commits into
jaekwon wants to merge 9 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>
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
|
Member
|
[bot] Review finding: [P2] The current head is failing |
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
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 you deploy a Gno package, the chain stores all of its
.gnofilestogether in one blob — the real code and the test files (
_test.gno,_filetest.gno).The problem: when another package imports yours, the chain has to read and
type-check the imported package's stored files — including its test files,
which an importer never actually uses. So importing a package that ships large
test files costs more gas than it should, and that cost is paid again by every
importer.
This PR stores each package as two blobs instead of one:
pkg:<path>Importing a package now reads only the production blob,
files no longer inflate anyone's import gas. Tools that need the whole package
(explorers,
gnotooling) still get everything — the ttogether on a full read.
Also included
A few small, related correctness fixes that come with the split:
.gnofiles (e.g. a packagecontaining only
_test.gnofiles). With the split thblob, which would make them behave differently after a node restart. They
have no on-chain use, so they're now rejected at depl
from a previous version can't linger.
#.Heads up: this changes the app hash
Because stored bytes change, the genesis app hash changes. Behavior is
unchanged — only the storage encoding shifted. Shipping
relaunch (genesis replay), the same as any storage-format change. The pinned
app-hash and gas test values are updated accordingly.
Testing
store_test.gocases: the two-blob split round-tredeploy clears stale blobs, and path listing de-duplicates the sibling key.
addpkg_import_testdep_gas.txtar: proves importitest files costs the same gas as importing one without (the whole point of
the split), plus a matching Go-level equality test.
gno.land/pkg/sdk/vm, the txtar integration suite,and the gnovm filetests.
Files changed
store.go(the split),store_test.go,machine.go(skip rebuilding nodes for production-less packages),
debugger.go(read the sibling key; fixes a possible nil dereference)
keeper.go(clear blobs on private redeploy; rejectproduction-less packages),
keeper_test.goaddpkg_import_testdep_gas.txtar; re-pinnedgnokey_gasfee.txtar,restart_gas.txtar, and the app-hash test