Skip to content

Express the wallet changeset over the FFI layer #756

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

Merged
merged 2 commits into from
May 30, 2025

Conversation

rustaceanrob
Copy link
Collaborator

@rustaceanrob rustaceanrob commented May 7, 2025

Description

Builds on #754 to create a fully expressive and serializable ChangeSet type. The goal is for this to enable arbitrary persistence backends across the FFI layer. Ultimately that would close #739 and any further database requests that are outside the immediately supported options.

For context: The iterators in the BDK changesets are BTree based to be no_std compatible and have deterministic ordering of duplicate data across all targets. UniFFI exposes only two iterators across the boundary, Vec and HashMap, but I can try to lay out why I think using these is acceptable.

The goal here is to convert between bdk_wallet::ChangeSet and FFI ChangeSet, and the easiest way to go about that is to just make each field convertible. For the LocalChain::ChangeSet, I iterate over the BTreeMap, which will return the key-value pairs in order. That means the vector will be in order over the FFI layer. Everything else coming from BDK could be ordered by key, but is not required in my opinion to implement the persistence. For instance any mapping from DescriptorId to last revealed index is okay, as the DescirptorId do not necessarily have to be in any particular order. Same goes for Txid or Outpoint mapping to some value. While it is nice to be reproducible, these keys are suitable for a HashMap.

Can elaborate on the next call

Notes to the reviewers

In Swift, and key of a HashMap must implement Equitable and Hashable. We can export Eq and Hash from Rust for Objects, but not for Records from what I can tell. As a result, I made a kind-of-gross wrapper called HashableOutPoint that is an Object so it can be used as a key, but it is convertible to the underlying Record if required. Open to ideas on that one.

Also, I cannot for the life of me figure out the best way to go from a Descriptor<DescriptorPublicKey> into DescriptorPublicKey and vise versa without introducing an unwrap. Feels like we can improve that.

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

@rustaceanrob rustaceanrob force-pushed the use-hash-types-5-7 branch 6 times, most recently from 5c7d850 to 1af0175 Compare May 8, 2025 08:43
@rustaceanrob rustaceanrob marked this pull request as ready for review May 8, 2025 09:23
@rustaceanrob rustaceanrob force-pushed the use-hash-types-5-7 branch 3 times, most recently from a2027f3 to 4940e39 Compare May 13, 2025 15:01
@rustaceanrob
Copy link
Collaborator Author

Best left for after #764

@rustaceanrob rustaceanrob force-pushed the use-hash-types-5-7 branch 2 times, most recently from 505a992 to 2c359be Compare May 23, 2025 08:05
@rustaceanrob
Copy link
Collaborator Author

Rebased and ready for discussion next call

@reez
Copy link
Collaborator

reez commented May 23, 2025

Rebased and ready for discussion next call

Sounds good I'll start reviewing leading up to next call

@rustaceanrob
Copy link
Collaborator Author

To clarify, when trying to compile

/// A reference to an unspent output by TXID and output index.
#[derive(Debug, Clone, Eq, PartialEq, std::hash::Hash, uniffi:: Record)]
#[uniffi::export(Eq, Hash)]
pub struct OutPoint {
    /// The transaction.
    pub txid: Arc<Txid>,
    /// The index of the output in the transaction.
    pub vout: u32,
}

UniFFI does not allow the export(Eq, Hash) which are required for keys in a hashmap. This is why HashableOutPoint exists.

rustaceanrob added a commit to rustaceanrob/bdk-ffi that referenced this pull request May 28, 2025
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 bitcoindevkit#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 cloud
storage.
@rustaceanrob rustaceanrob mentioned this pull request May 28, 2025
3 tasks
rustaceanrob added a commit to rustaceanrob/bdk-ffi that referenced this pull request May 28, 2025
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 bitcoindevkit#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 cloud
storage.
@rustaceanrob
Copy link
Collaborator Author

#771 demonstrates how this is used

rustaceanrob added a commit to rustaceanrob/bdk-ffi that referenced this pull request May 28, 2025
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 bitcoindevkit#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 cloud
storage.
rustaceanrob added a commit to rustaceanrob/bdk-ffi that referenced this pull request May 28, 2025
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 bitcoindevkit#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 cloud
storage.
rustaceanrob added a commit to rustaceanrob/bdk-ffi that referenced this pull request May 28, 2025
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 bitcoindevkit#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 cloud
storage.
rustaceanrob added a commit to rustaceanrob/bdk-ffi that referenced this pull request May 28, 2025
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 bitcoindevkit#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 cloud
storage.
rustaceanrob added a commit to rustaceanrob/bdk-ffi that referenced this pull request May 28, 2025
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 bitcoindevkit#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 cloud
storage.
Copy link
Collaborator

@reez reez left a comment

Choose a reason for hiding this comment

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

Sorry about the Swift specific Equatable and Hashable stuff needed, but yeah.

