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

dex: 🌸 apply "fix chain halt in testnet 68" to main #3950

Merged

Conversation

cratelyn
Copy link
Contributor

@cratelyn cratelyn commented Mar 5, 2024

Testnet 68 halted at height 100736, with an .expect here:

.expect("handling batch swaps is infaillible");

This hit an error bubbled up from here:

let Some(accurate_max_price) = execution.max_price()? else {

The error occurs in this method, which is only ever used at that callsite:

pub fn max_price(&self) -> Result<Option<U128x128>> {

It's a little unclear why that method has double fallibility. Unfortunately, the answer may not be easily determined. It was added here

9cd566d

which indicates that there was previously an infallible max_price, but that code isn't included in the commit; the previous reference to an infallible max_price was added in this commit

b4b2635

which doesn't have the impl either, so presumably it got mangled during rebasing.

In any case, removing the double fallibility, as in this commit, allows committing block 100736 on testnet 68, which I verified by running this code against a local copy of a state snapshot.

Testnet 68 halted at height 100736, with an `.expect` here:

https://github.com/penumbra-zone/penumbra/blob/1c99e24ad5cf1ecc2855849d66221ecec25f9235/crates/core/component/dex/src/component/dex.rs#L66

This hit an error bubbled up from here:

https://github.com/penumbra-zone/penumbra/blob/1c99e24ad5cf1ecc2855849d66221ecec25f9235/crates/core/component/dex/src/component/router/route_and_fill.rs#L277

The error occurs in this method, which is only ever used at that callsite:

https://github.com/penumbra-zone/penumbra/blob/1c99e24ad5cf1ecc2855849d66221ecec25f9235/crates/core/component/dex/src/swap_execution.rs#L18

It's a little unclear why that method has double fallibility. Unfortunately, the answer may not be easily determined. It was added here

9cd566d

which indicates that there was previously an infallible `max_price`, but that code isn't included in the commit; the previous reference to an infallible `max_price` was added in this commit

b4b2635

which doesn't have the impl either, so presumably it got mangled during rebasing.

In any case, removing the double fallibility, as in this commit, allows committing block 100736 on testnet 68, which I verified by running this code against a local copy of a state snapshot.
@cratelyn cratelyn added the A-dex Area: Relates to the dex label Mar 5, 2024
@cratelyn cratelyn self-assigned this Mar 5, 2024
@cratelyn
Copy link
Contributor Author

cratelyn commented Mar 5, 2024

this is #3949, but cherry-picked for main

@cratelyn cratelyn added this to the Sprint 1 milestone Mar 5, 2024
@cratelyn cratelyn added the C-bug Category: a bug label Mar 5, 2024
@cratelyn
Copy link
Contributor Author

cratelyn commented Mar 5, 2024

with https://github.com/penumbra-zone/penumbra/releases/tag/v0.68.2 published, i am going to merge this.

@cratelyn cratelyn merged commit 98c42d8 into main Mar 5, 2024
6 checks passed
@cratelyn cratelyn deleted the kate/cherry-pick-3949-to-main.address-dex-div-by-zero branch March 5, 2024 18:50
Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment
Labels
A-dex Area: Relates to the dex C-bug Category: a bug
Projects
None yet
Development

Successfully merging this pull request may close these issues.

2 participants