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

Modify IBC client governance unfreezing to reflect ADR changes #8405

Merged
merged 55 commits into from
Feb 16, 2021
Merged
Show file tree
Hide file tree
Changes from 10 commits
Commits
Show all changes
55 commits
Select commit Hold shift + click to select a range
a654f27
update proto files
colin-axner Jan 19, 2021
9aa500c
Merge branch 'master' of github.com:cosmos/cosmos-sdk into colin/8197…
colin-axner Jan 19, 2021
51b5001
Merge branch 'master' of github.com:cosmos/cosmos-sdk into colin/8197…
colin-axner Jan 19, 2021
a47dbbb
make proto-gen
colin-axner Jan 19, 2021
73b2821
update clienttypes
colin-axner Jan 21, 2021
8d27dcf
update localhost and solo machine
colin-axner Jan 21, 2021
fcb97ff
refactor tm client proposal handling
colin-axner Jan 21, 2021
0e93fb6
Merge branch 'master' into colin/8197-unfreeze-clients-gov-prop
colin-axner Jan 21, 2021
bbd6d26
copy metadata
colin-axner Jan 21, 2021
0282996
Merge branch 'colin/8197-unfreeze-clients-gov-prop' of github.com:cos…
colin-axner Jan 21, 2021
ce4fe37
self review fixes
colin-axner Jan 22, 2021
c4cb7de
update 02-client keeper tests
colin-axner Jan 22, 2021
a7ef263
fix 02-client type tests
colin-axner Jan 22, 2021
d15f901
fix localhost and solomachine tests
colin-axner Jan 22, 2021
0c6d98d
begin updating tm tests
colin-axner Jan 26, 2021
fa28622
partially fix tm tests
colin-axner Jan 28, 2021
4c6688c
increase codecov
colin-axner Jan 29, 2021
a041987
add more tests
colin-axner Jan 29, 2021
eefccd5
Merge branch 'master' of github.com:cosmos/cosmos-sdk into colin/8197…
colin-axner Jan 29, 2021
a5bc8d5
add changelog
colin-axner Jan 29, 2021
861b1cb
update specs
colin-axner Jan 29, 2021
908e5a0
add docs
colin-axner Jan 29, 2021
51905db
Merge branch 'master' into colin/8197-unfreeze-clients-gov-prop
colin-axner Feb 1, 2021
c74a5af
fix test
colin-axner Feb 1, 2021
dbe1341
Merge branch 'colin/8197-unfreeze-clients-gov-prop' of github.com:cos…
colin-axner Feb 1, 2021
0c96f99
modify adr
colin-axner Feb 1, 2021
0fda643
allow modified chain-ids
colin-axner Feb 2, 2021
f17b5e5
Merge branch 'master' of github.com:cosmos/cosmos-sdk into colin/8197…
colin-axner Feb 2, 2021
e141f19
add CLI command
colin-axner Feb 2, 2021
18b82ba
fix typos
colin-axner Feb 2, 2021
2369e07
Merge branch 'master' into colin/8197-unfreeze-clients-gov-prop
colin-axner Feb 2, 2021
bc6a359
fix lint
colin-axner Feb 2, 2021
2b55583
Merge branch 'colin/8197-unfreeze-clients-gov-prop' of github.com:cos…
colin-axner Feb 2, 2021
ef2fb8a
Apply suggestions from code review
colin-axner Feb 2, 2021
c9bca48
update docs, rm example
colin-axner Feb 5, 2021
6a96b6d
Merge branch 'colin/8197-unfreeze-clients-gov-prop' of github.com:cos…
colin-axner Feb 5, 2021
78d04f4
Update docs/ibc/proposals.md
colin-axner Feb 8, 2021
20b7a5d
merge master
colin-axner Feb 8, 2021
696b5f2
update height checks to reflect chain-id changes cc @AdityaSripal
colin-axner Feb 8, 2021
0d60013
Merge branch 'master' into colin/8197-unfreeze-clients-gov-prop
colin-axner Feb 10, 2021
b14ea6c
Apply suggestions from code review
colin-axner Feb 11, 2021
bbb4acf
Apply suggestions from code review
colin-axner Feb 11, 2021
10dfe43
address most of @AdityaSripal suggestions
colin-axner Feb 11, 2021
1edf9ab
Merge branch 'master' of github.com:cosmos/cosmos-sdk into colin/8197…
colin-axner Feb 11, 2021
eb5823c
Merge branch 'colin/8197-unfreeze-clients-gov-prop' of github.com:cos…
colin-axner Feb 11, 2021
e01e8c5
Merge branch 'master' of github.com:cosmos/cosmos-sdk into colin/8197…
colin-axner Feb 12, 2021
1869fbd
update docs per review suggestions
colin-axner Feb 12, 2021
84ebde6
Update x/ibc/core/02-client/types/proposal.go
colin-axner Feb 12, 2021
912fdf5
add proposal handler
colin-axner Feb 12, 2021
613cf69
Merge branch 'colin/8197-unfreeze-clients-gov-prop' of github.com:cos…
colin-axner Feb 12, 2021
b7834c6
register proposal type
colin-axner Feb 12, 2021
1ba2c3d
register proposal on codec
colin-axner Feb 12, 2021
9fae3ce
fix routing
colin-axner Feb 12, 2021
26837e2
Merge branch 'master' into colin/8197-unfreeze-clients-gov-prop
colin-axner Feb 16, 2021
a5c09a7
Merge branch 'colin/8197-unfreeze-clients-gov-prop' of github.com:cos…
colin-axner Feb 16, 2021
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
13 changes: 8 additions & 5 deletions docs/core/proto-docs.md
Original file line number Diff line number Diff line change
Expand Up @@ -7214,17 +7214,20 @@ client.
<a name="ibc.core.client.v1.ClientUpdateProposal"></a>

