-
Notifications
You must be signed in to change notification settings - Fork 586
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
Implement Channel Upgrade Try functionality for 04 channel #3333
Changes from 61 commits
50e84bc
72d2c75
cef2f6b
ca6bc94
c3bc3ae
43eeaab
86753be
95b6a2c
21dfc70
524ec5e
f1f8e69
012d768
186c279
a823c2f
16ced5b
bde47ac
ac962e1
39fead1
add66ad
903396f
032551f
0c6cbfb
6287f1b
165e23d
f34298d
9ecf67b
5dc87f1
474a7d8
e56607f
30d076c
18e81a8
1696a70
4c51388
e797812
e5fd5d6
eed9b91
f1b19ba
c5cf6e6
9b750c1
1dde2ea
3f4205e
fcc0c34
0de9d39
564ddec
3124a14
ba9b2c4
ccced69
438ea63
7f7c399
ddf179d
900e632
8ddc924
0d49837
8fcca78
1886a50
ea1d7dc
9352b30
e389c77
4839088
05a299f
4eea882
004162b
7df0e52
35c7e35
64ef5aa
abe38fe
01b883f
bc145f9
dab7eb1
163cd9a
6547783
f445158
5874333
00a3136
c158a98
d6660b0
b79cdfc
946d3c5
61427c0
bd8574c
f9d0ee6
a3603f8
77848dd
File filter
Filter by extension
Conversations
Jump to
Diff view
Diff view
There are no files selected for viewing
Original file line number | Diff line number | Diff line change | ||||
---|---|---|---|---|---|---|
|
@@ -8,12 +8,14 @@ import ( | |||||
sdk "github.com/cosmos/cosmos-sdk/types" | ||||||
capabilitytypes "github.com/cosmos/cosmos-sdk/x/capability/types" | ||||||
|
||||||
"github.com/cosmos/ibc-go/v7/internal/collections" | ||||||
clienttypes "github.com/cosmos/ibc-go/v7/modules/core/02-client/types" | ||||||
connectiontypes "github.com/cosmos/ibc-go/v7/modules/core/03-connection/types" | ||||||
"github.com/cosmos/ibc-go/v7/modules/core/04-channel/types" | ||||||
portkeeper "github.com/cosmos/ibc-go/v7/modules/core/05-port/keeper" | ||||||
porttypes "github.com/cosmos/ibc-go/v7/modules/core/05-port/types" | ||||||
host "github.com/cosmos/ibc-go/v7/modules/core/24-host" | ||||||
"github.com/cosmos/ibc-go/v7/modules/core/exported" | ||||||
) | ||||||
|
||||||
// ChanUpgradeInit is called by a module to initiate a channel upgrade handshake with | ||||||
|
@@ -86,17 +88,249 @@ func (k Keeper) WriteUpgradeInitChannel( | |||||
channelID string, | ||||||
upgradeSequence uint64, | ||||||
channelUpgrade types.Channel, | ||||||
) { | ||||||
) error { | ||||||
defer telemetry.IncrCounter(1, "ibc", "channel", "upgrade-init") | ||||||
|
||||||
channel, found := k.GetChannel(ctx, portID, channelID) | ||||||
if !found { | ||||||
return errorsmod.Wrapf(types.ErrChannelNotFound, "failed to retrieve channel %s on port %s", channelID, portID) | ||||||
There was a problem hiding this comment. Choose a reason for hiding this commentThe reason will be displayed to describe this comment to others. Learn more. This should panic in my opinion. Then we can remove the error on this function. |
||||||
} | ||||||
|
||||||
// assign directly the fields that are modifiable. | ||||||
// counterparty fields may not be changed. | ||||||
channel.Version = channelUpgrade.Version | ||||||
channel.Ordering = channelUpgrade.Ordering | ||||||
channel.ConnectionHops = channelUpgrade.ConnectionHops | ||||||
|
||||||
// prevent silent failures if fields that are not allowed to be modified are changed in the upgrade channel | ||||||
// request. | ||||||
if !reflect.DeepEqual(channel, channelUpgrade) { | ||||||
return errorsmod.Wrapf(types.ErrInvalidChannel, "expected: %v but got: %v", channel, channelUpgrade) | ||||||
} | ||||||
|
||||||
k.SetChannel(ctx, portID, channelID, channelUpgrade) | ||||||
k.Logger(ctx).Info("channel state updated", "port-id", portID, "channel-id", channelID, "previous-state", types.OPEN.String(), "new-state", types.INITUPGRADE.String()) | ||||||
|
||||||
emitChannelUpgradeInitEvent(ctx, portID, channelID, upgradeSequence, channelUpgrade) | ||||||
return nil | ||||||
} | ||||||
|
||||||
// ChanUpgradeTry is called by a module to accept the first step of a channel upgrade | ||||||
// handshake initiated by a module on another chain. | ||||||
There was a problem hiding this comment. Choose a reason for hiding this commentThe reason will be displayed to describe this comment to others. Learn more. Maybe it would be good to add something here in the good about the upgrade sequence that is returned, because if this function returns a not |
||||||
func (k Keeper) ChanUpgradeTry( | ||||||
ctx sdk.Context, | ||||||
portID string, | ||||||
channelID string, | ||||||
chanCap *capabilitytypes.Capability, | ||||||
counterpartyChannel types.Channel, | ||||||
There was a problem hiding this comment. Choose a reason for hiding this commentThe reason will be displayed to describe this comment to others. Learn more.
|
||||||
counterpartyUpgradeSequence uint64, | ||||||
proposedUpgradeChannel types.Channel, | ||||||
timeoutHeight clienttypes.Height, | ||||||
timeoutTimestamp uint64, | ||||||
proofChannel []byte, | ||||||
proofUpgradeTimeout []byte, | ||||||
proofUpgradeSequence []byte, | ||||||
proofHeight clienttypes.Height, | ||||||
) (upgradeSequence uint64, previousVersion string, err error) { | ||||||
channel, found := k.GetChannel(ctx, portID, channelID) | ||||||
if !found { | ||||||
return 0, "", errorsmod.Wrapf(types.ErrChannelNotFound, "port ID (%s) channel ID (%s)", portID, channelID) | ||||||
} | ||||||
|
||||||
if !collections.Contains(channel.State, []types.State{types.OPEN, types.INITUPGRADE}) { | ||||||
There was a problem hiding this comment. Choose a reason for hiding this commentThe reason will be displayed to describe this comment to others. Learn more. this looks really nice. Should we add a comment indicating that this is accounting for crossing hellos? |
||||||
return 0, "", errorsmod.Wrapf(types.ErrInvalidChannelState, "expected one of [%s, %s], got %s", types.OPEN, types.INITUPGRADE, channel.State) | ||||||
} | ||||||
|
||||||
if !k.scopedKeeper.AuthenticateCapability(ctx, chanCap, host.ChannelCapabilityPath(portID, channelID)) { | ||||||
return 0, "", errorsmod.Wrapf(types.ErrChannelCapabilityNotFound, "caller does not own capability for channel, port ID (%s) channel ID (%s)", portID, channelID) | ||||||
} | ||||||
There was a problem hiding this comment. Choose a reason for hiding this commentThe reason will be displayed to describe this comment to others. Learn more. note: I'm fairly certain this is unnecessary, see spec issue. I would be so happy to see these unnecessary checks removed 😄 |
||||||
|
||||||
if proposedUpgradeChannel.Counterparty.PortId != channel.Counterparty.PortId || | ||||||
proposedUpgradeChannel.Counterparty.ChannelId != channel.Counterparty.ChannelId { | ||||||
return 0, "", errorsmod.Wrap(types.ErrInvalidChannel, "proposed channel upgrade is invalid") | ||||||
There was a problem hiding this comment. Choose a reason for hiding this commentThe reason will be displayed to describe this comment to others. Learn more. Should we use same error message as in INIT? |
||||||
} | ||||||
|
||||||
if !channel.Ordering.SubsetOf(proposedUpgradeChannel.Ordering) { | ||||||
return 0, "", errorsmod.Wrap(types.ErrInvalidChannelOrdering, "channel ordering must be a subset of the new ordering") | ||||||
} | ||||||
There was a problem hiding this comment. Choose a reason for hiding this commentThe reason will be displayed to describe this comment to others. Learn more. what do we think about adding a There was a problem hiding this comment. Choose a reason for hiding this commentThe reason will be displayed to describe this comment to others. Learn more. this is a good idea, we will update in a followup pr along with other util methods/small updates (see PR description) |
||||||
|
||||||
if counterpartyChannel.Ordering != proposedUpgradeChannel.Ordering { | ||||||
return 0, "", errorsmod.Wrapf(types.ErrInvalidChannelOrdering, "channel ordering of counterparty channel and proposed channel must be equal") | ||||||
} | ||||||
|
||||||
connectionEnd, err := k.getUpgradeTryConnectionEnd(ctx, portID, channelID, channel) | ||||||
There was a problem hiding this comment. Choose a reason for hiding this commentThe reason will be displayed to describe this comment to others. Learn more. this looks great. What about There was a problem hiding this comment. Choose a reason for hiding this commentThe reason will be displayed to describe this comment to others. Learn more. yes already ran into this problem looking into upgrade ack! The only thing that changes is the condition for a crossing hello There was a problem hiding this comment. Choose a reason for hiding this commentThe reason will be displayed to describe this comment to others. Learn more. we will make a generic function for this in a followup pr |
||||||
if err != nil { | ||||||
return 0, "", err | ||||||
} | ||||||
|
||||||
if connectionEnd.GetState() != int32(connectiontypes.OPEN) { | ||||||
return 0, "", errorsmod.Wrapf( | ||||||
connectiontypes.ErrInvalidConnectionState, | ||||||
"connection state is not OPEN (got %s)", connectiontypes.State(connectionEnd.GetState()).String(), | ||||||
) | ||||||
} | ||||||
There was a problem hiding this comment. Choose a reason for hiding this commentThe reason will be displayed to describe this comment to others. Learn more. Maybe we should add a helper function. We could do this as a pr to main to cleanup other code as well if !connectionEnd.IsOpen() { |
||||||
|
||||||
if connectionEnd.GetCounterparty().GetConnectionID() != counterpartyChannel.ConnectionHops[0] { | ||||||
There was a problem hiding this comment. Choose a reason for hiding this commentThe reason will be displayed to describe this comment to others. Learn more. we should add a comment like the spec changes have. This is a tricky check to understand There was a problem hiding this comment. Choose a reason for hiding this commentThe reason will be displayed to describe this comment to others. Learn more. which spec changes are you referring to? |
||||||
return 0, "", errorsmod.Wrapf(connectiontypes.ErrInvalidConnection, "unexpected counterparty channel connection hops, expected %s but got %s", connectionEnd.GetCounterparty().GetConnectionID(), counterpartyChannel.ConnectionHops[0]) | ||||||
} | ||||||
|
||||||
if err := k.connectionKeeper.VerifyChannelState(ctx, connectionEnd, proofHeight, proofChannel, proposedUpgradeChannel.Counterparty.PortId, | ||||||
There was a problem hiding this comment. Choose a reason for hiding this commentThe reason will be displayed to describe this comment to others. Learn more. So we are verifying 3 things:
From my understanding, there is no particular reason that sequence and timeout are separated within the provable store (they could be under the same upgrade type). The channel is under its' own path for no strong reason, but one benefit is that you can be certain the counterparty has paused packet processing. There could be a construction where all 3 are under the same provable upgrade type:
We could add a (ctx, connectionEnd, channel.Counterparty.PortId, channel.Counterparty.ChannelId, proofHeight, proofChannel, counterpartyChannel, proofUpgradeTimeout, upgradeTimeout, proofUpgradeSequence, upgradeSequence After seeing that argument list, I think I actually prefer doing the split here |
||||||
proposedUpgradeChannel.Counterparty.ChannelId, counterpartyChannel); err != nil { | ||||||
There was a problem hiding this comment. Choose a reason for hiding this commentThe reason will be displayed to describe this comment to others. Learn more. Counterparty fields shouldn't change, lets use the existing channel end |
||||||
return 0, "", err | ||||||
} | ||||||
|
||||||
upgradeTimeout := types.UpgradeTimeout{TimeoutHeight: timeoutHeight, TimeoutTimestamp: timeoutTimestamp} | ||||||
if err := k.connectionKeeper.VerifyChannelUpgradeTimeout(ctx, connectionEnd, proofHeight, proofUpgradeTimeout, proposedUpgradeChannel.Counterparty.PortId, | ||||||
proposedUpgradeChannel.Counterparty.ChannelId, upgradeTimeout); err != nil { | ||||||
return 0, "", err | ||||||
} | ||||||
|
||||||
if err := k.connectionKeeper.VerifyChannelUpgradeSequence(ctx, connectionEnd, proofHeight, proofUpgradeSequence, proposedUpgradeChannel.Counterparty.PortId, | ||||||
proposedUpgradeChannel.Counterparty.ChannelId, counterpartyUpgradeSequence); err != nil { | ||||||
There was a problem hiding this comment. Choose a reason for hiding this commentThe reason will be displayed to describe this comment to others. Learn more. ditto on |
||||||
return 0, "", err | ||||||
} | ||||||
There was a problem hiding this comment. Choose a reason for hiding this commentThe reason will be displayed to describe this comment to others. Learn more. We should be doing state verification of counterparty using the original channel connection. This is currently using the connection looked up from the proposed upgrade channel connection hops. This might look a little ugly as depending on whether or not we are in a crossing hello then we'll need to retrieve the connection from the current channel (not crossing hello, state: OPEN) or the restore channel (crossing hello, state: INITUPGRADE). cc. @AdityaSripal There was a problem hiding this comment. Choose a reason for hiding this commentThe reason will be displayed to describe this comment to others. Learn more. this is now being handled in the above |
||||||
|
||||||
// check if upgrade timed out by comparing it with the latest height of the chain | ||||||
selfHeight := clienttypes.GetSelfHeight(ctx) | ||||||
if !timeoutHeight.IsZero() && selfHeight.GTE(timeoutHeight) { | ||||||
if err := k.RestoreChannelAndWriteErrorReceipt(ctx, portID, channelID, upgradeSequence, types.ErrUpgradeTimeout); err != nil { | ||||||
There was a problem hiding this comment. Choose a reason for hiding this commentThe reason will be displayed to describe this comment to others. Learn more. Restoring the channel and writing a receipt here would only be needed if the channel state is in There was a problem hiding this comment. Choose a reason for hiding this commentThe reason will be displayed to describe this comment to others. Learn more. yes you're right, this actually ended up happening in #3418 which is probably not where that change should have been introduced 😅 |
||||||
return 0, "", errorsmod.Wrap(types.ErrUpgradeAborted, err.Error()) | ||||||
} | ||||||
return 0, "", errorsmod.Wrap(types.ErrUpgradeAborted, "upgrade timed out") | ||||||
There was a problem hiding this comment. Choose a reason for hiding this commentThe reason will be displayed to describe this comment to others. Learn more. Should we have a similar error message as for receive packet? |
||||||
} | ||||||
|
||||||
// check if upgrade timed out by comparing it with the latest timestamp of the chain | ||||||
if timeoutTimestamp != 0 && uint64(ctx.BlockTime().UnixNano()) >= timeoutTimestamp { | ||||||
There was a problem hiding this comment. Choose a reason for hiding this commentThe reason will be displayed to describe this comment to others. Learn more. Should we move this check up and just abort the tx, the relayer can call timout at this point if needed. However, we get into an awkward situation if we are also at UPGRADE_INIT, then we would have to wait for timeout on the other side as well |
||||||
if err := k.RestoreChannelAndWriteErrorReceipt(ctx, portID, channelID, upgradeSequence, types.ErrUpgradeTimeout); err != nil { | ||||||
return 0, "", errorsmod.Wrap(types.ErrUpgradeAborted, err.Error()) | ||||||
} | ||||||
return 0, "", errorsmod.Wrap(types.ErrUpgradeAborted, "upgrade timed out") | ||||||
There was a problem hiding this comment. Choose a reason for hiding this commentThe reason will be displayed to describe this comment to others. Learn more. Ditto about the error message in receive packet. |
||||||
} | ||||||
|
||||||
switch channel.State { | ||||||
case types.OPEN: | ||||||
upgradeSequence = uint64(0) | ||||||
There was a problem hiding this comment. Choose a reason for hiding this commentThe reason will be displayed to describe this comment to others. Learn more. This should be 1? There was a problem hiding this comment. Choose a reason for hiding this commentThe reason will be displayed to describe this comment to others. Learn more. shouldn't it actually still be 0 because we are in channel state |
||||||
if seq, found := k.GetUpgradeSequence(ctx, portID, channelID); found { | ||||||
upgradeSequence = seq | ||||||
} | ||||||
|
||||||
// if the counterparty upgrade sequence is ahead then fast forward so both channel ends are using the same sequence for the current upgrade | ||||||
if counterpartyUpgradeSequence > upgradeSequence { | ||||||
upgradeSequence = counterpartyUpgradeSequence | ||||||
k.SetUpgradeSequence(ctx, portID, channelID, upgradeSequence) | ||||||
} else { | ||||||
errorReceipt := types.NewErrorReceipt(upgradeSequence, errorsmod.Wrapf(types.ErrUpgradeAborted, "upgrade sequence %d was not smaller than the counter party chain upgrade sequence %d", upgradeSequence, counterpartyUpgradeSequence)) | ||||||
There was a problem hiding this comment. Choose a reason for hiding this commentThe reason will be displayed to describe this comment to others. Learn more.
Suggested change
|
||||||
// the upgrade sequence is incremented so both sides start the next upgrade with a fresh sequence. | ||||||
upgradeSequence++ | ||||||
|
||||||
k.SetUpgradeErrorReceipt(ctx, portID, channelID, errorReceipt) | ||||||
k.SetUpgradeSequence(ctx, portID, channelID, upgradeSequence) | ||||||
|
||||||
// TODO: emit error receipt events | ||||||
There was a problem hiding this comment. Choose a reason for hiding this commentThe reason will be displayed to describe this comment to others. Learn more. I don't mind doing error receipt events in a follow up PR, considering this is already quite large |
||||||
|
||||||
// do we want to return upgrade sequence here to include in response?? | ||||||
return 0, "", errorsmod.Wrapf(types.ErrUpgradeAborted, "upgrade aborted, error receipt written for upgrade sequence: %d", errorReceipt.GetSequence()) | ||||||
} | ||||||
|
||||||
// this is first message in upgrade handshake on this chain so we must store original channel in restore channel path | ||||||
// in case we need to restore channel later. | ||||||
k.SetUpgradeRestoreChannel(ctx, portID, channelID, channel) | ||||||
case types.INITUPGRADE: | ||||||
upgradeSequence, found = k.GetUpgradeSequence(ctx, portID, channelID) | ||||||
if !found { | ||||||
errorReceipt := types.NewErrorReceipt(upgradeSequence, errorsmod.Wrapf(types.ErrUpgradeAborted, "upgrade sequence %d was not smaller than the counter party chain upgrade sequence %d", upgradeSequence, counterpartyUpgradeSequence)) | ||||||
There was a problem hiding this comment. Choose a reason for hiding this commentThe reason will be displayed to describe this comment to others. Learn more.
Suggested change
|
||||||
k.SetUpgradeErrorReceipt(ctx, portID, channelID, errorReceipt) | ||||||
|
||||||
return 0, "", errorsmod.Wrapf(types.ErrUpgradeAborted, "upgrade aborted, error receipt written for upgrade sequence: %d", upgradeSequence) | ||||||
} | ||||||
|
||||||
if upgradeSequence != counterpartyUpgradeSequence { | ||||||
errorReceipt := types.NewErrorReceipt(upgradeSequence, errorsmod.Wrapf(types.ErrUpgradeAborted, "upgrade sequence %d was not equal to the counter party chain upgrade sequence %d", upgradeSequence, counterpartyUpgradeSequence)) | ||||||
There was a problem hiding this comment. Choose a reason for hiding this commentThe reason will be displayed to describe this comment to others. Learn more.
Suggested change
|
||||||
|
||||||
// set to the max of the two | ||||||
if counterpartyUpgradeSequence > upgradeSequence { | ||||||
upgradeSequence = counterpartyUpgradeSequence | ||||||
k.SetUpgradeSequence(ctx, portID, channelID, upgradeSequence) | ||||||
} | ||||||
|
||||||
k.SetUpgradeErrorReceipt(ctx, portID, channelID, errorReceipt) | ||||||
return 0, "", errorsmod.Wrapf(types.ErrUpgradeAborted, "upgrade aborted, error receipt written for upgrade sequence: %d", upgradeSequence) | ||||||
There was a problem hiding this comment. Choose a reason for hiding this commentThe reason will be displayed to describe this comment to others. Learn more. The upgrade sequence mentioned in this error message could be different than the one in the error receipt if it gets updated in the if clause just above. |
||||||
} | ||||||
|
||||||
// if there is a crossing hello, i.e an UpgradeInit has been called on both channelEnds, | ||||||
// then we must ensure that the proposedUpgrade by the counterparty is the same as the currentChannel | ||||||
// except for the channel state (upgrade channel will be in TRYUPGRADE and current channel will be in INITUPGRADE) | ||||||
// if the proposed upgrades on either side are incompatible, then we will restore the channel and cancel the upgrade. | ||||||
channel.State = types.TRYUPGRADE | ||||||
k.SetChannel(ctx, portID, channelID, channel) | ||||||
There was a problem hiding this comment. Choose a reason for hiding this commentThe reason will be displayed to describe this comment to others. Learn more. Should this There was a problem hiding this comment. Choose a reason for hiding this commentThe reason will be displayed to describe this comment to others. Learn more. The updated channel state is used just a few lines down and I think separating them out makes things a little harder to follow in this case There was a problem hiding this comment. Choose a reason for hiding this commentThe reason will be displayed to describe this comment to others. Learn more. Yes, but to use the channel state you don't need it stored, right? For me it's just that I think it would be more clear if you set the channel when all the checks have passed (like in upgrade init). I know that if there's an error, the state changes will be reverted, but feels like the code would be more clear if it's at the end, instead of in between this complicated logic. There was a problem hiding this comment. Choose a reason for hiding this commentThe reason will be displayed to describe this comment to others. Learn more. I tend to agree with @chatton that it is clearer to keep the logic together, rather than having the channel state being updated in one place and then actually set further down with the potential restore channel logic happening in between |
||||||
|
||||||
if !reflect.DeepEqual(channel, proposedUpgradeChannel) { | ||||||
// TODO: log and emit events | ||||||
if err := k.RestoreChannelAndWriteErrorReceipt(ctx, portID, channelID, upgradeSequence, types.ErrInvalidChannel); err != nil { | ||||||
return 0, "", errorsmod.Wrap(types.ErrUpgradeAborted, err.Error()) | ||||||
} | ||||||
return 0, "", errorsmod.Wrap(types.ErrUpgradeAborted, "proposed upgrade channel did not equal expected channel") | ||||||
} | ||||||
|
||||||
// retrieve restore channel to return previous version | ||||||
restoreChannel, found := k.GetUpgradeRestoreChannel(ctx, portID, channelID) | ||||||
if !found { | ||||||
//(this case should be unreachable): write error receipt for upgrade sequence and abort / cancel upgrade | ||||||
errorReceipt := types.NewErrorReceipt(upgradeSequence, err) | ||||||
k.SetUpgradeErrorReceipt(ctx, portID, channelID, errorReceipt) | ||||||
return 0, "", errorsmod.Wrapf(types.ErrChannelNotFound, "upgrade aborted, error receipt written for upgrade sequence: %d", upgradeSequence) | ||||||
} | ||||||
|
||||||
return upgradeSequence, restoreChannel.Version, nil | ||||||
default: | ||||||
return 0, "", errorsmod.Wrapf(types.ErrInvalidChannelState, "expected one of [%s, %s] but got %s", types.OPEN, types.INITUPGRADE, channel.State) | ||||||
} | ||||||
|
||||||
return upgradeSequence, channel.Version, nil | ||||||
} | ||||||
|
||||||
// RestoreChannel restores the given channel to the state prior to upgrade. | ||||||
func (k Keeper) RestoreChannel(ctx sdk.Context, portID, channelID string, upgradeSequence uint64, err error) error { | ||||||
// WriteUpgradeTryChannel writes a channel which has successfully passed the UpgradeTry handshake step. | ||||||
// An event is emitted for the handshake step. | ||||||
func (k Keeper) WriteUpgradeTryChannel( | ||||||
ctx sdk.Context, | ||||||
portID, | ||||||
channelID, | ||||||
proposedUpgradeVersion string, | ||||||
upgradeSequence uint64, | ||||||
channelUpgrade types.Channel, | ||||||
There was a problem hiding this comment. Choose a reason for hiding this commentThe reason will be displayed to describe this comment to others. Learn more. see #3401. I know it isn't clean/optimized, but I actually think the channel end should be retrieved from state instead of writing the proposed upgrade channel. This comes from a general security principal that you should be recreating the data you want to write to disk rather than writing the user input (as in this case it is unsafe without an additional check). This is partially why we do this for openAck and openConfirm. I'm open to differing opinions, but it would be nice to be defensive here, if another field is added to the channel end which cannot be modified in an upgrade, we would need to remember to add another defensive check There was a problem hiding this comment. Choose a reason for hiding this commentThe reason will be displayed to describe this comment to others. Learn more. What if we add There was a problem hiding this comment. Choose a reason for hiding this commentThe reason will be displayed to describe this comment to others. Learn more. There was a problem hiding this comment. Choose a reason for hiding this commentThe reason will be displayed to describe this comment to others. Learn more. nice catch, but do we need to create a separate type for this? We can fetch the existing channel and then modify it based on values present in There was a problem hiding this comment. Choose a reason for hiding this commentThe reason will be displayed to describe this comment to others. Learn more. The benefit of a separate type I can think of is that not all fields are upgradable, and so some might end up being ignored. With a custom type we can provide only fields that are valid to upgrade. There was a problem hiding this comment. Choose a reason for hiding this commentThe reason will be displayed to describe this comment to others. Learn more. Yes exactly. The benefit of that is that there is less room for error if the submitter specifies the non-modifiable fields incorrectly. We could use a similar syntax to client state There was a problem hiding this comment. Choose a reason for hiding this commentThe reason will be displayed to describe this comment to others. Learn more. I think a helper like that is the best option, this PR is getting a little unwieldy so I think I will create a follow up for this if you're okay with it! |
||||||
) error { | ||||||
defer telemetry.IncrCounter(1, "ibc", "channel", "upgrade-try") | ||||||
|
||||||
channel, found := k.GetChannel(ctx, portID, channelID) | ||||||
if !found { | ||||||
return errorsmod.Wrapf(types.ErrChannelNotFound, "failed to retrieve channel %s on port %s", channelID, portID) | ||||||
} | ||||||
|
||||||
// assign directly the fields that are modifiable. | ||||||
// counterparty fields may not be changed. | ||||||
channel.Version = proposedUpgradeVersion | ||||||
channel.Ordering = channelUpgrade.Ordering | ||||||
channel.ConnectionHops = channelUpgrade.ConnectionHops | ||||||
There was a problem hiding this comment. Choose a reason for hiding this commentThe reason will be displayed to describe this comment to others. Learn more. Nit: this code updating the channel fields is duplicated in There was a problem hiding this comment. Choose a reason for hiding this commentThe reason will be displayed to describe this comment to others. Learn more. Are you happy if we introduce a helper function in a follow up? There was a problem hiding this comment. Choose a reason for hiding this commentThe reason will be displayed to describe this comment to others. Learn more. Yes, more than happy! :) |
||||||
|
||||||
// prevent silent failures if fields that are not allowed to be modified are changed in the upgrade channel | ||||||
// request. | ||||||
if !reflect.DeepEqual(channel, channelUpgrade) { | ||||||
return errorsmod.Wrapf(types.ErrInvalidChannel, "expected: %v but got: %v", channel, channelUpgrade) | ||||||
} | ||||||
|
||||||
k.SetChannel(ctx, portID, channelID, channel) | ||||||
|
||||||
// TODO: previous state will not be OPEN in the case of crossing hellos. Determine this state correctly. | ||||||
k.Logger(ctx).Info("channel state updated", "port-id", portID, "channel-id", channelID, "previous-state", types.OPEN.String(), "new-state", types.TRYUPGRADE.String()) | ||||||
There was a problem hiding this comment. Choose a reason for hiding this commentThe reason will be displayed to describe this comment to others. Learn more. minor nit: in crossing hellos previous state would be |
||||||
|
||||||
emitChannelUpgradeTryEvent(ctx, portID, channelID, upgradeSequence, channelUpgrade) | ||||||
return nil | ||||||
} | ||||||
|
||||||
// TODO: should we pull out the error receipt logic from this function? They seem like two discrete operations. | ||||||
|
||||||
// RestoreChannelAndWriteErrorReceipt restores the given channel to the state prior to upgrade. | ||||||
func (k Keeper) RestoreChannelAndWriteErrorReceipt(ctx sdk.Context, portID, channelID string, upgradeSequence uint64, err error) error { | ||||||
errorReceipt := types.NewErrorReceipt(upgradeSequence, err) | ||||||
k.SetUpgradeErrorReceipt(ctx, portID, channelID, errorReceipt) | ||||||
|
||||||
|
@@ -124,5 +358,31 @@ func (k Keeper) RestoreChannel(ctx sdk.Context, portID, channelID string, upgrad | |||||
return errorsmod.Wrapf(porttypes.ErrInvalidRoute, "route not found to module: %s", module) | ||||||
} | ||||||
|
||||||
return cbs.OnChanUpgradeRestore(ctx, portID, channelID) | ||||||
cbs.OnChanUpgradeRestore(ctx, portID, channelID) | ||||||
return nil | ||||||
} | ||||||
|
||||||
// getUpgradeTryConnectionEnd returns the connection end that should be used. During crossing hellos, the restore | ||||||
// channel connection end is used, while in a regular flow the current channel connection end is used. | ||||||
func (k Keeper) getUpgradeTryConnectionEnd(ctx sdk.Context, portID string, channelID string, currentChannel types.Channel) (exported.ConnectionI, error) { | ||||||
isCrossingHellos := currentChannel.State == types.INITUPGRADE | ||||||
if isCrossingHellos { | ||||||
// fetch restore channel | ||||||
restoreChannel, found := k.GetUpgradeRestoreChannel(ctx, portID, channelID) | ||||||
if !found { | ||||||
return nil, errorsmod.Wrapf(types.ErrChannelNotFound, "port ID (%s) channel ID (%s)", portID, channelID) | ||||||
} | ||||||
connectionEnd, err := k.GetConnection(ctx, restoreChannel.ConnectionHops[0]) | ||||||
if err != nil { | ||||||
return nil, err | ||||||
} | ||||||
return connectionEnd, nil | ||||||
} | ||||||
|
||||||
// use current channel | ||||||
connectionEnd, err := k.GetConnection(ctx, currentChannel.ConnectionHops[0]) | ||||||
if err != nil { | ||||||
return nil, err | ||||||
} | ||||||
return connectionEnd, nil | ||||||
} |
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
we should emit the channel order as well since it can change. Looks like it is missing in Init as well