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.

For me the logs are always truncated if I download them from the browser.

.github/workflows/test.yml

sveitser · 2024-11-26T14:44:00Z

.github/workflows/test.yml

          --workspace-remap $PWD $(if [ "${{ matrix.version }}" == "2" ]; then echo " smoke"; fi)
-        timeout-minutes: 40
+        timeout-minutes: 20


Based on the fact that this seems to fail because consensus gets stuck due to the upgrade that happens usually before 120 blocks. Shouldn't we fail this earlier instead of waiting for 20 minutes?

block: height=114, version=0.2 block: height=114, version=0.2 block: height=115, version=0.2 block: height=115, version=0.2 TRY 1 SLOW [>240.000s] tests::integration upgrades::test_upgrade block: height=115, version=0.2 block: height=115, version=0.2 TRY 1 SLOW [>360.000s] tests::integration upgrades::test_upgrade TRY 1 SLOW [>480.000s] tests::integration upgrades::test_upgrade ...

tbro · 2024-11-27T15:23:23Z

.github/workflows/test.yml

+          cache-all-crates: true
+          cache-provider: buildjet
+
+      - name: Build and archive tests


I think we are just building the same test binaries twice and giving them two different names. This doesn't appear to select sqlite tests, it just gives it an sqlite name. If so, we should just do the build once.

I may have answered my own question elsewhere, but if we are using --all-features in order to specify embedded-tb can we not specify feature embedded-db explicitly?

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?

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?

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

imabdulbasit added 4 commits November 26, 2024 01:54

cargo sort

897563f

more logging

f76f668

catchup from sequencer0

c5ce4cc

catchup from seq0

0a73fe8

imabdulbasit force-pushed the ab/sqlite branch from 312beba to 0a73fe8 Compare November 25, 2024 23:13

imabdulbasit added 2 commits November 26, 2024 04:26

catchup from seq5

3472669

exit if any one of the node has seen the upgrade

9aa9c4a

imabdulbasit force-pushed the ab/sqlite branch from 2927f8c to 9aa9c4a Compare November 26, 2024 00:28

catchup from seq0

975d8ee

imabdulbasit marked this pull request as draft November 26, 2024 01:29

imabdulbasit force-pushed the ab/sqlite branch from 4c2edb0 to a867fe2 Compare November 26, 2024 01:43

remove test from archive in integration test

4f28a44

imabdulbasit force-pushed the ab/sqlite branch from 7aadee5 to 4f28a44 Compare November 26, 2024 05:15

sveitser added 2 commits November 26, 2024 10:50

Upload logs as archive

bda2921

For me the logs are always truncated if I download them from the browser.

integration test: more debug output

dbbe97e

sveitser reviewed Nov 26, 2024

View reviewed changes

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

imabdulbasit added 5 commits November 26, 2024 16:02

use archive

6bbd9e8

set max connections to 3 for sqlite

521ce89

try with 5nodes

6f3479d

sequencer_clients

c181e35

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

a4577f1

sveitser reviewed Nov 26, 2024

View reviewed changes

imabdulbasit added 2 commits November 26, 2024 20:39

slow statement threshold

d89a669

fix state peer

4635c02

imabdulbasit force-pushed the ab/sqlite branch from 9201f50 to 4635c02 Compare November 26, 2024 23:31

disable aggregator

7fc657e

tbro reviewed Nov 27, 2024

View reviewed changes

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

sveitser Nov 26, 2024

tbro Nov 27, 2024

tbro Nov 27, 2024

tbro Nov 27, 2024

tbro Nov 27, 2024

tbro Nov 27, 2024

Ab/sqlite #2284

Are you sure you want to change the base?

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

imabdulbasit Nov 19, 2024 •

edited

Loading

sveitser Nov 19, 2024 •

edited

Loading