Skip to content

perf(rpc): avoid IndexedBatch allocation in read-only state views - #3941

Open
thiagodeev wants to merge 8 commits into
mainfrom
perf/getNonce
Open

perf(rpc): avoid IndexedBatch allocation in read-only state views#3941
thiagodeev wants to merge 8 commits into
mainfrom
perf/getNonce

Conversation

@thiagodeev

@thiagodeev thiagodeev commented Aug 13, 2026

Copy link
Copy Markdown
Contributor

User description

Read-only state views (HeadState, StateAtBlockNumber, StateAtBlockHash) in deprecatedStateBackend previously allocated a Pebble IndexedBatch per call, paying the batch setup cost and the batch-overlay merge on every subsequent Get - despite never writing. They now read the database directly through a readOnlyTxn adapter that satisfies the db.IndexedBatch interface deprecatedstate.New requires but rejects writes.

Also adds BenchmarkNonce, benchmarking Handler.Nonce against a real Pebble-backed state with 50k contracts under pseudorandom addresses.

Making the deprecated trie read path actually read-only

With writes rejected, another change was required: trie.Trie.Hash() is not read-only (updateValueIfDirty can Put through the txn on the proof-node branch, and a dirty root key is flushed via PutRootKey), and starknet_getStorageProof reaches it through State.ClassTrie() / ContractTrie() / ContractStorageTrie(). Those accessors previously built a full *trie.Trie and wrapped it in core.TrieReader interface; with the new readOnlyTxn type, a stray write would now surface as a user-visible ErrInternal instead of being silently discarded with the throwaway batch.

They now return an actual *trie.TrieReader struct type, which needs only a db.KeyValueReader and has no write capability at all:

  • Prove moved to TrieReader (it only reads); Trie keeps a thin Prove wrapper that preserves the "cannot prove a trie with unhashed writes" guard.
  • The RPC proof code (v8/v9/v10) dispatches on a new trie.Prover capability interface instead of the concrete *trie.Trie type, so both *trie.Trie (guarded) and *trie.TrieReader are accepted and future producers can't silently fall through to "unknown trie type". *trie2.Trie cannot match it (different ProofNodeSet type) and keeps its own branch.
  • Fixed a latent TrieReader.Hash() nil-pointer panic on empty tries (nil root key): it now returns felt.Zero like Trie.Hash(). Reachable via StateAtBlockHash(&felt.Zero) and storage proofs for non-deployed contracts. Also returns the root node to nodePool after hashing.

Numbers

BenchmarkNonce, benchstat over 6 runs each (-benchtime=2s):

metric main this PR delta
time/op 4.184us +-7% 3.392us +-4% -18.92%
B/op 2031 374 -81.59%
allocs/op 19 12 -36.84%

Every read-only RPC that goes through these state views benefits, not just starknet_getNonce.


PR Type

Enhancement, Tests


Description

  • Avoid IndexedBatch allocation in read-only state views via new readOnlyTxn

  • Make deprecated trie read paths truly read-only using *trie.TrieReader

  • Introduce trie.Prover interface for proof generation across RPC v8/v9/v10

  • Add BenchmarkNonce and unit tests for read-only state backend


File Walkthrough

Relevant files
Enhancement
7 files
deprecated.go
Introduce readOnlyTxn adapter to avoid IndexedBatch allocation
+46/-14 
contract.go
Change deployed and ContractStorage to accept KeyValueReader
+15/-4   
state.go
Use TrieReader for ClassTrie, ContractTrie, and ContractStorageTrie
+11/-6   
proof.go
Add Prover interface and move Prove logic to TrieReader   
+26/-3   
storage.go
Dispatch proof generation on trie.Prover interface             
+4/-4     
storage.go
Dispatch proof generation on trie.Prover interface             
+4/-4     
storage.go
Dispatch proof generation on trie.Prover interface             
+4/-4     
Tests
2 files
deprecated_test.go
Add tests for readOnlyTxn and read-only state backend views
+104/-0 
nonce_bench_test.go
Add BenchmarkNonce with synthetic Pebble-backed state       
+123/-0 
Bug fix
1 files
trie.go
Guard TrieReader.Hash against nil root key and reuse node pool
+4/-0     

@thiagodeev thiagodeev added the disable-deploy-test We don't want to run deploy tests with this PR because it might affect our development environment. label Aug 13, 2026
@codecov

codecov Bot commented Aug 13, 2026

