Skip to content

refactor: make ProveMulti the main logic for Prove - #4084

Open
cicr99 wants to merge 2 commits into
mainfrom
refactor/storage-proof
Open

cicr99 wants to merge 2 commits into
mainfrom
refactor/storage-proof

Conversation

@cicr99

@cicr99 cicr99 commented Sep 16, 2026

Copy link
Copy Markdown
Contributor

User description

This PR refactors trie proof generation so the single-key Prove method delegates to the shared multi-key proof traversal.
ProveMulti is now the primary implementation for proof generation in both trie implementations, while Prove remains as a compatibility wrapper for existing single-key callers.
Old Prove logic was moved for testing purposes.
This PR also includes some cleanup regarding variable and method naming, and general organization of the file.

RPC v8 note
RPC v8 is close to being removed, so this PR intentionally avoids changing that folder. Because v8 still has single-key proof call sites, Prove is kept for now as a compatibility wrapper. Once v8 is removed and remaining callers are migrated, we can revisit whether ProveMulti should become the main public proof API or whether the single-key wrapper can be removed.


PR Type

Enhancement, Tests


Description

  • Refactor Prove to delegate to ProveMulti in both core/trie and core/trie2 packages

  • Rename variables/methods in proof.go traversal logic for clarity (curcurrentKey, knownChildrenknownStorageNodes, binaryProofNodecreateBinaryProofNode, proofNodereadProofNode, shouldSkipMultiProofNodeisEndOfBranchReached)

  • Reorganize ensureNoUnhashedWrites placement in proof.go

  • Add prove_legacy_test.go to verify new Prove matches legacy single-key proof logic

  • Rename local variables (tr/t) to more descriptive names in rpc/v9 and rpc/v10 storage proof handlers


File Walkthrough

Relevant files
Enhancement
proof.go
Simplify Prove via ProveMulti and rename internals             

core/trie/proof.go

  • Prove now delegates to ProveMulti with a single-key slice instead of
    duplicating traversal logic
  • Moved ensureNoUnhashedWrites below ProveMulti
  • Renamed variables and functions for clarity: curcurrentKey,
    shouldSkipMultiProofNodeisEndOfBranchReached,
    proofNodereadProofNode, knownChildrenknownStorageNodes,
    binaryProofNodecreateBinaryProofNode, tritrie
+67/-89 
proof.go
Simplify Prove to use ProveMulti                                                 

core/trie2/proof.go

  • Prove now delegates to ProveMulti with a single-key slice instead of
    custom traversal implementation
+3/-54   
Tests
prove_legacy_test.go
Add regression test comparing Prove to legacy logic           

core/trie/prove_legacy_test.go

  • New test file added to verify new Prove implementation matches legacy
    single-key proof logic
  • Includes proveLegacyForTest helper reimplementing old proof traversal
  • Includes requireProofNodeSetEqualForTest helper to compare proof node
    sets by hash and string
+107/-0 
Formatting
storage.go
Rename trie variables for clarity                                               

rpc/v10/storage.go

  • Renamed local variables tr/t to more descriptive names (trieReader,
    stateTrie, contractTrie, storageTrie) for readability
+17/-17 
storage.go
Rename trie variables for clarity                                               

rpc/v9/storage.go

  • Renamed local variables tr/t to more descriptive names (trieReader,
    stateTrie, contractTrie, storageTrie) for readability
+17/-17 

@cicr99
cicr99 requested a review from rodrodros September 16, 2026 11:32
@github-actions

github-actions Bot commented Sep 16, 2026

Copy link
Copy Markdown
Contributor

Claude finished @cicr99's task in 10m 51s —— View job


Review complete

  • Gather context (PR diff, repo structure)
  • Review core/trie/proof.go
  • Review core/trie/prove_legacy_test.go
  • Review core/trie2/proof.go
  • Review rpc/v9/storage.go and rpc/v10/storage.go
  • Post findings / summary

Summary: This is a clean, mechanical refactor. Prove now delegates to ProveMulti([]felt.Felt{*key}, proof) in both core/trie and core/trie2, and the old single-key traversal logic was preserved verbatim in core/trie/prove_legacy_test.go (as proveLegacyForTest) with a new test (TestProveMatchesLegacySingleKeyProof) asserting the old and new paths produce identical proof sets. The rest of the diff is variable/parameter renaming (curcurrentKey, trtrieReader/stateTrie/contractTrie, shouldSkipMultiProofNodeisEndOfBranchReached, binaryProofNodecreateBinaryProofNode, etc.) with no behavioral changes. rpc/v8 is correctly left untouched, matching the PR description (v8 still needs single-key Prove).

Verified:

  • go build succeeds for core/trie, core/trie2, and rpc/....
  • go vet ./core/trie/... is clean.
  • Existing TestProve/TestProveNonExistent/TestProveRandom in both trie packages already exercise Prove end-to-end (via VerifyProof), so the new delegation path is covered by pre-existing tests, not just the new legacy-comparison test.
  • Traced through proveMultiFrom/isEndOfBranchReached (trie) and proveMultiFrom/addProofNode with isRoot (trie2) — logic is unchanged from before the refactor, only renamed.

