Skip to content

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

Open
jaoleal wants to merge 4 commits intogetfloresta:masterfrom
jaoleal:wallet_sqlite_descbased
Open

Wallet: move database to sqlite, remove kv and memmory db#804
jaoleal wants to merge 4 commits intogetfloresta:masterfrom
jaoleal:wallet_sqlite_descbased

Conversation

@jaoleal
Copy link
Collaborator

@jaoleal jaoleal commented Jan 24, 2026

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).

@Davidson-Souza Davidson-Souza added dependencies Pull requests that update a dependency file code quality Generally improves code readability and maintainability labels Jan 24, 2026
@jaoleal jaoleal force-pushed the wallet_sqlite_descbased branch from 90f9da2 to a5e86c3 Compare January 24, 2026 22:11
@jaoleal
Copy link
Collaborator Author

jaoleal commented Jan 24, 2026

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

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

jaoleal commented Jan 25, 2026

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:

Copy link
Member

@Davidson-Souza Davidson-Souza left a comment

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

Generally looks good. Minor comments

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

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

jaoleal commented Jan 28, 2026

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.

@jaoleal jaoleal force-pushed the wallet_sqlite_descbased branch 2 times, most recently from 4252dbd to 090fe9d Compare January 28, 2026 13:35
@jaoleal jaoleal force-pushed the wallet_sqlite_descbased branch 2 times, most recently from c8199b4 to 47bd5c7 Compare January 28, 2026 22:43
@jaoleal
Copy link
Collaborator Author

jaoleal commented Jan 29, 2026

Done @Davidson-Souza @luisschwab

Copy link
Member

@Davidson-Souza Davidson-Souza left a comment

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

Your commits are in reverse chronological order. What happened?

self.database.save(&new_address);
self.database
.save(&new_address)
.expect("Database not working");
Copy link
Member

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

Errors that can happens should either be handled or propagated

Copy link
Collaborator Author

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

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.

Copy link
Member

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

I don't think it's out of scope, but I would recommend opening another pr before this one gets merged, improving this trait to return a result. Since you are touching this, I think it's better to do it right. The old method was infallible because it had an expect inside the DB wrapper, completely terrible!

@jaoleal
Copy link
Collaborator Author

jaoleal commented Feb 2, 2026

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 jaoleal closed this Feb 2, 2026
@jaoleal jaoleal reopened this Feb 2, 2026
@jaoleal jaoleal force-pushed the wallet_sqlite_descbased branch from 47bd5c7 to cfaa2ab Compare February 2, 2026 22:47
@jaoleal
Copy link
Collaborator Author

jaoleal commented Feb 2, 2026

Applied @Davidson-Souza suggestions

@jaoleal jaoleal force-pushed the wallet_sqlite_descbased branch from cfaa2ab to e358b7f Compare February 2, 2026 23:35
@jaoleal
Copy link
Collaborator Author

jaoleal commented Feb 2, 2026

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

@JoseSK999
Copy link
Member

It's rebase season

@jaoleal jaoleal force-pushed the wallet_sqlite_descbased branch 2 times, most recently from a76a391 to a874519 Compare February 4, 2026 15:00
@jaoleal jaoleal force-pushed the wallet_sqlite_descbased branch from ba18458 to db07edd Compare February 9, 2026 19:55
@jaoleal jaoleal force-pushed the wallet_sqlite_descbased branch 2 times, most recently from d31ca78 to 612b3a2 Compare February 11, 2026 18:05
@jaoleal
Copy link
Collaborator Author

jaoleal commented Feb 11, 2026

Forgot to update Cargo.lock

@jaoleal jaoleal force-pushed the wallet_sqlite_descbased branch from 612b3a2 to 6f604f5 Compare February 11, 2026 19:55
Copy link
Member

@JoseSK999 JoseSK999 left a comment

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

Refactor needed again

[dependencies]
bitcoin = { workspace = true, features = ["serde"] }
kv = { workspace = true }
rusqlite = { version = "0.38", optional = true, features = [ "bundled", "buildtime_bindgen" ], default-features = false }
Copy link
Member

@JoseSK999 JoseSK999 Feb 13, 2026

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

I also think buildtime_bindgen is not needed nor relevant

Copy link
Collaborator Author

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

I had some problems with the bindgen at the beginning of this PR that made me include that feature, did you saw #804 (comment) ?


[dev-dependencies]
rand = { workspace = true }
rusqlite = { version = "0.38", features = [ "bundled", "buildtime_bindgen" ], default-features = false }
Copy link
Member

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

And here

@JoseSK999 JoseSK999 mentioned this pull request Feb 18, 2026
9 tasks
@JoseSK999 JoseSK999 added this to the v0.9.0 milestone Feb 18, 2026
@jaoleal jaoleal force-pushed the wallet_sqlite_descbased branch from 6f604f5 to c5c9058 Compare February 18, 2026 22:32
@jaoleal jaoleal force-pushed the wallet_sqlite_descbased branch from b1c7e3b to bdd317a Compare February 21, 2026 17:16
@jaoleal
Copy link
Collaborator Author

jaoleal commented Feb 21, 2026

@Davidson-Souza I made the af015f0 commit thats focused to address #804 (comment) and similar cases.

@JoseSK999 I took a different approach to make the tables more readable, please take a look.

@jaoleal jaoleal force-pushed the wallet_sqlite_descbased branch 2 times, most recently from af015f0 to d8bef82 Compare February 21, 2026 18:46
@jaoleal
Copy link
Collaborator Author

jaoleal commented Feb 21, 2026

b274920 addresses @moisesPompilio suggestions about gating new_ephemeral

@jaoleal jaoleal force-pushed the wallet_sqlite_descbased branch from d8bef82 to ccbb10d Compare March 2, 2026 20:21
@jaoleal
Copy link
Collaborator Author

jaoleal commented Mar 2, 2026

Rebased and Applied @JoseSK999 suggestions.

Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment

Labels

code quality Generally improves code readability and maintainability dependencies Pull requests that update a dependency file

Projects

None yet

Development

Successfully merging this pull request may close these issues.

6 participants