Copy link
Copy Markdown

Codecov Report

❌ Patch coverage is 91.89189% with 3 lines in your changes missing coverage. Please review.
✅ Project coverage is 79.18%. Comparing base (5a74a35) to head (156f3a3).

Files with missing lines Patch % Lines
core/deprecatedstate/state.go 77.77% 2 Missing ⚠️
core/deprecatedstate/contract.go 87.50% 1 Missing ⚠️
Additional details and impacted files
@@            Coverage Diff             @@
##             main    #3941      +/-   ##
==========================================
- Coverage   79.28%   79.18%   -0.10%     
==========================================
  Files         464      464              
  Lines       35793    35811      +18     
==========================================
- Hits        28377    28358      -19     
- Misses       7407     7444      +37     
  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.

@thiagodeev thiagodeev changed the title Perf/getNonce perf(rpc): avoid IndexedBatch allocation in read-only state views Aug 20, 2026
@thiagodeev
thiagodeev force-pushed the perf/getNonce branch 2 times, most recently from 0d237a9 to f44b004 Compare August 25, 2026 02:49
@thiagodeev thiagodeev removed the disable-deploy-test We don't want to run deploy tests with this PR because it might affect our development environment. label Aug 25, 2026
@thiagodeev
thiagodeev marked this pull request as ready for review August 25, 2026 03:03
@github-actions

github-actions Bot commented Aug 25, 2026

Copy link
Copy Markdown
Contributor

Claude finished @thiagodeev's task in 7m 59s —— View job


Review: perf(rpc): avoid IndexedBatch allocation in read-only state views

  • Gather context (diff, related code)
  • Verify the readOnlyTxn adapter satisfies db.IndexedBatch and the read semantics claim
  • Trace every read path reachable from core.StateReader for hidden writes
  • Review core/deprecatedstate/contract.go widening
  • Review the benchmark
  • Build / vet
  • Post inline comments

The core idea is sound and the reasoning in the description checks out. I verified the key claim: juno's NewIndexedBatch returns pebble's own indexed batch (db/pebble/db.go:197), whose Get goes to db.getInternal against the current read state — it is not a snapshot. So swapping it for a direct b.database read does not change visibility semantics for any of the three views. readOnlyTxn embeds db.KeyValueReader (Has/Get/NewIterator) and supplies the six write/lifecycle methods, which is exactly db.IndexedBatch.

Two things the PR fixes as a side effect and doesn't claim credit for: the old code leaked an IndexedBatch on every HeadState/StateAtBlockNumber/StateAtBlockHash call (created, returned with NoopStateCloser, never closed), and it handed each view a pebble batch that isn't designed for concurrent use — Trie.Hash() fans out reads across goroutines via updateChildTriesConcurrently. Reading the KeyValueStore directly is both leak-free and thread-safe.

Findings

Blocker — none.

Important

  • deprecated.go:29-32 — the "read-only views never write" premise has one reachable exception. Trie.Hash() (called by starknet_getStorageProof on ClassTrie()/ContractTrie(), and by Commitment() via the globalTrie closer) writes back through the txn on the proof-node branch of updateValueIfDirty (core/trie/trie.go:661,706). Those writes previously vanished into the throwaway batch; now they return errReadOnlyStateView and surface as ErrInternal. I could find no code that persists half-empty proof nodes into the deprecated trie buckets, so this looks dead in practice — but the error carries no provenance, and I'd want getStorageProof exercised against a real Pebble-backed deprecated state.
  • deprecated.go:23-25 — no test asserts the new invariant (there is no deprecated_test.go). Suggested a small test plus a var _ db.IndexedBatch = readOnlyTxn{} assertion.

Nit

  • deprecated.go:63 — the zero-block-hash branch can now share a package-level empty store instead of allocating memory.New() per call; rejecting writes is what makes that safe.
  • nonce_bench_test.go:107-116 — the benchmark only checks for a nil rpc error, so it wouldn't notice a change that returned the wrong nonce quickly. One assertion before b.Loop() fixes that.

The contract.go signature widening (deployed, ContractStoragedb.KeyValueReader) is a clean, correct narrowing of what those helpers demand and isn't needed for compilation — good change on its own.

I also confirmed blockchain.New defaults to stateVersion: falsedeprecatedStateBackend (blockchain/statebackend/types.go:84), so the benchmark really does exercise the modified path.

Verification

