Ab/sqlite #2284

imabdulbasit · 2024-11-14T17:23:49Z

This PR adds support for sqlite

Updates the hotshot-query-service which includes the sqlite support.
Adds separate migrations for PostgreSQL and SQLite.
Some data types in the state table columns have been updated to JSONB, and a separate migration updates those data types. This was necessary because SQLite does not support BYTEA or arrays, so a BLOB column is used instead. This change unifies the queries across databases.
Introduces a feature flag, embedded-db, which activates SQLite (embedded-db). If not provided, postgres is used by default.
Updates workflows to include a job for testing PostgreSQL, as all-features includes embedded-db and tests with SQLite.
-Updates the just recipes to enable local testing with both SQLite and PostgreSQL.

Note: SQLite sometimes lacks support for basic SQL syntax that one might expect, such as ORDER BY (col1, col2...) DESC. Instead, we use ORDER BY col1 DESC, col2 DESC to ensure compatibility with both PostgreSQL and SQLite.

.github/workflows/slowtest.yaml

jbearer · 2024-11-18T20:35:31Z

process-compose-sqlite.yaml

+      exit_on_skipped: true
+
+  sequencer2:
+    command: sequencer -- http -- catchup -- status


So these are all running the same binary with sqlite storage. Ideally, we would just have one demo, and we would run the query nodes with postgres and the other nodes with sqlite, as we will do in production.

The only way I can think of to get these binaries built at the same time with different feature flags is to move sequencer/main.rs into two separate crates. So we can have sequencer-sqlite/Cargo.toml and sequencer-postgres/Cargo.toml, each of which import sequencer with different feature flags. We can move most of the logic from main.rs into the sequencer library crate so it isn't duplicated.

This is a bit ugly, maybe there's a better way, but the point is we need to find out how to get two simultaneous executables with different names and different feature flags, so that we can use them at the same time in a demo, and also so that we can build a sequencer docker image that includes both of these binaries (this I think will be the easiest way for us to ship this to node operators)

The Cargo features are additive, so the embedded-db feature is automatically enabled for sequencer-postgres when running cargo build.

rust-lang/cargo#674 (comment)

Two options come to mind: use the cfg directive instead of a feature, or remove these two crates from the workspace or maybe only the sqlite crate

sequencer/api/migrations/postgres/V43__update_state_tables_types.sql

sequencer/api/migrations/sqlite/V31__drop_merklized_state_created_index.sql

sequencer/src/persistence/sql.rs

…ovider()

sveitser · 2024-11-19T11:50:32Z

sequencer/Cargo.toml

 marketplace-solver = { path = "../marketplace-solver" }
 num_enum = "0.7"
 portpicker = { workspace = true }
 rand = { workspace = true }
 rand_chacha = { workspace = true }
 rand_distr = { workspace = true }
-sequencer-utils = { path = "../utils" }
+sequencer-utils = { path = "../utils", features = ["testing"] }


Can we do without the "testing" feature in the build dependencies?

sveitser · 2024-11-19T11:53:45Z

justfile

    cargo clippy --workspace --all-features --all-targets -- -D warnings
+    @echo 'features: "testing"'
+    cargo clippy --workspace --all-features --features testing -- -D warnings


this has all-features and testing

sveitser · 2024-11-19T11:54:34Z

justfile

+    @echo 'features: "all"'
+    cargo nextest run --locked --workspace --all-features --verbose {{args}}
+    @echo 'features: "testing"'
+    cargo nextest run --locked --workspace --features testing --verbose {{args}}


I don't think we need to explicitly activate the testing feature to compile the tests. It should be activated via dev dependencies.

.github/workflows/test.yml

tbro · 2024-11-27T15:29:03Z

justfile

+    @echo 'Omitting slow tests. Use `test-slow` for those. Or `test-all` for all tests.'
+    @echo 'features: "all"'
+    cargo nextest run --locked --workspace --all-features --verbose {{args}}
+    cargo nextest run --locked --workspace --verbose {{args}}


Why are we running all the tests twice now?

one uses sqlite for tests using database and the other uses postgres

tbro · 2024-11-27T15:34:13Z

.github/workflows/build.yml


      - name: Build Espresso Dev Node
        # Espresso Dev Node currently requires testing feature, so it is built separately.
        run: |
-          cargo build --locked --release --features testing --bin espresso-dev-node
+          cargo build --locked --release --features "testing embedded-db" --bin espresso-dev-node


Can we explicitly specify the embedded-db features everywhere? I think in other places we are just specifying --all-features which isn't clear. Especially since we were using --all-features previously (I think).

tbro · 2024-11-27T15:36:19Z

.github/workflows/lint.yml

@@ -32,5 +32,8 @@ jobs:
      - name: Format Check
        run: cargo fmt -- --check

-      - name: Check
+      - name: Check (embedded-db)


Confused here too. Won't --all-features + --all-targets lint everything? If so, why run clippy a second time?

all-features would enable the "embedded-db" feature and so this would disable any postgres code. This is because we have #[cfg(not(feature = "embedded-db")) for the postgres code. We are doing lint check second time to see that postgres lint doesn't have any error. So one clippy run would check that sqlite lint and other one checks the postgres. The features here don't add the functionality but rather deactivate one and activate the other.

