deps: bump bdk to beta_6

[reez]

Description

This pull request updates the bdk-ffi crate to use the latest beta version of bdk, specifically version 1.0.0-beta.6.

The main changes include:

• Updating the Cargo.toml file.
• Add InvalidResponse to Esplora Error
• Adding transitively to ChainPosition
• FullScanResult -> FullScanResponse
• SyncResult -> SyncResponse
• made a small change, unrelated to beta 6, to error.rs to use BdkEsploraError in places where it used Error, both work but it keeps a consistent convention (and allowed us to remove Error from use bdk_esplora::esplora_client::{Error as BdkEsploraError, Error})... kinda hate throwing little things like this in a PR like this but since I was already updating that error I felt like it was worth it and not too confusing for reviewers hopefully

Notes to the reviewers

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]

LGTM. Would ACK once out of draft.

[reez]

LGTM. Would ACK once out of draft.

thanks for taking a look!

[reez]

Tests pass, next step is to build bindings locally and test out on BDK iOS, then I'll un-draft PR.

[reez]

Tests pass, next step is to build bindings locally and test out on BDK iOS, then I'll un-draft PR.

Build swift bindings locally and used in BDK iOS which still compiled bitcoindevkit/BDKSwiftExampleWallet#264

[thunderbiscuit]

I'm still working on my understanding of transitively anchored transactions and when exactly those can occur in your TxGraph, so I won't ACK just yet but this is looking good to me.

One small comment about our use of Txid.

[thunderbiscuit]

I don't particularly like the Txid type from bitcoin-ffi and think we should not use it. See bitcoindevkit/bitcoin-ffi#20 for details. In this case here it's a return value from the Rust side so we know it will be correct, but even so, I think using the type in its current form is not a good idea. Open to hear other people's thoughts on this, but IMO we should stick to String for txids for now.

[rustaceanrob]

Using the type allows for a more robust API in terms of traits. I directly export NodeEventHandler straight from bdk_kyoto in my PR which uses the actual Txid, but it gets coerced to a string for UniFFI users. I find that particularly nice, because Rust design choices have no effect on bindings in that scenario.

ldk-node basically uses string type aliases for almost every struct and event (i.e. payment invoice, preimage, channel ID). I think the correct approach is to use the abstraction if we are confident it is correct.

[thunderbiscuit]

My beef with it is the uncatchable, (runtime!) panic of your whole app if you or your users attempt for whatever reason to build one of those types incorrectly (a txid missing a letter at the end? The whole app collapses and you can't prevent it!).

@tnull how do you see this? Do you mitigate for this in any way? It just feels like a super dangerous type to have for bindings, and is the opposite of type-safety, because the type does not behave the way you'd want/expect it to, but the fact that it exists implies type-safety, whereas if you have a String it's clear that it's not type-safe, and you have at least that small setting of expectations.

To be clear I'm not against a Txid type at all, I just would like something that can throw an error if you try to build an incorrect one.

[reez]

FYI:

Moving to string for this PR. Then when we decide if we want to go with Txid we can have a follow up PR to change all instances of txid string to Txid type across the codebase.

Discussion can definitely continue here.

PR is ready to go as it relates to the txid discussion now.

[rustaceanrob]

you or your users attempt for whatever reason to build one of those types incorrectly

There is a big different here between using Txid as a return value and an argument. I could see the argument is strong for user-supplied values from potentially untrusted sources. Server provides "" for a response? Unlucky. However for return values that bdk-ffi constructs, are we sure the user can even mutate these? It seems abundantly safe to me to return Txid as opposed to string if we expect the user will treat it as a Txid, especially if they aren't allow to mutate Txid

[thunderbiscuit]

I agree with the fact that since it is returned, those specific txids it will not cause problems for the users. The problem (as I imagine it at least) is that the type now exists freely at the library level and users can create objects of that type themselves; it's implied that we believe in and support this type as representative of a Txid (maybe with the added complication that users familiar with the rust-bitcoin library will think it's a great, well thought-out type when really at the Swift/Kotlin layer it's just a string but I'm willing to disregard this for a moment).

Devs wanting to write typesafe software are likely to just start using it in other places (maybe in data structures they fully own), and the type will work more-or-less for them (they can build a txid abcd and nothing will get thrown). This is a little bad (because they think they're being typesafe but there are no checks for this Txid being valid in any way), but not terrible (as long as they don't try to use it in a bdk method their apps will not crash and burn). Over time however, if we widen our use of the type and start using it as an argument in methods/constructors, then... kaboom.

[tnull]

@tnull how do you see this? Do you mitigate for this in any way? It just feels like a super dangerous type to have for bindings, and is the opposite of type-safety, because the type does not behave the way you'd want/expect it to, but the fact that it exists implies type-safety, whereas if you have a String it's clear that it's not type-safe, and you have at least that small setting of expectations.

Excuse the delay here, this slipped off my radar for a bit I agree with the general sentiment that the UniffiCustomType conversion to string works nicely when we return such a value. But yeah, anywhere we take user input, we should probably avoid them and rather implement proper from_ methods etc.

I agree with the fact that since it is returned, those specific txids it will not cause problems for the users. The problem (as I imagine it at least) is that the type now exists freely at the library level and users can create objects of that type themselves; it's implied that we believe in and support this type as representative of a Txid (maybe with the added complication that users familiar with the rust-bitcoin library will think it's a great, well thought-out type when really at the Swift/Kotlin layer it's just a string but I'm willing to disregard this for a moment).

Hmm, tend to disagree with this in the context of BDK/LDK Node, as we there can make the excuse of what is and isn't intended API. But for bitcoin-ffi, we probably should avoid any non-trivial cases (ie. any cases which may fail).

[thunderbiscuit]

ACK 10c4d25.