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

fix(ibc)!: handle ibc withdrawals correctly #4787

Merged
merged 8 commits into from
Aug 4, 2024

Conversation

conorsch
Copy link
Contributor

@conorsch conorsch commented Aug 2, 2024

Describe your changes

This commit adds a use_compat_address bool to withdrawals, to allow issuing compat address withdrawals. It also implements correct error handling on withdrawal attemtps when counterparty chains return an error.

Issue ticket number and link

Checklist before requesting a review

  • If this code contains consensus-breaking changes, I have added the "consensus-breaking" label. Otherwise, I declare my belief that there are not consensus-breaking changes, for the following reason:

    This change does affect consensus, and should only be released after considerations have been made for compatibility.

This commit adds a `logic_version` flag to withdrawals,
to allow issuing compat address withdrawals. It also implements correct
error handling on withdrawal attemtps when counterparty chains
return an error.
// in the case where a counterparty chain acknowledges a packet with an error,
// for example due to a middleware processing issue or other behavior,
// the funds should be unescrowed back to the packet sender.
timeout_packet_inner(&mut state, &msg.packet)
Copy link
Contributor

Choose a reason for hiding this comment

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

So the logic in the ics 20 specification for handling error acknowledgements and for handling timeouts appears identical.

I do wonder if it would be more maintainable to make the code a bit more like spec.

Otherwise looks good

Copy link
Member

Choose a reason for hiding this comment

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

@avahowell can comment more here but I think the goal was to have a minimal diff that used the existing (audited) code as much as possible

Copy link
Contributor

Choose a reason for hiding this comment

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

Yes, I checked and verified that the behavior should be identical, then kept the diff minimal in order to minimize risk and maximize readability

Copy link
Contributor

Choose a reason for hiding this comment

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

Renamed timeout_packet_inner to conform to spec:

0b518d4

Copy link
Contributor

@cronokirby cronokirby left a comment

Choose a reason for hiding this comment

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

I went through all the files and this seems to correctly achieve that:

  1. that all outgoing ICS20 packets use the penumbracompat1... Bech32 encoding,
  2. that packet acknowledgements with errors are treated as timeouts.

One point of uncertainty I had which I managed to clarify away was whether or not we would be able to correctly handle FungibleTokenPacketData containing a penumbracompat1 address.
We can, because our handling of it uses the FromStr method for Address, which will handle both penumbracompat1 and penumbra1.

This commit adds a pd migration, intended to permit upgrade
coordination, so that breaking changes related to ibc withdrawal
handling can be made to pd safely. The migration doesn't alter chain
state: it simply flips the halt bit off, and permits the chain to
resume, after folks have upgraded to a new version of pd.
This approach assumes that an upgrade proposal was submitted and passed,
in order to halt the chain in a coordinated upgrade.

chore: bump crate versions to 0.80.0-alpha.1
This adds a state change to the migration, in order to replace all of
the packets which had error acknowledgements, so that, post-migration,
the acks can be replayed with the correct logic for error acks, the
packets having been replaced in the state.

This also changes the IBC component visibility of a trait to reuse the
logic for inserting packet commitments.
Comment on lines 5 to 6
//! There's no state-altering logic in this migration: the migration simply
//! flips the halt bit off, permitting the chain to restart post-upgrade.
Copy link
Member

Choose a reason for hiding this comment

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

This comment seems no longer accurate

@@ -52,6 +53,9 @@ pub enum Migration {
/// - Truncate various user-supplied `String` fields to a maximum length.
/// - Populate the DEX NV price idnexes with position data
Testnet78,
/// Mainnet-1 migration:
/// - Reset the halt bit
Copy link
Member

Choose a reason for hiding this comment

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

This comment seems no longer accurate

@conorsch conorsch marked this pull request as ready for review August 4, 2024 01:12
Copy link
Member

@hdevalence hdevalence left a comment

Choose a reason for hiding this comment

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

LGTM

@conorsch conorsch merged commit 69dd56a into release/v0.80.x Aug 4, 2024
11 checks passed
@conorsch conorsch deleted the compat-return-address branch August 4, 2024 01:32
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 state-breaking breaking change to on-chain data
Projects
None yet
Development

Successfully merging this pull request may close these issues.

6 participants