fix(gnovm): reclaim stored key object on delete() of object-keyed map#5882
Open
omarsy wants to merge 1 commit into
Open
fix(gnovm): reclaim stored key object on delete() of object-keyed map#5882omarsy wants to merge 1 commit into
omarsy wants to merge 1 commit into
Conversation
delete(m, key) on a realm map with array/struct keys detached the ARGUMENT key from the realm instead of the STORED key object the map held (an iv.Copy child). The stored key was therefore never DecRef'd, never marked deleted, and never removed from the store — a deterministic, unbounded on-chain storage leak (and unrefunded storage deposit) for any realm that add/deletes an array- or struct-keyed persistent map. DeleteForKey now returns the stored key TypedValue it removed; the delete builtin marks that stored key (not the argument key) in DidUpdate. Single caller; value path unchanged. Regression zrealm_map5.gno: init() persists a map[[1]int]int, main() deletes one entry; the finalize golden now emits d[keyOID](-213) for the reclaimed key. Verified red->green: buggy code omits the d[...] deletion, so the golden mismatches. Co-Authored-By: Claude Opus 4.8 <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):🟢 Maintainers must be able to edit this pull request (more info) ☑️ Contributor Actions:
☑️ Reviewer Actions:
📚 Resources:Debug
|
davd-gzl
approved these changes
Jul 6, 2026
Comment on lines
+828
to
840
| func (mv *MapValue) DeleteForKey(m *Machine, store Store, key *TypedValue) (deletedKey *TypedValue) { | ||
| // if key is NaN, do nothing. | ||
| kmk, isNaN := key.ComputeMapKey(m, store, false) | ||
| if isNaN { | ||
| return | ||
| return nil | ||
| } | ||
| if mli, ok := mv.vmap[kmk]; ok { | ||
| mv.List.Remove(mli) | ||
| delete(mv.vmap, kmk) | ||
| return &mli.Key | ||
| } | ||
| return nil | ||
| } |
Member
There was a problem hiding this comment.
DeleteForKey has the machine and could mark the removed key itself instead of returning it for the builtin. Is that deliberate, to keep DidUpdate out of MapValue?
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.
Fixes #5881.
Problem
delete(m, key)on a realm map with array/struct keys detached the argument key (arg1.Deref()) from the realm viaDidUpdate, not the distinct stored key object the map holds (aniv.Copychild). The stored key keptRefCount == 1, was never marked deleted, and was never removed from the store — realm storage grows across add/delete cycles and the key's storage deposit is never refunded. Deterministic (no consensus impact); the value side was already correct.Fix
MapValue.DeleteForKeynow returns the stored keyTypedValueit removed; thedeletebuiltin marks that stored key (not the argument key) inDidUpdate.DeleteForKeyhas a single caller, so the signature change is contained. Value handling unchanged.Test
gnovm/tests/files/zrealm_map5.gno:init()persists amap[[1]int]int,main()deletes one entry; the finalize golden now emitsd[keyOID](-213)for the reclaimed key — before the fix the map entry was dropped (u[map]) but the key object was never deleted (nod[...]). Verified red→green (the golden mismatches on unfixed code). Existingzrealm_map*/ map / delete filetests pass;build+vetclean.🤖 Generated with Claude Code