bdk-ffi icon indicating copy to clipboard operation
bdk-ffi copied to clipboard

Add compact block filter client via Kyoto

Open rustaceanrob opened this issue 1 year ago • 2 comments

Description

Adds a compact block filter backend via Kyoto. Should remain a draft PR until we have an official release for bdk_kyoto

Notes to the reviewers

I will need help writing tests, although I am not sure we need to test the node syncs, only that the node can be built and ran for a couple seconds.

Architectural notes:

Most of these decisions are a product of either kyoto or bdk_kyoto and passed through the bindings:

  • Users may build a LightClient and LightNode with a namespace level function that takes node configurations.
  • To start syncing, the user must run a LightNode in the background. I expose an method on the namespace level function called run_node that moves the process to a different OS thread and returns immediately
  • Syncing is simple, as the LightClient simply returns a wallet Update when calling update. This can be applied directly to the wallet.
  • At any time, a LightClient may broadcast a transaction
  • At any time, a LightClient may shutdown the node
  • A LightClient may also add more addresses (scripts) to watch with watch_address. When a user reveals a new address, it should be added to the node. Most of the time, however, the node will already be aware of the script, as bdk_kyoto peeks ahead of the last revealed index.
  • The node issues a number of events and warnings, which are deferred to the NodeMessageHandler trait. The idea is the application will define arbitrary behavior, from writing to a log file to updating user interface components.

Changelog notice

  • Add a compact block filter chain source

Checklists

All Submissions:

  • [x] I've signed all my commits
  • [x] I followed the contribution guidelines
  • [x] 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 avatar Sep 17 '24 21:09 rustaceanrob

Just for fun:

Binary size when including Kyoto, building the aarch64 binary on macOS using the release-smaller profile (libbdkffi.dylib): 9.7MB Binary size without Kyoto, building the aarch64 binary on macOS using the release-smaller profile (libbdkffi.dylib): 8.6MB

Not bad at all for what is likely to be one of the most popular clients one day.

thunderbiscuit avatar Sep 19 '24 20:09 thunderbiscuit

Awesome thank you for checking that out. Props to rust-bitcoin and tokio for keeping things tidy.

rustaceanrob avatar Sep 19 '24 20:09 rustaceanrob

Objects cannot currently be used in enum variant data

You can imagine my disappointment with this error message after hacking on this PR again. I'm trying to express the new Event<K> from bdk_kyoto but it returns objects like Update (FullScanResult) as a variant. We can return more primitive types as variants, but then we can't express the Update. It doesn't make sense to express an event as anything else than an enum, as only one variant is used at a time to express the event.

It's been so long but maybe that is what I had in mind with the callback interface in the first place lol. Open to new ideas, but until we can return objects from enum variants I think we may have to stick with callbacks...

rustaceanrob avatar Oct 30 '24 01:10 rustaceanrob

I consider this ready for review now. Some updates, I added live tests for android, JVM, swift, and python. Users now have to build a Peer with an IpAddress if they have a company or personal node they want to connect to. NodeMessageHandler is renamed to NodeEventHandler, which reflects a bit more what is happening. Some open questions I have:

Do we really need a separate live test in android when the JVM one exists? It's basically the exact same code and it would likely just increase the maintenance burden for no real benefit IMO.

Should I add docstrings? I think so, but I wanted to confirm

rustaceanrob avatar Nov 14 '24 18:11 rustaceanrob

To answer your questions @rustaceanrob:

  1. No I don't think we need a live test on Android if we have one on JVM (moreso because the Android emulators don't currently work in the CI!)
  2. Yes let's add docstrings.

Awesome! I'll try to review this again next week.

thunderbiscuit avatar Nov 15 '24 19:11 thunderbiscuit

I got it working on the Android example wallet! So far so good. I'll take a closer look at the code before giving the final ACK.

