fix(wallet): allow PersistedWallet to be Send + Sync

[notmandatory]

Description

fixes #1873 based on solution proposed by @praveenperera .

Notes to the reviewers

This is not a breaking change since it's only changing the internal _marker field's type.

Changelog notice

• Fix PersistedWallet to be Send + Sync, even when used with a !Sync persister type such as rusqlite::Connection.

Checklists

All Submissions:

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

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

[YuriQueirozAndrade]

ACK, i run build, test, clippy and fmt. All looks goods

[oleonardolima]

I'm quite divided on this one, still taking a look in SQLite docs, but it doesn't feel right to override the Send + Sync implementation for any given persister.

I think there's no need for this change and it should inherit the Send + Sync implementation as it's patterns and use a Mutex instead, there's a few issues/discussions on rusqlite about this and it seems their decision is related on how sqlite flags are used, see rusqlite/rusqlite#188.

[praveenperera]

@oleonardolima when you call persist you have to pass in the Connection if this Connection is stored in the same struct as the Wallet you still need to wrap the Connection in a mutex.

Without this change both the Connection and the Wallet would both need to be wrapped in a mutex for no reason.

Example without this change

#[derive(Debug, uniffi::Object)]
pub struct Wallet {
    pub id: WalletId,
    pub network: Network,
    pub bdk: Mutex<bdk_wallet::PersistedWallet<Connection>>,
    pub metadata: WalletMetadata,
    db: Mutex<Connection>,
}

Example with this change:

#[derive(Debug, uniffi::Object)]
pub struct Wallet {
    pub id: WalletId,
    pub network: Network,
    pub bdk: bdk_wallet::PersistedWallet<Connection>,
    pub metadata: WalletMetadata,
    db: Mutex<Connection>,
}

There isn't a safety issue here because the wallet doesn't actually store the Connection which is what is not thread safe. The only reason the Connection is included in the type at all is to separate it from other persister types.

[evanlinjin]

Suggested change

	_marker: PhantomData<fn(P) -> P>,
	_marker: PhantomData<*const P>,

According to @LLFourn

[notmandatory]

I tried this approach and get this error running test_thread_safety:

error[E0277]: `*const Connection` cannot be sent between threads safely
    --> crates/wallet/tests/wallet.rs:4215:19
     |
4215 |     thread_safe::<PersistedWallet<bdk_chain::rusqlite::Connection>>();
     |                   ^^^^^^^^^^^^^^^^^^^^^^^^^^^^^^^^^^^^^^^^^^^^^^^^ `*const Connection` cannot be sent between threads safely
     |
     = help: within `PersistedWallet<Connection>`, the trait `Send` is not implemented for `*const Connection`
     = help: the trait `Send` is implemented for `Connection`
note: required because it appears within the type `PhantomData<*const Connection>`
    --> /Users/steve/.rustup/toolchains/stable-aarch64-apple-darwin/lib/rustlib/src/rust/library/core/src/marker.rs:757:12
     |
757  | pub struct PhantomData<T: ?Sized>;
     |            ^^^^^^^^^^^
note: required because it appears within the type `PersistedWallet<Connection>`
    --> /Users/steve/git/notmandatory/bdk/crates/wallet/src/wallet/persisted.rs:121:12
     |
121  | pub struct PersistedWallet<P> {
     |            ^^^^^^^^^^^^^^^
note: required by a bound in `thread_safe`
    --> crates/wallet/tests/wallet.rs:4212:23
     |
4212 |     fn thread_safe<T: Send + Sync>() {}
     |                       ^^^^ required by this bound in `thread_safe`

[praveenperera]

PhantomData<*const P> makes it !Sync: https://doc.rust-lang.org/nomicon/phantom-data.html#table-of-phantomdata-patterns

[evanlinjin]

@notmandatory Sorry yeah my suggestion does the complete opposite of what we need.

[notmandatory]

According to Llama 3.3 70B I should use PhantomData<fn(&mut T)>:

"In this case, you want to ensure that your struct remains Send and Sync, even if the functions implemented on it have a &mut T parameter and T is not Sync.

To achieve this, you can use PhantomData<fn(&mut T)> instead of PhantomData<fn(T)>. This tells Rust that your struct is a "consumer" of &mut T, but it does not own T and therefore should not be tied to its lifetime."

[notmandatory]

I updated the code per comment above and also included some of the explanation in the commit message.

[praveenperera]

@notmandatory Perplexity seems to agree too

[notmandatory]

I pushed another commit to fix the CI issue. Ring just (4hrs ago) bumped their MSRV from 1.63 to 1.66 with a patch release 😞 (see briansmith/ring#2449).

[praveenperera]

@notmandatory I think in our case since we aren't dealing with lifetimes of any unsafe things, I think all three are practically equal, all of this compiles. I think the AIs are wrong.

PhantomData<fn(P) -> P>
PhantomData<fn(&mut P)>
PhantomData<fn(P)>

use std::marker::PhantomData;
use rusqlite::Connection;

pub struct PersistedWallet<P> {
    _marker: PhantomData<fn(&mut P)>,
}

impl<P> PersistedWallet<P> {
    pub fn new() -> Self {
        Self {
            _marker: PhantomData,
        }
    }
    pub fn persist(&self, _wallet: &mut P) {}
    pub fn persist_2(&self, _wallet: P) {}
}

pub struct PersistedWallet2<P> {
    _marker: PhantomData<fn(P)>,
}

impl<P> PersistedWallet2<P> {
    pub fn new() -> Self {
        Self {
            _marker: PhantomData,
        }
    }
    pub fn persist(&self, _wallet: &mut P) {}
    pub fn persist_2(&self, _wallet: P) {}
}

pub struct PersistedWallet3<P> {
    _marker: PhantomData<fn(P) -> P>,
}

impl<P> PersistedWallet3<P> {
    pub fn new() -> Self {
        Self {
            _marker: PhantomData,
        }
    }
    pub fn persist(&self, _wallet: &mut P) {}
    pub fn persist_2(&self, _wallet: P) {}
}

pub type Wallet = PersistedWallet<Connection>;
pub type Wallet2 = PersistedWallet2<Connection>;
pub type Wallet3 = PersistedWallet3<Connection>;

fn thead_safe<T>(_wallet: T)
where
    T: Sync + Sync,
{
}

fn main() {
    thead_safe(Wallet::new());
    thead_safe(Wallet2::new());
    thead_safe(Wallet3::new());
}

[notmandatory]

If all are safe and enable the same functionality then I think the one suggested by the AIs is still the best since it communicates that PersistWallet "consumes" the &mut P type in it's functions without owning P. But happy to change it to one of the other options if there's some compelling reason to.

[praveenperera]

That makes sense to me

[evanlinjin]

ACK cdba39d

TIL something about PhantomData.

[ValuedMammal]

ACK, needs a rebase now that #1876 is merged

[notmandatory]

@ValuedMammal I rebased this PR, OK to merge now?

[ValuedMammal]

ACK 14251a4