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.10% -0.08%
==========================================
Files 438 439 +1
Lines 39376 39500 +124
==========================================
+ Hits 29602 29665 +63
- Misses 7699 7741 +42
- Partials 2075 2094 +19 ☔ 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 (
|
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.