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

Require BucketsDB flag in config file #4202

Closed
Tracked by #3811
SirTyson opened this issue Feb 21, 2024 · 5 comments · Fixed by #4277
Closed
Tracked by #3811

Require BucketsDB flag in config file #4202

SirTyson opened this issue Feb 21, 2024 · 5 comments · Fixed by #4277
Assignees

Comments

@SirTyson
Copy link
Contributor

SirTyson commented Feb 21, 2024

To start the transition to BucketListDB, we should change the EXPERIMENTAL_BUCKETLIST_DB flag to a required, non-experimental flag USE_BUCKETLIST_DB. If this flag is not set in the config, we should crash on startup. We should crash instead of defaulting to USE_BUCKETLIST_DB = true because migrating from SQL to BucketList drops most of the SQL tables. If a operator is relying on Core's SQL database, this automatic migration could be detrimental. Instead, we should strongly persuade validators to set USE_BUCKETLIST_DB = true and print deprecation warning if USE_BUCKETLIST_DB = false, but should not automatically migrate.

@SirTyson
Copy link
Contributor Author

When USE_BUCKETLIST_DB = true we should also no longer maintain TX meta.

@mollykarcher
Copy link

mollykarcher commented Apr 26, 2024

Adding a new, mandatory configurable variable with no default value is a breaking change. If this is going to be going into v21.0.0rc2, then this is problematic because p21-compatible versions of both Horizon and RPC have already been released which do not send this flag. We could issue patches that do so, but this would mean that there are versions out there in the wild (which we know some partners have already installed) that will not be compatible with the first p21 stable release of core.

Could we do any of the following to avoid this:

  • Make this flag optional at first
  • Default it to true only for captive core / non-validators
  • Derive it based on existing systems that are currently still sending EXPERIMENTAL_BUCKETLIST_DB as I believe Horizon/RPC do today

@SirTyson
Copy link
Contributor Author

We do need this flag to be required. The issue is the initial migration to BucketListDB drops many tables that are very expensive to recreate. Some devs in the ecosystem still rely on these tables, and we're afraid if we have some sort of default flag, operators will upgrade their package and wipe their tables before noticing, so we want to have a required flag that crashes core before dropping the tables if not set.

We might be able to strike a middle ground. How about in 21.0, we make it such that either EXPERIMENTAL_BUCKETLIST_DB=true or the new USE_DEPRECATED_SQL flag is required? That way we can still enforce that the flag is required for validator operators (which is the primary goal) but not break horizon/RPC?

@mollykarcher
Copy link

How about in 21.0, we make it such that either EXPERIMENTAL_BUCKETLIST_DB=true or the new USE_DEPRECATED_SQL flag is required?

🙏 Indeed, this would be great. We can update Horizon/RPC to send the new flag in our next releases

@SirTyson
Copy link
Contributor Author

Great, we'll go this route then.

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 a pull request may close this issue.

3 participants
@mollykarcher @SirTyson and others