Skip to content
This repository has been archived by the owner on Nov 6, 2020. It is now read-only.

EIP-1186: add eth_getProof RPC-Method #9001

Merged
merged 100 commits into from
Nov 21, 2018
Merged

Conversation

simon-jentzsch
Copy link
Contributor

@simon-jentzsch simon-jentzsch commented Jun 28, 2018

Implementation for the EIP-1186

Ethereum uses MerkleTrees to store the state of accounts and their storage. This allows verification of each value by simply creating a MerkleProof. But currently the eth-Module does not give you access to these proofs. This EIP suggests a additional RPC-Method, which creates MerkleProofs for Accounts and Storage-Values.

Combined with a stateRoot (from the blockheader) it enables offline verification of any account or storage-value. This allows especially IOT-Devices or even mobile apps which are not able to run a light client to verify responses from a untrusted source.

@parity-cla-bot
Copy link

It looks like @simon-jentzsch hasn't signed our Contributor License Agreement, yet.

The purpose of a CLA is to ensure that the guardian of a project's outputs has the necessary ownership or grants of rights over all contributions to allow them to distribute under the chosen licence.
Wikipedia

You can read and sign our full Contributor License Agreement at the following URL: https://cla.parity.io

Once you've signed, please reply to this thread with [clabot:check] to prove it.

Many thanks,

Parity Technologies CLA Bot

@5chdn 5chdn added A4-clasignoffneeded 📛 Pull request requires author to sign off on CLA before review/merge can begin. M6-rpcapi 📣 RPC API. labels Jun 28, 2018
@5chdn 5chdn added this to the 1.12 milestone Jun 28, 2018
@5chdn
Copy link
Contributor

5chdn commented Jun 28, 2018

Hey, please sign our CLA.

This EIP suggests an additional RPC-Method, which creates MerkleProofs for Accounts and Storage-Values.

Which EIP are you referring to? Is this officially part of the eth namespace?

5chdn
5chdn previously requested changes Jun 28, 2018
docker/hub/Dockerfile Outdated Show resolved Hide resolved
ethcore/src/state/account.rs Outdated Show resolved Hide resolved
@simon-jentzsch
Copy link
Contributor Author

Thanks for the responses.
This referrs to the ethereum/EIPs#1186 we just started.
I'll check the CLA und of course I'll also fix the changes we made for our docker-script.

@5chdn 5chdn added the A3-inprogress ⏳ Pull request is in progress. No review needed at this stage. label Jun 28, 2018
@5chdn
Copy link
Contributor

5chdn commented Jun 28, 2018

Ah, gotcha. I'll mark this WIP.

@5chdn 5chdn changed the title add eth_getProof RPC-Method EIP-1186: add eth_getProof RPC-Method Jun 28, 2018
rpc/src/v1/impls/eth.rs Outdated Show resolved Hide resolved
rpc/src/v1/impls/eth.rs Outdated Show resolved Hide resolved
rpc/src/v1/impls/light/eth.rs Show resolved Hide resolved
@dvdplm
Copy link
Collaborator

dvdplm commented Jun 29, 2018

The CI failure is spurious here and should be fixed if you pull in the latest changes from master.

@simon-jentzsch
Copy link
Contributor Author

[clabot:check]

@parity-cla-bot
Copy link

It looks like @simon-jentzsch signed our Contributor License Agreement. 👍

Many thanks,

Parity Technologies CLA Bot

Copy link
Contributor Author

@simon-jentzsch simon-jentzsch left a comment

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

fixed and merged master into pull-request

@5chdn 5chdn removed the A4-clasignoffneeded 📛 Pull request requires author to sign off on CLA before review/merge can begin. label Jun 29, 2018
@5chdn 5chdn added the A1-onice 🌨 Pull request is reviewed well, but should not yet be merged. label Jul 2, 2018
@5chdn
Copy link
Contributor

5chdn commented Jul 2, 2018

Putting this on ice since you have chosen the eth_ namespace. This does not allow us to adapt this until the EIP is either accepted or a majority of clients implement it. 🐧

@simon-jentzsch
Copy link
Contributor Author

Thanks, Yes, I also considered using a different namespace. In the end I believe that offering the proof as a rpc-function should be part of the standard-module. That's why I choose the eth_ namespace, on the other hand it would be easier to have this function in one of the next releases, if this is a part of the parity -module. But in the long run I want all clients to support this. But I'm open for discussion. And until we get this feature in, we are using the forked-version.

@5chdn
Copy link
Contributor

5chdn commented Jul 9, 2018

Ok, let me know how you want to proceed.

@5chdn 5chdn closed this Jul 9, 2018
@sorpaas
Copy link
Collaborator

sorpaas commented Jul 19, 2018

@simon-jentzsch We can use the parity_ namespace first and then move it to eth_ namespace once it's standardized. This is how eth_chainId/parity_chainId was dealt with (ethereum/EIPs#695).

@simon-jentzsch
Copy link
Contributor Author

simon-jentzsch commented Sep 21, 2018

