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

Replace proof Verification by go-ethereum impl #3

Merged
merged 3 commits into from
Jul 7, 2021

Conversation

ed255
Copy link
Contributor

@ed255 ed255 commented Jul 6, 2021

  • Remove the custom Merkle Patricia Trie proof verification
    implementation
  • Remove the custom RLP encoding/decoding implementation
  • Use the Merkle Patricia Trie proof verification function from
    go-ethereum
    • This adds support to all types of non-existence proofs
  • Use the RLP encoding/decoding functions from go-ethereum
  • Add BytesHexQuantityBytes and SliceData to abstract encoding of QUANTITY and slices of bytes in
    the StorageProof types

@ed255 ed255 force-pushed the feature/goethereum-proof-verify branch 3 times, most recently from 67039e0 to cdf5400 Compare July 6, 2021 10:02
ethstorageproof/types.go Outdated Show resolved Hide resolved
ethstorageproof/types.go Outdated Show resolved Hide resolved
ethstorageproof/types.go Outdated Show resolved Hide resolved
ethstorageproof/types.go Outdated Show resolved Hide resolved
ethstorageproof/types.go Outdated Show resolved Hide resolved
@p4u
Copy link
Member

p4u commented Jul 6, 2021

It would be nice to add some extra test to this "new code". We could use the Proofs hardcoded on this test from vocdoni-node
https://github.com/vocdoni/vocdoni-node/blob/master/vochain/proof_test.go#L574

What do you think @ed255?

- Remove the custom Merkle Patricia Trie proof verification
  implementation
- Remove the custom RLP encoding/decoding implementation
- Use the Merkle Patricia Trie proof verification function from
  go-ethereum
  - This adds support to all types of non-existence proofs
- Use the RLP encoding/decoding functions from go-ethereum
- Add BytesHex and SliceBytesHex to abstract encoding of byte
  slices in the StorageProof types
@ed255 ed255 force-pushed the feature/goethereum-proof-verify branch from cdf5400 to d61533d Compare July 6, 2021 11:24
Add proofs from
https://github.com/vocdoni/vocdoni-node/blob/5b292fc7e74327f0127908636072482660b060b2/vochain/proof_test.go#L574

Add a minime storage proof of non-existence that couldn't be verified
with the previous VerifyProof implementation.
@ed255
Copy link
Contributor Author

ed255 commented Jul 6, 2021

It would be nice to add some extra test to this "new code". We could use the Proofs hardcoded on this test from vocdoni-node
https://github.com/vocdoni/vocdoni-node/blob/master/vochain/proof_test.go#L574

What do you think @ed255?

I've added a new commit which will:

I had to edit the value fields from https://github.com/vocdoni/vocdoni-node/blob/5b292fc7e74327f0127908636072482660b060b2/vochain/proof_test.go#L574 because they were even-length hex strings (I added a padding 0). How where those proofs generated?

@p4u
Copy link
Member

p4u commented Jul 6, 2021

RLP encoding can produce even-length hex strings, i.e 0x0 means "null" for RLP. So the Marshaling/Unmarshaling should take this into account, instead of using hex.Decode() you probably want to use some specific RLP function. Or you can just list the cases where the length can be even and modify the original hex string accordingly.

I.e https://github.com/vocdoni/storage-proofs-eth-go/blob/master/ethstorageproof/ethstorageproof.go#L339

@p4u
Copy link
Member

p4u commented Jul 6, 2021

Whenever this is addresses, ping me please :)

ethstorageproof/ethstorageproof_test.go Outdated Show resolved Hide resolved
ethstorageproof/ethstorageproof_test.go Outdated Show resolved Hide resolved
ethstorageproof/types.go Outdated Show resolved Hide resolved
@p4u
Copy link
Member

p4u commented Jul 6, 2021

LGTM now, just minor comments. You can address them or let it for a future PR.

Please, "rebase and merge" yourself.

@ed255 ed255 force-pushed the feature/goethereum-proof-verify branch from 9b1e72a to 5224583 Compare July 7, 2021 08:46
@ed255 ed255 merged commit 1c95e9a into master Jul 7, 2021
Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment
Labels
None yet
Projects
None yet
Development

Successfully merging this pull request may close these issues.

2 participants