My first little API-related comment is that the NodeEventHandler doesn't have an unique event for a new block being mined but that's a significant event from the point of view of the user, particularly because they loose mempool information when using Kyoto (if blocks pass and the transaction they expect doesn't get mined they need to look into it).

At the moment this is what I do to pull the new blocks out of the events:

override fun dialog(dialog: String) {
    if (dialog.contains("peer height")) {
        val height = dialog.split("peer height: ")[1].split(" ")[0]
        triggerSnackbar("New block mined: $height")
    }
}

Which is basically a custom parsing of the string just to get what I need. I wonder if a newBlock() method on the interface would be cleaner.

Just to showcase my use of it, here is the simple version of the idea, where a snackbar shows when new blocks are mined:

https://github.com/user-attachments/assets/76d2d9d5-b38a-4081-954f-1120e237c972

thunderbiscuit avatar Dec 03 '24 19:12 thunderbiscuit

The best known peer height is found when connecting to a peer, but our height may not yet match theirs yet. The Synced event is emitted every time we sync to their height, which includes when they advertise a new block to us. In effect, I believe the Synced event is more accurate as to what height the client is synced to. However, I did add a recent Progress event that does contain the best known height of remote peers, so the user has an idea of how many filters and filter headers must be downloaded until synced. I will add that in the next kyoto and bdk_kyoto releases and update this PR along with some other improvements.

For reference, I believe you can achieve the same effect in terms of the client height by using NodeEventHandler::synced and updating the toast bar

rustaceanrob avatar Dec 03 '24 20:12 rustaceanrob

Oh sweet yes I had not looked at all the methods carefully enough. It was easy to update the app to use that instead. 👍

thunderbiscuit avatar Dec 04 '24 17:12 thunderbiscuit

@rustaceanrob would you mind rebasing this? I think we're getting close to merging, or at least this PR is the next one in line for me.

thunderbiscuit avatar Dec 05 '24 18:12 thunderbiscuit

would you mind rebasing this?

I've been continually rebasing I think this should be up to date

rustaceanrob avatar Dec 05 '24 18:12 rustaceanrob

Clean up from the call:

  • [x] Add the fee filter so users can broadcast low-fee transactions without rejection
  • [x] Add the optional reject reason when tx_failed (needs patch to rust-bitcoin)
  • [x] Convert build_light_client to builder pattern (avoid breaking changes in the future)

I also got fixated on run_node and how it was seemingly out of place. I got a solution going for LightNode.run() that spawns the node process and immediately returns. Removed run_node in favor of the method.

rustaceanrob avatar Dec 10 '24 19:12 rustaceanrob

Rebased and updated to beta-6 in bdk_kyoto.

Also introduced, bdk_kyoto now allows for a user to pass a &Wallet to add all revealed scripts to the node on behalf of the user. This would simultaneously resolve adding scripts for change and receiving, because the change script is revealed during transaction construction and when calling reveal_next_address. The new methods on the event publisher would be add_revealed_scripts(&wallet) and broadcast_tx(&tx).

I think that is a preferable and unambiguous API, but will still get feedback before I change this PR

rustaceanrob avatar Dec 18 '24 02:12 rustaceanrob

Small note: the Android library is missing a dependency on the kotlinx-coroutine-core library.

implementation("org.jetbrains.kotlinx:kotlinx-coroutines-core:1.8.1")

thunderbiscuit avatar Feb 04 '25 18:02 thunderbiscuit

Added to bdk-android/lib/build-gradle.kts. Not sure if that is correct

rustaceanrob avatar Feb 05 '25 18:02 rustaceanrob

With the cut of the release/1.1 branch, I don't think we'll be merging anything else that'll make it in before the big release. If we end up needing to add anything to the release, we'll merge those commits right on the release branch and cherry-pick on master if needed.

This means master is free to keep going at its own pace and I think this PR is ready to merge (again it will not make it into the 1.1 but rather in 1.2).

No real rush, but also I feel like @rustaceanrob has been really patient and rebasing every few days... I have this working well in my variant/kyoto app (which needs a big rebase on the esplora variant which has added a bunch of cleanups and features, but works well nonetheless!).

@reez what do you say? Have you had a chance to test it out a bit? @rustaceanrob is this a version good to merge? Or are you expecting newer/better features in the next few days/weeks and would rather polish those up first?

thunderbiscuit avatar Feb 28 '25 20:02 thunderbiscuit

I can rebase this soon and I think it's ready to go. I would prefer to do follow-up patches for anything additional

edit: Rebased

rustaceanrob avatar Feb 28 '25 21:02 rustaceanrob

ACK d154117a125eec9fd3bee76db158e8722e5d2178.

reez avatar Mar 06 '25 19:03 reez