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

Add message and rpc handler to upgrade clients using v1 governance proposals #3673

Closed
5 tasks done
Tracked by #3674 ...
crodriguezvega opened this issue May 28, 2023 · 1 comment
Closed
5 tasks done
Tracked by #3674 ...
Assignees
Labels
02-client change: api breaking Issues or PRs that break Go API (need to be release in a new major version)
Milestone

Comments

@crodriguezvega
Copy link
Contributor

crodriguezvega commented May 28, 2023

Part of #1282.

Point for discussion: in #1282 @colin-axner mentions that one benefit for us to move to governance v1 proposal is simplifying UpgradeProposal (currently wraps calls to x/upgrade), an IBC chain upgrade could simply be combined with a x/upgrade message. I couldn't figure out a way to completely remove the dependency to the upgrade keeper, but I am open to discuss this to see if there's a way to do it. For now, the proposal here still uses the upgrade keeper in the same way as it is used currently in HandleUpgradeProposal.

Overview of changes:

Protobuf

  • Add a new rpc handler to 02-client MsgService. Something like this:
rpc ScheduleIBCUpgrade(MsgScheduleIBCUpgrade) returns (MsgScheduleIBCUpgradeResponse);

where MsgScheduleIBCUpgrade could look like this:

message MsgScheduleIBCUpgrade {
  option (gogoproto.equal)           = false;
  option (gogoproto.goproto_getters) = false;
  option (cosmos.msg.v1.signer)      = "authority";

  // authority is the address of the governance account.
  string authority = 1;
  cosmos.upgrade.v1beta1.Plan plan = 2 [(gogoproto.nullable) = false];

  // An UpgradedClientState must be provided to perform an IBC breaking upgrade.
  // This will make the chain commit to the correct upgraded (self) client state
  // before the upgrade occurs, so that connecting chains can verify that the
  // new upgraded client is valid by verifying a proof on the previous version
  // of the chain. This will allow IBC connections to persist smoothly across
  // planned chain upgrades
  google.protobuf.Any upgraded_client_state = 3;
}

and MsgScheduleIBCUpgradeResponse can be empty:

message MsgScheduleIBCUpgradeResponse {}

02-client

  • Add function ScheduleIBCUpgrade to 02-client keeper with logic to store the upgraded client state. The signature of the function could look like this:
func (k Keeper) ScheduleIBCUpgrade(ctx sdk.Context, plan types.Plan, upgradedClientState exported.ClientState) error
// zero out any custom fields before setting
cs := upgradedClientState.ZeroCustomFields()
bz, err := types.MarshalClientState(k.cdc, cs)
if err != nil {
  return errorsmod.Wrap(err, "could not marshal UpgradedClientState")
}

if err := k.upgradeKeeper.ScheduleIBCUpgrade(ctx, plan); err != nil {
  return err
}

// sets the new upgraded client in last height committed on this chain is at plan.Height,
// since the chain will panic at plan.Height and new chain will resume at plan.Height
if err = k.upgradeKeeper.SetUpgradedClient(ctx, plan.Height, bz); err != nil {
  return err
}

// emitting an event for handling client upgrade proposal
emitUpgradeClientProposalEvent(ctx, plan.Height)

return nil

Comparing with the existing implementation of HandleUpgradeProposal, the pseudocode above still depends on the upgrade keeper to schedule and set the upgraded client. Removing completely the dependency seems difficult at the moment, even if a client upgrade governance v1 proposal could execute two messages, one being the message added in this issue and another one SDK's MsgSoftwareUpgrade. With MsgSoftwareUpgrade the SDK can take care of scheduling the upgrade (i.e. executing call to upgrade keeper ScheduleUpgrade), but it would not handle the call to SetUpgradedClient, which we would still need to take care of ourselves. And since SetUpgradedClient requires the plan height, then I am not sure if it is really worth it to require governance to execute two different messages... But I am looking forward to discussing alternatives to do things better here.

  • Implement sdk.Msg interface for MsgScheduleIBCUpgrade.

Core

  • Add implementation of ScheduleIBCUpgrade rpc handler in IBC core msg_server.go (modules/core/keeper/msg_server.go). The signature of the handler could look like this:
func (k Keeper) ScheduleIBCUpgrade(
  goCtx context.Context, 
  msg *clienttypes.MsgScheduleUpgradeClient
) (*clienttypes.MsgScheduleUpgradeClientResponse, error)

and the implementation could look like this:

// check message signer matches authority set
if k.authority != msg.Signer {
  return nil, errorsmod.Wrapf(
    govtypes.ErrInvalidSigner, 
    "invalid authority: expected %s, got %s", k.authority, msg.Signer
  )
}

ctx := sdk.UnwrapSDKContext(goCtx)

clientState, err := types.UnpackClientState(p.UpgradedClientState)
if err != nil {
  return errorsmod.Wrap(err, "could not unpack UpgradedClientState")
}

err := k.ClientKeeper.ScheduleIBCUpgrade(ctx, msg.Plan, clientState)
if err != nil {
  return nil, errorsmod.Wrap(err, "client upgrade schedule failed")
}

return &types.MsgScheduleUpgradeClientResponse{}, nil

CLI

Documentation

Break up the implementation in multiple PRs. One possible suggestion:

@crodriguezvega crodriguezvega added needs discussion Issues that need discussion before they can be worked on 02-client change: api breaking Issues or PRs that break Go API (need to be release in a new major version) labels May 28, 2023
@crodriguezvega crodriguezvega added this to the v8.0.0 milestone May 28, 2023
@crodriguezvega crodriguezvega moved this to Todo in ibc-go May 28, 2023
@crodriguezvega crodriguezvega removed the needs discussion Issues that need discussion before they can be worked on label Jun 26, 2023
@charleenfei charleenfei self-assigned this Aug 21, 2023
@charleenfei charleenfei moved this from Todo to In review in ibc-go Aug 23, 2023
@crodriguezvega crodriguezvega moved this from In review to In progress in ibc-go Sep 3, 2023
@charleenfei charleenfei moved this from In progress to In review in ibc-go Sep 4, 2023
@colin-axner
Copy link
Contributor

accomplished by all the linked pr's. Commit to merge will come from #4620

@github-project-automation github-project-automation bot moved this from In review to Done in ibc-go Sep 11, 2023
Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment
Labels
02-client change: api breaking Issues or PRs that break Go API (need to be release in a new major version)
Projects
Archived in project
Development

No branches or pull requests

3 participants