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

Rework event logs and transaction database schema #1961

Merged
merged 22 commits into from
Jun 25, 2024
Merged

Conversation

tudor-malene
Copy link
Collaborator

@tudor-malene tudor-malene commented Jun 11, 2024

Why this change is needed

Event logs must be queried efficiently from the enclave, so we need to store them in normalised db tables, which are easy to index.

The normalisation has a knock-on effect, requiring some clarification of responsibilities and refactoring of transaction storage.

Transactions were stored twice in the database, loaded into memory and cached even though they were only needed during execution.

What changes were made as part of this PR

  • normalise the "event_logs" table
  • move logic from the database layer to the storage layer while keeping the database layer very simple and with a single responsibility
  • cache the Batch Header only, and modify the usages ( this is the bulk of the changes)
  • remove the batch_body table, which was really just the "batch_height"
  • read the transactions from the transactions table
  • when executing a batch, register a "contract change" hook, and based on that return what contracts are created.
  • unrelated to the logic, but required for debugging: upgrade the github actions

PR checks pre-merging

Please indicate below by ticking the checkbox that you have read and performed the required
PR checks

  • PR checks reviewed and performed

@tudor-malene tudor-malene marked this pull request as ready for review June 14, 2024 11:54
@tudor-malene tudor-malene changed the title WIP rework event logs Rework event logs and transaction database schema Jun 14, 2024
Copy link
Collaborator

@BedrockSquirrel BedrockSquirrel left a comment

Choose a reason for hiding this comment

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

LGTM so far. Went through over the phone with Tudor too and the table structure etc. makes sense. Left a few minor comments and I'll scan through your new changes once you've dealt with the todos you added.

Copy link
Collaborator

@BedrockSquirrel BedrockSquirrel left a comment

Choose a reason for hiding this comment

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

LGTM except for the tenscan contract count changing from 2 to 7 as discussed (the +1/+2 adjustments not quite right), and the segfault you mentioned

Copy link
Collaborator

@BedrockSquirrel BedrockSquirrel left a comment

Choose a reason for hiding this comment

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

LGTM - looking forward to having this one in!

@tudor-malene tudor-malene merged commit e1c9124 into main Jun 25, 2024
2 checks passed
@tudor-malene tudor-malene deleted the tudor/getlogs_perf branch June 25, 2024 09:24
Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment
Labels
None yet
Projects
None yet
Development

Successfully merging this pull request may close these issues.

3 participants