Wallet: move database to sqlite, remove kv and memmory db #804

jaoleal · 2026-01-24T21:13:02Z

Description and Notes

Addressing #665, I implemented the existing AddressCacheDatabase trait on top of rusqlite, the only feature that the changes present is "bundled" which they declared to be better when regarding windows support.

Regarding the implementation, it is pretty straightforward, I had the other two databases, memmory_database.rs and kv_database.rs to be inspired. I think the tests are extensive and ensure the implementation runs fine, for every commit I made sure for just lint-features and just test-feature to also run fine.

How to verify the changes you have done?

Review tests under sqlite_database.rs and run them.

Since this present a lot of features changes, running just lint-features and just test-feature is good.

Contributor Checklist

I've followed the contribution guidelines
[] I've verified one of the following:
- Ran just pcc (recommended but slower)
- Ran just lint-features '-- -D warnings' && cargo test --release
- Confirmed CI passed on my fork
I've linked any related issue(s) in the sections above

Finally, you are encouraged to sign all your commits (it proves authorship and guards against tampering—see How (and why) to sign Git commits and GitHub's guide to signing commits).

GustavoStingelin · 2026-01-24T21:16:35Z

crates/floresta-watch-only/src/sqlite_database.rs

+    fn init_tables(&self) -> Result<()> {
+        self.conn.lock()?.execute_batch(
+            "
+            CREATE TABLE IF NOT EXISTS addresses (
+                script_hash TEXT PRIMARY KEY,
+                data BLOB NOT NULL
+            );
+
+            CREATE TABLE IF NOT EXISTS transactions (
+                txid BLOB PRIMARY KEY,
+                data BLOB NOT NULL
+            );
+
+            CREATE TABLE IF NOT EXISTS descriptors (
+                id INTEGER PRIMARY KEY AUTOINCREMENT,
+                descriptor TEXT NOT NULL UNIQUE
+            );
+
+            CREATE TABLE IF NOT EXISTS metadata (
+                key TEXT PRIMARY KEY,
+                value BLOB NOT NULL
+            );
+            ",
+        )?;
+        Ok(())
+    }


This still looks like a KVDB, but in SQLite, what's the benefit?

I wanted for this PR to be simple to review and merged.

For sure further optimizations taking more advantage of the greatness of SQL can be proposed, but I want for this to be the first step.

@GustavoStingelin we want to get rid of the kv dependency.

jaoleal · 2026-01-24T22:18:22Z

a5e86c3: Im seeing some build related problems, it appears that "bundled" isnt enough, ill investigate further.

jaoleal · 2026-01-25T00:37:51Z

37e383c: Okay, this commit fixed the build problem i was having.

Basically what happened is that they were bundling pre-generated bindings and that was triggering a version compatibility problem basicaly because rust doesnt want to have a stable ABI, it can change trough versions affecting bindings and yada yada.

They took that decision to avoid bigger compile times but that doesnt appear to be a big problem, well... To solve that i activated the feature that should generate the bindings on the go and it worked, otherwise i would need to find a rusqlite version that was bundling the correct bindings for our MSRV and pin it, and im not sure if that would support our case.

If anyone want to further investigate to help me being sure that was the correct decision:

Davidson-Souza

Generally looks good. Minor comments

I think a5e86c3 and 37e383c can be squashed onto 9cbc9f1.

crates/floresta-watch-only/Cargo.toml

crates/floresta-watch-only/src/sqlite_database.rs

Davidson-Souza · 2026-01-26T15:53:54Z

crates/floresta-watch-only/src/sqlite_database.rs

+        let value = serde_json::to_vec(&address).expect("Invalid object serialization");
+
+        self.conn
+            .execute(
+                "INSERT OR REPLACE INTO addresses (script_hash, data) VALUES (?1, ?2)",
+                params![key, value],
+            )
+            .expect("Fatal: Database isn't working");


9cbc9f1
So, you plan to make this more sql-like in a follow-up (#804 (comment))? Because we definitely should stop using json here. But I understand if the goal is to do this in a follow-up

I was not planning to be the one to optimize the tables, because this was the very first time that I actually did something with SQL besides hating it in exams lol.

But I can totally do that, ill just need some more time studying the use of SQL, im also writing the PR for our wallet to be descriptor based with SQL in mind.

Regarding the use of json, I really thought of that being a problem because of jsons bloat but it was simple enough for a first implementation.

After searching solutions besides using json, ill prefer to follow an approach using BLOBS for rust-bitcoin types and mapping primitives for other data.

I can already get rid of serde_json for watch-only in this PR without much overhead, but theres room to upgrade, we can optimize types and tables so they can talk better, and I plan to do that in the Descriptor Based PR.

I know that schwab proposed for us to migrate to BDK but i see that change being more radical and having more concerns than what I want to do with the descriptor based PR. I feel that this will be something more for the future, what im trying to do now is just making easier to handle descriptors around in floresta.

I know that schwab proposed for us to migrate to BDK but i see that change being more radical and having more concerns than what I want to do with the descriptor based PR.

Yes, migrating to BDK is something for the future as it still needs some work upstream such that it plays nicely with Electrum's transaction Merkle proofs.

Davidson-Souza · 2026-01-26T15:55:15Z

crates/floresta-watch-only/src/sqlite_database.rs

+        let value = serde_json::to_vec(&address).expect("Invalid object serialization");
+
+        self.conn
+            .execute(
+                "INSERT OR REPLACE INTO addresses (script_hash, data) VALUES (?1, ?2)",
+                params![key, value],
+            )
+            .expect("Fatal: Database isn't working");


9cbc9f1

Propagate error here

#804 (comment)

It can return a Result<(), SqliteError>, so it can return an error and therefore panic. You should propagate the error upwards.

crates/floresta-watch-only/src/sqlite_database.rs

crates/floresta-watch-only/Cargo.toml

jaoleal · 2026-01-28T12:54:49Z

Notable changes on 57695dc

Rebased
No more json cast, removed serde_json.
Tables now looks more like the types we are storing, fields with rust-bitcoin types I used the Blob type with the consensus encoding, fields with primitives I mapped to equivalent SQL primitives.
Removed all unwraps(where possible), better error handling in general.
Cast to types internally on maps to save iterations.

crates/floresta-watch-only/src/sqlite_database.rs

jaoleal · 2026-01-29T11:59:41Z

Done @Davidson-Souza @luisschwab

Davidson-Souza

Your commits are in reverse chronological order. What happened?

Davidson-Souza · 2026-01-29T16:28:58Z

crates/floresta-watch-only/src/lib.rs

-        self.database.save(&new_address);
+        self.database
+            .save(&new_address)
+            .expect("Database not working");


Errors that can happens should either be handled or propagated

again, that refers to another method that does not return a result and IMHO changing this method would be out-of-scope for this pr.

crates/floresta-watch-only/src/sqlite_database.rs

jaoleal · 2026-02-02T18:53:30Z

Your commits are in reverse chronological order. What happened?

Yeah, i reversed the commits because its easier to apply suggestions on the latest commit instead of the first... The old order was erroring while editing the first commit because of feature flags and deprecation changes

jaoleal · 2026-02-02T22:48:17Z

Applied @Davidson-Souza suggestions

jaoleal · 2026-02-02T23:35:36Z

Blank push -f, for some reason the CI just DIED

GustavoStingelin reviewed Jan 24, 2026

View reviewed changes

Davidson-Souza added dependencies Pull requests that update a dependency file code quality Generally improves code readability and maintainability labels Jan 24, 2026

jaoleal force-pushed the wallet_sqlite_descbased branch from 90f9da2 to a5e86c3 Compare January 24, 2026 22:11

jaoleal force-pushed the wallet_sqlite_descbased branch from ba07354 to 14231f8 Compare January 25, 2026 00:12

Davidson-Souza requested changes Jan 26, 2026

View reviewed changes

Davidson-Souza reviewed Jan 26, 2026

View reviewed changes

crates/floresta-watch-only/Cargo.toml Outdated Show resolved Hide resolved

jaoleal force-pushed the wallet_sqlite_descbased branch 2 times, most recently from 6f0f109 to 05134fe Compare January 28, 2026 12:50

jaoleal force-pushed the wallet_sqlite_descbased branch 2 times, most recently from 4252dbd to 090fe9d Compare January 28, 2026 13:35

luisschwab reviewed Jan 28, 2026

View reviewed changes

crates/floresta-watch-only/src/sqlite_database.rs Show resolved Hide resolved

luisschwab assigned jaoleal Jan 28, 2026

jaoleal force-pushed the wallet_sqlite_descbased branch from 090fe9d to c8199b4 Compare January 28, 2026 22:37

jaoleal added 2 commits January 28, 2026 19:38

chore(wallet): deprecate kv database

120fffb

chore(wallet): deprecate memory database

2554a68

jaoleal force-pushed the wallet_sqlite_descbased branch from c8199b4 to 47bd5c7 Compare January 28, 2026 22:43

Davidson-Souza requested changes Jan 29, 2026

View reviewed changes

jaoleal closed this Feb 2, 2026

jaoleal reopened this Feb 2, 2026

jaoleal force-pushed the wallet_sqlite_descbased branch from 47bd5c7 to cfaa2ab Compare February 2, 2026 22:47

feat(wallet): SQLite database.

e358b7f

jaoleal force-pushed the wallet_sqlite_descbased branch from cfaa2ab to e358b7f Compare February 2, 2026 23:35

Wallet: move database to sqlite, remove kv and memmory db #804

Are you sure you want to change the base?

Wallet: move database to sqlite, remove kv and memmory db #804

Uh oh!

Conversation

jaoleal commented Jan 24, 2026

Description and Notes

How to verify the changes you have done?

Contributor Checklist

Uh oh!

Choose a reason for hiding this comment

Uh oh!

jaoleal Jan 24, 2026 • edited Loading Uh oh! There was an error while loading. Please reload this page.

Uh oh!

Choose a reason for hiding this comment

Uh oh!

Choose a reason for hiding this comment

Uh oh!

Choose a reason for hiding this comment

Uh oh!

jaoleal commented Jan 24, 2026

Uh oh!

jaoleal commented Jan 25, 2026

Uh oh!

Davidson-Souza left a comment

Choose a reason for hiding this comment

Uh oh!

Uh oh!

Uh oh!

Uh oh!

Uh oh!

Uh oh!

Uh oh!

Choose a reason for hiding this comment

Uh oh!

jaoleal Jan 27, 2026 • edited Loading Uh oh! There was an error while loading. Please reload this page.

Uh oh!

Choose a reason for hiding this comment

Uh oh!

luisschwab Jan 28, 2026 • edited Loading Uh oh! There was an error while loading. Please reload this page.

Uh oh!

Choose a reason for hiding this comment

Uh oh!

Choose a reason for hiding this comment

Uh oh!

Choose a reason for hiding this comment

Uh oh!

Davidson-Souza Jan 28, 2026 • edited Loading Uh oh! There was an error while loading. Please reload this page.

Uh oh!

Choose a reason for hiding this comment

Uh oh!

Uh oh!

Uh oh!

Uh oh!

jaoleal commented Jan 28, 2026 • edited Loading Uh oh! There was an error while loading. Please reload this page.

Uh oh!

Uh oh!

Uh oh!

jaoleal commented Jan 29, 2026

Uh oh!

Davidson-Souza left a comment

Choose a reason for hiding this comment

Uh oh!

Choose a reason for hiding this comment

Uh oh!

Choose a reason for hiding this comment

Uh oh!

Uh oh!

Uh oh!

Uh oh!

Uh oh!

Uh oh!

Uh oh!

jaoleal commented Feb 2, 2026

Uh oh!

jaoleal commented Feb 2, 2026

Uh oh!

jaoleal commented Feb 2, 2026

Uh oh!

Reviewers

jaoleal Jan 24, 2026 •

edited

Loading

jaoleal Jan 27, 2026 •

edited

Loading

luisschwab Jan 28, 2026 •

edited

Loading

Davidson-Souza Jan 28, 2026 •

edited

Loading

jaoleal commented Jan 28, 2026 •

edited

Loading