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: fix chain halt in testnet 68 #3949

Merged
merged 1 commit into from
Mar 5, 2024
Merged

Conversation

hdevalence
Copy link
Member

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.
@hdevalence
Copy link
Member Author

I opened this against the release branch, for ease of making a point release, we'd eventually want to land it in main.

@cratelyn cratelyn added the A-dex Area: Relates to the dex label Mar 5, 2024
@conorsch conorsch merged commit 7d16337 into release/v0.68.x Mar 5, 2024
4 checks passed
@conorsch conorsch deleted the address-dex-div-by-zero branch March 5, 2024 18:25
@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
@erwanor erwanor added the zellic-component-remediated Tag PRs that are remediating Zellic findings label Mar 12, 2024
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 zellic-component-remediated Tag PRs that are remediating Zellic findings
Projects
None yet
Development

Successfully merging this pull request may close these issues.

4 participants