Skip to content
New issue

Have a question about this project? Sign up for a free GitHub account to open an issue and contact its maintainers and the community.

By clicking “Sign up for GitHub”, you agree to our terms of service and privacy statement. We’ll occasionally send you account related emails.

Already on GitHub? Sign in to your account

Add compact block filter client via Kyoto #591

Open
wants to merge 1 commit into
base: master
Choose a base branch
from

Conversation

rustaceanrob
Copy link
Contributor

@rustaceanrob rustaceanrob commented Sep 17, 2024

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

This description has been updated to the latest forced push as of Jan 22

Architectural notes:

The language semantics of Rust make it difficult to access the same type in two different place within a project. Due to this the bdk_kyoto "client" is separated into smaller component types that handle logs, warnings, and wallet updates. Because the target languages for the bindings are garbage collected/reference counted, I think it is an advantage to unite all of these parts into a single Client type. When a user builds a new node, they receive a LightNode and Client.

The LightNode must be run to do anything, and a user would do so with LightNode.run().

The Client is responsible for everything else:

  • Getting log messages with next_log
  • Getting warnings
  • Getting wallet Update
  • Asking for the minimum fee rate to broadcast a transaction
  • Adding new scripts
  • Broadcasting transactions

Taking into account previous conversations, this was conveyed as a more intuitive approach than using callbacks to handle events.

Some additions in particular:

  • The client can add all revealed scripts with add_revealed_scripts that uses a &Wallet. This would be called after a user creates a transaction or reveals a receiving address
  • There is a simple is_running function to check on the node

Changelog notice

  • Add a compact block filter chain source

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
Copy link
Member

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.

@rustaceanrob
Copy link
Contributor Author

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

@rustaceanrob
Copy link
Contributor Author

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 rustaceanrob force-pushed the kyoto-bindings branch 4 times, most recently from f57905a to 4609fe1 Compare November 14, 2024 18:23
@rustaceanrob
Copy link
Contributor Author

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 rustaceanrob marked this pull request as ready for review November 14, 2024 18:59
@thunderbiscuit
Copy link
Member

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.

@rustaceanrob rustaceanrob force-pushed the kyoto-bindings branch 7 times, most recently from 8861ae8 to 31653bd Compare November 16, 2024 20:47
Copy link
Member

@thunderbiscuit thunderbiscuit left a comment

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

Just writing down some stuff as I build and test.

bdk-jvm/lib/build.gradle.kts Outdated Show resolved Hide resolved
bdk-jvm/lib/build.gradle.kts Show resolved Hide resolved
@thunderbiscuit
Copy link
Member

thunderbiscuit commented Dec 3, 2024

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:

new-blocks.mp4

@rustaceanrob
Copy link
Contributor Author

rustaceanrob commented Dec 3, 2024

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

@thunderbiscuit
Copy link
Member

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
Copy link
Member

@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.

Copy link
Member

@thunderbiscuit thunderbiscuit left a comment

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

Looking good! I have a few questions but nothing really blocking.

bdk-ffi/src/bdk.udl Outdated Show resolved Hide resolved
bdk-ffi/src/bdk.udl Outdated Show resolved Hide resolved
void broadcast([ByRef] Transaction transaction);
/// Add another [`Address`] to the [`LightNode`] to watch for inclusion in new blocks.
[Async, Throws=LightClientError]
void watch_address(Address address);
Copy link
Member

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

Let me know if I understand correctly the workflow:

  1. Kyoto peeks ahead on a number of addresses (how many?).
  2. When you give out an address you should notify the light client using watch_address()

If it's required to notify on new addresses, why peek ahead? They sort of meddle with each other in functionality in my mind. For example I don't actually need to use watch_address() on the first few, but at some point I should, but I don't know when. Docs that explain how to deal with that are needed IMO.

Copy link
Contributor Author

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

  1. Yes. Kyoto uses the Wallet lookahead to peek scripts into the future. This is of course set by the user but has a reasonable default of 30 I believe.
  2. The reason watch_address has to exist is the user may reveal more than the lookahead amount of scripts to receive a payment. Maybe not a casual user, but a donation wallet perhaps. I think the appropriate documentation is to say it is necessary to call watch_address after revealing the lookahead number of scripts, but there is no cost to calling it for every revealed script so the developer might as well do so.

bdk-python/scripts/generate-linux.sh Outdated Show resolved Hide resolved
@rustaceanrob
Copy link
Contributor Author

would you mind rebasing this?

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

@rustaceanrob rustaceanrob force-pushed the kyoto-bindings branch 6 times, most recently from a2a992d to 24bc4ce Compare December 10, 2024 01:02
@rustaceanrob
Copy link
Contributor Author

rustaceanrob commented Dec 10, 2024

Clean up from the call:

  • Add the fee filter so users can broadcast low-fee transactions without rejection
  • Add the optional reject reason when tx_failed (needs patch to rust-bitcoin)
  • 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 rustaceanrob force-pushed the kyoto-bindings branch 10 times, most recently from cf0eb71 to 2f1a80d Compare December 13, 2024 19:48
@rustaceanrob rustaceanrob force-pushed the kyoto-bindings branch 2 times, most recently from 2344bdc to b81c548 Compare December 18, 2024 02:29
@rustaceanrob
Copy link
Contributor Author

rustaceanrob commented Dec 18, 2024

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 rustaceanrob force-pushed the kyoto-bindings branch 3 times, most recently from c20e5dc to 0560e2a Compare January 22, 2025 03:01
Thanks to @thunderbiscuit for getting the work started initially.
Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment
Labels
None yet
Projects
None yet
Development

Successfully merging this pull request may close these issues.

2 participants