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

auction: process triggers/execute DAs #4254

Merged
merged 32 commits into from
Apr 25, 2024
Merged

auction: process triggers/execute DAs #4254

merged 32 commits into from
Apr 25, 2024

Conversation

erwanor
Copy link
Member

@erwanor erwanor commented Apr 22, 2024

Describe your changes

This PR implements #4240, in particular it adds the "end block" DA auction logic, and:

  • extend the auction with a HandleDutchTriggers trait, responsible for streaming auction state recorded for execution at a trigger_height
  • implement execute_dutch_auction, which encapsulate auction LP logic and state updates
  • add price interpolation logic
  • create penumbra_dex::lp::Position to take a user specified random nonce (computed based off the auction description, step index, and a counter, in order to evade id-squatting)
  • encapsulate TriggerData into its own submodule (DA step function utils)

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 is consensus breaking

@erwanor erwanor added A-dex Area: Relates to the dex A-auction Area: Relates to the auction component labels Apr 22, 2024
@erwanor erwanor self-assigned this Apr 22, 2024
@erwanor erwanor force-pushed the erwan/4211_end_block_impl branch from 0539ed2 to 1d66fb7 Compare April 22, 2024 17:09
@erwanor erwanor added the consensus-breaking breaking change to execution of on-chain data label Apr 22, 2024
@erwanor erwanor requested review from TalDerei and hdevalence April 22, 2024 18:06
@erwanor erwanor marked this pull request as ready for review April 22, 2024 18:06
@hdevalence
Copy link
Member

create penumbra_dex::lp::Position to take a user specified random nonce (specified in the auction description)

Interesting find, I didn't think about the position nonces during the design. Can we clarify this a little bit?

One case we should think through is what happens if the position ID is already used. Since the auction data is public, any position ID that the chain can deterministically derive from it can also be predicted by an external party. Someone could attempt to claim the position ID first and grief the auction that way.

  1. What happens in the current code if the position ID is already used? If this triggers an error, that's a potential DoS vector and should be avoided.

  2. Can we create position IDs in some other way that would be robust to position-claiming attacks? For instance, what if the nonce for the position was something like position_nonce = H( auction_nonce || step_number || counter ), starting with counter = 0 and incrementing counter until the resulting position_id was unused? Then creating the position would always be infallible, and the fact that it's infallible removes the incentive to grief (so counter = 0 would ~always work in practice).

@cratelyn cratelyn added this to the Sprint 5 milestone Apr 22, 2024
@erwanor
Copy link
Member Author

erwanor commented Apr 22, 2024

@hdevalence Thanks for the suggestion, I implemented it if you want to take a look - the current logic has an ugly failure mode because the position manager returns coarse errors (i.e. it's not possible to distinguish between a duplicate position id and an incoherent position state). I will fix it myself as part of M1 so that we only retry opening if the position id is squatted, and either terminate the auction, or bubble up the error, if a more serious error occurs.

@hdevalence
Copy link
Member

I think as a first pass we could ignore the error details and retry unconditionally

@erwanor
Copy link
Member Author

erwanor commented Apr 22, 2024

@hdevalence could you give this a +1?

@hdevalence
Copy link
Member

Will do as soon as I'm back at the computer

@erwanor
Copy link
Member Author

erwanor commented Apr 23, 2024

I did a round of improvements, thank you for the engaged review and feedback, I think this PR is much better as a result of your attention. In particular, I want to call out the following changes:

  1. we systematically zero out the action input state as its deployed in a PCLP
  2. the answer to value leakage on failure/state incoherence is that it doesn't happen because we panic (for now), and we will tackle fallibility in a dedicated value breaker effort.
  3. we interpolate to a minimum price at step_count - 1
  4. we add a termination condition clause if the input reserves are exhausted

@erwanor
Copy link
Member Author

erwanor commented Apr 24, 2024

@hdevalence this is ready for another round of review

Comment on lines +15 to +16
/// This method errors if the block interval is not a multiple of the
/// specified `step_count`, or if it operates over an invalid block
Copy link
Member

Choose a reason for hiding this comment

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

The method doesn't actually error in this case though right? The step_size will be rounded down. I'm not sure it matters here but it doesn't quite match the description.


// Compute the step size, based on the block interval and the number of
// discrete steps the auction specifies.
let step_size = block_interval / *step_count;
Copy link
Member

Choose a reason for hiding this comment

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

Why is it OK to assume that step_count != 0 here but not above? The behavior should be consistent, right? Maybe we should make TriggerData have private fields and enforce the invariants we want on construction?

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.

This looks great! My comments are non-blocking, and could be addressed in follow-up PRs if we want to get to them.

@erwanor
Copy link
Member Author

erwanor commented Apr 25, 2024

Thanks a lot for the review, I think this is mergeable but I agree with your comments about TriggerData. I think we want:

  1. Make TriggerData inner field private
  2. Enforce parameter validation via a constructor
  3. Use that constructor in the action handlers
  4. Drop checked operation (since we operate behind a validation boundary)

I will do that in a followup PR, and open a ticket in the meantime

@erwanor erwanor merged commit 1253187 into main Apr 25, 2024
8 checks passed
@erwanor erwanor deleted the erwan/4211_end_block_impl branch April 25, 2024 15:54
Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment
Labels
A-auction Area: Relates to the auction component A-dex Area: Relates to the dex consensus-breaking breaking change to execution of on-chain data
Projects
Status: Done
Development

Successfully merging this pull request may close these issues.

4 participants