Implement eth_getProof #1590
Implement eth_getProof #1590
Conversation
|
Dumping an example proof here for ropsten. |
Codecov Report
Flags with carried forward coverage won't be shown. Click here to find out more. |
|
Some notes: the first item of this proof is the first node (somewhat expected), so It looks like something fishy is going on in our MPT Non-existing account proof (Same block number / state root) |
| const value = Buffer.from('0000aabb00', 'hex') | ||
| const code = Buffer.from('6000', 'hex') | ||
| const stateManager = new DefaultStateManager() | ||
| await stateManager.checkpoint() |
There was a problem hiding this comment.
Note: without checkpoint/commit the cache is not flushed which means that the MPT cannot find the right path for the requested account (this should be addressed, ensure that cache is flushed before trying to create a proof)
There was a problem hiding this comment.
I guess you can just call into cache.flush() in the new proof functions? 🤔
There was a problem hiding this comment.
Ah yes that should work and makes more sense.
There was a problem hiding this comment.
in normal vm operation the stateManager should checkpoint and commit after every block, so we should never run into a scenario where this occurs, correct? moreso just a spec test setup particularity? just wanted to make sure before i resolved this conversation
|
Note: have renamed this from EIP 1186 to Implement eth_getProof. The EIP is stagnant and outdated. I have included 3 tests taken from Geth 1.10.8 and ensured that the returned objects by us are equal. There are 2 things which I noticed: (1) there is a field I also noticed that Geth does not enforce 32 bytes as keys, but just left-pads it with zeros if you dont input 32 bytes. You also can input a key like This PR is not entirely dedicated to create a full blown copy of Geths return values but it is good to be aware of these things. |
|
Note to self: Trie |
|
EDIT: this functionality already exists 😅 |
|
Hmm weird our Trie does not like the Geth proofs, throws invalid proof 🤔 We might have a bug in Trie's |
stateManager: fix storage slot proofs
|
Great, account proofs, non-existing account proofs, storage proofs and non-existing storage proofs should now work. |
packages/vm/src/state/interface.ts
Outdated
| @@ -42,4 +43,6 @@ export interface EIP2929StateManager extends StateManager { | |||
| isWarmedStorage(address: Buffer, slot: Buffer): boolean | |||
| clearWarmedAccounts(): void | |||
| generateAccessList?(addressesRemoved: Address[], addressesOnlyStorage: Address[]): AccessList | |||
| getProof?(address: Address, storageSlots: Buffer[]): Promise<Proof> | |||
| verifyProof?(proof: Proof): Promise<boolean> | |||
There was a problem hiding this comment.
Have thought about the API of this a bit. For now I would make a pledge to not add this to the StateManager interface at all but rather treat this as some custom functionality we add to our own StateManager implementation.
The functionality is not used in the VM at all and this will rather confuse people and make life harder for implementers who want to do custom state manager implementations, if we bloat the interface more and more.
Instead we should add a dev comment (so: no public code function comment but rather only visible within the code) to both functions like:
/*
* This function is currently not used within the VM but exists as a utility function for people
* who want to use the StateManager out of the VM context. It is therefore not part of the
* StateManager interface.
*
* If at some point it gets necessary to use VM internally make sure add to the StateManager interface.
*/Does this make sense?
There was a problem hiding this comment.
Yes, this makes sense. We should also create a standalone StateManager package, see #1599.
There was a problem hiding this comment.
Do you think we should create an extra "type" of StateManager which has these proof methods? Like we did with EIP2929 state manager - the state manager with proofs would inherit the EIP2929 functions.
|
What's the status of this? |
|
What is left here is implementing the RPC methods at the client side, which should be done by this week. |
client: add tests for getProof
* bolster invalid proof error messages * extract ProofStateManager tests to own file * move testdata files to own folder and use import syntax for improved type info
* already have array validator (usage: `validators.array(validators.hex)`) * add typedoc for getProof (thanks @gabrocheleau) * update getProof.spec.ts to match future test setup from PR #1598 (this slightly modifies the returned accountProof)
|
beautiful PR @jochem-brouwer! super clean, and easy to follow 🤩 changes from my review are noted in the two added commit descriptions. will approve when ci passes, if everything looks good feel free to merge @jochem-brouwer! Then I will update #1598 to include getProof. |
|
@ryanio Thanks for the updates :) @gabrocheleau Thanks for the review! :) Merged! 🎉 |
Closes #1078. Implements
eth_getProofbased on comparing results with Geth 1.10.8. Rebased version of #1180.TODOs
verifyProoffromtriereturnsnull(probably edge case; only happens on empty tries)Note: if
verifyProofreturnsnull, it simply means that the key does not exist in the trie (a non-existing account, or an empty storage slot).