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.

Sign up for GitHub

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

WIP: Fix ICS23-Proofs #6178

Closed
wants to merge 9 commits into from
Closed

WIP: Fix ICS23-Proofs #6178

Show file tree
Hide file tree
Changes from all commits
Commits
Show all changes
9 commits
Select commit Hold shift + click to select a range
73462b2
integrate @ethanfrey work on confio/ics23 into 23-commitment
AdityaSripal May 8, 2020
59d5fd1
start potential chain-proof implementation
AdityaSripal May 18, 2020
567fc51
Update x/ibc/07-tendermint/types/client_state.go
AdityaSripal May 19, 2020
2da7b0d
Update x/ibc/07-tendermint/types/msgs.go
AdityaSripal May 19, 2020
4c38f3e
add GetProofSpecs method to localhost clientstate
AdityaSripal May 19, 2020
dba44d8
Merge branch 'aditya/ics23-proofs' of github.com:cosmos/cosmos-sdk in…
AdityaSripal May 19, 2020
d26a2d8
move specs out of proofs
AdityaSripal May 20, 2020
f605880
implement batch proofs
AdityaSripal May 20, 2020
6c66834
replace MultiStoreProofOp with SimpleValueOp
AdityaSripal May 22, 2020
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
3 changes: 3 additions & 0 deletions go.mod
View file
Open in desktop
Original file line number Diff line number Diff line change
Expand Up @@ -4,6 +4,7 @@ require (
github.com/99designs/keyring v1.1.5
github.com/bgentry/speakeasy v0.1.0
github.com/btcsuite/btcd v0.20.1-beta
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 Expand Up @@ -36,4 +37,6 @@ require (

replace github.com/gogo/protobuf => github.com/regen-network/protobuf v1.3.2-alpha.regen.1

replace github.com/tendermint/tendermint => github.com/AdityaSripal/tendermint v1.2.3
Copy link
Contributor

Choose a reason for hiding this comment

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

I just looked at this. Essentially changing the hashing algorithm for SimpleMerkleProof (which breaks any external tooling that uses it). Why was that done? And is there any goal to move that into master??

Copy link
Contributor

Choose a reason for hiding this comment

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

AdityaSripal/tendermint@b8be4a0

Copy link
Contributor

Choose a reason for hiding this comment

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

I would remove this replace. Step by step. No need to require merging a breaking change into tendermint core to get this working. If you think it is a nice optimiziation, it may be, and can be another PR process there, but it feels odd to pull it into this PR.

Copy link
Member Author

@AdityaSripal AdityaSripal Jun 1, 2020 •
edited
Loading

Choose a reason for hiding this comment

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

This isn't an optimization. The way simple proofs were being verified didn't line up with the way the stores were getting hashed. There was an extra rounds of hashing happening for leaf values in proof verification that wasn't happening with the simple hash. Thus the proof did not commit to the multistore hash, when just using SimpleHash and SimpleProof without any changes.

After tracking the issue down, I found that removing these extra layers of hashing made the proof verification line up with the multistore commit. Haven't found alternative solutions that also work, so I left this for now and moved on with rest of the code. Will look into this further and try to understand issue fully, because I too do not want to change tendermint if I don't have to.

If it's possible to have an SDK only solution, I will use it. If not, this may be necessary. I haven't figured out if it is necessary yet.

Copy link
Contributor

Choose a reason for hiding this comment

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

Are we still hashing somewhere? Maybe this was accidental, or maybe there was a reason for it - can we ask the Tendermint team (are they using this code anywhere)?

If they're not using this code anywhere, maybe we should move it out of Tendermint altogether.


go 1.14
5 changes: 5 additions & 0 deletions go.sum
View file
Open in desktop
Original file line number Diff line number Diff line change
Expand Up @@ -2,6 +2,8 @@ cloud.google.com/go v0.26.0/go.mod h1:aQUYkXzVsufM+DwF1aE+0xfcU+56JwCaLick0ClmMT
cloud.google.com/go v0.34.0/go.mod h1:aQUYkXzVsufM+DwF1aE+0xfcU+56JwCaLick0ClmMTw=
github.com/99designs/keyring v1.1.5 h1:wLv7QyzYpFIyMSwOADq1CLTF9KbjbBfcnfmOGJ64aO4=
github.com/99designs/keyring v1.1.5/go.mod h1:7hsVvt2qXgtadGevGJ4ujg+u8m6SpJ5TpHqTozIPqf0=
github.com/AdityaSripal/tendermint v1.2.3 h1:jJUqiQBD2G7WKES7bA2y3bRof1W4kZHs4oAucNg+b2s=
github.com/AdityaSripal/tendermint v1.2.3/go.mod h1:6NW9DVkvsvqmCY6wbRsOo66qGDhMXglRL70aXajvBEA=
github.com/BurntSushi/toml v0.3.1 h1:WXkYYl6Yr3qBf1K79EBnL4mak0OimBfB0XUf9Vl28OQ=
github.com/BurntSushi/toml v0.3.1/go.mod h1:xHWCNGjB5oqiDr8zfno3MHue2Ht5sIBksp03qcyfWMU=
github.com/ChainSafe/go-schnorrkel v0.0.0-20200102211924-4bcbc698314f h1:4O1om+UVU+Hfcihr1timk8YNXHxzZWgCo7ofnrZRApw=
Expand Down Expand Up @@ -65,6 +67,9 @@ 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/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
7 changes: 1 addition & 6 deletions store/rootmulti/merkle_map.go
View file
Open in desktop
Original file line number Diff line number Diff line change
Expand Up @@ -6,7 +6,6 @@ import (
"io"

"github.com/tendermint/tendermint/crypto/merkle"
"github.com/tendermint/tendermint/crypto/tmhash"
"github.com/tendermint/tendermint/libs/kv"
)

Expand All @@ -30,13 +29,9 @@ func newMerkleMap() *merkleMap {
func (sm *merkleMap) set(key string, value []byte) {
sm.sorted = false

// The value is hashed, so you can check for equality with a cached value (say)
Copy link
Contributor

Choose a reason for hiding this comment

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

Okay, this is the same change as in tendermint. Again, why?

(Also this breaks ics23-tendermint, which needs a PR to update the format and regenerate the proofs)

Copy link
Contributor

Choose a reason for hiding this comment

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

https://github.com/confio/ics23-tendermint/pulls if you really want to make this.

But there should be no real reason to mix it in this PR.

Copy link
Contributor

Choose a reason for hiding this comment

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

Why do we have this code in two places in the first place?

// and make a determination to fetch or not.
vhash := tmhash.Sum(value)

sm.kvs = append(sm.kvs, kv.Pair{
Key: []byte(key),
Value: vhash,
Value: value,
})
}

Expand Down
12 changes: 6 additions & 6 deletions store/rootmulti/merkle_map_test.go
View file
Open in desktop
Original file line number Diff line number Diff line change
Expand Up @@ -13,29 +13,29 @@ func TestSimpleMap(t *testing.T) {
values []string // each string gets converted to []byte in test
want string
}{
{[]string{"key1"}, []string{"value1"}, "a44d3cc7daba1a4600b00a2434b30f8b970652169810d6dfa9fb1793a2189324"},
{[]string{"key1"}, []string{"value2"}, "0638e99b3445caec9d95c05e1a3fc1487b4ddec6a952ff337080360b0dcc078c"},
{[]string{"key1"}, []string{"value1"}, "09c468a07fe9bc1f14e754cff0acbad4faf9449449288be8e1d5d1199a247034"},
{[]string{"key1"}, []string{"value2"}, "2131d85de3a8ded5d3a72bfc657f7324138540c520de7401ac8594785a3082fb"},
// swap order with 2 keys
{
[]string{"key1", "key2"},
[]string{"value1", "value2"},
"8fd19b19e7bb3f2b3ee0574027d8a5a4cec370464ea2db2fbfa5c7d35bb0cff3",
"017788f37362dd0687beb59c0b3bfcc17a955120a4cb63dbdd4a0fdf9e07730e",
},
{
[]string{"key2", "key1"},
[]string{"value2", "value1"},
"8fd19b19e7bb3f2b3ee0574027d8a5a4cec370464ea2db2fbfa5c7d35bb0cff3",
"017788f37362dd0687beb59c0b3bfcc17a955120a4cb63dbdd4a0fdf9e07730e",
},
// swap order with 3 keys
{
[]string{"key1", "key2", "key3"},
[]string{"value1", "value2", "value3"},
"1dd674ec6782a0d586a903c9c63326a41cbe56b3bba33ed6ff5b527af6efb3dc",
"68f41a8a3508cb5f8eb3f1c7534a86fea9f59aa4898a5aac2f1bb92834ae2a36",
},
{
[]string{"key1", "key3", "key2"},
[]string{"value1", "value3", "value2"},
"1dd674ec6782a0d586a903c9c63326a41cbe56b3bba33ed6ff5b527af6efb3dc",
"68f41a8a3508cb5f8eb3f1c7534a86fea9f59aa4898a5aac2f1bb92834ae2a36",
},
}
for i, tc := range tests {
Expand Down
108 changes: 0 additions & 108 deletions store/rootmulti/proof.go
View file
Open in desktop
Original file line number Diff line number Diff line change
@@ -1,31 +1,10 @@
package rootmulti

import (
"bytes"
"errors"
"fmt"

"github.com/tendermint/iavl"
"github.com/tendermint/tendermint/crypto/merkle"
)

// MultiStoreProof defines a collection of store proofs in a multi-store
type MultiStoreProof struct {
StoreInfos []storeInfo
}

func NewMultiStoreProof(storeInfos []storeInfo) *MultiStoreProof {
return &MultiStoreProof{StoreInfos: storeInfos}
}

// ComputeRootHash returns the root hash for a given multi-store proof.
func (proof *MultiStoreProof) ComputeRootHash() []byte {
ci := commitInfo{
StoreInfos: proof.StoreInfos,
}
return ci.Hash()
}

// RequireProof returns whether proof is required for the subpath.
func RequireProof(subpath string) bool {
// XXX: create a better convention.
Expand All @@ -37,99 +16,12 @@ func RequireProof(subpath string) bool {

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

var _ merkle.ProofOperator = MultiStoreProofOp{}

// the multi-store proof operation constant value
const ProofOpMultiStore = "multistore"

// TODO: document
type MultiStoreProofOp struct {
// Encoded in ProofOp.Key
key []byte

// To encode in ProofOp.Data.
Proof *MultiStoreProof `json:"proof"`
}

func NewMultiStoreProofOp(key []byte, proof *MultiStoreProof) MultiStoreProofOp {
return MultiStoreProofOp{
key: key,
Proof: proof,
}
}

// MultiStoreProofOpDecoder returns a multi-store merkle proof operator from a
// given proof operation.
func MultiStoreProofOpDecoder(pop merkle.ProofOp) (merkle.ProofOperator, error) {
if pop.Type != ProofOpMultiStore {
return nil, fmt.Errorf("unexpected ProofOp.Type; got %v, want %v", pop.Type, ProofOpMultiStore)
}

// XXX: a bit strange as we'll discard this, but it works
var op MultiStoreProofOp

err := cdc.UnmarshalBinaryBare(pop.Data, &op)
if err != nil {
return nil, fmt.Errorf("decoding ProofOp.Data into MultiStoreProofOp: %w", err)
}

return NewMultiStoreProofOp(pop.Key, op.Proof), nil
}

// ProofOp return a merkle proof operation from a given multi-store proof
// operation.
func (op MultiStoreProofOp) ProofOp() merkle.ProofOp {
bz := cdc.MustMarshalBinaryBare(op)
return merkle.ProofOp{
Type: ProofOpMultiStore,
Key: op.key,
Data: bz,
}
}

// String implements the Stringer interface for a mult-store proof operation.
func (op MultiStoreProofOp) String() string {
return fmt.Sprintf("MultiStoreProofOp{%v}", op.GetKey())
}

// GetKey returns the key for a multi-store proof operation.
func (op MultiStoreProofOp) GetKey() []byte {
return op.key
}

// Run executes a multi-store proof operation for a given value. It returns
// the root hash if the value matches all the store's commitID's hash or an
// error otherwise.
func (op MultiStoreProofOp) Run(args [][]byte) ([][]byte, error) {
if len(args) != 1 {
return nil, errors.New("value size is not 1")
}

value := args[0]
root := op.Proof.ComputeRootHash()

for _, si := range op.Proof.StoreInfos {
if si.Name == string(op.key) {
if bytes.Equal(value, si.Core.CommitID.Hash) {
return [][]byte{root}, nil
}

return nil, fmt.Errorf("hash mismatch for substore %v: %X vs %X", si.Name, si.Core.CommitID.Hash, value)
}
}

return nil, fmt.Errorf("key %v not found in multistore proof", op.key)
}

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

// XXX: This should be managed by the rootMultiStore which may want to register
// more proof ops?
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(ProofOpMultiStore, MultiStoreProofOpDecoder)
Copy link
Contributor

Choose a reason for hiding this comment

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

If you can drop this one (:+1:) you an also add another proof op that returns ics23 format, no?

return
}
33 changes: 28 additions & 5 deletions store/rootmulti/store.go
View file
Open in desktop
Original file line number Diff line number Diff line change
Expand Up @@ -7,6 +7,7 @@ import (

"github.com/pkg/errors"
abci "github.com/tendermint/tendermint/abci/types"
"github.com/tendermint/tendermint/crypto/merkle"
"github.com/tendermint/tendermint/crypto/tmhash"
dbm "github.com/tendermint/tm-db"

Expand Down Expand Up @@ -456,11 +457,11 @@ func (rs *Store) Query(req abci.RequestQuery) abci.ResponseQuery {
}
}

// Get proofs for all store keys from commitInfo
popMap := commitInfo.StoreProofs()

// Restore origin path and append proof op.
res.Proof.Ops = append(res.Proof.Ops, NewMultiStoreProofOp(
[]byte(storeName),
NewMultiStoreProof(commitInfo.StoreInfos),
).ProofOp())
res.Proof.Ops = append(res.Proof.Ops, popMap[storeName].ProofOp())

// TODO: handle in another TM v0.26 update PR
// res.Proof = buildMultiStoreProof(res.Proof, storeName, commitInfo.StoreInfos)
Expand Down Expand Up @@ -572,6 +573,17 @@ func (ci commitInfo) Hash() []byte {
return SimpleHashFromMap(m)
}

// StoreProofs returns the simpleproofs for each store key
func (ci commitInfo) StoreProofs() map[string]merkle.SimpleValueOp {
// TODO: cache to ci.hash []byte
m := make(map[string][]byte, len(ci.StoreInfos))
for _, storeInfo := range ci.StoreInfos {
m[storeInfo.Name] = storeInfo.Hash()
}

return SimpleProofOpsFromMap(m)
}

func (ci commitInfo) CommitID() types.CommitID {
return types.CommitID{
Version: ci.Version,
Expand Down Expand Up @@ -609,7 +621,8 @@ func (si storeInfo) Hash() []byte {
panic(err)
}

return hasher.Sum(nil)
hash := hasher.Sum(nil)
Copy link
Contributor

Choose a reason for hiding this comment

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

Why this change?

Copy link
Member Author

Choose a reason for hiding this comment

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

Missed reverting this change during debugging

return hash
}

//----------------------------------------
Expand Down Expand Up @@ -714,3 +727,13 @@ func SimpleHashFromMap(m map[string][]byte) []byte {

return mm.hash()
}

func SimpleProofOpsFromMap(m map[string][]byte) map[string]merkle.SimpleValueOp {
Copy link
Contributor

Choose a reason for hiding this comment

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

No need to convert all the merkle.SimpleProofs into merkle.SimpleValueOp when we just use one of them.

Unless you actually use the multi-return value elsewhere (where this code would make sense), why not just:

// in real code handle the missing case better
func SimpleProofOpFromMap(m map[string][]byte, key string) merkle.SimpleValueOp {
    _, proofs, _ := merkle.SimpleProofsFromMap(m)
    return merkle.NewSimpleValueOp([]byte(key), proofs[key])
}

_, proofs, _ := merkle.SimpleProofsFromMap(m)
popMap := make(map[string]merkle.SimpleValueOp, len(proofs))

for k, p := range proofs {
popMap[k] = merkle.NewSimpleValueOp([]byte(k), p)
}
return popMap
}
2 changes: 2 additions & 0 deletions x/ibc/02-client/exported/exported.go
View file
Open in desktop
Original file line number Diff line number Diff line change
Expand Up @@ -6,6 +6,7 @@ import (

sdk "github.com/cosmos/cosmos-sdk/types"

ics23 "github.com/confio/ics23/go"
"github.com/cosmos/cosmos-sdk/codec"
evidenceexported "github.com/cosmos/cosmos-sdk/x/evidence/exported"
connectionexported "github.com/cosmos/cosmos-sdk/x/ibc/03-connection/exported"
Expand All @@ -21,6 +22,7 @@ type ClientState interface {
GetLatestHeight() uint64
IsFrozen() bool
Validate() error
GetProofSpecs() []*ics23.ProofSpec

// State verification functions

Expand Down
10 changes: 5 additions & 5 deletions x/ibc/02-client/types/querier.go
View file
Open in desktop
Original file line number Diff line number Diff line change
Expand Up @@ -3,7 +3,7 @@ package types
import (
"strings"

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

"github.com/cosmos/cosmos-sdk/x/ibc/02-client/exported"
commitmenttypes "github.com/cosmos/cosmos-sdk/x/ibc/23-commitment/types"
Expand Down Expand Up @@ -43,11 +43,11 @@ type StateResponse struct {

// NewClientStateResponse creates a new StateResponse instance.
func NewClientStateResponse(
clientID string, clientState exported.ClientState, proof *merkle.Proof, height int64,
clientID string, clientState exported.ClientState, proof []*ics23.CommitmentProof, height int64,
) StateResponse {
return StateResponse{
ClientState: clientState,
Proof: commitmenttypes.MerkleProof{Proof: proof},
Proof: commitmenttypes.MerkleProof{Proofs: proof},
ProofPath: commitmenttypes.NewMerklePath(append([]string{clientID}, strings.Split(ibctypes.ClientStatePath(), "/")...)),
ProofHeight: uint64(height),
}
Expand All @@ -64,11 +64,11 @@ type ConsensusStateResponse struct {

// NewConsensusStateResponse creates a new ConsensusStateResponse instance.
func NewConsensusStateResponse(
clientID string, cs exported.ConsensusState, proof *merkle.Proof, height int64,
clientID string, cs exported.ConsensusState, proof []*ics23.CommitmentProof, height int64,
) ConsensusStateResponse {
return ConsensusStateResponse{
ConsensusState: cs,
Proof: commitmenttypes.MerkleProof{Proof: proof},
Proof: commitmenttypes.MerkleProof{Proofs: proof},
ProofPath: commitmenttypes.NewMerklePath(append([]string{clientID}, strings.Split(ibctypes.ClientStatePath(), "/")...)),
ProofHeight: uint64(height),
}
Expand Down
10 changes: 5 additions & 5 deletions x/ibc/03-connection/types/querier.go
View file
Open in desktop
Original file line number Diff line number Diff line change
Expand Up @@ -3,7 +3,7 @@ package types
import (
"strings"

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

commitmenttypes "github.com/cosmos/cosmos-sdk/x/ibc/23-commitment/types"
ibctypes "github.com/cosmos/cosmos-sdk/x/ibc/types"
Expand All @@ -26,11 +26,11 @@ type ConnectionResponse struct {

// NewConnectionResponse creates a new ConnectionResponse instance
func NewConnectionResponse(
connectionID string, connection ConnectionEnd, proof *merkle.Proof, height int64,
connectionID string, connection ConnectionEnd, proof []*ics23.CommitmentProof, height int64,
) ConnectionResponse {
return ConnectionResponse{
Connection: connection,
Proof: commitmenttypes.MerkleProof{Proof: proof},
Proof: commitmenttypes.MerkleProof{Proofs: proof},
ProofPath: commitmenttypes.NewMerklePath(strings.Split(ibctypes.ConnectionPath(connectionID), "/")),
ProofHeight: uint64(height),
}
Expand Down Expand Up @@ -63,11 +63,11 @@ type ClientConnectionsResponse struct {

// NewClientConnectionsResponse creates a new ConnectionPaths instance
func NewClientConnectionsResponse(
clientID string, connectionPaths []string, proof *merkle.Proof, height int64,
clientID string, connectionPaths []string, proof []*ics23.CommitmentProof, height int64,
) ClientConnectionsResponse {
return ClientConnectionsResponse{
ConnectionPaths: connectionPaths,
Proof: commitmenttypes.MerkleProof{Proof: proof},
Proof: commitmenttypes.MerkleProof{Proofs: proof},
ProofPath: commitmenttypes.NewMerklePath(strings.Split(ibctypes.ClientConnectionsPath(clientID), "/")),
ProofHeight: uint64(height),
}
Expand Down
Loading