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

Switch IAVL Store query to use ics proofs #6324

Merged
merged 21 commits into from
Jun 8, 2020
Merged
Show file tree
Hide file tree
Changes from all commits
Commits
Show all changes
21 commits
Select commit Hold shift + click to select a range
File filter

Filter by extension

Filter by extension


Conversations
Failed to load comments.
Loading
Jump to
Jump to file
Failed to load files.
Loading
Diff view
Diff view
1 change: 1 addition & 0 deletions CHANGELOG.md
Original file line number Diff line number Diff line change
Expand Up @@ -149,6 +149,7 @@ be used to retrieve the actual proposal `Content`. Also the `NewMsgSubmitProposa
* (x/capability) [\#5828](https://github.com/cosmos/cosmos-sdk/pull/5828) Capability module integration as outlined in [ADR 3 - Dynamic Capability Store](https://github.com/cosmos/tree/master/docs/architecture/adr-003-dynamic-capability-store.md).
* (x/params) [\#6005](https://github.com/cosmos/cosmos-sdk/pull/6005) Add new CLI command for querying raw x/params parameters by subspace and key.
* (x/ibc) [\#5769](https://github.com/cosmos/cosmos-sdk/pull/5769) [ICS 009 - Loopback Client](https://github.com/cosmos/ics/tree/master/spec/ics-009-loopback-client) subpackage
* (store) [\#6324](https://github.com/cosmos/cosmos-sdk/pull/6324) IAVL store query proofs now return CommitmentOp which wraps an ics23 CommitmentProof
* (x/auth) [\6350](https://github.com/cosmos/cosmos-sdk/pull/6350) New sign-batch command to sign StdTx batch files.

### Bug Fixes
Expand Down
2 changes: 2 additions & 0 deletions go.mod
Original file line number Diff line number Diff line change
Expand Up @@ -5,6 +5,8 @@ require (
github.com/bgentry/speakeasy v0.1.0
github.com/btcsuite/btcd v0.20.1-beta
github.com/btcsuite/btcutil v1.0.2
github.com/confio/ics23-iavl v0.6.0
Copy link
Contributor

Choose a reason for hiding this comment

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

As a follow up to this (sometime before tagging the 0.39 sdk), it would be nice to merge this ics23-iavl code into tendermint/iavl, so this can be optimized and integrated in the tendermint codebase.

Do you want to make an issue for that if you agree (I feel odd doing so and cannot label/milestone it)

github.com/confio/ics23/go v0.0.0-20200325200809-9f53dd0c4212
github.com/cosmos/go-bip39 v0.0.0-20180819234021-555e2067c45d
github.com/cosmos/ledger-cosmos-go v0.11.1
github.com/gibson042/canonicaljson-go v1.0.3
Expand Down
8 changes: 8 additions & 0 deletions go.sum
Original file line number Diff line number Diff line change
Expand Up @@ -78,6 +78,12 @@ github.com/client9/misspell v0.3.4/go.mod h1:qj6jICC3Q7zFZvVWo7KLAzC3yx5G7kyvSDk
github.com/cncf/udpa/go v0.0.0-20191209042840-269d4d468f6f/go.mod h1:M8M6+tZqaGXZJjfX53e64911xZQV5JYwmTeXPW+k8Sc=
github.com/cockroachdb/datadriven v0.0.0-20190809214429-80d97fb3cbaa/go.mod h1:zn76sxSg3SzpJ0PPJaLDCu+Bu0Lg3sKTORVIj19EIF8=
github.com/codahale/hdrhistogram v0.0.0-20161010025455-3a0bb77429bd/go.mod h1:sE/e/2PUdi/liOCUjSTXgM1o87ZssimdTWN964YiIeI=
github.com/confio/ics23 v0.6.0 h1:bQsi55t2+xjW6EWDl83IBF1VWurplbUu+OT6pukeiEo=
github.com/confio/ics23-iavl v0.6.0 h1:vVRCuVaP38FCw1kTeEdFuGuiY+2vAGTBQoH7Zxkq/ws=
github.com/confio/ics23-iavl v0.6.0/go.mod h1:mmXAxD1vWoO0VP8YHu6mM1QHGv71NQqa1iSVm4HeKcY=
github.com/confio/ics23/go v0.0.0-20200323120010-7d9a00f0a2fa/go.mod h1:W1I3XC8d9N8OTu/ct5VJ84ylcOunZwMXsWkd27nvVts=
github.com/confio/ics23/go v0.0.0-20200325200809-9f53dd0c4212 h1:MgS8JP5m7fPl7kumRm+YyAe5le3JlwQ4n5T/JXvr36s=
github.com/confio/ics23/go v0.0.0-20200325200809-9f53dd0c4212/go.mod h1:W1I3XC8d9N8OTu/ct5VJ84ylcOunZwMXsWkd27nvVts=
github.com/coreos/bbolt v1.3.2/go.mod h1:iRUV2dpdMOn7Bo10OQBFzIJO9kkE559Wcmn+qkEiiKk=
github.com/coreos/etcd v3.3.10+incompatible/go.mod h1:uF7uidLiAD3TWHmW31ZFd/JWoc32PjwdhPthX9715RE=
github.com/coreos/etcd v3.3.13+incompatible/go.mod h1:uF7uidLiAD3TWHmW31ZFd/JWoc32PjwdhPthX9715RE=
Expand Down Expand Up @@ -478,13 +484,15 @@ github.com/tendermint/crypto v0.0.0-20191022145703-50d29ede1e15/go.mod h1:z4YtwM
github.com/tendermint/go-amino v0.14.1/go.mod h1:i/UKE5Uocn+argJJBb12qTZsCDBcAYMbR92AaJVmKso=
github.com/tendermint/go-amino v0.15.1 h1:D2uk35eT4iTsvJd9jWIetzthE5C0/k2QmMFkCN+4JgQ=
github.com/tendermint/go-amino v0.15.1/go.mod h1:TQU0M1i/ImAo+tYpZi73AU3V/dKeCoMC9Sphe2ZwGME=
github.com/tendermint/iavl v0.13.2/go.mod h1:vE1u0XAGXYjHykd4BLp8p/yivrw2PF1TuoljBcsQoGA=
github.com/tendermint/iavl v0.13.3 h1:expgBDY1MX+6/3sqrIxGChbTNf9N9aTJ67SH4bPchCs=
github.com/tendermint/iavl v0.13.3/go.mod h1:2lE7GiWdSvc7kvT78ncIKmkOjCnp6JEnSb2O7B9htLw=
github.com/tendermint/tendermint v0.33.2 h1:NzvRMTuXJxqSsFed2J7uHmMU5N1CVzSpfi3nCc882KY=
github.com/tendermint/tendermint v0.33.2/go.mod h1:25DqB7YvV1tN3tHsjWoc2vFtlwICfrub9XO6UBO+4xk=
github.com/tendermint/tendermint v0.33.5 h1:jYgRd9ImkzA9iOyhpmgreYsqSB6tpDa6/rXYPb8HKE8=
github.com/tendermint/tendermint v0.33.5/go.mod h1:0yUs9eIuuDq07nQql9BmI30FtYGcEC60Tu5JzB5IezM=
github.com/tendermint/tm-db v0.4.1/go.mod h1:JsJ6qzYkCGiGwm5GHl/H5GLI9XLb6qZX7PRe425dHAY=
github.com/tendermint/tm-db v0.5.0/go.mod h1:lSq7q5WRR/njf1LnhiZ/lIJHk2S8Y1Zyq5oP/3o9C2U=
github.com/tendermint/tm-db v0.5.1 h1:H9HDq8UEA7Eeg13kdYckkgwwkQLBnJGgX4PgLJRhieY=
github.com/tendermint/tm-db v0.5.1/go.mod h1:g92zWjHpCYlEvQXvy9M168Su8V1IBEeawpXVVBaK4f4=
github.com/tmc/grpc-websocket-proxy v0.0.0-20170815181823-89b8d40f7ca8/go.mod h1:ncp9v5uamzpCO7NfCPTXjqaC+bZgJeR0sMTm6dMHP7U=
Expand Down
70 changes: 47 additions & 23 deletions store/iavl/store.go
Original file line number Diff line number Diff line change
Expand Up @@ -5,6 +5,8 @@ import (
"io"
"sync"

ics23iavl "github.com/confio/ics23-iavl"
ics23 "github.com/confio/ics23/go"
"github.com/pkg/errors"
"github.com/tendermint/iavl"
abci "github.com/tendermint/tendermint/abci/types"
Expand Down Expand Up @@ -275,31 +277,25 @@ func (st *Store) Query(req abci.RequestQuery) (res abci.ResponseQuery) {
break
}

if req.Prove {
value, proof, err := tree.GetVersionedWithProof(key, res.Height)
if err != nil {
res.Log = err.Error()
break
}
if proof == nil {
// Proof == nil implies that the store is empty.
if value != nil {
panic("unexpected value for an empty proof")
}
}
if value != nil {
// value was found
res.Value = value
res.Proof = &merkle.Proof{Ops: []merkle.ProofOp{iavl.NewValueOp(key, proof).ProofOp()}}
} else {
// value wasn't found
res.Value = nil
res.Proof = &merkle.Proof{Ops: []merkle.ProofOp{iavl.NewAbsenceOp(key, proof).ProofOp()}}
}
} else {
_, res.Value = tree.GetVersioned(key, res.Height)
_, res.Value = tree.GetVersioned(key, res.Height)
if !req.Prove {
break
}

// Continue to prove existence/absence of value
// Must convert store.Tree to iavl.MutableTree with given version to use in CreateProof
Copy link
Contributor

Choose a reason for hiding this comment

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

Can you take a look at ics23-iavl to see if I can change this requirement (and accept iavl.ImmutableTree instead). I was just gettng it to work, but in this use case referring to an immutable tree seems better.

Copy link
Member Author

Choose a reason for hiding this comment

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

Yea since you aren't using any Versioned function, you should be able to just swap out MutableTree for ImmutableTree in the function signatures and everything else should still work.

This will make the code here cleaner

iTree, err := tree.GetImmutable(res.Height)
if err != nil {
// sanity check: If value for given version was retrieved, immutable tree must also be retrievable
panic(fmt.Sprintf("version exists in store but could not retrieve corresponding versioned tree in store, %s", err.Error()))
}
mtree := &iavl.MutableTree{
ImmutableTree: iTree,
}

// get proof from tree and convert to merkle.Proof before adding to result
res.Proof = getProofFromTree(mtree, req.Data, res.Value != nil)

case "/subspace":
var KVs []types.KVPair

Expand All @@ -321,6 +317,34 @@ func (st *Store) Query(req abci.RequestQuery) (res abci.ResponseQuery) {
return res
}

// Takes a MutableTree, a key, and a flag for creating existence or absence proof and returns the
// appropriate merkle.Proof. Since this must be called after querying for the value, this function should never error
// Thus, it will panic on error rather than returning it
func getProofFromTree(tree *iavl.MutableTree, key []byte, exists bool) *merkle.Proof {
var (
commitmentProof *ics23.CommitmentProof
err error
)

if exists {
// value was found
commitmentProof, err = ics23iavl.CreateMembershipProof(tree, key)
if err != nil {
// sanity check: If value was found, membership proof must be creatable
panic(fmt.Sprintf("unexpected value for empty proof: %s", err.Error()))
}
} else {
// value wasn't found
commitmentProof, err = ics23iavl.CreateNonMembershipProof(tree, key)
if err != nil {
// sanity check: If value wasn't found, nonmembership proof must be creatable
panic(fmt.Sprintf("unexpected error for nonexistence proof: %s", err.Error()))
}
}
op := types.NewIavlCommitmentOp(key, commitmentProof)
return &merkle.Proof{Ops: []merkle.ProofOp{op.ProofOp()}}
}

//----------------------------------------

// Implements types.Iterator.
Expand Down
7 changes: 4 additions & 3 deletions store/rootmulti/proof.go
Original file line number Diff line number Diff line change
@@ -1,8 +1,9 @@
package rootmulti

import (
"github.com/tendermint/iavl"
"github.com/tendermint/tendermint/crypto/merkle"

storetypes "github.com/cosmos/cosmos-sdk/store/types"
)

// RequireProof returns whether proof is required for the subpath.
Expand All @@ -21,7 +22,7 @@ func RequireProof(subpath string) bool {
func DefaultProofRuntime() (prt *merkle.ProofRuntime) {
prt = merkle.NewProofRuntime()
prt.RegisterOpDecoder(merkle.ProofOpSimpleValue, merkle.SimpleValueOpDecoder)
prt.RegisterOpDecoder(iavl.ProofOpIAVLValue, iavl.ValueOpDecoder)
prt.RegisterOpDecoder(iavl.ProofOpIAVLAbsence, iavl.AbsenceOpDecoder)
prt.RegisterOpDecoder(storetypes.ProofOpIAVLCommitment, storetypes.CommitmentOpDecoder)
prt.RegisterOpDecoder(storetypes.ProofOpSimpleMerkleCommitment, storetypes.CommitmentOpDecoder)
return
}
11 changes: 11 additions & 0 deletions store/types/errors.go
Original file line number Diff line number Diff line change
@@ -0,0 +1,11 @@
package types

import (
sdkerrors "github.com/cosmos/cosmos-sdk/types/errors"
)

const StoreCodespace = "store"

var (
ErrInvalidProof = sdkerrors.Register(StoreCodespace, 2, "invalid proof")
)
168 changes: 168 additions & 0 deletions store/types/proof.go
Original file line number Diff line number Diff line change
@@ -0,0 +1,168 @@
package types

import (
ics23 "github.com/confio/ics23/go"
"github.com/tendermint/tendermint/crypto/merkle"

sdkerrors "github.com/cosmos/cosmos-sdk/types/errors"
)

const (
ProofOpIAVLCommitment = "ics23:iavl"
ProofOpSimpleMerkleCommitment = "ics23:simple"
)

// CommitmentOp implements merkle.ProofOperator by wrapping an ics23 CommitmentProof
// It also contains a Key field to determine which key the proof is proving.
// NOTE: CommitmentProof currently can either be ExistenceProof or NonexistenceProof
//
// Type and Spec are classified by the kind of merkle proof it represents allowing
// the code to be reused by more types. Spec is never on the wire, but mapped from type in the code.
type CommitmentOp struct {
Type string
Spec *ics23.ProofSpec
Key []byte
Proof *ics23.CommitmentProof
}

var _ merkle.ProofOperator = CommitmentOp{}

func NewIavlCommitmentOp(key []byte, proof *ics23.CommitmentProof) CommitmentOp {
return CommitmentOp{
Type: ProofOpIAVLCommitment,
Spec: ics23.IavlSpec,
Key: key,
Proof: proof,
}
}

func NewSimpleMerkleCommitmentOp(key []byte, proof *ics23.CommitmentProof) CommitmentOp {
return CommitmentOp{
Type: ProofOpSimpleMerkleCommitment,
Spec: ics23.TendermintSpec,
Key: key,
Proof: proof,
}
}

// CommitmentOpDecoder takes a merkle.ProofOp and attempts to decode it into a CommitmentOp ProofOperator
// The proofOp.Data is just a marshalled CommitmentProof. The Key of the CommitmentOp is extracted
// from the unmarshalled proof.
func CommitmentOpDecoder(pop merkle.ProofOp) (merkle.ProofOperator, error) {
var spec *ics23.ProofSpec
switch pop.Type {
case ProofOpIAVLCommitment:
spec = ics23.IavlSpec
case ProofOpSimpleMerkleCommitment:
spec = ics23.TendermintSpec
default:
return nil, sdkerrors.Wrapf(ErrInvalidProof, "unexpected ProofOp.Type; got %s, want supported ics23 subtypes 'ProofOpIAVLCommitment' or 'ProofOpSimpleMerkleCommitment'", pop.Type)
}

proof := &ics23.CommitmentProof{}
err := proof.Unmarshal(pop.Data)
if err != nil {
return nil, err
}

op := CommitmentOp{
Type: pop.Type,
Key: pop.Key,
Spec: spec,
Proof: proof,
}
return op, nil
}

func (op CommitmentOp) GetKey() []byte {
return op.Key
}

// Run takes in a list of arguments and attempts to run the proof op against these arguments
// Returns the root wrapped in [][]byte if the proof op succeeds with given args. If not,
// it will return an error.
//
// CommitmentOp will accept args of length 1 or length 0
// If length 1 args is passed in, then CommitmentOp will attempt to prove the existence of the key
// with the value provided by args[0] using the embedded CommitmentProof and return the CommitmentRoot of the proof
// If length 0 args is passed in, then CommitmentOp will attempt to prove the absence of the key
// in the CommitmentOp and return the CommitmentRoot of the proof
func (op CommitmentOp) Run(args [][]byte) ([][]byte, error) {
colin-axner marked this conversation as resolved.
Show resolved Hide resolved
// Only support an existence proof or nonexistence proof (batch proofs currently unsupported)
switch len(args) {
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, sdkerrors.Wrap(ErrInvalidProof, "proof is not a nonexistence proof and args is nil")
}

// get root from either left or right existence proof. Note they must have the same root if both exist
// and at least one proof must be non-nil
var (
root []byte
err error
)
switch {
// check left proof to calculate root
case nonexistProof.Nonexist.Left != nil:
root, err = nonexistProof.Nonexist.Left.Calculate()
if err != nil {
return nil, sdkerrors.Wrap(ErrInvalidProof, "could not calculate root from nonexistence proof")
}
case nonexistProof.Nonexist.Right != nil:
// Left proof is nil, check right proof
root, err = nonexistProof.Nonexist.Right.Calculate()
if err != nil {
return nil, sdkerrors.Wrap(ErrInvalidProof, "could not calculate root from nonexistence proof")
}
default:
// both left and right existence proofs are empty
// this only proves absence against a nil root (empty store)
return [][]byte{nil}, nil
}

absent := ics23.VerifyNonMembership(op.Spec, root, op.Proof, op.Key)
if !absent {
return nil, sdkerrors.Wrapf(ErrInvalidProof, "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, sdkerrors.Wrap(ErrInvalidProof, "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, sdkerrors.Wrap(ErrInvalidProof, "could not calculate root from existence proof")
}

if !ics23.VerifyMembership(op.Spec, root, op.Proof, op.Key, args[0]) {
return nil, sdkerrors.Wrapf(ErrInvalidProof, "proof did not verify existence of key %s with given value %x", op.Key, args[0])
}

return [][]byte{root}, nil
default:
return nil, sdkerrors.Wrapf(ErrInvalidProof, "args must be length 0 or 1, got: %d", len(args))
}
}

// ProofOp implements ProofOperator interface and converts a CommitmentOp
// into a merkle.ProofOp format that can later be decoded by CommitmentOpDecoder
// back into a CommitmentOp for proof verification
func (op CommitmentOp) ProofOp() merkle.ProofOp {
fedekunze marked this conversation as resolved.
Show resolved Hide resolved
Copy link
Contributor

Choose a reason for hiding this comment

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

nit: I would opt to return an error here and allow the caller to decide to panic or not.

Copy link
Member Author

Choose a reason for hiding this comment

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

We cannot return an error here, this method is satisfying the ProofOperator interface

Copy link
Contributor

@alexanderbez alexanderbez Jun 4, 2020

Choose a reason for hiding this comment

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

Yikes, I would consider revising that interface if possible. Not a good UX IMHO. Ideally, you should always be able to bubble up errors as much as possible and allow callers to handle it.

Copy link
Member Author

Choose a reason for hiding this comment

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

Hmm... yea that would definitely require a separate discussion with TM team and a separate PR since this is a tendermint interface. Don't think a panic is the worst idea here since it should be an invariant that any well-constructed ProofOperator should successfully return a merkle.ProofOp here

bz, err := op.Proof.Marshal()
if err != nil {
panic(err.Error())
}
return merkle.ProofOp{
Type: op.Type,
Key: op.Key,
Data: bz,
}
}