feat(rpc): add partial `eth_getProof` implementation by MegaRedHand · Pull Request #1515 · paradigmxyz/reth

MegaRedHand · 2023-02-22T18:56:59Z

Closes: #1507

This PR adds the implementation for the eth_getProof RPC call, along with additional methods in DBTrieLoader for generating Merkle proofs.

The stages crate depends on the provider crate. As this struct was needed for further development of the eth_getProof endpoint, it was moved down on the dependency chain. Maybe it could be in its own crate. Co-authored-by: lambdaclass-user <github@lambdaclass.com>

Co-authored-by: lambdaclass-user <github@lambdaclass.com>

codecov-commenter · 2023-02-22T19:15:39Z

Codecov Report

Merging #1515 (5c63be8) into main (043098a) will decrease coverage by 30.82%.
The diff coverage is 4.04%.

📣 This organization is not using Codecov’s GitHub App Integration. We recommend you install it so Codecov can continue to function properly for your repositories. Learn more

@@             Coverage Diff             @@
##             main    #1515       +/-   ##
===========================================
- Coverage   72.95%   42.13%   -30.82%     
===========================================
  Files         391      391               
  Lines       46575    46869      +294     
===========================================
- Hits        33978    19749    -14229     
- Misses      12597    27120    +14523

Flag	Coverage Δ
integration-tests	`16.88% <0.00%> (-3.48%)`	⬇️
unit-tests	`35.49% <4.04%> (-32.24%)`	⬇️

Flags with carried forward coverage won't be shown. Click here to find out more.

Impacted Files	Coverage Δ
crates/executor/src/executor.rs	`65.53% <0.00%> (-28.44%)`	⬇️
crates/interfaces/src/provider.rs	`0.00% <ø> (ø)`
crates/primitives/src/account.rs	`12.50% <0.00%> (-75.38%)`	⬇️
crates/rpc/rpc/src/eth/api/call.rs	`0.00% <0.00%> (ø)`
crates/rpc/rpc/src/eth/api/mod.rs	`17.77% <0.00%> (-46.60%)`	⬇️
crates/rpc/rpc/src/eth/api/server.rs	`0.00% <0.00%> (-95.43%)`	⬇️
crates/rpc/rpc/src/eth/api/state.rs	`0.00% <0.00%> (-100.00%)`	⬇️
crates/storage/provider/src/lib.rs	`100.00% <ø> (ø)`
...storage/provider/src/providers/state/historical.rs	`0.00% <0.00%> (-92.00%)`	⬇️
...tes/storage/provider/src/providers/state/latest.rs	`5.17% <0.00%> (-52.90%)`	⬇️
... and 7 more

... and 197 files with indirect coverage changes

Help us with your feedback. Take ten seconds to tell us how you rate us. Have a feature suggestion? Share it here.

Co-authored-by: lambdaclass-user <github@lambdaclass.com>

MegaRedHand · 2023-02-22T21:08:21Z

@mattsse
I'm having a bit of trouble with this. As it stands, the HistoricalStateProvider would need to unwind the hashed account and storage tables to the past state before updating the trie and generating the proof. All of this would be really expensive and needs to be done while leaving the current tables untouched. I was thinking of just doing this in the transaction without committing (so changes are dropped), but this wouldn't work as it is read-only. Do you know of something that could be used for this?

On another note, the LatestStateProvider doesn't seem to be used aside from on tests.

gakonst · 2023-02-22T21:16:13Z

cc @joshieDo given you've been looking at Merkle stage and patricia tree in general

joshieDo · 2023-02-23T07:48:54Z

My impression is that we need to change the way we insert into the trie - and use a cursor instead (included that remark in the other PR). this would allow us to have more control over what we commit.

For this kind of unwinding, we have to "force" some hashing stages to use their incremental branch instead of batched/scratch which don't commit the db tx iirc

rakita · 2023-02-23T12:09:53Z

There could be only one write tx, this could block other write transactions.

Previously only HistorySP was returned and we didn't use LatestSP at all. Co-authored-by: lambdaclass-user <github@lambdaclass.com>

Co-authored-by: lambdaclass-user <github@lambdaclass.com>

MegaRedHand · 2023-02-23T19:29:20Z

Update: I left the implementation for the latest state provider only. If the block requested isn't the latest, it returns a "not implemented" error via RPC. As part of this, I modified the state access helpers to allow returning any of both the latest and history state providers.

I've noticed that geth uses multiple databases for caching previous states, and use this for the endpoint backend. Other implementations seem to have partial (only for the latest or cached) or no support for the endpoint.

My impression is that we need to change the way we insert into the trie - and use a cursor instead

Because of limitations with cita_trie and the need for cursor mutability, this would require a Mutex<RefCell<_>> inside the database wrapper. I can add this if it seems like a good enough tradeoff, but maybe we should first add the stage benchmarks and check it doesn't drop performance.

