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.

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

TOR+Esplora example #816

Closed
wants to merge 15 commits into from
Closed

TOR+Esplora example #816

wants to merge 15 commits into from

Conversation

rorp
Copy link

@rorp rorp commented Dec 8, 2022 •
edited
Loading

Description

A self contained example that demonstrates using a TOR proxy with the Esplora blockchain client.

Blog post PR bitcoindevkit/bitcoindevkit.org#134

Fixes #66

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

@notmandatory
Copy link
Member

Concept looks good, please rebase to fix the CI error.

rajarshimaitra
rajarshimaitra suggested changes Dec 20, 2022
View reviewed changes
Copy link
Contributor

@rajarshimaitra rajarshimaitra left a comment

Choose a reason for hiding this comment

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

Concept ACK, thanks for the example.. I think its more useful to demo how to run tor from the code.. But good to see that its actually working.. I have funded the wallet address with some coins so hopefully the full example will work again..

Below are few general comments..

examples/esplora_backend_with_tor.rs Outdated Show resolved Hide resolved
examples/esplora_backend_with_tor.rs Outdated Show resolved Hide resolved
examples/esplora_backend_with_tor.rs Outdated Show resolved Hide resolved
examples/esplora_backend_with_tor.rs Outdated Show resolved Hide resolved
Cargo.toml Outdated Show resolved Hide resolved
examples/esplora_backend_with_tor.rs Outdated Show resolved Hide resolved
rajarshimaitra
rajarshimaitra suggested changes Dec 27, 2022
View reviewed changes
Copy link
Contributor

@rajarshimaitra rajarshimaitra left a comment

Choose a reason for hiding this comment

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

Needs few more changes.. And can you please squash the commits and rebase on master?

Cargo.toml Outdated Show resolved Hide resolved
examples/esplora_backend_with_tor.rs Outdated Show resolved Hide resolved
examples/esplora_backend_with_tor.rs Outdated Show resolved Hide resolved
@rorp
Copy link
Author

rorp commented Dec 27, 2022

I'm sorry, I'm not a Git power user. I don't feel comfortable with interactive rebase. I always use GitHub's "Squash And Merge". It really does wonders, IMHO.

@rajarshimaitra
Copy link
Contributor

I'm sorry, I'm not a Git power user. I don't feel comfortable with interactive rebase.

Probably a good time google on those and get those basics done as they are rudimentary to contributing to open source projects.. You don't need interactive rebase.. git rebase master from your branch will work.. But you need to sanitize the commit history before this can be merged..

@rorp
Copy link
Author

rorp commented Dec 28, 2022

Probably a good time google on those and get those basics done as they are rudimentary to contributing to open source projects..

Probably. Nobody complained about me not being a git power user for many years of contributing to open source projects, though. I'd rather keep using automation...

@rorp rorp mentioned this pull request Dec 28, 2022
Open
@rajarshimaitra
Copy link
Contributor

@rorp this needs a rebase on master. With esplora_client updated now this configuration #816 (comment) should work.

@notmandatory notmandatory mentioned this pull request Jan 14, 2023
Closed
24 tasks
w0xlt
w0xlt reviewed Jan 24, 2023
View reviewed changes
Copy link
Contributor

@w0xlt w0xlt left a comment

Choose a reason for hiding this comment

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

Concept ACK

@notmandatory
Copy link
Member

@rorp we're getting an error with rustls. Can you check if there's a way to get around this without changing our MSRV to 1.57? Also please rebase to pickup our new stable rust version change from #815. Thanks!

In the meantime I'm going to have to remove this from 0.27 milestone.

error: package `rustls v0.20.8` cannot be built because it requires rustc 1.57 or newer, while the currently active rustc version is 1.56.1

See also: https://github.com/bitcoindevkit/bdk/actions/runs/3895859305/jobs/6739813321

