Skip to content

feat(cbf): add cbf feature using bdk_kyoto #181

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.

Sign up for GitHub

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

Jump to bottom
Merged
notmandatory merged 1 commit into from
May 8, 2025
Merged

feat(cbf): add cbf feature using bdk_kyoto #181

notmandatory merged 1 commit into from
May 8, 2025

Conversation

@tvpeter
Copy link
Collaborator

@tvpeter tvpeter commented May 3, 2025

Description

This PR re-enables the Compact Block Filters (cbf) feature using bdk_kyoto and it is part of updating the library
to use the latest bdk crates.

Notes to the reviewers

This is part of issue #172

Checklists

  • I've signed all my commits
  • I followed the contribution guidelines
  • I ran cargo fmt and cargo clippy before committing

@notmandatory
Copy link
Member

notmandatory commented May 6, 2025

Good job! I did a quick review and looks good to me. I still need to do some actual testing and see how it works from the cli.

@rustaceanrob if you have time please let us know if this is the reasonable approach to adding cbf to the bdk-cli test app.

@notmandatory notmandatory added the enhancement New feature or request label May 6, 2025
rustaceanrob
rustaceanrob reviewed May 6, 2025
View reviewed changes
rustaceanrob
rustaceanrob reviewed May 6, 2025
View reviewed changes
rustaceanrob
rustaceanrob reviewed May 6, 2025
View reviewed changes
rustaceanrob
rustaceanrob reviewed May 6, 2025
View reviewed changes
rustaceanrob
rustaceanrob reviewed May 6, 2025
View reviewed changes
rustaceanrob
rustaceanrob reviewed May 6, 2025
View reviewed changes
@tvpeter
Copy link
Collaborator Author

tvpeter commented May 6, 2025

Thank you for the review @rustaceanrob. I will update and revert.

@rustaceanrob
Copy link
Contributor

rustaceanrob commented May 6, 2025

Gave it one more read. There will be a fair amount of code churn to update to the new bdk_kyoto if this PR is merged. Would you be able to update this PR to bdk_kyoto = 0.9.0? There wouldn't be any follow-up required if we start on the latest version. Feel free to message me on discord if you need help

@tvpeter
Copy link
Collaborator Author

tvpeter commented May 6, 2025

Gave it one more read. There will be a fair amount of code churn to update to the new bdk_kyoto if this PR is merged. Would you be able to update this PR to bdk_kyoto = 0.9.0? There wouldn't be any follow-up required if we start on the latest version. Feel free to message me on discord if you need help

Yes, I've just seen that you've released version 0.9.0. I will update to 0.9.0 to avoid revisiting this issue.I will message you if I need any clarification. Thank you.

@tvpeter tvpeter force-pushed the feat/cbf branch 2 times, most recently from 6755483 to 44e9cd6 Compare May 7, 2025 14:03
rustaceanrob
rustaceanrob reviewed May 7, 2025
View reviewed changes
@rustaceanrob
Copy link
Contributor

rustaceanrob commented May 7, 2025

Nice job. One more review from me but otherwise looks good. I think you can also squash these to a single commit with git rebase -i HEAD~3. They all seem related to each other

@tvpeter
feat(cbf): add cbf feature using bdk_kyoto
6debc68 
- enable full_scan and sync operations

