Skip to content
This repository has been archived by the owner on Sep 4, 2024. It is now read-only.

[Feature Request] Enable Socks5 proxy support at Transport level #57

Closed
rajarshimaitra opened this issue Jun 7, 2022 · 7 comments
Closed

Comments

@rajarshimaitra
Copy link
Contributor

rajarshimaitra commented Jun 7, 2022

We had an usecase in BDK to have core-rpc client talk through a Tor proxy, like the electrum client, via a proxy configuration.

Potential use case: Mobile devices trying to connect to an Umbrel running at home for personal wallet usage via Tor.

Intention is to get RPC calls through Tor via cookie authentication through a Socks5 transport.

When I tried implementing it I ended up recreating the same SimpleHttpTransport for Socks5 in this PR in BDK
bitcoindevkit/bdk#493

The resting Transport looks like this.
https://github.com/bitcoindevkit/bdk/blob/b99f1cf6309c3daa6b7d432ecd77bbc69862af44/src/blockchain/rpc_proxy.rs

But its essentially duplicating code from low level, for an high level application.

Opening this issue to start discussing on the best approach to integrate it in jsonrpc level and then in bitcoincore-rpc.

My suggestion would be to add another module name simple_socks5 and export that back to bitcoincore-rpc. Use that specific transport, if proxy options are set in client config. (Also maybe via a feature flag?)

I am open to work on the PR for this crate and for bitcoincore-rpc, if we can decide on the approach.

@rajarshimaitra rajarshimaitra changed the title [Feature Request] Enable Socks5 procy support at Transport level [Feature Request] Enable Socks5 proxy support at Transport level Jun 7, 2022
@apoelstra
Copy link
Owner

This seems like a good idea. I notice that your socks dependency looks pretty solid: it has only a couple dependencies, all of which are long-stable and well maintained. One is byteorder which broke our MSRV unexpectedly in the past, but this is not a hill worth dying on.

I think the first step would be to update the MSRV of this library from 1.29 and 1.41, modernize the code a bit.

@tcharding are you willing to take a look at this? I think we're close to being able to pull this project under the rust-bitcoin org, it just needs a little bit of love.

@rajarshimaitra
Copy link
Contributor Author

Thanks @apoelstra for the look.. I believe it can be a really useful feature not just for BDK but for core rpc in general too.. There is really no good way to use home nodes under dynamic IPs for personal usages without Tor.

Wallet devs mostly need to cook up their own solutions today.. But having native support in bitcoincore-rpc will be very helpful for downstream usage..

@tcharding
Copy link
Collaborator

@tcharding are you willing to take a look at this?

Sure thing, no sweat.

@tcharding
Copy link
Collaborator

Done: #58

A lot of patches but should all be pretty easy to review.

@rajarshimaitra
Copy link
Contributor Author

Sorry took me some time to get back to this.. Is it PR-able now?? I can work on getting a proxy inside the Transport.. Open to suggestion on easiest approach..

I think some way of making the SimpleHttpTransport take up a Socks5 socket via a config flag, or feature gate would be least amount of code?

@apoelstra
Copy link
Owner

Yeah, that'd be awesome.

apoelstra added a commit that referenced this issue Sep 10, 2022
ce8f318 Add SOCKS5 Support for SimpleHttpTransport (rajarshimaitra)
dca6844 Ground Work: Refactor URL check from builder api (rajarshimaitra)

Pull request description:

  This is an attempt at #57. Which was a request for adding socks5 Proxy support into the http transport.

  This is useful to connect to RPC via Tor.

  Summary:
   - Adds a new feature flag `proxy` which does some extra steps to connect the `url` address via a `proxy` address.
   - New fields are added inside `SimpleHttpTransport` which are only visible in `proxy` feature.
   - The first PR contains a refactoring which takes the URL sanity checking logic to its own function. So that it can be used for the `proxy-addr` checking also.
   - New constructor:  Client::http_proxy()
   - Test covering basic behavior.

ACKs for top commit:
  apoelstra:
    ACK ce8f318

Tree-SHA512: 1a01cecd3b5bdcd85035a55b02aab5b8b5b02f1664106eadde48151e2be1b6bd28143ad8c33ef87d5ea043c04ee12995986d36f3d0082bb8da3d015f68194a14
@apoelstra
Copy link
Owner

Fixed in #70

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

No branches or pull requests

3 participants