Make store.Query use ics23-proofs #6315

AdityaSripal · 2020-06-02T04:18:27Z

Description

Currently changed iavl store queries to return ics23-proofs

closes: #XXXX

Before we can merge this PR, please make sure that all the following items have been
checked off. If any of the checklist items are not applicable, please leave them but
write a little note why.

For contributor use:

Targeted PR against correct branch (see CONTRIBUTING.md)
Linked to Github issue with discussion and accepted design OR link to spec that describes this work.
Code follows the module structure standards.
Wrote unit and integration tests
Updated relevant documentation (docs/) or specification (x/<module>/spec/)
Added relevant godoc comments.
Added a relevant changelog entry to the Unreleased section in CHANGELOG.md
Re-reviewed Files changed in the Github PR explorer

alexanderbez · 2020-06-02T13:35:00Z

store/iavl/proof.go

+	}
+}
+
+func IAVLOpDecoder(pop merkle.ProofOp) (merkle.ProofOperator, error) {


This guy definitely warrants a godoc 👍

alexanderbez · 2020-06-02T13:35:56Z

store/iavl/proof.go

+	}
+	op.Proof = proof
+
+	// Get Key from proof for now


Based on the logic comparisons to ics23 types, are you sure the ok semantics are correct? It reads as if it should be ; ok { .. } and not ; !ok { ... }

alexanderbez · 2020-06-02T13:36:10Z

store/iavl/proof.go