@notmandatory notmandatory removed this from the Release 0.27.0 Feature Freeze milestone Jan 24, 2023
@notmandatory notmandatory mentioned this pull request Jan 30, 2023
Closed
23 tasks
@notmandatory notmandatory mentioned this pull request Feb 14, 2023
Closed
24 tasks
@notmandatory
Copy link
Member

This should be ready to go after @rajarshimaitra's final review. I'll rebase/squash the commits for the original author.

rajarshimaitra
rajarshimaitra approved these changes Feb 21, 2023
View reviewed changes
Copy link
Contributor

@rajarshimaitra rajarshimaitra left a comment

Choose a reason for hiding this comment

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

ReACK d4b3f56

Needs a rebase and its good to go..

@notmandatory
Copy link
Member

Thanks for all your work on this but we've gotten to the point where I'd like to hold off on merging any new features and only merge critical bug fixes to the release/0.27 branch.

Can you rebase this after the new bdk_core_staging stuff in #793 is merged to master branch? then we can merge it in for one of the upcoming 1.0.0-alpha releases.

@notmandatory notmandatory removed this from the Release 0.27.2 Feature Freeze milestone Mar 3, 2023
@danielabrozzoni
Copy link
Member

Hey, can you please rebase this one on master?

@rorp
Copy link
Author

rorp commented Mar 17, 2023

@danielabrozzoni Done!

danielabrozzoni
danielabrozzoni reviewed Mar 17, 2023
View reviewed changes
Cargo.toml
Comment on lines +3 to +18
"crates/bdk",
"crates/chain",
"crates/file_store",
"crates/electrum",
"example-crates/keychain_tracker_electrum",
"example-crates/keychain_tracker_esplora",
"example-crates/keychain_tracker_example_cli",
"example-crates/wallet_electrum",
"example-crates/wallet_esplora",
"example-crates/wallet_esplora_async",
"example-crates/wallet_esplora_tor",
"nursery/tmp_plan",
"nursery/coin_select"
]

default-members = [
Copy link
Member

Choose a reason for hiding this comment

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

Why do we need both members and default-members?

Copy link
Author

Choose a reason for hiding this comment

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

To prevent example-crates/wallet_esplora_tor to be built by default. It depends on libtor, which requires a C compiler, but BDK as a library doesn't and most important it shouldn't.

In this configuration one can build BDK without installing the additional dependencies simply by running

cargo build

And after installing the additional dependencies one can build this example with

cargo build -p bdk-esplora-wallet-tor-example

I agree, this is not the most elegant solution, so I'm open for suggestions how to improve it.

@nondiremanuel nondiremanuel mentioned this pull request Aug 8, 2023
Closed
26 tasks
@nondiremanuel nondiremanuel added this to the 1.0.0-beta.0 milestone Aug 15, 2023
@notmandatory notmandatory mentioned this pull request Aug 29, 2023
Closed
30 tasks
@notmandatory notmandatory added documentation Improvements or additions to documentation module-blockchain labels Mar 18, 2024
@oleonardolima
Copy link
Contributor

I'll open a new PR that covers Tor support relying on arti-client through bitcoindevkit/rust-esplora-client#67.

Is there any active development on this one? cc @rorp @notmandatory

Otherwise, I can reuse the example of libtor here as an alternative there too.

@notmandatory
Copy link
Member

@oleonardolima I'll close this one so you can do a new PR based on the arti-client.

Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment
Reviewers

@w0xlt w0xlt w0xlt left review comments

@danielabrozzoni danielabrozzoni danielabrozzoni left review comments

@rajarshimaitra rajarshimaitra rajarshimaitra approved these changes

Assignees

@rorp rorp

Labels
documentation Improvements or additions to documentation module-blockchain
Projects
BDK
Archived in project
Milestone
1.0.0-beta
Development

Successfully merging this pull request may close these issues.

Integrated TOR proxy
7 participants
@rorp @notmandatory @rajarshimaitra @danielabrozzoni @oleonardolima @w0xlt @nondiremanuel