Make store.Query use ics23-proofs #6315
There are no files selected for viewing
| Original file line number | Diff line number | Diff line change |
|---|---|---|
| @@ -0,0 +1,109 @@ | ||
| package iavl | ||
|
|
||
| import ( | ||
| "errors" | ||
| "fmt" | ||
|
|
||
| ics23 "github.com/confio/ics23/go" | ||
| "github.com/tendermint/tendermint/crypto/merkle" | ||
| ) | ||
|
|
||
| const ProofOpIAVL = "iavlstore" | ||
|
|
||
| type IAVLOp struct { | ||
| Key []byte | ||
| Proof *ics23.CommitmentProof | ||
| } | ||
|
|
||
| var _ merkle.ProofOperator = IAVLOp{} | ||
|
|
||
| func NewIAVLOp(key []byte, proof *ics23.CommitmentProof) IAVLOp { | ||
| return IAVLOp{ | ||
| Key: key, | ||
| Proof: proof, | ||
| } | ||
| } | ||
|
|
||
| func IAVLOpDecoder(pop merkle.ProofOp) (merkle.ProofOperator, error) { | ||
|
There was a problem hiding this comment. This guy definitely warrants a godoc 👍 |
||
| if pop.Type != ProofOpIAVL { | ||
| return nil, errors.New(fmt.Sprintf("unexpected ProofOp.Type; got %v, want %v", pop.Type, ProofOpIAVL)) | ||
| } | ||
| var op IAVLOp | ||
| proof := &ics23.CommitmentProof{} | ||
| err := proof.Unmarshal(pop.Data) | ||
| if err != nil { | ||
| return nil, err | ||
| } | ||
| op.Proof = proof | ||
|
|
||
| // Get Key from proof for now | ||
|
There was a problem hiding this comment. Based on the logic comparisons to ics23 types, are you sure the There was a problem hiding this comment. 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 There was a problem hiding this comment. 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. |
||
| if existProof, ok := op.Proof.Proof.(*ics23.CommitmentProof_Exist); !ok { | ||
| op.Key = existProof.Exist.Key | ||
| } else if nonexistProof, ok := op.Proof.Proof.(*ics23.CommitmentProof_Nonexist); !ok { | ||
| op.Key = nonexistProof.Nonexist.Key | ||
| } else { | ||
| return nil, errors.New("Proof type unsupported") | ||
| } | ||
| return op, nil | ||
| } | ||
|
|
||
| func (op IAVLOp) GetKey() []byte { | ||
| return op.Key | ||
| } | ||
|
|
||
| func (op IAVLOp) Run(args [][]byte) ([][]byte, error) { | ||
|
There was a problem hiding this comment. What does There was a problem hiding this comment. 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. There was a problem hiding this comment. 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 |
||
| switch len(args) { | ||
|
There was a problem hiding this comment. Both Existence and Nonexistence proofs are being handled by the same IAVLop here |
||
| case 0: | ||
| // Args are nil, so we verify the absence of the key. | ||
| nonexistProof, ok := op.Proof.Proof.(*ics23.CommitmentProof_Nonexist) | ||
| if !ok { | ||
| return nil, errors.New("proof is not a nonexistence proof and args is nil") | ||
| } | ||
|
|
||
| root, err := nonexistProof.Nonexist.Left.Calculate() | ||
AdityaSripal marked this conversation as resolved.
Show resolved
Hide resolved
|
||
| if err != nil { | ||
| return nil, errors.New("could not calculate root from nonexistence proof") | ||
| } | ||
|
|
||
| absent := ics23.VerifyNonMembership(ics23.IavlSpec, root, op.Proof, op.Key) | ||
| if !absent { | ||
| return nil, errors.New(fmt.Sprintf("proof did not verify absence of key: %s", string(op.Key))) | ||
| } | ||
|
|
||
| return [][]byte{root}, nil | ||
|
|
||
| case 1: | ||
| // Args is length 1, verify existence of key with value args[0] | ||
| existProof, ok := op.Proof.Proof.(*ics23.CommitmentProof_Exist) | ||
| if !ok { | ||
| return nil, errors.New("proof is not a existence proof and args is length 1") | ||
| } | ||
| // For subtree verification, we simply calculate the root from the proof and use it to prove | ||
| // against the value | ||
| root, err := existProof.Exist.Calculate() | ||
| if err != nil { | ||
| return nil, errors.New("could not calculate root from existence proof") | ||
| } | ||
|
|
||
| exists := ics23.VerifyMembership(ics23.IavlSpec, root, op.Proof, op.Key, args[0]) | ||
|
There was a problem hiding this comment. The only iavl-specific code here is this Although that is for a future pr. It is fine as is now. |
||
| if !exists { | ||
| return nil, errors.New(fmt.Sprintf("proof did not verify existence of key %s with given value %x", op.Key, args[0])) | ||
| } | ||
|
|
||
| return [][]byte{root}, nil | ||
| default: | ||
| return nil, errors.New(fmt.Sprintf("args must be length 0 or 1, got: %d", len(args))) | ||
| } | ||
| } | ||
|
|
||
| func (op IAVLOp) ProofOp() merkle.ProofOp { | ||
| bz, err := op.Proof.Marshal() | ||
|
There was a problem hiding this comment. Note data just contains marshalled proof for now There was a problem hiding this comment. That is quite nice. This is perfect to just store the protobuf data. |
||
| if err != nil { | ||
| panic(err.Error()) | ||
| } | ||
| return merkle.ProofOp{ | ||
| Type: ProofOpIAVL, | ||
| Key: op.Key, | ||
| Data: bz, | ||
| } | ||
| } | ||
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.
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.
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.
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