Skip to content

perf(vm): key the class cache on declaration height instead of load height - #4029

Open
infrmtcs-agent[bot] wants to merge 1 commit into
mainfrom
dat/bench-trace-block-txs
Open

perf(vm): key the class cache on declaration height instead of load height#4029
infrmtcs-agent[bot] wants to merge 1 commit into
mainfrom
dat/bench-trace-block-txs

Conversation

@infrmtcs-agent

@infrmtcs-agent infrmtcs-agent Bot commented Sep 3, 2026

Copy link
Copy Markdown
Contributor

User description

Summary

  • The Rust class cache now stores each class with its declaration height, taken from DeclaredClassDefinition.At, instead of the height of the state that loaded it. A lookup at height H hits iff H > declared_at.
  • With the load-height key, a class first loaded at block N was invisible to every reader below N, so historical starknet_call and trace flows re-fetched and re-parsed classes that had existed for thousands of blocks.
  • JunoStateGetCompiledClass gains a declared_at out-param on the FFI. Classes read through a pending state are still never cached.
  • TestClassCacheKeyedOnDeclarationHeight covers a hit above the declaration height and a miss at the declaration height itself.

Notes

  • Sequencer mode only: Finalise sets the hash in place on the pending header that RPC readers already hold, so a pre_confirmed call can reach the VM with a hashed header over a pending overlay whose classes report At: 0, and cache them at height 0. Sync mode is unaffected because pre-confirmed headers never carry a hash. Sequencer mode is experimental and is being removed in refactor(node): remove sequencer support (and other refactors) #3892.
  • The cache has no invalidation on reorg. That is pre-existing, but with this key a reorged-out class is served at more heights than before. Follow-up: flush the class cache from RevertHead.

PR Type

Enhancement, Tests


Description

  • Key Rust class cache on declaration height instead of load height

  • Add declared_at out-param to JunoStateGetCompiledClass FFI

  • Fix stale cache misses for classes declared many blocks ago

  • Add test TestClassCacheKeyedOnDeclarationHeight verifying cache behavior


File Walkthrough

Relevant files
Enhancement
state.go
Add declared_at out-param to JunoStateGetCompiledClass     

vm/state.go

  • Added declaredAt *C.uint64_t out-parameter to
    JunoStateGetCompiledClass
  • Populates declaredAt with val.At (declaration height) before returning
    the compiled class
+6/-1     
ffi.rs
Update FFI signature for declared_at parameter                     

vm/rust/src/state_reader/ffi.rs

  • Updated JunoStateGetCompiledClass FFI declaration to include a
    declared_at: *mut u64 parameter
+1/-0     
state_reader.rs
Key class cache lookups on declaration height                       

vm/rust/src/state_reader/state_reader.rs

  • Renamed cached_on_height field to declared_at in
    CachedRunnableCompiledClass
  • Changed cache hit condition to compare against declaration height
    instead of load height
  • Passes a declared_at pointer to JunoStateGetCompiledClass FFI call and
    stores the returned value in the cache
  • Updated comments explaining new caching semantics and pending-state
    exclusion
+12/-17 
Tests
vm_test.go
Add test for declaration-height class cache keying             

vm/vm_test.go

  • Added classFetchCounter wrapper to track class fetch calls
  • Added TestClassCacheKeyedOnDeclarationHeight verifying cache hits
    above declaration height and misses at declaration height
+80/-0   

@github-actions

github-actions Bot commented Sep 3, 2026

Copy link
Copy Markdown
Contributor

Claude finished @infrmtcs-agent[bot]'s task in 6m 24s —— View job


Review: perf(vm): key the class cache on declaration height instead of load height

  • Gather context (diff, changed files)
  • Review vm/rust/src/state_reader/ffi.rs
  • Review vm/rust/src/state_reader/state_reader.rs
  • Review vm/state.go
  • Review vm/vm_test.go
  • Check reorg/sequencer-mode caveats mentioned in PR description
  • Post inline comments
  • Post final summary

Overall: the core logic change is sound. is_after(declared_at) correctly makes the declaration height itself a guaranteed miss (needed because trace vs. call/estimate can run the same block number against different state instances), the new test exercises exactly that boundary, and the FFI out-param (declared_at: *mut u64 / *C.uint64_t) is only read on the non-null success path in both state_reader.rs and state.go, so there's no uninitialized-read risk. go build ./vm/... and go vet ./vm/... pass; I could not run cargo build/go test for the vm package in this sandbox (network/toolchain commands required approval I don't have), so the Rust side and the new test itself are unverified by execution — worth a maintainer running go test ./vm/... before merge.