### ClientUpdateProposal
ClientUpdateProposal is a governance proposal. If it passes, the client is
updated with the provided header. The update may fail if the header is not
valid given certain conditions specified by the client implementation.
ClientUpdateProposal is a governance proposal. If it passes, the substitute client's
consensus states starting from the 'initial height' are copied over to the subjects
client state. The proposal handler may fail if the subject and the substitute do not
match in client and chain parameters (with exception to latest and frozen height).
The updated client must also be valid (cannot be expired).


| Field | Type | Label | Description |
| ----- | ---- | ----- | ----------- |
| `title` | [string](#string) | | the title of the update proposal |
| `description` | [string](#string) | | the description of the proposal |
| `client_id` | [string](#string) | | the client identifier for the client to be updated if the proposal passes |
| `header` | [google.protobuf.Any](#google.protobuf.Any) | | the header used to update the client if the proposal passes |
| `subject_client_id` | [string](#string) | | the client identifier for the client to be updated if the proposal passes |
| `substitute_client_id` | [string](#string) | | the substitute client identifier for the client standing in for the subject client |
| `initial_height` | [Height](#ibc.core.client.v1.Height) | | the intital height to copy consensus states from the substitute to the subject |



Expand Down
16 changes: 10 additions & 6 deletions proto/ibc/core/client/v1/client.proto
Original file line number Diff line number Diff line change
Expand Up @@ -33,19 +33,23 @@ message ClientConsensusStates {
[(gogoproto.moretags) = "yaml:\"consensus_states\"", (gogoproto.nullable) = false];
}

// ClientUpdateProposal is a governance proposal. If it passes, the client is
// updated with the provided header. The update may fail if the header is not
// valid given certain conditions specified by the client implementation.
// ClientUpdateProposal is a governance proposal. If it passes, the substitute client's
// consensus states starting from the 'initial height' are copied over to the subjects
// client state. The proposal handler may fail if the subject and the substitute do not
// match in client and chain parameters (with exception to latest and frozen height).
// The updated client must also be valid (cannot be expired).
message ClientUpdateProposal {
option (gogoproto.goproto_getters) = false;
// the title of the update proposal
string title = 1;
// the description of the proposal
string description = 2;
// the client identifier for the client to be updated if the proposal passes
string client_id = 3 [(gogoproto.moretags) = "yaml:\"client_id\""];
// the header used to update the client if the proposal passes
google.protobuf.Any header = 4;
string subject_client_id = 3 [(gogoproto.moretags) = "yaml:\"subject_client_id\""];
Copy link
Member

Choose a reason for hiding this comment

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

This should be repeated string

// the substitute client identifier for the client standing in for the subject client
string substitute_client_id = 4 [(gogoproto.moretags) = "yaml:\"subtitute_client_id\""];
// the intital height to copy consensus states from the substitute to the subject
Height initial_height = 5 [(gogoproto.moretags) = "yaml:\"initial_height\"", (gogoproto.nullable) = false];
}

// Height is a monotonically increasing data type
Expand Down
52 changes: 36 additions & 16 deletions x/ibc/core/02-client/keeper/proposal.go
Original file line number Diff line number Diff line change
Expand Up @@ -10,40 +10,60 @@ import (
"github.com/cosmos/cosmos-sdk/x/ibc/core/exported"
)

// ClientUpdateProposal will try to update the client with the new header if and only if
// the proposal passes. The localhost client is not allowed to be modified with a proposal.
// ClientUpdateProposal will retrieve the subject and substitute client.
// The initial height must be greater than the latest height of the subject
// client. A callback will occur to the subject client state with the client
// prefixed store being provided for both the subject and the substitute client.
// The localhost client is not allowed to be modified with a proposal. The IBC
// client implementations are responsible for validating the parameters of the
// subtitute (enusring they match the subject's parameters) as well as copying
// the necessary consensus states from the subtitute to the subject client
// store.
//
// NOTE: Substitute clients with revision numbers not equal to the revision
// number of the subject client is explicityly disallowed. We cannot support
// this until there is a way to range query for the all the consensus
// states which occurred between two IBC Revision heights.
// https://github.com/cosmos/cosmos-sdk/issues/7712
func (k Keeper) ClientUpdateProposal(ctx sdk.Context, p *types.ClientUpdateProposal) error {
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 thought of trying to enforce subject and substitute client types being equal here but just doing ClientType() doesn't add any more security from a malicious light client implementation, and if the light client implementation is malicious, it could just mess with other clients of its own type.

It is up to governance never to pass a vote where the subject belongs to a malicious light client implementation. Ideally, best practice (which I will document/recommend) would be to create an entirely new substitute client after the subject became frozen/expired. This way no other connection/channels are relying on the substitute.

if p.ClientId == exported.Localhost {
if p.SubjectClientId == exported.Localhost {
return sdkerrors.Wrap(types.ErrInvalidUpdateClientProposal, "cannot update localhost client with proposal")
}

clientState, found := k.GetClientState(ctx, p.ClientId)
subjectClientState, found := k.GetClientState(ctx, p.SubjectClientId)
if !found {
return sdkerrors.Wrapf(types.ErrClientNotFound, "cannot update client with ID %s", p.ClientId)
return sdkerrors.Wrapf(types.ErrClientNotFound, "subject client with ID %s", p.SubjectClientId)
}

header, err := types.UnpackHeader(p.Header)
if err != nil {
return err
if subjectClientState.GetLatestHeight().GTE(p.InitialHeight) {
return sdkerrors.Wrapf(types.ErrInvalidHeight, "subject client state latest height is greater or equal to initital height (%s >= %s)", subjectClientState.GetLatestHeight(), p.InitialHeight)
}

substituteClientState, found := k.GetClientState(ctx, p.SubstituteClientId)
if !found {
return sdkerrors.Wrapf(types.ErrClientNotFound, "substitute client with ID %s", p.SubstituteClientId)
}

// substitute clients with height across revision numbers is not allowed
if subjectClientState.GetLatestHeight().GetRevisionNumber() != substituteClientState.GetLatestHeight().GetRevisionNumber() {
colin-axner marked this conversation as resolved.
Show resolved Hide resolved
return sdkerrors.Wrapf(types.ErrInvalidHeight, "subject client state and substitute client state must have the same revision number (%d != %d)", subjectClientState.GetLatestHeight().GetRevisionNumber(), substituteClientState.GetLatestHeight().GetRevisionNumber())
}

clientState, consensusState, err := clientState.CheckProposedHeaderAndUpdateState(ctx, k.cdc, k.ClientStore(ctx, p.ClientId), header)
clientState, err := subjectClientState.CheckSubstituteAndUpdateState(ctx, k.cdc, k.ClientStore(ctx, p.SubjectClientId), k.ClientStore(ctx, p.SubstituteClientId), substituteClientState, p.InitialHeight)
if err != nil {
return err
}
k.SetClientState(ctx, p.SubjectClientId, clientState)

k.SetClientState(ctx, p.ClientId, clientState)
k.SetClientConsensusState(ctx, p.ClientId, header.GetHeight(), consensusState)

k.Logger(ctx).Info("client updated after governance proposal passed", "client-id", p.ClientId, "height", clientState.GetLatestHeight().String())
k.Logger(ctx).Info("client updated after governance proposal passed", "client-id", p.SubjectClientId, "height", clientState.GetLatestHeight().String())

defer func() {
telemetry.IncrCounterWithLabels(
[]string{"ibc", "client", "update"},
1,
[]metrics.Label{
telemetry.NewLabel("client-type", clientState.ClientType()),
telemetry.NewLabel("client-id", p.ClientId),
telemetry.NewLabel("client-id", p.SubjectClientId),
telemetry.NewLabel("update-type", "proposal"),
},
)
Expand All @@ -53,9 +73,9 @@ func (k Keeper) ClientUpdateProposal(ctx sdk.Context, p *types.ClientUpdatePropo
ctx.EventManager().EmitEvent(
sdk.NewEvent(
types.EventTypeUpdateClientProposal,
sdk.NewAttribute(types.AttributeKeyClientID, p.ClientId),
sdk.NewAttribute(types.AttributeKeySubjectClientID, p.SubjectClientId),
sdk.NewAttribute(types.AttributeKeyClientType, clientState.ClientType()),
sdk.NewAttribute(types.AttributeKeyConsensusHeight, header.GetHeight().String()),
sdk.NewAttribute(types.AttributeKeyConsensusHeight, clientState.GetLatestHeight().String()),
),
)

Expand Down
Loading