Tor #342
Conversation
--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.
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.
@JustinDrake you can't communicate with stores running exclusively on Tor if you are only using the clearnet. |
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.
There was a problem hiding this 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
Hey @El--Presidente interested in reviewing this with us? |
bitcoin/bitcoind/wallet.go
Outdated
@@ -77,13 +79,15 @@ func NewBitcoindWallet(mnemonic string, params *chaincfg.Params, repoPath string | |||
masterPrivateKey: mPrivKey, | |||
masterPublicKey: mPubKey, | |||
binary: binary, | |||
controlPort: torControlPort, | |||
userTor: useTor, |
There was a problem hiding this comment.
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
.
There was a problem hiding this comment.
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.
bitcoin/bitcoind/wallet.go
Outdated
@@ -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 { |
There was a problem hiding this comment.
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--
}
There was a problem hiding this comment.
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)}
There was a problem hiding this comment.
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)} |
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
repetition?
bitcoin/bitcoind/wallet.go
Outdated
port = 9050 | ||
} | ||
args = append(args, "-listen", "-proxy:127.0.0.1:"+strconv.Itoa(port), "-onlynet=onion") | ||
} |
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
repetition?
There was a problem hiding this comment.
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 |
There was a problem hiding this comment.
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.
There was a problem hiding this comment.
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.
There was a problem hiding this comment.
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.
There was a problem hiding this comment.
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.
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
since I believe they're initialized as 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 |
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. |
closes #99