fix(wallet): remove the generic from wallet

[rustaceanrob]

Description

The PersistenceBackend uses generics to describe errors returned while applying the change set to the persistence layer. This change removes generics wherever possible and introduces a new public error enum. Removing the generics from PersistenceBackend errors is the first step towards #1363

Update: I proceeded with removing the generics from Wallet by introducing a Box<dyn PersistenceBackend> .

Notes to the reviewers

This one sort of blew up in the number of changes due to the use of generics for most of the Wallet error variants. The generics were only used for the persistence errors, so I removed the generics from higher level errors whenever possible. The error variants of PersistenceBackend may also be more expressive, but I will level that up for discussion and make any changes required.

Changelog notice

• Changed PersistenceBackend errors to depend on the anyhow crate.
• Remove the generic T from Wallet

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

[evanlinjin]

Thanks for working on this. In the future, please state in the ticket that you'll be working on it, otherwise we'll result in duplicate work.

[rustaceanrob]

Will do! Thanks for the tip

[rustaceanrob]

It is not obvious why this change is useful for removing D from Wallet. I am going to continue work towards that goal on this branch.

[rustaceanrob]

Broke no_std with an error variant. Will fix soon.

[LLFourn]

So after looking and thinking about this more closely I'd expect two error enums, one for each interaction:

enum LoadError {
   #[cfg(feature = "std")]
   StdIo(std::io::Error),
   Other(String),
}

enum CommitError {
    #[cfg(feature = "std")]
   StdIo(std::io::Error),
   Other(String),
   Inconsistent(String) // maybe?
}

But looking at it we don't really get much value from this structure unless we add specific error variants that occur with our changesets that we can report and react to. But our changeset API is (mostly) makes invalid representations impossible. In so far as invalid representations are possible they depend on the concrete changeset which the backend doesn't have access to.

The correct thing to do here I think is to return Box<dyn core::error::Error> (except that doesn't exist yet!). How about we just use anyhow::Error to do this. It would be better to not have this dependency but I think replicating what anyhow does (by Boxing our own error trait) is the next best solution and seems like a waste of time when almost every rust app depends on anyhow anyway. I think we should just accept it and wait for core::error::Error.

cc @evanlinjin

[rustaceanrob]

Added my interpretation of how we would use anyhow here. Easy enough to add on or revert back.

[LLFourn]

This looks great. Main question is whether we're comfortable with this in bdk_chain or we should just make a new bdk_persist crate which defines this trait. It is rather orthogonal to everything else in bdk_chain and now it adds another dependency. If bdk_core existed (#1155) likely it wouldn't even depend on bdk_chain. "persist" could also be a feature flag in bdk_chain.

As a side note, for some reason we use this trait in the bdk_chain examples but I'm not sure why. They could easily be rewritten to not depend on this trait.

cc @thunderbiscuit wdyt about anyhow::Error. How do you handle errors in the FFI?

[evanlinjin]

This looks great! However, I'm wondering whether it's better to introduce our own lightweight trait instead on using anyhow (which is another dependency).

pub trait PersistError: core::any::Any + Debug {}

pub fn an_example_method() -> Result<(), Box<dyn PersistError>> { todo!() }

The downsides of this is that the PersistenceBackend implementations may need to wrap their returned errors in a new type to implement PersistError. However, I see this as a minor compromise as they need to implement PersistenceBackend anyway?

cc. @LLFourn @ValuedMammal

[rustaceanrob]

Committed those changes for review

[LLFourn]

The downsides of this is that the PersistenceBackend implementations may need to wrap their returned errors in a new type to implement PersistError. However, I see this as a minor compromise as they need to implement PersistenceBackend anyway?

cc. @LLFourn @ValuedMammal

I don't think this is a minor downside for the implementer. It means wrapping all errors from the underlying backend in a type and implementing a trait on the wrapped types. Also then what about downcasting? What about backtraces? anyhow::Error just does everything right and there's no point in trying to replicate it.

I'm leaning towards putting the persist trait in its own crate bdk_persist. It never made much sense in bdk_chain, it was just convenient to put it there but it no longer is.

[rustaceanrob]

Should I continue by breaking out into a bdk persist crate? Or is that best left for another PR?

[ValuedMammal]

The correct thing to do here I think is to return Box<dyn core::error::Error> (except that doesn't exist yet!). How about we just use anyhow::Error to do this. It would be better to not have this dependency but I think replicating what anyhow does (by Boxing our own error trait) is the next best solution and seems like a waste of time when almost every rust app depends on anyhow anyway. I think we should just accept it and wait for core::error::Error.

I'd be in favor of finding a solution that doesn't require anyhow, if possible. Currently, the PersistBackend errors are only constrained by Debug, so a new PersistError struct (or enum) could be something like

pub struct PersistError(pub Box<dyn fmt::Debug + Send + Sync>);

and make PersistBackend implementations return this concrete error, which still feels a bit backwards, but in practice could be as simple as writing a From conversion or constructing the type directly.

The persist module could also just live in bdk crate along with wallet, no?

[evanlinjin]

@rustaceanrob we haven't reached a consensus on this. Feel free to put in your input.

[LLFourn]

The correct thing to do here I think is to return Box<dyn core::error::Error> (except that doesn't exist yet!). How about we just use anyhow::Error to do this. It would be better to not have this dependency but I think replicating what anyhow does (by Boxing our own error trait) is the next best solution and seems like a waste of time when almost every rust app depends on anyhow anyway. I think we should just accept it and wait for core::error::Error.

Replicating what anyhow does 100% is definitely not in scope. You can attach context anyhow errors and backtraces are automatically attached if your rust version is new enough. Of course we don't need this but it is nice to have.

I'd be in favor of finding a solution that doesn't require anyhow, if possible. Currently, the PersistBackend errors are only constrained by Debug, so a new PersistError struct (or enum) could be something like

pub struct PersistError(pub Box<dyn fmt::Debug + Send + Sync>);

And also Display. But we need to go further and make it + core::any::Any + 'static so you can downcast the error to the concrete one. You will likely want to get the specific backend error at the application level e.g. the concrete postgres error. Then impl From<E> where: E: std::error::Error. This is quite a lot of stuff to maintain to avoid a dependency which virtually every application uses (and should use!). The question boils down to whether we want to maintain our own dynamic error system when there is already a standard choice for this very common problem in the rust ecosystem. I'd say no but it's less my decision to make than those who will actually be doing the work :)

and make PersistBackend implementations return this concrete error, which still feels a bit backwards, but in practice could be as simple as writing a From conversion or constructing the type directly.

The persist module could also just live in bdk crate along with wallet, no?

Yes that could also work but I have a feeling the persist trait will be updated much less frequently than bdk_wallet. It wouldn't make sense to have to bump the version on the sqlite crate because some orthogonal API change happened in bdk_wallet.

[thunderbiscuit]

cc @thunderbiscuit wdyt about anyhow::Error. How do you handle errors in the FFI?

I'm not familiar with anyhow but reading up on it now. In general I favour not reimplementing the wheel, and think that using tried-and-true libraries is more robust over time (i.e., I am in favour of using anyhow instead of building similar behaviour in-house). One question I'd ask is if we add the anyhow dependency here, does it mean we'll have 2 approaches to errors across the codebase? Or would it be used in this single instance only?

As for FFI, we can't pass the errors directly (very!) unfortunately, so we roughly have to create new ones and map the Rust errors to those. This change would not impact us too much either way; do whatever is best for the Rust side of things and we'll adapt.

[rustaceanrob]

I understand the reluctance to bring in a new dependency, but as @LLFourn described, the behavior that we are looking for would just be a re-implementation of what anyhow already does. anyhow also adds an additional end-user benefit of context and backtraces that feel out of scope for this PR, but improve the end-user developer experience imo. I am personally not seeing enough drawbacks compared to both the end-user and maintainer benefits gained from a drop in replacement, but I am new here of course!

On structure, I am not seeing chain import bdk, so I'm not sure about moving it to the wallet side of things. This is type of common behavior that should rarely change seems like a prime candidate for bdk_core. If I were to weigh in on structure, I would leave things how they are for this PR and wait for a definitive restructuring decision, or start off bdk_core with bdk_core/persist.

As for FFI, we can't pass the errors directly (very!) unfortunately, so we roughly have to create new ones and map the Rust > errors to those. This change would not impact us too much either way; do whatever is best for the Rust side of things and > we'll adapt.

I have contributed to code with public anyhow result types along with UniFFI bindings. I will reach out to him to see what his experience was. Happy to help with UniFFI chores.

[LLFourn]

Fine to leave it here for now. I still have a weak preference for moving it into bdk_persist rather than bdk_core (don't really want bdk_core to depend on anyhow).

ACK bfadae7

Might be nice to clean up the commit history before merging. I think this should just be one commit on top of master if we can manage it.

[ValuedMammal]

I acknowledge all the points in the previous discussion, and I can offer these review comments :)

[rustaceanrob]

Should I squash commits locally? I only ask because I think there's also a feature to do it on the merge GUI.

[ValuedMammal]

Sorry, I have a couple more notes.

Primary suggestions are for store.rs, and I'm also wondering whether the From impl in wallet::error module is needed. Any nits are definitely not blockers.

As far as squashing, my preference is to use git rebase --interactive.

[rustaceanrob]

Squashed to c958fb4 with changes suggested from @ValuedMammal. Should be ready for CI

[jirijakes]

This change silently removes Sync and Send auto traits from Persist and therefore from anything that uses it (e. g. Wallet).

One way to deal with it could be:

pub struct Persist<C> {
    backend: Box<dyn PersistBackend<C> + Send + Sync>,
    stage: C,
}

but that would require every backend to be Send + Sync, even when not needed.

I am not sure how big of a problem this is, but given that Wallet until now tried to be Sync + Send, it may be an issue.

[rustaceanrob]

As far as I can tell Wallet is already not Sync? Let me know if I am misunderstanding something!

[jirijakes]

As far as I can tell Wallet is already not Sync?

It is in alpha.8.

But I really don't know whether this was some accident or deliberate action. I don't know whether this is something to care about or not.

[rustaceanrob]

Interesting. I tried messing with it and I am not aware of any other work-arounds besides the one you proposed. Between Sync and Send, not having Send seems to be the most problematic as earlier releases of Wallet were Send. Is it reasonable for all backends to be Send and Sync? @LLFourn @evanlinjin

[ValuedMammal]

That's a great point @jirijakes what made you realize that? I mean is it evident from looking at the code, or is there something more subtle going on? I do think it's generally desirable to maintain thread safety of the wallet.

[jirijakes]

what made you realize that?

Rather a coincidence. A few days ago I read articles (mainly this) about this kind of semver breakage. Having the articles fresh in mind and seeing Box, I was curious.

[rustaceanrob]

I updated the PR to include the Sync + Send trait bounds on the PersistBackend when creating a new Wallet. Although this may be a breaking change for wallets that implement their own store, they should easily be able to enforce Sync + Send on their ChangeSet generic. The benefit gained of Wallet being thread safe feels like a reasonable tradeoff, especially in the server context. Thank you @jirijakes for that insight.

[LLFourn]

Yeah this change implies that we must force PersistBackend to be Send + Sync. Before Send and Sync depended on PersistBackend impl. Now since we are using a trait object we have to make a decision for the user and the right decision is to add Send + Sync to the trait object. It's a slight regression but easily worth it for what we get. For things that are not Send or Sync you can always just make a channel or something like that to communicate with it (where the channel sender would be Send + Sync).

[evanlinjin]

ACK e51af49

This is good work. However, I'm really not a fan of introducing the anyhow dependency to the bdk_chain crate.

I think we should create a separate RP which introduces a separate bdk_persist crate.