bitcoindevkit / bdk

A modern, lightweight, descriptor-based wallet library written in Rust!
Other
871 stars 312 forks source link

SQLite shouldn't depend on `serde_json` #1695

Open LLFourn opened 2 weeks ago

LLFourn commented 2 weeks ago

If we depend on serde_json for serializing anchors we will never be able to remove it since it will always be needed to do migrations. Currently there is only one widely used anchor. We can just use some custom serialization or normalize it into columns. We can require that the anchor implement {To/Fom}Sql for sqlite support.

I am bringing this up in the hope that we can fix this before doing a v1 release. Beta users can be advised to delete their DB data and rebuild it by doing a full_scan once we make the change.

notmandatory commented 2 weeks ago

Agreed. I originally put this in to handle the generic Anchors, but now I think we only need height and hash and those can go in their own columns.

nymius commented 3 days ago

I would like to work on this.

LLFourn commented 2 days ago

@nymius that would be great. IMO the approach should be to just implement things for anchors that implement ToSql/FromSql. I think we should use a BLOB type and just big endian encode the numbers as big-endian integers in the case of confirmation height and time.

cc @evanlinjin

nymius commented 2 days ago

Ok, so your approach is to ~reduce~ the anchor table to ~(txid, anchor)~ (txid, block_height, block_hash, anchor) where txid is stored as TEXT and anchor is a single BLOB with all the metadata serialized as a single stream of bytes (with numbers encoded in big endian).

Correcting me here: Anchors will always have block height and hash available, and we need something to individualize different anchors for the same tx, so I should not remove them from table, in fact, the table will stay as it is.

evanlinjin commented 2 days ago

Hey @nymius was just talking to @LLFourn in person about this. The conclusion is to constrain support to just be for a single anchor type ConfirmationBlockTime. The anchor table will represent all fields of ConfirmationBlockTime: block_height, block_hash and confirmation_time (+ txid).

Here are the reasons:

  1. No one is using rusqlite with any other anchor type, and if they are, they can do something custom anyway.
  2. We want to change how we represent the "anchor" concept in the future, supporting for multiple Anchor types here will cause more problems for migration later on.

I think this change will need to be backwards compatible. In tx_graph::ChangeSet::init_sqlite_tables, introduce schema_v1 which migrates the bdk_anchors table to rm anchor column in place of confirmation_time. This migration can assume that the contained anchor data is ConfirmationBlockTime (even though it is a generic right now).

nymius commented 15 minutes ago
  1. No one is using rusqlite with any other anchor type, and if they are, they can do something custom anyway.

I've just discovered on master re-base that #1736 breaks this assumption.