Skip to content
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

Make P2P and Tor configurations oneliners #1945

Merged
merged 40 commits into from Jul 22, 2019

Conversation

@nopara73
Copy link
Collaborator

commented Jul 18, 2019

Update

This PR now fixes all the config/settings related issues brought up.

Closes #1934
Fixes #1932
Fixes #1867
Fixes #1738
Fixes #1785
Closes #1956
Fixes #1955
Closes https://github.com/zkSNACKs/WalletWasabi/pull/1852/files
Closes #1928

This PR makes P2P and Tor configuration options to be oneliners. This PR does not attempt to fix the bug described in #1738 nor tries to accepts bitcoin-p2p:// or tcp:// urls, like PR #1928

This PR is a prerequisite for cleanly implementing @NicolasDorier's PR: #1928.

Both in the Backend's and the Gui's configs, I removed TorHost-TorPort, CoreHost-CorePort pairs and rather used TorEndPoint and CoreEndPoint instead (Note, these are pseudovariables.)

To ensure backwards compatibility, instead of obsolating fields, like in PR (#1928) I used the functional pattern we use at other places across the codebase (TryEnsureBackwardsCompatibility().)

Then I cleaned up the settings viewmodel accordingly.

nopara73 and others added 10 commits Jul 18, 2019
Read obsolete config values and set new ones according (#21)
Read obsolete config values and set new ones according
Revert "Read obsolete config values and set new ones according" (#22)
Revert "Read obsolete config values and set new ones according"

@nopara73 nopara73 requested review from lontivero and molnard Jul 18, 2019

WalletWasabi.Gui/Config.cs Outdated Show resolved Hide resolved
nopara73 added 2 commits Jul 18, 2019
@lontivero

This comment has been minimized.

Copy link
Collaborator

commented Jul 18, 2019

I am going to test it now. I would like to suggest add some unit test to EndPointParser class.

@NicolasDorier

This comment has been minimized.

Copy link
Collaborator

commented Jul 19, 2019

@nopara73 I would prefer my version where I repurpose the current Host fields.

For P2P a host without Port is fine, because you can guess the Port from the network. This is not the case for Tor.

Also my PR do not invalidate documentations. What worked before, willl still work now.

@NicolasDorier

This comment has been minimized.

Copy link
Collaborator

commented Jul 19, 2019

I would really appreciate if you merge my PR instead of this one, it would make my stuff a pain in the ass to rebase.

You are also refactoring a piece of code that I am completely removing with #1740 (taking advantage of Newtonsoft features instead of doing dumb mapping code). This PR is making my life worse.

See #1928

@nopara73

This comment has been minimized.

Copy link
Collaborator Author

commented Jul 19, 2019

@NicolasDorier at first I started looking at your PR, but then I realized it needs conceptual improvements before the functionality can be implemented, because it introduces a unique concept to the codebase and doesn't apply it to similar places. Regarding obsolate fields concept, that's a good way to do that, but I decided against it, because we already use a TryEnsureBackwardsCompatibility obsolating concept all across the codebase, so this makes it more consistent.
Also I found repurposing the host to be endpoint and keep calling it host to be dirty.

Regarding your Tor concern, guessing the socks port is just as easy as for Core, although it makes less sense to enable domains and such for Tor, but I believe that's for a later discussion. However I think handling endpoints in a consistent way is important.

Lastly in this PR I also took the opportunity to make the fields EndPoints instead of strings as they should have been in the beginning.

@NicolasDorier

This comment has been minimized.

Copy link
Collaborator

commented Jul 19, 2019

Lastly in this PR I also took the opportunity to make the fields EndPoints instead of string

I thought about this. But endpoint implies port. And the field might not have a port before the user expect it to be guessed.

WalletWasabi/Helpers/EndPointParser.cs Outdated Show resolved Hide resolved
@nopara73

This comment has been minimized.

Copy link
Collaborator Author

commented Jul 21, 2019

@lontivero Done.

nopara73 added 2 commits Jul 21, 2019
@nopara73

This comment has been minimized.

Copy link
Collaborator Author

commented Jul 21, 2019

@lontivero You can test it, but there're 2 issues I want to fix before merge.

  • CodeMaid for some reason started to put spaces in some files. It's really strange.
  • RegTests don't seem to work.
@nopara73

This comment has been minimized.

Copy link
Collaborator Author

commented Jul 21, 2019

@NicolasDorier in PR #1824 you messed around with the test environment which is I think related to why RegTests don't work here, but in non-test enviroment everything works fine.

image

There's that strangeness in the BitcoinRpcConnectionString field, but this makes no sense. For what bitcoin core the backend should connect to? To the correctly specified one or to this RPC string one? What was your thinking there? I cannot follow.

@nopara73

This comment has been minimized.

Copy link
Collaborator Author

commented Jul 21, 2019

I figured it out. You put the RPC there :/

nopara73 and others added 9 commits Jul 21, 2019
Warning message always visible at SettingsTab (#23)
Warning message always visible at SettingsTab
molnard and others added 2 commits Jul 22, 2019
Add wait on update error and let UI run (#24)
Add wait on update error and let UI run

@nopara73 nopara73 merged commit 572f2cd into zkSNACKs:master Jul 22, 2019

2 of 4 checks passed

Wasabi.Osx #20190722.10 failed
Details
Wasabi.Windows in progress
Details
CodeFactor No issues found.
Details
Wasabi.Linux #20190722.10 succeeded
Details

@nopara73 nopara73 deleted the nopara73:config branch Jul 22, 2019

@lontivero

This comment has been minimized.

Copy link
Collaborator

commented Jul 22, 2019

After this PR we've lost the ability to use IPv6 addresses. Those users who have Tor or Bitcoin Node in a ipv6 address will see their config file broken.

@benthecarman

This comment has been minimized.

Copy link
Collaborator

commented Jul 22, 2019

Should probably make an issue for that so it is not lost in this PR

Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment
Projects
None yet
5 participants
You can’t perform that action at this time.