For this kind of unwinding, we have to "force" some hashing stages to use their incremental branch instead of batched/scratch which don't commit the db tx iirc

That's right for the execute part, but the unwind doesn't commit changes. Also, the transaction given to the provider is read-only so we can't make changes to the DB and we can't make this RW because of what rakita said.

Co-authored-by: lambdaclass-user <github@lambdaclass.com>

joshieDo · 2023-02-24T06:12:55Z

blocked by #1497

Also, fix: was hashing received and already hashed address Co-authored-by: lambdaclass-user <github@lambdaclass.com>

Co-authored-by: lambdaclass-user <github@lambdaclass.com>

gakonst · 2023-03-01T20:46:49Z

Needs merge conflict post #1474, sorry about that.

MegaRedHand · 2023-03-01T21:06:37Z

Fixed!

gakonst

good work - mainly nits

gakonst · 2023-03-09T23:33:08Z

crates/rpc/rpc/src/eth/api/mod.rs

+// Transparent wrapper to enable state access helpers
+// returning latest state provider when appropiate
+pub(crate) enum SP<'a, H, L> {
+    History(H),
+    Latest(L),
+    _Unreachable(&'a ()), // like a PhantomData for 'a


Let's please avoid really short names like this

Yes. I was having a bit of trouble coming up with a fitting name. I was thinking of StateProviderType, or maybe GenericSP. wdyt?

I would call it StateProvider and do use xyz::StateProvider as StateProviderTrait

gakonst · 2023-03-09T23:34:30Z

crates/rpc/rpc/src/eth/api/server.rs

+        let res = EthApi::get_proof(self, address, keys, block_number);
+
+        Ok(res.map_err(|e| match e {
+            EthApiError::InvalidBlockRange => internal_rpc_err("unimplemented"),


Suggested change

EthApiError::InvalidBlockRange => internal_rpc_err("unimplemented"),

EthApiError::InvalidBlockRange => internal_rpc_err("eth_getProof is unimplemented for historical blocks"),

gakonst · 2023-03-09T23:35:15Z

crates/rpc/rpc/src/eth/api/mod.rs

        &self,
        block_id: Option<BlockId>,
-    ) -> Result<Option<<Client as StateProviderFactory>::HistorySP<'_>>> {
+    ) -> Result<Option<HistoryOrLatest<'_, Client>>> {


Why is this change needed? latest_state() did the same before, no?

Yes, but this way the consumer of the Factory doesn't need to explicitly use LatestSP, which would need an additional check so he knows if the BlockId received references the latest block of the chain or not. This check wasn't being done before, and we were always using the HistorySP by default.

gakonst · 2023-03-09T23:40:18Z

crates/rpc/rpc/src/eth/api/state.rs

+            EIP1186AccountProofResponse { address, code_hash: KECCAK_EMPTY, ..Default::default() };
+
+        if let Some(account) = state.basic_account(address)? {
+            let (account_proof, storage_hash, stg_proofs) = state.proof(address, keys.clone())?;


Suggested change

let (account_proof, storage_hash, stg_proofs) = state.proof(address, keys.clone())?;

let (account_proof, storage_hash, stg_proofs) = state.proof(address, &keys)?;

gakonst · 2023-03-09T23:40:33Z

crates/storage/provider/src/providers/state/historical.rs

+    fn proof(
+        &self,
+        _address: Address,
+        _keys: Vec<H256>,


Suggested change

_keys: Vec<H256>,

_keys: &[H256],

can't do like this?

Yes, you're right.

gakonst · 2023-03-09T23:41:14Z

crates/storage/provider/src/providers/state/latest.rs

+            .ok_or(ProviderError::Header { number: 0 })?
+            .1
+            .state_root;
+        let (acc_proof, stg_root) = loader


let's use full words, storage_proof, account_proof etc.

gakonst · 2023-03-09T23:43:45Z

crates/storage/provider/src/trie/mod.rs

+    fn get(&self, key: &[u8]) -> Result<Option<Vec<u8>>, Self::Error> {
+        let mut cursor = self.tx.cursor_dup_read::<tables::StoragesTrie>()?;
+        Ok(cursor.seek_by_key_subkey(self.key, H256::from_slice(key))?.map(|entry| entry.node))
+    }


cc @joshieDo @onbjerg

the reason why I don't like the Cita Trie lib is because it doesn't let us reuse our cursors when we're walking thru the entire table, feels a bit gross to re-instantiate a cursor on every get

gakonst · 2023-03-09T23:45:55Z

crates/storage/provider/src/trie/mod.rs

+    pub fn generate_acount_proof<'tx, 'itx>(
+        &self,
+        tx: &'tx impl DbTx<'itx>,
+        root: H256,


This is a weird API due to requiring the root to be passed by the user every time. Can we avoid that somehow?

Maybe we can make the DBTrieLoader receive the current root on instantiation in a similar way to the database wrappers we use inside the module (i.e. with new(root) and empty() methods).

Yeah I like that.

Opened #1743. Should be a small change, and a good first issue.

gakonst · 2023-03-09T23:46:26Z

crates/storage/provider/src/trie/mod.rs

+        storage_root: H256,
+        address: H256,
+        keys: Vec<H256>,
+    ) -> Result<Vec<Vec<Vec<u8>>>, TrieError> {


Vec<Vec<Vec<_>>> looks weird, can we introduce a new type for this?

Added a type alias for Vec<Vec<u8>> named MerkleProof. That should make this less weird (now Vec<MerkleProof>).

gakonst · 2023-03-09T23:47:15Z

crates/storage/provider/src/trie/mod.rs

+    #[test]
+    fn get_proof() {


let's please add a test for something that has multiple storage slots as well

Co-authored-by: lambdaclass-user <github@lambdaclass.com>

gakonst

Good from me - would like @joshieDo to approve before merging, cc @mattsse given proximity to RPC-related code

gakonst · 2023-03-10T23:54:33Z

crates/rpc/rpc/src/eth/api/mod.rs

+// Transparent wrapper to enable state access helpers
+// returning latest state provider when appropiate
+pub(crate) enum SP<'a, H, L> {
+    History(H),
+    Latest(L),
+    _Unreachable(&'a ()), // like a PhantomData for 'a


I would call it StateProvider and do use xyz::StateProvider as StateProviderTrait

gakonst · 2023-03-10T23:57:08Z

crates/rpc/rpc/src/eth/api/state.rs

+        let mut proof =
+            EIP1186AccountProofResponse { address, code_hash: KECCAK_EMPTY, ..Default::default() };


Does Geth return an empty proof like this when an account that is not found is requested?

No. It returns empty storage proofs when the account doesn't exist. I misread the docs 😅

gakonst · 2023-03-11T00:04:36Z

crates/storage/provider/src/trie/mod.rs

+    pub fn generate_acount_proof<'tx, 'itx>(
+        &self,
+        tx: &'tx impl DbTx<'itx>,
+        root: H256,


Yeah I like that.

Co-authored-by: lambdaclass-user <github@lambdaclass.com>

gakonst

OK - seems fine at this point. Let's merge and we can iterate if needed.

MegaRedHand and others added 5 commits February 22, 2023 15:13

Add DBTrieLoader proof generation functions

61501a3

Co-authored-by: lambdaclass-user <github@lambdaclass.com>

Add endpoint and provider impl for latest

43a9c81

Co-authored-by: lambdaclass-user <github@lambdaclass.com>

Make generate_storage_proofs public

cdee89e

Co-authored-by: lambdaclass-user <github@lambdaclass.com>

Add get_proof as implemented in test

e608ea6

Co-authored-by: lambdaclass-user <github@lambdaclass.com>

Add proof mock for historical provider

448ef51

Co-authored-by: lambdaclass-user <github@lambdaclass.com>

MegaRedHand and others added 3 commits February 23, 2023 15:15

Change return type of state access helpers

3a6ef90

Previously only HistorySP was returned and we didn't use LatestSP at all. Co-authored-by: lambdaclass-user <github@lambdaclass.com>

Return unimplemented error when block isn't latest

83d4a48

Co-authored-by: lambdaclass-user <github@lambdaclass.com>

Merge branch 'main' into rpc-eth-getproof

55bf8c0

MegaRedHand marked this pull request as ready for review February 23, 2023 20:49

MegaRedHand requested review from gakonst, joshieDo, onbjerg, rakita and rkrasiuk as code owners February 23, 2023 20:49

Add comment

7a9d99f

Co-authored-by: lambdaclass-user <github@lambdaclass.com>

MegaRedHand changed the title ~~feat(rpc): add eth_getProof implementation~~ feat(rpc): add partial eth_getProof implementation Feb 24, 2023

MegaRedHand and others added 2 commits February 24, 2023 17:11

Add test for generating account proof

3ee2483

Also, fix: was hashing received and already hashed address Co-authored-by: lambdaclass-user <github@lambdaclass.com>

Merge branch 'main' into rpc-eth-getproof

2808260

onbjerg added C-enhancement New feature or request A-rpc Related to the RPC implementation labels Feb 27, 2023

MegaRedHand and others added 2 commits March 1, 2023 10:24

Merge branch 'main' into rpc-eth-getproof

e26a3e7

Revert Cargo.lock

c95ad62

Co-authored-by: lambdaclass-user <github@lambdaclass.com>

Merge branch 'main' into rpc-eth-getproof

2a16068

Merge branch 'main' into rpc-eth-getproof

abe8a34

gakonst requested changes Mar 9, 2023

View reviewed changes

MegaRedHand and others added 6 commits March 10, 2023 11:05

Change unimplemented message and variable names

b3addcb

Co-authored-by: lambdaclass-user <github@lambdaclass.com>

Receive keys as slice of hashes

9187b20

Co-authored-by: lambdaclass-user <github@lambdaclass.com>

Merge branch 'main' into rpc-eth-getproof

166535b

Alias Vec<Vec<u8>> to MerkleProof in trie

3c30735

Co-authored-by: lambdaclass-user <github@lambdaclass.com>

Add proof test with storage

6799d82

Co-authored-by: lambdaclass-user <github@lambdaclass.com>

Fix: hash keys before generating storage proofs

eaf8c5e

Co-authored-by: lambdaclass-user <github@lambdaclass.com>

gakonst approved these changes Mar 11, 2023

View reviewed changes

MegaRedHand and others added 3 commits March 13, 2023 10:25

Rename SP -> StateProvider

c8aadea

Co-authored-by: lambdaclass-user <github@lambdaclass.com>

Return non-existence proof

b29366d

Co-authored-by: lambdaclass-user <github@lambdaclass.com>

Merge branch 'main' into rpc-eth-getproof

5c63be8

MegaRedHand mentioned this pull request Mar 13, 2023

Make DBTrieLoader receive the current root on constructor #1743

Closed

gakonst approved these changes Mar 14, 2023

View reviewed changes

gakonst merged commit 54aab53 into paradigmxyz:main Mar 14, 2023

gakonst deleted the rpc-eth-getproof branch March 14, 2023 00:49

gakonst mentioned this pull request Mar 14, 2023

fix(stages): add commit threshold to merkle stage v2 #1656

Merged

	EthApiError::InvalidBlockRange => internal_rpc_err("unimplemented"),
	EthApiError::InvalidBlockRange => internal_rpc_err("eth_getProof is unimplemented for historical blocks"),

	let (account_proof, storage_hash, stg_proofs) = state.proof(address, keys.clone())?;
	let (account_proof, storage_hash, stg_proofs) = state.proof(address, &keys)?;

		let mut proof =
		EIP1186AccountProofResponse { address, code_hash: KECCAK_EMPTY, ..Default::default() };

Conversation

MegaRedHand commented Feb 22, 2023

Uh oh!

codecov-commenter commented Feb 22, 2023 • edited Loading Uh oh! There was an error while loading. Please reload this page.

Uh oh!

Codecov Report

Uh oh!

MegaRedHand commented Feb 22, 2023

Uh oh!

gakonst commented Feb 22, 2023

Uh oh!

joshieDo commented Feb 23, 2023

Uh oh!

rakita commented Feb 23, 2023

Uh oh!

MegaRedHand commented Feb 23, 2023

Uh oh!

joshieDo commented Feb 24, 2023

Uh oh!

gakonst commented Mar 1, 2023

Uh oh!

MegaRedHand commented Mar 1, 2023

Uh oh!

gakonst left a comment

Choose a reason for hiding this comment

Uh oh!

Choose a reason for hiding this comment

Uh oh!

Choose a reason for hiding this comment

Uh oh!

Choose a reason for hiding this comment

Uh oh!

Choose a reason for hiding this comment

Uh oh!

Choose a reason for hiding this comment

Uh oh!

Choose a reason for hiding this comment

Uh oh!

Choose a reason for hiding this comment

Uh oh!

Choose a reason for hiding this comment

Uh oh!

Choose a reason for hiding this comment

Uh oh!

Choose a reason for hiding this comment

Uh oh!

Choose a reason for hiding this comment

Uh oh!

Choose a reason for hiding this comment

Uh oh!

Choose a reason for hiding this comment

Uh oh!

Choose a reason for hiding this comment

Uh oh!

Choose a reason for hiding this comment

Uh oh!

Choose a reason for hiding this comment

Uh oh!

MegaRedHand Mar 10, 2023 • edited Loading Uh oh! There was an error while loading. Please reload this page.

Uh oh!

Choose a reason for hiding this comment

Uh oh!

Choose a reason for hiding this comment

Uh oh!

Choose a reason for hiding this comment

Uh oh!

gakonst left a comment

Choose a reason for hiding this comment

Uh oh!

Choose a reason for hiding this comment

Uh oh!

Choose a reason for hiding this comment

Uh oh!

Choose a reason for hiding this comment

Uh oh!

Choose a reason for hiding this comment

Uh oh!

gakonst left a comment

Choose a reason for hiding this comment

Uh oh!

codecov-commenter commented Feb 22, 2023 •

edited

Loading

MegaRedHand Mar 10, 2023 •

edited

Loading