-
Notifications
You must be signed in to change notification settings - Fork 316
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
Record transactions into pindexer #5081
base: main
Are you sure you want to change the base?
Conversation
Needs rebasing, due to merge of #5063. |
ee1ac1b
to
1176d71
Compare
Updated to match what's currently run in the smoke test suite [0]. Made sure to test locally, in consultation with @vacekj on [1]. [0] https://github.com/penumbra-zone/penumbra/blob/259b498d894369b8e218ff87592a40681b2a149b/deployments/compose/process-compose-smoke-test.yml#L46-L47 [1] penumbra-zone/penumbra#5081
Updated to match what's currently run in the smoke test suite [0]. Made sure to test locally, in consultation with @vacekj on [1]. [0] https://github.com/penumbra-zone/penumbra/blob/259b498d894369b8e218ff87592a40681b2a149b/deployments/compose/process-compose-smoke-test.yml#L46-L47 [1] penumbra-zone/penumbra#5081
The raw ABCI event database already has all the transaction data, and this PR amends our processing pipeline in cometindex to present blocks containing both a list of events, each of which may directly have all of this transaction data (along with a transaction hash), and a list of all the transactions themselves. This also refactors things to be a bit more opaque, giving us more control over the internal representation of the batch of events. The `ContextualizedEvent` struct is probably a little over-engineered, but avoids needlessly copying the transaction data around. Ultimately the performance of indexing is tied to data throughput, so we should be mindful of needless copying in the architecture. This should help with the draft in #5081, avoiding the need for creating a bespoke event. I tested this by setting up a little indexer just parsing out the tx data and serializing the JSON, and ## 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 code changes only
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
While working locally on this branch, I encountered a compilation error, despite CI being green. Investigating CI caching...
f82c0a4
to
dd0cb8a
Compare
## Describe your changes Moving the smoke test invocations out of the process-compose wrapper, preferring running raw `cargo test` invocations instead. Doing so ensures that the test output is readable, both locally, and crucially in CI. ### Screenshot before  ### Screenshot after  ## Issue ticket number and link No guiding issue, just trying to improve the testing setup opportunistically. We did have inscrutable smoke test failures on #5063 & #5081, both of which were failures of the new pindexer integration tests (#5057), but it was hard to see the specific failure at a glance, which slowed down development. ## Testing and review Check the CI logs for the smoke test job. Are they readable? Consider running `just smoke` locally and confirm the same. ## 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: > tests-only, no code changes
This was caused by rebase-vs-merge differences in how CI was being executed. After rebasing on top of latest main, I can confirm that everything's building correctly. |
In the absence of testing guidance in the PR submission text, I opted to run So, the question is: should we be making these changes against main? Given that they unblock ongoing DEX-explorer work, that makes sense to me, but we need to be careful about honoring compatibility while we work towards LQT support. Below are some additional logging outputs from my local testing, for reference. pindexer debug logs
pindexer db height
pindexer db height
It might be worthwhile to run this version of |
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
Contains breaking changes that cause pindexer
to fail when run against testnet data. See detailed comment above. Requesting changes to block merge, to avoid breaking indexing pipelines.
Summarizing discussion with @cronokirby out of band: yes, we agree we should do this, and we both expect that testing against mainnet data will succeed. If not, we'll revise the PR accordingly. But assuming we get green on the mainnet reindex, I'll approve this PR, merge it, then we'll need to rebase |
Describe your changes
Issue ticket number and link
Checklist before requesting a review
I have added guiding text to explain how a reviewer should test these changes.
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: