refactor(v10): check storage existence only for zeros#3790
Conversation
|
Claude finished @brbrr's task in 2m 21s —— View job Code Review —
|
![github-actions[bot]](https://avatars.githubusercontent.com/in/15368?s=60&v=4){kind=small}
There was a problem hiding this comment.
Pull request overview
Refactors the v10 StorageAt RPC handler to avoid always probing contract existence, by only doing the existence check when the storage slot value is zero (where “unset slot” and “missing contract” can both appear as zero on the head reader).
Changes:
- Fetch storage first and only perform a
ContractClassHashexistence probe when the returned storage value is zero. - Update
StorageAtunit tests to reflect the new call sequence/behavior and add a test asserting non-zero values skip the probe. - Minor gofmt-driven test formatting adjustments.
Reviewed changes
Copilot reviewed 2 out of 2 changed files in this pull request and generated 1 comment.
| File | Description |
|---|---|
| rpc/v10/storage.go | Changes StorageAt to probe contract existence only when storage returns zero. |
| rpc/v10/storage_test.go | Updates mocks/expectations and adds a test to verify probe is skipped on non-zero values. |
💡 Add Copilot custom instructions for smarter, more guided reviews. Learn how to get started.
Codecov Report❌ Patch coverage is
Additional details and impacted files@@ Coverage Diff @@
## main #3790 +/- ##
==========================================
+ Coverage 75.17% 75.31% +0.14%
==========================================
Files 438 440 +2
Lines 39376 39463 +87
==========================================
+ Hits 29602 29723 +121
+ Misses 7699 7659 -40
- Partials 2075 2081 +6 ☔ View full report in Codecov by Harness. 🚀 New features to boost your workflow:
|
rodrodros
left a comment
rodrodros
left a comment
There was a problem hiding this comment.
benchmark numbers please
|
Looks good 👍 , you've good findings from Copilot and Claude |
|
@rodrodros i've addressed the bot's feedback and added bench results to the description. please take a look. |
|
Claude finished @brbrr's task in 1m 35s —— View job Re-review after addressing feedback
Both pieces of bot feedback are addressed correctly, and the change is a strict improvement over the prior revision. 👍 1. Copilot's point — skip the probe for historical readers ( The existence probe now only runs on head-type readers: if value.IsZero() && (id.IsLatest() || id.IsPreConfirmed()) {I traced
So the 2. Logging nit ( The internal-error path on the primary Test coverage — good. The new Summary
Note: I could not execute |
RafaelGranza
left a comment
RafaelGranza
left a comment
There was a problem hiding this comment.
It is a good optimization, but I noticed something strange about the pre_confirmed reader that the CI bot also let pass.
Please see my comment and give me feedback on the direction you will take.
Sorry for digging so deep, but I took my time to better understand these readers, states and block types.
| return nil, rpccore.ErrInternal.CloneWithData(err) | ||
| // Only head readers return zero (not ErrKeyNotFound) for a missing contract, | ||
| // so a zero value needs an existence probe there; historical readers don't. | ||
| if value.IsZero() && (id.IsLatest() || id.IsPreConfirmed()) { |
There was a problem hiding this comment.
The id.IsPreConfirmed() is a little bit annoying here.
Why I find it annoying
You see, a pre_confirmed state is inconsistent to whether or not it uses a historical or head reader.
The CI Bot told us it always falls back to a head one, but that is not true. Actually, it almost always falls back to a historical one, the only way for it to use a head one is when the block is at genesis.
You can check that a pre_confirmed state is set by baseState on sync/preconfirmed/chain_storage.go, and the internal methods varies depending whether the block is near the 0th state (genesis).
The problem it creates
It triggers unnecessary extra lookups.
We could fix this by making the genesis base in baseState return ErrKeyNotFound for a missing contract, like the historical base already does. then pre_confirmed is consistent everywhere and we don't touch the global reader.
But then we would need extra tests to ensure this behavior stays the same. Since someone could try to optimize pre_confirmed always using head readers.
Benchmarks to prove the problem exists
This is a plausibility check, I used a shallow chain (mainnet block 2, in-memory DB, so absolute latencies aren't production-representative), but the structural takeaway holds regardless of depth.
We can see that preconfirmed_mature matches the historical.
| Reader | storage_only | storage + probe | delta |
|---|---|---|---|
| historical | 60.3µs / 380 allocs | 120.6µs / 778 allocs | +100% / +398 allocs |
| preconfirmed_mature | 60.1µs / 380 allocs | 120.4µs / 778 allocs | +100% / +398 allocs |
| latest (head, hot) | 0.84µs / 13 allocs | 0.99µs / 16 allocs | +18% / +3 allocs |
What I expect before approving
I expect to hear your opinion and see one of these two:
-
Keep things as-is, and add a comment above the guard, to give more context about the
IsPreConfirmedthing. Maybe thepre_cofirmedcase is rare and the change does not pay off. -
Actual fix:
- Remove
IsPreConfirmed - make the genesis base in
baseState(sync/preconfirmed/chain_storage.go) returnErrKeyNotFoundfor a missing contract, like the historical base does. - Benchmark the mature
pre_confirmedto make sure it makes sense. - create tests to lock that
pre_confirmedwith a missing contract returnsErrContractNotFound, for both the mature and the genesis cases. this is what stops someone from later optimizingpre_confirmedto always use head readers and breaking it silently.
|
@brbrr why there is a case that worsens on performance, shouldn't everything be an improvement? |
|
@rodrodros This PR flips the data fetching approach from defensive (check contract existence, and only then access the storage slot) to optimistic (get the storage slot, and if it fails, figure out why - contract or storage key missing). in the case of This is PR builds up on the assumption that happy path requests are dominant. |
brbrr
force-pushed
the
refactor/v10-storage-existence-check
branch
from
2fa3f01 to
083df26
Compare
|
Claude finished @brbrr's task in 3m 7s —— View job Re-review after the
|
| require.ErrorIs(t, err, db.ErrKeyNotFound) | ||
| require.NoError(t, closer()) | ||
| } | ||
|
|
There was a problem hiding this comment.
This locks the genesis pre_confirmed missing-contract case (via emptyStateReader), but RafaelGranza's option 2 explicitly asked for both the genesis and the mature cases:
create tests to lock that
pre_confirmedwith a missing contract returnsErrContractNotFound, for both the mature and the genesis cases. this is what stops someone from later optimizingpre_confirmedto always use head readers and breaking it silently.
The mature path (baseState → StateAtBlockNumber(bottom-1), i.e. a historical reader) is currently unguarded here. Consider adding a sibling test that applies a chain with bottom > 0, mocks StateAtBlockNumber(bottom-1) to return a base whose ContractStorage yields db.ErrKeyNotFound, and asserts the pre_confirmed state surfaces it — that's precisely what prevents a future "always use head readers" refactor from silently regressing the mature case.
Non-blocking, but it's the specific regression guard the reviewer requested.
|
here's bench results for the latest commit:
|
|
Claude finished @brbrr's task in 3m 51s —— View job Review — latest commit (
|
|
Claude finished @brbrr's task in 5m 28s —— View job Review — latest commit (
|
| return nil, rpccore.ErrInternal.CloneWithData(err) | ||
| // Only the latest (head) reader returns zero for a missing contract; historical | ||
| // and pre_confirmed readers already surface it as ErrKeyNotFound. | ||
| if value.IsZero() && id.IsLatest() { |
There was a problem hiding this comment.
Blocker — the latest commit (chore: remove emptyStateReader) makes this comment false again and reintroduces the genesis pre_confirmed regression.
This guard only probes when value.IsZero() && id.IsLatest(), relying on the invariant stated in the comment above: "historical and pre_confirmed readers already surface it as ErrKeyNotFound." That held only because emptyStateReader forced the genesis pre_confirmed base to return db.ErrKeyNotFound. The last commit removed it, so baseState now returns the raw empty head state from StateAtBlockHash(&felt.Zero) (sync/preconfirmed/chain_storage.go:220), whose ContractStorage maps ErrKeyNotFound → felt.Zero, nil (core/state/state_reader.go:65-66).
Result: at genesis, starknet_getStorageAt on a pre_confirmed block for a non-existent contract returns 0x0 instead of ErrContractNotFound, because id.IsLatest() is false so the probe never runs — the exact inconsistency @RafaelGranza's option 2 set out to fix.
It also breaks testChainReaderPreConfirmedStateAtGenesisMissingContract (sync/preconfirmed/chain_storage_test.go:1067): its base mock has no ContractStorage expectation, so the pending fall-through (core/pending/state.go:69) now hits base.ContractStorage, which gomock fatals as an unexpected call — go test ./sync/preconfirmed/... goes red.
Please revert the emptyStateReader removal (restore the genesis base returning ErrKeyNotFound), or, if it must go, add id.IsPreConfirmed() back to this guard and update the comment + the genesis test.
brbrr
force-pushed
the
refactor/v10-storage-existence-check
branch
from
4b1a781 to
cf3e6cf
Compare
|
Claude finished @brbrr's task in 2m 8s —— View job Re-review — force-pushed
|
| BlockID | Missing contract → StorageAt result |
|---|---|
latest |
probe runs (id.IsLatest()) → ErrContractNotFound ✅ |
hash/number/l1_accepted |
historical reader → ErrKeyNotFound → ErrContractNotFound ✅ |
pre_confirmed mature (bottom>0) |
historical base → ErrKeyNotFound → ErrContractNotFound ✅ |
pre_confirmed genesis (bottom==0) |
head base → felt.Zero, nil; probe skipped (not IsLatest()) → 0x0 ❌ |
Root cause: baseState at genesis now returns the raw head state (StateAtBlockHash(&felt.Zero)), whose ContractStorage maps ErrKeyNotFound → felt.Zero, nil (core/state/state_reader.go:65-66), and the pending fall-through (core/pending/state.go:69) forwards straight to it. The comment at rpc/v10/storage.go:117-119 ("pre_confirmed readers surface it as ErrKeyNotFound") is now false for the genesis case.
Summary
- Blocker: none — the build/test regression is fixed; the suite passes.
- Important: genesis
pre_confirmedStorageAton a missing contract now returns0x0instead ofErrContractNotFound, disagreeing with the maturepre_confirmedpath and with the (now-inaccurate) comment. This is the exact inconsistency option 2 fixed. It's an extreme edge case (only while block 0 is itself unconfirmed), so it may be an acceptable trade-off — but if so, the guard should addid.IsPreConfirmed()back and the comment should note the genesis exception; otherwise restoreemptyStateReader. Inline detail onrpc/v10/storage.go:120. - Nit: none new.
Verified: go build ./rpc/v10/... ./sync/preconfirmed/... passes here. go test is not permitted in this environment, so test pass/fail is inferred from the updated gomock expectations, not executed.
· refactor/v10-storage-existence-check
| // Only the head (latest) reader returns zero for a missing contract, so probe | ||
| // the class hash to distinguish "contract absent" from "slot is zero". | ||
| // Historical and pre_confirmed readers surface it as ErrKeyNotFound above. | ||
| if value.IsZero() && id.IsLatest() { |
There was a problem hiding this comment.
The comment above is now inaccurate, and genesis pre_confirmed returns the wrong result.
The latest commit (chore: remove emptyStateReader) fixes the test failure from the prior revision (the genesis test now asserts 0x0, nil), but the underlying behavioral inconsistency it was guarding against is back:
- mature
pre_confirmed(bottom > 0):baseState→StateAtBlockNumber(bottom-1)→ historical reader → missing contract surfacesdb.ErrKeyNotFound→ mapped toErrContractNotFoundat line 110-112. ✅ - genesis
pre_confirmed(bottom == 0):baseState→StateAtBlockHash(&felt.Zero)→ headStateReader, whoseContractStoragemapsErrKeyNotFound → felt.Zero, nil(core/state/state_reader.go:65-66). The pending fall-through (core/pending/state.go:69) forwards straight to it. Sincepre_confirmedis notIsLatest(), the probe here never runs →starknet_getStorageAtreturns0x0instead ofErrContractNotFound.
So the comment "Historical and pre_confirmed readers surface it as ErrKeyNotFound above" is false for the genesis case, and the two pre_confirmed variants now disagree — the exact inconsistency @RafaelGranza's option 2 removed.
Practically the genesis-pre_confirmed path is an extreme edge case (only while block 0 itself is unconfirmed), so this isn't a hard blocker, but it is a real spec deviation + a comment that will mislead the next maintainer. Two ways to resolve:
- Restore the
emptyStateReaderso genesis matches the historical/mature path (option 2), or - Keep the removal but make the guard match reality — add
id.IsPreConfirmed()back and reword the comment to note genesis returns zero for a missing contract.
As-is, the two halves are inconsistent.
Bench results:
(Generated using sepolia snapshot, 1 block temp db wasn't representative, as the data was hot, and results were marginal)
Note that only for the missing contract on the latest block we would face degradation.