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

Jump to bottom

increase ibc basic genesis validation #8081

Merged
merged 7 commits into from
Dec 5, 2020
Merged

increase ibc basic genesis validation #8081

Show file tree
Hide file tree
Changes from all commits
Commits
Show all changes
7 commits
Select commit Hold shift + click to select a range
aa7c0be
increase client basic genesis validation
colin-axner Dec 4, 2020
464a4fa
add connection/channel genesis validation
colin-axner Dec 4, 2020
26513d0
Merge branch 'master' into colin/8073-ibc-init-gen-validation
colin-axner Dec 4, 2020
93e7edb
fix tests
colin-axner Dec 4, 2020
4147b7e
apply @AdityaSripal suggestion
colin-axner Dec 4, 2020
dd772cc
Merge branch 'master' into colin/8073-ibc-init-gen-validation
colin-axner Dec 4, 2020
ca916de
Merge branch 'master' into colin/8073-ibc-init-gen-validation
cwgoes Dec 4, 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
39 changes: 36 additions & 3 deletions x/ibc/core/02-client/types/genesis.go
View file
Open in desktop
Original file line number Diff line number Diff line change
Expand Up @@ -103,11 +103,15 @@ func (gs GenesisState) UnpackInterfaces(unpacker codectypes.AnyUnpacker) error {
// Validate performs basic genesis state validation returning an error upon any
// failure.
func (gs GenesisState) Validate() error {
// keep track of the max sequence to ensure it is less than
// the next sequence used in creating client identifers.
var maxSequence uint64 = 0

if err := gs.Params.Validate(); err != nil {
return err
}

validClients := make(map[string]bool)
validClients := make(map[string]string)

for i, client := range gs.Clients {
if err := host.ClientIdentifierValidator(client.ClientId); err != nil {
Expand All @@ -126,13 +130,31 @@ func (gs GenesisState) Validate() error {
return fmt.Errorf("invalid client %v index %d: %w", client, i, err)
}

clientType, sequence, err := ParseClientIdentifier(client.ClientId)
if err != nil {
return err
}

if clientType != clientState.ClientType() {
return fmt.Errorf("client state type %s does not equal client type in client identifier %s", clientState.ClientType(), clientType)
}

if err := ValidateClientType(clientType); err != nil {
return err
}

if sequence > maxSequence {
maxSequence = sequence
}

// add client id to validClients map
validClients[client.ClientId] = true
validClients[client.ClientId] = clientState.ClientType()
}

for i, cc := range gs.ClientsConsensus {
// check that consensus state is for a client in the genesis clients list
if !validClients[cc.ClientId] {
clientType, ok := validClients[cc.ClientId]
if !ok {
return fmt.Errorf("consensus state in genesis has a client id %s that does not map to a genesis client", cc.ClientId)
}

Expand All @@ -149,12 +171,23 @@ func (gs GenesisState) Validate() error {
if err := cs.ValidateBasic(); err != nil {
return fmt.Errorf("invalid client consensus state %v index %d: %w", cs, i, err)
}

// ensure consensus state type matches client state type
if clientType != cs.ClientType() {
return fmt.Errorf("consensus state client type %s does not equal client state client type %s", cs.ClientType(), clientType)
}

}

}

if gs.CreateLocalhost && !gs.Params.IsAllowedClient(exported.Localhost) {
return fmt.Errorf("localhost client is not registered on the allowlist")
}

if maxSequence != 0 && maxSequence >= gs.NextClientSequence {
Copy link
Member

Choose a reason for hiding this comment

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

Should we be checking that if maxSequence == 0 then gs.NextClientSequence == 0 as well?

And should we check that if maxSequence != 0, then gs.NextClientSequence == maxSequence + 1?

Or no?

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 don't think we need to put such strict restrictions on this. I think there is value in allowing the next client sequence to be very different from the maximum sequence. For example, if during a genesis migration, validators decide to prune old expired clients. Or if genesis validators decide to create clients using identifiers in genesis from 0-900 and start regular clients at 1000 to distinguish their origins

return fmt.Errorf("next client identifier sequence %d must be greater than the maximum sequence used in the provided client identifiers %d", gs.NextClientSequence, maxSequence)
}

return nil
}
138 changes: 114 additions & 24 deletions x/ibc/core/02-client/types/genesis_test.go
View file
Open in desktop
Original file line number Diff line number Diff line change
Expand Up @@ -17,8 +17,11 @@ import (
)

const (
chainID = "chainID"
clientID = "ethbridge"
chainID = "chainID"
tmClientID0 = "07-tendermint-0"
tmClientID1 = "07-tendermint-1"
invalidClientID = "myclient-0"
clientID = tmClientID0

height = 10
)
Expand Down Expand Up @@ -69,15 +72,15 @@ func (suite *TypesTestSuite) TestValidateGenesis() {
genState: types.NewGenesisState(
[]types.IdentifiedClientState{
types.NewIdentifiedClientState(
clientID, ibctmtypes.NewClientState(chainID, ibctesting.DefaultTrustLevel, ibctesting.TrustingPeriod, ibctesting.UnbondingPeriod, ibctesting.MaxClockDrift, clientHeight, commitmenttypes.GetSDKSpecs(), ibctesting.UpgradePath, false, false),
tmClientID0, ibctmtypes.NewClientState(chainID, ibctesting.DefaultTrustLevel, ibctesting.TrustingPeriod, ibctesting.UnbondingPeriod, ibctesting.MaxClockDrift, clientHeight, commitmenttypes.GetSDKSpecs(), ibctesting.UpgradePath, false, false),
),
types.NewIdentifiedClientState(
exported.Localhost, localhosttypes.NewClientState("chainID", clientHeight),
exported.Localhost+"-1", localhosttypes.NewClientState("chainID", clientHeight),
),
},
[]types.ClientConsensusStates{
types.NewClientConsensusStates(
clientID,
tmClientID0,
[]types.ConsensusStateWithHeight{
types.NewConsensusStateWithHeight(
header.GetHeight().(types.Height),
Expand All @@ -90,7 +93,7 @@ func (suite *TypesTestSuite) TestValidateGenesis() {
},
types.NewParams(exported.Tendermint, exported.Localhost),
false,
0,
2,
),
expPass: true,
},
Expand All @@ -99,15 +102,15 @@ func (suite *TypesTestSuite) TestValidateGenesis() {
genState: types.NewGenesisState(
[]types.IdentifiedClientState{
types.NewIdentifiedClientState(
"/~@$*", ibctmtypes.NewClientState(chainID, ibctmtypes.DefaultTrustLevel, ibctesting.TrustingPeriod, ibctesting.UnbondingPeriod, ibctesting.MaxClockDrift, clientHeight, commitmenttypes.GetSDKSpecs(), ibctesting.UpgradePath, false, false),
invalidClientID, ibctmtypes.NewClientState(chainID, ibctmtypes.DefaultTrustLevel, ibctesting.TrustingPeriod, ibctesting.UnbondingPeriod, ibctesting.MaxClockDrift, clientHeight, commitmenttypes.GetSDKSpecs(), ibctesting.UpgradePath, false, false),
),
types.NewIdentifiedClientState(
exported.Localhost, localhosttypes.NewClientState("chainID", clientHeight),
),
},
[]types.ClientConsensusStates{
types.NewClientConsensusStates(
"/~@$*",
invalidClientID,
[]types.ConsensusStateWithHeight{
types.NewConsensusStateWithHeight(
header.GetHeight().(types.Height),
Expand All @@ -129,7 +132,7 @@ func (suite *TypesTestSuite) TestValidateGenesis() {
genState: types.NewGenesisState(
[]types.IdentifiedClientState{
types.NewIdentifiedClientState(
clientID, ibctmtypes.NewClientState(chainID, ibctmtypes.DefaultTrustLevel, ibctesting.TrustingPeriod, ibctesting.UnbondingPeriod, ibctesting.MaxClockDrift, clientHeight, commitmenttypes.GetSDKSpecs(), ibctesting.UpgradePath, false, false),
tmClientID0, ibctmtypes.NewClientState(chainID, ibctmtypes.DefaultTrustLevel, ibctesting.TrustingPeriod, ibctesting.UnbondingPeriod, ibctesting.MaxClockDrift, clientHeight, commitmenttypes.GetSDKSpecs(), ibctesting.UpgradePath, false, false),
),
types.NewIdentifiedClientState(exported.Localhost, localhosttypes.NewClientState("chaindID", types.ZeroHeight())),
},
Expand All @@ -145,15 +148,15 @@ func (suite *TypesTestSuite) TestValidateGenesis() {
genState: types.NewGenesisState(
[]types.IdentifiedClientState{
types.NewIdentifiedClientState(
clientID, ibctmtypes.NewClientState(chainID, ibctmtypes.DefaultTrustLevel, ibctesting.TrustingPeriod, ibctesting.UnbondingPeriod, ibctesting.MaxClockDrift, clientHeight, commitmenttypes.GetSDKSpecs(), ibctesting.UpgradePath, false, false),
tmClientID0, ibctmtypes.NewClientState(chainID, ibctmtypes.DefaultTrustLevel, ibctesting.TrustingPeriod, ibctesting.UnbondingPeriod, ibctesting.MaxClockDrift, clientHeight, commitmenttypes.GetSDKSpecs(), ibctesting.UpgradePath, false, false),
),
types.NewIdentifiedClientState(
exported.Localhost, localhosttypes.NewClientState("chaindID", clientHeight),
),
},
[]types.ClientConsensusStates{
types.NewClientConsensusStates(
"wrongclientid",
tmClientID1,
[]types.ConsensusStateWithHeight{
types.NewConsensusStateWithHeight(
types.ZeroHeight(),
Expand All @@ -175,15 +178,15 @@ func (suite *TypesTestSuite) TestValidateGenesis() {
genState: types.NewGenesisState(
[]types.IdentifiedClientState{
types.NewIdentifiedClientState(
clientID, ibctmtypes.NewClientState(chainID, ibctmtypes.DefaultTrustLevel, ibctesting.TrustingPeriod, ibctesting.UnbondingPeriod, ibctesting.MaxClockDrift, clientHeight, commitmenttypes.GetSDKSpecs(), ibctesting.UpgradePath, false, false),
tmClientID0, ibctmtypes.NewClientState(chainID, ibctmtypes.DefaultTrustLevel, ibctesting.TrustingPeriod, ibctesting.UnbondingPeriod, ibctesting.MaxClockDrift, clientHeight, commitmenttypes.GetSDKSpecs(), ibctesting.UpgradePath, false, false),
),
types.NewIdentifiedClientState(
exported.Localhost, localhosttypes.NewClientState("chaindID", clientHeight),
),
},
[]types.ClientConsensusStates{
types.NewClientConsensusStates(
clientID,
tmClientID0,
[]types.ConsensusStateWithHeight{
types.NewConsensusStateWithHeight(
types.ZeroHeight(),
Expand All @@ -205,15 +208,15 @@ func (suite *TypesTestSuite) TestValidateGenesis() {
genState: types.NewGenesisState(
[]types.IdentifiedClientState{
types.NewIdentifiedClientState(
clientID, ibctmtypes.NewClientState(chainID, ibctmtypes.DefaultTrustLevel, ibctesting.TrustingPeriod, ibctesting.UnbondingPeriod, ibctesting.MaxClockDrift, clientHeight, commitmenttypes.GetSDKSpecs(), ibctesting.UpgradePath, false, false),
tmClientID0, ibctmtypes.NewClientState(chainID, ibctmtypes.DefaultTrustLevel, ibctesting.TrustingPeriod, ibctesting.UnbondingPeriod, ibctesting.MaxClockDrift, clientHeight, commitmenttypes.GetSDKSpecs(), ibctesting.UpgradePath, false, false),
),
types.NewIdentifiedClientState(
exported.Localhost, localhosttypes.NewClientState("chaindID", clientHeight),
),
},
[]types.ClientConsensusStates{
types.NewClientConsensusStates(
clientID,
tmClientID0,
[]types.ConsensusStateWithHeight{
types.NewConsensusStateWithHeight(
types.NewHeight(0, 1),
Expand All @@ -235,15 +238,15 @@ func (suite *TypesTestSuite) TestValidateGenesis() {
genState: types.NewGenesisState(
[]types.IdentifiedClientState{
types.NewIdentifiedClientState(
clientID, ibctmtypes.NewClientState(chainID, ibctesting.DefaultTrustLevel, ibctesting.TrustingPeriod, ibctesting.UnbondingPeriod, ibctesting.MaxClockDrift, clientHeight, commitmenttypes.GetSDKSpecs(), ibctesting.UpgradePath, false, false),
tmClientID0, ibctmtypes.NewClientState(chainID, ibctesting.DefaultTrustLevel, ibctesting.TrustingPeriod, ibctesting.UnbondingPeriod, ibctesting.MaxClockDrift, clientHeight, commitmenttypes.GetSDKSpecs(), ibctesting.UpgradePath, false, false),
),
types.NewIdentifiedClientState(
exported.Localhost, localhosttypes.NewClientState("chainID", clientHeight),
),
},
[]types.ClientConsensusStates{
types.NewClientConsensusStates(
clientID,
tmClientID0,
[]types.ConsensusStateWithHeight{
types.NewConsensusStateWithHeight(
header.GetHeight().(types.Height),
Expand All @@ -265,15 +268,15 @@ func (suite *TypesTestSuite) TestValidateGenesis() {
genState: types.NewGenesisState(
[]types.IdentifiedClientState{
types.NewIdentifiedClientState(
clientID, ibctmtypes.NewClientState(chainID, ibctesting.DefaultTrustLevel, ibctesting.TrustingPeriod, ibctesting.UnbondingPeriod, ibctesting.MaxClockDrift, clientHeight, commitmenttypes.GetSDKSpecs(), ibctesting.UpgradePath, false, false),
tmClientID0, ibctmtypes.NewClientState(chainID, ibctesting.DefaultTrustLevel, ibctesting.TrustingPeriod, ibctesting.UnbondingPeriod, ibctesting.MaxClockDrift, clientHeight, commitmenttypes.GetSDKSpecs(), ibctesting.UpgradePath, false, false),
),
types.NewIdentifiedClientState(
exported.Localhost, localhosttypes.NewClientState("chainID", clientHeight),
),
},
[]types.ClientConsensusStates{
types.NewClientConsensusStates(
clientID,
tmClientID0,
[]types.ConsensusStateWithHeight{
types.NewConsensusStateWithHeight(
header.GetHeight().(types.Height),
Expand All @@ -295,15 +298,15 @@ func (suite *TypesTestSuite) TestValidateGenesis() {
genState: types.NewGenesisState(
[]types.IdentifiedClientState{
types.NewIdentifiedClientState(
clientID, ibctmtypes.NewClientState(chainID, ibctesting.DefaultTrustLevel, ibctesting.TrustingPeriod, ibctesting.UnbondingPeriod, ibctesting.MaxClockDrift, clientHeight, commitmenttypes.GetSDKSpecs(), ibctesting.UpgradePath, false, false),
tmClientID0, ibctmtypes.NewClientState(chainID, ibctesting.DefaultTrustLevel, ibctesting.TrustingPeriod, ibctesting.UnbondingPeriod, ibctesting.MaxClockDrift, clientHeight, commitmenttypes.GetSDKSpecs(), ibctesting.UpgradePath, false, false),
),
types.NewIdentifiedClientState(
exported.Localhost, localhosttypes.NewClientState("chainID", clientHeight),
),
},
[]types.ClientConsensusStates{
types.NewClientConsensusStates(
clientID,
tmClientID0,
[]types.ConsensusStateWithHeight{
types.NewConsensusStateWithHeight(
header.GetHeight().(types.Height),
Expand All @@ -325,15 +328,15 @@ func (suite *TypesTestSuite) TestValidateGenesis() {
genState: types.NewGenesisState(
[]types.IdentifiedClientState{
types.NewIdentifiedClientState(
clientID, ibctmtypes.NewClientState(chainID, ibctesting.DefaultTrustLevel, ibctesting.TrustingPeriod, ibctesting.UnbondingPeriod, ibctesting.MaxClockDrift, clientHeight, commitmenttypes.GetSDKSpecs(), ibctesting.UpgradePath, false, false),
tmClientID1, ibctmtypes.NewClientState(chainID, ibctesting.DefaultTrustLevel, ibctesting.TrustingPeriod, ibctesting.UnbondingPeriod, ibctesting.MaxClockDrift, clientHeight, commitmenttypes.GetSDKSpecs(), ibctesting.UpgradePath, false, false),
),
types.NewIdentifiedClientState(
exported.Localhost, localhosttypes.NewClientState("chainID", clientHeight),
exported.Localhost+"-0", localhosttypes.NewClientState("chainID", clientHeight),
),
},
[]types.ClientConsensusStates{
types.NewClientConsensusStates(
clientID,
tmClientID1,
[]types.ConsensusStateWithHeight{
types.NewConsensusStateWithHeight(
header.GetHeight().(types.Height),
Expand All @@ -346,10 +349,97 @@ func (suite *TypesTestSuite) TestValidateGenesis() {
},
types.NewParams(exported.Tendermint),
true,
2,
),
expPass: false,
},
{
name: "next sequence too small",
genState: types.NewGenesisState(
[]types.IdentifiedClientState{
types.NewIdentifiedClientState(
tmClientID0, ibctmtypes.NewClientState(chainID, ibctesting.DefaultTrustLevel, ibctesting.TrustingPeriod, ibctesting.UnbondingPeriod, ibctesting.MaxClockDrift, clientHeight, commitmenttypes.GetSDKSpecs(), ibctesting.UpgradePath, false, false),
),
types.NewIdentifiedClientState(
exported.Localhost+"-1", localhosttypes.NewClientState("chainID", clientHeight),
),
},
[]types.ClientConsensusStates{
types.NewClientConsensusStates(
tmClientID0,
[]types.ConsensusStateWithHeight{
types.NewConsensusStateWithHeight(
header.GetHeight().(types.Height),
ibctmtypes.NewConsensusState(
header.GetTime(), commitmenttypes.NewMerkleRoot(header.Header.GetAppHash()), header.Header.NextValidatorsHash,
),
),
},
),
},
types.NewParams(exported.Tendermint, exported.Localhost),
false,
0,
),
expPass: false,
},
{
name: "failed to parse client identifier in client state loop",
genState: types.NewGenesisState(
[]types.IdentifiedClientState{
types.NewIdentifiedClientState(
"my-client", ibctmtypes.NewClientState(chainID, ibctesting.DefaultTrustLevel, ibctesting.TrustingPeriod, ibctesting.UnbondingPeriod, ibctesting.MaxClockDrift, clientHeight, commitmenttypes.GetSDKSpecs(), ibctesting.UpgradePath, false, false),
),
types.NewIdentifiedClientState(
exported.Localhost+"-1", localhosttypes.NewClientState("chainID", clientHeight),
),
},
[]types.ClientConsensusStates{
types.NewClientConsensusStates(
tmClientID0,
[]types.ConsensusStateWithHeight{
types.NewConsensusStateWithHeight(
header.GetHeight().(types.Height),
ibctmtypes.NewConsensusState(
header.GetTime(), commitmenttypes.NewMerkleRoot(header.Header.GetAppHash()), header.Header.NextValidatorsHash,
),
),
},
),
},
types.NewParams(exported.Tendermint, exported.Localhost),
false,
5,
),
expPass: false,
},
{
name: "consensus state different than client state type",
genState: types.NewGenesisState(
[]types.IdentifiedClientState{
types.NewIdentifiedClientState(
exported.Localhost+"-1", localhosttypes.NewClientState("chainID", clientHeight),
),
},
[]types.ClientConsensusStates{
types.NewClientConsensusStates(
exported.Localhost+"-1",
[]types.ConsensusStateWithHeight{
types.NewConsensusStateWithHeight(
header.GetHeight().(types.Height),
ibctmtypes.NewConsensusState(
header.GetTime(), commitmenttypes.NewMerkleRoot(header.Header.GetAppHash()), header.Header.NextValidatorsHash,
),
),
},
),
},
types.NewParams(exported.Tendermint, exported.Localhost),
false,
5,
),
expPass: false,
},
}

for _, tc := range testCases {
Expand Down
17 changes: 17 additions & 0 deletions x/ibc/core/03-connection/types/genesis.go
View file
Open in desktop
Original file line number Diff line number Diff line change
Expand Up @@ -38,7 +38,20 @@ func DefaultGenesisState() GenesisState {
// Validate performs basic genesis state validation returning an error upon any
// failure.
func (gs GenesisState) Validate() error {
// keep track of the max sequence to ensure it is less than
// the next sequence used in creating connection identifers.
var maxSequence uint64 = 0

for i, conn := range gs.Connections {
sequence, err := ParseConnectionSequence(conn.Id)
if err != nil {
return err
}

if sequence > maxSequence {
maxSequence = sequence
}

if err := conn.ValidateBasic(); err != nil {
return fmt.Errorf("invalid connection %v index %d: %w", conn, i, err)
}
Expand All @@ -55,5 +68,9 @@ func (gs GenesisState) Validate() error {
}
}

if maxSequence != 0 && maxSequence >= gs.NextConnectionSequence {
return fmt.Errorf("next connection sequence %d must be greater than maximum sequence used in connection identifier %d", gs.NextConnectionSequence, maxSequence)
}

return nil
}
Loading