[issue: bitcoindevkit#172]

feat(cbf): update broadcasting tx

- add wait time for node to connect to peers
before broadcasting tx
- add sync chain starting from 10 blocks below
the wallet tip to ensure tx is propagated
- update code_coverage workflow to cover cbf
feature

feat(cbf): update bdk-kyoto to 0.9.0

- refactor syncing into a fn
- made `skip-blocks` optional and removed default
value to use bdk-kyoto Sync scan type

feat(cbf): remove looping for kyoto sync

- remove looping for kyoto client sync operations
- fix compiler warnings
@rustaceanrob
Copy link
Contributor

rustaceanrob commented May 7, 2025

ACK 6debc68

Successfully ran a local sync. Thanks man 😎

@notmandatory
Copy link
Member

notmandatory commented May 8, 2025 •
edited
Loading

What does the "Unexpected invalid proof of work when importing a block header..." message mean here?

Running `target/debug/bdk-cli -n testnet4 wallet -d sqlite -c cbf sync`
2025-05-08T17:48:17.774732Z  INFO bdk_cli::utils: Kyoto node is running
2025-05-08T17:48:17.783905Z  INFO bdk_cli::utils: Starting node
2025-05-08T17:48:17.783917Z  INFO bdk_cli::utils: Configured connection requirement: 4 peers
2025-05-08T17:48:17.783922Z  INFO bdk_cli::utils: Attempting to load headers from the database
2025-05-08T17:48:17.783927Z  INFO bdk_cli::utils: Unexpected invalid proof of work when importing a block header. expected 419766864, got: 486604799
2025-05-08T17:48:17.783963Z  INFO bdk_cli::utils: Received update: applying to wallet
2025-05-08T17:48:17.784046Z  INFO bdk_cli::utils: Chain tip: 81476, Transactions: 0, Balance: 0
2025-05-08T17:48:17.784053Z  INFO bdk_cli::utils: Sync completed: tx_count=0, balance=0
{}

UPDATE:

I removed my light_client_data directory and re-synced and now it's working fine 🎉 :

2025-05-08T17:57:29.941846Z  INFO bdk_cli::utils: Sending block request to random peer
2025-05-08T17:57:29.941851Z  INFO bdk_cli::utils: Downloading blocks with relevant transactions.
2025-05-08T17:57:30.724664Z  INFO bdk_cli::utils: Fully synced to the highest block.
2025-05-08T17:57:30.769446Z  INFO bdk_cli::utils: Received update: applying to wallet
2025-05-08T17:57:30.769702Z  INFO bdk_cli::utils: Chain tip: 81480, Transactions: 1, Balance: 500000
2025-05-08T17:57:30.769773Z  INFO bdk_cli::utils: Sync completed: tx_count=1, balance=500000

@notmandatory
Copy link
Member

notmandatory commented May 8, 2025 •
edited
Loading

One nit, can we move the light_client_data the the same folder where the rest of the wallet data is kept? Can be done in a follow-up PR also.

notmandatory
notmandatory reviewed May 8, 2025
View reviewed changes
Copy link
Member

@notmandatory notmandatory left a comment

Choose a reason for hiding this comment

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

ACK 6debc68

Great work @tvpeter and thanks @rustaceanrob for your detailed review.

@tvpeter
Copy link
Collaborator Author

tvpeter commented May 8, 2025

One nit, can we move the light_client_data the the same folder where the rest of the wallet data is kept? Can be done in a follow-up PR also.

Yes, I will do that.

@notmandatory notmandatory merged commit 6ac12a1 into bitcoindevkit:master May 8, 2025
7 checks passed
@rustaceanrob rustaceanrob mentioned this pull request May 9, 2025
Merged
9 tasks
@tvpeter tvpeter mentioned this pull request May 10, 2025
Merged
6 tasks
notmandatory added a commit that referenced this pull request May 15, 2025
@notmandatory
Merge #183: chore(cbf): set kyoto client data dir
a7e6909 
afc2f9d chore(cbf): mv kyoto data to existing datadir (Vihiga Tyonum)

Pull request description:

  <!-- You can erase any parts of this template not applicable to your Pull Request. -->

  ### Description

  This PR set the data directory for the Kyoto client. See [comment](#181 (comment))

  <!-- Describe the purpose of this PR, what's being adding and/or fixed -->

  ### Notes to the reviewers

  <!-- In this section you can include notes directed to the reviewers, like explaining why some parts
  of the PR were done in a specific way -->

  This PR ensures that the `light_client_data` directory created by the Kyoto client is in the wallet's data directory.

  ## Changelog notice

  <!-- Notice the release manager should include in the release tag message changelog -->
  <!-- See https://keepachangelog.com/en/1.0.0/ for examples -->

  ### Checklists

  #### All Submissions:

  * [x] I've signed all my commits
  * [x] I followed the [contribution guidelines](https://github.com/bitcoindevkit/bdk-cli/blob/master/CONTRIBUTING.md)
  * [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
  * [ ] I've updated `CHANGELOG.md`

ACKs for top commit:
  notmandatory:
    tACK afc2f9d

Tree-SHA512: 899e060313fc3a4263d52a242e785ae30e4e15c6172fcfc1b5242f1038c1670f568f1e09d8080944c9a2912a04ca2971ba1eda21eb0ef53bbff01d037dc02bdf
notmandatory added a commit that referenced this pull request May 16, 2025
@notmandatory
Merge #182: Implement transaction broadcasting for Kyoto
62ff445 
811f614 fix(cbf): typo and cbf dir config when sqlite feature disabled (Steve Myers)
b04fed2 feat(cbf): implement transaction broadcasting (rustaceanrob)

Pull request description:

  The actual implementation comes down to listening for an info message that reports the transaction was sent to a peer. For simplicity I am ignoring any wallet updates, but if the user calls the `Sync` command they can catch them. Follows up #181

  ### Checklists

  #### All Submissions:

  * [x] I've signed all my commits
  * [x] I followed the [contribution guidelines](https://github.com/bitcoindevkit/bdk-cli/blob/master/CONTRIBUTING.md)
  * [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
  * [ ] I've updated `CHANGELOG.md`

  #### 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

ACKs for top commit:
  notmandatory:
    ACK 811f614

Tree-SHA512: 6a1ae4cee58170be5ac444598ea8362e8bd7c77137e2f010e8120869d7491c0ad37798508cea04da6a755d4a1a85b283454daf11624a3bb485e61003ae194007
Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment

Reviewers

@notmandatory notmandatory notmandatory approved these changes

+1 more reviewer

@rustaceanrob rustaceanrob rustaceanrob left review comments

Reviewers whose approvals may not affect merge requirements

Assignees

@tvpeter tvpeter

Labels

enhancement New feature or request

Projects

None yet

Milestone

No milestone

Development

Successfully merging this pull request may close these issues.

3 participants

@tvpeter @notmandatory @rustaceanrob