feat: add bdk_sqlite_store crate implementing PersistBackend

[notmandatory]

Description

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

Notes to the reviewers

Changelog notice

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

Bugfixes:

• [ ] This pull request breaks the existing API
• [ ] I've added tests to reproduce the issue which are now passing
• [ ] I'm linking the issue being fixed by this PR

[LLFourn]

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]

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]

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.

[notmandatory]

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

[notmandatory]

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]

I've rebased this PR on the latest version of #1203 and everything seems to be working fine in the new sqlite_store crate. There are still some build errors in other crates that I'll make sure are fixed after #1203 is merged and I rebase on master.

[notmandatory]

Rebased on latest version #1203.

[oleonardolima]

@notmandatory I got curious, what's the decision behind using rusqlite instead of something like sqlx ?

I'm not sure if this has been discussed before, please let me know if I should check some previous issue/discussion.

[notmandatory]

@notmandatory I got curious, what's the decision behind using rusqlite instead of something like sqlx ?

I'm not sure if this has been discussed before, please let me know if I should check some previous issue/discussion.

The reason for using rusqlite is just simplicity and fewer dependencies since all I want is a sqlite store for use in mobile apps. The sqlx crate has alot of dependencies, uses async, and I think is more suited for server apps.

But I think once we have this blocking/sqlite only store working its worth also creating a bdk_sqlx_store crate with async and at least postgres support for server apps.

[notmandatory]

I force pushed fixes mentioned above. I also updated the tests to persist multiple change sets with the scenarios:

• add a tx with last_seen but no whole_tx
• adding a whole_tx to the tx that only had a last seen
• add second anchors for existing tx1 and tx2

[notmandatory]

ACK acf3834

I appreciate you taking the time to implement this and making it easy for reviewers. The approach to use private traits in store.rs looks like a straightforward solution with minimal headaches. I noticed the sqlite store ChangeSet doesn't implement Default, and during read the changesets aren't appended, but with this implementation that doesn't appear necessary.

@ValuedMammal thanks for mentioning the missing Default. I ended up having to add Default to the ChangeSet type after all to test with multiple changesets and in the process did find an issue where append wouldn't work when appending from network = None to network = Some(xx).

[evanlinjin]

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

[evanlinjin]

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]

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

[notmandatory]

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.

[evanlinjin]

~~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]

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

[notmandatory]

@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]

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]

@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]

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"))
    }
}