Whatever things you thought might be kind-of-gross still look like the right thing to me after review, the only alternate ideas I had ended up with a worse tradeoff balance than the ones in the PR.

Tested locally, also build swift bindings and tested on bdk iOS app, built and ran fine, no changes needed in iOS app for me since I wasn't using ChangeSet directly anywhere.

ACK c10721a

@rustaceanrob
Copy link
Collaborator Author

Thanks for that thorough review and testing! I will continue to poke around the UniFFI docs to see if there is a way to get around the HashableOutPoint, but with my latest solution for persistence, this type doesn't matter unless you need custom persistence.

@rustaceanrob
Copy link
Collaborator Author

afc9fb8 rebased

Copy link
Member

@thunderbiscuit thunderbiscuit left a comment

Choose a reason for hiding this comment

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

ACK afc9fb8.

@thunderbiscuit
Copy link
Member

Question for @rustaceanrob: given the approach of #771 (which I have not reviewed yet), it's the job of the application layer to serialize the ChangeSet type into bytes to prepare for encryption into custom persistence right? i.e., we don't need a sort of to_bytes() or serialize() method on the ChangeSet type if I understand correctly your proposed approach?

@rustaceanrob rustaceanrob force-pushed the use-hash-types-5-7 branch 2 times, most recently from 3854590 to 07a2363 Compare May 30, 2025 09:09
@rustaceanrob
Copy link
Collaborator Author

rustaceanrob commented May 30, 2025

Yeah, I think it makes sense to spell this out explicitly for future reference:

Primitive objects that implement to/from bytes:
Txid, Script, u32, u64, Transaction, BlockHash, DescriptorId, Network (is an integer), Descriptor (UTF-8 string).

The records in the change sets that are made up of the primitives:
ChainChange: u32, Option<BlockHash>
BlockId: u32, BlockHash
ConfirmationBlockTime: BlockId, u64
Anchor: ConfirmationBlockTime, Txid
TxOut: Script, u64
OutPoint: Txid, u32
HashableOutPoint: OutPoint
TxGraphChangeSet: Transaction, Anchor, HashableOutPoint, Txid, TxOut

By representing each of the smallest sub-components as serializable, the user can determine how they will store each one of these fields. For instance, maybe the Descriptor goes to the secure element, the chain is held in a native K-V store, and the TxGraph is encrypted on disk.

Some last minute clean-up:

  • HashableOutPoint needed a constructor
  • I got mixed up with the descriptor types, there is no need to do any weird string serialization. All unwrap are removed. On the Rust side, FWIW I think ExtendedDescriptor is a stupid type alias

@rustaceanrob
Copy link
Collaborator Author

They are aware of this bug for Swift, so hopefully we can remove HashableOutPoint in the future.

Looking for re-ACK on 30675df

Copy link
Member

@thunderbiscuit thunderbiscuit left a comment

Choose a reason for hiding this comment

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

ACK 30675df.

@thunderbiscuit thunderbiscuit merged commit 30675df into bitcoindevkit:master May 30, 2025
26 checks passed
@rustaceanrob rustaceanrob deleted the use-hash-types-5-7 branch May 30, 2025 15:56
rustaceanrob added a commit to rustaceanrob/bdk-ffi that referenced this pull request May 30, 2025
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 bitcoindevkit#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 cloud
storage.
rustaceanrob added a commit to rustaceanrob/bdk-ffi that referenced this pull request May 30, 2025
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 bitcoindevkit#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 cloud
storage.
rustaceanrob added a commit to rustaceanrob/bdk-ffi that referenced this pull request May 30, 2025
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 bitcoindevkit#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 cloud
storage.
rustaceanrob added a commit to rustaceanrob/bdk-ffi that referenced this pull request Jun 2, 2025
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 bitcoindevkit#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 cloud
storage.
rustaceanrob added a commit to rustaceanrob/bdk-ffi that referenced this pull request Jun 2, 2025
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 bitcoindevkit#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 cloud
storage.
rustaceanrob added a commit to rustaceanrob/bdk-ffi that referenced this pull request Jun 2, 2025
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 bitcoindevkit#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 cloud
storage.
rustaceanrob added a commit to rustaceanrob/bdk-ffi that referenced this pull request Jun 3, 2025
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 bitcoindevkit#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 cloud
storage.
rustaceanrob added a commit to rustaceanrob/bdk-ffi that referenced this pull request Jun 3, 2025
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 bitcoindevkit#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 cloud
storage.
rustaceanrob added a commit to rustaceanrob/bdk-ffi that referenced this pull request Jun 3, 2025
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 bitcoindevkit#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 cloud
storage.
rustaceanrob added a commit to rustaceanrob/bdk-ffi that referenced this pull request Jun 5, 2025
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 bitcoindevkit#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 cloud
storage.
Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment
Labels
None yet
Projects
Status: Done
Development

Successfully merging this pull request may close these issues.

Continued discussion on encrypting persisted data
3 participants