deps: bump bdk to beta-2

[rustaceanrob]

Description

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

[thunderbiscuit]

I like it. This is starting to look good! Just writing notes here to keep track of stuff left to do:

• Run all tests and fix them
• Throw the correct errors in the correct places
• Implement finish_no_persist on the TxBuilder
• Enable the create method on Wallet and WalletNoPersist to be the default constructor
• Squash or at least clean up commit history
• Wait for the merge of Enable selecting use-rustls-ring feature on electrum client bdk#1491
• Use release versions of libraries

Also thanks for the macro @rustaceanrob! It's our first in the codebase so I want to make sure we do it right. But given the extra code required to deal with generics, I suspect this might be useful in the future as well.

[rustaceanrob]

Implement finish_no_persist on the TxBuilder

Is it okay if I write a slightly more complex macro for this?

Edit: We also need to implement finish and finish_no_persist for BumpFeeTxBuilder. I wrote another macro but even without repeating logic I feel like this duplication isn't scalable. Going to think about some different approaches but I think there may be a case to not support the wallet without persistence.

[thunderbiscuit]

Note on a required change from the Rust API: the builder pattern ending with load_wallet() returns an Option but the constructors in uniffi cannot. So here I decided to add a variant on the LoadWithPersistError that gets thrown if the load_wallet() returns None.

[rustaceanrob]

I chewed on this for a while and the duplicated logic + new macros for an otherwise macro-free codebase have left a bad taste in my mouth. If future backends are made, under the current API they would also be Persist<Wallet>. The only type that would not be an implementation of Persist is the completely stateless wallet. I would argue one could implement a stateless wallet by just not calling Wallet::persist. If the user is determined to not save state, it is easily done by doing nothing at all. Whereas the other way around, having first class support for a wallet that explicitly doesn't persist data, is the cause for all of this extra overhead.

[rustaceanrob]

@thunderbiscuit is it okay if I squash this down to a single commit?

[rustaceanrob]

Open questions I have:

• What are we doing with FFIGenericError
• Can we just remove WalletCreationError
• Are we going to offer support for single descriptor wallets?

The current representation of persist on Wallet is:

pub fn persist(&self, connection: Arc<Connection>) -> bool {
    let mut binding = connection.get_store();
    let db: &mut BdkConnection = binding.borrow_mut();
    self.get_wallet().persist(db).expect("persist failed")
}

I think we should return a Result<bool, SqliteError>

[reez]

• What are we doing with FFIGenericError

Was introduced temporarily from what I can see in the commit history, but since then it looks like we are not using it anymore because we have replaced it with more specific errors ya? (if so, we can remove FFIGenericError, but if ya'll have a need for it lmk)

[reez]

• Can we just remove WalletCreationError

Looks like it to me, unless I'm missing something, I don't see it being used anywhere/anymore either

[reez]

I think we should return a Result<bool, SqliteError>

I like that idea.

[rustaceanrob]

I couldn't look at the commit history anymore. Apologies @thunderbiscuit I forgot to pick out your commits.

I pushed up some changes not present in the previous commits:

• Keeping in line with what the Rust API is doing. start_full_scan now returns a FullScanRequestBuilder, which may be consumed by calling build, or may inspect the keychains first and then build. I changed the InspectError to a RequestBuilderError as it may be thrown for either method. Same thing goes for SyncRequest
• persist may now throw a SqliteError
• Hopefully the tests should pass

[rustaceanrob]

Updated the Android and Swift tests and ran cargo fmt with the CI config

Agh. I will fix those swift tests and try! again lol

[rustaceanrob]

Last try or else I will recruit Matthew for help. I've just been guessing how to pass the test cause I can't build with xcodebuild on Linux lol.

Update: I feel like the boy who cried wolf but I think this most recent patch should do the trick

[reez]

Last try or else I will recruit Matthew for help. I've just been guessing how to pass the test cause I can't build with xcodebuild on Linux lol.

for OfflineWalletTests its just that its using Wallet.load so its complaining that its seeing network: .testnet, since .load doesn't have that parameter, but I think you actually wanted Wallet.new there anyway (because that's what I'm seeing you're using in the parallel android test) which doesn't have the network parameter. I got that file's test to pass with:

let connection = try Connection.newInMemory()
let wallet = try Wallet(
            descriptor: descriptor,
            changeDescriptor: changeDescriptor,
            network: .testnet,
            connection: connection
)

But when getting those sorts of things cleared up in OffilineWalletTests and OfflinePersistanceTests I'm still seeing failures in 4 of the other Swift tests files.

Happy to be recruited for this if ya need me this week, we can do a screen share together or whatever works best-

[rustaceanrob]

If these don't pass I'm happy to do a screen share or take a commit on top of this branch and merge it

[thunderbiscuit]

I pushed the fix. @rustaceanrob if the tests pass, you squash that last commit into yours, and then rebase on master I think this is ready to go!