go build ./... and go vet ./blockchain/statebackend/ ./core/deprecatedstate/ ./rpc/v10/ are clean. cmd/juno fails only at the link step (cannot find -ljuno_starknet_rs, -lbz2) because the Rust artifacts aren't built in this environment — unrelated to the diff. I was not able to run the test suite: go test isn't in my allowed tools here, so the behavioural claims above come from reading the code, not from a green run.
• branch perf/getNonce

Comment thread blockchain/statebackend/deprecated.go Outdated
Comment thread blockchain/statebackend/deprecated.go
Comment thread blockchain/statebackend/deprecated.go Outdated
Comment thread rpc/v10/nonce_bench_test.go
@github-actions

github-actions Bot commented Aug 28, 2026

Copy link
Copy Markdown
Contributor

Claude finished @thiagodeev's task in 6m 42s —— View job


Re-review after the TrieReader refactor

  • Gather context (full diff vs origin/main)
  • Review readOnlyTxn + deprecated.go
  • Review the TrieReader refactor (core/trie/proof.go, trie.go, rpc storage.go ×3)
  • Check whether the previously-flagged Trie.Hash() write path is resolved
  • Review deprecatedstate changes + benchmark
  • Build / vet
  • Post inline comments

The new commit (2f09bcc) resolves the main finding from my last review. State.ClassTrie() / ContractTrie() / ContractStorageTrie() no longer hand out a writable *trie.Trie, so starknet_getStorageProof can't reach Storage.Put through Trie.Hash() and hit errReadOnlyStateView. TrieReader.Hash() reads the stored root node's Value — identical to what Trie.Hash() produced on a clean trie, since updateValueIfDirty short-circuits when dirtyNodes is empty and the root is a normal binary node. Good fix, and the right one.

Two supporting details I verified:

  • The new if t.rootKey == nil { return felt.Zero, nil } in TrieReader.Hash is load-bearing, not cosmetic — without it the empty-trie test helpers (emptyTrie&tempTrie.TrieReader) would nil-deref in dbKey, because the nil-root case previously lived only in Trie.Hash (core/trie/trie.go:820).
  • defer nodePool.Put(root) is safe: Node.Hash copies (node.go:27 derefs *n.Value; the hashFn branch returns a value), and the deferred Put runs after the return value is materialised.

Findings

Blocker — none.

Important

  • rpc/v10/storage.go:330 (mirrored in v8/v9) — case *trie.TrieReader is not a widening of case *trie.Trie; Go type switches match the exact dynamic type, so *trie.Trie now falls to default"unsupported trie type". I traced every core.TrieReader producer and nothing in production returns *trie.Trie any more, so this is correct today — but it's a silent runtime trap for future callers, and the &tempTrie.TrieReader churn across three test files is that trap already being worked around by hand. Switching on a Prove(...) capability interface would accept both. Fix this →

Nit

  • core/trie/proof.go:73 — the "unhashed writes" guard is now bypassable via t.TrieReader.Prove(...), and the updated tests do exactly that (safe only because Commit() precedes them).
  • Still open from my last pass, all four unaddressed: no deprecated_test.go covering readOnlyTxn; no var _ db.IndexedBatch = readOnlyTxn{} compile-time assertion; StateAtBlockHash(&felt.Zero) still allocates a fresh memory.New() per call; BenchmarkNonce still asserts only "no rpc error", so a change returning a wrong-but-fast nonce would read as a win. Reasonable to defer the first three now that errReadOnlyStateView is far less reachable, but the one-line benchmark assertion is basically free.
  • State.ClassTrie()/ContractTrie() now inline db.ClassesTrie.Key() / db.StateTrie.Key(), duplicating what classesTrie()/storage() already encode — two places to keep in sync.

Residual risk from my earlier errReadOnlyStateView finding is now confined to a degenerate proof-node root reached via Commitment(), which I still can't find a producer for in the deprecated buckets.

Verification

go build ./... compiles every Go package; only cmd/juno fails at the link step (cannot find -ljuno_starknet_rs, -ljuno_starknet_compiler_rs, -lbz2) because the Rust artifacts aren't built here — unrelated to this diff. I could not run the test suite or go vet: go test and go vet aren't in my allowed tools in this environment, so all behavioural claims above come from reading the code. Worth confirming TestStorageProof in v8/v9/v10 is green locally before merge, since those are the tests this PR rewires.
• branch perf/getNonce

Comment thread rpc/v10/storage.go Outdated
Comment thread core/trie/proof.go
@github-actions

github-actions Bot commented Aug 28, 2026

Copy link
Copy Markdown
Contributor

Claude finished @thiagodeev's task in 6m 21s —— View job


Re-review after Prover interface + trie simplification

  • Gather context (full diff vs origin/main, new commits dd55a6f, c28e1d2)
  • Review the new Prover interface and storage.go type switches
  • Review core/trie simplifications for behaviour changes
  • Re-check previously-open findings
  • Build / vet
  • Post inline comments + summary

c28e1d2 resolves the last review's important finding. trie.Prover is matched by both *trie.Trie (guarded Prove) and *trie.TrieReader, so the exact-dynamic-type trap is gone and the three storage_test.go files went back to passing tempTrie directly instead of &tempTrie.TrieReader. dd55a6f also puts GetRangeProof back on *Trie only, which closes the guard-bypass nit — Trie.GetRangeProofTrie.ProvecheckUnhashedWrites, with no TrieReader.GetRangeProof to route around it. Both fixes are the right ones.

Things I re-verified in this pass:

  • *trie2.Trie does not satisfy trie.Prover, so the case *trie2.Trie arm is still reachable. trie.ProofNodeSet = utils.OrderedSet[felt.Felt, trie.ProofNode] vs trie2.ProofNodeSet = utils.OrderedSet[felt.Felt, trienode.Node] — distinct instantiations, so distinct method signatures. (This is the basis of the one new finding below.)
  • getContractProofWithDeprecatedTrie narrowing to trie.Prover is safe: it only ever calls Prove; the root hash is taken separately from contractTrie.Hash() in StorageProof.
  • defer nodePool.Put(root) in TrieReader.Hash is still safe under the pool's aliasing rules. ReadStorage.Get takes a fresh object from nodePool on every call (storage.go:116), so the root node Hash() recycles is never the same object whose Value pointer a proof entry aliases (proof.go:103 — those are deliberately never returned). Node.Hash copies in both branches, and the deferred Put runs after the return value is materialised.
  • nodesFromRoot handles rootKey == nil by returning an empty slice, so Prove on a never-deployed contract's storage reader is a no-op rather than a nil deref — matches the old *trie.Trie behaviour.
  • TrieReader.Hash() still agrees with Trie.Hash() on a clean trie: updateValueIfDirty short-circuits at if !shouldUpdate { return node, nil } (trie.go:668) for a normal binary root with no dirty nodes.

Findings

Blocker — none.

Important

  • rpc/v10/storage.go:357 (mirrored at :330, :468 and in v8/v9) — the interface case case trie.Prover: is now listed before the concrete case *trie2.Trie:, so case ordering became load-bearing. *trie2.Trie reaches its own arm only because trie2.ProofNodeSet and trie.ProofNodeSet are different alias instantiations; the two Prove methods are otherwise textually identical. If core/trie2 ever adopts trie.ProofNode, new-state proofs would silently be built by the deprecated adapter in all three RPC versions — a wrong result with no error. Listing the concrete case first makes the switch robust to that.

Nit (all carried over from previous passes, none newly introduced)

  • BenchmarkNonce (rpc/v10/nonce_bench_test.go:112-116) still only asserts "no rpc error", so a change returning a wrong-but-fast nonce would read as a win. One require.Equal(b, felt.FromUint64[felt.Felt](1), *nonce) before b.Loop() closes it — this one is basically free and worth doing.
  • Still no blockchain/statebackend/deprecated_test.go and no var _ db.IndexedBatch = readOnlyTxn{} compile-time assertion (the only db.IndexedBatch assertion in the repo is db/remote/transaction.go:19). Today the adapter's shape is only checked implicitly at the deprecatedstate.New call sites.
  • StateAtBlockHash(&felt.Zero) still allocates a fresh memory.New() per call, which a package-level singleton could replace now that writes are rejected.
  • State.ClassTrie()/ContractTrie() inline db.ClassesTrie.Key() / db.StateTrie.Key(), duplicating what classesTrie()/storage() encode via globalTrie.

Residual risk from the original errReadOnlyStateView finding is unchanged and small: Commitment() still goes through s.storage()/s.classesTrie()gTrie.Commit(), which would hit readOnlyTxn.Put only for a degenerate proof-node root, and I still can't find a producer that persists those into the deprecated buckets. The root-key write in globalTrie's closer is short-circuited by resultingRootKey.Equal(rootKey) when nothing was updated.

