-
Notifications
You must be signed in to change notification settings - Fork 1.7k
EIP-1186: add eth_getProof
RPC-Method
#9001
Conversation
It looks like @simon-jentzsch hasn't signed our Contributor License Agreement, yet.
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 Many thanks, Parity Technologies CLA Bot |
Hey, please sign our CLA.
Which EIP are you referring to? Is this officially part of the eth namespace? |
Thanks for the responses. |
Ah, gotcha. I'll mark this WIP. |
eth_getProof
RPC-Methodeth_getProof
RPC-Method
The CI failure is spurious here and should be fixed if you pull in the latest changes from master. |
[clabot:check] |
It looks like @simon-jentzsch signed our Contributor License Agreement. 👍 Many thanks, Parity Technologies CLA Bot |
There was a problem hiding this 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
Putting this on ice since you have chosen the |
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 |
Ok, let me know how you want to proceed. |
@simon-jentzsch We can use the |
@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. ;-) |
@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. |
@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 |
There was a problem hiding this 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!
Could you please update the doc in src/interfaces/eth.js interfaces https://github.com/parity-js/jsonrpc |
@niklasad1 I fixed the formatting of the structs and also added tests. |
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
Thanks, I'm looking forward to see this merged! |
IMHO this should be enabled only with |
|
There was a problem hiding this 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).
rpc/src/v1/impls/eth.rs
Outdated
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) { |
There was a problem hiding this comment.
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
There was a problem hiding this comment.
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() { |
There was a problem hiding this comment.
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.
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. Or to put it with gavin's words: |
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 But when calling the https://github.com/paritytech/parity-ethereum/blob/master/ethcore/src/state/mod.rs#L1203 |
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.