-
Notifications
You must be signed in to change notification settings - Fork 37.7k
doc, test: improve i2p/tor docs and i2p reachable unit tests #22648
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
Conversation
ee86daa
to
8d51dd9
Compare
8d51dd9
to
e3a5814
Compare
Concept ACK on improving the doc. |
Should we add information about ports in this doc? #22634 (comment) |
Your suggestion? Feel free to propose a text. |
Looks like we need all ports open for outbound: https://www.reddit.com/r/i2p/comments/fzwwd0/confused_on_what_ports_to_open/fn76ly9 And for inbound users can follow everything mentioned here: https://geti2p.net/en/faq#ports |
or later recommended) | ||
- [i2p-zero](https://github.com/i2p-zero/i2p-zero) | ||
- [other alternatives](https://en.wikipedia.org/wiki/I2P#Routers) | ||
|
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 official i2prouter implementation in Java, visit their download page and follow the instructions for your Operating System.
Once installed, open a terminal window and type i2prouter start
. Then visit http://127.0.0.1:7657/configclients in your browser to enable SAM. On the left side of the page, there should be a green light next to "Shared Clients".
Edit: Fixed with @RandyMcMillan suggestions
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.
If the user has successfully config'd to this point - a link will take the user to the correct page.
Maybe add a link for convenience?
Then visit 127.0.0.1:7657/configclients
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.
L09
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.
It's not clear to me if a change is being requested here, and what, precisely. Install how-to seems a bit redundant with the existing docs of the various implementations.
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 think adding the above at Line 23 (insert new line first) would be helpful. I agree instructions for every implementation would be overkill, but I was completely new to i2p, and I had to do a bit of digging to figure out how to set it up. I found the wording a bit confusing (I thought I had to install a router and install SAM). The documentation on the website is mostly about setting up a browser. I think adding instructions for users for the reference implementation could be helpful in increasing i2p nodes. The tor docs include a bit of instructions as well
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.
Thanks. I agree it might be helpful, but I don't know if those instructions are platform agnostic. We would also need to maintain it / keep it up to date. Most people I talk to are using i2pd (admittedly perhaps because it is very easy to install), so if we explain how to install i2p-router, we should probably also do it for i2pd. Sounds like a dedicated "How to Install an I2P Router" section.
In any case, feel free to propose this separately. I would rather not make this pull larger.
@prayank23 I don't believe that is correct. I am behind a router/firewall I don't control right now (haven't forwarded any ports) and I am able to successfully create an i2p address ( That being said, I also have no incoming tor connections to my HS (
In my debug file, I see that my node is trying to connect to lots of i2p nodes, but I have tons of connection timeout errors. Edit: Maybe you could try to connect to me? |
Sure. Will experiment tomorrow. Will ping you in IRC. It's 4 AM here right now 😴 |
@Rspigler I manually connected successfully to both of your addresses. Edit: still connected but they both have really high ping times of 11 to 60 seconds versus under 1.5 seconds for all the others. |
I see your inbound for both i2p and tor, so I think we should be good, unless other users start reporting high ping times for these networks with v22.0. Thanks for helping! |
Things that you should try:
|
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.
ACK e3a5814
nit:
-
Would be great if we can add things suggested by @Rspigler in #22648
-
And information about ports or at least this link: https://geti2p.net/en/faq#ports
I have added both things in BlockchainCommons/Learning-Bitcoin-from-the-Command-Line#408
For ports I tried experimenting with allow/deny in ufw
and see if it affects outgoing and incoming connections in Bitcoin Core.
TL;DR:
Ports required by i2p:
1. Outbound (Internet facing): a random port between 9000 and 31000 is selected however its better if all ports are open and it doesn't affect your security. You can check firewall status using `sudo ufw status verbose` which shouldn't deny outgoing by default.
2. Inbound (Internet facing): Optional
Local ports: https://geti2p.net/en/faq#ports
If something looks incorrect feel free to comment :)
I can confirm this |
Thanks @prayank23! And thanks for the suggestions. I'd rather not copy information here that is already in https://geti2p.net/en/faq#ports, as it would add maintainance here and could go out of date. |
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.
ACK e3a5814
ACK e3a5814 |
and simplify similar neighboring assertions.
e3a5814
to
0175977
Compare
Updated |
Re-ACK 0175977 |
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.
ACK 0175977
reACK 0175977 Changes since last review: #22648 (comment) |
Sorry for merging this before #22651, |
Warning: if you use -onlynet with values other than onion, and | ||
the -onion or -proxy option is set, then outgoing onion | ||
connections will still be made; use -noonion or -onion=0 to | ||
disable outbound onion connections in this case. |
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 think I would personally prefer #22651 to fix this behavior instead of having to describe this long special case exception at all. Can't we make -onlynet=ipv4
(or any onlynet not involving -onlynet=onion
) do as one would expect, ignore onions, even if there happens to be a proxy for them? I think this is the most expected behavior, to have proxy setting be orthogonal to what networks to connect out on.
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.
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.
A counter-example: if I specify -onlynet=onion
(with the intent of only connecting to onions), I'd be bummed if it still connected to ipv4 and ipv6 nodes because a proxy is specified for them indirectly through -proxy=127.0.0.1:9050
.
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 think I would personally prefer #22651 to fix this behavior
It can be done in a follow up PR. Will be great if we can do it in the same PR.
Would also need to fix port used for Windows by default: #22651 (comment)
Can't we make -onlynet=ipv4 (or any onlynet not involving -onlynet=onion) do as one would expect, ignore onions, even if there happens to be a proxy for them?
Yes. According to my understanding this is how things would work: Consider Alice wants to use Tor proxy for outbound connections, however connect only with ipv4 nodes. Bob wants to use Tor proxy for outbound connections, he is okay with connecting to any nodes (onion, ipv4 etc.)
So Alice can use onlynet=ipv4
and proxy=127.0.0.1:9050
. Bob will use proxy=127.0.0.1:9050
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.
At least the onlynet docs in src/init.cpp, doc/tor.md and doc/i2p.md are now all aligned.
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.
Can't we make
-onlynet=ipv4
... do as one would expect ...
Done in #22834.
Github-Pull: bitcoin/bitcoin#22648 Rebased-From: b87a9c4d13329a6236124d4b93580c4df8107b32 Tree-SHA512: 3567e6dbfb7c3f410ea84d3f005c86be0283f92e1be1bc2a7ce93240a0b8460b985b3cec5873a19eade52dc1d78ca383c99e023d0bb4247048ae7e43b47522a5
Github-Pull: bitcoin/bitcoin#22648 Rebased-From: b87a9c4 Tree-SHA512: 3567e6dbfb7c3f410ea84d3f005c86be0283f92e1be1bc2a7ce93240a0b8460b985b3cec5873a19eade52dc1d78ca383c99e023d0bb4247048ae7e43b47522a5
Github-Pull: bitcoin/bitcoin#22648 Rebased-From: b87a9c4 Tree-SHA512: 3567e6dbfb7c3f410ea84d3f005c86be0283f92e1be1bc2a7ce93240a0b8460b985b3cec5873a19eade52dc1d78ca383c99e023d0bb4247048ae7e43b47522a5
This pull addresses #22634 (comment) and various user feedback/questions, updates the -onlynet documentation in doc/i2p.md and doc/tor.md per #22651 (src/init.cpp is already fine) and fills in some missing I2P unit test coverage.
Note: this PR depends in part on whether #22651 is merged in order to propose the correct -onlynet documentation (it is currently aligned with the change in #22651), so that PR should be decided or merged first.