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

chore: add solomachine client storage #1144

Merged
merged 7 commits into from
Mar 28, 2022
Merged

chore: add solomachine client storage #1144

Show file tree
Hide file tree
Changes from 1 commit
Commits
Show all changes
7 commits
Select commit Hold shift + click to select a range
9ce3b69
WIP: solomachine UpdateState now handles storage of ClientState and C…
damiannolan Mar 18, 2022
ae972c3
applying suggestions for review
damiannolan Mar 21, 2022
c749c70
removing unnecessary consensus state storage
damiannolan Mar 22, 2022
9628a60
Merge branch '02-client-refactor' into damian/solomachine-client-storage
damiannolan Mar 22, 2022
663ede8
updating tests to use store in favour of return values
damiannolan Mar 23, 2022
ca18137
Merge branch 'damian/solomachine-client-storage' of github.com:cosmos…
damiannolan Mar 23, 2022
cdf7d35
Merge branch '02-client-refactor' into damian/solomachine-client-storage
damiannolan Mar 28, 2022
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
10 changes: 5 additions & 5 deletions modules/light-clients/06-solomachine/types/misbehaviour_handle.go
View file
Open in desktop
Original file line number Diff line number Diff line change
Expand Up @@ -34,12 +34,12 @@ func (cs ClientState) CheckMisbehaviourAndUpdateState(
// misbehaviour.ValidateBasic which is called by the 02-client keeper.

// verify first signature
if err := verifySignatureAndData(cdc, cs, soloMisbehaviour, soloMisbehaviour.SignatureOne); err != nil {
if err := cs.verifySignatureAndData(cdc, soloMisbehaviour, soloMisbehaviour.SignatureOne); err != nil {
return nil, sdkerrors.Wrap(err, "failed to verify signature one")
}

// verify second signature
if err := verifySignatureAndData(cdc, cs, soloMisbehaviour, soloMisbehaviour.SignatureTwo); err != nil {
if err := cs.verifySignatureAndData(cdc, soloMisbehaviour, soloMisbehaviour.SignatureTwo); err != nil {
return nil, sdkerrors.Wrap(err, "failed to verify signature two")
}

Expand All @@ -50,7 +50,7 @@ func (cs ClientState) CheckMisbehaviourAndUpdateState(
// verifySignatureAndData verifies that the currently registered public key has signed
// over the provided data and that the data is valid. The data is valid if it can be
// unmarshaled into the specified data type.
func verifySignatureAndData(cdc codec.BinaryCodec, clientState ClientState, misbehaviour *Misbehaviour, sigAndData *SignatureAndData) error {
func (cs ClientState) verifySignatureAndData(cdc codec.BinaryCodec, misbehaviour *Misbehaviour, sigAndData *SignatureAndData) error {

// do not check misbehaviour timestamp since we want to allow processing of past misbehaviour

Expand All @@ -62,7 +62,7 @@ func verifySignatureAndData(cdc codec.BinaryCodec, clientState ClientState, misb
data, err := MisbehaviourSignBytes(
cdc,
misbehaviour.Sequence, sigAndData.Timestamp,
clientState.ConsensusState.Diversifier,
cs.ConsensusState.Diversifier,
sigAndData.DataType,
sigAndData.Data,
)
Expand All @@ -75,7 +75,7 @@ func verifySignatureAndData(cdc codec.BinaryCodec, clientState ClientState, misb
return err
}

publicKey, err := clientState.ConsensusState.GetPubKey()
publicKey, err := cs.ConsensusState.GetPubKey()
if err != nil {
return err
}
Expand Down
75 changes: 66 additions & 9 deletions modules/light-clients/06-solomachine/types/update.go
View file
Open in desktop
Original file line number Diff line number Diff line change
@@ -1,11 +1,17 @@
package types

import (
"encoding/hex"

"github.com/armon/go-metrics"
"github.com/cosmos/cosmos-sdk/codec"
"github.com/cosmos/cosmos-sdk/telemetry"
sdk "github.com/cosmos/cosmos-sdk/types"
sdkerrors "github.com/cosmos/cosmos-sdk/types/errors"

"github.com/cosmos/ibc-go/v3/modules/core/02-client/types"
colin-axner marked this conversation as resolved.
Show resolved Hide resolved
clienttypes "github.com/cosmos/ibc-go/v3/modules/core/02-client/types"
host "github.com/cosmos/ibc-go/v3/modules/core/24-host"
"github.com/cosmos/ibc-go/v3/modules/core/exported"
)

Expand Down Expand Up @@ -41,23 +47,23 @@ func (cs ClientState) VerifyClientMessage(ctx sdk.Context, cdc codec.BinaryCodec
case *Misbehaviour:
return cs.verifyMisbehaviour(ctx, cdc, clientStore, msg)
default:
return sdkerrors.Wrapf(clienttypes.ErrInvalidClientType, "expected type of %T or %T, got type %T", Header{}, Misbehaviour{}, msg)
return sdkerrors.Wrapf(types.ErrInvalidClientType, "expected type of %T or %T, got type %T", Header{}, Misbehaviour{}, msg)
}
}

func (cs ClientState) verifyHeader(ctx sdk.Context, cdc codec.BinaryCodec, clientStore sdk.KVStore, header *Header) error {
// assert update sequence is current sequence
if header.Sequence != cs.Sequence {
return sdkerrors.Wrapf(
clienttypes.ErrInvalidHeader,
types.ErrInvalidHeader,
"header sequence does not match the client state sequence (%d != %d)", header.Sequence, cs.Sequence,
)
}

// assert update timestamp is not less than current consensus state timestamp
if header.Timestamp < cs.ConsensusState.Timestamp {
return sdkerrors.Wrapf(
clienttypes.ErrInvalidHeader,
types.ErrInvalidHeader,
"header timestamp is less than to the consensus state timestamp (%d < %d)", header.Timestamp, cs.ConsensusState.Timestamp,
)
}
Expand Down Expand Up @@ -89,12 +95,12 @@ func (cs ClientState) verifyMisbehaviour(ctx sdk.Context, cdc codec.BinaryCodec,
// NOTE: a check that the misbehaviour message data are not equal is done by
// misbehaviour.ValidateBasic which is called by the 02-client keeper.
// verify first signature
if err := verifySignatureAndData(cdc, cs, misbehaviour, misbehaviour.SignatureOne); err != nil {
if err := cs.verifySignatureAndData(cdc, misbehaviour, misbehaviour.SignatureOne); err != nil {
return sdkerrors.Wrap(err, "failed to verify signature one")
}

// verify second signature
if err := verifySignatureAndData(cdc, cs, misbehaviour, misbehaviour.SignatureTwo); err != nil {
if err := cs.verifySignatureAndData(cdc, misbehaviour, misbehaviour.SignatureTwo); err != nil {
return sdkerrors.Wrap(err, "failed to verify signature two")
}

Expand All @@ -103,7 +109,12 @@ func (cs ClientState) verifyMisbehaviour(ctx sdk.Context, cdc codec.BinaryCodec,

// UpdateState updates the consensus state to the new public key and an incremented sequence.
func (cs ClientState) UpdateState(ctx sdk.Context, cdc codec.BinaryCodec, clientStore sdk.KVStore, clientMsg exported.ClientMessage) (exported.ClientState, exported.ConsensusState, error) {
smHeader := clientMsg.(*Header)
smHeader, ok := clientMsg.(*Header)
if !ok {
return nil, nil, sdkerrors.Wrapf(types.ErrInvalidClientType, "expected %T got %T", Header{}, clientMsg)
}

// create new solomachine ConsensusState
consensusState := &ConsensusState{
PublicKey: smHeader.NewPublicKey,
Diversifier: smHeader.NewDiversifier,
Expand All @@ -112,6 +123,49 @@ func (cs ClientState) UpdateState(ctx sdk.Context, cdc codec.BinaryCodec, client

cs.Sequence++
cs.ConsensusState = consensusState

clientStore.Set(host.ClientStateKey(), types.MustMarshalClientState(cdc, &cs))

// set default consensus height with header height
consensusHeight := smHeader.GetHeight()
if cs.ClientType() != exported.Localhost {
clientStore.Set(host.ConsensusStateKey(consensusHeight), types.MustMarshalConsensusState(cdc, consensusState))
} else {
consensusHeight = types.GetSelfHeight(ctx)
Copy link
Contributor

Choose a reason for hiding this comment

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

where is this coming from? Solo machines don't store consensus states

Copy link
Contributor Author

Choose a reason for hiding this comment

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

Slightly confused here, isn't this code run for solomachines too?

Copy link
Contributor

Choose a reason for hiding this comment

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

It is, but it shouldn't be. This was another justification for why setting the consensus state within the light client implementation is useful. I thought I documented this clearly, but this was all I could find:

This would require light client developers to set the consensus states themselves (allowing header.GetHeight to be removed and stopping unnecessary state storage for solo machines).

ref

Solo machines shouldn't be setting a consensus state ever. It has a single consensus state which is constantly updated and thus it is stored within the client state. The code linked above was an inefficiency, performing unnecessary storage

Copy link
Contributor Author

Choose a reason for hiding this comment

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

Ahh, okay! Tbh I was confused why ConsensusState existed as a field on the solomachine ClientState and the extra storage was happening.
This should all be much easier to reason about when the refactor is done. Thank you for clarifying this and providing context, much appreciated!

Updated and removed this code :)

}

// TODO: Should be telemetry and events be emitted from 02-client?
// The clientID would need to be passed an additional arg here
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 we need to make a decision on whether telemetry and events should remain to be emitted from 02-client or from within the ClientState implementation. We may need to pass the client ID as an additional arg if we want to do this in the lightclient implementation, or otherwise we may need to return the consensus height. Any thoughts on this? cc. @colin-axner @seantking

Copy link
Contributor

@colin-axner colin-axner Mar 21, 2022 •
edited
Loading

Choose a reason for hiding this comment

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

We should not require light client implementations to emit events. This decision informed the proposed design. We should allow light client to optionally emit their own events and telemetry. This is technically possible by constructing a ClientMessage which contains the clientID, but I'm not opposed to adding the clientID as an argument to the function since 02-client already has that information. It doesn't provide any usefulness outside of event emission/telemetry since the client store is prefixed by the clientID

Copy link
Contributor

Choose a reason for hiding this comment

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

To expand further on why we should not require light client implementations to emit events:

Relayers make use of the events we have in 02-client. By passing this to light client implementations, we lose standardization and risk light client implementators making mistakes. Since the functionality called by 02-client is very verbose now, 02-client can emit appropriate events when doing updates or updates on misbehaviour

Copy link
Contributor Author

Choose a reason for hiding this comment

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

Okay, great, that makes a lot of sense! Thanks, I'll remove the events and telemetry from here.

defer func() {
telemetry.IncrCounterWithLabels(
[]string{"ibc", "client", "update"},
1,
[]metrics.Label{
telemetry.NewLabel(types.LabelClientType, cs.ClientType()),
telemetry.NewLabel(types.LabelClientID, "clientID"), // TODO: Should clientID be passed as an arg
telemetry.NewLabel(types.LabelUpdateType, "msg"),
},
)
}()
colin-axner marked this conversation as resolved.
Show resolved Hide resolved

// Marshal the Header as an Any and encode the resulting bytes to hex.
// This prevents the event value from containing invalid UTF-8 characters
// which may cause data to be lost when JSON encoding/decoding.
headerStr := hex.EncodeToString(types.MustMarshalClientMessage(cdc, clientMsg))
ctx.EventManager().EmitEvents(sdk.Events{
sdk.NewEvent(
types.EventTypeUpdateClient,
sdk.NewAttribute(types.AttributeKeyClientID, "clientID"),
sdk.NewAttribute(types.AttributeKeyClientType, cs.ClientType()),
sdk.NewAttribute(types.AttributeKeyConsensusHeight, consensusHeight.String()),
sdk.NewAttribute(types.AttributeKeyHeader, headerStr),
),
sdk.NewEvent(
sdk.EventTypeMessage,
sdk.NewAttribute(sdk.AttributeKeyModule, types.AttributeValueCategory),
),
})
colin-axner marked this conversation as resolved.
Show resolved Hide resolved

return &cs, consensusState, nil
}

Expand All @@ -126,9 +180,12 @@ func (cs ClientState) CheckForMisbehaviour(_ sdk.Context, _ codec.BinaryCodec, _

// UpdateStateOnMisbehaviour updates state upon misbehaviour. This method should only be called on misbehaviour
// as it does not perform any misbehaviour checks.
func (cs ClientState) UpdateStateOnMisbehaviour(
_ sdk.Context, _ codec.BinaryCodec, _ sdk.KVStore, // prematurely include args for self storage of consensus state
) (*ClientState, exported.ConsensusState, error) {
func (cs ClientState) UpdateStateOnMisbehaviour(ctx sdk.Context, cdc codec.BinaryCodec, clientStore sdk.KVStore) (*ClientState, exported.ConsensusState, error) {
Copy link
Contributor

Choose a reason for hiding this comment

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

Is there a test for this function? Can you check that the correct client state is set in the store?

Copy link
Contributor Author

Choose a reason for hiding this comment

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

Yeah, there is a test function for this. I was initially just doing an assertion on the returned value but I can adapt it to query the store. Will do the same as you suggest for UpdateState 👍

Copy link
Contributor

Choose a reason for hiding this comment

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

Sweet, I just figure better to do now so when we remove the return values we don't have to add testing code

Copy link
Contributor Author

Choose a reason for hiding this comment

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

Definitely! Agreed 💯

cs.IsFrozen = true

clientStore.Set(host.ClientStateKey(), clienttypes.MustMarshalClientState(cdc, &cs))

// TODO: Telemetry and events
damiannolan marked this conversation as resolved.
Show resolved Hide resolved

return &cs, cs.ConsensusState, nil
}
10 changes: 9 additions & 1 deletion modules/light-clients/06-solomachine/types/update_test.go
View file
Open in desktop
Original file line number Diff line number Diff line change
Expand Up @@ -594,6 +594,14 @@ func (suite *SoloMachineTestSuite) TestUpdateState() {
},
true,
},
{
"invalid type misbehaviour",
func() {
clientState = solomachine.ClientState()
clientMsg = solomachine.CreateMisbehaviour()
},
false,
},
}

for _, tc := range testCases {
Expand All @@ -615,7 +623,7 @@ func (suite *SoloMachineTestSuite) TestUpdateState() {
suite.Require().Equal(consensusState, cs.(*types.ClientState).ConsensusState)
Copy link
Contributor

Choose a reason for hiding this comment

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

can you add checks on the successful case that the correct client state is set in the store?

} else {
suite.Require().Error(err)
suite.Require().Nil(clientState)
suite.Require().Nil(cs)
suite.Require().Nil(consensusState)
}
}
Expand Down