doc: I2P documentation updates

jonatack commented at 8:03 pm on January 6, 2023: contributor

Address the documentation updates requested in issue #26754, clarify/simplify the -i2pacceptincoming help, and a few other fixups.

DrahtBot commented at 8:03 pm on January 6, 2023: contributor

The following sections might be updated with supplementary metadata relevant to reviewers and maintainers.

Reviews

See the guideline for information on the review process.

Type	Reviewers
ACK	1440000bytes, w0xlt, vasild, willcl-ark
Concept ACK	zzzi2p

If your review is incorrectly listed, please react with 👎 to this comment and the bot will ignore it on the next update.

Conflicts

Reviewers, this pull request conflicts with the following ones:

#17783 (util: Fix -norpcwhitelist, -norpcallowip, and similar corner case behavior by ryanofsky)
#17580 (refactor: Add ALLOW_LIST flags and enforce usage in CheckArgFlags by ryanofsky)
#17493 (util: Forbid ambiguous multiple assignments in config file by ryanofsky)

If you consider this pull request important, please also help to review the conflicting pull requests. Ideally, start with the one that should be merged first.

DrahtBot added the label Docs on Jan 6, 2023

in doc/i2p.md:82 in 92f43b479a outdated

87+Whether your node allows I2P inbound connections (see the next section below).
88+
89+## Persistent vs transient I2P addresses
90+
91+The first time Bitcoin Core connects to the I2P router, it automatically
92+generates a persistent I2P address and its corresponding private key by default

jonatack commented at 8:23 pm on January 6, 2023:

Note to self: re-verify if the -i2pacceptincoming help ought to be updated regarding “If this is not set but -i2psam is set then only outgoing connections will be made to the I2P network”. IIRC listening is the default behavior.

unknown commented at 2:52 am on January 7, 2023:

Yes it should be updated as its confusing at this moment. Incoming i2p connections work by default if i2psam is set with i2p working correctly. Maybe we could change this to:

0     Incoming I2P connections are accepted via the SAM proxy
1     by default. If this is set to 0 and -i2psam is set
2     then only outgoing connections will be made to the I2P network.
3     Ignored if -i2psam is not set. Listening for incoming I2P
4     connections is done through the SAM proxy, not by binding to a
5     local address and port (default: 1)
6

vasild commented at 8:32 am on January 9, 2023:

The text is correct, but I can see how it can be confusing. By “to be set” I meant “to be enabled” or “=1” and “not set” => “disabled” or “=0”. Thus the current text:

If set and -i2psam is also set then incoming I2P connections are accepted via the SAM proxy. If this is not set but -i2psam is set then only outgoing connections will be made to the I2P network.

means:

If -i2pacceptincomng=1 and -i2psam=1 then incoming I2P connections are accepted via the SAM proxy. If -i2pacceptincoming=0 but -i2psam=1 then only outgoing connections will be made to the I2P network.

I think the text by @1440000bytes is better than the current text. Maybe it can be improved further - to talk about the “default” only at the end. Like “if 5, then it is blue, if 6 then red and if 7 - green. Default 6.” rather than “by default it is red, if 5 - blue, if 7 - green. Default 6”

jonatack commented at 3:39 pm on January 9, 2023:

Thanks. Proposed a hopefully clearer and simpler version.

0  -i2pacceptincoming
1       Whether to accept inbound I2P connections (default: 1). Ignored if
2       -i2psam is not set. Listening for inbound I2P connections is
3       done through the SAM proxy, not by binding to a local address and
4       port.
5
6  -i2psam=<ip:port>
7       I2P SAM proxy to reach I2P peers and accept I2P connections (default:
8       none)

zzzi2p commented at 2:21 pm on January 7, 2023: none

Thanks for this. You’re on the right track here and we also appreciate the updates on your blog.

Not sure of the audience (users vs. downstreams) for this doc vs. your blog, or whether this doc is more about your current release or your next one. So I won’t offer a detailed review.

Elaborating my request from OP of the ticket, overall, the messaging that would be most helpful is:

Emphasize that I2P is a P2P network (just like Bitcoin!), encourage high share limits (or discourage limit setting at all) to protect anonymity and contribute to the network
Be sure to run latest version of Java I2P (2.1.0 or later) or i2pd (2.45.0 or later)
For now, discourage -i2pacceptincoming=0, especially on low-performance hardware or low-bandwidth, until we work through the issues.
I don’t know if anybody is trying to bootstrap the whole blockchain over i2p, or if that’s even possible, but that’s a really bad idea also

Thanks again.

ghost commented at 3:42 am on January 8, 2023: none

whether this doc is more about your current release or your next one

Its the only i2p doc in this repository that users and developers can refer, update etc. for minimal and important things required to use i2p with bitcoin core. Not release specific. It was added in #22250 (2021) and updated multiple times since then.

There is also a doc for Tor: /doc/tor.md

in doc/i2p.md:97 in 92f43b479a outdated

