Skip to content

fix(rpc): avoid race when reading storage proof state - #4064

Open
cicr99 wants to merge 1 commit into
mainfrom
fix/storage-proof-state-at-block
Open

cicr99 wants to merge 1 commit into
mainfrom
fix/storage-proof-state-at-block

Conversation

@cicr99

@cicr99 cicr99 commented Sep 11, 2026

Copy link
Copy Markdown
Contributor

User description

Summary

Fixes a race condition in starknet_getStorageProof when resolving latest. (Reference to the comment here)

Before this change, the handler read the current chain height, validated the requested block against that height, and then opened HeadState(). If a new block was accepted between those two operations, the response could use metadata from block n while reading state from block n + 1.

This updates the handler to open state explicitly at the validated block number instead of using the moving head state.

Notes

This is a tactical fix for the existing inconsistency. A longer-term solution would be to use a DB snapshot so the chain height, header/hash lookup, and state access are all served from the same underlying database view.


PR Type

Bug fix, Tests


Description

  • Fix storage proof state race
    Pin state to validated height
    Replace
    Update v9/v1 tests

File Walkthrough

Relevant files
Bug
storage.go
Pin state at                                                                                         

rpc/v10/storage.go

  • Replace Head with `
    Preserve state to validated height
+1/-1     
storage.go
Pin state at                                                                                         

rpc/v9/storage.go

  • Replace Head with `
    Preserve existing-height validation
+1/-1     
Tests
storage_bench_test.go
Update storage proof benchmark mocks                                         

rpc/v10/storage_bench_test.go

  • Update benchmark mock expectations
    Use `
+4/-1     
storage_test.go
Update storage test mocks                                                               

rpc/v10/storage_test.go

  • Update test mock expectations
    Use `
+4/-1     
storage_test.go
Update v test mocks                                                                           

rpc/v9/storage_test.go

  • Update test mock expectations
    Use `
+4/-1     

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

github-actions Bot commented Sep 11, 2026

Copy link
Copy Markdown
Contributor

Claude finished @cicr99's task in 12m 42s —— View job


Review summary

Blocker (1 issue, posted inline on both rpc/v10/storage.go:209 and rpc/v9/storage.go:96):

Swapping HeadState() for h.bcReader.StateAtBlockNumber(chainHeight) fixes the described race, but StateAtBlockNumber returns a stateHistory-backed reader (core/state/history.go) whose ClassTrie(), ContractTrie(), and ContractStorageTrie() are all stubbed to unconditionally return ErrHistoricalTrieNotSupported. StorageProof needs exactly these methods to build the Merkle proofs, so every real (non-mocked) call to starknet_getStorageProof now fails with an internal error.

This isn't theoretical — this PR's own CI run already shows it: Juno Test (run 34593901648) fails on TestStorageProof_StorageRoots/get_contract_proof in both rpc/v9 and rpc/v10, which exercise a real blockchain.Blockchain + synced state rather than a mock. The unit tests added/touched in this PR pass only because they mock bcReader.StateAtBlockNumber to return a MockStateReader that doesn't have this historical-trie restriction, so the mocked tests mask the regression.

The underlying race is legitimate and worth fixing, but the fix needs a state accessor that pins to a specific block's global state root while still supporting trie reconstruction — e.g., resolving the state root at chainHeight directly and building a state.NewStateReader(stateRoot, ...), or extending the history state path to support ClassTrie/ContractTrie/ContractStorageTrie for the head block. As currently written, this PR would need to fail CI and shouldn't be merged in its current form.

@cicr99
cicr99 deployed to Development September 11, 2026 11:33 — with GitHub Actions Active
Comment thread rpc/v10/storage.go
}

state, closer, err := h.bcReader.HeadState()
state, closer, err := h.bcReader.StateAtBlockNumber(chainHeight)

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.

Blocker: StateAtBlockNumber returns a state view that cannot serve tries, so this handler now always fails.

h.bcReader.StateAtBlockNumber(chainHeight) returns a stateHistory wrapper (see blockchain/statebackend/statebackend.gocore/state/history.go). Its ClassTrie(), ContractTrie(), and ContractStorageTrie() are explicitly stubbed to return ErrHistoricalTrieNotSupported:

func (s *stateHistory) ClassTrie() (core.TrieReader, error) {
	return nil, ErrHistoricalTrieNotSupported
}

StorageProof below (lines 215/220 and getContractStorageProof/buildContractLeavesData) calls exactly these methods, which are required to build the Merkle proofs. This makes every real (non-mocked) call to starknet_getStorageProof return ErrInternal.

This isn't hypothetical — this PR's own CI run (Juno Test job, ubuntu-latest, run 34593901648) already fails on the integration test that exercises the real blockchain/state stack:

--- FAIL: TestStorageProof_StorageRoots (1.00s)
    --- FAIL: TestStorageProof_StorageRoots/get_contract_proof (0.00s)
        Error: Expected nil, but got: &jsonrpc.Error{Code:-32603, Message:"Internal error", ...}

The unit tests in this PR pass only because they mock bcReader.StateAtBlockNumber to return a mock StateReader that doesn't have this trie limitation (storage_test.go, storage_bench_test.go), masking the regression.

The same issue applies to rpc/v9/storage.go:96.

HeadState() returns a plain *StateReader (real trie access), whereas StateAtBlockNumber returns the history-view used for point-in-time historical queries, which deliberately doesn't support trie reconstruction. Since this handler only ever proves state at the current chain height (never truly historical, since isBlockSupported restricts to head-only anyway), swapping to StateAtBlockNumber breaks proof generation. The original race is real, but this fix needs a state accessor that pins to a specific block's root while still supporting ClassTrie/ContractTrie/ContractStorageTrie — e.g. reading the state root at chainHeight directly and constructing a state.NewStateReader(stateRoot, ...), or extending the history path to support trie reads.

Fix this →

Comment thread rpc/v9/storage.go
}

state, closer, err := h.bcReader.HeadState()
state, closer, err := h.bcReader.StateAtBlockNumber(chainHeight)

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.

Blocker: same issue as rpc/v10/storage.go:209. StateAtBlockNumber returns a stateHistory whose ClassTrie()/ContractTrie()/ContractStorageTrie() return ErrHistoricalTrieNotSupported, so this handler's calls to those methods below will always error. Confirmed failing in this PR's own CI: TestStorageProof_StorageRoots/get_contract_proof in rpc/v9 fails with ErrInternal. See the detailed comment on rpc/v10/storage.go:209 for the full explanation and suggested fix.

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.

2 participants