Important

  • vm/vm_test.go:462-464TestClassCacheKeyedOnDeclarationHeight reuses a class hash also used by TestCallDeprecatedCairo/TestCallDeprecatedCairoMaxSteps. The Rust CLASS_CACHE is a process-wide static with no reset between Go tests, so this only stays correct because those other tests pass a pending (Hash == nil) BlockInfo and never populate the cache for that hash. Left inline — not a current flake (assertions are relative, not absolute), but a footgun for future test edits. Suggest a class hash unique to this test.
  • The PR description's own caveat checks out against the code: vm/vm.go:328 derives is_pending purely from Header.Hash == nil, and core/pending/state.go:90-99 reports At: 0 for classes declared in the pending block. So in sequencer mode, once Finalise hashes the pending header in place, a pre_confirmed read can be classified as BlockHeight::Height(_) while still reading a pending overlay, caching a brand-new class at declared_at: 0 — making it visible to any later height. Confirmed real, but correctly scoped by the author to experimental sequencer mode (removal tracked in refactor(node): remove sequencer support (and other refactors) #3892), so not a blocker for this change.
  • Cache has no reorg invalidation (pre-existing, acknowledged by author as a follow-up via RevertHead). With this key, a reorged-out class is now visible at more heights than before, so I'd prioritize that follow-up relatively soon rather than letting it linger.

Nits

  • None beyond the above — no golangci-lint-style issues spotted in the diff.

No blockers. The design and test coverage for the primary fix (declaration-height keying) look correct; the two "important" items are either already disclosed/scoped by the author or a test-isolation robustness concern rather than a functional bug in the shipped code.
(branch: dat/bench-trace-block-txs)

Comment thread vm/vm_test.go
Comment on lines +462 to +464
// https://voyager.online/class/0x03297a93c52357144b7da71296d7e8231c3e0959f0a1d37222204f2f7712010e
classHash := felt.NewUnsafeFromString[felt.Felt](
"0x3297a93c52357144b7da71296d7e8231c3e0959f0a1d37222204f2f7712010e",

Copy link
Copy Markdown
Contributor

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

This test reuses the class hash 0x3297a93c...2010e that TestCallDeprecatedCairo and TestCallDeprecatedCairoMaxSteps also use. CLASS_CACHE on the Rust side is a process-wide static (vm/rust/src/state_reader/state_reader.rs:32), so it persists across every Go test in this binary — there's no reset hook between tests.

Today this is safe only because those two other tests call the VM with Header.Hash == nil (pending), so per vm/vm.go:328 they resolve to BlockHeight::Pending and never populate the cache for this hash. If a future test is added (or an existing one is edited) to call the VM with a non-pending header for this same class hash, it would silently pre-populate CLASS_CACHE with a declared_at this test doesn't expect, and TestClassCacheKeyedOnDeclarationHeight would start passing/failing based on test execution order rather than the behavior it's meant to verify.

Since the assertions are already relative to fetchesAfterFirstCall rather than absolute counts, this doesn't cause a current flake, but it's a footgun for future edits. Consider using a class hash that's unique to this test (or asserting/documenting the shared-cache dependency explicitly) so the test stays correct regardless of what other tests in the package do.

Fix this →

@codecov

codecov Bot commented Sep 3, 2026

Copy link
Copy Markdown

Codecov Report

✅ All modified and coverable lines are covered by tests.
✅ Project coverage is 79.16%. Comparing base (2705dd1) to head (3b56082).
⚠️ Report is 10 commits behind head on main.

Additional details and impacted files
@@            Coverage Diff             @@
##             main    #4029      +/-   ##
==========================================
- Coverage   79.21%   79.16%   -0.06%     
==========================================
  Files         464      464              
  Lines       35751    35793      +42     
==========================================
+ Hits        28321    28335      +14     
- Misses       7421     7449      +28     
  Partials        9        9              

☔ View full report in Codecov by Harness.
📢 Have feedback on the report? Share it here.

🚀 New features to boost your workflow:
  • ❄️ Test Analytics: Detect flaky tests, report on failures, and find test suite problems.
  • 📦 JS Bundle Analysis: Save yourself from yourself by tracking and limiting bundle sizes in JS merges.

@infrmtcs
infrmtcs requested a review from rodrodros September 3, 2026 08:47
Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment

Labels

None yet

Projects

None yet

Development

Successfully merging this pull request may close these issues.

1 participant