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

feat(en): Fix operator address assignment for ENs #688

Merged
merged 27 commits into from
Jan 31, 2024

Conversation

slowli
Copy link
Contributor

@slowli slowli commented Dec 14, 2023

What ❔

  • Moves fee_account_address from the l1_batches table to the miniblocks table using a combined SQL and programmatic migration.
  • Replaces uses of l1_gas_price, l2_fair_gas_price and base_fee_per_gas from l1_batches to the corresponding miniblocks fields. Removes uses of l1_batches.is_finished.
  • Updates L1BatchHeader / MiniblockHeader correspondingly.

Why ❔

Currently, operator address is not persisted for miniblocks, but only for L1 batches. This has several negative implications:

  • ENs cannot be used to synchronize other ENs since SyncBlock.operator_address they return may be bogus.
  • We have nowhere to get the real operator address for gossip-based syncing as well.

Checklist

  • PR title corresponds to the body of PR (we generate changelog entries from PRs).
  • Tests for the changes have been added / updated.
  • Documentation comments have been added / updated.
  • Code has been formatted via zk fmt and zk lint.
  • Spellcheck has been run via cargo spellcheck --cfg=./spellcheck/era.cfg --code 1.

Copy link
Contributor Author

@slowli slowli left a comment

Choose a reason for hiding this comment

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

The overall migration strategy is as follows:

  1. Perform SQL migration changing the DB schema. It should be fast and shouldn't disrupt system operation.
  2. Perform programmatic data migration (see fee_address_migration module). This migration is run in the background in the state keeper / EN pod. It relies that step 1 is complete (i.e., data schema is changed).
  3. (Will be implemented separately) Perform another SQL migration removing obsolete L1 batch columns.

DB queries are rewritten in a way that they will continue to work after the obsolete L1 batch columns are removed. (The natural exception is migration-specific queries, but they should never run at this point.) I.e., code deployed on step 2 should continue working both before and after completing step 3.

@slowli slowli marked this pull request as ready for review December 20, 2023 12:01
@Artemka374 Artemka374 mentioned this pull request Jan 14, 2024
5 tasks
perekopskiy
perekopskiy previously approved these changes Jan 23, 2024
Copy link
Contributor

@perekopskiy perekopskiy left a comment

Choose a reason for hiding this comment

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

Looks good! Sorry for the long review. Please resolve conflicts and I will reapprove

perekopskiy
perekopskiy previously approved these changes Jan 24, 2024
RomanBrodetski
RomanBrodetski previously approved these changes Jan 26, 2024
Copy link
Collaborator

@RomanBrodetski RomanBrodetski left a comment

Choose a reason for hiding this comment

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

Looks good! Surprized it's that involved though. I don't see how exactly we could do it simpler. But it's a good change and we need experience in migrating large tables.

@slowli slowli added this pull request to the merge queue Jan 31, 2024
@github-merge-queue github-merge-queue bot removed this pull request from the merge queue due to failed status checks Jan 31, 2024
@slowli slowli added this pull request to the merge queue Jan 31, 2024
Merged via the queue into main with commit 086ff56 Jan 31, 2024
37 checks passed
@slowli slowli deleted the aov-pla-674-fix-operator-address-assignment-for-ens-take2 branch January 31, 2024 12:00
RomanBrodetski added a commit that referenced this pull request Jan 31, 2024
@slowli slowli mentioned this pull request Apr 10, 2024
6 tasks
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