feat: add proof_getProofWithMeta JSON-RPC method#11498
feat: add proof_getProofWithMeta JSON-RPC method#11498AnkushinDaniil wants to merge 5 commits intomasterfrom
proof_getProofWithMeta JSON-RPC method#11498Conversation
Returns the standard EIP-1186 AccountProof plus per-call diagnostics (nodeLookups, cacheHits, maxDepth) for the same query. Provides a client-agnostic proxy for measuring how much work the EL does to serve a proof, useful for state-bloating benchmarks. The implementation is strictly additive: eth_getProof, AccountProofCollector, CachedTrieStore, the existing PatriciaTree.Accept and RunTreeVisitor overloads remain byte-identical. Diagnostics are reachable only when proof_getProofWithMeta is invoked, via a dedicated MeteredTrieNodeResolver decorator and a new AcceptMetered overload that wraps the resolver internally for the duration of one RPC call.
|
Claude finished @AnkushinDaniil's task in 5m 27s —— View job PR Review:
|
![claude[bot]](https://avatars.githubusercontent.com/in/1236702?s=60&v=4){kind=small}
- Refactor PatriciaTree.AcceptMetered to delegate to a private AcceptCore shared with Accept; eliminates the ~100 lines of duplication. - Make ProofDiagnostics counters thread-safe (Interlocked.Increment for long counters, CAS loop for MaxDepth) so the same instance can be shared across threads when MaxDegreeOfParallelism > 1. - Add XML doc comments on ProofDiagnostics, ProofMeta, AccountProofWithMeta, and IStateReader.RunTreeVisitorMetered. - Simplify ProofDiagnostics.CacheHits clamp to Math.Max with a remarks block explaining the invariant. - Add JsonRpcParameter attributes and ExampleResponse on proof_getProofWithMeta; document that storageKeys are deduplicated matching eth_getProof's existing HashSet<UInt256> shape. - Drop unused usings flagged by IDE0005 (Nethermind.Core in trie test, Nethermind.Evm in jsonrpc test) so Check code lint passes.
|
@claude re-review |
|
Claude finished @AnkushinDaniil's task in 4m 20s —— View job PR Re-review:
|
| # | Severity | Finding | Status |
|---|---|---|---|
| 1 | Medium | AcceptMetered duplicated ~100 lines from Accept |
✅ Resolved — AcceptCore private method introduced; both Accept and AcceptMetered delegate to it |
| 2 | Medium | ProofDiagnostics counters not thread-safe |
✅ Resolved — Interlocked.Increment, Interlocked.Read, Volatile.Read, and CAS loop in ObserveDepth |
| 3 | Medium | Missing XML docs on all new public APIs | ✅ Resolved — ProofDiagnostics, AccountProofWithMeta, ProofMeta, and IStateReader.RunTreeVisitorMetered all have complete <summary> / <remarks> / <param> |
| 4 | Low | CacheHits clamp code smell |
✅ Addressed — Math.Max(0, ...) + <remarks> documenting the invariant |
| 5 | Low | HashSet<UInt256> silently deduplicates |
✅ Addressed — method description and parameter description now document the deduplication |
Remaining Low finding
IBlockchainBridge.RunTreeVisitorMetered lacks XML documentation (IBlockchainBridge.cs:55). The coding style requires <summary> for all public API members. The existing RunTreeVisitor on the same interface also has no docs, so the omission is consistent with the file's current convention — this is a pre-existing pattern, not introduced by this PR. Consider adding a one-line <inheritdoc cref="IStateReader.RunTreeVisitorMetered{TCtx}"/> to both methods as a low-effort fix.
Positive notes
- The
AcceptCorerefactor is tight — the only difference betweenAcceptandAcceptMeteredcode paths is the null-check ondiagnosticsand the conditionalMeteredTrieNodeResolverwrap. No logic is duplicated. - Thread-safety in
ProofDiagnosticsis now formally correct and the XML<remarks>explain the DOP > 1 scenario clearly. ObserveDepthCAS loop is idiomatic and correct.- The
<remarks>onCacheHitsexplaining whyCacheMisses ≤ NodeLookupsholds is exactly the right place for that documentation. - All three
IStateReaderimplementations (StateReader,FlatStateReader,OverridableStateReader) correctly implementRunTreeVisitorMetered.
AnkushinDaniil
requested review from
LukaszRozmej,
asdacap,
benaadams,
damian-orzechowski,
flcl42,
smartprogrammer93 and
svlachakis
as code owners
|
Claude finished @AnkushinDaniil's task in 2m 40s —— View job PR Re-review:
|
| # | Severity | Finding | Status |
|---|---|---|---|
| 1 | Medium | AcceptMetered was a near-verbatim copy of Accept (DRY violation) |
✅ Fixed — both methods now delegate to a shared AcceptCore private method |
| 2 | Medium | ProofDiagnostics counters were not thread-safe |
✅ Fixed — Interlocked.Increment for _nodeLookups/_cacheMisses, CAS loop in ObserveDepth, Volatile.Read on MaxDepth |
| 3 | Medium | Missing XML documentation on all new public APIs | ✅ Fixed — ProofDiagnostics, ProofMeta, AccountProofWithMeta, and IStateReader.RunTreeVisitorMetered all have full <summary>/<param>/<returns>/<remarks> docs |
| 4 | Low | CacheHits defensive clamp signaled a fragile counting invariant |
✅ Fixed — uses Math.Max(0, NodeLookups - CacheMisses) with a <remarks> documenting the invariant |
| 5 | Low | HashSet<UInt256> silently deduplicates; missing [JsonRpcParameter] attributes and ExampleResponse |
✅ Addressed — deduplication is now explicitly documented in both the [JsonRpcMethod] description and the storageKeys parameter [JsonRpcParameter(Description = "...")]; all three params have ExampleValue; ExampleResponse is present |
Positive notes on the refactor
- The
AcceptCorefactoring is clean:Acceptbecomes a one-liner wrapper andAcceptMeteredadds a null-guard then delegates — future changes to the traversal body only need to be made in one place. ObserveDepth's CAS loop correctly handles the concurrent-update case; the logic is idiomatic and correct.- The documentation in
ProofDiagnosticsnow explains both the thread-safety guarantee and theCacheMisses ≤ NodeLookupsinvariant, which makes the class self-describing.
The PR is ready to merge.
AnkushinDaniil
changed the title
proof_getProofWithMeta JSON-RPC method
Per @LukaszRozmej's review, drop the separate AcceptMetered overload and the private AcceptCore helper. The single public Accept takes an optional ProofDiagnostics? as its last parameter; when non-null, the resolver is wrapped with MeteredTrieNodeResolver and per-call counters are populated during the traversal. Existing callers that pass only the first 2-5 parameters are unchanged (the new param is optional with a null default). The three metered call sites in StateReader, FlatStateReader, and FlatOverridableWorldScope now pass `diagnostics:` as a named argument.
Per @LukaszRozmej's review, apply the same collapse pattern at the IStateReader and IBlockchainBridge layers. Both interfaces now expose a single RunTreeVisitor with an optional `ProofDiagnostics? diagnostics` last parameter; when non-null, the underlying PatriciaTree.Accept wraps the resolver with metering and populates the counters. ProofRpcModule.proof_getProofWithMeta constructs a fresh ProofDiagnostics, passes it via the named argument, and reads the counters back after the visitor returns. Removed: IStateReader.RunTreeVisitorMetered, StateReader.*, FlatStateReader.*, FlatOverridableWorldScope.OverridableStateReader.*, IBlockchainBridge.RunTreeVisitorMetered, BlockchainBridge.*.
| /// (e.g. the <c>BatchedTrieVisitor</c> path). For the default proof-RPC code path the | ||
| /// traversal is single-threaded and the atomic ops are uncontended. | ||
| /// </remarks> | ||
| public sealed class ProofDiagnostics |
There was a problem hiding this comment.
Maybe rename to VisitingStats?
4 participants
Fixes NethermindEth/state-benchmarks#5
Changes
proof_getProofWithMeta(account, storageKeys, blockParameter)on the existingproof_*namespace, returningAccountProofWithMeta { Proof, Meta { NodeLookups, CacheHits, MaxDepth } }. Mirrorseth_getProofand adds per-call diagnostics:nodeLookups,cacheHits,maxDepth(in nibbles).Nethermind.Trie.ProofDiagnosticscarrier andMeteredTrieNodeResolver(a thinITrieNodeResolverdecorator) — observesFindCachedOrUnknown/LoadRlpand usesTreePath.Lengthfor depth.PatriciaTree.AcceptMeteredoverload; newIStateReader.RunTreeVisitorMeteredandIBlockchainBridge.RunTreeVisitorMetered.eth_getProof,AccountProofCollector,CachedTrieStore, the existingAccept/RunTreeVisitoroverloads, andITrieStoreDI registration are unchanged. Diagnostics are reachable only whenproof_getProofWithMetais invoked.Types of changes
What types of changes does your code introduce?
Testing
Requires testing
If yes, did you write tests?
Notes on testing
7 new unit tests added:
ProofRpcModuleMetaTests(4) andMeteredTrieNodeResolverTests(3). FullNethermind.Trie.Test(436),Nethermind.State.Test(777),Nethermind.Facade.Test(102), and existingProofRpcModuleTests(88) all pass with zero failures. FullNethermind.slnxRelease build is clean (0 warnings, 0 errors).Documentation
Requires documentation update
Requires explanation in Release Notes
proof_getProofWithMeta— new JSON-RPC method that mirrorseth_getProofand returns the same payload plus per-call diagnostics (nodeLookups,cacheHits,maxDepth). Useful as a client-agnostic proxy for measuring the work an EL does to serve a proof.eth_getProofand other existing endpoints are byte-identical to upstream.