-
Notifications
You must be signed in to change notification settings - Fork 45
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
base: master
Are you sure you want to change the base?
Conversation
Just for fun: Binary size when including Kyoto, building the aarch64 binary on macOS using the release-smaller profile (libbdkffi.dylib): 9.7MB Not bad at all for what is likely to be one of the most popular clients one day. |
Awesome thank you for checking that out. Props to |
9433c63
to
d391af5
Compare
d391af5
to
44b0dea
Compare
44b0dea
to
a12ab3e
Compare
f64bfbb
to
48cbd2a
Compare
You can imagine my disappointment with this error message after hacking on this PR again. I'm trying to express the new 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... |
f57905a
to
4609fe1
Compare
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 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 |
To answer your questions @rustaceanrob:
Awesome! I'll try to review this again next week. |
8861ae8
to
31653bd
Compare
There was a problem hiding this 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.
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 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 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 |
The best known peer height is found when connecting to a peer, but our height may not yet match theirs yet. The For reference, I believe you can achieve the same effect in terms of the client height by using |
31653bd
to
3531825
Compare
Oh sweet yes I had not looked at all the methods carefully enough. It was easy to update the app to use that instead. 👍 |
3531825
to
9c9643b
Compare
@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. |
9c9643b
to
c931df2
Compare
There was a problem hiding this 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
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); |
There was a problem hiding this comment.
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:
- Kyoto peeks ahead on a number of addresses (how many?).
- 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.
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
- 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. - The reason
watch_address
has to exist is the user may reveal more than thelookahead
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 callwatch_address
after revealing thelookahead
number of scripts, but there is no cost to calling it for every revealed script so the developer might as well do so.
I've been continually rebasing I think this should be up to date |
a2a992d
to
24bc4ce
Compare
Clean up from the call:
I also got fixated on |
8770a36
to
dd9d11a
Compare
Thanks to @thunderbiscuit for getting the work started initially.
dd9d11a
to
b9895a7
Compare
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
orbdk_kyoto
and passed through the bindings:LightClient
andLightNode
with a namespace level function that takes node configurations.LightNode
in the background. I expose an method on the namespace level function calledrun_node
that moves the process to a different OS thread and returns immediatelyLightClient
simply returns a walletUpdate
when callingupdate
. This can be applied directly to the wallet.LightClient
may broadcast a transactionLightClient
may shutdown the nodeLightClient
may also add more addresses (scripts) to watch withwatch_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, asbdk_kyoto
peeks ahead of the last revealed index.NodeMessageHandler
trait. The idea is the application will define arbitrary behavior, from writing to a log file to updating user interface components.Changelog notice
Checklists
All Submissions:
cargo fmt
andcargo clippy
before committingNew Features:
Bugfixes: