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

Fix proof verify #5313

Merged
merged 6 commits into from
Nov 20, 2019
Merged

Fix proof verify #5313

Show file tree
Hide file tree
Changes from 3 commits
Commits
Show all changes
6 commits
Select commit Hold shift + click to select a range
4b61c3f
fix poof verify
chengwenxi Nov 14, 2019
74318ca
minor cleanup
fedekunze Nov 14, 2019
5d4b1e3
fix tests
chengwenxi Nov 15, 2019
28f840f
remove key prefixes
fedekunze Nov 20, 2019
36c252a
fix tests
fedekunze Nov 20, 2019
4003c9f
Merge branch 'ibc-alpha' of https://github.com/cosmos/cosmos-sdk into…
fedekunze Nov 20, 2019
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
2 changes: 1 addition & 1 deletion client/keys/root_test.go
View file
Open in desktop
Original file line number Diff line number Diff line change
Expand Up @@ -11,5 +11,5 @@ func TestCommands(t *testing.T) {
assert.NotNil(t, rootCommands)

// Commands are registered
assert.Equal(t, 11, len(rootCommands.Commands()))
assert.Equal(t, 12, len(rootCommands.Commands()))
}
6 changes: 3 additions & 3 deletions x/ibc/03-connection/keeper/handshake.go
View file
Open in desktop
Original file line number Diff line number Diff line change
Expand Up @@ -86,7 +86,7 @@ func (k Keeper) ConnOpenTry(

ok := k.VerifyMembership(
ctx, connection, proofHeight, proofInit,
types.ConnectionPath(counterparty.ConnectionID), expConnBz,
string(types.KeyPrefixConnection)+types.ConnectionPath(counterparty.ConnectionID), expConnBz,
fedekunze marked this conversation as resolved.
Show resolved Hide resolved
)
if !ok {
return errors.New("couldn't verify connection membership on counterparty's client") // TODO: sdk.Error
Expand Down Expand Up @@ -182,7 +182,7 @@ func (k Keeper) ConnOpenAck(

ok := k.VerifyMembership(
ctx, connection, proofHeight, proofTry,
types.ConnectionPath(connection.Counterparty.ConnectionID), expConnBz,
string(types.KeyPrefixConnection)+types.ConnectionPath(connection.Counterparty.ConnectionID), expConnBz,
)
if !ok {
return errors.New("couldn't verify connection membership on counterparty's client") // TODO: sdk.Error
Expand Down Expand Up @@ -244,7 +244,7 @@ func (k Keeper) ConnOpenConfirm(

ok := k.VerifyMembership(
ctx, connection, proofHeight, proofAck,
types.ConnectionPath(connection.Counterparty.ConnectionID), expConnBz,
string(types.KeyPrefixConnection)+types.ConnectionPath(connection.Counterparty.ConnectionID), expConnBz,
)
if !ok {
return types.ErrInvalidCounterpartyConnection(k.codespace)
Expand Down
16 changes: 8 additions & 8 deletions x/ibc/03-connection/keeper/handshake_test.go
View file
Open in desktop
Original file line number Diff line number Diff line change
Expand Up @@ -52,8 +52,8 @@ func (suite *KeeperTestSuite) TestConnOpenTry() {
suite.createClient(testClientID1)
suite.createConnection(testConnectionID2, testConnectionID1, testClientID2, testClientID1, connection.INIT)

connectionKey := connection.ConnectionPath(testConnectionID2)
consensusKey := string(client.KeyConsensusState(testClientID2))
connectionKey := keyPrefix + connection.ConnectionPath(testConnectionID2)
fedekunze marked this conversation as resolved.
Show resolved Hide resolved
consensusKey := keyPrefix + string(client.KeyConsensusState(testClientID2))

invalidProof := func() error {
proofInit, proofHeight := suite.queryProof(connectionKey)
Expand Down Expand Up @@ -84,7 +84,7 @@ func (suite *KeeperTestSuite) TestConnOpenTry() {
//check connection state
conn, existed := suite.app.IBCKeeper.ConnectionKeeper.GetConnection(suite.ctx, testConnectionID1)
suite.True(existed)
suite.Equal(connection.TRYOPEN, conn.State)
suite.Equal(connection.TRYOPEN.String(), conn.State.String(), "invalid connection state")
fedekunze marked this conversation as resolved.
Show resolved Hide resolved
return err
}

Expand Down Expand Up @@ -119,8 +119,8 @@ func (suite *KeeperTestSuite) TestConnOpenAck() {
suite.createClient(testClientID1)

suite.createConnection(testConnectionID1, testConnectionID2, testClientID1, testClientID2, connection.TRYOPEN)
connectionKey := connection.ConnectionPath(testConnectionID1)
consensusKey := string(client.KeyConsensusState(testClientID1))
connectionKey := keyPrefix + connection.ConnectionPath(testConnectionID1)
consensusKey := keyPrefix + string(client.KeyConsensusState(testClientID1))

connectionNotFound := func() error {
//suite.updateClient(testClientID2)
Expand Down Expand Up @@ -172,7 +172,7 @@ func (suite *KeeperTestSuite) TestConnOpenAck() {
//check connection state
conn, existed := suite.app.IBCKeeper.ConnectionKeeper.GetConnection(suite.ctx, testConnectionID2)
suite.True(existed)
suite.Equal(connection.OPEN, conn.State)
suite.Equal(connection.OPEN.String(), conn.State.String(), "invalid connection state")
return err

}
Expand All @@ -196,7 +196,7 @@ func (suite *KeeperTestSuite) TestConnOpenConfirm() {
suite.createClient(testClientID1)
suite.createConnection(testConnectionID2, testConnectionID1, testClientID2, testClientID1, connection.OPEN)

connKey := connection.ConnectionPath(testConnectionID2)
connKey := keyPrefix + connection.ConnectionPath(testConnectionID2)
proof, h := suite.queryProof(connKey)

connectionNotFound := func() error {
Expand Down Expand Up @@ -228,7 +228,7 @@ func (suite *KeeperTestSuite) TestConnOpenConfirm() {
//check connection state
conn, existed := suite.app.IBCKeeper.ConnectionKeeper.GetConnection(suite.ctx, testConnectionID1)
suite.True(existed)
suite.Equal(connection.OPEN, conn.State)
suite.Equal(connection.OPEN.String(), conn.State.String(), "invalid connection state")
return err
}

Expand Down
15 changes: 0 additions & 15 deletions x/ibc/03-connection/keeper/keeper.go
View file
Open in desktop
Original file line number Diff line number Diff line change
Expand Up @@ -128,13 +128,6 @@ func (k Keeper) VerifyMembership(
pathStr string,
value []byte,
) bool {
// FIXME: commented out for demo
fedekunze marked this conversation as resolved.
Show resolved Hide resolved
/*
clientState, found := k.clientKeeper.GetClientState(ctx, connection.ClientID)
if !found {
return false
}
*/
path, err := commitment.ApplyPrefix(connection.Counterparty.Prefix, pathStr)
if err != nil {
return false
Expand All @@ -151,14 +144,6 @@ func (k Keeper) VerifyNonMembership(
proof commitment.ProofI,
pathStr string,
) bool {
// FIXME: commented out for demo
/*
clientState, found := k.clientKeeper.GetClientState(ctx, connection.ClientID)
if !found {
return false
}
*/

path, err := commitment.ApplyPrefix(connection.Counterparty.Prefix, pathStr)
if err != nil {
return false
Expand Down
2 changes: 2 additions & 0 deletions x/ibc/03-connection/keeper/keeper_test.go
View file
Open in desktop
Original file line number Diff line number Diff line change
Expand Up @@ -26,6 +26,8 @@ const (
testConnectionID2 = "connectionid2"
)

var keyPrefix = string([]byte{0x00})

type KeeperTestSuite struct {
suite.Suite

Expand Down
8 changes: 4 additions & 4 deletions x/ibc/04-channel/keeper/handshake.go
View file
Open in desktop
Original file line number Diff line number Diff line change
Expand Up @@ -135,7 +135,7 @@ func (k Keeper) ChanOpenTry(

if !k.connectionKeeper.VerifyMembership(
ctx, connectionEnd, proofHeight, proofInit,
types.ChannelPath(counterparty.PortID, counterparty.ChannelID),
string(types.KeyPrefixChannel)+types.ChannelPath(counterparty.PortID, counterparty.ChannelID),
bz,
) {
return types.ErrInvalidCounterpartyChannel(k.codespace, "channel membership verification failed")
Expand Down Expand Up @@ -211,7 +211,7 @@ func (k Keeper) ChanOpenAck(

if !k.connectionKeeper.VerifyMembership(
ctx, connectionEnd, proofHeight, proofTry,
types.ChannelPath(channel.Counterparty.PortID, channel.Counterparty.ChannelID),
string(types.KeyPrefixChannel)+types.ChannelPath(channel.Counterparty.PortID, channel.Counterparty.ChannelID),
bz,
) {
return types.ErrInvalidCounterpartyChannel(k.codespace, "channel membership verification failed")
Expand Down Expand Up @@ -283,7 +283,7 @@ func (k Keeper) ChanOpenConfirm(

if !k.connectionKeeper.VerifyMembership(
ctx, connectionEnd, proofHeight, proofAck,
types.ChannelPath(channel.Counterparty.PortID, channel.Counterparty.ChannelID),
string(types.KeyPrefixChannel)+types.ChannelPath(channel.Counterparty.PortID, channel.Counterparty.ChannelID),
bz,
) {
return types.ErrInvalidCounterpartyChannel(k.codespace, "channel membership verification failed")
Expand Down Expand Up @@ -397,7 +397,7 @@ func (k Keeper) ChanCloseConfirm(

if !k.connectionKeeper.VerifyMembership(
ctx, connectionEnd, proofHeight, proofInit,
types.ChannelPath(channel.Counterparty.PortID, channel.Counterparty.ChannelID),
string(types.KeyPrefixChannel)+types.ChannelPath(channel.Counterparty.PortID, channel.Counterparty.ChannelID),
bz,
) {
return types.ErrInvalidCounterpartyChannel(k.codespace, "channel membership verification failed")
Expand Down
22 changes: 11 additions & 11 deletions x/ibc/04-channel/keeper/handshake_test.go
View file
Open in desktop
Original file line number Diff line number Diff line change
Expand Up @@ -81,7 +81,7 @@ func (suite *KeeperTestSuite) createChannel(portID string, chanID string, connID

func (suite *KeeperTestSuite) deleteChannel(portID string, chanID string) {
store := prefix.NewStore(suite.ctx.KVStore(suite.app.GetKey(ibctypes.StoreKey)), []byte{})
store.Delete(types.KeyChannel(portID, chanID))
store.Delete(append([]byte(keyPrefix), types.KeyChannel(portID, chanID)...))
}

func (suite *KeeperTestSuite) bindPort(portID string) sdk.CapabilityKey {
Expand Down Expand Up @@ -124,13 +124,13 @@ func (suite *KeeperTestSuite) TestChanOpenInit() {

channel, found := suite.app.IBCKeeper.ChannelKeeper.GetChannel(suite.ctx, testPort1, testChannel1)
suite.True(found)
suite.Equal(types.INIT, channel.State)
suite.Equal(types.INIT.String(), channel.State.String(), "invalid channel state")
}

func (suite *KeeperTestSuite) TestChanOpenTry() {
counterparty := types.NewCounterparty(testPort1, testChannel1)
suite.bindPort(testPort2)
channelKey := types.ChannelPath(testPort1, testChannel1)
channelKey := keyPrefix + types.ChannelPath(testPort1, testChannel1)

suite.createChannel(testPort1, testChannel1, testConnection, testPort2, testChannel2, types.INIT)
suite.createChannel(testPort2, testChannel2, testConnection, testPort1, testChannel1, types.INIT)
Expand Down Expand Up @@ -165,12 +165,12 @@ func (suite *KeeperTestSuite) TestChanOpenTry() {

channel, found := suite.app.IBCKeeper.ChannelKeeper.GetChannel(suite.ctx, testPort2, testChannel2)
suite.True(found)
suite.Equal(types.OPENTRY, channel.State)
suite.Equal(types.OPENTRY.String(), channel.State.String(), "invalid channel state")
}

func (suite *KeeperTestSuite) TestChanOpenAck() {
suite.bindPort(testPort1)
channelKey := types.ChannelPath(testPort2, testChannel2)
channelKey := keyPrefix + types.ChannelPath(testPort2, testChannel2)

suite.createChannel(testPort2, testChannel2, testConnection, testPort1, testChannel1, types.OPENTRY)
suite.updateClient()
Expand Down Expand Up @@ -209,12 +209,12 @@ func (suite *KeeperTestSuite) TestChanOpenAck() {

channel, found := suite.app.IBCKeeper.ChannelKeeper.GetChannel(suite.ctx, testPort1, testChannel1)
suite.True(found)
suite.Equal(types.OPEN, channel.State)
suite.Equal(types.OPEN.String(), channel.State.String(), "invalid channel state")
}

func (suite *KeeperTestSuite) TestChanOpenConfirm() {
suite.bindPort(testPort2)
channelKey := types.ChannelPath(testPort1, testChannel1)
channelKey := keyPrefix + types.ChannelPath(testPort1, testChannel1)

suite.createChannel(testPort1, testChannel1, testConnection, testPort2, testChannel2, types.OPEN)
suite.updateClient()
Expand Down Expand Up @@ -253,7 +253,7 @@ func (suite *KeeperTestSuite) TestChanOpenConfirm() {

channel, found := suite.app.IBCKeeper.ChannelKeeper.GetChannel(suite.ctx, testPort2, testChannel2)
suite.True(found)
suite.Equal(types.OPEN, channel.State)
suite.Equal(types.OPEN.String(), channel.State.String(), "invalid channel state")
}

func (suite *KeeperTestSuite) TestChanCloseInit() {
Expand Down Expand Up @@ -283,12 +283,12 @@ func (suite *KeeperTestSuite) TestChanCloseInit() {

channel, found := suite.app.IBCKeeper.ChannelKeeper.GetChannel(suite.ctx, testPort1, testChannel1)
suite.True(found)
suite.Equal(types.CLOSED, channel.State)
suite.Equal(types.CLOSED.String(), channel.State.String(), "invalid channel state")
}

func (suite *KeeperTestSuite) TestChanCloseConfirm() {
suite.bindPort(testPort2)
channelKey := types.ChannelPath(testPort1, testChannel1)
channelKey := keyPrefix + types.ChannelPath(testPort1, testChannel1)

suite.createChannel(testPort1, testChannel1, testConnection, testPort2, testChannel2, types.CLOSED)
suite.updateClient()
Expand Down Expand Up @@ -326,5 +326,5 @@ func (suite *KeeperTestSuite) TestChanCloseConfirm() {

channel, found := suite.app.IBCKeeper.ChannelKeeper.GetChannel(suite.ctx, testPort2, testChannel2)
suite.True(found)
suite.Equal(types.CLOSED, channel.State)
suite.Equal(types.CLOSED.String(), channel.State.String(), "invalid channel state")
}
2 changes: 2 additions & 0 deletions x/ibc/04-channel/keeper/keeper_test.go
View file
Open in desktop
Original file line number Diff line number Diff line change
Expand Up @@ -29,6 +29,8 @@ const (
testChannelVersion = "1.0"
)

var keyPrefix = string([]byte{0x00})

type KeeperTestSuite struct {
suite.Suite

Expand Down
4 changes: 4 additions & 0 deletions x/ibc/05-port/keeper/keeper.go
View file
Open in desktop
Original file line number Diff line number Diff line change
Expand Up @@ -60,5 +60,9 @@ func (k Keeper) Authenticate(key sdk.CapabilityKey, portID string) bool {
panic(err.Error())
}

if key.Name() != portID {
return false
}

Comment on lines +63 to +66
Copy link
Contributor Author

Choose a reason for hiding this comment

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

I think there should add some complementary check, or panic? @fedekunze @AdityaSripal

Copy link
Member

Choose a reason for hiding this comment

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

What do you mean by complementary check?

Copy link
Contributor Author

@chengwenxi chengwenxi Nov 20, 2019 •
edited
Loading

Choose a reason for hiding this comment

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

On BindPort, the key.Name() is equal to the portID. So I think we should add this check, it is what the test case expects, refer

cosmos-sdk/x/ibc/05-port/keeper/keeper_test.go

Line 64 in 5d4b1e3

// Test that authenticating against incorrect portid fails

return k.ports[key.Name()]
}
7 changes: 2 additions & 5 deletions x/ibc/05-port/keeper/keeper_test.go
View file
Open in desktop
Original file line number Diff line number Diff line change
@@ -1,9 +1,10 @@
package keeper_test

import (
"testing"

"github.com/stretchr/testify/require"
"github.com/stretchr/testify/suite"
"testing"

"github.com/cosmos/cosmos-sdk/codec"
"github.com/cosmos/cosmos-sdk/simapp"
Expand Down Expand Up @@ -46,10 +47,6 @@ func (suite *KeeperTestSuite) TestBind() {
capKey := suite.keeper.BindPort(validPort)
require.NotNil(suite.T(), capKey, "capabilityKey is nil on valid BindPort")

// Test that port is added to bound list after BindPort is called
ports := suite.keeper.GetPorts()
require.Equal(suite.T(), []string{validPort}, ports, "portID not added to bound list")

// Test that rebinding the same portid causes panic
require.Panics(suite.T(), func() { suite.keeper.BindPort(validPort) }, "did not panic on re-binding the same port")
}
Expand Down
22 changes: 10 additions & 12 deletions x/ibc/23-commitment/merkle.go
View file
Open in desktop
Original file line number Diff line number Diff line change
Expand Up @@ -2,12 +2,9 @@ package commitment

import (
"errors"
"net/url"

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

"github.com/cosmos/cosmos-sdk/store/rootmulti"
host "github.com/cosmos/cosmos-sdk/x/ibc/24-host"
)

// ICS 023 Merkle Types Implementation
Expand Down Expand Up @@ -93,11 +90,11 @@ func (Path) GetCommitmentType() Type {

// String implements fmt.Stringer. It returns unescaped path of the URL string.
func (p Path) String() string {
path, err := url.PathUnescape(p.KeyPath.String())
if err != nil {
panic(err)
}
return path
//path, err := url.PathUnescape(p.KeyPath.String())
Copy link
Collaborator

Choose a reason for hiding this comment

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

I see a new test failing here:

--- FAIL: TestApplyPrefix (0.00s)
    merkle_test.go:115: 
        	Error Trace:	merkle_test.go:115
        	Error:      	Not equal: 
        	            	expected: "/storePrefixKey/path1/path2/path3/key"
        	            	actual  : "/storePrefixKey/path1%2Fpath2%2Fpath3%2Fkey"
        	            	
        	            	Diff:
        	            	--- Expected
        	            	+++ Actual
        	            	@@ -1 +1 @@
        	            	-/storePrefixKey/path1/path2/path3/key
        	            	+/storePrefixKey/path1%2Fpath2%2Fpath3%2Fkey
        	Test:       	TestApplyPrefix
        	Messages:   	Prefixed path incorrect

the problem is that p.KeyPath.String() does not represent the real path. That's why I added the url.PathUnescape method.

cc: @AdityaSripal

Copy link
Contributor Author

Choose a reason for hiding this comment

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

tendermint/merkel use the / as separator to split the multiple keys, p.KeyPath.String() convert the key can avoid the confusion.

Copy link
Member

Choose a reason for hiding this comment

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

I believe the original method was correct. Keypath should be separated by /. Not sure what you mean here by multiple keys

Copy link
Contributor Author

@chengwenxi chengwenxi Nov 20, 2019 •
edited
Loading

Choose a reason for hiding this comment

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

The Keypath is an array of Key which I mentioned multiple keys.

type Key struct {
	name []byte
	enc  keyEncoding
}

type KeyPath []Key

But the Key.name may contain /, so KeyPath.String() method escape the Key.name.
In this test case, there are two Keys with name storePrefixKey and path1/path2/path3/key. If we not escape the Key.name, we can't distinguish the Key and Key.name.

//if err != nil {
// panic(err)
//}
return p.KeyPath.String()
}

// ApplyPrefix constructs a new commitment path from the arguments. It interprets
Expand All @@ -106,10 +103,11 @@ func (p Path) String() string {
// CONTRACT: provided path string MUST be a well formated path. See ICS24 for
// reference.
func ApplyPrefix(prefix PrefixI, path string) (Path, error) {
err := host.DefaultPathValidator(path)
if err != nil {
return Path{}, err
}
// TODO: fix path check
Copy link
Collaborator

Choose a reason for hiding this comment

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

what is failing actually failing here?

Copy link
Contributor Author

@chengwenxi chengwenxi Nov 14, 2019 •
edited
Loading

Choose a reason for hiding this comment

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

yeah, host.DefaultPathValidator will check the path isAlphaNumeric, but it has the prefix []byte{0x00}. So I wonder if we can revert the prefix using submodule_name, such as connection, channel, etc.

func DefaultPathValidator(path string) error {
	pathArr := strings.Split(path, "/")
	for _, p := range pathArr {
		// Each path element must either be alphanumeric
		if !isAlphaNumeric(p) {
			return sdkerrors.Wrapf(ErrInvalidPath(DefaultCodespace, path), "invalid path element containing non-alphanumeric characters: %s", p)
		}
	}
	return nil
}

//err := host.DefaultPathValidator(path)
//if err != nil {
// return Path{}, err
//}

if prefix == nil || len(prefix.Bytes()) == 0 {
return Path{}, errors.New("prefix can't be empty")
Expand Down
4 changes: 4 additions & 0 deletions x/ibc/24-host/validate.go
View file
Open in desktop
Original file line number Diff line number Diff line change
Expand Up @@ -88,6 +88,10 @@ func NewPathValidator(idValidator ValidateFn) ValidateFn {
// checking that all path elements are alphanumeric
func DefaultPathValidator(path string) error {
pathArr := strings.Split(path, "/")
if pathArr[0] == path {
return sdkerrors.Wrap(ErrInvalidPath(DefaultCodespace, path), "path doesn't contain any separator '/'")
}

for _, p := range pathArr {
// Each path element must either be alphanumeric
if !isAlphaNumeric(p) {
Expand Down