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

services/horizon: update bucketlistdb config sent to core #5295

Closed
2 tasks
mollykarcher opened this issue Apr 26, 2024 · 0 comments
Closed
2 tasks

services/horizon: update bucketlistdb config sent to core #5295

mollykarcher opened this issue Apr 26, 2024 · 0 comments

Comments

@mollykarcher
Copy link
Contributor

mollykarcher commented Apr 26, 2024

What problem does your feature solve?

Core is upgrading bucketlist db from experimental status into default backend status (see blog draft for details). This means that they are going to be removing flag EXPERIMENTAL_BUCKETLIST_DB and replacing it with a new/non-experimental flag. At the time of writing this issue there are a couple different proposed names for this new flag (USE_BUCKETLIST_DB and USE_DEPRECATED_SQL), so confirm with core on the name before implementing.

What would you like to see?

We should to send the new field, and stop sending the old one. This is debatable, but I don't think we should expose a flag for this Horizon-side at all, non-bucketlist will eventually be removed core-side, and bucketlist has been default enabled in captive core for over a year. We've tended to overindex on choice/configurability, and this has proved to only confuse operators, so I'd prefer to limit options going forward. This would mean that we should:

  • Remove Horizon's EXPERIMENTAL_BUCKETLIST_DB configuration option
    • One could argue that we should deprecate this first, but it has "experimental" in the name so I actually think it's okay to just kill it off without warning. Open to differing opinions though
  • When CAPTIVE_CORE_USE_DB=true, send USE_DEPRECATED_SQL=false.
    • We deprecated CAPTIVE_CORE_USE_DB in the 2.30 release, so we'll eventually remove it entirely (but wait for a protocol boundary) and the USE_DEPRECTED_SQL=false will be hardcoded/not configurable
    • Figure out if version-gating, similar to what currently exists for EXPERIMENTAL_BUCKETLIST_DB, is necessary. Core exits when it sees config flags it doesn't recognize. The first stable version of core for p21 will include this new flag, but that won't be available until the testnet vote is successful, so it's possible for people to run core v20.* with 2.30.* of Horizon and see this issue.

The captive core toml creation + default setting is shared between Horizon + RPC, so the same fix should work for RPC but we should be mindful that we'll need to update/release both.

@mollykarcher mollykarcher added this to the platform sprint 47 milestone May 2, 2024
@urvisavla urvisavla self-assigned this Jun 3, 2024
sreuland added a commit that referenced this issue Jun 4, 2024
* Bump core to the stable v21 build.

* Try using 'unsafe-stellar-core' docker image

* Revert "Try using 'unsafe-stellar-core' docker image"

This reverts commit 19ac62f.

* Add DEPRECATED_SQL_LEDGER_STATE=false to (most) of the Core configs

* Try using `EXPERIMENTAL_BUCKETLIST_DB` for compatibility reasons

* Update horizon.yml

force all integration tests to have HORIZON_INTEGRATION_TESTS_CAPTIVE_CORE_USE_DB=true by default

* captive core use_db flag default to true for integration tests

should always be set to true, that flag is deprecated and defaulted to true, there is no test paths validating it being false.

* #5295: dealing with cc use db assumptions in tests

* #5295: fixing captive core use_db flag assumptions in tests

---------

Co-authored-by: shawn <[email protected]>
Co-authored-by: Shawn Reuland <[email protected]>
Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment
Projects
Status: Done
Development

No branches or pull requests

3 participants
@mollykarcher @urvisavla and others