@sorpaas Yes, this would be an option to use the parity-namespace, but since it is also important that geth will implement it as well, I rather have it as a real standard. For now we can live with using our fork of parity. I am also maintaining Docker-Images https://hub.docker.com/r/slockit/parity-in3/tags/ which we are using. But I just hope the EIP will go through soon. ;-)

@simon-jentzsch
Copy link
Contributor Author

@5chdn I created the PR for the EIP and hope that it will go through. I also merged the changes for parity 2.x and created a Docker-Container on dockerhub. So I think it would be good,if we can repopen this PR.

@simon-jentzsch
Copy link
Contributor Author

@5chdn Would be good to reopen this pull request, since the EIP is now merged and a offical Draft: https://eips.ethereum.org/EIPS/eip-1186

@5chdn 5chdn reopened this Oct 18, 2018
Copy link
Collaborator

@niklasad1 niklasad1 left a comment

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

Looks good now, however still some formatting issues on some struct fields, should be have the format field: val

Also, would be good to have tests for this too!

@niklasad1 niklasad1 added A6-mustntgrumble 💦 Pull request has areas for improvement. The author need not address them before merging. and removed A5-grumble 🔥 Pull request has minor issues that must be addressed before merging. labels Nov 8, 2018
@Tbaut
Copy link
Contributor

Tbaut commented Nov 8, 2018

Could you please update the doc in src/interfaces/eth.js interfaces https://github.com/parity-js/jsonrpc

@simon-jentzsch
Copy link
Contributor Author

@niklasad1 I fixed the formatting of the structs and also added tests.
@Tbaut Added eth_getProof at the doc. ( See PR parity-js/jsonrpc#14 )

Copy link
Collaborator

@ordian ordian left a comment

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

Looks good, thank you. I guess merging this with eth_ namespace is fine, since EIP-1186 was accepted and implemented in geth as well (cc @tomusdrw).

@ordian ordian added A8-looksgood 🦄 Pull request is reviewed well. and removed A6-mustntgrumble 💦 Pull request has areas for improvement. The author need not address them before merging. labels Nov 8, 2018
@simon-jentzsch
Copy link
Contributor Author

Thanks, I'm looking forward to see this merged!

@tomusdrw
Copy link
Collaborator

IMHO this should be enabled only with --jsonrpc-experimental, see #9928

@5chdn 5chdn added A7-looksgoodtestsfail 🤖 Pull request is reviewed well, but cannot be merged due to tests failing. and removed A8-looksgood 🦄 Pull request is reviewed well. labels Nov 18, 2018
@simon-jentzsch
Copy link
Contributor Author

  • tests are running again
  • implemented the check for the experimental RPCs flag

Copy link
Collaborator

@tomusdrw tomusdrw left a comment

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

Looks good. Left couple of minor comments that would be nice to address (but not super required).

This RPC seems a bit heavy though, I'm a bit concerned with performance here (imagine fetching a proof of the entire cryptokitties storage).

account_proof: proof.into_iter().map(Bytes::new).collect(),
storage_proof: values.into_iter().filter_map(|storage_index| {
let key2: H256 = storage_index.into();
match self.client.prove_storage(key1, keccak(key2), id) {
Copy link
Collaborator

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

could be a .map instead of match

Copy link
Contributor Author

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

ok, changed it.

@@ -198,6 +204,33 @@ fn eth_get_balance() {
assert_eq!(tester.handler.handle_request_sync(req_new_acc).unwrap(), res_new_acc);
}

#[test]
fn eth_get_proof() {
Copy link
Collaborator

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

Would be good to have a test already when experimental_rpcs are disabled.

@tomusdrw tomusdrw added A6-mustntgrumble 💦 Pull request has areas for improvement. The author need not address them before merging. and removed A7-looksgoodtestsfail 🤖 Pull request is reviewed well, but cannot be merged due to tests failing. labels Nov 19, 2018
@simon-jentzsch
Copy link
Contributor Author

simon-jentzsch commented Nov 19, 2018

Yes, getting a lot of proof could be heavy, but since you can't get the entire storage without specifying all available storage keys, it is only heavy, if you pass a long list of keys.
So it may make sense for infura to limit the number of storage values, but if you are running your own node and want to get all cryptokitties, then you should know what you are doing. ( and how to calculate all storage keys )

Or to put it with gavin's words:

https://youtu.be/26ucTSSaqog?t=7448

@simon-jentzsch
Copy link
Contributor Author

But concerning the performance, I think there is room for optimization here (which can be done later ).

after getting the account-proof we already have the BasicAccount-Object and so the storage_root.

But when calling the prove_storage (just like the lightclient), this function always fetches the Account-Object again before finding the storage key. And this happens for each storage key.
So it would be more efficient to get Account only once.

https://github.com/paritytech/parity-ethereum/blob/master/ethcore/src/state/mod.rs#L1203

Sign up for free to subscribe to this conversation on GitHub. Already have an account? Sign in.
Labels
A6-mustntgrumble 💦 Pull request has areas for improvement. The author need not address them before merging. M6-rpcapi 📣 RPC API.
Projects
None yet
Development

Successfully merging this pull request may close these issues.

9 participants