Verification

go build ./... compiles every Go package; cmd/juno fails only at the link step (cannot find -ljuno_starknet_rs, -ljuno_starknet_compiler_rs, -lbz2) because the Rust artifacts aren't built in this environment — unrelated to this diff. go vet ./core/trie/ ./blockchain/statebackend/ ./core/deprecatedstate/ ./rpc/v8/ ./rpc/v9/ ./rpc/v10/ is clean, which also type-checks the rewired storage_test.go files. I could not run the test suitego test isn't in my allowed tools here — so please confirm TestStorageProof in v8/v9/v10 is green locally before merge, since those are the tests this PR rewires.
• branch perf/getNonce

Comment thread rpc/v10/storage.go
@github-actions

github-actions Bot commented Sep 1, 2026

Copy link
Copy Markdown
Contributor

Claude Code is working…

I'll analyze this and get back to you.

View job run

@github-actions

github-actions Bot commented Sep 1, 2026

Copy link
Copy Markdown
Contributor

Claude finished @thiagodeev's task in 3m 55s —— View job


Re-review check — no new commits since last pass

  • Check for new commits since the last review round
  • Confirm current HEAD state matches what was already reviewed
  • Re-verify status of previously flagged findings
  • Attempt build/vet

This synchronize trigger didn't bring any new commits: HEAD is still c28e1d2 (refactor(trie): introduce Prover interface and update storage methods), identical to what my previous pass on 2026-08-28 already reviewed (git diff origin/main...HEAD is byte-for-byte the same +210/-39 across the same 9 files). So there's nothing new to review — this is a status recap, not a fresh pass.

Re-checked the files directly rather than trusting the prior summary:

Still open — important

  • rpc/v10/storage.go:330,357,468 (mirrored in v8/v9) — case trie.Prover: is still listed before case *trie2.Trie: in all three getClassProof/getContractProof/getContractStorageProof switches. This wasn't touched since I flagged it, so case ordering remains load-bearing: *trie2.Trie only avoids the interface arm because trie2.ProofNodeSet happens to differ from trie.ProofNodeSet today. Cheap fix, still recommend it before merge: put case *trie2.Trie: first in all three functions × three RPC versions.

Still open — nits (unaddressed, all low-risk to defer)

  • rpc/v10/nonce_bench_test.go:107-116 — the benchmark loop still only checks rpcErr == nil; no require.Equal on the returned nonce, so a fast-but-wrong result would read as a win.
  • No blockchain/statebackend/deprecated_test.go and no var _ db.IndexedBatch = readOnlyTxn{} compile-time assertion — confirmed neither exists anywhere in the tree.
  • StateAtBlockHash(&felt.Zero) (deprecated.go:63) still allocates a fresh memory.New() per call instead of a shared empty singleton.
  • State.ClassTrie()/ContractTrie() still inline the trie-key encoding that classesTrie()/storage() also encode.

Resolved in earlier rounds of this PR (no regression on re-check): the errReadOnlyStateView reachability via Trie.Hash()'s proof-node write path is closed off now that ClassTrie/ContractTrie/ContractStorageTrie return *trie.TrieReader; the exact-type type-switch trap from the TrieReader refactor is fixed by the trie.Prover capability interface; GetRangeProof stayed on *Trie only, so the "unhashed writes" guard isn't bypassable through it.

Verification: could not get a clean go build ./... in this environment — it fails in an unrelated third-party dependency (github.com/cockroachdb/swiss: undefined: hashFn/fastrand64), which reproduces identically on origin/main with no go.mod/go.sum diff in this PR, so it's a pre-existing toolchain/environment issue, not something introduced here. Reviewed by reading the code directly instead.

Nothing new to act on beyond the one still-open important item (type-switch ordering) and the four low-risk nits carried over from prior rounds.
• branch perf/getNonce

@github-actions

github-actions Bot commented Sep 1, 2026

Copy link
Copy Markdown
Contributor

Claude finished @thiagodeev's task in 4m 16s —— View job


Re-review — three new commits address prior nits; one important finding remains open

  • Diff new commits since last review (44356256e, 728a556df, 6fc818e2e) against previously reviewed HEAD (c28e1d2)
  • Verify readOnlyTxn error/assertion/singleton changes
  • Verify new deprecated_test.go
  • Verify benchmark assertion fix
  • Re-check outstanding case-ordering finding in storage.go (v8/v9/v10)
  • Build / vet