93+or if `-i2pacceptincoming=1`.  The private key is saved in a file named
94+`i2p_private_key` in the Bitcoin Core data directory.  The persistent I2P
95+address is used for making outgoing connections and accepting incoming
96+connections.
97+
98+In the I2P network, a the receiver of an inbound connection sees the address of

vasild commented at 8:05 am on January 9, 2023:

s/a the receiver/the receiver/

jonatack commented at 3:40 pm on January 9, 2023:

Thanks, done.

vasild approved

vasild commented at 8:33 am on January 9, 2023: contributor

ACK 92f43b479a9673f5475d0de9c731b9f7a37a1c86 modulo the typo mentioned below

jonatack force-pushed on Jan 9, 2023

jonatack commented at 4:08 pm on January 9, 2023: contributor

* Emphasize that I2P is a P2P network (just like Bitcoin!), encourage high share limits (or discourage limit setting at all) to protect anonymity and contribute to the network

Done.

* Be sure to run latest version of Java I2P (2.1.0 or later) or i2pd (2.45.0 or later)

Dropped the versions in the documentation as they go stale. Presumably users will install current versions.

* For now, discourage -i2pacceptincoming=0, especially on low-performance hardware or low-bandwidth, until we work through the issues.

Done.

* I don't know if anybody is trying to bootstrap the whole blockchain over i2p, or if that's even possible, but that's a really bad idea also

Indeed, this was discouraged in the documentation, as it takes a long time.

jonatack marked this as ready for review on Jan 9, 2023

jonatack commented at 4:12 pm on January 9, 2023: contributor

Thanks for the feedback, everyone. Pushed an update and brought this out of draft.

doc: clarify -i2pacceptincoming help documentation

and also hoist the default setting to a constexpr and
remove unused f-string operators in a related functional test.

0ed9cc5892

doc: update bandwidth section of I2P documentation dffa319457

doc: update/clarify/de-emphasize I2P transient address section 295849abb5

doc: remove recommended I2P router versions

as these go stale and users will generally install the current versions available.

3e1d2941e9

jonatack force-pushed on Jan 9, 2023

unknown approved

unknown commented at 5:11 pm on January 9, 2023: none

ACK https://github.com/bitcoin/bitcoin/pull/26838/commits/3e1d2941e921f96c489c5101e2a7a7bf7e7d8b73

w0xlt approved

w0xlt commented at 6:43 pm on January 9, 2023: contributor

ACK https://github.com/bitcoin/bitcoin/pull/26838/commits/3e1d2941e921f96c489c5101e2a7a7bf7e7d8b73

These changes improve the I2P documentation.

w0xlt commented at 7:10 pm on January 9, 2023: contributor

don’t know if anybody is trying to bootstrap the whole blockchain over i2p, or if that’s even possible, but that’s a really bad idea also

Maybe this shouldn’t be allowed by code ?

jonatack commented at 7:24 pm on January 9, 2023: contributor

Maybe this shouldn’t be allowed by code ?

I thought about this but would prefer not to limit it unless it’s been shown that this is really causing a serious issue with the network, both in principle to not make an exception for one network, and also doing it several releases after adding I2P support, as any users that do would see their nodes not working anymore after upgrading to that release of Bitcoin Core.

vasild approved

vasild commented at 10:42 am on January 10, 2023: contributor

ACK 3e1d2941e921f96c489c5101e2a7a7bf7e7d8b73

zzzi2p commented at 11:11 am on January 10, 2023: none

ACK

willcl-ark commented at 11:54 am on January 11, 2023: contributor

ACK 3e1d294

Iin the last 24h of running my node I see the following stats via nethogs, for Bitcoin Core v24.0 with ~11 I2P peers, ~85 HTTP peers and ~13 onion peers:

Selection_048

For contrast Tor has used ~600 MB of data total (figure shared with core-lightning). This is roughly the same usage pattern I saw which led me to try and limit I2P traffic only to bitcoin peers initially – in fact a few months ago when monitoring for longer periods, I found I2P using more than double the bitcoind totals.

I accept (and agree!) that we should want to and encourage contributing back to the wider I2P network for it’s network health and, for me personally it’s no issue, as I’m on an unmetered connection. But I still wonder whether a comment around “expected” I2P bandwidth usage might be warranted in a future PR to give a heads up to entities with limited resources?

maflcko referenced this in commit dbca00ef76 on Jan 11, 2023

maflcko closed this on Jan 11, 2023

sidhujag referenced this in commit 40493d31c6 on Jan 11, 2023

jonatack deleted the branch on Jan 11, 2023

jonatack commented at 9:59 pm on January 11, 2023: contributor

@willcl-ark Interesting feedback (thanks!), worth measuring how much bandwidth is used after #26837 and upgrading to the latest releases, currently Java I2P 2.1.0 and i2pd 2.45.1.

bitcoin locked this on Jan 11, 2024

doc: I2P documentation updates #26838

Reviews

Conflicts