Starknet's `getStorageProof` rpc method

pnowosie commented 1 month ago

Fixes #2180 Tests are based on proof-refactor PR

codecov[bot] commented 1 month ago

Codecov Report

Attention: Patch coverage is 76.75000% with 93 lines in your changes missing coverage. Please review.

Project coverage is 74.46%. Comparing base (e29c217) to head (3af5c47).

Files with missing lines	Patch %	Lines
core/trie/proof.go	67.56%	19 Missing and 5 partials :warning:
rpc/storage.go	87.76%	16 Missing and 7 partials :warning:
core/state.go	34.37%	15 Missing and 6 partials :warning:
core/trie/key.go	47.36%	20 Missing :warning:
blockchain/blockchain.go	62.50%	2 Missing and 1 partial :warning:
core/trie/trie.go	81.81%	2 Missing :warning:

Additional details and impacted files

```diff @@ Coverage Diff @@ ## main #2194 +/- ## ========================================== - Coverage 75.31% 74.46% -0.85% ========================================== Files 106 108 +2 Lines 11237 11388 +151 ========================================== + Hits 8463 8480 +17 - Misses 2135 2286 +151 + Partials 639 622 -17 ```

:umbrella: View full report in Codecov by Sentry.
:loudspeaker: Have feedback on the report? Share it here.

weiihann commented 3 weeks ago

I don't think it's a good idea to add HeadTrie method to the blockchain's Reader interface for several reasons:

There's a clear hierarchical relationship where:

Blockchain contains State
State internally uses Trie for implementation
Blockchain shouldn't need to know about these implementation details of State

The current HeadTrie() implementation is almost identical to the HeadState(), which suggests this abstraction isn't providing additional value.

Instead of having a separate TrieReader interface, I'd suggest:

Remove the TrieReader interface entirely
Move the trie-related methods directly into StateReader
This better reflects that trie operations are a capability of the state, not the blockchain

The problem with this approach is that we can only support trie operations on the latest state. Historical trie access (via PendingState and stateSnapshot) is not allowed. I think that's fine since the RPC method will return an error if the block ID is not latest

pnowosie commented 3 weeks ago

I don't think it's a good idea to add HeadTrie method to the blockchain's Reader interface for several reasons: ... Move the trie-related methods directly into StateReader

@weiihann Thanks for this comment. This was my initial implementation - trie related methods was added to the StateReader. Then I spotted a few problems which were the need to implement these methods for all StateReaders (where it would be possible only for the (current) State not historical ones. The another thing was that we only mock the StateHistoryReader - and there was something related to it in tests I cannot recall right now.

I discussed this issue with @kirugan and we agreed that we address this once we allow juno to prove historical states

pnowosie commented 3 weeks ago

Differences / Issues spotted comparing with PF

1. Params are not optional (why?)

Request

{
  "jsonrpc": "2.0",
  "method": "starknet_getStorageProof",
  "params": [
    "latest",
    ["0x1adadae7ce7ed3f5c39f275ef4758562e540f27ab0184b24e2b13861988751e"]
  ],
  "id": 1
}

Response "message": "Invalid Params",

2. Response does not filter out duplicate nodes in hash HashToNode mapping

Request

{
  "jsonrpc": "2.0",
  "method": "starknet_getStorageProof",
  "params": [
    "latest",
    ["0x1adadae7ce7ed3f5c39f275ef4758562e540f27ab0184b24e2b13861988751e", 
     "0x1adadae7ce7ed3f5c39f275ef4758562e540f27ab0184b24e2b13861988751e"],
    [],
    []
  ],
  "id": 1
}

Response

{
  "jsonrpc": "2.0",
  "result": {
    "classes_proof": [
      {
        "node_hash": "0x217d3fbc3bccaca79f86feb5e9dbbfa531ab481503594df9ca426245a65a12f",
        "node": { ... }
      },
// ...
      {
        "node_hash": "0x1c2825874f9b479403ea627428397852d86c67d955be072d68de143a98bb923",
        "node": { ...
          "child": "0x35f5f9b5523cdbd190f2d7a7310aadcfcf0231a9a580786b9cd5db8b512de2"
        }
      },
      {
        "node_hash": "0x217d3fbc3bccaca79f86feb5e9dbbfa531ab481503594df9ca426245a65a12f",
         "node": { ... }
      },
// ...
      {
        "node_hash": "0x1c2825874f9b479403ea627428397852d86c67d955be072d68de143a98bb923",
        "node": { ...
          "child": "0x35f5f9b5523cdbd190f2d7a7310aadcfcf0231a9a580786b9cd5db8b512de2"
        }
      }
    ],
  // ...
  "id": 1
}

weiihann commented 3 weeks ago

I discussed this issue with @kirugan and we agreed that we address this once we allow juno to prove historical states

So there are 2 approaches we can take:

Implements TrieReader interface and return HeadTrie in Blockchain Reader interface
Add Trie access methods in StateReader interface

For the 1st approach, we can agree on the architecture such that a Blockchain Reader shouldn't know the underlying State implementation (i.e. Trie). Adding HeadTrie method to Reader indicates we are coupling Trie and Blockchain together, which shouldn't happen. So essentially this is a technical debt.

For the 2nd approach, the architecture makes sense, the issue is that we have an unsupported feature for historical trie access. It's unsure when we will support historical trie access as it's a huge refactor on the state, trie and db modules.

I'd rather go with an unsupported feature and detect it early rather than incur technical debts and make the modules even more coupled and harder to refactor.

NethermindEth / juno