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

client identifier reuse #358

Merged
merged 14 commits into from
Jan 20, 2021
Merged
1 change: 1 addition & 0 deletions go.mod
Original file line number Diff line number Diff line change
Expand Up @@ -10,6 +10,7 @@ require (
github.com/cosmos/cosmos-sdk v0.40.0
github.com/cosmos/go-bip39 v1.0.0
github.com/gogo/protobuf v1.3.1
github.com/golang/protobuf v1.4.3
github.com/gorilla/mux v1.8.0
github.com/moby/term v0.0.0-20201101162038-25d840ce174a // indirect
github.com/ory/dockertest/v3 v3.6.2
Expand Down
127 changes: 87 additions & 40 deletions relayer/client-tx.go
Original file line number Diff line number Diff line change
Expand Up @@ -3,8 +3,12 @@ package relayer
import (
"fmt"

"github.com/golang/protobuf/proto"

sdk "github.com/cosmos/cosmos-sdk/types"
clientutils "github.com/cosmos/cosmos-sdk/x/ibc/core/02-client/client/utils"
clienttypes "github.com/cosmos/cosmos-sdk/x/ibc/core/02-client/types"
"github.com/cosmos/cosmos-sdk/x/ibc/core/exported"
)

// CreateClients creates clients for src on dst and dst on src if the client ids are unspecified.
Expand Down Expand Up @@ -32,28 +36,33 @@ func (c *Chain) CreateClients(dst *Chain) (modified bool, err error) {
if err != nil {
return modified, err
}
msgs := []sdk.Msg{
c.PathEnd.CreateClient(
dstH,
dst.GetTrustingPeriod(),
ubdPeriod,
c.MustGetAddress(),
),
}
res, success, err := c.SendMsgs(msgs)
if err != nil {
return modified, err
}
if !success {
return modified, fmt.Errorf("tx failed: %s", res.RawLog)
msg := c.PathEnd.CreateClient(
dstH,
dst.GetTrustingPeriod(),
ubdPeriod,
c.MustGetAddress(),
)

// Check if an identical light client already exists
clientID, found := FindMatchingClient(c, msg, dstH.GetHeight())
if !found {
// if a matching client does not exist, create one
res, success, err := c.SendMsgs([]sdk.Msg{msg})
if err != nil {
return modified, err
}
if !success {
return modified, fmt.Errorf("tx failed: %s", res.RawLog)
}

// update the client identifier
// use index 0, the transaction only has one message
clientID, err = ParseClientIDFromEvents(res.Logs[0].Events)
if err != nil {
return modified, err
}
}

// update the client identifier
// use index 0, the transaction only has one message
clientID, err := ParseClientIDFromEvents(res.Logs[0].Events)
if err != nil {
return modified, err
}
c.PathEnd.ClientID = clientID
modified = true

Expand All @@ -74,26 +83,29 @@ func (c *Chain) CreateClients(dst *Chain) (modified bool, err error) {
if err != nil {
return modified, err
}
msgs := []sdk.Msg{
dst.PathEnd.CreateClient(
srcH,
c.GetTrustingPeriod(),
ubdPeriod,
dst.MustGetAddress(),
),
}
res, success, err := dst.SendMsgs(msgs)
if err != nil {
return modified, err
}
if !success {
return modified, fmt.Errorf("tx failed: %s", res.RawLog)
}

// update client identifier
clientID, err := ParseClientIDFromEvents(res.Logs[0].Events)
if err != nil {
return modified, err
msg := dst.PathEnd.CreateClient(
srcH,
c.GetTrustingPeriod(),
ubdPeriod,
dst.MustGetAddress(),
)
// Check if an identical light client already exists
clientID, found := FindMatchingClient(c, msg, dstH.GetHeight())
if !found {
// if a matching client does not exist, create one
res, success, err := dst.SendMsgs([]sdk.Msg{msg})
if err != nil {
return modified, err
}
if !success {
return modified, fmt.Errorf("tx failed: %s", res.RawLog)
}

// update client identifier
clientID, err = ParseClientIDFromEvents(res.Logs[0].Events)
if err != nil {
return modified, err
}
}
dst.PathEnd.ClientID = clientID
modified = true
Expand Down Expand Up @@ -191,3 +203,38 @@ func (c *Chain) UpgradeClients(dst *Chain) error {

return nil
}

// FindMatchingClient will determine if there exists a client with identical client and consensus states
// to the client which would have been created.
func FindMatchingClient(c *Chain, msg sdk.Msg, consensusStateHeight exported.Height) (string, bool) {
createClientMsg, ok := msg.(*clienttypes.MsgCreateClient)
if !ok {
return "", false
}

// TODO: add appropriate offset and limits, along with retries
clientsResp, err := c.QueryClients(0, 1000)
if err != nil {
return "", false
}

for _, identifiedClientState := range clientsResp.ClientStates {
// check if the client states match
if proto.Equal(createClientMsg.ClientState, identifiedClientState.ClientState) {

// check if consensus state exists
consensusStateResp, err := clientutils.QueryConsensusStateABCI(c.CLIContext(0), identifiedClientState.ClientId, consensusStateHeight)
if err != nil {
// consensus state does not exist, try next client
continue
}

if proto.Equal(consensusStateResp.ConsensusState, createClientMsg.ConsensusState) {
// matching client found
return identifiedClientState.ClientId, true
}
Copy link
Member

Choose a reason for hiding this comment

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

Doing this makes it unlikely that the consensus state actually matches. We don't want to rely on the ConsensusState in the CreateClientMsg message.

Why can't we just take the latest consensus state for that client state, we can then just check if that consensus state matches the block at the specified height on chain c. If it does, then we can be sure it's a client of the same chain.

Copy link
Contributor Author

@colin-axner colin-axner Jan 12, 2021

Choose a reason for hiding this comment

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

perfect, exactly the feedback I was hoping for. That makes a lot of sense and should actually make client reuse viable and testing it should become trivial

Copy link
Member

Choose a reason for hiding this comment

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

Ahh it looks like c is the chain that you are sending the CreateClientMsg to. You want to query the header from the other chain, since that is the chain you are creating the client for. Will require a refactor, but still possible.

}
}

return "", false
}