Note: I wasn't able to actually execute go test in this sandboxed environment (tool approval for go test was unavailable), so I relied on go build/go vet plus manual diff tracing rather than running the suite. If test execution permissions can be granted, it'd be worth confirming TestProveMatchesLegacySingleKeyProof and the full core/trie/core/trie2 suites pass in CI.

No bugs, concurrency issues, or correctness regressions found. Nothing to flag as blocker/important. One nit-level observation only:

Nit: core/trie/prove_legacy_test.go duplicates the pre-refactor Prove logic almost verbatim as proveLegacyForTest. This is reasonable for a regression-comparison test and the PR description explicitly calls this out ("Old Prove logic was moved for testing purposes"), so no change requested.

@github-actions

Copy link
Copy Markdown
Contributor

Standalone PR Review

PR-Agent could not safely update the persistent review. This standalone result will not replace the canonical review.

PR Reviewer Guide 🔍

Here are some key observations to aid the review process:

⚠️ Risk level: Low
📂 Priority files

  • core/trie/proof.go
🏅 Score: 90
🧪 PR contains tests
🔒 No security concerns identified
⚡ Recommended focus areas for review

@codecov

codecov Bot commented Sep 16, 2026

Copy link
Copy Markdown

Codecov Report

❌ Patch coverage is 94.28571% with 4 lines in your changes missing coverage. Please review.
✅ Project coverage is 79.37%. Comparing base (64e105c) to head (ebfd98a).
⚠️ Report is 7 commits behind head on main.

Files with missing lines Patch % Lines
rpc/v10/storage.go 84.61% 2 Missing ⚠️
rpc/v9/storage.go 84.61% 2 Missing ⚠️
Additional details and impacted files
@@            Coverage Diff             @@
##             main    #4084      +/-   ##
==========================================
- Coverage   79.47%   79.37%   -0.11%     
==========================================
  Files         466      466              
  Lines       36015    36011       -4     
==========================================
- Hits        28623    28583      -40     
- Misses       7383     7419      +36     
  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.

@cicr99
cicr99 deployed to Development September 16, 2026 11:49 — with GitHub Actions Active
@github-actions

github-actions Bot commented Sep 17, 2026

Copy link
Copy Markdown
Contributor

Claude finished @cicr99's task in 9m 56s —— View job


Review complete

  • Gather context (PR diff, repo structure)
  • Review core/trie/proof.go
  • Review core/trie/prove_legacy_test.go
  • Review core/trie2/proof.go
  • Review rpc/v9/storage.go and rpc/v10/storage.go
  • Post findings / summary

Summary: This is a clean, mechanical refactor with no behavioral changes.

  • core/trie.Trie.Prove and core/trie2.Trie.Prove now both delegate to ProveMulti([]felt.Felt{*key}, proof), which was introduced by the prior PR (perf(rpc): optimize storage proof generation with multi-key trie proofs #4015) and is unchanged here.
  • The pre-refactor single-key traversal logic was preserved verbatim in core/trie/prove_legacy_test.go as proveLegacyForTest, and TestProveMatchesLegacySingleKeyProof asserts the legacy path and the new delegating Prove produce identical proof sets (same hash and string representation) across present and absent keys.
  • core/trie2/proof.go's old single-key Prove body (~54 lines of node-walking/hashing) was fully removed in favor of the wrapper — no equivalent legacy-comparison test was added for trie2, but Prove/ProveMulti are already exercised by existing trie2 proof tests, and the core/trie legacy test provides confidence the underlying traversal algorithm is correct.
  • rpc/v9/storage.go and rpc/v10/storage.go changes are pure identifier renames (trtrieReader/stateTrie/contractTrie/storageTrie) with no logic changes — confirmed via diff, identical control flow.
  • Naming cleanups in core/trie/proof.go (curcurrentKey, shouldSkipMultiProofNodeisEndOfBranchReached, binaryProofNodecreateBinaryProofNode, knownChildrenknownStorageNodes, tritrie) are consistent and improve readability; no logic altered.
  • rpc/v8 is correctly left untouched, matching the PR description (v8 still needs the single-key Prove API).

Verified:

  • go build ./core/... ./rpc/... succeeds.
  • go vet ./core/trie/... ./core/trie2/... ./rpc/v9/... ./rpc/v10/... is clean.
  • Traced proveMultiFrom/isEndOfBranchReached (trie) and proveMultiFrom/addProofNode with isRoot (trie2) — unchanged from before this refactor, only renamed.
  • I was not able to execute go test in this sandboxed environment (tool approval unavailable), so please confirm TestProveMatchesLegacySingleKeyProof and the full core/trie/core/trie2 suites pass in CI (Codecov shows patch coverage 94.3%, missing coverage is on the default:/error branches in the renamed rpc/v9, rpc/v10 type switches, which is pre-existing and not introduced by this PR).

No bugs, concurrency issues, or correctness regressions found.

Nit: core/trie/prove_legacy_test.go duplicates the pre-refactor Prove logic almost verbatim as proveLegacyForTest. This is reasonable for a regression-comparison test and the PR description explicitly calls this out ("Old Prove logic was moved for testing purposes"), so no change requested.

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