Skip to content
This repository has been archived by the owner on Mar 28, 2023. It is now read-only.

Tor #342

Merged
merged 18 commits into from Feb 13, 2017
Merged

Tor #342

merged 18 commits into from Feb 13, 2017

Conversation

cpacia
Copy link
Member

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

--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
Copy link
Contributor

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

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
Copy link
Member Author

cpacia commented Jan 25, 2017

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

@jbenet
Copy link

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.

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.
Copy link
Member

@hoffmabc hoffmabc left a comment

Choose a reason for hiding this comment

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

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

@hoffmabc
Copy link
Member

hoffmabc commented Feb 1, 2017

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

@coveralls
Copy link

Coverage Status

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

@cpacia cpacia mentioned this pull request Feb 8, 2017
@@ -77,13 +79,15 @@ func NewBitcoindWallet(mnemonic string, params *chaincfg.Params, repoPath string
masterPrivateKey: mPrivKey,
masterPublicKey: mPubKey,
binary: binary,
controlPort: torControlPort,
userTor: useTor,
Copy link
Contributor

Choose a reason for hiding this comment

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

#style
userTor? typo here? (reads weird) if w.userTor.

Copy link
Member Author

Choose a reason for hiding this comment

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

Good catch. That's a typo that it looks like the ide replaced everywhere. I'll fix.

@@ -92,13 +96,22 @@ func (w *BitcoindWallet) Start() {
if w.trustedPeer != "" {
args = append(args, "-connect="+w.trustedPeer)
}
if w.userTor {
var port int
if w.controlPort == 9151 {
Copy link
Contributor

Choose a reason for hiding this comment

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

#styleOptimization

if w.controlPort == 9151 || w.control == 9051 {
    port--
}

Copy link
Contributor

Choose a reason for hiding this comment

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

Q: what is the port number adjustment for?
Q2: is it ok to do this after what happens online 90
args := []string{"-walletnotify='" + path.Join(w.repoPath, "notify.sh") + " %s'", "-server", "-torcontrol=127.0.0.1:" + strconv.Itoa(w.controlPort)}

Copy link
Member Author

Choose a reason for hiding this comment

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

port is the socks port. I can make it more explicit.

@@ -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)}
Copy link
Contributor

Choose a reason for hiding this comment

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

repetition?

port = 9050
}
args = append(args, "-listen", "-proxy:127.0.0.1:"+strconv.Itoa(port), "-onlynet=onion")
}
Copy link
Contributor

Choose a reason for hiding this comment

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

repetition?

Copy link
Contributor

Choose a reason for hiding this comment

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

#DRY

@@ -89,11 +96,12 @@ func (b *BitcoinPriceFetcher) fetchCurrentRates() error {
}

type BitcoinAverage struct {
cache map[string]float64
cache map[string]float64
client *http.Client
Copy link
Contributor

@gubatron gubatron Feb 9, 2017

Choose a reason for hiding this comment

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

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.

Copy link
Member

Choose a reason for hiding this comment

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

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

Copy link
Member Author

Choose a reason for hiding this comment

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

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

Copy link
Member Author

Choose a reason for hiding this comment

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

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

Coverage Status

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

@cpacia
Copy link
Member Author

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
@gubatron
Copy link
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)

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

Successfully merging this pull request may close these issues.

Tor integration
8 participants