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

Tor #342

Merged
merged 18 commits into from Feb 13, 2017

Conversation

Projects
None yet
8 participants
@cpacia
Member

cpacia commented Jan 25, 2017

  • Make it work over the onion transport
  • Create command line options for --tor and --dualstack
  • Make external API calls (blockstack resolver, exchange rates, message-retriever when fetching https) go out over Tor
  • Make bitcoin wallet implementations use Tor

closes #99

cpacia added some commits Jan 25, 2017

Add Tor command line options
--tor will run exclusively over Tor, configuring the daemon as a hidden service
automatically.

--dualstack will listen on both Tor and the clearnet. This mode is NOT private
but will allow a store to be accessed by clearnet nodes while still being able
to browse Tor-only nodes.

Both modes require Tor to be running.
@JustinDrake

This comment has been minimized.

Show comment
Hide comment
@JustinDrake

JustinDrake Jan 25, 2017

Contributor

I think we discussed this already, but what's the advantage of dual stack over no Tor at all?

Contributor

JustinDrake commented Jan 25, 2017

I think we discussed this already, but what's the advantage of dual stack over no Tor at all?

cpacia added some commits Jan 25, 2017

Make bitcoin exchange rate API calls use Tor
Grab the proxy dialer from the onion transport and use it for the exchange rate
http client. The client can be nil if not using Tor and it will use the default
dialer.
@cpacia

This comment has been minimized.

Show comment
Hide comment
@cpacia

cpacia Jan 25, 2017

Member

@JustinDrake you can't communicate with stores running exclusively on Tor if you are only using the clearnet.

Member

cpacia commented Jan 25, 2017

@JustinDrake you can't communicate with stores running exclusively on Tor if you are only using the clearnet.

@jbenet

This comment has been minimized.

Show comment
Hide comment
@jbenet

jbenet Jan 26, 2017

This is awesome! 👍 solid work

Reminder: let's audit {ipfs, OB, and this transport} before people rely on it to be fully secure.

jbenet commented Jan 26, 2017

This is awesome! 👍 solid work

Reminder: let's audit {ipfs, OB, and this transport} before people rely on it to be fully secure.

cpacia added some commits Jan 26, 2017

@cpacia cpacia added the needs review label Jan 26, 2017

@cpacia cpacia added the ready label Jan 26, 2017

cpacia added some commits Jan 27, 2017

Fix bug connecting spvWallet to multiple peers when using Tor
The spvwallet normally prevents us from connecting to the same ip:post more
than once but this, of course, would prevent us from connecting to multiple
peers if we're using a proxy since they would all go to the same IP.

This commit disables the duplicate address check when using a proxy.
@hoffmabc

in openbazaard.go you have a couple misspellings of the word available and have it as availble

cpacia added some commits Jan 30, 2017

@hoffmabc

This comment has been minimized.

Show comment
Hide comment
@hoffmabc

hoffmabc Feb 1, 2017

Member

Hey @El--Presidente interested in reviewing this with us?

Member

hoffmabc commented Feb 1, 2017

Hey @El--Presidente interested in reviewing this with us?

@coveralls

This comment has been minimized.

Show comment
Hide comment
@coveralls

coveralls Feb 3, 2017

Coverage Status

Coverage decreased (-0.05%) to 27.651% when pulling ebd5ee5 on tor into aeef603 on master.

coveralls commented Feb 3, 2017

Coverage Status

Coverage decreased (-0.05%) to 27.651% when pulling ebd5ee5 on tor into aeef603 on master.

@cpacia cpacia referenced this pull request Feb 8, 2017

Closed

Tor integration #99

