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

WIP: Fix ICS23-Proofs #6178

Closed
wants to merge 9 commits into from
1 change: 1 addition & 0 deletions go.mod
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
3 changes: 3 additions & 0 deletions go.sum
Original file line number Diff line number Diff line change
Expand Up @@ -65,6 +65,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
2 changes: 2 additions & 0 deletions x/ibc/02-client/exported/exported.go
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
29 changes: 25 additions & 4 deletions x/ibc/07-tendermint/types/client_state.go
Original file line number Diff line number Diff line change
Expand Up @@ -17,6 +17,8 @@ import (
commitmenttypes "github.com/cosmos/cosmos-sdk/x/ibc/23-commitment/types"
host "github.com/cosmos/cosmos-sdk/x/ibc/24-host"
ibctypes "github.com/cosmos/cosmos-sdk/x/ibc/types"

ics23 "github.com/confio/ics23/go"
)

var _ clientexported.ClientState = ClientState{}
Expand All @@ -38,6 +40,9 @@ type ClientState struct {
// the future.
MaxClockDrift time.Duration

// ProofSpecs determines the proof format this client will verify against
ProofSpecs []*ics23.ProofSpecs

// Block height when the client was frozen due to a misbehaviour
FrozenHeight uint64 `json:"frozen_height" yaml:"frozen_height"`

Expand All @@ -48,27 +53,27 @@ type ClientState struct {
// InitializeFromMsg creates a tendermint client state from a CreateClientMsg
func InitializeFromMsg(msg MsgCreateClient) (ClientState, error) {
return Initialize(
msg.GetClientID(), msg.TrustingPeriod, msg.UnbondingPeriod, msg.MaxClockDrift, msg.Header,
msg.GetClientID(), msg.TrustingPeriod, msg.UnbondingPeriod, msg.MaxClockDrift, msg.Header, msg.ProofSpecs,
)
}

// Initialize creates a client state and validates its contents, checking that
// the provided consensus state is from the same client type.
func Initialize(
id string, trustingPeriod, ubdPeriod, maxClockDrift time.Duration, header Header,
id string, trustingPeriod, ubdPeriod, maxClockDrift time.Duration, header Header, specs []*ics23.ProofSpecs,
) (ClientState, error) {

if trustingPeriod >= ubdPeriod {
return ClientState{}, errors.New("trusting period should be < unbonding period")
}

clientState := NewClientState(id, trustingPeriod, ubdPeriod, maxClockDrift, header)
clientState := NewClientState(id, trustingPeriod, ubdPeriod, maxClockDrift, header, specs)
return clientState, nil
}

// NewClientState creates a new ClientState instance
func NewClientState(
id string, trustingPeriod, ubdPeriod, maxClockDrift time.Duration, header Header,
id string, trustingPeriod, ubdPeriod, maxClockDrift time.Duration, header Header, specs []*ics23.ProofSpecs,
) ClientState {

return ClientState{
Expand All @@ -77,6 +82,7 @@ func NewClientState(
UnbondingPeriod: ubdPeriod,
MaxClockDrift: maxClockDrift,
LastHeader: header,
ProofSpecs: specs,
FrozenHeight: 0,
}
}
Expand Down Expand Up @@ -109,6 +115,11 @@ func (cs ClientState) GetLatestTimestamp() time.Time {
return cs.LastHeader.Time
}

// GetProofSpecs returns proof specs
func (cs ClientState) GetProofSpecs() []*ics23.ProofSpec {
return cs.ProofSpecs
}

// IsFrozen returns true if the frozen height has been set.
func (cs ClientState) IsFrozen() bool {
return cs.FrozenHeight != 0
Expand All @@ -128,6 +139,14 @@ func (cs ClientState) Validate() error {
if cs.MaxClockDrift == 0 {
return errors.New("max clock drift cannot be zero")
}
if len(cs.ProofSpecs) == 0 {
return errors.New("proof specs cannot be empty")
}
for i, ps := range cs.ProofSpecs {
if ps == nil {
return fmt.Errorf("proof spec %d cannot be nil", i)
}
}
return cs.LastHeader.ValidateBasic(cs.GetChainID())
}

Expand Down Expand Up @@ -158,6 +177,8 @@ func (cs ClientState) VerifyClientConsensusState(
return err
}

// TODO: Inject proof specs of clientstate into proof or modify verify membership function to take proof specs
Copy link
Member Author

Choose a reason for hiding this comment

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

As @ethanfrey mentioned, the proof spec needs to be fixed by the client not passed in by the user. I can either inject the clientstate proofspecs into the MerkleProof struct (replacing whatever user passed in). Or i can remove specs from MerkleProof and pass it into the VerifyMembership function.

The latter requires a change in ICS spec

Copy link
Contributor

Choose a reason for hiding this comment

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

We should change ICS 23 to not put ProofSpec in the proofs

If this requires a spec change as well, we can do that


if err := proof.VerifyMembership(provingRoot, path, bz); err != nil {
return sdkerrors.Wrap(clienttypes.ErrFailedClientConsensusStateVerification, err.Error())
}
Expand Down
1 change: 1 addition & 0 deletions x/ibc/07-tendermint/types/errors.go
Original file line number Diff line number Diff line change
Expand Up @@ -13,4 +13,5 @@ var (
ErrInvalidTrustingPeriod = sdkerrors.Register(SubModuleName, 2, "invalid trusting period")
ErrInvalidUnbondingPeriod = sdkerrors.Register(SubModuleName, 3, "invalid unbonding period")
ErrInvalidHeader = sdkerrors.Register(SubModuleName, 4, "invalid header")
ErrInvalidProofSpecs = sdkerrors.Register(SubModuleName, 5, "invalid proof specs")
)
27 changes: 20 additions & 7 deletions x/ibc/07-tendermint/types/msgs.go
Original file line number Diff line number Diff line change
Expand Up @@ -11,6 +11,8 @@ import (
commitmenttypes "github.com/cosmos/cosmos-sdk/x/ibc/23-commitment/types"
host "github.com/cosmos/cosmos-sdk/x/ibc/24-host"
ibctypes "github.com/cosmos/cosmos-sdk/x/ibc/types"

ics23 "github.com/confio/ics23/go"
)

// Message types for the IBC client
Expand All @@ -28,18 +30,20 @@ var (

// MsgCreateClient defines a message to create an IBC client
type MsgCreateClient struct {
ClientID string `json:"client_id" yaml:"client_id"`
Header Header `json:"header" yaml:"header"`
TrustingPeriod time.Duration `json:"trusting_period" yaml:"trusting_period"`
UnbondingPeriod time.Duration `json:"unbonding_period" yaml:"unbonding_period"`
MaxClockDrift time.Duration `json:"max_clock_drift" yaml:"max_clock_drift"`
Signer sdk.AccAddress `json:"address" yaml:"address"`
ClientID string `json:"client_id" yaml:"client_id"`
Header Header `json:"header" yaml:"header"`
TrustingPeriod time.Duration `json:"trusting_period" yaml:"trusting_period"`
UnbondingPeriod time.Duration `json:"unbonding_period" yaml:"unbonding_period"`
MaxClockDrift time.Duration `json:"max_clock_drift" yaml:"max_clock_drift"`
ProofSpecs []*ics23.ProofSpec `json:"proof_specs" yaml:"proof_specs"`
Signer sdk.AccAddress `json:"address" yaml:"address"`
}

// NewMsgCreateClient creates a new MsgCreateClient instance
func NewMsgCreateClient(
id string, header Header,
trustingPeriod, unbondingPeriod, maxClockDrift time.Duration, signer sdk.AccAddress,
trustingPeriod, unbondingPeriod, maxClockDrift time.Duration,
specs []*ics23.ProofSpec, signer sdk.AccAddress,
) MsgCreateClient {

return MsgCreateClient{
Expand All @@ -48,6 +52,7 @@ func NewMsgCreateClient(
TrustingPeriod: trustingPeriod,
UnbondingPeriod: unbondingPeriod,
MaxClockDrift: maxClockDrift,
ProofSpecs: specs,
Signer: signer,
}
}
Expand Down Expand Up @@ -80,6 +85,14 @@ func (msg MsgCreateClient) ValidateBasic() error {
if err := msg.Header.ValidateBasic(msg.Header.ChainID); err != nil {
return sdkerrors.Wrapf(ErrInvalidHeader, "header failed validatebasic with its own chain-id: %v", err)
}
if len(msg.ProofSpecs) == 0 {
return sdkerrors.Wrap(ErrInvalidProofSpecs, "proof specs cannot be empty for tendermint client")
}
for i, ps := range msg.ProofSpecs {
if ps == nil {
return fmt.Errorf("proof spec %d cannot be nil", i)
}
}
return host.DefaultClientIdentifierValidator(msg.ClientID)
}

Expand Down
7 changes: 7 additions & 0 deletions x/ibc/09-localhost/types/client_state.go
Original file line number Diff line number Diff line change
Expand Up @@ -19,6 +19,8 @@ import (
commitmenttypes "github.com/cosmos/cosmos-sdk/x/ibc/23-commitment/types"
host "github.com/cosmos/cosmos-sdk/x/ibc/24-host"
ibctypes "github.com/cosmos/cosmos-sdk/x/ibc/types"

ics23 "github.com/confio/ics23/go"
)

var _ clientexported.ClientState = ClientState{}
Expand Down Expand Up @@ -61,6 +63,11 @@ func (cs ClientState) GetLatestHeight() uint64 {
return uint64(cs.Height)
}

// GetProofSpecs implements ClientState interface
func (cs ClientState) GetProofSpecs() []*ics23.ProofSpec {
return nil
}

// IsFrozen returns false.
func (cs ClientState) IsFrozen() bool {
return false
Expand Down
Loading