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

ibc: fix timeouts: use client height and timestamp #4638

Merged
merged 2 commits into from
Jun 24, 2024
Merged

Conversation

avahowell
Copy link
Contributor

Previously, when processing timeouts, we used the client_processed_height and client_processed_timestamp data to determine if a timeout duration had elapsed:

if !self.packet.timed_out(&chain_ts, chain_height) {

However, those refer to a height and timestamp on penumbra when any client update for that client had last been processed:

state_key::client_processed_times(&client_id, &height),

This comparison is invalid, we should instead be checking against the timestamp and height of the last consensus state from the counterparty client. This PR does that. This was verified to fix the interchain test failures in #4634 .

Closes #4634

@avahowell avahowell added the consensus-breaking breaking change to execution of on-chain data label Jun 17, 2024
@conorsch
Copy link
Contributor

Upon further discussion, we are confident that this logic is correct and should be merged. However, we suspect there may still be follow-up required: there are two types of relevant timeouts here, height timeouts and timestamp timeouts. The IBC withdrawal struct requires that clients specify both: https://buf.build/penumbra-zone/penumbra/docs/main:penumbra.core.component.ibc.v1#penumbra.core.component.ibc.v1.Ics20Withdrawal so we should have two test cases in ICT, ensuring that each timeout type is exercised. We've notified @jtieri of this and he's working on it now.

@conorsch
Copy link
Contributor

However, we suspect there may still be follow-up required: there are two types of relevant timeouts here, height timeouts and timestamp timeouts.{

@avahowell if you have more thoughts on this, please make sure it's tracked in an issue, so we can follow up completely. Any further changes to the IBC packet handling will be consensus-breaking.

@conorsch conorsch self-requested a review June 24, 2024 15:38
Copy link
Contributor

@conorsch conorsch left a comment

Choose a reason for hiding this comment

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

Merging. As stated in comments, we may have more related changes once we perform further testing via ICT.

@conorsch conorsch merged commit d075efb into main Jun 24, 2024
13 checks passed
@conorsch conorsch deleted the ibc-timeout-fix branch June 24, 2024 15:38
Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment
Labels
consensus-breaking breaking change to execution of on-chain data
Projects
Status: Done
Development

Successfully merging this pull request may close these issues.

ibc: packet timeouts elapse, but withdrawn funds aren't refunded
2 participants