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 filters blockchain feature using the Nakamoto client #207

Closed
wants to merge 5 commits into from

Conversation

thunderbiscuit
Copy link
Member

@thunderbiscuit thunderbiscuit commented Oct 13, 2022

Work in Progress.

Fixes #118.

Description

This PR adds the compact block filters blockchain option.

Notes to the reviewers

Not ready for merge yet. Lots to be tested still.

Questions I still want to answer:

  • How much size does it add to the final binary we ship for Android/iOS?
  • Full testing on testnet/mainnet

Changelog notice

## Added Features
    - Compact Block Filters client ([Nakamoto]) [#207]

[Nakamoto](https://github.com/cloudhead/nakamoto)
[#207](https://github.com/bitcoindevkit/bdk-ffi/pull/207)

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

@thunderbiscuit
Copy link
Member Author

thunderbiscuit commented Oct 13, 2022

Making progress!

This is the error I get when attempting to sync a testnet wallet on an Android emulator:

org.bitcoindevkit.BdkException$Cbf: Cbf(Nakamoto(Handle(Disconnected)))
2022-10-13 10:35:11.932 28544-28544 DevkitWallet            com.goldenraven.devkitwallet         I  Wallet is syncing
2022-10-13 10:35:11.965 28544-28544 AndroidRuntime          com.goldenraven.devkitwallet         E  FATAL EXCEPTION: main
                                                                                                    Process: com.goldenraven.devkitwallet, PID: 28544
                                                                                                    org.bitcoindevkit.BdkException$Cbf: Cbf(Nakamoto(Handle(Disconnected)))
                                                                                                    	at org.bitcoindevkit.FfiConverterTypeBdkError.read(bdk.kt:2511)
                                                                                                    	at org.bitcoindevkit.FfiConverterTypeBdkError.read(bdk.kt:2466)
                                                                                                    	at org.bitcoindevkit.FfiConverter$DefaultImpls.liftFromRustBuffer(bdk.kt:153)
                                                                                                    	at org.bitcoindevkit.FfiConverterRustBuffer$DefaultImpls.liftFromRustBuffer(bdk.kt:165)
                                                                                                    	at org.bitcoindevkit.FfiConverterTypeBdkError.liftFromRustBuffer(bdk.kt:2466)
                                                                                                    	at org.bitcoindevkit.FfiConverterTypeBdkError.liftFromRustBuffer(bdk.kt:2466)
                                                                                                    	at org.bitcoindevkit.FfiConverterRustBuffer$DefaultImpls.lift(bdk.kt:166)
                                                                                                    	at org.bitcoindevkit.FfiConverterTypeBdkError.lift(bdk.kt:2466)
                                                                                                    	at org.bitcoindevkit.FfiConverterTypeBdkError.lift(bdk.kt:2466)
                                                                                                    	at org.bitcoindevkit.BdkException$ErrorHandler.lift(bdk.kt:2462)
                                                                                                    	at org.bitcoindevkit.BdkException$ErrorHandler.lift(bdk.kt:2461)
                                                                                                    	at org.bitcoindevkit.Wallet.sync(bdk.kt:3163)
                                                                                                    	at com.goldenraven.devkitwallet.data.Wallet.sync(Wallet.kt:133)
                                                                                                    	at com.goldenraven.devkitwallet.ui.wallet.WalletViewModel.updateBalance(HomeScreen.kt:49)

@thunderbiscuit
Copy link
Member Author

I decided to try it on my desktop using bdk-jvm and a small Kotlin cli wallet I made a while back. It seems to be syncing something so far, so that's good. But as a side note the progress float returned seem to be going up and down. Sometimes in the 60s sometimes in the 40s, back and forth.

float

@thunderbiscuit
Copy link
Member Author

Oh darn it worked! Took about 3-4 minutes and the wallet is displaying the correct balance for the descriptor. 🔥 🔥 🔥

@thunderbiscuit
Copy link
Member Author

thunderbiscuit commented Oct 14, 2022

Ok so so far here is what is happening:

  1. On desktop and using bdk-jvm the new blockchain appears to be working 🚀
  2. On Android not at all, throwing this error: Cbf(Nakamoto(Handle(Disconnected))) (both in the emulator and on a physical device, same error)

The odd thing is that these two libraries are pretty much duplicates of each other. So the issue is definitely coming from something on Android, and most likely not from the FFI layer or uniffi-rs. I could try testing with Python and see what happens, but I suspect it will work just as well as with the jvm library.

@cloudhead @rajarshimaitra any ideas? I tried looking through the Nakamoto codebase but couldn't quite pick out the Handle(Disconnected) error and what could be triggering it.

@rajarshimaitra
Copy link

Thanks @thunderbiscuit .. Thats super progress.. Although I am as clueless about the HandleDisconnect error.. Just making some wild guess.. The sync process starts by spawning a separate thread that runs the nakamoto client process in background.. The HandleDisconnect could mean that for some reason that thread is dropped or terminated and the main bdk thread cannot communicate with it anymore..

Is that something that happens on android? Does it require special consideration for spawning threads?

@cloudhead
Copy link

Thanks @thunderbiscuit .. Thats super progress.. Although I am as clueless about the HandleDisconnect error.. Just making some wild guess.. The sync process starts by spawning a separate thread that runs the nakamoto client process in background.. The HandleDisconnect could mean that for some reason that thread is dropped or terminated and the main bdk thread cannot communicate with it anymore..

Is that something that happens on android? Does it require special consideration for spawning threads?

This is likely what's going on.

@thunderbiscuit
Copy link
Member Author

Ok good stuff guys, thanks for chiming in. I'll need to dig a bit more into this. It means that somehow the connection between the native binary and the glue code is handled differently on Android than it is server-side, and I don't know that we have encountered that before. My next step will be to make sure it works in the unit tests (which run on the dev machine and not in the emulator/device).

Side note, I tested with the Python library and it worked right off the bat.

@thunderbiscuit
Copy link
Member Author

thunderbiscuit commented Oct 17, 2022

Folks it looks like we have liftoff!

Nakamoto

The issue was that I had given a default value of ./nakamoto for the datadir field of the CBFConfig in the ffi layer, and in my tests on desktop/server/mobile I left that field null so it was using the default path. This worked on the server because I called my test wallet from a directory where I had write permission.

On Android however, the process is not started from a place where you have write permission (and I haven't quite figured out where this default path leads to the attempted directory creation but it doesn't really matter for now), and so that default path simply doesn't create the .nakamoto directory (I'd be nice to get a specific error for that maybe). Subsequent sync calls would then fail. The trick was to provide a valid path for the app to write the directory in. On Android this looks like

val path: String = applicationContext.filesDir.toString()
val nakamotoConfig = CbfConfig(
    Network.TESTNET,
    "$path/bdk-nakamoto",
)

I suspect the easiest way to deal with this in the short run is to make the datadir field required on the FFI layer (as per my latest commit), but there might be other ways as well.

A few notes for myself/future reference:

  1. On the server a full sync of testnet was about 300-400 MB for the 3 files (filters.db, headers.db, peers.json)
  2. The process times out on the UI thread (no surprise there), but that means we end up launching it in a coroutine, and that's where we can get into tricky situations (A few concurrency-related issues Block has faced while using bdk-ffi #205)
  3. The size of the native binary file of bdk-android 0.9.0 for the arm64-v8a architecture is 9.8MB. When building Nakamoto in it becomes 21.0MB.

@cloudhead
Copy link

Great work!

In terms of catching the error, is there something nakamoto could have done better? I suspect there would have been an error logged, though I recently reworked all the logging because there were some issues in 0.3.0.

On your notes:

  • Mainnet should take about 100 MB total disk space, since there are about 3x less blocks compared to testnet.
  • Since cloudhead/nakamoto@1c5afc9, the block header/filter loading is interruptible, though I'm still working on the API
  • I'm curious about the binary size, since the nakamoto-node which contains everything that would be imported into bdk is only 5.7MB on my system (release mode).

@andreasgriffin
Copy link
Contributor

andreasgriffin commented Apr 13, 2023

I tried to get CBF working in python, and was successfull on mainnet (and regtest) ( see diff on my branch https://github.com/andreasgriffin/bdk-ffi/commits/compact_filters2 )

image

Even better: It safes the latest tip and does not redownload everything again
image

@andreasgriffin
Copy link
Contributor

I really like CBF because it has good privacy, while also offering great speed.

@thunderbiscuit : Does it makes sense to develop https://github.com/andreasgriffin/bdk-ffi/commits/compact_filters2 into a PR right now, even though bdk has a major restructuring ahead?

@thunderbiscuit
Copy link
Member Author

Hey thanks for taking a look!

Instead of making a new PR on this repo directly, I think it might make sense to keep this PR up to date (it's quite old at this point) so that it's easier to review and keep up to date in the future. Would you mind making your PR on my fork instead? Your contributions will still be shown because your commits will actually show up on this PR once I merge your PR on my branch.

I'll rebase this (it's about 500 commits behind) and everything will break, but would you be interested in opening a PR on my cbf branch with your working implementation?

To be transparent with you I will not actually consider merging this in the FFI layer until the 1.0 release of BDK, which will impact significantly how sync is done (and so you can expect sizeable changes to the compact block filter crate as well). But for anyone who would like to experiment with it and for testing the advances made by the Rust BDK team on CBF it would be great to have this PR fairly up-to-date and ready to play with.

@thunderbiscuit thunderbiscuit force-pushed the cbf branch 2 times, most recently from 5b1f9b6 to 427e12b Compare April 14, 2023 11:04
@thunderbiscuit
Copy link
Member Author

Ok I've rebased the changes. If you PR on my CBF branch I'll take the time to review and merge there, so we can get this PR in working order!

@thunderbiscuit
Copy link
Member Author

I'm closing this in favour or #379.

Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment
Labels
None yet
Projects
Archived in project
Development

Successfully merging this pull request may close these issues.

Add compact block filters blockchain support
4 participants