Humm is this not addressed by having sqlite in a separate package? cargo lint --workspace would hit both the package w/ embedded-db and the other sequencer binary that does not enable that feature.

its not the binary we are checking. We are checking the sequencer library with embedded-db code activated and postgres disabled and vice versa

jbearer

Code looks good, some comments about configuration.

In terms of releasing, unfortunately we are a bit backed up atm due to recent patches regarding the L1 client. Ideally I think we should wait for the current weekly release to merge, then merge this right after. That way we can cut a release branch which includes the fixes for the L1 client but excludes sqlite, so we can QA and deploy one big change at a time. This would then go into the following release.

docker-compose.yaml

justfile

process-compose.yaml

sequencer/src/persistence/sql.rs

tbro

I'm still looking over some things, but since this is already approved and tests are passing I'm approving also. We can work out details in future PRs.

* Move Marketplace to higher version and create new v0_3 placeholder * test fixes * change number in test.yaml * demo-native-mp should be running w/ test profile * fixes * mark error * fix v3 serialization * uncomment base and upgrade versions * Fix versions in upgrade tests The tests still hang: block hight is stuck when upgrade happens around view 110. * update epoch version to v100 * Ab/sqlite (#2284) * update query-service to ab/sqlite-support * .. * use embedded-db for devnode * uncomment marketplace builder tests * point query-service to ab/sqlite-support * migrations to update state tables columns * update comments for sql options * update justfile * enable embedded-db for devnode * fix record action query for postgres * fix record action query for postgres * try with only testing feature * rm settings.json * lint * add process-compose for sqlite, fix migration, update query-service * update query-service * pass persistence to init_node(), fix migrations, rm create_catchup_provider() * fix migration version * fix migration version * fix migration * fix migration * sequencer-sqlite * update sequencer-sqlite lockfile * fix archive name * fix archive name * update name * fix test_integration ci * fix sequencer-sqlite dockerfile * try with disabling aggregator * fix sqlite migration for merkle root columns && enable restart_tests * enable aggregator * set ESPRESSO_STATE_SIGNATURE_TOTAL_STAKE to 7 * fix merkel_root_columns && use same persistence in test_restart * set connection parameters, revert tx delay to 2s, update query-service * comment out max connections setting * set max_connections for tmp db options * add sqlite nodes in docker-compose * enable embedded-db feature for devnode * merge origin/main * nix: fix process-compose demo - Add `test` profile to sequencer-sqlite crate to avoid compiling everything twice. - Rewrite just build / native demo commands to to work with the nix dev env. * ci: compile diff-test with test profile We previously changed the flake.nix to add `./target/nix/debug` to the PATH instead of `release`. * remove docker pull from integration test * comment out cleaning up tempdir in demo-native * update hotshot-query-service * ci: don't clean up process compose storage dir * ci: upload demo native process compose logs * ci: dump logs to terminal instead - reduce timeout: the whole job takes 5 minutes on main now. - remove `process compose down`: I don't think the CI minds if we don't run it and it will save some time. * ci: dump logs of process compose services * script for running sequencer binaries based on flag * increase blocks timeout to 180s, update query-service * adjust sequencer_blocks_timeout * try with timeout=25 * revert timeout * try2 * update query-service * max connections as env * wait 5s * update query-service to 0.1.74 * logging * cargo sort * more logging * catchup from sequencer0 * catchup from seq0 * catchup from seq5 * exit if any one of the node has seen the upgrade * catchup from seq0 * remove test from archive in integration test * Upload logs as archive For me the logs are always truncated if I download them from the browser. * integration test: more debug output * use archive * set max connections to 3 for sqlite * try with 5nodes * sequencer_clients * slow statement threshold * fix state peer * use embedded-db instead of all features * lint * make seq0 state peer for seq3 * add api peers for seq3 and seq4 * try with only 1 sqlite node * remove seq3 and 4 from clients * update docker-compose * catchup from seq4, set slow_statement_threshold to 1s * pass storage-sql in the sequencer-entrypoint for sqlite * use sqlite sub dir * create sqlite sub-dir if it does not exist * create_dir_all * set default slow threshold * fix dev node test * fix dockerfile --------- Co-authored-by: sveitser <[email protected]> * trigger CI * Cargo.lock * sqlite lock file * fix leaf conversion in collect_garbage() and migrate_quorum_proposal_leaf_hashes * update marketplace demo toml * Add nextest override for flaky tests - Kill tests after 6 minutes, the slowest tests in the default set take about 2 minutes. * nextest: give flaky test more time * update comment * fix marketplace version in demo toml --------- Co-authored-by: tbro <[email protected]> Co-authored-by: imabdulbasit <[email protected]> Co-authored-by: Abdul Basit <[email protected]>

imabdulbasit added 9 commits November 12, 2024 00:54

update query-service to ab/sqlite-support

94d642d

..

337469c

use embedded-db for devnode

3d8cb3e

uncomment marketplace builder tests

c345591

point query-service to ab/sqlite-support

7a8b113

migrations to update state tables columns

8aa2ed3

update comments for sql options

164c99a

update justfile

9dde077

enable embedded-db for devnode

16fc211

imabdulbasit force-pushed the ab/sqlite branch from 74510d4 to 16fc211 Compare November 14, 2024 17:41

imabdulbasit added 7 commits November 14, 2024 22:57

fix record action query for postgres

427c33f

fix record action query for postgres

a97bd18

try with only testing feature

ff44a1a

Merge remote-tracking branch 'origin/main' into ab/sqlite

11a899d

rm settings.json

6cc9acb

lint

78b5abd

add process-compose for sqlite, fix migration, update query-service

0d52d7f

imabdulbasit force-pushed the ab/sqlite branch from ae9c7e8 to 0d52d7f Compare November 17, 2024 23:23

imabdulbasit added 2 commits November 18, 2024 04:27

Merge remote-tracking branch 'origin/main' into ab/sqlite

bbcce8d

update query-service

0d63fbf

jbearer reviewed Nov 18, 2024

View reviewed changes

imabdulbasit added 5 commits November 19, 2024 04:26

pass persistence to init_node(), fix migrations, rm create_catchup_pr…

a6dbd91

…ovider()

Merge remote-tracking branch 'origin/main' into ab/sqlite

00db3f9

fix migration version

425320f

fix migration version

c8937e2

fix migration

da75a12

sveitser reviewed Nov 19, 2024

View reviewed changes

fix migration

993aba6

tbro reviewed Nov 27, 2024

View reviewed changes

.github/workflows/test.yml Show resolved Hide resolved

tbro reviewed Nov 27, 2024

View reviewed changes

imabdulbasit added 4 commits December 1, 2024 03:36

use embedded-db instead of all features

09e0878

Merge remote-tracking branch 'origin/main' into ab/sqlite

16ace39

lint

6054b29

make seq0 state peer for seq3

6e9d79c

imabdulbasit force-pushed the ab/sqlite branch from c628e3c to 6e9d79c Compare December 1, 2024 00:13

imabdulbasit added 3 commits December 1, 2024 05:38

add api peers for seq3 and seq4

452474b

try with only 1 sqlite node

80c8637

remove seq3 and 4 from clients

0574057

imabdulbasit marked this pull request as ready for review December 2, 2024 07:59

update docker-compose

7487faa

jbearer reviewed Dec 2, 2024

View reviewed changes

docker-compose.yaml Outdated Show resolved Hide resolved

justfile Outdated Show resolved Hide resolved

process-compose.yaml Outdated Show resolved Hide resolved

process-compose.yaml Outdated Show resolved Hide resolved

sequencer/src/persistence/sql.rs Outdated Show resolved Hide resolved

imabdulbasit added 8 commits December 2, 2024 23:13

catchup from seq4, set slow_statement_threshold to 1s

0040cb0

Merge remote-tracking branch 'origin/main' into ab/sqlite

6d4e87c

pass storage-sql in the sequencer-entrypoint for sqlite

b627615

use sqlite sub dir

ef72571

create sqlite sub-dir if it does not exist

b817446

create_dir_all

72544fe

set default slow threshold

d43f9ad

fix dev node test

3c6da8f

imabdulbasit mentioned this pull request Dec 4, 2024

Revert SQLite changes EspressoSystems/hotshot-query-service#747

Closed

tbro approved these changes Dec 4, 2024

View reviewed changes

imabdulbasit and others added 2 commits December 4, 2024 23:18

fix dockerfile

90b8276

Merge branch 'main' into ab/sqlite

9fee97a

imabdulbasit merged commit d6a7a18 into main Dec 4, 2024
24 checks passed

imabdulbasit deleted the ab/sqlite branch December 4, 2024 19:04

Provide feedback

Saved searches

Use saved searches to filter your results more quickly

Ab/sqlite #2284

Ab/sqlite #2284

imabdulbasit commented Nov 14, 2024

jbearer Nov 18, 2024

imabdulbasit Nov 19, 2024

imabdulbasit Nov 19, 2024 •

edited

Loading

sveitser Nov 19, 2024

sveitser Nov 19, 2024

sveitser Nov 19, 2024 •

edited

Loading

tbro Nov 27, 2024

imabdulbasit Dec 2, 2024

tbro Nov 27, 2024

imabdulbasit Dec 2, 2024

tbro Nov 27, 2024

imabdulbasit Nov 30, 2024

tbro Dec 4, 2024

imabdulbasit Dec 4, 2024

jbearer left a comment

tbro left a comment

Ab/sqlite #2284

Ab/sqlite #2284

Conversation

imabdulbasit commented Nov 14, 2024

Choose a reason for hiding this comment

Choose a reason for hiding this comment

imabdulbasit Nov 19, 2024 • edited Loading

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

sveitser Nov 19, 2024 • edited Loading

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

jbearer left a comment

Choose a reason for hiding this comment

tbro left a comment

Choose a reason for hiding this comment

imabdulbasit Nov 19, 2024 •

edited

Loading

sveitser Nov 19, 2024 •

edited

Loading