Skip to content
New issue

Have a question about this project? Sign up for a free GitHub account to open an issue and contact its maintainers and the community.

By clicking “Sign up for GitHub”, you agree to our terms of service and privacy statement. We’ll occasionally send you account related emails.

Already on GitHub? Sign in to your account

Implement EIP 1186 #1180

Closed
wants to merge 8 commits into from
Closed

Implement EIP 1186 #1180

wants to merge 8 commits into from

Conversation

jochem-brouwer
Copy link
Member

@jochem-brouwer jochem-brouwer commented Mar 31, 2021

Closes #1078. Implements EIP1186

TODOs

  • Implement tests for StateManager
  • Implement RPC method in client
  • Add client tests
  • Figure out what to do when verifyProof from trie returns null (probably edge case; only happens on empty tries)

@codecov
Copy link

codecov bot commented Mar 31, 2021

Codecov Report

Merging #1180 (4adf776) into master (00fc597) will decrease coverage by 0.46%.
The diff coverage is n/a.

Impacted file tree graph

Flag Coverage Δ
block 82.21% <ø> (-0.04%) ⬇️
blockchain 84.16% <ø> (-0.07%) ⬇️
client 83.80% <ø> (-0.23%) ⬇️
common 87.39% <ø> (+0.52%) ⬆️
devp2p 83.67% <ø> (-0.25%) ⬇️
ethash 82.08% <ø> (ø)
tx 81.76% <ø> (-2.55%) ⬇️
vm 81.97% <ø> (-1.67%) ⬇️

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

@holgerd77
Copy link
Member

Didn't dig deeper here (sorry, didn't answer on your initial question on this yet), but do you know that this functionality exists?

@jochem-brouwer
Copy link
Member Author

Yep, this PR internally calls into it. The goal of this PR is to implement the EIP and to actually export the merkle proofs as specified in the EIP, and also open it as endpoint in the client.

Based upon the EIP, it might be that our current proofs are not valid (not sure yet).

@holgerd77
Copy link
Member

Rebased this.

@holgerd77
Copy link
Member

Ok, this has gotten some renewed interest with a concrete use case. 😄

Lion from ChainSafe asked in the #lightclient R&D Discord channel on this, functionality would be needed to complement to Lightclient functionality in Lodestar, here some extract from the chat:

To recap, the goal is to demo a minimum viable browser based light-client that shows recent user data (no execution and no p2p networking, view only)

Specifically a browser based app (either UI or extension) that communicates via HTTP with:

  • consensus client to get head + execution payload state root (dedicated REST endpoints on the beacon node, pending > standardization) This part will be covered by @chainsafe/lodestar-light-client
  • execution client to get proofs (with eth_getProof) for the user input address(es)

If EthereumJS full node is able to serve eth_getProof requests it could also be the backend for the execution part. But > the blocker so far is parsing the eth_getProof responses in the browser

@jochem-brouwer I took another look into the code base since I wanted to determine if we really need to expose this through the StateManager, since Lodestar (and likely other use cases as well) would only need the proof functionality (and not the whole VM). But I guess you are right, this is really pretty connected to the higher level StateManager functionality. Guess for now this should be ok to force Lodestar (or - as I just read - it will be a dedicated library for testing) to import the whole VM. Maybe we want to take this as some trigger though and more seriously move forward with extracting the StateManager from the VM (which is actually planned for a long time).

Independently from the TODO list above (would say we do not need to also implement the eth_getProof endpoint atm (we very well can though, then Lodestar can also use our client as a backend for the experiement): is this PR somwhat ready for merge?

I guess we want to integrate this in a way into the StateManager that we are not adding this functionality to the interface and also not use in the VM directly.

@jochem-brouwer jochem-brouwer mentioned this pull request Nov 29, 2021
4 tasks
@jochem-brouwer
Copy link
Member Author

Closing this in favor of #1590. Note: the name has changed since the EIP is stagnant and also not up to date, the goal is to implement the eth_getProof RPC endpoint in the client.

@holgerd77 holgerd77 deleted the getProof branch January 22, 2022 08:01
Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment
Projects
None yet
Development

Successfully merging this pull request may close these issues.

Trie: Add support for EIP-1186 (eth_getProof)
2 participants