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

Fix migrations check at startup with a read-only connection #1656

Merged
merged 3 commits into from
Feb 15, 2023

Conversation

kind84
Copy link
Contributor

@kind84 kind84 commented Feb 10, 2023

Closes #1653.

Background

The rpc node is supposed to connect to the state database using a read-only connection, in order to not overwhelm the RW connection. This is made possible by the changes introduced in #1531 that allow the rpc node to not run migrations.

What does this PR do?

Unfortunately it turns out that the sql-migrate library requires a RW connection even for tasks that do not actually run the migrations. In our case we were calling migrate.PlanMigration and migrate.GetMigrationRecords but both fail on a RO connection.

To overcome this, here the proposal is to directly query the migrations table and compare the number of migrations found there with the number of Up migrations in our source code (by inspecting list the returned by migrationSource.FindMigrations()).

The solution has been tested to be working on the develop testnet. It may sound a bit "hacky" and thus alternative ideas/approaches are welcome.

Reviewers

Main reviewers:

@kind84 kind84 added the bug Something isn't working label Feb 10, 2023
@kind84 kind84 requested a review from tclemos as a code owner February 10, 2023 16:49
@kind84 kind84 self-assigned this Feb 10, 2023
@cla-bot cla-bot bot added the cla-signed label Feb 10, 2023
Copy link
Member

@arnaubennassar arnaubennassar left a comment

Choose a reason for hiding this comment

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

LGTM, but I think that this check is only done for the StateDB, it would be nice to do it for the PoolDB as well

EDIT: you can create issue and do this in a future PR if you prefere

@ARR552
Copy link
Contributor

ARR552 commented Feb 13, 2023

LGTM, but I think that this check is only done for the StateDB, it would be nice to do it for the PoolDB as well

EDIT: you can create issue and do this in a future PR if you prefere

@arnaubennassar As far as I know we can not have the pool db in readonly mode because the rpc needs to store new transactions on the pool db. Maybe we need to allow the rpc to handle read and write replicas. For instance, if the request is for getting if the gas price, then it should use a read replica, if the method is eth_sendRawTransaction, then it should be able to write into the pool db

@kind84
Copy link
Contributor Author

kind84 commented Feb 15, 2023

@ARR552 I tested this on develop testnet against all the 3 possible scenarios:

  • Clean start from an empty state DB
  • Start coming from a previous version, with some migrations to apply
  • Start from an up to date version with all migrations already applied

I didn't face any issue with this implementation.

In addition, I added a log message to inform that the check has been successful b4aebed

WDYT?

@ToniRamirezM ToniRamirezM merged commit 0a8cb65 into release/v0.0.2 Feb 15, 2023
@ToniRamirezM ToniRamirezM deleted the fix/1653-check-migrations branch February 15, 2023 13:08
kind84 added a commit that referenced this pull request Feb 21, 2023
Closes #1653

This PR brings to `develop` the changes from #1656
Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment
Labels
bug Something isn't working cla-signed
Projects
None yet
Development

Successfully merging this pull request may close these issues.

None yet

5 participants