fix!: Remove panics on failure to send IBC packets

[mpoke]

Description

Closes: #731

On both the provider and consumer, if the sent fails, the packet is kept in the queue and a re-sent is attempt in the next block.

• Replace the panic on the provider with StopConsumerChain. Note that most likely if the send if failing here, the consumer is malicious and it should be removed.
• Replace the panic on the consumer with an error message. If packets are not eventually sent and relayed, the vscTimeout on the provider will be triggered and the consumer will be removed.

---

Author Checklist

All items are required. Please add a note to the item if the item is not applicable and
please add links to any relevant follow up issues.

I have...

• Included the correct type prefix in the PR title
• Added ! to the type prefix if API or client breaking change
• Targeted the correct branch (see PR Targeting)
• Provided a link to the relevant issue or specification
• Followed the guidelines for building SDK modules
• Included the necessary unit and integration tests
  • @smarshall-spitzbart could you please add the necessary tests (it seems that no tests were broken)
• Added a changelog entry to CHANGELOG.md
• Included comments for documenting Go code
• Updated the relevant documentation or specification
  • @MSalopek Do we have docs on this behavior or it's too involved for the docs?
• Reviewed "Files changed" and left comments if necessary
• Confirmed all CI checks have passed

Reviewers Checklist

All items are required. Please add a note if the item is not applicable and please add
your handle next to the items reviewed if you only reviewed selected items.

I have...

• confirmed the correct type prefix in the PR title
• confirmed ! in the type prefix if API or client breaking change
• confirmed all author checklist items have been addressed
• reviewed state machine logic
• reviewed API design and naming
• reviewed documentation is accurate
• reviewed tests and test coverage

[shaspitz]

@mpoke it's not intuitive to me why a failed IBC send would always indicate a malicious consumer. The error being returned could be caused by corrupted IBC state on the provider, where stoping a consumer could make such a scenario worse

[mpoke]

Here's my analysis on the reasons for ccv.SendIBCPacket to return error on the provider side:

• Channel not found – not possible as SendIBCPacket is called only for consumer with an established CCV channel and channels are not removed from the IBC module
• Module does not own channel capability – not possible as the capability was already claimed in OnChanOpenTry, before the CCV channel was marked as established
• The next sequence send is not found – if that happens, something is wrong with the IBC module store
• The packet sent failed basic validation – if that happens, something is wrong with the code on the provider chain as the packet is constructed in SendIBCPacket
• The channel’s state is CLOSED – an opened channel can be closed only by one of the following functions
  • channelKeeper.ChanCloseInit, which is called on the provider only when the consumer chain is removed (note that OnChanCloseInit returns an error, thus user-initiated channel closing are not allowed)
  • channelKeeper.ChanCloseConfirm, which entails that the consumer endpoint of the channel is CLOSED; this can happen only if the consumer receives a negative ack for a packet sent to the provider
    • Attack: A malicious consumer could send an invalid SlashPacket that fails ValidateSlashPacket, this would lead to a nack sent to the consumer, which results in channelKeeper.ChanCloseInit being called on the consumer. Finally, this would enable a relayer to send a MsgChannelCloseConfirm to the provider, which would move the channel’s state to CLOSED without updating the ChainToChannel state.
  • channelKeeper.TimeoutExecuted, which means that OnTimeoutPacket is first executed on the provider, which results in the consumer chain being removed (i.e., the ChainToChannel state is being updated).
• The packet’s destination channelD and portID do not match the ones of the counterparty endpoint – not possible due to construction of the packet
• There is no underlying connection – not possible as it was checked during the handshake
• There is no underlying client state – not possible as it was checked during the handshake
• The packet is already timed out on the consumer – not possible as the packet timeout is set in SendIBCPacket to the current block timestamp plus the timeout period and the latest known timestamp on the consumer cannot be larger than this packet timeout.

[mpoke]

To be more conservative, we could just log an error and leave the packets in the queue. Operators need to notice that VSCPackets are no longer being relayed, otherwise the unbondings in those packets will never complete. Note that the VSC timeout is set when the packet is sent, and not when it's queued.

So we run the risk of nobody noticing for a while, which will affect users. Once we notice, we need to either do an emergency upgrade where we migrate the state (difficult) or we submit a ConsumerRemoval proposal, which would take 2 extra weeks before delegators that unbonded can get their funds back.

[jtremback]

Tough question. According to the untrusted consumer doctrine, we should not let consumers disrupt the provider. So that would lean towards just removing the consumer.

However, the unbonding pausing is somewhat of an exception that we allow, since it is limited by the VSC timeout. Since we already allow the consumer to disrupt the provider up to the VSC timeout just by not sending VSC acks, it would make sense to allow disruption up to the VSC timeout here as well.

However, if we don't stop the consumer, is it possible to recover from this state without a provider migration?

Finally, this would enable a relayer to send a MsgChannelCloseConfirm to the provider, which would move the channel’s state to CLOSED without updating the ChainToChannel state.

Is there any way to change this, so that a MsgChannelCloseConfirm would update the ChainToChannel state?

So in conclusion:

• I support stopping the consumer here, but I want to make sure that this can only happen due to very incorrect code on the consumer, and is very unlikely to happen "by accident".
• Why do we send a nack in response to an invalid slash packet? It seems like it would be better to just ignore those packets to avoid having the consumer stop in this code here.

[shaspitz]

The check for if clienttypes.ErrClientNotActive.Is(err) is no longer needed since behavior is the same whether that statement is true or not. The type of error will be printed

[mpoke]

The behavior is a bit different. For expired clients we log an info message. For other errors, we log an error as something is wrong.

[shaspitz]

But "error" is in the name, ErrClientNotActive. I see the logging behavior is slightly different for that error, but imo a semantic error should be logged as an error. Which also makes the code more simple/readable

[shaspitz]

@mpoke I've added a simple test for the consumer. Re provider i'll wait on writing the test until we chat, since if we remove the StopConsumerChain call, the behavior would be a lot simpler to test

[shaspitz]

@mpoke tests have been added, lmk if they seem correct

[mpoke]

LGTM

[shaspitz]

Approval

[MSalopek]

Approval.

So the behaviour is this:

• if the consumer cannot send, it will retain packets
• if the provider cannot send VSC packets it will:
  a) retain unsent packets for consumers with inactive clients
  b) delete all packets for consumers if the error is caused by anything other than an inactive client

Re: docs
This is very nuanced, it should probably go into a new section about relaying behaviour

[p-offtermatt]

LGTM, only very minor nitpicks. I looked most closely at the tests and I don't have an opinion on stopping consumer vs logging and ignoring

[p-offtermatt]

.Times(1) seems unnecessary

Suggested change

	"CCVChannelID").Return(channeltypes.Channel{}, false).Times(1),
	"CCVChannelID").Return(channeltypes.Channel{}, false),

[shaspitz]

Hmm I don't see the reason to remove the Times(1) since we indeed only want this method called once. Better to be explicit with the mocking package imo

[p-offtermatt]

Times(1) seems unnecessary

Suggested change

	"consumerCCVChannelID").Return(channeltypes.Channel{}, false).Times(1),
	"consumerCCVChannelID").Return(channeltypes.Channel{}, false),