Starknet's getStorageProof rpc method

[pnowosie]

Fixes #2180
Tests are based on proof-refactor PR

[codecov]

Codecov Report

Attention: Patch coverage is 75.43860% with 70 lines in your changes missing coverage. Please review.

Project coverage is 74.47%. Comparing base (eb1f2ca) to head (7e56c76).

Files with missing lines	Patch %	Lines
rpc/storage.go	87.76%	16 Missing and 7 partials ⚠️
core/trie/proofset.go	0.00%	22 Missing ⚠️
core/state.go	34.37%	15 Missing and 6 partials ⚠️
blockchain/blockchain.go	62.50%	2 Missing and 1 partial ⚠️
core/trie/node.go	66.66%	1 Missing ⚠️

Additional details and impacted files

@@            Coverage Diff             @@
##             main    #2194      +/-   ##
==========================================
+ Coverage   74.33%   74.47%   +0.14%     
==========================================
  Files         110      112       +2     
  Lines       11531    11785     +254     
==========================================
+ Hits         8571     8777     +206     
- Misses       2290     2330      +40     
- Partials      670      678       +8     

☔ View full report in Codecov by Sentry.
📢 Have feedback on the report? Share it here.

[pnowosie]

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]

please remove these debugging lines

[weiihann]

please remove these debugging lines

[weiihann]

why is this check needed?

[pnowosie]

I found a bug in proof-verification here in the test:

juno/rpc/storage_test.go

Line 166 in 7252e10

require.True(t, trie.VerifyProof(root, &kKey, value, proof, tempTrie.HashFunc()))

[weiihann]

I see, that actually doesn't fix the other test cases. It'll be clearer in my VerifyProof implementation.

[weiihann]

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:

1. Implements TrieReader interface and return HeadTrie in Blockchain Reader interface
2. 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.