Compact filter #26

rajarshimaitra · 2021-05-08T12:20:07Z

Description

This PR adds the compact filter configuration to connect bdk-cli with a Bitcoin Core node serving compact filters.

Note to Reviewers

I have updated the Readme doc with new usage instruction. If thats not clear (because github doesn't show .md outputs in PR) below is the instruction:

To sync a wallet to Bitcoin Core node (assuming a regtest node at 127.0.0.1:18444) serving compact filters:

Then run:

cargo run --features compact_filters -- --network regtest wallet --node 127.0.0.1:18444 --descriptor "wpkh(tpubEBr4i6yk5nf5DAaJpsi9N2pPYBeJ7fZ5Z9rmN4977iYLCGco1VyjB9tvvuvYtfZzjD5A8igzgw3HeWeeKFmanHYqksqZXYXGsw5zjnj7KM9/*)" sync

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

src/bdk_cli.rs

CHANGELOG.md

README.md

src/bdk_cli.rs

CHANGELOG.md

README.md

src/lib.rs

rajarshimaitra · 2021-05-16T10:59:13Z

Updated with the requested changes.

This adds a compact filter configuration to connect the wallet with a Bitcoin core node serving compact filters. Esplora and Compact filter configs are first parsed, and fall back to default electrum config if none of them are provided.

Readme updated to include instruction to connect to Bitcoin Core node serving compact filters.

This will append the wallet name provided via configuration flag to bdk-cli into the data directory. So the current structure of datadir is `~/.bdk-bitcoin/compact_filters/<wallet-name>. This will let bdk-cli keep multiple wallet directories simultaneously.

rajarshimaitra · 2021-05-22T16:40:30Z

Updated the database_path to include wallet-name provided with cli args, or use the default name.

The objective is to keep multiple wallet data directories separate.

This PR is ready for a second look.

src/lib.rs

notmandatory · 2021-05-24T05:48:04Z

src/lib.rs

+    pub passwd: Option<String>,
+
+    /// Sets the light client network address
+    #[structopt(name = "ADDRESS:PORT", short = "n", long = "node")]


Similar to ElectrumOpts server, I'd set a default_value for CompactFiltersOpts address ("node") to "127.0.0.1:18444", hopefully in the future we'll be able to make this optional and find our peers via dns seed discovery if none are given. But for now I think it's ok to just default to what most people will probably be testing with. This also give an example to show users the expected format. I'd also add to the description to make it clear this is peer full node address, something like: "Sets the compact block filter enabled full node address"

Yes agreed. Also enabling conn_count to start multiple connections to the same node. This parameter is not used right now.

Also thinking about having --node as a list of ADDRS:PORT strings. Instead of one address, user can give multiple addresses and wallet will make a total of no. of address * conn_count connections.

Does that make sense?

Sounds good but I don't think anyone has tested the CBF blockchain code with multiple nodes, just with multiple connections to the same node. But seems like it should work and maybe now is a good time to test it. Worst case we go back to just connecting to a single node multiple times.

Ya. Updated the code. Planning to run that exact test too. Even if a multiple node connection doesn't work, the user can still give a single node address and it will resume standard behavior.

This PR makes the following changes: - Proxy configuration is moved into a separate `ProxyOpt` structure inside `WalletOpts`. `ProxyOpts` are only enabled for `compact_filters` and `electrum` features. - `--node` config can now take `vec<String>` and make `conn_count` number of connections to each node.

rajarshimaitra · 2021-05-25T09:32:51Z

Updated with requested changes. Code is ready for further testing and review.

rajarshimaitra · 2021-05-26T14:26:36Z

Update for Reviewers

There is a likely error like this while trying to connect to a full node.

$ ./target/debug/bdk-cli --network regtest wallet --node 127.0.0.1:18444 --descriptor $desc sync
[2021-05-26T14:18:18Z ERROR bdk_cli] CompactFilters(InvalidHeaders)

This happens while connecting to fresh instant of regtest-box while having the old ~/.bdk-bitcoin/compact_filters datadir. The previous filters are not valid for the new regtest node.

Solution is to to delete the datadir, then run.

notmandatory · 2021-05-27T04:21:24Z

Looks like the electrum test is failing, I can show you how to spin up docker to run the tests. The steps are also on the bitcoin-regtest-box README under Local bdk testing.

rajarshimaitra · 2021-05-27T13:48:40Z

@notmandatory Yes, the doc tests were failing as I forgot to change them to be compatible with the current WalletOpts structure. The tests should pass now.

Note:

Looks like the electrum test is failing, I can show you how to spin up docker to run the tests.

It doesn't need docker containers to run the bdk-cli tests. That also means we are not covering any functional tests with bdk-cli. I think that's ok because BDK functionalities are already tested upstream.

Doc tests were failing due to incompatibility with recent changes made into the `walletOpts` structure.

notmandatory

ACK f22828d

Will merge tomorrow if no one has any objections.

afilini reviewed May 12, 2021

View reviewed changes

notmandatory requested changes May 13, 2021

View reviewed changes

CHANGELOG.md Outdated Show resolved Hide resolved

README.md Outdated Show resolved Hide resolved

README.md Show resolved Hide resolved

src/lib.rs Outdated Show resolved Hide resolved

rajarshimaitra added 8 commits May 22, 2021 22:07

Adds compact filter configuration

13dc23a

This adds a compact filter configuration to connect the wallet with a Bitcoin core node serving compact filters. Esplora and Compact filter configs are first parsed, and fall back to default electrum config if none of them are provided.

Updated Readme.md

157dcd6

Readme updated to include instruction to connect to Bitcoin Core node serving compact filters.

Update ChangeLog

d92b434

Updated Cargo.toml

100ccf1

Add SOCKS5 config options to CompactFilterOpts

9fb1d62

Apply SOCKS5 config and other minor changes

6023a3f

Update Changelog and Readme file

6f223ce

rajarshimaitra force-pushed the compact-filter branch from c97eb4d to 79e4fe1 Compare May 22, 2021 16:38

notmandatory reviewed May 24, 2021

View reviewed changes

src/lib.rs Outdated Show resolved Hide resolved

notmandatory reviewed May 24, 2021

View reviewed changes

src/lib.rs Outdated Show resolved Hide resolved

notmandatory reviewed May 24, 2021

View reviewed changes

src/lib.rs Outdated Show resolved Hide resolved

notmandatory reviewed May 24, 2021

View reviewed changes

rajarshimaitra force-pushed the compact-filter branch from 08d17a6 to 1dec3c7 Compare May 27, 2021 17:07

rajarshimaitra mentioned this pull request May 28, 2021

Light client tutorial using bdk-cli bitcoindevkit/bitcoindevkit.org#49

Merged

Fix dox tests for updated wallet API

0fb648c

Doc tests were failing due to incompatibility with recent changes made into the `walletOpts` structure.

rajarshimaitra force-pushed the compact-filter branch from 1dec3c7 to 0fb648c Compare May 28, 2021 09:20

notmandatory added 2 commits May 31, 2021 20:16

Add 'compact_filters' feature to CI, fix typo in ProxyOpts

1b9f050

Fix 'conn_count' option long name

f22828d

notmandatory approved these changes Jun 1, 2021

View reviewed changes

notmandatory merged commit f22828d into bitcoindevkit:master Jun 2, 2021

notmandatory added this to the Release 0.3.0 Feature Freeze milestone Oct 5, 2021

Provide feedback

Saved searches

Use saved searches to filter your results more quickly

Compact filter #26

Compact filter #26

rajarshimaitra commented May 8, 2021 •

edited

Loading

rajarshimaitra commented May 16, 2021 •

edited

Loading

rajarshimaitra commented May 22, 2021

notmandatory May 24, 2021

rajarshimaitra May 25, 2021 •

edited

Loading

notmandatory May 25, 2021

rajarshimaitra May 25, 2021

rajarshimaitra commented May 25, 2021

rajarshimaitra commented May 26, 2021

notmandatory commented May 27, 2021

rajarshimaitra commented May 27, 2021 •

edited

Loading

notmandatory left a comment •

edited

Loading

Compact filter #26

Compact filter #26

Conversation

rajarshimaitra commented May 8, 2021 • edited Loading

Description

Note to Reviewers

Checklists

All Submissions:

New Features:

Bugfixes:

rajarshimaitra commented May 16, 2021 • edited Loading

rajarshimaitra commented May 22, 2021

notmandatory May 24, 2021

Choose a reason for hiding this comment

rajarshimaitra May 25, 2021 • edited Loading

Choose a reason for hiding this comment

notmandatory May 25, 2021

Choose a reason for hiding this comment

rajarshimaitra May 25, 2021

Choose a reason for hiding this comment

rajarshimaitra commented May 25, 2021

rajarshimaitra commented May 26, 2021

Update for Reviewers

notmandatory commented May 27, 2021

rajarshimaitra commented May 27, 2021 • edited Loading

notmandatory left a comment • edited Loading

Choose a reason for hiding this comment

rajarshimaitra commented May 8, 2021 •

edited

Loading

rajarshimaitra commented May 16, 2021 •

edited

Loading

rajarshimaitra May 25, 2021 •

edited

Loading

rajarshimaitra commented May 27, 2021 •

edited

Loading

notmandatory left a comment •

edited

Loading