perf(rpc): optimize storage proof generation with multi-key trie proofs - #4015
Conversation
|
Claude finished @cicr99's task in 8m 37s —— View job Review: PR #4015 — optimize storage proof generation with multi-key trie proofs
Note: I was not able to get approval to run SummaryThis is a solid, carefully implemented optimization. I traced through both
No blocker or important issues found. No inline comments posted — the code is correct as far as I can trace it, and well covered by the added equivalence tests. Nit: |
Codecov Report❌ Patch coverage is
Additional details and impacted files@@ Coverage Diff @@
## main #4015 +/- ##
==========================================
- Coverage 79.56% 79.29% -0.28%
==========================================
Files 461 464 +3
Lines 35513 35994 +481
==========================================
+ Hits 28255 28540 +285
- Misses 7249 7445 +196
Partials 9 9 ☔ View full report in Codecov by Harness. 🚀 New features to boost your workflow:
|
|
Claude finished @cicr99's task in 2m 25s —— View job Review: PR #4015 — optimize storage proof generation with multi-key trie proofs
SummaryThis PR was already reviewed in depth after commit
I also re-verified the parts most relevant to protocol correctness given the "combine contract metadata reads" change: No blocker or important issues found in this delta. No inline comments posted. Nit: Patch coverage is 84% per Codecov, with the remaining gaps concentrated in error-handling branches of |
|
Claude finished @cicr99's task in 1m 35s —— View job Review: PR #4015 — optimize storage proof generation with multi-key trie proofs
SummaryThis PR's core algorithm (
While re-reviewing the refactored benchmark file, I also verified:
No blocker or important issues found in this delta. No inline comments posted — this commit is a clean, faithful response to the two outstanding review comments with no new logic to flag. |
| func buildContractLeavesDataFromMetadata( | ||
| state contractMetadataReader, | ||
| contracts []felt.Felt, | ||
| ) ([]*LeafData, error) { |
There was a problem hiding this comment.
I don't understand why we return a slice of pointers. Why not just returning []LeafData?
[]LeafData is just 1 heap allocation
[]*LeafData is n + 1 allocations: 1 from the slice, n from all the elements that also need to be allocated in the heap.
| contractLeavesData[i] = &LeafData{ | ||
| Nonce: &nonce, | ||
| ClassHash: &classHash, | ||
| StorageRoot: &storageRoot, | ||
| } |
There was a problem hiding this comment.
Adding on this, every LeafData is also responsible of 3 other heap allocations.
The reason this happens is because you're storing pointers instead of the actual data. The Go Runtime guarantees these pointers to always be valid by allocating these types in the heap.
More allocation in the heap doesn't only translate as more memory taken (not a big deal) but also, as more CPU time being taken (big deal) from the program itself and given to the Garbage Collection.
Think also, we are allocating in the heap a lot of individual data that it is meant to be short lived. This data is only being brought to memory to answer an RPC request and after that is no longer needed.
Hence it is ok to allocate data in the heap, but in this case, so many small allocations which would normally be harmless do make a difference when we are optimizing for micro-seconds
There was a problem hiding this comment.
All this can be made in a follow up, Juno has this bad practice every way around and it is a conscious effort to not replicate them.
Because they are everywhere, one simple fix here, becomes a hundred of lines refactor, just to avoid the RPC heap allocation.
There was a problem hiding this comment.
As it stands: the whole allocation cost of contractLeavesData is n + 3n + 1.
- If we change the type to
[]LeafDatathen is3n + 1 - If change the internal fields of
LeafDatato value types then is1(Ideally this one)
| type contractMetadataReader interface { | ||
| ContractMetadata(addr *felt.Felt) (classHash, nonce, storageRoot felt.Felt, err error) | ||
| } |
There was a problem hiding this comment.
I don't feel confortable creating a new type which is only going to be used in one single place. Is there a better way, where we avoid this?
Maybe we can add ContractMetadata to the StateReader interface? What do you think?
| knownChildren = append(knownChildren, rightChild) | ||
| } | ||
|
|
||
| return knownChildren, leftNode, rightNode, nil |
There was a problem hiding this comment.
Why not return leftStorageNode and rightStorageNode alongside with leftNode and rigthNode?
I believe the only downside would be manually handling them in addProofNode?
| func (t *Trie) readKnownProofChild( | ||
| keys []BitArray, | ||
| childKey *BitArray, | ||
| ) (*Node, *StorageNode, error) { | ||
| if len(keys) == 0 || childKey == nil || childKey.len == 0 { | ||
| return nil, nil, nil | ||
| } | ||
|
|
||
| node, err := t.readStorage.Get(childKey) | ||
| if err != nil { | ||
| return nil, nil, err | ||
| } | ||
|
|
||
| return node, &StorageNode{key: childKey, node: node}, nil |
There was a problem hiding this comment.
I am not sure if it would fit here, I believe we could return StorageNode by value. What do you think? Is returning it as nil important? If it is, we can keep it as a reference 👍
| binary, err := binaryProofNode(t, sNode, knownChildren...) | ||
| if err != nil { | ||
| return nil, err | ||
| } | ||
|
|
||
| if edge != nil { // Internal Edge | ||
| proof.Put(edgeHash(edge, carriedHash, t.hash), edge) | ||
| } | ||
| proof.Put(*sNode.node.Value, binary) | ||
|
|
||
| return binary, nil | ||
| } |
There was a problem hiding this comment.
There is no reason for this function to return *Binary, when it can easily return Binary and spare the allocation. Note that binaryProofNode, which should also be named createBinaryProofNode, creates the type and returns a reference to it which doesn't make sense. If a function/method create a value inside it, it should return it as a value type most of the time.
@brbrr did you write the function to return by pointer intentionally, or was it a miss?
Please add this to the refactor list alongside the LeafData type and array mentioned in a previous comment 🙏
|
The core of the PR is good but there are many little things missing. I will check |
|
|
||
| continuingPaths := nonEmptyPaths(paths) | ||
| if len(continuingPaths) == 0 { | ||
| return nil |
There was a problem hiding this comment.
I believe there should be a test, where this condition becomes the stop case for recursion
|
@cicr99 since this is a perf PR can you add benchmark results to the PR description 🙏 (using k9). I am aware you already got the numbers. I believe it is good putting them here for posterity |
|
@cicr99 since |
3 participants
Main Change
Before this PR, storage proof generation called
Proveonce per requested key. For requests with many classes, contracts, or storage keys, this meant repeating the same trie traversal from the root for each key, even when many keys shared prefixes.This PR adds
ProveMultito both trie implementations. Instead of callingProveindependently for each requested key, the new method sorts keys by trie path, deduplicates them, and walks shared prefixes once while adding all required proof nodes to the proof set.Other Changes
v9/v10storage proof handlers to useProveMultifor class, contract, and storage proofs.starknet_getStorageProofbenchmarks focused on the high-volume cases this PR optimizes: many classes, many contracts, and mixed many-class/many-contract requests.Benchmarks
ProveMultiwas initially implemented on top of the previous trie proof code, before #3945 was merged. Since #3945 also improved proof generation by reusing stored hashes, the storage proof benchmarks were re-ran after rebasing this PR on top of it and combining both solutions.The table below compares three points: