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

feat: add bdk_sqlite crate implementing PersistBackend #1128

Merged
merged 5 commits into from
May 23, 2024

Conversation

notmandatory
Copy link
Member

@notmandatory notmandatory commented Sep 19, 2023

Description

Add "bdk_sqlite_store" crate implementing PersistBackend backed by a SQLite database.

Notes to the reviewers

In addition to adding a SQLite based PersistenceBackend this PR also:

  • add CombinedChangeSet in bdk_persist and update wallet crate to use it.
  • updates the wallet/tests to also use this new sqlite store crate.
  • updates example-crates/wallet_esplora_async to use this new sqlite store crate.
  • fixes out of date README instructions for MSRV.
  • bumps the ci build_docs job to rust nightly-2024-05-12.

Changelog notice

Changed

  • Update Wallet to use CombinedChangeSet for persistence.

Added

  • Add CombinedChangeSet in bdk_persist crate.
  • Add bdk_sqlite crate implementing SQLite based PersistenceBackend storage for CombinedChangeSet.

Checklists

All Submissions:

  • I've signed all my commits
  • I followed the contribution guidelines
  • I ran cargo fmt and cargo clippy before committing

New Features:

  • I've added tests for the new feature
  • I've added docs for the new feature

@notmandatory notmandatory changed the title feat: add bdk_sqlite_store crate implementing PersistBackend backed b… feat: add bdk_sqlite_store crate implementing PersistBackend Sep 19, 2023
@LLFourn
Copy link
Contributor

LLFourn commented Sep 19, 2023

hey @notmandatory. What do you think about the approach of just putting the the data in actual tables rather than serializing it in some blob? For reference the tables we'd have for this would be:

  1. Raw transactions (txid, tx_binary)
  2. Block heights and hashes (height, hash) keyed by hash
  3. Transaction index data. This is currently an integer per keychain for KeychainTxOutIndex which Wallet uses. This might also include the descriptor in the future.

The advantage would be that the database would be intelligible on the sqlite cli and from other applications. The disadvantage would be that we couldn't use sqlite for just any type of changeset. You'd probably have some extra trait over changesets that applications would have to implement on their changesets that return the inner indexed_tx_graph::ChangeSet and the local_chain::ChangeSet.

@notmandatory
Copy link
Member Author

hey @notmandatory. What do you think about the approach of just putting the the data in actual tables rather than serializing it in some blob?

It's funny you should mention it, I started out with the multi tables/columns approach but thought maybe it wasn't in the spirit of the PersistBackend trait. Also I wasn't sure it was worth the trouble if it could only be used with one ChangeSet type. But I'll give it another shot this week, probably in a different PR in case we decide to go back to this blobs approach.

@LLFourn
Copy link
Contributor

LLFourn commented Sep 21, 2023

hey @notmandatory. What do you think about the approach of just putting the the data in actual tables rather than serializing it in some blob?

It's funny you should mention it, I started out with the multi tables/columns approach but thought maybe it wasn't in the spirit of the PersistBackend trait. Also I wasn't sure it was worth the trouble if it could only be used with one ChangeSet type. But I'll give it another shot this week, probably in a different PR in case we decide to go back to this blobs approach.

Sure if we do go blobs I think we should use bincode rather than JSON.

@nondiremanuel nondiremanuel added this to the 1.0.0-alpha.3 milestone Sep 26, 2023
@notmandatory notmandatory self-assigned this Nov 12, 2023
@notmandatory notmandatory modified the milestones: 1.0.0-alpha.3, 1.0.0-alpha.4 Nov 13, 2023
@nondiremanuel nondiremanuel modified the milestones: 1.0.0-alpha.4, 1.0.0 Jan 6, 2024
@notmandatory notmandatory added the new feature New feature or request label Feb 26, 2024
@notmandatory
Copy link
Member Author

FYI I rebased off of #1203 so only look at my commits.

@notmandatory
Copy link
Member Author

notmandatory commented Apr 11, 2024

I've implemented the PersistBackend trait for SQLite with the below change set type. Next step is to test it out with the examples.

#[derive(Clone, Debug, PartialEq)]
struct ChangeSet<K: Ord + for<'de> Deserialize<'de> + Serialize>(
    local_chain::ChangeSet,
    indexed_tx_graph::ChangeSet<ConfirmationTimeHeightAnchor, keychain::ChangeSet<K>>,
);

EDIT: I updated PR to use ConfirmationTimeHeightAnchor instead of ConfirmationHeightAnchor.

@notmandatory notmandatory force-pushed the feat/sqlite_store branch 5 times, most recently from 3376ecc to fd7b709 Compare April 16, 2024 03:16
crates/sqlite_store/src/store.rs Outdated Show resolved Hide resolved
crates/sqlite_store/src/store.rs Outdated Show resolved Hide resolved
@evanlinjin
Copy link
Member

I'm very happy with this codebase, thanks for spearheading this effort @notmandatory. Just the things I've mentioned above and I'll be happy to ACK.

@notmandatory
Copy link
Member Author

notmandatory commented May 20, 2024

Also, what do you think about naming the crate bdk_sqlite? The term store doesn't add any more meaning to the name.

On second thought can we leave the name bdk_sqlite_store? I started to rename it but then to be consistent I'd have to rename the crate folder to crates/sqlite which seems weird since it's not the sqlite project but the way to use sqlite to store bdk data. I renamed it in a0084dc.

@notmandatory notmandatory force-pushed the feat/sqlite_store branch 3 times, most recently from 8c34d0d to d614e9e Compare May 20, 2024 21:28
@notmandatory
Copy link
Member Author

