Starknet's `getStorageProof` rpc method #2194

pnowosie · 2024-10-03T08:15:55Z

Fixes #2180
Tests are based on proof-refactor PR

codecov · 2024-10-03T08:24:11Z

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 ⚠️
rpc/storage.go	87.76%	16 Missing and 7 partials ⚠️
core/state.go	34.37%	15 Missing and 6 partials ⚠️
core/trie/key.go	47.36%	20 Missing ⚠️
blockchain/blockchain.go	62.50%	2 Missing and 1 partial ⚠️
core/trie/trie.go	81.81%	2 Missing ⚠️

Additional details and impacted files

@@            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

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

rpc/contract.go

core/trie/trie.go

core/state.go

rpc/storage_test.go

rpc/storage.go

core/trie/proof.go

rpc/storage.go

rpc/handlers.go

blockchain/pending.go

rpc/handlers.go

rpc/storage.go

starknet/.gitignore

blockchain/blockchain.go

rpc/storage_test.go

pnowosie · 2024-11-01T08:58:06Z

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 · 2024-11-01T09:54:00Z

core/trie/proof.go

+			proofHash := proofNode.Hash(hash)
+			fmt.Println("Proof verification failure", "node", proofHash, "expected", expectedHash)
+			fmt.Printf("%v\n", proofNode)


please remove these debugging lines

weiihann · 2024-11-01T09:54:13Z

core/trie/proof.go

@@ -345,7 +348,8 @@ func VerifyProof(root *felt.Felt, key *Key, value *felt.Felt, proofs []ProofNode
 				return true
 			}

-			if !proofNode.Path.Equal(subKey) {
+			if !proofNode.Path.Equal(subKey) && !subKey.Equal(&Key{}) {
+				fmt.Println("Proof verification failure", "node", proofNode.Path, "expected", subKey)


please remove these debugging lines

weiihann · 2024-11-01T09:54:34Z

core/trie/key.go

+	if n == k.len {
+		return &Key{}, nil
+	}


why is this check needed?

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()))

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

weiihann · 2024-11-01T10:29:29Z

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.

pnowosie · 2024-11-07T11:27:19Z

core/trie/proof.go

@@ -134,153 +165,6 @@ func transformNode(tri *Trie, parentKey *Key, sNode StorageNode) (*Edge, *Binary
 	return edge, binary, nil
 }

-// pathSplitOccurredCheck checks if there happens at most one split in the merged path


Note: These functions are reported unused by linter, hence I deleted them.
Maybe further PR #2252 will bring them back.

pnowosie · 2024-11-08T11:33:11Z

core/trie/proof.go

-// If the trie is constructed incorrectly then the root will have an incorrect key(len,path), and value,
-// and therefore it's hash won't match the expected root.
-// ref: https://github.com/ethereum/go-ethereum/blob/v1.14.3/trie/proof.go#L484
-func VerifyRangeProof(root *felt.Felt, keys, values []*felt.Felt, proofKeys [2]*Key, proofValues [2]*felt.Felt,


Will be added back as of #2252 with proper test coverage. It isn't used anywhere and hence ongoing proof refactor it is safe to drop for now.

pnowosie · 2024-11-12T08:24:27Z

core/trie/proof_test.go

@@ -183,113 +184,270 @@ func build3KeyTrie(t *testing.T) *trie.Trie {
 	return tempTrie
 }

-func build4KeyTrie(t *testing.T) *trie.Trie {


Some of these deleted functions will be bring back with #2252.
For this PR they are unused.

pnowosie force-pushed the pnowosie/2180-starknet-getProof-rpc branch from 301250f to d383531 Compare October 3, 2024 15:11

pnowosie commented Oct 3, 2024

View reviewed changes

rpc/contract.go Show resolved Hide resolved

pnowosie force-pushed the pnowosie/2180-starknet-getProof-rpc branch from 1ef09de to a33464e Compare October 10, 2024 07:58

pnowosie marked this pull request as ready for review October 10, 2024 07:59

pnowosie requested review from kirugan and rianhughes October 10, 2024 07:59