UIP-5: outbound PFM support #4923
Closed
Conversation
This file contains bidirectional Unicode text that may be interpreted or compiled differently than what appears below. To review, open the file in an editor that reveals hidden Unicode characters.
Learn more about bidirectional Unicode characters
This restructures the AppView interface to allow processing events in a batch. Previously, app views had to index one event at a time. This PR changes things so that app views get a batch of several blocks worth of events, with a guarantee to have all of the events in any block in the batch. ## Making App Views Easier to Write By having access to all the events in a block, app views are more ergonomic to write. For example, the dex explorer app view wants to know the time of the candlestick events it processes, but to do this, it needs to wait for the block root event later in the block, which provides this timestamp. Currently, because we don't have access to any context, we need to manually implement a queuing system in the database, which is very annoying, and a performance hit. ## Making App Views More Performant We can make app views more performant by processing both an entire block, and multiple blocks, since: - we don't need to write an update more than once per block to the database - we may be able to write updates less frequently, depending on the app view (e.g. when we need only the current value) - we can keep transient state in memory, instead of on the database, reducing writes and reads in all cases ## Additional Performance Improvements Now the app views are run in parallel, which provides additional improvements when syncing up. ## Testing Pindexer should work as usual, after wiping the database. # Checklist - [x] I have added guiding text to explain how a reviewer should test these changes. - [x] 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: > pindexer only
## Describe your changes Closes #4914. The internal architecture of the app view tries to make use of batch processing to the extent possible, which simplifies a lot of the logic. The price charts remain unchanged, but I collapsed the two tables into one for performance and simplicity. I also **did not** implement insertion of empty candles ; if there are gaps in the events, there will be gaps in the resulting database as well. The main addition and where I spent most of my time on this is the addition of summaries of information over arbitrary windows. The idea behind the architecture here is that any time a change to liquidity, trade count, or a candle for a directed pair happens in a block, that block then gets a "snapshot" inserted, with the current price, liquidity, volume in that block, etc. At the end of this batch, the current summary is then updated, for each window, using those timed snapshots. And then an aggregate summary, across all pairs, is created from these summaries, for each window. In order to price values under a common denom, assets are filtered based on having a current USDC price, backed by enough liquidity (the denom and liquidity amount are parameters to the component). For testing, I'd recommend trying to run the app view against mainnet and testnet, and checking some sanity items like the price not seeming crazy, and matching in the summary across all windows, etc. I think for testing we'll notice potential issues relatively quickly when dogfooding the explorer. ## Checklist before requesting a review - [x] I have added guiding text to explain how a reviewer should test these changes. - [x] 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: > indexing only
cronokirby
added
the
consensus-breaking
|
This breaks consensus, right? I think we need to retarget this PR towards release/v0.81.X instead |
|
Yup, consensus breaking but not proto breaking |
erwanor
reviewed
Nov 15, 2024
erwanor
added
protobuf-changes
cronokirby
approved these changes
Nov 15, 2024
conorsch
requested changes
Nov 15, 2024
|
Ugh, changing the base has picked up other commits making the diff monstruous. I will re-open this and target the 81 branch directly. |
erwanor
added a commit
that referenced
this pull request
Nov 22, 2024
## Describe your changes Implementation of the candidate UIP-5: outbound PFM support (see https://forum.penumbra.zone/t/pre-uip-outbound-packet-forwarding-middleware-support/121) Should be reviewed for correctness and adherence to the spec. Note that this only includes the required protocol changes to allow clients to use the new memo field (a prerequisite for PFM support), and does not implement outbound packet forwarding directly (that is to be done in the client). By @avahowell cherry-picked from #4923 Co-authored-by: Ava Howell <ava@avahowell.me>
conorsch
pushed a commit
that referenced
this pull request
Nov 22, 2024
## Describe your changes Implementation of the candidate UIP-5: outbound PFM support (see https://forum.penumbra.zone/t/pre-uip-outbound-packet-forwarding-middleware-support/121) Should be reviewed for correctness and adherence to the spec. Note that this only includes the required protocol changes to allow clients to use the new memo field (a prerequisite for PFM support), and does not implement outbound packet forwarding directly (that is to be done in the client). By @avahowell cherry-picked from #4923 Co-authored-by: Ava Howell <ava@avahowell.me>
Sign up for free
to join this conversation on GitHub.
Already have an account?
Sign in to comment
Add this suggestion to a batch that can be applied as a single commit.
This suggestion is invalid because no changes were made to the code.
Suggestions cannot be applied while the pull request is closed.
Suggestions cannot be applied while viewing a subset of changes.
Only one suggestion per line can be applied in a batch.
Add this suggestion to a batch that can be applied as a single commit.
Applying suggestions on deleted lines is not supported.
You must change the existing code in this line in order to create a valid suggestion.
Outdated suggestions cannot be applied.
This suggestion has been applied or marked resolved.
Suggestions cannot be applied from pending reviews.
Suggestions cannot be applied on multi-line comments.
Suggestions cannot be applied while the pull request is queued to merge.
Suggestion cannot be applied right now. Please check back later.
Describe your changes
Implementation of the candidate UIP-5: outbound PFM support (see https://forum.penumbra.zone/t/pre-uip-outbound-packet-forwarding-middleware-support/121)
Should be reviewed for correctness and adherence to the spec.
Note that this only includes the required protocol changes to allow clients to use the new memo field (a prerequisite for PFM support), and does not implement outbound packet forwarding directly (that is to be done in the client).