Will give the ACK and merge once this is all done. LET'S GO! We can then rampage on other small PRs that have been waiting for this.

Superb work. I know it wasn't the easiest upgrade.

[thunderbiscuit]

Ok I figured out what the other issue was: the persistence was not compatible between versions, so we needed to add a new resource sqlite db to the Swift tests that was built using this version of the lib. Done. This should now pass all tests.

[rustaceanrob]

You legend! I am happy to keep that commit as a moment of history, or can pull down and rebase.

[thunderbiscuit]

Haha 👍👍.

Yeah feel free to squash and rebase. It's a bit big for a commit but whatever. Your other option is to split the source code changes and the tests changes if you prefer; I'm not picky on this.

The only thing I'm picky about is I don't merge merge commits, so I always need a clean rebase.

[rustaceanrob]

Squashed and rebased on master. Pop the champagne

Btw in the commit message I gave attribution to each of the steps to get us here.

[thunderbiscuit]

ACK fe50d23.

@reez told me he'd like to review this thoroughly as well so I'll wait before the merge, but will start rebasing other PRs on this.

[reez]

Big time good job on this Rob!

This is looking really good, the only open questions I have on this are related to testing.

I'm getting failures on the Live tests (I ran all the Live+Offline tests for Swift+JVM locally). Here's an example testSyncedBalance:

JVM

LiveWalletTest > testSyncedBalance() FAILED
    org.bitcoindevkit.EsploraException$Minreq: errorMessage=Cannot allocate memory (os error 12)

Swift

Test Case '-[BitcoinDevKitTests.LiveWalletTests testSyncedBalance]' started.
[BitcoinDevKitTests.LiveWalletTests testSyncedBalance] : failed: caught error: "Minreq(errorMessage: "Cannot allocate memory (os error 12)")"

Both platforms for this specific test get that Minreq error.

Not sure if it's just me? For a sanity check I was looking to use our Action for live-tests on this branch but couldn't run it on this branch from my end.

~

And then just as a smaller side note, for LiveWalletTests + LiveTxBuilderTests we don't seem to be consistent across the platforms:

• Swift+Android use in-memory
• JVM uses persistence

[rustaceanrob]

Normally I would think that is an underlying OS error where the process is over-requesting memory. If your tests run in parallel maybe try a single thread? It looks like the CI was able to run the Swift one. Otherwise that looks like an underlying minreq problem and I'm not sure what we could do about that.

And then just as a smaller side note, for LiveWalletTests + LiveTxBuilderTests we don't seem to be consistent across the platforms:

Agree that we should be consistent. I will honestly make the case that these can all be openInMemory as the underlying SQL connection is the exact same API as the filestore one.

[reez]

Normally I would think that is an underlying OS error where the process is over-requesting memory. If your tests run in parallel maybe try a single thread? It looks like the CI was able to run the Swift one. Otherwise that looks like an underlying minreq problem and I'm not sure what we could do about that.

Good info. And yeah I’m mostly just worried about our live tests that run on a weekly basis, even thought they’ve been fails recently, but (i think) this PR helps fix that with the updated bdk_electrum so we could hopefully get back to them passing on a weekly basis. I’m not too opinionated on the direction we go here, mostly just wanted to get ya’lls feedback/thoughts/opinions.

~

Agree that we should be consistent. I will honestly make the case that these can all be openInMemory as the underlying SQL connection is the exact same API as the filestore one.

I’m cool with that.

[rustaceanrob]

Now that I look at it I think we might want to keep the persistence as it calls the load constructor on the wallet. I think that is a good thing to test. As @thunderbiscuit said the Swift database needs a new mock because the schemas have changed. Would it be alright if we just did a follow up PR to add back persistence to the Android and Swift tests?

[reez]

(I'm open to whatever direction we go with the tests, but I'll ACK right now just so I dont hold up wherever we go)

[thunderbiscuit]

@reez I can't seem to reproduce the test failures locally... but I've seen this error before. Can't quite remember what the context was. I'll try to run the live CI tests now. Ok it turns out this has to be done by the owner of the PR on their fork (or we merge the PR to a branch on this repo and run it for that branch).

In any case if you're ok with it reez I think this is ready to go.

[reez]

@reez I can't seem to reproduce the test failures locally... but I've seen this error before. Can't quite remember what the context was. I'll try to run the live CI tests now. Ok it turns out this has to be done by the owner of the PR on their fork (or we merge the PR to a branch on this repo and run it for that branch).

In any case if you're ok with it reez I think this is ready to go.

Yeah that's cool, we can run that live CI on master when merged. And then the other part about consistency of in-memory/persistance being used across Swift+JVM would be good as a follow up PR.

[reez]

btw ran this PR's bindings on BDK Swift Example Wallet bitcoindevkit/BDKSwiftExampleWallet#184 and its working pretty great 📱

[thunderbiscuit]

Awesome work everyone!

[reez]

Let's goooooo 🚀🚀🚀