Abitrary persistence for wallet

[rustaceanrob]

Description

With the following approach we can accomplish two things at once:

1. Make it easy to add new Rust backends without sacrifing performance
2. Allow for arbitrary persistence accross the FFI

To accomplish this we can differentiate between a native backend and a foreign backend. The foreign backend interacts with the FFI ChangeSet, whereas the native backend can just use the Rust changeset directly. Whenever a new backend is introduced in Rust, a new enum variant may simply be added to the PersistenceType. To build a Sqlite backend, or a foreign backend, the user will use the Persister structure.

Abitrary persistence is allowed by implementing the Persistence trait. This was accomplished with no changes to #756.

I hope 1. motivates this change thoroughly, as we expect BDK will add support for new backends in the future. I am also interested in the applications of 2., where a user might be able to do encrypted and/or cloud storage.

Notes to the reviewers

We use an enum to allow for Rust backends to use Rust changesets directly. Otherwise we may convert them to FFI types. I've thought through many approaches, and I am convinced this is the most robust.

Changelog notice

• Add possibility for any type of persistence through a Persister type, that includes the usual sqlite approach

Checklists

All Submissions:

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

[rustaceanrob]

Should be ready to go. The integration tests ensure SQL still works.

I may implement a simple database in Swift or Python just to ensure the roundtrips are okay. Otherwise, I think this approach is pretty nifty, as the performance should be the same for SQL users.

[reez]

I may implement a simple database in Swift or Python just to ensure the roundtrips are okay.

I'd def love to see this for Swift, and could help be another tester when you implement for anything you need

[andreasgriffin]

Dummy implementation tested in python. Will check the round trip one.

[andreasgriffin]

Do I understand correctly:

• Each time I persist, only the latest ChangeSet is added in persist()

Do I have to merge multiple ChangeSet manually to be able to return 1 ChangeSet in initialize?

• Could the method for merging changesets be exposed too please? That way I could deserialize the changesets, merge them and then return the merged one in initialize

[rustaceanrob]

Do I have to merge multiple ChangeSet manually to be able to return 1 ChangeSet in initialize

You could also persist the total aggregate changeset, but this also seems to be a potential strategy and would certainly be easier.

Each time I persist, only the latest ChangeSet is added in persist

Yes

Could the method for merging changesets be exposed too please?

This would not be too hard to implement manually, i.e. for the chain just update the heights and hashes, increase the descriptor indexes, add any new transactions, etc. However, this would change the API in the future if we wanted to add it later, so I think it is best to add the convenience methods like merge now.

Because the ChangeSet is a uniffi::Record and this would be a method on ChangeSet, I will have to refactor the ChangeSet to be an uniffi::Object, which will result in significant changes with this PR. Instead of breaking it out into a new PR, I am just going to address it in this one.

[andreasgriffin]

Tested the round trip and it works great, Including serialization to Json .

[reez]

Looking pretty great:

1. I built local bindings for this and tested out on iOS app reez/BDKSwiftExampleWallet@1e3b6a5 , let me know if there is anything there that doesn't look right to you?
2. I see you updated the tests here, is there anything else being added to tests (I know we talked on the call yesterday about possible additional tests?)

[rustaceanrob]

Awesome thanks for giving that a real world test. @andreasgriffin is putting together a Python test when he has some time, I will potentially work on a Swift test, but wouldn't have time for a couple weeks. If you have any interest in pushing an offline Swift test with custom persistence to my branch, that would be much appreciated! We would just want to make sure a round trip from bytes back to the change set can be implemented and the load/persist matches.

[reez]

I see python test added 1ce620f, nice!

I'm not sure if the test is finalized or not yet, but I tested locally.

Locally I got a failure on line 130 where it uses bdk.Network | None syntax

(not a python expert but) from what I'm seeing that syntax looks like its only available in Python 3.10+ (I was on Python 3.9.6 locally). I changed the code to Optional[bdk.Network] and got it passing. I assume we need that since we have tests for python versions 3.8-3.12.

Also overall I think this test didn't get picked up in CI on the PR because it didn't use the "offline" word in it to get picked up, I think we should have it in the offline tests because to me it qualifies for that conceptually, totally open to others opinions though?

[rustaceanrob]

Good catch, I can add the offline prefix and fix that line tomorrow. Given how thorough the Python test is, I think we can keep the Swift and JVM with the default SQL options. Happy to accept other tests if they are added to my branch, but I think this should be finalized for review.

[rustaceanrob]

Test is renamed with test_offline prefix and bdk.Network | None is renamed to Optional[bdk.Network]

[andreasgriffin]

there might be a sorting issue of dicts in older python versions, that will let the test fail. I can investigate.

[rustaceanrob]

sorting issue of dicts in older python versions

Yeah, this is expected as the changeset uses hash maps, which can be initialized differently per two different dictionaries. I don't think the final assertion is necessary. The balance is equal if and only if the persist and initialize steps are applied properly.

[andreasgriffin]

sorting issue of dicts in older python versions

Yeah, this is expected as the changeset uses hash maps, which can be initialized differently per two different dictionaries. I don't think the final assertion is necessary. The balance is equal if and only if the persist and initialize steps are applied properly.

I think it is necessary, to check that persistence writing (not only initialization) works as expected.

I'll look into making the sorting deterministic

[andreasgriffin]

I fixed the issue in 2ac89a4 by using more sorting in the serializer.

[rustaceanrob]

f81b8d4 is updated to assert the two change sets are equal. Just in time for the weekend

[reez]

ACK f81b8d4

AWESOME