@@ -473,7 +487,7 @@ func (w *BitcoindWallet) ReSyncBlockchain(fromHeight int32) {
w.rpcClient.RawRequest("stop", []json.RawMessage{})
w.rpcClient.Shutdown()
time.Sleep(5 * time.Second)
args := []string{"-walletnotify='" + path.Join(w.repoPath, "notify.sh") + " %s'", "-server", "-rescan"}
args := []string{"-walletnotify='" + path.Join(w.repoPath, "notify.sh") + " %s'", "-server", "-rescan", "-torcontrol=127.0.0.1:" + strconv.Itoa(w.controlPort)}

This comment has been minimized.

@gubatron

gubatron Feb 9, 2017

Contributor

repetition?

@gubatron

gubatron Feb 9, 2017

Contributor

repetition?

@@ -89,11 +96,12 @@ func (b *BitcoinPriceFetcher) fetchCurrentRates() error {
}
type BitcoinAverage struct {
cache map[string]float64
cache map[string]float64
client *http.Client

This comment has been minimized.

@gubatron

gubatron Feb 9, 2017

Contributor

by the name of the struct I think this is a wrong shortcut (unless BitcoinAverage is considered as a price fetcher, but it seems it's just a cache of those values.

this might be better as part of BitcoinPriceFetcher, and perhaps passed to fetch(client *http.Client) as a parameter.

@gubatron

gubatron Feb 9, 2017

Contributor

by the name of the struct I think this is a wrong shortcut (unless BitcoinAverage is considered as a price fetcher, but it seems it's just a cache of those values.

this might be better as part of BitcoinPriceFetcher, and perhaps passed to fetch(client *http.Client) as a parameter.

This comment has been minimized.

@tyler-smith

tyler-smith Feb 9, 2017

Member

The cache may be share-able as well, so there's only a BitcoinPriceFetcher type with a method for each data source.

@tyler-smith

tyler-smith Feb 9, 2017

Member

The cache may be share-able as well, so there's only a BitcoinPriceFetcher type with a method for each data source.

This comment has been minimized.

@cpacia

cpacia Feb 10, 2017

Member

I can't look now, but I remember refactoring all this to make it easier to test.

@cpacia

cpacia Feb 10, 2017

Member

I can't look now, but I remember refactoring all this to make it easier to test.

This comment has been minimized.

@cpacia

cpacia Feb 12, 2017

Member

For the prices, I would like to standardize the API response there and have a list of price servers (maybe OB1, DUO) that all return the same thing rather than have to handle responses from multiple sources.

I think that's a much better approach than this.

@cpacia

cpacia Feb 12, 2017

Member

For the prices, I would like to standardize the API response there and have a list of price servers (maybe OB1, DUO) that all return the same thing rather than have to handle responses from multiple sources.

I think that's a much better approach than this.

@gubatron

This comment has been minimized.

Show comment
Hide comment
@gubatron

gubatron Feb 9, 2017

Contributor

by looking at the code patterns, this interface

type ExchangeRateProvider interface {
	fetch() error
}

could use being like this:

type ExchangeRateProvider interface {
        client() *http.Client
        fetchUrl() string
        decode(body io.ReadCloser) error
	fetch() error
}

the code on fetch() is identical, except for its return type, but since all the ExchangeRateProvider implement decode(body), fetch() could reuse a single generic implementation like this:

func (b *ExchangeRateProvider) fetch() (err error) {
	resp, err := b.client().Get(b.fetchUrl())
	if err != nil {
		return err
	}
	return b.decode(resp.Body)
}

since I believe they're initialized as ExchangeRateProvider

func NewBitcoinPriceFetcher(dialer proxy.Dialer) *BitcoinPriceFetcher {
...
b.providers = []ExchangeRateProvider{&BitcoinAverage{b.cache, client}, &BitPay{b.cache, client}, &BlockchainInfo{b.cache, client}, &BitcoinCharts{b.cache, client}}

b.run()
...
}

my guess is by doing that you don't have to implement fetch() so many times and avoid repetition. (warning: I'm not a go programmer, just curious)

Contributor

gubatron commented Feb 9, 2017

by looking at the code patterns, this interface

type ExchangeRateProvider interface {
	fetch() error
}

could use being like this:

type ExchangeRateProvider interface {
        client() *http.Client
        fetchUrl() string
        decode(body io.ReadCloser) error
	fetch() error
}

the code on fetch() is identical, except for its return type, but since all the ExchangeRateProvider implement decode(body), fetch() could reuse a single generic implementation like this:

func (b *ExchangeRateProvider) fetch() (err error) {
	resp, err := b.client().Get(b.fetchUrl())
	if err != nil {
		return err
	}
	return b.decode(resp.Body)
}

since I believe they're initialized as ExchangeRateProvider

func NewBitcoinPriceFetcher(dialer proxy.Dialer) *BitcoinPriceFetcher {
...
b.providers = []ExchangeRateProvider{&BitcoinAverage{b.cache, client}, &BitPay{b.cache, client}, &BlockchainInfo{b.cache, client}, &BitcoinCharts{b.cache, client}}

b.run()
...
}

my guess is by doing that you don't have to implement fetch() so many times and avoid repetition. (warning: I'm not a go programmer, just curious)

@coveralls

This comment has been minimized.

Show comment
Hide comment
@coveralls

coveralls Feb 12, 2017

Coverage Status

Coverage increased (+0.6%) to 28.304% when pulling d24c114 on tor into bc3a525 on master.

coveralls commented Feb 12, 2017

Coverage Status

Coverage increased (+0.6%) to 28.304% when pulling d24c114 on tor into bc3a525 on master.

@cpacia

This comment has been minimized.

Show comment
Hide comment
@cpacia

cpacia Feb 13, 2017

Member

OK merging.

Angel we will take a look at refactoring that price fetcher and possibly create a standard API schema so it's not so cumbersome to handle multiple APIs.

Other than that we plan to pay someone to review this (and the IPFS code) and the broader code base for any privacy and security issues before we take the disclaimer off and let people go wild.

Member

cpacia commented Feb 13, 2017

OK merging.

Angel we will take a look at refactoring that price fetcher and possibly create a standard API schema so it's not so cumbersome to handle multiple APIs.

Other than that we plan to pay someone to review this (and the IPFS code) and the broader code base for any privacy and security issues before we take the disclaimer off and let people go wild.

@cpacia cpacia merged commit 9fe5de0 into master Feb 13, 2017

2 checks passed

continuous-integration/travis-ci/pr The Travis CI build passed
Details
continuous-integration/travis-ci/push The Travis CI build passed
Details
@gubatron

This comment has been minimized.

Show comment
Hide comment
@gubatron

gubatron Feb 14, 2017

Contributor

nuclear_meltdown

(reading over this code got me pumped about go again, if you won't work on that refactoring right away, as I bet you have a bunch more stuff to do, perhaps I'll be able to submit a PR before you get to it)

Contributor

gubatron commented Feb 14, 2017

nuclear_meltdown

(reading over this code got me pumped about go again, if you won't work on that refactoring right away, as I bet you have a bunch more stuff to do, perhaps I'll be able to submit a PR before you get to it)

@cpacia cpacia deleted the tor branch Feb 14, 2017

@placer14 placer14 removed the needs review label Jul 2, 2018

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