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

Stream isolation for Tor #2081

Merged
merged 6 commits into from Dec 7, 2018

Conversation

Projects
None yet
2 participants
@freimair
Copy link
Member

freimair commented Dec 6, 2018

closes #731

Adds a command line parameter --torStreamIsolation. If set, Bisq uses a new stream for every request to the Tor network.

@freimair freimair requested a review from ManfredKarrer as a code owner Dec 6, 2018

Show resolved Hide resolved p2p/src/main/java/bisq/network/p2p/network/TorNetworkNode.java Outdated
@@ -206,7 +206,7 @@ public static boolean isDaoActivated(Environment environment) {
torRcFile, torRcOptions, externalTorControlPort, externalTorPassword, externalTorCookieFile,
socks5ProxyHttpAddress, useAllProvidedNodes, numConnectionForBtc, genesisTxId, genesisBlockHeight, referralId, daoActivated;

protected final boolean externalTorUseSafeCookieAuthentication;
protected final boolean externalTorUseSafeCookieAuthentication, torStreamIsolation;

This comment has been minimized.

@ManfredKarrer

ManfredKarrer Dec 6, 2018

Member

Should torStreamIsolation be by default turned on (true)?

This comment has been minimized.

@freimair

freimair Dec 7, 2018

Member

I have been thinking about that as well.

  • However, in the past, we had some issues with SecureRandom on Linux taking an extraordinary amount of time to produce its first output (> 2 minutes) which could only be sped up by changing something in the OS.
  • Furthermore, creating a random string and a new Socket every time takes time.
  • Last but not least, I am not entirely sure if changing the socket everytime does effect network performance to the worse.

I added an "experimental!" to the option so we can life the --streamIsolation for a little while and see if we encounter any unforeseen issues. If, one day, we are convinced that everything is fine, we can use the feature by default.

Or should we do it the other way around and provide an off-switch to the feature?

This comment has been minimized.

@ManfredKarrer

ManfredKarrer Dec 7, 2018

Member

Yes, good points. Is SecureRandom really needed at all here? I have not followed closely the context and motivation behind it but to keep it turned off by default is definitely better.

@ManfredKarrer
Copy link
Member

ManfredKarrer left a comment

utACK

@ManfredKarrer ManfredKarrer merged commit 66df969 into bisq-network:master Dec 7, 2018

1 check passed

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

@freimair freimair deleted the freimair:stream-isolation branch Dec 7, 2018

@freimair freimair referenced this pull request Dec 28, 2018

Closed

For December 2018 #185

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