I ran into a docs error that I fixed by removing the rust doc link in the README. Can add it back later after the crate is published but it's not that important.

Copy link
Member

Choose a reason for hiding this comment

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

This file should be removed? We should be using bdk_coin_select imported from our new repo.

Copy link
Member

Choose a reason for hiding this comment

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

Okay it's not removed yet, that's pretty annoying. This is out of scope for this PR though.

@evanlinjin
Copy link
Member

evanlinjin commented May 21, 2024

While thinking about how we can do away with the wallet feature flag, I had a rethink about how we were structuring everything for the persistence stuff. Here are my new thoughts.

Initially, we moved PersistBackend out of bdk_chain and into a new crate (bdk_persist) because we did not want bdk_file_store to be dependent on bdk_chain. This will only make sense if we expect bdk_file_store to be used outside of the context of BDK (which I assume to be the case). The introduction of bdk_persist then became a side-effect because we do not expect bdk_persist to be used outside of the scope of BDK (so ideally, it would also be contained in bdk_chain).

I think this was the wrong approach. We should have separated bdk_file_store into 2 crates:

  • flatfile_store - This would live outside of the bitcoindevkit/bdk repo and not have any bdk dependencies. This can be used by other projects that do not require BDK.
  • bdk_flatfile_store - This will be part of bitcoindevkit/bdk.

With this new structure, we can do away with bdk_persist because bdk_flatfile_store will always be used in the context of BDK and we have flatfile_store that has no BDK dependencies.

In addition to this, we can have combined-ChangeSet structures (that combine changesets from local_chain, indexed_tx_graph, etc.) which we maintain within bdk_chain. These changesets can be used in bdk_wallet and bdk_sqlite_store. This will get rid of the wallet feature flag requirement here.

Edit: Okay I got this wrong. We made bdk_persist because we did not want bdk_chain to be dependent on anyhow. In this case, it would be fine to have bdk_persist depend on bdk_chain. We can contain combined-changeset structures within bdk_persist.

@notmandatory
Copy link
Member Author

I renamed

Also, what do you think about naming the crate bdk_sqlite? The term store doesn't add any more meaning to the name.

We discussed the naming at our 17:00 UTC BDK call today and consensus agrees with the shorter naming so I made the change in a0084dc.

@notmandatory
Copy link
Member Author

@evanlinjin I merged your latest PR commits but removed DbCommitment and the transform() logic.

bdk_sqlite already depends on bdk_persist so I don't see any reason why Store shouldn't use persist::changesets::CombinedChangeSet directly instead of trying to transform it to a DbCommitment. If this looks OK to you I'll squash and cleanup the commits.

@evanlinjin
Copy link
Member

ConceptACK for removing DbCommitment and transform stuff for now.

Although there is an argument that the transform stuff might be useful if the caller wants to have custom changesets? However, I guess they can impl PersistBackend themselves. Let's leave it out for now.

Let's squash!

@notmandatory notmandatory changed the title feat: add bdk_sqlite_store crate implementing PersistBackend feat: add bdk_sqlite crate implementing PersistBackend May 22, 2024
Copy link
Member

@evanlinjin evanlinjin left a comment

Choose a reason for hiding this comment

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

Overall I'm quite happy how this turned out.

In terms of styling, is the use of traits just to "section" the code? If that is the case, I would have preferred alternative ways to do so (i.e. you can have multiple impl Store {} blocks with doc comments at the top of each block). Using traits makes it harder to read and adds weird quirks (i.e. the funky thing you needed to do with db_transaction).

I'll ACK once we get rid of bdk_wallet dependency from bdk_sqlite.

notmandatory and others added 4 commits May 22, 2024 23:02
It is a good idea to have common changeset types stored in
`bdk_persist`. This will make it convenient for persistence crates and
`bdk_wallet` interaction.
@notmandatory
Copy link
Member Author

notmandatory commented May 23, 2024

@evanlinjin great styling tip, I redid as you suggested and it look much better (I don't know what I was thinking with the traits 🙃 ) and removed the wallet features, sorry I forgot to remove that last push.

I also reorganized and squashed commits so it should be ready to merge. Thanks for all the help!

@notmandatory
Copy link
Member Author

I amended/force pushed another good suggestion from @evanlinjin to impl PersistBackend generic over types that can be converted From and Into CombinedChageSet:

impl<K, A, C> bdk_persist::PersistBackend<C> for Store<K, A>
where
    K: Ord + for<'de> Deserialize<'de> + Serialize + Send,
    A: Anchor + for<'de> Deserialize<'de> + Serialize + Send,
    C: Clone + From<CombinedChangeSet<K, A>> + Into<CombinedChangeSet<K, A>>,
{
    fn write_changes(&mut self, changeset: &C) -> anyhow::Result<()> {
        self.write(&changeset.clone().into())
            .map_err(|e| anyhow::anyhow!(e).context("unable to write changes to sqlite database"))
    }

    fn load_from_persistence(&mut self) -> anyhow::Result<Option<C>> {
        self.read()
            .map(|c| c.map(Into::into))
            .map_err(|e| anyhow::anyhow!(e).context("unable to read changes from sqlite database"))
    }
}

Copy link
Member

@evanlinjin evanlinjin left a comment

Choose a reason for hiding this comment

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

ACK 475c502

@evanlinjin evanlinjin merged commit 2eea0f4 into bitcoindevkit:master May 23, 2024
12 checks passed
@storopoli storopoli mentioned this pull request May 23, 2024
8 tasks
Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment
Labels
module-database new feature New feature or request
Projects
Archived in project
Development

Successfully merging this pull request may close these issues.

8 participants