+	return op.Key
+}
+
+func (op IAVLOp) Run(args [][]byte) ([][]byte, error) {


What does Run do? Add a godoc here 👍

Yeah, this is super confusing. Not from @AdityaSripal but from whoever designed this ProofOperator interface:

https://github.com/tendermint/tendermint/blob/master/crypto/merkle/proof.go#L12-L18

It is totally unclear if it is checking absence or existence. And why it returns multiple roots. I think this ProofOp interface needs an overhaul as well, but I think he is implementing this properly.

You can see how iavl existence proofs require exactly one entry: https://github.com/tendermint/iavl/blob/c4c6383ddd25f984467f78516aca7089bf110c97/proof_iavl_value.go

And absence proofs require exactly 0: https://github.com/tendermint/iavl/blob/c4c6383ddd25f984467f78516aca7089bf110c97/proof_iavl_absence.go#L62-L64

store/iavl/proof.go

alexanderbez · 2020-06-02T13:38:39Z

store/iavl/store.go

-				// Proof == nil implies that the store is empty.
-				if value != nil {
-					panic("unexpected value for an empty proof")
+			var commitmentProof *ics23.CommitmentProof


nit: group vars :)

alexanderbez · 2020-06-02T13:40:04Z

store/iavl/store.go

-				res.Proof = &merkle.Proof{Ops: []merkle.ProofOp{iavl.NewValueOp(key, proof).ProofOp()}}
+				commitmentProof, err = ics23iavl.CreateMembershipProof(mtree, req.Data)
+				if err != nil {
+					panic(fmt.Sprintf("unexpected value for empty proof: %s", err.Error()))


I don't understand why we both panic and return and error. It seems like we should always return an error.

This should never happen, so I panic. Also believe this matches previous behavior

Co-authored-by: Alexander Bezobchuk <alexanderbez@users.noreply.github.com>

alessio · 2020-06-02T14:05:19Z

store/iavl/store.go

+			var err error
+			if res.Value != nil {
+				// Only support query proof from MutableTree for now
+				mtree, ok := tree.(*iavl.MutableTree)


I'd love to see mtree declared outside of this block - mostly for readability's sake.

alessio · 2020-06-02T14:10:21Z

store/iavl/store.go

@@ -275,31 +277,36 @@ func (st *Store) Query(req abci.RequestQuery) (res abci.ResponseQuery) {
 			break
 		}

+		_, res.Value = tree.GetVersioned(key, res.Height)
 		if req.Prove {


I'd love to see this being implemented as follows:

Suggested change

if req.Prove {

if !req.Prove {

break

}

This would reduce the indentation level and make the code more readable

AdityaSripal · 2020-06-02T14:30:15Z

store/iavl/proof.go

+	}
+	op.Proof = proof
+
+	// Get Key from proof for now


Currently op.Data just contains marshalled ics proof while the op has the key in a separate field. I extract key from proof here, though we may want to instead wrap the ics-proof in a proto struct containing proof and key and just directly unmarshal the proofoperator

Reading this again, I see that the marshalled version is just the commitment proof and the key is only cached in memory when decoding Yeah, please ignore my comment about the newtype, I was really worrying about the serialized version.

AdityaSripal · 2020-06-02T14:30:46Z

store/iavl/proof.go

+}
+
+func (op IAVLOp) Run(args [][]byte) ([][]byte, error) {
+	switch len(args) {


Both Existence and Nonexistence proofs are being handled by the same IAVLop here

AdityaSripal · 2020-06-02T14:31:04Z

store/iavl/proof.go

+}
+
+func (op IAVLOp) ProofOp() merkle.ProofOp {
+	bz, err := op.Proof.Marshal()


Note data just contains marshalled proof for now

That is quite nice.
A client (eg. js) can parse out the proof op type and data fields. type is just a string, and data is a protobuf message with mutli-langauge decoding support.

This is perfect to just store the protobuf data.

…aditya/ics-query

ethanfrey

Nice work, simple and clean.

Left a number of comments on ways to polish it. Much of that would lead to PRs on other repos.

ethanfrey · 2020-06-02T19:05:26Z

store/iavl/proof.go

+	return op.Key
+}
+
+func (op IAVLOp) Run(args [][]byte) ([][]byte, error) {


Yeah, this is super confusing. Not from @AdityaSripal but from whoever designed this ProofOperator interface:

https://github.com/tendermint/tendermint/blob/master/crypto/merkle/proof.go#L12-L18

It is totally unclear if it is checking absence or existence. And why it returns multiple roots. I think this ProofOp interface needs an overhaul as well, but I think he is implementing this properly.

ethanfrey · 2020-06-02T19:07:21Z

store/iavl/proof.go

+	return op.Key
+}
+
+func (op IAVLOp) Run(args [][]byte) ([][]byte, error) {


You can see how iavl existence proofs require exactly one entry: https://github.com/tendermint/iavl/blob/c4c6383ddd25f984467f78516aca7089bf110c97/proof_iavl_value.go

And absence proofs require exactly 0: https://github.com/tendermint/iavl/blob/c4c6383ddd25f984467f78516aca7089bf110c97/proof_iavl_absence.go#L62-L64

ethanfrey · 2020-06-02T19:10:30Z

store/iavl/proof.go

+
+const ProofOpIAVL = "iavlstore"
+
+type IAVLOp struct {


Also, if it makes it easier, use a different proof type for existence and absence. So you don't have to type switch below. Makes more sense to this code, but not sure in how it is used externally.

Hmmm... if other people are in favor I may go for it, it's certainly doable. I agree it removes the type switch, but virtually everything else about how to proof is constructed is identical so I think having the switch statement here is fine

ethanfrey · 2020-06-02T19:11:47Z

store/iavl/proof.go

+			return nil, errors.New("could not calculate root from existence proof")
+		}
+
+		exists := ics23.VerifyMembership(ics23.IavlSpec, root, op.Proof, op.Key, args[0])


The only iavl-specific code here is this ics23.IavlSpec. It would be good to somehow abstract the code out so we can use 98% the same code for simple merkle proofs.

Although that is for a future pr. It is fine as is now.

ethanfrey · 2020-06-02T19:13:14Z

store/iavl/store.go

+		var commitmentProof *ics23.CommitmentProof
+		var err error
+
+		// Must convert store.Tree to iavl.MutableTree to use in CreateProof


If we can use another type in ics23iavl, please let me know and make a pr to simplify this logic

Thank you for handling this case, but it really should go in that library (once you get it working here, let's pull it over there)

ethanfrey · 2020-06-02T19:14:43Z

store/iavl/store.go

-				if value != nil {
-					panic("unexpected value for an empty proof")
-				}
+		_, res.Value = tree.GetVersioned(key, res.Height)


Note we get the value from res.Height, but below use the tree from the current height.
This will fail if the key changed between the querier and current heights.

ethanfrey · 2020-06-02T19:15:07Z

store/iavl/store.go

+
+		// Must convert store.Tree to iavl.MutableTree to use in CreateProof
+		var mtree *iavl.MutableTree
+		switch t := tree.(type) {


This is what I am talking about. We need the tree from the requested height here to use for proofs.

AdityaSripal · 2020-06-02T19:40:50Z

Closing and addressing outstanding comments in #6324

make iavl store.Query return ics proof

f351903

AdityaSripal requested review from aaronc, alessio, alexanderbez, clevinson, fedekunze and nylira as code owners June 2, 2020 04:18

alexanderbez added the R4R label Jun 2, 2020

alexanderbez reviewed Jun 2, 2020

View reviewed changes

alexanderbez suggested changes Jun 2, 2020

View reviewed changes

alexanderbez added the C:Store label Jun 2, 2020

AdityaSripal added WIP and removed R4R labels Jun 2, 2020

AdityaSripal and others added 2 commits June 2, 2020 09:50

Update store/iavl/proof.go

9a7da75

Co-authored-by: Alexander Bezobchuk <alexanderbez@users.noreply.github.com>

Apply suggestions from code review

8f13f42

Co-authored-by: Alexander Bezobchuk <alexanderbez@users.noreply.github.com>

alessio reviewed Jun 2, 2020

View reviewed changes

AdityaSripal commented Jun 2, 2020

View reviewed changes

AdityaSripal added 3 commits June 2, 2020 10:53

start cleanup

d99a01a

Merge branch 'aditya/ics-query' of github.com:cosmos/cosmos-sdk into …

65d6b53

…aditya/ics-query

fix decode

7d0a551

ethanfrey reviewed Jun 2, 2020

View reviewed changes

AdityaSripal closed this Jun 2, 2020

AdityaSripal deleted the aditya/ics-query branch June 15, 2020 18:46

Provide feedback

Saved searches

Use saved searches to filter your results more quickly

Make store.Query use ics23-proofs #6315

Make store.Query use ics23-proofs #6315

AdityaSripal commented Jun 2, 2020

alexanderbez Jun 2, 2020

alexanderbez Jun 2, 2020

alexanderbez Jun 2, 2020

ethanfrey Jun 2, 2020

ethanfrey Jun 2, 2020

alexanderbez Jun 2, 2020

alexanderbez Jun 2, 2020

AdityaSripal Jun 2, 2020

alessio Jun 2, 2020 •

edited

Loading

alessio Jun 2, 2020

AdityaSripal Jun 2, 2020

ethanfrey Jun 2, 2020

AdityaSripal Jun 2, 2020

AdityaSripal Jun 2, 2020

ethanfrey Jun 2, 2020

ethanfrey left a comment

ethanfrey Jun 2, 2020

ethanfrey Jun 2, 2020

ethanfrey Jun 2, 2020

AdityaSripal Jun 2, 2020

ethanfrey Jun 2, 2020

ethanfrey Jun 2, 2020

ethanfrey Jun 2, 2020

ethanfrey Jun 2, 2020

AdityaSripal commented Jun 2, 2020

Make store.Query use ics23-proofs #6315

Make store.Query use ics23-proofs #6315

Conversation

AdityaSripal commented Jun 2, 2020

Description

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

alessio Jun 2, 2020 • edited Loading

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

ethanfrey left a comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

AdityaSripal commented Jun 2, 2020

alessio Jun 2, 2020 •

edited

Loading