Three new commits landed since my last pass, and they clean up all four previously-open nits:

  1. 44356256e (blockchain/statebackend/deprecated.go) — wraps each rejected write with provenance (fmt.Errorf("%w: Put", ErrReadOnlyStateView) etc.), exports ErrReadOnlyStateView so callers can errors.Is, adds the compile-time var _ db.IndexedBatch = readOnlyTxn{} assertion, and replaces the per-call memory.New() for the zero-block-hash case with a package-level emptyStateReader singleton. All three exactly match what I suggested — good fixes.
  2. 6fc818e2e (blockchain/statebackend/deprecated_test.go, new) — TestReadOnlyTxn covers Put/Delete/DeleteRange/Write all erroring with ErrReadOnlyStateView, Size()==0, and that Close() is a no-op that leaves the underlying store open/readable. TestDeprecatedStateBackendReadOnlyViews exercises HeadState/StateAtBlockNumber/StateAtBlockHash against a populated Pebble-shaped memory DB and confirms the zero-block-hash view is empty-but-usable (db.ErrKeyNotFound on read). This closes the "no test coverage" gap from my first pass.
  3. 728a556df (rpc/v10/nonce_bench_test.go) — adds require.Equal(b, felt.FromUint64[felt.Felt](1), *nonce) before b.Loop(), so the benchmark now actually validates the read path instead of only checking for a nil error.

Findings

Blocker — none.

Important

  • rpc/v10/storage.go:330,357,468 (mirrored in rpc/v8/storage.go:213,240,355 and rpc/v9/storage.go:214,241,356) — still open, unchanged since my last two passes. case trie.Prover: is still ordered before case *trie2.Trie: in all three RPC versions' getClassProof/getContractProof/getContractStorageProof. This means *trie2.Trie only falls through to its own arm because trie2.ProofNodeSet (utils.OrderedSet[felt.Felt, trienode.Node]) happens to differ from trie.ProofNodeSet (utils.OrderedSet[felt.Felt, trie.ProofNode]) today — the two Prove methods are otherwise textually identical. If core/trie2 ever converges on trie.ProofNode, *trie2.Trie would start matching case trie.Prover first and every new-state starknet_getStorageProof would silently be built by the deprecated-trie adapter across all three RPC versions — a wrong result with no error. Putting the concrete case *trie2.Trie: first in all three functions × three versions removes the ordering dependency at no cost. Fix this →

Nit

  • None new. The four nits carried over from earlier rounds are now resolved by this round's commits (test coverage, IndexedBatch assertion, empty-store singleton, benchmark assertion).

Verification

go build ./... and go vet ./blockchain/statebackend/ ./core/deprecatedstate/ ./core/trie/ ./rpc/v8/ ./rpc/v9/ ./rpc/v10/ are both clean, including the new deprecated_test.go. go test is not in my allowed tools in this environment, so I could not execute TestReadOnlyTxn/TestDeprecatedStateBackendReadOnlyViews/BenchmarkNonce — please confirm they're green locally (the test logic reads correctly on inspection).

Only the type-switch ordering item remains outstanding from prior rounds; everything else previously flagged is now addressed.
• branch perf/getNonce

@github-actions

github-actions Bot commented Sep 1, 2026

Copy link
Copy Markdown
Contributor

Claude finished @thiagodeev's task in 3m 50s —— View job


Re-review check — no functional changes since the last full pass

  • Check for new commits since the last full review pass
  • Confirm whether previously flagged findings are resolved or still open
  • Attempt build/vet
  • Post summary

