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

Add onion address for @robkaandorp's btcnode #4100

Closed
wants to merge 4 commits into from
Closed

Add onion address for @robkaandorp's btcnode #4100

wants to merge 4 commits into from

Conversation

robkaandorp
Copy link
Contributor

Network proposal bisq-network/proposals#189

@boring-cyborg
Copy link

boring-cyborg bot commented Mar 30, 2020

Thanks for opening this pull request!

Please check out our contributor checklist and check if Travis or Codacy found any issues with your PR. Also make sure your commits are signed, and that you applied Bisq's code style and formatting.

A maintainer will add an is:priority label to your PR if it is up for compensation. Please see our Bisq Q1 2020 Update post for more details.

wiz
wiz previously approved these changes Mar 30, 2020
Copy link
Contributor

@wiz wiz left a comment

Choose a reason for hiding this comment

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

ACK

% ./bitcoin-service-check.py 2pj2o2mrawj7yotg.onion 8333
OK - 623592 /Satoshi:0.19.1/ 2439.5ms|time=2439.5

@ripcurlx
Copy link
Contributor

@robkaandorp Please sign your commit and I'm happy to merge your PR.

@wiz As we haven't defined it yet: Is a fixed IP and a DNS entry a requirement for BTC Core node operators?

@wiz
Copy link
Contributor

wiz commented Mar 31, 2020

This is totally fine, and currently @Emzy is running a Tor-only Bitcoin node, since he doesn't have a static IP address for that node. My understanding of the Bisq code is that you can optionally omit either the Tor onion or the clearnet IP/hostname and it handles this fine.

As we scale Bisq larger and require more Bitcoin nodes, and as moore's law progresses, I can see a future where all Bisq contributors simply run a Raspberry Pi node at their house with a Tor onion only, so we don't need to pay monthly hosting costs for Bitcoin nodes. We can allocate some of the ops budget to one-time $100 purchases of raspberry pi setups for contributors, instead of spending $50 per month for hosting somewhere.

@ripcurlx
Copy link
Contributor

Thanks @wiz for your comment. So please @robkaandorp just sign your commit as mentioned by the boring-cyborg and I'll merge this PR.

Also make sure your commits are signed, and that you applied Bisq's code style and formatting.

ghubstan and others added 3 commits March 31, 2020 13:21
Replaced the Scanner input read loop with upgraded joptsimple
dependency. Cli now takes a single, non-option program argument, runs
it and exits.  Also removed the "stop client" command because there is
no input loop, but shutdown() is called for orderly channel shudown
before the jvm terminates.  Also changed cmd syntax from camel case
to lowercase, mimicking bitcoin-cli.

Configured logback to supress all debug & info level netty output, and
bypassed logback to print results to System.out.
Change member name OptionParser cmdParser -> parser.
Change server listening port to 9998, client port to 9998.
Change constructor argument from String[] param -> args.
Print the result only, w/out exec time.
Handle help command & print that to stdout;  print help
triggered by user error to stderr.
Use explicit system SUCCESS/FAIL codes in System.exit(0 || 1).
This commit fixes #4103, where it was demonstrated that a
bisq.properties file containing the following entries would cause Bisq
to fail at startup:

    baseCurrencyNetwork=BTC_MAINNET
    bannedSeedNodes=
    bannedBtcNodes=
    bannedPriceRelayNodes=5bmpx76qllutpcyp

The source of the problem was that the jOptSimple argument parsing
library converts the empty value of bannedSeedNodes to a List<String> of
size 1 where the 0th element of the list is an empty string. This empty
string was then attempted to be converted into a new NodeAddress,
causing a validation error. This conversion happened during Guice
wiring, and manifested as a blank white screen appearing as wiring
errors often do in Bisq.

The fix is simple and surgical. We now filter out any empty string
elements before attempting to convert the banned seed node value to a
new node address. I have reviewed the other related options, such as
bannedPriceRelayNodes and bannedBtcNodes, and they do not cause the
problem described above, so no filtering or other changes have been made
to the way they work.
@robkaandorp robkaandorp requested a review from cbeams as a code owner March 31, 2020 11:29
@robkaandorp robkaandorp deleted the add-onion-address-for-btcnode branch March 31, 2020 11:47
Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment
Projects
None yet
Development

Successfully merging this pull request may close these issues.

5 participants