HEAD is ad038b1d5 (test(statebackend): new tests for the depredatedstate). Diffed against origin/main, the changeset is byte-for-byte the same +341/-39 across the same 10 files as the previous full review round (the test-file commit's SHA differs from what was reviewed before, 6fc818e2e vs ad038b1d5, but the content is identical — same 104-line deprecated_test.go). So there's nothing new to review here; this is a status recap.

Still open — important

  • rpc/v10/storage.go:330,357,468 (mirrored in rpc/v8/storage.go:213,240,355 and rpc/v9/storage.go:214,241,356) — confirmed via direct grep that case trie.Prover: is still ordered before case *trie2.Trie: in all three RPC versions' getClassProof/getContractProof/getContractStorageProof. This remains unaddressed across all review rounds. *trie2.Trie only avoids the interface arm today because trie2.ProofNodeSet (utils.OrderedSet[felt.Felt, trienode.Node]) differs from trie.ProofNodeSet (utils.OrderedSet[felt.Felt, trie.ProofNode]) — the two Prove methods are otherwise textually identical. If core/trie2 ever converges on trie.ProofNode, *trie2.Trie would start matching case trie.Prover first and every new-state starknet_getStorageProof would silently be built by the deprecated-trie adapter across all three RPC versions — a wrong proof with no error. Putting the concrete case *trie2.Trie: first in all three functions × three versions removes the ordering dependency at no cost. Fix this →

Resolved (confirmed unchanged on this pass)

  • readOnlyTxn provenance/assertion/singleton (blockchain/statebackend/deprecated.go) — ErrReadOnlyStateView exported, wrapped per-method (%w: Put etc.), var _ db.IndexedBatch = readOnlyTxn{} present, emptyStateReader singleton replaces per-call memory.New().
  • blockchain/statebackend/deprecated_test.goTestReadOnlyTxn and TestDeprecatedStateBackendReadOnlyViews present and cover the invariants raised in earlier passes.
  • rpc/v10/nonce_bench_test.go — asserts the returned nonce value, not just a nil error.
  • errReadOnlyStateView reachability via Trie.Hash()'s proof-node write path — closed off by ClassTrie/ContractTrie/ContractStorageTrie returning *trie.TrieReader.
  • Exact-type type-switch trap from the earlier TrieReader refactor — fixed by the trie.Prover capability interface.
  • GetRangeProof stayed on *Trie only, so the "unhashed writes" guard isn't bypassable through it.

Verification

go build ./... compiles every package; cmd/juno fails only at the link step (cannot find -ljuno_starknet_compiler_rs, -ljuno_starknet_rs, -lbz2) because the Rust artifacts aren't built in this environment — pre-existing, unrelated to this diff. go vet ./blockchain/statebackend/ ./core/deprecatedstate/ ./core/trie/ ./rpc/v8/ ./rpc/v9/ ./rpc/v10/ is clean. go test is not in my allowed tools in this environment, so I still could not execute TestReadOnlyTxn/TestDeprecatedStateBackendReadOnlyViews/BenchmarkNonce directly — please confirm they're green locally before merge.

Only the type-switch ordering item remains outstanding; everything else previously flagged is addressed.
• branch perf/getNonce

@github-actions

github-actions Bot commented Sep 1, 2026

Copy link
Copy Markdown
Contributor

PR Reviewer Guide 🔍

Here are some key observations to aid the review process:

⏱️ Estimated effort to review: 4 🔵🔵🔵🔵⚪
🏅 Score: 88
🧪 PR contains tests
🔒 No security concerns identified
⚡ No major issues detected

@rodrodros
rodrodros requested a review from infrmtcs September 2, 2026 14:15
@github-actions

github-actions Bot commented Sep 3, 2026

Copy link
Copy Markdown
Contributor

Claude finished @thiagodeev's task in 3m 32s —— View job


Re-review check — merge-from-main only, no PR content changes

  • Diff current HEAD (8d6c3ab1d, "Merge branch 'main' into perf/getNonce") against previously reviewed state (ad038b1d5)
  • Diff against origin/main to confirm PR content is unchanged
  • Re-verify previously flagged type-switch ordering finding
  • Build / vet
  • Post summary

git diff ad038b1d5 HEAD -- <the 10 files this PR touches> is empty — this push only merged main forward (picking up the fast-uri bump, docs contrast fix, feeder client options, and the block-id decode perf commit) and made no changes to any file in this PR's diff. git diff origin/main...HEAD --stat is still the same +341/-39 across the same 10 files as every prior full review round. Nothing new to review.

Still open — important

  • rpc/v10/storage.go:330,357,468 (mirrored in rpc/v8/storage.go:213,240,355 and rpc/v9/storage.go:214,241,356) — confirmed via grep this round: case trie.Prover: is still ordered before case *trie2.Trie: in all three RPC versions' getClassProof/getContractProof/getContractStorageProof. Unaddressed across all six review rounds now. *trie2.Trie only avoids the interface arm today because trie2.ProofNodeSet (utils.OrderedSet[felt.Felt, trienode.Node]) differs from trie.ProofNodeSet (utils.OrderedSet[felt.Felt, trie.ProofNode]) — the two Prove methods are otherwise textually identical. If core/trie2 ever converges on trie.ProofNode, *trie2.Trie would start matching case trie.Prover first and every new-state starknet_getStorageProof would silently be built by the deprecated-trie adapter across all three RPC versions — a wrong proof with no error. Putting the concrete case *trie2.Trie: first in all three functions × three versions removes the ordering dependency at no cost. Fix this →

Resolved (confirmed unchanged on this pass)

  • readOnlyTxn provenance/assertion/singleton, deprecated_test.go coverage, benchmark nonce-value assertion, errReadOnlyStateView reachability via Trie.Hash(), exact-type type-switch trap, GetRangeProof guard bypass — all previously addressed and still intact.

Verification

go build ./blockchain/statebackend/... ./core/deprecatedstate/... ./core/trie/... ./rpc/v8/... ./rpc/v9/... ./rpc/v10/... and the equivalent go vet are both clean. go test is still not in my allowed tools in this environment, so TestReadOnlyTxn/TestDeprecatedStateBackendReadOnlyViews/BenchmarkNonce could not be executed directly — please confirm they're green locally before merge.

Only the type-switch ordering item remains outstanding after six review passes; it's a cheap, low-risk fix (reorder two case arms in three files) whenever it's convenient to take.
• branch perf/getNonce

…hods

- Updated storage functions to return a read-only view of the storage trie.
- Refactored proof methods to utilize TrieReader instead of Trie for consistency.
- Adjusted related tests to accommodate the new changes
- Moved the Prove method back to the Trie struct
- Added Prover interface for generating Merkle proofs.
- Updated storage methods to use Prover instead of TrieReader, supporting both Trie and TrieReader.
- Adjusted related tests to reflect changes in trie handling.
@github-actions

github-actions Bot commented Sep 3, 2026

Copy link
Copy Markdown
Contributor

Claude finished @thiagodeev's task in 1m 17s —— View job


Re-review check — rebase only, no PR content changes

  • Diff current HEAD (156f3a3de) against origin/main (5a74a3506)
  • Confirm content is unchanged from the last fully-reviewed state
  • Re-verify the outstanding type-switch ordering finding
  • Post summary

This push rewrote history (linear rebase onto current origin/main, replacing the earlier merge commit) rather than adding new content: git diff origin/main...HEAD --stat is still +341/-39 across the same 10 files as every prior review round, and the individual file diffs are identical to what's already been reviewed six times. Nothing new to review.

Still open — important

  • rpc/v10/storage.go:330,357,468 (mirrored in rpc/v8/storage.go:213,240,355 and rpc/v9/storage.go:214,241,356) — confirmed again via grep: case trie.Prover: is still ordered before case *trie2.Trie: in all three RPC versions' getClassProof/getContractProof/getContractStorageProof. Unaddressed across all seven review rounds now. *trie2.Trie only avoids the interface arm today because trie2.ProofNodeSet (utils.OrderedSet[felt.Felt, trienode.Node]) differs from trie.ProofNodeSet (utils.OrderedSet[felt.Felt, trie.ProofNode]) — the two Prove methods are otherwise textually identical. If core/trie2 ever converges on trie.ProofNode, *trie2.Trie would start matching case trie.Prover first and every new-state starknet_getStorageProof would silently be built by the deprecated-trie adapter across all three RPC versions — a wrong proof with no error. Putting the concrete case *trie2.Trie: first in all three functions × three versions removes the ordering dependency at no cost. Fix this →

Resolved (confirmed unchanged on this pass)

  • readOnlyTxn provenance/assertion/singleton, deprecated_test.go coverage, benchmark nonce-value assertion, errReadOnlyStateView reachability via Trie.Hash(), exact-type type-switch trap, GetRangeProof guard bypass — all previously addressed and still intact.

Verification

go build ./blockchain/statebackend/... ./core/deprecatedstate/... ./core/trie/... ./rpc/v8/... ./rpc/v9/... ./rpc/v10/... and the equivalent go vet are both clean. go test is not in my allowed tools in this environment, so TestReadOnlyTxn/TestDeprecatedStateBackendReadOnlyViews/BenchmarkNonce could not be executed directly — please confirm they're green locally before merge.

Only the type-switch ordering item remains outstanding after seven review passes; it's a cheap, low-risk fix (reorder two case arms in three files) whenever it's convenient to take.
• branch perf/getNonce

Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment

Projects

None yet

Development

Successfully merging this pull request may close these issues.

1 participant